| Message ID | 20181106150810.9569-14-boris.brezillon@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: rawnand: 3rd batch of cleanup | expand |
Hi Boris, On Tuesday, November 6, 2018 4:08:01 PM CET Boris Brezillon wrote: > In order to deprecate the ->select_chip hook we need to pass the CS > line a NAND operations are targeting. This is done through the > addition of a cs field to the nand_operation struct. > > We also need to keep track of the currently selected target to > properly initialize op->cs, hence the ->cur_cs field addition to the > nand_chip struct. > > Note that op->cs is not assigned in nand_exec_op() because we might > rework the way we execute NAND operations in the future (adopt a > queuing mechanism instead of the serialization we have right now). > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > Changes in v2: > - Initialize ->cur_cs > --- > drivers/mtd/nand/raw/internals.h | 3 +++ > drivers/mtd/nand/raw/nand_base.c | 39 +++++++++++++++++-------------- > drivers/mtd/nand/raw/nand_hynix.c | 4 ++-- > include/linux/mtd/rawnand.h | 11 ++++++++- > 4 files changed, 37 insertions(+), 20 deletions(-) > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > index 6e2f61fbc5f0..b62728d5884b 100644 > --- a/drivers/mtd/nand/raw/internals.h > +++ b/drivers/mtd/nand/raw/internals.h > @@ -101,6 +101,9 @@ static inline int nand_exec_op(struct nand_chip *chip, > if (!chip->exec_op) > return -ENOTSUPP; > > + if (WARN_ON(op->cs >= chip->numchips)) > + return -EINVAL; This needs a fix to nand_scan_ident() like the following: --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -5034,10 +5034,11 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, /* Set the default functions */ nand_set_defaults(chip); /* Read the flash type */ + chip->numchips = 1; ret = nand_detect(chip, table); if (ret) { if (!(chip->options & NAND_SCAN_SILENT_NODEV)) pr_warn("No NAND device found\n"); nand_deselect_target(chip); Otherwise nand_detect() fails for me in nand_exec_op() called from the very first nand_reset_op(): WARNING: CPU: 0 PID: 1 at drivers/mtd/nand/raw/internals.h:104 nand_reset_op+0x100/0x17c ... nand: No NAND device found With that issue fixed one way or another, you can add my: Tested-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> to your whole series. Thanks, Janusz PS. I'm ready to submit a series which converts ams-delta-nand to use GPIO API for data I/O. My only concerns is which branch should I rebase it on.
Hi Janusz, On Sat, 10 Nov 2018 02:30:04 +0100 Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > Hi Boris, > > On Tuesday, November 6, 2018 4:08:01 PM CET Boris Brezillon wrote: > > In order to deprecate the ->select_chip hook we need to pass the CS > > line a NAND operations are targeting. This is done through the > > addition of a cs field to the nand_operation struct. > > > > We also need to keep track of the currently selected target to > > properly initialize op->cs, hence the ->cur_cs field addition to the > > nand_chip struct. > > > > Note that op->cs is not assigned in nand_exec_op() because we might > > rework the way we execute NAND operations in the future (adopt a > > queuing mechanism instead of the serialization we have right now). > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > Changes in v2: > > - Initialize ->cur_cs > > --- > > drivers/mtd/nand/raw/internals.h | 3 +++ > > drivers/mtd/nand/raw/nand_base.c | 39 +++++++++++++++++-------------- > > drivers/mtd/nand/raw/nand_hynix.c | 4 ++-- > > include/linux/mtd/rawnand.h | 11 ++++++++- > > 4 files changed, 37 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > > index 6e2f61fbc5f0..b62728d5884b 100644 > > --- a/drivers/mtd/nand/raw/internals.h > > +++ b/drivers/mtd/nand/raw/internals.h > > @@ -101,6 +101,9 @@ static inline int nand_exec_op(struct nand_chip *chip, > > if (!chip->exec_op) > > return -ENOTSUPP; > > > > + if (WARN_ON(op->cs >= chip->numchips)) > > + return -EINVAL; > > This needs a fix to nand_scan_ident() like the following: > > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -5034,10 +5034,11 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, > > /* Set the default functions */ > nand_set_defaults(chip); > > /* Read the flash type */ > + chip->numchips = 1; Oh, you're right. Actually it should be chip->numchips = maxchips; and should be done in patch 12. > ret = nand_detect(chip, table); > if (ret) { > if (!(chip->options & NAND_SCAN_SILENT_NODEV)) > pr_warn("No NAND device found\n"); > nand_deselect_target(chip); > > > Otherwise nand_detect() fails for me in nand_exec_op() called from the very > first nand_reset_op(): > > WARNING: CPU: 0 PID: 1 at drivers/mtd/nand/raw/internals.h:104 nand_reset_op+0x100/0x17c > ... > nand: No NAND device found > > With that issue fixed one way or another, you can add my: > > Tested-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > to your whole series. Thanks for testing. > > Thanks, > Janusz > > > PS. > I'm ready to submit a series which converts ams-delta-nand to use GPIO > API for data I/O. My only concerns is which branch should I rebase it on. I'd say nand/next, unless you have dependencies on new GPIO stuff that are not in 4.20-rc1. Regards, Boris
Hi Boris, On Saturday, November 10, 2018 9:04:59 AM CET Boris Brezillon wrote: > Hi Janusz, > > On Sat, 10 Nov 2018 02:30:04 +0100 > Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > > > Hi Boris, > > > > On Tuesday, November 6, 2018 4:08:01 PM CET Boris Brezillon wrote: > > > In order to deprecate the ->select_chip hook we need to pass the CS > > > line a NAND operations are targeting. This is done through the > > > addition of a cs field to the nand_operation struct. > > > > > > We also need to keep track of the currently selected target to > > > properly initialize op->cs, hence the ->cur_cs field addition to the > > > nand_chip struct. > > > > > > Note that op->cs is not assigned in nand_exec_op() because we might > > > rework the way we execute NAND operations in the future (adopt a > > > queuing mechanism instead of the serialization we have right now). > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > --- > > > Changes in v2: > > > - Initialize ->cur_cs > > > --- > > > drivers/mtd/nand/raw/internals.h | 3 +++ > > > drivers/mtd/nand/raw/nand_base.c | 39 +++++++++++++++++-------------- > > > drivers/mtd/nand/raw/nand_hynix.c | 4 ++-- > > > include/linux/mtd/rawnand.h | 11 ++++++++- > > > 4 files changed, 37 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > > > index 6e2f61fbc5f0..b62728d5884b 100644 > > > --- a/drivers/mtd/nand/raw/internals.h > > > +++ b/drivers/mtd/nand/raw/internals.h > > > @@ -101,6 +101,9 @@ static inline int nand_exec_op(struct nand_chip *chip, > > > if (!chip->exec_op) > > > return -ENOTSUPP; > > > > > > + if (WARN_ON(op->cs >= chip->numchips)) > > > + return -EINVAL; > > > > This needs a fix to nand_scan_ident() like the following: > > > > --- a/drivers/mtd/nand/raw/nand_base.c > > +++ b/drivers/mtd/nand/raw/nand_base.c > > @@ -5034,10 +5034,11 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, > > > > /* Set the default functions */ > > nand_set_defaults(chip); > > > > /* Read the flash type */ > > + chip->numchips = 1; > > Oh, you're right. Actually it should be > > chip->numchips = maxchips; Indeed, setting it to 1 would probably result in chips beyond the first one never detected, which is not my case. > ... > > PS. > > I'm ready to submit a series which converts ams-delta-nand to use GPIO > > API for data I/O. My only concerns is which branch should I rebase it on. > > I'd say nand/next, unless you have dependencies on new GPIO stuff that > are not in 4.20-rc1. All GPIO dependencies are already in 4..20-rc1. My concern was rather related to a future merge with this series which will raise some conflicts against mine. Thanks, Janusz
diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h index 6e2f61fbc5f0..b62728d5884b 100644 --- a/drivers/mtd/nand/raw/internals.h +++ b/drivers/mtd/nand/raw/internals.h @@ -101,6 +101,9 @@ static inline int nand_exec_op(struct nand_chip *chip, if (!chip->exec_op) return -ENOTSUPP; + if (WARN_ON(op->cs >= chip->numchips)) + return -EINVAL; + return chip->exec_op(chip, op, false); } diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index c684beab8f34..26e7bfb22a58 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -246,6 +246,7 @@ void nand_select_target(struct nand_chip *chip, unsigned int cs) if (WARN_ON(cs > chip->numchips)) return; + chip->cur_cs = cs; chip->select_chip(chip, cs); } EXPORT_SYMBOL_GPL(nand_select_target); @@ -260,6 +261,7 @@ EXPORT_SYMBOL_GPL(nand_select_target); void nand_deselect_target(struct nand_chip *chip) { chip->select_chip(chip, -1); + chip->cur_cs = -1; } EXPORT_SYMBOL_GPL(nand_deselect_target); @@ -1022,7 +1024,7 @@ static int nand_sp_exec_read_page_op(struct nand_chip *chip, unsigned int page, PSEC_TO_NSEC(sdr->tRR_min)), NAND_OP_DATA_IN(len, buf, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); int ret; /* Drop the DATA_IN instruction if len is set to 0. */ @@ -1065,7 +1067,7 @@ static int nand_lp_exec_read_page_op(struct nand_chip *chip, unsigned int page, PSEC_TO_NSEC(sdr->tRR_min)), NAND_OP_DATA_IN(len, buf, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); int ret; /* Drop the DATA_IN instruction if len is set to 0. */ @@ -1160,7 +1162,7 @@ int nand_read_param_page_op(struct nand_chip *chip, u8 page, void *buf, PSEC_TO_NSEC(sdr->tRR_min)), NAND_OP_8BIT_DATA_IN(len, buf, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); /* Drop the DATA_IN instruction if len is set to 0. */ if (!len) @@ -1216,7 +1218,7 @@ int nand_change_read_column_op(struct nand_chip *chip, PSEC_TO_NSEC(sdr->tCCS_min)), NAND_OP_DATA_IN(len, buf, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); int ret; ret = nand_fill_column_cycles(chip, addrs, offset_in_page); @@ -1298,7 +1300,7 @@ static int nand_exec_prog_page_op(struct nand_chip *chip, unsigned int page, NAND_OP_CMD(NAND_CMD_PAGEPROG, PSEC_TO_NSEC(sdr->tWB_max)), NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tPROG_max), 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); int naddrs = nand_fill_column_cycles(chip, addrs, offset_in_page); int ret; u8 status; @@ -1412,7 +1414,7 @@ int nand_prog_page_end_op(struct nand_chip *chip) PSEC_TO_NSEC(sdr->tWB_max)), NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tPROG_max), 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); ret = nand_exec_op(chip, &op); if (ret) @@ -1520,7 +1522,7 @@ int nand_change_write_column_op(struct nand_chip *chip, NAND_OP_ADDR(2, addrs, PSEC_TO_NSEC(sdr->tCCS_min)), NAND_OP_DATA_OUT(len, buf, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); int ret; ret = nand_fill_column_cycles(chip, addrs, offset_in_page); @@ -1574,7 +1576,7 @@ int nand_readid_op(struct nand_chip *chip, u8 addr, void *buf, NAND_OP_ADDR(1, &addr, PSEC_TO_NSEC(sdr->tADL_min)), NAND_OP_8BIT_DATA_IN(len, buf, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); /* Drop the DATA_IN instruction if len is set to 0. */ if (!len) @@ -1613,7 +1615,7 @@ int nand_status_op(struct nand_chip *chip, u8 *status) PSEC_TO_NSEC(sdr->tADL_min)), NAND_OP_8BIT_DATA_IN(1, status, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); if (!status) op.ninstrs--; @@ -1646,7 +1648,7 @@ int nand_exit_status_op(struct nand_chip *chip) struct nand_op_instr instrs[] = { NAND_OP_CMD(NAND_CMD_READ0, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); return nand_exec_op(chip, &op); } @@ -1685,7 +1687,7 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock) PSEC_TO_MSEC(sdr->tWB_max)), NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tBERS_max), 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); if (chip->options & NAND_ROW_ADDR_3) instrs[1].ctx.addr.naddrs++; @@ -1743,7 +1745,7 @@ static int nand_set_features_op(struct nand_chip *chip, u8 feature, PSEC_TO_NSEC(sdr->tWB_max)), NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tFEAT_max), 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); return nand_exec_op(chip, &op); } @@ -1791,7 +1793,7 @@ static int nand_get_features_op(struct nand_chip *chip, u8 feature, NAND_OP_8BIT_DATA_IN(ONFI_SUBFEATURE_PARAM_LEN, data, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); return nand_exec_op(chip, &op); } @@ -1811,7 +1813,7 @@ static int nand_wait_rdy_op(struct nand_chip *chip, unsigned int timeout_ms, NAND_OP_WAIT_RDY(PSEC_TO_MSEC(timeout_ms), PSEC_TO_NSEC(delay_ns)), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); return nand_exec_op(chip, &op); } @@ -1844,7 +1846,7 @@ int nand_reset_op(struct nand_chip *chip) NAND_OP_CMD(NAND_CMD_RESET, PSEC_TO_NSEC(sdr->tWB_max)), NAND_OP_WAIT_RDY(PSEC_TO_MSEC(sdr->tRST_max), 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); return nand_exec_op(chip, &op); } @@ -1878,7 +1880,7 @@ int nand_read_data_op(struct nand_chip *chip, void *buf, unsigned int len, struct nand_op_instr instrs[] = { NAND_OP_DATA_IN(len, buf, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); instrs[0].ctx.data.force_8bit = force_8bit; @@ -1922,7 +1924,7 @@ int nand_write_data_op(struct nand_chip *chip, const void *buf, struct nand_op_instr instrs[] = { NAND_OP_DATA_OUT(len, buf, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); instrs[0].ctx.data.force_8bit = force_8bit; @@ -5006,6 +5008,9 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, unsigned int i; int ret; + /* Assume all dies are deselected when we enter nand_scan_ident(). */ + chip->cur_cs = -1; + /* Enforce the right timings for reset/detection */ onfi_fill_data_interface(chip, NAND_SDR_IFACE, 0); diff --git a/drivers/mtd/nand/raw/nand_hynix.c b/drivers/mtd/nand/raw/nand_hynix.c index ac1b5c103968..1e4499d01e14 100644 --- a/drivers/mtd/nand/raw/nand_hynix.c +++ b/drivers/mtd/nand/raw/nand_hynix.c @@ -84,7 +84,7 @@ static int hynix_nand_cmd_op(struct nand_chip *chip, u8 cmd) struct nand_op_instr instrs[] = { NAND_OP_CMD(cmd, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); return nand_exec_op(chip, &op); } @@ -103,7 +103,7 @@ static int hynix_nand_reg_write_op(struct nand_chip *chip, u8 addr, u8 val) NAND_OP_ADDR(1, &addr, 0), NAND_OP_8BIT_DATA_OUT(1, &val, 0), }; - struct nand_operation op = NAND_OPERATION(instrs); + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); return nand_exec_op(chip, &op); } diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h index def6dff11e8b..aa1512df38a9 100644 --- a/include/linux/mtd/rawnand.h +++ b/include/linux/mtd/rawnand.h @@ -875,18 +875,21 @@ struct nand_op_parser { /** * struct nand_operation - NAND operation descriptor + * @cs: the CS line to select for this NAND operation * @instrs: array of instructions to execute * @ninstrs: length of the @instrs array * * The actual operation structure that will be passed to chip->exec_op(). */ struct nand_operation { + unsigned int cs; const struct nand_op_instr *instrs; unsigned int ninstrs; }; -#define NAND_OPERATION(_instrs) \ +#define NAND_OPERATION(_cs, _instrs) \ { \ + .cs = _cs, \ .instrs = _instrs, \ .ninstrs = ARRAY_SIZE(_instrs), \ } @@ -1008,6 +1011,10 @@ struct nand_legacy { * this nand device will encounter their life times. * @blocks_per_die: [INTERN] The number of PEBs in a die * @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. * @read_retries: [INTERN] the number of read retry modes supported * @setup_data_interface: [OPTIONAL] setup the data interface and timing. If * chipnr is set to %NAND_DATA_IFACE_CHECK_ONLY this @@ -1069,6 +1076,8 @@ struct nand_chip { struct nand_data_interface data_interface; + int cur_cs; + int read_retries; flstate_t state;
In order to deprecate the ->select_chip hook we need to pass the CS line a NAND operations are targeting. This is done through the addition of a cs field to the nand_operation struct. We also need to keep track of the currently selected target to properly initialize op->cs, hence the ->cur_cs field addition to the nand_chip struct. Note that op->cs is not assigned in nand_exec_op() because we might rework the way we execute NAND operations in the future (adopt a queuing mechanism instead of the serialization we have right now). Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- Changes in v2: - Initialize ->cur_cs --- drivers/mtd/nand/raw/internals.h | 3 +++ drivers/mtd/nand/raw/nand_base.c | 39 +++++++++++++++++-------------- drivers/mtd/nand/raw/nand_hynix.c | 4 ++-- include/linux/mtd/rawnand.h | 11 ++++++++- 4 files changed, 37 insertions(+), 20 deletions(-)