LinuxLists.cc - [PATCH 0/4] Add new series Micron SPI NAND devices

[permalink] [raw]

Subject: [PATCH 2/4] mtd: spinand: Add new Micron SPI NAND devices

From: Shivamurthy Shastri <[email protected]>

Add device table for M79A and M78A series Micron SPI NAND devices.

Signed-off-by: Shivamurthy Shastri <[email protected]>
---
drivers/mtd/nand/spi/micron.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index c028d0d7e236..5fd1f921ef12 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -91,6 +91,7 @@ static int micron_8_ecc_get_status(struct spinand_device *spinand,
}

static const struct spinand_info micron_spinand_table[] = {
+ /* M79A 2Gb 3.3V */
SPINAND_INFO("MT29F2G01ABAGD", 0x24,
NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
NAND_ECCREQ(8, 512),
@@ -100,6 +101,36 @@ static const struct spinand_info micron_spinand_table[] = {
0,
SPINAND_ECCINFO(&micron_8_ooblayout,
micron_8_ecc_get_status)),
+ /* M79A 2Gb 1.8V */
+ SPINAND_INFO("MT29F2G01ABBGD", 0x25,
+ NAND_MEMORG(1, 2048, 128, 64, 2048, 40, 2, 1, 1),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ 0,
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status)),
+ /* M78A 1Gb 3.3V */
+ SPINAND_INFO("MT29F1G01ABAFD", 0x14,
+ NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ 0,
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status)),
+ /* M78A 1Gb 1.8V */
+ SPINAND_INFO("MT29F1G01ABAFD", 0x15,
+ NAND_MEMORG(1, 2048, 128, 64, 1024, 20, 1, 1, 1),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ 0,
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status)),
};

static int micron_spinand_detect(struct spinand_device *spinand)
--
2.17.1

2020-01-19 15:00:03

[permalink] [raw]

Subject: [PATCH 1/4] mtd: spinand: Generalize the OOB layout structure and function names

From: Shivamurthy Shastri <[email protected]>

In order to add new Micron SPI NAND devices, we generalized the OOB
layout structure and function names.

Signed-off-by: Shivamurthy Shastri <[email protected]>
---
drivers/mtd/nand/spi/micron.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 7d7b1f7fcf71..c028d0d7e236 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -34,38 +34,38 @@ static SPINAND_OP_VARIANTS(update_cache_variants,
SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
SPINAND_PROG_LOAD(false, 0, NULL, 0));

-static int mt29f2g01abagd_ooblayout_ecc(struct mtd_info *mtd, int section,
- struct mtd_oob_region *region)
+static int micron_8_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *region)
{
if (section)
return -ERANGE;

- region->offset = 64;
- region->length = 64;
+ region->offset = mtd->oobsize / 2;
+ region->length = mtd->oobsize / 2;

return 0;
}

-static int mt29f2g01abagd_ooblayout_free(struct mtd_info *mtd, int section,
- struct mtd_oob_region *region)
+static int micron_8_ooblayout_free(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *region)
{
if (section)
return -ERANGE;

/* Reserve 2 bytes for the BBM. */
region->offset = 2;
- region->length = 62;
+ region->length = (mtd->oobsize / 2) - 2;

return 0;
}

-static const struct mtd_ooblayout_ops mt29f2g01abagd_ooblayout = {
- .ecc = mt29f2g01abagd_ooblayout_ecc,
- .free = mt29f2g01abagd_ooblayout_free,
+static const struct mtd_ooblayout_ops micron_8_ooblayout = {
+ .ecc = micron_8_ooblayout_ecc,
+ .free = micron_8_ooblayout_free,
};

-static int mt29f2g01abagd_ecc_get_status(struct spinand_device *spinand,
- u8 status)
+static int micron_8_ecc_get_status(struct spinand_device *spinand,
+ u8 status)
{
switch (status & MICRON_STATUS_ECC_MASK) {
case STATUS_ECC_NO_BITFLIPS:
@@ -98,8 +98,8 @@ static const struct spinand_info micron_spinand_table[] = {
&write_cache_variants,
&update_cache_variants),
0,
- SPINAND_ECCINFO(&mt29f2g01abagd_ooblayout,
- mt29f2g01abagd_ecc_get_status)),
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status)),
};

static int micron_spinand_detect(struct spinand_device *spinand)
--
2.17.1

2020-01-19 15:01:00

[permalink] [raw]

Subject: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices

From: Shivamurthy Shastri <[email protected]>

Add device table for M70A series Micron SPI NAND devices.

While at it, disable the Continuous Read feature which is enabled by
default.

Signed-off-by: Shivamurthy Shastri <[email protected]>
---
drivers/mtd/nand/spi/micron.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 5fd1f921ef12..45fc37c58f8a 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -131,6 +131,26 @@ static const struct spinand_info micron_spinand_table[] = {
0,
SPINAND_ECCINFO(&micron_8_ooblayout,
micron_8_ecc_get_status)),
+ /* M70A 4Gb 3.3V */
+ SPINAND_INFO("MT29F4G01ABAFD", 0x34,
+ NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ 0,
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status)),
+ /* M70A 4Gb 1.8V */
+ SPINAND_INFO("MT29F4G01ABBFD", 0x35,
+ NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ 0,
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status)),
};

static int micron_spinand_detect(struct spinand_device *spinand)
@@ -153,8 +173,19 @@ static int micron_spinand_detect(struct spinand_device *spinand)
return 1;
}

