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

Groups > linux.kernel > #1626357 > unrolled thread

[PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"

Started byAndrey Smirnov <andrew.smirnov@gmail.com>
First post2017-04-19 17:30 +0200
Last post2017-04-19 18:20 +0200
Articles 5 — 3 participants

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

  [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information" Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-04-19 17:30 +0200
    Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device  information" Marek Vasut <marek.vasut@gmail.com> - 2017-04-19 17:50 +0200
      Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information" Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-04-19 18:10 +0200
        Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device  information" Marek Vasut <marex@denx.de> - 2017-04-19 18:20 +0200
        Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device  information" Marek Vasut <marek.vasut@gmail.com> - 2017-04-19 18:20 +0200

#1626357 — [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"

FromAndrey Smirnov <andrew.smirnov@gmail.com>
Date2017-04-19 17:30 +0200
Subject[PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
Message-ID<txZIm-56L-13@gated-at.bofh.it>
In anticipation of supporting chips that need it, extend the size of
struct flash_info's 'jedec_id' field to make room 2 byte of extended
device information as well as add code to fetch this data during
jedec_probe().

Cc: cphealy@gmail.com
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Changes since [v2]:

	- Make 'id' have same size as 'jedec'

	- Get rid of eid_mask variable in favour of using GENMASK
          in-place

Changes since [v1]:

	- Formatting

[v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
[v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com

 drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 47 deletions(-)

diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 2d3e403..941e8b7 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -687,7 +687,7 @@ struct flash_info {
 	/* JEDEC id has a high byte of zero plus three data bytes:
 	 * the manufacturer id, then a two byte device id.
 	 */
-	u32		jedec_id;
+	u64		jedec_id;
 
 	/* The size listed here is what works with OP_ERASE_PAGE. */
 	unsigned	nr_pages;
@@ -710,62 +710,34 @@ static struct flash_info dataflash_data[] = {
 	 * These newer chips also support 128-byte security registers (with
 	 * 64 bytes one-time-programmable) and software write-protection.
 	 */
-	{ "AT45DB011B",  0x1f2200, 512, 264, 9, SUP_POW2PS},
-	{ "at45db011d",  0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
+	{ "AT45DB011B",  0x1f22000000, 512, 264, 9, SUP_POW2PS},
+	{ "at45db011d",  0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
 
-	{ "AT45DB021B",  0x1f2300, 1024, 264, 9, SUP_POW2PS},
-	{ "at45db021d",  0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
+	{ "AT45DB021B",  0x1f23000000, 1024, 264, 9, SUP_POW2PS},
+	{ "at45db021d",  0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
 
-	{ "AT45DB041x",  0x1f2400, 2048, 264, 9, SUP_POW2PS},
-	{ "at45db041d",  0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
+	{ "AT45DB041x",  0x1f24000000, 2048, 264, 9, SUP_POW2PS},
+	{ "at45db041d",  0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
 
-	{ "AT45DB081B",  0x1f2500, 4096, 264, 9, SUP_POW2PS},
-	{ "at45db081d",  0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
+	{ "AT45DB081B",  0x1f25000000, 4096, 264, 9, SUP_POW2PS},
+	{ "at45db081d",  0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
 
-	{ "AT45DB161x",  0x1f2600, 4096, 528, 10, SUP_POW2PS},
-	{ "at45db161d",  0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
+	{ "AT45DB161x",  0x1f26000000, 4096, 528, 10, SUP_POW2PS},
+	{ "at45db161d",  0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
 
-	{ "AT45DB321x",  0x1f2700, 8192, 528, 10, 0},		/* rev C */
+	{ "AT45DB321x",  0x1f27000000, 8192, 528, 10, 0},	/* rev C */
 
-	{ "AT45DB321x",  0x1f2701, 8192, 528, 10, SUP_POW2PS},
-	{ "at45db321d",  0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
+	{ "AT45DB321x",  0x1f27010000, 8192, 528, 10, SUP_POW2PS},
+	{ "at45db321d",  0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
 
-	{ "AT45DB642x",  0x1f2800, 8192, 1056, 11, SUP_POW2PS},
-	{ "at45db642d",  0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
+	{ "AT45DB642x",  0x1f28000000, 8192, 1056, 11, SUP_POW2PS},
+	{ "at45db642d",  0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
 };
 
-static struct flash_info *jedec_probe(struct spi_device *spi)
+static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec)
 {
-	int ret;
-	u8 code = OP_READ_ID;
-	u8 id[3];
-	u32 jedec;
-	struct flash_info *info;
 	int status;
-
-	/*
-	 * JEDEC also defines an optional "extended device information"
-	 * string for after vendor-specific data, after the three bytes
-	 * we use here.  Supporting some chips might require using it.
-	 *
-	 * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
-	 * That's not an error; only rev C and newer chips handle it, and
-	 * only Atmel sells these chips.
-	 */
-	ret = spi_write_then_read(spi, &code, 1, id, 3);
-	if (ret < 0) {
-		dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", ret);
-		return ERR_PTR(ret);
-	}
-
-	if (id[0] != CFI_MFR_ATMEL)
-		return NULL;
-
-	jedec = id[0];
-	jedec = jedec << 8;
-	jedec |= id[1];
-	jedec = jedec << 8;
-	jedec |= id[2];
+	struct flash_info *info;
 
 	for (info = dataflash_data;
 	     info < dataflash_data + ARRAY_SIZE(dataflash_data);
@@ -793,12 +765,61 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
 		}
 	}
 
+	return NULL;
+}
+
+static struct flash_info *jedec_probe(struct spi_device *spi)
+{
+	int ret;
+	u8 code = OP_READ_ID;
+	u64 jedec;
+	u8 id[sizeof(jedec)] = {0};
+	const unsigned int id_size = 5;
+	const unsigned int first_byte = sizeof(id) - id_size;
+	struct flash_info *info;
+
+	/*
+	 * JEDEC also defines an optional "extended device information"
+	 * string for after vendor-specific data, after the three bytes
+	 * we use here.  Supporting some chips might require using it.
+	 *
+	 * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
+	 * That's not an error; only rev C and newer chips handle it, and
+	 * only Atmel sells these chips.
+	 */
+	ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size);
+	if (ret < 0) {
+		pr_debug("%s: error %d reading JEDEC ID\n",
+			dev_name(&spi->dev), ret);
+		return ERR_PTR(ret);
+	}
+
+	if (id[first_byte] != CFI_MFR_ATMEL)
+		return NULL;
+
+	jedec = be64_to_cpup((__be64 *)id);
+
+	info = jedec_lookup(spi, jedec);
+	if (info)
+		return info;
+
+	/*
+	 * Clear extended id bits (bits 0 through 15) and try to find
+	 * a match again in case our chip does not support returning
+	 * extended ID information and we got invalid bits instead
+	 */
+	jedec &= GENMASK_ULL(63, 16);
+
+	info = jedec_lookup(spi, jedec);
+	if (info)
+		return info;
+
 	/*
 	 * Treat other chips as errors ... we won't know the right page
 	 * size (it might be binary) even when we can tell which density
 	 * class is involved (legacy chip id scheme).
 	 */
-	dev_warn(&spi->dev, "JEDEC id %06x not handled\n", jedec);
+	dev_warn(&spi->dev, "JEDEC id %010llx not handled\n", jedec);
 	return ERR_PTR(-ENODEV);
 }
 
-- 
2.9.3

[toc] | [next] | [standalone]

#1626414 — Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-04-19 17:50 +0200
SubjectRe: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
Message-ID<ty01I-5db-25@gated-at.bofh.it>
In reply to#1626357
On 04/19/2017 05:23 PM, Andrey Smirnov wrote:
> In anticipation of supporting chips that need it, extend the size of
> struct flash_info's 'jedec_id' field to make room 2 byte of extended
> device information as well as add code to fetch this data during
> jedec_probe().
> 
> Cc: cphealy@gmail.com
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> 
> Changes since [v2]:
> 
> 	- Make 'id' have same size as 'jedec'
> 
> 	- Get rid of eid_mask variable in favour of using GENMASK
>           in-place
> 
> Changes since [v1]:
> 
> 	- Formatting
> 
> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
> [v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com
> 
>  drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
>  1 file changed, 68 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
> index 2d3e403..941e8b7 100644
> --- a/drivers/mtd/devices/mtd_dataflash.c
> +++ b/drivers/mtd/devices/mtd_dataflash.c
> @@ -687,7 +687,7 @@ struct flash_info {
>  	/* JEDEC id has a high byte of zero plus three data bytes:
>  	 * the manufacturer id, then a two byte device id.
>  	 */
> -	u32		jedec_id;
> +	u64		jedec_id;
>  
>  	/* The size listed here is what works with OP_ERASE_PAGE. */
>  	unsigned	nr_pages;
> @@ -710,62 +710,34 @@ static struct flash_info dataflash_data[] = {
>  	 * These newer chips also support 128-byte security registers (with
>  	 * 64 bytes one-time-programmable) and software write-protection.
>  	 */
> -	{ "AT45DB011B",  0x1f2200, 512, 264, 9, SUP_POW2PS},
> -	{ "at45db011d",  0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
> +	{ "AT45DB011B",  0x1f22000000, 512, 264, 9, SUP_POW2PS},
> +	{ "at45db011d",  0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
>  
> -	{ "AT45DB021B",  0x1f2300, 1024, 264, 9, SUP_POW2PS},
> -	{ "at45db021d",  0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
> +	{ "AT45DB021B",  0x1f23000000, 1024, 264, 9, SUP_POW2PS},
> +	{ "at45db021d",  0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
>  
> -	{ "AT45DB041x",  0x1f2400, 2048, 264, 9, SUP_POW2PS},
> -	{ "at45db041d",  0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
> +	{ "AT45DB041x",  0x1f24000000, 2048, 264, 9, SUP_POW2PS},
> +	{ "at45db041d",  0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
>  
> -	{ "AT45DB081B",  0x1f2500, 4096, 264, 9, SUP_POW2PS},
> -	{ "at45db081d",  0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
> +	{ "AT45DB081B",  0x1f25000000, 4096, 264, 9, SUP_POW2PS},
> +	{ "at45db081d",  0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
>  
> -	{ "AT45DB161x",  0x1f2600, 4096, 528, 10, SUP_POW2PS},
> -	{ "at45db161d",  0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
> +	{ "AT45DB161x",  0x1f26000000, 4096, 528, 10, SUP_POW2PS},
> +	{ "at45db161d",  0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
>  
> -	{ "AT45DB321x",  0x1f2700, 8192, 528, 10, 0},		/* rev C */
> +	{ "AT45DB321x",  0x1f27000000, 8192, 528, 10, 0},	/* rev C */
>  
> -	{ "AT45DB321x",  0x1f2701, 8192, 528, 10, SUP_POW2PS},
> -	{ "at45db321d",  0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
> +	{ "AT45DB321x",  0x1f27010000, 8192, 528, 10, SUP_POW2PS},
> +	{ "at45db321d",  0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
>  
> -	{ "AT45DB642x",  0x1f2800, 8192, 1056, 11, SUP_POW2PS},
> -	{ "at45db642d",  0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
> +	{ "AT45DB642x",  0x1f28000000, 8192, 1056, 11, SUP_POW2PS},
> +	{ "at45db642d",  0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
>  };
>  
> -static struct flash_info *jedec_probe(struct spi_device *spi)
> +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec)
>  {
> -	int ret;
> -	u8 code = OP_READ_ID;
> -	u8 id[3];
> -	u32 jedec;
> -	struct flash_info *info;
>  	int status;
> -
> -	/*
> -	 * JEDEC also defines an optional "extended device information"
> -	 * string for after vendor-specific data, after the three bytes
> -	 * we use here.  Supporting some chips might require using it.
> -	 *
> -	 * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
> -	 * That's not an error; only rev C and newer chips handle it, and
> -	 * only Atmel sells these chips.
> -	 */
> -	ret = spi_write_then_read(spi, &code, 1, id, 3);
> -	if (ret < 0) {
> -		dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", ret);
> -		return ERR_PTR(ret);
> -	}
> -
> -	if (id[0] != CFI_MFR_ATMEL)
> -		return NULL;
> -
> -	jedec = id[0];
> -	jedec = jedec << 8;
> -	jedec |= id[1];
> -	jedec = jedec << 8;
> -	jedec |= id[2];
> +	struct flash_info *info;
>  
>  	for (info = dataflash_data;
>  	     info < dataflash_data + ARRAY_SIZE(dataflash_data);
> @@ -793,12 +765,61 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
>  		}
>  	}
>  
> +	return NULL;
> +}
> +
> +static struct flash_info *jedec_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	u8 code = OP_READ_ID;
> +	u64 jedec;
> +	u8 id[sizeof(jedec)] = {0};
> +	const unsigned int id_size = 5;
> +	const unsigned int first_byte = sizeof(id) - id_size;
> +	struct flash_info *info;
> +
> +	/*
> +	 * JEDEC also defines an optional "extended device information"
> +	 * string for after vendor-specific data, after the three bytes
> +	 * we use here.  Supporting some chips might require using it.
> +	 *
> +	 * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
> +	 * That's not an error; only rev C and newer chips handle it, and
> +	 * only Atmel sells these chips.
> +	 */
> +	ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size);

Hm, looking at this again, what prevents you from reading these bytes
into &id[0] ? Might be easier, ie.

ret = spi_write_then_read(spi, &code, 1, id, id_size);

> +	if (ret < 0) {
> +		pr_debug("%s: error %d reading JEDEC ID\n",
> +			dev_name(&spi->dev), ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	if (id[first_byte] != CFI_MFR_ATMEL)

if (id[0] != CFI_MFR_ATMEL)

> +		return NULL;
> +
> +	jedec = be64_to_cpup((__be64 *)id);
> +
> +	info = jedec_lookup(spi, jedec);

... define these somewhere at the beginning of the dataflash code ...
#define DATAFLASH_MASK_STD	GENMASK(63, 40)
#define DATAFLASH_MASK_EXT	GENMASK(63, 16)

... then ...
info = jedec_lookup(spi, jedec & DATAFLASH_MASK_EXT);

> +	if (info)
> +		return info;
> +
> +	/*
> +	 * Clear extended id bits (bits 0 through 15) and try to find
> +	 * a match again in case our chip does not support returning
> +	 * extended ID information and we got invalid bits instead
> +	 */
> +	jedec &= GENMASK_ULL(63, 16);

info = jedec_lookup(spi, jedec & DATAFLASH_MASK_STD);

> +	info = jedec_lookup(spi, jedec);
> +	if (info)
> +		return info;
> +
>  	/*
>  	 * Treat other chips as errors ... we won't know the right page
>  	 * size (it might be binary) even when we can tell which density
>  	 * class is involved (legacy chip id scheme).
>  	 */
> -	dev_warn(&spi->dev, "JEDEC id %06x not handled\n", jedec);
> +	dev_warn(&spi->dev, "JEDEC id %010llx not handled\n", jedec);
>  	return ERR_PTR(-ENODEV);
>  }
>  
> 


-- 
Best regards,
Marek Vasut

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

#1626424

FromAndrey Smirnov <andrew.smirnov@gmail.com>
Date2017-04-19 18:10 +0200
Message-ID<ty0l4-5yM-25@gated-at.bofh.it>
In reply to#1626414
On Wed, Apr 19, 2017 at 8:42 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 04/19/2017 05:23 PM, Andrey Smirnov wrote:
>> In anticipation of supporting chips that need it, extend the size of
>> struct flash_info's 'jedec_id' field to make room 2 byte of extended
>> device information as well as add code to fetch this data during
>> jedec_probe().
>>
>> Cc: cphealy@gmail.com
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Brian Norris <computersforpeace@gmail.com>
>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
>> Cc: Marek Vasut <marek.vasut@gmail.com>
>> Cc: Richard Weinberger <richard@nod.at>
>> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>
>> Changes since [v2]:
>>
>>       - Make 'id' have same size as 'jedec'
>>
>>       - Get rid of eid_mask variable in favour of using GENMASK
>>           in-place
>>
>> Changes since [v1]:
>>
>>       - Formatting
>>
>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
>> [v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com
>>
>>  drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
>>  1 file changed, 68 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
>> index 2d3e403..941e8b7 100644
>> --- a/drivers/mtd/devices/mtd_dataflash.c
>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>> @@ -687,7 +687,7 @@ struct flash_info {
>>       /* JEDEC id has a high byte of zero plus three data bytes:
>>        * the manufacturer id, then a two byte device id.
>>        */
>> -     u32             jedec_id;
>> +     u64             jedec_id;
>>
>>       /* The size listed here is what works with OP_ERASE_PAGE. */
>>       unsigned        nr_pages;
>> @@ -710,62 +710,34 @@ static struct flash_info dataflash_data[] = {
>>        * These newer chips also support 128-byte security registers (with
>>        * 64 bytes one-time-programmable) and software write-protection.
>>        */
>> -     { "AT45DB011B",  0x1f2200, 512, 264, 9, SUP_POW2PS},
>> -     { "at45db011d",  0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
>> +     { "AT45DB011B",  0x1f22000000, 512, 264, 9, SUP_POW2PS},
>> +     { "at45db011d",  0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
>>
>> -     { "AT45DB021B",  0x1f2300, 1024, 264, 9, SUP_POW2PS},
>> -     { "at45db021d",  0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
>> +     { "AT45DB021B",  0x1f23000000, 1024, 264, 9, SUP_POW2PS},
>> +     { "at45db021d",  0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
>>
>> -     { "AT45DB041x",  0x1f2400, 2048, 264, 9, SUP_POW2PS},
>> -     { "at45db041d",  0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
>> +     { "AT45DB041x",  0x1f24000000, 2048, 264, 9, SUP_POW2PS},
>> +     { "at45db041d",  0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
>>
>> -     { "AT45DB081B",  0x1f2500, 4096, 264, 9, SUP_POW2PS},
>> -     { "at45db081d",  0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
>> +     { "AT45DB081B",  0x1f25000000, 4096, 264, 9, SUP_POW2PS},
>> +     { "at45db081d",  0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
>>
>> -     { "AT45DB161x",  0x1f2600, 4096, 528, 10, SUP_POW2PS},
>> -     { "at45db161d",  0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
>> +     { "AT45DB161x",  0x1f26000000, 4096, 528, 10, SUP_POW2PS},
>> +     { "at45db161d",  0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
>>
>> -     { "AT45DB321x",  0x1f2700, 8192, 528, 10, 0},           /* rev C */
>> +     { "AT45DB321x",  0x1f27000000, 8192, 528, 10, 0},       /* rev C */
>>
>> -     { "AT45DB321x",  0x1f2701, 8192, 528, 10, SUP_POW2PS},
>> -     { "at45db321d",  0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
>> +     { "AT45DB321x",  0x1f27010000, 8192, 528, 10, SUP_POW2PS},
>> +     { "at45db321d",  0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
>>
>> -     { "AT45DB642x",  0x1f2800, 8192, 1056, 11, SUP_POW2PS},
>> -     { "at45db642d",  0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
>> +     { "AT45DB642x",  0x1f28000000, 8192, 1056, 11, SUP_POW2PS},
>> +     { "at45db642d",  0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
>>  };
>>
>> -static struct flash_info *jedec_probe(struct spi_device *spi)
>> +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec)
>>  {
>> -     int ret;
>> -     u8 code = OP_READ_ID;
>> -     u8 id[3];
>> -     u32 jedec;
>> -     struct flash_info *info;
>>       int status;
>> -
>> -     /*
>> -      * JEDEC also defines an optional "extended device information"
>> -      * string for after vendor-specific data, after the three bytes
>> -      * we use here.  Supporting some chips might require using it.
>> -      *
>> -      * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
>> -      * That's not an error; only rev C and newer chips handle it, and
>> -      * only Atmel sells these chips.
>> -      */
>> -     ret = spi_write_then_read(spi, &code, 1, id, 3);
>> -     if (ret < 0) {
>> -             dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", ret);
>> -             return ERR_PTR(ret);
>> -     }
>> -
>> -     if (id[0] != CFI_MFR_ATMEL)
>> -             return NULL;
>> -
>> -     jedec = id[0];
>> -     jedec = jedec << 8;
>> -     jedec |= id[1];
>> -     jedec = jedec << 8;
>> -     jedec |= id[2];
>> +     struct flash_info *info;
>>
>>       for (info = dataflash_data;
>>            info < dataflash_data + ARRAY_SIZE(dataflash_data);
>> @@ -793,12 +765,61 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
>>               }
>>       }
>>
>> +     return NULL;
>> +}
>> +
>> +static struct flash_info *jedec_probe(struct spi_device *spi)
>> +{
>> +     int ret;
>> +     u8 code = OP_READ_ID;
>> +     u64 jedec;
>> +     u8 id[sizeof(jedec)] = {0};
>> +     const unsigned int id_size = 5;
>> +     const unsigned int first_byte = sizeof(id) - id_size;
>> +     struct flash_info *info;
>> +
>> +     /*
>> +      * JEDEC also defines an optional "extended device information"
>> +      * string for after vendor-specific data, after the three bytes
>> +      * we use here.  Supporting some chips might require using it.
>> +      *
>> +      * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
>> +      * That's not an error; only rev C and newer chips handle it, and
>> +      * only Atmel sells these chips.
>> +      */
>> +     ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size);
>
> Hm, looking at this again, what prevents you from reading these bytes
> into &id[0] ? Might be easier, ie.
>
> ret = spi_write_then_read(spi, &code, 1, id, id_size);
>

Then I'd have to add 6 more '0's to each of the IDs in dataflash_data,
which would make that code even less readable.

>> +     if (ret < 0) {
>> +             pr_debug("%s: error %d reading JEDEC ID\n",
>> +                     dev_name(&spi->dev), ret);
>> +             return ERR_PTR(ret);
>> +     }
>> +
>> +     if (id[first_byte] != CFI_MFR_ATMEL)
>
> if (id[0] != CFI_MFR_ATMEL)
>
>> +             return NULL;
>> +
>> +     jedec = be64_to_cpup((__be64 *)id);
>> +
>> +     info = jedec_lookup(spi, jedec);
>
> ... define these somewhere at the beginning of the dataflash code ...
> #define DATAFLASH_MASK_STD      GENMASK(63, 40)
> #define DATAFLASH_MASK_EXT      GENMASK(63, 16)
>
> ... then ...
> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_EXT);
>
>> +     if (info)
>> +             return info;
>> +
>> +     /*
>> +      * Clear extended id bits (bits 0 through 15) and try to find
>> +      * a match again in case our chip does not support returning
>> +      * extended ID information and we got invalid bits instead
>> +      */
>> +     jedec &= GENMASK_ULL(63, 16);
>
> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_STD);
>

Sure, OK.

Thanks,
Andrey Smirnov

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

#1626431 — Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"

FromMarek Vasut <marex@denx.de>
Date2017-04-19 18:20 +0200
SubjectRe: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
Message-ID<ty0uJ-5Cb-1@gated-at.bofh.it>
In reply to#1626424
On 04/19/2017 06:08 PM, Marek Vasut wrote:
> On 04/19/2017 06:01 PM, Andrey Smirnov wrote:
>> On Wed, Apr 19, 2017 at 8:42 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 04/19/2017 05:23 PM, Andrey Smirnov wrote:
>>>> In anticipation of supporting chips that need it, extend the size of
>>>> struct flash_info's 'jedec_id' field to make room 2 byte of extended
>>>> device information as well as add code to fetch this data during
>>>> jedec_probe().
>>>>
>>>> Cc: cphealy@gmail.com
>>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>>> Cc: Brian Norris <computersforpeace@gmail.com>
>>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
>>>> Cc: Marek Vasut <marek.vasut@gmail.com>
>>>> Cc: Richard Weinberger <richard@nod.at>
>>>> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>>> ---
>>>>
>>>> Changes since [v2]:
>>>>
>>>>       - Make 'id' have same size as 'jedec'
>>>>
>>>>       - Get rid of eid_mask variable in favour of using GENMASK
>>>>           in-place
>>>>
>>>> Changes since [v1]:
>>>>
>>>>       - Formatting
>>>>
>>>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
>>>> [v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com
>>>>
>>>>  drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
>>>>  1 file changed, 68 insertions(+), 47 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
>>>> index 2d3e403..941e8b7 100644
>>>> --- a/drivers/mtd/devices/mtd_dataflash.c
>>>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>>>> @@ -687,7 +687,7 @@ struct flash_info {
>>>>       /* JEDEC id has a high byte of zero plus three data bytes:
>>>>        * the manufacturer id, then a two byte device id.
>>>>        */
>>>> -     u32             jedec_id;
>>>> +     u64             jedec_id;
>>>>
>>>>       /* The size listed here is what works with OP_ERASE_PAGE. */
>>>>       unsigned        nr_pages;
>>>> @@ -710,62 +710,34 @@ static struct flash_info dataflash_data[] = {
>>>>        * These newer chips also support 128-byte security registers (with
>>>>        * 64 bytes one-time-programmable) and software write-protection.
>>>>        */
>>>> -     { "AT45DB011B",  0x1f2200, 512, 264, 9, SUP_POW2PS},
>>>> -     { "at45db011d",  0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>> +     { "AT45DB011B",  0x1f22000000, 512, 264, 9, SUP_POW2PS},
>>>> +     { "at45db011d",  0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>>
>>>> -     { "AT45DB021B",  0x1f2300, 1024, 264, 9, SUP_POW2PS},
>>>> -     { "at45db021d",  0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>> +     { "AT45DB021B",  0x1f23000000, 1024, 264, 9, SUP_POW2PS},
>>>> +     { "at45db021d",  0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>>
>>>> -     { "AT45DB041x",  0x1f2400, 2048, 264, 9, SUP_POW2PS},
>>>> -     { "at45db041d",  0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>> +     { "AT45DB041x",  0x1f24000000, 2048, 264, 9, SUP_POW2PS},
>>>> +     { "at45db041d",  0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>>
>>>> -     { "AT45DB081B",  0x1f2500, 4096, 264, 9, SUP_POW2PS},
>>>> -     { "at45db081d",  0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>> +     { "AT45DB081B",  0x1f25000000, 4096, 264, 9, SUP_POW2PS},
>>>> +     { "at45db081d",  0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>>
>>>> -     { "AT45DB161x",  0x1f2600, 4096, 528, 10, SUP_POW2PS},
>>>> -     { "at45db161d",  0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
>>>> +     { "AT45DB161x",  0x1f26000000, 4096, 528, 10, SUP_POW2PS},
>>>> +     { "at45db161d",  0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
>>>>
>>>> -     { "AT45DB321x",  0x1f2700, 8192, 528, 10, 0},           /* rev C */
>>>> +     { "AT45DB321x",  0x1f27000000, 8192, 528, 10, 0},       /* rev C */
>>>>
>>>> -     { "AT45DB321x",  0x1f2701, 8192, 528, 10, SUP_POW2PS},
>>>> -     { "at45db321d",  0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
>>>> +     { "AT45DB321x",  0x1f27010000, 8192, 528, 10, SUP_POW2PS},
>>>> +     { "at45db321d",  0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
>>>>
>>>> -     { "AT45DB642x",  0x1f2800, 8192, 1056, 11, SUP_POW2PS},
>>>> -     { "at45db642d",  0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
>>>> +     { "AT45DB642x",  0x1f28000000, 8192, 1056, 11, SUP_POW2PS},
>>>> +     { "at45db642d",  0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
>>>>  };
>>>>
>>>> -static struct flash_info *jedec_probe(struct spi_device *spi)
>>>> +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec)
>>>>  {
>>>> -     int ret;
>>>> -     u8 code = OP_READ_ID;
>>>> -     u8 id[3];
>>>> -     u32 jedec;
>>>> -     struct flash_info *info;
>>>>       int status;
>>>> -
>>>> -     /*
>>>> -      * JEDEC also defines an optional "extended device information"
>>>> -      * string for after vendor-specific data, after the three bytes
>>>> -      * we use here.  Supporting some chips might require using it.
>>>> -      *
>>>> -      * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
>>>> -      * That's not an error; only rev C and newer chips handle it, and
>>>> -      * only Atmel sells these chips.
>>>> -      */
>>>> -     ret = spi_write_then_read(spi, &code, 1, id, 3);
>>>> -     if (ret < 0) {
>>>> -             dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", ret);
>>>> -             return ERR_PTR(ret);
>>>> -     }
>>>> -
>>>> -     if (id[0] != CFI_MFR_ATMEL)
>>>> -             return NULL;
>>>> -
>>>> -     jedec = id[0];
>>>> -     jedec = jedec << 8;
>>>> -     jedec |= id[1];
>>>> -     jedec = jedec << 8;
>>>> -     jedec |= id[2];
>>>> +     struct flash_info *info;
>>>>
>>>>       for (info = dataflash_data;
>>>>            info < dataflash_data + ARRAY_SIZE(dataflash_data);
>>>> @@ -793,12 +765,61 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
>>>>               }
>>>>       }
>>>>
>>>> +     return NULL;
>>>> +}
>>>> +
>>>> +static struct flash_info *jedec_probe(struct spi_device *spi)
>>>> +{
>>>> +     int ret;
>>>> +     u8 code = OP_READ_ID;
>>>> +     u64 jedec;
>>>> +     u8 id[sizeof(jedec)] = {0};
>>>> +     const unsigned int id_size = 5;
>>>> +     const unsigned int first_byte = sizeof(id) - id_size;
>>>> +     struct flash_info *info;
>>>> +
>>>> +     /*
>>>> +      * JEDEC also defines an optional "extended device information"
>>>> +      * string for after vendor-specific data, after the three bytes
>>>> +      * we use here.  Supporting some chips might require using it.
>>>> +      *
>>>> +      * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
>>>> +      * That's not an error; only rev C and newer chips handle it, and
>>>> +      * only Atmel sells these chips.
>>>> +      */
>>>> +     ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size);
>>>
>>> Hm, looking at this again, what prevents you from reading these bytes
>>> into &id[0] ? Might be easier, ie.
>>>
>>> ret = spi_write_then_read(spi, &code, 1, id, id_size);
>>>
>>
>> Then I'd have to add 6 more '0's to each of the IDs in dataflash_data,
>> which would make that code even less readable.
> 
> Or do >>= 24 on the jedec ID :)

In fact, you can use the flags field to introduce a flag to indicate
that the flash uses ext ID . Then the lookup function can do first try
the ext IDs (with correct shift) and then standard IDs (with correct
bitshift again).

Upside is, you don't need to patch the table above at all, you just add
your new flash with another flag , ie. SUP_EXTID .

>>>> +     if (ret < 0) {
>>>> +             pr_debug("%s: error %d reading JEDEC ID\n",
>>>> +                     dev_name(&spi->dev), ret);
>>>> +             return ERR_PTR(ret);
>>>> +     }
>>>> +
>>>> +     if (id[first_byte] != CFI_MFR_ATMEL)
>>>
>>> if (id[0] != CFI_MFR_ATMEL)
>>>
>>>> +             return NULL;
>>>> +
>>>> +     jedec = be64_to_cpup((__be64 *)id);
>>>> +
>>>> +     info = jedec_lookup(spi, jedec);
>>>
>>> ... define these somewhere at the beginning of the dataflash code ...
>>> #define DATAFLASH_MASK_STD      GENMASK(63, 40)
>>> #define DATAFLASH_MASK_EXT      GENMASK(63, 16)
>>>
>>> ... then ...
>>> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_EXT);
>>>
>>>> +     if (info)
>>>> +             return info;
>>>> +
>>>> +     /*
>>>> +      * Clear extended id bits (bits 0 through 15) and try to find
>>>> +      * a match again in case our chip does not support returning
>>>> +      * extended ID information and we got invalid bits instead
>>>> +      */
>>>> +     jedec &= GENMASK_ULL(63, 16);
>>>
>>> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_STD);
>>>
>>
>> Sure, OK.
>>
>> Thanks,
>> Andrey Smirnov
>>
> 
> 


-- 
Best regards,
Marek Vasut

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

#1626435 — Re: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-04-19 18:20 +0200
SubjectRe: [PATCH v3 5/6] mtd: dataflash: Make use of "extened device information"
Message-ID<ty0uJ-5Cb-3@gated-at.bofh.it>
In reply to#1626424
On 04/19/2017 06:01 PM, Andrey Smirnov wrote:
> On Wed, Apr 19, 2017 at 8:42 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 04/19/2017 05:23 PM, Andrey Smirnov wrote:
>>> In anticipation of supporting chips that need it, extend the size of
>>> struct flash_info's 'jedec_id' field to make room 2 byte of extended
>>> device information as well as add code to fetch this data during
>>> jedec_probe().
>>>
>>> Cc: cphealy@gmail.com
>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>> Cc: Brian Norris <computersforpeace@gmail.com>
>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
>>> Cc: Marek Vasut <marek.vasut@gmail.com>
>>> Cc: Richard Weinberger <richard@nod.at>
>>> Cc: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>> ---
>>>
>>> Changes since [v2]:
>>>
>>>       - Make 'id' have same size as 'jedec'
>>>
>>>       - Get rid of eid_mask variable in favour of using GENMASK
>>>           in-place
>>>
>>> Changes since [v1]:
>>>
>>>       - Formatting
>>>
>>> [v1] http://lkml.kernel.org/r/20170411161722.11164-1-andrew.smirnov@gmail.com
>>> [v2] http://lkml.kernel.org/r/20170418142127.23301-3-andrew.smirnov@gmail.com
>>>
>>>  drivers/mtd/devices/mtd_dataflash.c | 115 +++++++++++++++++++++---------------
>>>  1 file changed, 68 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
>>> index 2d3e403..941e8b7 100644
>>> --- a/drivers/mtd/devices/mtd_dataflash.c
>>> +++ b/drivers/mtd/devices/mtd_dataflash.c
>>> @@ -687,7 +687,7 @@ struct flash_info {
>>>       /* JEDEC id has a high byte of zero plus three data bytes:
>>>        * the manufacturer id, then a two byte device id.
>>>        */
>>> -     u32             jedec_id;
>>> +     u64             jedec_id;
>>>
>>>       /* The size listed here is what works with OP_ERASE_PAGE. */
>>>       unsigned        nr_pages;
>>> @@ -710,62 +710,34 @@ static struct flash_info dataflash_data[] = {
>>>        * These newer chips also support 128-byte security registers (with
>>>        * 64 bytes one-time-programmable) and software write-protection.
>>>        */
>>> -     { "AT45DB011B",  0x1f2200, 512, 264, 9, SUP_POW2PS},
>>> -     { "at45db011d",  0x1f2200, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
>>> +     { "AT45DB011B",  0x1f22000000, 512, 264, 9, SUP_POW2PS},
>>> +     { "at45db011d",  0x1f22000000, 512, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>
>>> -     { "AT45DB021B",  0x1f2300, 1024, 264, 9, SUP_POW2PS},
>>> -     { "at45db021d",  0x1f2300, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
>>> +     { "AT45DB021B",  0x1f23000000, 1024, 264, 9, SUP_POW2PS},
>>> +     { "at45db021d",  0x1f23000000, 1024, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>
>>> -     { "AT45DB041x",  0x1f2400, 2048, 264, 9, SUP_POW2PS},
>>> -     { "at45db041d",  0x1f2400, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
>>> +     { "AT45DB041x",  0x1f24000000, 2048, 264, 9, SUP_POW2PS},
>>> +     { "at45db041d",  0x1f24000000, 2048, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>
>>> -     { "AT45DB081B",  0x1f2500, 4096, 264, 9, SUP_POW2PS},
>>> -     { "at45db081d",  0x1f2500, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
>>> +     { "AT45DB081B",  0x1f25000000, 4096, 264, 9, SUP_POW2PS},
>>> +     { "at45db081d",  0x1f25000000, 4096, 256, 8, SUP_POW2PS | IS_POW2PS},
>>>
>>> -     { "AT45DB161x",  0x1f2600, 4096, 528, 10, SUP_POW2PS},
>>> -     { "at45db161d",  0x1f2600, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
>>> +     { "AT45DB161x",  0x1f26000000, 4096, 528, 10, SUP_POW2PS},
>>> +     { "at45db161d",  0x1f26000000, 4096, 512, 9, SUP_POW2PS | IS_POW2PS},
>>>
>>> -     { "AT45DB321x",  0x1f2700, 8192, 528, 10, 0},           /* rev C */
>>> +     { "AT45DB321x",  0x1f27000000, 8192, 528, 10, 0},       /* rev C */
>>>
>>> -     { "AT45DB321x",  0x1f2701, 8192, 528, 10, SUP_POW2PS},
>>> -     { "at45db321d",  0x1f2701, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
>>> +     { "AT45DB321x",  0x1f27010000, 8192, 528, 10, SUP_POW2PS},
>>> +     { "at45db321d",  0x1f27010000, 8192, 512, 9, SUP_POW2PS | IS_POW2PS},
>>>
>>> -     { "AT45DB642x",  0x1f2800, 8192, 1056, 11, SUP_POW2PS},
>>> -     { "at45db642d",  0x1f2800, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
>>> +     { "AT45DB642x",  0x1f28000000, 8192, 1056, 11, SUP_POW2PS},
>>> +     { "at45db642d",  0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
>>>  };
>>>
>>> -static struct flash_info *jedec_probe(struct spi_device *spi)
>>> +static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec)
>>>  {
>>> -     int ret;
>>> -     u8 code = OP_READ_ID;
>>> -     u8 id[3];
>>> -     u32 jedec;
>>> -     struct flash_info *info;
>>>       int status;
>>> -
>>> -     /*
>>> -      * JEDEC also defines an optional "extended device information"
>>> -      * string for after vendor-specific data, after the three bytes
>>> -      * we use here.  Supporting some chips might require using it.
>>> -      *
>>> -      * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
>>> -      * That's not an error; only rev C and newer chips handle it, and
>>> -      * only Atmel sells these chips.
>>> -      */
>>> -     ret = spi_write_then_read(spi, &code, 1, id, 3);
>>> -     if (ret < 0) {
>>> -             dev_dbg(&spi->dev, "error %d reading JEDEC ID\n", ret);
>>> -             return ERR_PTR(ret);
>>> -     }
>>> -
>>> -     if (id[0] != CFI_MFR_ATMEL)
>>> -             return NULL;
>>> -
>>> -     jedec = id[0];
>>> -     jedec = jedec << 8;
>>> -     jedec |= id[1];
>>> -     jedec = jedec << 8;
>>> -     jedec |= id[2];
>>> +     struct flash_info *info;
>>>
>>>       for (info = dataflash_data;
>>>            info < dataflash_data + ARRAY_SIZE(dataflash_data);
>>> @@ -793,12 +765,61 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
>>>               }
>>>       }
>>>
>>> +     return NULL;
>>> +}
>>> +
>>> +static struct flash_info *jedec_probe(struct spi_device *spi)
>>> +{
>>> +     int ret;
>>> +     u8 code = OP_READ_ID;
>>> +     u64 jedec;
>>> +     u8 id[sizeof(jedec)] = {0};
>>> +     const unsigned int id_size = 5;
>>> +     const unsigned int first_byte = sizeof(id) - id_size;
>>> +     struct flash_info *info;
>>> +
>>> +     /*
>>> +      * JEDEC also defines an optional "extended device information"
>>> +      * string for after vendor-specific data, after the three bytes
>>> +      * we use here.  Supporting some chips might require using it.
>>> +      *
>>> +      * If the vendor ID isn't Atmel's (0x1f), assume this call failed.
>>> +      * That's not an error; only rev C and newer chips handle it, and
>>> +      * only Atmel sells these chips.
>>> +      */
>>> +     ret = spi_write_then_read(spi, &code, 1, &id[first_byte], id_size);
>>
>> Hm, looking at this again, what prevents you from reading these bytes
>> into &id[0] ? Might be easier, ie.
>>
>> ret = spi_write_then_read(spi, &code, 1, id, id_size);
>>
> 
> Then I'd have to add 6 more '0's to each of the IDs in dataflash_data,
> which would make that code even less readable.

Or do >>= 24 on the jedec ID :)

>>> +     if (ret < 0) {
>>> +             pr_debug("%s: error %d reading JEDEC ID\n",
>>> +                     dev_name(&spi->dev), ret);
>>> +             return ERR_PTR(ret);
>>> +     }
>>> +
>>> +     if (id[first_byte] != CFI_MFR_ATMEL)
>>
>> if (id[0] != CFI_MFR_ATMEL)
>>
>>> +             return NULL;
>>> +
>>> +     jedec = be64_to_cpup((__be64 *)id);
>>> +
>>> +     info = jedec_lookup(spi, jedec);
>>
>> ... define these somewhere at the beginning of the dataflash code ...
>> #define DATAFLASH_MASK_STD      GENMASK(63, 40)
>> #define DATAFLASH_MASK_EXT      GENMASK(63, 16)
>>
>> ... then ...
>> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_EXT);
>>
>>> +     if (info)
>>> +             return info;
>>> +
>>> +     /*
>>> +      * Clear extended id bits (bits 0 through 15) and try to find
>>> +      * a match again in case our chip does not support returning
>>> +      * extended ID information and we got invalid bits instead
>>> +      */
>>> +     jedec &= GENMASK_ULL(63, 16);
>>
>> info = jedec_lookup(spi, jedec & DATAFLASH_MASK_STD);
>>
> 
> Sure, OK.
> 
> Thanks,
> Andrey Smirnov
> 


-- 
Best regards,
Marek Vasut

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web