[PATCH v2 0/2] mtd: spi-nor: macronix: workaround for device id re-use

[PATCH v2 0/2] mtd: spi-nor: macronix: workaround for device id re-use

[Esben Haabendal]

I only have access to boards with MX25L3233F flashes, so haven't been
able to test the backwards compatibility. If anybody has boards with
MX25L3205D and/or MX25L3206E, please help test this patch. Keep an eye
for read performance regression.

It is worth nothing that both MX25L3205D and MX25L3206E are
end-of-life, and is unavailable from Macronix, so any new boards
featuring a Macronix flash with this ID will likely be using
MX25L3233F.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Changes in v2:
- Added new flag (SPI_NOR_TRY_SFDP) to spi-nor core to allow trying
  SFDP and fallback to legacy parameters without having to specify
  DUAL/QUAD parameters.
- Rewrite macronix to use SPI_NOR_TRY_SFDP flag.
- Use with the ancient EoL MX25L3205D chip will not get speed
  increase, but stay at 1-bit mode as it is now.
- Link to v1: https://lore.kernel.org/r/20240524-macronix-mx25l3205d-fixups-v1-1-ee152e56afb3@geanix.com

---
Esben Haabendal (2):
      mtd: spi-nor: core: add flag for doing optional SFDP
      mtd: spi-nor: macronix: enable quad/dual speed for mx25l3205d chips

 drivers/mtd/spi-nor/core.c     | 3 ++-
 drivers/mtd/spi-nor/core.h     | 1 +
 drivers/mtd/spi-nor/macronix.c | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)
---
base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
change-id: 20240524-macronix-mx25l3205d-fixups-882e92eed7d7

Best regards,
-- 
Esben Haabendal <esben@geanix.com>

[PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Esben Haabendal]

A dedicated flag for triggering call to
spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
and parse, with fallback to legacy flash parameters, without having dual,
quad or octal parameters set in the legacy flash parameters.

With this, spi-nor flash parts without SFDP that is replaced with a
different flash NOR flash part that does have SFDP, but shares the same
manufacturer and device ID is easily handled.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/mtd/spi-nor/core.c | 3 ++-
 drivers/mtd/spi-nor/core.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 3e1f1913536b..1c4d66fc993b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
 
 	spi_nor_manufacturer_init_params(nor);
 