+static int micron_spinand_init(struct spinand_device *spinand)
+{
+ /*
+ * M70A device series enable Continuous Read feature at Power-up,
+ * which is not supported. Disable this bit to avoid any possible
+ * failure.
+ */
+ return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
+}
+
static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
.detect = micron_spinand_detect,
+ .init = micron_spinand_init,
};

const struct spinand_manufacturer micron_spinand_manufacturer = {
--
2.17.1

2020-01-19 15:01:15

[permalink] [raw]

Subject: [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies

From: Shivamurthy Shastri <[email protected]>

Add device table for new Micron SPI NAND devices, which have multiple
dies. While at it, add support to select the die.

Signed-off-by: Shivamurthy Shastri <[email protected]>
---
drivers/mtd/nand/spi/micron.c | 50 +++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 45fc37c58f8a..03b486843210 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -18,6 +18,8 @@
#define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
#define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)

+#define MICRON_DIE_SELECTION_BIT 6
+
static SPINAND_OP_VARIANTS(read_cache_variants,
SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
@@ -64,6 +66,21 @@ static const struct mtd_ooblayout_ops micron_8_ooblayout = {
.free = micron_8_ooblayout_free,
};

+static int micron_select_target(struct spinand_device *spinand,
+ unsigned int target)
+{
+ struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
+ spinand->scratchbuf);
+
+ /*
+ * As per datasheet, die selection is done by the 6th bit of Die
+ * Select Register (Address 0xD0).
+ */
+ *spinand->scratchbuf = target << MICRON_DIE_SELECTION_BIT;
+
+ return spi_mem_exec_op(spinand->spimem, &op);
+}
+
static int micron_8_ecc_get_status(struct spinand_device *spinand,
u8 status)
{
@@ -131,6 +148,17 @@ static const struct spinand_info micron_spinand_table[] = {
0,
SPINAND_ECCINFO(&micron_8_ooblayout,
micron_8_ecc_get_status)),
+ /* M79A 4Gb 3.3V */
+ SPINAND_INFO("MT29F4G01ADAGD", 0x36,
+ NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ 0,
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status),
+ SPINAND_SELECT_TARGET(micron_select_target)),
/* M70A 4Gb 3.3V */
SPINAND_INFO("MT29F4G01ABAFD", 0x34,
NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
@@ -151,6 +179,28 @@ static const struct spinand_info micron_spinand_table[] = {
0,
SPINAND_ECCINFO(&micron_8_ooblayout,
micron_8_ecc_get_status)),
+ /* M70A 8Gb 3.3V */
+ SPINAND_INFO("MT29F8G01ADAFD", 0x46,
+ NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ 0,
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status),
+ SPINAND_SELECT_TARGET(micron_select_target)),
+ /* M70A 8Gb 1.8V */
+ SPINAND_INFO("MT29F8G01ADBFD", 0x47,
+ NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ 0,
+ SPINAND_ECCINFO(&micron_8_ooblayout,
+ micron_8_ecc_get_status),
+ SPINAND_SELECT_TARGET(micron_select_target)),
};

static int micron_spinand_detect(struct spinand_device *spinand)
--
2.17.1

2020-01-20 10:18:02

[permalink] [raw]

Subject: Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices

Hi Shiva,

This is remark common to the four patches: you miss the 'v2' prefix in
the object.

[email protected] wrote on Sun, 19 Jan 2020 15:54:31 +0100:

> From: Shivamurthy Shastri <[email protected]>
>
> Add device table for M70A series Micron SPI NAND devices.
>
> While at it, disable the Continuous Read feature which is enabled by
> default.

Can you please give us more detail on why this is an issue?

Shall we backport it to stable?

As a rule of thumb, when you start a sentence by "while at it" in a
commit message and this is not a trivial change : split the patch,
please. Unless this is really related and in this case explain how and
why in the commit message.

>
> Signed-off-by: Shivamurthy Shastri <[email protected]>
> ---
> drivers/mtd/nand/spi/micron.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 5fd1f921ef12..45fc37c58f8a 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -131,6 +131,26 @@ static const struct spinand_info micron_spinand_table[] = {
> 0,
> SPINAND_ECCINFO(&micron_8_ooblayout,
> micron_8_ecc_get_status)),
> + /* M70A 4Gb 3.3V */
> + SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> + NAND_ECCREQ(8, 512),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + 0,
> + SPINAND_ECCINFO(&micron_8_ooblayout,
> + micron_8_ecc_get_status)),
> + /* M70A 4Gb 1.8V */
> + SPINAND_INFO("MT29F4G01ABBFD", 0x35,
> + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> + NAND_ECCREQ(8, 512),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + 0,
> + SPINAND_ECCINFO(&micron_8_ooblayout,
> + micron_8_ecc_get_status)),
> };
>
> static int micron_spinand_detect(struct spinand_device *spinand)
> @@ -153,8 +173,19 @@ static int micron_spinand_detect(struct spinand_device *spinand)
> return 1;
> }
>
> +static int micron_spinand_init(struct spinand_device *spinand)
> +{
> + /*
> + * M70A device series enable Continuous Read feature at Power-up,
> + * which is not supported. Disable this bit to avoid any possible
> + * failure.
> + */
> + return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
> +}
> +
> static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
> .detect = micron_spinand_detect,
> + .init = micron_spinand_init,
> };
>
> const struct spinand_manufacturer micron_spinand_manufacturer = {

Thanks,
Miquèl

2020-01-20 10:23:54

[permalink] [raw]

Subject: Re: [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies

Hi Shiva,

[email protected] wrote on Sun, 19 Jan 2020 15:54:32 +0100:

> From: Shivamurthy Shastri <[email protected]>
>
> Add device table for new Micron SPI NAND devices, which have multiple
> dies. While at it, add support to select the die.

Same comment as in 3/4.

>
> Signed-off-by: Shivamurthy Shastri <[email protected]>
> ---
> drivers/mtd/nand/spi/micron.c | 50 +++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 45fc37c58f8a..03b486843210 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -18,6 +18,8 @@
> #define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
> #define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
>
> +#define MICRON_DIE_SELECTION_BIT 6
> +
> static SPINAND_OP_VARIANTS(read_cache_variants,
> SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> @@ -64,6 +66,21 @@ static const struct mtd_ooblayout_ops micron_8_ooblayout = {
> .free = micron_8_ooblayout_free,
> };
>
> +static int micron_select_target(struct spinand_device *spinand,
> + unsigned int target)
> +{
> + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
> + spinand->scratchbuf);
> +
> + /*
> + * As per datasheet, die selection is done by the 6th bit of Die
> + * Select Register (Address 0xD0).
> + */

I would put this comment close to the macro definition.

> + *spinand->scratchbuf = target << MICRON_DIE_SELECTION_BIT;

Either target is or or 1 and you can use the BIT macro, or you suppose
it can go higher and the _BIT suffix does not fit. _SHIFT would work
and creating a macro directly would be even better.

> +
> + return spi_mem_exec_op(spinand->spimem, &op);
> +}
> +

Where is this function used?

> static int micron_8_ecc_get_status(struct spinand_device *spinand,
> u8 status)
> {
> @@ -131,6 +148,17 @@ static const struct spinand_info micron_spinand_table[] = {
> 0,
> SPINAND_ECCINFO(&micron_8_ooblayout,
> micron_8_ecc_get_status)),
> + /* M79A 4Gb 3.3V */
> + SPINAND_INFO("MT29F4G01ADAGD", 0x36,
> + NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
> + NAND_ECCREQ(8, 512),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + 0,
> + SPINAND_ECCINFO(&micron_8_ooblayout,
> + micron_8_ecc_get_status),
> + SPINAND_SELECT_TARGET(micron_select_target)),
> /* M70A 4Gb 3.3V */
> SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> @@ -151,6 +179,28 @@ static const struct spinand_info micron_spinand_table[] = {
> 0,
> SPINAND_ECCINFO(&micron_8_ooblayout,
> micron_8_ecc_get_status)),
> + /* M70A 8Gb 3.3V */
> + SPINAND_INFO("MT29F8G01ADAFD", 0x46,
> + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> + NAND_ECCREQ(8, 512),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + 0,
> + SPINAND_ECCINFO(&micron_8_ooblayout,
> + micron_8_ecc_get_status),
> + SPINAND_SELECT_TARGET(micron_select_target)),
> + /* M70A 8Gb 1.8V */
> + SPINAND_INFO("MT29F8G01ADBFD", 0x47,
> + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> + NAND_ECCREQ(8, 512),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> + &write_cache_variants,
> + &update_cache_variants),
> + 0,
> + SPINAND_ECCINFO(&micron_8_ooblayout,
> + micron_8_ecc_get_status),
> + SPINAND_SELECT_TARGET(micron_select_target)),
> };
>
> static int micron_spinand_detect(struct spinand_device *spinand)

Thanks,
Miquèl

2020-01-21 12:24:29

[permalink] [raw]

Subject: RE: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices

Hi Miquel,

>
> Hi Shiva,
>
> This is remark common to the four patches: you miss the 'v2' prefix in
> the object.
>

Sorry for this mistake.
I recognized this after sending out the patches.

> [email protected] wrote on Sun, 19 Jan 2020 15:54:31 +0100:
>
> > From: Shivamurthy Shastri <[email protected]>
> >
> > Add device table for M70A series Micron SPI NAND devices.
> >
> > While at it, disable the Continuous Read feature which is enabled by
> > default.
>
> Can you please give us more detail on why this is an issue?

"Continuous Read" is the new feature added by the Micron for
M70A series devices. If this feature is enabled, the READ command
doesn't output the OOB area. The following short description
describes this feature.

Description:
If the Continuous Read feature is enabled, the device provides
the capability to read the whole block with a single command.
However, the read command doesn't output the OOB area.

Read command behavior (if Continuous Read enabled):
The READ CACHE command doesn't require the starting column address.
The device always output the data starting from the first column of the
cache register, and once the end of the cache register reached, the data
output continues through the next page. With the continuous read mode,
it is possible to read out the entire block using a single READ command, and
once the end of the block reached, the output pins become High-Z state.

>
> Shall we backport it to stable?

This is not a bug fix and applicable only to M70A series devices, there is no
need to backport.
(FYI, the previously enabled device was M79A series)

>
> As a rule of thumb, when you start a sentence by "while at it" in a
> commit message and this is not a trivial change : split the patch,
> please. Unless this is really related and in this case explain how and
> why in the commit message.

Okay, I will explain in my next version.

>
> >
> > Signed-off-by: Shivamurthy Shastri <[email protected]>
> > ---
> > drivers/mtd/nand/spi/micron.c | 31
> +++++++++++++++++++++++++++++++
> > 1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/spi/micron.c
> b/drivers/mtd/nand/spi/micron.c
> > index 5fd1f921ef12..45fc37c58f8a 100644
> > --- a/drivers/mtd/nand/spi/micron.c
> > +++ b/drivers/mtd/nand/spi/micron.c
> > @@ -131,6 +131,26 @@ static const struct spinand_info
> micron_spinand_table[] = {
> > 0,
> > SPINAND_ECCINFO(&micron_8_ooblayout,
> > micron_8_ecc_get_status)),
> > + /* M70A 4Gb 3.3V */
> > + SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > + NAND_ECCREQ(8, 512),
> > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > + &write_cache_variants,
> > + &update_cache_variants),
> > + 0,
> > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > + micron_8_ecc_get_status)),
> > + /* M70A 4Gb 1.8V */
> > + SPINAND_INFO("MT29F4G01ABBFD", 0x35,
> > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > + NAND_ECCREQ(8, 512),
> > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > + &write_cache_variants,
> > + &update_cache_variants),
> > + 0,
> > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > + micron_8_ecc_get_status)),
> > };
> >
> > static int micron_spinand_detect(struct spinand_device *spinand)
> > @@ -153,8 +173,19 @@ static int micron_spinand_detect(struct
> spinand_device *spinand)
> > return 1;
> > }
> >
> > +static int micron_spinand_init(struct spinand_device *spinand)
> > +{
> > + /*
> > + * M70A device series enable Continuous Read feature at Power-up,
> > + * which is not supported. Disable this bit to avoid any possible
> > + * failure.
> > + */
> > + return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
> > +}
> > +
> > static const struct spinand_manufacturer_ops
> micron_spinand_manuf_ops = {
> > .detect = micron_spinand_detect,
> > + .init = micron_spinand_init,
> > };
> >
> > const struct spinand_manufacturer micron_spinand_manufacturer = {
>
> Thanks,
> Miqu?l

Thanks,
Shiva

2020-01-21 12:25:34

[permalink] [raw]

Subject: RE: [EXT] Re: [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies

Hi Miquel,

>
> Hi Shiva,
>
> [email protected] wrote on Sun, 19 Jan 2020 15:54:32 +0100:
>
> > From: Shivamurthy Shastri <[email protected]>
> >
> > Add device table for new Micron SPI NAND devices, which have multiple
> > dies. While at it, add support to select the die.
>
> Same comment as in 3/4.

I will correct the comment.

>
> >
> > Signed-off-by: Shivamurthy Shastri <[email protected]>
> > ---
> > drivers/mtd/nand/spi/micron.c | 50
> +++++++++++++++++++++++++++++++++++
> > 1 file changed, 50 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/spi/micron.c
> b/drivers/mtd/nand/spi/micron.c
> > index 45fc37c58f8a..03b486843210 100644
> > --- a/drivers/mtd/nand/spi/micron.c
> > +++ b/drivers/mtd/nand/spi/micron.c
> > @@ -18,6 +18,8 @@
> > #define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
> > #define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
> >
> > +#define MICRON_DIE_SELECTION_BIT 6
> > +
> > static SPINAND_OP_VARIANTS(read_cache_variants,
> > SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2,
> NULL, 0),
> > SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> > @@ -64,6 +66,21 @@ static const struct mtd_ooblayout_ops
> micron_8_ooblayout = {
> > .free = micron_8_ooblayout_free,
> > };
> >
> > +static int micron_select_target(struct spinand_device *spinand,
> > + unsigned int target)
> > +{
> > + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
> > + spinand->scratchbuf);
> > +
> > + /*
> > + * As per datasheet, die selection is done by the 6th bit of Die
> > + * Select Register (Address 0xD0).
> > + */
>
> I would put this comment close to the macro definition.

Sure, I will do it.

>
> > + *spinand->scratchbuf = target << MICRON_DIE_SELECTION_BIT;
>
> Either target is or or 1 and you can use the BIT macro, or you suppose
> it can go higher and the _BIT suffix does not fit. _SHIFT would work
> and creating a macro directly would be even better.
>

I will create macro directly and send the code in next version.

> > +
> > + return spi_mem_exec_op(spinand->spimem, &op);
> > +}
> > +
>
> Where is this function used?

IIUC your question, the function is used below in device table.
The line is something like,

SPINAND_SELECT_TARGET(micron_select_target))

