[DRAFT] Initial SDCA regmap integration #5150

charleskeepax · 2024-08-21T14:30:24Z

Example of where we are getting to integrating the register map stuff and the SDCA stuff. This series pulls in most of @plbossart pull request #5128, some of his older changes that added more DisCo parsing, and my pull request #5087. As well as a brief update to the cs42l43 driver to let it use the new stuff. Its a lot of changes so probably better to look at individual patches rather than trying to make sense of the so useful mega diff Github steers you towards.

The code adds marking which registers are readable/writeable etc. from DisCo, adds the disco constants into the map so they can be read like any other register, and does some initial handling of DisCo defaults.

The two major changes in the SDCA side of things: 1) sdca_function_data is now a child of sdca_function_desc 2) parsing the disco is now handled before the auxiliary devices are probed. These two are basically in service of getting the whole regmap for the device functional before any of the auxiliary devices probe.

Still all a bit of a work in progress but this is a good point to have some discussions around direction and general stuff.

This structure is used to copy information from the 'sdw_slave' structures, it's better to create a flexible array of 'sdw_slave' pointers and directly access the information. This will also help access additional information stored in the 'sdw_slave' structure, such as an SDCA context. This patch does not add new functionality, it only modified how the information is retrieved. Signed-off-by: Pierre-Louis Bossart <[email protected]>

Add new module for SDCA (SoundWire Device Class for Audio) support. For now just add a parser to identify the SDCA revision and the function mask. Note that the SDCA definitions and related MIPI DisCo properties are defined only for ACPI platforms and extracted with _DSD helpers. There is currently no support for Device Tree in the specification, the 'depends on ACPI' reflects this design limitation. This might change in a future revision of the specification but for SDCA 1.0 ACPI is the only supported type of platform firmware. The SDCA library is defined with static inline fallbacks, which will allow for unconditional addition of SDCA support in common parts of the code. The design follows a four-step process: 1) Basic information related to Functions is extracted from MIPI DisCo tables and stored in the 'struct sdw_slave'. Devm_ based memory allocation is not allowed at this point prior to a driver probe, so we only store the function node, address and type. 2) When a codec driver probes, it will register subdevices for each Function identified in phase 1) 3) a driver will probe for each subdevice and addition parsing/memory allocation takes place at this level. devm_ based allocation is highly encouraged to make error handling manageable. 4) Before the peripheral device becomes physically attached, register access is not permitted and the regmaps are cache-only. When peripheral device is enumerated, the bus level uses the 'update_status' notification; after optional device-level initialization, the codec driver will notify each of the subdevices so that they can start interacting with the hardware. Note that the context extracted in 1) should be arguably be handled completely in the codec driver probe. That would however make it difficult to use the ACPI information for machine quirks, and e.g. select different machine driver and topologies as done for the RT712_VB handling later in the series. To make the implementation of quirks simpler, this patchset extracts a minimal amount of context (interface revision and number/type of Functions) before the codec driver probe, and stores this context in the scope of the 'struct sdw_slave'. The SDCA library can also be used in a vendor-specific driver without creating subdevices, e.g. to retrieve the 'initialization-table' values to write platform-specific values as needed. For more technical details, the SDCA specification is available for public downloads at https://www.mipi.org/mipi-sdca-v1-0-download Signed-off-by: Pierre-Louis Bossart <[email protected]>

Use SDCA helpers to get the basic information and store it in the slave context. The information will be optionally be used in codec drivers to register sub-devices for each Function. When platforms are not based on ACPI the helpers do absolutely nothing. Signed-off-by: Pierre-Louis Bossart <[email protected]>

Add a generic match function for quirks, chances are we are going to have lots of those... Signed-off-by: Pierre-Louis Bossart <[email protected]>

The existing machine_quirk() returns a pointer to a soc_acpi_mach structure. For SoundWire/SDCA support, we need a slightly different functionality where a quirk function either validates or NACKs an initial selection, based on additional firmware/DMI information. Signed-off-by: Pierre-Louis Bossart <[email protected]>