-	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
+	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
+					SPI_NOR_DUAL_READ |
 					SPI_NOR_QUAD_READ |
 					SPI_NOR_OCTAL_READ |
 					SPI_NOR_OCTAL_DTR_READ))
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 442786685515..77f61a984cb3 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -535,6 +535,7 @@ struct flash_info {
 	u8 no_sfdp_flags;
 #define SPI_NOR_SKIP_SFDP		BIT(0)
 #define SECT_4K				BIT(1)
+#define SPI_NOR_TRY_SFDP		BIT(2)
 #define SPI_NOR_DUAL_READ		BIT(3)
 #define SPI_NOR_QUAD_READ		BIT(4)
 #define SPI_NOR_OCTAL_READ		BIT(5)

-- 
2.45.2

[PATCH v2 2/2] mtd: spi-nor: macronix: enable quad/dual speed for mx25l3205d chips

[Esben Haabendal]

Macronix engineers apparantly do not understand the purpose of having
an ID actually identify the chip and its capabilities. Sigh.

The original Macronix SPI NOR flash that identifies itself as 0xC22016
with RDID was MX25L3205D. This chip does not support SFDP, but does
support the 2READ command (1-2-2).

When Macronix announced EoL for MX25L3205D, the recommended
replacement part was MX25L3206E, which conveniently also identifies
itself as 0xC22016. It does not support 2READ, but supports DREAD
(1-1-2) instead, and supports SFDP for discovering this.

When Macronix announced EoL for MX25L3206E, the recommended
replacement part was MX25L3233F, which also identifies itself as
0xC22016. It supports DREAD, 2READ, and the quad modes QREAD (1-1-4)
and 4READ (1-4-4). This also support SFDP.

So far, all of these chips have been handled the same way by the Linux
driver. The SFDP information have not been read, and no dual and quad
read modes have been enabled.

The trouble begins when we want to enable the faster read modes. The
RDID command only return the same 3 bytes for all 3 chips, so that
doesn't really help.

Instead, we can use the SPI_NOR_TRY_SFDP flag, which forces the spi-nor
system to try using SFDP, but fallback to the parameters specified in
struct flash_info.

This way, boards using MX25L3205D will continue as before this change.
That is without taking advantage of the 1-2-2 that it supports.

For MX25L3206E and MX25L3233F, the SFDP parameters are used, and they will
therefore be using the optimal dual or quad mode supported by the flash
and the SPI controller it is attached to.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/mtd/spi-nor/macronix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index ea6be95e75a5..090f28e05a5d 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -61,7 +61,7 @@ static const struct flash_info macronix_nor_parts[] = {
 		.id = SNOR_ID(0xc2, 0x20, 0x16),
 		.name = "mx25l3205d",
 		.size = SZ_4M,
-		.no_sfdp_flags = SECT_4K,
+		.no_sfdp_flags = SECT_4K | SPI_NOR_TRY_SFDP,
 	}, {
 		.id = SNOR_ID(0xc2, 0x20, 0x17),
 		.name = "mx25l6405d",

-- 
2.45.2

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Tudor Ambarus]

On 6/3/24 14:09, Esben Haabendal wrote:
> A dedicated flag for triggering call to
> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
> and parse, with fallback to legacy flash parameters, without having dual,
> quad or octal parameters set in the legacy flash parameters.
> 
> With this, spi-nor flash parts without SFDP that is replaced with a
> different flash NOR flash part that does have SFDP, but shares the same
> manufacturer and device ID is easily handled.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/mtd/spi-nor/core.c | 3 ++-
>  drivers/mtd/spi-nor/core.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 3e1f1913536b..1c4d66fc993b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>  
>  	spi_nor_manufacturer_init_params(nor);
>  
> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |

I don't like that we update deprecated methods. The solution though is
elegant.

> +					SPI_NOR_DUAL_READ |
>  					SPI_NOR_QUAD_READ |
>  					SPI_NOR_OCTAL_READ |
>  					SPI_NOR_OCTAL_DTR_READ))
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 442786685515..77f61a984cb3 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -535,6 +535,7 @@ struct flash_info {
>  	u8 no_sfdp_flags;
>  #define SPI_NOR_SKIP_SFDP		BIT(0)
>  #define SECT_4K				BIT(1)
> +#define SPI_NOR_TRY_SFDP		BIT(2)
>  #define SPI_NOR_DUAL_READ		BIT(3)
>  #define SPI_NOR_QUAD_READ		BIT(4)
>  #define SPI_NOR_OCTAL_READ		BIT(5)
> 

Re: [PATCH v2 2/2] mtd: spi-nor: macronix: enable quad/dual speed for mx25l3205d chips

[Tudor Ambarus]

On 6/3/24 14:09, Esben Haabendal wrote:
> Macronix engineers apparantly do not understand the purpose of having
> an ID actually identify the chip and its capabilities. Sigh.
> 
> The original Macronix SPI NOR flash that identifies itself as 0xC22016
> with RDID was MX25L3205D. This chip does not support SFDP, but does
> support the 2READ command (1-2-2).
> 
> When Macronix announced EoL for MX25L3205D, the recommended
> replacement part was MX25L3206E, which conveniently also identifies
> itself as 0xC22016. It does not support 2READ, but supports DREAD
> (1-1-2) instead, and supports SFDP for discovering this.
> 
> When Macronix announced EoL for MX25L3206E, the recommended
> replacement part was MX25L3233F, which also identifies itself as
> 0xC22016. It supports DREAD, 2READ, and the quad modes QREAD (1-1-4)
> and 4READ (1-4-4). This also support SFDP.
> 
> So far, all of these chips have been handled the same way by the Linux
> driver. The SFDP information have not been read, and no dual and quad
> read modes have been enabled.
> 
> The trouble begins when we want to enable the faster read modes. The
> RDID command only return the same 3 bytes for all 3 chips, so that
> doesn't really help.
> 
> Instead, we can use the SPI_NOR_TRY_SFDP flag, which forces the spi-nor
> system to try using SFDP, but fallback to the parameters specified in
> struct flash_info.
> 
> This way, boards using MX25L3205D will continue as before this change.
> That is without taking advantage of the 1-2-2 that it supports.
> 
> For MX25L3206E and MX25L3233F, the SFDP parameters are used, and they will
> therefore be using the optimal dual or quad mode supported by the flash
> and the SPI controller it is attached to.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/mtd/spi-nor/macronix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index ea6be95e75a5..090f28e05a5d 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -61,7 +61,7 @@ static const struct flash_info macronix_nor_parts[] = {
>  		.id = SNOR_ID(0xc2, 0x20, 0x16),
>  		.name = "mx25l3205d",
>  		.size = SZ_4M,
> -		.no_sfdp_flags = SECT_4K,
> +		.no_sfdp_flags = SECT_4K | SPI_NOR_TRY_SFDP,
>  	}, {

let's remove support for MX25L3205D. You'll then be able to drop the
flash entry altogether and instead rely on SFDP to discover the flash's
capabilities.

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
> On 6/3/24 14:09, Esben Haabendal wrote:
> > A dedicated flag for triggering call to
> > spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
> > and parse, with fallback to legacy flash parameters, without having dual,
> > quad or octal parameters set in the legacy flash parameters.
> > 
> > With this, spi-nor flash parts without SFDP that is replaced with a
> > different flash NOR flash part that does have SFDP, but shares the same
> > manufacturer and device ID is easily handled.
> > 
> > Signed-off-by: Esben Haabendal <esben@geanix.com>
> > ---
> >  drivers/mtd/spi-nor/core.c | 3 ++-
> >  drivers/mtd/spi-nor/core.h | 1 +
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 3e1f1913536b..1c4d66fc993b 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
> >  
> >  	spi_nor_manufacturer_init_params(nor);
> >  
> > -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
> > +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>
> I don't like that we update deprecated methods. The solution though is
> elegant.

I actually had the same concern. But currently there is no
non-deprecated way to handle this case, right?

Right now we have the following cases:
 (1) pure SFDP parsing
 (2) non-SFDP flashes with static configuration only
 (3) legacy implementation, where the magic flags decide whether we
     use SFDP

Which case is eventually used depends on the ID of the flash -
assuming there will only be IDs which either fall into (1) *or* (2).
That assumption is clearly wrong :)

I'd propose a new case in spi_nor_init_params()
 (4) try SFDP with a fallback to the static flags from the
     flash_info db.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Tudor Ambarus]

