* [PATCH 0/3] Add support for S25FS512S1, and MX25U51279G
@ 2024-11-26 18:58 Vishwaroop A
2024-11-26 18:58 ` [PATCH 1/3] mtd: spi-nor: Add post-get-map-id fixup for S25FS512S/S1 Vishwaroop A
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Vishwaroop A @ 2024-11-26 18:58 UTC (permalink / raw)
To: tudor.ambarus, pratyush, mwalle, cmiquel.raynal, thierry.reding,
richard, vigneshr, linux-mtd, linux-kernel, jonathanh,
kyarlagadda, smangipudi
Cc: Vishwaroop A
This patch series adds support for three 64MB SPI NOR flash devices:
1. S25FS512S: Add a post-get-map-id fixup to address an issue with
the SFDP Address Map incorrectly reporting map ID as 0.
2. S25FS512S1: Add support for this Spansion device, including
dual/quad read capabilities and s25fs_s_nor_fixups.
3. MX25U51279G: Add support for this Macronix device with 4K sector
erase, dual/quad read capabilities, and 4-byte opcode support.
Vishwaroop A (3):
mtd: spi-nor: Add post-get-map-id fixup for S25FS512S/S1
mtd: spi-nor: Add support for spansion s25fs512s1
mtd: spi-nor: Add support for mx25u51279g
drivers/mtd/spi-nor/core.c | 25 ++++++++++++++++++
drivers/mtd/spi-nor/core.h | 4 +++
drivers/mtd/spi-nor/macronix.c | 6 +++++
drivers/mtd/spi-nor/sfdp.c | 7 +++++
drivers/mtd/spi-nor/spansion.c | 48 ++++++++++++++++++++++++++++++++++
5 files changed, 90 insertions(+)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] mtd: spi-nor: Add post-get-map-id fixup for S25FS512S/S1
2024-11-26 18:58 [PATCH 0/3] Add support for S25FS512S1, and MX25U51279G Vishwaroop A
@ 2024-11-26 18:58 ` Vishwaroop A
2024-12-06 15:05 ` Pratyush Yadav
2024-11-26 18:58 ` [PATCH 2/3] mtd: spi-nor: Add support for spansion s25fs512s1 Vishwaroop A
2024-11-26 18:58 ` [PATCH 3/3] mtd: spi-nor: Add support for mx25u51279g Vishwaroop A
2 siblings, 1 reply; 6+ messages in thread
From: Vishwaroop A @ 2024-11-26 18:58 UTC (permalink / raw)
To: tudor.ambarus, pratyush, mwalle, cmiquel.raynal, thierry.reding,
richard, vigneshr, linux-mtd, linux-kernel, jonathanh,
kyarlagadda, smangipudi
Cc: Vishwaroop A
The SFDP Address Map for S25FS512S / S25FS512S1 devices incorrectly
reports that the map ID is 0 when it should be 1. This issue can
cause problems when trying to erase sectors on the flash device.
Add a post-get-map-id fixup for S25FS512S / S1 flash devices. The fixup
reads the values of the CR3V and CR1V registers and determines the
map ID based on those values. The fixup also checks for invalid
combinations of CR3V and CR1V values.This fixup is necessary to
workaround an issue with the SFDP Address Map for S25FS512S flash.
Change-Id: Ide18bb4ee076cd36c57b0b52b5d49b63c3caf322
Signed-off-by: Vishwaroop A <va@nvidia.com>
---
drivers/mtd/spi-nor/core.c | 25 +++++++++++++++++++++
drivers/mtd/spi-nor/core.h | 4 ++++
drivers/mtd/spi-nor/sfdp.c | 7 ++++++
drivers/mtd/spi-nor/spansion.c | 41 ++++++++++++++++++++++++++++++++++
4 files changed, 77 insertions(+)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 66949d9f0cc5..a76202c6d252 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2408,6 +2408,31 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
return 0;
}
+/**
+ * spi_nor_post_get_map_id_fixups - Apply post-processing fixups for map ID
+ * @nor: Pointer to the spi_nor structure
+ * @smpt: Pointer to the sector map parameter table
+ * @smpt_len: Length of the sector map parameter table
+ * @map_id: Pointer to store the updated map ID
+ *
+ * Return: 0 on success (including when no fixup is applied),
+ * positive value if a new map_id is set,
+ * negative value on error
+ */
+int spi_nor_post_get_map_id_fixups(struct spi_nor *nor, const u32 *smpt,
+ u8 smpt_len, u8 *map_id)
+{
+ int ret;
+
+ if (nor->info->fixups && nor->info->fixups->post_get_map_id) {
+ ret = nor->info->fixups->post_get_map_id(nor, smpt, smpt_len);
+ if (ret < 0)
+ return ret;
+ *map_id = ret;
+ }
+ 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 5c33740ed7f5..37a9f43e1bf9 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -428,6 +428,7 @@ struct spi_nor_fixups {
const struct sfdp_bfpt *bfpt);
int (*post_sfdp)(struct spi_nor *nor);
int (*late_init)(struct spi_nor *nor);
+ int (*post_get_map_id)(struct spi_nor *nor, const u32 *smpt, u8 smpt_len);
};
/**
@@ -661,6 +662,9 @@ 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_get_map_id_fixups(struct spi_nor *nor, const u32 *smpt,
+ u8 smpt_len, u8 *map_id);
+
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 21727f9a4ac6..87af29d2c28b 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -769,6 +769,13 @@ 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_get_map_id_fixups(nor, smpt, smpt_len, &map_id);
+
+ if (err < 0) {
+ dev_err(nor->dev, "Error in post_get_map_id fixup: %d\n", 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 5a88a6096ca8..2e1dd023a1aa 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -25,6 +25,7 @@
#define SPINOR_REG_CYPRESS_STR1V \
(SPINOR_REG_CYPRESS_VREG + SPINOR_REG_CYPRESS_STR1)
#define SPINOR_REG_CYPRESS_CFR1 0x2
+#define SPINOR_REG_CYPRESS_CFR1V 0x00800002
#define SPINOR_REG_CYPRESS_CFR1_QUAD_EN BIT(1) /* Quad Enable */
#define SPINOR_REG_CYPRESS_CFR2 0x3
#define SPINOR_REG_CYPRESS_CFR2V \
@@ -33,6 +34,7 @@
#define SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24 0xb
#define SPINOR_REG_CYPRESS_CFR2_ADRBYT BIT(7)
#define SPINOR_REG_CYPRESS_CFR3 0x4
+#define SPINOR_REG_CYPRESS_CFR3V 0x00800004
#define SPINOR_REG_CYPRESS_CFR3_PGSZ BIT(4) /* Page size. */
#define SPINOR_REG_CYPRESS_CFR5 0x6
#define SPINOR_REG_CYPRESS_CFR5_BIT6 BIT(6)
@@ -754,8 +756,47 @@ s25fs_s_nor_post_bfpt_fixups(struct spi_nor *nor,
return 0;
}
+static int s25fs_s_nor_post_get_map_id(struct spi_nor *nor, const u32 *smpt, u8 smpt_len)
+{
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 1),
+ SPI_MEM_OP_ADDR(3, 0, 1),
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
+
+ u8 reg_cr3v_val, reg_cr1v_val;
+ int ret;
+
+ /* Read CR3V value from Configuration Register 3 Volatile */
+ op.addr.val = SPINOR_REG_CYPRESS_CFR3V;
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ if (ret)
+ return ret;
+ reg_cr3v_val = nor->bouncebuf[0];
+
+ /* Read CR1V value from Configuration Register 1 Volatile */
+ op.addr.val = SPINOR_REG_CYPRESS_CFR1V;
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ if (ret)
+ return ret;
+ reg_cr1v_val = nor->bouncebuf[0];
+
+ /* Determine the map ID based on CR3V[3] and CR1V[2] values */
+ if (!(reg_cr3v_val & BIT(3)) && !(reg_cr1v_val & BIT(2)))
+ return 1; /* CR3V[3] = 0, CR1V[2] = 0, map id = 1 */
+
+ if (!(reg_cr3v_val & BIT(3)) && (reg_cr1v_val & BIT(2)))
+ return 3; /* CR3V[3] = 0, CR1V[2] = 1, map id = 3 */
+
+ if ((reg_cr3v_val & BIT(3)) && !(reg_cr1v_val & BIT(2)))
+ return 5; /* CR3V[3] = 1, CR1V[2] = 0, map id = 5 */
+
+ return 0;
+}
+
static const struct spi_nor_fixups s25fs_s_nor_fixups = {
.post_bfpt = s25fs_s_nor_post_bfpt_fixups,
+ .post_get_map_id = s25fs_s_nor_post_get_map_id,
};
static const struct flash_info spansion_nor_parts[] = {
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] mtd: spi-nor: Add support for spansion s25fs512s1
2024-11-26 18:58 [PATCH 0/3] Add support for S25FS512S1, and MX25U51279G Vishwaroop A
2024-11-26 18:58 ` [PATCH 1/3] mtd: spi-nor: Add post-get-map-id fixup for S25FS512S/S1 Vishwaroop A
@ 2024-11-26 18:58 ` Vishwaroop A
2024-12-06 15:08 ` Pratyush Yadav
2024-11-26 18:58 ` [PATCH 3/3] mtd: spi-nor: Add support for mx25u51279g Vishwaroop A
2 siblings, 1 reply; 6+ messages in thread
From: Vishwaroop A @ 2024-11-26 18:58 UTC (permalink / raw)
To: tudor.ambarus, pratyush, mwalle, cmiquel.raynal, thierry.reding,
richard, vigneshr, linux-mtd, linux-kernel, jonathanh,
kyarlagadda, smangipudi
Cc: Vishwaroop A
Add support for the spansion s25fs512s1 SPI NOR flash. This device has
a 64MB size (SZ_64M), dual/quad read capabilities and apply
s25fs_s_nor_fixups to handle specific chip behavior.
Erasing, reading and writing this flash device has been validated on
the Jetson AGX Orin platform using mtd_debug and dd utilities.
Change-Id: I47ee39b33262c77a5f3601cd9f284e8291da27d5
Signed-off-by: Vishwaroop A <va@nvidia.com>
---
drivers/mtd/spi-nor/spansion.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 2e1dd023a1aa..472773891dad 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -901,6 +901,13 @@ static const struct flash_info spansion_nor_parts[] = {
.size = SZ_16M,
.no_sfdp_flags = SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
.mfr_flags = USE_CLSR,
+ }, {
+ .id = SNOR_ID(0x01, 0x02, 0x20, 0x4d, 0x01, 0x81),
+ .name = "s25fs512s1",
+ .size = SZ_64M,
+ .no_sfdp_flags = SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
+ .mfr_flags = USE_CLSR,
+ .fixups = &s25fs_s_nor_fixups,
}, {
.id = SNOR_ID(0x01, 0x20, 0x18, 0x4d, 0x01, 0x81),
.name = "s25fs128s1",
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] mtd: spi-nor: Add support for mx25u51279g
2024-11-26 18:58 [PATCH 0/3] Add support for S25FS512S1, and MX25U51279G Vishwaroop A
2024-11-26 18:58 ` [PATCH 1/3] mtd: spi-nor: Add post-get-map-id fixup for S25FS512S/S1 Vishwaroop A
2024-11-26 18:58 ` [PATCH 2/3] mtd: spi-nor: Add support for spansion s25fs512s1 Vishwaroop A
@ 2024-11-26 18:58 ` Vishwaroop A
2 siblings, 0 replies; 6+ messages in thread
From: Vishwaroop A @ 2024-11-26 18:58 UTC (permalink / raw)
To: tudor.ambarus, pratyush, mwalle, cmiquel.raynal, thierry.reding,
richard, vigneshr, linux-mtd, linux-kernel, jonathanh,
kyarlagadda, smangipudi
Cc: Vishwaroop A
Add support for the Macronix mx25u51279g SPI NOR flash. This device has
a 64MB size with 4K sector erase, dual/quad read capabilities and
support 4-bytes opcodes.
Erasing, reading and writing this flash device has been validated on
the Jetson AGX Orin platform using mtd_debug and dd utilities.
Change-Id: I740269c781d42781431935e593f651573e078372
Signed-off-by: Vishwaroop A <va@nvidia.com>
---
drivers/mtd/spi-nor/macronix.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 830da21eea08..4a15984f63a8 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -154,6 +154,12 @@ static const struct flash_info macronix_nor_parts[] = {
.size = SZ_64M,
.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
.fixup_flags = SPI_NOR_4B_OPCODES,
+ }, {
+ .id = SNOR_ID(0xc2, 0x95, 0x3a),
+ .name = "mx25u51279g",
+ .size = SZ_64M,
+ .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
+ .fixup_flags = SPI_NOR_4B_OPCODES,
}, {
.id = SNOR_ID(0xc2, 0x25, 0x3a),
.name = "mx66u51235f",
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] mtd: spi-nor: Add post-get-map-id fixup for S25FS512S/S1
2024-11-26 18:58 ` [PATCH 1/3] mtd: spi-nor: Add post-get-map-id fixup for S25FS512S/S1 Vishwaroop A
@ 2024-12-06 15:05 ` Pratyush Yadav
0 siblings, 0 replies; 6+ messages in thread
From: Pratyush Yadav @ 2024-12-06 15:05 UTC (permalink / raw)
To: Vishwaroop A
Cc: tudor.ambarus, pratyush, mwalle, cmiquel.raynal, thierry.reding,
richard, vigneshr, linux-mtd, linux-kernel, jonathanh,
kyarlagadda, smangipudi
Hi Vishwaroop,
Thanks for the patch.
On Tue, Nov 26 2024, Vishwaroop A wrote:
> The SFDP Address Map for S25FS512S / S25FS512S1 devices incorrectly
> reports that the map ID is 0 when it should be 1. This issue can
> cause problems when trying to erase sectors on the flash device.
This isn't exactly what your patch does. It also takes care of when map
ID should be 1, 3, or 5. Please update the commit message to reflect
that, and also probably describe in more detail how you got to those
values.
>
> Add a post-get-map-id fixup for S25FS512S / S1 flash devices. The fixup
> reads the values of the CR3V and CR1V registers and determines the
> map ID based on those values. The fixup also checks for invalid
> combinations of CR3V and CR1V values.This fixup is necessary to
> workaround an issue with the SFDP Address Map for S25FS512S flash.
Do you really need a new fixup for that? Why not set the erase map via
the post_sfdp fixup? Does that add too much extra code?
This is my main question for this patch and would like to get an answer
before we move forward. Still, leaving a quick code review below as
well.
>
> Change-Id: Ide18bb4ee076cd36c57b0b52b5d49b63c3caf322
What does this change ID mean? Is this for your internal systems
(gerrit?). In that case, it has no relevance for the upstream tree and
should be removed.
> Signed-off-by: Vishwaroop A <va@nvidia.com>
> ---
> drivers/mtd/spi-nor/core.c | 25 +++++++++++++++++++++
> drivers/mtd/spi-nor/core.h | 4 ++++
> drivers/mtd/spi-nor/sfdp.c | 7 ++++++
> drivers/mtd/spi-nor/spansion.c | 41 ++++++++++++++++++++++++++++++++++
> 4 files changed, 77 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 66949d9f0cc5..a76202c6d252 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2408,6 +2408,31 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
> return 0;
> }
>
> +/**
> + * spi_nor_post_get_map_id_fixups - Apply post-processing fixups for map ID
> + * @nor: Pointer to the spi_nor structure
> + * @smpt: Pointer to the sector map parameter table
> + * @smpt_len: Length of the sector map parameter table
> + * @map_id: Pointer to store the updated map ID
> + *
> + * Return: 0 on success (including when no fixup is applied),
> + * positive value if a new map_id is set,
Not true. The function never returns a positive value. Does it even need
to? I suppose the caller never needs to know, it should just use
whatever is in map_id.
Honestly, I would like to have similar return semantics for
spi_nor_post_get_map_id_fixups() and for the actual fixup callbacks.
Having one return map ID and another fill it in a pointer and return
just success/error is confusing. Pick one and stick with it, and I think
how the fixup callbacks do it makes more sense. Perhaps even pass in the
current map_id to the fixup so it can just choose the default if it sees
it doesn't have to fix anything up.
> + * negative value on error
> + */
> +int spi_nor_post_get_map_id_fixups(struct spi_nor *nor, const u32 *smpt,
> + u8 smpt_len, u8 *map_id)
> +{
> + int ret;
> +
> + if (nor->info->fixups && nor->info->fixups->post_get_map_id) {
Nit: might as well do:
struct spi_nor_fixups *fixups = nor->info->fixups;
and make this easier to parse.
> + ret = nor->info->fixups->post_get_map_id(nor, smpt, smpt_len);
> + if (ret < 0)
> + return ret;
> + *map_id = ret;
> + }
> + 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 5c33740ed7f5..37a9f43e1bf9 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -428,6 +428,7 @@ struct spi_nor_fixups {
> const struct sfdp_bfpt *bfpt);
> int (*post_sfdp)(struct spi_nor *nor);
> int (*late_init)(struct spi_nor *nor);
> + int (*post_get_map_id)(struct spi_nor *nor, const u32 *smpt, u8 smpt_len);
Should update documentation for the struct as well.
> };
>
> /**
> @@ -661,6 +662,9 @@ 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_get_map_id_fixups(struct spi_nor *nor, const u32 *smpt,
> + u8 smpt_len, u8 *map_id);
> +
> 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 21727f9a4ac6..87af29d2c28b 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -769,6 +769,13 @@ 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_get_map_id_fixups(nor, smpt, smpt_len, &map_id);
> +
Nit: drop the blank line.
> + if (err < 0) {
> + dev_err(nor->dev, "Error in post_get_map_id fixup: %d\n", 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 5a88a6096ca8..2e1dd023a1aa 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -25,6 +25,7 @@
> #define SPINOR_REG_CYPRESS_STR1V \
> (SPINOR_REG_CYPRESS_VREG + SPINOR_REG_CYPRESS_STR1)
> #define SPINOR_REG_CYPRESS_CFR1 0x2
> +#define SPINOR_REG_CYPRESS_CFR1V 0x00800002
> #define SPINOR_REG_CYPRESS_CFR1_QUAD_EN BIT(1) /* Quad Enable */
> #define SPINOR_REG_CYPRESS_CFR2 0x3
> #define SPINOR_REG_CYPRESS_CFR2V \
> @@ -33,6 +34,7 @@
> #define SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24 0xb
> #define SPINOR_REG_CYPRESS_CFR2_ADRBYT BIT(7)
> #define SPINOR_REG_CYPRESS_CFR3 0x4
> +#define SPINOR_REG_CYPRESS_CFR3V 0x00800004
> #define SPINOR_REG_CYPRESS_CFR3_PGSZ BIT(4) /* Page size. */
> #define SPINOR_REG_CYPRESS_CFR5 0x6
> #define SPINOR_REG_CYPRESS_CFR5_BIT6 BIT(6)
> @@ -754,8 +756,47 @@ s25fs_s_nor_post_bfpt_fixups(struct spi_nor *nor,
> return 0;
> }
>
> +static int s25fs_s_nor_post_get_map_id(struct spi_nor *nor, const u32 *smpt, u8 smpt_len)
> +{
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 1),
> + SPI_MEM_OP_ADDR(3, 0, 1),
Should you use addr_width here instead of hard-coding 3?
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_IN(1, nor->bouncebuf, 1));
> +
> + u8 reg_cr3v_val, reg_cr1v_val;
> + int ret;
> +
> + /* Read CR3V value from Configuration Register 3 Volatile */
> + op.addr.val = SPINOR_REG_CYPRESS_CFR3V;
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + if (ret)
> + return ret;
> + reg_cr3v_val = nor->bouncebuf[0];
> +
> + /* Read CR1V value from Configuration Register 1 Volatile */
> + op.addr.val = SPINOR_REG_CYPRESS_CFR1V;
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + if (ret)
> + return ret;
> + reg_cr1v_val = nor->bouncebuf[0];
> +
> + /* Determine the map ID based on CR3V[3] and CR1V[2] values */
> + if (!(reg_cr3v_val & BIT(3)) && !(reg_cr1v_val & BIT(2)))
Would be a good idea to give proper names to these bits in all the if
statements, instead of hard-coding them as BIT(x). For example, we do:
#define SPINOR_REG_CYPRESS_CFR3_PGSZ BIT(4) /* Page size. */
Instead of using BIT(4) directly.
Also would be nice to comment why these combinations select the map IDs
they select.
> + return 1; /* CR3V[3] = 0, CR1V[2] = 0, map id = 1 */
> +
> + if (!(reg_cr3v_val & BIT(3)) && (reg_cr1v_val & BIT(2)))
> + return 3; /* CR3V[3] = 0, CR1V[2] = 1, map id = 3 */
> +
> + if ((reg_cr3v_val & BIT(3)) && !(reg_cr1v_val & BIT(2)))
> + return 5; /* CR3V[3] = 1, CR1V[2] = 0, map id = 5 */
You say in the commit message that:
> The fixup also checks for invalid combinations of CR3V and CR1V
> values.This fixup is necessary to workaround an issue with the SFDP
> Address Map for S25FS512S flash.
I see no checks for invalid combinations. Also, what is the "issue" you
mention? Can you please elaborate?
> +
> + return 0;
The return value is used to set map_id for callers. Did you just return
0 as the "default success value" or do you really want to use map 0 if
we get here? If so, your function never uses map 2. Why is that? Can
that configuration never happen?
> +}
> +
> static const struct spi_nor_fixups s25fs_s_nor_fixups = {
> .post_bfpt = s25fs_s_nor_post_bfpt_fixups,
> + .post_get_map_id = s25fs_s_nor_post_get_map_id,
> };
>
> static const struct flash_info spansion_nor_parts[] = {
--
Regards,
Pratyush Yadav
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] mtd: spi-nor: Add support for spansion s25fs512s1
2024-11-26 18:58 ` [PATCH 2/3] mtd: spi-nor: Add support for spansion s25fs512s1 Vishwaroop A
@ 2024-12-06 15:08 ` Pratyush Yadav
0 siblings, 0 replies; 6+ messages in thread
From: Pratyush Yadav @ 2024-12-06 15:08 UTC (permalink / raw)
To: Vishwaroop A
Cc: tudor.ambarus, pratyush, mwalle, cmiquel.raynal, thierry.reding,
richard, vigneshr, linux-mtd, linux-kernel, jonathanh,
kyarlagadda, smangipudi
On Tue, Nov 26 2024, Vishwaroop A wrote:
> Add support for the spansion s25fs512s1 SPI NOR flash. This device has
> a 64MB size (SZ_64M), dual/quad read capabilities and apply
> s25fs_s_nor_fixups to handle specific chip behavior.
>
> Erasing, reading and writing this flash device has been validated on
> the Jetson AGX Orin platform using mtd_debug and dd utilities.
Please read https://docs.kernel.org/driver-api/mtd/spi-nor.html for
requirements for adding new flashes. Also, see if you even need a new
entry at all, or the generic SFDP-based driver works for you already. If
not, explain in your patch why.
Same applies to patch 3/3.
--
Regards,
Pratyush Yadav
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-06 15:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-26 18:58 [PATCH 0/3] Add support for S25FS512S1, and MX25U51279G Vishwaroop A
2024-11-26 18:58 ` [PATCH 1/3] mtd: spi-nor: Add post-get-map-id fixup for S25FS512S/S1 Vishwaroop A
2024-12-06 15:05 ` Pratyush Yadav
2024-11-26 18:58 ` [PATCH 2/3] mtd: spi-nor: Add support for spansion s25fs512s1 Vishwaroop A
2024-12-06 15:08 ` Pratyush Yadav
2024-11-26 18:58 ` [PATCH 3/3] mtd: spi-nor: Add support for mx25u51279g Vishwaroop A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox