[U-Boot] [PATCH v3 1/6] spi: Add SiFive SPI driver

Anup Patel Anup.Patel at wdc.com
Mon Jul 15 11:54:20 UTC 2019



> -----Original Message-----
> From: Jagan Teki <jagan at amarulasolutions.com>
> Sent: Monday, July 15, 2019 2:02 PM
> To: Anup Patel <Anup.Patel at wdc.com>
> Cc: Rick Chen <rick at andestech.com>; Bin Meng <bmeng.cn at gmail.com>;
> Lukas Auer <lukas.auer at aisec.fraunhofer.de>; Peng Fan
> <peng.fan at nxp.com>; Oleksandr Zhadan and Michael Durrant
> <arcsupport at arcturusnetworks.com>; Palmer Dabbelt
> <palmer at sifive.com>; Paul Walmsley <paul.walmsley at sifive.com>; Atish
> Patra <Atish.Patra at wdc.com>; Anup Patel <anup at brainfault.org>; Bhargav
> Shah <bhargavshah1988 at gmail.com>; U-Boot Mailing List <u-
> boot at lists.denx.de>
> Subject: Re: [PATCH v3 1/6] spi: Add SiFive SPI driver
> 
> On Mon, Jul 8, 2019 at 9:40 AM Anup Patel <Anup.Patel at wdc.com> wrote:
> >
> > From: Bhargav Shah <bhargavshah1988 at gmail.com>
> >
> > This patch adds SiFive SPI driver. The driver is 100% DM driver and it
> > determines input clock using clk framework.
> >
> > The SiFive SPI block is found on SiFive FU540 SOC and is used to
> > access flash and MMC devices on SiFive Unleashed board.
> >
> > This driver implementation is inspired from the Linux SiFive SPI
> > driver available in Linux-5.2 or higher and SiFive FSBL sources.
> >
> > Signed-off-by: Bhargav Shah <bhargavshah1988 at gmail.com>
> > Signed-off-by: Anup Patel <anup.patel at wdc.com>
> > Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> > Tested-by: Bin Meng <bmeng.cn at gmail.com>
> > ---
> >  drivers/spi/Kconfig      |   8 +
> >  drivers/spi/Makefile     |   1 +
> >  drivers/spi/spi-sifive.c | 403
> > +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 412 insertions(+)
> >  create mode 100644 drivers/spi/spi-sifive.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
> > eb32f082fe..2712bad310 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -224,6 +224,14 @@ config SANDBOX_SPI
> >                 };
> >           };
> >
> > +config SIFIVE_SPI
> > +       bool "SiFive SPI driver"
> > +       help
> > +         This driver supports the SiFive SPI IP. If unsure say N.
> > +         Enable the SiFive SPI controller driver.
> > +
> > +         The SiFive SPI controller driver is found on various SiFive SoCs.
> > +
> >  config SPI_SUNXI
> >         bool "Allwinner SoC SPI controllers"
> >         help
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
> > 8be9a4baa2..09a9d3697e 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -49,6 +49,7 @@ obj-$(CONFIG_PL022_SPI) += pl022_spi.o
> >  obj-$(CONFIG_RENESAS_RPC_SPI) += renesas_rpc_spi.o
> >  obj-$(CONFIG_ROCKCHIP_SPI) += rk_spi.o
> >  obj-$(CONFIG_SANDBOX_SPI) += sandbox_spi.o
> > +obj-$(CONFIG_SIFIVE_SPI) += spi-sifive.o
> >  obj-$(CONFIG_SPI_SUNXI) += spi-sunxi.o
> >  obj-$(CONFIG_SH_SPI) += sh_spi.o
> >  obj-$(CONFIG_SH_QSPI) += sh_qspi.o
> > diff --git a/drivers/spi/spi-sifive.c b/drivers/spi/spi-sifive.c new
> > file mode 100644 index 0000000000..1186f6e8f5
> > --- /dev/null
> > +++ b/drivers/spi/spi-sifive.c
> > @@ -0,0 +1,403 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018 SiFive, Inc.
> > + * Copyright 2019 Bhargav Shah <bhargavshah1988 at gmail.com>
> > + *
> > + * SiFive SPI controller driver (master mode only)  */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <malloc.h>
> > +#include <spi.h>
> > +#include <asm/io.h>
> > +#include <linux/log2.h>
> > +#include <clk.h>
> > +
> > +#define SIFIVE_SPI_MAX_CS              32
> > +
> > +#define SIFIVE_SPI_DEFAULT_DEPTH       8
> > +#define SIFIVE_SPI_DEFAULT_BITS                8
> > +
> > +/* register offsets */
> > +#define SIFIVE_SPI_REG_SCKDIV            0x00 /* Serial clock divisor */
> > +#define SIFIVE_SPI_REG_SCKMODE           0x04 /* Serial clock mode */
> > +#define SIFIVE_SPI_REG_CSID              0x10 /* Chip select ID */
> > +#define SIFIVE_SPI_REG_CSDEF             0x14 /* Chip select default */
> > +#define SIFIVE_SPI_REG_CSMODE            0x18 /* Chip select mode */
> > +#define SIFIVE_SPI_REG_DELAY0            0x28 /* Delay control 0 */
> > +#define SIFIVE_SPI_REG_DELAY1            0x2c /* Delay control 1 */
> > +#define SIFIVE_SPI_REG_FMT               0x40 /* Frame format */
> > +#define SIFIVE_SPI_REG_TXDATA            0x48 /* Tx FIFO data */
> > +#define SIFIVE_SPI_REG_RXDATA            0x4c /* Rx FIFO data */
> > +#define SIFIVE_SPI_REG_TXMARK            0x50 /* Tx FIFO watermark */
> > +#define SIFIVE_SPI_REG_RXMARK            0x54 /* Rx FIFO watermark */
> > +#define SIFIVE_SPI_REG_FCTRL             0x60 /* SPI flash interface control
> */
> > +#define SIFIVE_SPI_REG_FFMT              0x64 /* SPI flash instruction format
> */
> > +#define SIFIVE_SPI_REG_IE                0x70 /* Interrupt Enable Register */
> > +#define SIFIVE_SPI_REG_IP                0x74 /* Interrupt Pendings Register */
> > +
> > +/* sckdiv bits */
> > +#define SIFIVE_SPI_SCKDIV_DIV_MASK       0xfffU
> > +
> > +/* sckmode bits */
> > +#define SIFIVE_SPI_SCKMODE_PHA           BIT(0)
> > +#define SIFIVE_SPI_SCKMODE_POL           BIT(1)
> > +#define SIFIVE_SPI_SCKMODE_MODE_MASK
> (SIFIVE_SPI_SCKMODE_PHA | \
> > +                                         SIFIVE_SPI_SCKMODE_POL)
> > +
> > +/* csmode bits */
> > +#define SIFIVE_SPI_CSMODE_MODE_AUTO      0U
> > +#define SIFIVE_SPI_CSMODE_MODE_HOLD      2U
> > +#define SIFIVE_SPI_CSMODE_MODE_OFF       3U
> > +
> > +/* delay0 bits */
> > +#define SIFIVE_SPI_DELAY0_CSSCK(x)       ((u32)(x))
> > +#define SIFIVE_SPI_DELAY0_CSSCK_MASK     0xffU
> > +#define SIFIVE_SPI_DELAY0_SCKCS(x)       ((u32)(x) << 16)
> > +#define SIFIVE_SPI_DELAY0_SCKCS_MASK     (0xffU << 16)
> > +
> > +/* delay1 bits */
> > +#define SIFIVE_SPI_DELAY1_INTERCS(x)     ((u32)(x))
> > +#define SIFIVE_SPI_DELAY1_INTERCS_MASK   0xffU
> > +#define SIFIVE_SPI_DELAY1_INTERXFR(x)    ((u32)(x) << 16)
> > +#define SIFIVE_SPI_DELAY1_INTERXFR_MASK  (0xffU << 16)
> > +
> > +/* fmt bits */
> > +#define SIFIVE_SPI_FMT_PROTO_SINGLE      0U
> > +#define SIFIVE_SPI_FMT_PROTO_DUAL        1U
> > +#define SIFIVE_SPI_FMT_PROTO_QUAD        2U
> > +#define SIFIVE_SPI_FMT_PROTO_MASK        3U
> > +#define SIFIVE_SPI_FMT_ENDIAN            BIT(2)
> > +#define SIFIVE_SPI_FMT_DIR               BIT(3)
> > +#define SIFIVE_SPI_FMT_LEN(x)            ((u32)(x) << 16)
> > +#define SIFIVE_SPI_FMT_LEN_MASK          (0xfU << 16)
> > +
> > +/* txdata bits */
> > +#define SIFIVE_SPI_TXDATA_DATA_MASK      0xffU
> > +#define SIFIVE_SPI_TXDATA_FULL           BIT(31)
> > +
> > +/* rxdata bits */
> > +#define SIFIVE_SPI_RXDATA_DATA_MASK      0xffU
> > +#define SIFIVE_SPI_RXDATA_EMPTY          BIT(31)
> > +
> > +/* ie and ip bits */
> > +#define SIFIVE_SPI_IP_TXWM               BIT(0)
> > +#define SIFIVE_SPI_IP_RXWM               BIT(1)
> > +
> > +struct sifive_spi {
> > +       void            *regs;          /* base address of the registers */
> > +       u32             fifo_depth;
> > +       u32             bits_per_word;
> > +       u32             cs_inactive;    /* Level of the CS pins when inactive*/
> > +       u32             freq;
> > +       u32             num_cs;
> > +};
> > +
> > +static void sifive_spi_write(struct sifive_spi *spi, int offset, u32
> > +value) {
> > +       writel(value, spi->regs + offset); }
> > +
> > +static u32 sifive_spi_read(struct sifive_spi *spi, int offset) {
> > +       return readl(spi->regs + offset); }
> 
> Look like these are direct IO calls, so better call them directly.

