[PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[Matthias Schiffer]

All mt25q variants have the same features.

Unlike the smaller variants, no n25q with 2G exists, so we don't need to
match on the extended ID to distinguish n25q and mt25q series for these
models.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/mtd/spi-nor/micron-st.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index c224e59820a1..d5baa8762c8d 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -181,11 +181,11 @@ static const struct flash_info st_parts[] = {
 			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
 			      NO_CHIP_ERASE) },
 	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
-			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
-			      NO_CHIP_ERASE) },
+			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
+			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
 	{ "mt25qu02g",   INFO(0x20bb22, 0, 64 * 1024, 4096,
 			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
-			      SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
+			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
 
 	{ "m25p05",  INFO(0x202010,  0,  32 * 1024,   2, 0) },
 	{ "m25p10",  INFO(0x202011,  0,  32 * 1024,   4, 0) },
-- 
2.17.1

[PATCH 2/2] mtd: spi-nor: micron-st: add support for mt25ql01g and mt25qu01g

[Matthias Schiffer]

Split these mt25q models from the older n25q models by matching their
extended IDs to allow adding support for 4byte opcodes.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/mtd/spi-nor/micron-st.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index d5baa8762c8d..2fca5de2504f 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -172,11 +172,17 @@ static const struct flash_info st_parts[] = {
 			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
 			      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
 			      SPI_NOR_4BIT_BP | SPI_NOR_BP3_SR_BIT6) },
+	{ "mt25ql01g",   INFO6(0x20ba21, 0x104400, 64 * 1024, 2048,
+			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
+			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
 	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048,
 			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
 			      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
 			      SPI_NOR_4BIT_BP | SPI_NOR_BP3_SR_BIT6 |
 			      NO_CHIP_ERASE) },
+	{ "mt25qu01g",   INFO6(0x20bb21, 0x104400, 64 * 1024, 2048,
+			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
+			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
 	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048,
 			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
 			      NO_CHIP_ERASE) },
-- 
2.17.1

Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[Michael Walle]

Am 2021-07-23 13:27, schrieb Matthias Schiffer:
> All mt25q variants have the same features.
> 
> Unlike the smaller variants, no n25q with 2G exists, so we don't need 
> to
> match on the extended ID to distinguish n25q and mt25q series for these
> models.

But why shouldn't we? What if there will be another flash with
the same first three id bytes?

> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/mtd/spi-nor/micron-st.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c 
> b/drivers/mtd/spi-nor/micron-st.c
> index c224e59820a1..d5baa8762c8d 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -181,11 +181,11 @@ static const struct flash_info st_parts[] = {
>  			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
>  			      NO_CHIP_ERASE) },
>  	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
> -			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> -			      NO_CHIP_ERASE) },

This bothers me. I'm not sure how this will work. I see that
chip erase is command 0xc7, but both the new and the old flash
just supports 0xc3 (DIE ERASE). Did you test these changes?

> +			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> +			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>  	{ "mt25qu02g",   INFO(0x20bb22, 0, 64 * 1024, 4096,
>  			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> -			      SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> +			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> 
>  	{ "m25p05",  INFO(0x202010,  0,  32 * 1024,   2, 0) },
>  	{ "m25p10",  INFO(0x202011,  0,  32 * 1024,   4, 0) },

-michael

Re: [PATCH 2/2] mtd: spi-nor: micron-st: add support for mt25ql01g and mt25qu01g

[Michael Walle]

Am 2021-07-23 13:27, schrieb Matthias Schiffer:
> Split these mt25q models from the older n25q models by matching their
> extended IDs to allow adding support for 4byte opcodes.

You will have to supply SFDP data for all these chips, please have
a look at [1] how to do that.

Also, we'll switch to SFDP parsing, have a look at [2].

> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/mtd/spi-nor/micron-st.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c 
> b/drivers/mtd/spi-nor/micron-st.c
> index d5baa8762c8d..2fca5de2504f 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -172,11 +172,17 @@ static const struct flash_info st_parts[] = {
>  			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
>  			      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
>  			      SPI_NOR_4BIT_BP | SPI_NOR_BP3_SR_BIT6) },
> +	{ "mt25ql01g",   INFO6(0x20ba21, 0x104400, 64 * 1024, 2048,
> +			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> +			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },

As mentioned in my other mail, I don't see this chip supporting 0xc7
(BULK ERASE). Is erasing the whole chip working?

Thanks,
-michael

>  	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048,
>  			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
>  			      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB |
>  			      SPI_NOR_4BIT_BP | SPI_NOR_BP3_SR_BIT6 |
>  			      NO_CHIP_ERASE) },
> +	{ "mt25qu01g",   INFO6(0x20bb21, 0x104400, 64 * 1024, 2048,
> +			       SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> +			       SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>  	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048,
>  			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
>  			      NO_CHIP_ERASE) },

