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

[thomas.langer at lantiq.com]

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