| Message ID | 1713170945-44640-4-git-send-email-quic_qianyu@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add sysfs entry to EDL mode | expand |
On Mon, Apr 15, 2024 at 04:49:05PM +0800, Qiang Yu wrote: bus: mhi: host: pci_generic: Add support for triggering EDL mode in modems > 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. > 'Some of the MHI modems like SDX65 based ones are capable of entering the EDL mode as per the standard triggering mechanism defined in the MHI spec <enter spec version>. So let's add a common mhi_pci_generic_edl_trigger() function that triggers the EDL mode in the device when user writes to the <insert full sysfs entry here> file.' > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> > --- > 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) This is not needed as of now. Let the edl_trigger be bool for now. When vendors want to add their own methods of triggering EDL, we can extend it. > + > /** > * 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 'capable of triggering EDL mode in the device (if supported)' > * @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"); 'Failed to wakeup the device' - Mani
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@quicinc.com> > --- > 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() ? > + 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
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@quicinc.com> >> --- >> 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 >> + 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
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@quicinc.com> >>> --- >>> 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. >>> + 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
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@quicinc.com> >>>> --- >>>> 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 >>>> + 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
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@quicinc.com> >>>>> --- >>>>> 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 >
On 4/16/2024 9:41 PM, Qiang Yu wrote: > > 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@quicinc.com> >>>>>> --- >>>>>> 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 don't see that any specific step required within this API i.e. doing undo as it is just reading registers. so there is nothing else needed to be performed here as part of register read failure scenario. > 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. There are more than 21 places in MHI host stack mhi_read_reg() API based error handling is performed. With this new code addition, why it is supposed to be exception here. So these are possible options available here: 1. If you don't want to perform error handling, provide good inline comment here saying why you don't want to perform error handling. 2. Remove __must_check annotation with mhi_read_reg() and don't check return value here. 3. Other option is to check if device is not into M0, then just return without performing anything here. Regards, Mayank > 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 >>
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
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 >
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; + + 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;
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@quicinc.com> --- drivers/bus/mhi/host/pci_generic.c | 47 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)