Re: [PATCH 3/6] ASoC: codecs: wcd938x: Check for enumeration before using TX device

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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@linaro.org>
>>
>> ---
>>
>> Cc: Patrick Lai <quic_plai@quicinc.com>
>> ---
>>  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