* [PATCH v4 0/3] Polling for MHI ready @ 2021-03-10 23:31 Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Bhaumik Bhatt @ 2021-03-10 23:31 UTC (permalink / raw) To: manivannan.sadhasivam Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, carl.yin, naveen.kumar, loic.poulain, Bhaumik Bhatt v4: -Added reviewed-by tag -Return appropriate error code from mhi_poll_reg_field() -Fixed bug where mhi_poll_reg_field() returns success if polling times out -Added an interval_us variable in mhi_ready_state_transition() v3: -Removed config changes that crept in in the first patch v2: -Addressed review comments -Introduce new patch for to use controller defined read_reg() for polling -Add usage in RDDM download panic path as well Use polling instead of interrupt driven approach to wait for MHI ready state. 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. This patch series has been tested on an arm64 device. Bhaumik Bhatt (3): bus: mhi: core: Introduce internal register poll helper function bus: mhi: core: Move to polling method to wait for MHI ready bus: mhi: core: Use poll register read API for RDDM download drivers/bus/mhi/core/boot.c | 20 ++++++-------------- drivers/bus/mhi/core/internal.h | 3 +++ drivers/bus/mhi/core/main.c | 23 +++++++++++++++++++++++ drivers/bus/mhi/core/pm.c | 32 +++++++++++++++----------------- 4 files changed, 47 insertions(+), 31 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function 2021-03-10 23:31 [PATCH v4 0/3] Polling for MHI ready Bhaumik Bhatt @ 2021-03-10 23:31 ` Bhaumik Bhatt 2021-03-11 8:00 ` Loic Poulain 2021-03-10 23:31 ` [PATCH v4 2/3] bus: mhi: core: Move to polling method to wait for MHI ready Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 3/3] bus: mhi: core: Use poll register read API for RDDM download Bhaumik Bhatt 2 siblings, 1 reply; 10+ messages in thread From: Bhaumik Bhatt @ 2021-03-10 23:31 UTC (permalink / raw) To: manivannan.sadhasivam Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, carl.yin, naveen.kumar, loic.poulain, Bhaumik Bhatt 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..7c7f41a 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; + 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 ret; + + if (out == val) + return 0; + + udelay(delayus); + } + + return -ENOENT; +} + void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base, u32 offset, u32 val) { -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function 2021-03-10 23:31 ` [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt @ 2021-03-11 8:00 ` Loic Poulain 2021-03-11 19:59 ` Jeffrey Hugo 0 siblings, 1 reply; 10+ messages in thread From: Loic Poulain @ 2021-03-11 8:00 UTC (permalink / raw) To: Bhaumik Bhatt Cc: Manivannan Sadhasivam, linux-arm-msm, Hemant Kumar, Jeffrey Hugo, open list, Carl Yin(殷张成), Naveen Kumar Hi Bhaumik, On Thu, 11 Mar 2021 at 00:31, 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..7c7f41a 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; > + 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 ret; > + > + if (out == val) > + return 0; > + > + udelay(delayus); Have you read my previous comment? Do you really want to risk hogging the CPU for several seconds? we know that some devices take several seconds to start/boot. Why not using msleep variant here? Regards, Loic ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function 2021-03-11 8:00 ` Loic Poulain @ 2021-03-11 19:59 ` Jeffrey Hugo 2021-03-17 21:26 ` Bhaumik Bhatt 0 siblings, 1 reply; 10+ messages in thread From: Jeffrey Hugo @ 2021-03-11 19:59 UTC (permalink / raw) To: Loic Poulain, Bhaumik Bhatt Cc: Manivannan Sadhasivam, linux-arm-msm, Hemant Kumar, open list, Carl Yin(殷张成), Naveen Kumar On 3/11/2021 1:00 AM, Loic Poulain wrote: > Hi Bhaumik, > > On Thu, 11 Mar 2021 at 00:31, 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..7c7f41a 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; >> + 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 ret; >> + >> + if (out == val) >> + return 0; >> + >> + udelay(delayus); > > Have you read my previous comment? > Do you really want to risk hogging the CPU for several seconds? we > know that some devices take several seconds to start/boot. > Why not using msleep variant here? usleep_range() if there is a desire to stay in us units? Given that the use of this function is for 25ms in one case, I wonder if this warning is applicable: https://elixir.bootlin.com/linux/latest/source/include/linux/delay.h#L28 Counter point, 1ms latency over PCIe is not unusual. I know we've removed the PCIe dependencies from MHI, but PCIe is the real usecase at this time. Seems like this function could behave a bit weird if the parameter to udelay is something like "100", but the mhi_read_reg_field() call takes significantly longer than that. Feels like in some scenarios, we could actually exceed the timeout by a non-trivial margin. I guess I'm going back and forth in determining if us scale timing is a benefit in any way. -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function 2021-03-11 19:59 ` Jeffrey Hugo @ 2021-03-17 21:26 ` Bhaumik Bhatt 2021-03-18 16:13 ` Jeffrey Hugo 0 siblings, 1 reply; 10+ messages in thread From: Bhaumik Bhatt @ 2021-03-17 21:26 UTC (permalink / raw) To: Jeffrey Hugo Cc: Loic Poulain, Manivannan Sadhasivam, linux-arm-msm, Hemant Kumar, open list, Carl Yin(殷张成), Naveen Kumar, jhugo=codeaurora.org On 2021-03-11 11:59 AM, Jeffrey Hugo wrote: > On 3/11/2021 1:00 AM, Loic Poulain wrote: >> Hi Bhaumik, >> >> On Thu, 11 Mar 2021 at 00:31, 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..7c7f41a 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; >>> + 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 ret; >>> + >>> + if (out == val) >>> + return 0; >>> + >>> + udelay(delayus); >> >> Have you read my previous comment? >> Do you really want to risk hogging the CPU for several seconds? we >> know that some devices take several seconds to start/boot. >> Why not using msleep variant here? > > usleep_range() if there is a desire to stay in us units? > > Given that the use of this function is for 25ms in one case, I wonder > if this warning is applicable: > https://elixir.bootlin.com/linux/latest/source/include/linux/delay.h#L28 > > Counter point, 1ms latency over PCIe is not unusual. I know we've > removed the PCIe dependencies from MHI, but PCIe is the real usecase > at this time. Seems like this function could behave a bit weird if > the parameter to udelay is something like "100", but the > mhi_read_reg_field() call takes significantly longer than that. Feels > like in some scenarios, we could actually exceed the timeout by a > non-trivial margin. > > I guess I'm going back and forth in determining if us scale timing is > a benefit in any way. Thanks for all the inputs. I think a good idea here would be to use fsleep() API as we need to allow any timeout the caller specifies. Also, plan is to drop the patch #3 in this series since that will require a busywait due to the code being in panic path. I don't wish to accommodate another variable here for busywait but that would be an option to pick sleep or delay depending on the caller's path. Please respond if there are any concerns. Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function 2021-03-17 21:26 ` Bhaumik Bhatt @ 2021-03-18 16:13 ` Jeffrey Hugo 2021-03-18 16:43 ` Loic Poulain 0 siblings, 1 reply; 10+ messages in thread From: Jeffrey Hugo @ 2021-03-18 16:13 UTC (permalink / raw) To: bbhatt Cc: Loic Poulain, Manivannan Sadhasivam, linux-arm-msm, Hemant Kumar, open list, Carl Yin(殷张成), Naveen Kumar, jhugo=codeaurora.org On 3/17/2021 3:26 PM, Bhaumik Bhatt wrote: > On 2021-03-11 11:59 AM, Jeffrey Hugo wrote: >> On 3/11/2021 1:00 AM, Loic Poulain wrote: >>> Hi Bhaumik, >>> >>> On Thu, 11 Mar 2021 at 00:31, 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..7c7f41a 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; >>>> + 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 ret; >>>> + >>>> + if (out == val) >>>> + return 0; >>>> + >>>> + udelay(delayus); >>> >>> Have you read my previous comment? >>> Do you really want to risk hogging the CPU for several seconds? we >>> know that some devices take several seconds to start/boot. >>> Why not using msleep variant here? >> >> usleep_range() if there is a desire to stay in us units? >> >> Given that the use of this function is for 25ms in one case, I wonder >> if this warning is applicable: >> https://elixir.bootlin.com/linux/latest/source/include/linux/delay.h#L28 >> >> Counter point, 1ms latency over PCIe is not unusual. I know we've >> removed the PCIe dependencies from MHI, but PCIe is the real usecase >> at this time. Seems like this function could behave a bit weird if >> the parameter to udelay is something like "100", but the >> mhi_read_reg_field() call takes significantly longer than that. Feels >> like in some scenarios, we could actually exceed the timeout by a >> non-trivial margin. >> >> I guess I'm going back and forth in determining if us scale timing is >> a benefit in any way. > Thanks for all the inputs. I think a good idea here would be to use > fsleep() > API as we need to allow any timeout the caller specifies. Also, plan is to > drop the patch #3 in this series since that will require a busywait due to > the code being in panic path. > > I don't wish to accommodate another variable here for busywait but that > would be an option to pick sleep or delay depending on the caller's path. > > Please respond if there are any concerns. fsleep() would be some improvement, but I think there is still the issue Loic points out where if delayus is small, but timeout_ms is large (say 50us and 25s), this function will end up burning a lot of cpu cycles. However that is likely an edge case, and if your poll cycle is that small, I think it should be assumed that the event is expected to happen quickly, so the full timeout should not be hit. fsleep() does nothing to address this function possibly taking quite a bit longer than the timeout in overall wall time. Perhaps that is not a significant concern though. -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function 2021-03-18 16:13 ` Jeffrey Hugo @ 2021-03-18 16:43 ` Loic Poulain 2021-03-18 18:30 ` Bhaumik Bhatt 0 siblings, 1 reply; 10+ messages in thread From: Loic Poulain @ 2021-03-18 16:43 UTC (permalink / raw) To: Jeffrey Hugo, Bhaumik Bhatt Cc: Manivannan Sadhasivam, linux-arm-msm, Hemant Kumar, open list, Carl Yin(殷张成), Naveen Kumar, jhugo=codeaurora.org On Thu, 18 Mar 2021 at 17:13, Jeffrey Hugo <jhugo@codeaurora.org> wrote: > > On 3/17/2021 3:26 PM, Bhaumik Bhatt wrote: > > On 2021-03-11 11:59 AM, Jeffrey Hugo wrote: > >> On 3/11/2021 1:00 AM, Loic Poulain wrote: > >>> Hi Bhaumik, > >>> > >>> On Thu, 11 Mar 2021 at 00:31, 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..7c7f41a 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; > >>>> + 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 ret; > >>>> + > >>>> + if (out == val) > >>>> + return 0; > >>>> + > >>>> + udelay(delayus); > >>> > >>> Have you read my previous comment? > >>> Do you really want to risk hogging the CPU for several seconds? we > >>> know that some devices take several seconds to start/boot. > >>> Why not using msleep variant here? > >> > >> usleep_range() if there is a desire to stay in us units? > >> > >> Given that the use of this function is for 25ms in one case, I wonder > >> if this warning is applicable: > >> https://elixir.bootlin.com/linux/latest/source/include/linux/delay.h#L28 > >> > >> Counter point, 1ms latency over PCIe is not unusual. I know we've > >> removed the PCIe dependencies from MHI, but PCIe is the real usecase > >> at this time. Seems like this function could behave a bit weird if > >> the parameter to udelay is something like "100", but the > >> mhi_read_reg_field() call takes significantly longer than that. Feels > >> like in some scenarios, we could actually exceed the timeout by a > >> non-trivial margin. > >> > >> I guess I'm going back and forth in determining if us scale timing is > >> a benefit in any way. > > Thanks for all the inputs. I think a good idea here would be to use > > fsleep() > > API as we need to allow any timeout the caller specifies. Also, plan is to > > drop the patch #3 in this series since that will require a busywait due to > > the code being in panic path. > > > > I don't wish to accommodate another variable here for busywait but that > > would be an option to pick sleep or delay depending on the caller's path. > > > > Please respond if there are any concerns. > > fsleep() would be some improvement, but I think there is still the issue > Loic points out where if delayus is small, but timeout_ms is large (say > 50us and 25s), this function will end up burning a lot of cpu cycles > However that is likely an edge case, and if your poll cycle is that > small, I think it should be assumed that the event is expected to happen > quickly, so the full timeout should not be hit. Well, my point is that during initial power_up, with a device cold-booting, it can take several seconds for it to reach ready state (not a corner case). That why timeout_ms can be as large as 20 seconds for mhi_pci_modem. If polling is based on busy-wait, that means the while loop will not let the CPU running anything else for several seconds. Not sure what is the expected meaning of this timeout_ms in first place... maybe I just use it badly. Moreover, do we need microsecond latency on detecting ready transition, this is not a critical path, right? Regards, Loic ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function 2021-03-18 16:43 ` Loic Poulain @ 2021-03-18 18:30 ` Bhaumik Bhatt 0 siblings, 0 replies; 10+ messages in thread From: Bhaumik Bhatt @ 2021-03-18 18:30 UTC (permalink / raw) To: Loic Poulain, Manivannan Sadhasivam, hemantk Cc: Jeffrey Hugo, Manivannan Sadhasivam, linux-arm-msm, open list, Carl Yin(殷张成), Naveen Kumar, jhugo=codeaurora.org On 2021-03-18 09:43 AM, Loic Poulain wrote: > On Thu, 18 Mar 2021 at 17:13, Jeffrey Hugo <jhugo@codeaurora.org> > wrote: >> >> On 3/17/2021 3:26 PM, Bhaumik Bhatt wrote: >> > On 2021-03-11 11:59 AM, Jeffrey Hugo wrote: >> >> On 3/11/2021 1:00 AM, Loic Poulain wrote: >> >>> Hi Bhaumik, >> >>> >> >>> On Thu, 11 Mar 2021 at 00:31, 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..7c7f41a 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; >> >>>> + 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 ret; >> >>>> + >> >>>> + if (out == val) >> >>>> + return 0; >> >>>> + >> >>>> + udelay(delayus); >> >>> >> >>> Have you read my previous comment? >> >>> Do you really want to risk hogging the CPU for several seconds? we >> >>> know that some devices take several seconds to start/boot. >> >>> Why not using msleep variant here? >> >> >> >> usleep_range() if there is a desire to stay in us units? >> >> >> >> Given that the use of this function is for 25ms in one case, I wonder >> >> if this warning is applicable: >> >> https://elixir.bootlin.com/linux/latest/source/include/linux/delay.h#L28 >> >> >> >> Counter point, 1ms latency over PCIe is not unusual. I know we've >> >> removed the PCIe dependencies from MHI, but PCIe is the real usecase >> >> at this time. Seems like this function could behave a bit weird if >> >> the parameter to udelay is something like "100", but the >> >> mhi_read_reg_field() call takes significantly longer than that. Feels >> >> like in some scenarios, we could actually exceed the timeout by a >> >> non-trivial margin. >> >> >> >> I guess I'm going back and forth in determining if us scale timing is >> >> a benefit in any way. >> > Thanks for all the inputs. I think a good idea here would be to use >> > fsleep() >> > API as we need to allow any timeout the caller specifies. Also, plan is to >> > drop the patch #3 in this series since that will require a busywait due to >> > the code being in panic path. >> > >> > I don't wish to accommodate another variable here for busywait but that >> > would be an option to pick sleep or delay depending on the caller's path. >> > >> > Please respond if there are any concerns. >> >> fsleep() would be some improvement, but I think there is still the >> issue >> Loic points out where if delayus is small, but timeout_ms is large >> (say >> 50us and 25s), this function will end up burning a lot of cpu cycles >> However that is likely an edge case, and if your poll cycle is that >> small, I think it should be assumed that the event is expected to >> happen >> quickly, so the full timeout should not be hit. > > Well, my point is that during initial power_up, with a device > cold-booting, it can take several seconds for it to reach ready state > (not a corner case). That why timeout_ms can be as large as 20 seconds > for mhi_pci_modem. If polling is based on busy-wait, that means the > while loop will not let the CPU running anything else for several > seconds. Not sure what is the expected meaning of this timeout_ms in > first place... maybe I just use it badly. > > Moreover, do we need microsecond latency on detecting ready > transition, this is not a critical path, right? > > Regards, > Loic At initial boot, yes, device could take longer to boot. If we were to force caller to use an interval in the order of milliseconds, I'd still be using fsleep() internally anyway and just multiply the value by 1000 before passing it on as there's a need to check if the value is greater than 20ms or not. I don't wish to reinvent the wheel and implement what we already have in fsleep() internally for msec. It would be recommended that the caller specifies an interval of at least 20+ msec but I don't think we can enforce that. A point in favor of using microseconds is, if we were to expand usage of this API in the future for panic path to do a busywait, we wouldn't have to change the parameters. 3 options: 1. Use msec granularity and implement a partial fsleep for msec within the new API. 2. Use fsleep and leave it as usec granularity. 3. Leave it at usec, and add a busywait boolean allowing caller to choose between udelay() and fsleep() to also allow usage of this API in panic path (for patch #3). I like options 2 and 3. Hemant/Mani, your guidance is welcome. Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/3] bus: mhi: core: Move to polling method to wait for MHI ready 2021-03-10 23:31 [PATCH v4 0/3] Polling for MHI ready Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt @ 2021-03-10 23:31 ` Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 3/3] bus: mhi: core: Use poll register read API for RDDM download Bhaumik Bhatt 2 siblings, 0 replies; 10+ messages in thread From: Bhaumik Bhatt @ 2021-03-10 23:31 UTC (permalink / raw) To: manivannan.sadhasivam Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, carl.yin, naveen.kumar, loic.poulain, Bhaumik Bhatt 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 | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index 681960c..dcc7fe0 100644 --- a/drivers/bus/mhi/core/pm.c +++ b/drivers/bus/mhi/core/pm.c @@ -153,34 +153,32 @@ 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; + u32 interval_us = 25000; /* poll register field every 25 milliseconds */ 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, + interval_us); + 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, + interval_us); + if (ret) { + dev_err(dev, "Device failed to enter MHI Ready\n"); return -ETIMEDOUT; } -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 3/3] bus: mhi: core: Use poll register read API for RDDM download 2021-03-10 23:31 [PATCH v4 0/3] Polling for MHI ready Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 2/3] bus: mhi: core: Move to polling method to wait for MHI ready Bhaumik Bhatt @ 2021-03-10 23:31 ` Bhaumik Bhatt 2 siblings, 0 replies; 10+ messages in thread From: Bhaumik Bhatt @ 2021-03-10 23:31 UTC (permalink / raw) To: manivannan.sadhasivam Cc: linux-arm-msm, hemantk, jhugo, linux-kernel, carl.yin, naveen.kumar, loic.poulain, Bhaumik Bhatt Make use of mhi_poll_reg_field() API in order to poll for RDDM download in panic path to employ a common approach throughout the driver. Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org> --- drivers/bus/mhi/core/boot.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c index c2546bf..b9c44e0 100644 --- a/drivers/bus/mhi/core/boot.c +++ b/drivers/bus/mhi/core/boot.c @@ -60,7 +60,6 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl) u32 rx_status; enum mhi_ee_type ee; const u32 delayus = 2000; - u32 retry = (mhi_cntrl->timeout_ms * 1000) / delayus; const u32 rddm_timeout_us = 200000; int rddm_retry = rddm_timeout_us / delayus; void __iomem *base = mhi_cntrl->bhie; @@ -125,19 +124,12 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl) "Waiting for RDDM image download via BHIe, current EE:%s\n", TO_MHI_EXEC_STR(ee)); - while (retry--) { - ret = mhi_read_reg_field(mhi_cntrl, base, BHIE_RXVECSTATUS_OFFS, - BHIE_RXVECSTATUS_STATUS_BMSK, - BHIE_RXVECSTATUS_STATUS_SHFT, - &rx_status); - if (ret) - return -EIO; - - if (rx_status == BHIE_RXVECSTATUS_STATUS_XFER_COMPL) - return 0; - - udelay(delayus); - } + ret = mhi_poll_reg_field(mhi_cntrl, base, BHIE_RXVECSTATUS_OFFS, + BHIE_RXVECSTATUS_STATUS_BMSK, + BHIE_RXVECSTATUS_STATUS_SHFT, + BHIE_RXVECSTATUS_STATUS_XFER_COMPL, delayus); + if (!ret) + return 0; ee = mhi_get_exec_env(mhi_cntrl); ret = mhi_read_reg(mhi_cntrl, base, BHIE_RXVECSTATUS_OFFS, &rx_status); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-18 18:31 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-10 23:31 [PATCH v4 0/3] Polling for MHI ready Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 1/3] bus: mhi: core: Introduce internal register poll helper function Bhaumik Bhatt 2021-03-11 8:00 ` Loic Poulain 2021-03-11 19:59 ` Jeffrey Hugo 2021-03-17 21:26 ` Bhaumik Bhatt 2021-03-18 16:13 ` Jeffrey Hugo 2021-03-18 16:43 ` Loic Poulain 2021-03-18 18:30 ` Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 2/3] bus: mhi: core: Move to polling method to wait for MHI ready Bhaumik Bhatt 2021-03-10 23:31 ` [PATCH v4 3/3] bus: mhi: core: Use poll register read API for RDDM download Bhaumik Bhatt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).