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@xxxxxxxxxxxxxx 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@xxxxxxxxxxxxxx>
>> ---
>>  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