Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fe9b9897-ed37-386a-846f-c1f11087ed5c@linux.intel.com>
Date:   Fri, 16 Sep 2022 12:05:05 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [PATCH] soundwire: qcom: update status from device id 1
Content-Language: en-US
To:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        vkoul@kernel.org
Cc:     alsa-devel@alsa-project.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, sanyog.r.kale@intel.com,
        yung-chuan.liao@linux.intel.com, quic_srivasam@quicinc.com
References: <20220915124215.13703-1-srinivas.kandagatla@linaro.org>
 <3962348a-33b4-5941-4a0b-cb447a513a41@linux.intel.com>
 <bffab27c-6e25-5787-6958-648f9f2e2686@linaro.org>
 <2e1b81d2-e20f-db9f-b84e-b3c5ebb312cb@linux.intel.com>
 <4e42389e-c1c4-fe41-3bc8-03cc9a40ac0b@linaro.org>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <4e42389e-c1c4-fe41-3bc8-03cc9a40ac0b@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


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