diff mbox series

[v6,30/30] mtd: rawnand: Allocate the best interface configuration dynamically

Message ID 20200528231612.8958-31-miquel.raynal@bootlin.com
State Changes Requested
Delegated to: Miquel Raynal
Headers show
Series Allow vendor drivers to propose their own timings | expand

Commit Message

Miquel Raynal May 28, 2020, 11:16 p.m. UTC
Instead of manipulating the statically allocated structure and copy
timings around, allocate one at identification time and save it in the
nand_chip structure once it has been initialized.

Use a second pointer to always have a reference on the currently
applied interface configuration.

This way, either there is a "best interface configuration" and the
requested timings will be these, or there is none and the default set
will be used, statically defined in the core, shared across all NAND
chips.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 56 +++++++++++++++++++++++---------
 include/linux/mtd/rawnand.h      | 11 +++++--
 2 files changed, 48 insertions(+), 19 deletions(-)

Comments

Boris Brezillon May 29, 2020, 8:54 a.m. UTC | #1
On Fri, 29 May 2020 01:16:12 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

>  
> @@ -947,7 +949,8 @@ static int nand_reset_interface(struct nand_chip *chip, int chipnr)
>   */
>  static int nand_setup_interface(struct nand_chip *chip, int chipnr)
>  {
> -	u8 mode = chip->interface_config.timings.mode;
> +	const struct nand_controller_ops *ops = chip->controller->ops;
> +	u8 mode = chip->best_interface_config->timings.mode;

I didn't check, but I assume it's safe to dereference
->best_interface_config here (IOW, ->best_interface_config is
guaranteed to be != NULL).

>  	u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode, };
>  	int ret;
>  

>  
>  /**
> @@ -2523,8 +2542,8 @@ int nand_reset(struct nand_chip *chip, int chipnr)
>  	 * nand_setup_interface() uses ->set/get_features() which would
>  	 * fail anyway as the parameter page is not available yet.
>  	 */
> -	if (!memcmp(&chip->interface_config, nand_get_reset_interface(),
> -		    sizeof(chip->interface_config)))
> +	if (!chip->best_interface_config ||
> +	    chip->best_interface_config == nand_get_reset_interface())

I think 'chip->best_interface_config == nand_get_reset_interface()' is
never true.
Miquel Raynal May 29, 2020, 9:16 a.m. UTC | #2
Boris Brezillon <boris.brezillon@collabora.com> wrote on Fri, 29 May
2020 10:54:31 +0200:

> On Fri, 29 May 2020 01:16:12 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> >  
> > @@ -947,7 +949,8 @@ static int nand_reset_interface(struct nand_chip *chip, int chipnr)
> >   */
> >  static int nand_setup_interface(struct nand_chip *chip, int chipnr)
> >  {
> > -	u8 mode = chip->interface_config.timings.mode;
> > +	const struct nand_controller_ops *ops = chip->controller->ops;
> > +	u8 mode = chip->best_interface_config->timings.mode;  
> 
> I didn't check, but I assume it's safe to dereference
> ->best_interface_config here (IOW, ->best_interface_config is  
> guaranteed to be != NULL).

Yes it's safe. ->best_interface_config is always set, even with mode 0
configuration.

> 
> >  	u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode, };
> >  	int ret;
> >    
> 
> >  
> >  /**
> > @@ -2523,8 +2542,8 @@ int nand_reset(struct nand_chip *chip, int chipnr)
> >  	 * nand_setup_interface() uses ->set/get_features() which would
> >  	 * fail anyway as the parameter page is not available yet.
> >  	 */
> > -	if (!memcmp(&chip->interface_config, nand_get_reset_interface(),
> > -		    sizeof(chip->interface_config)))
> > +	if (!chip->best_interface_config ||
> > +	    chip->best_interface_config == nand_get_reset_interface())  
> 
> I think 'chip->best_interface_config == nand_get_reset_interface()' is
> never true.

Oh right. I'll drop this, and we'll accept to sometimes call
nand_setup_interface() to apply timings mode 0 while they are already
applied. I don't think it's a big deal.
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 2f913ce8328f..13fee17cd411 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -928,7 +928,9 @@  static int nand_reset_interface(struct nand_chip *chip, int chipnr)
 	 * timings to timing mode 0.
 	 */
 
-	ret = ops->setup_interface(chip, chipnr, nand_get_reset_interface());
+	chip->current_interface_config = nand_get_reset_interface();
+	ret = ops->setup_interface(chip, chipnr,
+				   chip->current_interface_config);
 	if (ret)
 		pr_err("Failed to configure data interface to SDR timing mode 0\n");
 
@@ -947,7 +949,8 @@  static int nand_reset_interface(struct nand_chip *chip, int chipnr)
  */
 static int nand_setup_interface(struct nand_chip *chip, int chipnr)
 {
-	u8 mode = chip->interface_config.timings.mode;
+	const struct nand_controller_ops *ops = chip->controller->ops;
+	u8 mode = chip->best_interface_config->timings.mode;
 	u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode, };
 	int ret;
 
@@ -965,14 +968,13 @@  static int nand_setup_interface(struct nand_chip *chip, int chipnr)
 	}
 
 	/* Change the mode on the controller side */
-	ret = chip->controller->ops->setup_interface(chip, chipnr,
-						     &chip->interface_config);
+	ret = ops->setup_interface(chip, chipnr, chip->best_interface_config);
 	if (ret)
 		return ret;
 
 	/* Check the mode has been accepted by the chip, if supported */
 	if (!nand_supports_get_features(chip, ONFI_FEATURE_ADDR_TIMING_MODE))
-		return 0;
+		goto update_interface_config;
 
 	memset(tmode_param, 0, ONFI_SUBFEATURE_PARAM_LEN);
 	nand_select_target(chip, chipnr);
