Kernel provide read*_poll_* API family to support looping based polling
code pattern like below:
while (...)
{
val = op(addr);
condition = cond(val);
if (condition)
break;
/* Maybe some timeout handling stuff */
cpu_relax();
udelay();
}
As such, use readl_relaxed_poll_timeout_atomic() to implement atomic
register polling logic in gic-v3.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/irqchip/irq-gic-v3.c | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 98b0329b7154..b9d9375a3434 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -19,6 +19,7 @@
#include <linux/percpu.h>
#include <linux/refcount.h>
#include <linux/slab.h>
+#include <linux/iopoll.h>
#include <linux/irqchip.h>
#include <linux/irqchip/arm-gic-common.h>
@@ -251,17 +252,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
{
- u32 count = 1000000; /* 1s! */
+ u32 val;
- while (readl_relaxed(base + GICD_CTLR) & bit) {
- count--;
- if (!count) {
- pr_err_ratelimited("RWP timeout, gone fishing\n");
- return;
- }
- cpu_relax();
- udelay(1);
- }
+ if (readl_relaxed_poll_timeout_atomic(base + GICD_CTLR,
+ val, !(val & bit), 1, 1000000) == -ETIMEDOUT)
+ pr_err_ratelimited("RWP timeout, gone fishing\n");
}
/* Wait for completion of a distributor change */
@@ -279,7 +274,6 @@ static void gic_redist_wait_for_rwp(void)
static void gic_enable_redist(bool enable)
{
void __iomem *rbase;
- u32 count = 1000000; /* 1s! */
u32 val;
if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996)
@@ -301,14 +295,9 @@ static void gic_enable_redist(bool enable)
return; /* No PM support in this redistributor */
}
- while (--count) {
- val = readl_relaxed(rbase + GICR_WAKER);
- if (enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep))
- break;
- cpu_relax();
- udelay(1);
- }
- if (!count)
+ if (readl_relaxed_poll_timeout_atomic(rbase + GICR_WAKER,
+ val, enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep),
+ 1, 1000000) == -ETIMEDOUT)
pr_err_ratelimited("redistributor failed to %s...\n",
enable ? "wakeup" : "sleep");
}
--
2.27.0On Thu, 18 Jan 2024 11:27:36 +0000,
Dawei Li <dawei.li@shingroup.cn> wrote:
>
> Kernel provide read*_poll_* API family to support looping based polling
> code pattern like below:
> while (...)
> {
> val = op(addr);
> condition = cond(val);
> if (condition)
> break;
>
> /* Maybe some timeout handling stuff */
>
> cpu_relax();
> udelay();
> }
>
> As such, use readl_relaxed_poll_timeout_atomic() to implement atomic
> register polling logic in gic-v3.
>
> Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> ---
> drivers/irqchip/irq-gic-v3.c | 27 ++++++++-------------------
> 1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 98b0329b7154..b9d9375a3434 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -19,6 +19,7 @@
> #include <linux/percpu.h>
> #include <linux/refcount.h>
> #include <linux/slab.h>
> +#include <linux/iopoll.h>
>
> #include <linux/irqchip.h>
> #include <linux/irqchip/arm-gic-common.h>
> @@ -251,17 +252,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
>
> static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
> {
> - u32 count = 1000000; /* 1s! */
> + u32 val;
>
> - while (readl_relaxed(base + GICD_CTLR) & bit) {
> - count--;
> - if (!count) {
> - pr_err_ratelimited("RWP timeout, gone fishing\n");
> - return;
> - }
> - cpu_relax();
> - udelay(1);
> - }
> + if (readl_relaxed_poll_timeout_atomic(base + GICD_CTLR,
> + val, !(val & bit), 1, 1000000) == -ETIMEDOUT)
If you are doing this, please use a constant such as USEC_PER_SEC for
the timeout. And fix the alignment of the second line so that the
parameters are aligned vertically.
> + pr_err_ratelimited("RWP timeout, gone fishing\n");
> }
>
> /* Wait for completion of a distributor change */
> @@ -279,7 +274,6 @@ static void gic_redist_wait_for_rwp(void)
> static void gic_enable_redist(bool enable)
> {
> void __iomem *rbase;
> - u32 count = 1000000; /* 1s! */
> u32 val;
>
> if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996)
> @@ -301,14 +295,9 @@ static void gic_enable_redist(bool enable)
> return; /* No PM support in this redistributor */
> }
>
> - while (--count) {
> - val = readl_relaxed(rbase + GICR_WAKER);
> - if (enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep))
> - break;
> - cpu_relax();
> - udelay(1);
> - }
> - if (!count)
> + if (readl_relaxed_poll_timeout_atomic(rbase + GICR_WAKER,
> + val, enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep),
> + 1, 1000000) == -ETIMEDOUT)
> pr_err_ratelimited("redistributor failed to %s...\n",
> enable ? "wakeup" : "sleep");
> }
Same thing here.
M.
--
Without deviation from the norm, progress is not possible.
Hi Marc,
Thanks for the quick review.
On Thu, Jan 18, 2024 at 02:00:15PM +0000, Marc Zyngier wrote:
> On Thu, 18 Jan 2024 11:27:36 +0000,
> Dawei Li <dawei.li@shingroup.cn> wrote:
> >
> > Kernel provide read*_poll_* API family to support looping based polling
> > code pattern like below:
> > while (...)
> > {
> > val = op(addr);
> > condition = cond(val);
> > if (condition)
> > break;
> >
> > /* Maybe some timeout handling stuff */
> >
> > cpu_relax();
> > udelay();
> > }
> >
> > As such, use readl_relaxed_poll_timeout_atomic() to implement atomic
> > register polling logic in gic-v3.
> >
> > Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> > ---
> > drivers/irqchip/irq-gic-v3.c | 27 ++++++++-------------------
> > 1 file changed, 8 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 98b0329b7154..b9d9375a3434 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -19,6 +19,7 @@
> > #include <linux/percpu.h>
> > #include <linux/refcount.h>
> > #include <linux/slab.h>
> > +#include <linux/iopoll.h>
> >
> > #include <linux/irqchip.h>
> > #include <linux/irqchip/arm-gic-common.h>
> > @@ -251,17 +252,11 @@ static inline void __iomem *gic_dist_base(struct irq_data *d)
> >
> > static void gic_do_wait_for_rwp(void __iomem *base, u32 bit)
> > {
> > - u32 count = 1000000; /* 1s! */
> > + u32 val;
> >
> > - while (readl_relaxed(base + GICD_CTLR) & bit) {
> > - count--;
> > - if (!count) {
> > - pr_err_ratelimited("RWP timeout, gone fishing\n");
> > - return;
> > - }
> > - cpu_relax();
> > - udelay(1);
> > - }
> > + if (readl_relaxed_poll_timeout_atomic(base + GICD_CTLR,
> > + val, !(val & bit), 1, 1000000) == -ETIMEDOUT)
>
> If you are doing this, please use a constant such as USEC_PER_SEC for
> the timeout. And fix the alignment of the second line so that the
> parameters are aligned vertically.
Agreed, well defined constant is always preferable than magic number;
And yes, alignment is for better readability.
>
> > + pr_err_ratelimited("RWP timeout, gone fishing\n");
> > }
> >
> > /* Wait for completion of a distributor change */
> > @@ -279,7 +274,6 @@ static void gic_redist_wait_for_rwp(void)
> > static void gic_enable_redist(bool enable)
> > {
> > void __iomem *rbase;
> > - u32 count = 1000000; /* 1s! */
> > u32 val;
> >
> > if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996)
> > @@ -301,14 +295,9 @@ static void gic_enable_redist(bool enable)
> > return; /* No PM support in this redistributor */
> > }
> >
> > - while (--count) {
> > - val = readl_relaxed(rbase + GICR_WAKER);
> > - if (enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep))
> > - break;
> > - cpu_relax();
> > - udelay(1);
> > - }
> > - if (!count)
> > + if (readl_relaxed_poll_timeout_atomic(rbase + GICR_WAKER,
> > + val, enable ^ (bool)(val & GICR_WAKER_ChildrenAsleep),
> > + 1, 1000000) == -ETIMEDOUT)
> > pr_err_ratelimited("redistributor failed to %s...\n",
> > enable ? "wakeup" : "sleep");
> > }
>
> Same thing here.
Ack.
I will address two issues above and send respin of V2.
Thanks,
Dawei
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
>
© 2016 - 2025 Red Hat, Inc.