[PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Linus Walleij]

The Winbond "w25q128" (actual vendor name W25Q128JV)
has exactly the same flags as the sibling device
"w25q128jv". The devices both require unlocking to
enable write access.

The actual product naming between devices vs the
Linux strings in winbond.c:

0xef4018: "w25q128"   W25Q128JV-IM/JM
0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ

The latter device, "w25q128jv" supports features
named DTQ and QPI, otherwise it is the same.

Not having the right flags has the annoying side
effect that write access does not work.

After this patch I can write to the flash on the
Inteno XG6846 router.

The flash memory also supports dual and quad SPI
modes. This does not currently manifest, but by
turning on SFDP parsing, the right SPI modes are
emitted in
/sys/kernel/debug/spi-nor/spi1.0/capabilities
for this chip, so we also turn on this.

Cc: stable@vger.kernel.org
Suggested-by: Michael Walle <michael@walle.cc>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v2:
- Only add the write access flags.
- Use SFDP parsing to properly detect the various
  available SPI modes.
- Link to v1: https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c@linaro.org
---
 drivers/mtd/spi-nor/winbond.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 834d6ba5ce70..6c82e525c801 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -121,7 +121,8 @@ static const struct flash_info winbond_nor_parts[] = {
 	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16)
 		NO_SFDP_FLAGS(SECT_4K) },
 	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
-		NO_SFDP_FLAGS(SECT_4K) },
+		PARSE_SFDP
+		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
 	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
 		.fixups = &w25q256_fixups },

---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230711-spi-nor-winbond-w25q128-321a602ee267

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Tudor Ambarus]

Hi, Linus,

On 13.07.2023 00:59, Linus Walleij wrote:
> The Winbond "w25q128" (actual vendor name W25Q128JV)
> has exactly the same flags as the sibling device
> "w25q128jv". The devices both require unlocking to
> enable write access.
> 
> The actual product naming between devices vs the
> Linux strings in winbond.c:
> 
> 0xef4018: "w25q128"   W25Q128JV-IM/JM
> 0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ
> 
> The latter device, "w25q128jv" supports features
> named DTQ and QPI, otherwise it is the same.
> 
> Not having the right flags has the annoying side
> effect that write access does not work.

I guess you refer to the locking flags. Probably your flash has the non
volatile block protection (BP) bits from the Status Register set, which
means the entire flash is write protected. The factory default for these
bits is 0/disabled on this flash so someone must have played with them.
The reason why one may want write protection set is to avoid inadvertent
writes during power-up.
One can control whether to disable the software write protection at boot
time with the MTD_SPI_NOR_SWP_ configs.
> 
> After this patch I can write to the flash on the
> Inteno XG6846 router.
> 
> The flash memory also supports dual and quad SPI
> modes. This does not currently manifest, but by

The fasted mode is chosen after SFDP parsing, so you should use quad
reads if your controller also supports 4 I/O lines.
> turning on SFDP parsing, the right SPI modes are
> emitted in
> /sys/kernel/debug/spi-nor/spi1.0/capabilities
> for this chip, so we also turn on this.
> 
> Cc: stable@vger.kernel.org
> Suggested-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes in v2:
> - Only add the write access flags.
> - Use SFDP parsing to properly detect the various
>   available SPI modes.
> - Link to v1: https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c@linaro.org
> ---
>  drivers/mtd/spi-nor/winbond.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 834d6ba5ce70..6c82e525c801 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -121,7 +121,8 @@ static const struct flash_info winbond_nor_parts[] = {
>  	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16)
>  		NO_SFDP_FLAGS(SECT_4K) },
>  	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)

while here try, using INFO with INFO(0xef4018, 0, 0, 0), those
parameters shall be discovered at run-time, so we prepare to get rid of
explicitly setting them sooner or later.

> -		NO_SFDP_FLAGS(SECT_4K) },
> +		PARSE_SFDP
> +		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },

Looks good. Also I would like you to run a small sanity test, just to
make sure the flash works after your changes. You can do that with
mtd_debug utility, see an example on Miquel's commit message from:
https://lore.kernel.org/linux-mtd/d479489736ee193609816dc2003bd0fb@walle.cc/T/#m3550973e0884ec4a288d344fabd4a9c3b64af46e

