mtd: spi-nor: Fixup page size and map selection for S25FS-S

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Spansion S25FS-S family has an issue in Basic Flash Parameter Table:
DWORD-11 bits 7-4 specify write page size 512 bytes. In reality this
is configurable in the non-volatile CR3NV register and even factory
default configuration is "wrap at 256 bytes". So blind relying on BFPT
breaks write operation on these Flashes.

All this story is vendor-specific, so add the corresponding fixup hook
which first restores the safe page size of 256 bytes from
struct flash_info but checks is more performant 512 bytes configuration
is active and adjusts the page_size accordingly.

To read CR3V RDAR command is required which in turn requires addr width
and read latency to be configured, which was not the case before. Setting
these parameters is also later required for sector map selection, because:

JESD216 allows "variable address length" and "variable latency" in
Configuration Detection Command Descriptors, in other words "as-is".
And they are still unset during Sector Map Parameter Table parsing,
which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.

New warning is added to catch the potential misconfiguration with other
chips.

Link: http://lists.infradead.org/pipermail/linux-mtd/2020-February/093950.html
Link: http://lists.infradead.org/pipermail/linux-mtd/2018-December/085956.html
Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables")
Cc: stable@vger.kernel.org
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
Yes, this is a combination of two previously sent patches as it turned out,
width/dummy quirk is necessary even earlier, during post_bfpt fixup.

 drivers/mtd/spi-nor/spi-nor.c | 132 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/spi-nor.h   |  11 ++++
 2 files changed, 141 insertions(+), 2 deletions(-)

Hi，

