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

Jagan Teki jagan at amarulasolutions.com
Wed Apr 25 05:16:43 UTC 2018


On Wed, Feb 7, 2018 at 3:40 PM, Siva Durga Prasad Paladugu
<sivadur at xilinx.com> wrote:
> Hi Jagan,
>
>
>> -----Original Message-----
>> From: Jagan Teki [mailto:jagan at amarulasolutions.com]
>> Sent: Tuesday, January 23, 2018 10:41 PM
>> To: Siva Durga Prasad Paladugu <sivadur at xilinx.com>
>> Cc: U-Boot-Denx <u-boot at lists.denx.de>; Siva Durga Prasad Paladugu
>> <sivadur at xilinx.com>
>> Subject: Re: [U-Boot] [PATCH v2 1/2] spi: zynqmp_qspi: Add support for
>> ZynqMP qspi driver
>>
>> On Thu, Jan 4, 2018 at 1: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 <sivadur at xilinx.com>
>> > ---
>> > Changes from v1:
>> > - Rebased on top of latest master
>> > - Moved macro definitions to .h file as per comment
>> > - Fixed magic values with macros as per comment
>> > ---
>> >  arch/arm/cpu/armv8/zynqmp/Kconfig              |   7 +
>> >  arch/arm/include/asm/arch-zynqmp/zynqmp_qspi.h | 154 ++++++
>> >  drivers/spi/Makefile                           |   1 +
>> >  drivers/spi/zynqmp_qspi.c                      | 678
>> +++++++++++++++++++++++++
>>
>> Was this gqspi like linux spi-zynqmp-gqspi.c or different?
> Yes.

then try to use similar naming conventions in macros, or in function
names as well.

