| Message ID | 20190304201522.11323-15-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: rawnand: 5th batch of cleanups | expand |
Hi, On Mon, Mar 04, 2019 at 09:15:21PM +0100, Miquel Raynal wrote: > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > index fbf6ca015cd7..a204f9d7e123 100644 > --- a/drivers/mtd/nand/raw/internals.h > +++ b/drivers/mtd/nand/raw/internals.h > @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip, > if (!nand_has_exec_op(chip)) > return -ENOTSUPP; > > - if (WARN_ON(op->cs >= chip->numchips)) > + if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base))) > return -EINVAL; This warning triggers when I apply my gpmi nand exec_op series. The gpmi driver calls: ret = nand_scan(chip, GPMI_IS_MX6(this) ? 2 : 1); This ends up in nand_scan_ident() with maxchips = 2. Here nand_detect() is called which sets memorg->ntargets = 1; Later in nand_scan_ident() we have: for (i = 1; i < maxchips; i++) { u8 id[2]; /* See comment in nand_get_flash_type for reset */ ret = nand_reset(chip, i); if (ret) break; .... this nand_reset() calls nand_exec_op() with op->cs = 1, nanddev_ntargets() = 1 and boom. I can't see how this can work with anything else but maxchips = 1. Do you have an idea how this is supposed to work? Sascha
On Tue, 21 May 2019 08:59:48 +0200 Sascha Hauer <s.hauer@pengutronix.de> wrote: > Hi, > > On Mon, Mar 04, 2019 at 09:15:21PM +0100, Miquel Raynal wrote: > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > > index fbf6ca015cd7..a204f9d7e123 100644 > > --- a/drivers/mtd/nand/raw/internals.h > > +++ b/drivers/mtd/nand/raw/internals.h > > @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip, > > if (!nand_has_exec_op(chip)) > > return -ENOTSUPP; > > > > - if (WARN_ON(op->cs >= chip->numchips)) > > + if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base))) > > return -EINVAL; > > This warning triggers when I apply my gpmi nand exec_op series. > > The gpmi driver calls: > > ret = nand_scan(chip, GPMI_IS_MX6(this) ? 2 : 1); > > This ends up in nand_scan_ident() with maxchips = 2. Here nand_detect() > is called which sets memorg->ntargets = 1; Later in nand_scan_ident() we > have: > > for (i = 1; i < maxchips; i++) { This loop should be fixed to test against nanddev_ntargets() instead of maxchips. > u8 id[2]; > > /* See comment in nand_get_flash_type for reset */ > ret = nand_reset(chip, i); > if (ret) > break; > .... > > this nand_reset() calls nand_exec_op() with op->cs = 1, nanddev_ntargets() = 1 > and boom. > > I can't see how this can work with anything else but maxchips = 1. Do you > have an idea how this is supposed to work? > > Sascha > >
On Tue, 21 May 2019 09:33:02 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Tue, 21 May 2019 08:59:48 +0200 > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > Hi, > > > > On Mon, Mar 04, 2019 at 09:15:21PM +0100, Miquel Raynal wrote: > > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > > > index fbf6ca015cd7..a204f9d7e123 100644 > > > --- a/drivers/mtd/nand/raw/internals.h > > > +++ b/drivers/mtd/nand/raw/internals.h > > > @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip, > > > if (!nand_has_exec_op(chip)) > > > return -ENOTSUPP; > > > > > > - if (WARN_ON(op->cs >= chip->numchips)) > > > + if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base))) > > > return -EINVAL; > > > > This warning triggers when I apply my gpmi nand exec_op series. > > > > The gpmi driver calls: > > > > ret = nand_scan(chip, GPMI_IS_MX6(this) ? 2 : 1); > > > > This ends up in nand_scan_ident() with maxchips = 2. Here nand_detect() > > is called which sets memorg->ntargets = 1; Later in nand_scan_ident() we > > have: > > > > for (i = 1; i < maxchips; i++) { > > This loop should be fixed to test against nanddev_ntargets() instead of > maxchips. > > > u8 id[2]; > > > > /* See comment in nand_get_flash_type for reset */ > > ret = nand_reset(chip, i); > > if (ret) > > break; > > .... > > > > this nand_reset() calls nand_exec_op() with op->cs = 1, nanddev_ntargets() = 1 > > and boom. > > > > I can't see how this can work with anything else but maxchips = 1. Do you > > have an idea how this is supposed to work? Forgot to reply to that one. ->ntargets is set to the number of dies/tartgets actually detected here [1], so it's not always 1 (can also be extracted from the ONFI table IIRC). Note that I've never been a big fan of this maxchip param, and I've asked that new drivers pass the actual number of CS connected to the NAND chip being initialized (which should be part of the HW desc, be it DT based or board-file based). So, ideally this argument should be named num_dies or num_targets and the function should return an error when one of the die returns a different ID. Unfortunately, that's not something we can do, because a lot of drivers rely on the old semantic... [1]https://elixir.bootlin.com/linux/v5.2-rc1/source/drivers/mtd/nand/raw/nand_base.c#L5073
On Tue, 21 May 2019 09:33:02 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Tue, 21 May 2019 08:59:48 +0200 > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > Hi, > > > > On Mon, Mar 04, 2019 at 09:15:21PM +0100, Miquel Raynal wrote: > > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > > > index fbf6ca015cd7..a204f9d7e123 100644 > > > --- a/drivers/mtd/nand/raw/internals.h > > > +++ b/drivers/mtd/nand/raw/internals.h > > > @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip, > > > if (!nand_has_exec_op(chip)) > > > return -ENOTSUPP; > > > > > > - if (WARN_ON(op->cs >= chip->numchips)) > > > + if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base))) > > > return -EINVAL; > > > > This warning triggers when I apply my gpmi nand exec_op series. > > > > The gpmi driver calls: > > > > ret = nand_scan(chip, GPMI_IS_MX6(this) ? 2 : 1); > > > > This ends up in nand_scan_ident() with maxchips = 2. Here nand_detect() > > is called which sets memorg->ntargets = 1; Later in nand_scan_ident() we > > have: > > > > for (i = 1; i < maxchips; i++) { > > This loop should be fixed to test against nanddev_ntargets() instead of > maxchips. Nevermind, I see what you mean. I guess we should set ->ntargets to maxchips before entering this loop.
On Tue, May 21, 2019 at 09:33:02AM +0200, Boris Brezillon wrote: > On Tue, 21 May 2019 08:59:48 +0200 > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > Hi, > > > > On Mon, Mar 04, 2019 at 09:15:21PM +0100, Miquel Raynal wrote: > > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > > > index fbf6ca015cd7..a204f9d7e123 100644 > > > --- a/drivers/mtd/nand/raw/internals.h > > > +++ b/drivers/mtd/nand/raw/internals.h > > > @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip, > > > if (!nand_has_exec_op(chip)) > > > return -ENOTSUPP; > > > > > > - if (WARN_ON(op->cs >= chip->numchips)) > > > + if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base))) > > > return -EINVAL; > > > > This warning triggers when I apply my gpmi nand exec_op series. > > > > The gpmi driver calls: > > > > ret = nand_scan(chip, GPMI_IS_MX6(this) ? 2 : 1); > > > > This ends up in nand_scan_ident() with maxchips = 2. Here nand_detect() > > is called which sets memorg->ntargets = 1; Later in nand_scan_ident() we > > have: > > > > for (i = 1; i < maxchips; i++) { > > This loop should be fixed to test against nanddev_ntargets() instead of > maxchips. This makes the maxchips argument to nand_scan() unused. A lot of drivers are calling nand_scan() with maxchips > 1. How are these working then? Or should there be a memorg->ntargets = maxchips before the loop? Sascha
On Tue, May 21, 2019 at 09:51:30AM +0200, Boris Brezillon wrote: > On Tue, 21 May 2019 09:33:02 +0200 > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > On Tue, 21 May 2019 08:59:48 +0200 > > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > Hi, > > > > > > On Mon, Mar 04, 2019 at 09:15:21PM +0100, Miquel Raynal wrote: > > > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > > > > index fbf6ca015cd7..a204f9d7e123 100644 > > > > --- a/drivers/mtd/nand/raw/internals.h > > > > +++ b/drivers/mtd/nand/raw/internals.h > > > > @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip, > > > > if (!nand_has_exec_op(chip)) > > > > return -ENOTSUPP; > > > > > > > > - if (WARN_ON(op->cs >= chip->numchips)) > > > > + if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base))) > > > > return -EINVAL; > > > > > > This warning triggers when I apply my gpmi nand exec_op series. > > > > > > The gpmi driver calls: > > > > > > ret = nand_scan(chip, GPMI_IS_MX6(this) ? 2 : 1); > > > > > > This ends up in nand_scan_ident() with maxchips = 2. Here nand_detect() > > > is called which sets memorg->ntargets = 1; Later in nand_scan_ident() we > > > have: > > > > > > for (i = 1; i < maxchips; i++) { > > > > This loop should be fixed to test against nanddev_ntargets() instead of > > maxchips. > > Nevermind, I see what you mean. I guess we should set ->ntargets to > maxchips before entering this loop. Okay, you got the same conclusion in the meantime ;) Sascha
On Tue, 21 May 2019 09:58:25 +0200 Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Tue, May 21, 2019 at 09:51:30AM +0200, Boris Brezillon wrote: > > On Tue, 21 May 2019 09:33:02 +0200 > > Boris Brezillon <boris.brezillon@collabora.com> wrote: > > > > > On Tue, 21 May 2019 08:59:48 +0200 > > > Sascha Hauer <s.hauer@pengutronix.de> wrote: > > > > > > > Hi, > > > > > > > > On Mon, Mar 04, 2019 at 09:15:21PM +0100, Miquel Raynal wrote: > > > > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > > > > > index fbf6ca015cd7..a204f9d7e123 100644 > > > > > --- a/drivers/mtd/nand/raw/internals.h > > > > > +++ b/drivers/mtd/nand/raw/internals.h > > > > > @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip, > > > > > if (!nand_has_exec_op(chip)) > > > > > return -ENOTSUPP; > > > > > > > > > > - if (WARN_ON(op->cs >= chip->numchips)) > > > > > + if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base))) > > > > > return -EINVAL; > > > > > > > > This warning triggers when I apply my gpmi nand exec_op series. > > > > > > > > The gpmi driver calls: > > > > > > > > ret = nand_scan(chip, GPMI_IS_MX6(this) ? 2 : 1); > > > > > > > > This ends up in nand_scan_ident() with maxchips = 2. Here nand_detect() > > > > is called which sets memorg->ntargets = 1; Later in nand_scan_ident() we > > > > have: > > > > > > > > for (i = 1; i < maxchips; i++) { > > > > > > This loop should be fixed to test against nanddev_ntargets() instead of > > > maxchips. > > > > Nevermind, I see what you mean. I guess we should set ->ntargets to > > maxchips before entering this loop. > > Okay, you got the same conclusion in the meantime ;) Actually, you can just replace memorg->ntargets = 1; by memorg->ntargers = maxchips;
diff --git a/drivers/mtd/nand/raw/diskonchip.c b/drivers/mtd/nand/raw/diskonchip.c index e9767e06415d..3cee832716ec 100644 --- a/drivers/mtd/nand/raw/diskonchip.c +++ b/drivers/mtd/nand/raw/diskonchip.c @@ -1291,7 +1291,7 @@ static int __init inftl_scan_bbt(struct mtd_info *mtd) struct doc_priv *doc = nand_get_controller_data(this); struct mtd_partition parts[5]; - if (this->numchips > doc->chips_per_floor) { + if (nanddev_ntargets(&this->base) > doc->chips_per_floor) { pr_err("Multi-floor INFTL devices not yet supported.\n"); return -EIO; } diff --git a/drivers/mtd/nand/raw/fsl_elbc_nand.c b/drivers/mtd/nand/raw/fsl_elbc_nand.c index 1d960a6cd691..293a5b71833a 100644 --- a/drivers/mtd/nand/raw/fsl_elbc_nand.c +++ b/drivers/mtd/nand/raw/fsl_elbc_nand.c @@ -653,7 +653,7 @@ static int fsl_elbc_attach_chip(struct nand_chip *chip) priv->fmr |= al << FMR_AL_SHIFT; dev_dbg(priv->dev, "fsl_elbc_init: nand->numchips = %d\n", - chip->numchips); + nanddev_ntargets(&chip->base)); dev_dbg(priv->dev, "fsl_elbc_init: nand->chipsize = %lld\n", nanddev_target_size(&chip->base)); dev_dbg(priv->dev, "fsl_elbc_init: nand->pagemask = %8x\n", diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c b/drivers/mtd/nand/raw/fsl_ifc_nand.c index a9e8f89aeebd..04a3dcd675bf 100644 --- a/drivers/mtd/nand/raw/fsl_ifc_nand.c +++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c @@ -722,7 +722,7 @@ static int fsl_ifc_attach_chip(struct nand_chip *chip) struct fsl_ifc_mtd *priv = nand_get_controller_data(chip); dev_dbg(priv->dev, "%s: nand->numchips = %d\n", __func__, - chip->numchips); + nanddev_ntargets(&chip->base)); dev_dbg(priv->dev, "%s: nand->chipsize = %lld\n", __func__, nanddev_target_size(&chip->base)); dev_dbg(priv->dev, "%s: nand->pagemask = %8x\n", __func__, diff --git a/drivers/mtd/nand/raw/hisi504_nand.c b/drivers/mtd/nand/raw/hisi504_nand.c index f3f9aa160cff..e4526fff9da4 100644 --- a/drivers/mtd/nand/raw/hisi504_nand.c +++ b/drivers/mtd/nand/raw/hisi504_nand.c @@ -849,7 +849,7 @@ static int hisi_nfc_resume(struct device *dev) struct hinfc_host *host = dev_get_drvdata(dev); struct nand_chip *chip = &host->chip; - for (cs = 0; cs < chip->numchips; cs++) + for (cs = 0; cs < nanddev_ntargets(&chip->base); cs++) hisi_nfc_send_cmd_reset(host, cs); hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH, HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH); diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h index fbf6ca015cd7..a204f9d7e123 100644 --- a/drivers/mtd/nand/raw/internals.h +++ b/drivers/mtd/nand/raw/internals.h @@ -110,7 +110,7 @@ static inline int nand_exec_op(struct nand_chip *chip, if (!nand_has_exec_op(chip)) return -ENOTSUPP; - if (WARN_ON(op->cs >= chip->numchips)) + if (WARN_ON(op->cs >= nanddev_ntargets(&chip->base))) return -EINVAL; return chip->controller->ops->exec_op(chip, op, false); diff --git a/drivers/mtd/nand/raw/jz4740_nand.c b/drivers/mtd/nand/raw/jz4740_nand.c index 06690b3603b1..c992a664bdee 100644 --- a/drivers/mtd/nand/raw/jz4740_nand.c +++ b/drivers/mtd/nand/raw/jz4740_nand.c @@ -354,7 +354,6 @@ static int jz_nand_detect_bank(struct platform_device *pdev, } /* Update size of the MTD. */ - chip->numchips++; memorg->ntargets++; mtd->size += nanddev_target_size(&chip->base); } diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 427c80abef3f..ce01824dcc62 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -240,10 +240,10 @@ static int check_offs_len(struct nand_chip *chip, loff_t ofs, uint64_t len) void nand_select_target(struct nand_chip *chip, unsigned int cs) { /* - * cs should always lie between 0 and chip->numchips, when that's not - * the case it's a bug and the caller should be fixed. + * cs should always lie between 0 and nanddev_ntargets(), when that's + * not the case it's a bug and the caller should be fixed. */ - if (WARN_ON(cs > chip->numchips)) + if (WARN_ON(cs > nanddev_ntargets(&chip->base))) return; chip->cur_cs = cs; @@ -5042,12 +5042,6 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, if (!mtd->name && mtd->dev.parent) mtd->name = dev_name(mtd->dev.parent); - /* - * Start with chips->numchips = maxchips to let nand_select_target() do - * its job. chip->numchips will be adjusted after. - */ - chip->numchips = maxchips; - /* Set the default functions */ nand_set_defaults(chip); @@ -5091,7 +5085,6 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, /* Store the number of chips and calc total size for mtd */ memorg->ntargets = i; - chip->numchips = i; mtd->size = i * nanddev_target_size(&chip->base); return 0; @@ -5833,7 +5826,7 @@ static int nand_scan_tail(struct nand_chip *chip) goto err_nanddev_cleanup; /* Enter fastest possible mode on all dies. */ - for (i = 0; i < chip->numchips; i++) { + for (i = 0; i < nanddev_ntargets(&chip->base); i++) { ret = nand_setup_data_interface(chip, i); if (ret) goto err_nanddev_cleanup; diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c index e3308857b2ee..303408039650 100644 --- a/drivers/mtd/nand/raw/nand_bbt.c +++ b/drivers/mtd/nand/raw/nand_bbt.c @@ -269,7 +269,7 @@ static int read_abs_bbt(struct nand_chip *this, uint8_t *buf, if (td->options & NAND_BBT_PERCHIP) { int offs = 0; - for (i = 0; i < this->numchips; i++) { + for (i = 0; i < nanddev_ntargets(&this->base); i++) { if (chip == -1 || chip == i) res = read_bbt(this, buf, td->pages[i], targetsize >> this->bbt_erase_shift, @@ -478,9 +478,9 @@ static int create_bbt(struct nand_chip *this, uint8_t *buf, startblock = 0; from = 0; } else { - if (chip >= this->numchips) { + if (chip >= nanddev_ntargets(&this->base)) { pr_warn("create_bbt(): chipnr (%d) > available chips (%d)\n", - chip + 1, this->numchips); + chip + 1, nanddev_ntargets(&this->base)); return -EINVAL; } numblocks = targetsize >> this->bbt_erase_shift; @@ -550,7 +550,7 @@ static int search_bbt(struct nand_chip *this, uint8_t *buf, /* Do we have a bbt per chip? */ if (td->options & NAND_BBT_PERCHIP) { - chips = this->numchips; + chips = nanddev_ntargets(&this->base); bbtblocks = targetsize >> this->bbt_erase_shift; startblock &= bbtblocks - 1; } else { @@ -643,7 +643,7 @@ static int get_bbt_block(struct nand_chip *this, struct nand_bbt_descr *td, numblocks = (int)(targetsize >> this->bbt_erase_shift); if (!(td->options & NAND_BBT_PERCHIP)) - numblocks *= this->numchips; + numblocks *= nanddev_ntargets(&this->base); /* * Automatic placement of the bad block table. Search direction @@ -745,7 +745,7 @@ static int write_bbt(struct nand_chip *this, uint8_t *buf, numblocks = (int)(targetsize >> this->bbt_erase_shift); /* Full device write or specific chip? */ if (chipsel == -1) { - nrchips = this->numchips; + nrchips = nanddev_ntargets(&this->base); } else { nrchips = chipsel + 1; chip = chipsel; @@ -932,7 +932,7 @@ static int check_create(struct nand_chip *this, uint8_t *buf, /* Do we have a bbt per chip? */ if (td->options & NAND_BBT_PERCHIP) - chips = this->numchips; + chips = nanddev_ntargets(&this->base); else chips = 1; @@ -1111,7 +1111,7 @@ static void mark_bbt_region(struct nand_chip *this, struct nand_bbt_descr *td) /* Do we have a bbt per chip? */ if (td->options & NAND_BBT_PERCHIP) { - chips = this->numchips; + chips = nanddev_ntargets(&this->base); nrblocks = (int)(targetsize >> this->bbt_erase_shift); } else { chips = 1; diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h index a127eb773b1a..e5a25ba75211 100644 --- a/include/linux/mtd/rawnand.h +++ b/include/linux/mtd/rawnand.h @@ -1014,7 +1014,6 @@ struct nand_legacy { * set to the actually used ONFI mode if the chip is * ONFI compliant or deduced from the datasheet if * the NAND chip is not ONFI compliant. - * @numchips: [INTERN] number of physical chips * @pagemask: [INTERN] page number mask = number of (pages / chip) - 1 * @data_buf: [INTERN] buffer for data, size is (page size + oobsize). * @pagecache: Structure containing page cache related fields @@ -1028,8 +1027,9 @@ struct nand_legacy { * @data_interface: [INTERN] NAND interface timing information * @cur_cs: currently selected target. -1 means no target selected, * otherwise we should always have cur_cs >= 0 && - * cur_cs < numchips. NAND Controller drivers should not - * modify this value, but they're allowed to read it. + * cur_cs < nanddev_ntargets(). NAND Controller drivers + * should not modify this value, but they're allowed to + * read it. * @read_retries: [INTERN] the number of read retry modes supported * @bbt: [INTERN] bad block table pointer * @bbt_td: [REPLACEABLE] bad block table descriptor for flash @@ -1060,7 +1060,6 @@ struct nand_chip { int phys_erase_shift; int bbt_erase_shift; int chip_shift; - int numchips; int pagemask; u8 *data_buf;