| Message ID | 1620029090-8975-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Accepted |
| Commit | 44b1eba44dc537edf076f131f1eeee7544d0e04f |
| Headers | show |
| Series | [v2] bus: mhi: core: Fix power down latency | expand |
Hi Manivannan and Hemant, On Mon, 3 May 2021 at 09:55, Loic Poulain <loic.poulain@linaro.org> wrote: > > On graceful power-down/disable transition, when an MHI reset is > performed, the MHI device loses its context, including interrupt > configuration. However, the current implementation is waiting for > event(irq) driven state change to confirm reset has been completed, > which never happens, and causes reset timeout, leading to unexpected > high latency of the mhi_power_down procedure (up to 45 seconds). > > Fix that by moving to the recently introduced poll_reg_field method, > waiting for the reset bit to be cleared, in the same way as the > power_on procedure. > > Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state transitions") > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Should be considered for 5.13-rc* since it's a fix. Regards, Loic > --- > drivers/bus/mhi/core/pm.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c > index e2e59a3..704a5e2 100644 > --- a/drivers/bus/mhi/core/pm.c > +++ b/drivers/bus/mhi/core/pm.c > @@ -465,23 +465,15 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl) > > /* Trigger MHI RESET so that the device will not access host memory */ > if (!MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) { > - u32 in_reset = -1; > - unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms); > - > dev_dbg(dev, "Triggering MHI Reset in device\n"); > mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET); > > /* Wait for the reset bit to be cleared by the device */ > - ret = wait_event_timeout(mhi_cntrl->state_event, > - mhi_read_reg_field(mhi_cntrl, > - mhi_cntrl->regs, > - MHICTRL, > - MHICTRL_RESET_MASK, > - MHICTRL_RESET_SHIFT, > - &in_reset) || > - !in_reset, timeout); > - if (!ret || in_reset) > - dev_err(dev, "Device failed to exit MHI Reset state\n"); > + ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL, > + MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0, > + 25000); > + if (ret) > + dev_err(dev, "Device failed to clear MHI Reset\n"); > > /* > * Device will clear BHI_INTVEC as a part of RESET processing, > -- > 2.7.4 >
On 2021-05-03 01:04 AM, Loic Poulain wrote: > On graceful power-down/disable transition, when an MHI reset is > performed, the MHI device loses its context, including interrupt > configuration. However, the current implementation is waiting for > event(irq) driven state change to confirm reset has been completed, > which never happens, and causes reset timeout, leading to unexpected > high latency of the mhi_power_down procedure (up to 45 seconds). > > Fix that by moving to the recently introduced poll_reg_field method, > waiting for the reset bit to be cleared, in the same way as the > power_on procedure. > > Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state > transitions") > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > drivers/bus/mhi/core/pm.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c > index e2e59a3..704a5e2 100644 > --- a/drivers/bus/mhi/core/pm.c > +++ b/drivers/bus/mhi/core/pm.c > @@ -465,23 +465,15 @@ static void mhi_pm_disable_transition(struct > mhi_controller *mhi_cntrl) > > /* Trigger MHI RESET so that the device will not access host memory > */ > if (!MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) { > - u32 in_reset = -1; > - unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms); > - > dev_dbg(dev, "Triggering MHI Reset in device\n"); > mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET); > > /* Wait for the reset bit to be cleared by the device */ > - ret = wait_event_timeout(mhi_cntrl->state_event, > - mhi_read_reg_field(mhi_cntrl, > - mhi_cntrl->regs, > - MHICTRL, > - MHICTRL_RESET_MASK, > - MHICTRL_RESET_SHIFT, > - &in_reset) || > - !in_reset, timeout); > - if (!ret || in_reset) > - dev_err(dev, "Device failed to exit MHI Reset state\n"); > + ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL, > + MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0, > + 25000); > + if (ret) > + dev_err(dev, "Device failed to clear MHI Reset\n"); > > /* > * Device will clear BHI_INTVEC as a part of RESET processing, Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On 5/3/21 1:04 AM, Loic Poulain wrote: > On graceful power-down/disable transition, when an MHI reset is > performed, the MHI device loses its context, including interrupt > configuration. However, the current implementation is waiting for > event(irq) driven state change to confirm reset has been completed, > which never happens, and causes reset timeout, leading to unexpected > high latency of the mhi_power_down procedure (up to 45 seconds). > > Fix that by moving to the recently introduced poll_reg_field method, > waiting for the reset bit to be cleared, in the same way as the > power_on procedure. > > Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state transitions") > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On Mon, May 03, 2021 at 10:04:50AM +0200, Loic Poulain wrote: > On graceful power-down/disable transition, when an MHI reset is > performed, the MHI device loses its context, including interrupt > configuration. However, the current implementation is waiting for > event(irq) driven state change to confirm reset has been completed, > which never happens, and causes reset timeout, leading to unexpected > high latency of the mhi_power_down procedure (up to 45 seconds). > > Fix that by moving to the recently introduced poll_reg_field method, > waiting for the reset bit to be cleared, in the same way as the > power_on procedure. > > Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state transitions") > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks, Mani > --- > drivers/bus/mhi/core/pm.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c > index e2e59a3..704a5e2 100644 > --- a/drivers/bus/mhi/core/pm.c > +++ b/drivers/bus/mhi/core/pm.c > @@ -465,23 +465,15 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl) > > /* Trigger MHI RESET so that the device will not access host memory */ > if (!MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) { > - u32 in_reset = -1; > - unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms); > - > dev_dbg(dev, "Triggering MHI Reset in device\n"); > mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET); > > /* Wait for the reset bit to be cleared by the device */ > - ret = wait_event_timeout(mhi_cntrl->state_event, > - mhi_read_reg_field(mhi_cntrl, > - mhi_cntrl->regs, > - MHICTRL, > - MHICTRL_RESET_MASK, > - MHICTRL_RESET_SHIFT, > - &in_reset) || > - !in_reset, timeout); > - if (!ret || in_reset) > - dev_err(dev, "Device failed to exit MHI Reset state\n"); > + ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL, > + MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0, > + 25000); > + if (ret) > + dev_err(dev, "Device failed to clear MHI Reset\n"); > > /* > * Device will clear BHI_INTVEC as a part of RESET processing, > -- > 2.7.4 >
On Mon, May 03, 2021 at 10:04:50AM +0200, Loic Poulain wrote: > On graceful power-down/disable transition, when an MHI reset is > performed, the MHI device loses its context, including interrupt > configuration. However, the current implementation is waiting for > event(irq) driven state change to confirm reset has been completed, > which never happens, and causes reset timeout, leading to unexpected > high latency of the mhi_power_down procedure (up to 45 seconds). > > Fix that by moving to the recently introduced poll_reg_field method, > waiting for the reset bit to be cleared, in the same way as the > power_on procedure. > > Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state transitions") > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Applied to mhi-fixes! Thanks, Mani > --- > drivers/bus/mhi/core/pm.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c > index e2e59a3..704a5e2 100644 > --- a/drivers/bus/mhi/core/pm.c > +++ b/drivers/bus/mhi/core/pm.c > @@ -465,23 +465,15 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl) > > /* Trigger MHI RESET so that the device will not access host memory */ > if (!MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) { > - u32 in_reset = -1; > - unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms); > - > dev_dbg(dev, "Triggering MHI Reset in device\n"); > mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET); > > /* Wait for the reset bit to be cleared by the device */ > - ret = wait_event_timeout(mhi_cntrl->state_event, > - mhi_read_reg_field(mhi_cntrl, > - mhi_cntrl->regs, > - MHICTRL, > - MHICTRL_RESET_MASK, > - MHICTRL_RESET_SHIFT, > - &in_reset) || > - !in_reset, timeout); > - if (!ret || in_reset) > - dev_err(dev, "Device failed to exit MHI Reset state\n"); > + ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL, > + MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0, > + 25000); > + if (ret) > + dev_err(dev, "Device failed to clear MHI Reset\n"); > > /* > * Device will clear BHI_INTVEC as a part of RESET processing, > -- > 2.7.4 >
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index e2e59a3..704a5e2 100644 --- a/drivers/bus/mhi/core/pm.c +++ b/drivers/bus/mhi/core/pm.c @@ -465,23 +465,15 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl) /* Trigger MHI RESET so that the device will not access host memory */ if (!MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) { - u32 in_reset = -1; - unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms); - dev_dbg(dev, "Triggering MHI Reset in device\n"); mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET); /* Wait for the reset bit to be cleared by the device */ - ret = wait_event_timeout(mhi_cntrl->state_event, - mhi_read_reg_field(mhi_cntrl, - mhi_cntrl->regs, - MHICTRL, - MHICTRL_RESET_MASK, - MHICTRL_RESET_SHIFT, - &in_reset) || - !in_reset, timeout); - if (!ret || in_reset) - dev_err(dev, "Device failed to exit MHI Reset state\n"); + ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL, + MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 0, + 25000); + if (ret) + dev_err(dev, "Device failed to clear MHI Reset\n"); /* * Device will clear BHI_INTVEC as a part of RESET processing,
On graceful power-down/disable transition, when an MHI reset is performed, the MHI device loses its context, including interrupt configuration. However, the current implementation is waiting for event(irq) driven state change to confirm reset has been completed, which never happens, and causes reset timeout, leading to unexpected high latency of the mhi_power_down procedure (up to 45 seconds). Fix that by moving to the recently introduced poll_reg_field method, waiting for the reset bit to be cleared, in the same way as the power_on procedure. Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state transitions") Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/bus/mhi/core/pm.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) -- 2.7.4