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

[Schrempf Frieder]

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.

> 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