Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Arseniy Krasnov <avkrasnov@sberdevices.ru>
Cc: Liang Yang <liang.yang@amlogic.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	<oxffffaa@gmail.com>, <kernel@sberdevices.ru>,
	<linux-mtd@lists.infradead.org>, <devicetree@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-amlogic@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size
Date: Tue, 4 Jul 2023 17:32:23 +0200	[thread overview]
Message-ID: <20230704173223.771acd99@xps-13> (raw)
In-Reply-To: <e45afb19-d77b-6ef3-08bf-68e8626371be@sberdevices.ru>

Hi Arseniy,

avkrasnov@sberdevices.ru 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel