[RFC PATCH v1 0/2] support 512B ECC step size for Meson NAND

[RFC PATCH v1 0/2] support 512B ECC step size for Meson NAND

[Arseniy Krasnov]

Hello,

this patchset adds support for 512B ECC step size for Meson NAND. Current
implementation only supports 1024B. There are two patches:

1) Update for device tree bindings to replace 'const' type of field
   'nand-ecc-step-size' with 'enum' which contains 512 and 1024. Example
   is also updated.

2) Update for Meson driver - new enum value for 512B ECC and reworked
   ECC capabilities structure to support both 512B and 1024B ECC. By
   default this driver uses 1024B ECC, 512B could be enabled in device
   tree.

Arseniy Krasnov (2):
  dt-bindings: nand: meson: support for 512B ECC step size
  mtd: rawnand: meson: support for 512B ECC step size

 .../bindings/mtd/amlogic,meson-nand.yaml      |  3 +-
 drivers/mtd/nand/raw/meson_nand.c             | 47 ++++++++++++++-----
 2 files changed, 37 insertions(+), 13 deletions(-)

-- 
2.35.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v1 1/2] dt-bindings: nand: meson: support for 512B ECC step size

[Arseniy Krasnov]

Meson NAND supports both 512B and 1024B ECC step size, so replace
'const' for only 1024B step size with enum for both sizes.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
index 3bec8af91bbb..d42daa285d17 100644
--- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
+++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
@@ -49,7 +49,7 @@ patternProperties:
         const: hw
 
       nand-ecc-step-size:
-        const: 1024
+        enum: [512, 1024]
 
       nand-ecc-strength:
         enum: [8, 16, 24, 30, 40, 50, 60]
@@ -93,6 +93,7 @@ examples:
       nand@0 {
         reg = <0>;
         nand-rb = <0>;
+        nand-ecc-step-size = <1024>;
       };
     };
 
-- 
2.35.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 1/2] dt-bindings: nand: meson: support for 512B ECC step size

[Rob Herring]

On Wed, 28 Jun 2023 12:29:35 +0300, Arseniy Krasnov wrote:
> Meson NAND supports both 512B and 1024B ECC step size, so replace
> 'const' for only 1024B step size with enum for both sizes.
> 
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 1/2] dt-bindings: nand: meson: support for 512B ECC step size

[Miquel Raynal]

Hi Arseniy,

AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:35 +0300:

> Meson NAND supports both 512B and 1024B ECC step size, so replace
> 'const' for only 1024B step size with enum for both sizes.
> 
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
> index 3bec8af91bbb..d42daa285d17 100644
> --- a/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
> +++ b/Documentation/devicetree/bindings/mtd/amlogic,meson-nand.yaml
> @@ -49,7 +49,7 @@ patternProperties:
>          const: hw
>  
>        nand-ecc-step-size:
> -        const: 1024
> +        enum: [512, 1024]

I believe you should as well mention the default value.

>        nand-ecc-strength:
>          enum: [8, 16, 24, 30, 40, 50, 60]
> @@ -93,6 +93,7 @@ examples:
>        nand@0 {
>          reg = <0>;
>          nand-rb = <0>;
> +        nand-ecc-step-size = <1024>;

And I don't think this line is really required, but I don't mind.

>        };
>      };
>  


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Arseniy Krasnov]

