From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 71384EB64DD for ; Tue, 4 Jul 2023 09:56:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231296AbjGDJ4j (ORCPT ); Tue, 4 Jul 2023 05:56:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36624 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231255AbjGDJ4g (ORCPT ); Tue, 4 Jul 2023 05:56:36 -0400 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::227]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 54F2AE5; Tue, 4 Jul 2023 02:56:34 -0700 (PDT) X-GND-Sasl: miquel.raynal@bootlin.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1688464592; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mMapt+1v6arSkASxO8Vo/k1eb4URuf8TQG4qlhn8MnA=; b=MqsYEmANYbQeHQzDbkLhMUahxnUMbuzqWizhWc2AjfbV/tbNF8tsEpgVAK0lHnZt7SADVD pj9s56P5VDsQkjoDpoN4gz4JDh35m0ZolCz6wDDkrfZ6jqETug28jcpXZf9j6iAtbe1zxO UveLyrixfCgqolvN/xfNUfyIA/4tJsx3Rnm1Xu/gtIgrXW9vPY3t1vZP0cIgnXSWJfd/v4 DlPL6/xCmg++MDDhsv0w1zlQCG4DKii69CLqch37HeqTjxwLgOboN9ph6LGjG4DTxVoVcm Zag5KVmebxeQKwJ8ap/rMOiC40SKCt98ZFZBN1AlRFKQBk/FwPwVAtCWINO2Aw== X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com X-GND-Sasl: miquel.raynal@bootlin.com Received: by mail.gandi.net (Postfix) with ESMTPSA id D4F882000C; Tue, 4 Jul 2023 09:56:29 +0000 (UTC) Date: Tue, 4 Jul 2023 11:56:28 +0200 From: Miquel Raynal To: Arseniy Krasnov Cc: Liang Yang , Richard Weinberger , Vignesh Raghavendra , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Neil Armstrong , Kevin Hilman , Jerome Brunet , Martin Blumenstingl , , , , , , , Subject: Re: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size Message-ID: <20230704115628.55320428@xps-13> In-Reply-To: References: <20230628092937.538683-1-AVKrasnov@sberdevices.ru> <20230628092937.538683-3-AVKrasnov@sberdevices.ru> <20230704103617.4affae8a@xps-13> <9e6eaa87-887c-f955-113a-43860c8ea00c@sberdevices.ru> <20230704114110.25ca9de4@xps-13> Organization: Bootlin X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Arseniy, avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:46:51 +0300: > On 04.07.2023 12:41, Miquel Raynal wrote: > > Hi Arseniy, > >=20 > > avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: > > =20 > >> On 04.07.2023 11:36, Miquel Raynal wrote: =20 > >>> Hi Arseniy, > >>> > >>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > >>> =20 > >>>> Meson NAND supports both 512B and 1024B ECC step size. > >>>> > >>>> Signed-off-by: Arseniy Krasnov > >>>> --- > >>>> drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++-----= --- > >>>> 1 file changed, 35 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/ra= w/meson_nand.c > >>>> index 345212e8c691..6cc4f63b86c8 100644 > >>>> --- a/drivers/mtd/nand/raw/meson_nand.c > >>>> +++ b/drivers/mtd/nand/raw/meson_nand.c > >>>> @@ -135,6 +135,7 @@ struct meson_nfc_nand_chip { > >>>> struct meson_nand_ecc { > >>>> u32 bch; > >>>> u32 strength; > >>>> + u32 size; > >>>> }; > >>>> =20 > >>>> struct meson_nfc_data { > >>>> @@ -190,7 +191,8 @@ struct meson_nfc { > >>>> }; > >>>> =20 > >>>> enum { > >>>> - NFC_ECC_BCH8_1K =3D 2, > >>>> + NFC_ECC_BCH8_512 =3D 1, > >>>> + NFC_ECC_BCH8_1K, > >>>> NFC_ECC_BCH24_1K, > >>>> NFC_ECC_BCH30_1K, > >>>> NFC_ECC_BCH40_1K, > >>>> @@ -198,15 +200,16 @@ enum { > >>>> NFC_ECC_BCH60_1K, > >>>> }; > >>>> =20 > >>>> -#define MESON_ECC_DATA(b, s) { .bch =3D (b), .strength =3D (s)} > >>>> +#define MESON_ECC_DATA(b, s, sz) { .bch =3D (b), .strength =3D (s),= .size =3D (sz) } > >>>> =20 > >>>> static struct meson_nand_ecc meson_ecc[] =3D { > >>>> - MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50), > >>>> - MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH8_512, 8, 512), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH8_1K, 8, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH24_1K, 24, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH30_1K, 30, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH40_1K, 40, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH50_1K, 50, 1024), > >>>> + MESON_ECC_DATA(NFC_ECC_BCH60_1K, 60, 1024), > >>>> }; > >>>> =20 > >>>> static int meson_nand_calc_ecc_bytes(int step_size, int strength) > >>>> @@ -224,8 +227,27 @@ static int meson_nand_calc_ecc_bytes(int step_s= ize, int strength) > >>>> =20 > >>>> NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, > >>>> meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); > >>>> -NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, > >>>> - meson_nand_calc_ecc_bytes, 1024, 8); > >>>> + > >>>> +static const int axg_stepinfo_strengths[] =3D { 8 }; > >>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 =3D { > >>>> + .stepsize =3D 1024, > >>>> + .strengths =3D axg_stepinfo_strengths, > >>>> + .nstrengths =3D ARRAY_SIZE(axg_stepinfo_strengths) > >>>> +}; > >>>> + > >>>> +static const struct nand_ecc_step_info axg_stepinfo_512 =3D { > >>>> + .stepsize =3D 512, > >>>> + .strengths =3D axg_stepinfo_strengths, > >>>> + .nstrengths =3D ARRAY_SIZE(axg_stepinfo_strengths) > >>>> +}; > >>>> + > >>>> +static const struct nand_ecc_step_info axg_stepinfo[] =3D { axg_ste= pinfo_1024, axg_stepinfo_512 }; > >>>> + > >>>> +static const struct nand_ecc_caps meson_axg_ecc_caps =3D { > >>>> + .stepinfos =3D axg_stepinfo, > >>>> + .nstepinfos =3D ARRAY_SIZE(axg_stepinfo), > >>>> + .calc_ecc_bytes =3D meson_nand_calc_ecc_bytes, > >>>> +}; > >>>> =20 > >>>> static struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *= nand) > >>>> { > >>>> @@ -1259,7 +1281,8 @@ static int meson_nand_bch_mode(struct nand_chi= p *nand) > >>>> return -EINVAL; > >>>> =20 > >>>> for (i =3D 0; i < ARRAY_SIZE(meson_ecc); i++) { > >>>> - if (meson_ecc[i].strength =3D=3D nand->ecc.strength) { > >>>> + if (meson_ecc[i].strength =3D=3D nand->ecc.strength && > >>>> + meson_ecc[i].size =3D=3D nand->ecc.size) { > >>>> meson_chip->bch_mode =3D meson_ecc[i].bch; > >>>> return 0; > >>>> } > >>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_= chip *nand) > >>>> struct meson_nfc *nfc =3D nand_get_controller_data(nand); > >>>> struct meson_nfc_nand_chip *meson_chip =3D to_meson_nand(nand); > >>>> struct mtd_info *mtd =3D nand_to_mtd(nand); > >>>> - int nsectors =3D mtd->writesize / 1024; > >>>> + int nsectors =3D mtd->writesize / 512; =20 > >>> > >>> This cannot be unconditional, right? =20 > >> > >> Hello Miquel! > >> > >> 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 'oobava= il' > >> 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, the= n 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' i= n the > >> current manner: =20 > >=20 > > It will always be known when these function are run. There is no > > guessing here. =20 >=20 > Hm I checked, that but if step size is not set in DTS, here it will be 0,= =20 > then it will be selected in 'nand_ecc_choose_conf()' according provided '= ecc_caps' > and 'oobavail'... >=20 > Anyway, I'll do the following thing: >=20 > int nsectors; >=20 > if (nand->ecc.size) > nsectors =3D mtd->writesize / nand->ecc.size; <--- this is for 512 ECC You should set nand->ecc.size in ->attach_chip() instead. > else > nsectors =3D mtd->writesize / 1024; <--- this is for default 1024 ECC >=20 > Thanks, Arseniy >=20 > > =20 > >> > >> int nsectors =3D mtd->writesize / 1024; > >> > >> Moreover 1024 is default ECC step size for this driver, so default beh= aviour > >> will be preserved. =20 > >=20 > > Yes, otherwise you would break existing users. > > =20 > >> > >> Thanks, Arseniy > >> =20 > >>> =20 > >>>> int raw_writesize; > >>>> int ret; > >>>> =20 > >>> > >>> > >>> Thanks, > >>> Miqu=C3=A8l =20 > >=20 > >=20 > > Thanks, > > Miqu=C3=A8l =20 Thanks, Miqu=C3=A8l