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

[Ashish Kumar]

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.
> 
> >
> >>
> >>>
> >>> 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?
> Why don't you just use sf/mtd to access the flash?
> 
> >
> >> 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.
Is setting SFA1AD, SFA2AD, SFB1AD, SFB2AD to flash size, QSPI-NAND access is 
broken? I cannot check this since our board does not have such configuration.
If yes, then it should be reverted. 

otherwise I believe there is no harm in this version of driver, it does everything intended.

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