Hi Yogesh,
On 10.12.18 10:41, Yogesh Narayan Gaur wrote:
[...]>>> +
>>> +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 */
This comment should match the code below. So maybe only "addr" should be
enough.
>>> + 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.
>
>>
>>> +
>>> + lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_ADDR,
>>> + LUT_PAD(op->addr.buswidth),
>>> + addrlen);
>>
>> You can also just remove the whole switch statement above and use this:
>>
>> lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_ADDR,
>> LUT_PAD(op->addr.buswidth),
>> op->addr.nbytes * 8);
>>
> Ok, would include in next version.
>
>>> + lutidx++;
>>> + }
>>> +
>>> + /* dummy bytes, if needed */
>>> + if (op->dummy.nbytes) {
>>> + lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_DUMMY,
>>> + /*
>>> + * Due to FlexSPI controller limitation number of PAD for
>> dummy
>>> + * buswidth needs to be programmed as equal to data buswidth.
>>> + */
>>> + LUT_PAD(op->data.buswidth),
>>> + op->dummy.nbytes * 8 /
>>> + op->dummy.buswidth);
>>> + lutidx++;
>>> + }
>>> +
>>> + /* read/write data bytes */
>>> + if (op->data.nbytes) {
>>> + lutval[lutidx / 2] |= LUT_DEF(lutidx,
>>> + op->data.dir ==
>> SPI_MEM_DATA_IN ?
>>> + LUT_NXP_READ : LUT_NXP_WRITE,
>>> + LUT_PAD(op->data.buswidth),
>>> + 0);
>>> + lutidx++;
>>> + }
>>> +
>>> + /* stop condition. */
>>> + lutval[lutidx / 2] |= LUT_DEF(lutidx, LUT_STOP, 0, 0);
>>> +
>>> + /* unlock LUT */
>>> + fspi_writel(f, FSPI_LUTKEY_VALUE, f->iobase + FSPI_LUTKEY);
>>> + fspi_writel(f, FSPI_LCKER_UNLOCK, f->iobase + FSPI_LCKCR);
>>> +
>>> + /* fill LUT */
>>> + for (i = 0; i < ARRAY_SIZE(lutval); i++)
>>> + fspi_writel(f, lutval[i], base + FSPI_LUT_REG(i));
>>> +
>>> + dev_dbg(f->dev, "CMD[%x] lutval[0:%x \t 1:%x \t 2:%x \t 3:%x]\n",
>>> + op->cmd.opcode, lutval[0], lutval[1], lutval[2], lutval[3]);
>>> +
>>> + /* lock LUT */
>>> + fspi_writel(f, FSPI_LUTKEY_VALUE, f->iobase + FSPI_LUTKEY);
>>> + fspi_writel(f, FSPI_LCKER_LOCK, f->iobase + FSPI_LCKCR); }
[...]
>>> +
>>> +static int nxp_fspi_exec_op(struct spi_mem *mem, const struct
>>> +spi_mem_op *op) {
>>> + struct nxp_fspi *f = spi_controller_get_devdata(mem->spi->master);
>>> + int err = 0;
>>> +
>>> + mutex_lock(&f->lock);
>>> +
>>> + /* Wait for controller being ready. */
>>> + err = fspi_readl_poll_tout(f, f->iobase + FSPI_STS0,
>>> + FSPI_STS0_ARB_IDLE, 1, POLL_TOUT, true);
>>> + WARN_ON(err);
>>> +
>>> + nxp_fspi_select_mem(f, mem->spi);
>>> +
>>> + nxp_fspi_prepare_lut(f, op);
>>> + /*
>>> + * If we have large chunks of data, we read them through the AHB bus
>>> + * by accessing the mapped memory. In all other cases we use
>>> + * IP commands to access the flash.
>>> + */
>>> + if (op->data.nbytes > (f->devtype_data->rxfifo - 4) &&
>>> + op->data.dir == SPI_MEM_DATA_IN) {
>>> + nxp_fspi_read_ahb(f, op);
>>> + } else {
>>> + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT)
>>> + nxp_fspi_fill_txfifo(f, op);
>>> +
>>> + err = nxp_fspi_do_op(f, op);
>>> +
>>> + /* Invalidate the data in the AHB buffer. */
>>> + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT)
>>> + nxp_fspi_invalid(f);
>>
>> E.g. in case of an erase operation or a NAND load page operation, the
>> invalidation is not triggered, but flash/buffer contents have changed.
>> So I'm not sure if this is enough...
> Ok, would change this and have invalidate for all operations.
Maybe you can find out the correct way through testing with NOR and NAND.
