[PATCH] mtd: nand: gpmi: fall back to legacy mode if no ECC information present

[PATCH] mtd: nand: gpmi: fall back to legacy mode if no ECC information present

[Stefan Agner]

In case fsl,use-minimum-ecc is set, the driver tries to determine
ECC layout by using the ECC information provided by the MTD stack.
However, in case the NAND chip does not provide any information,
the driver currently fails with:
  nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
  nand: Macronix NAND 128MiB 3,3V 8-bit
  nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
  gpmi-nand 1806000.gpmi-nand: Error setting BCH geometry : 1
  gpmi-nand: probe of 1806000.gpmi-nand failed with error 1

Fall back to implementation specific default mode if no ECC
information are provided by the NAND chip and fsl,use-minimum-ecc
is specified.

This is more in line with what the device tree binding documentation
promises:
  "However, note that if this strength is not
  discoverable or this property is not enabled,
  the software may chooses an implementation-defined
  ECC scheme."

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 50f8d4a1b983..7b8e8c629081 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -206,7 +206,7 @@ static int set_geometry_by_ecc_info(struct gpmi_nand_data *this)
 	unsigned int block_mark_bit_offset;
 
 	if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
-		return -EINVAL;
+		return -ENOTSUPP;
 
 	switch (chip->ecc_step_ds) {
 	case SZ_512:
@@ -423,11 +423,15 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
 
 int common_nfc_set_geometry(struct gpmi_nand_data *this)
 {
-	if ((of_property_read_bool(this->dev->of_node, "fsl,use-minimum-ecc"))
-				|| legacy_set_geometry(this))
-		return set_geometry_by_ecc_info(this);
+	int ret = -ENOTSUPP;
 
-	return 0;
+	if (of_property_read_bool(this->dev->of_node, "fsl,use-minimum-ecc"))
+		ret = set_geometry_by_ecc_info(this);
+
+	if (ret == -ENOTSUPP)
+		return legacy_set_geometry(this);
+
+	return ret;
 }
 
 struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)
-- 
2.16.1

Re: [PATCH] mtd: nand: gpmi: fall back to legacy mode if no ECC information present

[Boris Brezillon]

Hi Stefan,

On Mon, 29 Jan 2018 15:44:40 +0100
Stefan Agner <stefan@agner.ch> wrote:

> In case fsl,use-minimum-ecc is set, the driver tries to determine
> ECC layout by using the ECC information provided by the MTD stack.
> However, in case the NAND chip does not provide any information,
> the driver currently fails with:
>   nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
>   nand: Macronix NAND 128MiB 3,3V 8-bit
>   nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
>   gpmi-nand 1806000.gpmi-nand: Error setting BCH geometry : 1
>   gpmi-nand: probe of 1806000.gpmi-nand failed with error 1
> 
> Fall back to implementation specific default mode if no ECC
> information are provided by the NAND chip and fsl,use-minimum-ecc
> is specified.

Hm, this sounds a bit fragile: if we ever fix the Macronix driver
(which should be done BTW) to set the appropriate ECC requirements, it
will break all platforms that were relying on this 'fall back to legacy
logic'.
So, if what you really want is legacy_set_geometry(), don't specify
"fsl,use-minimum-ecc" in your DT and you should be good. Otherwise, fix
the Macronix driver to initialize ->ecc_{strength,step_size}_ds
appropriately.

Regards,

Boris

> 
> This is more in line with what the device tree binding documentation
> promises:
>   "However, note that if this strength is not
>   discoverable or this property is not enabled,
>   the software may chooses an implementation-defined
>   ECC scheme."
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 50f8d4a1b983..7b8e8c629081 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -206,7 +206,7 @@ static int set_geometry_by_ecc_info(struct gpmi_nand_data *this)
>  	unsigned int block_mark_bit_offset;
>  
>  	if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
> -		return -EINVAL;
> +		return -ENOTSUPP;
>  
>  	switch (chip->ecc_step_ds) {
>  	case SZ_512:
> @@ -423,11 +423,15 @@ static int legacy_set_geometry(struct gpmi_nand_data *this)
>  
>  int common_nfc_set_geometry(struct gpmi_nand_data *this)
>  {
> -	if ((of_property_read_bool(this->dev->of_node, "fsl,use-minimum-ecc"))
> -				|| legacy_set_geometry(this))
> -		return set_geometry_by_ecc_info(this);
> +	int ret = -ENOTSUPP;
>  
> -	return 0;
> +	if (of_property_read_bool(this->dev->of_node, "fsl,use-minimum-ecc"))
> +		ret = set_geometry_by_ecc_info(this);
> +
> +	if (ret == -ENOTSUPP)
> +		return legacy_set_geometry(this);
> +
> +	return ret;
>  }
>  
>  struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)