@@ -988,6 +990,9 @@  static int nand_setup_interface(struct nand_chip *chip, int chipnr)
 		goto err_reset_chip;
 	}
 
+update_interface_config:
+	chip->current_interface_config = chip->best_interface_config;
+
 	return 0;
 
 err_reset_chip:
@@ -1029,8 +1034,10 @@  int nand_choose_best_sdr_timings(struct nand_chip *chip,
 		/* Verify the controller supports the requested interface */
 		ret = ops->setup_interface(chip, NAND_DATA_IFACE_CHECK_ONLY,
 					   iface);
-		if (!ret)
+		if (!ret) {
+			chip->best_interface_config = iface;
 			return ret;
+		}
 
 		/* Fallback to slower modes */
 		best_mode = iface->timings.mode;
@@ -1044,9 +1051,11 @@  int nand_choose_best_sdr_timings(struct nand_chip *chip,
 		ret = ops->setup_interface(chip, NAND_DATA_IFACE_CHECK_ONLY,
 					   iface);
 		if (!ret)
-			return 0;
+			break;
 	}
 
+	chip->best_interface_config = iface;
+
 	return 0;
 }
 
@@ -1065,15 +1074,25 @@  int nand_choose_best_sdr_timings(struct nand_chip *chip,
  */
 static int nand_choose_interface(struct nand_chip *chip)
 {
+	struct nand_interface_config *iface;
+	int ret;
+
 	if (!nand_controller_can_setup_interface(chip))
 		return 0;
 
+	iface = kzalloc(sizeof(*iface), GFP_KERNEL);
+	if (!iface)
+		return -ENOMEM;
+
 	if (chip->ops.choose_interface)
-		return chip->ops.choose_interface(chip,
-						  &chip->interface_config);
+		ret = chip->ops.choose_interface(chip, iface);
+	else
+		ret = nand_choose_best_sdr_timings(chip, iface, NULL);
 
-	return nand_choose_best_sdr_timings(chip, &chip->interface_config,
-					    NULL);
+	if (ret)
+		kfree(iface);
+
+	return ret;
 }
 
 /**
@@ -2523,8 +2542,8 @@  int nand_reset(struct nand_chip *chip, int chipnr)
 	 * nand_setup_interface() uses ->set/get_features() which would
 	 * fail anyway as the parameter page is not available yet.
 	 */
-	if (!memcmp(&chip->interface_config, nand_get_reset_interface(),
-		    sizeof(chip->interface_config)))
+	if (!chip->best_interface_config ||
+	    chip->best_interface_config == nand_get_reset_interface())
 		return 0;
 
 	ret = nand_setup_interface(chip, chipnr);
@@ -5194,7 +5213,7 @@  static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips,
 	mutex_init(&chip->lock);
 
 	/* Enforce the right timings for reset/detection */
-	onfi_fill_interface_config(chip, &chip->interface_config, NAND_SDR_IFACE, 0);
+	chip->current_interface_config = nand_get_reset_interface();
 
 	ret = nand_dt_init(chip);
 	if (ret)
@@ -5990,7 +6009,7 @@  static int nand_scan_tail(struct nand_chip *chip)
 	for (i = 0; i < nanddev_ntargets(&chip->base); i++) {
 		ret = nand_setup_interface(chip, i);
 		if (ret)
-			goto err_nanddev_cleanup;
+			goto err_free_interface_config;
 	}
 
 	/* Check, if we should skip the bad block table scan */
@@ -6000,10 +6019,12 @@  static int nand_scan_tail(struct nand_chip *chip)
 	/* Build bad block table */
 	ret = nand_create_bbt(chip);
 	if (ret)
-		goto err_nanddev_cleanup;
+		goto err_free_interface_config;
 
 	return 0;
 
+err_free_interface_config:
+	kfree(chip->best_interface_config);
 
 err_nanddev_cleanup:
 	nanddev_cleanup(&chip->base);
@@ -6097,6 +6118,9 @@  void nand_cleanup(struct nand_chip *chip)
 			& NAND_BBT_DYNAMICSTRUCT)
 		kfree(chip->badblock_pattern);
 
+	/* Free the data interface */
+	kfree(chip->best_interface_config);
+
 	/* Free manufacturer priv data. */
 	nand_manufacturer_cleanup(chip);
 
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 7b7db2d91a9a..50ea4b825324 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1069,7 +1069,11 @@  struct nand_manufacturer {
  * @options: Various chip options. They can partly be set to inform nand_scan
  *           about special functionality. See the defines for further
  *           explanation.
- * @interface_config: NAND interface timing information
+ * @current_interface_config: The currently used NAND interface configuration
+ * @best_interface_config: The best NAND interface configuration which fits both
+ *                         the NAND chip and NAND controller constraints. If
+ *                         unset, the default reset interface configuration must
+ *                         be used.
  * @bbt_erase_shift: Number of address bits in a bbt entry
  * @bbt_options: Bad block table specific options. All options used here must
  *               come from bbm.h. By default, these options will be copied to
@@ -1116,7 +1120,8 @@  struct nand_chip {
 	unsigned int options;
 
 	/* Data interface */
-	struct nand_interface_config interface_config;
+	const struct nand_interface_config *current_interface_config;
+	struct nand_interface_config *best_interface_config;
 
 	/* Bad block information */
 	unsigned int bbt_erase_shift;
@@ -1212,7 +1217,7 @@  const struct nand_interface_config *nand_get_reset_interface(void);
 static inline const struct nand_interface_config *
 nand_get_interface_config(struct nand_chip *chip)
 {
-	return &chip->interface_config;
+	return chip->current_interface_config;
 }
 
 /*