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

Hannes Schmelzer
2015-11-05T07:33:42Z

Hi Jagan,

did you take notice about that?
Maybe i've not seen your answer.

regards,
Hannes

On 15.10.2015 10:39, Hannes Schmelzer wrote:
>
> 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
>