Re: [PATCH] mtd: nand: gpmi: fall back to legacy mode if no ECC information present

[stefan]

I accidentally removed ML/cc before, re-adding.

On 31.01.2018 10:57, Boris Brezillon wrote:
> On Wed, 31 Jan 2018 10:19:05 +0100
> stefan@agner.ch wrote:
> 
>> On 30.01.2018 14:23, Boris Brezillon wrote:
>> > Hi Stefan,
>> >
>> > On Mon, 29 Jan 2018 15:44:40 +0100
>> > Stefan Agner <stefan@agner.ch> wrote:
>> >
>> >> In case fsl,use-minimum-ecc is set, the driver tries to determine
>> >> ECC layout by using the ECC information provided by the MTD stack.
>> >> However, in case the NAND chip does not provide any information,
>> >> the driver currently fails with:
>> >>   nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
>> >>   nand: Macronix NAND 128MiB 3,3V 8-bit
>> >>   nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
>> >>   gpmi-nand 1806000.gpmi-nand: Error setting BCH geometry : 1
>> >>   gpmi-nand: probe of 1806000.gpmi-nand failed with error 1
>> >>
>> >> Fall back to implementation specific default mode if no ECC
>> >> information are provided by the NAND chip and fsl,use-minimum-ecc
>> >> is specified.
>> >
>> > Hm, this sounds a bit fragile: if we ever fix the Macronix driver
>> > (which should be done BTW) to set the appropriate ECC requirements, it
>> > will break all platforms that were relying on this 'fall back to legacy
>> > logic'.
>>
>> I see. It is just that downstream behaves that way, hence we sell
>> modules which use minimal ECC on ONFI enabled chips and legacy (maximum
>> ECC which fits into OOB) on modules with non-ONFI chips.
> 
> And I guess you use the same DT for both variants of the board :-/
> 

Actually we only have two SKUs, and they differ also otherwise so I have
two DTs anyway.

>>
>> Currently we operate the above Macronix chip with 8-bit ECC since quite
>> a while.
> 
> Honestly, I don't see a good solution here except adding an extra DT or
> live-patching it from the bootloader, because, even if this hack works
> for you know, it might not in the future.

Extra DT is fine for Linux.

The problem is more with U-Boot, where I tried to add minimal ECC
support via Kconfig symbol and align with Linux behavior. For U-Boot I
would really prefer to have a single binary for all SKUs...

I already sent a first patchset
https://patchwork.ozlabs.org/patch/867180/

I guess it should be somehow possible to do a board specific selection
of ECC. But this is a discussion for another thread.

> 
> In the future, if you plan to have boards with different variants of
> NANDs, I recommend that you always maximize ECC, this way you won't
> have this kind of issues.

Makes sense. Unfortunately, for those products we already ship, changing
would be rather painful.

> 
>>
>> > So, if what you really want is legacy_set_geometry(), don't specify
>> > "fsl,use-minimum-ecc" in your DT and you should be good. Otherwise, fix
>> > the Macronix driver to initialize ->ecc_{strength,step_size}_ds
>> > appropriately.
>>
>> The datasheet says:
>> • High Reliability
>> - Endurance: 100K cycles (with 1-bit ECC per 528-byte)
>>
>> So we would set ecc_strenght to 1?
> 
> If the datasheet says so, then yes, you should have
> ->ecc_strength_ds = 1 and ->ecc_step_size_ds = 512.
> 
>> But then there is almost no room for
>> wear leveling. I remember that I dumped the fixed bits once on such a
>> chip, and there were several blocks from factory which needed one bit
>> fixed...
> 
> Well, that's a different issue. You might want to maximize the ECC
> strength for your specific board. In this case, you should not specify
> "fsl,use-minimum-ecc" in your DT, or, if the driver supports it (but I
> doubt it does), you should add "nand-ecc-maximize". Alternatively, if
> you want to keep some of the OOB space, you can ask for a specific ECC
> config with the "nand-ecc-strength" and "nand-ecc-step-size" properties.

Different issue, but in the end all I care about: Does wear leveling
work properly.

The NAND chip documentation also mentions that typical access is per
page (2K), I guess if one uses a single ECC across the complete page
then 4-bits are available, which should allow a somewhat decent wear
leveling.

I guess we can go with nand-ecc-strength/nand-ecc-step-size for that
chip for now.

However, in Linux we should at least fix the device tree bindings
documentation for "fsl,use-minimum-ecc" then.

--
Stefan

Re: [PATCH] mtd: nand: gpmi: fall back to legacy mode if no ECC information present

[stefan]

Hi Boris,

[Also adding Huang]

On 31.01.2018 22:18, stefan@agner.ch wrote:
> I accidentally removed ML/cc before, re-adding.
> 
> On 31.01.2018 10:57, Boris Brezillon wrote:
>> On Wed, 31 Jan 2018 10:19:05 +0100
>> stefan@agner.ch wrote:
>>
>>> On 30.01.2018 14:23, Boris Brezillon wrote:
>>> > Hi Stefan,
>>> >
>>> > On Mon, 29 Jan 2018 15:44:40 +0100
>>> > Stefan Agner <stefan@agner.ch> wrote:
>>> >
>>> >> In case fsl,use-minimum-ecc is set, the driver tries to determine
>>> >> ECC layout by using the ECC information provided by the MTD stack.
>>> >> However, in case the NAND chip does not provide any information,
>>> >> the driver currently fails with:
>>> >>   nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
>>> >>   nand: Macronix NAND 128MiB 3,3V 8-bit
>>> >>   nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
>>> >>   gpmi-nand 1806000.gpmi-nand: Error setting BCH geometry : 1
>>> >>   gpmi-nand: probe of 1806000.gpmi-nand failed with error 1
>>> >>
>>> >> Fall back to implementation specific default mode if no ECC
>>> >> information are provided by the NAND chip and fsl,use-minimum-ecc
>>> >> is specified.
>>> >
>>> > Hm, this sounds a bit fragile: if we ever fix the Macronix driver
>>> > (which should be done BTW) to set the appropriate ECC requirements, it
>>> > will break all platforms that were relying on this 'fall back to legacy
>>> > logic'.
>>>
>>> I see. It is just that downstream behaves that way, hence we sell
>>> modules which use minimal ECC on ONFI enabled chips and legacy (maximum
>>> ECC which fits into OOB) on modules with non-ONFI chips.
>>
>> And I guess you use the same DT for both variants of the board :-/
>>
> 
> Actually we only have two SKUs, and they differ also otherwise so I have
> two DTs anyway.
> 
>>>
>>> Currently we operate the above Macronix chip with 8-bit ECC since quite
>>> a while.
>>
>> Honestly, I don't see a good solution here except adding an extra DT or
>> live-patching it from the bootloader, because, even if this hack works
>> for you know, it might not in the future.
> 
> Extra DT is fine for Linux.
> 
> The problem is more with U-Boot, where I tried to add minimal ECC
> support via Kconfig symbol and align with Linux behavior. For U-Boot I
> would really prefer to have a single binary for all SKUs...
> 
> I already sent a first patchset
> https://patchwork.ozlabs.org/patch/867180/
> 
> I guess it should be somehow possible to do a board specific selection
> of ECC. But this is a discussion for another thread.
> 
>>
>> In the future, if you plan to have boards with different variants of
>> NANDs, I recommend that you always maximize ECC, this way you won't
>> have this kind of issues.
> 
> Makes sense. Unfortunately, for those products we already ship, changing
> would be rather painful.
> 
>>
>>>
>>> > So, if what you really want is legacy_set_geometry(), don't specify
>>> > "fsl,use-minimum-ecc" in your DT and you should be good. Otherwise, fix
>>> > the Macronix driver to initialize ->ecc_{strength,step_size}_ds
>>> > appropriately.
>>>
>>> The datasheet says:
>>> • High Reliability
>>> - Endurance: 100K cycles (with 1-bit ECC per 528-byte)
>>>
>>> So we would set ecc_strenght to 1?
>>
>> If the datasheet says so, then yes, you should have
>> ->ecc_strength_ds = 1 and ->ecc_step_size_ds = 512.
>>
>>> But then there is almost no room for
>>> wear leveling. I remember that I dumped the fixed bits once on such a
>>> chip, and there were several blocks from factory which needed one bit
>>> fixed...
>>
>> Well, that's a different issue. You might want to maximize the ECC
>> strength for your specific board. In this case, you should not specify
>> "fsl,use-minimum-ecc" in your DT, or, if the driver supports it (but I
>> doubt it does), you should add "nand-ecc-maximize". Alternatively, if
>> you want to keep some of the OOB space, you can ask for a specific ECC
>> config with the "nand-ecc-strength" and "nand-ecc-step-size" properties.
> 
> Different issue, but in the end all I care about: Does wear leveling
> work properly.
> 
> The NAND chip documentation also mentions that typical access is per
> page (2K), I guess if one uses a single ECC across the complete page
> then 4-bits are available, which should allow a somewhat decent wear
> leveling.
> 
> I guess we can go with nand-ecc-strength/nand-ecc-step-size for that
> chip for now.

