Hi Yogesh,

On Tue, 18 Sep 2018 11:34:18 +0000
Yogesh Narayan Gaur [off-list ref] wrote:

quoted

Do we really need all those macros for registers and modes, that
aren't even used in the driver? I don't know what the common
practice is, but to me it seems like removing all the unused macros
would make the driver much smaller and more readable.
  

We don't need all Macros currently, but can be needed in future and
then have to add again. Generally, we add them all so that in future
don't have to dig in datasheet to add basic register details.

I guess it's just a matter of taste, but I also prefer when all regs are
defined even if not all of them are used.

[...]

quoted

You are only considering 3 and 4 byte long addresses, which is fine
for NOR chips, but the SPI mem interface allows to connect other
chips like SPI NAND which also use 1 byte addresses.

In the QSPI driver Boris worked around this restriction by using
LUT_MODE instead of LUT_ADDRESS.

Does this restriction also exist for FSPI?  

Yes, I have seen that implementation and first tries with that same
logic, using LUT_MODE instead of LUT_ADDR, but didn?t work for the
FlexSPI controller.

In this controller, we are having separate LUT_XX for RowAddress and
ColumnAddress. For case of the Nand flash, we need to program both
RowAddress and ColumnAddress in single LUT sequence.

Hm, I don't get it. LUT_MODE was just a way to pass raw data on the I/O
bus, so the row vs column thing has no meaning in this case, and the
offset withing the QSPI AHB range should just be ignored.

IMO, when support needs to be added for NAND flash, then slight
modification can be done in the logic. As per my discussion with
controller validation guys, needs to send 16-bit addrlen for
RowAddress, LUT_ADDR (0x2) Addrlen can vary for the column-address
and needs to be programmed for sequence LUT_CADDR_SDR (0x3)

And that's again flash specific details leaking into the spi-mem layer,
which I'd like to avoid (as repeated many times before).

quoted

You are using the remapping procedure as in the QSPI NOR driver.
The original purpose was to start with a rather small mapping size
and increase it when a larger memory device is used.

At the same time you use the logic from the QSPI SPI mem driver,
that adjusts the data.nbytes of each read op to a maximum of
ahb_buf_size in nxp_fspi_adjust_op_size().
This is the logic that Boris introduced for the QSPI driver until
we replace it with something like dirmap.

Unless there is something I missed, this means the ramapping is
useless and it's enough to reserve memory with the fixed size of
ahb_buf_size. 

My concern was for performance and that's why has done remap for the
4MB buffer size so that if any subsequent Read request would come
within the range then don?t have to perform remap and can just
directly do memcpy()

I would re-visit again and see if getting any issue in doing direct
memcpy() instead of remap. We need to perform AHB buffer invalidation
when using controller in both IP(write, erase etc) and AHB (read)
mode.

Then you should really review my dirmap proposal instead of trying to
hack things directly into your driver. The only reason I did no send a
new version of the dirmap patchset is because I got no reviews from
people that might need it, so please have a look at it, try to
implement a backend for your controller, and let me know if you face
any issues or think things should be done differently.

Thanks,

Boris