From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1bhH3I-0001Vc-QT for linux-mtd@lists.infradead.org; Tue, 06 Sep 2016 14:08:45 +0000 Date: Tue, 6 Sep 2016 16:08:17 +0200 From: Sascha Hauer To: Boris Brezillon Cc: linux-mtd@lists.infradead.org, kernel@pengutronix.de, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 4/7] mtd: nand: automate NAND timings selection Message-ID: <20160906140817.dil5noauumhfbtd3@pengutronix.de> References: <1473158355-22451-1-git-send-email-s.hauer@pengutronix.de> <1473158355-22451-5-git-send-email-s.hauer@pengutronix.de> <20160906135807.29257d00@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160906135807.29257d00@bbrezillon> List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tue, Sep 06, 2016 at 01:58:07PM +0200, Boris Brezillon wrote: > On Tue, 6 Sep 2016 12:39:12 +0200 > Sascha Hauer wrote: > > > From: Boris Brezillon > > > > The NAND framework provides several helpers to query timing modes supported > > by a NAND chip, but this implies that all NAND controller drivers have > > to implement the same timings selection dance. Also currently NAND > > devices can be resetted at arbitrary places which also resets the timing > > for ONFI chips to timing mode 0. > > > > Provide a common logic to select the best timings based on ONFI or > > ->onfi_timing_mode_default information. Hook this into nand_reset() > > to make sure the new timing is applied each time during a reset. > > > > NAND controller willing to support timings adjustment should just > > implement the ->setup_data_interface() method. > > > > Signed-off-by: Boris Brezillon > > Signed-off-by: Sascha Hauer > > --- > > drivers/mtd/nand/nand_base.c | 112 +++++++++++++++++++++++++++++++++++++++++++ > > include/linux/mtd/nand.h | 14 ++++-- > > 2 files changed, 122 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index 20151fc..37852e9 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -955,8 +955,63 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip) > > */ > > int nand_reset(struct mtd_info *mtd) > > { > > + struct nand_chip *chip = mtd_to_nand(mtd); > > + int ret; > > + > > + if (chip->setup_data_interface) { > > + struct nand_data_interface conf = { > > + .type = NAND_SDR_IFACE, > > + .timings.sdr = *onfi_async_timing_mode_to_sdr_timings(0), > > + }; > > + > > Let's try to avoid putting such a huge structure on the stack. I could do that by doing const struct nand_data_interface *conf = onfi_async_timing_mode_to_data_interface(0); But you just discussed this function away ;) I need two different timings, first the ONFI timing mode 0 and then the real timing. I wanted to avoid re-initializing the same timing struct instance multiple times (twice in nand_reset() and two times again for each additional chip in an array). > > > + /* > > + * The ONFI specification says: > > + * " > > + * To transition from NV-DDR or NV-DDR2 to the SDR data > > + * interface, the host shall use the Reset (FFh) command > > + * using SDR timing mode 0. A device in any timing mode is > > + * required to recognize Reset (FFh) command issued in SDR > > + * timing mode 0. > > + * " > > + * > > + * Configure the data interface in SDR mode and set the > > + * timings to timing mode 0. > > + */ > > + > > + ret = chip->setup_data_interface(mtd, &conf, false); > > + if (ret) { > > + pr_err("Failed to configure data interface to SDR timing mode 0\n"); > > + return ret; > > + } > > + } > > Can you put this code in a separate function? I'd like to keep the > nand_reset() function as small as possible. > > How about nand_reset_data_interface()? Yes, can do. In that case I would move the test if setting the data interface is supported to that function aswell. Are you okay with that? > > > + > > chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1); > > > > + /* > > + * Setup the NAND interface (interface type + timings). > > + */ > > + if (chip->data_iface) { > > + uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { > > + chip->onfi_timing_mode_default, > > + }; > > + > > + /* > > + * Ensure the timing mode has be changed on the chip side > ^ been > > + * before changing timings on the controller side. > > + */ > > + if (chip->onfi_version) { > > + ret = chip->onfi_set_features(mtd, chip, > > + ONFI_FEATURE_ADDR_TIMING_MODE, > > + tmode_param); > > + if (ret) > > + return ret; > > + } > > + > > + ret = chip->setup_data_interface(mtd, chip->data_iface, false); > > + if (ret) > > + return ret; > > + } > > + > > Ditto: nand_setup_data_interface()? > > > return 0; > > } > > > > @@ -3335,6 +3390,54 @@ static void nand_onfi_detect_micron(struct nand_chip *chip, > > chip->setup_read_retry = nand_setup_read_retry_micron; > > } > > > > +/** > > + * nand_find_data_interface - Find the best data interface and timings > > + * @mtd: MTD device structure > > + * > > + * Try to find the best data interface and NAND timings supported by the > > + * chip and the driver. > > + * First tries to retrieve supported timing modes from ONFI information, > > + * and if the NAND chip does not support ONFI, relies on the > > + * ->onfi_timing_mode_default specified in the nand_ids table. > > + * > > + * Returns 0 for success or negative error code otherwise. > > + */ > > +static int nand_find_data_interface(struct mtd_info *mtd) > > How about nand_init_data_interface() or nand_init_data_iface_config()? > > > +{ > > + struct nand_chip *chip = mtd_to_nand(mtd); > > + int modes, mode, ret; > > + const struct nand_data_interface *conf; > > + > > + /* > > + * First try to identify the best timings from ONFI parameters and > > + * if the NAND does not support ONFI, fallback to the default ONFI > > + * timing mode. > > + */ > > + modes = onfi_get_async_timing_mode(chip); > > + if (modes == ONFI_TIMING_MODE_UNKNOWN) > > + modes = GENMASK(chip->onfi_timing_mode_default, 0); > > + > > + ret = -EINVAL; > > + for (mode = fls(modes) - 1; mode >= 0; mode--) { > > + conf = onfi_async_timing_mode_to_data_interface(mode); > > I'd still prefer to have conf allocated at the beginning of the > function and timings copied from > onfi_async_timing_mode_to_sdr_timings(mode), but maybe you can convince > me otherwise. Let me ask the other way round: If we need struct nand_data_interface to fully describe a timing, why don't we keep an array of these in the kernel? Having an array of struct nand_sdr_timings() means we always have to copy it to a bigger struct to make it usable. > > @@ -759,6 +759,10 @@ struct nand_chip { > > int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip, > > int feature_addr, uint8_t *subfeature_para); > > int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode); > > + int (*setup_data_interface)(struct mtd_info *mtd, > > + const struct nand_data_interface *conf, > > + bool check_only); > > + > > > > int chip_delay; > > unsigned int options; > > @@ -788,6 +792,8 @@ struct nand_chip { > > struct nand_jedec_params jedec_params; > > }; > > > > + const struct nand_data_interface *data_iface; > > + > > How about making this field non-const so that you only allocate it once > and modify it when you switch from one mode to another. As said above, I need two different timings. If we modify this nand_data_interface instance twice during reset there's not much point in storing it in struct nand_chip at all. That was one variant I tried: Always calculcate the timing from the supported ONFI modes when we need it in nand_reset(). I stepped away from this variant because of the overhead. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | From mboxrd@z Thu Jan 1 00:00:00 1970 From: s.hauer@pengutronix.de (Sascha Hauer) Date: Tue, 6 Sep 2016 16:08:17 +0200 Subject: [PATCH 4/7] mtd: nand: automate NAND timings selection In-Reply-To: <20160906135807.29257d00@bbrezillon> References: <1473158355-22451-1-git-send-email-s.hauer@pengutronix.de> <1473158355-22451-5-git-send-email-s.hauer@pengutronix.de> <20160906135807.29257d00@bbrezillon> Message-ID: <20160906140817.dil5noauumhfbtd3@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Sep 06, 2016 at 01:58:07PM +0200, Boris Brezillon wrote: > On Tue, 6 Sep 2016 12:39:12 +0200 > Sascha Hauer wrote: > > > From: Boris Brezillon > > > > The NAND framework provides several helpers to query timing modes supported > > by a NAND chip, but this implies that all NAND controller drivers have > > to implement the same timings selection dance. Also currently NAND > > devices can be resetted at arbitrary places which also resets the timing > > for ONFI chips to timing mode 0. > > > > Provide a common logic to select the best timings based on ONFI or > > ->onfi_timing_mode_default information. Hook this into nand_reset() > > to make sure the new timing is applied each time during a reset. > > > > NAND controller willing to support timings adjustment should just > > implement the ->setup_data_interface() method. > > > > Signed-off-by: Boris Brezillon > > Signed-off-by: Sascha Hauer > > --- > > drivers/mtd/nand/nand_base.c | 112 +++++++++++++++++++++++++++++++++++++++++++ > > include/linux/mtd/nand.h | 14 ++++-- > > 2 files changed, 122 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index 20151fc..37852e9 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -955,8 +955,63 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip) > > */ > > int nand_reset(struct mtd_info *mtd) > > { > > + struct nand_chip *chip = mtd_to_nand(mtd); > > + int ret; > > + > > + if (chip->setup_data_interface) { > > + struct nand_data_interface conf = { > > + .type = NAND_SDR_IFACE, > > + .timings.sdr = *onfi_async_timing_mode_to_sdr_timings(0), > > + }; > > + > > Let's try to avoid putting such a huge structure on the stack. I could do that by doing const struct nand_data_interface *conf = onfi_async_timing_mode_to_data_interface(0); But you just discussed this function away ;) I need two different timings, first the ONFI timing mode 0 and then the real timing. I wanted to avoid re-initializing the same timing struct instance multiple times (twice in nand_reset() and two times again for each additional chip in an array). > > > + /* > > + * The ONFI specification says: > > + * " > > + * To transition from NV-DDR or NV-DDR2 to the SDR data > > + * interface, the host shall use the Reset (FFh) command > > + * using SDR timing mode 0. A device in any timing mode is > > + * required to recognize Reset (FFh) command issued in SDR > > + * timing mode 0. > > + * " > > + * > > + * Configure the data interface in SDR mode and set the > > + * timings to timing mode 0. > > + */ > > + > > + ret = chip->setup_data_interface(mtd, &conf, false); > > + if (ret) { > > + pr_err("Failed to configure data interface to SDR timing mode 0\n"); > > + return ret; > > + } > > + } > > Can you put this code in a separate function? I'd like to keep the > nand_reset() function as small as possible. > > How about nand_reset_data_interface()? Yes, can do. In that case I would move the test if setting the data interface is supported to that function aswell. Are you okay with that? > > > + > > chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1); > > > > + /* > > + * Setup the NAND interface (interface type + timings). > > + */ > > + if (chip->data_iface) { > > + uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { > > + chip->onfi_timing_mode_default, > > + }; > > + > > + /* > > + * Ensure the timing mode has be changed on the chip side > ^ been > > + * before changing timings on the controller side. > > + */ > > + if (chip->onfi_version) { > > + ret = chip->onfi_set_features(mtd, chip, > > + ONFI_FEATURE_ADDR_TIMING_MODE, > > + tmode_param); > > + if (ret) > > + return ret; > > + } > > + > > + ret = chip->setup_data_interface(mtd, chip->data_iface, false); > > + if (ret) > > + return ret; > > + } > > + > > Ditto: nand_setup_data_interface()? > > > return 0; > > } > > > > @@ -3335,6 +3390,54 @@ static void nand_onfi_detect_micron(struct nand_chip *chip, > > chip->setup_read_retry = nand_setup_read_retry_micron; > > } > > > > +/** > > + * nand_find_data_interface - Find the best data interface and timings > > + * @mtd: MTD device structure > > + * > > + * Try to find the best data interface and NAND timings supported by the > > + * chip and the driver. > > + * First tries to retrieve supported timing modes from ONFI information, > > + * and if the NAND chip does not support ONFI, relies on the > > + * ->onfi_timing_mode_default specified in the nand_ids table. > > + * > > + * Returns 0 for success or negative error code otherwise. > > + */ > > +static int nand_find_data_interface(struct mtd_info *mtd) > > How about nand_init_data_interface() or nand_init_data_iface_config()? > > > +{ > > + struct nand_chip *chip = mtd_to_nand(mtd); > > + int modes, mode, ret; > > + const struct nand_data_interface *conf; > > + > > + /* > > + * First try to identify the best timings from ONFI parameters and > > + * if the NAND does not support ONFI, fallback to the default ONFI > > + * timing mode. > > + */ > > + modes = onfi_get_async_timing_mode(chip); > > + if (modes == ONFI_TIMING_MODE_UNKNOWN) > > + modes = GENMASK(chip->onfi_timing_mode_default, 0); > > + > > + ret = -EINVAL; > > + for (mode = fls(modes) - 1; mode >= 0; mode--) { > > + conf = onfi_async_timing_mode_to_data_interface(mode); > > I'd still prefer to have conf allocated at the beginning of the > function and timings copied from > onfi_async_timing_mode_to_sdr_timings(mode), but maybe you can convince > me otherwise. Let me ask the other way round: If we need struct nand_data_interface to fully describe a timing, why don't we keep an array of these in the kernel? Having an array of struct nand_sdr_timings() means we always have to copy it to a bigger struct to make it usable. > > @@ -759,6 +759,10 @@ struct nand_chip { > > int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip, > > int feature_addr, uint8_t *subfeature_para); > > int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode); > > + int (*setup_data_interface)(struct mtd_info *mtd, > > + const struct nand_data_interface *conf, > > + bool check_only); > > + > > > > int chip_delay; > > unsigned int options; > > @@ -788,6 +792,8 @@ struct nand_chip { > > struct nand_jedec_params jedec_params; > > }; > > > > + const struct nand_data_interface *data_iface; > > + > > How about making this field non-const so that you only allocate it once > and modify it when you switch from one mode to another. As said above, I need two different timings. If we modify this nand_data_interface instance twice during reset there's not much point in storing it in struct nand_chip at all. That was one variant I tried: Always calculcate the timing from the supported ONFI modes when we need it in nand_reset(). I stepped away from this variant because of the overhead. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |