Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753924AbdH2Nls convert rfc822-to-8bit (ORCPT ); Tue, 29 Aug 2017 09:41:48 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:58997 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751953AbdH2Nlr (ORCPT ); Tue, 29 Aug 2017 09:41:47 -0400 Date: Tue, 29 Aug 2017 15:41:44 +0200 From: Boris Brezillon To: Lothar =?UTF-8?B?V2HDn21hbm4=?= Cc: Brian Norris , Cyrille Pitchen , David Woodhouse , Marek Vasut , Richard Weinberger , linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org Subject: Re: [PATCH 1/2] mtd: nand: make Samsung SLC NAND usable again Message-ID: <20170829154144.25f9d0ac@bbrezillon> In-Reply-To: <20170829151807.3a31a1fe@karo-electronics.de> References: <1504001833-18097-1-git-send-email-LW@KARO-electronics.de> <1504001833-18097-2-git-send-email-LW@KARO-electronics.de> <20170829141658.252a84e0@bbrezillon> <20170829151807.3a31a1fe@karo-electronics.de> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8189 Lines: 221 On Tue, 29 Aug 2017 15:18:07 +0200 Lothar Waßmann wrote: > Hi, > > On Tue, 29 Aug 2017 14:16:58 +0200 Boris Brezillon wrote: > > Hi Lothar, > > > > On Tue, 29 Aug 2017 12:17:12 +0200 > > Lothar Waßmann wrote: > > > > > commit c51d0ac59f24 ("mtd: nand: Move Samsung specific init/detection > > > logic in nand_samsung.c") introduced a regression for Samsung SLC NAND > > > chips. Prior to this commit chip->bits_per_cell was initialized by calling > > > nand_get_bits_per_cell() before using nand_is_slc(). > > > With the offending commit this call is skipped, leaving > > > chip->bits_per_cell cleared to zero when the manufacturer specific > > > '.detect' function calls nand_is_slc() which in turn interprets > > > bits_per_cell != 1 as indication for an MLC chip. > > > The effect is that e.g. a K9F1G08U0F NAND chip is falsely detected as > > > MLC NAND with 4KiB page size rather than SLC with 2KiB page size. > > > > Oops, sorry for this regression. > > > > > > > > Add a call to nand_get_bits_per_cell() before calling the .detect hook > > > function in nand_manufacturer_detect(), so that the nand_is_slc() > > > calls in the manufacturer specific code will return correct results. > > > > I'd prefer a different solution (see below). > > > > > > > > Signed-off-by: Lothar Waßmann > > > --- > > > drivers/mtd/nand/nand_base.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > > index 9900476..bcc8cef1 100644 > > > --- a/drivers/mtd/nand/nand_base.c > > > +++ b/drivers/mtd/nand/nand_base.c > > > @@ -3820,10 +3820,13 @@ static void nand_manufacturer_detect(struct nand_chip *chip) > > > * nand_decode_ext_id() otherwise. > > > */ > > > if (chip->manufacturer.desc && chip->manufacturer.desc->ops && > > > - chip->manufacturer.desc->ops->detect) > > > + chip->manufacturer.desc->ops->detect) { > > > + /* The 3rd id byte holds MLC / multichip data */ > > > + chip->bits_per_cell = nand_get_bits_per_cell(chip->id.data[2]); > > > > I'd prefer not to force this bit_per_cell detection here. How about > > explicitly calling nand_decode_ext_id() from the samsung and hynix > > ->detect() hooks (see proposed diff below)? > > > I chose the same place in the code flow where this initialization had > been before. And it does only that portion of nand_decode_ext_id() that > was executed prior to the vendor specific code in the old code. > A call to nand_decode_ext_id() would do more than has been done > previously. My main concern is, can we be sure this portion of the 3rd byte is always used to encode the bits-per-cell information? NAND vendors tend to take liberties with the NAND ids fields and I fear we'll someday have a NAND that does not follow this encoding scheme. This being said, find_full_id_nand() also calls nand_get_bits_per_cell() even though it's using full-id information for other characteristics, which tend to confirm noone ever had a NAND abusing this bits-per-cell field. I'll just take your patch as is and add Cc-stable a Fixes tags. Note that I'm planning to rework the NAND detection logic a bit to let manufacturer code tweak the characteristics even if the NAND is ONFI or JEDEC compliant (see below). > > I prefer not to have to rely on every single manufacturer dependent > code calling this function on its own. But you are the maintainer and > have to decide finally. > With my second patch it should be easy to spot when the call is missing > though. Yep, your second patch is fine. > > Another alternative were to let nand_is_slc() do the initialization > from id_data when it is first called (bits_per_cell == 0). Well, you could do return bits_per_cell <= 1 but I think your WARN() is appropriate (though I'd put it somewhere else, like just after the detection logic). --->8--- diff --git a/drivers/mtd/nand/nand_amd.c b/drivers/mtd/nand/nand_amd.c index 170403a3bfa8..95342862fd31 100644 --- a/drivers/mtd/nand/nand_amd.c +++ b/drivers/mtd/nand/nand_amd.c @@ -21,7 +21,9 @@ static void amd_nand_decode_id(struct nand_chip *chip) { struct mtd_info *mtd = nand_to_mtd(chip); - nand_decode_ext_id(chip); + /* Skip ID decoding for JEDEC or ONFI chips. */ + if (chip->onfi_version || chip->jedec_version) + return; /* * Check for Spansion/AMD ID + repeating 5th, 6th byte since diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index c6c18b82f8f4..0640687b50c2 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -3982,9 +3982,9 @@ static bool find_full_id_nand(struct nand_chip *chip, } /* - * Manufacturer detection. Only used when the NAND is not ONFI or JEDEC - * compliant and does not have a full-id or legacy-id entry in the nand_ids - * table. + * Manufacturer detection. This follows the regular ONFI, JEDEC or ID decoding + * detection and allows driver specific code to tweak the NAND chip + * characteristics. */ static void nand_manufacturer_detect(struct nand_chip *chip) { @@ -3995,8 +3995,6 @@ static void nand_manufacturer_detect(struct nand_chip *chip) if (chip->manufacturer.desc && chip->manufacturer.desc->ops && chip->manufacturer.desc->ops->detect) chip->manufacturer.desc->ops->detect(chip); - else - nand_decode_ext_id(chip); } /* @@ -4128,7 +4126,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type) chip->chipsize = (uint64_t)type->chipsize << 20; if (!type->pagesize) - nand_manufacturer_detect(chip); + nand_decode_ext_id(chip) else nand_decode_id(chip, type); @@ -4136,6 +4134,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type) chip->options |= type->options; ident_done: + nand_manufacturer_detect(chip); if (chip->options & NAND_BUSWIDTH_AUTO) { WARN_ON(busw & NAND_BUSWIDTH_16); diff --git a/drivers/mtd/nand/nand_hynix.c b/drivers/mtd/nand/nand_hynix.c index b12dc7325378..b77b45c4ed03 100644 --- a/drivers/mtd/nand/nand_hynix.c +++ b/drivers/mtd/nand/nand_hynix.c @@ -547,6 +547,10 @@ static void hynix_nand_decode_id(struct nand_chip *chip) bool valid_jedecid; u8 tmp; + /* Skip ID decoding for JEDEC or ONFI chips. */ + if (chip->onfi_version || chip->jedec_version) + return; + /* * Exclude all SLC NANDs from this advanced detection scheme. * According to the ranges defined in several datasheets, it might @@ -554,10 +558,8 @@ static void hynix_nand_decode_id(struct nand_chip *chip) * If that the case rework the test to let SLC NANDs go through the * detection process. */ - if (chip->id.len < 6 || nand_is_slc(chip)) { - nand_decode_ext_id(chip); + if (chip->id.len < 6 || nand_is_slc(chip)) return; - } /* Extract pagesize */ mtd->writesize = 2048 << (chip->id.data[3] & 0x03); diff --git a/drivers/mtd/nand/nand_samsung.c b/drivers/mtd/nand/nand_samsung.c index 1e0755997762..9da0a2101d63 100644 --- a/drivers/mtd/nand/nand_samsung.c +++ b/drivers/mtd/nand/nand_samsung.c @@ -21,6 +21,10 @@ static void samsung_nand_decode_id(struct nand_chip *chip) { struct mtd_info *mtd = nand_to_mtd(chip); + /* Skip ID decoding for JEDEC or ONFI chips. */ + if (chip->onfi_version || chip->jedec_version) + return; + /* New Samsung (6 byte ID): Samsung K9GAG08U0F (p.44) */ if (chip->id.len == 6 && !nand_is_slc(chip) && chip->id.data[5] != 0x00) { @@ -89,8 +93,6 @@ static void samsung_nand_decode_id(struct nand_chip *chip) chip->ecc_step_ds = 0; } } - } else { - nand_decode_ext_id(chip); } } diff --git a/drivers/mtd/nand/nand_toshiba.c b/drivers/mtd/nand/nand_toshiba.c index fa787ba38dcd..1aa56363bc16 100644 --- a/drivers/mtd/nand/nand_toshiba.c +++ b/drivers/mtd/nand/nand_toshiba.c @@ -21,7 +21,9 @@ static void toshiba_nand_decode_id(struct nand_chip *chip) { struct mtd_info *mtd = nand_to_mtd(chip); - nand_decode_ext_id(chip); + /* Skip ID decoding for JEDEC or ONFI chips. */ + if (chip->onfi_version || chip->jedec_version) + return; /* * Toshiba 24nm raw SLC (i.e., not BENAND) have 32B OOB per