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

Stefan Mavrodiev stefan at olimex.com
Fri Feb 15 06:42:05 UTC 2019


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 at 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.


More information about the U-Boot mailing list