This seems not to be the case for the driver in question gpmi_nand_init
calls:
nand_scan_ident -> nand_dt_init (which fills
chip->ecc.strength/chip->ecc.size)

then

gpmi_init_last -> gpmi_set_geometry -> bch_set_geometry ->
legacy_set_geometry/set_geometry_by_ecc_info

In both cases struct bch_geometry is calculated and overwrites
ecc.strength/ecc.size (without considering either of them,
set_geometry_by_ecc_info is considering ecc_strength_ds/ecc_step_ds
though).

I guess we would have to add a third option in case device tree
specifies strength/size, and validate whether it can be reasonably
fulfilled?

E.g. extend common_nfc_set_geometry:


 int common_nfc_set_geometry(struct gpmi_nand_data *this)
 {
+	struct nand_chip *chip = &this->nand;
+
+	if (chip->ecc.strength set && chip->ecc.strength set)
+		return set_geometry_by_ecc_dt_info(this);
+
 	if ((of_property_read_bool(this->dev->of_node, "fsl,use-minimum-ecc"))
 				|| legacy_set_geometry(this))
 		return set_geometry_by_ecc_info(this);
 
 	return 0;
 }

--
Stefan


> 
> However, in Linux we should at least fix the device tree bindings
> documentation for "fsl,use-minimum-ecc" then.
> 
> --
> Stefan

Re: [PATCH] mtd: nand: gpmi: fall back to legacy mode if no ECC information present

[Boris Brezillon]

On Mon, 05 Feb 2018 23:16:57 +0100
stefan@agner.ch wrote:

