On Mon, 10 Dec 2018 09:41:51 +0000
Yogesh Narayan Gaur <yogeshnarayan.gaur@xxxxxxx> wrote:
> > > +/* Instead of busy looping invoke readl_poll_timeout functionality.
> > > +*/ static int fspi_readl_poll_tout(struct nxp_fspi *f, void __iomem *base,
> > > + u32 mask, u32 delay_us,
> > > + u32 timeout_us, bool condition)
> > > +{
> > > + u32 reg;
> > > +
> > > + if (!f->devtype_data->little_endian)
> > > + mask = (u32)cpu_to_be32(mask);
> > > +
> > > + if (condition)
> > > + return readl_poll_timeout(base, reg, (reg & mask),
> > > + delay_us, timeout_us);
> > > + else
> > > + return readl_poll_timeout(base, reg, !(reg & mask),
> > > + delay_us, timeout_us);
> >
> > I would rather use a local variable to store the condition:
> >
> > bool c = condition ? (reg & mask):!(reg & mask);
> >
> With these type of usage getting below warning messages.
>
> drivers/spi/spi-nxp-fspi.c: In function ‘fspi_readl_poll_tout.isra.10.constprop’:
> drivers/spi/spi-nxp-fspi.c:446:21: warning: ‘reg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> bool cn = c ? (reg & mask) : !(reg & mask);
>
> If assign value to reg = 0xffffffff then timeout is start getting hit for False case and if assign value 0 then start getting timeout hit for true case.
>
> I would rather not try to modify this function.
I agree. Let's keep this function readable even if this implies
duplicating a few lines of code.
>
> > return readl_poll_timeout(base, reg, c, delay_us, timeout_us);
> >
> > > +}
> > > +
> > > +/*
> > > + * If the slave device content being changed by Write/Erase, need to
> > > + * invalidate the AHB buffer. This can be achieved by doing the reset
> > > + * of controller after setting MCR0[SWRESET] bit.
> > > + */
> > > +static inline void nxp_fspi_invalid(struct nxp_fspi *f) {
> > > + u32 reg;
> > > + int ret;
> > > +
> > > + reg = fspi_readl(f, f->iobase + FSPI_MCR0);
> > > + fspi_writel(f, reg | FSPI_MCR0_SWRST, f->iobase + FSPI_MCR0);
> > > +
> > > + /* w1c register, wait unit clear */
> > > + ret = fspi_readl_poll_tout(f, f->iobase + FSPI_MCR0,
> > > + FSPI_MCR0_SWRST, 0, POLL_TOUT, false);
> > > + WARN_ON(ret);
> > > +}
> > > +
> > > +static void nxp_fspi_prepare_lut(struct nxp_fspi *f,
> > > + const struct spi_mem_op *op)
> > > +{
> > > + void __iomem *base = f->iobase;
> > > + u32 lutval[4] = {};
> > > + int lutidx = 1, i;
> > > +
> > > + /* cmd */
> > > + lutval[0] |= LUT_DEF(0, LUT_CMD, LUT_PAD(op->cmd.buswidth),
> > > + op->cmd.opcode);
> > > +
> > > + /* addr bus width */
> > > + if (op->addr.nbytes) {
> > > + u32 addrlen = 0;
> > > +
> > > + switch (op->addr.nbytes) {
> > > + case 1:
> > > + addrlen = ADDR8BIT;
> > > + break;
> > > + case 2:
> > > + addrlen = ADDR16BIT;
> > > + break;
> > > + case 3:
> > > + addrlen = ADDR24BIT;
> > > + break;
> > > + case 4:
> > > + addrlen = ADDR32BIT;
> > > + break;
> > > + default:
> > > + dev_err(f->dev, "In-correct address length\n");
> > > + return;
> > > + }
> >
> > You don't need to validate op->addr.nbytes here, this is already done in
> > nxp_fspi_supports_op().
>
> Yes, I need to validate op->addr.nbytes else LUT would going to be programmed for 0 addrlen.
> I have checked this on the target.
Also agree there. Some operations have 0 address bytes. We could also
test addr.buswidth, but I'm fine with the addr.nbytes test too.
> > > +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);
> >
> Above description of this function, explains the reason for using memmap_phy_size.
> This is not the arbitrary size, but the memory mapped size being assigned to the controller.
>
> > You are still using memory of arbitrary size (memmap_phy_size) for mapping the
> > flash. Why not use the same approach as in the QSPI driver and just map
> > ahb_buf_size until we implement the dirmap API?
> The approach which being used in QSPI driver didn't work here, I have tried with that.
> In QSPI driver, while preparing LUT we are assigning read/write address in the LUT preparation and have to for some unknown hack have to provide macro for LUT_MODE instead of LUT_ADDR.
> But this thing didn't work for FlexSPI.
> I discussed with HW IP owner and they suggested only to use LUT_ADDR for specifying the address length of the command i.e. 3-byte or 4-byte address command (NOR) or 1-2 byte address command for NAND.
Actually, we would have used a LUT_ADDR too if the QSPI IP was support
ADDR instructions with a number of bytes < 3, but for some unknown
reasons it does not work.
>
> Thus, in LUT preparation we have assigned only the base address.
> Now if I have assigned ahb_buf_size to FSPI_FLSHXXCR0 register then for read/write data beyond limit of ahb_buf_size offset I get data corruption.
Why would you do that? We have the ->adjust_op_size() exactly for this
reason, so, if someone tries to do a spi_mem_op with data.nbytes >
ahb_buf_size you should return an error.
>
> Thus, for generic approach have assigned FSPI_FLSHXXCR0 equal to the memory mapped size to the controller. This would also not going to depend on the number of CS present on the target.
I kind of agree with Frieder on that one, I think it's preferable to
limit the per-read-op size to ahb_buf_size and let the upper layer
split the request in several sub-requests. On the controller side of
things, you just have to have a mapping of ahb_buf_size per-CS. If you
want to further optimize things, implement the dirmap hooks.
>
> > You are already aligning the AHB reads for this in nxp_fspi_adjust_op_size().
> >
> Yes, max read data size can be ahb_buf_size. Thus we need to check max read size with ahb_buf_size.
Well, it's never a bad thing to check it twice, just in case the
spi-mem user is misusing the API.
> > > +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_size, wm_size;
> > > + u32 data = 0;
> > > + u32 *txbuf = (u32 *) op->data.buf.out;
> > > +
> > > + /* clear the TX FIFO. */
> > > + fspi_writel(f, FSPI_IPTXFCR_CLR, base + FSPI_IPTXFCR);
> > > +
> > > + /* Default value of water mark level is 8 bytes. */
> > > + wm_size = 8;
> > > + size = op->data.nbytes / wm_size;
> > > + for (i = 0; i < size; i++) {
> > > + /* Wait for TXFIFO empty */
> > > + ret = fspi_readl_poll_tout(f, f->iobase + FSPI_INTR,
> > > + FSPI_INTR_IPTXWE, 0,
> > > + POLL_TOUT, true);
> > > + WARN_ON(ret);
> > > +
> > > + j = 0;
> > > + tmp_size = wm_size;
> > > + while (tmp_size > 0) {
> > > + data = 0;
> > > + memcpy(&data, txbuf, 4);
> > > + fspi_writel(f, data, base + FSPI_TFDR + j * 4);
> > > + tmp_size -= 4;
> > > + j++;
> > > + txbuf += 1;
> > > + }
> > > + fspi_writel(f, FSPI_INTR_IPTXWE, base + FSPI_INTR);
> > > + }
> > > +
> > > + size = op->data.nbytes % wm_size;
> > > + if (size) {
> > > + /* Wait for TXFIFO empty */
> > > + ret = fspi_readl_poll_tout(f, f->iobase + FSPI_INTR,
> > > + FSPI_INTR_IPTXWE, 0,
> > > + POLL_TOUT, true);
> > > + WARN_ON(ret);
> > > +
> > > + j = 0;
> > > + tmp_size = 0;
> > > + while (size > 0) {
> > > + data = 0;
> > > + tmp_size = (size < 4) ? size : 4;
> > > + memcpy(&data, txbuf, tmp_size);
> > > + fspi_writel(f, data, base + FSPI_TFDR + j * 4);
> > > + size -= tmp_size;
> > > + j++;
> > > + txbuf += 1;
> > > + }
> > > + fspi_writel(f, FSPI_INTR_IPTXWE, base + FSPI_INTR);
> > > + }
> >
> > All these nested loops to fill the TX buffer and also the ones below to read the
> > RX buffer look much more complicated than they should really be. Can you try to
> > make this more readable?
> Yes
> >
> > Maybe something like this would work:
> >
> > for (i = 0; i < ALIGN_DOWN(op->data.nbytes, 8); i += 8) {
> > /* Wait for TXFIFO empty */
> > ret = fspi_readl_poll_tout(f, f->iobase + FSPI_INTR,
> > FSPI_INTR_IPTXWE, 0,
> > POLL_TOUT, true);
> >
> > fspi_writel(f, op->data.buf.out + i, base + FSPI_TFDR);
> > fspi_writel(f, op->data.buf.out + i + 4, base + FSPI_TFDR + 4);
> > fspi_writel(f, FSPI_INTR_IPTXWE, base + FSPI_INTR); }
> With this above 2 lines we are hardcoding it for read/write with watermark size as 8 bytes.
> Watermark size can be variable and depends on the value of IPRXFCR/IPTXFCR register with default value as 8 bytes
> Thus, I would still prefer to use the internal for loop instead of 2 fspi_writel(...) for FSPI_TFDR and FSPI_TFDR + 4 register write commands.
Just like you're hardcoding wm_size to 8, so I don't see a difference
here. And I indeed prefer Frieder's version.
