[PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Marek Vasut]

The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
and 256kiB uniform sectors device configuration, however according to
section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
manufactured devices, which means 64kiB sectors. Since the device does not
support 64kiB uniform sectors in any configuration, parsing SMPT table
cannot find a valid sector map entry and fails.

Fix this up by setting SMPT configuration index bit 0, which is populated
exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
fixup hook and a wrapper function. The only implementation of the hook is
currently specific to S25FS512S.

This fixes the following failure on S25FS512S:
spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Michael Walle <mwalle@kernel.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Pratyush Yadav <pratyush@kernel.org>
Cc: Richard Weinberger <richard@nod.at>
Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Cc: linux-mtd@lists.infradead.org
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
 drivers/mtd/spi-nor/core.h     |  5 +++++
 drivers/mtd/spi-nor/sfdp.c     |  4 ++++
 drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 9d6e85bf227b..ca65f36e5638 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
 	return 0;
 }
 
+int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
+{
+	int ret;
+
+	if (nor->manufacturer && nor->manufacturer->fixups &&
+	    nor->manufacturer->fixups->post_smpt) {
+		ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
+		if (ret)
+			return ret;
+	}
+
+	if (nor->info->fixups && nor->info->fixups->post_smpt)
+		return nor->info->fixups->post_smpt(nor, smpt);
+
+	return 0;
+}
+
 static int spi_nor_select_read(struct spi_nor *nor,
 			       u32 shared_hwcaps)
 {
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 1516b6d0dc37..d5294424ab9d 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
  *             parameters that could not be extracted by other means (i.e.
  *             when information provided by the SFDP/flash_info tables are
  *             incomplete or wrong).
+ * @post_smpt: update sector map configuration ID selector according to
+ *             chip-specific quirks.
  * @late_init: used to initialize flash parameters that are not declared in the
  *             JESD216 SFDP standard, or where SFDP tables not defined at all.
  *             Will replace the default_init() hook.
@@ -426,6 +428,7 @@ struct spi_nor_fixups {
 			 const struct sfdp_parameter_header *bfpt_header,
 			 const struct sfdp_bfpt *bfpt);
 	int (*post_sfdp)(struct spi_nor *nor);
+	int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
 	int (*late_init)(struct spi_nor *nor);
 };
 
@@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
 			     const struct sfdp_parameter_header *bfpt_header,
 			     const struct sfdp_bfpt *bfpt);
 
+int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
+
 void spi_nor_init_default_locking_ops(struct spi_nor *nor);
 void spi_nor_try_unlock_all(struct spi_nor *nor);
 void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 5b1117265bd2..542c775918ad 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 		map_id = map_id << 1 | !!(*buf & read_data_mask);
 	}
 
+	err = spi_nor_post_smpt_fixups(nor, &map_id);
+	if (err)
+		return ERR_PTR(err);
+
 	/*
 	 * If command descriptors are provided, they always precede map
 	 * descriptors in the table. There is no need to start the iteration
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index d6c92595f6bc..d446d12371e1 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
 	.post_bfpt = s25fs_s_nor_post_bfpt_fixups,
 };
 
+static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
+{
+	/*
+	 * The S25FS512S chip datasheet rev.O Table 71 on page 153
+	 * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
+	 * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
+	 * device configuration, however according to section 7.5.5.1
+	 * Configuration Register 3 Non-volatile (CR3NV) page 61, the
+	 * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
+	 * 0 on newly manufactured devices, which means 64kiB sectors.
+	 * Since the device does not support 64kiB uniform sectors in
+	 * any configuration, parsing SMPT table cannot find a valid
+	 * sector map entry and fails. Fix this up by setting SMPT
+	 * configuration index bit 0, which is populated exactly by
+	 * the CR3NV bit 1 being 1.
+	 */
+	*smpt |= BIT(0);
+	return 0;
+}
+
+static const struct spi_nor_fixups s25fs512s_nor_fixups = {
+	.post_bfpt = s25fs_s_nor_post_bfpt_fixups,
+	.post_smpt = s25fs512s_nor_post_smpt_fixups,
+};
+
 static const struct flash_info spansion_nor_parts[] = {
 	{
 		.id = SNOR_ID(0x01, 0x02, 0x12),
@@ -829,7 +854,7 @@ static const struct flash_info spansion_nor_parts[] = {
 		.sector_size = SZ_256K,
 		.no_sfdp_flags = SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
 		.mfr_flags = USE_CLSR,
-		.fixups = &s25fs_s_nor_fixups,
+		.fixups = &s25fs512s_nor_fixups,
 	}, {
 		.id = SNOR_ID(0x01, 0x20, 0x18, 0x03, 0x00),
 		.name = "s25sl12800",
-- 
2.45.2


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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Tudor Ambarus]

Hi, Marek,

On 9/14/24 11:08 PM, Marek Vasut wrote:
> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
> and 256kiB uniform sectors device configuration, however according to
> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
> manufactured devices, which means 64kiB sectors. Since the device does not
> support 64kiB uniform sectors in any configuration, parsing SMPT table
> cannot find a valid sector map entry and fails.
> 
> Fix this up by setting SMPT configuration index bit 0, which is populated
> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
> fixup hook and a wrapper function. The only implementation of the hook is
> currently specific to S25FS512S.
> 
> This fixes the following failure on S25FS512S:
> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Michael Walle <mwalle@kernel.org>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Pratyush Yadav <pratyush@kernel.org>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>  drivers/mtd/spi-nor/core.h     |  5 +++++
>  drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>  drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>  4 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 9d6e85bf227b..ca65f36e5638 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>  	return 0;
>  }
>  
> +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
> +{
> +	int ret;
> +
> +	if (nor->manufacturer && nor->manufacturer->fixups &&
> +	    nor->manufacturer->fixups->post_smpt) {
> +		ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (nor->info->fixups && nor->info->fixups->post_smpt)
> +		return nor->info->fixups->post_smpt(nor, smpt);
> +
> +	return 0;
> +}
> +
>  static int spi_nor_select_read(struct spi_nor *nor,
>  			       u32 shared_hwcaps)
>  {
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 1516b6d0dc37..d5294424ab9d 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>   *             parameters that could not be extracted by other means (i.e.
>   *             when information provided by the SFDP/flash_info tables are
>   *             incomplete or wrong).
> + * @post_smpt: update sector map configuration ID selector according to
> + *             chip-specific quirks.
>   * @late_init: used to initialize flash parameters that are not declared in the
>   *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>   *             Will replace the default_init() hook.
> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>  			 const struct sfdp_parameter_header *bfpt_header,
>  			 const struct sfdp_bfpt *bfpt);
>  	int (*post_sfdp)(struct spi_nor *nor);
> +	int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>  	int (*late_init)(struct spi_nor *nor);
>  };
>  
> @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>  			     const struct sfdp_parameter_header *bfpt_header,
>  			     const struct sfdp_bfpt *bfpt);
>  
> +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
> +
>  void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>  void spi_nor_try_unlock_all(struct spi_nor *nor);
>  void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 5b1117265bd2..542c775918ad 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>  		map_id = map_id << 1 | !!(*buf & read_data_mask);
>  	}
>  
> +	err = spi_nor_post_smpt_fixups(nor, &map_id);
> +	if (err)
> +		return ERR_PTR(err);
> +
>  	/*
>  	 * If command descriptors are provided, they always precede map
>  	 * descriptors in the table. There is no need to start the iteration
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index d6c92595f6bc..d446d12371e1 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>  	.post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>  };
>  
> +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
> +{
> +	/*
> +	 * The S25FS512S chip datasheet rev.O Table 71 on page 153
> +	 * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
> +	 * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
> +	 * device configuration, however according to section 7.5.5.1
> +	 * Configuration Register 3 Non-volatile (CR3NV) page 61, the
> +	 * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
> +	 * 0 on newly manufactured devices, which means 64kiB sectors.
> +	 * Since the device does not support 64kiB uniform sectors in
> +	 * any configuration, parsing SMPT table cannot find a valid
> +	 * sector map entry and fails. Fix this up by setting SMPT
> +	 * configuration index bit 0, which is populated exactly by
> +	 * the CR3NV bit 1 being 1.
> +	 */
> +	*smpt |= BIT(0);

Please help me understand this. Maybe a link to your revision of
datasheet would help me. In the flash datasheets that I see, there shall
be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
table is described showing an Index Value constructed by the CRxNV[y]
return values. That index value is the map_id in the code.

By reading your description I understand CR3NV[1] has value zero as it
is marked as RFU, but at the same time that bit is expected in SMPT to
always have value 1. That's why datasheets like this one [1] in their
"Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.

I assume what you're doing is fine as it shouldn't break backward
compatibility with other older flashes as their CR3NV[1] has value one
anyway. Correct me if I'm wrong.

Now looking at the code, what we usually do is to save the flash
parameters described by SFDP in nor->params, then amend those parameters
with fixup hooks if that's needed. Here you modify the map_id and then
let the code use it in order to determine the sector_map. Then that
sector_map (which is SMPT data from the table itself) is used to
initialize erase regions. That sector_map can contain wrong data too.

I'd suggest saving a nor->params->sector_map then call a
int spi_nor_post_smpt_fixups(struct spi_nor *nor,
	const struct sfdp_parameter_header *smpt_header,
	const u32 *smpt)
in case spi_nor_get_map_in_use() fails. This way others can update
sector_map as well if that's ever needed. What do you think?

Cheers,
ta

[1]
https://www.mouser.com/datasheet/2/100/CYPR_S_A0011121083_1-2541215.pdf?srsltid=AfmBOorpMLKdybn-E_tAUjSBW0AigP0bQ3JR3tD9VUPwf4nGVBYBC1fB


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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Marek Vasut]

On 9/23/24 10:54 AM, Tudor Ambarus wrote:

Hello Tudor,

sorry for the late reply.

> On 9/14/24 11:08 PM, Marek Vasut wrote:
>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>> and 256kiB uniform sectors device configuration, however according to
>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>> manufactured devices, which means 64kiB sectors. Since the device does not
>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>> cannot find a valid sector map entry and fails.
>>
>> Fix this up by setting SMPT configuration index bit 0, which is populated
>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>> fixup hook and a wrapper function. The only implementation of the hook is
>> currently specific to S25FS512S.
>>
>> This fixes the following failure on S25FS512S:
>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>> ---
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Michael Walle <mwalle@kernel.org>
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> Cc: Pratyush Yadav <pratyush@kernel.org>
>> Cc: Richard Weinberger <richard@nod.at>
>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>> Cc: linux-mtd@lists.infradead.org
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 9d6e85bf227b..ca65f36e5638 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>   	return 0;
>>   }
>>   
>> +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>> +{
>> +	int ret;
>> +
>> +	if (nor->manufacturer && nor->manufacturer->fixups &&
>> +	    nor->manufacturer->fixups->post_smpt) {
>> +		ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (nor->info->fixups && nor->info->fixups->post_smpt)
>> +		return nor->info->fixups->post_smpt(nor, smpt);
>> +
>> +	return 0;
>> +}
>> +
>>   static int spi_nor_select_read(struct spi_nor *nor,
>>   			       u32 shared_hwcaps)
>>   {
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 1516b6d0dc37..d5294424ab9d 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>    *             parameters that could not be extracted by other means (i.e.
>>    *             when information provided by the SFDP/flash_info tables are
>>    *             incomplete or wrong).
>> + * @post_smpt: update sector map configuration ID selector according to
>> + *             chip-specific quirks.
>>    * @late_init: used to initialize flash parameters that are not declared in the
>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>    *             Will replace the default_init() hook.
>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>   			 const struct sfdp_parameter_header *bfpt_header,
>>   			 const struct sfdp_bfpt *bfpt);
>>   	int (*post_sfdp)(struct spi_nor *nor);
>> +	int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>   	int (*late_init)(struct spi_nor *nor);
>>   };
>>   
>> @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>   			     const struct sfdp_parameter_header *bfpt_header,
>>   			     const struct sfdp_bfpt *bfpt);
>>   
>> +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>> +
>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 5b1117265bd2..542c775918ad 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>   		map_id = map_id << 1 | !!(*buf & read_data_mask);
>>   	}
>>   
>> +	err = spi_nor_post_smpt_fixups(nor, &map_id);
>> +	if (err)
>> +		return ERR_PTR(err);
>> +
>>   	/*
>>   	 * If command descriptors are provided, they always precede map
>>   	 * descriptors in the table. There is no need to start the iteration
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index d6c92595f6bc..d446d12371e1 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>   	.post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>   };
>>   
>> +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>> +{
>> +	/*
>> +	 * The S25FS512S chip datasheet rev.O Table 71 on page 153
>> +	 * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>> +	 * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>> +	 * device configuration, however according to section 7.5.5.1
>> +	 * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>> +	 * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>> +	 * 0 on newly manufactured devices, which means 64kiB sectors.
>> +	 * Since the device does not support 64kiB uniform sectors in
>> +	 * any configuration, parsing SMPT table cannot find a valid
>> +	 * sector map entry and fails. Fix this up by setting SMPT
>> +	 * configuration index bit 0, which is populated exactly by
>> +	 * the CR3NV bit 1 being 1.
>> +	 */
>> +	*smpt |= BIT(0);
> 
> Please help me understand this. Maybe a link to your revision of
> datasheet would help me.

https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf

SMPT values:

