[PATCH] mtd: spi-nor: spansion: Add SMPT fixup for S25FS512S
Tudor Ambarus
tudor.ambarus at linaro.org
Tue Oct 7 23:47:41 PDT 2025
+ 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 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
> "
>
> 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.
>
More information about the linux-mtd
mailing list