[U-Boot] [PATCH 03/10] dm: spi: add BCM63xx SPI driver

["Álvaro Fernández Rojas" <noltari@gmail.com>]

Hi Jagan,

El 07/06/2017 a las 19:29, Jagan Teki escribió:
> On Wed, Jun 7, 2017 at 9:05 PM, Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
>> Hi Jagan,
>>
>> El 7/6/17 a las 9:35, Jagan Teki escribió:
>>> On Fri, May 19, 2017 at 12:59 AM, Álvaro Fernández Rojas
>>> <noltari@gmail.com> wrote:
>>>> This driver is a simplified version of linux/drivers/spi/spi-bcm63xx.c
>>>> Instead of supporting both HW revisions of the controller in a single build,
>>>> support has been split by the selected config to save space.
>>>>
>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>>> ---
>>>>  drivers/spi/Kconfig       |  23 +++
>>>>  drivers/spi/Makefile      |   1 +
>>>>  drivers/spi/bcm63xx_spi.c | 404 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 428 insertions(+)
>>>>  create mode 100644 drivers/spi/bcm63xx_spi.c
>>>>
>>>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>>>> index a00d401..d458dd7 100644
>>>> --- a/drivers/spi/Kconfig
>>>> +++ b/drivers/spi/Kconfig
>>>> @@ -43,6 +43,29 @@ config ATMEL_SPI
>>>>           many AT32 (AVR32) and AT91 (ARM) chips. This driver can be
>>>>           used to access the SPI Flash, such as AT25DF321.
>>>>
>>>> +choice
>>>> +       prompt "BCM63xx SPI driver"
>>>> +       depends on ARCH_BMIPS
>>>> +       optional
>>>> +
>>>> +config BCM6338_SPI
>>>> +       bool "BCM6338 SPI driver"
>>>> +       select SPI_MAX_WRITE_CMD_BYTES
>>>> +       help
>>>> +         Enable the BCM6338 SPI driver. This driver can be used to
>>>> +         access the SPI NOR flash on platforms embedding this Broadcom
>>>> +         SPI core.
>>>> +
>>>> +config BCM6358_SPI
>>>> +       bool "BCM6358 SPI driver"
>>>> +       select SPI_MAX_WRITE_CMD_BYTES
>>>> +       help
>>>> +         Enable the BCM6358 SPI driver. This driver can be used to
>>>> +         access the SPI NOR flash on platforms embedding this Broadcom
>>>> +         SPI core.
>>>> +
>>>> +endchoice
>>>> +
>>>>  config CADENCE_QSPI
>>>>         bool "Cadence QSPI driver"
>>>>         help
>>>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>>>> index c090562..c9ba648 100644
>>>> --- a/drivers/spi/Makefile
>>>> +++ b/drivers/spi/Makefile
>>>> @@ -19,6 +19,7 @@ obj-$(CONFIG_ALTERA_SPI) += altera_spi.o
>>>>  obj-$(CONFIG_ATH79_SPI) += ath79_spi.o
>>>>  obj-$(CONFIG_ATMEL_DATAFLASH_SPI) += atmel_dataflash_spi.o
>>>>  obj-$(CONFIG_ATMEL_SPI) += atmel_spi.o
>>>> +obj-$(CONFIG_BCM6338_SPI)$(CONFIG_BCM6358_SPI) += bcm63xx_spi.o
>>>>  obj-$(CONFIG_CADENCE_QSPI) += cadence_qspi.o cadence_qspi_apb.o
>>>>  obj-$(CONFIG_CF_SPI) += cf_spi.o
>>>>  obj-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
>>>> diff --git a/drivers/spi/bcm63xx_spi.c b/drivers/spi/bcm63xx_spi.c
>>>> new file mode 100644
>>>> index 0000000..6e64607
>>>> --- /dev/null
>>>> +++ b/drivers/spi/bcm63xx_spi.c
>>>> @@ -0,0 +1,404 @@
>>>> +/*
>>>> + * Copyright (C) 2017 Álvaro Fernández Rojas <noltari@gmail.com>
>>>> + *
>>>> + * Derived from linux/drivers/spi/spi-bcm63xx.c:
>>>> + *     Copyright (C) 2009-2012 Florian Fainelli <florian@openwrt.org>
>>>> + *     Copyright (C) 2010 Tanguy Bouzeloc <tanguy.bouzeloc@efixo.com>
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0+
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <clk.h>
>>>> +#include <dm.h>
>>>> +#include <spi.h>
>>>> +#include <reset.h>
>>>> +#include <asm/io.h>
>>>> +
>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>> +
>>>> +#if defined(CONFIG_BCM6338_SPI)
>>>> +
>>>> +# define SPI_DT_ID             "brcm,bcm6338-spi"
>>>> +
>>>> +/* SPI Command register */
>>>> +# define SPI_CMD_REG           0x00
>>>> +
>>>> +/* SPI Interrupt registers */
>>>> +# define SPI_IR_STAT_REG       0x02
>>>> +# define SPI_IR_MASK_REG       0x04
>>>> +
>>>> +/* SPI Clock register */
>>>> +# define SPI_CLK_REG           0x06
>>>> +
>>>> +/* SPI Fill register */
>>>> +# define SPI_FILL_REG          0x07
>>>> +
>>>> +/* SPI Control register (8 bit) */
>>>> +# define SPI_CTL_REG           0x40
>>>> +# define SPI_CTL_BYTES_SHIFT   0
>>>> +# define SPI_CTL_BYTES_MASK    (0x3f << SPI_CTL_BYTES_SHIFT)
>>>> +# define SPI_CTL_TYPE_SHIFT    6
>>>> +# define SPI_CTL_TYPE_FD_RW    (0 << SPI_CTL_TYPE_SHIFT)
>>>> +# define SPI_CTL_TYPE_HD_W     (1 << SPI_CTL_TYPE_SHIFT)
>>>> +# define SPI_CTL_TYPE_HD_R     (2 << SPI_CTL_TYPE_SHIFT)
>>>> +
>>>> +# define bcm63xx_spi_wctl(v,a) writeb_be(v, a);
>>>> +
>>>> +/* SPI TX Data registers */
>>>> +# define SPI_TX_DATA_REG       0x41
>>>> +# define SPI_TX_DATA_SIZE      0x3f
>>>> +
>>>> +/* SPI RX Data registers */
>>>> +# define SPI_RX_DATA_REG       0x80
>>>> +# define SPI_RX_DATA_SIZE      0x3f
>>>> +
>>>> +#elif defined(CONFIG_BCM6358_SPI)
>>>> +
>>>> +# define SPI_DT_ID             "brcm,bcm6358-spi"
>>>> +
>>>> +/* SPI Control register (16 bit) */
>>>> +# define SPI_CTL_REG           0x000
>>>> +# define SPI_CTL_BYTES_SHIFT   0
>>>> +# define SPI_CTL_BYTES_MASK    (0x3ff << SPI_CTL_BYTES_SHIFT)
>>>> +# define SPI_CTL_TYPE_SHIFT    14
>>>> +# define SPI_CTL_TYPE_FD_RW    (0 << SPI_CTL_TYPE_SHIFT)
>>>> +# define SPI_CTL_TYPE_HD_W     (1 << SPI_CTL_TYPE_SHIFT)
>>>> +# define SPI_CTL_TYPE_HD_R     (2 << SPI_CTL_TYPE_SHIFT)
>>>> +
>>>> +# define bcm63xx_spi_wctl(v,a) writew_be(v, a);
>>>> +
>>>> +/* SPI TX Data registers */
>>>> +# define SPI_TX_DATA_REG       0x002
>>>> +# define SPI_TX_DATA_SIZE      0x21e
>>>> +
>>>> +/* SPI RX Data registers */
>>>> +# define SPI_RX_DATA_REG       0x400
>>>> +# define SPI_RX_DATA_SIZE      0x220
>>>> +
>>>> +/* SPI Command register */
>>>> +# define SPI_CMD_REG           0x700
>>>> +
>>>> +/* SPI Interrupt registers */
>>>> +# define SPI_IR_STAT_REG       0x702
>>>> +# define SPI_IR_MASK_REG       0x704
>>>> +
>>>> +/* SPI Clock register */
>>>> +# define SPI_CLK_REG           0x706
>>>> +
>>>> +/* SPI Fill register */
>>>> +# define SPI_FILL_REG          0x707
>>>> +
>>>> +#endif
>>>> +
>>>> +/* SPI Command register */
>>>> +#define SPI_CMD_OP_SHIFT       0
>>>> +#define SPI_CMD_OP_START       (0x3 << SPI_CMD_OP_SHIFT)
>>>> +#define SPI_CMD_SLAVE_SHIFT    4
>>>> +#define SPI_CMD_SLAVE_MASK     (0xf << SPI_CMD_SLAVE_SHIFT)
>>>> +#define SPI_CMD_PREPEND_SHIFT  8
>>>> +#define SPI_CMD_PREPEND_BYTES  0xf
>>>> +#define SPI_CMD_3WIRE_SHIFT    12
>>>> +#define SPI_CMD_3WIRE_MASK     (1 << SPI_CMD_3WIRE_SHIFT)
>>>> +
>>>> +/* SPI Interrupt registers */
>>>> +#define SPI_IR_DONE_SHIFT      0
>>>> +#define SPI_IR_DONE_MASK       (1 << SPI_IR_DONE_SHIFT)
>>>> +#define SPI_IR_RXOVER_SHIFT    1
>>>> +#define SPI_IR_RXOVER_MASK     (1 << SPI_IR_RXOVER_SHIFT)
>>>> +#define SPI_IR_TXUNDER_SHIFT   2
>>>> +#define SPI_IR_TXUNDER_MASK    (1 << SPI_IR_TXUNDER_SHIFT)
>>>> +#define SPI_IR_TXOVER_SHIFT    3
>>>> +#define SPI_IR_TXOVER_MASK     (1 << SPI_IR_TXOVER_SHIFT)
>>>> +#define SPI_IR_RXUNDER_SHIFT   4
>>>> +#define SPI_IR_RXUNDER_MASK    (1 << SPI_IR_RXUNDER_SHIFT)
>>>> +#define SPI_IR_CLEAR_MASK      (SPI_IR_DONE_MASK |\
>>>> +                                SPI_IR_RXOVER_MASK |\
>>>> +                                SPI_IR_TXUNDER_MASK |\
>>>> +                                SPI_IR_TXOVER_MASK |\
>>>> +                                SPI_IR_RXUNDER_MASK)
>>>> +
>>>> +/* SPI Clock register */
>>>> +#define SPI_CLK_SHIFT          0
>>>> +#define SPI_CLK_20MHZ          (0 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_0_391MHZ       (1 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_0_781MHZ       (2 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_1_563MHZ       (3 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_3_125MHZ       (4 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_6_250MHZ       (5 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_12_50MHZ       (6 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_25MHZ          (7 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_MASK           (7 << SPI_CLK_SHIFT)
>>>> +#define SPI_CLK_SSOFF_SHIFT    3
>>>> +#define SPI_CLK_SSOFF_2                (2 << SPI_CLK_SSOFF_SHIFT)
>>>> +#define SPI_CLK_SSOFF_MASK     (7 << SPI_CLK_SSOFF_SHIFT)
>>>> +#define SPI_CLK_BSWAP_SHIFT    7
>>>> +#define SPI_CLK_BSWAP_MASK     (1 << SPI_CLK_BSWAP_SHIFT)
>>>> +
>>>> +#define SPI_CLK_CNT            8
>>>> +static const unsigned bcm63xx_spi_freq_table[SPI_CLK_CNT][2] = {
>>>> +       { 25000000, SPI_CLK_25MHZ },
>>>> +       { 20000000, SPI_CLK_20MHZ },
>>>> +       { 12500000, SPI_CLK_12_50MHZ },
>>>> +       {  6250000, SPI_CLK_6_250MHZ },
>>>> +       {  3125000, SPI_CLK_3_125MHZ },
>>>> +       {  1563000, SPI_CLK_1_563MHZ },
>>>> +       {   781000, SPI_CLK_0_781MHZ },
>>>> +       {   391000, SPI_CLK_0_391MHZ }
>>>> +};
>>>> +
>>>> +struct bcm63xx_spi_priv {
>>>> +       void __iomem *regs;
>>>> +       uint8_t num_cs;
>>>> +       size_t tx_bytes;
>>>> +};
>>>> +
>>>> +static int bcm63xx_spi_cs_info(struct udevice *bus, uint cs,
>>>> +                          struct spi_cs_info *info)
>>>> +{
>>>> +       struct bcm63xx_spi_priv *priv = dev_get_priv(bus);
>>>> +
>>>> +       if (cs >= priv->num_cs) {
>>>> +               error("no cs %u\n", cs);
>>>> +               return -ENODEV;
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int bcm63xx_spi_set_mode(struct udevice *bus, uint mode)
>>>> +{
>>>> +       struct bcm63xx_spi_priv *priv = dev_get_priv(bus);
>>>> +
>>>> +       if (mode & SPI_LSB_FIRST)
>>>> +               setbits_8(priv->regs + SPI_CLK_REG, SPI_CLK_BSWAP_MASK);
>>>> +       else
>>>> +               clrbits_8(priv->regs + SPI_CLK_REG, SPI_CLK_BSWAP_MASK);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int bcm63xx_spi_set_speed(struct udevice *bus, uint speed)
>>>> +{
>>>> +       struct bcm63xx_spi_priv *priv = dev_get_priv(bus);
>>>> +       uint8_t clk_cfg;
>>>> +       int i;
>>>> +
>>>> +       /* default to lowest clock configuration */
>>>> +       clk_cfg = SPI_CLK_0_391MHZ;
>>>> +
>>>> +       /* find the closest clock configuration */
>>>> +       for (i = 0; i < SPI_CLK_CNT; i++) {
>>>> +               if (speed >= bcm63xx_spi_freq_table[i][0]) {
>>>> +                       clk_cfg = bcm63xx_spi_freq_table[i][1];
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       /* write clock configuration */
>>>> +       clrsetbits_8(priv->regs + SPI_CLK_REG,
>>>> +                    SPI_CLK_SSOFF_MASK | SPI_CLK_MASK,
>>>> +                    clk_cfg | SPI_CLK_SSOFF_2);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * BCM63xx SPI driver doesn't allow keeping CS active between transfers since
>>>> + * they are HW controlled.
>>>> + * However, it provides a mechanism to prepend write transfers prior to read
>>>> + * transfers (with a maximum prepend of 15 bytes), which is usually enough for
>>>> + * SPI-connected flashes since reading requires prepending a write transfer of
>>>> + * 5 bytes.
>>>> + *
>>>> + * This implementation takes advantage of the prepend mechanism and combines
>>>> + * multiple transfers into a single one where possible (single/multiple write
>>>> + * transfer(s) followed by a final read/write transfer).
>>>> + * However, it's not possible to buffer reads, which means that read transfers
>>>> + * should always be done as the final ones.
>>>> + * On the other hand, take into account that combining write transfers into
>>>> + * a single one is just buffering and doesn't require prepend mechanism.
>>>> + */
>>>> +static int bcm63xx_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>>> +               const void *dout, void *din, unsigned long flags)
>>>> +{
>>>> +       struct bcm63xx_spi_priv *priv = dev_get_priv(dev->parent);
>>>> +       size_t data_bytes = bitlen / 8;
>>>> +
>>>> +       if (flags & SPI_XFER_BEGIN) {
>>>> +               /* clear prepends */
>>>> +               priv->tx_bytes = 0;
>>>> +
>>>> +               /* initialize hardware */
>>>> +               writeb_be(0, priv->regs + SPI_IR_MASK_REG);
>>>> +       }
>>>> +
>>>> +       if (din) {
>>>> +               /* buffering reads not possible since cs is hw controlled */
>>>> +               if (!(flags & SPI_XFER_END)) {
>>>> +                       error("unable to buffer reads\n");
>>>> +                       return -EINVAL;
>>>> +               }
>>>> +
>>>> +               /* check rx size */
>>>> +                if (data_bytes > SPI_RX_DATA_SIZE) {
>>>> +                       error("max rx bytes exceeded\n");
>>>> +                       return -EMSGSIZE;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       if (dout) {
>>>> +               /* check tx size */
>>>> +               if (priv->tx_bytes + data_bytes > SPI_TX_DATA_SIZE) {
>>>> +                       error("max tx bytes exceeded\n");
>>>> +                       return -EMSGSIZE;
>>>> +               }
>>>> +
>>>> +               /* copy tx data */
>>>> +               memcpy_toio(priv->regs + SPI_TX_DATA_REG + priv->tx_bytes,
>>>> +                           dout, data_bytes);
>>>> +               priv->tx_bytes += data_bytes;
>>>> +       }
>>>> +
>>>> +       if (flags & SPI_XFER_END) {
>>>> +               struct dm_spi_slave_platdata *plat =
>>>> +                       dev_get_parent_platdata(dev);
>>>> +               uint16_t val = 0;
>>>> +               uint8_t irq;
>>>> +
>>>> +               /* determine control config */
>>>> +               if (dout && !din) {
>>>> +                       /* buffered write transfers */
>>>> +                       val |= (priv->tx_bytes << SPI_CTL_BYTES_SHIFT);
>>>> +                       val |= SPI_CTL_TYPE_HD_W;
>>>> +                       priv->tx_bytes = 0;
>>>> +               } else {
>>>> +                       if (dout && din && (flags & SPI_XFER_ONCE)) {
>>>> +                               /* full duplex read/write */
>>>> +                               val |= (data_bytes << SPI_CTL_BYTES_SHIFT);
>>>> +                               val |= SPI_CTL_TYPE_FD_RW;
>>>> +                               priv->tx_bytes = 0;
>>>> +                       } else {
>>>> +                               /* prepended write transfer */
>>>> +                               val |= (data_bytes << SPI_CTL_BYTES_SHIFT);
>>>> +                               val |= SPI_CTL_TYPE_HD_R;
>>>> +                               if (priv->tx_bytes > SPI_CMD_PREPEND_BYTES) {
>>>> +                                       error("max prepend bytes exceeded\n");
>>>> +                                       return -EMSGSIZE;
>>>> +                               }
>>>> +                       }
>>>> +               }
>>>> +               bcm63xx_spi_wctl(val, priv->regs + SPI_CTL_REG);
>>>> +
>>>> +               /* clear interrupts */
>>>> +               writeb_be(SPI_IR_CLEAR_MASK, priv->regs + SPI_IR_STAT_REG);
>>>> +
>>>> +               /* issue the transfer */
>>>> +               val = SPI_CMD_OP_START;
>>>> +               val |= (plat->cs << SPI_CMD_SLAVE_SHIFT) & SPI_CMD_SLAVE_MASK;
>>>> +               val |= (priv->tx_bytes << SPI_CMD_PREPEND_SHIFT);
>>>> +               if (plat->mode & SPI_3WIRE)
>>>> +                       val |= SPI_CMD_3WIRE_MASK;
>>>> +               writew_be(val, priv->regs + SPI_CMD_REG);
>>>> +
>>>> +               /* enable interrupts */
>>>> +               writeb_be(SPI_IR_DONE_MASK, priv->regs + SPI_IR_MASK_REG);
>>>> +
>>>> +               do {
>>>> +                       /* read interupts */
>>>> +                       irq = readb_be(priv->regs + SPI_IR_STAT_REG);
>>>> +
>>>> +                       /* transfer completed */
>>>> +                       if (irq & SPI_IR_DONE_MASK)
>>>> +                               break;
>>>> +               } while (1);
>>>> +
>>>> +               /* copy rx data */
>>>> +               if (din)
>>>> +                       memcpy_fromio(din, priv->regs + SPI_RX_DATA_REG,
>>>> +                                     data_bytes);
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static const struct dm_spi_ops bcm63xx_spi_ops = {
>>>> +       .cs_info = bcm63xx_spi_cs_info,
>>>> +       .set_mode = bcm63xx_spi_set_mode,
>>>> +       .set_speed = bcm63xx_spi_set_speed,
>>>> +       .xfer = bcm63xx_spi_xfer,
>>>> +};
>>>> +
>>>> +static const struct udevice_id bcm63xx_spi_ids[] = {
>>>> +       { .compatible = SPI_DT_ID, },
>>>> +       { /* sentinel */ }
>>>
>>> Try to add .data with reg_space of respective controller, this will
>>> eventually reduce to driver configs and make it CONFIG_BCM63XX and
>>> reduce unneeded#ifdef.
>> This implies adding considerable bloat to the driver...
>> Is it imperative for the driver to be accepted?
> 
> Yes, as per as recent U-boot developement we're trying to reduce
> defconfig code as possible and even this method can improve code
> quality.
I don't think that method applies here, since u-boot is usally built for each SoC and supporting two cores in the same build makes no sense to me.
These should be done in two different drivers but I merged them into a single one to improve maintainability.
BTW, two u-boot developers already reviewed this patch and didn't complain about it...

> 
> thanks!
> 
Thanks.