Re: Questions about the Freescale/NXP QuadSPI controller

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1.1: Type: text/plain, Size: 9065 bytes --]

Hi Frieder,

> Hi Lukasz,
> 
> On 21.09.2018 00:13, Lukasz Majewski wrote:
> > Hi Frieder,
> > 
> > Please find some more comments regarding the new spi-fsl-qspi.c
> > driver. 
> >> Hi Lukasz,
> >>
> >> On 20.09.2018 17:00, Lukasz Majewski wrote:  
> >>> Hi Frieder,
> >>>      
> >>>> Hi Frieder,
> >>>>     
> >>>>> Hi Lukasz,
> >>>>>
> >>>>> On 19.09.2018 00:42, Lukasz Majewski wrote:  
> >>>>>> Dear All,
> >>>>>>
> >>>>>> Maybe I do jump a bit off topic here, but...
> >>>>>>
> >>>>>> I've read through the following thread:
> >>>>>> https://patchwork.ozlabs.org/patch/939885/
> >>>>>>
> >>>>>> And it mentions some issues with reading AHB content (buffers)
> >>>>>> in fsl-quadspi.c driver discovered when new QuadSPI driver was
> >>>>>> developed.  
> >>>>>
> >>>>> The only setup with two chips that is known to be problematic
> >>>>> with the new driver, is when both chips are connected to the
> >>>>> same bus (e.g. QSPIA) using separate chip selects.  
> >>>>
> >>>> I'm using QSPI0 controller, with two memories connected to
> >>>> QSPI0_A and QSPI0_B lines.
> >>>>     
> >>>>>
> >>>>> Does your board use this kind of setup, or are the two chips
> >>>>> connected two different buses (QSPIA and QSPIB)?
> >>>>>
> >>>>> Have you tested the new driver? It would be great to receive
> >>>>> some more feedback.  
> >>>>
> >>>> I will check (test) this new driver. No problem.  
> >>>
> >>> I've ported your patch on v4.19-rc3.
> >>>
> >>> I had to fix the "div0 issue" by adding:
> >>> if (op->dummy.buswidth &&
> >>> 	(op->dummy.nbytes * 8 / op->dummy.buswidth > 64))
> >>>
> >>> In fsl_qspi_supports_op() function.
> >>>
> >>>
> >>> Unfortunately, on Vybrid vf610 when testing I do see corruption of
> >>> read data from the mtd device:
> >>>
> >>> ~# hexdump aaa.img
> >>> 0000000 464c eec0 baa5 c5ff 7b99 4dfb e0b6 8a2e
> >>> 0000010 e98e 5265 683c a635 c069 e402 303f d936
> >>> 0000020 c243 01a7 7064 fce8 e3a9 200a 7e85 28bc
> >>> 0000030 4296 a30e 1bb4 88d4 b456 b4a6 f3aa 8cff
> >>> 0000040 ffff ffff ffff ffff ffff ffff ffff ffff
> >>> *
> >>> 0000100
> >>>
> >>> ~# hexdump data_test.img
> >>> 0000000 464c eec0 baa5 c5ff 7b99 4dfb e0b6 8a2e
> >>> 0000010 e98e 5265 683c a635 c069 e402 303f d936
> >>> 0000020 c243 01a7 7064 fce8 e3a9 200a 7e85 28bc
> >>> 0000030 4296 a30e 1bb4 88d4 b456 b4a6 f3aa 8cff
> >>> 0000040 01c9 462d 0a43 f893 0e42 67f1 57f0 787c
> >>> 0000050 49c0 fb2a e514 e954 1d21 affa bac4 38f1
> >>> 0000060 1ca5 ec46 77eb a854 285b 8e21 12d7 f377
> >>> 0000070 b441 2baf ee33 596c 98b6 e71a c1cb 876c
> >>>
> >>>
> >>> Test case:
> >>> flash_erase /dev/mtd3 0 1
> >>> dd if=data_test.img of=/dev/mtd3
> >>> dd if=/dev/mtd3 of=aaa.img bs=1 count=256
> >>>
> >>>
> >>>   From the code I see that the AHB buffer has 1024B. It is
> >>> accessed in 8 bytes packs.
> >>>
> >>> This seems like some cache or HW issue....
> >>>
> >>> In the old driver, to fix this problem one needed to
> >>> 1. Reduce the RX fifo size from 128B to 64B
> >>>
> >>> 2. Disable (set to 0) the AHB transfer's BUF3CR ADATSZ field - the
> >>> transfer size is the same as in LUT.
> >>>
> >>> (Original patch: https://patchwork.ozlabs.org/patch/675401/)  
> >>
> >> Thanks a lot for doing the test. It seems like Vybrid needs some
> >> special handling to work around this problem.
> >> So this means the current driver has never worked for Vybrid, as
> >> the mentioned patch was never merged!?
> >>
> >> As these issues seem to be specific to Vybrid and as they seem to
> >> exist in the current driver too, I would like to fix them after we
> >> have a first version of the new driver that works for the other
> >> platforms.
> >>
> >> I hope we can figure out the other issues soon.
> >>
> >> Thanks,
> >> Frieder
> >>  
> >>>
> >>>      
> >>>>     
> >>>>>         
> >>>>>> I do have a setup with qspi0 having two SPI memories connected
> >>>>>> (2x16 MiB), and I'm wondering if anybody has some more info
> >>>>>> regarding:
> >>>>>>
> >>>>>> (What's more is that there is a bug in
> >>>>>>     * the "IP Command Read" in the Vybrid.) found here:  
> >>> 	^^^^^^^^^^^^^^^^^^^^^^^^ - It would be handy to have the
> >>> exact explanation of this issue.
> >>>      
> >>>>>> https://elixir.bootlin.com/linux/v4.19-rc4/source/drivers/mtd/spi-nor/fsl-quadspi.c#L671  
> >         ^^^^^^^^^^^^ - [1]
> > 
> > 
> > I've looked a bit closer to the new spi-fsl-qspi.c driver and it
> > looks like it uses fsl_qspi_exec_op() and then calls
> > fsl_qspi_do_op().
> > 
> > In the last function we setup the QUADSPI_IPCR register to initiate
> > the transfer between SPI-NOR and the internal RX buffer.
> > 
> > Please correct me if I'm wrong but it seems to me like we are using
> > "IP Command Read" approach here.  
> 
> "IP Read" is only used if data size is smaller then RX FIFO size (128 
> bytes for Vybrid).

I would like to just point out that the "legacy" (in-kernel) driver
(fsl_quadspi.c) is using AHB transfers for even small chunks of data
(even RX FIFO size).

> For bigger chunks of data "AHB Read" is used.
> 
> > 
> > If this is true - what was the rationale to use it in the new
> > driver, as in the old one the "AHB Command Read" approach was
> > recommended (i.e. faster) [1]?  
> 
> As explained above we only use "IP Read" for small chunks of data. To 
> speed up the tranfer for bigger chunks we do use "AHB Read".
> And in the future we plan to add an interface for mapping memory, so
> we can use the "AHB Read" even more efficiently.

It looks like I do read the AHB internal QUADSPI buffer before it is
filled up completely.
(I've asked the NXP community support about it,
but no reply so far https://community.nxp.com/thread/485139).

> 
> > 
> > To be even more problematic the info in link [1] states that on
> > vybrid there is a bug on the "IP Command Read" HW block and it
> > should be avoided.  
> 
> If "IP Read" should be avoided completely for Vybrid, we have to add 
> some kind of quirk.

Yes, I think so. According to the in-code comment the "IP Read" mode
shall be avoided as it has some bugs.

> 
> Thanks for you comments,
> Frieder
> 
> >   
> >>>>>>
> >>>>>> I've googled for some errata or known issues for vybryd's QSPI
> >>>>>> IP block (vf610) but without luck so far ...  
> >>>>>
> >>>>> Unfortunately I don't know the background for this comment.  
> >>>>
> >>>> The comment was added by some Freescale employee when the driver
> >>>> was added to Linux (by Huang - CC'ed).
> >>>>     
> >>>>> Is your
> >>>>> board using VF610?  
> >>>>
> >>>> Yes, it uses vf610 (A5 + M4 cores).
> >>>>     
> >>>>> Do you experience problems?  
> >>>>
> >>>> I've already observed following issue:
> >>>>
> >>>> For the current QSPI ML driver (fsl-quadspi.c - 4.19-rc3) only
> >>>> half of the AHB buffer is valid. When I do read the whole one -
> >>>> I do see read data corruption (on UBI or raw memory).
> >>>>
> >>>> This was pointed out in the patch:
> >>>> https://patchwork.ozlabs.org/patch/675401/
> >>>>
> >>>> Unfortunately, I did not found any info regarding this problem
> >>>> (in NXP's errata or other doc).
> >>>>
> >>>> I will check if this issue shows up on new patches.
> >>>>
> >>>> Thanks in advance for your help.
> >>>>     
> >>>>>
> >>>>> Regards,
> >>>>> Frieder
> >>>>>
> >>>>> ______________________________________________________
> >>>>> Linux MTD discussion mailing list
> >>>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/  
> >>>>
> >>>> Best regards,
> >>>>
> >>>> Lukasz Majewski
> >>>>
> >>>> --
> >>>>
> >>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
> >>>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> >>>> Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax:
> >>>> (+49)-8142-66989-80 Email: wd@denx.de  
> >>>
> >>>
> >>>
> >>>
> >>> Best regards,
> >>>
> >>> Lukasz Majewski
> >>>
> >>> --
> >>>
> >>> DENX Software Engineering GmbH,      Managing Director: Wolfgang
> >>> Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> >>> Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> >>> wd@denx.de  
> >>
> >> ______________________________________________________
> >> Linux MTD discussion mailing list
> >> http://lists.infradead.org/mailman/listinfo/linux-mtd/  
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email:
> > wd@denx.de 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/