[EXT] Re: [PATCH 0/8] Transition of fsl qspi driver to spi-mem framework

[Schrempf Frieder]

On 03.12.19 11:56, Kuldeep Singh wrote:
> Hi Frieder,
> 
>> -----Original Message-----
>> From: Schrempf Frieder <frieder.schrempf at kontron.de>
>> Sent: Tuesday, December 3, 2019 2:27 PM
>> To: Kuldeep Singh <kuldeep.singh at nxp.com>; u-boot at lists.denx.de
>> Cc: Priyanka Jain <priyanka.jain at nxp.com>; jagan at amarulasolutions.com;
>> sr at denx.de; Ashish Kumar <ashish.kumar at nxp.com>; Ye Li <ye.li at nxp.com>
>> Subject: Re: [EXT] Re: [PATCH 0/8] Transition of fsl qspi driver to spi-mem
>> framework
>>
>> Caution: EXT Email
>>
>> On 03.12.19 07:30, Kuldeep Singh wrote:
>>> Hi Frieder,
>>>
>>>> -----Original Message-----
>>>> From: Schrempf Frieder <frieder.schrempf at kontron.de>
>>>> Sent: Monday, December 2, 2019 5:35 PM
>>>> To: Kuldeep Singh <kuldeep.singh at nxp.com>; u-boot at lists.denx.de
>>>> Cc: Priyanka Jain <priyanka.jain at nxp.com>;
>>>> jagan at amarulasolutions.com; sr at denx.de; Ashish Kumar
>>>> <ashish.kumar at nxp.com>
>>>> Subject: [EXT] Re: [PATCH 0/8] Transition of fsl qspi driver to
>>>> spi-mem framework
>>>>
>>>> Caution: EXT Email
>>>>
>>>> + Ashish
>>>>
>>>> Hi Kuldeep,
>>>>
>>>> On 29.11.19 06:54, Kuldeep Singh wrote:
>>>>> This entire patch series migrate freescale qspi driver to spi-mem
>> framework.
>>>>
>>>> First, thanks for working on this. I have this on my list for quite a
>>>> long time, but struggled to find enough time to actually get it done.
>>>>
>>>>>
>>>>> Patch 1 removes the old fsl qspi driver
>>>>
>>>> You shouldn't remove the old driver before the new one is in place,
>>>> as this breaks the build in between the two patches.
>>>
>>> I first merged the patch1 and patch2 and found that the diff output was very
>> much less readable.
>>> That's why I split them into 2 patches so as to make new driver changes
>> legible.
>>> Please let me know how shall I proceed. Shall I merge the two patches?
>>
>> Yes, the merged patch will not be very good to read, but as far as I know there
>> is no other option. We must not break the build to keep bisectability.
> 
> Alright I will merge the two patches.
> 
>>
>>>
>>>>
>>>>>
>>>>> Patch 2 adds new qspi driver incorporating spi-mem framework which
>>>>> is ported version of linux qspi driver. Initial port was done by Frieder.
>>>>> Now, no more direct access to spi-nor memory is possible. Few
>>>>> changes were introduced to prevent uboot crash such as to add
>>>>> complete flash size to SFA1AD, SFA2AD, SFB1AD, SFB2AD based on chip
>>>>> select number instead of 1k. Immediate effect was observed on pfe
>>>>> while using 1k size as it access spi-nor memory directly. Using
>>>>> complete flash size resolves
>>>> the crash but data read will not be valid.
>>>>
>>>> Can you provide more information about the problem/crash you
>>>> experience and the platform you are working on?
>>>
>>> I observed crash on LS1012. Also, any access to flash direct memory above
>> 1k will crash without this change.
>>
>> As I already told Ashish in the conversation referenced in my last mail:
>> I can't see any good reason why the direct memory access is something that
>> we need or should support. We should always use the APIs provided by U-Boot
>> to access the flash and that is mtd.
>>
>>> By adding this, crash will be resolved but data is invalid as mentioned in
>> patch-set.
>>
>> So what's the purpose of your changes at all, if they do not solve the problem
>> you're trying to solve?
> 
> I observed booting crash on all ls1012 platforms. Control does not reach even end of uboot prompt.
> I dig in deeper, and found that "pfe (packet forwarding engine)" was using spi-nor memory directly.
> With this change, booting crash was resolved. Now, at least other network interfaces can be used.
> Without this changes, I have to disable pfe on adhoc basis so as to get uboot prompt.
> This is to make sure all intended qspi targets are booting.

Ok, thanks for pointing out the PFE driver. I didn't know about such a 
peripheral. So this seems to be the actual problem here.

I don't really understand, why Ashish didn't mention this when we were 
talking about this issue some weeks ago.

> 
>> Why don't you just use sf/mtd to access the flash?
> 
> Pfe framework have to bring in changes to access flash using sf in uboot.

Yes and that's something that should be done first instead of hacking 
the QSPI controller driver. It shouldn't be too hard to modify the PFE 
driver so that it uses the serial flash API (spi_flash_read()) to access 
the SPI NOR.
Can you try to come up with a patch for the PFE driver?

> 
> Thanks
> Kuldeep
> 
>>
>>>
>>>> Are you referring to the same issue as Ashish in this discussion here [1]?
>>>
>>> Yes, I had a discussion with him.
>>>
>>>> There are two reasons why I'd like to avoid using the whole memory
>>>> mapped area for AHB access.
>>>> First, I'd like to keep the U-Boot driver as close as possible to the Linux
>> driver.
>>>> Second, the intention of the spi-mem layer is to abstract the flash
>>>> layer and therefore this driver should work independently of flash type or
>> size.
>>>
>>> Boot from QSPI-NAND will still not be possible. Code in bootROM is only to
>> access QSPI-NOR.
>>
>> It will not be possible to use SPI NAND directly from the BootROM, but you can
>> just load the bootloader from a different device like SPI NOR and then fetch
>> the rest of the system (Kernel, rootfs, etc.) from a SPI NAND device. Actually
>> that's exactly the use case, that led to the development of the SPI MEM layer
>> and the migration of the QSPI driver.
>>
>>>
>>>> With your version this wouldn't be the case if you connect a flash
>>>> that is bigger than the memory map for example.
>>>
>>> I agree such use case can be valid for Linux but in case of Uboot, I believe
>> access to flash size greater than 256M will not be required.
>>> If in case there is a requirement, there is another region in CCSR space to
>> map flash memories up to 4G.
>>> Random crashes can be avoided by adding these changes. Please let us know
>> your views as well.
>>
>> We don't even need to consider these cases, if we would just stick to the SPI
>> MEM API and use it as intended. Apart from some possible performance
>> penalty (that shouldn't matter too much and could be resolved by
>> implementing the direct mapping API as in Linux), I can't see the reason for
>> not doing so.