Add a filter to skip the RT172 VB configuration if a SmartMic Function is not found in the SDCA descriptors. If the ACPI information is incorrect this can only be quirked further with DMI information. Signed-off-by: Pierre-Louis Bossart <[email protected]>

Use the new machine_check() callback to select an alternate topology for RT712-VB devices. Signed-off-by: Pierre-Louis Bossart <[email protected]>

All existing SDCA codec drivers implement their own custom SDCA interrupt processing. The registers are standard, but actions resulting from an interrupt are specific, which really calls for a change in partitioning with common parts implemented once. In addition, SDCA functions may be supported by separate drivers, but the interrupt processing is handled at the SoundWire peripheral level. This means that SDCA function drivers need a new interface to register an interrupt source with the SDCA device interrupt handler, with the ability to provide a context to be used by a callback invoked in a standard hw-agnostic interrupt handler. Note: these helpers need to be in a dedicated module to avoid circular dependencies. The SoundWire bus code now relies on snd-soc-sdca, so we cannot call SoundWire bus functions from snd-soc-sdca. This is really annoying and maybe we have to split the SoundWire bus code from the slave probe code. Signed-off-by: Pierre-Louis Bossart <[email protected]>

SDCA Function driver shall first parse the information from ACPI, typically during their probe() callback. All the data is devm_ allocated to avoid issues with error flows. Signed-off-by: Pierre-Louis Bossart <[email protected]>

Use the auxiliary bus to register/unregister subdevices for each function. Each function will be handled with a separate driver, matched using a name. If a vendor wants to override a specific function driver, they could use a custom name to match with a custom function driver. Signed-off-by: Pierre-Louis Bossart <[email protected]>

Signed-off-by: Charles Keepax <[email protected]>

Compliment the existing macro to construct an SDCA control address with additional macros to extract the constituent parts of such an address. Also add defines for the FUNCTION_STATUS register. Signed-off-by: Charles Keepax <[email protected]>

SoundWire MBQ register maps typically contain a variety of register sizes, which doesn't map ideally to the regmap abstraction which expects register maps to have a consistent size. Currently the MBQ register map only allows 16-bit registers to be defined, however this leads to complex CODEC driver implementations with an 8-bit register map and a 16-bit MBQ, every control will then have a custom get and put handler that allows them to access different register maps. Further more 32-bit MBQ quantities are not currently supported. Add support for additional MBQ sizes and to avoid the complexity of multiple register maps treat the val_size as a maximum size for the register map. Within the regmap use an ancillary callback to determine how many bytes to actually read/write to the hardware for a specific register. In the case that no callback is defined the behaviour defaults back to the existing behaviour of a fixed size register map. Signed-off-by: Charles Keepax <[email protected]>

The SDCA specification allows for controls to be deferred. In the case of a deferred control the device will return COMMAND_IGNORED to the 8-bit operation that would cause the value to commit. Which is the final 8-bits on a write, or the first 8-bits on a read. In the case of receiving a defer, the regmap will poll the SDCA function busy bit, after which the transaction will be retried, returning an error if the function busy does not clear within a chip specific timeout. Since this is common SDCA functionality which is the 99% use-case for MBQs it makes sense to incorporate this functionality into the register map. If no MBQ configuration is specified, the behaviour will default to the existing behaviour. Signed-off-by: Charles Keepax <[email protected]>

Signed-off-by: Charles Keepax <[email protected]>

Strip the driver back to just the basics to register audio interfaces and boot the part with the firmware running. Change-Id: Ic37fb538e429727d82d84f092da9d8ef74245174 Signed-off-by: Charles Keepax <[email protected]>

Set the PLL and CODEC configs for the firmware to allow it to start up properly functional. Change-Id: I7697acefdef0accb797feab07c9cc1ee7bdf2390 Signed-off-by: Charles Keepax <[email protected]>

