Messages in this thread | | | Date | Wed, 17 Apr 2024 11:01:41 +0800 | Subject | Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL | From | Qiang Yu <> |
| |
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
| |