在 2020/2/27 20:36, Alexander A Sverdlin 写道:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>
> Spansion S25FS-S family has an issue in Basic Flash Parameter Table:
> DWORD-11 bits 7-4 specify write page size 512 bytes. In reality this
> is configurable in the non-volatile CR3NV register and even factory
> default configuration is "wrap at 256 bytes". So blind relying on BFPT
> breaks write operation on these Flashes.
>
> All this story is vendor-specific, so add the corresponding fixup hook
> which first restores the safe page size of 256 bytes from
> struct flash_info but checks is more performant 512 bytes configuration
> is active and adjusts the page_size accordingly.
>
> To read CR3V RDAR command is required which in turn requires addr width
> and read latency to be configured, which was not the case before. Setting
> these parameters is also later required for sector map selection, because:
>
> JESD216 allows "variable address length" and "variable latency" in
> Configuration Detection Command Descriptors, in other words "as-is".
> And they are still unset during Sector Map Parameter Table parsing,
> which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.
>
> New warning is added to catch the potential misconfiguration with other
> chips.
>
> Link: http://lists.infradead.org/pipermail/linux-mtd/2020-February/093950.html
> Link: http://lists.infradead.org/pipermail/linux-mtd/2018-December/085956.html
> Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
> Yes, this is a combination of two previously sent patches as it turned out,
> width/dummy quirk is necessary even earlier, during post_bfpt fixup.
>
>   drivers/mtd/spi-nor/spi-nor.c | 132 +++++++++++++++++++++++++++++++++++++++++-
>   include/linux/mtd/spi-nor.h   |  11 ++++
>   2 files changed, 141 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 1224247..1d0e2ef 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2326,6 +2326,122 @@ static struct spi_nor_fixups gd25q256_fixups = {
>   	.default_init = gd25q256_default_init,
>   };
>   
> +/*
> + * Return true if it was possible to read something known and non-zero with
> + * the probed parameters. struct spi_nor is updated in this case as well.
> + */
> +static bool spi_nor_probe_width_and_dummy(struct spi_nor *nor, u8 width,
> +					  u8 dummy)
> +{
> +	u8 read_opcode = nor->read_opcode;
> +	u8 savedw = nor->addr_width;
> +	u8 savedd = nor->read_dummy;
> +	int ret;
> +	u8 buf;
> +
> +	nor->addr_width = width;
> +	nor->read_dummy = dummy;
> +	nor->read_opcode = SPINOR_OP_RDAR;
> +	ret = spi_nor_read_data(nor, SPINOR_REG_CR2V, 1, &buf);
> +	nor->read_opcode = read_opcode;
> +
> +	if (ret == 1 && (CR2V_RL(buf) == dummy) &&
> +	    (!!(buf & CR2V_AL) == (width == 4)))
> +		return true;
> +
> +	nor->addr_width = savedw;
> +	nor->read_dummy = savedd;
> +
> +	return false;
> +}
> +
> +/*
> + * JESD216 allows to omit particular address length or latency specification in
> + * the header and at this point they are still unset, so we need some
> + * heuristics. One example is S25FS128S.
> + *
> + * It was observed that RDAR with incorrect parameters result in all-zeroes or
> + * all-ones reads. That's why probed dummy is limited to 14 and loops are built
> + * in a way to probe width 3 and 0 dummy bits last to avoid false-positive
> + * (refer to CR2 mapping). 8 dummy bits are probed on the first iteration.
> + */
> +static void spi_nor_fixup_width_and_dummy(struct spi_nor *nor)
> +{
> +	u8 width_min = 3;
> +	u8 width_max = 4;
> +	u8 dummy_min = 0;
> +	u8 dummy_max = 14;
> +	u8 w, d;
> +
> +	if (nor->addr_width && nor->read_dummy)
> +		return;
> +
> +	if (nor->addr_width) {
> +		width_min = nor->addr_width;
> +		width_max = nor->addr_width;
> +	}
> +	if (nor->read_dummy) {
> +		dummy_min = nor->read_dummy;
> +		dummy_max = nor->read_dummy;
> +	}
> +
> +	for (w = width_min; w <= width_max; ++w)
> +		for (d = 8; d <= dummy_max; ++d)
> +			if (d >= dummy_min &&
> +			    spi_nor_probe_width_and_dummy(nor, w, d))
> +				return;
> +	for (w = width_max; w >= width_min; --w)
> +		for (d = 7; d >= dummy_min; --d)
> +			if (d <= dummy_max &&
> +			    spi_nor_probe_width_and_dummy(nor, w, d))
> +				return;
> +}
> +
> +/* Spansion S25FS-S SFDP workarounds */
> +static int s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
> +	const struct sfdp_parameter_header *bfpt_header,
> +	const struct sfdp_bfpt *bfpt,
> +	struct spi_nor_flash_parameter *params)
> +{
> +	const struct flash_info *info = nor->info;
> +	u8 read_opcode, buf;
> +	int ret;
> +
> +	/*
> +	 * RDAR command below requires nor->addr_width and nor->dummy correctly
> +	 * set. spi_nor_get_map_in_use() later requires them as well.
> +	 */
> +	spi_nor_fixup_width_and_dummy(nor);
> +
> +	/* Default is safe */
> +	params->page_size = info->page_size;
> +
> +	/*
> +	 * But is the chip configured for more performant 512 bytes write page
> +	 * size?
> +	 */
> +	read_opcode = nor->read_opcode;
> +
> +	nor->read_opcode = SPINOR_OP_RDAR;
> +	ret = spi_nor_read_data(nor, SPINOR_REG_CR3V, 1, &buf);
> +	nor->read_opcode = read_opcode;
> +
> +	switch (ret) {
> +	case 0:
> +		return -EIO;
> +	case 1:
> +		if (buf & CR3V_02H_V)
> +			params->page_size = 512;
> +		return 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct spi_nor_fixups s25fs_s_fixups = {
> +	.post_bfpt = s25fs_s_post_bfpt_fixups,
> +};
> +
>   /* NOTE: double check command sets and memory organization when you add
>    * more nor chips.  This current list focusses on newer chips, which
>    * have been converging on command sets which including JEDEC ID.
> @@ -2560,7 +2676,8 @@ static const struct flash_info spi_nor_ids[] = {
>   			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>   	{ "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
>   			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
> +	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR)
> +			.fixups = &s25fs_s_fixups, },
>   	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>   	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
>   			SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> @@ -2570,7 +2687,8 @@ static const struct flash_info spi_nor_ids[] = {
>   	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>   	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>   	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> +	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +			.fixups = &s25fs_s_fixups, },

It seems SFDP is not supported on s25fl129p (you can check it on 
https://www.cypress.com/file/400586/download), so is it necessary to add 
this for this type flash?


>   	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
>   	{ "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16, 0) },
>   	{ "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32, 0) },
> @@ -3897,6 +4015,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>   		nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
>   		addr = smpt[i + 1];
>   
> +		switch (nor->read_opcode) {
> +		case SPINOR_OP_RDAR:
> +			if (nor->read_dummy && nor->addr_width)
> +				break;
> +			dev_warn(nor->dev, "OP 0x%02x width %u dummy %u\n",
> +				 nor->read_opcode, nor->addr_width,
> +				 nor->read_dummy);
> +			break;
> +		}
> +
>   		err = spi_nor_read_raw(nor, addr, 1, buf);
>   		if (err) {
>   			ret = ERR_PTR(err);
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index de90724..1e21592 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -116,6 +116,7 @@
>   /* Used for Spansion flashes only. */
>   #define SPINOR_OP_BRWR		0x17	/* Bank register write */
>   #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
> +#define SPINOR_OP_RDAR		0x65	/* Read Any Register */
>   
>   /* Used for Micron flashes only. */
>   #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
> @@ -152,6 +153,16 @@
>   #define SR2_QUAD_EN_BIT1	BIT(1)
>   #define SR2_QUAD_EN_BIT7	BIT(7)
>   
> +/* Used for Spansion flashes RDAR command only. */
> +#define SPINOR_REG_CR2V		0x800003
> +#define CR2V_AL			BIT(7)	/* Address Length */
> +/* Read Latency */
> +#define CR2V_RL_MASK		GENMASK(3, 0)
> +#define CR2V_RL(_nbits)		((_nbits) & CR2V_RL_MASK)
> +
> +#define SPINOR_REG_CR3V		0x800004
> +#define CR3V_02H_V		BIT(4)	/* Page Buffer Wrap */
> +
>   /* Supported SPI protocols */
>   #define SNOR_PROTO_INST_MASK	GENMASK(23, 16)
>   #define SNOR_PROTO_INST_SHIFT	16

Hi!

On 28/02/2020 04:01, chenxiang (M) wrote:
>> JESD216 allows "variable address length" and "variable latency" in
>> Configuration Detection Command Descriptors, in other words "as-is".
>> And they are still unset during Sector Map Parameter Table parsing,
>> which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.

[...]

>> @@ -2570,7 +2687,8 @@ static const struct flash_info spi_nor_ids[] = {
>>       { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>>       { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>>       { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> -    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>> +    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +            .fixups = &s25fs_s_fixups, },
> 
> It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download), so is it necessary to add this for this type flash?

Yes, all of the above is necessary to repair S25FS128S, which supports SFDP and lands
in the above table entry.

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables").

The bot has tested the following trees: v5.5.6, v5.4.22, v4.19.106, v4.14.171.

v5.5.6: Build OK!
v5.4.22: Build OK!
v4.19.106: Failed to apply! Possible dependencies:
    1d5ceff25aa1 ("mtd: spi_nor: pass DMA-able buffer to spi_nor_read_raw()")
    2aaa5f7e0c07 ("mtd: spi-nor: Add a post BFPT parsing fixup hook")
    2bffa65da43e ("mtd: spi-nor: Add a post BFPT fixup for MX25L25635E")
    48e4d973aefe ("mtd: spi-nor: Add a default_init() fixup hook for gd25q256")
    50685024f273 ("mtd: spi-nor: split s25fl128s into s25fl128s0 and s25fl128s1")
    5390a8df769e ("mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories")
    815541713730 ("mtd: spi-nor: Add support for mx25u12835f")
    b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
    b9f07cc8207a ("mtd: spi-nor: don't overwrite errno in spi_nor_get_map_in_use()")
    c797bd81d10e ("mtd: spi-nor: fix iteration over smpt array")

v4.14.171: Failed to apply! Possible dependencies:
    1d5ceff25aa1 ("mtd: spi_nor: pass DMA-able buffer to spi_nor_read_raw()")
    2aaa5f7e0c07 ("mtd: spi-nor: Add a post BFPT parsing fixup hook")
    2bffa65da43e ("mtd: spi-nor: Add a post BFPT fixup for MX25L25635E")
    46dde01f6bab ("mtd: spi-nor: add spi_nor_init() function")
    48e4d973aefe ("mtd: spi-nor: Add a default_init() fixup hook for gd25q256")
    50685024f273 ("mtd: spi-nor: split s25fl128s into s25fl128s0 and s25fl128s1")
    5390a8df769e ("mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories")
    65153846b18c ("mtd: spi-nor: add support for GD25Q256")
    815541713730 ("mtd: spi-nor: Add support for mx25u12835f")
    b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
    b9f07cc8207a ("mtd: spi-nor: don't overwrite errno in spi_nor_get_map_in_use()")
    c797bd81d10e ("mtd: spi-nor: fix iteration over smpt array")
    e27072851bf7 ("mtd: spi-nor: add a quad_enable callback in struct flash_info")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

On 28/02/2020 06:45, Alexander Sverdlin wrote:
> Hi!
> 
> On 28/02/2020 04:01, chenxiang (M) wrote:
>>> JESD216 allows "variable address length" and "variable latency" in
>>> Configuration Detection Command Descriptors, in other words "as-is".
>>> And they are still unset during Sector Map Parameter Table parsing,
>>> which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.
> 
> [...]
> 
>>> @@ -2570,7 +2687,8 @@ static const struct flash_info spi_nor_ids[] = {
>>>        { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>>>        { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>>>        { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>> -    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>> +    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>>> +            .fixups = &s25fs_s_fixups, },
>>
>> It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download), so is it necessary to add this for this type flash?
> 
> Yes, all of the above is necessary to repair S25FS128S, which supports SFDP and lands
> in the above table entry.

So do you know how we can tell if the part is s25fl129p1 or S25FS128S? 
Is it based on family id? For the part of my board, it has the same id 
according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP 
signature is not present (signature reads as 0x4d182001 vs expected 
0x50444653). I printed the family id, and it is 81h, which seems to 
align with S25FS (which should support SFDP). Confused.

What's more, the spi-nor probe is failing for this part since I enabled 
quad spi. So I am interested to know if there is some differences 
between these part families for that.

Thanks,
John

Hi John,

On 02/03/2020 19:25, John Garry wrote:
>>>> -    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>> +    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>>>> +            .fixups = &s25fs_s_fixups, },
>>>
>>> It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download), so is it necessary to add this for this type flash?
>>
>> Yes, all of the above is necessary to repair S25FS128S, which supports SFDP and lands
>> in the above table entry.
> 
> So do you know how we can tell if the part is s25fl129p1 or S25FS128S? Is it based on family id? For the part of my board, it has the same id according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP signature is not present (signature reads as 0x4d182001 vs expected 0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
> 
> What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.

I'd say, one can distinguish them by the fact one does support SFDP and another doesn't.
Is it really necessary to distinguish them?

On 03/03/2020 14:27, Alexander Sverdlin wrote:

Hi Alexander,

> 
> On 02/03/2020 19:25, John Garry wrote:
>>>>> -    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
>>>>> +    { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>>>>> +            .fixups = &s25fs_s_fixups, },
>>>>
>>>> It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download), so is it necessary to add this for this type flash?
>>>
>>> Yes, all of the above is necessary to repair S25FS128S, which supports SFDP and lands
>>> in the above table entry.
>>
>> So do you know how we can tell if the part is s25fl129p1 or S25FS128S? Is it based on family id? For the part of my board, it has the same id according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP signature is not present (signature reads as 0x4d182001 vs expected 0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
>>
>> What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.
> 
> I'd say, one can distinguish them by the fact one does support SFDP and another doesn't.
> Is it really necessary to distinguish them?

Well it would help me to know for sure which part is on my board :)

As an example of a relevant difference, S25FS128S datasheet has CR1V and 
CR1NV, but S25FL129 only has a single configuration register, and this 
is related to quad mode enable, which I am debugging.

BTW, Have you tried to enable quad mode for your S25FS-S part?

Thanks,
John

Hi, John,

On Monday, March 2, 2020 8:25:48 PM EEST John Garry wrote:
> So do you know how we can tell if the part is s25fl129p1 or S25FS128S?
> Is it based on family id? For the part of my board, it has the same id
> according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP
> signature is not present (signature reads as 0x4d182001 vs expected

 0x4d182001 looks like the flash id, but in reversed order.

> 0x50444653). I printed the family id, and it is 81h, which seems to
> align with S25FS (which should support SFDP). Confused.
> 

We can differentiate between flashes by the family id:  80h for FL-S and 81h 
for FS-S. If I understood correctly your flash id is 0x01, 0x20, 0x18, 0x4d, 
0x01, 0x81. According to the spansion datasheets, this should identify with a 
s25fs128s1 entry. Please check the patch from below, let me know if it's ok.

> What's more, the spi-nor probe is failing for this part since I enabled
> quad spi. So I am interested to know if there is some differences
> between these part families for that.

In which conditions is it failing? Please open a separate thread.

Cheers,
ta

Author: Tudor Ambarus <tudor.ambarus@microchip.com>
Date:   Sun Apr 26 17:59:23 2020 +0300

    mtd: spi-nor: spansion: Add support for s25fs128s1
    
    The old s25fl129p1 flash uses just five bytes for manufacturer and
    device identification, while newer spansion flashes use six bytes.
    s25fs128s1 was incorrectly identified as s25fl129p1. Use INFO6
    to differentiate between them.
    
    Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 88183eba8ac1..ea72f0e5be73 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -22,6 +22,9 @@ static const struct flash_info spansion_parts[] = {
        { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
                              SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
                              USE_CLSR) },
+       { "s25fs128s1", INFO6(0x012018, 0x4d0181,  64 * 1024, 256,
+                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+                             USE_CLSR) },
        { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128,
                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
                             USE_CLSR) },

On Thursday, February 27, 2020 2:36:57 PM EEST Alexander A Sverdlin wrote:
> Spansion S25FS-S family has an issue in Basic Flash Parameter Table:

But you modify the s25fl256s0 entry. We have to fix the flash identification 
first. How about the patch from below?


Author: Tudor Ambarus <tudor.ambarus@microchip.com>
Date:   Sun Apr 26 18:33:33 2020 +0300

    mtd: spi-nor: spansion: Differentiate between s25fl256s and s25fs256s
    
    s25fs256s was identified as s25fl256s. Differentiate between them by
    the Family ID using the INFO6 macro.
    
    Fixes: c4b3eacc1dfe ("spi-nor: Recover from Spansion/Cypress errors")
    Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index ea72f0e5be73..8ea30491cdd7 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -25,15 +25,21 @@ static const struct flash_info spansion_parts[] = {
        { "s25fs128s1", INFO6(0x012018, 0x4d0181,  64 * 1024, 256,
                              SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
                              USE_CLSR) },
-       { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128,
-                            SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-                            USE_CLSR) },
-       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512,
-                            SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-                            USE_CLSR) },
+       { "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128,
+                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+                             USE_CLSR) },
+       { "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512,
+                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+                             USE_CLSR) },
        { "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
                              SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
                              SPI_NOR_HAS_LOCK | USE_CLSR) },