i=0 smpt[i]=08ff65fc
i=1 smpt[i]=00000004
i=2 smpt[i]=04ff65fc
i=3 smpt[i]=00000002
i=4 smpt[i]=02ff65fd
i=5 smpt[i]=00000004
i=6 smpt[i]=ff0201fe
i=7 smpt[i]=00007ff1
i=8 smpt[i]=00037ff4
i=9 smpt[i]=03fbfff4
i=10 smpt[i]=ff0203fe
i=11 smpt[i]=03fbfff4
i=12 smpt[i]=00037ff4
i=13 smpt[i]=00007ff1
i=14 smpt[i]=ff0005ff
i=15 smpt[i]=03fffff4

> In the flash datasheets that I see, there shall
> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
> table is described showing an Index Value constructed by the CRxNV[y]
> return values. That index value is the map_id in the code.
> 
> By reading your description I understand CR3NV[1] has value zero as it
> is marked as RFU, but at the same time that bit is expected in SMPT to
> always have value 1. That's why datasheets like this one [1] in their
> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.

Where does this last part "define the index value as CR3NV[3] << 2 | 
CR1NV[2] << 1 | 1" come from ?

> I assume what you're doing is fine as it shouldn't break backward
> compatibility with other older flashes as their CR3NV[1] has value one
> anyway. Correct me if I'm wrong.

I hope so.

> Now looking at the code, what we usually do is to save the flash
> parameters described by SFDP in nor->params, then amend those parameters
> with fixup hooks if that's needed. Here you modify the map_id and then
> let the code use it in order to determine the sector_map. Then that
> sector_map (which is SMPT data from the table itself) is used to
> initialize erase regions. That sector_map can contain wrong data too.

By sector_map, do you refer to the "smpt" array ?

> I'd suggest saving a nor->params->sector_map then call a
> int spi_nor_post_smpt_fixups(struct spi_nor *nor,
> 	const struct sfdp_parameter_header *smpt_header,
> 	const u32 *smpt)
> in case spi_nor_get_map_in_use() fails. This way others can update
> sector_map as well if that's ever needed. What do you think?

This won't work for me, would it ? In my case, I need to patch content 
of CR3NV register to assure CR3NV bit 1 is well defined. I don't need to 
patch the sector_map itself.

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Marek Vasut]

Hello again,

>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>> and 256kiB uniform sectors device configuration, however according to
>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>> manufactured devices, which means 64kiB sectors. Since the device 
>>> does not
>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>> cannot find a valid sector map entry and fails.
>>>
>>> Fix this up by setting SMPT configuration index bit 0, which is 
>>> populated
>>> exactly by the CR3NV bit 1 being 1. This is implemented via 
>>> new .post_smpt
>>> fixup hook and a wrapper function. The only implementation of the 
>>> hook is
>>> currently specific to S25FS512S.
>>>
>>> This fixes the following failure on S25FS512S:
>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>> ---
>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>> Cc: Michael Walle <mwalle@kernel.org>
>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>> Cc: Richard Weinberger <richard@nod.at>
>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>> Cc: linux-mtd@lists.infradead.org
>>> Cc: linux-renesas-soc@vger.kernel.org
>>> ---
>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 9d6e85bf227b..ca65f36e5638 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>       return 0;
>>>   }
>>> +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>> +        nor->manufacturer->fixups->post_smpt) {
>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>                      u32 shared_hwcaps)
>>>   {
>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>> index 1516b6d0dc37..d5294424ab9d 100644
>>> --- a/drivers/mtd/spi-nor/core.h
>>> +++ b/drivers/mtd/spi-nor/core.h
>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>    *             parameters that could not be extracted by other 
>>> means (i.e.
>>>    *             when information provided by the SFDP/flash_info 
>>> tables are
>>>    *             incomplete or wrong).
>>> + * @post_smpt: update sector map configuration ID selector according to
>>> + *             chip-specific quirks.
>>>    * @late_init: used to initialize flash parameters that are not 
>>> declared in the
>>>    *             JESD216 SFDP standard, or where SFDP tables not 
>>> defined at all.
>>>    *             Will replace the default_init() hook.
>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>                const struct sfdp_parameter_header *bfpt_header,
>>>                const struct sfdp_bfpt *bfpt);
>>>       int (*post_sfdp)(struct spi_nor *nor);
>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>       int (*late_init)(struct spi_nor *nor);
>>>   };
>>> @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>                    const struct sfdp_bfpt *bfpt);
>>> +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>> +
>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>> index 5b1117265bd2..542c775918ad 100644
>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct 
>>> spi_nor *nor, const u32 *smpt,
>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>       }
>>> +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>> +    if (err)
>>> +        return ERR_PTR(err);
>>> +
>>>       /*
>>>        * If command descriptors are provided, they always precede map
>>>        * descriptors in the table. There is no need to start the 
>>> iteration
>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/ 
>>> spansion.c
>>> index d6c92595f6bc..d446d12371e1 100644
>>> --- a/drivers/mtd/spi-nor/spansion.c
>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups 
>>> s25fs_s_nor_fixups = {
>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>   };
>>> +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 
>>> *smpt)
>>> +{
>>> +    /*
>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>> +     * device configuration, however according to section 7.5.5.1
>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>> +     * Since the device does not support 64kiB uniform sectors in
>>> +     * any configuration, parsing SMPT table cannot find a valid
>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>> +     * configuration index bit 0, which is populated exactly by
>>> +     * the CR3NV bit 1 being 1.
>>> +     */
>>> +    *smpt |= BIT(0);
>>
>> Please help me understand this. Maybe a link to your revision of
>> datasheet would help me.
> 
> https://www.infineon.com/assets/row/public/documents/10/49/infineon- 
> s25fs512s-512-mb-1-datasheet-en.pdf
> 
> SMPT values:
> 
> i=0 smpt[i]=08ff65fc
> i=1 smpt[i]=00000004
> i=2 smpt[i]=04ff65fc
> i=3 smpt[i]=00000002
> i=4 smpt[i]=02ff65fd
> i=5 smpt[i]=00000004
> i=6 smpt[i]=ff0201fe
> i=7 smpt[i]=00007ff1
> i=8 smpt[i]=00037ff4
> i=9 smpt[i]=03fbfff4
> i=10 smpt[i]=ff0203fe
> i=11 smpt[i]=03fbfff4
> i=12 smpt[i]=00037ff4
> i=13 smpt[i]=00007ff1
> i=14 smpt[i]=ff0005ff
> i=15 smpt[i]=03fffff4
> 
>> In the flash datasheets that I see, there shall
>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>> table is described showing an Index Value constructed by the CRxNV[y]
>> return values. That index value is the map_id in the code.
>>
>> By reading your description I understand CR3NV[1] has value zero as it
>> is marked as RFU, but at the same time that bit is expected in SMPT to
>> always have value 1. That's why datasheets like this one [1] in their
>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
> 
> Where does this last part "define the index value as CR3NV[3] << 2 | 
> CR1NV[2] << 1 | 1" come from ?
> 
>> I assume what you're doing is fine as it shouldn't break backward
>> compatibility with other older flashes as their CR3NV[1] has value one
>> anyway. Correct me if I'm wrong.
> 
> I hope so.
> 
>> Now looking at the code, what we usually do is to save the flash
>> parameters described by SFDP in nor->params, then amend those parameters
>> with fixup hooks if that's needed. Here you modify the map_id and then
>> let the code use it in order to determine the sector_map. Then that
>> sector_map (which is SMPT data from the table itself) is used to
>> initialize erase regions. That sector_map can contain wrong data too.
> 
> By sector_map, do you refer to the "smpt" array ?
> 
>> I'd suggest saving a nor->params->sector_map then call a
>> int spi_nor_post_smpt_fixups(struct spi_nor *nor,
>>     const struct sfdp_parameter_header *smpt_header,
>>     const u32 *smpt)
>> in case spi_nor_get_map_in_use() fails. This way others can update
>> sector_map as well if that's ever needed. What do you think?
> 
> This won't work for me, would it ? In my case, I need to patch content 
> of CR3NV register to assure CR3NV bit 1 is well defined. I don't need to 
> patch the sector_map itself.

Let me bump this discussion.

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Tudor Ambarus]

On 9/3/25 2:40 AM, Marek Vasut wrote:
> On 9/23/24 10:54 AM, Tudor Ambarus wrote:
> 
> Hello Tudor,

Hi, Marek,

> 
> sorry for the late reply.

That's okay.

> 
>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>> and 256kiB uniform sectors device configuration, however according to
>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>> cannot find a valid sector map entry and fails.
>>>
>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>> fixup hook and a wrapper function. The only implementation of the hook is
>>> currently specific to S25FS512S.
>>>
>>> This fixes the following failure on S25FS512S:
>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>> ---
>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>> Cc: Michael Walle <mwalle@kernel.org>
>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>> Cc: Richard Weinberger <richard@nod.at>
>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>> Cc: linux-mtd@lists.infradead.org
>>> Cc: linux-renesas-soc@vger.kernel.org
>>> ---
>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 9d6e85bf227b..ca65f36e5638 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>       return 0;
>>>   }
>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>> +        nor->manufacturer->fixups->post_smpt) {
>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>                      u32 shared_hwcaps)
>>>   {
>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>> index 1516b6d0dc37..d5294424ab9d 100644
>>> --- a/drivers/mtd/spi-nor/core.h
>>> +++ b/drivers/mtd/spi-nor/core.h
>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>    *             parameters that could not be extracted by other means (i.e.
>>>    *             when information provided by the SFDP/flash_info tables are
>>>    *             incomplete or wrong).
>>> + * @post_smpt: update sector map configuration ID selector according to
>>> + *             chip-specific quirks.
>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>    *             Will replace the default_init() hook.
>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>                const struct sfdp_parameter_header *bfpt_header,
>>>                const struct sfdp_bfpt *bfpt);
>>>       int (*post_sfdp)(struct spi_nor *nor);
>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>       int (*late_init)(struct spi_nor *nor);
>>>   };
>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>                    const struct sfdp_bfpt *bfpt);
>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>> +
>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>> index 5b1117265bd2..542c775918ad 100644
>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>       }
>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>> +    if (err)
>>> +        return ERR_PTR(err);
>>> +
>>>       /*
>>>        * If command descriptors are provided, they always precede map
>>>        * descriptors in the table. There is no need to start the iteration
>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>> index d6c92595f6bc..d446d12371e1 100644
>>> --- a/drivers/mtd/spi-nor/spansion.c
>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>   };
>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>> +{
>>> +    /*
>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>> +     * device configuration, however according to section 7.5.5.1
>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>> +     * Since the device does not support 64kiB uniform sectors in
>>> +     * any configuration, parsing SMPT table cannot find a valid
>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>> +     * configuration index bit 0, which is populated exactly by
>>> +     * the CR3NV bit 1 being 1.
>>> +     */
>>> +    *smpt |= BIT(0);
>>
>> Please help me understand this. Maybe a link to your revision of
>> datasheet would help me.
> 
> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
> 
> SMPT values:
> 
> i=0 smpt[i]=08ff65fc
> i=1 smpt[i]=00000004
> i=2 smpt[i]=04ff65fc
> i=3 smpt[i]=00000002
> i=4 smpt[i]=02ff65fd
> i=5 smpt[i]=00000004
> i=6 smpt[i]=ff0201fe
> i=7 smpt[i]=00007ff1
> i=8 smpt[i]=00037ff4
> i=9 smpt[i]=03fbfff4
> i=10 smpt[i]=ff0203fe
> i=11 smpt[i]=03fbfff4
> i=12 smpt[i]=00037ff4
> i=13 smpt[i]=00007ff1
> i=14 smpt[i]=ff0005ff
> i=15 smpt[i]=03fffff4
> 

thanks!

>> In the flash datasheets that I see, there shall
>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>> table is described showing an Index Value constructed by the CRxNV[y]
>> return values. That index value is the map_id in the code.
>>
>> By reading your description I understand CR3NV[1] has value zero as it
>> is marked as RFU, but at the same time that bit is expected in SMPT to
>> always have value 1. That's why datasheets like this one [1] in their
>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
> 
> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?

In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
"Index Value" column. I deduced the formula by using the CR3NV[3] and
CR1NV[2] values. The datasheet says:

"When more than one configuration bit must be read, all the bits are
concatenated into an index value that is used to select the current address map."

and that the "the following MSb to LSb order to form the configuration map
index value:
• CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
• CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
"

The "Index Value" shall be the map_id that you passed in the code:
spi_nor_post_smpt_fixups(nor, &map_id);

Can you please print the map_id value that you obtain without updating it?
Let's also print the values of CR3NV and CR1NV.

(idea for enthusiasts: we can add a debugfs interface to query registers)

> 
>> I assume what you're doing is fine as it shouldn't break backward
>> compatibility with other older flashes as their CR3NV[1] has value one
>> anyway. Correct me if I'm wrong.

Now that I revisited the datasheet, your comment "use CR3NV bit 1 to discern
64kiB/256kiB uniform sectors" is misleading because a) I understand that
CR3NV[1] is not involved in the sector map selection (the "Index Value" is),
and b) the choice as per table 70 is between uniform 256 KB sectors and
non-uniform map with 4KB sectors at top or bottom and the remaining 256KB.

Let's get those CR3NV and CR1NV values so we decide how to tackle this.

Cheers,
ta

> 
> I hope so.
> 
>> Now looking at the code, what we usually do is to save the flash
>> parameters described by SFDP in nor->params, then amend those parameters
>> with fixup hooks if that's needed. Here you modify the map_id and then
>> let the code use it in order to determine the sector_map. Then that
>> sector_map (which is SMPT data from the table itself) is used to
>> initialize erase regions. That sector_map can contain wrong data too.
> 
> By sector_map, do you refer to the "smpt" array ?
> 
>> I'd suggest saving a nor->params->sector_map then call a
>> int spi_nor_post_smpt_fixups(struct spi_nor *nor,
>>     const struct sfdp_parameter_header *smpt_header,
>>     const u32 *smpt)
>> in case spi_nor_get_map_in_use() fails. This way others can update
>> sector_map as well if that's ever needed. What do you think?
> 
> This won't work for me, would it ? In my case, I need to patch content of CR3NV register to assure CR3NV bit 1 is well defined. I don't need to patch the sector_map itself.


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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Tudor Ambarus]

+ Takahiro, he works actively with IFX flashes.

Maybe we shall add a per vendor MAINTAINERS line, so that relevant people
are added by scripts/get_maintainer.pl.

No other comment after this line.

On 10/8/25 7:20 AM, Tudor Ambarus wrote:
> 
> 
> On 9/3/25 2:40 AM, Marek Vasut wrote:
>> On 9/23/24 10:54 AM, Tudor Ambarus wrote:
>>
>> Hello Tudor,
> 
> Hi, Marek,
> 
>>
>> sorry for the late reply.
> 
> That's okay.
> 
>>
>>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>>> and 256kiB uniform sectors device configuration, however according to
>>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>>> cannot find a valid sector map entry and fails.
>>>>
>>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>>> fixup hook and a wrapper function. The only implementation of the hook is
>>>> currently specific to S25FS512S.
>>>>
>>>> This fixes the following failure on S25FS512S:
>>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>> ---
>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Cc: Michael Walle <mwalle@kernel.org>
>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>>> Cc: Richard Weinberger <richard@nod.at>
>>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>>> Cc: linux-mtd@lists.infradead.org
>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>> ---
>>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 9d6e85bf227b..ca65f36e5638 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>       return 0;
>>>>   }
>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>>> +        nor->manufacturer->fixups->post_smpt) {
>>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>>                      u32 shared_hwcaps)
>>>>   {
>>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>>> index 1516b6d0dc37..d5294424ab9d 100644
>>>> --- a/drivers/mtd/spi-nor/core.h
>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>>    *             parameters that could not be extracted by other means (i.e.
>>>>    *             when information provided by the SFDP/flash_info tables are
>>>>    *             incomplete or wrong).
>>>> + * @post_smpt: update sector map configuration ID selector according to
>>>> + *             chip-specific quirks.
>>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>>    *             Will replace the default_init() hook.
>>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>>                const struct sfdp_parameter_header *bfpt_header,
>>>>                const struct sfdp_bfpt *bfpt);
>>>>       int (*post_sfdp)(struct spi_nor *nor);
>>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>>       int (*late_init)(struct spi_nor *nor);
>>>>   };
>>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>>                    const struct sfdp_bfpt *bfpt);
>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>>> +
>>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>> index 5b1117265bd2..542c775918ad 100644
>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>>       }
>>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>>> +    if (err)
>>>> +        return ERR_PTR(err);
>>>> +
>>>>       /*
>>>>        * If command descriptors are provided, they always precede map
>>>>        * descriptors in the table. There is no need to start the iteration
>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>> index d6c92595f6bc..d446d12371e1 100644
>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>   };
>>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>> +{
>>>> +    /*
>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>> +     * device configuration, however according to section 7.5.5.1
>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>> +     * configuration index bit 0, which is populated exactly by
>>>> +     * the CR3NV bit 1 being 1.
>>>> +     */
>>>> +    *smpt |= BIT(0);
>>>
>>> Please help me understand this. Maybe a link to your revision of
>>> datasheet would help me.
>>
>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>
>> SMPT values:
>>
>> i=0 smpt[i]=08ff65fc
>> i=1 smpt[i]=00000004
>> i=2 smpt[i]=04ff65fc
>> i=3 smpt[i]=00000002
>> i=4 smpt[i]=02ff65fd
>> i=5 smpt[i]=00000004
>> i=6 smpt[i]=ff0201fe
>> i=7 smpt[i]=00007ff1
>> i=8 smpt[i]=00037ff4
>> i=9 smpt[i]=03fbfff4
>> i=10 smpt[i]=ff0203fe
>> i=11 smpt[i]=03fbfff4
>> i=12 smpt[i]=00037ff4
>> i=13 smpt[i]=00007ff1
>> i=14 smpt[i]=ff0005ff
>> i=15 smpt[i]=03fffff4
>>
> 
> thanks!
> 
>>> In the flash datasheets that I see, there shall
>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>> table is described showing an Index Value constructed by the CRxNV[y]
>>> return values. That index value is the map_id in the code.
>>>
>>> By reading your description I understand CR3NV[1] has value zero as it
>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>> always have value 1. That's why datasheets like this one [1] in their
>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>
>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
> 
> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
> "Index Value" column. I deduced the formula by using the CR3NV[3] and
> CR1NV[2] values. The datasheet says:
> 
> "When more than one configuration bit must be read, all the bits are
> concatenated into an index value that is used to select the current address map."
> 
> and that the "the following MSb to LSb order to form the configuration map
> index value:
> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
> "
> 
> The "Index Value" shall be the map_id that you passed in the code:
> spi_nor_post_smpt_fixups(nor, &map_id);
> 
> Can you please print the map_id value that you obtain without updating it?
> Let's also print the values of CR3NV and CR1NV.
> 
> (idea for enthusiasts: we can add a debugfs interface to query registers)
> 
>>
>>> I assume what you're doing is fine as it shouldn't break backward
>>> compatibility with other older flashes as their CR3NV[1] has value one
>>> anyway. Correct me if I'm wrong.
> 
> Now that I revisited the datasheet, your comment "use CR3NV bit 1 to discern
> 64kiB/256kiB uniform sectors" is misleading because a) I understand that
> CR3NV[1] is not involved in the sector map selection (the "Index Value" is),
> and b) the choice as per table 70 is between uniform 256 KB sectors and
> non-uniform map with 4KB sectors at top or bottom and the remaining 256KB.
> 
> Let's get those CR3NV and CR1NV values so we decide how to tackle this.
> 
> Cheers,
> ta
> 
>>
>> I hope so.
>>
>>> Now looking at the code, what we usually do is to save the flash
>>> parameters described by SFDP in nor->params, then amend those parameters
>>> with fixup hooks if that's needed. Here you modify the map_id and then
>>> let the code use it in order to determine the sector_map. Then that
>>> sector_map (which is SMPT data from the table itself) is used to
>>> initialize erase regions. That sector_map can contain wrong data too.
>>
>> By sector_map, do you refer to the "smpt" array ?
>>
>>> I'd suggest saving a nor->params->sector_map then call a
>>> int spi_nor_post_smpt_fixups(struct spi_nor *nor,
>>>     const struct sfdp_parameter_header *smpt_header,
>>>     const u32 *smpt)
>>> in case spi_nor_get_map_in_use() fails. This way others can update
>>> sector_map as well if that's ever needed. What do you think?
>>
>> This won't work for me, would it ? In my case, I need to patch content of CR3NV register to assure CR3NV bit 1 is well defined. I don't need to patch the sector_map itself.
> 


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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Takahiro Kuwano]

Hi,

On 10/8/2025 3:20 PM, Tudor Ambarus wrote:
> 
> 
> On 9/3/25 2:40 AM, Marek Vasut wrote:
>> On 9/23/24 10:54 AM, Tudor Ambarus wrote:
>>
>> Hello Tudor,
> 
> Hi, Marek,
> 
>>
>> sorry for the late reply.
> 
> That's okay.
> 
>>
>>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>>> and 256kiB uniform sectors device configuration, however according to
>>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>>> cannot find a valid sector map entry and fails.
>>>>
>>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>>> fixup hook and a wrapper function. The only implementation of the hook is
>>>> currently specific to S25FS512S.
>>>>
>>>> This fixes the following failure on S25FS512S:
>>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>> ---
>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Cc: Michael Walle <mwalle@kernel.org>
>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>>> Cc: Richard Weinberger <richard@nod.at>
>>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>>> Cc: linux-mtd@lists.infradead.org
>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>> ---
>>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 9d6e85bf227b..ca65f36e5638 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>       return 0;
>>>>   }
>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>>> +        nor->manufacturer->fixups->post_smpt) {
>>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>>                      u32 shared_hwcaps)
>>>>   {
>>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>>> index 1516b6d0dc37..d5294424ab9d 100644
>>>> --- a/drivers/mtd/spi-nor/core.h
>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>>    *             parameters that could not be extracted by other means (i.e.
>>>>    *             when information provided by the SFDP/flash_info tables are
>>>>    *             incomplete or wrong).
>>>> + * @post_smpt: update sector map configuration ID selector according to
>>>> + *             chip-specific quirks.
>>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>>    *             Will replace the default_init() hook.
>>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>>                const struct sfdp_parameter_header *bfpt_header,
>>>>                const struct sfdp_bfpt *bfpt);
>>>>       int (*post_sfdp)(struct spi_nor *nor);
>>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>>       int (*late_init)(struct spi_nor *nor);
>>>>   };
>>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>>                    const struct sfdp_bfpt *bfpt);
>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>>> +
>>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>> index 5b1117265bd2..542c775918ad 100644
>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>>       }
>>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>>> +    if (err)
>>>> +        return ERR_PTR(err);
>>>> +
>>>>       /*
>>>>        * If command descriptors are provided, they always precede map
>>>>        * descriptors in the table. There is no need to start the iteration
>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>> index d6c92595f6bc..d446d12371e1 100644
>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>   };
>>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>> +{
>>>> +    /*
>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>> +     * device configuration, however according to section 7.5.5.1
>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>> +     * configuration index bit 0, which is populated exactly by
>>>> +     * the CR3NV bit 1 being 1.
>>>> +     */
>>>> +    *smpt |= BIT(0);
>>>
>>> Please help me understand this. Maybe a link to your revision of
>>> datasheet would help me.
>>
>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>
>> SMPT values:
>>
>> i=0 smpt[i]=08ff65fc
>> i=1 smpt[i]=00000004
>> i=2 smpt[i]=04ff65fc
>> i=3 smpt[i]=00000002
>> i=4 smpt[i]=02ff65fd
>> i=5 smpt[i]=00000004
>> i=6 smpt[i]=ff0201fe
>> i=7 smpt[i]=00007ff1
>> i=8 smpt[i]=00037ff4
>> i=9 smpt[i]=03fbfff4
>> i=10 smpt[i]=ff0203fe
>> i=11 smpt[i]=03fbfff4
>> i=12 smpt[i]=00037ff4
>> i=13 smpt[i]=00007ff1
>> i=14 smpt[i]=ff0005ff
>> i=15 smpt[i]=03fffff4
>>
> 
> thanks!
> 
>>> In the flash datasheets that I see, there shall
>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>> table is described showing an Index Value constructed by the CRxNV[y]
>>> return values. That index value is the map_id in the code.
>>>
>>> By reading your description I understand CR3NV[1] has value zero as it
>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>> always have value 1. That's why datasheets like this one [1] in their
>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>
>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
> 
> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
> "Index Value" column. I deduced the formula by using the CR3NV[3] and
> CR1NV[2] values. The datasheet says:
> 
> "When more than one configuration bit must be read, all the bits are
> concatenated into an index value that is used to select the current address map."
> 
> and that the "the following MSb to LSb order to form the configuration map
> index value:
> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
> "
> 
In old datasheet, Table 70 had CR3NV[1] column and the description about index
value had another bullet:
  • CR3NV[1] — 1 = 256 kB uniform sector size
These were removed when the default CRNV3[1] value was changed from 1 to 0.
As Marek pointed out, SMPT still involves CR3NV[1] expecting the value is
always 1.

> The "Index Value" shall be the map_id that you passed in the code:
> spi_nor_post_smpt_fixups(nor, &map_id);
> 
> Can you please print the map_id value that you obtain without updating it?
> Let's also print the values of CR3NV and CR1NV.
> 
I did this in my environment and found another, fundamental issue in SMPT
contents and the way to parse it...

The SMPT describes Configuration detection command latency as '1111b: variable
latency' and in spi_nor_smpt_read_dummy(),

	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
		return nor->read_dummy;

This results in 0 latency.
However, in S25FS-S, register read latency is 8 cycles by default. That means
the SPI controller reads garbage data as CR3NV and CR1NV value. In my particular
environment they are 0xFF as IO1 is pulled up, so map_id is 111b.

Let me think about how we can solve this.

Thanks,
Takahiro

> (idea for enthusiasts: we can add a debugfs interface to query registers)
> 
>>
>>> I assume what you're doing is fine as it shouldn't break backward
>>> compatibility with other older flashes as their CR3NV[1] has value one
>>> anyway. Correct me if I'm wrong.
> 
> Now that I revisited the datasheet, your comment "use CR3NV bit 1 to discern
> 64kiB/256kiB uniform sectors" is misleading because a) I understand that
> CR3NV[1] is not involved in the sector map selection (the "Index Value" is),
> and b) the choice as per table 70 is between uniform 256 KB sectors and
> non-uniform map with 4KB sectors at top or bottom and the remaining 256KB.
> 
> Let's get those CR3NV and CR1NV values so we decide how to tackle this.
> 
> Cheers,
> ta
> 
>>
>> I hope so.
>>
>>> Now looking at the code, what we usually do is to save the flash
>>> parameters described by SFDP in nor->params, then amend those parameters
>>> with fixup hooks if that's needed. Here you modify the map_id and then
>>> let the code use it in order to determine the sector_map. Then that
>>> sector_map (which is SMPT data from the table itself) is used to
>>> initialize erase regions. That sector_map can contain wrong data too.
>>
>> By sector_map, do you refer to the "smpt" array ?
>>
>>> I'd suggest saving a nor->params->sector_map then call a
>>> int spi_nor_post_smpt_fixups(struct spi_nor *nor,
>>>     const struct sfdp_parameter_header *smpt_header,
>>>     const u32 *smpt)
>>> in case spi_nor_get_map_in_use() fails. This way others can update
>>> sector_map as well if that's ever needed. What do you think?
>>
>> This won't work for me, would it ? In my case, I need to patch content of CR3NV register to assure CR3NV bit 1 is well defined. I don't need to patch the sector_map itself.

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Tudor Ambarus]

