Add SDCA DisCo Parsing #5302

charleskeepax · 2025-01-20T15:39:55Z

Having just missed the merge window with this series I will push it here first and then start upstream review once the merge window closes. The series adds support for parsing most of the relevant information from ACPI/DisCo. The next steps I am looking at after this are adding helper functions (part of the library concept) for creating regmaps, lists of controls and DAPM graphs.

lgirdwood

Nice work @charleskeepax ! LGTM
@bardliao @ujfalusi pls review

lgirdwood · 2025-01-20T16:01:17Z

include/sound/sdca_function.h

-	SDCA_FUNCTION_TYPE_SMART_AMP	= 0x01,	/* Amplifier with protection features */
-	SDCA_FUNCTION_TYPE_SIMPLE_AMP	= 0x02,	/* subset of SmartAmp */
-	SDCA_FUNCTION_TYPE_SMART_MIC	= 0x03,	/* Smart microphone with acoustic triggers */
-	SDCA_FUNCTION_TYPE_SIMPLE_MIC	= 0x04,	/* subset of SmartMic */
-	SDCA_FUNCTION_TYPE_SPEAKER_MIC	= 0x05,	/* Combination of SmartMic and SmartAmp */
-	SDCA_FUNCTION_TYPE_UAJ		= 0x06,	/* 3.5mm Universal Audio jack */
-	SDCA_FUNCTION_TYPE_RJ		= 0x07,	/* Retaskable jack */
-	SDCA_FUNCTION_TYPE_SIMPLE_JACK	= 0x08,	/* Subset of UAJ */
-	SDCA_FUNCTION_TYPE_HID		= 0x0A,	/* Human Interface Device, for e.g. buttons */
-	SDCA_FUNCTION_TYPE_IMP_DEF	= 0x1F,	/* Implementation-defined function */
+	SDCA_FUNCTION_TYPE_SMART_AMP			= 0x01,
+	SDCA_FUNCTION_TYPE_SIMPLE_AMP			= 0x02,
+	SDCA_FUNCTION_TYPE_SMART_MIC			= 0x03,
+	SDCA_FUNCTION_TYPE_SIMPLE_MIC			= 0x04,
+	SDCA_FUNCTION_TYPE_SPEAKER_MIC			= 0x05,
+	SDCA_FUNCTION_TYPE_UAJ				= 0x06,
+	SDCA_FUNCTION_TYPE_RJ				= 0x07,
+	SDCA_FUNCTION_TYPE_SIMPLE_JACK			= 0x08,
+	SDCA_FUNCTION_TYPE_HID				= 0x0A,
+	SDCA_FUNCTION_TYPE_IMP_DEF			= 0x1F,


any reason to remove all the comments ?

Moved them into kerneldoc, just above.

bardliao · 2025-01-21T03:24:50Z