Signed-off-by: Charles Keepax <[email protected]>

plbossart

Lots of good idea and useful helpers @charleskeepax

The one big open is the notion that there would be a single SDCA regmap shared across all functions. That seems scary and I don't see any benefit. I must be missing something important that drove you to choose that design?

plbossart · 2024-08-22T06:32:06Z

sound/soc/sdca/sdca_functions.c

+ if (!fwnode_property_read_u32(control_node,
+ "mipi-sdca-control-dc-value",
+ &clist))
+ entity->controls[j].value = clist;


global comment: there should probably need an error thrown if the required properties are not defined, found or there's some problem accessing it.
Fail big and fail early....

Yeah mostly this is still fairly early stage stuff for a lot of things we should return a few more errors etc., although some of it might require some thinking as I have a feeling errors might not be that rare and we generally don't get to fix the ACPI, but at least some bit fat warnings would be nice.

plbossart · 2024-08-22T06:33:13Z

sound/soc/sdca/sdca_functions.c

@@ -190,14 +269,14 @@ static int find_sdca_entities(struct device *dev,
 __func__, "mipi-sdca-entity-id-list",
 num_entities);
 return -EINVAL;
- }
- if (num_entities > SDCA_MAX_ENTITY_COUNT) {
+ } else if (num_entities > SDCA_MAX_ENTITY_COUNT) {


we could fold this change in the initial patch, it's unrelated to the controls.

Yeah certainly makes sense was mostly trying to minimise the number of changes I folded into your patch whilst they are still separate chains. But yeah would make more sense folded into your one.

plbossart · 2024-08-22T06:34:31Z

sound/soc/sdca/sdca_functions.c

@@ -250,16 +329,30 @@ static int find_sdca_entities(struct device *dev,
 }
 }

- fwnode_handle_put(entity_node);
-


nit-pick: we could move this after the dev_info in the initial patch to make the next change more obvious.

plbossart · 2024-08-22T06:36:28Z

sound/soc/sdca/sdca_functions.c

+ if (ret)
+ return ret;
+
+ entities[num_entities].label = "entity0";


nit-pick: one would have thought using the index0 for entity0 would be logical... Maybe we need a comment to say that the entities are not sorted by entity number.

Yeah I basically went this way for two reasons, 1) it makes the code slightly easier to follow, 2) entity0 is a bit of a fake entity doesn't have like connections etc. so seems mild more efficient if they entities you are more likely to want to access are earlier in the array. Regarding the sorting its populated from the ACPI anyway so there is no guarantee it is sorted anyway, no objections to adding a comment to that effect though.

plbossart · 2024-08-22T06:43:52Z

sound/soc/sdca/sdca_function_device.c

+
+ return 0;
+}
+EXPORT_SYMBOL_NS(sdca_dev_parse_functions, SND_SOC_SDCA);


this is a bit controversial. This will result in a layering violation, where the properties that are in scope of a Function device are now parsed at the parent (SoundWire peripheral) device level.

I am not sure why this is needed really. It's perfectly reasonable to have an SDCA regmap for each function device.

I guess if all the functionality is implemented at the SoundWire peripheral device level without creating any SDCA function subdevices then this approach would make sense.

This is a rather important architectural point that needs more pros/cons explanations.

Yeah again commit messages are still very lacking in this chain, apologies for that. I thought this might be the primary point of discussion between us on this stuff.

Firstly to be clear my intention is very much to have subdevices for each function, just to have them all share a common register map.

There are few reasons this makes sense, primarily its a single hardware device and generally one wants a register map to correspond to the hardware device. Only a single thing can talk to the hardware at one time, so it should be locked as such. Especially so in this case where SDCA only mandates a single MBQ buffer for the whole device, rather than one per function, which means you need several actual bus transactions to be atomic and not interrupted by another function. If you were to add a register map for each function you would have to have a lock at the soundwire device level that each subdevice took before it accessed the register map which is rather messy and error prone.

