diff mbox series

[v2,2/5] gpio: loongson1: Use readl() & writel()

Message ID 20230302125215.214014-3-keguang.zhang@gmail.com (mailing list archive)
State Superseded
Headers show
Series Devicetree support for Loongson-1 GPIO | expand

Commit Message

Keguang Zhang March 2, 2023, 12:52 p.m. UTC 
This patch replace __raw_readl() & __raw_writel() with readl() & writel().

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V1 -> V2: Split this change to a separate patch
---
 drivers/gpio/gpio-loongson1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Bartosz Golaszewski March 6, 2023, 9:30 a.m. UTC | #1 
On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
>
> This patch replace __raw_readl() & __raw_writel() with readl() & writel().
>

Please say WHY you're doing this.

Bart

> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
> V1 -> V2: Split this change to a separate patch
> ---
>  drivers/gpio/gpio-loongson1.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> index 8862c9ea0d41..b6c11caa3ade 100644
> --- a/drivers/gpio/gpio-loongson1.c
> +++ b/drivers/gpio/gpio-loongson1.c
> @@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
>         unsigned long flags;
>
>         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> -                    gpio_reg_base + GPIO_CFG);
> +       writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> +              gpio_reg_base + GPIO_CFG);
>         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
>
>         return 0;
> @@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
>         unsigned long flags;
>
>         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> -                    gpio_reg_base + GPIO_CFG);
> +       writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> +              gpio_reg_base + GPIO_CFG);
>         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
>  }
>
> --
> 2.34.1
>
Keguang Zhang March 7, 2023, 3:45 a.m. UTC | #2 
On Mon, Mar 6, 2023 at 5:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> >
> > This patch replace __raw_readl() & __raw_writel() with readl() & writel().
> >
>
> Please say WHY you're doing this.
>
readl & writel contain memory barriers which can guarantee access order.

> Bart
>
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> > V1 -> V2: Split this change to a separate patch
> > ---
> >  drivers/gpio/gpio-loongson1.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > index 8862c9ea0d41..b6c11caa3ade 100644
> > --- a/drivers/gpio/gpio-loongson1.c
> > +++ b/drivers/gpio/gpio-loongson1.c
> > @@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
> >         unsigned long flags;
> >
> >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > -                    gpio_reg_base + GPIO_CFG);
> > +       writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > +              gpio_reg_base + GPIO_CFG);
> >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> >
> >         return 0;
> > @@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
> >         unsigned long flags;
> >
> >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > -                    gpio_reg_base + GPIO_CFG);
> > +       writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > +              gpio_reg_base + GPIO_CFG);
> >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> >  }
> >
> > --
> > 2.34.1
> >



--
Best regards,

Kelvin Cheung
David Laight March 8, 2023, 9:42 a.m. UTC | #3 
From: Keguang Zhang
> Sent: 07 March 2023 03:46
> 
> On Mon, Mar 6, 2023 at 5:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > >
> > > This patch replace __raw_readl() & __raw_writel() with readl() & writel().
> > >
> >
> > Please say WHY you're doing this.
> >
> readl & writel contain memory barriers which can guarantee access order.

So what...

There is a data dependency between the read and write.
The read can't be scheduled before the lock is acquired.
The write can't be scheduled after the lock is released.

So any barriers in readl()/writel() aren't needed.

If they are only compile barriers they'll have no real effect.
OTOH if the cpu needs actual synchronising instructions (as some
ppc do) then they will slow things down.

	David

> 
> > Bart
> >
> > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > ---
> > > V1 -> V2: Split this change to a separate patch
> > > ---
> > >  drivers/gpio/gpio-loongson1.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > > index 8862c9ea0d41..b6c11caa3ade 100644
> > > --- a/drivers/gpio/gpio-loongson1.c
> > > +++ b/drivers/gpio/gpio-loongson1.c
> > > @@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
> > >         unsigned long flags;
> > >
> > >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > > -                    gpio_reg_base + GPIO_CFG);
> > > +       writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > > +              gpio_reg_base + GPIO_CFG);
> > >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> > >
> > >         return 0;
> > > @@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
> > >         unsigned long flags;
> > >
> > >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > > -                    gpio_reg_base + GPIO_CFG);
> > > +       writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > > +              gpio_reg_base + GPIO_CFG);
> > >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> > >  }
> > >
> > > --
> > > 2.34.1
> > >
> 
> 
> 
> --
> Best regards,
> 
> Kelvin Cheung

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Keguang Zhang March 8, 2023, 11:35 a.m. UTC | #4 
On Wed, Mar 8, 2023 at 5:42 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Keguang Zhang
> > Sent: 07 March 2023 03:46
> >
> > On Mon, Mar 6, 2023 at 5:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > > >
> > > > This patch replace __raw_readl() & __raw_writel() with readl() & writel().
> > > >
> > >
> > > Please say WHY you're doing this.
> > >
> > readl & writel contain memory barriers which can guarantee access order.
>
> So what...
>
> There is a data dependency between the read and write.
> The read can't be scheduled before the lock is acquired.
> The write can't be scheduled after the lock is released.
>
> So any barriers in readl()/writel() aren't needed.
>
> If they are only compile barriers they'll have no real effect.
> OTOH if the cpu needs actual synchronising instructions (as some
> ppc do) then they will slow things down.
>
Thanks for the explanation.
The intention of this change is to prevent possible order issues.
At present, __raw_readl() & __raw_writel() do work fine.
I will drop this patch in the next version.


>         David
>
> >
> > > Bart
> > >
> > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > > ---
> > > > V1 -> V2: Split this change to a separate patch
> > > > ---
> > > >  drivers/gpio/gpio-loongson1.c | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > > > index 8862c9ea0d41..b6c11caa3ade 100644
> > > > --- a/drivers/gpio/gpio-loongson1.c
> > > > +++ b/drivers/gpio/gpio-loongson1.c
> > > > @@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
> > > >         unsigned long flags;
> > > >
> > > >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > > > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > > > -                    gpio_reg_base + GPIO_CFG);
> > > > +       writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > > > +              gpio_reg_base + GPIO_CFG);
> > > >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> > > >
> > > >         return 0;
> > > > @@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
> > > >         unsigned long flags;
> > > >
> > > >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > > > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > > > -                    gpio_reg_base + GPIO_CFG);
> > > > +       writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > > > +              gpio_reg_base + GPIO_CFG);
> > > >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> > > >  }
> > > >
> > > > --
> > > > 2.34.1
> > > >
> >
> >
> >
> > --
> > Best regards,
> >
> > Kelvin Cheung
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)



--
Best regards,

Kelvin Cheung
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
index 8862c9ea0d41..b6c11caa3ade 100644
--- a/drivers/gpio/gpio-loongson1.c
+++ b/drivers/gpio/gpio-loongson1.c
@@ -23,8 +23,8 @@  static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
-	__raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
-		     gpio_reg_base + GPIO_CFG);
+	writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
+	       gpio_reg_base + GPIO_CFG);
 	raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 
 	return 0;
@@ -35,8 +35,8 @@  static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
-	__raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
-		     gpio_reg_base + GPIO_CFG);
+	writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
+	       gpio_reg_base + GPIO_CFG);
 	raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 }