[U-Boot] [PATCH v4 01/16] spi: Add zynq qspi controller driver
Jagan Teki
jteki at openedev.com
Thu Nov 5 08:56:56 CET 2015
On 5 November 2015 at 13:03, Hannes Schmelzer <hannes at schmelzer.or.at> wrote:
> Hi Jagan,
>
> did you take notice about that?
> Maybe i've not seen your answer.
I will get my hardware next week, sure we can discuss this.
>
>
> 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, ®s->enr);
>>> +
>>> + /* Disable Interrupts */
>>> + writel(ZYNQ_QSPI_IXR_ALL_MASK, ®s->idr);
>>> +
>>> + /* Clear the TX and RX threshold reg */
>>> + writel(ZYNQ_QSPI_TXFIFO_THRESHOLD, ®s->txftr);
>>> + writel(ZYNQ_QSPI_RXFIFO_THRESHOLD, ®s->rxftr);
>>> +
>>> + /* Clear the RX FIFO */
>>> + while (readl(®s->isr) & ZYNQ_QSPI_IXR_RXNEMPTY_MASK)
>>> + readl(®s->drxr);
>>> +
>>> + /* Clear Interrupts */
>>> + writel(ZYNQ_QSPI_IXR_ALL_MASK, ®s->isr);
>>> +
>>> + /* Manual slave select and Auto start */
>>> + confr = readl(®s->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, ®s->cr);
>>> +
>>> + /* Enable SPI */
>>> + writel(ZYNQ_QSPI_ENR_SPI_EN_MASK, ®s->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(®s->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, ®s->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, ®s->txd0r);
>>> + priv->bytes_to_transfer -= 4;
>>> + fifocount++;
>>> + } else {
>>> + /* Write TXD1, TXD2, TXD3 only if TxFIFO is empty. */
>>> + if (!(readl(®s->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, ®s->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(®s->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, ®s->isr);
>>> +
>>> + /* Disable all interrupts */
>>> + writel(ZYNQ_QSPI_IXR_ALL_MASK, ®s->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(®s->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, ®s->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,
>>> + ®s->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, ®s->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
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
--
Jagan | openedev.
More information about the U-Boot
mailing list