Meson NAND supports both 512B and 1024B ECC step size.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index 345212e8c691..6cc4f63b86c8 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
 struct meson_nand_ecc {
 	u32 bch;
 	u32 strength;
+	u32 size;
 };
 
 struct meson_nfc_data {
@@ -190,7 +191,8 @@ struct meson_nfc {
 };
 
 enum {
-	NFC_ECC_BCH8_1K		= 2,
+	NFC_ECC_BCH8_512	= 1,
+	NFC_ECC_BCH8_1K,
 	NFC_ECC_BCH24_1K,
 	NFC_ECC_BCH30_1K,
 	NFC_ECC_BCH40_1K,
@@ -198,15 +200,16 @@ enum {
 	NFC_ECC_BCH60_1K,
 };
 
-#define MESON_ECC_DATA(b, s)	{ .bch = (b),	.strength = (s)}
+#define MESON_ECC_DATA(b, s, sz)	{ .bch = (b), .strength = (s), .size = (sz) }
 
 static struct meson_nand_ecc meson_ecc[] = {
-	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
-	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
-	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
-	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
-	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
-	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
+	MESON_ECC_DATA(NFC_ECC_BCH8_512, 8,  512),
+	MESON_ECC_DATA(NFC_ECC_BCH8_1K,  8,  1024),
+	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
+	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
+	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
+	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
+	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
 };
 
 static int meson_nand_calc_ecc_bytes(int step_size, int strength)
@@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
 
 NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
 		     meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
-NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
-		     meson_nand_calc_ecc_bytes, 1024, 8);
+
+static const int axg_stepinfo_strengths[] = { 8 };
+static const struct nand_ecc_step_info axg_stepinfo_1024 = {
+	.stepsize = 1024,
+	.strengths = axg_stepinfo_strengths,
+	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
+};
+
+static const struct nand_ecc_step_info axg_stepinfo_512 = {
+	.stepsize = 512,
+	.strengths = axg_stepinfo_strengths,
+	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
+};
+
+static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
+
+static const struct nand_ecc_caps meson_axg_ecc_caps = {
+	.stepinfos = axg_stepinfo,
+	.nstepinfos = ARRAY_SIZE(axg_stepinfo),
+	.calc_ecc_bytes = meson_nand_calc_ecc_bytes,
+};
 
 static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
 {
@@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
 		return -EINVAL;
 
 	for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
-		if (meson_ecc[i].strength == nand->ecc.strength) {
+		if (meson_ecc[i].strength == nand->ecc.strength &&
+		    meson_ecc[i].size == nand->ecc.size) {
 			meson_chip->bch_mode = meson_ecc[i].bch;
 			return 0;
 		}
@@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
 	struct meson_nfc *nfc = nand_get_controller_data(nand);
 	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
 	struct mtd_info *mtd = nand_to_mtd(nand);
-	int nsectors = mtd->writesize / 1024;
+	int nsectors = mtd->writesize / 512;
 	int raw_writesize;
 	int ret;
 
-- 
2.35.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Miquel Raynal]

Hi Arseniy,

AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300:

> Meson NAND supports both 512B and 1024B ECC step size.
> 
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index 345212e8c691..6cc4f63b86c8 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
>  struct meson_nand_ecc {
>  	u32 bch;
>  	u32 strength;
> +	u32 size;
>  };
>  
>  struct meson_nfc_data {
> @@ -190,7 +191,8 @@ struct meson_nfc {
>  };
>  
>  enum {
> -	NFC_ECC_BCH8_1K		= 2,
> +	NFC_ECC_BCH8_512	= 1,
> +	NFC_ECC_BCH8_1K,
>  	NFC_ECC_BCH24_1K,
>  	NFC_ECC_BCH30_1K,
>  	NFC_ECC_BCH40_1K,
> @@ -198,15 +200,16 @@ enum {
>  	NFC_ECC_BCH60_1K,
>  };
>  
> -#define MESON_ECC_DATA(b, s)	{ .bch = (b),	.strength = (s)}
> +#define MESON_ECC_DATA(b, s, sz)	{ .bch = (b), .strength = (s), .size = (sz) }
>  
>  static struct meson_nand_ecc meson_ecc[] = {
> -	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
> -	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
> -	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
> -	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
> -	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
> -	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
> +	MESON_ECC_DATA(NFC_ECC_BCH8_512, 8,  512),
> +	MESON_ECC_DATA(NFC_ECC_BCH8_1K,  8,  1024),
> +	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
> +	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
> +	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
> +	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
> +	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
>  };
>  
>  static int meson_nand_calc_ecc_bytes(int step_size, int strength)
> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
>  
>  NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
>  		     meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
> -		     meson_nand_calc_ecc_bytes, 1024, 8);
> +
> +static const int axg_stepinfo_strengths[] = { 8 };
> +static const struct nand_ecc_step_info axg_stepinfo_1024 = {
> +	.stepsize = 1024,
> +	.strengths = axg_stepinfo_strengths,
> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
> +};
> +
> +static const struct nand_ecc_step_info axg_stepinfo_512 = {
> +	.stepsize = 512,
> +	.strengths = axg_stepinfo_strengths,
> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
> +};
> +
> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
> +
> +static const struct nand_ecc_caps meson_axg_ecc_caps = {
> +	.stepinfos = axg_stepinfo,
> +	.nstepinfos = ARRAY_SIZE(axg_stepinfo),
> +	.calc_ecc_bytes = meson_nand_calc_ecc_bytes,
> +};
>  
>  static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
>  {
> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
>  		return -EINVAL;
>  
>  	for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
> -		if (meson_ecc[i].strength == nand->ecc.strength) {
> +		if (meson_ecc[i].strength == nand->ecc.strength &&
> +		    meson_ecc[i].size == nand->ecc.size) {
>  			meson_chip->bch_mode = meson_ecc[i].bch;
>  			return 0;
>  		}
> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
>  	struct meson_nfc *nfc = nand_get_controller_data(nand);
>  	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>  	struct mtd_info *mtd = nand_to_mtd(nand);
> -	int nsectors = mtd->writesize / 1024;
> +	int nsectors = mtd->writesize / 512;

This cannot be unconditional, right?

>  	int raw_writesize;
>  	int ret;
>  


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Arseniy Krasnov]

On 04.07.2023 11:36, Miquel Raynal wrote:
> Hi Arseniy,
> 
> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300:
> 
>> Meson NAND supports both 512B and 1024B ECC step size.
>>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>>  drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
>>  1 file changed, 35 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>> index 345212e8c691..6cc4f63b86c8 100644
>> --- a/drivers/mtd/nand/raw/meson_nand.c
>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
>>  struct meson_nand_ecc {
>>  	u32 bch;
>>  	u32 strength;
>> +	u32 size;
>>  };
>>  
>>  struct meson_nfc_data {
>> @@ -190,7 +191,8 @@ struct meson_nfc {
>>  };
>>  
>>  enum {
>> -	NFC_ECC_BCH8_1K		= 2,
>> +	NFC_ECC_BCH8_512	= 1,
>> +	NFC_ECC_BCH8_1K,
>>  	NFC_ECC_BCH24_1K,
>>  	NFC_ECC_BCH30_1K,
>>  	NFC_ECC_BCH40_1K,
>> @@ -198,15 +200,16 @@ enum {
>>  	NFC_ECC_BCH60_1K,
>>  };
>>  
>> -#define MESON_ECC_DATA(b, s)	{ .bch = (b),	.strength = (s)}
>> +#define MESON_ECC_DATA(b, s, sz)	{ .bch = (b), .strength = (s), .size = (sz) }
>>  
>>  static struct meson_nand_ecc meson_ecc[] = {
>> -	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
>> -	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
>> -	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
>> -	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
>> -	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
>> -	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
>> +	MESON_ECC_DATA(NFC_ECC_BCH8_512, 8,  512),
>> +	MESON_ECC_DATA(NFC_ECC_BCH8_1K,  8,  1024),
>> +	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
>> +	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
>> +	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
>> +	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
>> +	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
>>  };
>>  
>>  static int meson_nand_calc_ecc_bytes(int step_size, int strength)
>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
>>  
>>  NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
>>  		     meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
>> -		     meson_nand_calc_ecc_bytes, 1024, 8);
>> +
>> +static const int axg_stepinfo_strengths[] = { 8 };
>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = {
>> +	.stepsize = 1024,
>> +	.strengths = axg_stepinfo_strengths,
>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
>> +};
>> +
>> +static const struct nand_ecc_step_info axg_stepinfo_512 = {
>> +	.stepsize = 512,
>> +	.strengths = axg_stepinfo_strengths,
>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
>> +};
>> +
>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
>> +
>> +static const struct nand_ecc_caps meson_axg_ecc_caps = {
>> +	.stepinfos = axg_stepinfo,
>> +	.nstepinfos = ARRAY_SIZE(axg_stepinfo),
>> +	.calc_ecc_bytes = meson_nand_calc_ecc_bytes,
>> +};
>>  
>>  static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
>>  {
>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
>>  		return -EINVAL;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
>> -		if (meson_ecc[i].strength == nand->ecc.strength) {
>> +		if (meson_ecc[i].strength == nand->ecc.strength &&
>> +		    meson_ecc[i].size == nand->ecc.size) {
>>  			meson_chip->bch_mode = meson_ecc[i].bch;
>>  			return 0;
>>  		}
>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
>>  	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>  	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>  	struct mtd_info *mtd = nand_to_mtd(nand);
>> -	int nsectors = mtd->writesize / 1024;
>> +	int nsectors = mtd->writesize / 512;
> 
> This cannot be unconditional, right?

Hello Miquel!

Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
that could be used by ECC engine (this value will be passed as 'oobavail'
to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
maximum number of free bytes). For Meson, if ECC step size is 512, then we
have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
bytes in OOB).

I think this code could be reworked in the following way:

if ECC step size is already known here (from DTS), calculate 'nsectors' using
given value (div by 512 for example). Otherwise calculate 'nsectors' in the
current manner:

int nsectors = mtd->writesize / 1024;

Moreover 1024 is default ECC step size for this driver, so default behaviour
will be preserved.

Thanks, Arseniy

> 
>>  	int raw_writesize;
>>  	int ret;
>>  
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300:

> On 04.07.2023 11:36, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300:
> >   
> >> Meson NAND supports both 512B and 1024B ECC step size.
> >>
> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> >> ---
> >>  drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
> >>  1 file changed, 35 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >> index 345212e8c691..6cc4f63b86c8 100644
> >> --- a/drivers/mtd/nand/raw/meson_nand.c
> >> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
> >>  struct meson_nand_ecc {
> >>  	u32 bch;
> >>  	u32 strength;
> >> +	u32 size;
> >>  };
> >>  
> >>  struct meson_nfc_data {
> >> @@ -190,7 +191,8 @@ struct meson_nfc {
> >>  };
> >>  
> >>  enum {
> >> -	NFC_ECC_BCH8_1K		= 2,
> >> +	NFC_ECC_BCH8_512	= 1,
> >> +	NFC_ECC_BCH8_1K,
> >>  	NFC_ECC_BCH24_1K,
> >>  	NFC_ECC_BCH30_1K,
> >>  	NFC_ECC_BCH40_1K,
> >> @@ -198,15 +200,16 @@ enum {
> >>  	NFC_ECC_BCH60_1K,
> >>  };
> >>  
> >> -#define MESON_ECC_DATA(b, s)	{ .bch = (b),	.strength = (s)}
> >> +#define MESON_ECC_DATA(b, s, sz)	{ .bch = (b), .strength = (s), .size = (sz) }
> >>  
> >>  static struct meson_nand_ecc meson_ecc[] = {
> >> -	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
> >> -	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
> >> -	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
> >> -	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
> >> -	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
> >> -	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
> >> +	MESON_ECC_DATA(NFC_ECC_BCH8_512, 8,  512),
> >> +	MESON_ECC_DATA(NFC_ECC_BCH8_1K,  8,  1024),
> >> +	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
> >> +	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
> >> +	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
> >> +	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
> >> +	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
> >>  };
> >>  
> >>  static int meson_nand_calc_ecc_bytes(int step_size, int strength)
> >> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
> >>  
> >>  NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
> >>  		     meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
> >> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
> >> -		     meson_nand_calc_ecc_bytes, 1024, 8);
> >> +
> >> +static const int axg_stepinfo_strengths[] = { 8 };
> >> +static const struct nand_ecc_step_info axg_stepinfo_1024 = {
> >> +	.stepsize = 1024,
> >> +	.strengths = axg_stepinfo_strengths,
> >> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
> >> +};
> >> +
> >> +static const struct nand_ecc_step_info axg_stepinfo_512 = {
> >> +	.stepsize = 512,
> >> +	.strengths = axg_stepinfo_strengths,
> >> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
> >> +};
> >> +
> >> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
> >> +
> >> +static const struct nand_ecc_caps meson_axg_ecc_caps = {
> >> +	.stepinfos = axg_stepinfo,
> >> +	.nstepinfos = ARRAY_SIZE(axg_stepinfo),
> >> +	.calc_ecc_bytes = meson_nand_calc_ecc_bytes,
> >> +};
> >>  
> >>  static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
> >>  {
> >> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
> >>  		return -EINVAL;
> >>  
> >>  	for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
> >> -		if (meson_ecc[i].strength == nand->ecc.strength) {
> >> +		if (meson_ecc[i].strength == nand->ecc.strength &&
> >> +		    meson_ecc[i].size == nand->ecc.size) {
> >>  			meson_chip->bch_mode = meson_ecc[i].bch;
> >>  			return 0;
> >>  		}
> >> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
> >>  	struct meson_nfc *nfc = nand_get_controller_data(nand);
> >>  	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >>  	struct mtd_info *mtd = nand_to_mtd(nand);
> >> -	int nsectors = mtd->writesize / 1024;
> >> +	int nsectors = mtd->writesize / 512;  
> > 
> > This cannot be unconditional, right?  
> 
> Hello Miquel!
> 
> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
> that could be used by ECC engine (this value will be passed as 'oobavail'
> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
> maximum number of free bytes). For Meson, if ECC step size is 512, then we
> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
> bytes in OOB).
> 
> I think this code could be reworked in the following way:
> 
> if ECC step size is already known here (from DTS), calculate 'nsectors' using
> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
> current manner:

It will always be known when these function are run. There is no
guessing here.

> 
> int nsectors = mtd->writesize / 1024;
> 
> Moreover 1024 is default ECC step size for this driver, so default behaviour
> will be preserved.

Yes, otherwise you would break existing users.

> 
> Thanks, Arseniy
> 
> >   
> >>  	int raw_writesize;
> >>  	int ret;
> >>    
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Arseniy Krasnov]

On 04.07.2023 12:41, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300:
> 
>> On 04.07.2023 11:36, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>
>>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300:
>>>   
>>>> Meson NAND supports both 512B and 1024B ECC step size.
>>>>
>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>> ---
>>>>  drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
>>>>  1 file changed, 35 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>>> index 345212e8c691..6cc4f63b86c8 100644
>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
>>>>  struct meson_nand_ecc {
>>>>  	u32 bch;
>>>>  	u32 strength;
>>>> +	u32 size;
>>>>  };
>>>>  
>>>>  struct meson_nfc_data {
>>>> @@ -190,7 +191,8 @@ struct meson_nfc {
>>>>  };
>>>>  
>>>>  enum {
>>>> -	NFC_ECC_BCH8_1K		= 2,
>>>> +	NFC_ECC_BCH8_512	= 1,
>>>> +	NFC_ECC_BCH8_1K,
>>>>  	NFC_ECC_BCH24_1K,
>>>>  	NFC_ECC_BCH30_1K,
>>>>  	NFC_ECC_BCH40_1K,
>>>> @@ -198,15 +200,16 @@ enum {
>>>>  	NFC_ECC_BCH60_1K,
>>>>  };
>>>>  
>>>> -#define MESON_ECC_DATA(b, s)	{ .bch = (b),	.strength = (s)}
>>>> +#define MESON_ECC_DATA(b, s, sz)	{ .bch = (b), .strength = (s), .size = (sz) }
>>>>  
>>>>  static struct meson_nand_ecc meson_ecc[] = {
>>>> -	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
>>>> -	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
>>>> -	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
>>>> -	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
>>>> -	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
>>>> -	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
>>>> +	MESON_ECC_DATA(NFC_ECC_BCH8_512, 8,  512),
>>>> +	MESON_ECC_DATA(NFC_ECC_BCH8_1K,  8,  1024),
>>>> +	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
>>>> +	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
>>>> +	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
>>>> +	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
>>>> +	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
>>>>  };
>>>>  
>>>>  static int meson_nand_calc_ecc_bytes(int step_size, int strength)
>>>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
>>>>  
>>>>  NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
>>>>  		     meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
>>>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
>>>> -		     meson_nand_calc_ecc_bytes, 1024, 8);
>>>> +
>>>> +static const int axg_stepinfo_strengths[] = { 8 };
>>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = {
>>>> +	.stepsize = 1024,
>>>> +	.strengths = axg_stepinfo_strengths,
>>>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
>>>> +};
>>>> +
>>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = {
>>>> +	.stepsize = 512,
>>>> +	.strengths = axg_stepinfo_strengths,
>>>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
>>>> +};
>>>> +
>>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
>>>> +
>>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = {
>>>> +	.stepinfos = axg_stepinfo,
>>>> +	.nstepinfos = ARRAY_SIZE(axg_stepinfo),
>>>> +	.calc_ecc_bytes = meson_nand_calc_ecc_bytes,
>>>> +};
>>>>  
>>>>  static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
>>>>  {
>>>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
>>>>  		return -EINVAL;
>>>>  
>>>>  	for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
>>>> -		if (meson_ecc[i].strength == nand->ecc.strength) {
>>>> +		if (meson_ecc[i].strength == nand->ecc.strength &&
>>>> +		    meson_ecc[i].size == nand->ecc.size) {
>>>>  			meson_chip->bch_mode = meson_ecc[i].bch;
>>>>  			return 0;
>>>>  		}
>>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
>>>>  	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>>  	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>>  	struct mtd_info *mtd = nand_to_mtd(nand);
>>>> -	int nsectors = mtd->writesize / 1024;
>>>> +	int nsectors = mtd->writesize / 512;  
>>>
>>> This cannot be unconditional, right?  
>>
>> Hello Miquel!
>>
>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
>> that could be used by ECC engine (this value will be passed as 'oobavail'
>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
>> maximum number of free bytes). For Meson, if ECC step size is 512, then we
>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
>> bytes in OOB).
>>
>> I think this code could be reworked in the following way:
>>
>> if ECC step size is already known here (from DTS), calculate 'nsectors' using
>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
>> current manner:
> 
> It will always be known when these function are run. There is no
> guessing here.

Hm I checked, that but if step size is not set in DTS, here it will be 0, 
then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps'
and 'oobavail'...

Anyway, I'll do the following thing:

int nsectors;

if (nand->ecc.size)
    nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC
else
    nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC

Thanks, Arseniy

> 
>>
>> int nsectors = mtd->writesize / 1024;
>>
>> Moreover 1024 is default ECC step size for this driver, so default behaviour
>> will be preserved.
> 
> Yes, otherwise you would break existing users.
> 
>>
>> Thanks, Arseniy
>>
>>>   
>>>>  	int raw_writesize;
>>>>  	int ret;
>>>>    
>>>
>>>
>>> Thanks,
>>> Miquèl  
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:46:51 +0300:

> On 04.07.2023 12:41, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300:
> >   
> >> On 04.07.2023 11:36, Miquel Raynal wrote:  
> >>> Hi Arseniy,
> >>>
> >>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300:
> >>>     
> >>>> Meson NAND supports both 512B and 1024B ECC step size.
> >>>>
> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> >>>> ---
> >>>>  drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
> >>>>  1 file changed, 35 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >>>> index 345212e8c691..6cc4f63b86c8 100644
> >>>> --- a/drivers/mtd/nand/raw/meson_nand.c
> >>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >>>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
> >>>>  struct meson_nand_ecc {
> >>>>  	u32 bch;
> >>>>  	u32 strength;
> >>>> +	u32 size;
> >>>>  };
> >>>>  
> >>>>  struct meson_nfc_data {
> >>>> @@ -190,7 +191,8 @@ struct meson_nfc {
> >>>>  };
> >>>>  
> >>>>  enum {
> >>>> -	NFC_ECC_BCH8_1K		= 2,
> >>>> +	NFC_ECC_BCH8_512	= 1,
> >>>> +	NFC_ECC_BCH8_1K,
> >>>>  	NFC_ECC_BCH24_1K,
> >>>>  	NFC_ECC_BCH30_1K,
> >>>>  	NFC_ECC_BCH40_1K,
> >>>> @@ -198,15 +200,16 @@ enum {
> >>>>  	NFC_ECC_BCH60_1K,
> >>>>  };
> >>>>  
> >>>> -#define MESON_ECC_DATA(b, s)	{ .bch = (b),	.strength = (s)}
> >>>> +#define MESON_ECC_DATA(b, s, sz)	{ .bch = (b), .strength = (s), .size = (sz) }
> >>>>  
> >>>>  static struct meson_nand_ecc meson_ecc[] = {
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
> >>>> -	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH8_512, 8,  512),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH8_1K,  8,  1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
> >>>> +	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
> >>>>  };
> >>>>  
> >>>>  static int meson_nand_calc_ecc_bytes(int step_size, int strength)
> >>>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
> >>>>  
> >>>>  NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
> >>>>  		     meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
> >>>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
> >>>> -		     meson_nand_calc_ecc_bytes, 1024, 8);
> >>>> +
> >>>> +static const int axg_stepinfo_strengths[] = { 8 };
> >>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = {
> >>>> +	.stepsize = 1024,
> >>>> +	.strengths = axg_stepinfo_strengths,
> >>>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
> >>>> +};
> >>>> +
> >>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = {
> >>>> +	.stepsize = 512,
> >>>> +	.strengths = axg_stepinfo_strengths,
> >>>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
> >>>> +};
> >>>> +
> >>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
> >>>> +
> >>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = {
> >>>> +	.stepinfos = axg_stepinfo,
> >>>> +	.nstepinfos = ARRAY_SIZE(axg_stepinfo),
> >>>> +	.calc_ecc_bytes = meson_nand_calc_ecc_bytes,
> >>>> +};
> >>>>  
> >>>>  static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
> >>>>  {
> >>>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
> >>>>  		return -EINVAL;
> >>>>  
> >>>>  	for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
> >>>> -		if (meson_ecc[i].strength == nand->ecc.strength) {
> >>>> +		if (meson_ecc[i].strength == nand->ecc.strength &&
> >>>> +		    meson_ecc[i].size == nand->ecc.size) {
> >>>>  			meson_chip->bch_mode = meson_ecc[i].bch;
> >>>>  			return 0;
> >>>>  		}
> >>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
> >>>>  	struct meson_nfc *nfc = nand_get_controller_data(nand);
> >>>>  	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >>>>  	struct mtd_info *mtd = nand_to_mtd(nand);
> >>>> -	int nsectors = mtd->writesize / 1024;
> >>>> +	int nsectors = mtd->writesize / 512;    
> >>>
> >>> This cannot be unconditional, right?    
> >>
> >> Hello Miquel!
> >>
> >> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
> >> that could be used by ECC engine (this value will be passed as 'oobavail'
> >> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
> >> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
> >> maximum number of free bytes). For Meson, if ECC step size is 512, then we
> >> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
> >> bytes in OOB).
> >>
> >> I think this code could be reworked in the following way:
> >>
> >> if ECC step size is already known here (from DTS), calculate 'nsectors' using
> >> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
> >> current manner:  
> > 
> > It will always be known when these function are run. There is no
> > guessing here.  
> 
> Hm I checked, that but if step size is not set in DTS, here it will be 0, 
> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps'
> and 'oobavail'...
> 
> Anyway, I'll do the following thing:
> 
> int nsectors;
> 
> if (nand->ecc.size)
>     nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC

You should set nand->ecc.size in ->attach_chip() instead.

> else
>     nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC
> 
> Thanks, Arseniy
> 
> >   
> >>
> >> int nsectors = mtd->writesize / 1024;
> >>
> >> Moreover 1024 is default ECC step size for this driver, so default behaviour
> >> will be preserved.  
> > 
> > Yes, otherwise you would break existing users.
> >   
> >>
> >> Thanks, Arseniy
> >>  
> >>>     
> >>>>  	int raw_writesize;
> >>>>  	int ret;
> >>>>      
> >>>
> >>>
> >>> Thanks,
> >>> Miquèl    
> > 
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Arseniy Krasnov]

On 04.07.2023 12:56, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:46:51 +0300:
> 
>> On 04.07.2023 12:41, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>
>>> avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300:
>>>   
>>>> On 04.07.2023 11:36, Miquel Raynal wrote:  
>>>>> Hi Arseniy,
>>>>>
>>>>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300:
>>>>>     
>>>>>> Meson NAND supports both 512B and 1024B ECC step size.
>>>>>>
>>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>>> ---
>>>>>>  drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
>>>>>>  1 file changed, 35 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>>>>> index 345212e8c691..6cc4f63b86c8 100644
>>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>>>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip {
>>>>>>  struct meson_nand_ecc {
>>>>>>  	u32 bch;
>>>>>>  	u32 strength;
>>>>>> +	u32 size;
>>>>>>  };
>>>>>>  
>>>>>>  struct meson_nfc_data {
>>>>>> @@ -190,7 +191,8 @@ struct meson_nfc {
>>>>>>  };
>>>>>>  
>>>>>>  enum {
>>>>>> -	NFC_ECC_BCH8_1K		= 2,
>>>>>> +	NFC_ECC_BCH8_512	= 1,
>>>>>> +	NFC_ECC_BCH8_1K,
>>>>>>  	NFC_ECC_BCH24_1K,
>>>>>>  	NFC_ECC_BCH30_1K,
>>>>>>  	NFC_ECC_BCH40_1K,
>>>>>> @@ -198,15 +200,16 @@ enum {
>>>>>>  	NFC_ECC_BCH60_1K,
>>>>>>  };
>>>>>>  
>>>>>> -#define MESON_ECC_DATA(b, s)	{ .bch = (b),	.strength = (s)}
>>>>>> +#define MESON_ECC_DATA(b, s, sz)	{ .bch = (b), .strength = (s), .size = (sz) }
>>>>>>  
>>>>>>  static struct meson_nand_ecc meson_ecc[] = {
>>>>>> -	MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8),
>>>>>> -	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24),
>>>>>> -	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30),
>>>>>> -	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40),
>>>>>> -	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50),
>>>>>> -	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60),
>>>>>> +	MESON_ECC_DATA(NFC_ECC_BCH8_512, 8,  512),
>>>>>> +	MESON_ECC_DATA(NFC_ECC_BCH8_1K,  8,  1024),
>>>>>> +	MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024),
>>>>>> +	MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024),
>>>>>> +	MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024),
>>>>>> +	MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024),
>>>>>> +	MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024),
>>>>>>  };
>>>>>>  
>>>>>>  static int meson_nand_calc_ecc_bytes(int step_size, int strength)
>>>>>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_size, int strength)
>>>>>>  
>>>>>>  NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps,
>>>>>>  		     meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60);
>>>>>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps,
>>>>>> -		     meson_nand_calc_ecc_bytes, 1024, 8);
>>>>>> +
>>>>>> +static const int axg_stepinfo_strengths[] = { 8 };
>>>>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = {
>>>>>> +	.stepsize = 1024,
>>>>>> +	.strengths = axg_stepinfo_strengths,
>>>>>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
>>>>>> +};
>>>>>> +
>>>>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = {
>>>>>> +	.stepsize = 512,
>>>>>> +	.strengths = axg_stepinfo_strengths,
>>>>>> +	.nstrengths = ARRAY_SIZE(axg_stepinfo_strengths)
>>>>>> +};
>>>>>> +
>>>>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 };
>>>>>> +
>>>>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = {
>>>>>> +	.stepinfos = axg_stepinfo,
>>>>>> +	.nstepinfos = ARRAY_SIZE(axg_stepinfo),
>>>>>> +	.calc_ecc_bytes = meson_nand_calc_ecc_bytes,
>>>>>> +};
>>>>>>  
>>>>>>  static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand)
>>>>>>  {
>>>>>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chip *nand)
>>>>>>  		return -EINVAL;
>>>>>>  
>>>>>>  	for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) {
>>>>>> -		if (meson_ecc[i].strength == nand->ecc.strength) {
>>>>>> +		if (meson_ecc[i].strength == nand->ecc.strength &&
>>>>>> +		    meson_ecc[i].size == nand->ecc.size) {
>>>>>>  			meson_chip->bch_mode = meson_ecc[i].bch;
>>>>>>  			return 0;
>>>>>>  		}
>>>>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
>>>>>>  	struct meson_nfc *nfc = nand_get_controller_data(nand);
>>>>>>  	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>>>>  	struct mtd_info *mtd = nand_to_mtd(nand);
>>>>>> -	int nsectors = mtd->writesize / 1024;
>>>>>> +	int nsectors = mtd->writesize / 512;    
>>>>>
>>>>> This cannot be unconditional, right?    
>>>>
>>>> Hello Miquel!
>>>>
>>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
>>>> that could be used by ECC engine (this value will be passed as 'oobavail'
>>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
>>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
>>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we
>>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
>>>> bytes in OOB).
>>>>
>>>> I think this code could be reworked in the following way:
>>>>
>>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using
>>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
>>>> current manner:  
>>>
>>> It will always be known when these function are run. There is no
>>> guessing here.  
>>
>> Hm I checked, that but if step size is not set in DTS, here it will be 0, 
>> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps'
>> and 'oobavail'...
>>
>> Anyway, I'll do the following thing:
>>
>> int nsectors;
>>
>> if (nand->ecc.size)
>>     nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC
> 
> You should set nand->ecc.size in ->attach_chip() instead.

Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach
callback it will be already known (DT part was processed in 'rawnand_dt_init()').
If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it
according provided ecc caps. What do You mean for "You should set ..." ?

Thanks, Arseniy


> 
>> else
>>     nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC
>>
>> Thanks, Arseniy
>>
>>>   
>>>>
>>>> int nsectors = mtd->writesize / 1024;
>>>>
>>>> Moreover 1024 is default ECC step size for this driver, so default behaviour
>>>> will be preserved.  
>>>
>>> Yes, otherwise you would break existing users.
>>>   
>>>>
>>>> Thanks, Arseniy
>>>>  
>>>>>     
>>>>>>  	int raw_writesize;
>>>>>>  	int ret;
>>>>>>      
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Miquèl    
>>>
>>>
>>> Thanks,
>>> Miquèl  
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Miquel Raynal]

Hi Arseniy,

> >>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
> >>>> that could be used by ECC engine (this value will be passed as 'oobavail'
> >>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
> >>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
> >>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we
> >>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
> >>>> bytes in OOB).
> >>>>
> >>>> I think this code could be reworked in the following way:
> >>>>
> >>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using
> >>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
> >>>> current manner:    
> >>>
> >>> It will always be known when these function are run. There is no
> >>> guessing here.    
> >>
> >> Hm I checked, that but if step size is not set in DTS, here it will be 0, 
> >> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps'
> >> and 'oobavail'...
> >>
> >> Anyway, I'll do the following thing:
> >>
> >> int nsectors;
> >>
> >> if (nand->ecc.size)
> >>     nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC  
> > 
> > You should set nand->ecc.size in ->attach_chip() instead.  
> 
> Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach
> callback it will be already known (DT part was processed in 'rawnand_dt_init()').
> If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it
> according provided ecc caps. What do You mean for "You should set ..." ?

The current approach is wrong, it decides the number of ECC chunks
(called nsectors in the driver) and then asks the core to decide the
number of ECC chunks to use.

Just provide mtd->oobsize - 2 as last parameter and then rely on the
core's logic to find the right ECC step-size/strength?

There is no point in requesting a particular step size without a
specific strength, or? So I believe you should provide both in the DTS
if you want particular parameters to be applied, otherwise you can let
the core decide what is best.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Arseniy Krasnov]

