[PATCH 1/3] mtd: rawnand: Add a helper to check if a CHANGE_READ_COLUMN is possible

[PATCH 1/3] mtd: rawnand: Add a helper to check if a CHANGE_READ_COLUMN is possible

[Miquel Raynal]

Certain controllers are limited and cannot perform all type of
CHANGE_READ_COLUMN. Let's add a helper that allows checking this.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 15 +++++++++++++++
 include/linux/mtd/rawnand.h      |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 57a583149cc0..7f29f27bb6fd 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -1312,6 +1312,15 @@ int nand_read_param_page_op(struct nand_chip *chip, u8 page, void *buf,
 int nand_change_read_column_op(struct nand_chip *chip,
 			       unsigned int offset_in_page, void *buf,
 			       unsigned int len, bool force_8bit)
+{
+	return nand_check_change_read_column_op(chip, offset_in_page, buf, len,
+						force_8bit, false);
+}
+
+int nand_check_change_read_column_op(struct nand_chip *chip,
+				     unsigned int offset_in_page, void *buf,
+				     unsigned int len, bool force_8bit,
+				     bool check_only)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
@@ -1349,9 +1358,15 @@ int nand_change_read_column_op(struct nand_chip *chip,
 
 		instrs[3].ctx.data.force_8bit = force_8bit;
 
+		if (check_only)
+			return nand_check_op(chip, &op);
+
 		return nand_exec_op(chip, &op);
 	}
 
+	if (check_only)
+		return 0;
+
 	chip->legacy.cmdfunc(chip, NAND_CMD_RNDOUT, offset_in_page, -1);
 	if (len)
 		chip->legacy.read_buf(chip, buf, len);
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index b2f9dd3cbd69..a453386aee70 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1523,6 +1523,10 @@ int nand_read_page_op(struct nand_chip *chip, unsigned int page,
 int nand_change_read_column_op(struct nand_chip *chip,
 			       unsigned int offset_in_page, void *buf,
 			       unsigned int len, bool force_8bit);
+int nand_check_change_read_column_op(struct nand_chip *chip,
+				     unsigned int offset_in_page, void *buf,
+				     unsigned int len, bool force_8bit,
+				     bool check_only);
 int nand_read_oob_op(struct nand_chip *chip, unsigned int page,
 		     unsigned int offset_in_page, void *buf, unsigned int len);
 int nand_prog_page_begin_op(struct nand_chip *chip, unsigned int page,
-- 
2.27.0


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

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

[Miquel Raynal]

There are constraint controllers which do not support
CHANGE_READ_COLUMNs at any offset. In particular, the offset alignment
might be an issue. Ensure that the operation is supported before trying
it.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

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;
 	}
-- 
2.27.0


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

[PATCH 3/3] mtd: rawnand: arasan: Provide an additional ->exec_op() check

[Miquel Raynal]

The controller only supports data payload requests which are multiple of
4. Rounding up will not work if we reached the end of the device and the
controller will just refuse the request. Hence any unaligned data request
that ends at the device boundary should be refused.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/arasan-nand-controller.c | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/mtd/nand/raw/arasan-nand-controller.c b/drivers/mtd/nand/raw/arasan-nand-controller.c
index 9cbcc698c64d..8377994f3c49 100644
--- a/drivers/mtd/nand/raw/arasan-nand-controller.c
+++ b/drivers/mtd/nand/raw/arasan-nand-controller.c
@@ -891,6 +891,7 @@ static const struct nand_op_parser anfc_op_parser = NAND_OP_PARSER(
 static int anfc_check_op(struct nand_chip *chip,
 			 const struct nand_operation *op)
 {
+	struct mtd_info *mtd = nand_to_mtd(chip);
 	const struct nand_op_instr *instr;
 	int op_id;
 
@@ -940,6 +941,35 @@ static int anfc_check_op(struct nand_chip *chip,
 	    op->instrs[1].type == NAND_OP_DATA_IN_INSTR)
 		return -ENOTSUPP;
 
+	/*
+	 * The controller only supports data payload requests which are a
+	 * multiple of 4. This may confuse the core as the core could request a
+	 * given number of bytes and then another number of bytes without
+	 * re-synchronizing the pointer. In practice, most data accesses are
+	 * 4-byte aligned and thus this is not an issue in practice. However,
+	 * rounding up will not work if we reached the end of the device. Any
+	 * unaligned data request that ends at the device boundary would confuse
+	 * the controller and cannot be performed.
+	 *
+	 * TODO: The nand_op_parser framework should be extended to
+	 * support custom checks on DATA instructions.
+	 */
+	if (op->ninstrs == 4 &&
+	    op->instrs[0].type == NAND_OP_CMD_INSTR &&
+	    op->instrs[1].type == NAND_OP_ADDR_INSTR &&
+	    op->instrs[1].ctx.addr.naddrs == 2 &&
+	    op->instrs[2].type == NAND_OP_CMD_INSTR &&
+	    op->instrs[3].type == NAND_OP_DATA_IN_INSTR) {
+		unsigned int start_off, end_off;
+
+		start_off = (op->instrs[1].ctx.addr.addrs[1] << 8) +
+			    op->instrs[1].ctx.addr.addrs[0];
+		end_off = start_off + round_up(op->instrs[3].ctx.data.len, 4);
+
+		if (end_off >= mtd->writesize + mtd->oobsize)
+			return -ENOTSUPP;
+	}
+
 	return nand_op_parser_exec_op(chip, &anfc_op_parser, op, true);
 }
 
-- 
2.27.0


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

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

[Boris Brezillon]

On Mon,  6 Sep 2021 15:29:41 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> There are constraint controllers which do not support
> CHANGE_READ_COLUMNs at any offset. In particular, the offset alignment
> might be an issue. Ensure that the operation is supported before trying
> it.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> 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;

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()...

>  	}


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

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

[Miquel Raynal]

Hi Boris,

boris.brezillon@collabora.com wrote on Mon, 6 Sep 2021 17:08:27 +0200:

> On Mon,  6 Sep 2021 15:29:41 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > There are constraint controllers which do not support
> > CHANGE_READ_COLUMNs at any offset. In particular, the offset alignment
> > might be an issue. Ensure that the operation is supported before trying
> > it.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > 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);
	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?

Thanks,
Miquèl

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

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

[Boris Brezillon]

On Mon, 6 Sep 2021 18:13:41 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> boris.brezillon@collabora.com wrote on Mon, 6 Sep 2021 17:08:27 +0200:
> 
> > On Mon,  6 Sep 2021 15:29:41 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > There are constraint controllers which do not support
> > > CHANGE_READ_COLUMNs at any offset. In particular, the offset alignment
> > > might be an issue. Ensure that the operation is supported before trying
> > > it.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/mtd/nand/raw/nand_base.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > 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?

> 	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. 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.

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/

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

[Miquel Raynal]

> > > > 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/

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

[Boris Brezillon]

On Mon, 6 Sep 2021 19:32:55 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> > > > > 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.

I suspect those parameters can be extracted from the NAND page size/ECC
config, which are known before the ECC initialization IIRC.

> 
> > > 	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).

Which would be good to detect at probe time, with a probe failure when
this happens.

> 
> > 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.

I see. Maybe it's simpler to add NAND_NO_SUBPAGE_READ flag until you
find a better way to describe such limitations.

> 
> 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.

Duh, this is nasty, indeed.

> 
> > 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/