On 6/6/24 14:59, Michael Walle wrote:
> On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>> On 6/3/24 14:09, Esben Haabendal wrote:
>>> A dedicated flag for triggering call to
>>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>>> and parse, with fallback to legacy flash parameters, without having dual,
>>> quad or octal parameters set in the legacy flash parameters.
>>>
>>> With this, spi-nor flash parts without SFDP that is replaced with a
>>> different flash NOR flash part that does have SFDP, but shares the same
>>> manufacturer and device ID is easily handled.
>>>
>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>> ---
>>>  drivers/mtd/spi-nor/core.c | 3 ++-
>>>  drivers/mtd/spi-nor/core.h | 1 +
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 3e1f1913536b..1c4d66fc993b 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>>>  
>>>  	spi_nor_manufacturer_init_params(nor);
>>>  
>>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>>
>> I don't like that we update deprecated methods. The solution though is
>> elegant.
> 
> I actually had the same concern. But currently there is no
> non-deprecated way to handle this case, right?
> 
> Right now we have the following cases:
>  (1) pure SFDP parsing
>  (2) non-SFDP flashes with static configuration only
>  (3) legacy implementation, where the magic flags decide whether we
>      use SFDP
> 
> Which case is eventually used depends on the ID of the flash -
> assuming there will only be IDs which either fall into (1) *or* (2).
> That assumption is clearly wrong :)
> 
> I'd propose a new case in spi_nor_init_params()
>  (4) try SFDP with a fallback to the static flags from the
>      flash_info db.
> 

that's not that bad, but I would avoid doing it if it's not common. You
also have to update the core a bit, you can't use no_sfdp_flags &
TRY_SFDP, it's misleading. Does it worth it?

I won't oppose too much, but to me it feels that we're trying to keep
alive a dead man.

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 3182 bytes --]

On Thu Jun 6, 2024 at 4:52 PM CEST, Tudor Ambarus wrote:
> On 6/6/24 14:59, Michael Walle wrote:
> > On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
> >> On 6/3/24 14:09, Esben Haabendal wrote:
> >>> A dedicated flag for triggering call to
> >>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
> >>> and parse, with fallback to legacy flash parameters, without having dual,
> >>> quad or octal parameters set in the legacy flash parameters.
> >>>
> >>> With this, spi-nor flash parts without SFDP that is replaced with a
> >>> different flash NOR flash part that does have SFDP, but shares the same
> >>> manufacturer and device ID is easily handled.
> >>>
> >>> Signed-off-by: Esben Haabendal <esben@geanix.com>
> >>> ---
> >>>  drivers/mtd/spi-nor/core.c | 3 ++-
> >>>  drivers/mtd/spi-nor/core.h | 1 +
> >>>  2 files changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >>> index 3e1f1913536b..1c4d66fc993b 100644
> >>> --- a/drivers/mtd/spi-nor/core.c
> >>> +++ b/drivers/mtd/spi-nor/core.c
> >>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
> >>>  
> >>>  	spi_nor_manufacturer_init_params(nor);
> >>>  
> >>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
> >>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
> >>
> >> I don't like that we update deprecated methods. The solution though is
> >> elegant.
> > 
> > I actually had the same concern. But currently there is no
> > non-deprecated way to handle this case, right?
> > 
> > Right now we have the following cases:
> >  (1) pure SFDP parsing
> >  (2) non-SFDP flashes with static configuration only
> >  (3) legacy implementation, where the magic flags decide whether we
> >      use SFDP
> > 
> > Which case is eventually used depends on the ID of the flash -
> > assuming there will only be IDs which either fall into (1) *or* (2).
> > That assumption is clearly wrong :)
> > 
> > I'd propose a new case in spi_nor_init_params()
> >  (4) try SFDP with a fallback to the static flags from the
> >      flash_info db.
> > 
>
> that's not that bad, but I would avoid doing it if it's not common. You
> also have to update the core a bit, you can't use no_sfdp_flags &
> TRY_SFDP, it's misleading. Does it worth it?

IMHO no_sfdp_flags is the correct place (maybe TRY_SFDP is wrong,
maybe SFDP_FALLBACK?) because the flash is first treated like in
case (2). Then SFDP is tried based on that flag. Is it worth it? I
don't know, Esben is doing the development here ;) So up to him.

> I won't oppose too much, but to me it feels that we're trying to keep
> alive a dead man.

Maybe, but we'd have a readily solution if we face a similar
problem in the future. I'm really not sure, how many flashes there
are, but I think these magic bits (which tells the legacy
implementation to try SFDP) will mask quite a few of these.
I.e. in an ideal world where we could finally drop case (3) and
you'd need to split the flashes between case (1) or (2), I think
there will be quite some in (4).

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Esben Haabendal]

Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> On 6/3/24 14:09, Esben Haabendal wrote:
>> A dedicated flag for triggering call to
>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>> and parse, with fallback to legacy flash parameters, without having dual,
>> quad or octal parameters set in the legacy flash parameters.
>> 
>> With this, spi-nor flash parts without SFDP that is replaced with a
>> different flash NOR flash part that does have SFDP, but shares the same
>> manufacturer and device ID is easily handled.
>> 
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/mtd/spi-nor/core.c | 3 ++-
>>  drivers/mtd/spi-nor/core.h | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 3e1f1913536b..1c4d66fc993b 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>>  
>>  	spi_nor_manufacturer_init_params(nor);
>>  
>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>
> I don't like that we update deprecated methods. The solution though is
> elegant.

