[U-Boot,03/10] spi: sun4i: Simplify reg writes using set/clrbits_le32

Message ID 20190209131500.29954-4-jagan@amarulasolutions.com
State Changes Requested
Delegated to: Jagannadha Sutradharudu Teki
Headers show
Series
  • spi: Add Allwinner A31 SPI driver
Related show

Commit Message

Jagan Teki Feb. 9, 2019, 1:14 p.m.
Update the existing register writes using setbits_le32 and
clrbits_le32 in required places.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/spi/sun4i_spi.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

Comments

André Przywara Feb. 13, 2019, 1:14 a.m. | #1
On 09/02/2019 13:14, Jagan Teki wrote:
> Update the existing register writes using setbits_le32 and
> clrbits_le32 in required places.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/spi/sun4i_spi.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
> index 87b396a96e..5446cebe7c 100644
> --- a/drivers/spi/sun4i_spi.c
> +++ b/drivers/spi/sun4i_spi.c
> @@ -283,20 +283,18 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
>  {
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
>  
> -	writel(SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP |
> -	       SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW,
> -	       &priv->regs->ctl);
> +	setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE |
> +		     SUN4I_CTL_MASTER | SUN4I_CTL_TP |
> +		     SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW);
> +

Careful, this changes semantics. The original call explicitly cleared
all the remaining bits, which is important for the setup (some bits are
default 1).
And besides I am not sure what this change would improve anyway, as it
isn't shorter. If at all, I'd use clrsetbits_le32(), to mask off the
reserved bits 31:21.

Rest is fine.

Cheers,
Andre.


>  	return 0;
>  }
>  
>  static int sun4i_spi_release_bus(struct udevice *dev)
>  {
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
> -	u32 reg;
>  
> -	reg = readl(&priv->regs->ctl);
> -	reg &= ~SUN4I_CTL_ENABLE;
> -	writel(reg, &priv->regs->ctl);
> +	clrbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE);
>  
>  	return 0;
>  }
> @@ -309,7 +307,7 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  	struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
>  
>  	u32 len = bitlen / 8;
> -	u32 reg, rx_fifocnt;
> +	u32 rx_fifocnt;
>  	u8 nbytes;
>  	int ret;
>  
> @@ -324,10 +322,8 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  	if (flags & SPI_XFER_BEGIN)
>  		sun4i_spi_set_cs(bus, slave_plat->cs, true);
>  
> -	reg = readl(&priv->regs->ctl);
> -
>  	/* Reset FIFOs */
> -	writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl);
> +	setbits_le32(&priv->regs->ctl, SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
>  
>  	while (len) {
>  		/* Setup the transfer now... */
> @@ -341,8 +337,7 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  		sun4i_spi_fill_fifo(priv, nbytes);
>  
>  		/* Start the transfer */
> -		reg = readl(&priv->regs->ctl);
> -		writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl);
> +		setbits_le32(&priv->regs->ctl, SUN4I_CTL_XCH);
>  
>  		/* Wait till RX FIFO to be empty */
>  		ret = readl_poll_timeout(&priv->regs->ctl, rx_fifocnt,
>
Jagan Teki Feb. 13, 2019, 10:24 a.m. | #2
On Wed, Feb 13, 2019 at 6:46 AM André Przywara <andre.przywara@arm.com> wrote:
>
> On 09/02/2019 13:14, Jagan Teki wrote:
> > Update the existing register writes using setbits_le32 and
> > clrbits_le32 in required places.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  drivers/spi/sun4i_spi.c | 21 ++++++++-------------
> >  1 file changed, 8 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
> > index 87b396a96e..5446cebe7c 100644
> > --- a/drivers/spi/sun4i_spi.c
> > +++ b/drivers/spi/sun4i_spi.c
> > @@ -283,20 +283,18 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
> >  {
> >       struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
> >
> > -     writel(SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP |
> > -            SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW,
> > -            &priv->regs->ctl);
> > +     setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE |
> > +                  SUN4I_CTL_MASTER | SUN4I_CTL_TP |
> > +                  SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW);
> > +
>
> Careful, this changes semantics. The original call explicitly cleared
> all the remaining bits, which is important for the setup (some bits are
> default 1).

Usually claiming would need to setup require bits, on that case
setbits is safer rather than complete writel

> And besides I am not sure what this change would improve anyway, as it
> isn't shorter. If at all, I'd use clrsetbits_le32(), to mask off the
> reserved bits 31:21.

It's not about the shorter and just enable require bits which is
usually do any claim call in SPI.

Patch

diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
index 87b396a96e..5446cebe7c 100644
--- a/drivers/spi/sun4i_spi.c
+++ b/drivers/spi/sun4i_spi.c
@@ -283,20 +283,18 @@  static int sun4i_spi_claim_bus(struct udevice *dev)
 {
 	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
 
-	writel(SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP |
-	       SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW,
-	       &priv->regs->ctl);
+	setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE |
+		     SUN4I_CTL_MASTER | SUN4I_CTL_TP |
+		     SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW);
+
 	return 0;
 }
 
 static int sun4i_spi_release_bus(struct udevice *dev)
 {
 	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
-	u32 reg;
 
-	reg = readl(&priv->regs->ctl);
-	reg &= ~SUN4I_CTL_ENABLE;
-	writel(reg, &priv->regs->ctl);
+	clrbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE);
 
 	return 0;
 }
@@ -309,7 +307,7 @@  static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
 
 	u32 len = bitlen / 8;
-	u32 reg, rx_fifocnt;
+	u32 rx_fifocnt;
 	u8 nbytes;
 	int ret;
 
@@ -324,10 +322,8 @@  static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	if (flags & SPI_XFER_BEGIN)
 		sun4i_spi_set_cs(bus, slave_plat->cs, true);
 
-	reg = readl(&priv->regs->ctl);
-
 	/* Reset FIFOs */
-	writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl);
+	setbits_le32(&priv->regs->ctl, SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
 
 	while (len) {
 		/* Setup the transfer now... */
@@ -341,8 +337,7 @@  static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		sun4i_spi_fill_fifo(priv, nbytes);
 
 		/* Start the transfer */
-		reg = readl(&priv->regs->ctl);
-		writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl);
+		setbits_le32(&priv->regs->ctl, SUN4I_CTL_XCH);
 
 		/* Wait till RX FIFO to be empty */
 		ret = readl_poll_timeout(&priv->regs->ctl, rx_fifocnt,