On 04.07.2023 18:32, Miquel Raynal wrote: > 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 :) Thanks again for review and details! :) Thanks, Arseniy > > Cheers, > Miquèl