Maybe we should un-deprecate it? I don't understand why it should be
deprecated. It obviously has a valid purpose.

>> +					SPI_NOR_DUAL_READ |
>>  					SPI_NOR_QUAD_READ |
>>  					SPI_NOR_OCTAL_READ |
>>  					SPI_NOR_OCTAL_DTR_READ))
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 442786685515..77f61a984cb3 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -535,6 +535,7 @@ struct flash_info {
>>  	u8 no_sfdp_flags;
>>  #define SPI_NOR_SKIP_SFDP		BIT(0)
>>  #define SECT_4K				BIT(1)
>> +#define SPI_NOR_TRY_SFDP		BIT(2)
>>  #define SPI_NOR_DUAL_READ		BIT(3)
>>  #define SPI_NOR_QUAD_READ		BIT(4)
>>  #define SPI_NOR_OCTAL_READ		BIT(5)
>> 

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Esben Haabendal]

"Michael Walle" <mwalle@kernel.org> writes:

> On Thu Jun 6, 2024 at 4:52 PM CEST, Tudor Ambarus wrote:
>> On 6/6/24 14:59, Michael Walle wrote:
>> > On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>> >> On 6/3/24 14:09, Esben Haabendal wrote:
>> >>> A dedicated flag for triggering call to
>> >>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>> >>> and parse, with fallback to legacy flash parameters, without having dual,
>> >>> quad or octal parameters set in the legacy flash parameters.
>> >>>
>> >>> With this, spi-nor flash parts without SFDP that is replaced with a
>> >>> different flash NOR flash part that does have SFDP, but shares the same
>> >>> manufacturer and device ID is easily handled.
>> >>>
>> >>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> >>> ---
>> >>>  drivers/mtd/spi-nor/core.c | 3 ++-
>> >>>  drivers/mtd/spi-nor/core.h | 1 +
>> >>>  2 files changed, 3 insertions(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> >>> index 3e1f1913536b..1c4d66fc993b 100644
>> >>> --- a/drivers/mtd/spi-nor/core.c
>> >>> +++ b/drivers/mtd/spi-nor/core.c
>> >>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>> >>>  
>> >>>  	spi_nor_manufacturer_init_params(nor);
>> >>>  
>> >>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>> >>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>> >>
>> >> I don't like that we update deprecated methods. The solution though is
>> >> elegant.
>> > 
>> > I actually had the same concern. But currently there is no
>> > non-deprecated way to handle this case, right?
>> > 
>> > Right now we have the following cases:
>> >  (1) pure SFDP parsing
>> >  (2) non-SFDP flashes with static configuration only
>> >  (3) legacy implementation, where the magic flags decide whether we
>> >      use SFDP
>> > 
>> > Which case is eventually used depends on the ID of the flash -
>> > assuming there will only be IDs which either fall into (1) *or* (2).
>> > That assumption is clearly wrong :)
>> > 
>> > I'd propose a new case in spi_nor_init_params()
>> >  (4) try SFDP with a fallback to the static flags from the
>> >      flash_info db.
>> > 
>>
>> that's not that bad, but I would avoid doing it if it's not common. You
>> also have to update the core a bit, you can't use no_sfdp_flags &
>> TRY_SFDP, it's misleading. Does it worth it?
>
> IMHO no_sfdp_flags is the correct place (maybe TRY_SFDP is wrong,
> maybe SFDP_FALLBACK?)

TRY_SFDP might not be the best choice. But SFDP_FALLBACK sounds to me
like it is fallback _to_ SFDP, so rather counter-intuitive.

> because the flash is first treated like in
> case (2). Then SFDP is tried based on that flag.

It is first treated like in case (2), and then tried for case (1),
falling back to the result from case (2) if/when case (1) fails.

> Is it worth it? I
> don't know, Esben is doing the development here ;) So up to him.

I am not sure exactly how it should look like, but I do like the idea
proposed above, case (4). It is easier to describe and understand than
the current legacy implementation.

>> I won't oppose too much, but to me it feels that we're trying to keep
>> alive a dead man.
>
> Maybe, but we'd have a readily solution if we face a similar
> problem in the future. I'm really not sure, how many flashes there
> are, but I think these magic bits (which tells the legacy
> implementation to try SFDP) will mask quite a few of these.
> I.e. in an ideal world where we could finally drop case (3) and
> you'd need to split the flashes between case (1) or (2), I think
> there will be quite some in (4).

I like this. Judging by the way Macronix is handling this particular
chip, I strongly assume that there are several other examples of this
for other Macronix parts. Of-course, as long as the original part using
the particular flash id supported SFDP, and all later flashes using the
same id also does, none of this is needed.