Cheers,
ta

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Michael Walle]

Hi,

Am 2023-07-13 05:32, schrieb Tudor Ambarus:
> Hi, Linus,
> 
> On 13.07.2023 00:59, Linus Walleij wrote:
>> The Winbond "w25q128" (actual vendor name W25Q128JV)
>> has exactly the same flags as the sibling device
>> "w25q128jv". The devices both require unlocking to
>> enable write access.
>> 
>> The actual product naming between devices vs the
>> Linux strings in winbond.c:
>> 
>> 0xef4018: "w25q128"   W25Q128JV-IM/JM
>> 0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ
>> 
>> The latter device, "w25q128jv" supports features
>> named DTQ and QPI, otherwise it is the same.
>> 
>> Not having the right flags has the annoying side
>> effect that write access does not work.
> 
> I guess you refer to the locking flags. Probably your flash has the non
> volatile block protection (BP) bits from the Status Register set, which
> means the entire flash is write protected. The factory default for 
> these
> bits is 0/disabled on this flash so someone must have played with them.
> The reason why one may want write protection set is to avoid 
> inadvertent
> writes during power-up.
> One can control whether to disable the software write protection at 
> boot
> time with the MTD_SPI_NOR_SWP_ configs.
>> 
>> After this patch I can write to the flash on the
>> Inteno XG6846 router.
>> 
>> The flash memory also supports dual and quad SPI
>> modes. This does not currently manifest, but by
> 
> The fasted mode is chosen after SFDP parsing, so you should use quad
> reads if your controller also supports 4 I/O lines.
>> turning on SFDP parsing, the right SPI modes are
>> emitted in
>> /sys/kernel/debug/spi-nor/spi1.0/capabilities
>> for this chip, so we also turn on this.
>> 
>> Cc: stable@vger.kernel.org
>> Suggested-by: Michael Walle <michael@walle.cc>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> Changes in v2:
>> - Only add the write access flags.
>> - Use SFDP parsing to properly detect the various
>>   available SPI modes.
>> - Link to v1: 
>> https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c@linaro.org
>> ---
>>  drivers/mtd/spi-nor/winbond.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/winbond.c 
>> b/drivers/mtd/spi-nor/winbond.c
>> index 834d6ba5ce70..6c82e525c801 100644
>> --- a/drivers/mtd/spi-nor/winbond.c
>> +++ b/drivers/mtd/spi-nor/winbond.c
>> @@ -121,7 +121,8 @@ static const struct flash_info winbond_nor_parts[] 
>> = {
>>  	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16)
>>  		NO_SFDP_FLAGS(SECT_4K) },
>>  	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
> 
> while here try, using INFO with INFO(0xef4018, 0, 0, 0), those
> parameters shall be discovered at run-time, so we prepare to get rid of
> explicitly setting them sooner or later.

This is an entry matching various flash families from Winbond, see my
reply in v1. I'm not sure we should remove these as we could break the
older ones, which might or might not have SFDP tables. We don't know.

> 
>> -		NO_SFDP_FLAGS(SECT_4K) },

Thus, I'd also keep this one.

-michael

>> +		PARSE_SFDP
>> +		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> 
> Looks good. Also I would like you to run a small sanity test, just to
> make sure the flash works after your changes. You can do that with
> mtd_debug utility, see an example on Miquel's commit message from:
> https://lore.kernel.org/linux-mtd/d479489736ee193609816dc2003bd0fb@walle.cc/T/#m3550973e0884ec4a288d344fabd4a9c3b64af46e
> 
> Cheers,
> ta

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Tudor Ambarus]