Sure, we were using this for debugging purpose. I will replace this
with direct IO calls.

> 
> > +
> > +static void sifive_spi_prep_device(struct sifive_spi *spi,
> > +                                  struct dm_spi_slave_platdata
> > +*slave) {
> > +       /* Update the chip select polarity */
> > +       if (slave->mode & SPI_CS_HIGH)
> > +               spi->cs_inactive &= ~BIT(slave->cs);
> > +       else
> > +               spi->cs_inactive |= BIT(slave->cs);
> > +       sifive_spi_write(spi, SIFIVE_SPI_REG_CSDEF, spi->cs_inactive);
> > +
> > +       /* Select the correct device */
> > +       sifive_spi_write(spi, SIFIVE_SPI_REG_CSID, slave->cs); }
> > +
> > +static int sifive_spi_set_cs(struct sifive_spi *spi,
> > +                            struct dm_spi_slave_platdata *slave) {
> > +       u32 cs_mode = SIFIVE_SPI_CSMODE_MODE_HOLD;
> > +
> > +       if (slave->cs > spi->num_cs)
> > +               return -EINVAL;
> > +
> > +       if (slave->mode & SPI_CS_HIGH)
> > +               cs_mode = SIFIVE_SPI_CSMODE_MODE_AUTO;
> > +
> > +       sifive_spi_write(spi, SIFIVE_SPI_REG_CSMODE, cs_mode);
> > +
> > +       return 0;
> > +}
> > +
> > +static void sifive_spi_clear_cs(struct sifive_spi *spi) {
> > +       sifive_spi_write(spi, SIFIVE_SPI_REG_CSMODE,
> > +                        SIFIVE_SPI_CSMODE_MODE_AUTO); }
> 
> This seems to be different CS and bus handling compared to existing spi
> drivers.
> 
> 00: for claim or release - we enable and disable the bus
> 01: we have SPI_XFER_BEGIN flag ie where we activate the CS
> 02: we have SPI_XFER_END flag ie where we deactivae the CS
> 
> Look like the above code is managing all above points in calim and release
> func calls, which is not a better approach interms of data transfer b/w slave
> to bus. example the spi-nor do claim the bus before flash operation but will
> call spi_xfer with device activate and deactive flags (like BEGIN and END). so
> managing cs on spi_xfer can make the data tranfer feasible, atleast from u-
> boot spi-nor or spi-uclass point-of-view.

