Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Qiang Yu <quic_qianyu@quicinc.com>]

On 4/22/2024 4:08 PM, Manivannan Sadhasivam wrote:
> On Wed, Apr 17, 2024 at 12:41:25PM +0800, Qiang Yu wrote:
>
> [...]
>
>>>>>>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>>>>>> +    if (ret)
>>>>>>> +        return ret;
>>>>>> Don't we need error handling part here i.e. calling
>>>>>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
>>>>> Hi Mayank
>>>>>
>>>>> After soc_reset, device will reboot to EDL mode and MHI state
>>>>> will be SYSERR. So host will fail to suspend
>>>>> anyway. The "error handling" we need here is actually to enter
>>>>> EDL mode, this will be done by SYSERR irq.
>>>>> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to
>>>>> balance mhi_cntrl->runtime_get() and
>>>>> mhi_device_get_sync().
>>>>>
>>>>> Thanks,
>>>>> Qiang
>>>> I am saying is there possibility that mhi_get_channel_doorbell()
>>>> returns error ?
>>>> If yes, then why don't we need error handling as part of it. you are
>>>> exiting if this API return error without doing anything.
>>> I think here mhi_get_channel_doorbell will not return error. But I still
>>> add a error checking because it invoked mhi_read_reg, which is a must
>>> check
>>> API. For modem mhi controller, this API eventually does a memory read.
>>> This memory read will return a normal value if link is up and all f's if
>>> link
>>> is bad.
>>>
>>> Thanks,
>>> Qiang
>> Actually, mhi_get_channel_doorbell should also be used in mhi_init_mmio to
>> replace the getting chdb operation by invoking mhi_read_reg as Mani
>> commented.
>> In mhi_init_mmio, we invoke mhi_read_reg many times, but there is also not
>> additionnal error handling.
>>
>> I'm not very sure the reason but perhaps if mhi_read_reg returns error (I
>> don't
>> know which controller will provide a memory read callback returning errors),
> Take a look at AIC100 driver: drivers/accel/qaic/mhi_controller.c
>
>> most
>> probably something is wrong in PCIe, which is not predictable by MHI and we
>> can
>> not add err handling every time invoking mhi_read_reg. But we have a timer
>> to
>> do health_check in pci_generic.c. If link is down, we will do
>> pci_function_reset
>> to try to reovery.
>>
> Right, but the MHI stack is designed to be bus agnostic. So if we happen to use
> it with other busses like USB, I2C etc... then read APIs may fail.
>
> Even with PCIe, read transaction may return all 1 response and MHI needs to
> treat it as an error condition. But sadly, both pci_generic and ath controllers
> are not checking for invalid read value. But those need to be fixed.
>
> Regarding Mayank's query, you should do error cleanups if
> mhi_get_channel_doorbell() API fails.
>
> - Mani
Hi Mani, Mayank

Checked drivers/accel/qaic/mhi_controller.c, the mhi_read_reg
callback of AIC100 does return -EIO if it reads out all'f.

Considering that pci_generic controller also needs to check for
invalid read value which is not fixed though. It's better to invoke
mhi_cntrl->runtime_put() and mhi_device_put() as error cleanups if
mhi_get_channel_doorbell returns error here.

Let me address all your comments and prepare next version patch.

Thanks,
Qiang
>