>
>>
>> >  4 files changed, 840 insertions(+)
>> >  create mode 100644 arch/arm/include/asm/arch-
>> zynqmp/zynqmp_qspi.h
>> >  create mode 100644 drivers/spi/zynqmp_qspi.c
>> >
>> > diff --git a/arch/arm/cpu/armv8/zynqmp/Kconfig
>> > b/arch/arm/cpu/armv8/zynqmp/Kconfig
>> > index 3f922b4..2fe4f71 100644
>> > --- a/arch/arm/cpu/armv8/zynqmp/Kconfig
>> > +++ b/arch/arm/cpu/armv8/zynqmp/Kconfig
>> > @@ -65,6 +65,13 @@ config PMUFW_INIT_FILE
>> >           Include external PMUFW (Platform Management Unit FirmWare) to
>> >           a Xilinx bootable image (boot.bin).
>> >
>> > +config ZYNQMP_QSPI
>> > +       bool "Configure ZynqMP QSPI"
>> > +       select DM_SPI
>> > +       help
>> > +         This option is used to enable ZynqMP QSPI controller driver which
>> > +         is used to communicate with qspi flash devices.
>>
>> I've commented this before, what is the reason for adding spi kconfig entry
>> in arch area instead of drivers/spi?
>
> I might have missed it, Will move to drivers/spi
>>
>> > +
>> >  config ZYNQMP_USB
>> >         bool "Configure ZynqMP USB"
>> >
>> > diff --git a/arch/arm/include/asm/arch-zynqmp/zynqmp_qspi.h
>> > b/arch/arm/include/asm/arch-zynqmp/zynqmp_qspi.h
>> > new file mode 100644
>> > index 0000000..5e2926e
>> > --- /dev/null
>> > +++ b/arch/arm/include/asm/arch-zynqmp/zynqmp_qspi.h
>> > @@ -0,0 +1,154 @@
>> > +/*
>> > + * (C) Copyright 2014 - 2015 Xilinx
>> > + *
>> > + * Xilinx ZynqMP Quad-SPI(QSPI) controller driver (master mode only)
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0+
>> > + */
>> > +
>> > +#ifndef _ASM_ARCH_ZYNQMP_QSPI_H_
>> > +#define _ASM_ARCH_ZYNQMP_QSPI_H_
>> > +
>> > +#define ZYNQMP_QSPI_GFIFO_STRT_MODE_MASK       BIT(29)
>> > +#define ZYNQMP_QSPI_CONFIG_MODE_EN_MASK        (3 << 30)
>> > +#define ZYNQMP_QSPI_CONFIG_DMA_MODE    (2 << 30)
>> > +#define ZYNQMP_QSPI_CONFIG_CPHA_MASK   BIT(2)
>> > +#define ZYNQMP_QSPI_CONFIG_CPOL_MASK   BIT(1)
>> > +
>> > +/* QSPI MIO's count for different connection topologies */
>> > +#define ZYNQMP_QSPI_MIO_NUM_QSPI0              6
>> > +#define ZYNQMP_QSPI_MIO_NUM_QSPI1              5
>> > +#define ZYNQMP_QSPI_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 ZYNQMP_QSPI_IXR_TXNFULL_MASK   0x00000004 /* QSPI TX
>> FIFO Overflow */
>> > +#define ZYNQMP_QSPI_IXR_TXFULL_MASK    0x00000008 /* QSPI TX
>> FIFO is full */
>> > +#define ZYNQMP_QSPI_IXR_RXNEMTY_MASK   0x00000010 /* QSPI RX
>> FIFO Not Empty */
>> > +#define ZYNQMP_QSPI_IXR_GFEMTY_MASK    0x00000080 /* QSPI
>> Generic FIFO Empty */
>> > +#define ZYNQMP_QSPI_IXR_ALL_MASK
>> (ZYNQMP_QSPI_IXR_TXNFULL_MASK | \
>> > +                                       ZYNQMP_QSPI_IXR_RXNEMTY_MASK)
>> > +
>> > +/*
>> > + * QSPI Enable Register bit Masks
>> > + *
>> > + * This register is used to enable or disable the QSPI controller  */
>> > +#define ZYNQMP_QSPI_ENABLE_ENABLE_MASK 0x00000001 /* QSPI
>> Enable Bit
>> > +Mask */
>> > +
>> > +#define ZYNQMP_QSPI_GFIFO_LOW_BUS              BIT(14)
>> > +#define ZYNQMP_QSPI_GFIFO_CS_LOWER     BIT(12)
>> > +#define ZYNQMP_QSPI_GFIFO_UP_BUS               BIT(15)
>> > +#define ZYNQMP_QSPI_GFIFO_CS_UPPER     BIT(13)
>> > +#define ZYNQMP_QSPI_SPI_MODE_QSPI              (3 << 10)
>> > +#define ZYNQMP_QSPI_SPI_MODE_SPI               BIT(10)
>> > +#define ZYNQMP_QSPI_SPI_MODE_DUAL_SPI          (2 << 10)
>> > +#define ZYNQMP_QSPI_IMD_DATA_CS_ASSERT 5
>> > +#define ZYNQMP_QSPI_IMD_DATA_CS_DEASSERT       5
>> > +#define ZYNQMP_QSPI_GFIFO_TX           BIT(16)
>> > +#define ZYNQMP_QSPI_GFIFO_RX           BIT(17)
>> > +#define ZYNQMP_QSPI_GFIFO_STRIPE_MASK  BIT(18)
>> > +#define ZYNQMP_QSPI_GFIFO_IMD_MASK     0xFF
>> > +#define ZYNQMP_QSPI_GFIFO_EXP_MASK     BIT(9)
>> > +#define ZYNQMP_QSPI_GFIFO_DATA_XFR_MASK        BIT(8)
>> > +#define ZYNQMP_QSPI_STRT_GEN_FIFO              BIT(28)
>> > +#define ZYNQMP_QSPI_GEN_FIFO_STRT_MOD  BIT(29)
>> > +#define ZYNQMP_QSPI_GFIFO_WP_HOLD              BIT(19)
>> > +#define ZYNQMP_QSPI_BAUD_DIV_MASK      (7 << 3)
>> > +#define ZYNQMP_QSPI_DFLT_BAUD_RATE_DIV BIT(3) #define
>> > +ZYNQMP_QSPI_GFIFO_ALL_INT_MASK 0xFBE #define
>> > +ZYNQMP_QSPI_DMA_DST_I_STS_DONE BIT(1) #define
>> > +ZYNQMP_QSPI_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
>> > +
>> > +#define ZYNQMP_QSPI_GFIFO_SELECT               BIT(0)
>> > +
>> > +#define ZYNQMP_QSPI_FIFO_THRESHOLD 1
>> > +
>> > +#define SPI_XFER_ON_BOTH       0
>> > +#define SPI_XFER_ON_LOWER      1
>> > +#define SPI_XFER_ON_UPPER      2
>> > +
>> > +#define ZYNQMP_QSPI_DMA_ALIGN  0x4
>> > +#define ZYNQMP_QSPI_MAX_BAUD_RATE_VAL  7 #define
>> > +ZYNQMP_QSPI_DFLT_BAUD_RATE_VAL 2
>> > +
>> > +#define ZYNQMP_QSPI_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 ZYNQMP_GQSPI_REG_OFFSET                0x100
>> > +#define ZYNQMP_GQSPI_DMA_REG_OFFSET    0x800
>> > +
>> > +/* 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 */
>> > +};
>> > +
>> > +#endif /* _ASM_ARCH_ZYNQMP_QSPI_H_ */
>>
>> Move all these on driver file itself, also try to use short macro names.
>
> I moved to .h based on v1 comments, now vice versa. I didn’t see any problem in having these in .h
>
>>
>> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
>> > e3184db..9cdd94a 100644
>> > --- a/drivers/spi/Makefile
>> > +++ b/drivers/spi/Makefile
>> > @@ -50,3 +50,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_QSPI) += zynqmp_qspi.o
>> > diff --git a/drivers/spi/zynqmp_qspi.c b/drivers/spi/zynqmp_qspi.c new
>> > file mode 100644 index 0000000..199e704
>> > --- /dev/null
>> > +++ b/drivers/spi/zynqmp_qspi.c
>> > @@ -0,0 +1,678 @@
>> > +/*
>> > + * (C) Copyright 2014 - 2015 Xilinx
>> > + *
>> > + * Xilinx ZynqMP Quad-SPI(QSPI) controller driver (master mode only)
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0
>> > + */
>> > +
>> > +#include <common.h>
>> > +#include <malloc.h>
>> > +#include <memalign.h>
>> > +#include <ubi_uboot.h>
>> > +#include <spi.h>
>> > +#include <spi_flash.h>
>> > +#include <asm/io.h>
>> > +#include <asm/arch/hardware.h>
>> > +#include <asm/arch/sys_proto.h>
>> > +#include <asm/arch/clk.h>
>> > +#include "../mtd/spi/sf_internal.h"
>>
>> This I've already commented and still there any reason?
>>
>> > +#include <clk.h>
>> > +#include <asm/arch/zynqmp_qspi.h>
>> > +
>> > +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 u8 last_cmd;
>> > +
>> > +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__);
>>
>> no need
>
> Any issue in having it with debug..?? I can remove if you don’t like.
>>
>> > +
>> > +       plat->regs = (struct zynqmp_qspi_regs *)(devfdt_get_addr(bus) +
>> > +                                                ZYNQMP_GQSPI_REG_OFFSET);
>> > +       plat->dma_regs = (struct zynqmp_qspi_dma_regs *)
>> > +                         (devfdt_get_addr(bus) +
>> > + ZYNQMP_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;
>> > +       }
>> > +
>> > +       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;
>> > +
>> > +       plat->frequency = clock;
>> > +       plat->speed_hz = plat->frequency;
>>
>> why we need this? all these are generic stuff which is available at spi-
>> uclass.c
>
> Somehow I am not able to get these from spi-uclass.c , the routine which reads all these info from dt in spi-uclass.c is never getting invoked in my flow.
> I checked other driver as well,. Do you have an idea on why is it so?

All these attributes are from platdata which were initialized by
spi-uclass.c so if you need any of these we can get the
dm_spi_slave_platdata from your driver using dev_get_parent_platdata()
function.

Remeber let not add spi-nor features in this driver, you may ended-up
more adding more redundent code in driver if you do so.

Fix, what ever comments you agree and resend we will review again.

Jagan.


-- 
Jagan Teki
Senior Linux Kernel Engineer | Amarula Solutions
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.


More information about the U-Boot mailing list