for all the devices with multiple dies.

>
> > static int micron_8_ecc_get_status(struct spinand_device *spinand,
> > u8 status)
> > {
> > @@ -131,6 +148,17 @@ static const struct spinand_info
> micron_spinand_table[] = {
> > 0,
> > SPINAND_ECCINFO(&micron_8_ooblayout,
> > micron_8_ecc_get_status)),
> > + /* M79A 4Gb 3.3V */
> > + SPINAND_INFO("MT29F4G01ADAGD", 0x36,
> > + NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
> > + NAND_ECCREQ(8, 512),
> > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > + &write_cache_variants,
> > + &update_cache_variants),
> > + 0,
> > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > + micron_8_ecc_get_status),
> > + SPINAND_SELECT_TARGET(micron_select_target)),
> > /* M70A 4Gb 3.3V */
> > SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > @@ -151,6 +179,28 @@ static const struct spinand_info
> micron_spinand_table[] = {
> > 0,
> > SPINAND_ECCINFO(&micron_8_ooblayout,
> > micron_8_ecc_get_status)),
> > + /* M70A 8Gb 3.3V */
> > + SPINAND_INFO("MT29F8G01ADAFD", 0x46,
> > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> > + NAND_ECCREQ(8, 512),
> > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > + &write_cache_variants,
> > + &update_cache_variants),
> > + 0,
> > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > + micron_8_ecc_get_status),
> > + SPINAND_SELECT_TARGET(micron_select_target)),
> > + /* M70A 8Gb 1.8V */
> > + SPINAND_INFO("MT29F8G01ADBFD", 0x47,
> > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> > + NAND_ECCREQ(8, 512),
> > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > + &write_cache_variants,
> > + &update_cache_variants),
> > + 0,
> > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > + micron_8_ecc_get_status),
> > + SPINAND_SELECT_TARGET(micron_select_target)),
> > };
> >
> > static int micron_spinand_detect(struct spinand_device *spinand)
>
>
>
>
> Thanks,
> Miqu?l