[1] 
https://lore.kernel.org/linux-mtd/7038f037de3e224016d269324517400d@walle.cc/
[2] 
https://lore.kernel.org/linux-mtd/20210727045222.905056-36-tudor.ambarus@microchip.com/

Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[Matthias Schiffer]

On Tue, 2021-07-27 at 09:09 +0200, Michael Walle wrote:
> Am 2021-07-23 13:27, schrieb Matthias Schiffer:
> > All mt25q variants have the same features.
> > 
> > Unlike the smaller variants, no n25q with 2G exists, so we don't need 
> > to
> > match on the extended ID to distinguish n25q and mt25q series for these
> > models.
> 
> But why shouldn't we? What if there will be another flash with
> the same first three id bytes?

That makes sense, I'll update my patch accordingly.

It looked to me like the current ID list only checks the extended ID
when necessary to distinguish two known flash models.


> 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/mtd/spi-nor/micron-st.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/micron-st.c 
> > b/drivers/mtd/spi-nor/micron-st.c
> > index c224e59820a1..d5baa8762c8d 100644
> > --- a/drivers/mtd/spi-nor/micron-st.c
> > +++ b/drivers/mtd/spi-nor/micron-st.c
> > @@ -181,11 +181,11 @@ static const struct flash_info st_parts[] = {
> >  			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> >  			      NO_CHIP_ERASE) },
> >  	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
> > -			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> > -			      NO_CHIP_ERASE) },
> 
> This bothers me. I'm not sure how this will work. I see that
> chip erase is command 0xc7, but both the new and the old flash
> just supports 0xc3 (DIE ERASE). Did you test these changes?

Thanks for catching this. I overlooked that the 1G and 2G variants
don't support the same erase commands as the smaller versions after
all... It is possible that I only tested this with partitioned MTD, so
I didn't hit the whole-chip erase case.

Which command should I use to test the chip erase? Will a `flash_erase
/dev/mtdX 0 0` trigger the correct operation?




> 
> > +			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> > +			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> >  	{ "mt25qu02g",   INFO(0x20bb22, 0, 64 * 1024, 4096,
> >  			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> > -			      SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> > +			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> > 
> >  	{ "m25p05",  INFO(0x202010,  0,  32 * 1024,   2, 0) },
> >  	{ "m25p10",  INFO(0x202011,  0,  32 * 1024,   4, 0) },
> 
> -michael

Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[Michael Walle]

Am 2021-07-27 12:45, schrieb Matthias Schiffer:
> On Tue, 2021-07-27 at 09:09 +0200, Michael Walle wrote:
[..]
>> > --- a/drivers/mtd/spi-nor/micron-st.c
>> > +++ b/drivers/mtd/spi-nor/micron-st.c
>> > @@ -181,11 +181,11 @@ static const struct flash_info st_parts[] = {
>> >  			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
>> >  			      NO_CHIP_ERASE) },
>> >  	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
>> > -			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
>> > -			      NO_CHIP_ERASE) },
>> 
>> This bothers me. I'm not sure how this will work. I see that
>> chip erase is command 0xc7, but both the new and the old flash
>> just supports 0xc3 (DIE ERASE). Did you test these changes?
> 
> Thanks for catching this. I overlooked that the 1G and 2G variants
> don't support the same erase commands as the smaller versions after
> all... It is possible that I only tested this with partitioned MTD, so
> I didn't hit the whole-chip erase case.
> 
> Which command should I use to test the chip erase? Will a `flash_erase
> /dev/mtdX 0 0` trigger the correct operation?

