[U-Boot] [PATCH v5 1/2] spi: zynqmp_gqspi: Add support for ZynqMP qspi driver

Jagan Teki jagan at amarulasolutions.com
Wed Jun 27 08:12:56 UTC 2018


On Wed, Jun 27, 2018 at 1:13 PM, Siva Durga Prasad Paladugu
<sivadur at xilinx.com> wrote:
> Hi Jagan,
>
>> -----Original Message-----
>> From: Jagan Teki [mailto:jagan at amarulasolutions.com]
>> Sent: Wednesday, June 27, 2018 12:52 PM
>> To: Siva Durga Prasad Paladugu <sivadur at xilinx.com>
>> Cc: U-Boot-Denx <u-boot at lists.denx.de>; Jagan Teki
>> <jagan at openedev.com>; Michal Simek <michals at xilinx.com>
>> Subject: Re: [U-Boot] [PATCH v5 1/2] spi: zynqmp_gqspi: Add support for
>> ZynqMP qspi driver
>>
>> On Tue, Jun 26, 2018 at 3:07 PM, Siva Durga Prasad Paladugu
>> <siva.durga.paladugu at xilinx.com> wrote:
>> > This patch adds qspi driver support for ZynqMP SoC. This driver is
>> > responsible for communicating with qspi flash devices.
>> >
>> > Signed-off-by: Siva Durga Prasad Paladugu
>> > <siva.durga.paladugu at xilinx.com>
>> > ---
>> > Changed for v5:
>> > - Removed zynqm_gqspi.h file which was added
>> >   by mistake.
>> >
>> > Changes for v4:
>> > - Moved macro definitions back to .c
>> > - Removed last_cmd and flash command checks in driver
>> > - Used macros and GENMASK as per comments
>> > - Removed debugs wherever commented.
>> > - Modified set_mode routine as per comment
>> >
>> > Changes for v3:
>> > - Renamed all macros, functions, files and configs as per comment
>> > - Used wait_for_bit wherever required
>> > - Removed unnecessary header inclusion
>> >
>> > Changes for v2:
>> > - Rebased on top of latest master
>> > - Moved macro definitions to .h file as per comment
>> > - Fixed magic values with macros as per comment
>> > ---
>> >  drivers/spi/Kconfig        |   7 +
>> >  drivers/spi/Makefile       |   1 +
>> >  drivers/spi/zynqmp_gqspi.c | 794
>> > +++++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 802 insertions(+)
>> >  create mode 100644 drivers/spi/zynqmp_gqspi.c
>> >
>> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
>> > 3532c2a..c3c424e 100644
>> > --- a/drivers/spi/Kconfig
>> > +++ b/drivers/spi/Kconfig
>> > @@ -223,6 +223,13 @@ config ZYNQ_QSPI
>> >           Zynq QSPI IP core. This IP is used to connect the flash in
>> >           4-bit qspi, 8-bit dual stacked and shared 4-bit dual parallel.
>> >
>> > +config ZYNQMP_GQSPI
>> > +       bool "Configure ZynqMP Generic QSPI"
>> > +       depends on ARCH_ZYNQMP
>> > +       help
>> > +         This option is used to enable ZynqMP QSPI controller driver which
>> > +         is used to communicate with qspi flash devices.
>> > +
>> >  endif # if DM_SPI
>> >
>> >  config SOFT_SPI
>> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
>> > 5a2c00e..2187633 100644
>> > --- a/drivers/spi/Makefile
>> > +++ b/drivers/spi/Makefile
>> > @@ -51,3 +51,4 @@ obj-$(CONFIG_TI_QSPI) += ti_qspi.o
>> >  obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
>> >  obj-$(CONFIG_ZYNQ_SPI) += zynq_spi.o
>> >  obj-$(CONFIG_ZYNQ_QSPI) += zynq_qspi.o
>> > +obj-$(CONFIG_ZYNQMP_GQSPI) += zynqmp_gqspi.o
>> > diff --git a/drivers/spi/zynqmp_gqspi.c b/drivers/spi/zynqmp_gqspi.c
>> > new file mode 100644 index 0000000..9a04839
>> > --- /dev/null
>> > +++ b/drivers/spi/zynqmp_gqspi.c
>> > @@ -0,0 +1,794 @@
>> > +// SPDX-License-Identifier: GPL-2.0+
>> > +/*
>> > + * (C) Copyright 2018 Xilinx
>> > + *
>> > + * Xilinx ZynqMP Generic Quad-SPI(QSPI) controller driver(master mode
>> > +only)  */
>> > +
>> > +#include <common.h>
>> > +#include <asm/arch/clk.h>
>> > +#include <asm/arch/hardware.h>
>> > +#include <asm/arch/sys_proto.h>
>> > +#include <asm/io.h>
>> > +#include <clk.h>
>> > +#include <malloc.h>
>> > +#include <memalign.h>
>> > +#include <spi.h>
>> > +#include <spi_flash.h>
>>
>> remove this header.
> Yes can be removed now
>
>>
>> > +#include <ubi_uboot.h>
>> > +#include <wait_bit.h>
>> > +
>> > +#define GQSPI_GFIFO_STRT_MODE_MASK     BIT(29)
>> > +#define GQSPI_CONFIG_MODE_EN_MASK      (3 << 30)
>> > +#define GQSPI_CONFIG_DMA_MODE  (2 << 30) #define
>> > +GQSPI_CONFIG_CPHA_MASK BIT(2) #define
>> GQSPI_CONFIG_CPOL_MASK BIT(1)
>> > +
>> > +/* QSPI MIO's count for different connection topologies */
>> > +#define GQSPI_MIO_NUM_QSPI0            6
>> > +#define GQSPI_MIO_NUM_QSPI1            5
>> > +#define GQSPI_MIO_NUM_QSPI1_CS 1
>> > +
>> > +/*
>> > + * QSPI Interrupt Registers bit Masks
>> > + *
>> > + * All the four interrupt registers (Status/Mask/Enable/Disable) have
>> > +the same
>> > + * bit definitions.
>> > + */
>> > +#define GQSPI_IXR_TXNFULL_MASK 0x00000004 /* QSPI TX FIFO
>> Overflow */
>> > +#define GQSPI_IXR_TXFULL_MASK  0x00000008 /* QSPI TX FIFO is full
>> */
>> > +#define GQSPI_IXR_RXNEMTY_MASK 0x00000010 /* QSPI RX FIFO Not
>> Empty
>> > +*/ #define GQSPI_IXR_GFEMTY_MASK  0x00000080 /* QSPI Generic
>> FIFO Empty */
>> > +#define GQSPI_IXR_ALL_MASK     (GQSPI_IXR_TXNFULL_MASK | \
>> > +                                       GQSPI_IXR_RXNEMTY_MASK)
>> > +
>> > +/*
>> > + * QSPI Enable Register bit Masks
>> > + *
>> > + * This register is used to enable or disable the QSPI controller  */
>> > +#define GQSPI_ENABLE_ENABLE_MASK       0x00000001 /* QSPI Enable
>> Bit Mask */
>> > +
>> > +#define GQSPI_GFIFO_LOW_BUS            BIT(14)
>> > +#define GQSPI_GFIFO_CS_LOWER   BIT(12)
>> > +#define GQSPI_GFIFO_UP_BUS             BIT(15)
>> > +#define GQSPI_GFIFO_CS_UPPER   BIT(13)
>> > +#define GQSPI_SPI_MODE_QSPI            (3 << 10)
>> > +#define GQSPI_SPI_MODE_SPI             BIT(10)
>> > +#define GQSPI_SPI_MODE_DUAL_SPI                (2 << 10)
>> > +#define GQSPI_IMD_DATA_CS_ASSERT       5
>> > +#define GQSPI_IMD_DATA_CS_DEASSERT     5
>> > +#define GQSPI_GFIFO_TX         BIT(16)
>> > +#define GQSPI_GFIFO_RX         BIT(17)
>> > +#define GQSPI_GFIFO_STRIPE_MASK        BIT(18)
>> > +#define GQSPI_GFIFO_IMD_MASK   0xFF
>> > +#define GQSPI_GFIFO_EXP_MASK   BIT(9)
>> > +#define GQSPI_GFIFO_DATA_XFR_MASK      BIT(8)
>> > +#define GQSPI_STRT_GEN_FIFO            BIT(28)
>> > +#define GQSPI_GEN_FIFO_STRT_MOD        BIT(29)
>> > +#define GQSPI_GFIFO_WP_HOLD            BIT(19)
>> > +#define GQSPI_BAUD_DIV_MASK    (7 << 3)
>> > +#define GQSPI_DFLT_BAUD_RATE_DIV       BIT(3)
>> > +#define GQSPI_GFIFO_ALL_INT_MASK       0xFBE
>> > +#define GQSPI_DMA_DST_I_STS_DONE       BIT(1)
>> > +#define GQSPI_DMA_DST_I_STS_MASK       0xFE
>> > +#define MODEBITS       0x6
>> > +
>> > +#define QUAD_OUT_READ_CMD              0x6B
>> > +#define QUAD_PAGE_PROGRAM_CMD          0x32
>> > +#define DUAL_OUTPUT_FASTRD_CMD         0x3B
>>
>> remove these as well.
> Same as above
>
>>
>> > +
>> > +#define GQSPI_GFIFO_SELECT             BIT(0)
>> > +
>> > +#define GQSPI_FIFO_THRESHOLD 1
>> > +
>> > +#define SPI_XFER_ON_BOTH       0
>> > +#define SPI_XFER_ON_LOWER      1
>> > +#define SPI_XFER_ON_UPPER      2
>> > +
>> > +#define GQSPI_DMA_ALIGN        0x4
>> > +#define GQSPI_MAX_BAUD_RATE_VAL        7
>> > +#define GQSPI_DFLT_BAUD_RATE_VAL       2
>> > +
>> > +#define GQSPI_TIMEOUT  100000000
>> > +
>> > +#define GQSPI_BAUD_DIV_SHIFT           2
>> > +#define GQSPI_LPBK_DLY_ADJ_LPBK_SHIFT  5
>> > +#define GQSPI_LPBK_DLY_ADJ_DLY_1       0x2
>> > +#define GQSPI_LPBK_DLY_ADJ_DLY_1_SHIFT 3
>> > +#define GQSPI_LPBK_DLY_ADJ_DLY_0       0x3
>> > +#define GQSPI_USE_DATA_DLY             0x1
>> > +#define GQSPI_USE_DATA_DLY_SHIFT       31
>> > +#define GQSPI_DATA_DLY_ADJ_VALUE       0x2
>> > +#define GQSPI_DATA_DLY_ADJ_SHIFT       28
>> > +#define TAP_DLY_BYPASS_LQSPI_RX_VALUE  0x1 #define
>> > +TAP_DLY_BYPASS_LQSPI_RX_SHIFT  2
>> > +#define GQSPI_DATA_DLY_ADJ_OFST                0x000001F8
>> > +#define IOU_TAPDLY_BYPASS_OFST         0xFF180390
>> > +#define GQSPI_LPBK_DLY_ADJ_USE_LPBK_MASK       0x00000020
>> > +#define GQSPI_FREQ_40MHZ               40000000
>> > +#define GQSPI_FREQ_100MHZ              100000000
>> > +#define GQSPI_FREQ_150MHZ              150000000
>> > +#define IOU_TAPDLY_BYPASS_MASK         0x7
>> > +
>> > +#define GQSPI_REG_OFFSET               0x100
>> > +#define GQSPI_DMA_REG_OFFSET           0x800
>>
>> most of the macros in this file are not tab alignied, please align properly.
> All are tab aligned only, do you mean that there only be a single tab between MACRO and value?