On 13.07.2023 10:01, Michael Walle wrote:
> Hi,
> 
> Am 2023-07-13 05:32, schrieb Tudor Ambarus:
>> Hi, Linus,
>>
>> On 13.07.2023 00:59, Linus Walleij wrote:
>>> The Winbond "w25q128" (actual vendor name W25Q128JV)
>>> has exactly the same flags as the sibling device
>>> "w25q128jv". The devices both require unlocking to
>>> enable write access.
>>>
>>> The actual product naming between devices vs the
>>> Linux strings in winbond.c:
>>>
>>> 0xef4018: "w25q128"   W25Q128JV-IM/JM
>>> 0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ
>>>
>>> The latter device, "w25q128jv" supports features
>>> named DTQ and QPI, otherwise it is the same.
>>>
>>> Not having the right flags has the annoying side
>>> effect that write access does not work.
>>
>> I guess you refer to the locking flags. Probably your flash has the non
>> volatile block protection (BP) bits from the Status Register set, which
>> means the entire flash is write protected. The factory default for these
>> bits is 0/disabled on this flash so someone must have played with them.
>> The reason why one may want write protection set is to avoid inadvertent
>> writes during power-up.
>> One can control whether to disable the software write protection at boot
>> time with the MTD_SPI_NOR_SWP_ configs.
>>>
>>> After this patch I can write to the flash on the
>>> Inteno XG6846 router.
>>>
>>> The flash memory also supports dual and quad SPI
>>> modes. This does not currently manifest, but by
>>
>> The fasted mode is chosen after SFDP parsing, so you should use quad
>> reads if your controller also supports 4 I/O lines.
>>> turning on SFDP parsing, the right SPI modes are
>>> emitted in
>>> /sys/kernel/debug/spi-nor/spi1.0/capabilities
>>> for this chip, so we also turn on this.
>>>
>>> Cc: stable@vger.kernel.org
>>> Suggested-by: Michael Walle <michael@walle.cc>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>> Changes in v2:
>>> - Only add the write access flags.
>>> - Use SFDP parsing to properly detect the various
>>>   available SPI modes.
>>> - Link to v1:
>>> https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c@linaro.org
>>> ---
>>>  drivers/mtd/spi-nor/winbond.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/winbond.c
>>> b/drivers/mtd/spi-nor/winbond.c
>>> index 834d6ba5ce70..6c82e525c801 100644
>>> --- a/drivers/mtd/spi-nor/winbond.c
>>> +++ b/drivers/mtd/spi-nor/winbond.c
>>> @@ -121,7 +121,8 @@ static const struct flash_info
>>> winbond_nor_parts[] = {
>>>      { "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16)
>>>          NO_SFDP_FLAGS(SECT_4K) },
>>>      { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
>>
>> while here try, using INFO with INFO(0xef4018, 0, 0, 0), those
>> parameters shall be discovered at run-time, so we prepare to get rid of
>> explicitly setting them sooner or later.
> 
> This is an entry matching various flash families from Winbond, see my
> reply in v1. I'm not sure we should remove these as we could break the
> older ones, which might or might not have SFDP tables. We don't know.

I'd take the risk and break the older ones if there are some that don't
define SFDP indeed, just to handle the conflict properly. We can't
encourage code based on assumptions otherwise we'll get back to the
knotted spi-nor code that we tried to untie in the last years.

> 
>>
>>> -        NO_SFDP_FLAGS(SECT_4K) },
> 
> Thus, I'd also keep this one.
> 

Keeping this one does not have the effect that you want as SECT_4K is
used in spi_nor_no_sfdp_init_params() which is not called when
PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if
bugs will be reported, I commit I'll fix them in the same release cycle.

If both of you agree, I'll amend Linus's v4 patch when applying.

Cheers,
ta

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Michael Walle]

Hi,

>>> while here try, using INFO with INFO(0xef4018, 0, 0, 0), those
>>> parameters shall be discovered at run-time, so we prepare to get rid 
>>> of
>>> explicitly setting them sooner or later.
>> 
>> This is an entry matching various flash families from Winbond, see my
>> reply in v1. I'm not sure we should remove these as we could break the
>> older ones, which might or might not have SFDP tables. We don't know.
> 
> I'd take the risk and break the older ones if there are some that don't
> define SFDP indeed, just to handle the conflict properly. We can't
> encourage code based on assumptions otherwise we'll get back to the
> knotted spi-nor code that we tried to untie in the last years.

