Re: [PATCH 3/3] spi: spi-mem: MediaTek: Add SPI NAND Flash interface driver for MediaTek MT7622

[Miquel Raynal <miquel.raynal-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>]

Hi Chuanhong,

Chuanhong Guo <gch981213-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on Wed, 11 Mar 2020 17:15:38
+0800:

> Hi!
> 
> On Wed, Mar 11, 2020 at 4:18 PM Boris Brezillon
> <boris.brezillon-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
> >
> > +Miquel who worked on the ECC engine abstraction [3] recently.
> >
> > Hello Chuanhong,
> >
> > On Wed, 11 Mar 2020 15:35:43 +0800
> > Chuanhong Guo <gch981213-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >  
> > > Hi Boris!
> > >
> > > [resend to you because of the wrong mail address in previous one.]
> > >
> > > I'm about to pick this driver up and start upstream it in the future.
> > > So I'm answering
> > > your questions below and would like to get your further suggestions.
> > >
> > > On Fri, Oct 26, 2018 at 2:09 PM Boris Brezillon
> > > <boris.brezillon-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> wrote:  
> > > > > > There's a fundamental issue with this driver: spi-mem was designed to be
> > > > > > memory agnostic, and you seem to have a SPI controller that supports
> > > > > > only SPI NANDs. Is that correct, or is it just that you developed the
> > > > > > driver with SPI NANDs in  mind?
> > > > > >  
> > > > > Yes, this driver supports only SPI NANDs.
> > > > > Actually, Mediatek's controller is designed for NAND specifically, which
> > > > > can support SPI NANDs and PARALLEL NANDs,and this driver is just for SPI
> > > > > NANDs.  
> > > >
> > > > Hm, I'm not so sure about that (I might be wrong though), it seems you
> > > > can send any command you want, not only SPI NAND related ones. Maybe the
> > > > controller is optimized for SPI NANDs but can do all kind of SPI
> > > > transfers.  
> > >
> > > You are correct here. This controller can perform generic spi-mem operations,
> > > and it has special routines for page cache R/W that utilize controller's ECC
> > > functionality.  
> >
> > Sounds similar to the way the MXIC controller works, and that's
> > actually what Miquel is trying to support with his ECC engine
> > abstraction series [3].
> >  
> > > I think the purpose of this is to provide better ECC capability
> > > for some SPI NANDs with worse ECC algorithm on chip.  
> >
> > Yep, or make it faster. Actually the reason doesn't matter, I think
> > we all agree that we'll have to support external ECC for SPI NANDs at
> > some point, hence the work Miquel has been doing.
> >  
> > >  
> > > > > > Don't know what's possible to do with your controller, and maybe it's
> > > > > > not able to execute random SPI memory operations, but in this case we
> > > > > > should consider a different solution to support this driver.
> > > > > >
> > > > > > Do you have a public datasheet I can look at?
> > > > > >  
> > > > > We do not have a public datasheet for Mediatek controller currently.  
> > > >
> > > > Unfortunately, there's not much I can do without a clear understanding
> > > > of how the controller works.
> > > >  
> > >
> > > I found a public datasheet [0] on wiki page for Banana Pi R64 [1].
> > > Register description is available under "NAND flash interface" section
> > > starting at page 592.
> > > There's a hackier version of this driver in OpenWrt [2] which checks
> > > opcode and use controller routine for page cache R/W.
> > >
> > > ECC part of this controller can also be used as a standalone ECC
> > > algorithm and perform ECC operations on data provided by CPU.  
> 
> The solution I'm referring to here is:
> 1. read uncorrected data to host directly from SPI NAND
> 2. start an ECC correction separately
> 
> > > But I think if it's implement this way, we wasted the page cache
> > > R/W routines in this controller.  
> >
> > Oh, you probably don't want the page cache to be active anyway. When
> > the framework reads a NAND page, it also checks the number of ECC
> > errors, if the page is held in some internal cache, you won't see
> > the evolution of this number. Note that the FS should do some caching,
> > so caching things at the HW level is probably useless.  
> 
> It doesn't cache anything in controller. The R/W routine I refer to is:
> When we issue a request to read page cache on SPI NAND:
> 1. host programs a DMA-able memory area for receiving data.
> 2. controller reads the page cache from SPI NAND
> 3. controller get the data and start ECC correction
> 4. corrected data will be sent back to host via DMA
> 5. host could check ECC status
> 
> writing of page cache goes similarly.
> There's no need for a separated ECC request comparing to previous
> one.
> 
> >  
> > >
> > > I have two other initial thoughts:
> > > 1. abstract some kind of ECC functionality in spi-mem interface
> > >     I haven't really learned ECC stuff so I don't know whether this is
> > >     possible and what kind of argument we needs for it.  
> >
> > Nope, spi-mem should stay focused on SPI transfers, nothing
> > memory-specific should leak there.
> >  
> > > 2. modify SPI-NAND core to add support for special SPI-NAND controller.
> > >     This limits controller's ability and adds extra burden for future extention
> > >     of SPI-NAND framework.  
> >
> > That doesn't work either as some ECC engines are shared between the
> > raw NAND and spi-mem IPs.
> >  
> > >
> > > Which way would you prefer and do you have other suggestions?  
> >
> > See [3]. I think you can already base your work on Miquel's series, but
> > maybe he has a more up-to-date version to share. I'll let you sync with
> > him.  

I am actively working on it, this series is adding an "ECC engine
framework" that could potentially fit any architecture. I am currently
working with a Macronix external ECC engine, I will "soon" send a new
version of it, I'll copy you.

Thanks,
Miquèl