Hi,
> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
> Sent: Wednesday, May 09, 2018 1:12 PM
> To: Siva Durga Prasad Paladugu <siva...@xilinx.com>
> Cc: Jagan Teki <ja...@amarulasolutions.com>; U-Boot-Denx <u-
> b...@lists.denx.de>; michal.si...@xilinx.com
> Subject: Re: [U-Boot] [PATCH v3 1/2] spi: zynqmp_gqspi: Add support for
> ZynqMP qspi driver
>
> On Wed, May 9, 2018 at 12:33 PM, Siva Durga Prasad Paladugu
> <siva...@xilinx.com> wrote:
> > Hi Jagan,
> >
> >> -----Original Message-----
> >> From: Jagan Teki [mailto:ja...@amarulasolutions.com]
> >> Sent: Wednesday, May 09, 2018 12:01 PM
> >> To: Siva Durga Prasad Paladugu <siva...@xilinx.com>
> >> Cc: U-Boot-Denx <u-boot@lists.denx.de>; michal.si...@xilinx.com
> >> Subject: Re: [PATCH v3 1/2] spi: zynqmp_gqspi: Add support for
> ZynqMP
> >> qspi driver
> >>
> >> On Tue, May 8, 2018 at 3:43 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.durga.palad...@xilinx.com>
> >> > ---
> >> > 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
> >> > ---
> >> > arch/arm/include/asm/arch-zynqmp/zynqmp_gqspi.h | 154 ++++++
> >> > drivers/spi/Kconfig | 7 +
> >> > drivers/spi/Makefile | 1 +
> >> > drivers/spi/zynqmp_gqspi.c | 670
> >> ++++++++++++++++++++++++
> >> > 4 files changed, 832 insertions(+) create mode 100644
> >> > arch/arm/include/asm/arch-
> >> zynqmp/zynqmp_gqspi.h
> >> > create mode 100644 drivers/spi/zynqmp_gqspi.c
> >> >
> >> > diff --git a/arch/arm/include/asm/arch-zynqmp/zynqmp_gqspi.h
> >> > b/arch/arm/include/asm/arch-zynqmp/zynqmp_gqspi.h
> >>
> >> already asked you to move this header code in driver .c file
> >
> > You might have missed my reply to your earlier comment on this. These
> > were moved to .h based on comment from Lukasz in v1.
> > I don’t have any issue in having them anywhere. Let me know your choice.
>
> I'm trying to align Linux code, better to move like that and make sure to use
> similar macros.
>
> >
[snip]
> >> > +static void zynqmp_qspi_genfifo_cmd(struct zynqmp_qspi_priv
> *priv) {
> >> > + u8 command = 1;
> >> > + u32 gen_fifo_cmd;
> >> > + u32 bytecount = 0;
> >> > +
> >> > + while (priv->len) {
> >> > + gen_fifo_cmd = zynqmp_qspi_bus_select(priv);
> >> > + gen_fifo_cmd |= GQSPI_GFIFO_TX;
> >> > +
> >> > + if (command) {
> >> > + command = 0;
> >> > + last_cmd = *(u8 *)priv->tx_buf;
> >> > + }
> >>
> >> don't understand this code can you explain? command assigned 1 it
> >> will not updated anywhere?
> >
> > I want to store last command sent. As the first byte in loop only
> > contains command, it ensures it fills only for one time and next time it
> may contain data to be sent along with command.
> > Command initialized to 1 while declaring it above(u8 command = 1).
>
> Ok the first TX buf has command and reused in tx and rx fifo, how come to
> use use same in rx fifo? why is is last_cmd it is first_cmd?
This holds the command that was sent last to the device before we use in tx/rx
fill, hence used this name.
Thanks,
Siva
>
>
> >
> >>
> >> > +
> >> > + gen_fifo_cmd |= GQSPI_SPI_MODE_SPI;
> >> > + gen_fifo_cmd |= *(u8 *)priv->tx_buf;
> >> > + bytecount++;
> >> > + priv->len--;
> >> > + priv->tx_buf = (u8 *)priv->tx_buf + 1;
> >> > +
> >> > + debug("GFIFO_CMD_Cmd = 0x%x\n", gen_fifo_cmd);
> >> > +
> >> > + zynqmp_qspi_fill_gen_fifo(priv, gen_fifo_cmd);
> >> > + }
> >> > +}
> >> > +
> >> > +static u32 zynqmp_qspi_calc_exp(struct zynqmp_qspi_priv *priv,
> >> > + u32 *gen_fifo_cmd) {
> >> > + u32 expval = 8;
> >> > + u32 len;
> >> > +
> >> > + while (1) {
> >> > + if (priv->len > 255) {
> >>
> >> what is 255 here?
> > When length is greater than 2^8 then we should fill length in different
> way, hence this check.
> > I will anyway use macro for better readability.
>
> make sure to write proper comments on why?
>
> >
> >>
> >> > + if (priv->len & (1 << expval)) {
> >> > + *gen_fifo_cmd &= ~GQSPI_GFIFO_IMD_MASK;
> >> > + *gen_fifo_cmd |= GQSPI_GFIFO_EXP_MASK;
> >> > + *gen_fifo_cmd |= expval;
> >> > + priv->len -= (1 << expval);
> >> > + return expval;
> >> > + }
> >> > + expval++;
> >> > + } else {
> >> > + *gen_fifo_cmd &= ~(GQSPI_GFIFO_IMD_MASK |
> >> > + GQSPI_GFIFO_EXP_MASK);
> >> > + *gen_fifo_cmd |= (u8)priv->len;
> >> > + len = (u8)priv->len;
> >> > + priv->len = 0;
> >> > + return len;
> >> > + }
> >> > + }
> >> > +}
> >> > +
> >> > +static int zynqmp_qspi_genfifo_fill_tx(struct zynqmp_qspi_priv
> >> > +*priv) {
> >> > + u32 gen_fifo_cmd;
> >> > + u32 len;
> >> > + int ret = 0;
> >> > +
> >> > + gen_fifo_cmd = zynqmp_qspi_bus_select(priv);
> >> > + gen_fifo_cmd |= GQSPI_GFIFO_TX |
> >> > + GQSPI_GFIFO_DATA_XFR_MASK;
> >> > +
> >> > + if (last_cmd == QUAD_PAGE_PROGRAM_CMD)
> >> > + gen_fifo_cmd |= GQSPI_SPI_MODE_QSPI;
> >> > + else
> >> > + gen_fifo_cmd |= GQSPI_SPI_MODE_SPI;
> >> > +
> >> > + while (priv->len) {
> >> > + len = zynqmp_qspi_calc_exp(priv, &gen_fifo_cmd);
> >> > + zynqmp_qspi_fill_gen_fifo(priv, gen_fifo_cmd);
> >> > +
> >> > + debug("GFIFO_CMD_TX:0x%x\n", gen_fifo_cmd);
> >> > +
> >> > + if (gen_fifo_cmd & GQSPI_GFIFO_EXP_MASK)
> >> > + ret = zynqmp_qspi_fill_tx_fifo(priv,
> >> > + 1 << len);
> >> > + else
> >> > + ret = zynqmp_qspi_fill_tx_fifo(priv,
> >> > + len);
> >> > +
> >> > + if (ret)
> >> > + return ret;
> >> > + }
> >> > + return ret;
> >> > +}
> >> > +
> >> > +static int zynqmp_qspi_start_dma(struct zynqmp_qspi_priv *priv,
> >> > + u32 gen_fifo_cmd, u32 *buf) {
> >> > + u32 addr;
> >> > + u32 size, len;
> >> > + u32 actuallen = priv->len;
> >> > + int ret = 0;
> >> > + struct zynqmp_qspi_dma_regs *dma_regs = priv->dma_regs;
> >> > +
> >> > + writel((unsigned long)buf, &dma_regs->dmadst);
> >> > + writel(roundup(priv->len, 4), &dma_regs->dmasize);
> >> > + writel(GQSPI_DMA_DST_I_STS_MASK, &dma_regs->dmaier);
> >> > + addr = (unsigned long)buf;
> >> > + size = roundup(priv->len, ARCH_DMA_MINALIGN);
> >> > + flush_dcache_range(addr, addr + size);
> >> > +
> >> > + while (priv->len) {
> >> > + len = zynqmp_qspi_calc_exp(priv, &gen_fifo_cmd);
> >> > + if (!(gen_fifo_cmd & GQSPI_GFIFO_EXP_MASK) &&
> >> > + (len % 4)) {
> >> > + gen_fifo_cmd &= ~(0xFF);
> >> > + gen_fifo_cmd |= (len / 4 + 1) * 4;
> >>
> >> Need macros for these numerics
> > Ok
> >>
> >> > + }
> >> > + zynqmp_qspi_fill_gen_fifo(priv, gen_fifo_cmd);
> >> > +
> >> > + debug("GFIFO_CMD_RX:0x%x\n", gen_fifo_cmd);
> >> > + }
> >> > +
> >> > + ret = wait_for_bit_le32(&dma_regs->dmaisr,
> >> GQSPI_DMA_DST_I_STS_DONE,
> >> > + 1, GQSPI_TIMEOUT, 1);
> >> > + if (ret) {
> >> > + printf("DMA Timeout:0x%x\n", readl(&dma_regs->dmaisr));
> >> > + return -1;
> >> > + }
> >> > +
> >> > + writel(GQSPI_DMA_DST_I_STS_DONE, &dma_regs->dmaisr);
> >> > +
> >> > + debug("buf:0x%lx, rxbuf:0x%lx, *buf:0x%x len: 0x%x\n",
> >> > + (unsigned long)buf, (unsigned long)priv->rx_buf, *buf,
> >> > + actuallen);
> >> > +
> >> > + if (buf != priv->rx_buf)
> >> > + memcpy(priv->rx_buf, buf, actuallen);
> >> > +
> >> > + return 0;
> >> > +}
> >> > +
> >> > +static int zynqmp_qspi_genfifo_fill_rx(struct zynqmp_qspi_priv
> >> > +*priv) {
> >> > + u32 gen_fifo_cmd;
> >> > + u32 *buf;
> >> > + u32 actuallen = priv->len;
> >> > +
> >> > + gen_fifo_cmd = zynqmp_qspi_bus_select(priv);
> >> > + gen_fifo_cmd |= GQSPI_GFIFO_RX |
> >> > + GQSPI_GFIFO_DATA_XFR_MASK;
> >> > +
> >> > + if (last_cmd == QUAD_OUT_READ_CMD)
> >> > + gen_fifo_cmd |= GQSPI_SPI_MODE_QSPI;
> >> > + else if (last_cmd == DUAL_OUTPUT_FASTRD_CMD)
> >> > + gen_fifo_cmd |= GQSPI_SPI_MODE_DUAL_SPI;
> >> > + else
> >> > + gen_fifo_cmd |= GQSPI_SPI_MODE_SPI;
> >> > +
> >> > + /*
> >> > + * Check if receive buffer is aligned to 4 byte and length
> >> > + * is multiples of four byte as we are using dma to receive.
> >> > + */
> >> > + if (!((unsigned long)priv->rx_buf & (GQSPI_DMA_ALIGN - 1)) &&
> >> > + !(actuallen % GQSPI_DMA_ALIGN)) {
> >> > + buf = (u32 *)priv->rx_buf;
> >> > + return zynqmp_qspi_start_dma(priv, gen_fifo_cmd, buf);
> >> > + }
> >> > +
> >> > + ALLOC_CACHE_ALIGN_BUFFER(u8, tmp, roundup(priv->len,
> >> > + GQSPI_DMA_ALIGN));
> >> > + buf = (u32 *)tmp;
> >> > + return zynqmp_qspi_start_dma(priv, gen_fifo_cmd, buf); }
> >> > +
> >> > +static int zynqmp_qspi_start_transfer(struct zynqmp_qspi_priv
> >> > +*priv) {
> >> > + int ret = 0;
> >> > +
> >> > + if (priv->is_inst) {
> >> > + if (priv->tx_buf)
> >> > + zynqmp_qspi_genfifo_cmd(priv);
> >> > + else
> >> > + ret = -1;
> >> > + } else {
> >> > + if (priv->tx_buf)
> >> > + ret = zynqmp_qspi_genfifo_fill_tx(priv);
> >> > + else if (priv->rx_buf)
> >> > + ret = zynqmp_qspi_genfifo_fill_rx(priv);
> >> > + else
> >> > + ret = -1;
> >> > + }
> >> > + return ret;
> >> > +}
> >> > +
> >> > +static int zynqmp_qspi_transfer(struct zynqmp_qspi_priv *priv) {
> >> > + static unsigned int cs_change = 1;
> >> > + int status = 0;
> >> > +
> >> > + debug("%s\n", __func__);
> >> > +
> >> > + while (1) {
> >> > + /* Select the chip if required */
> >> > + if (cs_change)
> >> > + zynqmp_qspi_chipselect(priv, 1);
> >> > +
> >> > + cs_change = priv->cs_change;
> >> > +
> >> > + if (!priv->tx_buf && !priv->rx_buf && priv->len) {
> >> > + status = -1;
> >> > + break;
> >> > + }
> >> > +
> >> > + /* Request the transfer */
> >> > + if (priv->len) {
> >> > + status = zynqmp_qspi_start_transfer(priv);
> >> > + priv->is_inst = 0;
> >> > + if (status < 0)
> >> > + break;
> >> > + }
> >> > +
> >> > + if (cs_change)
> >> > + /* Deselect the chip */
> >> > + zynqmp_qspi_chipselect(priv, 0);
> >> > + break;
> >> > + }
> >> > +
> >> > + return status;
> >> > +}
> >> > +
> >> > +static int zynqmp_qspi_claim_bus(struct udevice *dev) {
> >> > + struct udevice *bus = dev->parent;
> >> > + struct zynqmp_qspi_priv *priv = dev_get_priv(bus);
> >> > + struct zynqmp_qspi_regs *regs = priv->regs;
> >> > +
> >> > + debug("%s\n", __func__);
> >>
> >> drop this.
> > I didn’t see any problem in having, its just for debugging.
> > If you don’t like I can remove.
>
> Yes drop it.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
