Re: [PATCH v5 1/5] spi: spi-mem: Add driver for NXP FlexSPI controller

[Schrempf Frieder]

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.