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@xxxxxxxxxx>
>>>>> ---
>>>>> 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));
