Hi,
-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Wednesday, May 09, 2018 1:12 PM To: Siva Durga Prasad Paladugu sivadur@xilinx.com Cc: Jagan Teki jagan@amarulasolutions.com; U-Boot-Denx <u- boot@lists.denx.de>; michal.simek@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 sivadur@xilinx.com wrote:
Hi Jagan,
-----Original Message----- From: Jagan Teki [mailto:jagan@amarulasolutions.com] Sent: Wednesday, May 09, 2018 12:01 PM To: Siva Durga Prasad Paladugu sivadur@xilinx.com Cc: U-Boot-Denx u-boot@lists.denx.de; michal.simek@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.paladugu@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@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;elsegen_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);elseret = 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;elsegen_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);elseret = -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);elseret = -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.