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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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;  
> 
> 
> 



-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com