+       { "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128,
+                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+                             USE_CLSR) },
+       { "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512,
+                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+                             USE_CLSR) },
        { "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256,
                              SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
                              USE_CLSR) },

Hi, John,

On Sunday, April 26, 2020 6:15:24 PM EEST Tudor.Ambarus@microchip.com wrote:
> > 0x50444653). I printed the family id, and it is 81h, which seems to
> > align with S25FS (which should support SFDP). Confused.
> 
> We can differentiate between flashes by the family id:  80h for FL-S and 81h
> for FS-S. If I understood correctly your flash id is 0x01, 0x20, 0x18,
> 0x4d, 0x01, 0x81. According to the spansion datasheets, this should
> identify with a s25fs128s1 entry. Please check the patch from below, let me
> know if it's ok.

I see that Yicong already did such a patch, check https://
patchwork.ozlabs.org/project/linux-mtd/patch/1587546809-3797-1-git-send-email-
yangyicong@hisilicon.com/

> > What's more, the spi-nor probe is failing for this part since I enabled
> > quad spi. So I am interested to know if there is some differences
> > between these part families for that.

Might fail because of the page_size problem. We're trying to fix it soon.

Cheers,
ta

+ Yicong Yang

Hi Tudor,

> 
> On Monday, March 2, 2020 8:25:48 PM EEST John Garry wrote:
>> So do you know how we can tell if the part is s25fl129p1 or S25FS128S?
>> Is it based on family id? For the part of my board, it has the same id
>> according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP
>> signature is not present (signature reads as 0x4d182001 vs expected
> 
>   0x4d182001 looks like the flash id, but in reversed order.
> 
>> 0x50444653). I printed the family id, and it is 81h, which seems to
>> align with S25FS (which should support SFDP). Confused.
>>
> 
> We can differentiate between flashes by the family id:  80h for FL-S and 81h
> for FS-S. If I understood correctly your flash id is 0x01, 0x20, 0x18, 0x4d,
> 0x01, 0x81. According to the spansion datasheets, this should identify with a
> s25fs128s1 entry. Please check the patch from below, let me know if it's ok.
> 
>> What's more, the spi-nor probe is failing for this part since I enabled
>> quad spi. So I am interested to know if there is some differences
>> between these part families for that.
> 
> In which conditions is it failing? Please open a separate thread.

