Message ID | 20230628092937.538683-3-AVKrasnov@sberdevices.ru |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp8804142vqr; Wed, 28 Jun 2023 03:06:10 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ67+Ytq0CGs3pfepV45gfSoB/zXSgjVKhjeWCE9iKVNLtWOFzuYWOutRtJ0Yd9U6jg8cbTZ X-Received: by 2002:a17:902:bcc3:b0:1b0:307c:e6fe with SMTP id o3-20020a170902bcc300b001b0307ce6femr7150893pls.10.1687946770295; Wed, 28 Jun 2023 03:06:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1687946770; cv=none; d=google.com; s=arc-20160816; b=lktOq+nhsosOWGyda+Gpluf1G64UskCQfAoLZzwqtWiEPB1mFJkYvPuVtZ54yIBHS2 x0cYLBDSXNJJkWHb7a4AKmL0pjQVpDCnr3iIGO/+pJrOkryeAsxGyjqECmjFERFzYKTd VcZ3OV89q086Ba92/nGbsMQOovPmS9Mnt4Uh1xGiy8tFvKWE4mxCFNE9qwYMCf08j4Zq AZhJkE2jVS+P9aml3EH195L6SpQ/CRSrML6TDGn5Unag9goem7GsG18xez2XQqNASawN dN8snuxFh7skYsd42vMqaPpMfiLDoauU0lJrzlnGJaBuTdM0PSfB33LLl5ErHCwXfCmM hnSA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature:dkim-filter; bh=GclWWL59oht8VgFW8Tc4TABopkyYORqVnTmqa0M/qOg=; fh=lJoip+811P4DCdtapn8hp1VALdd89JRa/wJoc6Om3Qk=; b=dJ4DFfpy2wxX8M0VUetioPLbBeV/gzzhBaoD+LuN6mr/ETwlGHNzjMCE//0R8Iw3Mp kxDuiqjkuyKjniRQhGQZmKAEIPdctqez9q2HKPF+/ZWEz48P+iZZQJfW2zXBf7RVabuP dXTKEYyukfEehBpfPCqBxVZAaRTMSzB67Q7odw0OI1hb/aPj5I121iRGoRP4PJlTtRfL 8y0YGrrlUITt8fJpaQxQY/f8WlmJrVa4VoOdKrRohGYBaKT0fi37SRdsaDklC8BDpK4a 2hnp/VSejDQu6ZnuSL9VGjO2YXgR11pMr48eZBasL82i1WsARWMD/uZAq/9Fta9e0I// 12Tw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sberdevices.ru header.s=mail header.b=isgdeBQN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id f17-20020a170902f39100b001b3dff53359si6187233ple.467.2023.06.28.03.05.56; Wed, 28 Jun 2023 03:06:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@sberdevices.ru header.s=mail header.b=isgdeBQN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232848AbjF1Jwx (ORCPT <rfc822;adanhawthorn@gmail.com> + 99 others); Wed, 28 Jun 2023 05:52:53 -0400 Received: from mx1.sberdevices.ru ([37.18.73.165]:14222 "EHLO mx1.sberdevices.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235520AbjF1JfC (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 28 Jun 2023 05:35:02 -0400 Received: from p-infra-ksmg-sc-msk01 (localhost [127.0.0.1]) by mx1.sberdevices.ru (Postfix) with ESMTP id 6D6B8100010; Wed, 28 Jun 2023 12:34:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.sberdevices.ru 6D6B8100010 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sberdevices.ru; s=mail; t=1687944899; bh=GclWWL59oht8VgFW8Tc4TABopkyYORqVnTmqa0M/qOg=; h=From:To:Subject:Date:Message-ID:MIME-Version:Content-Type:From; b=isgdeBQNhCOXqAPWFxY23/4YS2SM5oveSMYOXgZN04ijpuuJPs8QDSJo86zoxFWSq akx548USahmjFutrW98GiQW9Yu5c3HMUQyRrw8PBE4RxahH3AYcuCpE66ts5FHhudy UcgvLJxbfBkOGNYZnxlYDseuBe13CHMgxp5XJTcdciGCUwpvDTiamXOo7D+JYIEvqL 39cJ82LQBy62R7fdzuQWngyJXD2ZY7eifYTKneKsO6/Bvv2S27QlWD9jxGMj1WI/Es wmm7viJHorE3OsE//MeQ/HQMsgGk0Rs4bNAlJJPH6F0j4qvFx4CdkpHLc9m2xMeuGU hhdW8OtiOlxJg== Received: from p-i-exch-sc-m01.sberdevices.ru (p-i-exch-sc-m01.sberdevices.ru [172.16.192.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.sberdevices.ru (Postfix) with ESMTPS; Wed, 28 Jun 2023 12:34:59 +0300 (MSK) Received: from localhost.localdomain (100.64.160.123) by p-i-exch-sc-m01.sberdevices.ru (172.16.192.107) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.30; Wed, 28 Jun 2023 12:34:02 +0300 From: Arseniy Krasnov <AVKrasnov@sberdevices.ru> To: Liang Yang <liang.yang@amlogic.com>, Miquel Raynal <miquel.raynal@bootlin.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> CC: <oxffffaa@gmail.com>, <kernel@sberdevices.ru>, Arseniy Krasnov <AVKrasnov@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: [RFC PATCH v1 2/2] mtd: rawnand: meson: support for 512B ECC step size Date: Wed, 28 Jun 2023 12:29:36 +0300 Message-ID: <20230628092937.538683-3-AVKrasnov@sberdevices.ru> X-Mailer: git-send-email 2.35.0 In-Reply-To: <20230628092937.538683-1-AVKrasnov@sberdevices.ru> References: <20230628092937.538683-1-AVKrasnov@sberdevices.ru> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [100.64.160.123] X-ClientProxiedBy: p-i-exch-sc-m02.sberdevices.ru (172.16.192.103) To p-i-exch-sc-m01.sberdevices.ru (172.16.192.107) X-KSMG-Rule-ID: 10 X-KSMG-Message-Action: clean X-KSMG-AntiSpam-Lua-Profiles: 178300 [Jun 28 2023] X-KSMG-AntiSpam-Version: 5.9.59.0 X-KSMG-AntiSpam-Envelope-From: AVKrasnov@sberdevices.ru X-KSMG-AntiSpam-Rate: 0 X-KSMG-AntiSpam-Status: not_detected X-KSMG-AntiSpam-Method: none X-KSMG-AntiSpam-Auth: dkim=none X-KSMG-AntiSpam-Info: LuaCore: 517 517 b0056c19d8e10afbb16cb7aad7258dedb0179a79, {Tracking_from_domain_doesnt_match_to}, 127.0.0.199:7.1.2;sberdevices.ru:5.0.1,7.1.1;d41d8cd98f00b204e9800998ecf8427e.com:7.1.1;p-i-exch-sc-m01.sberdevices.ru:5.0.1,7.1.1, FromAlignment: s, {Tracking_white_helo}, ApMailHostAddress: 100.64.160.123 X-MS-Exchange-Organization-SCL: -1 X-KSMG-AntiSpam-Interceptor-Info: scan successful X-KSMG-AntiPhishing: Clean X-KSMG-LinksScanning: Clean X-KSMG-AntiVirus: Kaspersky Secure Mail Gateway, version 2.0.1.6960, bases: 2023/06/28 08:00:00 #21591748 X-KSMG-AntiVirus-Status: Clean, skipped Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1769940472373539938?= X-GMAIL-MSGID: =?utf-8?q?1769940472373539938?= |
Series |
support 512B ECC step size for Meson NAND
|
|
Commit Message
Arseniy Krasnov
June 28, 2023, 9:29 a.m. UTC
Meson NAND supports both 512B and 1024B ECC step size.
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
drivers/mtd/nand/raw/meson_nand.c | 47 +++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 12 deletions(-)
Comments
Hi Arseniy, AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > Meson NAND supports both 512B and 1024B ECC step size. > > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > --- > 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/raw/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; > }; > > struct meson_nfc_data { > @@ -190,7 +191,8 @@ struct meson_nfc { > }; > > enum { > - NFC_ECC_BCH8_1K = 2, > + NFC_ECC_BCH8_512 = 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, > }; > > -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } > > static struct meson_nand_ecc meson_ecc[] = { > - 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), > }; > > 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_size, int strength) > > 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[] = { 8 }; > +static const struct nand_ecc_step_info axg_stepinfo_1024 = { > + .stepsize = 1024, > + .strengths = axg_stepinfo_strengths, > + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > +}; > + > +static const struct nand_ecc_step_info axg_stepinfo_512 = { > + .stepsize = 512, > + .strengths = axg_stepinfo_strengths, > + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > +}; > + > +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; > + > +static const struct nand_ecc_caps meson_axg_ecc_caps = { > + .stepinfos = axg_stepinfo, > + .nstepinfos = ARRAY_SIZE(axg_stepinfo), > + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, > +}; > > 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_chip *nand) > return -EINVAL; > > for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { > - if (meson_ecc[i].strength == nand->ecc.strength) { > + if (meson_ecc[i].strength == nand->ecc.strength && > + meson_ecc[i].size == nand->ecc.size) { > meson_chip->bch_mode = meson_ecc[i].bch; > return 0; > } > @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > struct meson_nfc *nfc = nand_get_controller_data(nand); > struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > struct mtd_info *mtd = nand_to_mtd(nand); > - int nsectors = mtd->writesize / 1024; > + int nsectors = mtd->writesize / 512; This cannot be unconditional, right? > int raw_writesize; > int ret; > Thanks, Miquèl
On 04.07.2023 11:36, Miquel Raynal wrote: > Hi Arseniy, > > AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > >> Meson NAND supports both 512B and 1024B ECC step size. >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> 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/raw/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; >> }; >> >> struct meson_nfc_data { >> @@ -190,7 +191,8 @@ struct meson_nfc { >> }; >> >> enum { >> - NFC_ECC_BCH8_1K = 2, >> + NFC_ECC_BCH8_512 = 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, >> }; >> >> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} >> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } >> >> static struct meson_nand_ecc meson_ecc[] = { >> - 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), >> }; >> >> 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_size, int strength) >> >> 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[] = { 8 }; >> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { >> + .stepsize = 1024, >> + .strengths = axg_stepinfo_strengths, >> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >> +}; >> + >> +static const struct nand_ecc_step_info axg_stepinfo_512 = { >> + .stepsize = 512, >> + .strengths = axg_stepinfo_strengths, >> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >> +}; >> + >> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; >> + >> +static const struct nand_ecc_caps meson_axg_ecc_caps = { >> + .stepinfos = axg_stepinfo, >> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), >> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, >> +}; >> >> 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_chip *nand) >> return -EINVAL; >> >> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { >> - if (meson_ecc[i].strength == nand->ecc.strength) { >> + if (meson_ecc[i].strength == nand->ecc.strength && >> + meson_ecc[i].size == nand->ecc.size) { >> meson_chip->bch_mode = meson_ecc[i].bch; >> return 0; >> } >> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >> struct meson_nfc *nfc = nand_get_controller_data(nand); >> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >> struct mtd_info *mtd = nand_to_mtd(nand); >> - int nsectors = mtd->writesize / 1024; >> + int nsectors = mtd->writesize / 512; > > This cannot be unconditional, right? 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 '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: int nsectors = mtd->writesize / 1024; Moreover 1024 is default ECC step size for this driver, so default behaviour will be preserved. Thanks, Arseniy > >> int raw_writesize; >> int ret; >> > > > Thanks, > Miquèl
Hi Arseniy, avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: > On 04.07.2023 11:36, Miquel Raynal wrote: > > Hi Arseniy, > > > > AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > > > >> Meson NAND supports both 512B and 1024B ECC step size. > >> > >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >> --- > >> 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/raw/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; > >> }; > >> > >> struct meson_nfc_data { > >> @@ -190,7 +191,8 @@ struct meson_nfc { > >> }; > >> > >> enum { > >> - NFC_ECC_BCH8_1K = 2, > >> + NFC_ECC_BCH8_512 = 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, > >> }; > >> > >> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > >> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } > >> > >> static struct meson_nand_ecc meson_ecc[] = { > >> - 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), > >> }; > >> > >> 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_size, int strength) > >> > >> 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[] = { 8 }; > >> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { > >> + .stepsize = 1024, > >> + .strengths = axg_stepinfo_strengths, > >> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > >> +}; > >> + > >> +static const struct nand_ecc_step_info axg_stepinfo_512 = { > >> + .stepsize = 512, > >> + .strengths = axg_stepinfo_strengths, > >> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > >> +}; > >> + > >> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; > >> + > >> +static const struct nand_ecc_caps meson_axg_ecc_caps = { > >> + .stepinfos = axg_stepinfo, > >> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), > >> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, > >> +}; > >> > >> 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_chip *nand) > >> return -EINVAL; > >> > >> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { > >> - if (meson_ecc[i].strength == nand->ecc.strength) { > >> + if (meson_ecc[i].strength == nand->ecc.strength && > >> + meson_ecc[i].size == nand->ecc.size) { > >> meson_chip->bch_mode = meson_ecc[i].bch; > >> return 0; > >> } > >> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > >> struct meson_nfc *nfc = nand_get_controller_data(nand); > >> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >> struct mtd_info *mtd = nand_to_mtd(nand); > >> - int nsectors = mtd->writesize / 1024; > >> + int nsectors = mtd->writesize / 512; > > > > This cannot be unconditional, right? > > 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 '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. > > int nsectors = mtd->writesize / 1024; > > Moreover 1024 is default ECC step size for this driver, so default behaviour > will be preserved. Yes, otherwise you would break existing users. > > Thanks, Arseniy > > > > >> int raw_writesize; > >> int ret; > >> > > > > > > Thanks, > > Miquèl Thanks, Miquèl
On 04.07.2023 12:41, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: > >> On 04.07.2023 11:36, Miquel Raynal wrote: >>> Hi Arseniy, >>> >>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: >>> >>>> Meson NAND supports both 512B and 1024B ECC step size. >>>> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>> --- >>>> 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/raw/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; >>>> }; >>>> >>>> struct meson_nfc_data { >>>> @@ -190,7 +191,8 @@ struct meson_nfc { >>>> }; >>>> >>>> enum { >>>> - NFC_ECC_BCH8_1K = 2, >>>> + NFC_ECC_BCH8_512 = 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, >>>> }; >>>> >>>> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} >>>> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } >>>> >>>> static struct meson_nand_ecc meson_ecc[] = { >>>> - 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), >>>> }; >>>> >>>> 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_size, int strength) >>>> >>>> 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[] = { 8 }; >>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { >>>> + .stepsize = 1024, >>>> + .strengths = axg_stepinfo_strengths, >>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >>>> +}; >>>> + >>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = { >>>> + .stepsize = 512, >>>> + .strengths = axg_stepinfo_strengths, >>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >>>> +}; >>>> + >>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; >>>> + >>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = { >>>> + .stepinfos = axg_stepinfo, >>>> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), >>>> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, >>>> +}; >>>> >>>> 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_chip *nand) >>>> return -EINVAL; >>>> >>>> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { >>>> - if (meson_ecc[i].strength == nand->ecc.strength) { >>>> + if (meson_ecc[i].strength == nand->ecc.strength && >>>> + meson_ecc[i].size == nand->ecc.size) { >>>> meson_chip->bch_mode = meson_ecc[i].bch; >>>> return 0; >>>> } >>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>>> struct meson_nfc *nfc = nand_get_controller_data(nand); >>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>> - int nsectors = mtd->writesize / 1024; >>>> + int nsectors = mtd->writesize / 512; >>> >>> This cannot be unconditional, right? >> >> 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 '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 else nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC Thanks, Arseniy > >> >> int nsectors = mtd->writesize / 1024; >> >> Moreover 1024 is default ECC step size for this driver, so default behaviour >> will be preserved. > > Yes, otherwise you would break existing users. > >> >> Thanks, Arseniy >> >>> >>>> int raw_writesize; >>>> int ret; >>>> >>> >>> >>> Thanks, >>> Miquèl > > > Thanks, > Miquèl
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, > > > > avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: > > > >> On 04.07.2023 11:36, Miquel Raynal wrote: > >>> Hi Arseniy, > >>> > >>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: > >>> > >>>> Meson NAND supports both 512B and 1024B ECC step size. > >>>> > >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> > >>>> --- > >>>> 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/raw/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; > >>>> }; > >>>> > >>>> struct meson_nfc_data { > >>>> @@ -190,7 +191,8 @@ struct meson_nfc { > >>>> }; > >>>> > >>>> enum { > >>>> - NFC_ECC_BCH8_1K = 2, > >>>> + NFC_ECC_BCH8_512 = 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, > >>>> }; > >>>> > >>>> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > >>>> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } > >>>> > >>>> static struct meson_nand_ecc meson_ecc[] = { > >>>> - 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), > >>>> }; > >>>> > >>>> 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_size, int strength) > >>>> > >>>> 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[] = { 8 }; > >>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { > >>>> + .stepsize = 1024, > >>>> + .strengths = axg_stepinfo_strengths, > >>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > >>>> +}; > >>>> + > >>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = { > >>>> + .stepsize = 512, > >>>> + .strengths = axg_stepinfo_strengths, > >>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) > >>>> +}; > >>>> + > >>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; > >>>> + > >>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = { > >>>> + .stepinfos = axg_stepinfo, > >>>> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), > >>>> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, > >>>> +}; > >>>> > >>>> 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_chip *nand) > >>>> return -EINVAL; > >>>> > >>>> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { > >>>> - if (meson_ecc[i].strength == nand->ecc.strength) { > >>>> + if (meson_ecc[i].strength == nand->ecc.strength && > >>>> + meson_ecc[i].size == nand->ecc.size) { > >>>> meson_chip->bch_mode = meson_ecc[i].bch; > >>>> return 0; > >>>> } > >>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) > >>>> struct meson_nfc *nfc = nand_get_controller_data(nand); > >>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > >>>> struct mtd_info *mtd = nand_to_mtd(nand); > >>>> - int nsectors = mtd->writesize / 1024; > >>>> + int nsectors = mtd->writesize / 512; > >>> > >>> This cannot be unconditional, right? > >> > >> 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 '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. > else > nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC > > Thanks, Arseniy > > > > >> > >> int nsectors = mtd->writesize / 1024; > >> > >> Moreover 1024 is default ECC step size for this driver, so default behaviour > >> will be preserved. > > > > Yes, otherwise you would break existing users. > > > >> > >> Thanks, Arseniy > >> > >>> > >>>> int raw_writesize; > >>>> int ret; > >>>> > >>> > >>> > >>> Thanks, > >>> Miquèl > > > > > > Thanks, > > Miquèl Thanks, Miquèl
On 04.07.2023 12:56, Miquel Raynal wrote: > 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, >>> >>> avkrasnov@sberdevices.ru wrote on Tue, 4 Jul 2023 12:23:03 +0300: >>> >>>> On 04.07.2023 11:36, Miquel Raynal wrote: >>>>> Hi Arseniy, >>>>> >>>>> AVKrasnov@sberdevices.ru wrote on Wed, 28 Jun 2023 12:29:36 +0300: >>>>> >>>>>> Meson NAND supports both 512B and 1024B ECC step size. >>>>>> >>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>>>> --- >>>>>> 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/raw/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; >>>>>> }; >>>>>> >>>>>> struct meson_nfc_data { >>>>>> @@ -190,7 +191,8 @@ struct meson_nfc { >>>>>> }; >>>>>> >>>>>> enum { >>>>>> - NFC_ECC_BCH8_1K = 2, >>>>>> + NFC_ECC_BCH8_512 = 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, >>>>>> }; >>>>>> >>>>>> -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} >>>>>> +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } >>>>>> >>>>>> static struct meson_nand_ecc meson_ecc[] = { >>>>>> - 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), >>>>>> }; >>>>>> >>>>>> 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_size, int strength) >>>>>> >>>>>> 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[] = { 8 }; >>>>>> +static const struct nand_ecc_step_info axg_stepinfo_1024 = { >>>>>> + .stepsize = 1024, >>>>>> + .strengths = axg_stepinfo_strengths, >>>>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >>>>>> +}; >>>>>> + >>>>>> +static const struct nand_ecc_step_info axg_stepinfo_512 = { >>>>>> + .stepsize = 512, >>>>>> + .strengths = axg_stepinfo_strengths, >>>>>> + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) >>>>>> +}; >>>>>> + >>>>>> +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; >>>>>> + >>>>>> +static const struct nand_ecc_caps meson_axg_ecc_caps = { >>>>>> + .stepinfos = axg_stepinfo, >>>>>> + .nstepinfos = ARRAY_SIZE(axg_stepinfo), >>>>>> + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, >>>>>> +}; >>>>>> >>>>>> 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_chip *nand) >>>>>> return -EINVAL; >>>>>> >>>>>> for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { >>>>>> - if (meson_ecc[i].strength == nand->ecc.strength) { >>>>>> + if (meson_ecc[i].strength == nand->ecc.strength && >>>>>> + meson_ecc[i].size == nand->ecc.size) { >>>>>> meson_chip->bch_mode = meson_ecc[i].bch; >>>>>> return 0; >>>>>> } >>>>>> @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) >>>>>> struct meson_nfc *nfc = nand_get_controller_data(nand); >>>>>> struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); >>>>>> struct mtd_info *mtd = nand_to_mtd(nand); >>>>>> - int nsectors = mtd->writesize / 1024; >>>>>> + int nsectors = mtd->writesize / 512; >>>>> >>>>> This cannot be unconditional, right? >>>> >>>> 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 '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 ..." ? Thanks, Arseniy > >> else >> nsectors = mtd->writesize / 1024; <--- this is for default 1024 ECC >> >> Thanks, Arseniy >> >>> >>>> >>>> int nsectors = mtd->writesize / 1024; >>>> >>>> Moreover 1024 is default ECC step size for this driver, so default behaviour >>>> will be preserved. >>> >>> Yes, otherwise you would break existing users. >>> >>>> >>>> Thanks, Arseniy >>>> >>>>> >>>>>> int raw_writesize; >>>>>> int ret; >>>>>> >>>>> >>>>> >>>>> Thanks, >>>>> Miquèl >>> >>> >>> Thanks, >>> Miquèl > > > Thanks, > Miquèl
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
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. Thanks, Arseniy > > Thanks, > Miquèl
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
On 04.07.2023 18:32, Miquel Raynal wrote: > 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 :) Thanks again for review and details! :) Thanks, Arseniy > > Cheers, > Miquèl
diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/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; }; struct meson_nfc_data { @@ -190,7 +191,8 @@ struct meson_nfc { }; enum { - NFC_ECC_BCH8_1K = 2, + NFC_ECC_BCH8_512 = 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, }; -#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} +#define MESON_ECC_DATA(b, s, sz) { .bch = (b), .strength = (s), .size = (sz) } static struct meson_nand_ecc meson_ecc[] = { - 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), }; 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_size, int strength) 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[] = { 8 }; +static const struct nand_ecc_step_info axg_stepinfo_1024 = { + .stepsize = 1024, + .strengths = axg_stepinfo_strengths, + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) +}; + +static const struct nand_ecc_step_info axg_stepinfo_512 = { + .stepsize = 512, + .strengths = axg_stepinfo_strengths, + .nstrengths = ARRAY_SIZE(axg_stepinfo_strengths) +}; + +static const struct nand_ecc_step_info axg_stepinfo[] = { axg_stepinfo_1024, axg_stepinfo_512 }; + +static const struct nand_ecc_caps meson_axg_ecc_caps = { + .stepinfos = axg_stepinfo, + .nstepinfos = ARRAY_SIZE(axg_stepinfo), + .calc_ecc_bytes = meson_nand_calc_ecc_bytes, +}; 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_chip *nand) return -EINVAL; for (i = 0; i < ARRAY_SIZE(meson_ecc); i++) { - if (meson_ecc[i].strength == nand->ecc.strength) { + if (meson_ecc[i].strength == nand->ecc.strength && + meson_ecc[i].size == nand->ecc.size) { meson_chip->bch_mode = meson_ecc[i].bch; return 0; } @@ -1278,7 +1301,7 @@ static int meson_nand_attach_chip(struct nand_chip *nand) struct meson_nfc *nfc = nand_get_controller_data(nand); struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); struct mtd_info *mtd = nand_to_mtd(nand); - int nsectors = mtd->writesize / 1024; + int nsectors = mtd->writesize / 512; int raw_writesize; int ret;