/Esben

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Esben Haabendal]

Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> On 6/6/24 14:59, Michael Walle wrote:
>> On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>>> On 6/3/24 14:09, Esben Haabendal wrote:
>>>> A dedicated flag for triggering call to
>>>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>>>> and parse, with fallback to legacy flash parameters, without having dual,
>>>> quad or octal parameters set in the legacy flash parameters.
>>>>
>>>> With this, spi-nor flash parts without SFDP that is replaced with a
>>>> different flash NOR flash part that does have SFDP, but shares the same
>>>> manufacturer and device ID is easily handled.
>>>>
>>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/core.c | 3 ++-
>>>>  drivers/mtd/spi-nor/core.h | 1 +
>>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 3e1f1913536b..1c4d66fc993b 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>>>>  
>>>>  	spi_nor_manufacturer_init_params(nor);
>>>>  
>>>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>>>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>>>
>>> I don't like that we update deprecated methods. The solution though is
>>> elegant.
>> 
>> I actually had the same concern. But currently there is no
>> non-deprecated way to handle this case, right?
>> 
>> Right now we have the following cases:
>>  (1) pure SFDP parsing
>>  (2) non-SFDP flashes with static configuration only
>>  (3) legacy implementation, where the magic flags decide whether we
>>      use SFDP
>> 
>> Which case is eventually used depends on the ID of the flash -
>> assuming there will only be IDs which either fall into (1) *or* (2).
>> That assumption is clearly wrong :)
>> 
>> I'd propose a new case in spi_nor_init_params()
>>  (4) try SFDP with a fallback to the static flags from the
>>      flash_info db.
>> 
>
> that's not that bad, but I would avoid doing it if it's not common. You
> also have to update the core a bit, you can't use no_sfdp_flags &
> TRY_SFDP, it's misleading. Does it worth it?
>
> I won't oppose too much, but to me it feels that we're trying to keep
> alive a dead man.

Which dead man are you referring to? The legacy implementation or the
old non-SFDP Macronix part?

/Esben

Re: [PATCH v2 2/2] mtd: spi-nor: macronix: enable quad/dual speed for mx25l3205d chips

[Esben Haabendal]

Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> On 6/3/24 14:09, Esben Haabendal wrote:
>> Macronix engineers apparantly do not understand the purpose of having
>> an ID actually identify the chip and its capabilities. Sigh.
>> 
>> The original Macronix SPI NOR flash that identifies itself as 0xC22016
>> with RDID was MX25L3205D. This chip does not support SFDP, but does
>> support the 2READ command (1-2-2).
>> 
>> When Macronix announced EoL for MX25L3205D, the recommended
>> replacement part was MX25L3206E, which conveniently also identifies
>> itself as 0xC22016. It does not support 2READ, but supports DREAD
>> (1-1-2) instead, and supports SFDP for discovering this.
>> 
>> When Macronix announced EoL for MX25L3206E, the recommended
>> replacement part was MX25L3233F, which also identifies itself as
>> 0xC22016. It supports DREAD, 2READ, and the quad modes QREAD (1-1-4)
>> and 4READ (1-4-4). This also support SFDP.
>> 
>> So far, all of these chips have been handled the same way by the Linux
>> driver. The SFDP information have not been read, and no dual and quad
>> read modes have been enabled.
>> 
>> The trouble begins when we want to enable the faster read modes. The
>> RDID command only return the same 3 bytes for all 3 chips, so that
>> doesn't really help.
>> 
>> Instead, we can use the SPI_NOR_TRY_SFDP flag, which forces the spi-nor
>> system to try using SFDP, but fallback to the parameters specified in
>> struct flash_info.
>> 
>> This way, boards using MX25L3205D will continue as before this change.
>> That is without taking advantage of the 1-2-2 that it supports.
>> 
>> For MX25L3206E and MX25L3233F, the SFDP parameters are used, and they will
>> therefore be using the optimal dual or quad mode supported by the flash
>> and the SPI controller it is attached to.
>> 
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/mtd/spi-nor/macronix.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
>> index ea6be95e75a5..090f28e05a5d 100644
>> --- a/drivers/mtd/spi-nor/macronix.c
>> +++ b/drivers/mtd/spi-nor/macronix.c
>> @@ -61,7 +61,7 @@ static const struct flash_info macronix_nor_parts[] = {
>>  		.id = SNOR_ID(0xc2, 0x20, 0x16),
>>  		.name = "mx25l3205d",
>>  		.size = SZ_4M,
>> -		.no_sfdp_flags = SECT_4K,
>> +		.no_sfdp_flags = SECT_4K | SPI_NOR_TRY_SFDP,
>>  	}, {
>
> let's remove support for MX25L3205D. You'll then be able to drop the
> flash entry altogether and instead rely on SFDP to discover the flash's
> capabilities.

So anybody updating their Linux kernel for boards using MX25L3205D will
get a bad surprise? While in the embedded world, upgrading Linux kernel
is not the common case, it doesn't seem right to knowingly make it
difficult to those few who actually tries to do the right thing.

/Esben

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Tudor Ambarus]

