diff mbox series

[U-Boot,v2,4/8] spi: sun4i: Access registers and bits via enum offsets

Message ID 20190214083614.29559-5-jagan@amarulasolutions.com
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show
Series spi: Add Allwinner A31 SPI driver | expand

Commit Message

Jagan Teki Feb. 14, 2019, 8:36 a.m. UTC
Allwinner support two different SPI controllers one for A10 and
another for A31 with minimal changes in register offsets and
respective register bits, but the logic for accessing the SPI
master via SPI slave remains nearly similar.

Add enum offsets for register set and register bits, so-that
it can access both classes of SPI controllers.

Assign same control register for global, transfer and fifo control
registers to make the same code compatible with A31 SPI controller.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Note: SPI_REG still seems to have checkpatch warning.

 drivers/spi/sun4i_spi.c | 153 +++++++++++++++++++++++++++++-----------
 1 file changed, 111 insertions(+), 42 deletions(-)

Comments

Andre Przywara Feb. 14, 2019, 11:56 p.m. UTC | #1
On 14/02/2019 08:36, Jagan Teki wrote:
> Allwinner support two different SPI controllers one for A10 and
> another for A31 with minimal changes in register offsets and
> respective register bits, but the logic for accessing the SPI
> master via SPI slave remains nearly similar.
> 
> Add enum offsets for register set and register bits, so-that
> it can access both classes of SPI controllers.
> 
> Assign same control register for global, transfer and fifo control
> registers to make the same code compatible with A31 SPI controller.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Note: SPI_REG still seems to have checkpatch warning.

It's a CHECK, and it's fine, as long as you pass only "priv" in, at
least. I think we can leave it this way.

> 
>  drivers/spi/sun4i_spi.c | 153 +++++++++++++++++++++++++++++-----------
>  1 file changed, 111 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
> index 0b1663038c..afc351c292 100644
> --- a/drivers/spi/sun4i_spi.c
> +++ b/drivers/spi/sun4i_spi.c
> @@ -83,7 +83,6 @@
>  #define SUN4I_XMIT_CNT(cnt)		((cnt) & SUN4I_MAX_XFER_SIZE)
>  
>  #define SUN4I_FIFO_STA_REG	0x28
> -#define SUN4I_FIFO_STA_RF_CNT_MASK	0x7f
>  #define SUN4I_FIFO_STA_RF_CNT_BITS	0
>  #define SUN4I_FIFO_STA_TF_CNT_MASK	0x7f
>  #define SUN4I_FIFO_STA_TF_CNT_BITS	16
> @@ -93,28 +92,55 @@
>  #define SUN4I_SPI_DEFAULT_RATE	1000000
>  #define SUN4I_SPI_TIMEOUT_US	1000000
>  
> -/* sun4i spi register set */
> -struct sun4i_spi_regs {
> -	u32 rxdata;
> -	u32 txdata;
> -	u32 ctl;
> -	u32 intctl;
> -	u32 st;
> -	u32 dmactl;
> -	u32 wait;
> -	u32 cctl;
> -	u32 bc;
> -	u32 tc;
> -	u32 fifo_sta;
> +#define SPI_REG(priv, reg)		((priv)->base_addr + \
> +					(priv)->variant->regs[reg])
> +#define SPI_BIT(priv, bit)		((priv)->variant->bits[bit])
> +#define SPI_CS(cs, priv)		(((cs) << SPI_BIT(priv, SPI_TCR_CS_SEL)) & \
> +					SPI_BIT(priv, SPI_TCR_CS_MASK))

Any chance you can swap the order of the parameters for SPI_CS? It's
quite error prone to have it different from the other two macros ....

