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

[Kuldeep Singh]

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.

> 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.

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.