So my colleague Yicon debugged this, and it seems to be an issue with 
our controller. The background is that we can blacklist certain commands 
in firmware, and some relevant commands were blacklisted such that quad 
enable failed.

But we have it working now, I think. Yicon can confirm (or start a 
thread please for outstanding issues).

> 
> Cheers,
> ta

Thanks,
John

> 
> Author: Tudor Ambarus <tudor.ambarus@microchip.com>
> Date:   Sun Apr 26 17:59:23 2020 +0300
> 
>      mtd: spi-nor: spansion: Add support for s25fs128s1
>      
>      The old s25fl129p1 flash uses just five bytes for manufacturer and
>      device identification, while newer spansion flashes use six bytes.
>      s25fs128s1 was incorrectly identified as s25fl129p1. Use INFO6
>      to differentiate between them.
>      
>      Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 88183eba8ac1..ea72f0e5be73 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -22,6 +22,9 @@ static const struct flash_info spansion_parts[] = {
>          { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
>                                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                                USE_CLSR) },
> +       { "s25fs128s1", INFO6(0x012018, 0x4d0181,  64 * 1024, 256,
> +                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +                             USE_CLSR) },

I wasn't sure if you wanted to add a separate entry if it has same 
properties as other part, due to extra maintenance. It is nice to know 
the exact part, though.

