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 15:41:06 +0200	[thread overview]
Message-ID: <20230704154106.5c5aafd8@xps-13> (raw)
In-Reply-To: <ee2eb73a-fb25-58ae-cf7e-83d971b7b8b2@sberdevices.ru>

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.

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.

Thanks,
Miquèl

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