Thanks,
Shiva

2020-01-21 13:41:46

[permalink] [raw]

Subject: Re: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices

Hi Shivamurthy,

"Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote on
Tue, 21 Jan 2020 12:23:20 +0000:

> Hi Miquel,
>
> >
> > Hi Shiva,
> >
> > This is remark common to the four patches: you miss the 'v2' prefix in
> > the object.
> >
>
> Sorry for this mistake.
> I recognized this after sending out the patches.
>
> > [email protected] wrote on Sun, 19 Jan 2020 15:54:31 +0100:
> >
> > > From: Shivamurthy Shastri <[email protected]>
> > >
> > > Add device table for M70A series Micron SPI NAND devices.
> > >
> > > While at it, disable the Continuous Read feature which is enabled by
> > > default.
> >
> > Can you please give us more detail on why this is an issue?
>
> "Continuous Read" is the new feature added by the Micron for
> M70A series devices. If this feature is enabled, the READ command
> doesn't output the OOB area. The following short description
> describes this feature.
>
> Description:
> If the Continuous Read feature is enabled, the device provides
> the capability to read the whole block with a single command.
> However, the read command doesn't output the OOB area.
>
> Read command behavior (if Continuous Read enabled):
> The READ CACHE command doesn't require the starting column address.
> The device always output the data starting from the first column of the
> cache register, and once the end of the cache register reached, the data
> output continues through the next page. With the continuous read mode,
> it is possible to read out the entire block using a single READ command, and
> once the end of the block reached, the output pins become High-Z state.

Ok I understand better. In this case there is no need to split this
commit, instead just reword the commit log to something like:

--->8---
Add device table for M70A series Micron SPI-NAND devices.

As opposed to the M60A series already supported, M70A parts have the
"Continuous Read" feature enabled by default which does not fit the
subsystem needs.

<here explain the feature>.

Hence, we disable the feature at probe time.
---8<---

However, below, you disable this bit for all the parts. Is this really
ok? Souldn't we make it more specific to this series?

>
> >
> > Shall we backport it to stable?
>
> This is not a bug fix and applicable only to M70A series devices, there is no
> need to backport.
> (FYI, the previously enabled device was M79A series)
>
> >
> > As a rule of thumb, when you start a sentence by "while at it" in a
> > commit message and this is not a trivial change : split the patch,
> > please. Unless this is really related and in this case explain how and
> > why in the commit message.
>
> Okay, I will explain in my next version.
>
> >
> > >
> > > Signed-off-by: Shivamurthy Shastri <[email protected]>
> > > ---
> > > drivers/mtd/nand/spi/micron.c | 31
> > +++++++++++++++++++++++++++++++
> > > 1 file changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/mtd/nand/spi/micron.c
> > b/drivers/mtd/nand/spi/micron.c
> > > index 5fd1f921ef12..45fc37c58f8a 100644
> > > --- a/drivers/mtd/nand/spi/micron.c
> > > +++ b/drivers/mtd/nand/spi/micron.c
> > > @@ -131,6 +131,26 @@ static const struct spinand_info
> > micron_spinand_table[] = {
> > > 0,
> > > SPINAND_ECCINFO(&micron_8_ooblayout,
> > > micron_8_ecc_get_status)),
> > > + /* M70A 4Gb 3.3V */
> > > + SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > + NAND_ECCREQ(8, 512),
> > > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > + &write_cache_variants,
> > > + &update_cache_variants),
> > > + 0,
> > > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > > + micron_8_ecc_get_status)),
> > > + /* M70A 4Gb 1.8V */
> > > + SPINAND_INFO("MT29F4G01ABBFD", 0x35,
> > > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > + NAND_ECCREQ(8, 512),
> > > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > + &write_cache_variants,
> > > + &update_cache_variants),
> > > + 0,
> > > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > > + micron_8_ecc_get_status)),
> > > };
> > >
> > > static int micron_spinand_detect(struct spinand_device *spinand)
> > > @@ -153,8 +173,19 @@ static int micron_spinand_detect(struct
> > spinand_device *spinand)
> > > return 1;
> > > }
> > >
> > > +static int micron_spinand_init(struct spinand_device *spinand)
> > > +{
> > > + /*
> > > + * M70A device series enable Continuous Read feature at Power-up,
> > > + * which is not supported. Disable this bit to avoid any possible
> > > + * failure.
> > > + */
> > > + return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
> > > +}
> > > +
> > > static const struct spinand_manufacturer_ops
> > micron_spinand_manuf_ops = {
> > > .detect = micron_spinand_detect,
> > > + .init = micron_spinand_init,
> > > };
> > >
> > > const struct spinand_manufacturer micron_spinand_manufacturer = {
> >
> > Thanks,
> > Miquèl
>
> Thanks,
> Shiva

Thanks,
Miquèl

2020-01-21 13:45:12

[permalink] [raw]

Subject: Re: [EXT] Re: [PATCH 4/4] mtd: spinand: Add new Micron SPI NAND devices with multiple dies

Hi Shivamurthy,

"Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote on
Tue, 21 Jan 2020 12:23:36 +0000:

> Hi Miquel,
>
> >
> > Hi Shiva,
> >
> > [email protected] wrote on Sun, 19 Jan 2020 15:54:32 +0100:
> >
> > > From: Shivamurthy Shastri <[email protected]>
> > >
> > > Add device table for new Micron SPI NAND devices, which have multiple
> > > dies. While at it, add support to select the die.
> >
> > Same comment as in 3/4.
>
> I will correct the comment.

Actually now with more explanation I understand better. Please
keep in mind that anybody not knowing what you do on a daily basis
should understand what this commit does and why.

So like before, you actually don't need to split this patch, but
instead rework the commit message.

>
> >
> > >
> > > Signed-off-by: Shivamurthy Shastri <[email protected]>
> > > ---
> > > drivers/mtd/nand/spi/micron.c | 50
> > +++++++++++++++++++++++++++++++++++
> > > 1 file changed, 50 insertions(+)
> > >
> > > diff --git a/drivers/mtd/nand/spi/micron.c
> > b/drivers/mtd/nand/spi/micron.c
> > > index 45fc37c58f8a..03b486843210 100644
> > > --- a/drivers/mtd/nand/spi/micron.c
> > > +++ b/drivers/mtd/nand/spi/micron.c
> > > @@ -18,6 +18,8 @@
> > > #define MICRON_STATUS_ECC_4TO6_BITFLIPS (3 << 4)
> > > #define MICRON_STATUS_ECC_7TO8_BITFLIPS (5 << 4)
> > >
> > > +#define MICRON_DIE_SELECTION_BIT 6
> > > +
> > > static SPINAND_OP_VARIANTS(read_cache_variants,
> > > SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2,
> > NULL, 0),
> > > SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> > > @@ -64,6 +66,21 @@ static const struct mtd_ooblayout_ops
> > micron_8_ooblayout = {
> > > .free = micron_8_ooblayout_free,
> > > };
> > >
> > > +static int micron_select_target(struct spinand_device *spinand,
> > > + unsigned int target)
> > > +{
> > > + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(0xd0,
> > > + spinand->scratchbuf);
> > > +
> > > + /*
> > > + * As per datasheet, die selection is done by the 6th bit of Die
> > > + * Select Register (Address 0xD0).
> > > + */
> >
> > I would put this comment close to the macro definition.
>
> Sure, I will do it.
>
> >
> > > + *spinand->scratchbuf = target << MICRON_DIE_SELECTION_BIT;
> >
> > Either target is or or 1 and you can use the BIT macro, or you suppose
> > it can go higher and the _BIT suffix does not fit. _SHIFT would work
> > and creating a macro directly would be even better.
> >
>
> I will create macro directly and send the code in next version.
>
> > > +
> > > + return spi_mem_exec_op(spinand->spimem, &op);
> > > +}
> > > +
> >
> > Where is this function used?
>
> IIUC your question, the function is used below in device table.
> The line is something like,
>
> SPINAND_SELECT_TARGET(micron_select_target))

I just missed it :)

>
> for all the devices with multiple dies.
>
> >
> > > static int micron_8_ecc_get_status(struct spinand_device *spinand,
> > > u8 status)
> > > {
> > > @@ -131,6 +148,17 @@ static const struct spinand_info
> > micron_spinand_table[] = {
> > > 0,
> > > SPINAND_ECCINFO(&micron_8_ooblayout,
> > > micron_8_ecc_get_status)),
> > > + /* M79A 4Gb 3.3V */
> > > + SPINAND_INFO("MT29F4G01ADAGD", 0x36,
> > > + NAND_MEMORG(1, 2048, 128, 64, 2048, 80, 2, 1, 2),
> > > + NAND_ECCREQ(8, 512),
> > > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > + &write_cache_variants,
> > > + &update_cache_variants),
> > > + 0,
> > > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > > + micron_8_ecc_get_status),
> > > + SPINAND_SELECT_TARGET(micron_select_target)),
> > > /* M70A 4Gb 3.3V */
> > > SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > > NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > @@ -151,6 +179,28 @@ static const struct spinand_info
> > micron_spinand_table[] = {
> > > 0,
> > > SPINAND_ECCINFO(&micron_8_ooblayout,
> > > micron_8_ecc_get_status)),
> > > + /* M70A 8Gb 3.3V */
> > > + SPINAND_INFO("MT29F8G01ADAFD", 0x46,
> > > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> > > + NAND_ECCREQ(8, 512),
> > > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > + &write_cache_variants,
> > > + &update_cache_variants),
> > > + 0,
> > > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > > + micron_8_ecc_get_status),
> > > + SPINAND_SELECT_TARGET(micron_select_target)),
> > > + /* M70A 8Gb 1.8V */
> > > + SPINAND_INFO("MT29F8G01ADBFD", 0x47,
> > > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 2),
> > > + NAND_ECCREQ(8, 512),
> > > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > + &write_cache_variants,
> > > + &update_cache_variants),
> > > + 0,
> > > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > > + micron_8_ecc_get_status),
> > > + SPINAND_SELECT_TARGET(micron_select_target)),
> > > };
> > >
> > > static int micron_spinand_detect(struct spinand_device *spinand)
> >
> >
> >
> >
> > Thanks,
> > Miquèl
>
> Thanks,
> Shiva

Thanks,
Miquèl

2020-01-21 15:00:28

[permalink] [raw]

Subject: RE: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices

Hi Miquel,

