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

Wed Dec 11 13:23:32 CET 2019

Hi Frieder,

> -----Original Message-----
> From: Schrempf Frieder <frieder.schrempf at kontron.de>
> Sent: Tuesday, December 3, 2019 6:17 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 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?

I have sent out PFE driver patch upstream[1] and booting crash is now resolved.

Moreover, After using 1k size, I faced a random crash in environment which was resolved after enabling SYS_RELOC_GD_ENV_ADDR in defconfig.
I am not sure why this needed when setting 1k size? Note that, same is not required if I use my previous implementation.

Now, I found a new bug while testing read functionality in LS1012A, LS1046A on commit "4b19b89ca4a8".
I cannot access memory above 16MB. For example, when I try to access 16M, data read is actually from 0x0 offset.
Could you please share your views on this behavior.

--Kuldeep
> 
> >
> > 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.
[1] https://patchwork.ozlabs.org/patch/1207610/