ASoC/SoundWire: add SDCA helper library#5128
Conversation
|
@bardliao I think the jack is no longer detected even without the last commit, checking with topic/sof-dev. |
plbossart
force-pushed
the
sdw/sdca-library
branch
from
302d7d7 to
86fef73
Compare
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 <pierre-louis.bossart@linux.intel.com>
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 <pierre-louis.bossart@linux.intel.com>
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 <pierre-louis.bossart@linux.intel.com>
Add a generic match function for quirks, chances are we are going to have lots of those... Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We shouldn't do any devm_ based allocation in the io_init(), this need to happen in the probe(). Luckily, we now have an SDCA helper to look in ACPI tables if a SMART_MIC function is exposed. FIXME: the registers are not well handled today, the regmap lists registers which are not really supported in all platforms. The regmap needs to throw an error if those registers are accessed without existing. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
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 <pierre-louis.bossart@linux.intel.com>
In theory the dailinks are created based on the number of endpoints reported in ACPI match tables, so it should harmless to add a new dailink: RT712 VA would not use it since it has only 2 endpoints. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In theory the dailinks are created based on the number of endpoints reported in ACPI match tables, so it should harmless to add a new dailink: RT713 VA would not use it since it has only 2 endpoints. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
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 <pierre-louis.bossart@linux.intel.com>
Use the new machine_check() callback to select an alternate topology for RT712-VB devices. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
force-pushed
the
sdw/sdca-library
branch
from
86fef73 to
1814b82
Compare
|
@bardliao @shumingfan can you try this on RT712-VB? I don't have hardware available so not sure if the detection works ... Thanks! TODO: add the missing registered_source reported early and make sure all the actions are consistent. Need your feedback on the interrupt handling as well. |
|
@plbossart I use the rt712-vb-l0-disco-table-test.asl with this PR to test it. It works to detect RT712VB. |
|
@plbossart I tested rt712_vb on LNL RVP and the jack detection and button detection work. |
|
@bardliao did you get any warning about a non-registered source as discussed earlier? |
Yes, |
|
Humm, this is confusing @bardliao, the 'registered_sources' are not supposed to change. Maybe the log is incorrect? that also means BIT(0), BIT(3) and BIT(6) are reported as interrupts, in the code I registered BIT(0) and BIT(8). No idea what BIT(3) and BIT(6) might be. Odd. |
There are 2 interrupt sources sdca_int_0 and sdca_int_8. So, the 'registered_sources' are not changed, but for different interrupt sources.
It seems to be the codec behavior. The registers will be raised no matter they are registered or not. But the interrupt will only be triggered when the registered bits are raised. Need @shumingfan 's comment.
|
|
There's still no way we can have registered sources 0x101 by using BIT(0) and BIT(8). We must do something wrong. |
plbossart
force-pushed
the
sdw/sdca-library
branch
from
1814b82 to
ce367bf
Compare
Sorry, I didn't get the point. BIT(0) | BIT(8) is 0x101, right? |
plbossart
force-pushed
the
sdw/sdca-library
branch
from
ce367bf to
a7d040f
Compare
|
sorry, I confused binary and hex haha. I added a 0xff mask to only look at the 8 bits per register, that should make things clearer in the log message. Still not sure how BIT(3) and BIT(6) can be shown with an active interrupt? |
|
@plbossart @bardliao |
| * device_register() below. | ||
| */ | ||
| sdca_lookup_interface_revision(slave); | ||
| sdca_lookup_functions(slave); |
There was a problem hiding this comment.
Does this really need to be called from the core, feels like it makes things a lot nicer if it is called from the end driver rather than the soundwire core?
There was a problem hiding this comment.
You missed my note in the commit message
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'.
There was a problem hiding this comment.
Yeah the machine match stuff is kinda interesting I kinda wonder if it could actually be handy for some of the cs42l43 quirks, but I really don't like the way it splits the disco parsing and kinda forces one to rely on a disco constant for the function type. Whilst I imagine most parts will use a disco constant for that I think it would be totally fine for that not to be a constant no?
Just to make sure I understand the situation you have with this part, the part has all the same IDs etc, but the later version of the part adds a smart mic feature? I guess I can't help but wonder if this is actually being driven by a larger issue in the machine driver, in that really with a class device one probably wants to add all the DAI links conditionally on the disco.
I am still pretty deep in a noodling session based on one of your slightly earlier pull requests, still focusing on the register map stuff. Think I have mostly got to the point where I can parse the whole register map from DisCo and handle the disco constants and fixed/default values. I will try to tidy things up a bit and port stuff onto your newer pull here. Hopefully I can then push a series for some discussion, the reason I am looking at this bit is the splitting of the disco parsing is a little tricky for the regmap stuff as it wants to see all the disco and well it feels a bit like the wrong layer to be parsing it at the soundwire layer. Also in keeping with the library concept feel nicer to have the driver call something to parse it. Will do some more thinking and report back.
There was a problem hiding this comment.
the problem statement is that the part has all the same IDs etc, but the later version of the part adds a smart mic feature function. that means we need a topology that adds the mic path.
We could add the dailink for the mic conditionally, but it's not necessary. The rule is that the machine driver needs to expose all the dailinks referenced by the topology, but it's ok to expose additional dailinks not used by the topology. So if the machine driver exposes a mic path with RT713 VA, it'd be just fine. Nothing bad would happen. In other words, exposing all links unconditionally actually makes the machine driver less complex.
There was a problem hiding this comment.
Re: the parsing stuff, I don't disagree that it should be handled in a codec driver for all disco properties,
The main issue is that the hardware discovery MUST work reliably without depending on if/when the codec driver probe happens. if the codec driver is blacklisted and modprobe'd manually 'later', we still want the information on topology to be available on the SOF PCI driver side. That's the reason why I added this short parsing of ACPI stuff before the codec driver probe happens.
With the updated change, |
| }, | ||
| }, | ||
| .dai_num = 1, | ||
| .dai_num = 2, |
There was a problem hiding this comment.
RT713 VA would not use it since it has only 2 endpoints.
Presumably that should read 1 endpoint.
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 <pierre-louis.bossart@linux.intel.com>
Rather than open-code, use the generic SDCA interrupt handler after registering the interrupt sources and callbacks. FIXME: Need to figure out what actions need to be taken for INT0 and INT8. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Rather than open-code, use the generic SDCA interrupt handler after registering the interrupt sources and callbacks. FIXME: Need to figure out what actions need to be taken for INT0 and INT8. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
force-pushed
the
sdw/sdca-library
branch
from
a7d040f to
3ab9b70
Compare
|
Let's continue with #5199 and focus on SDCA functions and version lookup first. |
4 participants
Updated versions of code last touched in 3 months ago.
@ujfalusi can I ask you to test the TGL RT711-sdca part to see if there's any change.
@bardliao if you can try with the MTL RT712 that would appreciated, my test device doesn't seem to detect the jacks - or maybe the code is just wrong haha.
For now this library only deals with detection of Functions and adds a generic SDCA interrupt handler.