Secondly there are situations where one might want to access registers, especially the function level registers, before registering the subdevices. Currently the way we have this laid out, as per my comment on your patch chain, we force the function-type control to be a disco constant, however it would be totally spec compliant for that to be a device register. Creating the register map at this level would allow us to access these values as registers regardless of if they are a device register or a disco constant. Although I still need to work through your quirks issues and see if we can avoid forcing the constants there.

In short I see a lot of problems with creating a register map for each function and the only really advantage one gets is that it is slightly harder for one function to accidentally access another functions registers, which a function shouldn't be doing anyway. If a function is accessing the wrong registers it is likely the system is already not working properly.

The premise has always been that Functions are independent.

The fact that there are choke points at the device level such as interrupt handling and MBQ buffers should not be a reason to rejoin functions together.

The mutual exclusion for MBQ access is not hard to add, it's one lock at the device level that needs to be taken/released in regmap_sdw_mbq_write/read. It's simpler IMHO than creating a giant regmap that includes all the SDCA controls, even if/when there's no function driver probed.

That said, if for some reason a function driver needs to access a vendor-specific register that's not in the SDCA space, then I have no idea what to do. Even for SDCA memories for e.g. firmware download the regmap directions are not clear to me.

Re: the function-level registers that would need to be accessed, if they are read-only they might as well be DC constant. If they are read-write, then something needs to have write access before the OS boots, and that adds a whole new level of complexity that is probably unmanageable with DisCo. You cannot discover information from ACPI and from the hardware, this doesn't work. Either all the information is read from hardware (USB, HDaudio) or we rely on ACPI. So if we are going to rely on DisCo, I think we can safely assume that all resources needed for driver probe are DC constant and defined by ACPI.

We are not joining functions together just adding a common mechanism for accessing the hardware. Sharing the register map gets you all those things for free. Passing extra locks down into the register map is much worse layering than having a single mechanism to access the hardware. What problems are we actually trying to avoid with the separate register maps? All it does is add complexity and opportunities for disaster.

Re-the function level registers, they can be read only, but not disco constant, you can have variations on the hardware.

well in hindsight that's another layering violation. We should have implemented the multiple MBQ read/writes in the SoundWire core with the proper API and locking. We currently did all this at the regmap layer by piggy-backing on regmap locking, so now regmap is the answer to everything. When all you have is a hammer, etc.

I really don't think it would be a 'disaster' to keep registers in the scope in which they are defined, and to deal with the transport separately.

Another practical aspect is that it'll be rather unlikely that all functions in a multi-function device are used at the same time. So if you only want to use the mic function, it'd be handy to only have to deal with regmap for the mic part only. Do we really want to do a regmap_sync() for everything or just for what we need when resuming?

Hm... will have a think about what MBQing at the SoundWire level would look like, although the MBQ stuff has always felt like a protocol that sits on top of SoundWire to me, which is why I chose the regmap layer to put it in. If anything since we added the function_status stuff into it rather than the retrying I was nervous it was a bit low in the hierarchy, since we are now assuming the function_status control exists, which granted it is mandatory but you know people do things. Directly assuming SDCA controls at the soundwire level feels like we are poking up through the layers quite a lot.

Syncing the registers is a fair point in favour of multiple register maps, it would be nice to sync less, although in most practical situations I am not sure we are talking about that many registers here but it is probably worth investigating.

There's also the case where the Function may set the Function_Needs_Initialization bit. In those cases it'd make sense to first update regmap then do a regmap_sync().

Note that the aux device model is missing something for the initial boot. The regmap creation, be it at the device or function level, takes place during a probe. When the SoundWire device actually shows up on the bus, there's a need to exit the regmap-cached mode. If the regmap handling is done in the function driver, then we're missing a signal sent to all subdevices.

