Hi, > -----Original Message----- > From: Jagan Teki [mailto:ja...@amarulasolutions.com] > Sent: Wednesday, April 25, 2018 10:47 AM > To: Siva Durga Prasad Paladugu <siva...@xilinx.com> > Cc: U-Boot-Denx <u-boot@lists.denx.de> > Subject: Re: [U-Boot] [PATCH v2 1/2] spi: zynqmp_qspi: Add support for > ZynqMP qspi driver > > On Wed, Feb 7, 2018 at 3:40 PM, Siva Durga Prasad Paladugu > <siva...@xilinx.com> wrote: > > Hi Jagan, > > > > > >> -----Original Message----- > >> From: Jagan Teki [mailto:ja...@amarulasolutions.com] > >> Sent: Tuesday, January 23, 2018 10:41 PM > >> To: Siva Durga Prasad Paladugu <siva...@xilinx.com> > >> Cc: U-Boot-Denx <u-boot@lists.denx.de>; Siva Durga Prasad Paladugu > >> <siva...@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.palad...@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...@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.
Looks like you didn’t get my point, in my flow the routine spi_slave_ofdata_to_platdata() in spi-uclass.c is not getting invoked at all that’s why I am not getting this data from spi-uclass.c. do you have an idea on what could be the issue that spi_slave_ofdata_to_platdata() is not getting invoked. I will anyway debug it but, if you already encountered this and aware of any reason for this , please let me know, that really helps me. Thanks, Siva > > 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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot