No functional change intended.
Cc: [email protected]
Cc: David Woodhouse <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Cyrille Pitchen <[email protected]>
Cc: [email protected]
Acked-by: Marek Vasut <[email protected]>
Signed-off-by: Andrey Smirnov <[email protected]>
---
Changes since [v2]:
- Re-worded commit message
- Collected Acked-by from Marek
Not present in v1
[v2]: http://lkml.kernel.org/r/[email protected]
drivers/mtd/devices/mtd_dataflash.c | 40 ++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index f9e9bd1..a566231 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -84,7 +84,7 @@
struct dataflash {
- uint8_t command[4];
+ u8 command[4];
char name[24];
unsigned short page_offset; /* offset in flash address */
@@ -153,8 +153,8 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
struct spi_transfer x = { };
struct spi_message msg;
unsigned blocksize = priv->page_size << 3;
- uint8_t *command;
- uint32_t rem;
+ u8 *command;
+ u32 rem;
pr_debug("%s: erase addr=0x%llx len 0x%llx\n",
dev_name(&spi->dev), (long long)instr->addr,
@@ -187,8 +187,8 @@ static int dataflash_erase(struct mtd_info *mtd, struct erase_info *instr)
pageaddr = pageaddr << priv->page_offset;
command[0] = do_block ? OP_ERASE_BLOCK : OP_ERASE_PAGE;
- command[1] = (uint8_t)(pageaddr >> 16);
- command[2] = (uint8_t)(pageaddr >> 8);
+ command[1] = (u8)(pageaddr >> 16);
+ command[2] = (u8)(pageaddr >> 8);
command[3] = 0;
pr_debug("ERASE %s: (%x) %x %x %x [%i]\n",
@@ -239,7 +239,7 @@ static int dataflash_read(struct mtd_info *mtd, loff_t from, size_t len,
struct spi_transfer x[2] = { };
struct spi_message msg;
unsigned int addr;
- uint8_t *command;
+ u8 *command;
int status;
pr_debug("%s: read 0x%x..0x%x\n", dev_name(&priv->spi->dev),
@@ -271,9 +271,9 @@ static int dataflash_read(struct mtd_info *mtd, loff_t from, size_t len,
* fewer "don't care" bytes. Both buffers stay unchanged.
*/
command[0] = OP_READ_CONTINUOUS;
- command[1] = (uint8_t)(addr >> 16);
- command[2] = (uint8_t)(addr >> 8);
- command[3] = (uint8_t)(addr >> 0);
+ command[1] = (u8)(addr >> 16);
+ command[2] = (u8)(addr >> 8);
+ command[3] = (u8)(addr >> 0);
/* plus 4 "don't care" bytes */
status = spi_sync(priv->spi, &msg);
@@ -308,7 +308,7 @@ static int dataflash_write(struct mtd_info *mtd, loff_t to, size_t len,
size_t remaining = len;
u_char *writebuf = (u_char *) buf;
int status = -EINVAL;
- uint8_t *command;
+ u8 *command;
pr_debug("%s: write 0x%x..0x%x\n",
dev_name(&spi->dev), (unsigned)to, (unsigned)(to + len));
@@ -455,11 +455,11 @@ static int dataflash_get_otp_info(struct mtd_info *mtd, size_t len,
}
static ssize_t otp_read(struct spi_device *spi, unsigned base,
- uint8_t *buf, loff_t off, size_t len)
+ u8 *buf, loff_t off, size_t len)
{
struct spi_message m;
size_t l;
- uint8_t *scratch;
+ u8 *scratch;
struct spi_transfer t;
int status;
@@ -538,7 +538,7 @@ static int dataflash_write_user_otp(struct mtd_info *mtd,
{
struct spi_message m;
const size_t l = 4 + 64;
- uint8_t *scratch;
+ u8 *scratch;
struct spi_transfer t;
struct dataflash *priv = mtd->priv;
int status;
@@ -689,14 +689,14 @@ struct flash_info {
/* JEDEC id has a high byte of zero plus three data bytes:
* the manufacturer id, then a two byte device id.
*/
- uint32_t jedec_id;
+ u32 jedec_id;
/* The size listed here is what works with OP_ERASE_PAGE. */
unsigned nr_pages;
- uint16_t pagesize;
- uint16_t pageoffset;
+ u16 pagesize;
+ u16 pageoffset;
- uint16_t flags;
+ u16 flags;
#define SUP_POW2PS 0x0002 /* supports 2^N byte pages */
#define IS_POW2PS 0x0001 /* uses 2^N byte pages */
};
@@ -739,9 +739,9 @@ static struct flash_info dataflash_data[] = {
static struct flash_info *jedec_probe(struct spi_device *spi)
{
int tmp;
- uint8_t code = OP_READ_ID;
- uint8_t id[3];
- uint32_t jedec;
+ u8 code = OP_READ_ID;
+ u8 id[3];
+ u32 jedec;
struct flash_info *info;
int status;
--
2.9.3
"For" loop in jedec_probe can be simplified to not need counter
'i'. Convert the code and get rid of the variable.
Cc: [email protected]
Cc: David Woodhouse <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Cyrille Pitchen <[email protected]>
Cc: [email protected]
Signed-off-by: Andrey Smirnov <[email protected]>
---
Not present in v2, v1.
drivers/mtd/devices/mtd_dataflash.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index ccd1e02..2d3e403 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -736,7 +736,7 @@ static struct flash_info dataflash_data[] = {
static struct flash_info *jedec_probe(struct spi_device *spi)
{
- int ret, i;
+ int ret;
u8 code = OP_READ_ID;
u8 id[3];
u32 jedec;
@@ -767,9 +767,9 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
jedec = jedec << 8;
jedec |= id[2];
- for (i = 0, info = dataflash_data;
- i < ARRAY_SIZE(dataflash_data);
- i++, info++) {
+ for (info = dataflash_data;
+ info < dataflash_data + ARRAY_SIZE(dataflash_data);
+ info++) {
if (info->jedec_id == jedec) {
dev_dbg(&spi->dev, "OTP, sector protect%s\n",
(info->flags & SUP_POW2PS) ?
--
2.9.3
Change the following:
- Replace indentation between type and name of local variable from
tabs to spaces
- Replace magic number 0x1F with CFI_MFR_ATMEL macro
- Replace variable 'tmp' with 'ret' and 'i' where appropriate
- Reformat multi-line comments and add newlines where appropriate
No functional change intended.
Cc: [email protected]
Cc: David Woodhouse <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Cyrille Pitchen <[email protected]>
Cc: [email protected]
Acked-by: Marek Vasut <[email protected]>
Signed-off-by: Andrey Smirnov <[email protected]>
---
Changes since [v2]:
- Re-worded commit message
- Collected Acked-by from Marek
Not present in v1
[v2] http://lkml.kernel.org/r/[email protected]
drivers/mtd/devices/mtd_dataflash.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index a566231..5b7a8c3 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -82,6 +82,7 @@
#define OP_WRITE_SECURITY_REVC 0x9A
#define OP_WRITE_SECURITY 0x9B /* revision D */
+#define CFI_MFR_ATMEL 0x1F
struct dataflash {
u8 command[4];
@@ -738,14 +739,15 @@ static struct flash_info dataflash_data[] = {
static struct flash_info *jedec_probe(struct spi_device *spi)
{
- int tmp;
- u8 code = OP_READ_ID;
- u8 id[3];
- u32 jedec;
- struct flash_info *info;
+ int ret, i;
+ u8 code = OP_READ_ID;
+ u8 id[3];
+ u32 jedec;
+ struct flash_info *info;
int status;
- /* JEDEC also defines an optional "extended device information"
+ /*
+ * 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.
*
@@ -753,13 +755,14 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
* That's not an error; only rev C and newer chips handle it, and
* only Atmel sells these chips.
*/
- tmp = spi_write_then_read(spi, &code, 1, id, 3);
- if (tmp < 0) {
+ ret = spi_write_then_read(spi, &code, 1, id, 3);
+ if (ret < 0) {
pr_debug("%s: error %d reading JEDEC ID\n",
- dev_name(&spi->dev), tmp);
- return ERR_PTR(tmp);
+ dev_name(&spi->dev), ret);
+ return ERR_PTR(ret);
}
- if (id[0] != 0x1f)
+
+ if (id[0] != CFI_MFR_ATMEL)
return NULL;
jedec = id[0];
@@ -768,9 +771,9 @@ static struct flash_info *jedec_probe(struct spi_device *spi)
jedec = jedec << 8;
jedec |= id[2];
- for (tmp = 0, info = dataflash_data;
- tmp < ARRAY_SIZE(dataflash_data);
- tmp++, info++) {
+ for (i = 0, info = dataflash_data;
+ i < ARRAY_SIZE(dataflash_data);
+ i++, info++) {
if (info->jedec_id == jedec) {
pr_debug("%s: OTP, sector protect%s\n",
dev_name(&spi->dev),
--
2.9.3
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: [email protected]
Cc: David Woodhouse <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Cyrille Pitchen <[email protected]>
Cc: [email protected]
Signed-off-by: Andrey Smirnov <[email protected]>
---
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/[email protected]
[v2] http://lkml.kernel.org/r/[email protected]
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
Cc: [email protected]
Cc: David Woodhouse <[email protected]>
Cc: Brian Norris <[email protected]>
Cc: Boris Brezillon <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Cyrille Pitchen <[email protected]>
Cc: [email protected]
Signed-off-by: Andrey Smirnov <[email protected]>
---
No changes between v3 through v1.
drivers/mtd/devices/mtd_dataflash.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/devices/mtd_dataflash.c b/drivers/mtd/devices/mtd_dataflash.c
index 941e8b7..c83f341 100644
--- a/drivers/mtd/devices/mtd_dataflash.c
+++ b/drivers/mtd/devices/mtd_dataflash.c
@@ -732,6 +732,9 @@ static struct flash_info dataflash_data[] = {
{ "AT45DB642x", 0x1f28000000, 8192, 1056, 11, SUP_POW2PS},
{ "at45db642d", 0x1f28000000, 8192, 1024, 10, SUP_POW2PS | IS_POW2PS},
+
+ { "AT45DB641E", 0x1f28000100, 32768, 264, 9, SUP_POW2PS},
+ { "at45db641e", 0x1f28000100, 32768, 256, 8, SUP_POW2PS | IS_POW2PS},
};
static struct flash_info *jedec_lookup(struct spi_device *spi, u64 jedec)
--
2.9.3
On Wed, 2017-04-19 at 08:23 -0700, Andrey Smirnov wrote:
> No functional change intended.
FWIW I *deliberately* used the proper C datatypes throughout all of the
MTD code instead of the silly kernel-speshul datatypes. To the extent
of *fixing* submissions to be written in C when they were using the
silly kernel types instead.
There is a reason for the __u32 type in userspace APIs. There is
basically no reason for 'u32' et al, just to save a trivial few
characters in the type name. We might as well be using the proper
language types.
I appreciate that not everyone agrees with that, but at least it used
to be consistent throughout the code I was maintaining. I'll defer to
the currently active maintainers though...
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: [email protected]
> Cc: David Woodhouse <[email protected]>
> Cc: Brian Norris <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Marek Vasut <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: Cyrille Pitchen <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andrey Smirnov <[email protected]>
> ---
>
> 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/[email protected]
> [v2] http://lkml.kernel.org/r/[email protected]
>
> 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
On 04/19/2017 05:23 PM, Andrey Smirnov wrote:
> "For" loop in jedec_probe can be simplified to not need counter
> 'i'. Convert the code and get rid of the variable.
>
> Cc: [email protected]
> Cc: David Woodhouse <[email protected]>
> Cc: Brian Norris <[email protected]>
> Cc: Boris Brezillon <[email protected]>
> Cc: Marek Vasut <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: Cyrille Pitchen <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andrey Smirnov <[email protected]>
> ---
Reviewed-by: Marek Vasut <[email protected]>
--
Best regards,
Marek Vasut
On Wed, Apr 19, 2017 at 8:42 AM, Marek Vasut <[email protected]> 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: [email protected]
>> Cc: David Woodhouse <[email protected]>
>> Cc: Brian Norris <[email protected]>
>> Cc: Boris Brezillon <[email protected]>
>> Cc: Marek Vasut <[email protected]>
>> Cc: Richard Weinberger <[email protected]>
>> Cc: Cyrille Pitchen <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Andrey Smirnov <[email protected]>
>> ---
>>
>> 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/[email protected]
>> [v2] http://lkml.kernel.org/r/[email protected]
>>
>> 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
[ +cc: Marek, since I accidentally dropped him off CC list ]
On Wed, Apr 19, 2017 at 8:35 AM, David Woodhouse <[email protected]> wrote:
> On Wed, 2017-04-19 at 08:23 -0700, Andrey Smirnov wrote:
>> No functional change intended.
>
> FWIW I *deliberately* used the proper C datatypes throughout all of the
> MTD code instead of the silly kernel-speshul datatypes. To the extent
> of *fixing* submissions to be written in C when they were using the
> silly kernel types instead.
>
> There is a reason for the __u32 type in userspace APIs. There is
> basically no reason for 'u32' et al, just to save a trivial few
> characters in the type name. We might as well be using the proper
> language types.
>
> I appreciate that not everyone agrees with that, but at least it used
> to be consistent throughout the code I was maintaining. I'll defer to
> the currently active maintainers though...
This conversion was an explicit request from Marek, see
https://marc.info/?l=linux-kernel&m=149192751625941
Thanks,
Andrey Smirnov
On 04/19/2017 06:01 PM, Andrey Smirnov wrote:
> On Wed, Apr 19, 2017 at 8:42 AM, Marek Vasut <[email protected]> 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: [email protected]
>>> Cc: David Woodhouse <[email protected]>
>>> Cc: Brian Norris <[email protected]>
>>> Cc: Boris Brezillon <[email protected]>
>>> Cc: Marek Vasut <[email protected]>
>>> Cc: Richard Weinberger <[email protected]>
>>> Cc: Cyrille Pitchen <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Andrey Smirnov <[email protected]>
>>> ---
>>>
>>> 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/[email protected]
>>> [v2] http://lkml.kernel.org/r/[email protected]
>>>
>>> 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
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 <[email protected]> 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: [email protected]
>>>> Cc: David Woodhouse <[email protected]>
>>>> Cc: Brian Norris <[email protected]>
>>>> Cc: Boris Brezillon <[email protected]>
>>>> Cc: Marek Vasut <[email protected]>
>>>> Cc: Richard Weinberger <[email protected]>
>>>> Cc: Cyrille Pitchen <[email protected]>
>>>> Cc: [email protected]
>>>> Signed-off-by: Andrey Smirnov <[email protected]>
>>>> ---
>>>>
>>>> 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/[email protected]
>>>> [v2] http://lkml.kernel.org/r/[email protected]
>>>>
>>>> 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