linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mtd: spi-nor: Add support for w25qNNjwim
@ 2020-01-03 22:34 Michael Walle
  2020-01-11 14:19 ` Tudor.Ambarus
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2020-01-03 22:34 UTC (permalink / raw)
  To: linux-mtd, linux-kernel
  Cc: Richard Weinberger, Michael Walle, Tudor Ambarus,
	Vignesh Raghavendra, Miquel Raynal

Add support for the Winbond W25QnnJW-IM flashes. These have a
programmable QE bit. There are also the W25QnnJW-IQ variant which shares
the ID with the W25QnnFW parts. These have the QE bit hard strapped to
1, thus don't support hardware write protection.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index addb6319fcbb..3fa8a81bdab0 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = {
 			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
 	},
+	{
+		"w25q16jwim", INFO(0xef8015, 0, 64 * 1024,  32,
+			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+	},
 	{ "w25x32", INFO(0xef3016, 0, 64 * 1024,  64, SECT_4K) },
 	{
 		"w25q16jv-im/jm", INFO(0xef7015, 0, 64 * 1024,  32,
@@ -2647,6 +2652,11 @@ static const struct flash_info spi_nor_ids[] = {
 			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
 	},
+	{
+		"w25q32jwim", INFO(0xef8016, 0, 64 * 1024,  64,
+			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+	},
 	{ "w25x64", INFO(0xef3017, 0, 64 * 1024, 128, SECT_4K) },
 	{ "w25q64", INFO(0xef4017, 0, 64 * 1024, 128, SECT_4K) },
 	{
@@ -2654,6 +2664,11 @@ static const struct flash_info spi_nor_ids[] = {
 			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
 	},
+	{
+		"w25q64jwim", INFO(0xef8017, 0, 64 * 1024, 128,
+			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+	},
 	{
 		"w25q128fw", INFO(0xef6018, 0, 64 * 1024, 256,
 			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
@@ -2664,6 +2679,11 @@ static const struct flash_info spi_nor_ids[] = {
 			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
 	},
+	{
+		"w25q128jwim", INFO(0xef8018, 0, 64 * 1024, 256,
+			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+	},
 	{ "w25q80", INFO(0xef5014, 0, 64 * 1024,  16, SECT_4K) },
 	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16, SECT_4K) },
 	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256, SECT_4K) },
@@ -2674,6 +2694,8 @@ static const struct flash_info spi_nor_ids[] = {
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "w25q256jw", INFO(0xef6019, 0, 64 * 1024, 512,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+	{ "w25q256jwim", INFO(0xef8019, 0, 64 * 1024, 512,
+			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "w25m512jv", INFO(0xef7119, 0, 64 * 1024, 1024,
 			SECT_4K | SPI_NOR_QUAD_READ | SPI_NOR_DUAL_READ) },
 
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-03 22:34 [PATCH] mtd: spi-nor: Add support for w25qNNjwim Michael Walle
@ 2020-01-11 14:19 ` Tudor.Ambarus
  2020-01-11 23:16   ` Michael Walle
  0 siblings, 1 reply; 13+ messages in thread
From: Tudor.Ambarus @ 2020-01-11 14:19 UTC (permalink / raw)
  To: linux-mtd; +Cc: richard, michael, miquel.raynal, linux-kernel, vigneshr

Hi, Michael,

On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote:
> Add support for the Winbond W25QnnJW-IM flashes. These have a
> programmable QE bit. There are also the W25QnnJW-IQ variant which shares
> the ID with the W25QnnFW parts. These have the QE bit hard strapped to
> 1, thus don't support hardware write protection.

There are few flavors of hw write protection supported by this flash, the Q 
version does not disable them all. How about saying just that the /HOLD 
function is disabled?

When we receive new flash id patches, we ask the contributors to specify if 
they test the flash, in which modes (single, quad), and with which controller. 
Ideally all the flash's flags should be tested, but there are cases in which 
the controllers do not support quad read for example, and we accept the 
patches even if tested in single read mode. SPI_NOR_HAS_LOCK and 
SPI_NOR_HAS_TB must be tested as well.

Even if the patches are rather simple, we ask for this to be sure that we 
don't add a flash that is broken from day one. So, would you please tell us 
what flashes did you test, what flags, and with which controller?

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index addb6319fcbb..3fa8a81bdab0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = {
>  			SECT_4K | SPI_NOR_DUAL_READ | 
SPI_NOR_QUAD_READ |
>  			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>  	},
> +	{
> +		"w25q16jwim", INFO(0xef8015, 0, 64 * 1024,  32,

"i" is for the temperature range, which is not a fixed characteristic. Usually 
there are flashes with the same jedec-id, but with different temperature 
ranges. Let's drop the "i" and rename it to "w25q16jwm"

Cheers,
ta



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-11 14:19 ` Tudor.Ambarus
@ 2020-01-11 23:16   ` Michael Walle
  2020-01-13  9:06     ` Tudor.Ambarus
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2020-01-11 23:16 UTC (permalink / raw)
  To: Tudor.Ambarus; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi Tudor,

Am 2020-01-11 15:19, schrieb Tudor.Ambarus@microchip.com:
> Hi, Michael,
> 
> On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote:
>> Add support for the Winbond W25QnnJW-IM flashes. These have a
>> programmable QE bit. There are also the W25QnnJW-IQ variant which 
>> shares
>> the ID with the W25QnnFW parts. These have the QE bit hard strapped to
>> 1, thus don't support hardware write protection.
> 
> There are few flavors of hw write protection supported by this flash, 
> the Q
> version does not disable them all. How about saying just that the /HOLD
> function is disabled?

I don't get your point here ;) My understanding is that HOLD# and WP# 
will
be disabled. Thus there is no "hardware write protection". What other hw
write protection do you have in mind?

> When we receive new flash id patches, we ask the contributors to 
> specify if
> they test the flash, in which modes (single, quad), and with which 
> controller.
> Ideally all the flash's flags should be tested, but there are cases in 
> which
> the controllers do not support quad read for example, and we accept the
> patches even if tested in single read mode. SPI_NOR_HAS_LOCK and
> SPI_NOR_HAS_TB must be tested as well.
> 
> Even if the patches are rather simple, we ask for this to be sure that 
> we
> don't add a flash that is broken from day one. So, would you please 
> tell us
> what flashes did you test, what flags, and with which controller?

Ok will add that to the commit message. Just to make sure. I've only 
tested the
32mbit part. So is it still ok to include all other flashes of this 
family?

For now. tested with the NXP FlexSPI, single and dual (no quad since we 
are
using the write protection feature and IO2 and IO3 are not connected to 
the
CPU). So write protection is also tested. I will retest the TB bit.

