Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1425681 > unrolled thread

Re: [PATCH v2 05/15] mtd: nand: add manufacturer specific initialization/detection steps

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 05/15] mtd: nand: add manufacturer specific  initialization/detection steps Richard Weinberger <richard@nod.at> - 2016-06-18 11:30 +0200
    Re: [PATCH v2 05/15] mtd: nand: add manufacturer specific  initialization/detection steps Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-18 13:40 +0200
      Re: [PATCH v2 05/15] mtd: nand: add manufacturer specific  initialization/detection steps Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-18 13:50 +0200
    Re: [PATCH v2 05/15] mtd: nand: add manufacturer specific  initialization/detection steps Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-18 13:40 +0200
      Re: [PATCH v2 05/15] mtd: nand: add manufacturer specific  initialization/detection steps Richard Weinberger <richard@nod.at> - 2016-06-18 14:40 +0200

#1425681 — Re: [PATCH v2 05/15] mtd: nand: add manufacturer specific initialization/detection steps

Boris,

Am 08.06.2016 um 15:00 schrieb Boris Brezillon:
> A lot of NANDs are implementing generic features in a non-generic way,
> or are providing advanced auto-detection logic where the NAND ID bytes
> meaning changes with the NAND generation.
> 
> Providing this vendor specific initialization step will allow us to get
> rid of the full ids in the nand_ids table or all the vendor specific
> cases added over the time in the generic NAND ID decoding logic.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c | 42 ++++++++++++++++++++++++++++++++----------
>  include/linux/mtd/nand.h     | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 95e9a8e..0a7d1c6 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3580,7 +3580,7 @@ static int nand_get_bits_per_cell(u8 cellinfo)
>   * chip. The rest of the parameters must be decoded according to generic or
>   * manufacturer-specific "extended ID" decoding patterns.
>   */
> -static void nand_decode_ext_id(struct nand_chip *chip)
> +void nand_decode_ext_id(struct nand_chip *chip)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	int extid, id_len = chip->id.len;
> @@ -3705,6 +3705,7 @@ static void nand_decode_ext_id(struct nand_chip *chip)
>  
>  	}
>  }
> +EXPORT_SYMBOL_GPL(nand_decode_ext_id);
>  
>  /*
>   * Old devices have chip data hardcoded in the device ID table. nand_decode_id
> @@ -3815,7 +3816,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	int busw;
> -	int i, maf_idx;
> +	int i, maf_idx, ret;
>  	u8 *id_data = chip->id.data;
>  	u8 maf_id, dev_id;
>  
> @@ -3856,6 +3857,14 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
>  
>  	chip->id.len = nand_id_len(id_data, 8);
>  
> +	/* Try to identify manufacturer */
> +	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
> +		if (nand_manuf_ids[maf_idx].id == maf_id)
> +			break;
> +	}
> +
> +	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
> +

Instead of checking on every access whether chip->manufacturer.ops is present, just
assign a nop field.
i.e.

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 55f3ab8..aadebe7 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3703,6 +3703,10 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)

 	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;

+	if (!chip->manufacturer.ops)
+		/* assign no operations */
+		chip->manufacturer.ops = nand_manuf_ids[0].ops;
+
 	if (!type)
 		type = nand_flash_ids;

diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index f1476ff..cdd26af 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -173,6 +173,8 @@ extern const struct nand_manufacturer_ops micron_nand_manuf_ops;
 extern const struct nand_manufacturer_ops amd_nand_manuf_ops;
 extern const struct nand_manufacturer_ops macronix_nand_manuf_ops;

+static const struct nand_manufacturer_ops no_ops;
+
 struct nand_manufacturers nand_manuf_ids[] = {
 	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
 	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
@@ -188,7 +190,7 @@ struct nand_manufacturers nand_manuf_ids[] = {
 	{NAND_MFR_SANDISK, "SanDisk"},
 	{NAND_MFR_INTEL, "Intel"},
 	{NAND_MFR_ATO, "ATO"},
-	{0x0, "Unknown"}
+	{0x0, "Unknown", &no_ops}
 };

 EXPORT_SYMBOL(nand_manuf_ids);


>  	if (!type)
>  		type = nand_flash_ids;
>  
> @@ -3903,8 +3912,14 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
>  	chip->chipsize = (uint64_t)type->chipsize << 20;
>  
>  	if (!type->pagesize) {
> -		/* Decode parameters from extended ID */
> -		nand_decode_ext_id(chip);
> +		/*
> +		 * Try manufacturer detection if available and use
> +		 * nand_decode_ext_id() otherwise.
> +		 */
> +		if (chip->manufacturer.ops->detect)

You need to check here for chip->manufacturer.ops first.

Thanks,
//richard

[toc] | [next] | [standalone]

#1425701

On Sat, 18 Jun 2016 11:23:01 +0200
Richard Weinberger <richard@nod.at> wrote:

> Boris,
> 
> Am 08.06.2016 um 15:00 schrieb Boris Brezillon:
> > A lot of NANDs are implementing generic features in a non-generic way,
> > or are providing advanced auto-detection logic where the NAND ID bytes
> > meaning changes with the NAND generation.
> > 
> > Providing this vendor specific initialization step will allow us to get
> > rid of the full ids in the nand_ids table or all the vendor specific
> > cases added over the time in the generic NAND ID decoding logic.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c | 42 ++++++++++++++++++++++++++++++++----------
> >  include/linux/mtd/nand.h     | 36 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 68 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 95e9a8e..0a7d1c6 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3580,7 +3580,7 @@ static int nand_get_bits_per_cell(u8 cellinfo)
> >   * chip. The rest of the parameters must be decoded according to generic or
> >   * manufacturer-specific "extended ID" decoding patterns.
> >   */
> > -static void nand_decode_ext_id(struct nand_chip *chip)
> > +void nand_decode_ext_id(struct nand_chip *chip)
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  	int extid, id_len = chip->id.len;
> > @@ -3705,6 +3705,7 @@ static void nand_decode_ext_id(struct nand_chip *chip)
> >  
> >  	}
> >  }
> > +EXPORT_SYMBOL_GPL(nand_decode_ext_id);
> >  
> >  /*
> >   * Old devices have chip data hardcoded in the device ID table. nand_decode_id
> > @@ -3815,7 +3816,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> >  {
> >  	struct mtd_info *mtd = nand_to_mtd(chip);
> >  	int busw;
> > -	int i, maf_idx;
> > +	int i, maf_idx, ret;
> >  	u8 *id_data = chip->id.data;
> >  	u8 maf_id, dev_id;
> >  
> > @@ -3856,6 +3857,14 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> >  
> >  	chip->id.len = nand_id_len(id_data, 8);
> >  
> > +	/* Try to identify manufacturer */
> > +	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
> > +		if (nand_manuf_ids[maf_idx].id == maf_id)
> > +			break;
> > +	}
> > +
> > +	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
> > +  
> 
> Instead of checking on every access whether chip->manufacturer.ops is present, just
> assign a nop field.
> i.e.
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 55f3ab8..aadebe7 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3703,6 +3703,10 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> 
>  	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
> 
> +	if (!chip->manufacturer.ops)
> +		/* assign no operations */
> +		chip->manufacturer.ops = nand_manuf_ids[0].ops;
> +
>  	if (!type)
>  		type = nand_flash_ids;
> 
> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
> index f1476ff..cdd26af 100644
> --- a/drivers/mtd/nand/nand_ids.c
> +++ b/drivers/mtd/nand/nand_ids.c
> @@ -173,6 +173,8 @@ extern const struct nand_manufacturer_ops micron_nand_manuf_ops;
>  extern const struct nand_manufacturer_ops amd_nand_manuf_ops;
>  extern const struct nand_manufacturer_ops macronix_nand_manuf_ops;
> 
> +static const struct nand_manufacturer_ops no_ops;
> +
>  struct nand_manufacturers nand_manuf_ids[] = {
>  	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
>  	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
> @@ -188,7 +190,7 @@ struct nand_manufacturers nand_manuf_ids[] = {
>  	{NAND_MFR_SANDISK, "SanDisk"},
>  	{NAND_MFR_INTEL, "Intel"},
>  	{NAND_MFR_ATO, "ATO"},
> -	{0x0, "Unknown"}
> +	{0x0, "Unknown", &no_ops}
>  };
> 
>  EXPORT_SYMBOL(nand_manuf_ids);
> 

Sorry, but I don't see any added value in adding this no_ops instance.
The NULL value is supposed to represent no_ops.
That's not like this path was critical: it's only call once during NAND
initialization.


How about adding the following helpers instead:

static inline bool nand_has_manufacturer_ops(struct nand_chip * chip)
{
	return chip->manufacturer.ops != NULL;
}

static inline void nand_manufacturer_detect(struct nand_chip * chip)
{
	if (!chip->manufacturer.ops || chip->manufacturer.ops->detect)
		return;

	chip->manufacturer.ops->detect(chip);
}

static inline int nand_manufacturer_init(struct nand_chip * chip)
{
	if (!chip->manufacturer.ops || chip->manufacturer.ops->init)
		return 0;

	return chip->manufacturer.ops->init(chip);
}

> 
> >  	if (!type)
> >  		type = nand_flash_ids;
> >  
> > @@ -3903,8 +3912,14 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> >  	chip->chipsize = (uint64_t)type->chipsize << 20;
> >  
> >  	if (!type->pagesize) {
> > -		/* Decode parameters from extended ID */
> > -		nand_decode_ext_id(chip);
> > +		/*
> > +		 * Try manufacturer detection if available and use
> > +		 * nand_decode_ext_id() otherwise.
> > +		 */
> > +		if (chip->manufacturer.ops->detect)  
> 
> You need to check here for chip->manufacturer.ops first.

Correct.

[toc] | [prev] | [next] | [standalone]

#1425703

On Sat, 18 Jun 2016 13:31:21 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Sat, 18 Jun 2016 11:23:01 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
> > Boris,
> > 
> > Am 08.06.2016 um 15:00 schrieb Boris Brezillon:  
> > > A lot of NANDs are implementing generic features in a non-generic way,
> > > or are providing advanced auto-detection logic where the NAND ID bytes
> > > meaning changes with the NAND generation.
> > > 
> > > Providing this vendor specific initialization step will allow us to get
> > > rid of the full ids in the nand_ids table or all the vendor specific
> > > cases added over the time in the generic NAND ID decoding logic.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > >  drivers/mtd/nand/nand_base.c | 42 ++++++++++++++++++++++++++++++++----------
> > >  include/linux/mtd/nand.h     | 36 ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 68 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > index 95e9a8e..0a7d1c6 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -3580,7 +3580,7 @@ static int nand_get_bits_per_cell(u8 cellinfo)
> > >   * chip. The rest of the parameters must be decoded according to generic or
> > >   * manufacturer-specific "extended ID" decoding patterns.
> > >   */
> > > -static void nand_decode_ext_id(struct nand_chip *chip)
> > > +void nand_decode_ext_id(struct nand_chip *chip)
> > >  {
> > >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > >  	int extid, id_len = chip->id.len;
> > > @@ -3705,6 +3705,7 @@ static void nand_decode_ext_id(struct nand_chip *chip)
> > >  
> > >  	}
> > >  }
> > > +EXPORT_SYMBOL_GPL(nand_decode_ext_id);
> > >  
> > >  /*
> > >   * Old devices have chip data hardcoded in the device ID table. nand_decode_id
> > > @@ -3815,7 +3816,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> > >  {
> > >  	struct mtd_info *mtd = nand_to_mtd(chip);
> > >  	int busw;
> > > -	int i, maf_idx;
> > > +	int i, maf_idx, ret;
> > >  	u8 *id_data = chip->id.data;
> > >  	u8 maf_id, dev_id;
> > >  
> > > @@ -3856,6 +3857,14 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> > >  
> > >  	chip->id.len = nand_id_len(id_data, 8);
> > >  
> > > +	/* Try to identify manufacturer */
> > > +	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
> > > +		if (nand_manuf_ids[maf_idx].id == maf_id)
> > > +			break;
> > > +	}
> > > +
> > > +	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
> > > +    
> > 
> > Instead of checking on every access whether chip->manufacturer.ops is present, just
> > assign a nop field.
> > i.e.
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 55f3ab8..aadebe7 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3703,6 +3703,10 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> > 
> >  	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
> > 
> > +	if (!chip->manufacturer.ops)
> > +		/* assign no operations */
> > +		chip->manufacturer.ops = nand_manuf_ids[0].ops;
> > +
> >  	if (!type)
> >  		type = nand_flash_ids;
> > 
> > diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
> > index f1476ff..cdd26af 100644
> > --- a/drivers/mtd/nand/nand_ids.c
> > +++ b/drivers/mtd/nand/nand_ids.c
> > @@ -173,6 +173,8 @@ extern const struct nand_manufacturer_ops micron_nand_manuf_ops;
> >  extern const struct nand_manufacturer_ops amd_nand_manuf_ops;
> >  extern const struct nand_manufacturer_ops macronix_nand_manuf_ops;
> > 
> > +static const struct nand_manufacturer_ops no_ops;
> > +
> >  struct nand_manufacturers nand_manuf_ids[] = {
> >  	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
> >  	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
> > @@ -188,7 +190,7 @@ struct nand_manufacturers nand_manuf_ids[] = {
> >  	{NAND_MFR_SANDISK, "SanDisk"},
> >  	{NAND_MFR_INTEL, "Intel"},
> >  	{NAND_MFR_ATO, "ATO"},
> > -	{0x0, "Unknown"}
> > +	{0x0, "Unknown", &no_ops}
> >  };
> > 
> >  EXPORT_SYMBOL(nand_manuf_ids);
> >   
> 
> Sorry, but I don't see any added value in adding this no_ops instance.
> The NULL value is supposed to represent no_ops.
> That's not like this path was critical: it's only call once during NAND
> initialization.
> 
> 
> How about adding the following helpers instead:
> 
> static inline bool nand_has_manufacturer_ops(struct nand_chip * chip)
> {
> 	return chip->manufacturer.ops != NULL;
> }

I meant

static inline bool nand_has_manufacturer_detect(struct nand_chip * chip)
{
	return chip->manufacturer.ops && chip->manufacturer.ops->detect;
}

> 
> static inline void nand_manufacturer_detect(struct nand_chip * chip)
> {
> 	if (!chip->manufacturer.ops || chip->manufacturer.ops->detect)
> 		return;
> 
> 	chip->manufacturer.ops->detect(chip);
> }
> 
> static inline int nand_manufacturer_init(struct nand_chip * chip)
> {
> 	if (!chip->manufacturer.ops || chip->manufacturer.ops->init)
> 		return 0;
> 
> 	return chip->manufacturer.ops->init(chip);
> }
> 
> >   
> > >  	if (!type)
> > >  		type = nand_flash_ids;
> > >  
> > > @@ -3903,8 +3912,14 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> > >  	chip->chipsize = (uint64_t)type->chipsize << 20;
> > >  
> > >  	if (!type->pagesize) {
> > > -		/* Decode parameters from extended ID */
> > > -		nand_decode_ext_id(chip);
> > > +		/*
> > > +		 * Try manufacturer detection if available and use
> > > +		 * nand_decode_ext_id() otherwise.
> > > +		 */
> > > +		if (chip->manufacturer.ops->detect)    
> > 
> > You need to check here for chip->manufacturer.ops first.  
> 
> Correct.

[toc] | [prev] | [next] | [standalone]

#1425702

On Sat, 18 Jun 2016 11:23:01 +0200
Richard Weinberger <richard@nod.at> wrote:

> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 55f3ab8..aadebe7 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3703,6 +3703,10 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> 
>  	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
> 
> +	if (!chip->manufacturer.ops)
> +		/* assign no operations */
> +		chip->manufacturer.ops = nand_manuf_ids[0].ops;
> +

BTW, this is wrong, the manufacturer id code is not the index of the
nand_manuf_ids[] table ;). If we go for this option, we should probably
declare no_ops in nand_base.c and assign it here:

		chip->manufacturer.ops = nand_manuf_no_ops;

>  	if (!type)
>  		type = nand_flash_ids;
> 

[toc] | [prev] | [next] | [standalone]

#1425713

Am 18.06.2016 um 13:34 schrieb Boris Brezillon:
> On Sat, 18 Jun 2016 11:23:01 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 55f3ab8..aadebe7 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -3703,6 +3703,10 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
>>
>>  	chip->manufacturer.ops = nand_manuf_ids[maf_idx].ops;
>>
>> +	if (!chip->manufacturer.ops)
>> +		/* assign no operations */
>> +		chip->manufacturer.ops = nand_manuf_ids[0].ops;
>> +
> 
> BTW, this is wrong, the manufacturer id code is not the index of the
> nand_manuf_ids[] table ;). If we go for this option, we should probably
> declare no_ops in nand_base.c and assign it here:
> 

Oh, yes. You are right.
Anyway, I think having a wrapper is also a good solution.

Thanks,
//richard

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web