On 04.07.2023 16:41, Miquel Raynal wrote:
> Hi Arseniy,
> 
>>>>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
>>>>>> that could be used by ECC engine (this value will be passed as 'oobavail'
>>>>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
>>>>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
>>>>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we
>>>>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
>>>>>> bytes in OOB).
>>>>>>
>>>>>> I think this code could be reworked in the following way:
>>>>>>
>>>>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using
>>>>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
>>>>>> current manner:    
>>>>>
>>>>> It will always be known when these function are run. There is no
>>>>> guessing here.    
>>>>
>>>> Hm I checked, that but if step size is not set in DTS, here it will be 0, 
>>>> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps'
>>>> and 'oobavail'...
>>>>
>>>> Anyway, I'll do the following thing:
>>>>
>>>> int nsectors;
>>>>
>>>> if (nand->ecc.size)
>>>>     nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC  
>>>
>>> You should set nand->ecc.size in ->attach_chip() instead.  
>>
>> Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach
>> callback it will be already known (DT part was processed in 'rawnand_dt_init()').
>> If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it
>> according provided ecc caps. What do You mean for "You should set ..." ?
> 
> The current approach is wrong, it decides the number of ECC chunks
> (called nsectors in the driver) and then asks the core to decide the
> number of ECC chunks to use.