On 6/6/24 18:20, Esben Haabendal wrote:
> "Michael Walle" <mwalle@kernel.org> writes:
> 
>> On Thu Jun 6, 2024 at 4:52 PM CEST, Tudor Ambarus wrote:
>>> On 6/6/24 14:59, Michael Walle wrote:
>>>> On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>>>>> On 6/3/24 14:09, Esben Haabendal wrote:
>>>>>> A dedicated flag for triggering call to
>>>>>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>>>>>> and parse, with fallback to legacy flash parameters, without having dual,
>>>>>> quad or octal parameters set in the legacy flash parameters.
>>>>>>
>>>>>> With this, spi-nor flash parts without SFDP that is replaced with a
>>>>>> different flash NOR flash part that does have SFDP, but shares the same
>>>>>> manufacturer and device ID is easily handled.
>>>>>>
>>>>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>>>>> ---
>>>>>>  drivers/mtd/spi-nor/core.c | 3 ++-
>>>>>>  drivers/mtd/spi-nor/core.h | 1 +
>>>>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>>> index 3e1f1913536b..1c4d66fc993b 100644
>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>>>>>>  
>>>>>>  	spi_nor_manufacturer_init_params(nor);
>>>>>>  
>>>>>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>>>>>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>>>>>
>>>>> I don't like that we update deprecated methods. The solution though is
>>>>> elegant.
>>>>
>>>> I actually had the same concern. But currently there is no
>>>> non-deprecated way to handle this case, right?
>>>>
>>>> Right now we have the following cases:
>>>>  (1) pure SFDP parsing
>>>>  (2) non-SFDP flashes with static configuration only
>>>>  (3) legacy implementation, where the magic flags decide whether we
>>>>      use SFDP
>>>>
>>>> Which case is eventually used depends on the ID of the flash -
>>>> assuming there will only be IDs which either fall into (1) *or* (2).
>>>> That assumption is clearly wrong :)
>>>>
>>>> I'd propose a new case in spi_nor_init_params()
>>>>  (4) try SFDP with a fallback to the static flags from the
>>>>      flash_info db.
>>>>
>>>
>>> that's not that bad, but I would avoid doing it if it's not common. You
>>> also have to update the core a bit, you can't use no_sfdp_flags &
>>> TRY_SFDP, it's misleading. Does it worth it?
>>
>> IMHO no_sfdp_flags is the correct place (maybe TRY_SFDP is wrong,
>> maybe SFDP_FALLBACK?)
> 
> TRY_SFDP might not be the best choice. But SFDP_FALLBACK sounds to me
> like it is fallback _to_ SFDP, so rather counter-intuitive.
> 
>> because the flash is first treated like in
>> case (2). Then SFDP is tried based on that flag.
> 
> It is first treated like in case (2), and then tried for case (1),
> falling back to the result from case (2) if/when case (1) fails.
> 
>> Is it worth it? I
>> don't know, Esben is doing the development here ;) So up to him.
> 
> I am not sure exactly how it should look like, but I do like the idea
> proposed above, case (4). It is easier to describe and understand than
> the current legacy implementation.
> 
>>> I won't oppose too much, but to me it feels that we're trying to keep
>>> alive a dead man.
>>
>> Maybe, but we'd have a readily solution if we face a similar
>> problem in the future. I'm really not sure, how many flashes there
>> are, but I think these magic bits (which tells the legacy
>> implementation to try SFDP) will mask quite a few of these.
>> I.e. in an ideal world where we could finally drop case (3) and
>> you'd need to split the flashes between case (1) or (2), I think
>> there will be quite some in (4).
> 
> I like this. Judging by the way Macronix is handling this particular
> chip, I strongly assume that there are several other examples of this
> for other Macronix parts. Of-course, as long as the original part using
> the particular flash id supported SFDP, and all later flashes using the
> same id also does, none of this is needed.
> 

