From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.bootlin.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1erXW5-0002yC-48 for linux-mtd@lists.infradead.org; Thu, 01 Mar 2018 23:21:40 +0000 Date: Fri, 2 Mar 2018 00:21:28 +0100 From: Miquel Raynal To: Boris Brezillon Cc: Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , Cyrille Pitchen , linux-mtd@lists.infradead.org, Miquel Raynal Subject: Re: [PATCH 5/6] mtd: nand: Allow vendors to declare (un)supported features Message-ID: <20180302002128.2cc72fef@xps13> In-Reply-To: <20180214095137.6b9cd957@bbrezillon> References: <20180203095544.9855-1-miquel.raynal@bootlin.com> <20180203095544.9855-6-miquel.raynal@bootlin.com> <20180214095137.6b9cd957@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Boris, On Wed, 14 Feb 2018 09:51:37 +0100, Boris Brezillon wrote: > On Sat, 3 Feb 2018 10:55:43 +0100 > Miquel Raynal wrote: > > > From: Miquel Raynal > > > > 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 > > --- > > 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 > > #include > > #include > > +#include > > > > 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