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@xxxxxxxxxxxxxx wrote on Tue, 4 Jul 2023 12:46:51 +0300:
> 
>> On 04.07.2023 12:41, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>
>>> avkrasnov@xxxxxxxxxxxxxx wrote on Tue, 4 Jul 2023 12:23:03 +0300:
>>>   
>>>> 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:  
>>>
>>> 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