| Message ID | 1614138270-2374-1-git-send-email-bbhatt@codeaurora.org |
|---|---|
| Headers | show |
| Series | Polling for MHI ready | expand |
On Wed, 24 Feb 2021 at 04:44, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote: > > Introduce helper function to allow MHI core driver to poll for > a value in a register field. This helps reach a common path to > read and poll register values along with a retry time interval. > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > --- > drivers/bus/mhi/core/internal.h | 3 +++ > drivers/bus/mhi/core/main.c | 23 +++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h > index 6f80ec3..005286b 100644 > --- a/drivers/bus/mhi/core/internal.h > +++ b/drivers/bus/mhi/core/internal.h > @@ -643,6 +643,9 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl, > int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl, > void __iomem *base, u32 offset, u32 mask, > u32 shift, u32 *out); > +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl, > + void __iomem *base, u32 offset, u32 mask, > + u32 shift, u32 val, u32 delayus); > void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base, > u32 offset, u32 val); > void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base, > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 4e0131b..249ae26 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -4,6 +4,7 @@ > * > */ > > +#include <linux/delay.h> > #include <linux/device.h> > #include <linux/dma-direction.h> > #include <linux/dma-mapping.h> > @@ -37,6 +38,28 @@ int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl, > return 0; > } > > +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl, > + void __iomem *base, u32 offset, > + u32 mask, u32 shift, u32 val, u32 delayus) > +{ > + int ret = -ENOENT; > + u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus; Can we get the timeout from parameter, not sure all callers will want to wait the controller timeout_ms in the future. In case of PCI the mhi_cntrl->timeout_ms can be really huge given the device can take up to 15 seconds to completely start. > + > + while (retry--) { > + ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, shift, > + &out); > + if (ret) > + return -EIO; > + > + if (out == val) > + return 0; > + > + udelay(delayus); I would use a sleep variant (msleep) and millisecond parameter for the function. Regards, Loic
On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote: > Introduce helper function to allow MHI core driver to poll for > a value in a register field. This helps reach a common path to > read and poll register values along with a retry time interval. > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > --- > drivers/bus/mhi/core/internal.h | 3 +++ > drivers/bus/mhi/core/main.c | 23 +++++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h > index 6f80ec3..005286b 100644 > --- a/drivers/bus/mhi/core/internal.h > +++ b/drivers/bus/mhi/core/internal.h > @@ -643,6 +643,9 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl, > int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl, > void __iomem *base, u32 offset, u32 mask, > u32 shift, u32 *out); > +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl, > + void __iomem *base, u32 offset, u32 mask, > + u32 shift, u32 val, u32 delayus); > void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base, > u32 offset, u32 val); > void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base, > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 4e0131b..249ae26 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -4,6 +4,7 @@ > * > */ > > +#include <linux/delay.h> > #include <linux/device.h> > #include <linux/dma-direction.h> > #include <linux/dma-mapping.h> > @@ -37,6 +38,28 @@ int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl, > return 0; > } > > +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl, > + void __iomem *base, u32 offset, > + u32 mask, u32 shift, u32 val, u32 delayus) > +{ > + int ret = -ENOENT; > + u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus; > + > + while (retry--) { > + ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, shift, > + &out); > + if (ret) > + return -EIO; I generally dislike recoding return codes. Do you believe it adds value here? I'm concerned that if I'm debugging an error, I'll get EIO, which I trace to here, but then I don't know what the actual error from mhi_read_reg_field() was. > + > + if (out == val) > + return 0; > + > + udelay(delayus); > + } > + > + return ret; > +} > + > void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base, > u32 offset, u32 val) > { > -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote: > In certain devices, it is likely that there is no incoming MHI > interrupt for a transition to MHI READY state. One such example > is the move from Pass Through to an SBL or AMSS execution > environment. In order to facilitate faster bootup times as there > is no need to wait until timeout_ms completes, MHI host can poll > every 25 milliseconds to check if device has entered MHI READY > until a maximum timeout of twice the timeout_ms is reached. > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > --- > drivers/bus/mhi/core/pm.c | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c > index 681960c..5fe33d4 100644 > --- a/drivers/bus/mhi/core/pm.c > +++ b/drivers/bus/mhi/core/pm.c > @@ -153,34 +153,31 @@ static void mhi_toggle_dev_wake(struct mhi_controller *mhi_cntrl) > /* Handle device ready state transition */ > int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl) > { > - void __iomem *base = mhi_cntrl->regs; > struct mhi_event *mhi_event; > enum mhi_pm_state cur_state; > struct device *dev = &mhi_cntrl->mhi_dev->dev; > - u32 reset = 1, ready = 0; > int ret, i; > > - /* Wait for RESET to be cleared and READY bit to be set by the device */ > - wait_event_timeout(mhi_cntrl->state_event, > - MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state) || > - mhi_read_reg_field(mhi_cntrl, base, MHICTRL, > - MHICTRL_RESET_MASK, > - MHICTRL_RESET_SHIFT, &reset) || > - mhi_read_reg_field(mhi_cntrl, base, MHISTATUS, > - MHISTATUS_READY_MASK, > - MHISTATUS_READY_SHIFT, &ready) || > - (!reset && ready), > - msecs_to_jiffies(mhi_cntrl->timeout_ms)); > - > /* Check if device entered error state */ > if (MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) { > dev_err(dev, "Device link is not accessible\n"); > return -EIO; > } > > - /* Timeout if device did not transition to ready state */ > - if (reset || !ready) { > - dev_err(dev, "Device Ready timeout\n"); > + /* Wait for RESET to be cleared and READY bit to be set by the device */ > + 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"); > + return -ETIMEDOUT; > + } > + > + ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHISTATUS, > + MHISTATUS_READY_MASK, MHISTATUS_READY_SHIFT, 1, > + 25000); You use the magic number "25000" twice here. Its my understanding that the preference is to inline a magic number if its used in one spot, but use a macro if its used more than that. Both uses are confined to this function, and in close proximity, so chances that one gets updated without the other seem minimal, so this feels like a borderline case. I don't know if Mani has an opinion here. I'd probably err on the side of making a macro or a single variable. If not, I think some comments explaining the value are warranted (should comment the macro as well). > + if (ret) { > + dev_err(dev, "Device failed to enter MHI Ready\n"); > return -ETIMEDOUT; > } > > -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On 2021-03-10 10:57 AM, Jeffrey Hugo wrote: > On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote: >> Introduce helper function to allow MHI core driver to poll for >> a value in a register field. This helps reach a common path to >> read and poll register values along with a retry time interval. >> >> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >> --- >> drivers/bus/mhi/core/internal.h | 3 +++ >> drivers/bus/mhi/core/main.c | 23 +++++++++++++++++++++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/drivers/bus/mhi/core/internal.h >> b/drivers/bus/mhi/core/internal.h >> index 6f80ec3..005286b 100644 >> --- a/drivers/bus/mhi/core/internal.h >> +++ b/drivers/bus/mhi/core/internal.h >> @@ -643,6 +643,9 @@ int __must_check mhi_read_reg(struct >> mhi_controller *mhi_cntrl, >> int __must_check mhi_read_reg_field(struct mhi_controller >> *mhi_cntrl, >> void __iomem *base, u32 offset, u32 mask, >> u32 shift, u32 *out); >> +int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl, >> + void __iomem *base, u32 offset, u32 mask, >> + u32 shift, u32 val, u32 delayus); >> void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem >> *base, >> u32 offset, u32 val); >> void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void >> __iomem *base, >> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c >> index 4e0131b..249ae26 100644 >> --- a/drivers/bus/mhi/core/main.c >> +++ b/drivers/bus/mhi/core/main.c >> @@ -4,6 +4,7 @@ >> * >> */ >> +#include <linux/delay.h> >> #include <linux/device.h> >> #include <linux/dma-direction.h> >> #include <linux/dma-mapping.h> >> @@ -37,6 +38,28 @@ int __must_check mhi_read_reg_field(struct >> mhi_controller *mhi_cntrl, >> return 0; >> } >> +int __must_check mhi_poll_reg_field(struct mhi_controller >> *mhi_cntrl, >> + void __iomem *base, u32 offset, >> + u32 mask, u32 shift, u32 val, u32 delayus) >> +{ >> + int ret = -ENOENT; Make this int ret; >> + u32 out, retry = (mhi_cntrl->timeout_ms * 1000) / delayus; >> + >> + while (retry--) { >> + ret = mhi_read_reg_field(mhi_cntrl, base, offset, mask, shift, >> + &out); >> + if (ret) >> + return -EIO; > > I generally dislike recoding return codes. Do you believe it adds > value here? I'm concerned that if I'm debugging an error, I'll get > EIO, which I trace to here, but then I don't know what the actual > error from mhi_read_reg_field() was. > Thanks for pointing out. I don't think its necessary to recode and I can go back to returning whatever the reg_field API returns. I have added the proposed changes here which help fix a potential bug that we'd return 0 if read is successful but polling fails. >> + >> + if (out == val) >> + return 0; >> + >> + udelay(delayus); >> + } >> + >> + return ret; return -ENOENT; to signify failure to find the entry after poll duration completes >> +} >> + >> void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem >> *base, >> u32 offset, u32 val) >> { >> Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On 2021-03-10 11:02 AM, Jeffrey Hugo wrote: > On 2/23/2021 8:44 PM, Bhaumik Bhatt wrote: >> In certain devices, it is likely that there is no incoming MHI >> interrupt for a transition to MHI READY state. One such example >> is the move from Pass Through to an SBL or AMSS execution >> environment. In order to facilitate faster bootup times as there >> is no need to wait until timeout_ms completes, MHI host can poll >> every 25 milliseconds to check if device has entered MHI READY >> until a maximum timeout of twice the timeout_ms is reached. >> >> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >> --- >> drivers/bus/mhi/core/pm.c | 31 ++++++++++++++----------------- >> 1 file changed, 14 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c >> index 681960c..5fe33d4 100644 >> --- a/drivers/bus/mhi/core/pm.c >> +++ b/drivers/bus/mhi/core/pm.c >> @@ -153,34 +153,31 @@ static void mhi_toggle_dev_wake(struct >> mhi_controller *mhi_cntrl) >> /* Handle device ready state transition */ >> int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl) >> { >> - void __iomem *base = mhi_cntrl->regs; >> struct mhi_event *mhi_event; >> enum mhi_pm_state cur_state; >> struct device *dev = &mhi_cntrl->mhi_dev->dev; >> - u32 reset = 1, ready = 0; >> int ret, i; >> - /* Wait for RESET to be cleared and READY bit to be set by the >> device */ >> - wait_event_timeout(mhi_cntrl->state_event, >> - MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state) || >> - mhi_read_reg_field(mhi_cntrl, base, MHICTRL, >> - MHICTRL_RESET_MASK, >> - MHICTRL_RESET_SHIFT, &reset) || >> - mhi_read_reg_field(mhi_cntrl, base, MHISTATUS, >> - MHISTATUS_READY_MASK, >> - MHISTATUS_READY_SHIFT, &ready) || >> - (!reset && ready), >> - msecs_to_jiffies(mhi_cntrl->timeout_ms)); >> - >> /* Check if device entered error state */ >> if (MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) { >> dev_err(dev, "Device link is not accessible\n"); >> return -EIO; >> } >> - /* Timeout if device did not transition to ready state */ >> - if (reset || !ready) { >> - dev_err(dev, "Device Ready timeout\n"); >> + /* Wait for RESET to be cleared and READY bit to be set by the >> device */ >> + 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"); >> + return -ETIMEDOUT; >> + } >> + >> + ret = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs, MHISTATUS, >> + MHISTATUS_READY_MASK, MHISTATUS_READY_SHIFT, 1, >> + 25000); > > You use the magic number "25000" twice here. Its my understanding > that the preference is to inline a magic number if its used in one > spot, but use a macro if its used more than that. > > Both uses are confined to this function, and in close proximity, so > chances that one gets updated without the other seem minimal, so this > feels like a borderline case. I don't know if Mani has an opinion > here. > > I'd probably err on the side of making a macro or a single variable. > If not, I think some comments explaining the value are warranted > (should comment the macro as well). > I think just using a variable would be good enough here. I can add a comment when defining the interval variable. >> + if (ret) { >> + dev_err(dev, "Device failed to enter MHI Ready\n"); >> return -ETIMEDOUT; >> } >> Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project