I guess so. Looking at
http://git.infradead.org/mtd-utils.git/blob/HEAD:/misc-utils/flash_erase.c#l226

It seems you should see a different output for either erasing individual
sectors or the whole chip (as long as the kernel doesn't the invidual
block erase itself).

-michael

Re: (EXT) Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[Matthias Schiffer]

On Tue, 2021-07-27 at 09:09 +0200, Michael Walle wrote:
> Am 2021-07-23 13:27, schrieb Matthias Schiffer:
> > All mt25q variants have the same features.
> > 
> > Unlike the smaller variants, no n25q with 2G exists, so we don't need 
> > to
> > match on the extended ID to distinguish n25q and mt25q series for these
> > models.
> 
> But why shouldn't we? What if there will be another flash with
> the same first three id bytes?

How do you suggest we proceed here? At the moment there are entries
matching on 0x20b[ab]22 (ignoring the extended ID) with the name
mt25q[lu]02g.

Should I change these entries to match on on the extended ID
0x20b[ab]22 / 0x104400 instead when I add the bits for the features
specific to the variant, removing support for other 0x20b[ab]22
variants that may or may not actually exist? Keeping both entries (with
and without extended ID match) would preserve compatiblity with such
variants, but this approach seems problematic to me as well, as I can't
even give a name to the more generic entries (and there is no natural
extension of the n25q naming scheme to a 2G variant).


> 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/mtd/spi-nor/micron-st.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/micron-st.c 
> > b/drivers/mtd/spi-nor/micron-st.c
> > index c224e59820a1..d5baa8762c8d 100644
> > --- a/drivers/mtd/spi-nor/micron-st.c
> > +++ b/drivers/mtd/spi-nor/micron-st.c
> > @@ -181,11 +181,11 @@ static const struct flash_info st_parts[] = {
> >  			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> >  			      NO_CHIP_ERASE) },
> >  	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096,
> > -			      SECT_4K | USE_FSR | SPI_NOR_QUAD_READ |
> > -			      NO_CHIP_ERASE) },
> 
> This bothers me. I'm not sure how this will work. I see that
> chip erase is command 0xc7, but both the new and the old flash
> just supports 0xc3 (DIE ERASE). Did you test these changes?