>
> Hi Shivamurthy,
>
> "Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote
> on
> Tue, 21 Jan 2020 12:23:20 +0000:
>
> > Hi Miquel,
> >
> > >
> > > Hi Shiva,
> > >
> > > This is remark common to the four patches: you miss the 'v2' prefix in
> > > the object.
> > >
> >
> > Sorry for this mistake.
> > I recognized this after sending out the patches.
> >
> > > [email protected] wrote on Sun, 19 Jan 2020 15:54:31 +0100:
> > >
> > > > From: Shivamurthy Shastri <[email protected]>
> > > >
> > > > Add device table for M70A series Micron SPI NAND devices.
> > > >
> > > > While at it, disable the Continuous Read feature which is enabled by
> > > > default.
> > >
> > > Can you please give us more detail on why this is an issue?
> >
> > "Continuous Read" is the new feature added by the Micron for
> > M70A series devices. If this feature is enabled, the READ command
> > doesn't output the OOB area. The following short description
> > describes this feature.
> >
> > Description:
> > If the Continuous Read feature is enabled, the device provides
> > the capability to read the whole block with a single command.
> > However, the read command doesn't output the OOB area.
> >
> > Read command behavior (if Continuous Read enabled):
> > The READ CACHE command doesn't require the starting column address.
> > The device always output the data starting from the first column of the
> > cache register, and once the end of the cache register reached, the data
> > output continues through the next page. With the continuous read mode,
> > it is possible to read out the entire block using a single READ command, and
> > once the end of the block reached, the output pins become High-Z state.
>
> Ok I understand better. In this case there is no need to split this
> commit, instead just reword the commit log to something like:
>
> --->8---
> Add device table for M70A series Micron SPI-NAND devices.
>
> As opposed to the M60A series already supported, M70A parts have the
> "Continuous Read" feature enabled by default which does not fit the
> subsystem needs.
>
> <here explain the feature>.
>
> Hence, we disable the feature at probe time.
> ---8<---
>

Sure, I will change as per your suggestion.

> However, below, you disable this bit for all the parts. Is this really
> ok? Souldn't we make it more specific to this series?

It is ok because this bit is unused in other series.