>          { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128,
>                               SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                               USE_CLSR) },
> 
> 
> .
>

Hi, Alexander,

On Thursday, February 27, 2020 2:36:57 PM EEST Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Spansion S25FS-S family has an issue in Basic Flash Parameter Table:
> DWORD-11 bits 7-4 specify write page size 512 bytes. In reality this
> is configurable in the non-volatile CR3NV register and even factory
> default configuration is "wrap at 256 bytes". So blind relying on BFPT
> breaks write operation on these Flashes.
> 
> All this story is vendor-specific, so add the corresponding fixup hook
> which first restores the safe page size of 256 bytes from
> struct flash_info but checks is more performant 512 bytes configuration
> is active and adjusts the page_size accordingly.

Right, clear.

> 
> To read CR3V RDAR command is required which in turn requires addr width
> and read latency to be configured, which was not the case before. Setting
> these parameters is also later required for sector map selection, because:
> 
> JESD216 allows "variable address length" and "variable latency" in
> Configuration Detection Command Descriptors, in other words "as-is".
> And they are still unset during Sector Map Parameter Table parsing,
> which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.

Please let me know I I get this right. You need to determine the addr_width 
and read_dummy for the RDAR command, in order to determine if the 512 
page_size is active and use that instead. addr_width is not yet set because 
you probably are in the BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 case, and as of now 
the case is not treated. nor->read_dummy is not set because the read operation 
is not yet selected.

