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@xxxxxxxxxxx>
> ---
> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Cc: Michael Walle <mwalle@xxxxxxxxxx>
> Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> Cc: Pratyush Yadav <pratyush@xxxxxxxxxx>
> Cc: Richard Weinberger <richard@xxxxxx>
> Cc: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> Cc: Vignesh Raghavendra <vigneshr@xxxxxx>
> Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> ---
> 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