On 10/10/25 7:01 AM, Takahiro Kuwano wrote:
> Hi,

Hi!

Thanks for chiming in, Takahiro!

> 
> On 10/8/2025 3:20 PM, Tudor Ambarus wrote:
>>
>>
>> On 9/3/25 2:40 AM, Marek Vasut wrote:
>>> On 9/23/24 10:54 AM, Tudor Ambarus wrote:
>>>
>>> Hello Tudor,
>>
>> Hi, Marek,
>>
>>>
>>> sorry for the late reply.
>>
>> That's okay.
>>
>>>
>>>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>>>> and 256kiB uniform sectors device configuration, however according to
>>>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>>>> cannot find a valid sector map entry and fails.
>>>>>
>>>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>>>> fixup hook and a wrapper function. The only implementation of the hook is
>>>>> currently specific to S25FS512S.
>>>>>
>>>>> This fixes the following failure on S25FS512S:
>>>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>>>
>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>>> ---
>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>> Cc: Michael Walle <mwalle@kernel.org>
>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>>>> Cc: Richard Weinberger <richard@nod.at>
>>>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>>>> Cc: linux-mtd@lists.infradead.org
>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>> ---
>>>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>> index 9d6e85bf227b..ca65f36e5638 100644
>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>       return 0;
>>>>>   }
>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>> +{
>>>>> +    int ret;
>>>>> +
>>>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>> +        nor->manufacturer->fixups->post_smpt) {
>>>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>>>> +        if (ret)
>>>>> +            return ret;
>>>>> +    }
>>>>> +
>>>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>>>                      u32 shared_hwcaps)
>>>>>   {
>>>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>>>> index 1516b6d0dc37..d5294424ab9d 100644
>>>>> --- a/drivers/mtd/spi-nor/core.h
>>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>>>    *             parameters that could not be extracted by other means (i.e.
>>>>>    *             when information provided by the SFDP/flash_info tables are
>>>>>    *             incomplete or wrong).
>>>>> + * @post_smpt: update sector map configuration ID selector according to
>>>>> + *             chip-specific quirks.
>>>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>>>    *             Will replace the default_init() hook.
>>>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>>>                const struct sfdp_parameter_header *bfpt_header,
>>>>>                const struct sfdp_bfpt *bfpt);
>>>>>       int (*post_sfdp)(struct spi_nor *nor);
>>>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>>>       int (*late_init)(struct spi_nor *nor);
>>>>>   };
>>>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>>>                    const struct sfdp_bfpt *bfpt);
>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>>>> +
>>>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>>> index 5b1117265bd2..542c775918ad 100644
>>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>>>       }
>>>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>>>> +    if (err)
>>>>> +        return ERR_PTR(err);
>>>>> +
>>>>>       /*
>>>>>        * If command descriptors are provided, they always precede map
>>>>>        * descriptors in the table. There is no need to start the iteration
>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>> index d6c92595f6bc..d446d12371e1 100644
>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>>   };
>>>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>> +{
>>>>> +    /*
>>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>>> +     * device configuration, however according to section 7.5.5.1
>>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>>> +     * configuration index bit 0, which is populated exactly by
>>>>> +     * the CR3NV bit 1 being 1.
>>>>> +     */
>>>>> +    *smpt |= BIT(0);
>>>>
>>>> Please help me understand this. Maybe a link to your revision of
>>>> datasheet would help me.
>>>
>>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>>
>>> SMPT values:
>>>
>>> i=0 smpt[i]=08ff65fc
>>> i=1 smpt[i]=00000004
>>> i=2 smpt[i]=04ff65fc
>>> i=3 smpt[i]=00000002
>>> i=4 smpt[i]=02ff65fd
>>> i=5 smpt[i]=00000004
>>> i=6 smpt[i]=ff0201fe
>>> i=7 smpt[i]=00007ff1
>>> i=8 smpt[i]=00037ff4
>>> i=9 smpt[i]=03fbfff4
>>> i=10 smpt[i]=ff0203fe
>>> i=11 smpt[i]=03fbfff4
>>> i=12 smpt[i]=00037ff4
>>> i=13 smpt[i]=00007ff1
>>> i=14 smpt[i]=ff0005ff
>>> i=15 smpt[i]=03fffff4
>>>
>>
>> thanks!
>>
>>>> In the flash datasheets that I see, there shall
>>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>>> table is described showing an Index Value constructed by the CRxNV[y]
>>>> return values. That index value is the map_id in the code.
>>>>
>>>> By reading your description I understand CR3NV[1] has value zero as it
>>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>>> always have value 1. That's why datasheets like this one [1] in their
>>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>>
>>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
>>
>> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
>> "Index Value" column. I deduced the formula by using the CR3NV[3] and
>> CR1NV[2] values. The datasheet says:
>>
>> "When more than one configuration bit must be read, all the bits are
>> concatenated into an index value that is used to select the current address map."
>>
>> and that the "the following MSb to LSb order to form the configuration map
>> index value:
>> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
>> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
>> "
>>
> In old datasheet, Table 70 had CR3NV[1] column and the description about index
> value had another bullet:
>   • CR3NV[1] — 1 = 256 kB uniform sector size
> These were removed when the default CRNV3[1] value was changed from 1 to 0.
> As Marek pointed out, SMPT still involves CR3NV[1] expecting the value is
> always 1.
> 
>> The "Index Value" shall be the map_id that you passed in the code:
>> spi_nor_post_smpt_fixups(nor, &map_id);
>>
>> Can you please print the map_id value that you obtain without updating it?
>> Let's also print the values of CR3NV and CR1NV.
>>
> I did this in my environment and found another, fundamental issue in SMPT
> contents and the way to parse it...
> 
> The SMPT describes Configuration detection command latency as '1111b: variable
> latency' and in spi_nor_smpt_read_dummy(),
> 
> 	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
> 		return nor->read_dummy;
> 
> This results in 0 latency.
> However, in S25FS-S, register read latency is 8 cycles by default. That means

If you hack the code and set 8 cycles for the read latency, do you get correct
results?

Can the variable latency be determined by parsing SFDP?

Cheers,
ta

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Takahiro Kuwano]

Hi Tudor,

On 10/10/2025 3:12 PM, Tudor Ambarus wrote:
> 
> 
> On 10/10/25 7:01 AM, Takahiro Kuwano wrote:
>> Hi,
> 
> Hi!
> 
> Thanks for chiming in, Takahiro!
> 
>>
>> On 10/8/2025 3:20 PM, Tudor Ambarus wrote:
>>>
>>>
>>> On 9/3/25 2:40 AM, Marek Vasut wrote:
>>>> On 9/23/24 10:54 AM, Tudor Ambarus wrote:
>>>>
>>>> Hello Tudor,
>>>
>>> Hi, Marek,
>>>
>>>>
>>>> sorry for the late reply.
>>>
>>> That's okay.
>>>
>>>>
>>>>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>>>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>>>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>>>>> and 256kiB uniform sectors device configuration, however according to
>>>>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>>>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>>>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>>>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>>>>> cannot find a valid sector map entry and fails.
>>>>>>
>>>>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>>>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>>>>> fixup hook and a wrapper function. The only implementation of the hook is
>>>>>> currently specific to S25FS512S.
>>>>>>
>>>>>> This fixes the following failure on S25FS512S:
>>>>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>>>> ---
>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>> Cc: Michael Walle <mwalle@kernel.org>
>>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>>>>> Cc: Richard Weinberger <richard@nod.at>
>>>>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>>>>> Cc: linux-mtd@lists.infradead.org
>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>> ---
>>>>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>>> index 9d6e85bf227b..ca65f36e5638 100644
>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>       return 0;
>>>>>>   }
>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>>> +        nor->manufacturer->fixups->post_smpt) {
>>>>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>>>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>>>>                      u32 shared_hwcaps)
>>>>>>   {
>>>>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>>>>> index 1516b6d0dc37..d5294424ab9d 100644
>>>>>> --- a/drivers/mtd/spi-nor/core.h
>>>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>>>>    *             parameters that could not be extracted by other means (i.e.
>>>>>>    *             when information provided by the SFDP/flash_info tables are
>>>>>>    *             incomplete or wrong).
>>>>>> + * @post_smpt: update sector map configuration ID selector according to
>>>>>> + *             chip-specific quirks.
>>>>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>>>>    *             Will replace the default_init() hook.
>>>>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>>>>                const struct sfdp_parameter_header *bfpt_header,
>>>>>>                const struct sfdp_bfpt *bfpt);
>>>>>>       int (*post_sfdp)(struct spi_nor *nor);
>>>>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>>>>       int (*late_init)(struct spi_nor *nor);
>>>>>>   };
>>>>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>>>>                    const struct sfdp_bfpt *bfpt);
>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>>>>> +
>>>>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>>>> index 5b1117265bd2..542c775918ad 100644
>>>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>>>>       }
>>>>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>> +    if (err)
>>>>>> +        return ERR_PTR(err);
>>>>>> +
>>>>>>       /*
>>>>>>        * If command descriptors are provided, they always precede map
>>>>>>        * descriptors in the table. There is no need to start the iteration
>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>> index d6c92595f6bc..d446d12371e1 100644
>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>>>   };
>>>>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>> +{
>>>>>> +    /*
>>>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>>>> +     * device configuration, however according to section 7.5.5.1
>>>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>>>> +     * configuration index bit 0, which is populated exactly by
>>>>>> +     * the CR3NV bit 1 being 1.
>>>>>> +     */
>>>>>> +    *smpt |= BIT(0);
>>>>>
>>>>> Please help me understand this. Maybe a link to your revision of
>>>>> datasheet would help me.
>>>>
>>>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>>>
>>>> SMPT values:
>>>>
>>>> i=0 smpt[i]=08ff65fc
>>>> i=1 smpt[i]=00000004
>>>> i=2 smpt[i]=04ff65fc
>>>> i=3 smpt[i]=00000002
>>>> i=4 smpt[i]=02ff65fd
>>>> i=5 smpt[i]=00000004
>>>> i=6 smpt[i]=ff0201fe
>>>> i=7 smpt[i]=00007ff1
>>>> i=8 smpt[i]=00037ff4
>>>> i=9 smpt[i]=03fbfff4
>>>> i=10 smpt[i]=ff0203fe
>>>> i=11 smpt[i]=03fbfff4
>>>> i=12 smpt[i]=00037ff4
>>>> i=13 smpt[i]=00007ff1
>>>> i=14 smpt[i]=ff0005ff
>>>> i=15 smpt[i]=03fffff4
>>>>
>>>
>>> thanks!
>>>
>>>>> In the flash datasheets that I see, there shall
>>>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>>>> table is described showing an Index Value constructed by the CRxNV[y]
>>>>> return values. That index value is the map_id in the code.
>>>>>
>>>>> By reading your description I understand CR3NV[1] has value zero as it
>>>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>>>> always have value 1. That's why datasheets like this one [1] in their
>>>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>>>
>>>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
>>>
>>> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
>>> "Index Value" column. I deduced the formula by using the CR3NV[3] and
>>> CR1NV[2] values. The datasheet says:
>>>
>>> "When more than one configuration bit must be read, all the bits are
>>> concatenated into an index value that is used to select the current address map."
>>>
>>> and that the "the following MSb to LSb order to form the configuration map
>>> index value:
>>> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
>>> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
>>> "
>>>
>> In old datasheet, Table 70 had CR3NV[1] column and the description about index
>> value had another bullet:
>>   • CR3NV[1] — 1 = 256 kB uniform sector size
>> These were removed when the default CRNV3[1] value was changed from 1 to 0.
>> As Marek pointed out, SMPT still involves CR3NV[1] expecting the value is
>> always 1.
>>
>>> The "Index Value" shall be the map_id that you passed in the code:
>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>
>>> Can you please print the map_id value that you obtain without updating it?
>>> Let's also print the values of CR3NV and CR1NV.
>>>
>> I did this in my environment and found another, fundamental issue in SMPT
>> contents and the way to parse it...
>>
>> The SMPT describes Configuration detection command latency as '1111b: variable
>> latency' and in spi_nor_smpt_read_dummy(),
>>
>> 	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
>> 		return nor->read_dummy;
>>
>> This results in 0 latency.
>> However, in S25FS-S, register read latency is 8 cycles by default. That means
> 
> If you hack the code and set 8 cycles for the read latency, do you get correct
> results?
> 
No, we still need fix like Marek's or something.

> Can the variable latency be determined by parsing SFDP?
> 
No, I don't think we can determine by SFDP.

> Cheers,
> ta


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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Tudor Ambarus]

Hi, Takahiro,

