[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: Wednesday, December 11, 2019 6:56 PM
> To: Kuldeep Singh <kuldeep.singh at nxp.com>; u-boot at lists.denx.de;
> jagan at amarulasolutions.com
> Cc: Priyanka Jain <priyanka.jain at nxp.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
> 
> Hi Kuldeep,
> 
> On 11.12.19 13:23, Kuldeep Singh wrote:
> > Hi Frieder,
> [...]
> >>>>>>> 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.
> 
> Ok, good.
> 
> >
> > 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.
> 
> SYS_RELOC_GD_ENV_ADDR was only introduced very recently and it seems
> like it should be enabled for your boards (see [1]) when using something more
> recent than 8d8ee47e03ef.
> 
> My guess would be that you're missing the
> "CONFIG_SYS_RELOC_GD_ENV_ADDR=y" because of some mistake while
> rebasing or merging .

I checked and found that Tom had made changes in all LS1012A variants except LS1012ARDB.
I will make the required changes for the same.

> 
> >
> > 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.
> 
> This is usually a problem if the addressing mode for the SPI NOR is incorrect.
> When using 2-bytes addresses, only the first 16MiB of the flash can be
> accessed. For SPI NOR flashes with sizes bigger than 16MiB, 3-byte mode is
> mandatory to access areas above 16MiB.
> 
> What's the manufacturer and type of the SPI flash you are using?
> Also please try to test on latest master with all the latest changes for MTD, etc.

Yes, I have rebased the patches and read functionality is now working.
Actually, I had SPI_FLASH_BAR config enabled by mistake and that's why run into the bug.
Thanks for providing the info. I will send v2 version of series after incorporating all the changes.

Thanks
Kuldeep
> 
> Thanks,
> Frieder
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.de
> nx.de%2Fu-boot%2Fu-
> boot%2Fcommit%2F8d8ee47e03ef23b0d0e842ea455a30bf0d2023b9&dat
> a=02%7C01%7Ckuldeep.singh%40nxp.com%7C1764cecf960246f8a66808d77e3
> db32b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637116675774
> 853728&sdata=spAy%2B02oTno6IdUovwyZzRRqY5%2FqZJtg%2FLCo%2BU
> 9TiLI%3D&reserved=0
> 
> >
> > --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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwo
> rk.ozlabs.org%2Fpatch%2F1207610%2F&data=02%7C01%7Ckuldeep.singh
> %40nxp.com%7C1764cecf960246f8a66808d77e3db32b%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C637116675774853728&sdata=RKXrWE
> sYgLFfJAACFSa%2BPFZ%2FzXTEH8SUDW6dv3prkWM%3D&reserved=0
> >