Re: [U-Boot] [PATCH 10/34] spi: Add zynq qspi controller driver

Hello Jagan,

I have some comments and questions:

Am 05.11.2013 18:51, schrieb Jagannadha Sutradharudu Teki:

+/* Definitions of the flash commands - Flash insts in ascending order */ +#define ZYNQ_QSPI_FLASH_INST_WRSR 0x01 /* Write status register */ +#define ZYNQ_QSPI_FLASH_INST_PP 0x02 /* Page program */ +#define ZYNQ_QSPI_FLASH_INST_WRDS 0x04 /* Write disable */ +#define ZYNQ_QSPI_FLASH_INST_RDSR1 0x05 /* Read status register 1 */ +#define ZYNQ_QSPI_FLASH_INST_WREN 0x06 /* Write enable */ +#define ZYNQ_QSPI_FLASH_INST_AFR 0x0B /* Fast read data bytes */ +#define ZYNQ_QSPI_FLASH_INST_BE_4K 0x20 /* Erase 4KiB block */ +#define ZYNQ_QSPI_FLASH_INST_RDSR2 0x35 /* Read status register 2 */ +#define ZYNQ_QSPI_FLASH_INST_BE_32K 0x52 /* Erase 32KiB block */ +#define ZYNQ_QSPI_FLASH_INST_RDID 0x9F /* Read JEDEC ID */ +#define ZYNQ_QSPI_FLASH_INST_SE 0xD8 /* Sector erase (usually 64KB)*/

Why needs the spi controller this list of flash commands? It is the job of the flash driver to handle this, the spi controller only forwards this to the devices!

+/* List of all the QSPI instructions and its format */ +static struct zynq_qspi_inst_format flash_inst[] = {

• {ZYNQ_QSPI_FLASH_INST_WRSR, 1, ZYNQ_QSPI_TXD_00_01_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_PP, 4, ZYNQ_QSPI_TXD_00_00_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_WRDS, 1, ZYNQ_QSPI_TXD_00_01_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_RDSR1, 1, ZYNQ_QSPI_TXD_00_01_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_WREN, 1, ZYNQ_QSPI_TXD_00_01_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_AFR, 1, ZYNQ_QSPI_TXD_00_01_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_BE_4K, 4, ZYNQ_QSPI_TXD_00_00_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_RDSR2, 1, ZYNQ_QSPI_TXD_00_01_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_BE_32K, 4, ZYNQ_QSPI_TXD_00_00_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_RDID, 1, ZYNQ_QSPI_TXD_00_01_OFFSET},
• {ZYNQ_QSPI_FLASH_INST_SE, 4, ZYNQ_QSPI_TXD_00_00_OFFSET},
• /* Add all the instructions supported by the flash device */

+};

Is this table used to encode which parts needs DUAL or QUAD transfers? And are you sure the table is complete? The flash drivers don't use more instructions? What happens if some device is connected, which is no flash?

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,

• void *din, unsigned long flags)
  

+{

• struct zynq_qspi_slave *zslave = to_zynq_qspi_slave(slave);
• u32 len = bitlen / 8, tx_tvl;
• u32 buf, status;
• debug("spi_xfer: bus:%i cs:%i bitlen:%i len:%i flags:%lx\n",

•      slave->bus, slave->cs, bitlen, len, flags);
  

• if (bitlen == 0)

• return -1;
  

• if (bitlen % 8) {

• debug("spi_xfer: Non byte aligned SPI transfer\n");
  

• return -1;
  

• }
• if (flags & SPI_XFER_BEGIN)

• spi_cs_activate(slave);
  

• zslave->tx_len = len;
• zslave->rx_len = len;
• zslave->tx_buf = dout;
• zslave->rx_buf = din;
• while (zslave->rx_len > 0) {

• /* Write the data into TX FIFO - tx threshold is fifo_depth */
  

• tx_tvl = 0;
  

• while ((tx_tvl < zslave->fifo_depth) && zslave->tx_len) {
  

• 	if (zynq_qspi_process_tx(zslave) < 0) {
  

• 		flags |= SPI_XFER_END;
  

• 		goto out;
  

• 	}
  

• 	tx_tvl++;
  

• }
  

• /* Check TX FIFO completion */
  

• if (zynq_qspi_check_txfifo(zslave) < 0) {
  

• 	flags |= SPI_XFER_END;
  

• 	goto out;
  

• }
  

• /* Read the data from RX FIFO */
  

• status = readl(&zslave->base->isr);
  

• while (status & ZYNQ_QSPI_IXR_RXNEMPTY_MASK) {
  

• 	buf = readl(&zslave->base->rxdr);
  

• 	if (zslave->rx_len < 4)
  

• 		zynq_qspi_read(zslave, buf, zslave->rx_len);
  

• 	else
  

• 		zynq_qspi_read(zslave, buf, 4);
  

• 	status = readl(&zslave->base->isr);
  

• }
  

• }

+out:

• if (flags & SPI_XFER_END)

• spi_cs_deactivate(slave);
  

• return 0;

+}

In this function I miss the parts, where the caller (e.g. the flash driver) tells the controller to transfer some parts in DUAL or QUAD mode!

Best Regards, Thomas