Yes! I was also confused about that.

> 
> Just provide mtd->oobsize - 2 as last parameter and then rely on the
> core's logic to find the right ECC step-size/strength?
> 
> There is no point in requesting a particular step size without a
> specific strength, or? So I believe you should provide both in the DTS
> if you want particular parameters to be applied, otherwise you can let
> the core decide what is best.

So I think this could be a separated patch as it doesn't rely on 512 step size ECC
support for Meson and may be it should be "Fix" tagged.

Thanks, Arseniy

> 
> Thanks,
> Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 18:07:04 +0300:

> On 04.07.2023 16:41, Miquel Raynal wrote:
> > Hi Arseniy,
> >   
> >>>>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
> >>>>>> that could be used by ECC engine (this value will be passed as 'oobavail'
> >>>>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
> >>>>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
> >>>>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we
> >>>>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
> >>>>>> bytes in OOB).
> >>>>>>
> >>>>>> I think this code could be reworked in the following way:
> >>>>>>
> >>>>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using
> >>>>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
> >>>>>> current manner:      
> >>>>>
> >>>>> It will always be known when these function are run. There is no
> >>>>> guessing here.      
> >>>>
> >>>> Hm I checked, that but if step size is not set in DTS, here it will be 0, 
> >>>> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps'
> >>>> and 'oobavail'...
> >>>>
> >>>> Anyway, I'll do the following thing:
> >>>>
> >>>> int nsectors;
> >>>>
> >>>> if (nand->ecc.size)
> >>>>     nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC    
> >>>
> >>> You should set nand->ecc.size in ->attach_chip() instead.    
> >>
> >> Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach
> >> callback it will be already known (DT part was processed in 'rawnand_dt_init()').
> >> If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it
> >> according provided ecc caps. What do You mean for "You should set ..." ?  
> > 
> > The current approach is wrong, it decides the number of ECC chunks
> > (called nsectors in the driver) and then asks the core to decide the
> > number of ECC chunks to use.  
> 
> Yes! I was also confused about that.
> 
> > 
> > Just provide mtd->oobsize - 2 as last parameter and then rely on the
> > core's logic to find the right ECC step-size/strength?
> > 
> > There is no point in requesting a particular step size without a
> > specific strength, or? So I believe you should provide both in the DTS
> > if you want particular parameters to be applied, otherwise you can let
> > the core decide what is best.  
> 
> So I think this could be a separated patch as it doesn't rely on 512 step size ECC
> support for Meson and may be it should be "Fix" tagged.

