Message ID | 20200520133854.25241-1-rickaran@axis.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [v2,1/2] mtd: rawnand: Add a helper for testing data interface | expand |
Hi Rickard, Rickard Andersson <rickaran@axis.com> wrote on Wed, 20 May 2020 15:38:53 +0200: > From: Rickard x Andersson <rickaran@axis.com> > > This helper tests the current data interface timings. If > the controller does not accept the timings then the timings s/accept/support/ > are erased and onfi mode 0 timings are chosen at a later > stage. See below, I don't think this is needed. > > Signed-off-by: Rickard x Andersson <rickaran@axis.com> > --- > drivers/mtd/nand/raw/internals.h | 1 + > drivers/mtd/nand/raw/nand_base.c | 38 ++++++++++++++++++++++++++++---------- > 2 files changed, 29 insertions(+), 10 deletions(-) > > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h > index 615677820338..7df0a8e674cb 100644 > --- a/drivers/mtd/nand/raw/internals.h > +++ b/drivers/mtd/nand/raw/internals.h > @@ -100,6 +100,7 @@ int nand_read_param_page_op(struct nand_chip *chip, u8 page, void *buf, > void nand_decode_ext_id(struct nand_chip *chip); > void panic_nand_wait(struct nand_chip *chip, unsigned long timeo); > void sanitize_string(uint8_t *s, size_t len); > +int nand_test_data_interface(struct nand_chip *chip); > > static inline bool nand_has_exec_op(struct nand_chip *chip) > { > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > index c42cbeb7e446..29e7be3811e7 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -956,6 +956,32 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) > } > > /** > + * nand_test_data_interface - Check if controller can handle the current I'm fine adding an helper for that. It could also be used for DDR timing support. What about renaming it "nand_controller_supports_data_interface()"? > + * timings. Clear timings if not usable. > + * > + * @chip: The NAND chip > + */ > +int nand_test_data_interface(struct nand_chip *chip) > +{ > + int ret; Missing extra line > + /* > + * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the > + * controller supports the requested timings. > + */ > + ret = chip->controller->ops->setup_data_interface(chip, > + NAND_DATA_IFACE_CHECK_ONLY, > + &chip->data_interface); Could you align these lines to the opened parenthesis? > + > + if (ret) { > + /* The provided data interface timings did not work */ > + memset(&chip->data_interface, 0, > + sizeof(struct nand_data_interface)); I'm not sure this is needed. I think it will only be necessary in your case, so I think it's better to move it ouf of this function. > + } > + > + return ret; > +} Maybe it is best to move this function in internals.h directly. > + > +/** > * nand_choose_data_interface - find the best data interface and timings > * @chip: The NAND chip > * > @@ -994,9 +1020,6 @@ static int nand_choose_data_interface(struct nand_chip *chip) > if (chip->parameters.onfi) { > modes = chip->parameters.onfi->async_timing_mode; > } else { > - if (!chip->default_timing_mode) > - return 0; > - This should not be removed > modes = GENMASK(chip->default_timing_mode, 0); > } > > @@ -1005,13 +1028,8 @@ static int nand_choose_data_interface(struct nand_chip *chip) > if (ret) > continue; > > - /* > - * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the > - * controller supports the requested timings. > - */ > - ret = chip->controller->ops->setup_data_interface(chip, > - NAND_DATA_IFACE_CHECK_ONLY, > - &chip->data_interface); > + /* Check if the controller supports the requested timings. */ > + ret = nand_test_data_interface(chip); > if (!ret) { > chip->default_timing_mode = mode; > break; Thanks, Miquèl
Hi Miquel, Comments on two of your comments. (I am fine with all the other comments.) > > + /* > > + * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the > > + * controller supports the requested timings. > > + */ > > + ret = chip->controller->ops->setup_data_interface(chip, > > + NAND_DATA_IFACE_CHECK_ONLY, > > + &chip->data_interface); > > Could you align these lines to the opened parenthesis? Then the lines will have 80+ characters. > > @@ -994,9 +1020,6 @@ static int nand_choose_data_interface(struct nand_chip *chip) > > if (chip->parameters.onfi) { > > modes = chip->parameters.onfi->async_timing_mode; > > } else { > > - if (!chip->default_timing_mode) > > - return 0; > > - > > This should not be removed Then onfi_fill_data_interface would not be called for default_timing_mode 0. (In case we have called chip->ops.choose_data_interface and got an error.). BR, Rickard
On Wed, 20 May 2020 14:52:52 +0000 Rickard X Andersson <Rickard.Andersson@axis.com> wrote: > Hi Miquel, > > Comments on two of your comments. (I am fine with all the other comments.) > > > > + /* > > > + * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the > > > + * controller supports the requested timings. > > > + */ > > > + ret = chip->controller->ops->setup_data_interface(chip, > > > + NAND_DATA_IFACE_CHECK_ONLY, > > > + &chip->data_interface); > > > > Could you align these lines to the opened parenthesis? > > Then the lines will have 80+ characters. Just add a local ops var. const struct nand_controller_ops *ops = chip->controller->ops; ret = ops->setup_data_interface(chip, NAND_DATA_IFACE_CHECK_ONLY, &chip->data_interface);
diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h index 615677820338..7df0a8e674cb 100644 --- a/drivers/mtd/nand/raw/internals.h +++ b/drivers/mtd/nand/raw/internals.h @@ -100,6 +100,7 @@ int nand_read_param_page_op(struct nand_chip *chip, u8 page, void *buf, void nand_decode_ext_id(struct nand_chip *chip); void panic_nand_wait(struct nand_chip *chip, unsigned long timeo); void sanitize_string(uint8_t *s, size_t len); +int nand_test_data_interface(struct nand_chip *chip); static inline bool nand_has_exec_op(struct nand_chip *chip) { diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index c42cbeb7e446..29e7be3811e7 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -956,6 +956,32 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) } /** + * nand_test_data_interface - Check if controller can handle the current + * timings. Clear timings if not usable. + * + * @chip: The NAND chip + */ +int nand_test_data_interface(struct nand_chip *chip) +{ + int ret; + /* + * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the + * controller supports the requested timings. + */ + ret = chip->controller->ops->setup_data_interface(chip, + NAND_DATA_IFACE_CHECK_ONLY, + &chip->data_interface); + + if (ret) { + /* The provided data interface timings did not work */ + memset(&chip->data_interface, 0, + sizeof(struct nand_data_interface)); + } + + return ret; +} + +/** * nand_choose_data_interface - find the best data interface and timings * @chip: The NAND chip * @@ -994,9 +1020,6 @@ static int nand_choose_data_interface(struct nand_chip *chip) if (chip->parameters.onfi) { modes = chip->parameters.onfi->async_timing_mode; } else { - if (!chip->default_timing_mode) - return 0; - modes = GENMASK(chip->default_timing_mode, 0); } @@ -1005,13 +1028,8 @@ static int nand_choose_data_interface(struct nand_chip *chip) if (ret) continue; - /* - * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the - * controller supports the requested timings. - */ - ret = chip->controller->ops->setup_data_interface(chip, - NAND_DATA_IFACE_CHECK_ONLY, - &chip->data_interface); + /* Check if the controller supports the requested timings. */ + ret = nand_test_data_interface(chip); if (!ret) { chip->default_timing_mode = mode; break;