From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hannes Schmelzer Date: Thu, 05 Nov 2015 08:33:42 +0100 Subject: [U-Boot] [PATCH v4 01/16] spi: Add zynq qspi controller driver In-Reply-To: <561F6625.30901@schmelzer.or.at> References: <1441087907-25993-1-git-send-email-jteki@openedev.com> <1441087907-25993-2-git-send-email-jteki@openedev.com> <561F6625.30901@schmelzer.or.at> Message-ID: <563B0656.9000308@schmelzer.or.at> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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, ®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 >