[U-Boot] [EXT] Re: [PATCH 2/2] fsl_qspi: Improve QSPI driver to incorporate 4 byte commands

[Ashish Kumar]

> -----Original Message-----
> From: Schrempf Frieder <frieder.schrempf at kontron.de>
> Sent: Thursday, May 2, 2019 12:38 PM
> To: Ashish Kumar <ashish.kumar at nxp.com>; Vignesh Raghavendra
> <vigneshr at ti.com>; Rajat Srivastava <rajat.srivastava at nxp.com>; u-
> boot at lists.denx.de; jagan at openedev.com
> Subject: Re: [EXT] Re: [U-Boot] [PATCH 2/2] fsl_qspi: Improve QSPI driver to
> incorporate 4 byte commands
> 
> Caution: EXT Email
> 
> Hi Ashish,
> 
> On 01.05.19 07:32, Ashish Kumar wrote:
> >
> >
> >> -----Original Message-----
> >> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Schrempf
> >> Frieder
> >> Sent: Tuesday, April 30, 2019 1:14 PM
> >> To: Vignesh Raghavendra <vigneshr at ti.com>; Rajat Srivastava
> >> <rajat.srivastava at nxp.com>; u-boot at lists.denx.de;
> jagan at openedev.com
> >> Subject: [EXT] Re: [U-Boot] [PATCH 2/2] fsl_qspi: Improve QSPI driver
> >> to incorporate 4 byte commands
> >>
> >> Caution: EXT Email
> >>
> >> Hi,
> >>
> >> On 26.04.19 06:58, Vignesh Raghavendra wrote:
> >>>
> >>>
> >>> On 25/04/19 5:20 PM, Rajat Srivastava wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Vignesh Raghavendra <vigneshr at ti.com>
> >>>>> Sent: Wednesday, April 24, 2019 10:17 PM
> >>>>> To: Rajat Srivastava <rajat.srivastava at nxp.com>;
> >>>>> u-boot at lists.denx.de; jagan at openedev.com
> >>>>> Cc: Ashish Kumar <ashish.kumar at nxp.com>
> >>>>> Subject: [EXT] Re: [PATCH 2/2] fsl_qspi: Improve QSPI driver to
> >>>>> incorporate 4 byte commands
> >>>>>
> >>>>> Caution: EXT Email
> >>>>>
> >>>>> On 24-Apr-19 6:10 PM, Rajat Srivastava wrote:
> >>>>>> Signed-off-by: Ashish Kumar <ashish.kumar at nxp.com>
> >>>>>> Signed-off-by: Rajat Srivastava <rajat.srivastava at nxp.com>
> >>>>>
> >>>>> Commit message is missing.
> >>>>
> >>>> I'll add proper commit message in the next patch version.
> >>>>
> >>>>> But from $patch subject, I infer that $patch is adding new feature
> >>>>> and not actually fixing something broken?
> >>>>
> >>>> Earlier the framework was designed to work for only 3-byte opcodes
> >>>> but our controller supports flashes of size greater than 16 MB. As
> >>>> a workaround, FSL QSPI driver used to explicitly send 4-byte
> >>>> opcodes for 3-byte opcodes sent by framework to the flash. Also
> >>>> there used to exist a temporary patch for framework which never got
> >>>> accepted In
> >> upstream.
> >>>> Now the new framework supports 4-byte opcodes and FSL QSPI driver
> >>>> needs correction. I am not introducing any new feature. I am just
> >>>> fixing the driver to suit the current framework.
> >>>>
> >>>
> >>> With SPL_FLASH_BAR set fsl_qspi driver should never see 4 byte
> >>> opcodes and should work like it did before. I guess you are disabling
> SPI_FLASH_BAR?
> >>>
> >>> Please add all details to the commit message and I recommend you to
> >>> move the driver over to spi-mem as soon as possible. Else you would
> >>> have to keep handling new opcodes in your driver as and when
> >>> spi-nor-core
> >> changes.
> >>>
> >>> Regards
> >>> Vignesh
> >>>
> >>>> Please let me know your feedback.
> >>>>
> >>>> Regards
> >>>> Rajat
> >>>>
> >>>>> If so, you should move the driver over to use spi-mem APIs instead
> >>>>> of adding more features and hard coding more flash specific
> >>>>> commands in
> >> the driver.
> >>>>> This makes driver duplicate more of spi-nor core code. I
> >>>>> discourage adding new features w/o moving driver over to spi-mem.
> >>>>> IMHO, converting the driver would not be a huge effort. And I
> >>>>> believe its already
> >> done in kernel.
> >>
> >> I started moving the driver to spi-mem, by porting the kernel driver
> >> over to U- Boot. I still have some Kconfig dependency problem for the
> >> LS2081 platform (CONFIG_SPI_MEM is not enabled implicitly), that
> >> needs to be solved, otherwise it should be more or less ready.
> > Hi Frieder,
> >
> > What is the change, it seems it is moving the driver from Linux as it is to
> uboot ?
> 
> Yes, it's moving the current Linux driver to U-Boot with some small
> compatibility changes and simplifications.
> 
> > I am not sure how stable is the current Linux driver, since it is not yet tested
> by FSL/NXP system test team. Last time I tested the current Linux
> driver(QSPI) it was functional in only 1-1-1 mode. Moving to u-boot may not
> be good idea at this point of time.
> 
> The current mainline Linux driver is a SPI controller driver using the SPI MEM
> API and it replaced the old SPI NOR driver in Linux 5.1. If nothing went wrong
> in the upstreaming process, the driver should be just as good as the old one,
> while bringing all the advantages of the new SPI MEM interface. The driver
> does support 1-1-1 mode, just like all other modes supported by the QSPI
> controller.
> 
> While upstreaming to the kernel, the driver was tested by Han Xu and Yogesh
> Gaur.
Hi Frieder,

I had a discussion Han Xu, He was also able to find some defects with current linux QSPI driver on i.mx7 platform.

I had also tried testing this with QUAD/DUAL on LS1088 and It was not functional.
"s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256,  SPI_NOR_DUAL_READ |SPI_NOR_QUAD_READ | USE_CLSR| SPI_NOR_4B_OPCODES),

I will be debugging further and update with findings in sometime.

Regards 
Ashish 
> 
> > How do you plan to cater  CONFIG_SPI_BAR change in uboot now. Some
> old board still uses flash that supports CONFIG_SPI_BAR.
> 
> The new driver is not a SPI NOR driver, so it does not care about
> CONFIG_SPI_BAR. It just handels SPI MEM operations created by the upper
> layer.
> 
> > Me and Rajat have recently posted patches to fix current u-boot driver to
> be functional with new frame work pushed by Vignesh, and it serves all the
> current supported board with and without CONFIG_SPI_BAR.
> >
> > May be spi-nxp-fspi.c can be a good starting point, but then again I not sure
> how booting from serial-nand will be taken care in that case.
> 
> Please have a look at the SPI MEM and SPI NAND APIs to understand why
> porting the Linux driver is the right thing to do instead of patching the old
> driver. By supporting the SPI MEM API, the driver will implicitly allow to
> interface SPI NAND chips.
> 
> Thanks,
> Frieder