Can we safely assume that the read_dummy for the RDAR command has the same 
value as the read_dummy value used for all the array read commands except Read 
(zero latency cycles) and RSFDP (8 latency cycles)? If yes, we can determine 
read_dummy from BFPT and get rid of the try and error iteration.

Next, we can probably use the same values at the SMPT parsing, if you hit the 
SMPT_CMD_ADDRESS_LEN_USE_CURRENT and SMPT_CMD_READ_DUMMY_IS_VARIABLE cases. 

Cheers,
ta

Hi John and Tudor,


On 2020/4/27 15:27, John Garry wrote:
> + Yicong Yang
>
> Hi Tudor,
>
>>
>> On Monday, March 2, 2020 8:25:48 PM EEST John Garry wrote:
>>> So do you know how we can tell if the part is s25fl129p1 or S25FS128S?
>>> Is it based on family id? For the part of my board, it has the same id
>>> according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP
>>> signature is not present (signature reads as 0x4d182001 vs expected
>>
>>   0x4d182001 looks like the flash id, but in reversed order.
>>
>>> 0x50444653). I printed the family id, and it is 81h, which seems to
>>> align with S25FS (which should support SFDP). Confused.
>>>
>>
>> We can differentiate between flashes by the family id:  80h for FL-S and 81h
>> for FS-S. If I understood correctly your flash id is 0x01, 0x20, 0x18, 0x4d,
>> 0x01, 0x81. According to the spansion datasheets, this should identify with a
>> s25fs128s1 entry. Please check the patch from below, let me know if it's ok.
>>
>>> What's more, the spi-nor probe is failing for this part since I enabled
>>> quad spi. So I am interested to know if there is some differences
>>> between these part families for that.
>>
>> In which conditions is it failing? Please open a separate thread.
>
> So my colleague Yicon debugged this, and it seems to be an issue with our controller. The background is that we can blacklist certain commands in firmware, and some relevant commands were blacklisted such that quad enable failed.
>
> But we have it working now, I think. Yicon can confirm (or start a thread please for outstanding issues).