> +
> +/* sun spi register set */
> +enum sun4i_spi_regs {
> +	SPI_GCR,
> +	SPI_TCR,
> +	SPI_FCR,
> +	SPI_FSR,
> +	SPI_CCR,
> +	SPI_BC,
> +	SPI_TC,
> +	SPI_BCTL,
> +	SPI_TXD,
> +	SPI_RXD,
> +};
> +
> +/* sun spi register bits */
> +enum sun4i_spi_bits {
> +	SPI_GCR_TP,
> +	SPI_TCR_CPHA,
> +	SPI_TCR_CPOL,
> +	SPI_TCR_CS_ACTIVE_LOW,
> +	SPI_TCR_CS_SEL,
> +	SPI_TCR_CS_MASK,
> +	SPI_TCR_XCH,
> +	SPI_TCR_CS_MANUAL,
> +	SPI_TCR_CS_LEVEL,
> +	SPI_FCR_TF_RST,
> +	SPI_FCR_RF_RST,
> +	SPI_FSR_RF_CNT_MASK,
> +};
> +
> +struct sun4i_spi_variant {
> +	const unsigned long *regs, *bits;
>  };
>  
>  struct sun4i_spi_platdata {
> +	struct sun4i_spi_variant *variant;
>  	u32 base_addr;
>  	u32 max_hz;
>  };
>  
>  struct sun4i_spi_priv {
> -	struct sun4i_spi_regs *regs;
> +	struct sun4i_spi_variant *variant;
> +	u32 base_addr;
>  	u32 freq;
>  	u32 mode;
>  
> @@ -129,7 +155,7 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len)
>  	u8 byte;
>  
>  	while (len--) {
> -		byte = readb(&priv->regs->rxdata);
> +		byte = readb(SPI_REG(priv, SPI_RXD));
>  		if (priv->rx_buf)
>  			*priv->rx_buf++ = byte;
>  	}
> @@ -141,7 +167,7 @@ static inline void sun4i_spi_fill_fifo(struct sun4i_spi_priv *priv, int len)
>  
>  	while (len--) {
>  		byte = priv->tx_buf ? *priv->tx_buf++ : 0;
> -		writeb(byte, &priv->regs->txdata);
> +		writeb(byte, SPI_REG(priv, SPI_TXD));
>  	}
>  }
>  
> @@ -150,17 +176,17 @@ static void sun4i_spi_set_cs(struct udevice *bus, u8 cs, bool enable)
>  	struct sun4i_spi_priv *priv = dev_get_priv(bus);
>  	u32 reg;
>  
> -	reg = readl(&priv->regs->ctl);
> +	reg = readl(SPI_REG(priv, SPI_TCR));
>  
> -	reg &= ~SUN4I_CTL_CS_MASK;
> -	reg |= SUN4I_CTL_CS(cs);
> +	reg &= ~SPI_BIT(priv, SPI_TCR_CS_MASK);
> +	reg |= SPI_CS(cs, priv);
>  
>  	if (enable)
> -		reg &= ~SUN4I_CTL_CS_LEVEL;
> +		reg &= ~SPI_BIT(priv, SPI_TCR_CS_LEVEL);
>  	else
> -		reg |= SUN4I_CTL_CS_LEVEL;
> +		reg |= SPI_BIT(priv, SPI_TCR_CS_LEVEL);
>  
> -	writel(reg, &priv->regs->ctl);
> +	writel(reg, SPI_REG(priv, SPI_TCR));
>  }
>  
>  static int sun4i_spi_parse_pins(struct udevice *dev)
> @@ -255,6 +281,7 @@ static int sun4i_spi_ofdata_to_platdata(struct udevice *bus)
>  	int node = dev_of_offset(bus);
>  
>  	plat->base_addr = devfdt_get_addr(bus);
> +	plat->variant = (struct sun4i_spi_variant *)dev_get_driver_data(bus);
>  	plat->max_hz = fdtdec_get_int(gd->fdt_blob, node,
>  				      "spi-max-frequency",
>  				      SUN4I_SPI_DEFAULT_RATE);
> @@ -273,7 +300,8 @@ static int sun4i_spi_probe(struct udevice *bus)
>  	sun4i_spi_enable_clock();
>  	sun4i_spi_parse_pins(bus);
>  
> -	priv->regs = (struct sun4i_spi_regs *)(uintptr_t)plat->base_addr;
> +	priv->variant = plat->variant;
> +	priv->base_addr = plat->base_addr;
>  	priv->freq = plat->max_hz;
>  
>  	return 0;
> @@ -283,9 +311,11 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
>  {
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
>  
> -	setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE |
> -		     SUN4I_CTL_MASTER | SUN4I_CTL_TP |
> -		     SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW);
> +	setbits_le32(SPI_REG(priv, SPI_GCR), SUN4I_CTL_ENABLE |
> +		     SUN4I_CTL_MASTER | SPI_BIT(priv, SPI_GCR_TP));
> +
> +	setbits_le32(SPI_REG(priv, SPI_TCR), SPI_BIT(priv, SPI_TCR_CS_MANUAL) |
> +		     SPI_BIT(priv, SPI_TCR_CS_ACTIVE_LOW));
>  
>  	return 0;
>  }
> @@ -294,7 +324,7 @@ static int sun4i_spi_release_bus(struct udevice *dev)
>  {
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
>  
> -	clrbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE);
> +	clrbits_le32(SPI_REG(priv, SPI_GCR), SUN4I_CTL_ENABLE);
>  
>  	return 0;
>  }
> @@ -323,25 +353,29 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  		sun4i_spi_set_cs(bus, slave_plat->cs, true);
>  
>  	/* Reset FIFOs */
> -	setbits_le32(&priv->regs->ctl, SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
> +	setbits_le32(SPI_REG(priv, SPI_FCR), SPI_BIT(priv, SPI_FCR_RF_RST) |
> +		     SPI_BIT(priv, SPI_FCR_TF_RST));
>  
>  	while (len) {
>  		/* Setup the transfer now... */
>  		nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1));
>  
>  		/* Setup the counters */
> -		writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc);
> -		writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc);
> +		writel(SUN4I_BURST_CNT(nbytes), SPI_REG(priv, SPI_BC));
> +		writel(SUN4I_XMIT_CNT(nbytes), SPI_REG(priv, SPI_TC));
>  
>  		/* Fill the TX FIFO */
>  		sun4i_spi_fill_fifo(priv, nbytes);
>  
>  		/* Start the transfer */
> -		setbits_le32(&priv->regs->ctl, SUN4I_CTL_XCH);
> +		setbits_le32(SPI_REG(priv, SPI_TCR),
> +			     SPI_BIT(priv, SPI_TCR_XCH));
>  
>  		/* Wait till RX FIFO to be empty */
> -		ret = readl_poll_timeout(&priv->regs->fifo_sta, rx_fifocnt,
> -					 (((rx_fifocnt & SUN4I_FIFO_STA_RF_CNT_MASK) >>
> +		ret = readl_poll_timeout(SPI_REG(priv, SPI_FSR),
> +					 rx_fifocnt,
> +					 (((rx_fifocnt &
> +					 SPI_BIT(priv, SPI_FSR_RF_CNT_MASK)) >>
>  					 SUN4I_FIFO_STA_RF_CNT_BITS) >= nbytes),
>  					 SUN4I_SPI_TIMEOUT_US);
>  		if (ret < 0) {
> @@ -390,7 +424,7 @@ static int sun4i_spi_set_speed(struct udevice *dev, uint speed)
>  	 */
>  
>  	div = SUN4I_SPI_MAX_RATE / (2 * speed);
> -	reg = readl(&priv->regs->cctl);
> +	reg = readl(SPI_REG(priv, SPI_CCR));
>  
>  	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
>  		if (div > 0)
> @@ -405,7 +439,7 @@ static int sun4i_spi_set_speed(struct udevice *dev, uint speed)
>  	}
>  
>  	priv->freq = speed;
> -	writel(reg, &priv->regs->cctl);
> +	writel(reg, SPI_REG(priv, SPI_CCR));
>  
>  	return 0;
>  }
> @@ -415,17 +449,17 @@ static int sun4i_spi_set_mode(struct udevice *dev, uint mode)
>  	struct sun4i_spi_priv *priv = dev_get_priv(dev);
>  	u32 reg;
>  
> -	reg = readl(&priv->regs->ctl);
> -	reg &= ~(SUN4I_CTL_CPOL | SUN4I_CTL_CPHA);
> +	reg = readl(SPI_REG(priv, SPI_TCR));
> +	reg &= ~(SPI_BIT(priv, SPI_TCR_CPOL) | SPI_BIT(priv, SPI_TCR_CPHA));
>  
>  	if (mode & SPI_CPOL)
> -		reg |= SUN4I_CTL_CPOL;
> +		reg |= SPI_BIT(priv, SPI_TCR_CPOL);
>  
>  	if (mode & SPI_CPHA)
> -		reg |= SUN4I_CTL_CPHA;
> +		reg |= SPI_BIT(priv, SPI_TCR_CPHA);
>  
>  	priv->mode = mode;
> -	writel(reg, &priv->regs->ctl);
> +	writel(reg, SPI_REG(priv, SPI_TCR));
>  
>  	return 0;
>  }
> @@ -438,8 +472,43 @@ static const struct dm_spi_ops sun4i_spi_ops = {
>  	.set_mode		= sun4i_spi_set_mode,
>  };
>  
> +static const unsigned long sun4i_spi_regs[] = {

Please, don't use long here, that's totally pointless. You can actually
use uint16_t, since it's just register offsets.

> +	[SPI_GCR]		= SUN4I_CTL_REG,
> +	[SPI_TCR]		= SUN4I_CTL_REG,
> +	[SPI_FCR]		= SUN4I_CTL_REG,
> +	[SPI_FSR]		= SUN4I_FIFO_STA_REG,
> +	[SPI_CCR]		= SUN4I_CLK_CTL_REG,
> +	[SPI_BC]		= SUN4I_BURST_CNT_REG,
> +	[SPI_TC]		= SUN4I_XMIT_CNT_REG,
> +	[SPI_TXD]		= SUN4I_TXDATA_REG,
> +	[SPI_RXD]		= SUN4I_RXDATA_REG,
> +};
> +
> +static const unsigned long sun4i_spi_bits[] = {

Same here, make it uint32_t, since it describes register masks in 32 bit
registers.

> +	[SPI_GCR_TP]		= BIT(18),
> +	[SPI_TCR_CPHA]		= BIT(2),
> +	[SPI_TCR_CPOL]		= BIT(3),
> +	[SPI_TCR_CS_ACTIVE_LOW] = BIT(4),
> +	[SPI_TCR_XCH]		= BIT(10),
> +	[SPI_TCR_CS_SEL]	= 12,
> +	[SPI_TCR_CS_MASK]	= 0x3000,
> +	[SPI_TCR_CS_MANUAL]	= BIT(16),
> +	[SPI_TCR_CS_LEVEL]	= BIT(17),
> +	[SPI_FCR_TF_RST]	= BIT(8),
> +	[SPI_FCR_RF_RST]	= BIT(9),
> +	[SPI_FSR_RF_CNT_MASK]	= GENMASK(6, 0),
> +};
> +
> +static const struct sun4i_spi_variant sun4i_a10_spi_variant = {
> +	.regs			= sun4i_spi_regs,
> +	.bits			= sun4i_spi_bits,
> +};
> +
>  static const struct udevice_id sun4i_spi_ids[] = {
> -	{ .compatible = "allwinner,sun4i-a10-spi"  },
> +	{
> +	  .compatible = "allwinner,sun4i-a10-spi",
> +	  .data = (ulong)&sun4i_a10_spi_variant,
> +	},
>  	{ }
>  };
>  
> 

I checked the rest as good as my brain allows me at 11pm, but it's still
quite a change with a lot of bits here and there :-(

Stefan, can you please test that this still works for you on the A20? If
I find some time I can try to hook up some SPI chip to my BananaPi, but
I guess it's easier for you to test.

Cheers,
Andre.
Stefan Mavrodiev Feb. 15, 2019, 6:42 a.m. UTC | #2
On 2/15/19 1:56 AM, André Przywara wrote:
> On 14/02/2019 08:36, Jagan Teki wrote:
>> Allwinner support two different SPI controllers one for A10 and
>> another for A31 with minimal changes in register offsets and
>> respective register bits, but the logic for accessing the SPI
>> master via SPI slave remains nearly similar.
>>
>> Add enum offsets for register set and register bits, so-that
>> it can access both classes of SPI controllers.
>>
>> Assign same control register for global, transfer and fifo control
>> registers to make the same code compatible with A31 SPI controller.
>>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>> ---
>> Note: SPI_REG still seems to have checkpatch warning.
> It's a CHECK, and it's fine, as long as you pass only "priv" in, at
> least. I think we can leave it this way.
>
>>   drivers/spi/sun4i_spi.c | 153 +++++++++++++++++++++++++++++-----------
>>   1 file changed, 111 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
>> index 0b1663038c..afc351c292 100644
>> --- a/drivers/spi/sun4i_spi.c
>> +++ b/drivers/spi/sun4i_spi.c
>> @@ -83,7 +83,6 @@
>>   #define SUN4I_XMIT_CNT(cnt)		((cnt) & SUN4I_MAX_XFER_SIZE)
>>   
>>   #define SUN4I_FIFO_STA_REG	0x28
>> -#define SUN4I_FIFO_STA_RF_CNT_MASK	0x7f
>>   #define SUN4I_FIFO_STA_RF_CNT_BITS	0
>>   #define SUN4I_FIFO_STA_TF_CNT_MASK	0x7f
>>   #define SUN4I_FIFO_STA_TF_CNT_BITS	16
>> @@ -93,28 +92,55 @@
>>   #define SUN4I_SPI_DEFAULT_RATE	1000000
>>   #define SUN4I_SPI_TIMEOUT_US	1000000
>>   
>> -/* sun4i spi register set */
>> -struct sun4i_spi_regs {
>> -	u32 rxdata;
>> -	u32 txdata;
>> -	u32 ctl;
>> -	u32 intctl;
>> -	u32 st;
>> -	u32 dmactl;
>> -	u32 wait;
>> -	u32 cctl;
>> -	u32 bc;
>> -	u32 tc;
>> -	u32 fifo_sta;
>> +#define SPI_REG(priv, reg)		((priv)->base_addr + \
>> +					(priv)->variant->regs[reg])
>> +#define SPI_BIT(priv, bit)		((priv)->variant->bits[bit])
>> +#define SPI_CS(cs, priv)		(((cs) << SPI_BIT(priv, SPI_TCR_CS_SEL)) & \
>> +					SPI_BIT(priv, SPI_TCR_CS_MASK))
> Any chance you can swap the order of the parameters for SPI_CS? It's
> quite error prone to have it different from the other two macros ....
>
>> +
>> +/* sun spi register set */
>> +enum sun4i_spi_regs {
>> +	SPI_GCR,
>> +	SPI_TCR,
>> +	SPI_FCR,
>> +	SPI_FSR,
>> +	SPI_CCR,
>> +	SPI_BC,
>> +	SPI_TC,
>> +	SPI_BCTL,
>> +	SPI_TXD,
>> +	SPI_RXD,
>> +};
>> +
>> +/* sun spi register bits */
>> +enum sun4i_spi_bits {
>> +	SPI_GCR_TP,
>> +	SPI_TCR_CPHA,
>> +	SPI_TCR_CPOL,
>> +	SPI_TCR_CS_ACTIVE_LOW,
>> +	SPI_TCR_CS_SEL,
>> +	SPI_TCR_CS_MASK,
>> +	SPI_TCR_XCH,
>> +	SPI_TCR_CS_MANUAL,
>> +	SPI_TCR_CS_LEVEL,
>> +	SPI_FCR_TF_RST,
>> +	SPI_FCR_RF_RST,
>> +	SPI_FSR_RF_CNT_MASK,
>> +};
>> +
>> +struct sun4i_spi_variant {
>> +	const unsigned long *regs, *bits;
>>   };
>>   
>>   struct sun4i_spi_platdata {
>> +	struct sun4i_spi_variant *variant;
>>   	u32 base_addr;
>>   	u32 max_hz;
>>   };
>>   
>>   struct sun4i_spi_priv {
>> -	struct sun4i_spi_regs *regs;
>> +	struct sun4i_spi_variant *variant;
>> +	u32 base_addr;
>>   	u32 freq;
>>   	u32 mode;
>>   
>> @@ -129,7 +155,7 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len)
>>   	u8 byte;
>>   
>>   	while (len--) {
>> -		byte = readb(&priv->regs->rxdata);
>> +		byte = readb(SPI_REG(priv, SPI_RXD));
>>   		if (priv->rx_buf)
>>   			*priv->rx_buf++ = byte;
>>   	}
>> @@ -141,7 +167,7 @@ static inline void sun4i_spi_fill_fifo(struct sun4i_spi_priv *priv, int len)
>>   
>>   	while (len--) {
>>   		byte = priv->tx_buf ? *priv->tx_buf++ : 0;
>> -		writeb(byte, &priv->regs->txdata);
>> +		writeb(byte, SPI_REG(priv, SPI_TXD));
>>   	}
>>   }
>>   
>> @@ -150,17 +176,17 @@ static void sun4i_spi_set_cs(struct udevice *bus, u8 cs, bool enable)
>>   	struct sun4i_spi_priv *priv = dev_get_priv(bus);
>>   	u32 reg;
>>   
>> -	reg = readl(&priv->regs->ctl);
>> +	reg = readl(SPI_REG(priv, SPI_TCR));
>>   
>> -	reg &= ~SUN4I_CTL_CS_MASK;
>> -	reg |= SUN4I_CTL_CS(cs);
>> +	reg &= ~SPI_BIT(priv, SPI_TCR_CS_MASK);
>> +	reg |= SPI_CS(cs, priv);
>>   
>>   	if (enable)
>> -		reg &= ~SUN4I_CTL_CS_LEVEL;
>> +		reg &= ~SPI_BIT(priv, SPI_TCR_CS_LEVEL);
>>   	else
>> -		reg |= SUN4I_CTL_CS_LEVEL;
>> +		reg |= SPI_BIT(priv, SPI_TCR_CS_LEVEL);
>>   
>> -	writel(reg, &priv->regs->ctl);
>> +	writel(reg, SPI_REG(priv, SPI_TCR));
>>   }
>>   
>>   static int sun4i_spi_parse_pins(struct udevice *dev)
>> @@ -255,6 +281,7 @@ static int sun4i_spi_ofdata_to_platdata(struct udevice *bus)
>>   	int node = dev_of_offset(bus);
>>   
>>   	plat->base_addr = devfdt_get_addr(bus);
>> +	plat->variant = (struct sun4i_spi_variant *)dev_get_driver_data(bus);
>>   	plat->max_hz = fdtdec_get_int(gd->fdt_blob, node,
>>   				      "spi-max-frequency",
>>   				      SUN4I_SPI_DEFAULT_RATE);
>> @@ -273,7 +300,8 @@ static int sun4i_spi_probe(struct udevice *bus)
>>   	sun4i_spi_enable_clock();
>>   	sun4i_spi_parse_pins(bus);
>>   
>> -	priv->regs = (struct sun4i_spi_regs *)(uintptr_t)plat->base_addr;
>> +	priv->variant = plat->variant;
>> +	priv->base_addr = plat->base_addr;
>>   	priv->freq = plat->max_hz;
>>   
>>   	return 0;
>> @@ -283,9 +311,11 @@ static int sun4i_spi_claim_bus(struct udevice *dev)
>>   {
>>   	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
>>   
>> -	setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE |
>> -		     SUN4I_CTL_MASTER | SUN4I_CTL_TP |
>> -		     SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW);
>> +	setbits_le32(SPI_REG(priv, SPI_GCR), SUN4I_CTL_ENABLE |
>> +		     SUN4I_CTL_MASTER | SPI_BIT(priv, SPI_GCR_TP));
>> +
>> +	setbits_le32(SPI_REG(priv, SPI_TCR), SPI_BIT(priv, SPI_TCR_CS_MANUAL) |
>> +		     SPI_BIT(priv, SPI_TCR_CS_ACTIVE_LOW));
>>   
>>   	return 0;
>>   }
>> @@ -294,7 +324,7 @@ static int sun4i_spi_release_bus(struct udevice *dev)
>>   {
>>   	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
>>   
>> -	clrbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE);
>> +	clrbits_le32(SPI_REG(priv, SPI_GCR), SUN4I_CTL_ENABLE);
>>   
>>   	return 0;
>>   }
>> @@ -323,25 +353,29 @@ static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>   		sun4i_spi_set_cs(bus, slave_plat->cs, true);
>>   
>>   	/* Reset FIFOs */
>> -	setbits_le32(&priv->regs->ctl, SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
>> +	setbits_le32(SPI_REG(priv, SPI_FCR), SPI_BIT(priv, SPI_FCR_RF_RST) |
>> +		     SPI_BIT(priv, SPI_FCR_TF_RST));
>>   
>>   	while (len) {
>>   		/* Setup the transfer now... */
>>   		nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1));
>>   
>>   		/* Setup the counters */
>> -		writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc);
>> -		writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc);
>> +		writel(SUN4I_BURST_CNT(nbytes), SPI_REG(priv, SPI_BC));
>> +		writel(SUN4I_XMIT_CNT(nbytes), SPI_REG(priv, SPI_TC));
>>   
>>   		/* Fill the TX FIFO */
>>   		sun4i_spi_fill_fifo(priv, nbytes);
>>   
>>   		/* Start the transfer */
>> -		setbits_le32(&priv->regs->ctl, SUN4I_CTL_XCH);
>> +		setbits_le32(SPI_REG(priv, SPI_TCR),
>> +			     SPI_BIT(priv, SPI_TCR_XCH));
>>   
>>   		/* Wait till RX FIFO to be empty */
>> -		ret = readl_poll_timeout(&priv->regs->fifo_sta, rx_fifocnt,
>> -					 (((rx_fifocnt & SUN4I_FIFO_STA_RF_CNT_MASK) >>
>> +		ret = readl_poll_timeout(SPI_REG(priv, SPI_FSR),
>> +					 rx_fifocnt,
>> +					 (((rx_fifocnt &
>> +					 SPI_BIT(priv, SPI_FSR_RF_CNT_MASK)) >>
>>   					 SUN4I_FIFO_STA_RF_CNT_BITS) >= nbytes),
>>   					 SUN4I_SPI_TIMEOUT_US);
>>   		if (ret < 0) {
>> @@ -390,7 +424,7 @@ static int sun4i_spi_set_speed(struct udevice *dev, uint speed)
>>   	 */
>>   
>>   	div = SUN4I_SPI_MAX_RATE / (2 * speed);
>> -	reg = readl(&priv->regs->cctl);
>> +	reg = readl(SPI_REG(priv, SPI_CCR));
>>   
>>   	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
>>   		if (div > 0)
>> @@ -405,7 +439,7 @@ static int sun4i_spi_set_speed(struct udevice *dev, uint speed)
>>   	}
>>   
>>   	priv->freq = speed;
>> -	writel(reg, &priv->regs->cctl);
>> +	writel(reg, SPI_REG(priv, SPI_CCR));
>>   
>>   	return 0;
>>   }
>> @@ -415,17 +449,17 @@ static int sun4i_spi_set_mode(struct udevice *dev, uint mode)
>>   	struct sun4i_spi_priv *priv = dev_get_priv(dev);
>>   	u32 reg;
>>   
>> -	reg = readl(&priv->regs->ctl);
>> -	reg &= ~(SUN4I_CTL_CPOL | SUN4I_CTL_CPHA);
>> +	reg = readl(SPI_REG(priv, SPI_TCR));
>> +	reg &= ~(SPI_BIT(priv, SPI_TCR_CPOL) | SPI_BIT(priv, SPI_TCR_CPHA));
>>   
>>   	if (mode & SPI_CPOL)
>> -		reg |= SUN4I_CTL_CPOL;
>> +		reg |= SPI_BIT(priv, SPI_TCR_CPOL);
>>   
>>   	if (mode & SPI_CPHA)
>> -		reg |= SUN4I_CTL_CPHA;
>> +		reg |= SPI_BIT(priv, SPI_TCR_CPHA);
>>   
>>   	priv->mode = mode;
>> -	writel(reg, &priv->regs->ctl);
>> +	writel(reg, SPI_REG(priv, SPI_TCR));
>>   
>>   	return 0;
>>   }
>> @@ -438,8 +472,43 @@ static const struct dm_spi_ops sun4i_spi_ops = {
>>   	.set_mode		= sun4i_spi_set_mode,
>>   };
>>   
>> +static const unsigned long sun4i_spi_regs[] = {
> Please, don't use long here, that's totally pointless. You can actually
> use uint16_t, since it's just register offsets.
>
>> +	[SPI_GCR]		= SUN4I_CTL_REG,
>> +	[SPI_TCR]		= SUN4I_CTL_REG,
>> +	[SPI_FCR]		= SUN4I_CTL_REG,
>> +	[SPI_FSR]		= SUN4I_FIFO_STA_REG,
>> +	[SPI_CCR]		= SUN4I_CLK_CTL_REG,
>> +	[SPI_BC]		= SUN4I_BURST_CNT_REG,
>> +	[SPI_TC]		= SUN4I_XMIT_CNT_REG,
>> +	[SPI_TXD]		= SUN4I_TXDATA_REG,
>> +	[SPI_RXD]		= SUN4I_RXDATA_REG,
>> +};
>> +
>> +static const unsigned long sun4i_spi_bits[] = {
> Same here, make it uint32_t, since it describes register masks in 32 bit
> registers.
>
>> +	[SPI_GCR_TP]		= BIT(18),
>> +	[SPI_TCR_CPHA]		= BIT(2),
>> +	[SPI_TCR_CPOL]		= BIT(3),
>> +	[SPI_TCR_CS_ACTIVE_LOW] = BIT(4),
>> +	[SPI_TCR_XCH]		= BIT(10),
>> +	[SPI_TCR_CS_SEL]	= 12,
>> +	[SPI_TCR_CS_MASK]	= 0x3000,
>> +	[SPI_TCR_CS_MANUAL]	= BIT(16),
>> +	[SPI_TCR_CS_LEVEL]	= BIT(17),
>> +	[SPI_FCR_TF_RST]	= BIT(8),
>> +	[SPI_FCR_RF_RST]	= BIT(9),
>> +	[SPI_FSR_RF_CNT_MASK]	= GENMASK(6, 0),
>> +};
>> +
>> +static const struct sun4i_spi_variant sun4i_a10_spi_variant = {
>> +	.regs			= sun4i_spi_regs,
>> +	.bits			= sun4i_spi_bits,
>> +};
>> +
>>   static const struct udevice_id sun4i_spi_ids[] = {
>> -	{ .compatible = "allwinner,sun4i-a10-spi"  },
>> +	{
>> +	  .compatible = "allwinner,sun4i-a10-spi",
>> +	  .data = (ulong)&sun4i_a10_spi_variant,
>> +	},
>>   	{ }
>>   };
>>   
>>
> I checked the rest as good as my brain allows me at 11pm, but it's still
> quite a change with a lot of bits here and there :-(
>
> Stefan, can you please test that this still works for you on the A20? If
> I find some time I can try to hook up some SPI chip to my BananaPi, but
> I guess it's easier for you to test.

Here are some test results:

=> sf probe 0:0
SF: Detected w25q128 with page size 256 Bytes, erase size 4 KiB, total 
16 MiB

=> sf test 0 100000
SPI flash test:
0 erase: 11363 ticks, 90 KiB/s 0.720 Mbps
1 check: 825 ticks, 1241 KiB/s 9.928 Mbps
2 write: 2472 ticks, 414 KiB/s 3.312 Mbps
3 read: 815 ticks, 1256 KiB/s 10.048 Mbps
Test passed
0 erase: 11363 ticks, 90 KiB/s 0.720 Mbps
1 check: 825 ticks, 1241 KiB/s 9.928 Mbps
2 write: 2472 ticks, 414 KiB/s 3.312 Mbps
3 read: 815 ticks, 1256 KiB/s 10.048 Mbps

The original tests can be seen here [1].

Apparently the patch works and it can be seen some
speed improvement.

The test is done on A20-SOM204 board. I can run it on other boards as
well, but since the routing is the same, I don't this there is a need.


Best regards,
Stefan Mavrodiev

[1] https://patchwork.ozlabs.org/patch/869763/

>
> Cheers,
> Andre.
Jagan Teki Feb. 18, 2019, 10:04 a.m. UTC | #3
On Fri, Feb 15, 2019 at 5:28 AM André Przywara <andre.przywara@arm.com> wrote:
>
> On 14/02/2019 08:36, Jagan Teki wrote:
> > Allwinner support two different SPI controllers one for A10 and
> > another for A31 with minimal changes in register offsets and
> > respective register bits, but the logic for accessing the SPI
> > master via SPI slave remains nearly similar.
> >
> > Add enum offsets for register set and register bits, so-that
> > it can access both classes of SPI controllers.
> >
> > Assign same control register for global, transfer and fifo control
> > registers to make the same code compatible with A31 SPI controller.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Note: SPI_REG still seems to have checkpatch warning.
>
> It's a CHECK, and it's fine, as long as you pass only "priv" in, at
> least. I think we can leave it this way.

I tried many way to clear this warning, but seems like it not possible
programmitacally using these macro assignement. Do you have any idea
how to fix this? one way to use direct function calls instead of macro
assigement but that would eventaully add another code.
Jagan Teki Feb. 18, 2019, 10:38 a.m. UTC | #4
Hi Stefan,

On Fri, Feb 15, 2019 at 12:12 PM Stefan Mavrodiev <stefan@olimex.com> wrote:
>
>

[snip]

> >> +static const unsigned long sun4i_spi_bits[] = {
> > Same here, make it uint32_t, since it describes register masks in 32 bit
> > registers.
> >
> >> +    [SPI_GCR_TP]            = BIT(18),
> >> +    [SPI_TCR_CPHA]          = BIT(2),
> >> +    [SPI_TCR_CPOL]          = BIT(3),
> >> +    [SPI_TCR_CS_ACTIVE_LOW] = BIT(4),
> >> +    [SPI_TCR_XCH]           = BIT(10),
> >> +    [SPI_TCR_CS_SEL]        = 12,
> >> +    [SPI_TCR_CS_MASK]       = 0x3000,
> >> +    [SPI_TCR_CS_MANUAL]     = BIT(16),
> >> +    [SPI_TCR_CS_LEVEL]      = BIT(17),
> >> +    [SPI_FCR_TF_RST]        = BIT(8),
> >> +    [SPI_FCR_RF_RST]        = BIT(9),
> >> +    [SPI_FSR_RF_CNT_MASK]   = GENMASK(6, 0),
> >> +};
> >> +
> >> +static const struct sun4i_spi_variant sun4i_a10_spi_variant = {
> >> +    .regs                   = sun4i_spi_regs,
> >> +    .bits                   = sun4i_spi_bits,
> >> +};
> >> +
> >>   static const struct udevice_id sun4i_spi_ids[] = {
> >> -    { .compatible = "allwinner,sun4i-a10-spi"  },
> >> +    {
> >> +      .compatible = "allwinner,sun4i-a10-spi",
> >> +      .data = (ulong)&sun4i_a10_spi_variant,
> >> +    },
> >>      { }
> >>   };
> >>
> >>
> > I checked the rest as good as my brain allows me at 11pm, but it's still
> > quite a change with a lot of bits here and there :-(
> >
> > Stefan, can you please test that this still works for you on the A20? If
> > I find some time I can try to hook up some SPI chip to my BananaPi, but
> > I guess it's easier for you to test.
>
> Here are some test results:
>
> => sf probe 0:0
> SF: Detected w25q128 with page size 256 Bytes, erase size 4 KiB, total
> 16 MiB
>
> => sf test 0 100000
> SPI flash test:
> 0 erase: 11363 ticks, 90 KiB/s 0.720 Mbps
> 1 check: 825 ticks, 1241 KiB/s 9.928 Mbps
> 2 write: 2472 ticks, 414 KiB/s 3.312 Mbps
> 3 read: 815 ticks, 1256 KiB/s 10.048 Mbps
> Test passed
> 0 erase: 11363 ticks, 90 KiB/s 0.720 Mbps
> 1 check: 825 ticks, 1241 KiB/s 9.928 Mbps
> 2 write: 2472 ticks, 414 KiB/s 3.312 Mbps
> 3 read: 815 ticks, 1256 KiB/s 10.048 Mbps
>
> The original tests can be seen here [1].
>
> Apparently the patch works and it can be seen some
> speed improvement.

Thanks for testing this.

Can I add your Tested-by credit?
Stefan Mavrodiev Feb. 18, 2019, 10:41 a.m. UTC | #5
On 2/18/19 12:38 PM, Jagan Teki wrote:
> Hi Stefan,
>
> On Fri, Feb 15, 2019 at 12:12 PM Stefan Mavrodiev <stefan@olimex.com> wrote:
>>
> [snip]
>
>>>> +static const unsigned long sun4i_spi_bits[] = {
>>> Same here, make it uint32_t, since it describes register masks in 32 bit
>>> registers.
>>>
>>>> +    [SPI_GCR_TP]            = BIT(18),
>>>> +    [SPI_TCR_CPHA]          = BIT(2),
>>>> +    [SPI_TCR_CPOL]          = BIT(3),
>>>> +    [SPI_TCR_CS_ACTIVE_LOW] = BIT(4),
>>>> +    [SPI_TCR_XCH]           = BIT(10),
>>>> +    [SPI_TCR_CS_SEL]        = 12,
>>>> +    [SPI_TCR_CS_MASK]       = 0x3000,
>>>> +    [SPI_TCR_CS_MANUAL]     = BIT(16),
>>>> +    [SPI_TCR_CS_LEVEL]      = BIT(17),
>>>> +    [SPI_FCR_TF_RST]        = BIT(8),
>>>> +    [SPI_FCR_RF_RST]        = BIT(9),
>>>> +    [SPI_FSR_RF_CNT_MASK]   = GENMASK(6, 0),
>>>> +};
>>>> +
>>>> +static const struct sun4i_spi_variant sun4i_a10_spi_variant = {
>>>> +    .regs                   = sun4i_spi_regs,
>>>> +    .bits                   = sun4i_spi_bits,
>>>> +};
>>>> +
>>>>    static const struct udevice_id sun4i_spi_ids[] = {
>>>> -    { .compatible = "allwinner,sun4i-a10-spi"  },
>>>> +    {
>>>> +      .compatible = "allwinner,sun4i-a10-spi",
>>>> +      .data = (ulong)&sun4i_a10_spi_variant,
>>>> +    },
>>>>       { }
>>>>    };
>>>>
>>>>
>>> I checked the rest as good as my brain allows me at 11pm, but it's still
>>> quite a change with a lot of bits here and there :-(
>>>
>>> Stefan, can you please test that this still works for you on the A20? If
>>> I find some time I can try to hook up some SPI chip to my BananaPi, but
>>> I guess it's easier for you to test.
>> Here are some test results:
>>
>> => sf probe 0:0
>> SF: Detected w25q128 with page size 256 Bytes, erase size 4 KiB, total
>> 16 MiB
>>
>> => sf test 0 100000
>> SPI flash test:
>> 0 erase: 11363 ticks, 90 KiB/s 0.720 Mbps
>> 1 check: 825 ticks, 1241 KiB/s 9.928 Mbps
>> 2 write: 2472 ticks, 414 KiB/s 3.312 Mbps
>> 3 read: 815 ticks, 1256 KiB/s 10.048 Mbps
>> Test passed
>> 0 erase: 11363 ticks, 90 KiB/s 0.720 Mbps
>> 1 check: 825 ticks, 1241 KiB/s 9.928 Mbps
>> 2 write: 2472 ticks, 414 KiB/s 3.312 Mbps
>> 3 read: 815 ticks, 1256 KiB/s 10.048 Mbps
>>
>> The original tests can be seen here [1].
>>
>> Apparently the patch works and it can be seen some
>> speed improvement.
> Thanks for testing this.
>
> Can I add your Tested-by credit?
Sure.
diff mbox series

Patch

diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
index 0b1663038c..afc351c292 100644
--- a/drivers/spi/sun4i_spi.c
+++ b/drivers/spi/sun4i_spi.c
@@ -83,7 +83,6 @@ 
 #define SUN4I_XMIT_CNT(cnt)		((cnt) & SUN4I_MAX_XFER_SIZE)
 
 #define SUN4I_FIFO_STA_REG	0x28
-#define SUN4I_FIFO_STA_RF_CNT_MASK	0x7f
 #define SUN4I_FIFO_STA_RF_CNT_BITS	0
 #define SUN4I_FIFO_STA_TF_CNT_MASK	0x7f
 #define SUN4I_FIFO_STA_TF_CNT_BITS	16
@@ -93,28 +92,55 @@ 
 #define SUN4I_SPI_DEFAULT_RATE	1000000
 #define SUN4I_SPI_TIMEOUT_US	1000000
 
-/* sun4i spi register set */
-struct sun4i_spi_regs {
-	u32 rxdata;
-	u32 txdata;
-	u32 ctl;
-	u32 intctl;
-	u32 st;
-	u32 dmactl;
-	u32 wait;
-	u32 cctl;
-	u32 bc;
-	u32 tc;
-	u32 fifo_sta;
+#define SPI_REG(priv, reg)		((priv)->base_addr + \
+					(priv)->variant->regs[reg])
+#define SPI_BIT(priv, bit)		((priv)->variant->bits[bit])
+#define SPI_CS(cs, priv)		(((cs) << SPI_BIT(priv, SPI_TCR_CS_SEL)) & \
+					SPI_BIT(priv, SPI_TCR_CS_MASK))
+
+/* sun spi register set */
+enum sun4i_spi_regs {
+	SPI_GCR,
+	SPI_TCR,
+	SPI_FCR,
+	SPI_FSR,
+	SPI_CCR,
+	SPI_BC,
+	SPI_TC,
+	SPI_BCTL,
+	SPI_TXD,
+	SPI_RXD,
+};
+
+/* sun spi register bits */
+enum sun4i_spi_bits {
+	SPI_GCR_TP,
+	SPI_TCR_CPHA,
+	SPI_TCR_CPOL,
+	SPI_TCR_CS_ACTIVE_LOW,
+	SPI_TCR_CS_SEL,
+	SPI_TCR_CS_MASK,
+	SPI_TCR_XCH,
+	SPI_TCR_CS_MANUAL,
+	SPI_TCR_CS_LEVEL,
+	SPI_FCR_TF_RST,
+	SPI_FCR_RF_RST,
+	SPI_FSR_RF_CNT_MASK,
+};
+
+struct sun4i_spi_variant {
+	const unsigned long *regs, *bits;
 };
 
 struct sun4i_spi_platdata {
+	struct sun4i_spi_variant *variant;
 	u32 base_addr;
 	u32 max_hz;
 };
 
 struct sun4i_spi_priv {
-	struct sun4i_spi_regs *regs;
+	struct sun4i_spi_variant *variant;
+	u32 base_addr;
 	u32 freq;
 	u32 mode;
 
@@ -129,7 +155,7 @@  static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len)
 	u8 byte;
 
 	while (len--) {
-		byte = readb(&priv->regs->rxdata);
+		byte = readb(SPI_REG(priv, SPI_RXD));
 		if (priv->rx_buf)
 			*priv->rx_buf++ = byte;
 	}
@@ -141,7 +167,7 @@  static inline void sun4i_spi_fill_fifo(struct sun4i_spi_priv *priv, int len)
 
 	while (len--) {
 		byte = priv->tx_buf ? *priv->tx_buf++ : 0;
-		writeb(byte, &priv->regs->txdata);
+		writeb(byte, SPI_REG(priv, SPI_TXD));
 	}
 }
 
@@ -150,17 +176,17 @@  static void sun4i_spi_set_cs(struct udevice *bus, u8 cs, bool enable)
 	struct sun4i_spi_priv *priv = dev_get_priv(bus);
 	u32 reg;
 
-	reg = readl(&priv->regs->ctl);
+	reg = readl(SPI_REG(priv, SPI_TCR));
 
-	reg &= ~SUN4I_CTL_CS_MASK;
-	reg |= SUN4I_CTL_CS(cs);
+	reg &= ~SPI_BIT(priv, SPI_TCR_CS_MASK);
+	reg |= SPI_CS(cs, priv);
 
 	if (enable)
-		reg &= ~SUN4I_CTL_CS_LEVEL;
+		reg &= ~SPI_BIT(priv, SPI_TCR_CS_LEVEL);
 	else
-		reg |= SUN4I_CTL_CS_LEVEL;
+		reg |= SPI_BIT(priv, SPI_TCR_CS_LEVEL);
 
-	writel(reg, &priv->regs->ctl);
+	writel(reg, SPI_REG(priv, SPI_TCR));
 }
 
 static int sun4i_spi_parse_pins(struct udevice *dev)
@@ -255,6 +281,7 @@  static int sun4i_spi_ofdata_to_platdata(struct udevice *bus)
 	int node = dev_of_offset(bus);
 
 	plat->base_addr = devfdt_get_addr(bus);
+	plat->variant = (struct sun4i_spi_variant *)dev_get_driver_data(bus);
 	plat->max_hz = fdtdec_get_int(gd->fdt_blob, node,
 				      "spi-max-frequency",
 				      SUN4I_SPI_DEFAULT_RATE);
@@ -273,7 +300,8 @@  static int sun4i_spi_probe(struct udevice *bus)
 	sun4i_spi_enable_clock();
 	sun4i_spi_parse_pins(bus);
 
-	priv->regs = (struct sun4i_spi_regs *)(uintptr_t)plat->base_addr;
+	priv->variant = plat->variant;
+	priv->base_addr = plat->base_addr;
 	priv->freq = plat->max_hz;
 
 	return 0;
@@ -283,9 +311,11 @@  static int sun4i_spi_claim_bus(struct udevice *dev)
 {
 	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
 
-	setbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE |
-		     SUN4I_CTL_MASTER | SUN4I_CTL_TP |
-		     SUN4I_CTL_CS_MANUAL | SUN4I_CTL_CS_ACTIVE_LOW);
+	setbits_le32(SPI_REG(priv, SPI_GCR), SUN4I_CTL_ENABLE |
+		     SUN4I_CTL_MASTER | SPI_BIT(priv, SPI_GCR_TP));
+
+	setbits_le32(SPI_REG(priv, SPI_TCR), SPI_BIT(priv, SPI_TCR_CS_MANUAL) |
+		     SPI_BIT(priv, SPI_TCR_CS_ACTIVE_LOW));
 
 	return 0;
 }
