[PATCH 1/2] mtd: nand: raw: gpmi: new bch geometry settings

Wed May 26 08:31:23 PDT 2021

Hi Han,

Han Xu <han.xu at nxp.com> wrote on Wed, 26 May 2021 09:17:00 -0500:

> On 21/05/26 09:41AM, Miquel Raynal wrote:
> > Hi Han,
> > 
> > Han Xu <han.xu at nxp.com> wrote on Tue, 25 May 2021 14:13:08 -0500:
> >   
> > > On 21/05/23 07:44PM, Sean Nyekjaer wrote:  
> > > > On 22/05/2021 22.51, Han Xu wrote:    
> > > > > The code change updates the gpmi driver bch geometry settings, the NAND
> > > > > chips required minimum ecc strength and step size will be the default
> > > > > value. It also proposes a new way to set bch geometry for large oob NAND
> > > > > and provides an option to keep the legacy bch geometry setting for
> > > > > backward compatibility.    
> > > > 
> > > > This will break all existing devicetree's. (this happened to us with the same style already merged u-boot patch)
> > > >     
> > > > > 
> > > > > - For the legacy bch geometry settings
> > > > > The driver uses legacy bch geometry settings if explicitly enabled it in
> > > > > DT with fsl, legacy-bch-geometry flag, or the NAND chips are too old
> > > > > that do NOT provide any required ecc info.    
> > > > 
> > > > NAND's are providing the minimum required ecc, the now legacy method
> > > > actually gives more ecc bits and quite cheap when using hw ecc.
> > > >     
> > > > > 
> > > > > The legacy_set_geometry() sets the data chunk size(step_size) larger
> > > > > than oob size to make sure BBM locates in data chunk, then set the
> > > > > maximum ecc stength oob can hold. It always use unbalanced ECC layout,
> > > > > which ecc0 will cover both meta and data0 chunk.
> > > > > 
> > > > > This algorithm can NOT provide strong enough ecc for some NAND chips,
> > > > > such as some 8K+744 MLC NAND. We still leave it here just for backward
> > > > > compatibility and als for some quite old version NAND chips support. It
> > > > > should be en/disabled in both u-boot and kernel at the same time.
> > > > > 
> > > > > - For the large oob bch geometry settings
> > > > > This type of setting will be used for NAND chips, which oob size is
> > > > > larger than 1024B OR NAND required step size is small than oob size,
> > > > > the general idea is,
> > > > > 
> > > > >     1.Try all ECC strength from the minimum value required by NAND chip
> > > > >       to the maximum one that works, any ECC makes the BBM locate in
> > > > >       data chunk can be eligible.
> > > > > 
> > > > >     2.If none of them works, using separate ECC for meta, which will add
> > > > >       one extra ecc with the same ECC strength as other data chunks.
> > > > >       This extra ECC can guarantee BBM located in data chunk, also we
> > > > >       need to check if oob can afford it.
> > > > > 
> > > > > - For all other common cases
> > > > > set the bch geometry by chip required strength and step size, which uses
> > > > > the minimum ecc strength chip required.
> > > > > 
> > > > > Signed-off-by: Han Xu <han.xu at nxp.com>    
> > > > 
> > > > One further point, u-boot older than v2020.04 will not be aligned with the way ecc bits is
> > > > calculated with this patch applied(without the legacy option set).
> > > > 
> > > > It's quite a mess :/
> > > > I would recommend to use the legacy mode as default, and add the new style as "modern" option.
> > > > (Also in u-boot)
> > > > 
> > > > /Sean    
> > > 
> > > 
> > > Hi Sean,
> > > 
> > > I know this change brings mess but the legacy way is wrong. The way it determine
> > > the data chunk size is arbitrary,  
> > 
> > Yes, that's always the case: all default choices are arbitrary in the
> > Linux kernel, there is actually a lot of logic provided by the core to
> > handle that, unless the user requires something specific.
> >   
> > > take any 8K + 448 (not 744, typo in previous
> > > comments) Micron NAND chips as example, the legacy way can only provide 16bit
> > > ecc since data chunk size is set to 512B, but 24b/1K is required by NAND chips.  
> > 
> > This means a strength of 32 bits per kilobyte of data vs 24 bits per
> > kilobyte.
> >   
> > > According to the A/B X/Y ecc requirement, this is not acceptable.  
> > 
> > What you call the legacy way is even better, the only situation where
> > it may be an issue is if the NAND chip does not feature enough OOB
> > bytes to store the ECC codes, which does not seem to be your primary
> > concern here.  
> 
> Hi Miquel,
> 
> The legacy ecc strength is fine or even better by average, but it doesn't meet
> the requirement #2
> 
> (1) A / B >= X / Y
> (2) A >= X
> 
> that's my primary concern.

I understand that (2) might be ideal to meet but is breaking all the
boards that use this driver really worth the trouble?

Short answer: no. So we need to adapt the calculation for new
boards/new flash chips/certain geometries at most.

> > > The new implementation might get weak ecc than legacy way in some cases but it
> > > is safety guaranteed.  
> > 
> > What does "safety guaranteed" means?  
> 
> set minimum ecc required by nand chip at least meet all requirements
> 
> >   
> > > This reminds me the gpmi raw access mode changes in kernel 3.19, it also changes
> > > the driver behaviors and makes totally different output compared with older
> > > versions. I know changes bring mess but we have to accept it at some point
> > > rather than keep compromising to the wrong way.  
> > 
> > How is this an argument? I am usually in favor of moving forward when
> > there is a real justification, but this does not seem the case, unless
> > I am understanding it all the wrong way.
> >   
> > > The change has been in NXP kernel fork for a while, so quite a few customers are
> > > using this bch geometry settings. I hope it can be upstreamed, any other things
> > > I can do may mitigate the imapact?  
> > 
> > You are well aware of the upstreaming process, trying to merge
> > something locally, making it used and then complaining because not
> > upstreaming it would break your customers really is your own
> > responsibility.  
> 
> Sorry I understand I should try upstreaming it early, so I am still looking for
> a chance to avoid further divergence.
> 
> > 
> > IMHO the solutions are:
> > - the current (mainline) default will remain the standard for
> >   geometries which are already widely supported
> > - if there are new geometries that must be supported and do not fit
> >   because of the "legacy" logic, then you may detect that and try
> >   to fallback to the "modern" way of calculating the ECC
> >   parameters (or even jump directly to the modern way if the geometry
> >   really is not currently supported officially)
> > - if your customers want a specific chunk size/strength when
> >   rebasing on top of a mainline kernel there are DT properties which do
> >   that anyway
> > - follow Sean advice: introduce a property requesting to use the
> >   'modern' or 'legacy' logic (with a better name than modern) but first
> >   check with Rob that this if valid.

Another hint: please check the core helpers and use them instead of
trying to re-invent the wheel: normally just describing the engine
capabilities and calling a single helper should do the trick. But this
'new' calculation should only apply to eg. MLC devices or devices with
specific geometries, not to all devices.

Thanks,
Miquèl