[PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S
Marek Vasut
marek.vasut at mailbox.org
Tue Oct 7 01:28:36 PDT 2025
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 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
>
>> 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.
More information about the linux-mtd
mailing list