@@ -294,7 +324,7 @@  static int sun4i_spi_release_bus(struct udevice *dev)
 {
 	struct sun4i_spi_priv *priv = dev_get_priv(dev->parent);
 
-	clrbits_le32(&priv->regs->ctl, SUN4I_CTL_ENABLE);
+	clrbits_le32(SPI_REG(priv, SPI_GCR), SUN4I_CTL_ENABLE);
 
 	return 0;
 }
@@ -323,25 +353,29 @@  static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		sun4i_spi_set_cs(bus, slave_plat->cs, true);
 
 	/* Reset FIFOs */
-	setbits_le32(&priv->regs->ctl, SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST);
+	setbits_le32(SPI_REG(priv, SPI_FCR), SPI_BIT(priv, SPI_FCR_RF_RST) |
+		     SPI_BIT(priv, SPI_FCR_TF_RST));
 
 	while (len) {
 		/* Setup the transfer now... */
 		nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1));
 
 		/* Setup the counters */
-		writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc);
-		writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc);
+		writel(SUN4I_BURST_CNT(nbytes), SPI_REG(priv, SPI_BC));
+		writel(SUN4I_XMIT_CNT(nbytes), SPI_REG(priv, SPI_TC));
 
 		/* Fill the TX FIFO */
 		sun4i_spi_fill_fifo(priv, nbytes);
 
 		/* Start the transfer */