okay, let's implement 4/

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Esben Haabendal]

Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> On 6/6/24 18:20, Esben Haabendal wrote:
>> "Michael Walle" <mwalle@kernel.org> writes:
>> 
>>> On Thu Jun 6, 2024 at 4:52 PM CEST, Tudor Ambarus wrote:
>>>> On 6/6/24 14:59, Michael Walle wrote:
>>>>> On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>>>>>> On 6/3/24 14:09, Esben Haabendal wrote:
>>>>>>> A dedicated flag for triggering call to
>>>>>>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>>>>>>> and parse, with fallback to legacy flash parameters, without having dual,
>>>>>>> quad or octal parameters set in the legacy flash parameters.
>>>>>>>
>>>>>>> With this, spi-nor flash parts without SFDP that is replaced with a
>>>>>>> different flash NOR flash part that does have SFDP, but shares the same
>>>>>>> manufacturer and device ID is easily handled.
>>>>>>>
>>>>>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>>>>>> ---
>>>>>>>  drivers/mtd/spi-nor/core.c | 3 ++-
>>>>>>>  drivers/mtd/spi-nor/core.h | 1 +
>>>>>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>>>> index 3e1f1913536b..1c4d66fc993b 100644
>>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>>>>>>>  
>>>>>>>  	spi_nor_manufacturer_init_params(nor);
>>>>>>>  
>>>>>>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>>>>>>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>>>>>>
>>>>>> I don't like that we update deprecated methods. The solution though is
>>>>>> elegant.
>>>>>
>>>>> I actually had the same concern. But currently there is no
>>>>> non-deprecated way to handle this case, right?
>>>>>
>>>>> Right now we have the following cases:
>>>>>  (1) pure SFDP parsing
>>>>>  (2) non-SFDP flashes with static configuration only
>>>>>  (3) legacy implementation, where the magic flags decide whether we
>>>>>      use SFDP
>>>>>
>>>>> Which case is eventually used depends on the ID of the flash -
>>>>> assuming there will only be IDs which either fall into (1) *or* (2).
>>>>> That assumption is clearly wrong :)
>>>>>
>>>>> I'd propose a new case in spi_nor_init_params()
>>>>>  (4) try SFDP with a fallback to the static flags from the
>>>>>      flash_info db.
>>>>>
>>>>
>>>> that's not that bad, but I would avoid doing it if it's not common. You
>>>> also have to update the core a bit, you can't use no_sfdp_flags &
>>>> TRY_SFDP, it's misleading. Does it worth it?
>>>
>>> IMHO no_sfdp_flags is the correct place (maybe TRY_SFDP is wrong,
>>> maybe SFDP_FALLBACK?)
>> 
>> TRY_SFDP might not be the best choice. But SFDP_FALLBACK sounds to me
>> like it is fallback _to_ SFDP, so rather counter-intuitive.
>> 
>>> because the flash is first treated like in
>>> case (2). Then SFDP is tried based on that flag.
>> 
>> It is first treated like in case (2), and then tried for case (1),
>> falling back to the result from case (2) if/when case (1) fails.
>> 
>>> Is it worth it? I
>>> don't know, Esben is doing the development here ;) So up to him.
>> 
>> I am not sure exactly how it should look like, but I do like the idea
>> proposed above, case (4). It is easier to describe and understand than
>> the current legacy implementation.
>> 
>>>> I won't oppose too much, but to me it feels that we're trying to keep
>>>> alive a dead man.
>>>
>>> Maybe, but we'd have a readily solution if we face a similar
>>> problem in the future. I'm really not sure, how many flashes there
>>> are, but I think these magic bits (which tells the legacy
>>> implementation to try SFDP) will mask quite a few of these.
>>> I.e. in an ideal world where we could finally drop case (3) and
>>> you'd need to split the flashes between case (1) or (2), I think
>>> there will be quite some in (4).
>> 
>> I like this. Judging by the way Macronix is handling this particular
>> chip, I strongly assume that there are several other examples of this
>> for other Macronix parts. Of-course, as long as the original part using
>> the particular flash id supported SFDP, and all later flashes using the
>> same id also does, none of this is needed.
>
> okay, let's implement 4/

Great. Let's do that.

But other than avoiding the "magic flags decide whether we use SFDP",
should I be doing anything different?

I assume we should still be calling the default_init() fixup functions,
both for manufacturer and flash level. Or should we leave this for the
deprecated case only?

If the semantics is basically the same as for the deprecated, why not
simply change the implementation of the deprecated approach to what we
need? So having 3 cases:

(1) SFDP only [indicated by size==0]
(2) static config only [indicated by no_sfdp_flags & SPI_NOR_SKIP_SFDP]
(3) SFDP with fallback to static config [indicated with size!=0 and
    !(no_sfdp_flags & SPI_NOR_SKIP_SFDP]

Any reason that we should not be able to easily convert existing
depracted flash info specifications to the new SFDP with fallback to
static config?

Also I am wondering if anyone can remember or otherwise figure out why
we are doing this memcpy() dance with nor->params in
spi_nor_sfdp_init_params_deprecated()? Why not simply call
spi_nor_parse_sfdp() before
spi_nor_no_sfdp_init_params()/spi_nor_manufacturer_init_params()?

/Esben

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

Hi,

On Fri Jun 7, 2024 at 3:30 PM CEST, Esben Haabendal wrote:
> But other than avoiding the "magic flags decide whether we use SFDP",
> should I be doing anything different?
>
> I assume we should still be calling the default_init() fixup functions,
> both for manufacturer and flash level. Or should we leave this for the
> deprecated case only?
No, they have to be handled.

> If the semantics is basically the same as for the deprecated, why not
> simply change the implementation of the deprecated approach to what we
> need? So having 3 cases:
>
> (1) SFDP only [indicated by size==0]
> (2) static config only [indicated by no_sfdp_flags & SPI_NOR_SKIP_SFDP]
> (3) SFDP with fallback to static config [indicated with size!=0 and
>     !(no_sfdp_flags & SPI_NOR_SKIP_SFDP]
>
> Any reason that we should not be able to easily convert existing
> depracted flash info specifications to the new SFDP with fallback to
> static config?

IMHO the only difference is that dual/quad read will imply the sfdp
fallback. So maybe the function can really be renamed accordingly
and the "deprecated handling" is just that dual/quad read will set
the sfdp fallback flag.

> Also I am wondering if anyone can remember or otherwise figure out why
> we are doing this memcpy() dance with nor->params in
> spi_nor_sfdp_init_params_deprecated()? Why not simply call
> spi_nor_parse_sfdp() before
> spi_nor_no_sfdp_init_params()/spi_nor_manufacturer_init_params()?

spi_nor_parse_sfdp() will overwrite the parameters set by the former.
So if you'll swap the order, the latter (static) ones will overwrite
the the ones from sfdp.

The correct fix would be to make spi_nor_parse_sfdp() not fiddle
around the parameters if it might fail. The quick fix here might be
to push that memcpy into the parse function. But I haven't looked at
the code right now.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Esben Haabendal]

"Michael Walle" <mwalle@kernel.org> writes:

> On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>> On 6/3/24 14:09, Esben Haabendal wrote:
>> > A dedicated flag for triggering call to
>> > spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>> > and parse, with fallback to legacy flash parameters, without having dual,
>> > quad or octal parameters set in the legacy flash parameters.
>> > 
>> > With this, spi-nor flash parts without SFDP that is replaced with a
>> > different flash NOR flash part that does have SFDP, but shares the same
>> > manufacturer and device ID is easily handled.
>> > 
>> > Signed-off-by: Esben Haabendal <esben@geanix.com>
>> > ---
>> >  drivers/mtd/spi-nor/core.c | 3 ++-
>> >  drivers/mtd/spi-nor/core.h | 1 +
>> >  2 files changed, 3 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> > index 3e1f1913536b..1c4d66fc993b 100644
>> > --- a/drivers/mtd/spi-nor/core.c
>> > +++ b/drivers/mtd/spi-nor/core.c
>> > @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>> >  
>> >  	spi_nor_manufacturer_init_params(nor);
>> >  
>> > -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>> > +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>>
>> I don't like that we update deprecated methods. The solution though is
>> elegant.
>
> I actually had the same concern. But currently there is no
> non-deprecated way to handle this case, right?
>
> Right now we have the following cases:
>  (1) pure SFDP parsing
>  (2) non-SFDP flashes with static configuration only
>  (3) legacy implementation, where the magic flags decide whether we
>      use SFDP

Actually, in the code we have two variants of 2.

(2a) non-SFDP flashes with SPI_NOR_SKIP_SFDP set
(2b) non-SFDP flashes without SPI_NOR_SKIP_SFDP and with none of the
     DUAL/QUAD/OCTAL read bits set

These almost handled the same way. But
spi_nor_manufacturer_init_params() is only called for 2b, and not for
2a.

Is this desired behavior, or something that we want to align?

> Which case is eventually used depends on the ID of the flash -
> assuming there will only be IDs which either fall into (1) *or* (2).
> That assumption is clearly wrong :)
>
> I'd propose a new case in spi_nor_init_params()
>  (4) try SFDP with a fallback to the static flags from the
>      flash_info db.

