Re: [PATCH] mtd: rawnand: micron: handle "ecc off" devices correctly

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Marco,

Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote on Fri, 26 Jul 2019
11:40:10 +0200:

> Hi Miquel,
> 
> On 19-07-26 11:20, Miquel Raynal wrote:
> > Wrong address for Boris again, sorry for the noise.
> >   
> > > Hi Lucas, Marco,
> > > 
> > > Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote on Fri, 26 Jul 2019 10:54:11
> > > +0200:
> > >   
> > > > Hi Miguel,
> > > > 
> > > > Am Freitag, den 26.07.2019, 10:28 +0200 schrieb Miquel Raynal:    
> > > > > Hi Marco,
> > > > > 
> > > > > + Richard
> > > > > + Working e-mail address for Boris
> > > > >       
> > > > > > Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote on Fri, 26 Jul 2019      
> > > > > 09:44:34 +0200:
> > > > >       
> > > > > > Some devices don't support ecc "official". By "official" I mean that the    
> > > 
> > >                                  ^ uppercase ECC
> > >   
> > > > > > feature can be set trough the "SET FEATURE (EFh)" command but isn't
> > > > > > reported to the "READ ID Parameter Tables". Because the "ECC Field"
> > > > > > still says that it is disabled. This is applicable at least
> > > > > > for the MT29F2G08ABAGA and MT29F2G08ABBGA devices. Even worse the
> > > > > > datasheet describes the ECC feature in chapter "ECC Protection".    
> > > 
> > > What about:
> > > 
> > > "Some devices are supposed to do not support on-die ECC but
> > > experience shows that internal ECC machinery can actually be enabled
> > > through the "SET FEATURE (EFh)" command, even if a read of the "READ ID
> > > Parameter Tables" returns that it is not."
> > >   
> > > > > > 
> > > > > > Currently the driver checks the "READ ID Parameter" field directly after
> > > > > > we enabled the feature. If the check fails we return immediately but
> > > > > > leave the ECC on. Now all future read/program cycles goes trough the ecc
> > > > > > and the host nfc gets confused and reports ECC errors.    
> > > 
> > > And here:
> > > 
> > > "Currently, the driver checks the "READ ID Parameter" field
> > > directly after having enabled the feature. If the check fails it returns
> > > immediately but leaves the ECC on. When using buggy chips like
> > > MT29F2G08ABAGA and MT29F2G08ABBGA, all future read/program cycles will
> > > go through the on-die ECC, confusing the host controller which is
> > > supposed to be the one handling correction."
> > >   
> > > > > > To address this in a common way we need to turn off the ECC directly
> > > > > > after reading the "READ ID Parameter" and before checking the
> > > > > > "ECC status".
> > > > > > 
> > > > > > Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>      
> > > > > 
> > > > > Good catch! However you report that on-die ECC correction is working
> > > > > but you still disable it; any reason to do so ? Would it be better to
> > > > > actually enable on-die ECC and explicitly mark these two chips as
> > > > > buggy (see [1] for checking the chip IDs)?      
> > > > 
> > > > It's the other way around. The chip is not supposed to have on-die ECC
> > > > according to the datasheet and correctly reflects this fact in the
> > > > READ_ID, so Linux should not try to use the on-die ECC.    
> > > 
> > > Ok I understood the opposite because of the "Even worse the datasheet
> > > describes the ECC feature [...]" which implied to me that the on-die ECC
> > > feature was actually expected despite the status bit not being set.
> > > 
> > > Marco, can you rephrase a bit the commit log? I proposed something,
> > > feel free to adapt.  
> 
> Thanks for the fast reply :) Of course I can adapt it and adding Boris rb-tag.

I suppose you can also add Fixes and Stable tags.

Thanks,
Miquèl

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