I saw most of macros are missaliged with tab space when I apply in tree.

>
>>
>> > +
>> > +/* QSPI register offsets */
>> > +struct zynqmp_qspi_regs {
>> > +       u32 confr;      /* 0x00 */
>> > +       u32 isr;        /* 0x04 */
>> > +       u32 ier;        /* 0x08 */
>> > +       u32 idisr;      /* 0x0C */
>> > +       u32 imaskr;     /* 0x10 */
>> > +       u32 enbr;       /* 0x14 */
>> > +       u32 dr;         /* 0x18 */
>> > +       u32 txd0r;      /* 0x1C */
>> > +       u32 drxr;       /* 0x20 */
>> > +       u32 sicr;       /* 0x24 */
>> > +       u32 txftr;      /* 0x28 */
>> > +       u32 rxftr;      /* 0x2C */
>> > +       u32 gpior;      /* 0x30 */
>> > +       u32 reserved0;  /* 0x34 */
>> > +       u32 lpbkdly;    /* 0x38 */
>> > +       u32 reserved1;  /* 0x3C */
>> > +       u32 genfifo;    /* 0x40 */
>> > +       u32 gqspisel;   /* 0x44 */
>> > +       u32 reserved2;  /* 0x48 */
>> > +       u32 gqfifoctrl; /* 0x4C */
>> > +       u32 gqfthr;     /* 0x50 */
>> > +       u32 gqpollcfg;  /* 0x54 */
>> > +       u32 gqpollto;   /* 0x58 */
>> > +       u32 gqxfersts;  /* 0x5C */
>> > +       u32 gqfifosnap; /* 0x60 */
>> > +       u32 gqrxcpy;    /* 0x64 */
>> > +       u32 reserved3[36];      /* 0x68 */
>> > +       u32 gqspidlyadj;        /* 0xF8 */
>> > +};
>> > +
>> > +struct zynqmp_qspi_dma_regs {
>> > +       u32 dmadst;     /* 0x00 */
>> > +       u32 dmasize;    /* 0x04 */
>> > +       u32 dmasts;     /* 0x08 */
>> > +       u32 dmactrl;    /* 0x0C */
>> > +       u32 reserved0;  /* 0x10 */
>> > +       u32 dmaisr;     /* 0x14 */
>> > +       u32 dmaier;     /* 0x18 */
>> > +       u32 dmaidr;     /* 0x1C */
>> > +       u32 dmaimr;     /* 0x20 */
>> > +       u32 dmactrl2;   /* 0x24 */
>> > +       u32 dmadstmsb;  /* 0x28 */
>> > +};
>> > +
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +struct zynqmp_qspi_platdata {
>> > +       struct zynqmp_qspi_regs *regs;
>> > +       struct zynqmp_qspi_dma_regs *dma_regs;
>> > +       u32 frequency;
>> > +       u32 speed_hz;
>> > +       unsigned int tx_rx_mode;
>> > +};
>> > +
>> > +struct zynqmp_qspi_priv {
>> > +       struct zynqmp_qspi_regs *regs;
>> > +       struct zynqmp_qspi_dma_regs *dma_regs;
>> > +       u8 mode;
>> > +       const void *tx_buf;
>> > +       void *rx_buf;
>> > +       unsigned int len;
>> > +       int bytes_to_transfer;
>> > +       int bytes_to_receive;
>> > +       unsigned int is_inst;
>> > +       unsigned int cs_change:1;
>> > +};
>> > +
>> > +static int zynqmp_qspi_ofdata_to_platdata(struct udevice *bus) {
>> > +       struct zynqmp_qspi_platdata *plat = bus->platdata;
>> > +       u32 mode = 0;
>> > +       u32 value;
>> > +       int ret;
>> > +       struct clk clk;
>> > +       unsigned long clock;
>> > +
>> > +       debug("%s\n", __func__);
>> > +
>> > +       plat->regs = (struct zynqmp_qspi_regs *)(devfdt_get_addr(bus) +
>> > +                                                GQSPI_REG_OFFSET);
>> > +       plat->dma_regs = (struct zynqmp_qspi_dma_regs *)
>> > +                         (devfdt_get_addr(bus) +
>> > + GQSPI_DMA_REG_OFFSET);
>> > +
>> > +       ret = clk_get_by_index(bus, 0, &clk);
>> > +       if (ret < 0) {
>> > +               dev_err(dev, "failed to get clock\n");
>> > +               return ret;
>> > +       }
>> > +
>> > +       clock = clk_get_rate(&clk);
>> > +       if (IS_ERR_VALUE(clock)) {
>> > +               dev_err(dev, "failed to get rate\n");
>> > +               return clock;
>> > +       }
>> > +       debug("%s: CLK %ld\n", __func__, clock);
>> > +
>> > +       ret = clk_enable(&clk);
>> > +       if (ret && ret != -ENOSYS) {
>> > +               dev_err(dev, "failed to enable clock\n");
>> > +               return ret;
>> > +       }
>>
>> these clock stuff, add directly in probe.
>
> Will check and move it if possible.
>
>>
>> > +
>> > +       if (dev_read_bool(bus, "spi-cpol"))
>> > +               mode |= SPI_CPOL;
>> > +       if (dev_read_bool(bus, "spi-cpha"))
>> > +               mode |= SPI_CPHA;
>> > +
>> > +       value = dev_read_u32_default(bus, "spi-rx-bus-width", 1);
>> > +       switch (value) {
>> > +       case 1:
>> > +               break;
>> > +       case 2:
>> > +               mode |= SPI_RX_DUAL;
>> > +               break;
>> > +       case 4:
>> > +               mode |= SPI_RX_QUAD;
>> > +               break;
>> > +       default:
>> > +               printf("Invalid spi-rx-bus-width %d\n", value);
>> > +               break;
>> > +       }
>> > +
>> > +       value = dev_read_u32_default(bus, "spi-tx-bus-width", 1);
>> > +       switch (value) {
>> > +       case 1:
>> > +               break;
>> > +       case 2:
>> > +               mode |= SPI_TX_DUAL;
>> > +               break;
>> > +       case 4:
>> > +               mode |= SPI_TX_QUAD;
>> > +               break;
>> > +       default:
>> > +               printf("Invalid spi-tx-bus-width %d\n", value);
>> > +               break;
>> > +       }
>> > +
>> > +       plat->tx_rx_mode = mode;
>>
>> So, here I'm confused. tx_rx_mode store the mode from dt and assigned in
>> slave->mode in zynqmp_qspi_child_pre_probe, why we need that? and it's
>> never used anywhere. are you trying to change mode in spi-uclass.c?
>
> I agree. If this is the only place, will check and update slave mode here itself. Then we can remove preprobe.

Ha, if ie the case why we need to update the mode? since the mode is
always come from spi-uclass which is again grabbed in .set_mode. if we
want the mode to use anywhere in private driver we can assign it from
there.


More information about the U-Boot mailing list