Yup! Thanks for cleaning so thoroughly this driver :)

Cheers,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

[Arseniy Krasnov]

On 04.07.2023 18:32, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 18:07:04 +0300:
> 
>> On 04.07.2023 16:41, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>   
>>>>>>>> Yes, this code looks strange. 'nsectors' is used to calculate space in OOB
>>>>>>>> that could be used by ECC engine (this value will be passed as 'oobavail'
>>>>>>>> to 'nand_ecc_choose_conf()'). Idea of 512 is to consider "worst" case
>>>>>>>> for ECC, e.g. minimal number of bytes for ECC engine (and at the same time
>>>>>>>> maximum number of free bytes). For Meson, if ECC step size is 512, then we
>>>>>>>> have 4 x 2 free bytes in OOB (if step size if 1024 then we have 2 x 2 free
>>>>>>>> bytes in OOB).
>>>>>>>>
>>>>>>>> I think this code could be reworked in the following way:
>>>>>>>>
>>>>>>>> if ECC step size is already known here (from DTS), calculate 'nsectors' using
>>>>>>>> given value (div by 512 for example). Otherwise calculate 'nsectors' in the
>>>>>>>> current manner:      
>>>>>>>
>>>>>>> It will always be known when these function are run. There is no
>>>>>>> guessing here.      
>>>>>>
>>>>>> Hm I checked, that but if step size is not set in DTS, here it will be 0, 
>>>>>> then it will be selected in 'nand_ecc_choose_conf()' according provided 'ecc_caps'
>>>>>> and 'oobavail'...
>>>>>>
>>>>>> Anyway, I'll do the following thing:
>>>>>>
>>>>>> int nsectors;
>>>>>>
>>>>>> if (nand->ecc.size)
>>>>>>     nsectors = mtd->writesize / nand->ecc.size; <--- this is for 512 ECC    
>>>>>
>>>>> You should set nand->ecc.size in ->attach_chip() instead.    
>>>>
>>>> Sorry, didn't get it... if ECC step size is set in DTS, then here, in chip attach
>>>> callback it will be already known (DT part was processed in 'rawnand_dt_init()').
>>>> If ECC step size is unknown (e.g. 0 here), 'nand_ecc_choose_conf()' will set it
>>>> according provided ecc caps. What do You mean for "You should set ..." ?  
>>>
>>> The current approach is wrong, it decides the number of ECC chunks
>>> (called nsectors in the driver) and then asks the core to decide the
>>> number of ECC chunks to use.  
>>
>> Yes! I was also confused about that.
>>
>>>
>>> Just provide mtd->oobsize - 2 as last parameter and then rely on the
>>> core's logic to find the right ECC step-size/strength?
>>>
>>> There is no point in requesting a particular step size without a
>>> specific strength, or? So I believe you should provide both in the DTS
>>> if you want particular parameters to be applied, otherwise you can let
>>> the core decide what is best.  
>>
>> So I think this could be a separated patch as it doesn't rely on 512 step size ECC
>> support for Meson and may be it should be "Fix" tagged.
> 
> Yup! Thanks for cleaning so thoroughly this driver :)

Thanks again for review and details! :)

Thanks, Arseniy

> 
> Cheers,
> Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel