[RFC,13/34] mtd: spi-nor: Add the concept of SPI NOR manufacturer driver
diff mbox series

Message ID 20181207092637.18687-14-boris.brezillon@bootlin.com
State Under Review
Delegated to: Ambarus Tudor
Headers show
Series
  • mtd: spi-nor: Move manufacturer/SFDP code out of the core
Related show

Commit Message

Boris Brezillon Dec. 7, 2018, 9:26 a.m. UTC
Declare a spi_nor_manufacturer struct and add basic building blocks to
move manufacturer specific code outside of the core. We also rename
spi-nor.c into spi-nor-core.c and adjust the Makefile to be able to
create spi-nor.o by linking the core and manufacturer drivers together.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/mtd/spi-nor/core.c      | 82 +++++++++++++++++++++++++++------
 drivers/mtd/spi-nor/internals.h | 14 ++++++
 include/linux/mtd/spi-nor.h     |  8 ++++
 3 files changed, 89 insertions(+), 15 deletions(-)

Comments

Vignesh Raghavendra Dec. 10, 2018, 8:20 a.m. UTC | #1
Hi Boris,

On 07/12/18 2:56 PM, Boris Brezillon wrote:
> Declare a spi_nor_manufacturer struct and add basic building blocks to
> move manufacturer specific code outside of the core. We also rename
> spi-nor.c into spi-nor-core.c and adjust the Makefile to be able to
> create spi-nor.o by linking the core and manufacturer drivers together.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/spi-nor/core.c      | 82 +++++++++++++++++++++++++++------
>  drivers/mtd/spi-nor/internals.h | 14 ++++++
>  include/linux/mtd/spi-nor.h     |  8 ++++
>  3 files changed, 89 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f8b7c8fbe960..8efd0490d2a0 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1722,6 +1722,23 @@ static const struct flash_info spi_nor_ids[] = {
>  	{ },
>  };
>  
> +static const struct spi_nor_manufacturer *manufacturers[0];
> +
> +static const struct flash_info *
> +spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
> +			  const u8 *id)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < nparts; i++) {
> +		if (parts[i].id_len &&
> +		    !memcmp(parts[i].id, id, parts[i].id_len))
> +			return &parts[i];
> +	}
> +
> +	return NULL;
> +}
> +
>  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>  {
>  	int			tmp;
> @@ -1734,13 +1751,21 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>  		return ERR_PTR(tmp);
>  	}
>  
> -	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
> -		info = &spi_nor_ids[tmp];
> -		if (info->id_len) {
> -			if (!memcmp(info->id, id, info->id_len))
> -				return &spi_nor_ids[tmp];
> +	for (tmp = 0; tmp < ARRAY_SIZE(manufacturers); tmp++) {
> +		info = spi_nor_search_part_by_id(manufacturers[tmp]->parts,
> +						 manufacturers[tmp]->nparts,
> +						 id);

This could probably be optimized a bit by comparing manufacturer ID and
then looking through parts list of that particular manufacturer only.
(with expection of winbond manufacturer ID, where will have to go
through spansion parts list as well)

Regards
Vignesh

> +		if (info) {
> +			nor->manufacturer = manufacturers[tmp];
> +			return info;
>  		}
>  	}
> +
> +	info = spi_nor_search_part_by_id(spi_nor_ids,
> +					 ARRAY_SIZE(spi_nor_ids) - 1, id);
> +	if (info)
> +		return info;
> +
>  	dev_err(nor->dev, "unrecognized JEDEC id bytes: %02x, %02x, %02x\n",
>  		id[0], id[1], id[2]);
>  	return ERR_PTR(-ENODEV);
> @@ -2334,9 +2359,22 @@ spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>  			 const struct sfdp_bfpt *bfpt,
>  			 struct spi_nor_flash_parameter *params)
>  {
> -	if (nor->info->fixups && nor->info->fixups->post_bfpt)
> -		return nor->info->fixups->post_bfpt(nor, bfpt_header, bfpt,
> -						    params);
> +	int ret;
> +
> +	if (nor->manufacturer && nor->manufacturer->fixups &&
> +	    nor->manufacturer->fixups->post_bfpt) {
> +		ret = nor->manufacturer->fixups->post_bfpt(nor, bfpt_header,
> +							   bfpt, params);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (nor->info->fixups && nor->info->fixups->post_bfpt) {
> +		ret = nor->info->fixups->post_bfpt(nor, bfpt_header, bfpt,
> +						   params);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -3426,6 +3464,10 @@ static int
>  spi_nor_manufacturer_post_sfdp_fixups(struct spi_nor *nor,
>  				      struct spi_nor_flash_parameter *params)
>  {
> +	if (nor->manufacturer && nor->manufacturer->fixups &&
> +	    nor->manufacturer->fixups->post_sfdp)
> +		return nor->manufacturer->fixups->post_sfdp(nor, params);
> +
>  	switch (JEDEC_MFR(nor->info)) {
>  	case SNOR_MFR_ATMEL:
>  		atmel_post_sfdp_fixups(nor);
> @@ -3481,15 +3523,25 @@ static int spi_nor_post_sfdp_fixups(struct spi_nor *nor,
>  	return ret;
>  }
>  
> -static const struct flash_info *spi_nor_match_id(const char *name)
> +static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
> +						 const char *name)
>  {
> -	const struct flash_info *id = spi_nor_ids;
> +	unsigned int i, j;
>  
> -	while (id->name) {
> -		if (!strcmp(name, id->name))
> -			return id;
> -		id++;
> +	for (i = 0; i < ARRAY_SIZE(spi_nor_ids) - 1; i++) {
> +		if (!strcmp(name, spi_nor_ids[i].name))
> +			return &spi_nor_ids[i];
>  	}
> +
> +	for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
> +		for (j = 0; j < manufacturers[i]->nparts; j++) {
> +			if (!strcmp(name, manufacturers[i]->parts[j].name)) {
> +				nor->manufacturer = manufacturers[i];
> +				return &manufacturers[i]->parts[j];
> +			}
> +		}
> +	}
> +
>  	return NULL;
>  }
>  
> @@ -3514,7 +3566,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	nor->write_proto = SNOR_PROTO_1_1_1;
>  
>  	if (name)
> -		info = spi_nor_match_id(name);
> +		info = spi_nor_match_id(nor, name);
>  	/* Try to auto-detect if chip name wasn't specified or not found */
>  	if (!info)
>  		info = spi_nor_read_id(nor);
> diff --git a/drivers/mtd/spi-nor/internals.h b/drivers/mtd/spi-nor/internals.h
> index cc06fed99f49..c442d0bfa21a 100644
> --- a/drivers/mtd/spi-nor/internals.h
> +++ b/drivers/mtd/spi-nor/internals.h
> @@ -318,4 +318,18 @@ struct flash_info {
>  		.addr_width = 3,					\
>  		.flags = SPI_NOR_NO_FR | SPI_S3AN,
>  
> +/**
> + * struct spi_nor_manufacturer - SPI NOR manufacturer object
> + * @name: manufacturer name
> + * @parts: array of parts supported by this manufacturer
> + * @nparts: number of entries in the parts array
> + * @fixups: hooks called at various points in time during spi_nor_scan()
> + */
> +struct spi_nor_manufacturer {
> +	const char *name;
> +	const struct flash_info *parts;
> +	unsigned int nparts;
> +	const struct spi_nor_fixups *fixups;
> +};
> +
>  #endif /* __LINUX_MTD_SPI_NOR_INTERNALS_H */
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 8c64f1dcd35e..44ab116ce3d9 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -332,12 +332,19 @@ struct spi_nor_erase_map {
>   */
>  struct flash_info;
>  
> +/**
> + * struct flash_info - Forward declaration of a structure used internally by
> + *		       the core and manufacturer drivers
> + */
> +struct spi_nor_manufacturer;
> +
>  /**
>   * struct spi_nor - Structure for defining a the SPI NOR layer
>   * @mtd:		point to a mtd_info structure
>   * @lock:		the lock for the read/write/erase/lock/unlock operations
>   * @dev:		point to a spi device, or a spi nor controller device.
>   * @info:		spi-nor part JDEC MFR id and other info
> + * @manufacturer:	spi-nor manufacturer
>   * @page_size:		the page size of the SPI NOR
>   * @addr_width:		number of address bytes
>   * @erase_opcode:	the opcode for erasing a sector
> @@ -376,6 +383,7 @@ struct spi_nor {
>  	struct mutex		lock;
>  	struct device		*dev;
>  	const struct flash_info	*info;
> +	const struct spi_nor_manufacturer *manufacturer;
>  	u32			page_size;
>  	u8			addr_width;
>  	u8			erase_opcode;
>
Boris Brezillon Dec. 10, 2018, 8:35 a.m. UTC | #2
Hi Vignesh,

On Mon, 10 Dec 2018 13:50:03 +0530
Vignesh R <vigneshr@ti.com> wrote:

> >  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> >  {
> >  	int			tmp;
> > @@ -1734,13 +1751,21 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
> >  		return ERR_PTR(tmp);
> >  	}
> >  
> > -	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
> > -		info = &spi_nor_ids[tmp];
> > -		if (info->id_len) {
> > -			if (!memcmp(info->id, id, info->id_len))
> > -				return &spi_nor_ids[tmp];
> > +	for (tmp = 0; tmp < ARRAY_SIZE(manufacturers); tmp++) {
> > +		info = spi_nor_search_part_by_id(manufacturers[tmp]->parts,
> > +						 manufacturers[tmp]->nparts,
> > +						 id);  
> 
> This could probably be optimized a bit by comparing manufacturer ID and
> then looking through parts list of that particular manufacturer only.
> (with expection of winbond manufacturer ID, where will have to go
> through spansion parts list as well)

That's what I did at first, but it doesn't work. Some manufacturer IDs
are prefixed by one or several continuation codes (0x7F). That was done
to extend the 256 IDs space (see spec JEP106AX). We could have the
manufacturer ID encoded with an array of byte instead of a single byte,
but there's more: some flashes that are supposed to used continuation
codes, just output the last (non-continuation) byte (see the ISSI IDs
here [1] if you need a proof ;-)). So we're screwed.

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v4.20-rc6/source/drivers/mtd/spi-nor/spi-nor.c#L1351
Vignesh Raghavendra Dec. 10, 2018, 11:28 a.m. UTC | #3
On 10/12/18 2:05 PM, Boris Brezillon wrote:
> Hi Vignesh,
> 
> On Mon, 10 Dec 2018 13:50:03 +0530
> Vignesh R <vigneshr@ti.com> wrote:
> 
>>>  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>>>  {
>>>  	int			tmp;
>>> @@ -1734,13 +1751,21 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>>>  		return ERR_PTR(tmp);
>>>  	}
>>>  
>>> -	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>>> -		info = &spi_nor_ids[tmp];
>>> -		if (info->id_len) {
>>> -			if (!memcmp(info->id, id, info->id_len))
>>> -				return &spi_nor_ids[tmp];
>>> +	for (tmp = 0; tmp < ARRAY_SIZE(manufacturers); tmp++) {
>>> +		info = spi_nor_search_part_by_id(manufacturers[tmp]->parts,
>>> +						 manufacturers[tmp]->nparts,
>>> +						 id);  
>>
>> This could probably be optimized a bit by comparing manufacturer ID and
>> then looking through parts list of that particular manufacturer only.
>> (with expection of winbond manufacturer ID, where will have to go
>> through spansion parts list as well)
> 
> That's what I did at first, but it doesn't work. Some manufacturer IDs
> are prefixed by one or several continuation codes (0x7F). That was done
> to extend the 256 IDs space (see spec JEP106AX). We could have the
> manufacturer ID encoded with an array of byte instead of a single byte,
> but there's more: some flashes that are supposed to used continuation
> codes, just output the last (non-continuation) byte (see the ISSI IDs
> here [1] if you need a proof ;-)). So we're screwed.
> 

Thats bad.. Few ISSI IDs are in conflict with company that does not have
Continuation Code. Thanks for the information!

> Regards,
> 
> Boris
> 
> [1]https://elixir.bootlin.com/linux/v4.20-rc6/source/drivers/mtd/spi-nor/spi-nor.c#L1351
>

Patch
diff mbox series

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f8b7c8fbe960..8efd0490d2a0 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1722,6 +1722,23 @@  static const struct flash_info spi_nor_ids[] = {
 	{ },
 };
 
+static const struct spi_nor_manufacturer *manufacturers[0];
+
+static const struct flash_info *
+spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
+			  const u8 *id)
+{
+	unsigned int i;
+
+	for (i = 0; i < nparts; i++) {
+		if (parts[i].id_len &&
+		    !memcmp(parts[i].id, id, parts[i].id_len))
+			return &parts[i];
+	}
+
+	return NULL;
+}
+
 static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 {
 	int			tmp;
@@ -1734,13 +1751,21 @@  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 		return ERR_PTR(tmp);
 	}
 
-	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
-		info = &spi_nor_ids[tmp];
-		if (info->id_len) {
-			if (!memcmp(info->id, id, info->id_len))
-				return &spi_nor_ids[tmp];
+	for (tmp = 0; tmp < ARRAY_SIZE(manufacturers); tmp++) {
+		info = spi_nor_search_part_by_id(manufacturers[tmp]->parts,
+						 manufacturers[tmp]->nparts,
+						 id);
+		if (info) {
+			nor->manufacturer = manufacturers[tmp];
+			return info;
 		}
 	}
+
+	info = spi_nor_search_part_by_id(spi_nor_ids,
+					 ARRAY_SIZE(spi_nor_ids) - 1, id);
+	if (info)
+		return info;
+
 	dev_err(nor->dev, "unrecognized JEDEC id bytes: %02x, %02x, %02x\n",
 		id[0], id[1], id[2]);
 	return ERR_PTR(-ENODEV);
@@ -2334,9 +2359,22 @@  spi_nor_post_bfpt_fixups(struct spi_nor *nor,
 			 const struct sfdp_bfpt *bfpt,
 			 struct spi_nor_flash_parameter *params)
 {
-	if (nor->info->fixups && nor->info->fixups->post_bfpt)
-		return nor->info->fixups->post_bfpt(nor, bfpt_header, bfpt,
-						    params);
+	int ret;
+
+	if (nor->manufacturer && nor->manufacturer->fixups &&
+	    nor->manufacturer->fixups->post_bfpt) {
+		ret = nor->manufacturer->fixups->post_bfpt(nor, bfpt_header,
+							   bfpt, params);
+		if (ret)
+			return ret;
+	}
+
+	if (nor->info->fixups && nor->info->fixups->post_bfpt) {
+		ret = nor->info->fixups->post_bfpt(nor, bfpt_header, bfpt,
+						   params);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -3426,6 +3464,10 @@  static int
 spi_nor_manufacturer_post_sfdp_fixups(struct spi_nor *nor,
 				      struct spi_nor_flash_parameter *params)
 {
+	if (nor->manufacturer && nor->manufacturer->fixups &&
+	    nor->manufacturer->fixups->post_sfdp)
+		return nor->manufacturer->fixups->post_sfdp(nor, params);
+
 	switch (JEDEC_MFR(nor->info)) {
 	case SNOR_MFR_ATMEL:
 		atmel_post_sfdp_fixups(nor);
@@ -3481,15 +3523,25 @@  static int spi_nor_post_sfdp_fixups(struct spi_nor *nor,
 	return ret;
 }
 
-static const struct flash_info *spi_nor_match_id(const char *name)
+static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
+						 const char *name)
 {
-	const struct flash_info *id = spi_nor_ids;
+	unsigned int i, j;
 
-	while (id->name) {
-		if (!strcmp(name, id->name))
-			return id;
-		id++;
+	for (i = 0; i < ARRAY_SIZE(spi_nor_ids) - 1; i++) {
+		if (!strcmp(name, spi_nor_ids[i].name))
+			return &spi_nor_ids[i];
 	}
+
+	for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
+		for (j = 0; j < manufacturers[i]->nparts; j++) {
+			if (!strcmp(name, manufacturers[i]->parts[j].name)) {
+				nor->manufacturer = manufacturers[i];
+				return &manufacturers[i]->parts[j];
+			}
+		}
+	}
+
 	return NULL;
 }
 
@@ -3514,7 +3566,7 @@  int spi_nor_scan(struct spi_nor *nor, const char *name,
 	nor->write_proto = SNOR_PROTO_1_1_1;
 
 	if (name)
-		info = spi_nor_match_id(name);
+		info = spi_nor_match_id(nor, name);
 	/* Try to auto-detect if chip name wasn't specified or not found */
 	if (!info)
 		info = spi_nor_read_id(nor);
diff --git a/drivers/mtd/spi-nor/internals.h b/drivers/mtd/spi-nor/internals.h
index cc06fed99f49..c442d0bfa21a 100644
--- a/drivers/mtd/spi-nor/internals.h
+++ b/drivers/mtd/spi-nor/internals.h
@@ -318,4 +318,18 @@  struct flash_info {
 		.addr_width = 3,					\
 		.flags = SPI_NOR_NO_FR | SPI_S3AN,
 
+/**
+ * struct spi_nor_manufacturer - SPI NOR manufacturer object
+ * @name: manufacturer name
+ * @parts: array of parts supported by this manufacturer
+ * @nparts: number of entries in the parts array
+ * @fixups: hooks called at various points in time during spi_nor_scan()
+ */
+struct spi_nor_manufacturer {
+	const char *name;
+	const struct flash_info *parts;
+	unsigned int nparts;
+	const struct spi_nor_fixups *fixups;
+};
+
 #endif /* __LINUX_MTD_SPI_NOR_INTERNALS_H */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 8c64f1dcd35e..44ab116ce3d9 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -332,12 +332,19 @@  struct spi_nor_erase_map {
  */
 struct flash_info;
 
+/**
+ * struct flash_info - Forward declaration of a structure used internally by
+ *		       the core and manufacturer drivers
+ */
+struct spi_nor_manufacturer;
+
 /**
  * struct spi_nor - Structure for defining a the SPI NOR layer
  * @mtd:		point to a mtd_info structure
  * @lock:		the lock for the read/write/erase/lock/unlock operations
  * @dev:		point to a spi device, or a spi nor controller device.
  * @info:		spi-nor part JDEC MFR id and other info
+ * @manufacturer:	spi-nor manufacturer
  * @page_size:		the page size of the SPI NOR
  * @addr_width:		number of address bytes
  * @erase_opcode:	the opcode for erasing a sector
@@ -376,6 +383,7 @@  struct spi_nor {
 	struct mutex		lock;
 	struct device		*dev;
 	const struct flash_info	*info;
+	const struct spi_nor_manufacturer *manufacturer;
 	u32			page_size;
 	u8			addr_width;
 	u8			erase_opcode;