[PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S
Takahiro Kuwano
tkuw584924 at gmail.com
Thu Oct 16 20:44:35 PDT 2025
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 at mailbox.org>
>>>>>>>>> ---
>>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas at glider.be>
>>>>>>>>> Cc: Michael Walle <mwalle at kernel.org>
>>>>>>>>> Cc: Miquel Raynal <miquel.raynal at bootlin.com>
>>>>>>>>> Cc: Pratyush Yadav <pratyush at kernel.org>
>>>>>>>>> Cc: Richard Weinberger <richard at nod.at>
>>>>>>>>> Cc: Tudor Ambarus <tudor.ambarus at linaro.org>
>>>>>>>>> Cc: Vignesh Raghavendra <vigneshr at ti.com>
>>>>>>>>> Cc: linux-mtd at lists.infradead.org
>>>>>>>>> Cc: linux-renesas-soc at 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.
>>>
>
More information about the linux-mtd
mailing list