Wait a minute, now I'm the more conservative one of the both of
us? (:

Jokes aside, basically you are saying that if there are two flashes
with the same id, one supports JEDEC one doesn't, we can brake the
latter one.

>>>> -        NO_SFDP_FLAGS(SECT_4K) },
>> 
>> Thus, I'd also keep this one.
>> 
> 
> Keeping this one does not have the effect that you want as SECT_4K is
> used in spi_nor_no_sfdp_init_params() which is not called when
> PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if
> bugs will be reported, I commit I'll fix them in the same release 
> cycle.

Mhh, I should have been more curious to why Linus needed the PARSE_SFDP
flag in the first place. Taking a closer look, that is because in the
legacy behavior, the SFDP is only read if the chip supports dual or
quad read (whatever the rationale was for that).

Also, if PARSE_SFDP is set, none of the no_sfdp_flags are ever handled.
If the chip doesn't support the SFDP is will just fail probing.

As I'm reading the code right now, for the new behavior
it is either
  * expect the flash supports SFDP, if not, probe fails
  * expect the flash to don't support SFDP, no SFDP is ever read at all

Shouldn't we handle the third case in the new behavior, too:
  * start with the static data we have and try reading SFDP to 
amend/correct it.

If not, will you accept breakage for future flashes, too? Looking at 
winbond.c
for example, I guess all chips with 0xef40.. 0xef50.. and 0xef60.. 
supports
SFDP nowadays and most of them only have SECT_4K set.

> If both of you agree, I'll amend Linus's v4 patch when applying.

So it would be the first chip without flags at all? Then we could
drop the entry entirely :) And if we do this, then we should also
drop all the other entries for the newer winbond flashes.

If you decide to break older flashes, then I'd prefer to also drop
the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).

-michael

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Tudor Ambarus]

On 7/18/23 07:25, Michael Walle wrote:
> Hi,
> 
>>>> while here try, using INFO with INFO(0xef4018, 0, 0, 0), those
>>>> parameters shall be discovered at run-time, so we prepare to get rid of
>>>> explicitly setting them sooner or later.
>>>
>>> This is an entry matching various flash families from Winbond, see my
>>> reply in v1. I'm not sure we should remove these as we could break the
>>> older ones, which might or might not have SFDP tables. We don't know.
>>
>> I'd take the risk and break the older ones if there are some that don't
>> define SFDP indeed, just to handle the conflict properly. We can't
>> encourage code based on assumptions otherwise we'll get back to the
>> knotted spi-nor code that we tried to untie in the last years.
> 
> Wait a minute, now I'm the more conservative one of the both of
> us? (:

;)

> 
> Jokes aside, basically you are saying that if there are two flashes
> with the same id, one supports JEDEC one doesn't, we can brake the
> latter one.

I say I don't want to suffocate the code based on assumptions and I'm ready
to take the risk and break some presumably old flashes that don't support
SFDP. If we know for sure that we break old variants of this flash ID,
then we have to rework the core now. Otherwise I'll rework it when a bug
is reported.

> 
>>>>> -        NO_SFDP_FLAGS(SECT_4K) },
>>>
>>> Thus, I'd also keep this one.
>>>
>>
>> Keeping this one does not have the effect that you want as SECT_4K is
>> used in spi_nor_no_sfdp_init_params() which is not called when
>> PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if
>> bugs will be reported, I commit I'll fix them in the same release cycle.
> 
> Mhh, I should have been more curious to why Linus needed the PARSE_SFDP
> flag in the first place. Taking a closer look, that is because in the
> legacy behavior, the SFDP is only read if the chip supports dual or
> quad read (whatever the rationale was for that).

dual or quad reads are params that could be discovered in the first version of
SFDP, that why they associated them with reading SFDP. Not great, but it
worked.

> 
> Also, if PARSE_SFDP is set, none of the no_sfdp_flags are ever handled.
> If the chip doesn't support the SFDP is will just fail probing.
> 
> As I'm reading the code right now, for the new behavior
> it is either
>  * expect the flash supports SFDP, if not, probe fails
>  * expect the flash to don't support SFDP, no SFDP is ever read at all
> 

