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

[Qiang Yu]

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
>