>>>>>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>>>>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>>>>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>>>>>> and 256kiB uniform sectors device configuration, however according to
>>>>>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>>>>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>>>>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>>>>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>>>>>> cannot find a valid sector map entry and fails.
>>>>>>>
>>>>>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>>>>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>>>>>> fixup hook and a wrapper function. The only implementation of the hook is
>>>>>>> currently specific to S25FS512S.
>>>>>>>
>>>>>>> This fixes the following failure on S25FS512S:
>>>>>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>>>>> ---
>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>> Cc: Michael Walle <mwalle@kernel.org>
>>>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>>>>>> Cc: Richard Weinberger <richard@nod.at>
>>>>>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>>>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>>>>>> Cc: linux-mtd@lists.infradead.org
>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>> ---
>>>>>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>>>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>>>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>>>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>>>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>>>> index 9d6e85bf227b..ca65f36e5638 100644
>>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>>> +{
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>>>> +        nor->manufacturer->fixups->post_smpt) {
>>>>>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>>>>>> +        if (ret)
>>>>>>> +            return ret;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>>>>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>>>>>                      u32 shared_hwcaps)
>>>>>>>   {
>>>>>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>>>>>> index 1516b6d0dc37..d5294424ab9d 100644
>>>>>>> --- a/drivers/mtd/spi-nor/core.h
>>>>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>>>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>>>>>    *             parameters that could not be extracted by other means (i.e.
>>>>>>>    *             when information provided by the SFDP/flash_info tables are
>>>>>>>    *             incomplete or wrong).
>>>>>>> + * @post_smpt: update sector map configuration ID selector according to
>>>>>>> + *             chip-specific quirks.
>>>>>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>>>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>>>>>    *             Will replace the default_init() hook.
>>>>>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>>>>>                const struct sfdp_parameter_header *bfpt_header,
>>>>>>>                const struct sfdp_bfpt *bfpt);
>>>>>>>       int (*post_sfdp)(struct spi_nor *nor);
>>>>>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>>>>>       int (*late_init)(struct spi_nor *nor);
>>>>>>>   };
>>>>>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>>>>>                    const struct sfdp_bfpt *bfpt);
>>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>>>>>> +
>>>>>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>>>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>>>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>>>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>>>>> index 5b1117265bd2..542c775918ad 100644
>>>>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>>>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>>>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>>>>>       }
>>>>>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>>> +    if (err)
>>>>>>> +        return ERR_PTR(err);
>>>>>>> +
>>>>>>>       /*
>>>>>>>        * If command descriptors are provided, they always precede map
>>>>>>>        * descriptors in the table. There is no need to start the iteration
>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>> index d6c92595f6bc..d446d12371e1 100644
>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>>>>   };
>>>>>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>>> +{
>>>>>>> +    /*
>>>>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>>>>> +     * device configuration, however according to section 7.5.5.1
>>>>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>>>>> +     * configuration index bit 0, which is populated exactly by
>>>>>>> +     * the CR3NV bit 1 being 1.
>>>>>>> +     */
>>>>>>> +    *smpt |= BIT(0);
>>>>>>
>>>>>> Please help me understand this. Maybe a link to your revision of
>>>>>> datasheet would help me.
>>>>>
>>>>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>>>>
>>>>> SMPT values:
>>>>>
>>>>> i=0 smpt[i]=08ff65fc
>>>>> i=1 smpt[i]=00000004
>>>>> i=2 smpt[i]=04ff65fc
>>>>> i=3 smpt[i]=00000002
>>>>> i=4 smpt[i]=02ff65fd
>>>>> i=5 smpt[i]=00000004
>>>>> i=6 smpt[i]=ff0201fe
>>>>> i=7 smpt[i]=00007ff1
>>>>> i=8 smpt[i]=00037ff4
>>>>> i=9 smpt[i]=03fbfff4
>>>>> i=10 smpt[i]=ff0203fe
>>>>> i=11 smpt[i]=03fbfff4
>>>>> i=12 smpt[i]=00037ff4
>>>>> i=13 smpt[i]=00007ff1
>>>>> i=14 smpt[i]=ff0005ff
>>>>> i=15 smpt[i]=03fffff4
>>>>>
>>>>
>>>> thanks!
>>>>
>>>>>> In the flash datasheets that I see, there shall
>>>>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>>>>> table is described showing an Index Value constructed by the CRxNV[y]
>>>>>> return values. That index value is the map_id in the code.
>>>>>>
>>>>>> By reading your description I understand CR3NV[1] has value zero as it
>>>>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>>>>> always have value 1. That's why datasheets like this one [1] in their
>>>>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>>>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>>>>
>>>>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
>>>>
>>>> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
>>>> "Index Value" column. I deduced the formula by using the CR3NV[3] and
>>>> CR1NV[2] values. The datasheet says:
>>>>
>>>> "When more than one configuration bit must be read, all the bits are
>>>> concatenated into an index value that is used to select the current address map."
>>>>
>>>> and that the "the following MSb to LSb order to form the configuration map
>>>> index value:
>>>> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
>>>> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
>>>> "
>>>>
>>> In old datasheet, Table 70 had CR3NV[1] column and the description about index
>>> value had another bullet:
>>>   • CR3NV[1] — 1 = 256 kB uniform sector size
>>> These were removed when the default CRNV3[1] value was changed from 1 to 0.
>>> As Marek pointed out, SMPT still involves CR3NV[1] expecting the value is
>>> always 1.

If CR3NV[1] is always 1 even in the old datasheets, then replacing
CR3NV[3] << 2 | CR1NV[2] << 1 | CR3NV[1]
with
CR3NV[3] << 2 | CR1NV[2] << 1 | 1 (CR3NV[1] was always 1 anyway)
for the index value/map_id is backward compatible and okay, even for the
older flashes.

Then Marek's idea of updating the map_id via a hook is sound. If we're going
to update just the map id, I'd rename spi_nor_post_smpt_fixups() to
spi_nor_smpt_map_id_fixup().

>>>
>>>> The "Index Value" shall be the map_id that you passed in the code:
>>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>>
>>>> Can you please print the map_id value that you obtain without updating it?
>>>> Let's also print the values of CR3NV and CR1NV.
>>>>
>>> I did this in my environment and found another, fundamental issue in SMPT
>>> contents and the way to parse it...
>>>
>>> The SMPT describes Configuration detection command latency as '1111b: variable
>>> latency' and in spi_nor_smpt_read_dummy(),
>>>
>>> 	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
>>> 		return nor->read_dummy;
>>>
>>> This results in 0 latency.
>>> However, in S25FS-S, register read latency is 8 cycles by default. That means

Is there any other reg interaction in the probe path? What number of dummy cycles
is it using?

>>
>> If you hack the code and set 8 cycles for the read latency, do you get correct
>> results?
>>
> No, we still need fix like Marek's or something.

Right

> 
>> Can the variable latency be determined by parsing SFDP?
>>
> No, I don't think we can determine by SFDP.
> 

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Marek Vasut]

On 10/8/25 8:20 AM, Tudor Ambarus wrote:

Hello Tudor,

>> sorry for the late reply.
> 
> That's okay.

Thank you

[...]

>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>> index d6c92595f6bc..d446d12371e1 100644
>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>        .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>    };
>>>>    +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>> +{
>>>> +    /*
>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>> +     * device configuration, however according to section 7.5.5.1
>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>> +     * configuration index bit 0, which is populated exactly by
>>>> +     * the CR3NV bit 1 being 1.
>>>> +     */
>>>> +    *smpt |= BIT(0);
>>>
>>> Please help me understand this. Maybe a link to your revision of
>>> datasheet would help me.
>>
>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>
>> SMPT values:
>>
>> i=0 smpt[i]=08ff65fc
>> i=1 smpt[i]=00000004
>> i=2 smpt[i]=04ff65fc
>> i=3 smpt[i]=00000002
>> i=4 smpt[i]=02ff65fd
>> i=5 smpt[i]=00000004
>> i=6 smpt[i]=ff0201fe
>> i=7 smpt[i]=00007ff1
>> i=8 smpt[i]=00037ff4
>> i=9 smpt[i]=03fbfff4
>> i=10 smpt[i]=ff0203fe
>> i=11 smpt[i]=03fbfff4
>> i=12 smpt[i]=00037ff4
>> i=13 smpt[i]=00007ff1
>> i=14 smpt[i]=ff0005ff
>> i=15 smpt[i]=03fffff4
>>
> 
> thanks!
> 
>>> In the flash datasheets that I see, there shall
>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>> table is described showing an Index Value constructed by the CRxNV[y]
>>> return values. That index value is the map_id in the code.
>>>
>>> By reading your description I understand CR3NV[1] has value zero as it
>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>> always have value 1. That's why datasheets like this one [1] in their
>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>
>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
> 
> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
> "Index Value" column. I deduced the formula by using the CR3NV[3] and
> CR1NV[2] values. The datasheet says:
> 
> "When more than one configuration bit must be read, all the bits are
> concatenated into an index value that is used to select the current address map."
> 
> and that the "the following MSb to LSb order to form the configuration map
> index value:
> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
> "
> 
> The "Index Value" shall be the map_id that you passed in the code:
> spi_nor_post_smpt_fixups(nor, &map_id);
> 
> Can you please print the map_id value that you obtain without updating it?

0x4

> Let's also print the values of CR3NV and CR1NV.

Both 0x0 and 0x0 .

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Tudor Ambarus]

On 10/12/25 5:19 PM, Marek Vasut wrote:
> Hello Tudor,

Hello!

>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>> index d6c92595f6bc..d446d12371e1 100644
>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>>        .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>>    };
>>>>>    +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>> +{
>>>>> +    /*
>>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>>> +     * device configuration, however according to section 7.5.5.1
>>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>>> +     * configuration index bit 0, which is populated exactly by
>>>>> +     * the CR3NV bit 1 being 1.
>>>>> +     */
>>>>> +    *smpt |= BIT(0);
>>>>
>>>> Please help me understand this. Maybe a link to your revision of
>>>> datasheet would help me.
>>>
>>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>>
>>> SMPT values:
>>>
>>> i=0 smpt[i]=08ff65fc
>>> i=1 smpt[i]=00000004
>>> i=2 smpt[i]=04ff65fc
>>> i=3 smpt[i]=00000002
>>> i=4 smpt[i]=02ff65fd
>>> i=5 smpt[i]=00000004
>>> i=6 smpt[i]=ff0201fe
>>> i=7 smpt[i]=00007ff1
>>> i=8 smpt[i]=00037ff4
>>> i=9 smpt[i]=03fbfff4
>>> i=10 smpt[i]=ff0203fe
>>> i=11 smpt[i]=03fbfff4
>>> i=12 smpt[i]=00037ff4
>>> i=13 smpt[i]=00007ff1
>>> i=14 smpt[i]=ff0005ff
>>> i=15 smpt[i]=03fffff4
>>>
>>
>> thanks!
>>
>>>> In the flash datasheets that I see, there shall
>>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>>> table is described showing an Index Value constructed by the CRxNV[y]
>>>> return values. That index value is the map_id in the code.
>>>>
>>>> By reading your description I understand CR3NV[1] has value zero as it
>>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>>> always have value 1. That's why datasheets like this one [1] in their
>>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>>
>>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
>>
>> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
>> "Index Value" column. I deduced the formula by using the CR3NV[3] and
>> CR1NV[2] values. The datasheet says:
>>
>> "When more than one configuration bit must be read, all the bits are
>> concatenated into an index value that is used to select the current address map."
>>
>> and that the "the following MSb to LSb order to form the configuration map
>> index value:
>> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
>> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
>> "
>>
>> The "Index Value" shall be the map_id that you passed in the code:
>> spi_nor_post_smpt_fixups(nor, &map_id);
>>
>> Can you please print the map_id value that you obtain without updating it?
> 
> 0x4

This translates to CR3NV[3] = 1, CR1NV[2] = 0,  CR3NV[1] = 0.
> 
>> Let's also print the values of CR3NV and CR1NV.
> 
> Both 0x0 and 0x0 .

But here CR3NV is 0, it contradicts the result from above.

Maybe it's the same problem that Takahiro identified: the flash needs
8 dummy cycles, but the code uses zero dummy cycles, resulting in
reading garbage data, depending on whether your IO lines are pulled up/down
or floating.

Can you redo the test with the following please?

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 21727f9a4ac6..85443c903e59 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -752,7 +752,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
 
                read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
                nor->addr_nbytes = spi_nor_smpt_addr_nbytes(nor, smpt[i]);
-               nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
+               nor->read_dummy = 8;
                nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
                addr = smpt[i + 1];
 
@@ -767,6 +767,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
                 * Configuration that is currently in use.
                 */
                map_id = map_id << 1 | !!(*buf & read_data_mask);
+               dev_err(nor->dev, "i = %d, buf = %02x, map_id = %02x\n",
+                       i, buf[0], map_id);
        }
 
Cheers,
ta

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Takahiro Kuwano]

Hi Tudor,

