[5/6] mtd: nand: Allow vendors to declare (un)supported features

Message ID 20180203095544.9855-6-miquel.raynal@bootlin.com
State Changes Requested
Delegated to: Boris Brezillon
Headers show
Series
  • Improve timings handling in the NAND framework
Related show

Commit Message

Miquel Raynal Feb. 3, 2018, 9:55 a.m.
From: Miquel Raynal <miquel.raynal@free-electrons.com>

If SET/GET_FEATURES is available (from the parameter page), use a
bitmap to declare what feature is actually supported.

Initialize the bitmap in the core to support timing changes (only
feature used by the core), also add support for Micron specific features
used in Micron initialization code (in the init routine).

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c   | 11 ++++++++---
 drivers/mtd/nand/nand_micron.c |  4 ++++
 include/linux/mtd/rawnand.h    |  5 ++++-
 3 files changed, 16 insertions(+), 4 deletions(-)

Comments

Boris Brezillon Feb. 14, 2018, 8:51 a.m. | #1
On Sat,  3 Feb 2018 10:55:43 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> From: Miquel Raynal <miquel.raynal@free-electrons.com>
> 
> If SET/GET_FEATURES is available (from the parameter page), use a
> bitmap to declare what feature is actually supported.
> 
> Initialize the bitmap in the core to support timing changes (only
> feature used by the core), also add support for Micron specific features
> used in Micron initialization code (in the init routine).
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c   | 11 ++++++++---
>  drivers/mtd/nand/nand_micron.c |  4 ++++
>  include/linux/mtd/rawnand.h    |  5 ++++-
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 4a879b1635b3..859d9ba2678f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1174,7 +1174,8 @@ int nand_get_features(struct nand_chip *chip, int addr,
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  
> -	if (!chip->parameters.support_setting_features)
> +	if (!chip->parameters.support_setting_features ||
> +	    !test_bit(addr, chip->parameters.feature_list))
>  		return -ENOTSUPP;
>  
>  	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
> @@ -1195,7 +1196,8 @@ int nand_set_features(struct nand_chip *chip, int addr,
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  
> -	if (!chip->parameters.support_setting_features)
> +	if (!chip->parameters.support_setting_features ||
> +	    !test_bit(addr, chip->parameters.feature_list))
>  		return -ENOTSUPP;
>  
>  	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
> @@ -5304,8 +5306,11 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  	}
>  
>  	/* Save some parameters from the parameter page for future use */
> -	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
> +	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES) {
>  		chip->parameters.support_setting_features = true;
> +		bitmap_set(chip->parameters.feature_list,
> +			   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
> +	}
>  	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
>  	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
>  	chip->parameters.t_r = le16_to_cpu(p->t_r);
> diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
> index eaf14885e059..3c5b884e0eae 100644
> --- a/drivers/mtd/nand/nand_micron.c
> +++ b/drivers/mtd/nand/nand_micron.c
> @@ -64,6 +64,10 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
>  		chip->setup_read_retry = micron_nand_setup_read_retry;
>  	}
>  
> +	if (p->support_setting_features) {
> +		set_bit(ONFI_FEATURE_ADDR_TIMING_MODE, p->feature_list);

No need to set it again since it's already set by default.

> +		set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->feature_list);
> +	}
>  
>  	return 0;
>  }
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 6d8667bb96f4..39a38196dbac 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -21,6 +21,7 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/flashchip.h>
>  #include <linux/mtd/bbm.h>
> +#include <linux/types.h>
>  
>  struct mtd_info;
>  struct nand_flash_dev;
> @@ -235,7 +236,8 @@ struct nand_chip;
>  #define ONFI_TIMING_MODE_5		(1 << 5)
>  #define ONFI_TIMING_MODE_UNKNOWN	(1 << 6)
>  
> -/* ONFI feature address */
> +/* ONFI feature number/address */
> +#define ONFI_FEATURE_NUMBER		256
>  #define ONFI_FEATURE_ADDR_TIMING_MODE	0x1
>  
>  /* Vendor-specific feature address (Micron) */
> @@ -434,6 +436,7 @@ struct nand_parameters {
>  	char model[20];
>  	/* ONFI parameters */
>  	bool support_setting_features;
> +	DECLARE_BITMAP(feature_list, ONFI_FEATURE_NUMBER);

This is not ONFI specific. The SET/GET_FEATURES are supported by JEDEC
and also non-JEDEC/ONFI chips.

Could we move that to the generic section, or maybe declare a
nand_features struct that you could place directly in nand_chip:

struct nand_features {
	DECLARE_BITMAP(supported, ONFI_FEATURE_NUMBER);
};

BTW, are we sure that all features can both be set and retrieved? If
that's not the case, then maybe we should have a bitmap for each
operation (set and get).

>  	u16 t_prog;
>  	u16 t_bers;
>  	u16 t_r;
Miquel Raynal March 1, 2018, 11:21 p.m. | #2
Hi Boris,

On Wed, 14 Feb 2018 09:51:37 +0100, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> On Sat,  3 Feb 2018 10:55:43 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > From: Miquel Raynal <miquel.raynal@free-electrons.com>
> > 
> > If SET/GET_FEATURES is available (from the parameter page), use a
> > bitmap to declare what feature is actually supported.
> > 
> > Initialize the bitmap in the core to support timing changes (only
> > feature used by the core), also add support for Micron specific features
> > used in Micron initialization code (in the init routine).
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c   | 11 ++++++++---
> >  drivers/mtd/nand/nand_micron.c |  4 ++++
> >  include/linux/mtd/rawnand.h    |  5 ++++-
> >  3 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 4a879b1635b3..859d9ba2678f 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -1174,7 +1174,8 @@ int nand_get_features(struct nand_chip *chip, int addr,
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  
> > -	if (!chip->parameters.support_setting_features)
> > +	if (!chip->parameters.support_setting_features ||
> > +	    !test_bit(addr, chip->parameters.feature_list))
> >  		return -ENOTSUPP;
> >  
> >  	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
> > @@ -1195,7 +1196,8 @@ int nand_set_features(struct nand_chip *chip, int addr,
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  
> > -	if (!chip->parameters.support_setting_features)
> > +	if (!chip->parameters.support_setting_features ||
> > +	    !test_bit(addr, chip->parameters.feature_list))
> >  		return -ENOTSUPP;
> >  
> >  	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
> > @@ -5304,8 +5306,11 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
> >  	}
> >  
> >  	/* Save some parameters from the parameter page for future use */
> > -	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
> > +	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES) {
> >  		chip->parameters.support_setting_features = true;
> > +		bitmap_set(chip->parameters.feature_list,
> > +			   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
> > +	}
> >  	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
> >  	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
> >  	chip->parameters.t_r = le16_to_cpu(p->t_r);
> > diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
> > index eaf14885e059..3c5b884e0eae 100644
> > --- a/drivers/mtd/nand/nand_micron.c
> > +++ b/drivers/mtd/nand/nand_micron.c
> > @@ -64,6 +64,10 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
> >  		chip->setup_read_retry = micron_nand_setup_read_retry;
> >  	}
> >  
> > +	if (p->support_setting_features) {
> > +		set_bit(ONFI_FEATURE_ADDR_TIMING_MODE, p->feature_list);  
> 
> No need to set it again since it's already set by default.

Good catch. Removed.

> 
> > +		set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->feature_list);
> > +	}
> >  
> >  	return 0;
> >  }
> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index 6d8667bb96f4..39a38196dbac 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -21,6 +21,7 @@
> >  #include <linux/mtd/mtd.h>
> >  #include <linux/mtd/flashchip.h>
> >  #include <linux/mtd/bbm.h>
> > +#include <linux/types.h>
> >  
> >  struct mtd_info;
> >  struct nand_flash_dev;
> > @@ -235,7 +236,8 @@ struct nand_chip;
> >  #define ONFI_TIMING_MODE_5		(1 << 5)
> >  #define ONFI_TIMING_MODE_UNKNOWN	(1 << 6)
> >  
> > -/* ONFI feature address */
> > +/* ONFI feature number/address */
> > +#define ONFI_FEATURE_NUMBER		256
> >  #define ONFI_FEATURE_ADDR_TIMING_MODE	0x1
> >  
> >  /* Vendor-specific feature address (Micron) */
> > @@ -434,6 +436,7 @@ struct nand_parameters {
> >  	char model[20];
> >  	/* ONFI parameters */
> >  	bool support_setting_features;
> > +	DECLARE_BITMAP(feature_list, ONFI_FEATURE_NUMBER);  
> 
> This is not ONFI specific. The SET/GET_FEATURES are supported by JEDEC
> and also non-JEDEC/ONFI chips.

Moved in the "generic" section, together with the
support_get_set_features (ex. support_setting_features).

> 
> Could we move that to the generic section, or maybe declare a
> nand_features struct that you could place directly in nand_chip:
> 
> struct nand_features {
> 	DECLARE_BITMAP(supported, ONFI_FEATURE_NUMBER);
> };
> 
> BTW, are we sure that all features can both be set and retrieved? If
> that's not the case, then maybe we should have a bitmap for each
> operation (set and get).

Actually, no. I added a second bitmap for that.

> 
> >  	u16 t_prog;
> >  	u16 t_bers;
> >  	u16 t_r;  
> 
> 
>

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4a879b1635b3..859d9ba2678f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1174,7 +1174,8 @@  int nand_get_features(struct nand_chip *chip, int addr,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
-	if (!chip->parameters.support_setting_features)
+	if (!chip->parameters.support_setting_features ||
+	    !test_bit(addr, chip->parameters.feature_list))
 		return -ENOTSUPP;
 
 	return chip->onfi_get_features(mtd, chip, addr, subfeature_param);
@@ -1195,7 +1196,8 @@  int nand_set_features(struct nand_chip *chip, int addr,
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 
-	if (!chip->parameters.support_setting_features)
+	if (!chip->parameters.support_setting_features ||
+	    !test_bit(addr, chip->parameters.feature_list))
 		return -ENOTSUPP;
 
 	return chip->onfi_set_features(mtd, chip, addr, subfeature_param);
@@ -5304,8 +5306,11 @@  static int nand_flash_detect_onfi(struct nand_chip *chip)
 	}
 
 	/* Save some parameters from the parameter page for future use */
-	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES)
+	if (le16_to_cpu(p->opt_cmd) & ONFI_OPT_CMD_SET_GET_FEATURES) {
 		chip->parameters.support_setting_features = true;
+		bitmap_set(chip->parameters.feature_list,
+			   ONFI_FEATURE_ADDR_TIMING_MODE, 1);
+	}
 	chip->parameters.t_prog = le16_to_cpu(p->t_prog);
 	chip->parameters.t_bers = le16_to_cpu(p->t_bers);
 	chip->parameters.t_r = le16_to_cpu(p->t_r);
diff --git a/drivers/mtd/nand/nand_micron.c b/drivers/mtd/nand/nand_micron.c
index eaf14885e059..3c5b884e0eae 100644
--- a/drivers/mtd/nand/nand_micron.c
+++ b/drivers/mtd/nand/nand_micron.c
@@ -64,6 +64,10 @@  static int micron_nand_onfi_init(struct nand_chip *chip)
 		chip->setup_read_retry = micron_nand_setup_read_retry;
 	}
 
+	if (p->support_setting_features) {
+		set_bit(ONFI_FEATURE_ADDR_TIMING_MODE, p->feature_list);
+		set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->feature_list);
+	}
 
 	return 0;
 }
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 6d8667bb96f4..39a38196dbac 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -21,6 +21,7 @@ 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/flashchip.h>
 #include <linux/mtd/bbm.h>
+#include <linux/types.h>
 
 struct mtd_info;
 struct nand_flash_dev;
@@ -235,7 +236,8 @@  struct nand_chip;
 #define ONFI_TIMING_MODE_5		(1 << 5)
 #define ONFI_TIMING_MODE_UNKNOWN	(1 << 6)
 
-/* ONFI feature address */
+/* ONFI feature number/address */
+#define ONFI_FEATURE_NUMBER		256
 #define ONFI_FEATURE_ADDR_TIMING_MODE	0x1
 
 /* Vendor-specific feature address (Micron) */
@@ -434,6 +436,7 @@  struct nand_parameters {
 	char model[20];
 	/* ONFI parameters */
 	bool support_setting_features;
+	DECLARE_BITMAP(feature_list, ONFI_FEATURE_NUMBER);
 	u16 t_prog;
 	u16 t_bers;
 	u16 t_r;