Re: [PATCH] mtd: spi-nor-ids: Add Winbond W25Q128JW ID

[Marek Vasut <marex@denx.de>]

On 3/9/22 05:33, Tudor.Ambarus@microchip.com wrote:

Hi,

[...]

>>   drivers/mtd/spi/spi-nor-ids.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index b551ebd75ef..e2d09fc747d 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -345,6 +345,11 @@ 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)
>>          },
>> +       {
>> +               INFO("w25q128jw", 0xef8018, 0, 64 * 1024, 256,
> 
> I'm glad I can discuss with you about the name of these winbond flashes,
> you can help with an advice. In linux we name this flash "w25q128jwm",
> but maybe we should change it.

How did you arrive at "jwm" suffix ? Per both [1] and [2] "11.1 Valid 
Part Numbers and Top Side Marking" there is no "M" PACKAGE TYPE, so that 
seems wrong.

> Winbond names this flash "W25Q128JW_DTR", you can find the name on the
> top right of each page from [1].

Per 8.1 the IDs are the same for both _DTR [1] and non-DTR [2] variants.

> What I propose:
> 1/ according to [1] W25Q128JW_IM/JM is W25Q128JW_DTR, let's use "-dtr"
>     for the M flavor.
> 2/ according to [2] W25Q128JW has two flavors: W25Q128JW-IQ/JQ and
>     W25Q128JW-IM/JM*. Since W25Q128JW_IM/JM is called W25Q128JW_DTR, let's
>     use just "W25Q128JW" for W25Q128JW-IQ/JQ, as winbond uses too.

1/ That looks wrong to me. The W25Q128JW and W25Q128JW_DTR are two 
different products of Winbond, see "Features" column in:

https://www.winbond.com/hq/product/code-storage-flash-memory/serial-nor-flash/

Density -> 128Mb

W25Q128JW
1.7V - 1.95V
133MHz
SPI 4 I/O Fixed <------

W25Q128JW_DTR
1.7V - 1.95V
133MHz
SPI / QPI / DTR <------

Then, compare the datasheets [1] and [2], notice how the _DTR variant 
supports more functionality compared to the non-DTR variant:

8.1.4 Instruction Set Table 3 (QPI Instructions)
8.1.5 Instruction Set Table 4 (DTR with SPI Instructions)
8.1.6 Instruction Set Table 5 (DTR with QPI Instructions)

So my counter-proposal would be to call it just "w25q128jw" , that's the 
lowest common denominator of the DTR and non-DTR variant. And then, use 
SFDP to determine whether the QPI/DTR extras are supported by the flash 
(and thus whether it is the DTR variant or not).

The only downside is, I only have the non-DTR variant, so I cannot dump 
the SFDP tables of the DTR variant and check whether it differs from the 
non-DTR variant. I would expect it does.

2/ That looks wrong to me too. Look at [1] for the meaning of -xM/-xQ 
suffix of the flash, it has nothing to do with the DTR support. It just 
says what is the QE bit setting in SR2:

11. ORDERING INFORMATION
Q ... with QE = 1 (fixed) in Status register-2.
N ... with QE = 1 (fixed) in Status register-2 & DRV=75%. Backward 
compatible to FW family.
M ... with QE = 0 (programmable) in Status register-2. New device ID is 
used to identify FW family

> Looking at [3], I see that W25Q128JV and W25Q128FV use the same flash ID,
> I wonder what's the difference between them. I made a proposal in linux on
> how to handle flash collisions, a v5 will follow.

https://www.winbond.com/hq/product/code-storage-flash-memory/serial-nor-flash/

Density -> 128Mb

W25Q128FV
2.7V - 3.6V
104MHz <-----------
SPI / QPI <--------

W25Q128JV
2.7V - 3.6V
133MHz <-----------
SPI 4 I/O Fixed <--

Unlike the -JV, the -FV has
Instruction Set Table 3 (QPI Instructions)

See my suggestion above, use the lowest common denominator flash (in 
this case JV) as a base and then use SFDP to determine whether extra 
features are available. Would that work ?

> Cheers,
> ta
> [1] https://www.winbond.com/resource-files/W25Q128JW_DTR%20RevF1%2005212021.pdf
> [2] https://www.winbond.com/resource-files/W25Q128JW_RevG_07292021%20Plus.pdf
> [3] https://www.winbond.com/hq/product/code-storage-flash-memory/serial-nor-flash/?__locale=en&selected=128Mb#Density
> 
>> +                       SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>> +                       SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>> +       },
>>          {
>>                  INFO("w25q256fw", 0xef6019, 0, 64 * 1024, 512,
>>                          SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>> --
>> 2.34.1
>>
> 

[...]