I finally got my hands on hardware with this flash again (well, a
mt25qu01g, I don't think we have the 2G variants anywhere) and I can
confirm that the chip erase does not work with my patch. I will send an
updated version that keeps NO_CHIP_ERASE.


> 
> > +			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> > +			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> >  	{ "mt25qu02g",   INFO(0x20bb22, 0, 64 * 1024, 4096,
> >  			      SECT_4K | USE_FSR | SPI_NOR_DUAL_READ |
> > -			      SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> > +			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> > 
> >  	{ "m25p05",  INFO(0x202010,  0,  32 * 1024,   2, 0) },
> >  	{ "m25p10",  INFO(0x202011,  0,  32 * 1024,   4, 0) },
> 
> -michael

Re: (EXT) Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[Michael Walle]

Am 2021-10-06 14:32, schrieb Matthias Schiffer:
> On Tue, 2021-07-27 at 09:09 +0200, Michael Walle wrote:
>> Am 2021-07-23 13:27, schrieb Matthias Schiffer:
>> > All mt25q variants have the same features.
>> >
>> > Unlike the smaller variants, no n25q with 2G exists, so we don't need
>> > to
>> > match on the extended ID to distinguish n25q and mt25q series for these
>> > models.
>> 
>> But why shouldn't we? What if there will be another flash with
>> the same first three id bytes?
> 
> How do you suggest we proceed here? At the moment there are entries
> matching on 0x20b[ab]22 (ignoring the extended ID) with the name
> mt25q[lu]02g.
> 
> Should I change these entries to match on on the extended ID
> 0x20b[ab]22 / 0x104400 instead when I add the bits for the features
> specific to the variant, removing support for other 0x20b[ab]22
> variants that may or may not actually exist? Keeping both entries (with
> and without extended ID match) would preserve compatiblity with such
> variants, but this approach seems problematic to me as well, as I can't
> even give a name to the more generic entries (and there is no natural
> extension of the n25q naming scheme to a 2G variant).

Mh, what do you think of adding three entries and make the last one,
the one with the short id, as a fallback so to speak. This should
retrain backwards compatibility, right? It should probably have a
comment because the order will matter then.

-michael

Re: (EXT) Re: (EXT) Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[Matthias Schiffer]

On Thu, 2021-10-07 at 09:08 +0200, Michael Walle wrote:
> Am 2021-10-06 14:32, schrieb Matthias Schiffer:
> > On Tue, 2021-07-27 at 09:09 +0200, Michael Walle wrote:
> > > Am 2021-07-23 13:27, schrieb Matthias Schiffer:
> > > > All mt25q variants have the same features.
> > > > 
> > > > Unlike the smaller variants, no n25q with 2G exists, so we don't need
> > > > to
> > > > match on the extended ID to distinguish n25q and mt25q series for these
> > > > models.
> > > 
> > > But why shouldn't we? What if there will be another flash with
> > > the same first three id bytes?
> > 
> > How do you suggest we proceed here? At the moment there are entries
> > matching on 0x20b[ab]22 (ignoring the extended ID) with the name
> > mt25q[lu]02g.
> > 
> > Should I change these entries to match on on the extended ID
> > 0x20b[ab]22 / 0x104400 instead when I add the bits for the features
> > specific to the variant, removing support for other 0x20b[ab]22
> > variants that may or may not actually exist? Keeping both entries (with
> > and without extended ID match) would preserve compatiblity with such
> > variants, but this approach seems problematic to me as well, as I can't
> > even give a name to the more generic entries (and there is no natural
> > extension of the n25q naming scheme to a 2G variant).
> 
> Mh, what do you think of adding three entries and make the last one,
> the one with the short id, as a fallback so to speak. This should
> retrain backwards compatibility, right? It should probably have a
> comment because the order will matter then.
> 
> -michael

Is it okay for multiple entries to use the same value for the "name"
field? In the existing definitions I couldn't find any example of
different ID matches mapping to the same name.

Re: (EXT) Re: (EXT) Re: [PATCH 1/2] mtd: spi-nor: micron-st: sync flags of mt25ql02g and mt25qu02g with other mt25q

[Michael Walle]

Am 2021-10-07 09:18, schrieb Matthias Schiffer:
> On Thu, 2021-10-07 at 09:08 +0200, Michael Walle wrote:
>> Am 2021-10-06 14:32, schrieb Matthias Schiffer:
>> > On Tue, 2021-07-27 at 09:09 +0200, Michael Walle wrote:
>> > > Am 2021-07-23 13:27, schrieb Matthias Schiffer:
>> > > > All mt25q variants have the same features.
>> > > >
>> > > > Unlike the smaller variants, no n25q with 2G exists, so we don't need
>> > > > to
>> > > > match on the extended ID to distinguish n25q and mt25q series for these
>> > > > models.
>> > >
>> > > But why shouldn't we? What if there will be another flash with
>> > > the same first three id bytes?
>> >
>> > How do you suggest we proceed here? At the moment there are entries
>> > matching on 0x20b[ab]22 (ignoring the extended ID) with the name
>> > mt25q[lu]02g.
>> >
>> > Should I change these entries to match on on the extended ID
>> > 0x20b[ab]22 / 0x104400 instead when I add the bits for the features
>> > specific to the variant, removing support for other 0x20b[ab]22
>> > variants that may or may not actually exist? Keeping both entries (with
>> > and without extended ID match) would preserve compatiblity with such
>> > variants, but this approach seems problematic to me as well, as I can't
>> > even give a name to the more generic entries (and there is no natural
>> > extension of the n25q naming scheme to a 2G variant).
>> 
>> Mh, what do you think of adding three entries and make the last one,
>> the one with the short id, as a fallback so to speak. This should
>> retrain backwards compatibility, right? It should probably have a
>> comment because the order will matter then.
>> 
>> -michael
> 
> Is it okay for multiple entries to use the same value for the "name"
> field? In the existing definitions I couldn't find any example of
> different ID matches mapping to the same name.

You're right, thats probably not good. Ok, if there is no objection
from Tudor, I'd say we change the entry of the mt25 variant to the
longer one. If there is a flash chip out there which matched the
shorter one, but not the longer one, thats probably not a mt25 anyway.

-michael