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

[Qiang Yu <quic_qianyu@xxxxxxxxxxx>]

On 4/17/2024 11:01 AM, Qiang Yu wrote:
> On 4/17/2024 2:12 AM, Mayank Rana wrote:
> > On 4/15/2024 8:50 PM, Qiang Yu wrote:
> > > On 4/16/2024 7:53 AM, Mayank Rana wrote:
> > > > Hi Qiang
> > > >
> > > > On 4/15/2024 1:49 AM, Qiang Yu wrote:
> > > > > Add mhi_pci_generic_edl_trigger as edl_trigger for some devices 
> > > > > (eg. SDX65)
> > > > > to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
> > > > > doorbell register and forcing an SOC reset afterwards.
> > > > >
> > > > > Signed-off-by: Qiang Yu <quic_qianyu@xxxxxxxxxxx>
> > > > > ---
> > > > >   drivers/bus/mhi/host/pci_generic.c | 47 
> > > > > ++++++++++++++++++++++++++++++++++++++
> > > > >   1 file changed, 47 insertions(+)
> > > > >
> > > > > diff --git a/drivers/bus/mhi/host/pci_generic.c 
> > > > > b/drivers/bus/mhi/host/pci_generic.c
> > > > > index 51639bf..cbf8a58 100644
> > > > > --- a/drivers/bus/mhi/host/pci_generic.c
> > > > > +++ b/drivers/bus/mhi/host/pci_generic.c
> > > > > @@ -27,12 +27,19 @@
> > > > >   #define PCI_VENDOR_ID_THALES    0x1269
> > > > >   #define PCI_VENDOR_ID_QUECTEL    0x1eac
> > > > >   +#define MHI_EDL_DB            91
> > > > > +#define MHI_EDL_COOKIE            0xEDEDEDED
> > > > > +
> > > > > +/* Device can enter EDL by first setting edl cookie then issuing 
> > > > > inband reset*/
> > > > > +#define MHI_PCI_GENERIC_EDL_TRIGGER    BIT(0)
> > > > > +
> > > > >   /**
> > > > >    * struct mhi_pci_dev_info - MHI PCI device specific information
> > > > >    * @config: MHI controller configuration
> > > > >    * @name: name of the PCI module
> > > > >    * @fw: firmware path (if any)
> > > > >    * @edl: emergency download mode firmware path (if any)
> > > > > + * @edl_trigger: each bit represents a different way to enter EDL
> > > > >    * @bar_num: PCI base address register to use for MHI MMIO 
> > > > > register space
> > > > >    * @dma_data_width: DMA transfer word size (32 or 64 bits)
> > > > >    * @mru_default: default MRU size for MBIM network packets
> > > > > @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
> > > > >       const char *name;
> > > > >       const char *fw;
> > > > >       const char *edl;
> > > > > +    unsigned int edl_trigger;
> > > > >       unsigned int bar_num;
> > > > >       unsigned int dma_data_width;
> > > > >       unsigned int mru_default;
> > > > > @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info 
> > > > > mhi_qcom_sdx75_info = {
> > > > >       .name = "qcom-sdx75m",
> > > > >       .fw = "qcom/sdx75m/xbl.elf",
> > > > >       .edl = "qcom/sdx75m/edl.mbn",
> > > > > +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> > > > >       .config = &modem_qcom_v2_mhiv_config,
> > > > >       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > > >       .dma_data_width = 32,
> > > > > @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info 
> > > > > mhi_qcom_sdx65_info = {
> > > > >       .name = "qcom-sdx65m",
> > > > >       .fw = "qcom/sdx65m/xbl.elf",
> > > > >       .edl = "qcom/sdx65m/edl.mbn",
> > > > > +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> > > > >       .config = &modem_qcom_v1_mhiv_config,
> > > > >       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > > >       .dma_data_width = 32,
> > > > > @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info 
> > > > > mhi_qcom_sdx55_info = {
> > > > >       .name = "qcom-sdx55m",
> > > > >       .fw = "qcom/sdx55m/sbl1.mbn",
> > > > >       .edl = "qcom/sdx55m/edl.mbn",
> > > > > +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
> > > > >       .config = &modem_qcom_v1_mhiv_config,
> > > > >       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
> > > > >       .dma_data_width = 32,
> > > > > @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
> > > > >       mod_timer(&mhi_pdev->health_check_timer, jiffies + 
> > > > > HEALTH_CHECK_PERIOD);
> > > > >   }
> > > > >   +static int mhi_pci_generic_edl_trigger(struct mhi_controller 
> > > > > *mhi_cntrl)
> > > > > +{
> > > > > +    void __iomem *base = mhi_cntrl->regs;
> > > > > +    void __iomem *edl_db;
> > > > > +    int ret;
> > > > > +    u32 val;
> > > > > +
> > > > > +    ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> > > > > +    if (ret) {
> > > > > +        dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before 
> > > > > trigger EDL\n");
> > > > > +        return ret;
> > > > > +    }
> > > > > +
> > > > > +    pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
> > > > > +    mhi_cntrl->runtime_get(mhi_cntrl);
> > > > > +
> > > > > +    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), 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.

Hi Mani, sorry, may I know the purpose of adding must_check attribute to
mhi_read_reg? In which case will mhi controller provide a callback that
returns error?

Thanks,
Qiang
> > > > > +    edl_db = base + val + (8 * MHI_EDL_DB);
> > > > > +
> > > > > +    mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, 
> > > > > upper_32_bits(MHI_EDL_COOKIE));
> > > > > +    mhi_cntrl->write_reg(mhi_cntrl, edl_db, 
> > > > > lower_32_bits(MHI_EDL_COOKIE));
> > > > > +
> > > > > +    mhi_soc_reset(mhi_cntrl);
> > > > > +
> > > > > +    mhi_cntrl->runtime_put(mhi_cntrl);
> > > > > +    mhi_device_put(mhi_cntrl->mhi_dev);
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > >   static int mhi_pci_probe(struct pci_dev *pdev, const struct 
> > > > > pci_device_id *id)
> > > > >   {
> > > > >       const struct mhi_pci_dev_info *info = (struct 
> > > > > mhi_pci_dev_info *) id->driver_data;
> > > > > @@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev 
> > > > > *pdev, const struct pci_device_id *id)
> > > > >       mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> > > > >       mhi_cntrl->mru = info->mru_default;
> > > > >   +    if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
> > > > > +        mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
> > > > > +
> > > > >       if (info->sideband_wake) {
> > > > >           mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
> > > > >           mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
> > > > Regards,
> > > > Mayank