| Message ID | 20180124004454.5759-2-miquel.raynal@free-electrons.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | Bring NAND support to Nintendo NES Classic | expand |
Hi, On Wed, Jan 24, 2018 at 01:44:47AM +0100, Miquel Raynal wrote: > When no requirement in Device Tree is given about the ECC strength and > step size, the engine should fallback on the minimal working case for > this engine (16b/1024B) instead of the NAND chip requirement which might > be simply unreachable. > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > --- > drivers/mtd/nand/sunxi_nand.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > index 8bc3828854..e8e7ad8ac5 100644 > --- a/drivers/mtd/nand/sunxi_nand.c > +++ b/drivers/mtd/nand/sunxi_nand.c > @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd, > goto err; > } > > + ecc->strength = strengths[i]; You should have a comment here as well mentionning why you're doing this (your commit log would be a good start :)) Maxime
On Wed, 24 Jan 2018 01:44:47 +0100 Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > When no requirement in Device Tree is given about the ECC strength and > step size, the engine should fallback on the minimal working case for > this engine (16b/1024B) instead of the NAND chip requirement which might > be simply unreachable. Actually, this is not what this patch is fixing. It fixes all cases where the requested ECC strength does not exactly match the strengths supported by the ECC engine. In this case, the driver is selecting the closest strength meeting the 'selected_strength > requested_strength' constraint. The problem was that we were not updating ecc->strength with the real strength, which is what you're fixing here. > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > --- > drivers/mtd/nand/sunxi_nand.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > index 8bc3828854..e8e7ad8ac5 100644 > --- a/drivers/mtd/nand/sunxi_nand.c > +++ b/drivers/mtd/nand/sunxi_nand.c > @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd, > goto err; > } > > + ecc->strength = strengths[i]; > data->mode = i; > > /* HW ECC always request ECC bytes for 1024 bytes blocks */
Hello Boris, On Wed, 24 Jan 2018 08:57:23 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Wed, 24 Jan 2018 01:44:47 +0100 > Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > > > When no requirement in Device Tree is given about the ECC strength and > > step size, the engine should fallback on the minimal working case for > > this engine (16b/1024B) instead of the NAND chip requirement which might > > be simply unreachable. > > Actually, this is not what this patch is fixing. It fixes all cases > where the requested ECC strength does not exactly match the strengths > supported by the ECC engine. In this case, the driver is selecting the > closest strength meeting the 'selected_strength > requested_strength' > constraint. The problem was that we were not updating ecc->strength > with the real strength, which is what you're fixing here. That is entirely right, I kept one explanation on the easiest case where this is needed: when there is not ECC-related property in the DT. > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > > --- > > drivers/mtd/nand/sunxi_nand.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > > index 8bc3828854..e8e7ad8ac5 100644 > > --- a/drivers/mtd/nand/sunxi_nand.c > > +++ b/drivers/mtd/nand/sunxi_nand.c > > @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd, > > goto err; > > } > > > > + ecc->strength = strengths[i]; > > data->mode = i; > > > > /* HW ECC always request ECC bytes for 1024 bytes blocks */ >
Hello Maxime, On Wed, 24 Jan 2018 08:46:18 +0100 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Wed, Jan 24, 2018 at 01:44:47AM +0100, Miquel Raynal wrote: > > When no requirement in Device Tree is given about the ECC strength and > > step size, the engine should fallback on the minimal working case for > > this engine (16b/1024B) instead of the NAND chip requirement which might > > be simply unreachable. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > > --- > > drivers/mtd/nand/sunxi_nand.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c > > index 8bc3828854..e8e7ad8ac5 100644 > > --- a/drivers/mtd/nand/sunxi_nand.c > > +++ b/drivers/mtd/nand/sunxi_nand.c > > @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd, > > goto err; > > } > > > > + ecc->strength = strengths[i]; > > You should have a comment here as well mentionning why you're doing > this (your commit log would be a good start :)) Sure, I also moved that line above, where the strength is selected (with a comment). This way it seems more clear to me why this is needed. Thanks, Miquèl
diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c index 8bc3828854..e8e7ad8ac5 100644 --- a/drivers/mtd/nand/sunxi_nand.c +++ b/drivers/mtd/nand/sunxi_nand.c @@ -1417,6 +1417,7 @@ static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd, goto err; } + ecc->strength = strengths[i]; data->mode = i; /* HW ECC always request ECC bytes for 1024 bytes blocks */
When no requirement in Device Tree is given about the ECC strength and step size, the engine should fallback on the minimal working case for this engine (16b/1024B) instead of the NAND chip requirement which might be simply unreachable. Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> --- drivers/mtd/nand/sunxi_nand.c | 1 + 1 file changed, 1 insertion(+)