-		setbits_le32(&priv->regs->ctl, SUN4I_CTL_XCH);
+		setbits_le32(SPI_REG(priv, SPI_TCR),
+			     SPI_BIT(priv, SPI_TCR_XCH));
 
 		/* Wait till RX FIFO to be empty */
-		ret = readl_poll_timeout(&priv->regs->fifo_sta, rx_fifocnt,
-					 (((rx_fifocnt & SUN4I_FIFO_STA_RF_CNT_MASK) >>
+		ret = readl_poll_timeout(SPI_REG(priv, SPI_FSR),
+					 rx_fifocnt,
+					 (((rx_fifocnt &
+					 SPI_BIT(priv, SPI_FSR_RF_CNT_MASK)) >>
 					 SUN4I_FIFO_STA_RF_CNT_BITS) >= nbytes),
 					 SUN4I_SPI_TIMEOUT_US);
 		if (ret < 0) {
@@ -390,7 +424,7 @@  static int sun4i_spi_set_speed(struct udevice *dev, uint speed)
 	 */
 
 	div = SUN4I_SPI_MAX_RATE / (2 * speed);
-	reg = readl(&priv->regs->cctl);
+	reg = readl(SPI_REG(priv, SPI_CCR));
 
 	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
 		if (div > 0)
@@ -405,7 +439,7 @@  static int sun4i_spi_set_speed(struct udevice *dev, uint speed)
 	}
 
 	priv->freq = speed;
-	writel(reg, &priv->regs->cctl);
+	writel(reg, SPI_REG(priv, SPI_CCR));
 
 	return 0;
 }
@@ -415,17 +449,17 @@  static int sun4i_spi_set_mode(struct udevice *dev, uint mode)
 	struct sun4i_spi_priv *priv = dev_get_priv(dev);
 	u32 reg;
 
-	reg = readl(&priv->regs->ctl);
-	reg &= ~(SUN4I_CTL_CPOL | SUN4I_CTL_CPHA);
+	reg = readl(SPI_REG(priv, SPI_TCR));
+	reg &= ~(SPI_BIT(priv, SPI_TCR_CPOL) | SPI_BIT(priv, SPI_TCR_CPHA));
 
 	if (mode & SPI_CPOL)
-		reg |= SUN4I_CTL_CPOL;
+		reg |= SPI_BIT(priv, SPI_TCR_CPOL);
 
 	if (mode & SPI_CPHA)
-		reg |= SUN4I_CTL_CPHA;
+		reg |= SPI_BIT(priv, SPI_TCR_CPHA);
 
 	priv->mode = mode;
-	writel(reg, &priv->regs->ctl);
+	writel(reg, SPI_REG(priv, SPI_TCR));
 
 	return 0;
 }
@@ -438,8 +472,43 @@  static const struct dm_spi_ops sun4i_spi_ops = {
 	.set_mode		= sun4i_spi_set_mode,
 };
 
+static const unsigned long sun4i_spi_regs[] = {
+	[SPI_GCR]		= SUN4I_CTL_REG,
+	[SPI_TCR]		= SUN4I_CTL_REG,
+	[SPI_FCR]		= SUN4I_CTL_REG,
+	[SPI_FSR]		= SUN4I_FIFO_STA_REG,
+	[SPI_CCR]		= SUN4I_CLK_CTL_REG,
+	[SPI_BC]		= SUN4I_BURST_CNT_REG,
+	[SPI_TC]		= SUN4I_XMIT_CNT_REG,
+	[SPI_TXD]		= SUN4I_TXDATA_REG,
+	[SPI_RXD]		= SUN4I_RXDATA_REG,
+};
+
+static const unsigned long sun4i_spi_bits[] = {
+	[SPI_GCR_TP]		= BIT(18),
+	[SPI_TCR_CPHA]		= BIT(2),
+	[SPI_TCR_CPOL]		= BIT(3),
+	[SPI_TCR_CS_ACTIVE_LOW] = BIT(4),
+	[SPI_TCR_XCH]		= BIT(10),
+	[SPI_TCR_CS_SEL]	= 12,
+	[SPI_TCR_CS_MASK]	= 0x3000,
+	[SPI_TCR_CS_MANUAL]	= BIT(16),
+	[SPI_TCR_CS_LEVEL]	= BIT(17),
+	[SPI_FCR_TF_RST]	= BIT(8),
+	[SPI_FCR_RF_RST]	= BIT(9),
+	[SPI_FSR_RF_CNT_MASK]	= GENMASK(6, 0),
+};
+
+static const struct sun4i_spi_variant sun4i_a10_spi_variant = {
+	.regs			= sun4i_spi_regs,
+	.bits			= sun4i_spi_bits,
+};
+
 static const struct udevice_id sun4i_spi_ids[] = {
-	{ .compatible = "allwinner,sun4i-a10-spi"  },
+	{
+	  .compatible = "allwinner,sun4i-a10-spi",
+	  .data = (ulong)&sun4i_a10_spi_variant,
+	},
 	{ }
 };