Re: [PATCH] soundwire: qcom: update status from device id 1

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 9/16/22 11:49, Srinivas Kandagatla wrote:
> 
> 
> On 16/09/2022 10:39, Pierre-Louis Bossart wrote:
>>
>>
>> On 9/16/22 11:12, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 15/09/2022 14:10, Pierre-Louis Bossart wrote:
>>>>
>>>>
>>>> On 9/15/22 14:42, Srinivas Kandagatla wrote:
>>>>> By default autoenumeration is enabled on QCom SoundWire controller
>>>>> which means the core should not be dealing with device 0 w.r.t
>>>>> enumeration.
>>>>> Currently device 0 status is also shared with SoundWire core which
>>>>> confuses
>>>>> the core sometimes and we endup adding 0:0:0:0 slave device.
>>>>
>>>> The change looks fine, but the description of the issue is surprising.
>>>
>>> Thanks Pierre,
>>>
>>>>
>>>> Whether autoenumeration is enabled or not is irrelevant, by spec the
>>>> device0 cannot be in ALERT status and throw in-band interrupts to the
>>>> host with this mechanism.
>>>
>>> This issue is more of around enumeration stage in specific during device
>>> status change interrupt from controller. Sharing the device 0 status
>>> with core makes it think that there is a device with 0:0:0:0 address and
>>> it tries to park device to group 13.
>> Still not clear, sorry, see my comment below.
> 
> 
>>
>>>
>>>
>>> --srini
>>>
>>>>
>>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>>> ---
>>>>>    drivers/soundwire/qcom.c | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>>>>> index e21a3306bf01..871e4d8b32c7 100644
>>>>> --- a/drivers/soundwire/qcom.c
>>>>> +++ b/drivers/soundwire/qcom.c
>>>>> @@ -428,7 +428,7 @@ static int
>>>>> qcom_swrm_get_alert_slave_dev_num(struct qcom_swrm_ctrl *ctrl)
>>>>>          ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
>>>>>    -    for (dev_num = 0; dev_num <= SDW_MAX_DEVICES; dev_num++) {
>>>>> +    for (dev_num = 1; dev_num <= SDW_MAX_DEVICES; dev_num++) {
>>>>>            status = (val >> (dev_num * SWRM_MCP_SLV_STATUS_SZ));
>>>>>              if ((status & SWRM_MCP_SLV_STATUS_MASK) ==
>>>>> SDW_SLAVE_ALERT) {
>>
>> can this really happen?
>>
> I have not see this happening, I had to change this line for consistency
> reasons due to other changes in the patch.
> 
> Only case the issue was seen is during enumeration.
> 
>> Device0 cannot be in alert status, can it? The only this it can do is
>> assert PREQ and set the Device0 status to 1 (ATTACHED). I don't get how
>> a device status could be 2.
>>
>> So even if the status is shared somehow,I don't see how this could be
>> related to parking the device as suggested above. If the condition is
>> always false then changing the loop counter from 0 to 1 would not have
>> an effect?
> 
> The reason why core tries to park this device is because it sees
> status[0] as SDW_SLAVE_ATTACHED and start programming the device id,
> however reading DEVID registers return zeros which does not match to any
> of the slaves in the list and the core attempts to park this device to
> Group 13.

ok, that makes sense, thanks for the explanations.

I would recommend splitting this patch in two then:

1) the change for the handling of the alert status, which is unrelated
to the auto-enumeration. That removes a test for an always-false condition

2) the change for the device status, that indeed is related to enumeration.


> 
> 
> 
> 
> --srini
> 
>>
>>
>>>>> @@ -448,7 +448,7 @@ static void qcom_swrm_get_device_status(struct
>>>>> qcom_swrm_ctrl *ctrl)
>>>>>        ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
>>>>>        ctrl->slave_status = val;
>>>>>    -    for (i = 0; i <= SDW_MAX_DEVICES; i++) {
>>>>> +    for (i = 1; i <= SDW_MAX_DEVICES; i++) {
>>>>>            u32 s;
>>>>>              s = (val >> (i * 2));