On 10/10/2025 5:34 PM, Tudor Ambarus wrote:
> Hi, Takahiro,
> 
>>>>>>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>>>>>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>>>>>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>>>>>>> and 256kiB uniform sectors device configuration, however according to
>>>>>>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>>>>>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>>>>>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>>>>>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>>>>>>> cannot find a valid sector map entry and fails.
>>>>>>>>
>>>>>>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>>>>>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>>>>>>> fixup hook and a wrapper function. The only implementation of the hook is
>>>>>>>> currently specific to S25FS512S.
>>>>>>>>
>>>>>>>> This fixes the following failure on S25FS512S:
>>>>>>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>>>>>> ---
>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>>> Cc: Michael Walle <mwalle@kernel.org>
>>>>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>>>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>>>>>>> Cc: Richard Weinberger <richard@nod.at>
>>>>>>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>>>>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>>>>>>> Cc: linux-mtd@lists.infradead.org
>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>>> ---
>>>>>>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>>>>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>>>>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>>>>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>>>>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>>>>> index 9d6e85bf227b..ca65f36e5638 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>>       return 0;
>>>>>>>>   }
>>>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>>>> +{
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>>>>> +        nor->manufacturer->fixups->post_smpt) {
>>>>>>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>>>>>>> +        if (ret)
>>>>>>>> +            return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>>>>>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>>>>>>                      u32 shared_hwcaps)
>>>>>>>>   {
>>>>>>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>>>>>>> index 1516b6d0dc37..d5294424ab9d 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/core.h
>>>>>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>>>>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>>>>>>    *             parameters that could not be extracted by other means (i.e.
>>>>>>>>    *             when information provided by the SFDP/flash_info tables are
>>>>>>>>    *             incomplete or wrong).
>>>>>>>> + * @post_smpt: update sector map configuration ID selector according to
>>>>>>>> + *             chip-specific quirks.
>>>>>>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>>>>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>>>>>>    *             Will replace the default_init() hook.
>>>>>>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>>>>>>                const struct sfdp_parameter_header *bfpt_header,
>>>>>>>>                const struct sfdp_bfpt *bfpt);
>>>>>>>>       int (*post_sfdp)(struct spi_nor *nor);
>>>>>>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>>>>>>       int (*late_init)(struct spi_nor *nor);
>>>>>>>>   };
>>>>>>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>>>>>>                    const struct sfdp_bfpt *bfpt);
>>>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>>>>>>> +
>>>>>>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>>>>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>>>>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>>>>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>>>>>> index 5b1117265bd2..542c775918ad 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>>>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>>>>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>>>>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>>>>>>       }
>>>>>>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>>>> +    if (err)
>>>>>>>> +        return ERR_PTR(err);
>>>>>>>> +
>>>>>>>>       /*
>>>>>>>>        * If command descriptors are provided, they always precede map
>>>>>>>>        * descriptors in the table. There is no need to start the iteration
>>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>>> index d6c92595f6bc..d446d12371e1 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>>>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>>>>>   };
>>>>>>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>>>> +{
>>>>>>>> +    /*
>>>>>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>>>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>>>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>>>>>> +     * device configuration, however according to section 7.5.5.1
>>>>>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>>>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>>>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>>>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>>>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>>>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>>>>>> +     * configuration index bit 0, which is populated exactly by
>>>>>>>> +     * the CR3NV bit 1 being 1.
>>>>>>>> +     */
>>>>>>>> +    *smpt |= BIT(0);
>>>>>>>
>>>>>>> Please help me understand this. Maybe a link to your revision of
>>>>>>> datasheet would help me.
>>>>>>
>>>>>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>>>>>
>>>>>> SMPT values:
>>>>>>
>>>>>> i=0 smpt[i]=08ff65fc
>>>>>> i=1 smpt[i]=00000004
>>>>>> i=2 smpt[i]=04ff65fc
>>>>>> i=3 smpt[i]=00000002
>>>>>> i=4 smpt[i]=02ff65fd
>>>>>> i=5 smpt[i]=00000004
>>>>>> i=6 smpt[i]=ff0201fe
>>>>>> i=7 smpt[i]=00007ff1
>>>>>> i=8 smpt[i]=00037ff4
>>>>>> i=9 smpt[i]=03fbfff4
>>>>>> i=10 smpt[i]=ff0203fe
>>>>>> i=11 smpt[i]=03fbfff4
>>>>>> i=12 smpt[i]=00037ff4
>>>>>> i=13 smpt[i]=00007ff1
>>>>>> i=14 smpt[i]=ff0005ff
>>>>>> i=15 smpt[i]=03fffff4
>>>>>>
>>>>>
>>>>> thanks!
>>>>>
>>>>>>> In the flash datasheets that I see, there shall
>>>>>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>>>>>> table is described showing an Index Value constructed by the CRxNV[y]
>>>>>>> return values. That index value is the map_id in the code.
>>>>>>>
>>>>>>> By reading your description I understand CR3NV[1] has value zero as it
>>>>>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>>>>>> always have value 1. That's why datasheets like this one [1] in their
>>>>>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>>>>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>>>>>
>>>>>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
>>>>>
>>>>> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
>>>>> "Index Value" column. I deduced the formula by using the CR3NV[3] and
>>>>> CR1NV[2] values. The datasheet says:
>>>>>
>>>>> "When more than one configuration bit must be read, all the bits are
>>>>> concatenated into an index value that is used to select the current address map."
>>>>>
>>>>> and that the "the following MSb to LSb order to form the configuration map
>>>>> index value:
>>>>> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
>>>>> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
>>>>> "
>>>>>
>>>> In old datasheet, Table 70 had CR3NV[1] column and the description about index
>>>> value had another bullet:
>>>>   • CR3NV[1] — 1 = 256 kB uniform sector size
>>>> These were removed when the default CRNV3[1] value was changed from 1 to 0.
>>>> As Marek pointed out, SMPT still involves CR3NV[1] expecting the value is
>>>> always 1.
> 
> If CR3NV[1] is always 1 even in the old datasheets, then replacing
> CR3NV[3] << 2 | CR1NV[2] << 1 | CR3NV[1]
> with
> CR3NV[3] << 2 | CR1NV[2] << 1 | 1 (CR3NV[1] was always 1 anyway)
> for the index value/map_id is backward compatible and okay, even for the
> older flashes.
> 
> Then Marek's idea of updating the map_id via a hook is sound. If we're going
> to update just the map id, I'd rename spi_nor_post_smpt_fixups() to
> spi_nor_smpt_map_id_fixup().
> 
>>>>
>>>>> The "Index Value" shall be the map_id that you passed in the code:
>>>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>
>>>>> Can you please print the map_id value that you obtain without updating it?
>>>>> Let's also print the values of CR3NV and CR1NV.
>>>>>
>>>> I did this in my environment and found another, fundamental issue in SMPT
>>>> contents and the way to parse it...
>>>>
>>>> The SMPT describes Configuration detection command latency as '1111b: variable
>>>> latency' and in spi_nor_smpt_read_dummy(),
>>>>
>>>> 	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
>>>> 		return nor->read_dummy;
>>>>
>>>> This results in 0 latency.
>>>> However, in S25FS-S, register read latency is 8 cycles by default. That means
Sorry, this was not accurate. In S25FS-S, register read latency of RDAR(65h) is
8 cycle by default. RDSR(05h)/RDCR(35h) has no latency.

> 
> Is there any other reg interaction in the probe path? What number of dummy cycles
> is it using?
> 
Only SMPT parse uses RDAR(65h) that requires dummy cycles. 

>>>
>>> If you hack the code and set 8 cycles for the read latency, do you get correct
>>> results?
>>>
>> No, we still need fix like Marek's or something.
> 
> Right
> 
>>
>>> Can the variable latency be determined by parsing SFDP?
>>>
>> No, I don't think we can determine by SFDP.
>>


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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Takahiro Kuwano]

Hi Tudor,

On 10/14/2025 4:22 PM, Takahiro Kuwano wrote:
> Hi Tudor,
> 
> On 10/10/2025 5:34 PM, Tudor Ambarus wrote:
>> Hi, Takahiro,
>>
>>>>>>>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>>>>>>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>>>>>>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>>>>>>>> and 256kiB uniform sectors device configuration, however according to
>>>>>>>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>>>>>>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>>>>>>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>>>>>>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>>>>>>>> cannot find a valid sector map entry and fails.
>>>>>>>>>
>>>>>>>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>>>>>>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>>>>>>>> fixup hook and a wrapper function. The only implementation of the hook is
>>>>>>>>> currently specific to S25FS512S.
>>>>>>>>>
>>>>>>>>> This fixes the following failure on S25FS512S:
>>>>>>>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>>>>>>> ---
>>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>>>> Cc: Michael Walle <mwalle@kernel.org>
>>>>>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>>>>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>>>>>>>> Cc: Richard Weinberger <richard@nod.at>
>>>>>>>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>>>>>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>>>>>>>> Cc: linux-mtd@lists.infradead.org
>>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>>>> ---
>>>>>>>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>>>>>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>>>>>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>>>>>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>>>>>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>>>>>> index 9d6e85bf227b..ca65f36e5638 100644
>>>>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>>>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>>>       return 0;
>>>>>>>>>   }
>>>>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>>>>> +{
>>>>>>>>> +    int ret;
>>>>>>>>> +
>>>>>>>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>>>>>> +        nor->manufacturer->fixups->post_smpt) {
>>>>>>>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>>>>>>>> +        if (ret)
>>>>>>>>> +            return ret;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>>>>>>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>>>>>>>> +
>>>>>>>>> +    return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>>>>>>>                      u32 shared_hwcaps)
>>>>>>>>>   {
>>>>>>>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>>>>>>>> index 1516b6d0dc37..d5294424ab9d 100644
>>>>>>>>> --- a/drivers/mtd/spi-nor/core.h
>>>>>>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>>>>>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>>>>>>>    *             parameters that could not be extracted by other means (i.e.
>>>>>>>>>    *             when information provided by the SFDP/flash_info tables are
>>>>>>>>>    *             incomplete or wrong).
>>>>>>>>> + * @post_smpt: update sector map configuration ID selector according to
>>>>>>>>> + *             chip-specific quirks.
>>>>>>>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>>>>>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>>>>>>>    *             Will replace the default_init() hook.
>>>>>>>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>>>>>>>                const struct sfdp_parameter_header *bfpt_header,
>>>>>>>>>                const struct sfdp_bfpt *bfpt);
>>>>>>>>>       int (*post_sfdp)(struct spi_nor *nor);
>>>>>>>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>>>>>>>       int (*late_init)(struct spi_nor *nor);
>>>>>>>>>   };
>>>>>>>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>>>>>>>                    const struct sfdp_bfpt *bfpt);
>>>>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>>>>>>>> +
>>>>>>>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>>>>>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>>>>>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>>>>>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>>>>>>> index 5b1117265bd2..542c775918ad 100644
>>>>>>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>>>>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>>>>>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>>>>>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>>>>>>>       }
>>>>>>>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>>>>> +    if (err)
>>>>>>>>> +        return ERR_PTR(err);
>>>>>>>>> +
>>>>>>>>>       /*
>>>>>>>>>        * If command descriptors are provided, they always precede map
>>>>>>>>>        * descriptors in the table. There is no need to start the iteration
>>>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>>>> index d6c92595f6bc..d446d12371e1 100644
>>>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>>>>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>>>>>>   };
>>>>>>>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>>>>> +{
>>>>>>>>> +    /*
>>>>>>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>>>>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>>>>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>>>>>>> +     * device configuration, however according to section 7.5.5.1
>>>>>>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>>>>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>>>>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>>>>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>>>>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>>>>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>>>>>>> +     * configuration index bit 0, which is populated exactly by
>>>>>>>>> +     * the CR3NV bit 1 being 1.
>>>>>>>>> +     */
>>>>>>>>> +    *smpt |= BIT(0);
>>>>>>>>
>>>>>>>> Please help me understand this. Maybe a link to your revision of
>>>>>>>> datasheet would help me.
>>>>>>>
>>>>>>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>>>>>>
>>>>>>> SMPT values:
>>>>>>>
>>>>>>> i=0 smpt[i]=08ff65fc
>>>>>>> i=1 smpt[i]=00000004
>>>>>>> i=2 smpt[i]=04ff65fc
>>>>>>> i=3 smpt[i]=00000002
>>>>>>> i=4 smpt[i]=02ff65fd
>>>>>>> i=5 smpt[i]=00000004
>>>>>>> i=6 smpt[i]=ff0201fe
>>>>>>> i=7 smpt[i]=00007ff1
>>>>>>> i=8 smpt[i]=00037ff4
>>>>>>> i=9 smpt[i]=03fbfff4
>>>>>>> i=10 smpt[i]=ff0203fe
>>>>>>> i=11 smpt[i]=03fbfff4
>>>>>>> i=12 smpt[i]=00037ff4
>>>>>>> i=13 smpt[i]=00007ff1
>>>>>>> i=14 smpt[i]=ff0005ff
>>>>>>> i=15 smpt[i]=03fffff4
>>>>>>>
>>>>>>
>>>>>> thanks!
>>>>>>
>>>>>>>> In the flash datasheets that I see, there shall
>>>>>>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>>>>>>> table is described showing an Index Value constructed by the CRxNV[y]
>>>>>>>> return values. That index value is the map_id in the code.
>>>>>>>>
>>>>>>>> By reading your description I understand CR3NV[1] has value zero as it
>>>>>>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>>>>>>> always have value 1. That's why datasheets like this one [1] in their
>>>>>>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>>>>>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>>>>>>
>>>>>>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
>>>>>>
>>>>>> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
>>>>>> "Index Value" column. I deduced the formula by using the CR3NV[3] and
>>>>>> CR1NV[2] values. The datasheet says:
>>>>>>
>>>>>> "When more than one configuration bit must be read, all the bits are
>>>>>> concatenated into an index value that is used to select the current address map."
>>>>>>
>>>>>> and that the "the following MSb to LSb order to form the configuration map
>>>>>> index value:
>>>>>> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
>>>>>> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
>>>>>> "
>>>>>>
>>>>> In old datasheet, Table 70 had CR3NV[1] column and the description about index
>>>>> value had another bullet:
>>>>>   • CR3NV[1] — 1 = 256 kB uniform sector size
>>>>> These were removed when the default CRNV3[1] value was changed from 1 to 0.
>>>>> As Marek pointed out, SMPT still involves CR3NV[1] expecting the value is
>>>>> always 1.
>>
>> If CR3NV[1] is always 1 even in the old datasheets, then replacing
>> CR3NV[3] << 2 | CR1NV[2] << 1 | CR3NV[1]
>> with
>> CR3NV[3] << 2 | CR1NV[2] << 1 | 1 (CR3NV[1] was always 1 anyway)
>> for the index value/map_id is backward compatible and okay, even for the
>> older flashes.
>>
>> Then Marek's idea of updating the map_id via a hook is sound. If we're going
>> to update just the map id, I'd rename spi_nor_post_smpt_fixups() to
>> spi_nor_smpt_map_id_fixup().
>>
>>>>>
>>>>>> The "Index Value" shall be the map_id that you passed in the code:
>>>>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>>
>>>>>> Can you please print the map_id value that you obtain without updating it?
>>>>>> Let's also print the values of CR3NV and CR1NV.
>>>>>>
>>>>> I did this in my environment and found another, fundamental issue in SMPT
>>>>> contents and the way to parse it...
>>>>>
>>>>> The SMPT describes Configuration detection command latency as '1111b: variable
>>>>> latency' and in spi_nor_smpt_read_dummy(),
>>>>>
>>>>> 	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
>>>>> 		return nor->read_dummy;
>>>>>
>>>>> This results in 0 latency.
>>>>> However, in S25FS-S, register read latency is 8 cycles by default. That means
> Sorry, this was not accurate. In S25FS-S, register read latency of RDAR(65h) is
> 8 cycle by default. RDSR(05h)/RDCR(35h) has no latency.
> 
>>
>> Is there any other reg interaction in the probe path? What number of dummy cycles
>> is it using?
>>
> Only SMPT parse uses RDAR(65h) that requires dummy cycles. 
> 
How about introducing new fixup like get_smpt_read_dummy() and call it when
read dummy is variable?

 static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
 {
 	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
 
-	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
+	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE) {
+		if (nor->info->fixups && nor->info->fixups->get_smpt_read_dummy)
+			return nor->info->fixups->get_smpt_read_dummy(nor);
+
 		return nor->read_dummy;
+	}
+
 	return read_dummy;
 }