>
> >
> > >
> > > Shall we backport it to stable?
> >
> > This is not a bug fix and applicable only to M70A series devices, there is no
> > need to backport.
> > (FYI, the previously enabled device was M79A series)
> >
> > >
> > > As a rule of thumb, when you start a sentence by "while at it" in a
> > > commit message and this is not a trivial change : split the patch,
> > > please. Unless this is really related and in this case explain how and
> > > why in the commit message.
> >
> > Okay, I will explain in my next version.
> >
> > >
> > > >
> > > > Signed-off-by: Shivamurthy Shastri <[email protected]>
> > > > ---
> > > > drivers/mtd/nand/spi/micron.c | 31
> > > +++++++++++++++++++++++++++++++
> > > > 1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/drivers/mtd/nand/spi/micron.c
> > > b/drivers/mtd/nand/spi/micron.c
> > > > index 5fd1f921ef12..45fc37c58f8a 100644
> > > > --- a/drivers/mtd/nand/spi/micron.c
> > > > +++ b/drivers/mtd/nand/spi/micron.c
> > > > @@ -131,6 +131,26 @@ static const struct spinand_info
> > > micron_spinand_table[] = {
> > > > 0,
> > > > SPINAND_ECCINFO(&micron_8_ooblayout,
> > > > micron_8_ecc_get_status)),
> > > > + /* M70A 4Gb 3.3V */
> > > > + SPINAND_INFO("MT29F4G01ABAFD", 0x34,
> > > > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > > + NAND_ECCREQ(8, 512),
> > > > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > > + &write_cache_variants,
> > > > + &update_cache_variants),
> > > > + 0,
> > > > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > > > + micron_8_ecc_get_status)),
> > > > + /* M70A 4Gb 1.8V */
> > > > + SPINAND_INFO("MT29F4G01ABBFD", 0x35,
> > > > + NAND_MEMORG(1, 4096, 256, 64, 2048, 40, 1, 1, 1),
> > > > + NAND_ECCREQ(8, 512),
> > > > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> > > > + &write_cache_variants,
> > > > + &update_cache_variants),
> > > > + 0,
> > > > + SPINAND_ECCINFO(&micron_8_ooblayout,
> > > > + micron_8_ecc_get_status)),
> > > > };
> > > >
> > > > static int micron_spinand_detect(struct spinand_device *spinand)
> > > > @@ -153,8 +173,19 @@ static int micron_spinand_detect(struct
> > > spinand_device *spinand)
> > > > return 1;
> > > > }
> > > >
> > > > +static int micron_spinand_init(struct spinand_device *spinand)
> > > > +{
> > > > + /*
> > > > + * M70A device series enable Continuous Read feature at Power-up,
> > > > + * which is not supported. Disable this bit to avoid any possible
> > > > + * failure.
> > > > + */
> > > > + return spinand_upd_cfg(spinand, CFG_QUAD_ENABLE, 0);
> > > > +}
> > > > +
> > > > static const struct spinand_manufacturer_ops
> > > micron_spinand_manuf_ops = {
> > > > .detect = micron_spinand_detect,
> > > > + .init = micron_spinand_init,
> > > > };
> > > >
> > > > const struct spinand_manufacturer micron_spinand_manufacturer = {
> > >

Thanks,
Shiva

2020-01-21 15:29:48

[permalink] [raw]

Subject: Re: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices

Hi Shivamurthy,

"Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote on
Tue, 21 Jan 2020 14:59:11 +0000:

> Hi Miquel,
>
> >
> > Hi Shivamurthy,
> >
> > "Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote
> > on
> > Tue, 21 Jan 2020 12:23:20 +0000:
> >
> > > Hi Miquel,
> > >
> > > >
> > > > Hi Shiva,
> > > >
> > > > This is remark common to the four patches: you miss the 'v2' prefix in
> > > > the object.
> > > >
> > >
> > > Sorry for this mistake.
> > > I recognized this after sending out the patches.
> > >
> > > > [email protected] wrote on Sun, 19 Jan 2020 15:54:31 +0100:
> > > >
> > > > > From: Shivamurthy Shastri <[email protected]>
> > > > >
> > > > > Add device table for M70A series Micron SPI NAND devices.
> > > > >
> > > > > While at it, disable the Continuous Read feature which is enabled by
> > > > > default.
> > > >
> > > > Can you please give us more detail on why this is an issue?
> > >
> > > "Continuous Read" is the new feature added by the Micron for
> > > M70A series devices. If this feature is enabled, the READ command
> > > doesn't output the OOB area. The following short description
> > > describes this feature.
> > >
> > > Description:
> > > If the Continuous Read feature is enabled, the device provides
> > > the capability to read the whole block with a single command.
> > > However, the read command doesn't output the OOB area.
> > >
> > > Read command behavior (if Continuous Read enabled):
> > > The READ CACHE command doesn't require the starting column address.
> > > The device always output the data starting from the first column of the
> > > cache register, and once the end of the cache register reached, the data
> > > output continues through the next page. With the continuous read mode,
> > > it is possible to read out the entire block using a single READ command, and
> > > once the end of the block reached, the output pins become High-Z state.
> >
> > Ok I understand better. In this case there is no need to split this
> > commit, instead just reword the commit log to something like:
> >
> > --->8---
> > Add device table for M70A series Micron SPI-NAND devices.
> >
> > As opposed to the M60A series already supported, M70A parts have the
> > "Continuous Read" feature enabled by default which does not fit the
> > subsystem needs.
> >
> > <here explain the feature>.
> >
> > Hence, we disable the feature at probe time.
> > ---8<---
> >
>
> Sure, I will change as per your suggestion.
>
> > However, below, you disable this bit for all the parts. Is this really
> > ok? Souldn't we make it more specific to this series?
>
> It is ok because this bit is unused in other series.

I would rather prefer to avoid this kind of arrangement. Please change
this bit only for the series which has it.

Thanks,
Miquèl

2020-01-23 09:04:06

[permalink] [raw]

Subject: RE: [EXT] Re: [PATCH 3/4] mtd: spinand: Add M70A series Micron SPI NAND devices

Hi Miquel,

>
> Hi Shivamurthy,
>
> "Shivamurthy Shastri (sshivamurthy)" <[email protected]> wrote
> on
> Tue, 21 Jan 2020 14:59:11 +0000:
>
> > Hi Miquel,
> >
> > >
> > > Hi Shivamurthy,
> > >
> > > "Shivamurthy Shastri (sshivamurthy)" <[email protected]>
> wrote
> > > on
> > > Tue, 21 Jan 2020 12:23:20 +0000:
> > >
> > > > Hi Miquel,
> > > >
> > > > >
> > > > > Hi Shiva,
> > > > >
> > > > > This is remark common to the four patches: you miss the 'v2' prefix in
> > > > > the object.
> > > > >
> > > >
> > > > Sorry for this mistake.
> > > > I recognized this after sending out the patches.
> > > >
> > > > > [email protected] wrote on Sun, 19 Jan 2020 15:54:31
> +0100:
> > > > >
> > > > > > From: Shivamurthy Shastri <[email protected]>
> > > > > >
> > > > > > Add device table for M70A series Micron SPI NAND devices.
> > > > > >
> > > > > > While at it, disable the Continuous Read feature which is enabled by
> > > > > > default.
> > > > >
> > > > > Can you please give us more detail on why this is an issue?
> > > >
> > > > "Continuous Read" is the new feature added by the Micron for
> > > > M70A series devices. If this feature is enabled, the READ command
> > > > doesn't output the OOB area. The following short description
> > > > describes this feature.
> > > >
> > > > Description:
> > > > If the Continuous Read feature is enabled, the device provides
> > > > the capability to read the whole block with a single command.
> > > > However, the read command doesn't output the OOB area.
> > > >
> > > > Read command behavior (if Continuous Read enabled):
> > > > The READ CACHE command doesn't require the starting column
> address.
> > > > The device always output the data starting from the first column of the
> > > > cache register, and once the end of the cache register reached, the
> data
> > > > output continues through the next page. With the continuous read
> mode,
> > > > it is possible to read out the entire block using a single READ command,
> and
> > > > once the end of the block reached, the output pins become High-Z
> state.
> > >
> > > Ok I understand better. In this case there is no need to split this
> > > commit, instead just reword the commit log to something like:
> > >
> > > --->8---
> > > Add device table for M70A series Micron SPI-NAND devices.
> > >
> > > As opposed to the M60A series already supported, M70A parts have the
> > > "Continuous Read" feature enabled by default which does not fit the
> > > subsystem needs.
> > >
> > > <here explain the feature>.
> > >
> > > Hence, we disable the feature at probe time.
> > > ---8<---
> > >
> >
> > Sure, I will change as per your suggestion.
> >
> > > However, below, you disable this bit for all the parts. Is this really
> > > ok? Souldn't we make it more specific to this series?
> >
> > It is ok because this bit is unused in other series.
>
> I would rather prefer to avoid this kind of arrangement. Please change
> this bit only for the series which has it.

There is no other way to identify the series of the device except by using the
"flags" from the structure spinand_info. So I will proceed with this approach.

1. In spinand.h --> I will create macro

#define SPINAND_HAS_CR_FEAT_BIT BIT(1)

2. Use this flag inside micron_spinand_init function to identify the device.

>
>
> Thanks,
> Miquèl

Thanks,
Shiva

2020-01-30 08:32:05