| Message ID | 1605279602-18749-3-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mhi: pci_generic: Misc improvements | expand |
Hi Loic, On 11/13/20 6:59 AM, Loic Poulain wrote: > Ensure that the device is hard-reset on remove to restore its initial > state and avoid further issues on subsequent probe. > > This has been tested with Telit FN980m module. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/bus/mhi/pci_generic.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c > index 13a7e4f..09c6b26 100644 > --- a/drivers/bus/mhi/pci_generic.c > +++ b/drivers/bus/mhi/pci_generic.c > @@ -15,6 +15,8 @@ > > #define MHI_PCI_DEFAULT_BAR_NUM 0 > > +#define DEV_RESET_REG (0xB0) > + > /** > * struct mhi_pci_dev_info - MHI PCI device specific information > * @config: MHI controller configuration > @@ -166,6 +168,11 @@ static void mhi_pci_status_cb(struct mhi_controller *mhi_cntrl, > /* Nothing to do for now */ > } > > +static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl) > +{ > + writel(1, mhi_cntrl->regs + DEV_RESET_REG); > +} > + > static int mhi_pci_claim(struct mhi_controller *mhi_cntrl, > unsigned int bar_num, u64 dma_mask) > { > @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev) > mhi_power_down(mhi_cntrl, true); > mhi_unprepare_after_power_down(mhi_cntrl); > mhi_unregister_controller(mhi_cntrl); > + > + /* MHI-layer reset could not be enough, always hard-reset the device */ > + mhi_pci_reset(mhi_cntrl); Referring to MHI spec: Hosts writes this register to trigger a reset. This can be used when the host detects a timeout in the MHI protocol and can use the reset as a last resort to recover the device. Host should first attempt an MHI Reset procedure before resetting the entire device. What issue are you facing which requires you to do full device reset ? Based on the spec recommendation, looks like this should be a last resort. Thanks, Hemant -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Hemant, On Thu, 19 Nov 2020 at 02:46, Hemant Kumar <hemantk@codeaurora.org> wrote: > > Hi Loic, > > On 11/13/20 6:59 AM, Loic Poulain wrote: > > Ensure that the device is hard-reset on remove to restore its initial > > state and avoid further issues on subsequent probe. > > > > This has been tested with Telit FN980m module. > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > --- > > drivers/bus/mhi/pci_generic.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c > > index 13a7e4f..09c6b26 100644 > > --- a/drivers/bus/mhi/pci_generic.c > > +++ b/drivers/bus/mhi/pci_generic.c > > @@ -15,6 +15,8 @@ > > > > #define MHI_PCI_DEFAULT_BAR_NUM 0 > > > > +#define DEV_RESET_REG (0xB0) > > + > > /** > > * struct mhi_pci_dev_info - MHI PCI device specific information > > * @config: MHI controller configuration > > @@ -166,6 +168,11 @@ static void mhi_pci_status_cb(struct mhi_controller *mhi_cntrl, > > /* Nothing to do for now */ > > } > > > > +static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl) > > +{ > > + writel(1, mhi_cntrl->regs + DEV_RESET_REG); > > +} > > + > > static int mhi_pci_claim(struct mhi_controller *mhi_cntrl, > > unsigned int bar_num, u64 dma_mask) > > { > > @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev) > > mhi_power_down(mhi_cntrl, true); > > mhi_unprepare_after_power_down(mhi_cntrl); > > mhi_unregister_controller(mhi_cntrl); > > + > > + /* MHI-layer reset could not be enough, always hard-reset the device */ > > + mhi_pci_reset(mhi_cntrl); > > Referring to MHI spec: > Hosts writes this register to trigger a reset. This can be used when the > host detects a timeout in the MHI protocol and can use the reset as a > last resort to recover the device. Host should first attempt an MHI > Reset procedure before resetting the entire device. > > What issue are you facing which requires you to do full device reset ? > Based on the spec recommendation, looks like this should be a last resort. On module unload/reload, the device does not go through cold reset and can be in error state on further reload, causing mhi power up to fail. This patch simply resets the device in remove so that it is in the exact same condition as before probing (not only MHI layer, but all the device context), I think it makes sense and prevents any unexpected state on further reloading. Note also that unloading the module (or unbinding the device) is not something that usually happens, except when the user does it explicitly for any reason. Regards, Loic
On 11/19/2020 2:21 AM, Loic Poulain wrote: > Hi Hemant, > > On Thu, 19 Nov 2020 at 02:46, Hemant Kumar <hemantk@codeaurora.org> wrote: >> >> Hi Loic, >> >> On 11/13/20 6:59 AM, Loic Poulain wrote: >>> Ensure that the device is hard-reset on remove to restore its initial >>> state and avoid further issues on subsequent probe. >>> >>> This has been tested with Telit FN980m module. >>> >>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >>> --- >>> drivers/bus/mhi/pci_generic.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c >>> index 13a7e4f..09c6b26 100644 >>> --- a/drivers/bus/mhi/pci_generic.c >>> +++ b/drivers/bus/mhi/pci_generic.c >>> @@ -15,6 +15,8 @@ >>> >>> #define MHI_PCI_DEFAULT_BAR_NUM 0 >>> >>> +#define DEV_RESET_REG (0xB0) >>> + >>> /** >>> * struct mhi_pci_dev_info - MHI PCI device specific information >>> * @config: MHI controller configuration >>> @@ -166,6 +168,11 @@ static void mhi_pci_status_cb(struct mhi_controller *mhi_cntrl, >>> /* Nothing to do for now */ >>> } >>> >>> +static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl) >>> +{ >>> + writel(1, mhi_cntrl->regs + DEV_RESET_REG); >>> +} >>> + >>> static int mhi_pci_claim(struct mhi_controller *mhi_cntrl, >>> unsigned int bar_num, u64 dma_mask) >>> { >>> @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev) >>> mhi_power_down(mhi_cntrl, true); >>> mhi_unprepare_after_power_down(mhi_cntrl); >>> mhi_unregister_controller(mhi_cntrl); >>> + >>> + /* MHI-layer reset could not be enough, always hard-reset the device */ >>> + mhi_pci_reset(mhi_cntrl); >> >> Referring to MHI spec: >> Hosts writes this register to trigger a reset. This can be used when the >> host detects a timeout in the MHI protocol and can use the reset as a >> last resort to recover the device. Host should first attempt an MHI >> Reset procedure before resetting the entire device. >> >> What issue are you facing which requires you to do full device reset ? >> Based on the spec recommendation, looks like this should be a last resort. > > On module unload/reload, the device does not go through cold reset and > can be in error state on further reload, causing mhi power up to fail. > This patch simply resets the device in remove so that it is in the > exact same condition as before probing (not only MHI layer, but all > the device context), I think it makes sense and prevents any > unexpected state on further reloading. Note also that unloading the > module (or unbinding the device) is not something that usually > happens, except when the user does it explicitly for any reason. This seems unnecessary to me. Qaic has the same usecase, and the MHI state machine reset is sufficient. Perhaps you have a unique edge case though. However, you are implementing the soc_reset functionality in your driver, when its a common MHI functionality, and is something I would like to use. If you dig back, I proposed a sysfs extension to expose that to userspace, but I have a desire to use it from my driver, same as you. Would you please make MHI core changes to expose the soc_reset functionality out so that multiple drivers can use a common implementation? -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Jeffrey, On Wed, 25 Nov 2020 at 18:41, Jeffrey Hugo <jhugo@codeaurora.org> wrote: > >>> @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev) > >>> mhi_power_down(mhi_cntrl, true); > >>> mhi_unprepare_after_power_down(mhi_cntrl); > >>> mhi_unregister_controller(mhi_cntrl); > >>> + > >>> + /* MHI-layer reset could not be enough, always hard-reset the device */ > >>> + mhi_pci_reset(mhi_cntrl); > >> > >> Referring to MHI spec: > >> Hosts writes this register to trigger a reset. This can be used when the > >> host detects a timeout in the MHI protocol and can use the reset as a > >> last resort to recover the device. Host should first attempt an MHI > >> Reset procedure before resetting the entire device. > >> > >> What issue are you facing which requires you to do full device reset ? > >> Based on the spec recommendation, looks like this should be a last resort. > > > > On module unload/reload, the device does not go through cold reset and > > can be in error state on further reload, causing mhi power up to fail. > > This patch simply resets the device in remove so that it is in the > > exact same condition as before probing (not only MHI layer, but all > > the device context), I think it makes sense and prevents any > > unexpected state on further reloading. Note also that unloading the > > module (or unbinding the device) is not something that usually > > happens, except when the user does it explicitly for any reason. > > This seems unnecessary to me. Qaic has the same usecase, and the MHI > state machine reset is sufficient. Perhaps you have a unique edge case > though. > > However, you are implementing the soc_reset functionality in your > driver, when its a common MHI functionality, and is something I would > like to use. If you dig back, I proposed a sysfs extension to expose > that to userspace, but I have a desire to use it from my driver, same as > you. > > Would you please make MHI core changes to expose the soc_reset > functionality out so that multiple drivers can use a common implementation? I overlooked this reply, going to move that into MHI core, as you suggested. Thanks, Loic
On 2020-11-27 08:21 AM, Loic Poulain wrote: > Hi Jeffrey, > > On Wed, 25 Nov 2020 at 18:41, Jeffrey Hugo <jhugo@codeaurora.org> > wrote: >> >>> @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev) >> >>> mhi_power_down(mhi_cntrl, true); >> >>> mhi_unprepare_after_power_down(mhi_cntrl); >> >>> mhi_unregister_controller(mhi_cntrl); >> >>> + >> >>> + /* MHI-layer reset could not be enough, always hard-reset the device */ >> >>> + mhi_pci_reset(mhi_cntrl); >> >> >> >> Referring to MHI spec: >> >> Hosts writes this register to trigger a reset. This can be used when the >> >> host detects a timeout in the MHI protocol and can use the reset as a >> >> last resort to recover the device. Host should first attempt an MHI >> >> Reset procedure before resetting the entire device. >> >> >> >> What issue are you facing which requires you to do full device reset ? >> >> Based on the spec recommendation, looks like this should be a last resort. >> > >> > On module unload/reload, the device does not go through cold reset and >> > can be in error state on further reload, causing mhi power up to fail. >> > This patch simply resets the device in remove so that it is in the >> > exact same condition as before probing (not only MHI layer, but all >> > the device context), I think it makes sense and prevents any >> > unexpected state on further reloading. Note also that unloading the >> > module (or unbinding the device) is not something that usually >> > happens, except when the user does it explicitly for any reason. >> >> This seems unnecessary to me. Qaic has the same usecase, and the MHI >> state machine reset is sufficient. Perhaps you have a unique edge >> case >> though. >> >> However, you are implementing the soc_reset functionality in your >> driver, when its a common MHI functionality, and is something I would >> like to use. If you dig back, I proposed a sysfs extension to expose >> that to userspace, but I have a desire to use it from my driver, same >> as >> you. >> >> Would you please make MHI core changes to expose the soc_reset >> functionality out so that multiple drivers can use a common >> implementation? > > I overlooked this reply, going to move that into MHI core, as you > suggested. > > Thanks, > Loic Yes, this makes sense to do as Jeff suggested. Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
diff --git a/drivers/bus/mhi/pci_generic.c b/drivers/bus/mhi/pci_generic.c index 13a7e4f..09c6b26 100644 --- a/drivers/bus/mhi/pci_generic.c +++ b/drivers/bus/mhi/pci_generic.c @@ -15,6 +15,8 @@ #define MHI_PCI_DEFAULT_BAR_NUM 0 +#define DEV_RESET_REG (0xB0) + /** * struct mhi_pci_dev_info - MHI PCI device specific information * @config: MHI controller configuration @@ -166,6 +168,11 @@ static void mhi_pci_status_cb(struct mhi_controller *mhi_cntrl, /* Nothing to do for now */ } +static inline void mhi_pci_reset(struct mhi_controller *mhi_cntrl) +{ + writel(1, mhi_cntrl->regs + DEV_RESET_REG); +} + static int mhi_pci_claim(struct mhi_controller *mhi_cntrl, unsigned int bar_num, u64 dma_mask) { @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev) mhi_power_down(mhi_cntrl, true); mhi_unprepare_after_power_down(mhi_cntrl); mhi_unregister_controller(mhi_cntrl); + + /* MHI-layer reset could not be enough, always hard-reset the device */ + mhi_pci_reset(mhi_cntrl); + mhi_free_controller(mhi_cntrl); }
Ensure that the device is hard-reset on remove to restore its initial state and avoid further issues on subsequent probe. This has been tested with Telit FN980m module. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/bus/mhi/pci_generic.c | 11 +++++++++++ 1 file changed, 11 insertions(+) -- 2.7.4