[PATCH v1 1/2] mtd: fsl-quadspi: add support to create dynamic LUT entry

[Yogesh Narayan Gaur]

Hello Frieder,

> -----Original Message-----
> From: Frieder Schrempf [mailto:frieder.schrempf at exceet.de]
> Sent: Tuesday, January 09, 2018 9:56 PM
> To: Yogesh Narayan Gaur <yogeshnarayan.gaur at nxp.com>
> Cc: linux-mtd at lists.infradead.org; Boris Brezillon <boris.brezillon at free-
> electrons.com>; cyrille.pitchen at wedev4u.fr; computersforpeace at gmail.com;
> Han Xu <han.xu at nxp.com>; festevam at gmail.com; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; Suresh Gupta <suresh.gupta at nxp.com>
> Subject: Re: [PATCH v1 1/2] mtd: fsl-quadspi: add support to create dynamic LUT
> entry
> 
> Hello Yogesh,
> 
> > Add support to create dynamic LUT entry.
> >
> > Current approach of creating LUT entries for various cmds like read,
> > write, erase, readid, readsr, we, wd etc is that when QSPI controller
> > gets initialized at that time static LUT entries for these cmds get created.
> >
> > Patch add support to create the LUT at run time based on the operation
> > being performed.
> >
> > Added API fsl_qspi_prepare_lut(), this API would going to be called
> > from fsl_qspi_read_reg, fsl_qspi_write_reg, fsl_qspi_write,
> > fsl_qspi_read and fsl_qspi_erase APIs.
> > This API would fetch required info like opcode, protocol info, dummy
> > info for creating LUT from instance of 'struct spi_nor' and then
> > prepare LUT entry for the required command.
> 
> I'm just about to get started working on a similar topic, so I have some general
> (maybe stupid) questions concerning the dynamic LUT implementation:
> 
> Why do you actually need to be able to switch between different kind of
> commands for READ/WRITE/ERASE at time of execution?
Actually we want to handle different NOR flash devices on different chip select. 
QSPI controller has 4 chip select and one can use different flashes on all different chip select (that might not be the use case for today). We analyzed that the flash have different parameters for the same command and this is true for NAND devices.
Also reg_protocol information, required for read register, read any register commands, read SFDP register, not available at the time of nor scan.
This change was done after understanding the requirement for NAND and Hybrid flash also.
NXP is going to support hybrid flash which has NOR and NAND at one chip select. So, to handle these type of flashes dynamic LUT is the best approach, condition NAND framework also provide required information (datalines, dummy bytes, addrlen)as provided by NOR framework as of now.
To handle all chip select and hybrid complexities, this dynamic LUT need to be required as we can have maximum 16 static LUT entries.

> 
> Wouldn't it be better to init the LUT statically, after the chip was detected and at
> that time decide which READ/WRITE/ERASE command is best for the connected
> chip?
As explained above if we have different flashes on different chip select (even for 2 flash we can maximum provide 8 LUT entries for different flashes). 
It is tough to handle such scenario and lead to code complexity.

> 
> Have you tested what kind of impact the dynamic loading of a command
> sequence to the LUT registers before each operation has on the performance?
We are programming 4 LUT register for every command sent from MTD layer. Theoretically it is max 16bytes per command.
It looks to be trivial overhead. We are doing analysis of performance impact of dynamic LUT creation.

> 
> In the context of the upcoming SPI NAND framework, I am working on NAND
> support for the FSL QSPI controller.
Hope you will take care of having support of NAND on chip select 1 and 2.

> I have a working driver at [1] derived from the NOR driver.
> As discussed with Boris ([2]) the plan is to add NAND support to the NOR driver
> until a common spi-flash layer is available to hold code, that is used by both
> types of SPI flash, NOR and NAND.
> 
> To achieve this, some kind of dynamic LUT allocation is needed, too.
> But at first glance it seems me, that it would be better to actually use the LUT
> entries as much as possible and create the entries at time of init depending on
> flash type (NOR/NAND) and flash capabilities (QUAD/DUAL/QIO
> READ/WRITE...).
> 
> In the exotic case of needing to switch between different command sets (for
> hybrid NOR/NAND devices, see [3]) one could reinit the LUT for the current
> target upon selecting it.
> 
> Or maybe it is even better to create a mechanism that fills the LUT with entries
> when they are first needed and be able to reuse them until the space in the LUT
> runs out and entries need to be overriden.
> 
> What do you think?
Right, we also though of this approach but we think this would going to increase the code complexity so the simple and best possible approach we use is to create run-time LUT for every command when function ptr (read_reg, write_reg, read, write and erase) gets invoked.

Thanks
Yogesh Gaur
> 
> As I'm quite new to this, maybe someone can point to other implementations
> with similar approaches for reference, if there are any.
> 
> Thanks and regards,
> 
> Frieder
> 
> [1]:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Ffschrempf%2Flinux-0day%2Fblob%2Fspi-nand-
> exceet%2Fdrivers%2Fmtd%2Fnand%2Fspi%2Fcontrollers%2Ffsl-quadspi-
> controller.c&data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C0a92ac7c
> f9194daaa1e408d5577dac6f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C636511119642760392&sdata=97LBSctrv6set73W%2FMio5H8Mxa8PUOn
> af1qnbJibycs%3D&reserved=0
> [2]:
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr
> adead.org%2Fpipermail%2Flinux-mtd%2F2018-
> January%2F078592.html&data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%
> 7C0a92ac7cf9194daaa1e408d5577dac6f%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636511119642760392&sdata=ur%2F%2BfRg0TIfuthyA%2BliL
> qOc4T1mCU2exZd62YQCOk1Y%3D&reserved=0
> [3]:
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr
> adead.org%2Fpipermail%2Flinux-mtd%2F2018-
> January%2F078488.html&data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%
> 7C0a92ac7cf9194daaa1e408d5577dac6f%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C636511119642760392&sdata=8ItjA5b%2FRnvVmBl0YrUoHet
> 4chNzVJq5b0s6vsxY%2BD4%3D&reserved=0