On 20/04/2023 16:18, Pierre-Louis Bossart wrote:
>
>
> On 4/20/23 05:16, Krzysztof Kozlowski wrote:
>> Qualcomm WCD938x Soundwire codecs come as two Soundwire devices - TX
>> and RX - on two Soundwire buses. In DTS they are represented as three
>> device nodes: Soundwire TX, Soundwire RX and the platform codec node
>> (binding to this driver).
>>
>> Probing (and Soundwire enumeration) of all devices can happen in any
>> order, but only the Soundwire TX WCD938x device is used for accessing
>> actual WCD938x registers. It is possible that component bind() in the
>> platform driver will be called too early, before the Soundwire TX device
>> is fully enumerated. This might work or might not, but we cannot handle
>> it correctly from the codec driver. It's job for Soundwire master to
>> bring up devices in correct order.
>
> That last sentence isn't aligned with the way enumeration works in
> general for SoundWire.
I was rather referring to driver point of view. The Qualcomm Soundwire
should work, not expect devices to be powered off during their bind...
>
> The Manager starts the clock, usually after a bus reset, and waits for
> Peripherals to signal their presence with Device0 Attached.
>
> If multiple Peripherals are attached as Device0, the enumeration will
> resolve conflicts at the hardware level, and the Manager *cannot*
> control the order of enumeration; the order is defined by the values in
> the devID registers, whichever Peripheral has the highest value in the
> DevID registers wins the enumeration, and others have to back-off and be
> enumerated later.
>
> Probing and enumeration are also different concepts. The SoundWire
> design allows for drivers to be probed even in the absence of any active
> hardware. This was added on purpose so that the driver could e.g.
> program a GPIO or talk to a power-management chip to allow SoundWire
> devices to start interacting with the bus.
>
> see also suggestion below...
>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>>
>> ---
>>
>> Cc: Patrick Lai <quic_plai@xxxxxxxxxxx>
>> ---
>> sound/soc/codecs/wcd938x.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
>> index 212667a7249c..e8e07e120fa1 100644
>> --- a/sound/soc/codecs/wcd938x.c
>> +++ b/sound/soc/codecs/wcd938x.c
>> @@ -77,6 +77,8 @@
>> #define WCD938X_MBHC_MOISTURE_RREF R_24_KOHM
>> #define WCD_MBHC_HS_V_MAX 1600
>>
>> +#define WCD938X_ENUM_TIMEOUT_MS 500
>> +
>> #define WCD938X_EAR_PA_GAIN_TLV(xname, reg, shift, max, invert, tlv_array) \
>> { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
>> .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
>> @@ -4425,6 +4427,15 @@ static int wcd938x_bind(struct device *dev)
>> wcd938x->sdw_priv[AIF1_PB]->slave_irq = wcd938x->virq;
>> wcd938x->sdw_priv[AIF1_CAP]->slave_irq = wcd938x->virq;
>>
>> + /*
>> + * Before any TX slave regmap usage, be sure the TX slave is actually
>> + * enumerated.
>> + */
>
> ...
>
> the alternative is to move regmap to be cache-only in the probe and
> remove the cache-only property when the device is enumerated.
The driver wants already to use the regmap in RW just few lines below in
wcd938x_set_micbias_data().
I guess I could move this entire piece of code to other place...
>
> That's a trick that's used for all resume cases in codecs in Intel
> platforms, and we need to extend it for the startup cases as well.
Can you point me to some specific piece of driver, so I could see how it
is done? It might help me to prepare a better patch for this.
>
>> + ret = wait_for_completion_timeout(&wcd938x->tx_sdw_dev->enumeration_complete,
>> + msecs_to_jiffies(WCD938X_ENUM_TIMEOUT_MS));
>> + if (!ret)
>> + dev_warn(dev, "Enumeration timeout in bind, possible failures in accessing registers\n");
>> +
>> ret = wcd938x_set_micbias_data(wcd938x);
>> if (ret < 0) {
>> dev_err(dev, "%s: bad micbias pdata\n", __func__);
Best regards,
Krzysztof