>>>>
>>>> If you hack the code and set 8 cycles for the read latency, do you get correct
>>>> results?
>>>>
>>> No, we still need fix like Marek's or something.
>>
>> Right
>>
>>>
>>>> Can the variable latency be determined by parsing SFDP?
>>>>
>>> No, I don't think we can determine by SFDP.
>>>
> 


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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Tudor Ambarus]

On 10/17/25 4:44 AM, Takahiro Kuwano wrote:
> Hi Tudor,

Hi!

> 
> On 10/14/2025 4:22 PM, Takahiro Kuwano wrote:
>> Hi Tudor,
>>
>> On 10/10/2025 5:34 PM, Tudor Ambarus wrote:
>>> Hi, Takahiro,
>>>
>>>>>>>>> On 9/14/24 11:08 PM, Marek Vasut wrote:
>>>>>>>>>> The S25FS512S chip datasheet rev.O Table 71 on page 153 JEDEC Sector Map
>>>>>>>>>> Parameter Dword-6 Config. Detect-3 does use CR3NV bit 1 to discern 64kiB
>>>>>>>>>> and 256kiB uniform sectors device configuration, however according to
>>>>>>>>>> section 7.5.5.1 Configuration Register 3 Non-volatile (CR3NV) page 61,
>>>>>>>>>> the CR3NV bit 1 is RFU Reserved for Future Use, and is set to 0 on newly
>>>>>>>>>> manufactured devices, which means 64kiB sectors. Since the device does not
>>>>>>>>>> support 64kiB uniform sectors in any configuration, parsing SMPT table
>>>>>>>>>> cannot find a valid sector map entry and fails.
>>>>>>>>>>
>>>>>>>>>> Fix this up by setting SMPT configuration index bit 0, which is populated
>>>>>>>>>> exactly by the CR3NV bit 1 being 1. This is implemented via new .post_smpt
>>>>>>>>>> fixup hook and a wrapper function. The only implementation of the hook is
>>>>>>>>>> currently specific to S25FS512S.
>>>>>>>>>>
>>>>>>>>>> This fixes the following failure on S25FS512S:
>>>>>>>>>> spi-nor spi0.0: Failed to parse optional parameter table: ff81 (err=-22)
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>>>>>>>> ---
>>>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>>>>> Cc: Michael Walle <mwalle@kernel.org>
>>>>>>>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>>>>>>> Cc: Pratyush Yadav <pratyush@kernel.org>
>>>>>>>>>> Cc: Richard Weinberger <richard@nod.at>
>>>>>>>>>> Cc: Tudor Ambarus <tudor.ambarus@linaro.org>
>>>>>>>>>> Cc: Vignesh Raghavendra <vigneshr@ti.com>
>>>>>>>>>> Cc: linux-mtd@lists.infradead.org
>>>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>>>>> ---
>>>>>>>>>>   drivers/mtd/spi-nor/core.c     | 17 +++++++++++++++++
>>>>>>>>>>   drivers/mtd/spi-nor/core.h     |  5 +++++
>>>>>>>>>>   drivers/mtd/spi-nor/sfdp.c     |  4 ++++
>>>>>>>>>>   drivers/mtd/spi-nor/spansion.c | 27 ++++++++++++++++++++++++++-
>>>>>>>>>>   4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>>>>>>> index 9d6e85bf227b..ca65f36e5638 100644
>>>>>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>>>>>> @@ -2405,6 +2405,23 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>>>>       return 0;
>>>>>>>>>>   }
>>>>>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>>>>>> +{
>>>>>>>>>> +    int ret;
>>>>>>>>>> +
>>>>>>>>>> +    if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>>>>>>> +        nor->manufacturer->fixups->post_smpt) {
>>>>>>>>>> +        ret = nor->manufacturer->fixups->post_smpt(nor, smpt);
>>>>>>>>>> +        if (ret)
>>>>>>>>>> +            return ret;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    if (nor->info->fixups && nor->info->fixups->post_smpt)
>>>>>>>>>> +        return nor->info->fixups->post_smpt(nor, smpt);
>>>>>>>>>> +
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>   static int spi_nor_select_read(struct spi_nor *nor,
>>>>>>>>>>                      u32 shared_hwcaps)
>>>>>>>>>>   {
>>>>>>>>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>>>>>>>>> index 1516b6d0dc37..d5294424ab9d 100644
>>>>>>>>>> --- a/drivers/mtd/spi-nor/core.h
>>>>>>>>>> +++ b/drivers/mtd/spi-nor/core.h
>>>>>>>>>> @@ -413,6 +413,8 @@ struct spi_nor_flash_parameter {
>>>>>>>>>>    *             parameters that could not be extracted by other means (i.e.
>>>>>>>>>>    *             when information provided by the SFDP/flash_info tables are
>>>>>>>>>>    *             incomplete or wrong).
>>>>>>>>>> + * @post_smpt: update sector map configuration ID selector according to
>>>>>>>>>> + *             chip-specific quirks.
>>>>>>>>>>    * @late_init: used to initialize flash parameters that are not declared in the
>>>>>>>>>>    *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>>>>>>>>>>    *             Will replace the default_init() hook.
>>>>>>>>>> @@ -426,6 +428,7 @@ struct spi_nor_fixups {
>>>>>>>>>>                const struct sfdp_parameter_header *bfpt_header,
>>>>>>>>>>                const struct sfdp_bfpt *bfpt);
>>>>>>>>>>       int (*post_sfdp)(struct spi_nor *nor);
>>>>>>>>>> +    int (*post_smpt)(struct spi_nor *nor, u8 *smpt);
>>>>>>>>>>       int (*late_init)(struct spi_nor *nor);
>>>>>>>>>>   };
>>>>>>>>>>   @@ -660,6 +663,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>>>>                    const struct sfdp_parameter_header *bfpt_header,
>>>>>>>>>>                    const struct sfdp_bfpt *bfpt);
>>>>>>>>>>   +int spi_nor_post_smpt_fixups(struct spi_nor *nor, u8 *stmp);
>>>>>>>>>> +
>>>>>>>>>>   void spi_nor_init_default_locking_ops(struct spi_nor *nor);
>>>>>>>>>>   void spi_nor_try_unlock_all(struct spi_nor *nor);
>>>>>>>>>>   void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>>>>>>>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>>>>>>>> index 5b1117265bd2..542c775918ad 100644
>>>>>>>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>>>>>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>>>>>>>> @@ -765,6 +765,10 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>>>>>>>>           map_id = map_id << 1 | !!(*buf & read_data_mask);
>>>>>>>>>>       }
>>>>>>>>>>   +    err = spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>>>>>> +    if (err)
>>>>>>>>>> +        return ERR_PTR(err);
>>>>>>>>>> +
>>>>>>>>>>       /*
>>>>>>>>>>        * If command descriptors are provided, they always precede map
>>>>>>>>>>        * descriptors in the table. There is no need to start the iteration
>>>>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>>>>> index d6c92595f6bc..d446d12371e1 100644
>>>>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>>>>> @@ -757,6 +757,31 @@ static const struct spi_nor_fixups s25fs_s_nor_fixups = {
>>>>>>>>>>       .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
>>>>>>>>>>   };
>>>>>>>>>>   +static int s25fs512s_nor_post_smpt_fixups(struct spi_nor *nor, u8 *smpt)
>>>>>>>>>> +{
>>>>>>>>>> +    /*
>>>>>>>>>> +     * The S25FS512S chip datasheet rev.O Table 71 on page 153
>>>>>>>>>> +     * JEDEC Sector Map Parameter Dword-6 Config. Detect-3 does
>>>>>>>>>> +     * use CR3NV bit 1 to discern 64kiB/256kiB uniform sectors
>>>>>>>>>> +     * device configuration, however according to section 7.5.5.1
>>>>>>>>>> +     * Configuration Register 3 Non-volatile (CR3NV) page 61, the
>>>>>>>>>> +     * CR3NV bit 1 is RFU Reserved for Future Use, and is set to
>>>>>>>>>> +     * 0 on newly manufactured devices, which means 64kiB sectors.
>>>>>>>>>> +     * Since the device does not support 64kiB uniform sectors in
>>>>>>>>>> +     * any configuration, parsing SMPT table cannot find a valid
>>>>>>>>>> +     * sector map entry and fails. Fix this up by setting SMPT
>>>>>>>>>> +     * configuration index bit 0, which is populated exactly by
>>>>>>>>>> +     * the CR3NV bit 1 being 1.
>>>>>>>>>> +     */
>>>>>>>>>> +    *smpt |= BIT(0);
>>>>>>>>>
>>>>>>>>> Please help me understand this. Maybe a link to your revision of
>>>>>>>>> datasheet would help me.
>>>>>>>>
>>>>>>>> https://www.infineon.com/assets/row/public/documents/10/49/infineon-s25fs512s-512-mb-1-datasheet-en.pdf
>>>>>>>>
>>>>>>>> SMPT values:
>>>>>>>>
>>>>>>>> i=0 smpt[i]=08ff65fc
>>>>>>>> i=1 smpt[i]=00000004
>>>>>>>> i=2 smpt[i]=04ff65fc
>>>>>>>> i=3 smpt[i]=00000002
>>>>>>>> i=4 smpt[i]=02ff65fd
>>>>>>>> i=5 smpt[i]=00000004
>>>>>>>> i=6 smpt[i]=ff0201fe
>>>>>>>> i=7 smpt[i]=00007ff1
>>>>>>>> i=8 smpt[i]=00037ff4
>>>>>>>> i=9 smpt[i]=03fbfff4
>>>>>>>> i=10 smpt[i]=ff0203fe
>>>>>>>> i=11 smpt[i]=03fbfff4
>>>>>>>> i=12 smpt[i]=00037ff4
>>>>>>>> i=13 smpt[i]=00007ff1
>>>>>>>> i=14 smpt[i]=ff0005ff
>>>>>>>> i=15 smpt[i]=03fffff4
>>>>>>>>
>>>>>>>
>>>>>>> thanks!
>>>>>>>
>>>>>>>>> In the flash datasheets that I see, there shall
>>>>>>>>> be a "Sector Map Parameter Table Notes" where a "Sector Map Parameter"
>>>>>>>>> table is described showing an Index Value constructed by the CRxNV[y]
>>>>>>>>> return values. That index value is the map_id in the code.
>>>>>>>>>
>>>>>>>>> By reading your description I understand CR3NV[1] has value zero as it
>>>>>>>>> is marked as RFU, but at the same time that bit is expected in SMPT to
>>>>>>>>> always have value 1. That's why datasheets like this one [1] in their
>>>>>>>>> "Table 70. Sector Map Parameter" do not describe CR3NV[1] at all, and
>>>>>>>>> define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1.
>>>>>>>>
>>>>>>>> Where does this last part "define the index value as CR3NV[3] << 2 | CR1NV[2] << 1 | 1" come from ?
>>>>>>>
>>>>>>> In your datasheet flavor, from "Table 70 Sector map parameter", page 152,
>>>>>>> "Index Value" column. I deduced the formula by using the CR3NV[3] and
>>>>>>> CR1NV[2] values. The datasheet says:
>>>>>>>
>>>>>>> "When more than one configuration bit must be read, all the bits are
>>>>>>> concatenated into an index value that is used to select the current address map."
>>>>>>>
>>>>>>> and that the "the following MSb to LSb order to form the configuration map
>>>>>>> index value:
>>>>>>> • CR3NV[3] — 0 = Hybrid Architecture, 1 = Uniform Architecture
>>>>>>> • CR1NV[2] — 0 = 4 KB parameter sectors at bottom, 1 = 4 KB sectors at top
>>>>>>> "
>>>>>>>
>>>>>> In old datasheet, Table 70 had CR3NV[1] column and the description about index
>>>>>> value had another bullet:
>>>>>>   • CR3NV[1] — 1 = 256 kB uniform sector size
>>>>>> These were removed when the default CRNV3[1] value was changed from 1 to 0.
>>>>>> As Marek pointed out, SMPT still involves CR3NV[1] expecting the value is
>>>>>> always 1.
>>>
>>> If CR3NV[1] is always 1 even in the old datasheets, then replacing
>>> CR3NV[3] << 2 | CR1NV[2] << 1 | CR3NV[1]
>>> with
>>> CR3NV[3] << 2 | CR1NV[2] << 1 | 1 (CR3NV[1] was always 1 anyway)
>>> for the index value/map_id is backward compatible and okay, even for the
>>> older flashes.
>>>
>>> Then Marek's idea of updating the map_id via a hook is sound. If we're going
>>> to update just the map id, I'd rename spi_nor_post_smpt_fixups() to
>>> spi_nor_smpt_map_id_fixup().
>>>
>>>>>>
>>>>>>> The "Index Value" shall be the map_id that you passed in the code:
>>>>>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>>>
>>>>>>> Can you please print the map_id value that you obtain without updating it?
>>>>>>> Let's also print the values of CR3NV and CR1NV.
>>>>>>>
>>>>>> I did this in my environment and found another, fundamental issue in SMPT
>>>>>> contents and the way to parse it...
>>>>>>
>>>>>> The SMPT describes Configuration detection command latency as '1111b: variable
>>>>>> latency' and in spi_nor_smpt_read_dummy(),
>>>>>>
>>>>>> 	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
>>>>>> 		return nor->read_dummy;
>>>>>>
>>>>>> This results in 0 latency.
>>>>>> However, in S25FS-S, register read latency is 8 cycles by default. That means
>> Sorry, this was not accurate. In S25FS-S, register read latency of RDAR(65h) is
>> 8 cycle by default. RDSR(05h)/RDCR(35h) has no latency.
>>
>>>
>>> Is there any other reg interaction in the probe path? What number of dummy cycles
>>> is it using?
>>>
>> Only SMPT parse uses RDAR(65h) that requires dummy cycles. 
>>
> How about introducing new fixup like get_smpt_read_dummy() and call it when
> read dummy is variable?
> 
>  static u8 spi_nor_smpt_read_dummy(const struct spi_nor *nor, const u32 settings)
>  {
>  	u8 read_dummy = SMPT_CMD_READ_DUMMY(settings);
>  
> -	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE)
> +	if (read_dummy == SMPT_CMD_READ_DUMMY_IS_VARIABLE) {
> +		if (nor->info->fixups && nor->info->fixups->get_smpt_read_dummy)
> +			return nor->info->fixups->get_smpt_read_dummy(nor);
> +
>  		return nor->read_dummy;
> +	}
> +
>  	return read_dummy;
>  }

