Messages in this thread | | | Date | Wed, 17 Apr 2024 12:41:25 +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 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 >
| |