Hi Arseniy, avkrasnov@xxxxxxxxxxxxxx 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