That's okay. You'll also need a fixup for the CR3NV[1] bit.

Cheers,
ta

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Marek Vasut]

On 10/13/25 5:41 PM, Tudor Ambarus wrote:

Hello Tudor,

>>> The "Index Value" shall be the map_id that you passed in the code:
>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>
>>> Can you please print the map_id value that you obtain without updating it?
>>
>> 0x4
> 
> This translates to CR3NV[3] = 1, CR1NV[2] = 0,  CR3NV[1] = 0.
>>
>>> Let's also print the values of CR3NV and CR1NV.
>>
>> Both 0x0 and 0x0 .
> 
> But here CR3NV is 0, it contradicts the result from above.

Maybe I messed up and was reading it the wrong way ?

> Maybe it's the same problem that Takahiro identified: the flash needs
> 8 dummy cycles, but the code uses zero dummy cycles, resulting in
> reading garbage data, depending on whether your IO lines are pulled up/down
> or floating.
> 
> Can you redo the test with the following please?

Sure, although I saw some further discussion between you and Kuwano-san 
, is this still applicable ?

> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 21727f9a4ac6..85443c903e59 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -752,7 +752,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>   
>                  read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
>                  nor->addr_nbytes = spi_nor_smpt_addr_nbytes(nor, smpt[i]);
> -               nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
> +               nor->read_dummy = 8;
>                  nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
>                  addr = smpt[i + 1];
>   
> @@ -767,6 +767,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>                   * Configuration that is currently in use.
>                   */
>                  map_id = map_id << 1 | !!(*buf & read_data_mask);
> +               dev_err(nor->dev, "i = %d, buf = %02x, map_id = %02x\n",
> +                       i, buf[0], map_id);
>          }
Sorry for the late reply.

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Tudor Ambarus]

On 10/17/25 5:04 PM, Marek Vasut wrote:
> On 10/13/25 5:41 PM, Tudor Ambarus wrote:
> 
> Hello Tudor,

Hi!

> 
>>>> The "Index Value" shall be the map_id that you passed in the code:
>>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>>
>>>> Can you please print the map_id value that you obtain without updating it?
>>>
>>> 0x4
>>
>> This translates to CR3NV[3] = 1, CR1NV[2] = 0,  CR3NV[1] = 0.
>>>
>>>> Let's also print the values of CR3NV and CR1NV.
>>>
>>> Both 0x0 and 0x0 .
>>
>> But here CR3NV is 0, it contradicts the result from above.
> 
> Maybe I messed up and was reading it the wrong way ?
> 
>> Maybe it's the same problem that Takahiro identified: the flash needs
>> 8 dummy cycles, but the code uses zero dummy cycles, resulting in
>> reading garbage data, depending on whether your IO lines are pulled up/down
>> or floating.
>>
>> Can you redo the test with the following please?
> 
> Sure, although I saw some further discussion between you and Kuwano-san , is this still applicable ?

It's still applicable, it will confirm what Kuwano-san discovered:
the flash needs 8 dummy cycles for using that read reg command.

We'll also need a hook to amend the CR3NV[1] value (which is reserved,
zero on some flavors of flash) and replace it with 1, similar to what
you did in this patch.

You can either test or let Kuwano-san come up with a patch addressing
both issues and test his patch afterwards, sync up with him please.

> 
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index 21727f9a4ac6..85443c903e59 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -752,7 +752,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>                    read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
>>                  nor->addr_nbytes = spi_nor_smpt_addr_nbytes(nor, smpt[i]);
>> -               nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
>> +               nor->read_dummy = 8;
>>                  nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
>>                  addr = smpt[i + 1];
>>   @@ -767,6 +767,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>                   * Configuration that is currently in use.
>>                   */
>>                  map_id = map_id << 1 | !!(*buf & read_data_mask);
>> +               dev_err(nor->dev, "i = %d, buf = %02x, map_id = %02x\n",
>> +                       i, buf[0], map_id);
>>          }
> Sorry for the late reply.
yeah, no worries, it's still fresh.

Cheers,
ta

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Takahiro Kuwano]

Hi Tudor and Marek,

I have submitted new series.
https://lore.kernel.org/linux-mtd/20251022-s25fs-s-smpt-fixup-v1-0-ce26d4084b2d@infineon.com/T/#m2fa41b89e36d6061df9e979f49c573e46fd0d5c4

Please review and test then give feedback.

Thanks,
Takahiro

On 10/20/2025 8:35 PM, Tudor Ambarus wrote:
> 
> 
> On 10/17/25 5:04 PM, Marek Vasut wrote:
>> On 10/13/25 5:41 PM, Tudor Ambarus wrote:
>>
>> Hello Tudor,
> 
> Hi!
> 
>>
>>>>> The "Index Value" shall be the map_id that you passed in the code:
>>>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>
>>>>> Can you please print the map_id value that you obtain without updating it?
>>>>
>>>> 0x4
>>>
>>> This translates to CR3NV[3] = 1, CR1NV[2] = 0,  CR3NV[1] = 0.
>>>>
>>>>> Let's also print the values of CR3NV and CR1NV.
>>>>
>>>> Both 0x0 and 0x0 .
>>>
>>> But here CR3NV is 0, it contradicts the result from above.
>>
>> Maybe I messed up and was reading it the wrong way ?
>>
>>> Maybe it's the same problem that Takahiro identified: the flash needs
>>> 8 dummy cycles, but the code uses zero dummy cycles, resulting in
>>> reading garbage data, depending on whether your IO lines are pulled up/down
>>> or floating.
>>>
>>> Can you redo the test with the following please?
>>
>> Sure, although I saw some further discussion between you and Kuwano-san , is this still applicable ?
> 
> It's still applicable, it will confirm what Kuwano-san discovered:
> the flash needs 8 dummy cycles for using that read reg command.
> 
> We'll also need a hook to amend the CR3NV[1] value (which is reserved,
> zero on some flavors of flash) and replace it with 1, similar to what
> you did in this patch.
> 
> You can either test or let Kuwano-san come up with a patch addressing
> both issues and test his patch afterwards, sync up with him please.
> 
>>
>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>> index 21727f9a4ac6..85443c903e59 100644
>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>> @@ -752,7 +752,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>                    read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
>>>                  nor->addr_nbytes = spi_nor_smpt_addr_nbytes(nor, smpt[i]);
>>> -               nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
>>> +               nor->read_dummy = 8;
>>>                  nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
>>>                  addr = smpt[i + 1];
>>>   @@ -767,6 +767,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>                   * Configuration that is currently in use.
>>>                   */
>>>                  map_id = map_id << 1 | !!(*buf & read_data_mask);
>>> +               dev_err(nor->dev, "i = %d, buf = %02x, map_id = %02x\n",
>>> +                       i, buf[0], map_id);
>>>          }
>> Sorry for the late reply.
> yeah, no worries, it's still fresh.
> 
> Cheers,
> ta


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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Marek Vasut]

On 10/20/25 1:35 PM, Tudor Ambarus wrote:

Hello Tudor,

I apologize for my slow response.

>>>>> The "Index Value" shall be the map_id that you passed in the code:
>>>>> spi_nor_post_smpt_fixups(nor, &map_id);
>>>>>
>>>>> Can you please print the map_id value that you obtain without updating it?
>>>>
>>>> 0x4
>>>
>>> This translates to CR3NV[3] = 1, CR1NV[2] = 0,  CR3NV[1] = 0.
>>>>
>>>>> Let's also print the values of CR3NV and CR1NV.
>>>>
>>>> Both 0x0 and 0x0 .
>>>
>>> But here CR3NV is 0, it contradicts the result from above.
>>
>> Maybe I messed up and was reading it the wrong way ?

I really wonder if I might be reading the CR3NV the wrong way ?

>>> Maybe it's the same problem that Takahiro identified: the flash needs
>>> 8 dummy cycles, but the code uses zero dummy cycles, resulting in
>>> reading garbage data, depending on whether your IO lines are pulled up/down
>>> or floating.
>>>
>>> Can you redo the test with the following please?
>>
>> Sure, although I saw some further discussion between you and Kuwano-san , is this still applicable ?
> 
> It's still applicable, it will confirm what Kuwano-san discovered:
> the flash needs 8 dummy cycles for using that read reg command.
> 
> We'll also need a hook to amend the CR3NV[1] value (which is reserved,
> zero on some flavors of flash) and replace it with 1, similar to what
> you did in this patch.
> 
> You can either test or let Kuwano-san come up with a patch addressing
> both issues and test his patch afterwards, sync up with him please.
> 
>>
>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>> index 21727f9a4ac6..85443c903e59 100644
>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>> @@ -752,7 +752,7 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>                     read_data_mask = SMPT_CMD_READ_DATA(smpt[i]);
>>>                   nor->addr_nbytes = spi_nor_smpt_addr_nbytes(nor, smpt[i]);
>>> -               nor->read_dummy = spi_nor_smpt_read_dummy(nor, smpt[i]);
>>> +               nor->read_dummy = 8;
>>>                   nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]);
>>>                   addr = smpt[i + 1];
>>>    @@ -767,6 +767,8 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt,
>>>                    * Configuration that is currently in use.
>>>                    */
>>>                   map_id = map_id << 1 | !!(*buf & read_data_mask);
>>> +               dev_err(nor->dev, "i = %d, buf = %02x, map_id = %02x\n",
>>> +                       i, buf[0], map_id);
>>>           }
>> Sorry for the late reply.
> yeah, no worries, it's still fresh.

With current mainline, without the aforementioned patch, with my 
CR1NV/CR3NV printing (maybe broken):

CR1NV=00
CR3NV=00
map_id=00000004

With the read_dummy=8 addition above and the dev_err, I get:

spi-nor spi0.0: i = 0, buf = 00, map_id = 00
spi-nor spi0.0: i = 2, buf = 02, map_id = 00
spi-nor spi0.0: i = 4, buf = 00, map_id = 00
map_id=00000000

The reported failure persists:

spi-nor spi0.0: Failed to parse optional parameter table: ff81

Let me test the patchset from Kuwano-san next .

Thank you for your help!

-- 
Best regards,
Marek Vasut

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

Re: [PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S

[Marek Vasut]

On 10/22/25 12:13 PM, Takahiro Kuwano wrote:
> Hi Tudor and Marek,

Hello Kuwano-san,

I apologize for my delayed reply.

> I have submitted new series.
> https://lore.kernel.org/linux-mtd/20251022-s25fs-s-smpt-fixup-v1-0-ce26d4084b2d@infineon.com/T/#m2fa41b89e36d6061df9e979f49c573e46fd0d5c4
> 
> Please review and test then give feedback.

With the patchset applied, the readback looks as follows and the map ID 
seems correct now:

spi-nor spi0.0: i = 0, buf = 00, map_id = 00
spi-nor spi0.0: i = 2, buf = 02, map_id = 00
spi-nor spi0.0: i = 4, buf = 00, map_id = 00
spi_nor_get_map_in_use[800] map_id=00000001

Thank you for your help !

-- 
Best regards,
Marek Vasut

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