At the risk of starting another argument :-) I would argue it makes sense to only register the subdevices once the SoundWire device has shown up on the bus. This is what the cs42l43 example there does. This has many advantages meaning a lot of the hacks around waiting for the first time the device shows up on the bus (that we have in a lot of the current SoundWire devices) are no longer necessary. It also means you don't get madness like the soundcard being available to user-space but it not actually being able to do anything.

The init writes are also an interesting point, they feel very much like they should be modeled as a register_patch. The primary reason being they are likely to access the registers in a weird direct way (ie. either open coding the MBQ or not using the MBQ at all and directly addressing each byte). So it almost certainly makes sense to bypass the cache with those and just have them go straight to the hardware, as in a register patch.

It does raise an interesting question in my head of: can a function that has init writes, not require initialisation but still have lost the register values? ie. implying you want to sync but not initialise.

There are pros and cons on the subdevice registration in the probe or update_status callback

probe:

pros: all error handling is easier to do, fail big and early(tm).

cons: need to deal with the device being attached on the bus

update_status

pros: no need to deal with additional signaling

cons: error handling is really messy.

The main reason why we used the regmap cache mode is so that we could use devm_ in the peripheral probe, that wouldn't work in the update_status callback.

But if you register a device, then devm_ is allowed in the probe, so it's only the error handling that's messy.

the init writes cannot use the MBQ, the format is a list of pairs of 32-bit address and 8 bit data. I guess you could encode the MBQ sequence but that would have to be split in N transactions. Deferred writes are not allowed either.

In theory according to the spec the Function_Needs_Initialization bit might be set at any time, but it's hard to think of a design where this would be required.

plbossart · 2024-08-22T06:51:14Z

include/sound/sdca_function.h