> Hi Boris,
> 
> [Also adding Huang]
> 
> On 31.01.2018 22:18, stefan@agner.ch wrote:
> > I accidentally removed ML/cc before, re-adding.
> > 
> > On 31.01.2018 10:57, Boris Brezillon wrote:  
> >> On Wed, 31 Jan 2018 10:19:05 +0100
> >> stefan@agner.ch wrote:
> >>  
> >>> On 30.01.2018 14:23, Boris Brezillon wrote:  
> >>> > Hi Stefan,
> >>> >
> >>> > On Mon, 29 Jan 2018 15:44:40 +0100
> >>> > Stefan Agner <stefan@agner.ch> wrote:
> >>> >  
> >>> >> In case fsl,use-minimum-ecc is set, the driver tries to determine
> >>> >> ECC layout by using the ECC information provided by the MTD stack.
> >>> >> However, in case the NAND chip does not provide any information,
> >>> >> the driver currently fails with:
> >>> >>   nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
> >>> >>   nand: Macronix NAND 128MiB 3,3V 8-bit
> >>> >>   nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
> >>> >>   gpmi-nand 1806000.gpmi-nand: Error setting BCH geometry : 1
> >>> >>   gpmi-nand: probe of 1806000.gpmi-nand failed with error 1
> >>> >>
> >>> >> Fall back to implementation specific default mode if no ECC
> >>> >> information are provided by the NAND chip and fsl,use-minimum-ecc
> >>> >> is specified.  
> >>> >
> >>> > Hm, this sounds a bit fragile: if we ever fix the Macronix driver
> >>> > (which should be done BTW) to set the appropriate ECC requirements, it
> >>> > will break all platforms that were relying on this 'fall back to legacy
> >>> > logic'.  
> >>>
> >>> I see. It is just that downstream behaves that way, hence we sell
> >>> modules which use minimal ECC on ONFI enabled chips and legacy (maximum
> >>> ECC which fits into OOB) on modules with non-ONFI chips.  
> >>
> >> And I guess you use the same DT for both variants of the board :-/
> >>  
> > 
> > Actually we only have two SKUs, and they differ also otherwise so I have
> > two DTs anyway.
> >   
> >>>
> >>> Currently we operate the above Macronix chip with 8-bit ECC since quite
> >>> a while.  
> >>
> >> Honestly, I don't see a good solution here except adding an extra DT or
> >> live-patching it from the bootloader, because, even if this hack works
> >> for you know, it might not in the future.  
> > 
> > Extra DT is fine for Linux.
> > 
> > The problem is more with U-Boot, where I tried to add minimal ECC
> > support via Kconfig symbol and align with Linux behavior. For U-Boot I
> > would really prefer to have a single binary for all SKUs...
> > 
> > I already sent a first patchset
> > https://patchwork.ozlabs.org/patch/867180/
> > 
> > I guess it should be somehow possible to do a board specific selection
> > of ECC. But this is a discussion for another thread.
> >   
> >>
> >> In the future, if you plan to have boards with different variants of
> >> NANDs, I recommend that you always maximize ECC, this way you won't
> >> have this kind of issues.  
> > 
> > Makes sense. Unfortunately, for those products we already ship, changing
> > would be rather painful.
> >   
> >>  
> >>>  
> >>> > So, if what you really want is legacy_set_geometry(), don't specify
> >>> > "fsl,use-minimum-ecc" in your DT and you should be good. Otherwise, fix
> >>> > the Macronix driver to initialize ->ecc_{strength,step_size}_ds
> >>> > appropriately.  
> >>>
> >>> The datasheet says:
> >>> • High Reliability
> >>> - Endurance: 100K cycles (with 1-bit ECC per 528-byte)
> >>>
> >>> So we would set ecc_strenght to 1?  
> >>
> >> If the datasheet says so, then yes, you should have  
> >> ->ecc_strength_ds = 1 and ->ecc_step_size_ds = 512.  
> >>  
> >>> But then there is almost no room for
> >>> wear leveling. I remember that I dumped the fixed bits once on such a
> >>> chip, and there were several blocks from factory which needed one bit
> >>> fixed...  
> >>
> >> Well, that's a different issue. You might want to maximize the ECC
> >> strength for your specific board. In this case, you should not specify
> >> "fsl,use-minimum-ecc" in your DT, or, if the driver supports it (but I
> >> doubt it does), you should add "nand-ecc-maximize". Alternatively, if
> >> you want to keep some of the OOB space, you can ask for a specific ECC
> >> config with the "nand-ecc-strength" and "nand-ecc-step-size" properties.  
> > 
> > Different issue, but in the end all I care about: Does wear leveling
> > work properly.
> > 
> > The NAND chip documentation also mentions that typical access is per
> > page (2K), I guess if one uses a single ECC across the complete page
> > then 4-bits are available, which should allow a somewhat decent wear
> > leveling.
> > 
> > I guess we can go with nand-ecc-strength/nand-ecc-step-size for that
> > chip for now.  
> 
> This seems not to be the case for the driver in question gpmi_nand_init
> calls:
> nand_scan_ident -> nand_dt_init (which fills
> chip->ecc.strength/chip->ecc.size)
> 
> then
> 
> gpmi_init_last -> gpmi_set_geometry -> bch_set_geometry ->
> legacy_set_geometry/set_geometry_by_ecc_info
> 
> In both cases struct bch_geometry is calculated and overwrites
> ecc.strength/ecc.size (without considering either of them,
> set_geometry_by_ecc_info is considering ecc_strength_ds/ecc_step_ds
> though).
> 
> I guess we would have to add a third option in case device tree
> specifies strength/size, and validate whether it can be reasonably
> fulfilled?
> 
> E.g. extend common_nfc_set_geometry:
> 
> 
>  int common_nfc_set_geometry(struct gpmi_nand_data *this)
>  {
> +	struct nand_chip *chip = &this->nand;
> +
> +	if (chip->ecc.strength set && chip->ecc.strength set)
> +		return set_geometry_by_ecc_dt_info(this);
> +
>  	if ((of_property_read_bool(this->dev->of_node, "fsl,use-minimum-ecc"))
>  				|| legacy_set_geometry(this))
>  		return set_geometry_by_ecc_info(this);
>  
>  	return 0;
>  }

