Re: [PATCH 2/3] mtd: rawnand: Check the CHANGE_READ_COLUMN from nand_read_subpage() is supported

[Miquel Raynal <miquel.raynal@bootlin.com>]

> > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > > > index 7f29f27bb6fd..8175635b9802 100644
> > > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > > @@ -3065,10 +3065,19 @@ static int nand_read_subpage(struct nand_chip *chip, uint32_t data_offs,
> > > >  		    (busw - 1))
> > > >  			aligned_len++;
> > > >  
> > > > -		ret = nand_change_read_column_op(chip,
> > > > -						 mtd->writesize + aligned_pos,
> > > > -						 &chip->oob_poi[aligned_pos],
> > > > -						 aligned_len, false);
> > > > +		ret = nand_check_change_read_column_op(chip,
> > > > +						       mtd->writesize + aligned_pos,
> > > > +						       &chip->oob_poi[aligned_pos],
> > > > +						       aligned_len, false, true);
> > > > +		if (!ret)
> > > > +			ret = nand_change_read_column_op(chip,
> > > > +							 mtd->writesize + aligned_pos,
> > > > +							 &chip->oob_poi[aligned_pos],
> > > > +							 aligned_len, false);
> > > > +		else
> > > > +			ret = nand_change_read_column_op(chip, mtd->writesize,
> > > > +							 chip->oob_poi,
> > > > +							 mtd->oobsize, false);
> > > >  		if (ret)
> > > >  			return ret;      
> > >     
> > 
> > 
> > The previous behavior was:
> > 
> > /*
> >  * gaps == true means we need to request the entire OOB area and we
> >  * will call an OOB layout helper to extract the ECC bytes.
> >  * gaps == false means there is no data interleaved, we can just read
> >  * the number of ECC bytes by starting at the right offset and no
> >  * extracting operation will be needed.
> >  */
> > gaps = ...;
> > if (!gaps)
> > 	nand_change_read_column(at a specific offset and a specific
> > 				length just to match the ECC bytes);
> > else
> > 	nand_change_read_column(the entire OOB);
> > 
> > While the new behavior is:
> > 
> > if (!gaps) {
> > 	op_supported = nand_check_change_read_column();
> > 	if (!op_supported)
> > 		// same operation as if gaps == true
> > 		nand_change_read_column(the entire OOB);  
> 
> What if this one is not supported either?

If this one is not supported either I guess the entire software ECC
support cannot be used. But what I am trying to achieve here is more
complicated: the controller does not support one thing: reading the ECC
bytes until the OOB end minus 2. Only a call to
nand_change_read_column with these particular parameters would fail
because it depends on the actual offset and length that are requested.

> > 	else
> > 		nand_change_read_column(at a specific offset);
> > } else {
> > 	nand_change_read_column(the entire OOB)
> > }
> >   
> > > So you just fail if the CHANGE column op is not supported? Looks like
> > > this check should be done before we assign ecc->read_subpage to
> > > nand_read_subpage in
> > > nand_set_ecc_on_host_ops()/nand_set_ecc_soft_ops()...    
> > 
> > So I actually don't understand the above comment as the code is not
> > simply failing, it's actually falling back to second method which is
> > maybe slightly slower but still valid. Do you think it's wrong?  
> 
> Well, it's falling back to a different method that might be unsupported
> too, so it might still fail on other controllers.

Well, yes it may fail on other controllers but as I said, if
controllers do not support any type of nand_change_read_column() then
they cannot use software ECC at all (and are close to be almost
useless).

> Moreover, I really
> think this sort of things should be checked at probe time so you don't
> spend time checking it every time you do a read.

Agreed. I can certainly make something like this in theory, but what
would be the condition? I am describing it in patch 3/3 : there is
really a tiny set of conditions where this will fail so in the end I
would end-up writing a condition that precisely matches the Arasan
limitation.

Not mentioning that it only fails with NV-DDR enabled (also something
that I missed to capture in patch 3/3). The data interface being
initialized after the read_subpage() hook it means I couldn't use this
information. 

> Something like:
> 
> 	if (nand_check_change_read_column(gaps=false))
> 		ecc->read_subpage = nand_read_subpage_nogaps;
> 	else if (nand_check_change_read_column(gaps=true))
> 		ecc->read_subpage = nand_read_subpage_gaps;
> 	else
> 		return -EOPNOTSUP;


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