Yes, now the flash is fully enabled. It's the firmware matters, not about the kernel drivers nor the flash.

>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 88183eba8ac1..ea72f0e5be73 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -22,6 +22,9 @@ static const struct flash_info spansion_parts[] = {
>>          { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256,
>>                                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>                                USE_CLSR) },
>> +       { "s25fs128s1", INFO6(0x012018, 0x4d0181,  64 * 1024, 256,
>> +                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>> +                             USE_CLSR) },
>
> I wasn't sure if you wanted to add a separate entry if it has same properties as other part, due to extra maintenance. It is nice to know the exact part, though.

The flash need some post bfpt fixup. As the page size may configured as 512byte, not the 256byte
parsed from bfpt. So a separate entry is needed. Both Alexander and Sergei provide the solution,
and I add it in my patch.

Regards,
Yicong

>
>>          { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128,
>>                               SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>                               USE_CLSR) },
>>
>>
>> .
>>
>
> .
>

Hi!

On 26/04/2020 17:40, Tudor.Ambarus@microchip.com wrote:
> On Thursday, February 27, 2020 2:36:57 PM EEST Alexander A Sverdlin wrote:
>> Spansion S25FS-S family has an issue in Basic Flash Parameter Table:
> 
> But you modify the s25fl256s0 entry. We have to fix the flash identification 
> first. How about the patch from below?
> 
> 
> Author: Tudor Ambarus <tudor.ambarus@microchip.com>
> Date:   Sun Apr 26 18:33:33 2020 +0300
> 
>     mtd: spi-nor: spansion: Differentiate between s25fl256s and s25fs256s
>     
>     s25fs256s was identified as s25fl256s. Differentiate between them by
>     the Family ID using the INFO6 macro.
>     
>     Fixes: c4b3eacc1dfe ("spi-nor: Recover from Spansion/Cypress errors")

The patch itself looks fine to me except the "Fixes:" tag above, which has
a potential for improvement. The mentioned commit is not related to FL-FS
aliasing.

>     Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index ea72f0e5be73..8ea30491cdd7 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -25,15 +25,21 @@ static const struct flash_info spansion_parts[] = {
>         { "s25fs128s1", INFO6(0x012018, 0x4d0181,  64 * 1024, 256,
>                               SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                               USE_CLSR) },
> -       { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128,
> -                            SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -                            USE_CLSR) },
> -       { "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512,
> -                            SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -                            USE_CLSR) },
> +       { "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128,
> +                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +                             USE_CLSR) },
> +       { "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512,
> +                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +                             USE_CLSR) },
>         { "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256,
>                               SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                               SPI_NOR_HAS_LOCK | USE_CLSR) },
> +       { "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128,
> +                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +                             USE_CLSR) },
> +       { "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512,
> +                             SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +                             USE_CLSR) },
>         { "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256,
>                               SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                               USE_CLSR) },
> 
> 
>

Hi, Alexander,

On Monday, April 27, 2020 11:23:40 AM EEST Alexander Sverdlin wrote:
> > Fixes: c4b3eacc1dfe ("spi-nor: Recover from Spansion/Cypress errors")
> 
> The patch itself looks fine to me except the "Fixes:" tag above, which has
> a potential for improvement. The mentioned commit is not related to FL-FS
> aliasing.

Oh, right, the flash was added with the addition of the SPI NOR framework, 
will use the following instead:
Fixes: b199489d37b ("mtd: spi-nor: add the framework for SPI NOR")

Have you seen my other reply? https://www.spinics.net/lists/stable/
msg382714.html

Cheers,
ta