Or you can just patch set_geometry_by_ecc_info() to use
chip->ecc.strength and chip->ecc.size if they are set and fall back to 
chip->ecc_strength_ds and chip->ecc_step_ds when they're not:

--->8---
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index ab9a0a2ed3b2..ded4b7389960 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -204,11 +204,19 @@ static int set_geometry_by_ecc_info(struct gpmi_nand_data *this)
        struct nand_chip *chip = &this->nand;
        struct mtd_info *mtd = nand_to_mtd(chip);
        unsigned int block_mark_bit_offset;
-
-       if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
+       unsigned int ecc_strength, ecc_step;
+
+       if (chip->ecc.strength > 0 && chip->ecc.size > 0) {
+               ecc_strength = chip->ecc.strength;
+               ecc_step = chip->ecc.size;
+       } else if (chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0) {
+               ecc_strength = chip->ecc_strength_ds;
+               ecc_step = chip->ecc_step_ds;
+       } else {
                return -EINVAL;
+       }
 
-       switch (chip->ecc_step_ds) {
+       switch (ecc_step) {
        case SZ_512:
                geo->gf_len = 13;
                break;
@@ -218,11 +226,11 @@ static int set_geometry_by_ecc_info(struct gpmi_nand_data *this)
        default:
                dev_err(this->dev,
                        "unsupported nand chip. ecc bits : %d, ecc size : %d\n",
-                       chip->ecc_strength_ds, chip->ecc_step_ds);
+                       ecc_strength, ecc_step);
                return -EINVAL;
        }
-       geo->ecc_chunk_size = chip->ecc_step_ds;
-       geo->ecc_strength = round_up(chip->ecc_strength_ds, 2);
+       geo->ecc_chunk_size = ecc_step;
+       geo->ecc_strength = round_up(ecc_strength, 2);
        if (!gpmi_check_ecc(this))
                return -EINVAL;
 
@@ -230,10 +238,12 @@ static int set_geometry_by_ecc_info(struct gpmi_nand_data *this)
        if (geo->ecc_chunk_size < mtd->oobsize) {
                dev_err(this->dev,
                        "unsupported nand chip. ecc size: %d, oob size : %d\n",
-                       chip->ecc_step_ds, mtd->oobsize);
+                       ecc_step, mtd->oobsize);
                return -EINVAL;
        }
 
+       chip->ecc.strength = geo->ecc_strength;
+
        /* The default value, see comment in the legacy_set_geometry(). */
        geo->metadata_size = 10;

Re: [PATCH] mtd: nand: gpmi: fall back to legacy mode if no ECC information present

[stefan]

