RE: [PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller
Hi Boris, > -Original Message- > From: Boris Brezillon [mailto:bbrezil...@kernel.org] > Sent: Monday, January 14, 2019 2:08 PM > To: Schrempf Frieder > Cc: Yogesh Narayan Gaur ; linux- > m...@lists.infradead.org; boris.brezil...@bootlin.com; marek.va...@gmail.com; > broo...@kernel.org; linux-...@vger.kernel.org; devicet...@vger.kernel.org; > mark.rutl...@arm.com; r...@kernel.org; linux-kernel@vger.kernel.org; > computersforpe...@gmail.com; shawn...@kernel.org; linux-arm- > ker...@lists.infradead.org > Subject: Re: [PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI > controller > > On Thu, 10 Jan 2019 17:28:57 + > Schrempf Frieder wrote: > > > Hi Yogesh, > > > > my comments below are mainly about things I already mentioned in my > > review for v5 and about removing or simplifying some unnecessary or > > complex code. > > [...] > > > > Once you've addressed all of Frieder's comments you can add > > Reviewed-by: Boris Brezillon > Thanks. I have send next version, v7, of the patch series having added code changes as per Frieder's comments. [1] Based on the feedback from Frieder, would add yours r-o-b tag. -- Regards Yogesh Gaur [1] https://patchwork.ozlabs.org/project/linux-mtd/list/?series=86130 > Regards, > > Boris
Re: [PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller
On Thu, 10 Jan 2019 17:28:57 +
Schrempf Frieder wrote:
> Hi Yogesh,
>
> my comments below are mainly about things I already mentioned in my
> review for v5 and about removing or simplifying some unnecessary or
> complex code.
>
> Also as I gathered from your conversation with Boris, there's still a
> check for the length of the requested memory missing.
>
> On 08.01.19 10:24, Yogesh Narayan Gaur wrote:
> [...]
> > +
> > +static bool nxp_fspi_supports_op(struct spi_mem *mem,
> > +const struct spi_mem_op *op)
> > +{
> > + struct nxp_fspi *f = spi_controller_get_devdata(mem->spi->master);
> > + int ret;
> > +
> > + ret = nxp_fspi_check_buswidth(f, op->cmd.buswidth);
> > +
> > + if (op->addr.nbytes)
> > + ret |= nxp_fspi_check_buswidth(f, op->addr.buswidth);
> > +
> > + if (op->dummy.nbytes)
> > + ret |= nxp_fspi_check_buswidth(f, op->dummy.buswidth);
> > +
> > + if (op->data.nbytes)
> > + ret |= nxp_fspi_check_buswidth(f, op->data.buswidth);
> > +
> > + if (ret)
> > + return false;
> > +
> > + /*
> > +* The number of instructions needed for the op, needs
> > +* to fit into a single LUT entry.
> > +*/
> > + if (op->addr.nbytes +
> > + (op->dummy.nbytes ? 1:0) +
> > + (op->data.nbytes ? 1:0) > 6)
> > + return false;
>
> Actually this check was only needed in the QSPI driver, as we were using
> LUT_MODE and there we needed one instruction for each address byte.
> Here the number of instructions will always fit into one LUT entry.
>
> Instead of this, a check for op->addr.nbytes > 4 (as already suggested
> in my comments for v5) would make more sense. So we can make sure that
> the address length passed in is supported (between 1 and 4 bytes).
>
> > +
> > + /* Max 64 dummy clock cycles supported */
> > + if (op->dummy.buswidth &&
> > + (op->dummy.nbytes * 8 / op->dummy.buswidth > 64))
> > + return false;
> > +
> > + /* Max data length, check controller limits and alignment */
> > + if (op->data.dir == SPI_MEM_DATA_IN &&
> > + (op->data.nbytes > f->devtype_data->ahb_buf_size ||
> > +(op->data.nbytes > f->devtype_data->rxfifo - 4 &&
> > + !IS_ALIGNED(op->data.nbytes, 8
> > + return false;
> > +
> > + if (op->data.dir == SPI_MEM_DATA_OUT &&
> > + op->data.nbytes > f->devtype_data->txfifo)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> [...]
> > +static void nxp_fspi_select_mem(struct nxp_fspi *f, struct spi_device *spi)
> > +{
> > + unsigned long rate = spi->max_speed_hz;
> > + int ret;
> > + uint64_t size_kb;
> > +
> > + /*
> > +* Return, if previously selected slave device is same as current
> > +* requested slave device.
> > +*/
> > + if (f->selected == spi->chip_select)
> > + return;
> > +
> > + /* Reset FLSHxxCR0 registers */
> > + fspi_writel(f, 0, f->iobase + FSPI_FLSHA1CR0);
> > + fspi_writel(f, 0, f->iobase + FSPI_FLSHA2CR0);
> > + fspi_writel(f, 0, f->iobase + FSPI_FLSHB1CR0);
> > + fspi_writel(f, 0, f->iobase + FSPI_FLSHB2CR0);
> > +
> > + /* Assign controller memory mapped space as size, KBytes, of flash. */
> > + size_kb = FSPI_FLSHXCR0_SZ(f->memmap_phy_size);
> > +
> > + switch (spi->chip_select) {
> > + case 0:
> > + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0);
> > + break;
> > + case 1:
> > + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA2CR0);
> > + break;
> > + case 2:
> > + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHB1CR0);
> > + break;
> > + case 3:
> > + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHB2CR0);
> > + break;
> > + }
>
> The switch statement above can be replaced by:
>
> fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0 +
> 4 * spi->chip_select);
>
> > +
> > + dev_dbg(f->dev, "Slave device [CS:%x] selected\n", spi->chip_select);
> > +
> > + nxp_fspi_clk_disable_unprep(f);
> > +
> > + ret = clk_set_rate(f->clk, rate);
> > + if (ret)
> > + return;
> > +
> > + ret = nxp_fspi_clk_prep_enable(f);
> > + if (ret)
> > + return;
> > +
> > + f->selected = spi->chip_select;
> > +}
> > +
> > +static void nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op
> > *op)
> > +{
> > + u32 len = op->data.nbytes;
> > +
> > + /* Read out the data directly from the AHB buffer. */
> > + memcpy_fromio(op->data.buf.in, (f->ahb_addr + op->addr.val), len);
> > +}
> > +
> > +static void nxp_fspi_fill_txfifo(struct nxp_fspi *f,
> > +const struct spi_mem_op *op)
> > +{
> > + void __iomem *base = f->iobase;
> > + int i, j, ret;
> > + int size, tmp, wm_size;
> > + u32 data = 0;
> > + u32 *txbuf = (u32 *) op->data.buf.out;
>
> You can cast the u8 buffer to u32 and increment it by 1 in each cycle
> below, or you can just use the u8 buffe
Re: [PATCH v6 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller
Hi Yogesh,
my comments below are mainly about things I already mentioned in my
review for v5 and about removing or simplifying some unnecessary or
complex code.
Also as I gathered from your conversation with Boris, there's still a
check for the length of the requested memory missing.
On 08.01.19 10:24, Yogesh Narayan Gaur wrote:
[...]
> +
> +static bool nxp_fspi_supports_op(struct spi_mem *mem,
> + const struct spi_mem_op *op)
> +{
> + struct nxp_fspi *f = spi_controller_get_devdata(mem->spi->master);
> + int ret;
> +
> + ret = nxp_fspi_check_buswidth(f, op->cmd.buswidth);
> +
> + if (op->addr.nbytes)
> + ret |= nxp_fspi_check_buswidth(f, op->addr.buswidth);
> +
> + if (op->dummy.nbytes)
> + ret |= nxp_fspi_check_buswidth(f, op->dummy.buswidth);
> +
> + if (op->data.nbytes)
> + ret |= nxp_fspi_check_buswidth(f, op->data.buswidth);
> +
> + if (ret)
> + return false;
> +
> + /*
> + * The number of instructions needed for the op, needs
> + * to fit into a single LUT entry.
> + */
> + if (op->addr.nbytes +
> +(op->dummy.nbytes ? 1:0) +
> +(op->data.nbytes ? 1:0) > 6)
> + return false;
Actually this check was only needed in the QSPI driver, as we were using
LUT_MODE and there we needed one instruction for each address byte.
Here the number of instructions will always fit into one LUT entry.
Instead of this, a check for op->addr.nbytes > 4 (as already suggested
in my comments for v5) would make more sense. So we can make sure that
the address length passed in is supported (between 1 and 4 bytes).
> +
> + /* Max 64 dummy clock cycles supported */
> + if (op->dummy.buswidth &&
> + (op->dummy.nbytes * 8 / op->dummy.buswidth > 64))
> + return false;
> +
> + /* Max data length, check controller limits and alignment */
> + if (op->data.dir == SPI_MEM_DATA_IN &&
> + (op->data.nbytes > f->devtype_data->ahb_buf_size ||
> + (op->data.nbytes > f->devtype_data->rxfifo - 4 &&
> + !IS_ALIGNED(op->data.nbytes, 8
> + return false;
> +
> + if (op->data.dir == SPI_MEM_DATA_OUT &&
> + op->data.nbytes > f->devtype_data->txfifo)
> + return false;
> +
> + return true;
> +}
> +
[...]
> +static void nxp_fspi_select_mem(struct nxp_fspi *f, struct spi_device *spi)
> +{
> + unsigned long rate = spi->max_speed_hz;
> + int ret;
> + uint64_t size_kb;
> +
> + /*
> + * Return, if previously selected slave device is same as current
> + * requested slave device.
> + */
> + if (f->selected == spi->chip_select)
> + return;
> +
> + /* Reset FLSHxxCR0 registers */
> + fspi_writel(f, 0, f->iobase + FSPI_FLSHA1CR0);
> + fspi_writel(f, 0, f->iobase + FSPI_FLSHA2CR0);
> + fspi_writel(f, 0, f->iobase + FSPI_FLSHB1CR0);
> + fspi_writel(f, 0, f->iobase + FSPI_FLSHB2CR0);
> +
> + /* Assign controller memory mapped space as size, KBytes, of flash. */
> + size_kb = FSPI_FLSHXCR0_SZ(f->memmap_phy_size);
> +
> + switch (spi->chip_select) {
> + case 0:
> + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0);
> + break;
> + case 1:
> + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA2CR0);
> + break;
> + case 2:
> + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHB1CR0);
> + break;
> + case 3:
> + fspi_writel(f, size_kb, f->iobase + FSPI_FLSHB2CR0);
> + break;
> + }
The switch statement above can be replaced by:
fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0 +
4 * spi->chip_select);
> +
> + dev_dbg(f->dev, "Slave device [CS:%x] selected\n", spi->chip_select);
> +
> + nxp_fspi_clk_disable_unprep(f);
> +
> + ret = clk_set_rate(f->clk, rate);
> + if (ret)
> + return;
> +
> + ret = nxp_fspi_clk_prep_enable(f);
> + if (ret)
> + return;
> +
> + f->selected = spi->chip_select;
> +}
> +
> +static void nxp_fspi_read_ahb(struct nxp_fspi *f, const struct spi_mem_op
> *op)
> +{
> + u32 len = op->data.nbytes;
> +
> + /* Read out the data directly from the AHB buffer. */
> + memcpy_fromio(op->data.buf.in, (f->ahb_addr + op->addr.val), len);
> +}
> +
> +static void nxp_fspi_fill_txfifo(struct nxp_fspi *f,
> + const struct spi_mem_op *op)
> +{
> + void __iomem *base = f->iobase;
> + int i, j, ret;
> + int size, tmp, wm_size;
> + u32 data = 0;
> + u32 *txbuf = (u32 *) op->data.buf.out;
You can cast the u8 buffer to u32 and increment it by 1 in each cycle
below, or you can just use the u8 buffer and align and increment by 8 as
I did in my proposal for v5.
I still like my version better as it seems simpler and easier to
understand ;)
> +
> + /* clear the TX FIFO. */
> + fspi_writ