/Esben

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]

Hi Esben,

> > I actually had the same concern. But currently there is no
> > non-deprecated way to handle this case, right?
> >
> > Right now we have the following cases:
> >  (1) pure SFDP parsing
> >  (2) non-SFDP flashes with static configuration only
> >  (3) legacy implementation, where the magic flags decide whether we
> >      use SFDP
>
> Actually, in the code we have two variants of 2.
>
> (2a) non-SFDP flashes with SPI_NOR_SKIP_SFDP set
> (2b) non-SFDP flashes without SPI_NOR_SKIP_SFDP and with none of the
>      DUAL/QUAD/OCTAL read bits set

Isn't (2b) my case (3)? The SPI_NOR_SKIP_SFDP flag was intended to
be for flashes we know for a fact, there are no SFDP tables.

I'm looking at spi_nor_init_params(). Maybe I'm missing something?

-michael

> These almost handled the same way. But
> spi_nor_manufacturer_init_params() is only called for 2b, and not for
> 2a.
>
> Is this desired behavior, or something that we want to align?
>
> > Which case is eventually used depends on the ID of the flash -
> > assuming there will only be IDs which either fall into (1) *or* (2).
> > That assumption is clearly wrong :)
> >
> > I'd propose a new case in spi_nor_init_params()
> >  (4) try SFDP with a fallback to the static flags from the
> >      flash_info db.
>
> /Esben


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Esben Haabendal]

"Michael Walle" <mwalle@kernel.org> writes:

>> > I actually had the same concern. But currently there is no
>> > non-deprecated way to handle this case, right?
>> >
>> > Right now we have the following cases:
>> >  (1) pure SFDP parsing
>> >  (2) non-SFDP flashes with static configuration only
>> >  (3) legacy implementation, where the magic flags decide whether we
>> >      use SFDP
>>
>> Actually, in the code we have two variants of 2.
>>
>> (2a) non-SFDP flashes with SPI_NOR_SKIP_SFDP set
>> (2b) non-SFDP flashes without SPI_NOR_SKIP_SFDP and with none of the
>>      DUAL/QUAD/OCTAL read bits set
>
> Isn't (2b) my case (3)? The SPI_NOR_SKIP_SFDP flag was intended to
> be for flashes we know for a fact, there are no SFDP tables.
>
> I'm looking at spi_nor_init_params(). Maybe I'm missing something?

Probably not. I might just be confusing things here.

Your case (3) is conditional. Based on the magic flags checking in
spi_nor_init_params_deprecated(), it is either doing static
configuration only (this is what I tried to redefine as case (2b)) or
parsing SFDP with fallback to static configuration.

The issue I am getting at is that while the 2 different ways to end up
doing static configuration only is almost identical, there is a slight
difference.

But I will be addressing this in a v3 patch of this series.

Coming up shortly, and looking forward to discuss it.

/Esben

>> These almost handled the same way. But
>> spi_nor_manufacturer_init_params() is only called for 2b, and not for
>> 2a.
>>
>> Is this desired behavior, or something that we want to align?
>>
>> > Which case is eventually used depends on the ID of the flash -
>> > assuming there will only be IDs which either fall into (1) *or* (2).
>> > That assumption is clearly wrong :)
>> >
>> > I'd propose a new case in spi_nor_init_params()
>> >  (4) try SFDP with a fallback to the static flags from the
>> >      flash_info db.
>>
>> /Esben