+enum sdca_fu_controls {
+ SDCA_CONTROL_FU_MUTE = 0x01,
+ SDCA_CONTROL_FU_CHANNEL_VOLUME = 0x02,
+ SDCA_CONTROL_FU_LATENCY = 0x10,


I wouldn't worry too much about latency, it was added in the spec because it was already in the USB spec, but no one seems to know how to account for latencies reported here. Best to ignore and focus on important features :-)

Yeah obviously like a million more controls to add to this in the long term, this was just a little set that handily is enough to get my smart amp to produce sound and gives me a register of each MBQ size.

plbossart · 2024-08-22T06:53:35Z

sound/soc/sdca/sdca_device.c

+
+static bool sdca_valid_address(unsigned int reg)
+{
+ if ((reg & (GENMASK(31, 25) | BIT(18) | BIT(13))) != BIT(30))


It wouldn't hurt to add a comment for humans.

plbossart · 2024-08-22T07:01:18Z

sound/soc/sdca/sdca_function_hid.c

+ ret = sdca_disco_write_defaults(sdev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to write default values\n");
+


Now this is odd. The parsing was done at the SoundWire device level, and the regmap is passed to the aux device during registration, but the management of default values is now handled during the SDCA aux device probe..

Humm, that doesn't seem right.

Its important to separate what the two are doing, the soundwire device level operation creates the register map and parses any constant values adding them to the register map such that devices can read them. That operation doesn't actually cause any interaction with this device at all. This level actually writes out values to the hardware. Whilst I would agree there is some debate about which level to place this, it seemed to me there was no point in writing out configuration to functions that are not getting probed, unless anyone has a good reason we should?

But basically the partition I was looking at is we initialise the register map at the soundwire level, but then accesses to specific functions should all come from that function.

plbossart · 2024-08-22T07:03:14Z

drivers/pinctrl/cirrus/Kconfig

- help
- Select this to support the GPIO/Pinctrl functions of the Cirrus
- Logic CS42L43 PC CODEC.
-


You probably need a different way of handling the changes that roll-back and re-add. This would be problematic for any git bisect with intermediate commits removing functionality.

There is no intention of actually updating cs42l43 to use the SDCA stuff certainly in the short term and even if we did we would likely do so in a keeping both options available way. This is just to provide me a test platform.

Worth saying I included the code in the chain in case you guys wanted to try things out, this should all work with the cs42l43's you have. Although you would need some more changes I haven't pushed yet to get audio out, but this will probe the device and setup the register map.

plbossart · 2024-08-22T07:05:55Z

drivers/mfd/cs42l43.c

+ dev_err(cs42l43->dev, "Failed to add subdevices: %d\n", ret);
+ goto err;
+ }
+


Again I am not sure why a single regmap across all functions would make sense, compared to a regmap allocated during the probe of each SDCA function driver.

I don't see anything done here that couldn't be done at a lower level.

plbossart and others added 21 commits August 21, 2024 14:40

ASoC: SDCA: add quirk function for RT712_VB match

4ad35f8

Add a generic match function for quirks, chances are we are going to have lots of those... Signed-off-by: Pierre-Louis Bossart <[email protected]>

ASoC: SOF: Intel: hda: use machine_check() for SoundWire

64d415e

Use the new machine_check() callback to select an alternate topology for RT712-VB devices. Signed-off-by: Pierre-Louis Bossart <[email protected]>

ASoC: SDCA: add code to parse Function information

75d9b0d

SDCA Function driver shall first parse the information from ACPI, typically during their probe() callback. All the data is devm_ allocated to avoid issues with error flows. Signed-off-by: Pierre-Louis Bossart <[email protected]>

ASoC: SDCA: Add control parsing

e0500c0

Signed-off-by: Charles Keepax <[email protected]>

ASoC: SDCA: Move calls to sdca_parse_function to device level

506f757

Signed-off-by: Charles Keepax <[email protected]>

ASoC: SDCA: Allow passing in a regmap for each auxiliary device

ec260ac

Signed-off-by: Charles Keepax <[email protected]>

ASoC: SDCA: Add generic regmap sdca helpers

0294c5a

Signed-off-by: Charles Keepax <[email protected]>

ASoC: SDCA: Add disco constants/default handling

4001fed

Signed-off-by: Charles Keepax <[email protected]>

mfd: cs42l43: Strip the driver right back

6a64950

Strip the driver back to just the basics to register audio interfaces and boot the part with the firmware running. Change-Id: Ic37fb538e429727d82d84f092da9d8ef74245174 Signed-off-by: Charles Keepax <[email protected]>

mfd: cs42l43: Properly configure the firmware

7229554

Set the PLL and CODEC configs for the firmware to allow it to start up properly functional. Change-Id: I7697acefdef0accb797feab07c9cc1ee7bdf2390 Signed-off-by: Charles Keepax <[email protected]>

mfd: cs42l43: Setup device to use new SDCA class functionality

4f60798

Signed-off-by: Charles Keepax <[email protected]>

charleskeepax requested review from bardliao, lgirdwood, plbossart, ranj063, kv2019i, dbaluta and ujfalusi as code owners August 21, 2024 14:30

plbossart reviewed Aug 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Initial SDCA regmap integration #5150

[DRAFT] Initial SDCA regmap integration #5150

charleskeepax commented Aug 21, 2024

plbossart left a comment

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024

plbossart Aug 22, 2024

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024

plbossart Aug 22, 2024

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024 •

edited

Loading

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024 •

edited

Loading

plbossart Aug 22, 2024

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024

plbossart Aug 22, 2024

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024

plbossart Aug 22, 2024

charleskeepax Aug 22, 2024

charleskeepax Aug 22, 2024

plbossart Aug 22, 2024

[DRAFT] Initial SDCA regmap integration #5150

Are you sure you want to change the base?

[DRAFT] Initial SDCA regmap integration #5150

Conversation

charleskeepax commented Aug 21, 2024

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

charleskeepax Aug 22, 2024 • edited Loading

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

charleskeepax Aug 22, 2024 • edited Loading

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

charleskeepax Aug 22, 2024 •

edited

Loading

charleskeepax Aug 22, 2024 •

edited

Loading