| Message ID | 1509368685-29112-5-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/30, Tero Kristo wrote: > This will happen on certain platforms when entering / leaving suspend > and the system attempts to disable certain clocks at very early/late > phase, burping out a warning from timekeeping core. Avoid the issue > by checking if the timekeeping is suspended and using the fallback > udelay approach for checking timeouts. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/ti/clkctrl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c > index 284ba449..91ddc92 100644 > --- a/drivers/clk/ti/clkctrl.c > +++ b/drivers/clk/ti/clkctrl.c > @@ -21,6 +21,7 @@ > #include <linux/of_address.h> > #include <linux/clk/ti.h> > #include <linux/delay.h> > +#include <linux/timekeeping.h> > #include "clock.h" > > #define NO_IDLEST 0x1 > @@ -90,7 +91,7 @@ static bool _omap4_is_ready(u32 val) > > static bool _omap4_is_timeout(union omap4_timeout *time, u32 timeout) > { > - if (unlikely(_early_timeout)) { > + if (unlikely(_early_timeout || timekeeping_suspended)) { > if (time->cycles++ < timeout) { This would be the second user of timekeeping_suspended outside of timekeeping core. Why don't we just udelay(1) every time we call this function? The loop on ktime without any sort of delay in it may actually spin faster, especially because we can't get interrupted here (irqs are off). And irqs + preemption enabled is typically where you would want to use ktime instead of counting udelay() calls to see if you hit a timeout. This code seems over-complicated given the constraints at the call-sites.
On 03/11/17 17:43, Stephen Boyd wrote: > On 10/30, Tero Kristo wrote: >> This will happen on certain platforms when entering / leaving suspend >> and the system attempts to disable certain clocks at very early/late >> phase, burping out a warning from timekeeping core. Avoid the issue >> by checking if the timekeeping is suspended and using the fallback >> udelay approach for checking timeouts. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/clk/ti/clkctrl.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c >> index 284ba449..91ddc92 100644 >> --- a/drivers/clk/ti/clkctrl.c >> +++ b/drivers/clk/ti/clkctrl.c >> @@ -21,6 +21,7 @@ >> #include <linux/of_address.h> >> #include <linux/clk/ti.h> >> #include <linux/delay.h> >> +#include <linux/timekeeping.h> > >> #include "clock.h" >> >> #define NO_IDLEST 0x1 >> @@ -90,7 +91,7 @@ static bool _omap4_is_ready(u32 val) >> >> static bool _omap4_is_timeout(union omap4_timeout *time, u32 timeout) >> { >> - if (unlikely(_early_timeout)) { >> + if (unlikely(_early_timeout || timekeeping_suspended)) { >> if (time->cycles++ < timeout) { > > This would be the second user of timekeeping_suspended outside of > timekeeping core. Why don't we just udelay(1) every time we call > this function? The loop on ktime without any sort of delay in it > may actually spin faster, especially because we can't get > interrupted here (irqs are off). And irqs + preemption enabled is > typically where you would want to use ktime instead of counting > udelay() calls to see if you hit a timeout. It actually was originally just udelay() but I changed it to use the ktime_get() approach way back due to comments provided on some early revisions of this patch. See: https://patchwork.kernel.org/patch/7884371/ > > This code seems over-complicated given the constraints at the > call-sites. > True, I can easily change it to just use udelay() if wanted. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/06, Tero Kristo wrote: > On 03/11/17 17:43, Stephen Boyd wrote: > >On 10/30, Tero Kristo wrote: > >>@@ -90,7 +91,7 @@ static bool _omap4_is_ready(u32 val) > >> static bool _omap4_is_timeout(union omap4_timeout *time, u32 timeout) > >> { > >>- if (unlikely(_early_timeout)) { > >>+ if (unlikely(_early_timeout || timekeeping_suspended)) { > >> if (time->cycles++ < timeout) { > > > >This would be the second user of timekeeping_suspended outside of > >timekeeping core. Why don't we just udelay(1) every time we call > >this function? The loop on ktime without any sort of delay in it > >may actually spin faster, especially because we can't get > >interrupted here (irqs are off). And irqs + preemption enabled is > >typically where you would want to use ktime instead of counting > >udelay() calls to see if you hit a timeout. > > It actually was originally just udelay() but I changed it to use the > ktime_get() approach way back due to comments provided on some early > revisions of this patch. See: > https://patchwork.kernel.org/patch/7884371/ > Ok, so we use ktime to spin faster on the bit than udelay() would allow us to. That looks to be on purpose because udelay(1) is too long between bit checks. What is causing us to call this path after timekeeping has been suspended? Please add some more specifics to the commit text so we know exactly where it's happening. Also add a comment above the if statement describing why we're checking the variable so it isn't buried in commit text somewhere and Cc timekeeping maintainers on the patch please.
On 07/11/17 00:18, Stephen Boyd wrote: > On 11/06, Tero Kristo wrote: >> On 03/11/17 17:43, Stephen Boyd wrote: >>> On 10/30, Tero Kristo wrote: >>>> @@ -90,7 +91,7 @@ static bool _omap4_is_ready(u32 val) >>>> static bool _omap4_is_timeout(union omap4_timeout *time, u32 timeout) >>>> { >>>> - if (unlikely(_early_timeout)) { >>>> + if (unlikely(_early_timeout || timekeeping_suspended)) { >>>> if (time->cycles++ < timeout) { >>> >>> This would be the second user of timekeeping_suspended outside of >>> timekeeping core. Why don't we just udelay(1) every time we call >>> this function? The loop on ktime without any sort of delay in it >>> may actually spin faster, especially because we can't get >>> interrupted here (irqs are off). And irqs + preemption enabled is >>> typically where you would want to use ktime instead of counting >>> udelay() calls to see if you hit a timeout. >> >> It actually was originally just udelay() but I changed it to use the >> ktime_get() approach way back due to comments provided on some early >> revisions of this patch. See: >> https://patchwork.kernel.org/patch/7884371/ >> > > Ok, so we use ktime to spin faster on the bit than udelay() would > allow us to. That looks to be on purpose because udelay(1) is too > long between bit checks. > > What is causing us to call this path after timekeeping has been > suspended? Please add some more specifics to the commit text so > we know exactly where it's happening. Also add a comment above > the if statement describing why we're checking the variable so it > isn't buried in commit text somewhere and Cc timekeeping > maintainers on the patch please. Ok, I'll comment this in the code and repost. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c index 284ba449..91ddc92 100644 --- a/drivers/clk/ti/clkctrl.c +++ b/drivers/clk/ti/clkctrl.c @@ -21,6 +21,7 @@ #include <linux/of_address.h> #include <linux/clk/ti.h> #include <linux/delay.h> +#include <linux/timekeeping.h> #include "clock.h" #define NO_IDLEST 0x1 @@ -90,7 +91,7 @@ static bool _omap4_is_ready(u32 val) static bool _omap4_is_timeout(union omap4_timeout *time, u32 timeout) { - if (unlikely(_early_timeout)) { + if (unlikely(_early_timeout || timekeeping_suspended)) { if (time->cycles++ < timeout) { udelay(1); return false;
This will happen on certain platforms when entering / leaving suspend and the system attempts to disable certain clocks at very early/late phase, burping out a warning from timekeeping core. Avoid the issue by checking if the timekeeping is suspended and using the fallback udelay approach for checking timeouts. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/clk/ti/clkctrl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)