The problem is we have MMC connected over SPI bus.

In MMC SPI protocol, we send MMC CMD over SPI and keep polling the
expected byte pattern so setting SPI_XFER_BEGIN and SPI_XFER_END
in MMC_SPI slave driver becomes tricky. Right now MMC_SPI driver
is always passing zero flags.

The driver in it's current shape works fine with MMC_SPI driver.

We have to eventually get this driver to work with SPI NOR driver
as well.

Do you mind if I add separate patches to use SPI_XFER_ flags in 
SiFive SPI driver and MMC SPI driver?

> 
> > +
> > +static void sifive_spi_prep_transfer(struct sifive_spi *spi,
> > +                                    bool is_rx_xfer,
> > +                                    struct dm_spi_slave_platdata
> > +*slave) {
> > +       u32 cr;
> > +
> > +       /* Modify the SPI protocol mode */
> > +       cr = sifive_spi_read(spi, SIFIVE_SPI_REG_FMT);
> > +
> > +       /* Bits per word ? */
> > +       cr &= ~SIFIVE_SPI_FMT_LEN_MASK;
> > +       cr |= SIFIVE_SPI_FMT_LEN(spi->bits_per_word);
> > +
> > +       /* LSB first? */
> > +       cr &= ~SIFIVE_SPI_FMT_ENDIAN;
> > +       if (slave->mode & SPI_LSB_FIRST)
> > +               cr |= SIFIVE_SPI_FMT_ENDIAN;
> > +
> > +       /* Number of wires ? */
> > +       cr &= ~SIFIVE_SPI_FMT_PROTO_MASK;
> > +       if ((slave->mode & SPI_TX_QUAD) || (slave->mode & SPI_RX_QUAD))
> > +               cr |= SIFIVE_SPI_FMT_PROTO_QUAD;
> > +       else if ((slave->mode & SPI_TX_DUAL) || (slave->mode &
> SPI_RX_DUAL))
> > +               cr |= SIFIVE_SPI_FMT_PROTO_DUAL;
> > +       else
> > +               cr |= SIFIVE_SPI_FMT_PROTO_SINGLE;
> > +
> > +       /* SPI direction in/out ? */
> > +       cr &= ~SIFIVE_SPI_FMT_DIR;
> > +       if (!is_rx_xfer)
> > +               cr |= SIFIVE_SPI_FMT_DIR;
> > +
> > +       sifive_spi_write(spi, SIFIVE_SPI_REG_FMT, cr); }
> > +
> > +static void sifive_spi_rx(struct sifive_spi *spi, u8 *rx_ptr) {
> > +       u32 data;
> > +
> > +       do {
> > +               data = sifive_spi_read(spi, SIFIVE_SPI_REG_RXDATA);
> > +       } while (data & SIFIVE_SPI_RXDATA_EMPTY);
> 
> can be handle via wait_for_bit calls?

Can't use wait_for_bit_xyz() here because RXDATA is a FIFO.

> 
> > +
> > +       if (rx_ptr)
> > +               *rx_ptr = data & SIFIVE_SPI_RXDATA_DATA_MASK; }
> > +
> > +static void sifive_spi_tx(struct sifive_spi *spi, const u8 *tx_ptr) {
> > +       u32 data;
> > +       u8 tx_data = (tx_ptr) ? *tx_ptr & SIFIVE_SPI_TXDATA_DATA_MASK :
> > +                               SIFIVE_SPI_TXDATA_DATA_MASK;
> > +
> > +       do {
> > +               data = sifive_spi_read(spi, SIFIVE_SPI_REG_TXDATA);
> > +       } while (data & SIFIVE_SPI_TXDATA_FULL);
> 
> can be handle via wait_for_bit calls?

Can't use wait_for_bit_xyz() here because TXDATA is a FIFO.

> 
> > +
> > +       sifive_spi_write(spi, SIFIVE_SPI_REG_TXDATA, tx_data); }
> > +
> > +static u8 sifive_spi_txrx(struct sifive_spi *spi, const u8 *tx_ptr) {
> > +       u8 rx = 0;
> > +
> > +       sifive_spi_tx(spi, tx_ptr);
> > +       sifive_spi_rx(spi, &rx);
> > +
> > +       return rx;
> > +}
> > +
> > +static int sifive_spi_claim_bus(struct udevice *dev) {
> > +       int ret;
> > +       struct udevice *bus = dev->parent;
> > +       struct sifive_spi *spi = dev_get_priv(bus);
> > +       struct dm_spi_slave_platdata *slave =
> > +dev_get_parent_platdata(dev);
> > +
> > +       sifive_spi_prep_device(spi, slave);
> > +
> > +       ret = sifive_spi_set_cs(spi, slave);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> > +static int sifive_spi_release_bus(struct udevice *dev) {
> > +       struct sifive_spi *spi = dev_get_priv(dev->parent);
> > +
> > +       sifive_spi_clear_cs(spi);
> > +
> > +       return 0;
> > +}
> > +
> > +static int sifive_spi_xfer(struct udevice *dev, unsigned int bitlen,
> > +                          const void *dout, void *din, unsigned long
> > +flags) {
> > +       struct udevice *bus = dev->parent;
> > +       struct sifive_spi *spi = dev_get_priv(bus);
> > +       struct dm_spi_slave_platdata *slave =
> dev_get_parent_platdata(dev);
> > +       const unsigned char *tx_ptr = dout;
> > +       unsigned char *rx_ptr = din;
> > +       u32 remaining_len;
> > +
> > +       sifive_spi_prep_transfer(spi, true, slave);
> > +
> > +       remaining_len = bitlen / 8;
> > +
> > +       while (remaining_len) {
> > +               int n_words, tx_words, rx_words;
> > +
> > +               n_words = min(remaining_len, spi->fifo_depth);
> > +
> > +               /* Enqueue n_words for transmission */
> > +               if (tx_ptr) {
> > +                       for (tx_words = 0; tx_words < n_words; ++tx_words) {
> > +                               sifive_spi_txrx(spi, tx_ptr);
> > +                               tx_ptr++;
> > +                       }
> > +               }
> > +
> > +               /* Read out all the data from the RX FIFO */
> > +               if (rx_ptr) {
> > +                       for (rx_words = 0; rx_words < n_words; ++rx_words) {
> > +                               *rx_ptr = sifive_spi_txrx(spi, NULL);
> > +                               rx_ptr++;
> > +                       }
> > +               }
> > +
> 
> I don't see much code on sifive_spi_txrx except register read/writes better
> handle all in this function itself.

Sure , I will remove sifive_spi_txrx().

> 
> 
> > +               remaining_len -= n_words;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int sifive_spi_set_speed(struct udevice *bus, uint speed) {
> > +       struct sifive_spi *spi = dev_get_priv(bus);
> > +       u32 scale;
> > +
> > +       if (speed > spi->freq)
> > +               speed = spi->freq;
> > +
> > +       /* Cofigure max speed */
> > +       scale = (DIV_ROUND_UP(spi->freq >> 1, speed) - 1)
> > +                                       & SIFIVE_SPI_SCKDIV_DIV_MASK;
> > +       sifive_spi_write(spi, SIFIVE_SPI_REG_SCKDIV, scale);
> 
> space here.

Okay.

> 
> > +       return 0;
> > +}
> > +
> > +static int sifive_spi_set_mode(struct udevice *bus, uint mode) {
> > +       struct sifive_spi *spi = dev_get_priv(bus);
> > +       u32 cr;
> > +
> > +       /* Switch clock mode bits */
> > +       cr = sifive_spi_read(spi, SIFIVE_SPI_REG_SCKMODE) &
> > +                                       ~SIFIVE_SPI_SCKMODE_MODE_MASK;
> > +       if (mode & SPI_CPHA)
> > +               cr |= SIFIVE_SPI_SCKMODE_PHA;
> > +       if (mode & SPI_CPOL)
> > +               cr |= SIFIVE_SPI_SCKMODE_POL;
> > +
> > +       sifive_spi_write(spi, SIFIVE_SPI_REG_SCKMODE, cr);
> > +
> > +       return 0;
> > +}
> > +
> > +static int sifive_spi_cs_info(struct udevice *bus, uint cs,
> > +                         struct spi_cs_info *info) {
> > +       return 0;
> > +}
> 
> Usually it has check for supported bus and cs number, if unsure better
> remove this.

Sure, I will remove it.

Regards,
Anup


More information about the U-Boot mailing list