>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>> 
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c 
>> b/drivers/mtd/spi-nor/spi-nor.c
>> index addb6319fcbb..3fa8a81bdab0 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = 
>> {
>>  			SECT_4K | SPI_NOR_DUAL_READ |
> SPI_NOR_QUAD_READ |
>>  			SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>>  	},
>> +	{
>> +		"w25q16jwim", INFO(0xef8015, 0, 64 * 1024,  32,
> 
> "i" is for the temperature range, which is not a fixed characteristic. 
> Usually
> there are flashes with the same jedec-id, but with different 
> temperature
> ranges. Let's drop the "i" and rename it to "w25q16jwm"

Only that there is no flash with that part name :( according to the 
datasheet
there is only this one temp range available. From what I've seen for now 
(esp
looking at the macronix parts) it seems to first come first serve ;)
That being said, I don't insist on keeping that name, I'm fine with any 
name,
since I've learned you cannot rely on it in any way. Eg. the w25q32jwiq 
will
be discovered as w25q32dw. Or some Macronix flashes will be discovered 
as
ancient ones.

Btw. is renaming the flashes also considered a backwards incomaptible 
change?
And can there be two flashes with the same name? Because IMHO it would 
be
better to just have the name "w25q16" regardless whether its an FW/JW/JV 
etc.
It's better to show an ambiguous name than a wrong name.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-11 23:16   ` Michael Walle
@ 2020-01-13  9:06     ` Tudor.Ambarus
  2020-01-13 10:07       ` Michael Walle
  0 siblings, 1 reply; 13+ messages in thread
From: Tudor.Ambarus @ 2020-01-13  9:06 UTC (permalink / raw)
  To: michael; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi, Michael,

On Sunday, January 12, 2020 1:16:12 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,
> 
> Am 2020-01-11 15:19, schrieb Tudor.Ambarus@microchip.com:
> > Hi, Michael,
> > 
> > On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote:
> >> Add support for the Winbond W25QnnJW-IM flashes. These have a
> >> programmable QE bit. There are also the W25QnnJW-IQ variant which
> >> shares
> >> the ID with the W25QnnFW parts. These have the QE bit hard strapped to
> >> 1, thus don't support hardware write protection.
> > 
> > There are few flavors of hw write protection supported by this flash,
> > the Q
> > version does not disable them all. How about saying just that the /HOLD
> > function is disabled?
> 
> I don't get your point here ;) My understanding is that HOLD# and WP#
> will
> be disabled. Thus there is no "hardware write protection". What other hw
> write protection do you have in mind?

Time delay write disable after Power-up for example.

> 
> > When we receive new flash id patches, we ask the contributors to
> > specify if
> > they test the flash, in which modes (single, quad), and with which
> > controller.
> > Ideally all the flash's flags should be tested, but there are cases in
> > which
> > the controllers do not support quad read for example, and we accept the
> > patches even if tested in single read mode. SPI_NOR_HAS_LOCK and
> > SPI_NOR_HAS_TB must be tested as well.
> > 
> > Even if the patches are rather simple, we ask for this to be sure that
> > we
> > don't add a flash that is broken from day one. So, would you please
> > tell us
> > what flashes did you test, what flags, and with which controller?
> 
> Ok will add that to the commit message. Just to make sure. I've only
> tested the
> 32mbit part. So is it still ok to include all other flashes of this
> family?

No, just the ones that you can test please.

> 
> For now. tested with the NXP FlexSPI, single and dual (no quad since we
> are
> using the write protection feature and IO2 and IO3 are not connected to
> the
> CPU). So write protection is also tested. I will retest the TB bit.

Great, thanks.

> 
> >> Signed-off-by: Michael Walle <michael@walle.cc>
> >> ---
> >> 
> >>  drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >> 
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> >> b/drivers/mtd/spi-nor/spi-nor.c
> >> index addb6319fcbb..3fa8a81bdab0 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] =
> >> {
> >> 
> >>                      SECT_4K | SPI_NOR_DUAL_READ |
> > 
> > SPI_NOR_QUAD_READ |
> > 
> >>                      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> >>      
> >>      },
> >> 
> >> +    {
> >> +            "w25q16jwim", INFO(0xef8015, 0, 64 * 1024,  32,
> > 
> > "i" is for the temperature range, which is not a fixed characteristic.
> > Usually
> > there are flashes with the same jedec-id, but with different
> > temperature
> > ranges. Let's drop the "i" and rename it to "w25q16jwm"
> 
> Only that there is no flash with that part name :( according to the
> datasheet

The datasheet describes the W25Q32JW flash (check the first page of the 
datasheet). There are two flavors of this flash, each with its own jedec-id: Q 
version uses 156016h, M 158016h. We should name this flashes as "w25q32jwq" 
and "w25q32jwm". Please notice that I skipped intentionally the "i"  that 
stands for temperature range. Manufacturers can provide better temperature 
ranges for the same flash without changing the jedec-id. See this datasheet:

https://ro.mouser.com/datasheet/2/949/w25q128jv_revf_03272018_plus-1489608.pdf

> there is only this one temp range available. From what I've seen for now
> (esp
> looking at the macronix parts) it seems to first come first serve ;)
> That being said, I don't insist on keeping that name, I'm fine with any
> name,

you should be fine just with the name that best describes the flash :)

> since I've learned you cannot rely on it in any way. Eg. the w25q32jwiq
> will
> be discovered as w25q32dw. Or some Macronix flashes will be discovered
> as
> ancient ones.

Would you please study what's wrong with these names and provide a patch to 
fix them?

> 
> Btw. is renaming the flashes also considered a backwards incomaptible
> change?

No, we can fix the names.

> And can there be two flashes with the same name? Because IMHO it would
> be

I would prefer that we don't. Why would you have two different jedec-ids with 
the same name?

Cheers,
ta

> better to just have the name "w25q16" regardless whether its an FW/JW/JV
> etc.
> It's better to show an ambiguous name than a wrong name.
> 
> -michael




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-13  9:06     ` Tudor.Ambarus
@ 2020-01-13 10:07       ` Michael Walle
  2020-01-13 13:15         ` Michael Walle
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2020-01-13 10:07 UTC (permalink / raw)
  To: Tudor.Ambarus; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi Tudor,

Am 2020-01-13 10:06, schrieb Tudor.Ambarus@microchip.com:
> Hi, Michael,
> 
> On Sunday, January 12, 2020 1:16:12 AM EET Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> 
>> Hi Tudor,
>> 
>> Am 2020-01-11 15:19, schrieb Tudor.Ambarus@microchip.com:
>> > Hi, Michael,
>> >
>> > On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote:
>> >> Add support for the Winbond W25QnnJW-IM flashes. These have a
>> >> programmable QE bit. There are also the W25QnnJW-IQ variant which
>> >> shares
>> >> the ID with the W25QnnFW parts. These have the QE bit hard strapped to
>> >> 1, thus don't support hardware write protection.
>> >
>> > There are few flavors of hw write protection supported by this flash,
>> > the Q
>> > version does not disable them all. How about saying just that the /HOLD
>> > function is disabled?
>> 
>> I don't get your point here ;) My understanding is that HOLD# and WP#
>> will
>> be disabled. Thus there is no "hardware write protection". What other 
>> hw
>> write protection do you have in mind?
> 
> Time delay write disable after Power-up for example.

That is the usual "write enable" mechanism. while marketing may seem to 
see
that as write protection, I do not, esp. not hardware write protection.

What about changing it to the following?
"These have the QE bit hard strapped to 1, thus don't the /HOLD and /WP
pins".

>> 
>> > When we receive new flash id patches, we ask the contributors to
>> > specify if
>> > they test the flash, in which modes (single, quad), and with which
>> > controller.
>> > Ideally all the flash's flags should be tested, but there are cases in
>> > which
>> > the controllers do not support quad read for example, and we accept the
>> > patches even if tested in single read mode. SPI_NOR_HAS_LOCK and
>> > SPI_NOR_HAS_TB must be tested as well.
>> >
>> > Even if the patches are rather simple, we ask for this to be sure that
>> > we
>> > don't add a flash that is broken from day one. So, would you please
>> > tell us
>> > what flashes did you test, what flags, and with which controller?
>> 
>> Ok will add that to the commit message. Just to make sure. I've only
>> tested the
>> 32mbit part. So is it still ok to include all other flashes of this
>> family?
> 
> No, just the ones that you can test please.

ok

>> For now. tested with the NXP FlexSPI, single and dual (no quad since 
>> we
>> are
>> using the write protection feature and IO2 and IO3 are not connected 
>> to
>> the
>> CPU). So write protection is also tested. I will retest the TB bit.
> 
> Great, thanks.
> 
>> >> Signed-off-by: Michael Walle <michael@walle.cc>
>> >> ---
>> >>
>> >>  drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
>> >>  1 file changed, 22 insertions(+)
>> >>
>> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>> >> b/drivers/mtd/spi-nor/spi-nor.c
>> >> index addb6319fcbb..3fa8a81bdab0 100644
>> >> --- a/drivers/mtd/spi-nor/spi-nor.c
>> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> >> @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] =
>> >> {
>> >>
>> >>                      SECT_4K | SPI_NOR_DUAL_READ |
>> >
>> > SPI_NOR_QUAD_READ |
>> >
>> >>                      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>> >>
>> >>      },
>> >>
>> >> +    {
>> >> +            "w25q16jwim", INFO(0xef8015, 0, 64 * 1024,  32,
>> >
>> > "i" is for the temperature range, which is not a fixed characteristic.
>> > Usually
>> > there are flashes with the same jedec-id, but with different
>> > temperature
>> > ranges. Let's drop the "i" and rename it to "w25q16jwm"
>> 
>> Only that there is no flash with that part name :( according to the
>> datasheet
> 
> The datasheet describes the W25Q32JW flash (check the first page of the
> datasheet). There are two flavors of this flash, each with its own 
> jedec-id: Q
> version uses 156016h, M 158016h. We should name this flashes as 
> "w25q32jwq"
> and "w25q32jwm".

You mean ef6016 and ef8016, yes that is correct. My point was there is 
no
"w25q15jwm": If linux kernel messages write "detected w25q32jwm" nobody 
will
know what that part is, because there is no such part. There is a 
w25q32jwim
or maybe w25q32jwXm but no w25q32jwm.

And naming the Q version w25q32jwq is not possible, because the id is 
shared
with the w25q32dw, which is already in spi-nor.c.

> Please notice that I skipped intentionally the "i"  that
> stands for temperature range. Manufacturers can provide better 
> temperature
> ranges for the same flash without changing the jedec-id. See this 
> datasheet:

I know that and get your point. But I fear that this will confuse 
others.

> 
> https://ro.mouser.com/datasheet/2/949/w25q128jv_revf_03272018_plus-1489608.pdf
> 
>> there is only this one temp range available. From what I've seen for 
>> now
>> (esp
>> looking at the macronix parts) it seems to first come first serve ;)
>> That being said, I don't insist on keeping that name, I'm fine with 
>> any
>> name,
> 
> you should be fine just with the name that best describes the flash :)
> 
>> since I've learned you cannot rely on it in any way. Eg. the 
>> w25q32jwiq
>> will
>> be discovered as w25q32dw. Or some Macronix flashes will be discovered
>> as
>> ancient ones.
> 
> Would you please study what's wrong with these names and provide a 
> patch to
> fix them?

Well, I've did that last week. But TBH I don't know if I want to go down 
that
road. Its not only the names, its also the flags. There is a mix of old 
and
new flashes in spi-nor.c; for example the newer ones supports dual and 
quad
mode. But the crux is, Macronix shares the same id over different 
generations
of the flash chip. For example look at the MX25L8005 (as it is supported 
in
the kernel) [1]. It doesn't support dual I/O mode. But the newer 
generation
MX25L8006E, which has the same id, supports it. So even I could come up 
with
something to fix it, I doubt it will be accepted without testing.


>> 
>> Btw. is renaming the flashes also considered a backwards incomaptible
>> change?
> 
> No, we can fix the names.
> 
>> And can there be two flashes with the same name? Because IMHO it would
>> be
> 
> I would prefer that we don't. Why would you have two different 
> jedec-ids with
> the same name?

Because as pointed out in the Winbond example you cannot distiguish 
between
W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 
and
MX25L8006E. Thus my reasoning was to show only the common part, ie 
W25Q32
or MX25L80 which should be the same for this particular ID. Like I said, 
I'd
prefer showing an ambiguous name instead of a wrong one. But then you 
may
have different IDs with the same ambiguous name.

>> better to just have the name "w25q16" regardless whether its an 
>> FW/JW/JV
>> etc.
>> It's better to show an ambiguous name than a wrong name.
>> 
>> -michael

-michael

[1] 
http://web.archive.org/web/20180712194807/https://www.mct.net/download/macronix/mx25l8005.pdf


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-13 10:07       ` Michael Walle
@ 2020-01-13 13:15         ` Michael Walle
  2020-01-19  7:13           ` Tudor.Ambarus
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2020-01-13 13:15 UTC (permalink / raw)
  To: Tudor.Ambarus; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi Tudor,

Am 2020-01-13 11:07, schrieb Michael Walle:
>>> 
>>> Btw. is renaming the flashes also considered a backwards incomaptible
>>> change?
>> 
>> No, we can fix the names.
>> 
>>> And can there be two flashes with the same name? Because IMHO it 
>>> would
>>> be
>> 
>> I would prefer that we don't. Why would you have two different 
>> jedec-ids with
>> the same name?
> 
> Because as pointed out in the Winbond example you cannot distiguish 
> between
> W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 
> and
> MX25L8006E. Thus my reasoning was to show only the common part, ie 
> W25Q32
> or MX25L80 which should be the same for this particular ID. Like I 
> said, I'd
> prefer showing an ambiguous name instead of a wrong one. But then you 
> may
> have different IDs with the same ambiguous name.


Another solution would be to have the device tree provide a hint for the
actual flash chip. There would be multiple entries in the spi_nor_ids 
with the
same flash id. By default the first one is used (keeping the current
behaviour). If there is for example

   compatible = "jedec,spi-nor", "w25q32jwq";

the flash_info for the w25q32jwq will be chosen.

I know this will conflict with the new rule that there should only be

   compatible = "jedec,spi-nor";

without the actual flash chip. But it seems that it is not always 
possible
to just use the jedec id to match the correct chip.

Also see for example mx25l25635_post_bfpt_fixups() which tries to figure
out different behaviour by looking at "some" SFDP data. In this case we
might have been lucky, but I fear that this won't work in all cases and
for older flashes it won't work at all.

BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].

I guess that would be a less invasive way to fix different flashes with
same jedec ids.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-13 13:15         ` Michael Walle
@ 2020-01-19  7:13           ` Tudor.Ambarus
  2020-01-19 22:24             ` Michael Walle
  0 siblings, 1 reply; 13+ messages in thread
From: Tudor.Ambarus @ 2020-01-19  7:13 UTC (permalink / raw)
  To: michael; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

On Monday, January 13, 2020 3:15:15 PM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,

Hi, Michael,
> 
> Am 2020-01-13 11:07, schrieb Michael Walle:
> >>> Btw. is renaming the flashes also considered a backwards incomaptible
> >>> change?
> >> 
> >> No, we can fix the names.
> >> 
> >>> And can there be two flashes with the same name? Because IMHO it
> >>> would
> >>> be
> >> 
> >> I would prefer that we don't. Why would you have two different
> >> jedec-ids with
> >> the same name?
> > 
> > Because as pointed out in the Winbond example you cannot distiguish
> > between
> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005
> > and
> > MX25L8006E. Thus my reasoning was to show only the common part, ie
> > W25Q32
> > or MX25L80 which should be the same for this particular ID. Like I
> > said, I'd
> > prefer showing an ambiguous name instead of a wrong one. But then you
> > may
> > have different IDs with the same ambiguous name.
> 
> Another solution would be to have the device tree provide a hint for the
> actual flash chip. There would be multiple entries in the spi_nor_ids
> with the
> same flash id. By default the first one is used (keeping the current
> behaviour). If there is for example
> 
>    compatible = "jedec,spi-nor", "w25q32jwq";
> 
> the flash_info for the w25q32jwq will be chosen.

This won't work for plug-able flashes. You will influence the name in dt to be 
chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will end up 
with a wrong name for w25q32dw, thus the same problem.

If the flashes are identical but differ just in terms of name, we can rename 
the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences between 
these flashes; if you want to fix them, send a patch and I'll try to help.

Cheers,
ta

> 
> I know this will conflict with the new rule that there should only be
> 
>    compatible = "jedec,spi-nor";
> 
> without the actual flash chip. But it seems that it is not always
> possible
> to just use the jedec id to match the correct chip.
> 
> Also see for example mx25l25635_post_bfpt_fixups() which tries to figure
> out different behaviour by looking at "some" SFDP data. In this case we
> might have been lucky, but I fear that this won't work in all cases and
> for older flashes it won't work at all.
> 
> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].
> 
> I guess that would be a less invasive way to fix different flashes with
> same jedec ids.
> 
> -michael




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-19  7:13           ` Tudor.Ambarus
@ 2020-01-19 22:24             ` Michael Walle
  2020-01-20 11:03               ` Tudor.Ambarus
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2020-01-19 22:24 UTC (permalink / raw)
  To: Tudor.Ambarus; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi Tudor,

>> Am 2020-01-13 11:07, schrieb Michael Walle:
>> >>> Btw. is renaming the flashes also considered a backwards incomaptible
>> >>> change?
>> >>
>> >> No, we can fix the names.
>> >>
>> >>> And can there be two flashes with the same name? Because IMHO it
>> >>> would
>> >>> be
>> >>
>> >> I would prefer that we don't. Why would you have two different
>> >> jedec-ids with
>> >> the same name?
>> >
>> > Because as pointed out in the Winbond example you cannot distiguish
>> > between
>> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005
>> > and
>> > MX25L8006E. Thus my reasoning was to show only the common part, ie
>> > W25Q32
>> > or MX25L80 which should be the same for this particular ID. Like I
>> > said, I'd
>> > prefer showing an ambiguous name instead of a wrong one. But then you
>> > may
>> > have different IDs with the same ambiguous name.
>> 
>> Another solution would be to have the device tree provide a hint for 
>> the
>> actual flash chip. There would be multiple entries in the spi_nor_ids
>> with the
>> same flash id. By default the first one is used (keeping the current
>> behaviour). If there is for example
>> 
>>    compatible = "jedec,spi-nor", "w25q32jwq";
>> 
>> the flash_info for the w25q32jwq will be chosen.
> 
> This won't work for plug-able flashes. You will influence the name in 
> dt to be
> chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will 
> end up
> with a wrong name for w25q32dw, thus the same problem.

No, because then the device tree is wrong and doesn't fit the hardware. 
You'd
have to some instance which could change the device tree node, like the
bootloader or some device tree overlay for plugable flashes. We should 
try to
solve the actual problem at hand first..

It is just not possible to autodetect the SPI flash, just because
the vendors reuse the same IDs for flashes with different features (and 
the
SFDP is likely not enough). Therefore, you need to have a hint in some 
place
to use the flash properly.

> If the flashes are identical but differ just in terms of name, we can 
> rename
> the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences 
> between
> these flashes; if you want to fix them, send a patch and I'll try to 
> help.

It is not only the name, here are two examples which differ in 
functionality:
  (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports 
dual/quad
      mode
  (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit.

well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third 
example.

-michael

> 
> Cheers,
> ta
> 
>> 
>> I know this will conflict with the new rule that there should only be
>> 
>>    compatible = "jedec,spi-nor";
>> 
>> without the actual flash chip. But it seems that it is not always
>> possible
>> to just use the jedec id to match the correct chip.
>> 
>> Also see for example mx25l25635_post_bfpt_fixups() which tries to 
>> figure
>> out different behaviour by looking at "some" SFDP data. In this case 
>> we
>> might have been lucky, but I fear that this won't work in all cases 
>> and
>> for older flashes it won't work at all.
>> 
>> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].
>> 
>> I guess that would be a less invasive way to fix different flashes 
>> with
>> same jedec ids.
>> 
>> -michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-19 22:24             ` Michael Walle
@ 2020-01-20 11:03               ` Tudor.Ambarus
  2020-01-20 15:55                 ` Michael Walle
  0 siblings, 1 reply; 13+ messages in thread
From: Tudor.Ambarus @ 2020-01-20 11:03 UTC (permalink / raw)
  To: michael; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,

Hi, Michael,

> 
> >> Am 2020-01-13 11:07, schrieb Michael Walle:
> >> >>> Btw. is renaming the flashes also considered a backwards incomaptible
> >> >>> change?
> >> >> 
> >> >> No, we can fix the names.
> >> >> 
> >> >>> And can there be two flashes with the same name? Because IMHO it
> >> >>> would
> >> >>> be
> >> >> 
> >> >> I would prefer that we don't. Why would you have two different
> >> >> jedec-ids with
> >> >> the same name?
> >> > 
> >> > Because as pointed out in the Winbond example you cannot distiguish
> >> > between
> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005
> >> > and
> >> > MX25L8006E. Thus my reasoning was to show only the common part, ie
> >> > W25Q32
> >> > or MX25L80 which should be the same for this particular ID. Like I
> >> > said, I'd
> >> > prefer showing an ambiguous name instead of a wrong one. But then you
> >> > may
> >> > have different IDs with the same ambiguous name.
> >> 
> >> Another solution would be to have the device tree provide a hint for
> >> the
> >> actual flash chip. There would be multiple entries in the spi_nor_ids
> >> with the
> >> same flash id. By default the first one is used (keeping the current
> >> behaviour). If there is for example
> >> 
> >>    compatible = "jedec,spi-nor", "w25q32jwq";
> >> 
> >> the flash_info for the w25q32jwq will be chosen.
> > 
> > This won't work for plug-able flashes. You will influence the name in
> > dt to be
> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will
> > end up
> > with a wrong name for w25q32dw, thus the same problem.
> 
> No, because then the device tree is wrong and doesn't fit the hardware.
> You'd
> have to some instance which could change the device tree node, like the
> bootloader or some device tree overlay for plugable flashes. We should
> try to
> solve the actual problem at hand first..
> 
> It is just not possible to autodetect the SPI flash, just because
> the vendors reuse the same IDs for flashes with different features (and
> the
> SFDP is likely not enough). Therefore, you need to have a hint in some
> place
> to use the flash properly.
> 
> > If the flashes are identical but differ just in terms of name, we can
> > rename
> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences
> > between
> > these flashes; if you want to fix them, send a patch and I'll try to
> > help.
> 
> It is not only the name, here are two examples which differ in
> functionality:
>   (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports
> dual/quad
>       mode
>   (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit.
> 
> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third

sorry if this exhausted you.
> example.
> 

Flash auto-detection is nice and we should preserve it if possible. I would 
prefer having a post bfpt fixup than giving a hint about the flash in the 
compatible. The flashes that you mention are quite old and I don't know if it 
is worth to harm the auto-detection for them. A compromise has to be made.

You can gain traction in your endeavor if you have such a flash and there's 
nothing auto-detectable that differentiates it from some other flash that 
shares the sama jedec-id.

If you have such a flash and you care about it, send a patch and I'll try to 
help.

> -michael
> 
> > Cheers,
> > ta
> > 
> >> I know this will conflict with the new rule that there should only be
> >> 
> >>    compatible = "jedec,spi-nor";
> >> 
> >> without the actual flash chip. But it seems that it is not always
> >> possible
> >> to just use the jedec id to match the correct chip.
> >> 
> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to
> >> figure
> >> out different behaviour by looking at "some" SFDP data. In this case
> >> we
> >> might have been lucky, but I fear that this won't work in all cases
> >> and
> >> for older flashes it won't work at all.
> >> 
> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].
> >> 
> >> I guess that would be a less invasive way to fix different flashes
> >> with
> >> same jedec ids.
> >> 
> >> -michael




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-20 11:03               ` Tudor.Ambarus
@ 2020-01-20 15:55                 ` Michael Walle
  2020-01-21 18:40                   ` Tudor.Ambarus
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2020-01-20 15:55 UTC (permalink / raw)
  To: Tudor.Ambarus; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi Tudor,

Am 2020-01-20 12:03, schrieb Tudor.Ambarus@microchip.com:
> On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> 
>> Hi Tudor,
> 
> Hi, Michael,
> 
>> 
>> >> Am 2020-01-13 11:07, schrieb Michael Walle:
>> >> >>> Btw. is renaming the flashes also considered a backwards incomaptible
>> >> >>> change?
>> >> >>
>> >> >> No, we can fix the names.
>> >> >>
>> >> >>> And can there be two flashes with the same name? Because IMHO it
>> >> >>> would
>> >> >>> be
>> >> >>
>> >> >> I would prefer that we don't. Why would you have two different
>> >> >> jedec-ids with
>> >> >> the same name?
>> >> >
>> >> > Because as pointed out in the Winbond example you cannot distiguish
>> >> > between
>> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005
>> >> > and
>> >> > MX25L8006E. Thus my reasoning was to show only the common part, ie
>> >> > W25Q32
>> >> > or MX25L80 which should be the same for this particular ID. Like I
>> >> > said, I'd
>> >> > prefer showing an ambiguous name instead of a wrong one. But then you
>> >> > may
>> >> > have different IDs with the same ambiguous name.
>> >>
>> >> Another solution would be to have the device tree provide a hint for
>> >> the
>> >> actual flash chip. There would be multiple entries in the spi_nor_ids
>> >> with the
>> >> same flash id. By default the first one is used (keeping the current
>> >> behaviour). If there is for example
>> >>
>> >>    compatible = "jedec,spi-nor", "w25q32jwq";
>> >>
>> >> the flash_info for the w25q32jwq will be chosen.
>> >
>> > This won't work for plug-able flashes. You will influence the name in
>> > dt to be
>> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will
>> > end up
>> > with a wrong name for w25q32dw, thus the same problem.
>> 
>> No, because then the device tree is wrong and doesn't fit the 
>> hardware.
>> You'd
>> have to some instance which could change the device tree node, like 
>> the
>> bootloader or some device tree overlay for plugable flashes. We should
>> try to
>> solve the actual problem at hand first..
>> 
>> It is just not possible to autodetect the SPI flash, just because
>> the vendors reuse the same IDs for flashes with different features 
>> (and
>> the
>> SFDP is likely not enough). Therefore, you need to have a hint in some
>> place
>> to use the flash properly.
>> 
>> > If the flashes are identical but differ just in terms of name, we can
>> > rename
>> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences
>> > between
>> > these flashes; if you want to fix them, send a patch and I'll try to
>> > help.
>> 
>> It is not only the name, here are two examples which differ in
>> functionality:
>>   (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports
>> dual/quad
>>       mode
>>   (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit.
>> 
>> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third
> 
> sorry if this exhausted you.

TBH, this is no fun (and I'm doing this on my spare time because I like
open source). I guess our opinions differ waaay too much. I don't
really like band-aid fixes; eg. with vague information "it seems that
the F version adveritses support for Fast Read 4-4-4", what about other
flashes with that idcode and this property. This might break at any time
or with anyone trying support for other flashes with that ID.

That's what I've meant with first come first serve, I'm lucky now that
there was no flash with the same jedec id as the W25Q32JW.

To add the MX25U3232F I could check the JEDEC revision (or the BFPT
length) because it differers from the MX25U3235F. But I don't feel well
doing that. Who says Macronix won't update their description for the
MX25U3235F to the new revision.. FYI the Winbond guys apparently use the
first OTP region to store the JEDEC data, which is clever because they
can update it during production.

>> example.
>> 
> 
> Flash auto-detection is nice and we should preserve it if possible. I 
> would
> prefer having a post bfpt fixup than giving a hint about the flash in 
> the
> compatible.

see above.

> The flashes that you mention are quite old and I don't know if it
> is worth to harm the auto-detection for them. A compromise has to be 
> made.

so you'd drop support for them? because SFDP is never read if there is 
no
DUAL_READ or QUAD_READ flag.

> You can gain traction in your endeavor if you have such a flash and 
> there's
> nothing auto-detectable that differentiates it from some other flash 
> that
> shares the sama jedec-id.
> 
> If you have such a flash and you care about it, send a patch and I'll 
> try to
> help.

Given my reasoning above.. well maybe in the future. The Macronix would 
be
a second source candidate. For now we are using the Winbond flash.

I would rather like to have the flash protection topic and OTP support
sorted out, because that is something we are actually using.

-michael

> 
>> -michael
>> 
>> > Cheers,
>> > ta
>> >
>> >> I know this will conflict with the new rule that there should only be
>> >>
>> >>    compatible = "jedec,spi-nor";
>> >>
>> >> without the actual flash chip. But it seems that it is not always
>> >> possible
>> >> to just use the jedec id to match the correct chip.
>> >>
>> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to
>> >> figure
>> >> out different behaviour by looking at "some" SFDP data. In this case
>> >> we
>> >> might have been lucky, but I fear that this won't work in all cases
>> >> and
>> >> for older flashes it won't work at all.
>> >>
>> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].
>> >>
>> >> I guess that would be a less invasive way to fix different flashes
>> >> with
>> >> same jedec ids.
>> >>
>> >> -michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-20 15:55                 ` Michael Walle
@ 2020-01-21 18:40                   ` Tudor.Ambarus
  2020-01-21 23:28                     ` Michael Walle
  0 siblings, 1 reply; 13+ messages in thread
From: Tudor.Ambarus @ 2020-01-21 18:40 UTC (permalink / raw)
  To: michael; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi, Michael,

On Monday, January 20, 2020 5:55:55 PM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,
> 
> Am 2020-01-20 12:03, schrieb Tudor.Ambarus@microchip.com:
> > On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> the
> >> content is safe
> >> 
> >> Hi Tudor,
> > 
> > Hi, Michael,
> > 
> >> >> Am 2020-01-13 11:07, schrieb Michael Walle:
> >> >> >>> Btw. is renaming the flashes also considered a backwards
> >> >> >>> incomaptible
> >> >> >>> change?
> >> >> >> 
> >> >> >> No, we can fix the names.
> >> >> >> 
> >> >> >>> And can there be two flashes with the same name? Because IMHO it
> >> >> >>> would
> >> >> >>> be
> >> >> >> 
> >> >> >> I would prefer that we don't. Why would you have two different
> >> >> >> jedec-ids with
> >> >> >> the same name?
> >> >> > 
> >> >> > Because as pointed out in the Winbond example you cannot distiguish
> >> >> > between
> >> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between
> >> >> > MX25L8005
> >> >> > and
> >> >> > MX25L8006E. Thus my reasoning was to show only the common part, ie
> >> >> > W25Q32
> >> >> > or MX25L80 which should be the same for this particular ID. Like I
> >> >> > said, I'd
> >> >> > prefer showing an ambiguous name instead of a wrong one. But then
> >> >> > you
> >> >> > may
> >> >> > have different IDs with the same ambiguous name.
> >> >> 
> >> >> Another solution would be to have the device tree provide a hint for
> >> >> the
> >> >> actual flash chip. There would be multiple entries in the spi_nor_ids
> >> >> with the
> >> >> same flash id. By default the first one is used (keeping the current
> >> >> behaviour). If there is for example
> >> >> 
> >> >>    compatible = "jedec,spi-nor", "w25q32jwq";
> >> >> 
> >> >> the flash_info for the w25q32jwq will be chosen.
> >> > 
> >> > This won't work for plug-able flashes. You will influence the name in
> >> > dt to be
> >> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will
> >> > end up
> >> > with a wrong name for w25q32dw, thus the same problem.
> >> 
> >> No, because then the device tree is wrong and doesn't fit the
> >> hardware.
> >> You'd
> >> have to some instance which could change the device tree node, like
> >> the
> >> bootloader or some device tree overlay for plugable flashes. We should
> >> try to
> >> solve the actual problem at hand first..
> >> 
> >> It is just not possible to autodetect the SPI flash, just because
> >> the vendors reuse the same IDs for flashes with different features
> >> (and
> >> the
> >> SFDP is likely not enough). Therefore, you need to have a hint in some
> >> place
> >> to use the flash properly.
> >> 
> >> > If the flashes are identical but differ just in terms of name, we can
> >> > rename
> >> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences
> >> > between
> >> > these flashes; if you want to fix them, send a patch and I'll try to
> >> > help.
> >> 
> >> It is not only the name, here are two examples which differ in
> >> 
> >> functionality:
> >>   (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports
> >> 
> >> dual/quad
> >> 
> >>       mode
> >>   
> >>   (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit.
> >> 
> >> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third
> > 
> > sorry if this exhausted you.
> 
> TBH, this is no fun (and I'm doing this on my spare time because I like

It's not my fault that you're not having fun when someone disagrees with you.

> open source). I guess our opinions differ waaay too much. I don't

Up to a point, yes, our opinions differ. I'm not rejecting your suggestion, I 
just say that we should implement it as a last resort, when there's nothing 
auto-detectable at run-time that can differentiate between two flashes that 
share the same id.

> really like band-aid fixes; eg. with vague information "it seems that
> the F version adveritses support for Fast Read 4-4-4", what about other

We can update the comment to clear the incertitude: "The F version advertises 
support for Fast Read 4-4-4""

> flashes with that idcode and this property. This might break at any time
> or with anyone trying support for other flashes with that ID.

The jedec-id should be unique in the first place, manufacturers that use the 
same jedec-id for different flavors of flashes are doing a bad thing. A third 
flash with the same jedec-id is unlikely to happen.

> 
> That's what I've meant with first come first serve, I'm lucky now that
> there was no flash with the same jedec id as the W25Q32JW.
> 
> To add the MX25U3232F I could check the JEDEC revision (or the BFPT
> length) because it differers from the MX25U3235F. But I don't feel well

I prefer this because it's auto-detectable. If you don't feel well doing it, 
don't do it.

> doing that. Who says Macronix won't update their description for the
> MX25U3235F to the new revision.. FYI the Winbond guys apparently use the

You are raising theoretical problems. We can fix this when we will encounter 
it.

> first OTP region to store the JEDEC data, which is clever because they
> can update it during production.

If you say so.

> 
> >> example.
> > 
> > Flash auto-detection is nice and we should preserve it if possible. I
> > would
> > prefer having a post bfpt fixup than giving a hint about the flash in
> > the
> > compatible.
> 
> see above.
> 
> > The flashes that you mention are quite old and I don't know if it
> > is worth to harm the auto-detection for them. A compromise has to be
> > made.
> 
> so you'd drop support for them? because SFDP is never read if there is
> no
> DUAL_READ or QUAD_READ flag.

mx25l8006e  defines bfpt, while mx25l8005 doesn't. We can differentiate these 
too.
> 
> > You can gain traction in your endeavor if you have such a flash and
> > there's
> > nothing auto-detectable that differentiates it from some other flash
> > that
> > shares the sama jedec-id.
> > 
> > If you have such a flash and you care about it, send a patch and I'll
> > try to
> > help.
> 
> Given my reasoning above.. well maybe in the future. The Macronix would

ok

> be
> a second source candidate. For now we are using the Winbond flash.
> 
> I would rather like to have the flash protection topic and OTP support
> sorted out, because that is something we are actually using.

You can speed up the process by reviewing/testing the BP3 support. In turn, 
maybe Jungseung will review your OTP patches.

To sum up: the flash auto-detection (with capabilities) greatly ease the 
device tree node description and it allows us to plug and play different 
manufacturer flashes using the same dtb. I have a connector on one of my 
boards, to which I connect different types of flashes (assuming they have 
similar frequency and modes). So I would always prefer to have a post bfpt 
hook to differentiate between flashes which share the same jedec-id, than 
compromising the generic compatible. Of course, if there's nothing auto-
detectable that can differentiate between the flashes, then your idea can be 
implemented, but I would do this as a last resort.

There's also the idea of compromise. The jedec-id should be unique in the 
first place, manufacturers that use the same jedec-id for different flavors of 
flashes are taking a wrong design decision. Do I want to cripple the generic 
compatible just for an old flash with a bad jedec-id? I don't know yet. Also, 
the flashes that share the same id are quite old, and if nobody screamed about 
this until now, it's fine by me. You raised some theoretical questions, you 
don't really use the macronix flashes, what I say is that we should consider 
fixing them when it's actually required. And that the extension of the 
compatible should be done as a last resort, as of now it has more 
disadvantages than advantages.

Cheers,
ta

> 
> -michael
> 
> >> -michael
> >> 
> >> > Cheers,
> >> > ta
> >> > 
> >> >> I know this will conflict with the new rule that there should only be
> >> >> 
> >> >>    compatible = "jedec,spi-nor";
> >> >> 
> >> >> without the actual flash chip. But it seems that it is not always
> >> >> possible
> >> >> to just use the jedec id to match the correct chip.
> >> >> 
> >> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to
> >> >> figure
> >> >> out different behaviour by looking at "some" SFDP data. In this case
> >> >> we
> >> >> might have been lucky, but I fear that this won't work in all cases
> >> >> and
> >> >> for older flashes it won't work at all.
> >> >> 
> >> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].
> >> >> 
> >> >> I guess that would be a less invasive way to fix different flashes
> >> >> with
> >> >> same jedec ids.
> >> >> 
> >> >> -michael




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-21 18:40                   ` Tudor.Ambarus
@ 2020-01-21 23:28                     ` Michael Walle
  2020-01-22  6:48                       ` Tudor.Ambarus
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2020-01-21 23:28 UTC (permalink / raw)
  To: Tudor.Ambarus; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi Tudor,

Am 2020-01-21 19:40, schrieb Tudor.Ambarus@microchip.com:
> Hi, Michael,
> 
> On Monday, January 20, 2020 5:55:55 PM EET Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> 
>> Hi Tudor,
>> 
>> Am 2020-01-20 12:03, schrieb Tudor.Ambarus@microchip.com:
>> > On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote:
>> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> >> the
>> >> content is safe
>> >>
>> >> Hi Tudor,
>> >
>> > Hi, Michael,
>> >
>> >> >> Am 2020-01-13 11:07, schrieb Michael Walle:
>> >> >> >>> Btw. is renaming the flashes also considered a backwards
>> >> >> >>> incomaptible
>> >> >> >>> change?
>> >> >> >>
>> >> >> >> No, we can fix the names.
>> >> >> >>
>> >> >> >>> And can there be two flashes with the same name? Because IMHO it
>> >> >> >>> would
>> >> >> >>> be
>> >> >> >>
>> >> >> >> I would prefer that we don't. Why would you have two different
>> >> >> >> jedec-ids with
>> >> >> >> the same name?
>> >> >> >
>> >> >> > Because as pointed out in the Winbond example you cannot distiguish
>> >> >> > between
>> >> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between
>> >> >> > MX25L8005
>> >> >> > and
>> >> >> > MX25L8006E. Thus my reasoning was to show only the common part, ie
>> >> >> > W25Q32
>> >> >> > or MX25L80 which should be the same for this particular ID. Like I
>> >> >> > said, I'd
>> >> >> > prefer showing an ambiguous name instead of a wrong one. But then
>> >> >> > you
>> >> >> > may
>> >> >> > have different IDs with the same ambiguous name.
>> >> >>
>> >> >> Another solution would be to have the device tree provide a hint for
>> >> >> the
>> >> >> actual flash chip. There would be multiple entries in the spi_nor_ids
>> >> >> with the
>> >> >> same flash id. By default the first one is used (keeping the current
>> >> >> behaviour). If there is for example
>> >> >>
>> >> >>    compatible = "jedec,spi-nor", "w25q32jwq";
>> >> >>
>> >> >> the flash_info for the w25q32jwq will be chosen.
>> >> >
>> >> > This won't work for plug-able flashes. You will influence the name in
>> >> > dt to be
>> >> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will
>> >> > end up
>> >> > with a wrong name for w25q32dw, thus the same problem.
>> >>
>> >> No, because then the device tree is wrong and doesn't fit the
>> >> hardware.
>> >> You'd
>> >> have to some instance which could change the device tree node, like
>> >> the
>> >> bootloader or some device tree overlay for plugable flashes. We should
>> >> try to
>> >> solve the actual problem at hand first..
>> >>
>> >> It is just not possible to autodetect the SPI flash, just because
>> >> the vendors reuse the same IDs for flashes with different features
>> >> (and
>> >> the
>> >> SFDP is likely not enough). Therefore, you need to have a hint in some
>> >> place
>> >> to use the flash properly.
>> >>
>> >> > If the flashes are identical but differ just in terms of name, we can
>> >> > rename
>> >> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences
>> >> > between
>> >> > these flashes; if you want to fix them, send a patch and I'll try to
>> >> > help.
>> >>
>> >> It is not only the name, here are two examples which differ in
>> >>
>> >> functionality:
>> >>   (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports
>> >>
>> >> dual/quad
>> >>
>> >>       mode
>> >>
>> >>   (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit.
>> >>
>> >> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third
>> >
>> > sorry if this exhausted you.
>> 
>> TBH, this is no fun (and I'm doing this on my spare time because I 
>> like
> 
> It's not my fault that you're not having fun when someone disagrees 
> with you.

The reason is not the disagreement, but how you're (not) answering my 
arguments.
Like in the other thread, the question about the uselessness of the 
flash_lock
and flash_unlock tools with SPI-NOR and the (IMHO) bad behaviour when 
the user
actually uses flash_lock. Please, don't take this personally, I'll buy 
you a
beer at FOSDEM :p back to the technical stuff.

> 
>> open source). I guess our opinions differ waaay too much. I don't
> 
> Up to a point, yes, our opinions differ. I'm not rejecting your 
> suggestion, I
> just say that we should implement it as a last resort, when there's 
> nothing
> auto-detectable at run-time that can differentiate between two flashes 
> that
> share the same id.
> 
>> really like band-aid fixes; eg. with vague information "it seems that
>> the F version adveritses support for Fast Read 4-4-4", what about 
>> other
> 
> We can update the comment to clear the incertitude: "The F version 
> advertises
> support for Fast Read 4-4-4""
> 
>> flashes with that idcode and this property. This might break at any 
>> time
>> or with anyone trying support for other flashes with that ID.
> 
> The jedec-id should be unique in the first place, manufacturers that 
> use the
> same jedec-id for different flavors of flashes are doing a bad thing. A 
> third
> flash with the same jedec-id is unlikely to happen.

MX25U3232F, MX25U3235F, MX25U3273F, MX25U3235E all use the same 0x2c2536
identification. And these are only the active ones. I bet there are a
bunch of older 32MBit flashes.

MX25U6432F, MX25U6472F, MX25U6433F, MX25U6435F, MX25U6473F all use the
same 0x2c2537 id.

W25Q32JW-IQ, W25Q32DW, W25Q32FW all use the same 0x156016 id.

btw. thats why I argued to just have MX25U32 or W25Q32 as a name for the
flashes.

>> 
>> That's what I've meant with first come first serve, I'm lucky now that
>> there was no flash with the same jedec id as the W25Q32JW.
>> 
>> To add the MX25U3232F I could check the JEDEC revision (or the BFPT
>> length) because it differers from the MX25U3235F. But I don't feel 
>> well
> 
> I prefer this because it's auto-detectable. If you don't feel well 
> doing it,
> don't do it.

ok, I'll do so for the MX25U3232F support.

>> doing that. Who says Macronix won't update their description for the
>> MX25U3235F to the new revision.. FYI the Winbond guys apparently use 
>> the
> 
> You are raising theoretical problems. We can fix this when we will 
> encounter
> it.
> 
>> first OTP region to store the JEDEC data, which is clever because they
>> can update it during production.
> 
> If you say so.
> 
>> 
>> >> example.
>> >
>> > Flash auto-detection is nice and we should preserve it if possible. I
>> > would
>> > prefer having a post bfpt fixup than giving a hint about the flash in
>> > the
>> > compatible.
>> 
>> see above.
>> 
>> > The flashes that you mention are quite old and I don't know if it
>> > is worth to harm the auto-detection for them. A compromise has to be
>> > made.
>> 
>> so you'd drop support for them? because SFDP is never read if there is
>> no
>> DUAL_READ or QUAD_READ flag.
> 
> mx25l8006e  defines bfpt, while mx25l8005 doesn't. We can differentiate 
> these
> too.
>> 
>> > You can gain traction in your endeavor if you have such a flash and
>> > there's
>> > nothing auto-detectable that differentiates it from some other flash
>> > that
>> > shares the sama jedec-id.
>> >
>> > If you have such a flash and you care about it, send a patch and I'll
>> > try to
>> > help.
>> 
>> Given my reasoning above.. well maybe in the future. The Macronix 
>> would
> 
> ok
> 
>> be
>> a second source candidate. For now we are using the Winbond flash.
>> 
>> I would rather like to have the flash protection topic and OTP support
>> sorted out, because that is something we are actually using.
> 
> You can speed up the process by reviewing/testing the BP3 support. In 
> turn,
> maybe Jungseung will review your OTP patches.
> 
> To sum up: the flash auto-detection (with capabilities) greatly ease 
> the
> device tree node description and it allows us to plug and play 
> different
> manufacturer flashes using the same dtb. I have a connector on one of 
> my
> boards, to which I connect different types of flashes (assuming they 
> have
> similar frequency and modes). So I would always prefer to have a post 
> bfpt
> hook to differentiate between flashes which share the same jedec-id, 
> than
> compromising the generic compatible.

and making assumptions which are true for the flashes you currently know
about.

> Of course, if there's nothing auto-
> detectable that can differentiate between the flashes, then your idea 
> can be
> implemented, but I would do this as a last resort.
> 
> There's also the idea of compromise. The jedec-id should be unique in 
> the
> first place, manufacturers that use the same jedec-id for different 
> flavors of
> flashes are taking a wrong design decision. Do I want to cripple the 
> generic
> compatible just for an old flash with a bad jedec-id? I don't know yet. 
> Also,
> the flashes that share the same id are quite old, and if nobody 
> screamed about
> this until now, it's fine by me.

See above, the assumption that newer flashes have differnet jedec-ids is 
wrong.

> You raised some theoretical questions, you
> don't really use the macronix flashes, what I say is that we should 
> consider
> fixing them when it's actually required. And that the extension of the
> compatible should be done as a last resort, as of now it has more
> disadvantages than advantages.

Well what are the disadvantages? I don't argue against the 
autodetection. I
argue to have a mechanism _already_ in place when the autodetection 
fails.
If you don't specify the hint, everything stays the same.

We could have the advantages of both worlds, have a generic "w25q32" 
which tries
its best for autodetection and a specific "w25q32fw" which could can be 
hinted.
Same for like "mx25u32" and "mx25u3232f", "mx25u3235f" etc.


-michael


> 
> Cheers,
> ta
> 
>> 
>> -michael
>> 
>> >> -michael
>> >>
>> >> > Cheers,
>> >> > ta
>> >> >
>> >> >> I know this will conflict with the new rule that there should only be
>> >> >>
>> >> >>    compatible = "jedec,spi-nor";
>> >> >>
>> >> >> without the actual flash chip. But it seems that it is not always
>> >> >> possible
>> >> >> to just use the jedec id to match the correct chip.
>> >> >>
>> >> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to
>> >> >> figure
>> >> >> out different behaviour by looking at "some" SFDP data. In this case
>> >> >> we
>> >> >> might have been lucky, but I fear that this won't work in all cases
>> >> >> and
>> >> >> for older flashes it won't work at all.
>> >> >>
>> >> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].
>> >> >>
>> >> >> I guess that would be a less invasive way to fix different flashes
>> >> >> with
>> >> >> same jedec ids.
>> >> >>
>> >> >> -michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim
  2020-01-21 23:28                     ` Michael Walle
@ 2020-01-22  6:48                       ` Tudor.Ambarus
  0 siblings, 0 replies; 13+ messages in thread
From: Tudor.Ambarus @ 2020-01-22  6:48 UTC (permalink / raw)
  To: michael; +Cc: richard, miquel.raynal, linux-mtd, linux-kernel, vigneshr

Hi, Michael,

On Wednesday, January 22, 2020 1:28:52 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,
> 
> Am 2020-01-21 19:40, schrieb Tudor.Ambarus@microchip.com:
> > Hi, Michael,
> > 
> > On Monday, January 20, 2020 5:55:55 PM EET Michael Walle wrote:
> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> the
> >> content is safe
> >> 
> >> Hi Tudor,
> >> 
> >> Am 2020-01-20 12:03, schrieb Tudor.Ambarus@microchip.com:
> >> > On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote:
> >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >> >> the
> >> >> content is safe
> >> >> 
> >> >> Hi Tudor,
> >> > 
> >> > Hi, Michael,
> >> > 
> >> >> >> Am 2020-01-13 11:07, schrieb Michael Walle:
> >> >> >> >>> Btw. is renaming the flashes also considered a backwards
> >> >> >> >>> incomaptible
> >> >> >> >>> change?
> >> >> >> >> 
> >> >> >> >> No, we can fix the names.
> >> >> >> >> 
> >> >> >> >>> And can there be two flashes with the same name? Because IMHO
> >> >> >> >>> it
> >> >> >> >>> would
> >> >> >> >>> be
> >> >> >> >> 
> >> >> >> >> I would prefer that we don't. Why would you have two different
> >> >> >> >> jedec-ids with
> >> >> >> >> the same name?
> >> >> >> > 
> >> >> >> > Because as pointed out in the Winbond example you cannot
> >> >> >> > distiguish
> >> >> >> > between
> >> >> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between
> >> >> >> > MX25L8005
> >> >> >> > and
> >> >> >> > MX25L8006E. Thus my reasoning was to show only the common part,
> >> >> >> > ie
> >> >> >> > W25Q32
> >> >> >> > or MX25L80 which should be the same for this particular ID. Like
> >> >> >> > I
> >> >> >> > said, I'd
> >> >> >> > prefer showing an ambiguous name instead of a wrong one. But then
> >> >> >> > you
> >> >> >> > may
> >> >> >> > have different IDs with the same ambiguous name.
> >> >> >> 
> >> >> >> Another solution would be to have the device tree provide a hint
> >> >> >> for
> >> >> >> the
> >> >> >> actual flash chip. There would be multiple entries in the
> >> >> >> spi_nor_ids
> >> >> >> with the
> >> >> >> same flash id. By default the first one is used (keeping the
> >> >> >> current
> >> >> >> behaviour). If there is for example
> >> >> >> 
> >> >> >>    compatible = "jedec,spi-nor", "w25q32jwq";
> >> >> >> 
> >> >> >> the flash_info for the w25q32jwq will be chosen.
> >> >> > 
> >> >> > This won't work for plug-able flashes. You will influence the name
> >> >> > in
> >> >> > dt to be
> >> >> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you
> >> >> > will
> >> >> > end up
> >> >> > with a wrong name for w25q32dw, thus the same problem.
> >> >> 
> >> >> No, because then the device tree is wrong and doesn't fit the
> >> >> hardware.
> >> >> You'd
> >> >> have to some instance which could change the device tree node, like
> >> >> the
> >> >> bootloader or some device tree overlay for plugable flashes. We should
> >> >> try to
> >> >> solve the actual problem at hand first..
> >> >> 
> >> >> It is just not possible to autodetect the SPI flash, just because
> >> >> the vendors reuse the same IDs for flashes with different features
> >> >> (and
> >> >> the
> >> >> SFDP is likely not enough). Therefore, you need to have a hint in some
> >> >> place
> >> >> to use the flash properly.
> >> >> 
> >> >> > If the flashes are identical but differ just in terms of name, we
> >> >> > can
> >> >> > rename
> >> >> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the
> >> >> > differences
> >> >> > between
> >> >> > these flashes; if you want to fix them, send a patch and I'll try to
> >> >> > help.
> >> >> 
> >> >> It is not only the name, here are two examples which differ in
> >> >> 
> >> >> functionality:
> >> >>   (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports
> >> >> 
> >> >> dual/quad
> >> >> 
> >> >>       mode
> >> >>   
> >> >>   (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit.
> >> >> 
> >> >> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third
> >> > 
> >> > sorry if this exhausted you.
> >> 
> >> TBH, this is no fun (and I'm doing this on my spare time because I
> >> like
> > 
> > It's not my fault that you're not having fun when someone disagrees
> > with you.
> 
> The reason is not the disagreement, but how you're (not) answering my
> arguments.
> Like in the other thread, the question about the uselessness of the
> flash_lock
> and flash_unlock tools with SPI-NOR and the (IMHO) bad behaviour when
> the user

The flash unlock at probe was a bad decision, but we can't be backward 
compatible. kconfig or module param will solve the problem without breaking 
backward compatibility.

> actually uses flash_lock. Please, don't take this personally, I'll buy
> you a
> beer at FOSDEM :p back to the technical stuff.

I don't take this personally, but I think the discussion went in a wrong 
direction, and I feel that we waste time on theoretical problems.

> 
> >> open source). I guess our opinions differ waaay too much. I don't
> > 
> > Up to a point, yes, our opinions differ. I'm not rejecting your
> > suggestion, I
> > just say that we should implement it as a last resort, when there's
> > nothing
> > auto-detectable at run-time that can differentiate between two flashes
> > that
> > share the same id.
> > 
> >> really like band-aid fixes; eg. with vague information "it seems that
> >> the F version adveritses support for Fast Read 4-4-4", what about
> >> other
> > 
> > We can update the comment to clear the incertitude: "The F version
> > advertises
> > support for Fast Read 4-4-4""
> > 
> >> flashes with that idcode and this property. This might break at any
> >> time
> >> or with anyone trying support for other flashes with that ID.
> > 
> > The jedec-id should be unique in the first place, manufacturers that
> > use the
> > same jedec-id for different flavors of flashes are doing a bad thing. A
> > third
> > flash with the same jedec-id is unlikely to happen.
> 
> MX25U3232F, MX25U3235F, MX25U3273F, MX25U3235E all use the same 0x2c2536
> identification. And these are only the active ones. I bet there are a
> bunch of older 32MBit flashes.
> 
> MX25U6432F, MX25U6472F, MX25U6433F, MX25U6435F, MX25U6473F all use the
> same 0x2c2537 id.
> 
> W25Q32JW-IQ, W25Q32DW, W25Q32FW all use the same 0x156016 id.
> 
> btw. thats why I argued to just have MX25U32 or W25Q32 as a name for the
> flashes.
> 
> >> That's what I've meant with first come first serve, I'm lucky now that
> >> there was no flash with the same jedec id as the W25Q32JW.
> >> 
> >> To add the MX25U3232F I could check the JEDEC revision (or the BFPT
> >> length) because it differers from the MX25U3235F. But I don't feel
> >> well
> > 
> > I prefer this because it's auto-detectable. If you don't feel well
> > doing it,
> > don't do it.
> 
> ok, I'll do so for the MX25U3232F support.
> 
> >> doing that. Who says Macronix won't update their description for the
> >> MX25U3235F to the new revision.. FYI the Winbond guys apparently use
> >> the
> > 
> > You are raising theoretical problems. We can fix this when we will
> > encounter
> > it.
> > 
> >> first OTP region to store the JEDEC data, which is clever because they
> >> can update it during production.
> > 
> > If you say so.
> > 
> >> >> example.
> >> > 
> >> > Flash auto-detection is nice and we should preserve it if possible. I
> >> > would
> >> > prefer having a post bfpt fixup than giving a hint about the flash in
> >> > the
> >> > compatible.
> >> 
> >> see above.
> >> 
> >> > The flashes that you mention are quite old and I don't know if it
> >> > is worth to harm the auto-detection for them. A compromise has to be
> >> > made.
> >> 
> >> so you'd drop support for them? because SFDP is never read if there is
> >> no
> >> DUAL_READ or QUAD_READ flag.
> > 
> > mx25l8006e  defines bfpt, while mx25l8005 doesn't. We can differentiate
> > these
> > too.
> > 
> >> > You can gain traction in your endeavor if you have such a flash and
> >> > there's
> >> > nothing auto-detectable that differentiates it from some other flash
> >> > that
> >> > shares the sama jedec-id.
> >> > 
> >> > If you have such a flash and you care about it, send a patch and I'll
> >> > try to
> >> > help.
> >> 
> >> Given my reasoning above.. well maybe in the future. The Macronix
> >> would
> > 
> > ok
> > 
> >> be
> >> a second source candidate. For now we are using the Winbond flash.
> >> 
> >> I would rather like to have the flash protection topic and OTP support
> >> sorted out, because that is something we are actually using.
> > 
> > You can speed up the process by reviewing/testing the BP3 support. In
> > turn,
> > maybe Jungseung will review your OTP patches.
> > 
> > To sum up: the flash auto-detection (with capabilities) greatly ease
> > the
> > device tree node description and it allows us to plug and play
> > different
> > manufacturer flashes using the same dtb. I have a connector on one of
> > my
> > boards, to which I connect different types of flashes (assuming they
> > have
> > similar frequency and modes). So I would always prefer to have a post
> > bfpt
> > hook to differentiate between flashes which share the same jedec-id,
> > than
> > compromising the generic compatible.
> 
> and making assumptions which are true for the flashes you currently know
> about.

I don't want to introduce code which I don't know if it will ever be used.

> > Of course, if there's nothing auto-
> > detectable that can differentiate between the flashes, then your idea
> > can be
> > implemented, but I would do this as a last resort.
> > 
> > There's also the idea of compromise. The jedec-id should be unique in
> > the
> > first place, manufacturers that use the same jedec-id for different
> > flavors of
> > flashes are taking a wrong design decision. Do I want to cripple the
> > generic
> > compatible just for an old flash with a bad jedec-id? I don't know yet.
> > Also,
> > the flashes that share the same id are quite old, and if nobody
> > screamed about
> > this until now, it's fine by me.
> 
> See above, the assumption that newer flashes have differnet jedec-ids is
> wrong.
> 
> > You raised some theoretical questions, you
> > don't really use the macronix flashes, what I say is that we should
> > consider
> > fixing them when it's actually required. And that the extension of the
> > compatible should be done as a last resort, as of now it has more
> > disadvantages than advantages.
> 
> Well what are the disadvantages? I don't argue against the
> autodetection. I

- generic compatible eases the use. I don't have to check the dtb each time I 
change a plug-able flash, to see if I gave a wrong hint for the flash in the 
extended compatible
- people will prefer to use the extended compatible instead of trying to auto-
detect the differences at run-time (it's easier, but wrong).

> argue to have a mechanism _already_ in place when the autodetection
> fails.
> If you don't specify the hint, everything stays the same.
> 
> We could have the advantages of both worlds, have a generic "w25q32"
> which tries
> its best for autodetection and a specific "w25q32fw" which could can be
> hinted.
> Same for like "mx25u32" and "mx25u3232f", "mx25u3235f" etc.
> 

If you possess 2 flashes that we can't correctly detect at run-time and you 
care to fix them, then your suggestion has a good change to be implemented. I 
will not introduce code that is not used, in the hope that it will be used. No 
compatible extension if we have a way to auto-detect the flash.

Cheers,
ta

> 
> > Cheers,
> > ta
> > 
> >> -michael
> >> 
> >> >> -michael
> >> >> 
> >> >> > Cheers,
> >> >> > ta
> >> >> > 
> >> >> >> I know this will conflict with the new rule that there should only
> >> >> >> be
> >> >> >> 
> >> >> >>    compatible = "jedec,spi-nor";
> >> >> >> 
> >> >> >> without the actual flash chip. But it seems that it is not always
> >> >> >> possible
> >> >> >> to just use the jedec id to match the correct chip.
> >> >> >> 
> >> >> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to
> >> >> >> figure
> >> >> >> out different behaviour by looking at "some" SFDP data. In this
> >> >> >> case
> >> >> >> we
> >> >> >> might have been lucky, but I fear that this won't work in all cases
> >> >> >> and
> >> >> >> for older flashes it won't work at all.
> >> >> >> 
> >> >> >> BTW I do not suggest to add the strings to the the
> >> >> >> spi_nor_dev_ids[].
> >> >> >> 
> >> >> >> I guess that would be a less invasive way to fix different flashes
> >> >> >> with
> >> >> >> same jedec ids.
> >> >> >> 
> >> >> >> -michael




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-01-22  6:48 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-03 22:34 [PATCH] mtd: spi-nor: Add support for w25qNNjwim Michael Walle
2020-01-11 14:19 ` Tudor.Ambarus
2020-01-11 23:16   ` Michael Walle
2020-01-13  9:06     ` Tudor.Ambarus
2020-01-13 10:07       ` Michael Walle
2020-01-13 13:15         ` Michael Walle
2020-01-19  7:13           ` Tudor.Ambarus
2020-01-19 22:24             ` Michael Walle
2020-01-20 11:03               ` Tudor.Ambarus
2020-01-20 15:55                 ` Michael Walle
2020-01-21 18:40                   ` Tudor.Ambarus
2020-01-21 23:28                     ` Michael Walle
2020-01-22  6:48                       ` Tudor.Ambarus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).