[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