sound/soc/sdca/sdca_functions.c

 void sdca_lookup_functions(struct sdw_slave *slave)
 {
 	struct device *dev = &slave->dev;
 	struct acpi_device *adev = to_acpi_device_node(dev->fwnode);

 	if (!adev) {
-		dev_info(dev, "No matching ACPI device found, ignoring peripheral\n");
+		dev_info(dev, "no matching ACPI device found, ignoring peripheral\n");


I don't have objection. But may I know the reason of the change?

Was just consistency all the other error messages in the file didn't capitalize so I made this one match them.

bardliao · 2025-01-21T03:28:12Z

include/sound/sdca_function.h

+	const char *label;
+	int id;
+	enum sdca_entity_type type;
+


nitpick: No new line is required.

bardliao · 2025-01-21T03:28:49Z

include/sound/sdca_function.h

+
+	struct sdca_entity *entities;
+	int num_entities;
+


nitpick: No new line is required.

plbossart

nice update @charleskeepax, see comments below.

plbossart · 2025-01-22T21:32:15Z

sound/soc/sdca/sdca_functions.c

@@ -17,6 +17,14 @@
 #include <sound/sdca.h>
 #include <sound/sdca_function.h>

+#define SDCA_PROPERTY_LENGTH 48


I can't recall where this comes from, a comment wouldn't hurt.

plbossart · 2025-01-22T21:36:51Z

include/sound/sdca_function.h

+	u32 addr;
+	u8 val;
+};
+


This comment is not correct

"
Each SDCA Function may contain a table of register writes that should be
written out when the Function is first probed.
"

The register writes can only happens after the device attaches, which could be much later than the SDCA driver probe based on ACPI information.

I will update the comment to be a little more vague, although it does say when the Function probes, rather than device probes, which I would still intend to only happen well after the device has attached.

plbossart · 2025-01-22T21:38:38Z

sound/soc/sdca/sdca_functions.c

+/*
+ * Sanity check on number of initialization writes, can be expanded if needed.
+ */
+#define SDCA_MAX_INIT_COUNT 64


that is much too low IMHO. This could include large set of parameters.

I would make this 64k...

64k seems bonkers to me, like if you need that many writes that is just circumventing the FDL process. I will update this to like 2k, if devices come along that need more its easy to update the number anyway. It would be good that updating this number would solicit some discussion if someone was making a device that needed so many init writes.

Aside from anything else 64k of init writes would take an ages to download, with a 48k frame rate, that is 1.5 seconds if you can do a write in every frame which current systems won't come close to, so that is probably like 10 seconds of doing init writes.

plbossart · 2025-01-22T21:39:53Z

sound/soc/sdca/sdca_functions.c

+
+	fwnode_property_read_u8_array(function_node,
+				       "mipi-sdca-function-initialization-table",
+				       init_list, num_init_writes);


didn't I write the initial code, would be nice if that was mentioned...

also we need to check for all fwnode_ return values to be consistent.

Apologies I actually did miss that you had a function for this in one of your pull requests and wrote this one from scratch, but I will add a signed off by to the patch.

Sorry also do we need to check the read_u8_array return value, I think any errors would be caught at the count stage?

plbossart · 2025-01-22T21:43:59Z

include/sound/sdca_function.h

+/**
+ * define SDCA_CTL_TYPE - create a unique identifier for an SDCA Control
+ * @ent: Entity Type code.
+ * @sel: Control Selector code.


don't we need a version with the channel selector as well? If we want to e.g. mute the right channel, we need a means to access the volume.1 control, no?

Edit: this was mentioned in the commit message but worth adding as a comment in the code as well for reference.

Perhaps my wording needs more work here, the purpose does not require differentiating channels (Control Numbers), this is used for looking up the many fixed properties of the controls in SDCA. All of the channels, have the same properties (name, size, etc.).

Sorry and to answer your actually question if constructing an actual register address there is the SDW_SDCA_CTL in the sdw_registers.h header. That does include the Control Number.

plbossart · 2025-01-22T21:55:59Z

include/sound/sdca_function.h

+ * struct sdca_channel - a single Channel with a Cluster
+ * @id: Identifier used for addressing.
+ * @purpose: Indicates the purpose of the Channel, usually to give
+ * symantic meaning to the audio, eg. voice, ultrasound.


Hmm... yes my spelling always on point :-)

plbossart · 2025-01-22T22:00:13Z

include/sound/sdca_function.h

@@ -752,6 +752,29 @@ struct sdca_entity_iot {
 	int num_transducer;
 };

+/**


commit message type: add support

plbossart · 2025-01-22T22:00:46Z

include/sound/sdca_function.h

+/**
+ * enum sdca_clock_type - SDCA Clock Types
+ *
+ * Indicate the synchronisity of an Clock Entity, see section 6.4.1.3


synchronicity?

plbossart · 2025-01-22T22:00:57Z

include/sound/sdca_function.h

+
+/**
+ * struct sdca_entity_cs - information specific to Clock Source Entities
+ * @type: Synchronisity of the Clock Source.


plbossart · 2025-01-22T22:03:40Z

include/sound/sdca_function.h

+struct sdca_pde_delay {
+	int from_ps;
+	int to_ps;
+	int us;


it's possible that the delay is > 64k us, I would use u32

It's not clear if we should care about such delays, except maybe to implement a timeout when the transition to the desired power state is stuck?

Yeah I am imagining the delays would likely be used either as delays or as timeouts in DAPM widgets for the PDEs. But either way even if we didn't no harm in them being parsed anyways. It is worth I did only parse the max delay, as I couldn't see any use for the typical delay. Both delays and timeouts would have to run on the max delay.

Fix up some variable/struct member naming, add some missing kerneldoc and fix some minor formatting/whitespace issues. Signed-off-by: Charles Keepax <[email protected]>

Add a helper function to parse all the Function and Entity information from ACPI. In SDCA each device may have several Functions and each corresponds to a specific audio capability such as say amplifier playback or microphone capture. Each Function then contains a number of Entities that represent individual parts of the audio signal chain and are linked together in a graph similar to DAPM. Signed-off-by: Pierre-Louis Bossart <[email protected]> Signed-off-by: Charles Keepax <[email protected]>

Each SDCA Function may contain a table of register writes that should be written out before the Function is used. Add code to parse this table from the DisCo tables in ACPI. Signed-off-by: Pierre-Louis Bossart <[email protected]> Signed-off-by: Charles Keepax <[email protected]>

Within SDCA there is a special Entity called Entity 0 which is used to hold Function level controls. Whilst Entity 0 isn't a full SDCA entity, it is helpful to add an sdca_entity structure for it. This will allow it to be treated identically when the code to add control handling is added. Signed-off-by: Charles Keepax <[email protected]>

Each SDCA Entity will contain a number of Controls, these are basically equivalent to registers. Although a single Control will only ever contain a single field. Some of these would map directly to ALSA controls once more of the SDCA class driver is implemented. These controls are parsed out of the DisCo ACPI tables. One small todo here is that each Control can have multiple sub-entries (Control Numbers), these are typically used to represent channels. Whilst support is present for these, currently the ACPI properties that would allow differing defaults for each channel are not parsed. But there is nothing here that should prevent that being added in the future. Signed-off-by: Charles Keepax <[email protected]>

DisCo/SDCA contains small buffers of data that hold ranges of valid values for the various SDCA Controls. Add support for parsing these from ACPI. Signed-off-by: Charles Keepax <[email protected]>

Within SDCA collections of Channels are referred to as Clusters, each Channel within a Cluster can have various properties attached to it. For example a stereo audio stream, would have a Cluster with 2 Channels one marked as left and the other as right. Various Clusters are specified in DisCo/ACPI and controls then allow the class driver to select between these channel configurations. Add support for parsing these Cluster definitions. Signed-off-by: Charles Keepax <[email protected]>

Add support for parsing the Input/Output Terminal Entity properties from DisCo/ACPI. Signed-off-by: Charles Keepax <[email protected]>

Add support for parsing the Clock Source Entity properties from DisCo/ACPI. Signed-off-by: Charles Keepax <[email protected]>

Add support for parsing the Power Domain Entity properties from DisCo/ACPI. Signed-off-by: Charles Keepax <[email protected]>

charleskeepax requested review from lgirdwood, plbossart, ranj063, kv2019i, dbaluta, bardliao and ujfalusi as code owners January 20, 2025 15:39

lgirdwood reviewed Jan 20, 2025

View reviewed changes

charleskeepax force-pushed the sof/sdca branch from ad1cf56 to 440c58f Compare January 20, 2025 16:28

bardliao reviewed Jan 21, 2025

View reviewed changes

include/sound/sdca_function.h

const char *label;

int id;

enum sdca_entity_type type;

Copy link

Collaborator

bardliao Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: No new line is required.

bardliao reviewed Jan 21, 2025

View reviewed changes

include/sound/sdca_function.h

struct sdca_entity *entities;

int num_entities;

Copy link

Collaborator

bardliao Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: No new line is required.

plbossart reviewed Jan 22, 2025

View reviewed changes

ASoC: SDCA: Minor formatting and naming tweaks

fa3886a

Fix up some variable/struct member naming, add some missing kerneldoc and fix some minor formatting/whitespace issues. Signed-off-by: Charles Keepax <[email protected]>

charleskeepax force-pushed the sof/sdca branch 2 times, most recently from 0d34472 to 874699e Compare January 23, 2025 13:01

charleskeepax force-pushed the sof/sdca branch from 874699e to 4baf5a5 Compare January 23, 2025 13:05

plbossart and others added 8 commits January 23, 2025 14:32

ASoC: SDCA: Add parsing for Control range structures

6c51ce5

DisCo/SDCA contains small buffers of data that hold ranges of valid values for the various SDCA Controls. Add support for parsing these from ACPI. Signed-off-by: Charles Keepax <[email protected]>

ASoC: SDCA: Add support for IT/OT Entity properties

e04f806

Add support for parsing the Input/Output Terminal Entity properties from DisCo/ACPI. Signed-off-by: Charles Keepax <[email protected]>

ASoC: SDCA: Add support for clock Entity properties

b4a8ee3

Add support for parsing the Clock Source Entity properties from DisCo/ACPI. Signed-off-by: Charles Keepax <[email protected]>

ASoC: SDCA: Add support for PDE Entity properties

664f347

Add support for parsing the Power Domain Entity properties from DisCo/ACPI. Signed-off-by: Charles Keepax <[email protected]>

charleskeepax force-pushed the sof/sdca branch from 4baf5a5 to 664f347 Compare January 23, 2025 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SDCA DisCo Parsing #5302

Add SDCA DisCo Parsing #5302

charleskeepax commented Jan 20, 2025

lgirdwood left a comment

lgirdwood Jan 20, 2025

charleskeepax Jan 20, 2025

bardliao Jan 21, 2025

charleskeepax Jan 21, 2025

bardliao Jan 21, 2025

bardliao Jan 21, 2025

plbossart left a comment

plbossart Jan 22, 2025

plbossart Jan 22, 2025

charleskeepax Jan 23, 2025

plbossart Jan 22, 2025

charleskeepax Jan 23, 2025

charleskeepax Jan 23, 2025

plbossart Jan 22, 2025

charleskeepax Jan 23, 2025

charleskeepax Jan 23, 2025

plbossart Jan 22, 2025

charleskeepax Jan 23, 2025

charleskeepax Jan 23, 2025

plbossart Jan 22, 2025

charleskeepax Jan 23, 2025

plbossart Jan 22, 2025

plbossart Jan 22, 2025

plbossart Jan 22, 2025

plbossart Jan 22, 2025

plbossart Jan 22, 2025

charleskeepax Jan 23, 2025

Add SDCA DisCo Parsing #5302

Are you sure you want to change the base?

Add SDCA DisCo Parsing #5302

Conversation

charleskeepax commented Jan 20, 2025

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment