From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Agner Subject: Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver Date: Thu, 24 May 2018 13:00:41 +0200 Message-ID: References: <86fdf19ec92b732709732fb60199f16488b4b727.1526990589.git.stefan@agner.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Benjamin Lindqvist Cc: boris.brezillon@bootlin.com, dwmw2@infradead.org, computersforpeace@gmail.com, marek.vasut@gmail.com, robh+dt@kernel.org, mark.rutland@arm.com, thierry.reding@gmail.com, mturquette@baylibre.com, sboyd@kernel.org, Lucas Stach , miquel.raynal@bootlin.com, richard@nod.at, marcel@ziswiler.com, krzk@kernel.org, digetx@gmail.com, jonathanh@nvidia.com, pdeschrijver@nvidia.com, pgaikwad@nvidia.com, Mirza Krak , linux-mtd@lists.infradead.org, linux-tegra@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org List-Id: linux-tegra@vger.kernel.org On 24.05.2018 09:45, Benjamin Lindqvist wrote: > Hi Stefan (and all), > > First off, I apoloigize in advance if I'm deviating from common > kernel mailing list courtesy -- this is my first time responding. > I just have a comment on the NAND driver that I'd like to bring > to the public. > Welcome! >> + switch (mtd->oobsize) { >> ... >> + case 224: >> + mtd_set_ooblayout(mtd, &tegra_nand_oob_224_ops); >> + chip->ecc.strength = 8; >> + chip->ecc.bytes = 18; >> + value |= CFG_ECC_SEL | CFG_TVAL_8; >> + break; + case 224: > > I am not sure how you arrived at this oobsize-based inference. I > have not seen any explicit relation between oob size and ECC > algorithm used in the reference manual. Indeed, the U-Boot I was > working on (a fork of the Toradex 2015.04 U-Boot) always has > oobsize == 224 but used either BCH[t=16] or RS[t=4]. In fact, we > tried choosing RS[t=8] in U-Boot but we failed to make the > BootROM decode this at all. So we had to use RS[t=4]. But > changing the algorithm did not automatically change the oobsize, > at least it didn't for us. So maybe you should consider if this > is really the way to go about deciding which algorithm is used. The oobsize based inference to set the HW ECC mode comes from the patchset I picked up. Typically, the size of the OOB area is such that it allows "good enough" error correction required for that chip. So using it as indicator for the ECC algorithm is not entirely off... But yeah I agree we have better means, and I already started working on a better mechanism. Also, I worked on BCH support, and it looks pretty good already. If we want to auto select mode we can use the ECC requirements from ONFI/JEDEC/parameter page. Or we could use device tree only. Thanks for bringing up the Boot ROM issue. In fact I investigated the supported modes the recent days too. First off, as you mentioned, the boot ROM seems to probe different modes until it succeeds. By trying through all RS and BCH modes, it seems that it only probes some modes: - RS t=4 - BCH t=8 - BCH t=16 I guess those are preferred modes in practise. Not sure if we should make sure the auto selection such that it only chooses from this list... > > Note that we're in fact using this patch set in Linux today, but > we had to remove the oobsize inference part. Currently we're > simply hard coding it to CFG_TVAL_4, but maybe it would be > cleaner to add ECC algo as a board config instead, e.g. in the > .dts file or whatever. This seems to be done by other vendors > already, see for example excerpt of > Documentation/devicetree/bindings/mtd/gpmc-nand.txt below: > > - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > "sw" 1-bit Hamming ecc code via software > "hw" use "ham1" instead > "hw-romcode" use "ham1" instead > "ham1" 1-bit Hamming ecc code > "bch4" 4-bit BCH ecc code > "bch8" 8-bit BCH ecc code > "bch16" 16-bit BCH ECC code > Refer below "How to select correct ECC scheme for your device ?" > > It seems as if this method would be equally applicable to Tegra NAND. Yeah, ideally we can reuse "nand-ecc-algo". Although, Reed-Solomon is not yet in the list. So using this property would require to extend this standard property. -- Stefan