On 06.02.2018 16:40, Boris Brezillon wrote:
> On Mon, 05 Feb 2018 23:16:57 +0100
> stefan@agner.ch wrote:
> 
>> Hi Boris,
>>
>> [Also adding Huang]
>>
>> On 31.01.2018 22:18, stefan@agner.ch wrote:
>> > I accidentally removed ML/cc before, re-adding.
>> >
>> > On 31.01.2018 10:57, Boris Brezillon wrote:
>> >> On Wed, 31 Jan 2018 10:19:05 +0100
>> >> stefan@agner.ch wrote:
>> >>
>> >>> On 30.01.2018 14:23, Boris Brezillon wrote:
>> >>> > Hi Stefan,
>> >>> >
>> >>> > On Mon, 29 Jan 2018 15:44:40 +0100
>> >>> > Stefan Agner <stefan@agner.ch> wrote:
>> >>> >
>> >>> >> In case fsl,use-minimum-ecc is set, the driver tries to determine
>> >>> >> ECC layout by using the ECC information provided by the MTD stack.
>> >>> >> However, in case the NAND chip does not provide any information,
>> >>> >> the driver currently fails with:
>> >>> >>   nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
>> >>> >>   nand: Macronix NAND 128MiB 3,3V 8-bit
>> >>> >>   nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
>> >>> >>   gpmi-nand 1806000.gpmi-nand: Error setting BCH geometry : 1
>> >>> >>   gpmi-nand: probe of 1806000.gpmi-nand failed with error 1
>> >>> >>
>> >>> >> Fall back to implementation specific default mode if no ECC
>> >>> >> information are provided by the NAND chip and fsl,use-minimum-ecc
>> >>> >> is specified.
>> >>> >
>> >>> > Hm, this sounds a bit fragile: if we ever fix the Macronix driver
>> >>> > (which should be done BTW) to set the appropriate ECC requirements, it
>> >>> > will break all platforms that were relying on this 'fall back to legacy
>> >>> > logic'.
>> >>>
>> >>> I see. It is just that downstream behaves that way, hence we sell
>> >>> modules which use minimal ECC on ONFI enabled chips and legacy (maximum
>> >>> ECC which fits into OOB) on modules with non-ONFI chips.
>> >>
>> >> And I guess you use the same DT for both variants of the board :-/
>> >>
>> >
>> > Actually we only have two SKUs, and they differ also otherwise so I have
>> > two DTs anyway.
>> >
>> >>>
>> >>> Currently we operate the above Macronix chip with 8-bit ECC since quite
>> >>> a while.
>> >>
>> >> Honestly, I don't see a good solution here except adding an extra DT or
>> >> live-patching it from the bootloader, because, even if this hack works
>> >> for you know, it might not in the future.
>> >
>> > Extra DT is fine for Linux.
>> >
>> > The problem is more with U-Boot, where I tried to add minimal ECC
>> > support via Kconfig symbol and align with Linux behavior. For U-Boot I
>> > would really prefer to have a single binary for all SKUs...
>> >
>> > I already sent a first patchset
>> > https://patchwork.ozlabs.org/patch/867180/
>> >
>> > I guess it should be somehow possible to do a board specific selection
>> > of ECC. But this is a discussion for another thread.
>> >
>> >>
>> >> In the future, if you plan to have boards with different variants of
>> >> NANDs, I recommend that you always maximize ECC, this way you won't
>> >> have this kind of issues.
>> >
>> > Makes sense. Unfortunately, for those products we already ship, changing
>> > would be rather painful.
>> >
>> >>
>> >>>
>> >>> > So, if what you really want is legacy_set_geometry(), don't specify
>> >>> > "fsl,use-minimum-ecc" in your DT and you should be good. Otherwise, fix
>> >>> > the Macronix driver to initialize ->ecc_{strength,step_size}_ds
>> >>> > appropriately.
>> >>>
>> >>> The datasheet says:
>> >>> • High Reliability
>> >>> - Endurance: 100K cycles (with 1-bit ECC per 528-byte)
>> >>>
>> >>> So we would set ecc_strenght to 1?
>> >>
>> >> If the datasheet says so, then yes, you should have
>> >> ->ecc_strength_ds = 1 and ->ecc_step_size_ds = 512.
>> >>
>> >>> But then there is almost no room for
>> >>> wear leveling. I remember that I dumped the fixed bits once on such a
>> >>> chip, and there were several blocks from factory which needed one bit
>> >>> fixed...
>> >>
>> >> Well, that's a different issue. You might want to maximize the ECC
>> >> strength for your specific board. In this case, you should not specify
>> >> "fsl,use-minimum-ecc" in your DT, or, if the driver supports it (but I
>> >> doubt it does), you should add "nand-ecc-maximize". Alternatively, if
>> >> you want to keep some of the OOB space, you can ask for a specific ECC
>> >> config with the "nand-ecc-strength" and "nand-ecc-step-size" properties.
>> >
>> > Different issue, but in the end all I care about: Does wear leveling
>> > work properly.
>> >
>> > The NAND chip documentation also mentions that typical access is per
>> > page (2K), I guess if one uses a single ECC across the complete page
>> > then 4-bits are available, which should allow a somewhat decent wear
>> > leveling.
>> >
>> > I guess we can go with nand-ecc-strength/nand-ecc-step-size for that
>> > chip for now.
>>
>> This seems not to be the case for the driver in question gpmi_nand_init
>> calls:
>> nand_scan_ident -> nand_dt_init (which fills
>> chip->ecc.strength/chip->ecc.size)
>>
>> then
>>
>> gpmi_init_last -> gpmi_set_geometry -> bch_set_geometry ->
>> legacy_set_geometry/set_geometry_by_ecc_info
>>
>> In both cases struct bch_geometry is calculated and overwrites
>> ecc.strength/ecc.size (without considering either of them,
>> set_geometry_by_ecc_info is considering ecc_strength_ds/ecc_step_ds
>> though).
>>
>> I guess we would have to add a third option in case device tree
>> specifies strength/size, and validate whether it can be reasonably
>> fulfilled?
>>
>> E.g. extend common_nfc_set_geometry:
>>
>>
>>  int common_nfc_set_geometry(struct gpmi_nand_data *this)
>>  {
>> +	struct nand_chip *chip = &this->nand;
>> +
>> +	if (chip->ecc.strength set && chip->ecc.strength set)
>> +		return set_geometry_by_ecc_dt_info(this);
>> +
>>  	if ((of_property_read_bool(this->dev->of_node, "fsl,use-minimum-ecc"))
>>  				|| legacy_set_geometry(this))
>>  		return set_geometry_by_ecc_info(this);
>>
>>  	return 0;
>>  }
> 
> Or you can just patch set_geometry_by_ecc_info() to use
> chip->ecc.strength and chip->ecc.size if they are set and fall back to 
> chip->ecc_strength_ds and chip->ecc_step_ds when they're not:

Hm, that sounds reasonable, will give this at try. Thanks!

--
Stefan

> 
> --->8---
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index ab9a0a2ed3b2..ded4b7389960 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -204,11 +204,19 @@ static int set_geometry_by_ecc_info(struct
> gpmi_nand_data *this)
>         struct nand_chip *chip = &this->nand;
>         struct mtd_info *mtd = nand_to_mtd(chip);
>         unsigned int block_mark_bit_offset;
> -
> -       if (!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0))
> +       unsigned int ecc_strength, ecc_step;
> +
> +       if (chip->ecc.strength > 0 && chip->ecc.size > 0) {
> +               ecc_strength = chip->ecc.strength;
> +               ecc_step = chip->ecc.size;
> +       } else if (chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0) {
> +               ecc_strength = chip->ecc_strength_ds;
> +               ecc_step = chip->ecc_step_ds;
> +       } else {
>                 return -EINVAL;
> +       }
>  
> -       switch (chip->ecc_step_ds) {
> +       switch (ecc_step) {
>         case SZ_512:
>                 geo->gf_len = 13;
>                 break;
> @@ -218,11 +226,11 @@ static int set_geometry_by_ecc_info(struct
> gpmi_nand_data *this)
>         default:
>                 dev_err(this->dev,
>                         "unsupported nand chip. ecc bits : %d, ecc size : %d\n",
> -                       chip->ecc_strength_ds, chip->ecc_step_ds);
> +                       ecc_strength, ecc_step);
>                 return -EINVAL;
>         }
> -       geo->ecc_chunk_size = chip->ecc_step_ds;
> -       geo->ecc_strength = round_up(chip->ecc_strength_ds, 2);
> +       geo->ecc_chunk_size = ecc_step;
> +       geo->ecc_strength = round_up(ecc_strength, 2);
>         if (!gpmi_check_ecc(this))
>                 return -EINVAL;
>  
> @@ -230,10 +238,12 @@ static int set_geometry_by_ecc_info(struct
> gpmi_nand_data *this)
>         if (geo->ecc_chunk_size < mtd->oobsize) {
>                 dev_err(this->dev,
>                         "unsupported nand chip. ecc size: %d, oob size : %d\n",
> -                       chip->ecc_step_ds, mtd->oobsize);
> +                       ecc_step, mtd->oobsize);
>                 return -EINVAL;
>         }
>  
> +       chip->ecc.strength = geo->ecc_strength;
> +
>         /* The default value, see comment in the legacy_set_geometry(). */
>         geo->metadata_size = 10;