sort of. It's more elaborate than that. Check spi_nor_init_params().
> Shouldn't we handle the third case in the new behavior, too:
>  * start with the static data we have and try reading SFDP to amend/correct it.

This case is already supported and it's the old way of initializing flash
parameters. Check spi_nor_init_params_deprecated().

I don't want to do that for the SFDP capable flashes for now, otherwise
we'll have again parameters initialized all over the place, which results
in ugly hard to read code. You have the fixup hooks if you need to amend SFDP
data. And since the first table of SFDP is mandatory (BFPT), if you set PARSE_SFDP
and get an error, then you shouldn't have set PARSE_SFDP in the first place.
Optional SFDP tables return void and we have a rollback mechanism for the parameters
set in those optional tables in case of errors.

> 
> If not, will you accept breakage for future flashes, too? Looking at winbond.c
> for example, I guess all chips with 0xef40.. 0xef50.. and 0xef60.. supports
> SFDP nowadays and most of them only have SECT_4K set.

I will. Note that you have to actually have a physical flash to do changes,
I don't queue untested patches.
> 
>> If both of you agree, I'll amend Linus's v4 patch when applying.
> 
> So it would be the first chip without flags at all? Then we could
> drop the entry entirely :) And if we do this, then we should also

No, you have the locking flags that can't be discovered by parsing SFDP,
thus you need to define a flash entry for it.

> drop all the other entries for the newer winbond flashes.

If you can test it, and there's no dedicated compatible for that flash,
I'm ok to drop them.
> 
> If you decide to break older flashes, then I'd prefer to also drop
> the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).
> 

Isn't v4 already doing this? I'll amend it if not. Waiting for ack from
both you and Linus.

Cheers,
ta

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Tudor Ambarus]

On 7/18/23 08:44, Tudor Ambarus wrote:
>> If you decide to break older flashes, then I'd prefer to also drop
>> the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).
>>
> Isn't v4 already doing this? I'll amend it if not. Waiting for ack from
> both you and Linus.

It needs another round of testing if we set n_sectors and sector_size
to 0, thus Linus has to spend a little more time on it, sorry.

Let's agree on the final version and close the topic.
Cheers,
ta

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Michael Walle]

>> Jokes aside, basically you are saying that if there are two flashes
>> with the same id, one supports JEDEC one doesn't, we can brake the
>> latter one.
> 
> I say I don't want to suffocate the code based on assumptions and I'm 
> ready
> to take the risk and break some presumably old flashes that don't 
> support
> SFDP. If we know for sure that we break old variants of this flash ID,
> then we have to rework the core now. Otherwise I'll rework it when a 
> bug
> is reported.

Ok, fair enough.

>>>>>> -        NO_SFDP_FLAGS(SECT_4K) },
>>>> 
>>>> Thus, I'd also keep this one.
>>>> 
>>> 
>>> Keeping this one does not have the effect that you want as SECT_4K is
>>> used in spi_nor_no_sfdp_init_params() which is not called when
>>> PARSE_SFDP is set, which makes perfectly sense. Let's drop this and 
>>> if
>>> bugs will be reported, I commit I'll fix them in the same release 
>>> cycle.
>> 
>> Mhh, I should have been more curious to why Linus needed the 
>> PARSE_SFDP
>> flag in the first place. Taking a closer look, that is because in the
>> legacy behavior, the SFDP is only read if the chip supports dual or
>> quad read (whatever the rationale was for that).
> 
> dual or quad reads are params that could be discovered in the first 
> version of
> SFDP, that why they associated them with reading SFDP. Not great, but 
> it
> worked.
> 
>> 
>> Also, if PARSE_SFDP is set, none of the no_sfdp_flags are ever 
>> handled.
>> If the chip doesn't support the SFDP is will just fail probing.
>> 
>> As I'm reading the code right now, for the new behavior
>> it is either
>>  * expect the flash supports SFDP, if not, probe fails
>>  * expect the flash to don't support SFDP, no SFDP is ever read at all
>> 
> 
> sort of. It's more elaborate than that. Check spi_nor_init_params().

