[U-Boot] [PATCH v4 01/16] spi: Add zynq qspi controller driver

[Hannes Schmelzer]

Hi Jagan,

during bringing up QSPI within SPL on my ZYNQ ZC702 board i made some 
review of your code.
Have a look.

On 01.09.2015 08:11, Jagan Teki wrote:
> Added zynq qspi controller driver for Xilinx Zynq APSOC,
> this driver is driver-model driven with devicetree support.
(...)
> +
> +/* zynq qspi priv */
> +struct zynq_qspi_priv {
> +	struct zynq_qspi_regs *regs;
> +	u8 cs;
> +	u8 mode;
> +	u8 fifo_depth;
> +	u32 freq;		/* required frequency */
> +	const void *tx_buf;
> +	void *rx_buf;
> +	unsigned len;
> +	int bytes_to_transfer;
> +	int bytes_to_receive;
> +	unsigned int is_inst;
> +	unsigned cs_change:1;
> +};
why not use "u32" for len ?
> +static void zynq_qspi_init_hw(struct zynq_qspi_priv *priv)
> +{
> +	struct zynq_qspi_regs *regs = priv->regs;
> +	u32 confr;
During bring up this driver within SPL i've figured out, that it is very 
important to do a clear reset to the QSPI unit.
Initially the ROM codes fetches our SPL-binary from the flash into the 
OCM and executes.
We don't know at this point how ROM-code has left the QSPI unit, and 
have to reset the Unit using responsible Bits in sclr area.
Otherwise we can see strange behaviours like hang on reading RxFIFO 
sometimes and sometimes not.
> +
> +	/* Disable QSPI */
> +	writel(~ZYNQ_QSPI_ENR_SPI_EN_MASK, &regs->enr);
> +
> +	/* Disable Interrupts */
> +	writel(ZYNQ_QSPI_IXR_ALL_MASK, &regs->idr);
> +
> +	/* Clear the TX and RX threshold reg */
> +	writel(ZYNQ_QSPI_TXFIFO_THRESHOLD, &regs->txftr);
> +	writel(ZYNQ_QSPI_RXFIFO_THRESHOLD, &regs->rxftr);
> +
> +	/* Clear the RX FIFO */
> +	while (readl(&regs->isr) & ZYNQ_QSPI_IXR_RXNEMPTY_MASK)
> +		readl(&regs->drxr);
> +
> +	/* Clear Interrupts */
> +	writel(ZYNQ_QSPI_IXR_ALL_MASK, &regs->isr);
> +
> +	/* Manual slave select and Auto start */
> +	confr = readl(&regs->cr);
> +	confr &= ~ZYNQ_QSPI_CR_MSA_MASK;
> +	confr |= ZYNQ_QSPI_CR_IFMODE_MASK | ZYNQ_QSPI_CR_MCS_MASK |
> +		ZYNQ_QSPI_CR_PCS_MASK | ZYNQ_QSPI_CR_FW_MASK |
> +		ZYNQ_QSPI_CR_MSTREN_MASK;
> +	writel(confr, &regs->cr);
> +
> +	/* Enable SPI */
> +	writel(ZYNQ_QSPI_ENR_SPI_EN_MASK, &regs->enr);
> +}
(...)
> +
> +/*
> + * zynq_qspi_read_data - Copy data to RX buffer
> + * @zqspi:	Pointer to the zynq_qspi structure
> + * @data:	The 32 bit variable where data is stored
> + * @size:	Number of bytes to be copied from data to RX buffer
> + */
> +static void zynq_qspi_read_data(struct zynq_qspi_priv *priv, u32 data, u8 size)
> +{
> +	u8 byte3;
> +
> +	debug("%s: data 0x%04x rx_buf addr: 0x%08x size %d\n", __func__ ,
> +	      data, (unsigned)(priv->rx_buf), size);
use 0x%p instead 0x%08x to display pointer addresses, with the advantage 
that no cast is necessary.
> +
> +	if (priv->rx_buf) {
> +		switch (size) {
> +		case 1:
> +			*((u8 *)priv->rx_buf) = data;
> +			priv->rx_buf += 1;
> +			break;
> +		case 2:
> +			*((u16 *)priv->rx_buf) = data;
> +			priv->rx_buf += 2;
> +			break;
> +		case 3:
> +			*((u16 *)priv->rx_buf) = data;
> +			priv->rx_buf += 2;
> +			byte3 = (u8)(data >> 16);
> +			*((u8 *)priv->rx_buf) = byte3;
> +			priv->rx_buf += 1;
> +			break;
> +		case 4:
> +			/* Can not assume word aligned buffer */
> +			memcpy(priv->rx_buf, &data, size);
> +			priv->rx_buf += 4;
> +			break;
> +		default:
> +			/* This will never execute */
> +			break;
> +		}
> +	}
> +	priv->bytes_to_receive -= size;
> +	if (priv->bytes_to_receive < 0)
> +		priv->bytes_to_receive = 0;
> +}
wouldn't it be good enough using always "memcpy" ?
maybe we can drop this function completely ?
> +
> +/*
> + * zynq_qspi_write_data - Copy data from TX buffer
> + * @zqspi:	Pointer to the zynq_qspi structure
> + * @data:	Pointer to the 32 bit variable where data is to be copied
> + * @size:	Number of bytes to be copied from TX buffer to data
> + */
> +static void zynq_qspi_write_data(struct  zynq_qspi_priv *priv,
> +		u32 *data, u8 size)
> +{
> +	if (priv->tx_buf) {
> +		switch (size) {
> +		case 1:
> +			*data = *((u8 *)priv->tx_buf);
> +			priv->tx_buf += 1;
> +			*data |= 0xFFFFFF00;
> +			break;
> +		case 2:
> +			*data = *((u16 *)priv->tx_buf);
> +			priv->tx_buf += 2;
> +			*data |= 0xFFFF0000;
> +			break;
> +		case 3:
> +			*data = *((u16 *)priv->tx_buf);
> +			priv->tx_buf += 2;
> +			*data |= (*((u8 *)priv->tx_buf) << 16);
> +			priv->tx_buf += 1;
> +			*data |= 0xFF000000;
> +			break;
> +		case 4:
> +			/* Can not assume word aligned buffer */
> +			memcpy(data, priv->tx_buf, size);
> +			priv->tx_buf += 4;
> +			break;
> +		default:
> +			/* This will never execute */
> +			break;
> +		}
> +	} else {
> +		*data = 0;
> +	}
> +
> +	debug("%s: data 0x%08x tx_buf addr: 0x%08x size %d\n", __func__,
> +	      *data, (u32)priv->tx_buf, size);
> +
> +	priv->bytes_to_transfer -= size;
> +	if (priv->bytes_to_transfer < 0)
> +		priv->bytes_to_transfer = 0;
> +}
maybe same thing here as above during "zynq_qspi_read_data".
> +
> +static void zynq_qspi_chipselect(struct  zynq_qspi_priv *priv, int is_on)
> +{
> +	u32 confr;
> +	struct zynq_qspi_regs *regs = priv->regs;
> +
> +	confr = readl(&regs->cr);
> +
> +	if (is_on) {
> +		/* Select the slave */
> +		confr &= ~ZYNQ_QSPI_CR_SS_MASK;
> +		confr |= (~(1 << priv->cs) << ZYNQ_QSPI_CR_SS_SHIFT) &
> +					ZYNQ_QSPI_CR_SS_MASK;
in TRM (UG585, V1.1) pg. 1525, these bits 13..11 are described as 
reserved, only bit 10 (if priv->cs == 0) is allowed to write.
For my reading we actually have only one CS.

So:

+	if (is_on) {
+		/* Select the slave */
+		confr &= ~(1 << ZYNQ_QSPI_CR_SS_SHIFT);
+	}
would be enough.

Or do we use here some undocumented feature ?
> +	} else
> +		/* Deselect the slave */
> +		confr |= ZYNQ_QSPI_CR_SS_MASK;
> +
> +	writel(confr, &regs->cr);
> +}
> +
> +/*
> + * zynq_qspi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possible
> + * @zqspi:	Pointer to the zynq_qspi structure
> + */
> +static void zynq_qspi_fill_tx_fifo(struct zynq_qspi_priv *priv, u32 size)
> +{
> +	u32 data = 0;
> +	u32 fifocount = 0;
> +	unsigned len, offset;
why not using u32 instead unsigned ?
> +	struct zynq_qspi_regs *regs = priv->regs;
> +	static const unsigned offsets[4] = {
> +		ZYNQ_QSPI_TXD_00_00_OFFSET, ZYNQ_QSPI_TXD_00_01_OFFSET,
> +		ZYNQ_QSPI_TXD_00_10_OFFSET, ZYNQ_QSPI_TXD_00_11_OFFSET };
> +
> +	while ((fifocount < size) &&
> +			(priv->bytes_to_transfer > 0)) {
> +		if (priv->bytes_to_transfer >= 4) {
> +			if (priv->tx_buf) {
> +				memcpy(&data, priv->tx_buf, 4);
> +				priv->tx_buf += 4;
> +			} else {
> +				data = 0;
> +			}
> +			writel(data, &regs->txd0r);
> +			priv->bytes_to_transfer -= 4;
> +			fifocount++;
> +		} else {
> +			/* Write TXD1, TXD2, TXD3 only if TxFIFO is empty. */
> +			if (!(readl(&regs->isr)
> +					& ZYNQ_QSPI_IXR_TXOW_MASK) &&
> +					!priv->rx_buf)
> +				return;
would suggest "continue" instead "return", otherwise we may loose data 
in case of busy TX-FIFO.
> +			len = priv->bytes_to_transfer;
> +			zynq_qspi_write_data(priv, &data, len);
> +			offset = (priv->rx_buf) ? offsets[0] : offsets[len];
> +			writel(data, &regs->cr + (offset / 4));
during review i understood my question from yesterday :-) sorry for noise.
> +		}
> +	}
> +}
> +
> +/*
> + * zynq_qspi_irq_poll - Interrupt service routine of the QSPI controller
> + * @zqspi:	Pointer to the zynq_qspi structure
> + *
> + * This function handles TX empty and Mode Fault interrupts only.
> + * On TX empty interrupt this function reads the received data from RX FIFO and
> + * fills the TX FIFO if there is any data remaining to be transferred.
> + * On Mode Fault interrupt this function indicates that transfer is completed,
> + * the SPI subsystem will identify the error as the remaining bytes to be
> + * transferred is non-zero.
> + *
> + * returns:	0 for poll timeout
> + *		1 transfer operation complete
> + */
> +static int zynq_qspi_irq_poll(struct zynq_qspi_priv *priv)
> +{
> +	struct zynq_qspi_regs *regs = priv->regs;
> +	u32 rxindex = 0;
> +	u32 rxcount;
> +	u32 status, timeout;
> +
> +	/* Poll until any of the interrupt status bits are set */
> +	timeout = get_timer(0);
> +	do {
> +		status = readl(&regs->isr);
> +	} while ((status == 0) &&
> +		(get_timer(timeout) < CONFIG_SYS_ZYNQ_QSPI_WAIT));
> +
> +	if (status == 0) {
> +		printf("zynq_qspi_irq_poll: Timeout!\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	writel(status, &regs->isr);
> +
> +	/* Disable all interrupts */
> +	writel(ZYNQ_QSPI_IXR_ALL_MASK, &regs->idr);
> +	if ((status & ZYNQ_QSPI_IXR_TXOW_MASK) ||
> +	    (status & ZYNQ_QSPI_IXR_RXNEMPTY_MASK)) {
> +		/*
> +		 * This bit is set when Tx FIFO has < THRESHOLD entries. We have
> +		 * the THRESHOLD value set to 1, so this bit indicates Tx FIFO
> +		 * is empty
> +		 */
> +		rxcount = priv->bytes_to_receive - priv->bytes_to_transfer;
> +		rxcount = (rxcount % 4) ? ((rxcount/4)+1) : (rxcount/4);
> +		while ((rxindex < rxcount) &&
> +				(rxindex < ZYNQ_QSPI_RXFIFO_THRESHOLD)) {
> +			/* Read out the data from the RX FIFO */
> +			u32 data;
> +			data = readl(&regs->drxr);
> +
> +			if (priv->bytes_to_receive >= 4) {
> +				if (priv->rx_buf) {
> +					memcpy(priv->rx_buf, &data, 4);
> +					priv->rx_buf += 4;
> +				}
> +				priv->bytes_to_receive -= 4;
> +			} else {
> +				zynq_qspi_read_data(priv, data,
> +						    priv->bytes_to_receive);
> +			}
> +			rxindex++;
> +		}
> +
> +		if (priv->bytes_to_transfer) {
> +			/* There is more data to send */
> +			zynq_qspi_fill_tx_fifo(priv,
> +					       ZYNQ_QSPI_RXFIFO_THRESHOLD);
> +
> +			writel(ZYNQ_QSPI_IXR_ALL_MASK, &regs->ier);
> +		} else {
> +			/*
> +			 * If transfer and receive is completed then only send
> +			 * complete signal
> +			 */
> +			if (!priv->bytes_to_receive) {
> +				/* return operation complete */
> +				writel(ZYNQ_QSPI_IXR_ALL_MASK,
> +				       &regs->idr);
> +				return 1;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * zynq_qspi_start_transfer - Initiates the QSPI transfer
> + * @qspi:	Pointer to the spi_device structure
> + * @transfer:	Pointer to the spi_transfer structure which provide information
> + *		about next transfer parameters
> + *
> + * This function fills the TX FIFO, starts the QSPI transfer, and waits for the
> + * transfer to be completed.
> + *
> + * returns:	Number of bytes transferred in the last transfer
> + */
> +static int zynq_qspi_start_transfer(struct zynq_qspi_priv *priv)
> +{
> +	u32 data = 0;
> +	struct zynq_qspi_regs *regs = priv->regs;
> +
> +	debug("%s: qspi: 0x%08x transfer: 0x%08x len: %d\n", __func__,
> +	      (u32)priv, (u32)priv, priv->len);
use 0x%p for displaying pointer addresses.
> +
> +	priv->bytes_to_transfer = priv->len;
> +	priv->bytes_to_receive = priv->len;
> +
> +	if (priv->len < 4)
> +		zynq_qspi_fill_tx_fifo(priv, priv->len);
> +	else
> +		zynq_qspi_fill_tx_fifo(priv, priv->fifo_depth);
i think we can call the function always with "priv->fifo_depth",
because the decision if the "size" parameter is used or not is there 
made based on priv->bytes_to_transfer.
> +
> +	writel(ZYNQ_QSPI_IXR_ALL_MASK, &regs->ier);
> +	/* Start the transfer by enabling manual start bit */
this comment is confusing since we are in auto-mode with manually forced 
chipselect.
> +
> +	/* wait for completion */
> +	do {
> +		data = zynq_qspi_irq_poll(priv);
> +	} while (data == 0);
> +
> +	return (priv->len) - (priv->bytes_to_transfer);
> +}
best regards,
Hannes