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