I had checked that. And there is either parse_sfdp = true, which will
fail if SFDP is not available - or SPI_NOR_SKIP_SFDP, which won't handle
SFDP at all. And then there is the third case which will handle legacy
behavior. So what am I missing? )

>> Shouldn't we handle the third case in the new behavior, too:
>>  * start with the static data we have and try reading SFDP to 
>> amend/correct it.
> 
> This case is already supported and it's the old way of initializing 
> flash
> parameters. Check spi_nor_init_params_deprecated().

But it's not encouraged for new flashes and it doesn't work. Otherwise
we wouldn't have a problem here. It (suprisingly!) only parses SFDP
if the dual or quad flags are set.

> I don't want to do that for the SFDP capable flashes for now, otherwise
> we'll have again parameters initialized all over the place, which 
> results
> in ugly hard to read code. You have the fixup hooks if you need to 
> amend SFDP
> data. And since the first table of SFDP is mandatory (BFPT), if you set 
> PARSE_SFDP
> and get an error, then you shouldn't have set PARSE_SFDP in the first 
> place.

Unless of course there are flashes with the same id where one supports
SFDP and another don't. But as you said, we can handle that one if there
is actual breakage. I'm fine with that.

> Optional SFDP tables return void and we have a rollback mechanism for 
> the parameters
> set in those optional tables in case of errors.
> 
>> 
>> If not, will you accept breakage for future flashes, too? Looking at 
>> winbond.c
>> for example, I guess all chips with 0xef40.. 0xef50.. and 0xef60.. 
>> supports
>> SFDP nowadays and most of them only have SECT_4K set.
> 
> I will. Note that you have to actually have a physical flash to do 
> changes,
> I don't queue untested patches.
> 
>> 
>>> If both of you agree, I'll amend Linus's v4 patch when applying.
>> 
>> So it would be the first chip without flags at all? Then we could
>> drop the entry entirely :) And if we do this, then we should also
> 
> No, you have the locking flags that can't be discovered by parsing 
> SFDP,
> thus you need to define a flash entry for it.

Ah, right, they were added :)

>> drop all the other entries for the newer winbond flashes.
> 
> If you can test it, and there's no dedicated compatible for that flash,
> I'm ok to drop them.

I've had a look at the dedicated compatibles, too. They are only needed
for the spi core to probe. But if I read the code correctly, it should
work just fine with the generic driver (even if it is probed by
name). Right?

>> 
>> If you decide to break older flashes, then I'd prefer to also drop
>> the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).
>> 
> 
> Isn't v4 already doing this? I'll amend it if not. Waiting for ack from
> both you and Linus.

FWIW, I'm fine with the removed no_sfdp_flags if INFO(, 0, 0, 0).

-michael

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Tudor Ambarus]

Hi, Linus,

On 7/18/23 09:32, Michael Walle wrote:
> FWIW, I'm fine with the removed no_sfdp_flags if INFO(, 0, 0, 0).

We'll need a v5 where you test again the flash with mtd_utils,
as we want to get rid of n_sectors and sectors_size and instead
determine them from SFDP. We agreed that the flash entry should
be defined with the following params:

 	{ "w25q128", INFO(0xef4018, 0, 0, 0)
		PARSE_SFDP
		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
	},

Thanks!
ta

Re: [PATCH v2] mtd: spi-nor: Correct flags for Winbond w25q128

[Linus Walleij]

On Tue, Jul 18, 2023 at 11:47 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> Hi, Linus,
>
> On 7/18/23 09:32, Michael Walle wrote:
> > FWIW, I'm fine with the removed no_sfdp_flags if INFO(, 0, 0, 0).
>
> We'll need a v5 where you test again the flash with mtd_utils,
> as we want to get rid of n_sectors and sectors_size and instead
> determine them from SFDP. We agreed that the flash entry should
> be defined with the following params:
>
>         { "w25q128", INFO(0xef4018, 0, 0, 0)
>                 PARSE_SFDP
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>         },

Roger that!

I sent a v5 which does this, tested by taking the reported
size in dmesg and divide by reported (correct) eraseblock size and
it results in the same number of sectors as well.

I put the details into the commit message.

Yours,
Linus Walleij