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

[Hannes Schmelzer <hannes@schmelzer.or.at>]

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
>