From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jagan Teki Date: Thu, 5 Nov 2015 13:26:56 +0530 Subject: [U-Boot] [PATCH v4 01/16] spi: Add zynq qspi controller driver In-Reply-To: <563B0656.9000308@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> <563B0656.9000308@schmelzer.or.at> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 5 November 2015 at 13:03, Hannes Schmelzer 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.