* [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
@ 2020-11-16 15:39 yaliang.wang
2021-01-23 17:45 ` Tudor.Ambarus
0 siblings, 1 reply; 11+ messages in thread
From: yaliang.wang @ 2020-11-16 15:39 UTC (permalink / raw)
To: tudor.ambarus, miquel.raynal, richard, vigneshr; +Cc: Yaliang Wang, linux-mtd
From: Yaliang Wang <Yaliang.Wang@windriver.com>
S25FL{064|128|256}L chips can't recover from errors, when there are
program error or erase error, P_ERR or E_ERR bit will set to one, WIP
bit will remain set to one, A Clear Status Register command must be sent
to return the device to STANDBY state.
The error first recorded in commit <c4b3eacc1dfe> ("mtd: spi-nor: Recover
from Spansion/Cypress errors"). Whlie FL-L chips shifted P_ERR or E_ERR
bits to Status Register 2, which causing the current recover process
doesn't work any more after enabling using CLSR.
Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
---
drivers/mtd/spi-nor/core.c | 96 +++++++++++++++++++++++++++++++++-
drivers/mtd/spi-nor/spansion.c | 6 +--
include/linux/mtd/spi-nor.h | 1 +
3 files changed, 99 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f0ae7a01703a..3aa1484deb85 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -537,6 +537,88 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
dev_dbg(nor->dev, "error %d clearing SR\n", ret);
}
+static const struct flash_info *spi_nor_read_id(struct spi_nor *nor);
+
+/**
+ * spi_nor_s25fl_l_read_sr2() - Read the Status Register 2 using the
+ * SPINOR_OP_FL_L_RDSR2 (07h) command.
+ * @nor: pointer to 'struct spi_nor'.
+ * @sr2: pointer to DMA-able buffer where the value of the
+ * Status Register 2 will be written.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_s25fl_l_read_sr2(struct spi_nor *nor, u8 *sr2)
+{
+ int ret;
+
+ if (nor->spimem) {
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_FL_L_RDSR2, 1),
+ SPI_MEM_OP_NO_ADDR,
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_DATA_IN(1, sr2, 1));
+
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ } else {
+ ret = nor->controller_ops->read_reg(nor, SPINOR_OP_FL_L_RDSR2,
+ sr2, 1);
+ }
+
+ if (ret)
+ dev_dbg(nor->dev, "error %d reading SR2\n", ret);
+
+ return ret;
+}
+
+/*
+ * spi_nor_s25fl_l_sr_ready() - Query the Status Register to see if the flash
+ * is ready for new commands. Used by Cypress FL-L series chips.
+ * @nor: pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
+{
+ u8 *sr = nor->bouncebuf;
+ int ret;
+
+ ret = spi_nor_read_sr(nor, sr);
+ if (ret)
+ return ret;
+
+ /**
+ * P_ERR and E_ERR bits are located in the Status Register 2
+ * of Cypress FL-L series chips.
+ */
+ ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
+ if (ret)
+ return ret;
+
+ if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
+ if (sr[1] & SR_E_ERR)
+ dev_err(nor->dev, "Erase Error occurred\n");
+ else
+ dev_err(nor->dev, "Programming Error occurred\n");
+
+ spi_nor_clear_sr(nor);
+
+ /*
+ * WEL bit remains set to one when an erase or page program
+ * error occurs. Issue a Write Disable command to protect
+ * against inadvertent writes that can possibly corrupt the
+ * contents of the memory.
+ */
+ ret = spi_nor_write_disable(nor);
+ if (ret)
+ return ret;
+
+ return -EIO;
+ }
+
+ return !(sr[0] & SR_WIP);
+}
+
/**
* spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
* for new commands.
@@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
*/
static int spi_nor_sr_ready(struct spi_nor *nor)
{
- int ret = spi_nor_read_sr(nor, nor->bouncebuf);
+ int ret;
+ const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
+
+ if (IS_ERR_OR_NULL(tmpinfo))
+ return -ENOENT;
+
+ if (!strcmp(tmpinfo->name, "s25fl064l")
+ || !strcmp(tmpinfo->name, "s25fl128l")
+ || !strcmp(tmpinfo->name, "s25fl256l")) {
+ return spi_nor_s25fl_l_sr_ready(nor);
+ }
+
+ ret = spi_nor_read_sr(nor, nor->bouncebuf);
if (ret)
return ret;
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 8429b4af999a..e0ccef793e12 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -95,13 +95,13 @@ static const struct flash_info spansion_parts[] = {
SECT_4K | SPI_NOR_DUAL_READ) },
{ "s25fl064l", INFO(0x016017, 0, 64 * 1024, 128,
SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
- SPI_NOR_4B_OPCODES) },
+ SPI_NOR_4B_OPCODES | USE_CLSR) },
{ "s25fl128l", INFO(0x016018, 0, 64 * 1024, 256,
SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
- SPI_NOR_4B_OPCODES) },
+ SPI_NOR_4B_OPCODES | USE_CLSR) },
{ "s25fl256l", INFO(0x016019, 0, 64 * 1024, 512,
SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
- SPI_NOR_4B_OPCODES) },
+ SPI_NOR_4B_OPCODES | USE_CLSR) },
{ "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
SPI_NOR_NO_ERASE) },
};
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 60bac2c0ec45..a802f8f9e1e5 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -99,6 +99,7 @@
/* Used for Spansion flashes only. */
#define SPINOR_OP_BRWR 0x17 /* Bank register write */
#define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */
+#define SPINOR_OP_FL_L_RDSR2 0x07 /* FL-L chips Read status register 2 */
/* Used for Micron flashes only. */
#define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */
--
2.25.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2020-11-16 15:39 [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips yaliang.wang
@ 2021-01-23 17:45 ` Tudor.Ambarus
2021-01-24 6:12 ` Vignesh Raghavendra
0 siblings, 1 reply; 11+ messages in thread
From: Tudor.Ambarus @ 2021-01-23 17:45 UTC (permalink / raw)
To: yaliang.wang, miquel.raynal, richard, vigneshr; +Cc: linux-mtd
Hi, Yaliang,
This is really useful, but we'll need it in a different form.
See below.
On 11/16/20 5:39 PM, yaliang.wang@windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang@windriver.com>
>
> S25FL{064|128|256}L chips can't recover from errors, when there are
> program error or erase error, P_ERR or E_ERR bit will set to one, WIP
> bit will remain set to one, A Clear Status Register command must be sent
> to return the device to STANDBY state.
>
> The error first recorded in commit <c4b3eacc1dfe> ("mtd: spi-nor: Recover
> from Spansion/Cypress errors"). Whlie FL-L chips shifted P_ERR or E_ERR
> bits to Status Register 2, which causing the current recover process
> doesn't work any more after enabling using CLSR.
>
> Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
> ---
> drivers/mtd/spi-nor/core.c | 96 +++++++++++++++++++++++++++++++++-
> drivers/mtd/spi-nor/spansion.c | 6 +--
> include/linux/mtd/spi-nor.h | 1 +
> 3 files changed, 99 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f0ae7a01703a..3aa1484deb85 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -537,6 +537,88 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
> dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> }
>
> +static const struct flash_info *spi_nor_read_id(struct spi_nor *nor);
> +
> +/**
> + * spi_nor_s25fl_l_read_sr2() - Read the Status Register 2 using the
> + * SPINOR_OP_FL_L_RDSR2 (07h) command.
> + * @nor: pointer to 'struct spi_nor'.
> + * @sr2: pointer to DMA-able buffer where the value of the
> + * Status Register 2 will be written.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_s25fl_l_read_sr2(struct spi_nor *nor, u8 *sr2)
> +{
> + int ret;
> +
> + if (nor->spimem) {
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_FL_L_RDSR2, 1),
We really want manufacturer specific code out of the SPI NOR core.
Can you please check other manufacturer datasheets and verify if
others are using RDSR2 (07h) opcode?
> + SPI_MEM_OP_NO_ADDR,
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_IN(1, sr2, 1));
> +
> + ret = spi_mem_exec_op(nor->spimem, &op);
> + } else {
> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_FL_L_RDSR2,
> + sr2, 1);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d reading SR2\n", ret);
> +
> + return ret;
> +}
> +
> +/*
> + * spi_nor_s25fl_l_sr_ready() - Query the Status Register to see if the flash
> + * is ready for new commands. Used by Cypress FL-L series chips.
> + * @nor: pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
> +{
> + u8 *sr = nor->bouncebuf;
> + int ret;
> +
> + ret = spi_nor_read_sr(nor, sr);
> + if (ret)
> + return ret;
> +
> + /**
> + * P_ERR and E_ERR bits are located in the Status Register 2
> + * of Cypress FL-L series chips.
> + */
> + ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
> + if (ret)
> + return ret;
> +
> + if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
If checking other manufacturer datasheets, would you please check if
CLSR is used by any other manufacturer?
> + if (sr[1] & SR_E_ERR)
> + dev_err(nor->dev, "Erase Error occurred\n");
> + else
> + dev_err(nor->dev, "Programming Error occurred\n");
> +
> + spi_nor_clear_sr(nor);
> +
> + /*
> + * WEL bit remains set to one when an erase or page program
> + * error occurs. Issue a Write Disable command to protect
> + * against inadvertent writes that can possibly corrupt the
> + * contents of the memory.
> + */
> + ret = spi_nor_write_disable(nor);
> + if (ret)
> + return ret;
> +
> + return -EIO;
> + }
> +
> + return !(sr[0] & SR_WIP);
> +}
> +
> /**
> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
> * for new commands.
> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
> */
> static int spi_nor_sr_ready(struct spi_nor *nor)
> {
> - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
> + int ret;
> + const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
> +
> + if (IS_ERR_OR_NULL(tmpinfo))
> + return -ENOENT;
> +
> + if (!strcmp(tmpinfo->name, "s25fl064l")
> + || !strcmp(tmpinfo->name, "s25fl128l")
> + || !strcmp(tmpinfo->name, "s25fl256l")) {
> + return spi_nor_s25fl_l_sr_ready(nor);
> + }
No, we can't accept flash name comparisons in the SPI NOR core.
If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
function pointer that can be filled by spansion flashes. Or some other
method depending on the CLSR and RDSR2 exposure through other
manufacturers.
Ideally one would skim through at least 2 - 3 datasheets from each
manufacturer available in SPI NOR. Preferable each datasheet from
a different manufacturer family. Unfortunately I'm not aware of any
standard that describes all the supported SPI NOR commands.
If you find this overwhelming, I can share the workload with you,
but at best effort. If you go through this by yourself, please
save the name of the datasheet flashes that you go through.
Cheers,
ta
> +
> + ret = spi_nor_read_sr(nor, nor->bouncebuf);
>
> if (ret)
> return ret;
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 8429b4af999a..e0ccef793e12 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -95,13 +95,13 @@ static const struct flash_info spansion_parts[] = {
> SECT_4K | SPI_NOR_DUAL_READ) },
> { "s25fl064l", INFO(0x016017, 0, 64 * 1024, 128,
> SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> - SPI_NOR_4B_OPCODES) },
> + SPI_NOR_4B_OPCODES | USE_CLSR) },
> { "s25fl128l", INFO(0x016018, 0, 64 * 1024, 256,
> SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> - SPI_NOR_4B_OPCODES) },
> + SPI_NOR_4B_OPCODES | USE_CLSR) },
> { "s25fl256l", INFO(0x016019, 0, 64 * 1024, 512,
> SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> - SPI_NOR_4B_OPCODES) },
> + SPI_NOR_4B_OPCODES | USE_CLSR) },
> { "cy15x104q", INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
> SPI_NOR_NO_ERASE) },
> };
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 60bac2c0ec45..a802f8f9e1e5 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -99,6 +99,7 @@
> /* Used for Spansion flashes only. */
> #define SPINOR_OP_BRWR 0x17 /* Bank register write */
> #define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */
> +#define SPINOR_OP_FL_L_RDSR2 0x07 /* FL-L chips Read status register 2 */
>
> /* Used for Micron flashes only. */
> #define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-01-23 17:45 ` Tudor.Ambarus
@ 2021-01-24 6:12 ` Vignesh Raghavendra
2021-01-26 2:06 ` Wang, Yaliang
0 siblings, 1 reply; 11+ messages in thread
From: Vignesh Raghavendra @ 2021-01-24 6:12 UTC (permalink / raw)
To: Tudor.Ambarus, yaliang.wang, miquel.raynal, richard; +Cc: linux-mtd
Hi, Yaliang
On 1/23/21 11:15 PM, Tudor.Ambarus@microchip.com wrote:
> Hi, Yaliang,
>
[...]
>> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
>> +{
>> + u8 *sr = nor->bouncebuf;
>> + int ret;
>> +
>> + ret = spi_nor_read_sr(nor, sr);
>> + if (ret)
>> + return ret;
>> +
>> + /**
>> + * P_ERR and E_ERR bits are located in the Status Register 2
>> + * of Cypress FL-L series chips.
>> + */
>> + ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
>> + if (ret)
>> + return ret;
>> +
>> + if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
> If checking other manufacturer datasheets, would you please check if
> CLSR is used by any other manufacturer?
>
This is limited to certain subsets of Cypress/Spansion flashes.
>> + if (sr[1] & SR_E_ERR)
>> + dev_err(nor->dev, "Erase Error occurred\n");
>> + else
>> + dev_err(nor->dev, "Programming Error occurred\n");
>> +
>> + spi_nor_clear_sr(nor);
>> +
>> + /*
>> + * WEL bit remains set to one when an erase or page program
>> + * error occurs. Issue a Write Disable command to protect
>> + * against inadvertent writes that can possibly corrupt the
>> + * contents of the memory.
>> + */
>> + ret = spi_nor_write_disable(nor);
>> + if (ret)
>> + return ret;
>> +
>> + return -EIO;
>> + }
>> +
>> + return !(sr[0] & SR_WIP);
>> +}
>> +
>> /**
>> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>> * for new commands.
>> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>> */
>> static int spi_nor_sr_ready(struct spi_nor *nor)
>> {
>> - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> + int ret;
>> + const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
>> +
>> + if (IS_ERR_OR_NULL(tmpinfo))
>> + return -ENOENT;
>> +
>> + if (!strcmp(tmpinfo->name, "s25fl064l")
>> + || !strcmp(tmpinfo->name, "s25fl128l")
>> + || !strcmp(tmpinfo->name, "s25fl256l")) {
>> + return spi_nor_s25fl_l_sr_ready(nor);
>> + }
>
> No, we can't accept flash name comparisons in the SPI NOR core.
> If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
> function pointer that can be filled by spansion flashes. Or some other
> method depending on the CLSR and RDSR2 exposure through other
> manufacturers.
>
> Ideally one would skim through at least 2 - 3 datasheets from each
> manufacturer available in SPI NOR. Preferable each datasheet from
> a different manufacturer family. Unfortunately I'm not aware of any
> standard that describes all the supported SPI NOR commands.
> If you find this overwhelming, I can share the workload with you,
> but at best effort. If you go through this by yourself, please
> save the name of the datasheet flashes that you go through.
>
Agree with Tudor. There is quite a bit of variation even within
Cypress/Spansion devices. S28 series of flashes have these error bits
within SR1 register. See: https://www.cypress.com/file/513996/download
So, this cannot be in SPI NOR core.
spi_nor_xread_sr(), spi_nor_fsr_ready() and spi_nor_clear_sr() are all
vendor/family specific and should be moved to appropriate vendor
specific drivers.
Would highly appreciate it, if you could fix these up as part of your
current work. Thanks!
Regards
Vignesh
[...]
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-01-24 6:12 ` Vignesh Raghavendra
@ 2021-01-26 2:06 ` Wang, Yaliang
2021-01-26 8:51 ` Tudor.Ambarus
0 siblings, 1 reply; 11+ messages in thread
From: Wang, Yaliang @ 2021-01-26 2:06 UTC (permalink / raw)
To: Vignesh Raghavendra, Tudor.Ambarus, miquel.raynal, richard; +Cc: linux-mtd
Hi, Vignesh and Tudor
It's really inspiring receiving from you, and
On 1/24/2021 2:12 PM, Vignesh Raghavendra wrote:
> Hi, Yaliang
>
> On 1/23/21 11:15 PM, Tudor.Ambarus@microchip.com wrote:
>> Hi, Yaliang,
>>
> [...]
>>> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
>>> +{
>>> + u8 *sr = nor->bouncebuf;
>>> + int ret;
>>> +
>>> + ret = spi_nor_read_sr(nor, sr);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /**
>>> + * P_ERR and E_ERR bits are located in the Status Register 2
>>> + * of Cypress FL-L series chips.
>>> + */
>>> + ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
>> If checking other manufacturer datasheets, would you please check if
>> CLSR is used by any other manufacturer?
>>
>
> This is limited to certain subsets of Cypress/Spansion flashes.
>
>>> + if (sr[1] & SR_E_ERR)
>>> + dev_err(nor->dev, "Erase Error occurred\n");
>>> + else
>>> + dev_err(nor->dev, "Programming Error occurred\n");
>>> +
>>> + spi_nor_clear_sr(nor);
>>> +
>>> + /*
>>> + * WEL bit remains set to one when an erase or page program
>>> + * error occurs. Issue a Write Disable command to protect
>>> + * against inadvertent writes that can possibly corrupt the
>>> + * contents of the memory.
>>> + */
>>> + ret = spi_nor_write_disable(nor);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return -EIO;
>>> + }
>>> +
>>> + return !(sr[0] & SR_WIP);
>>> +}
>>> +
>>> /**
>>> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>>> * for new commands.
>>> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>>> */
>>> static int spi_nor_sr_ready(struct spi_nor *nor)
>>> {
>>> - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>> + int ret;
>>> + const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
>>> +
>>> + if (IS_ERR_OR_NULL(tmpinfo))
>>> + return -ENOENT;
>>> +
>>> + if (!strcmp(tmpinfo->name, "s25fl064l")
>>> + || !strcmp(tmpinfo->name, "s25fl128l")
>>> + || !strcmp(tmpinfo->name, "s25fl256l")) {
>>> + return spi_nor_s25fl_l_sr_ready(nor);
>>> + }
>>
>> No, we can't accept flash name comparisons in the SPI NOR core.
>> If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
>> function pointer that can be filled by spansion flashes. Or some other
>> method depending on the CLSR and RDSR2 exposure through other
>> manufacturers.
>>
>> Ideally one would skim through at least 2 - 3 datasheets from each
>> manufacturer available in SPI NOR. Preferable each datasheet from
>> a different manufacturer family. Unfortunately I'm not aware of any
>> standard that describes all the supported SPI NOR commands.
>> If you find this overwhelming, I can share the workload with you,
>> but at best effort. If you go through this by yourself, please
>> save the name of the datasheet flashes that you go through.
>>
>
> Agree with Tudor. There is quite a bit of variation even within
> Cypress/Spansion devices. S28 series of flashes have these error bits
> within SR1 register. See: https://www.cypress.com/file/513996/download
>
> So, this cannot be in SPI NOR core.
>
> spi_nor_xread_sr(), spi_nor_fsr_ready() and spi_nor_clear_sr() are all
> vendor/family specific and should be moved to appropriate vendor
> specific drivers.
Have to admit the comparisons and the function here are inappropriate,
though it's the fastest way to fix this problem.
I'd like to move spansion specific operations out of the core.c first,
basing on sr_ready function pointer or other mechanisms mentioned by
Tudor, and after that I'll see if I'm capable to cope with the other
several vendor/family specific functions.
Regards
Yaliang
>
> Would highly appreciate it, if you could fix these up as part of your
> current work. Thanks!
>
> Regards
> Vignesh
>
> [...]
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-01-26 2:06 ` Wang, Yaliang
@ 2021-01-26 8:51 ` Tudor.Ambarus
2021-02-23 16:36 ` Yaliang.Wang
0 siblings, 1 reply; 11+ messages in thread
From: Tudor.Ambarus @ 2021-01-26 8:51 UTC (permalink / raw)
To: yaliang.wang, vigneshr, miquel.raynal, richard; +Cc: linux-mtd
Hi, Yaliang,
On 1/26/21 4:06 AM, Wang, Yaliang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi, Vignesh and Tudor
>
> It's really inspiring receiving from you, and
>
> On 1/24/2021 2:12 PM, Vignesh Raghavendra wrote:
>> Hi, Yaliang
>>
>> On 1/23/21 11:15 PM, Tudor.Ambarus@microchip.com wrote:
>>> Hi, Yaliang,
>>>
>> [...]
>>>> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
>>>> +{
>>>> + u8 *sr = nor->bouncebuf;
>>>> + int ret;
>>>> +
>>>> + ret = spi_nor_read_sr(nor, sr);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + /**
>>>> + * P_ERR and E_ERR bits are located in the Status Register 2
>>>> + * of Cypress FL-L series chips.
>>>> + */
>>>> + ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
>>> If checking other manufacturer datasheets, would you please check if
>>> CLSR is used by any other manufacturer?
>>>
>>
>> This is limited to certain subsets of Cypress/Spansion flashes.
>>
>>>> + if (sr[1] & SR_E_ERR)
>>>> + dev_err(nor->dev, "Erase Error occurred\n");
>>>> + else
>>>> + dev_err(nor->dev, "Programming Error occurred\n");
>>>> +
>>>> + spi_nor_clear_sr(nor);
>>>> +
>>>> + /*
>>>> + * WEL bit remains set to one when an erase or page program
>>>> + * error occurs. Issue a Write Disable command to protect
>>>> + * against inadvertent writes that can possibly corrupt the
>>>> + * contents of the memory.
>>>> + */
>>>> + ret = spi_nor_write_disable(nor);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return -EIO;
>>>> + }
>>>> +
>>>> + return !(sr[0] & SR_WIP);
>>>> +}
>>>> +
>>>> /**
>>>> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>>>> * for new commands.
>>>> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>>>> */
>>>> static int spi_nor_sr_ready(struct spi_nor *nor)
>>>> {
>>>> - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>>> + int ret;
>>>> + const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
>>>> +
>>>> + if (IS_ERR_OR_NULL(tmpinfo))
>>>> + return -ENOENT;
>>>> +
>>>> + if (!strcmp(tmpinfo->name, "s25fl064l")
>>>> + || !strcmp(tmpinfo->name, "s25fl128l")
>>>> + || !strcmp(tmpinfo->name, "s25fl256l")) {
>>>> + return spi_nor_s25fl_l_sr_ready(nor);
>>>> + }
>>>
>>> No, we can't accept flash name comparisons in the SPI NOR core.
>>> If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
>>> function pointer that can be filled by spansion flashes. Or some other
>>> method depending on the CLSR and RDSR2 exposure through other
>>> manufacturers.
>>>
>>> Ideally one would skim through at least 2 - 3 datasheets from each
>>> manufacturer available in SPI NOR. Preferable each datasheet from
>>> a different manufacturer family. Unfortunately I'm not aware of any
>>> standard that describes all the supported SPI NOR commands.
>>> If you find this overwhelming, I can share the workload with you,
>>> but at best effort. If you go through this by yourself, please
>>> save the name of the datasheet flashes that you go through.
>>>
>>
>> Agree with Tudor. There is quite a bit of variation even within
>> Cypress/Spansion devices. S28 series of flashes have these error bits
>> within SR1 register. See: https://www.cypress.com/file/513996/download
>>
>> So, this cannot be in SPI NOR core.
>>
>> spi_nor_xread_sr(), spi_nor_fsr_ready() and spi_nor_clear_sr() are all
>> vendor/family specific and should be moved to appropriate vendor
>> specific drivers.
>
> Have to admit the comparisons and the function here are inappropriate,
> though it's the fastest way to fix this problem.
>
> I'd like to move spansion specific operations out of the core.c first,
> basing on sr_ready function pointer or other mechanisms mentioned by
> Tudor, and after that I'll see if I'm capable to cope with the other
> several vendor/family specific functions.
>
The difficulty is not in moving the code in spansion.c, but rather
the documentation effort, to skim through all the datasheets. So if
you're going through all the datasheets to check whether RDSR2 is used
by other vendor or not, please also check the other ops that me and
Vignesh indicated, so that we don't duplicate the effort.
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-01-26 8:51 ` Tudor.Ambarus
@ 2021-02-23 16:36 ` Yaliang.Wang
2021-02-23 17:31 ` Tudor.Ambarus
0 siblings, 1 reply; 11+ messages in thread
From: Yaliang.Wang @ 2021-02-23 16:36 UTC (permalink / raw)
To: Tudor.Ambarus, vigneshr, miquel.raynal, richard; +Cc: linux-mtd
Hi, Tudor
So sorry to reply so late, now I'm committed to solving this problem.
After checking over sixty datasheets and almost at least one datasheet
for each family of the manufacturers, I can finally deviler some
conclusions.
1. RDSR2(07H) instruction is just used by Spansion manufacturer.
As for all other manufacturers, most common instructions for reading SR2
is "35H", and "15H" is for reading SR3. Some manufactures have specific
registers that are similar to status register, such as "Information
Register", "Suspend Status Register", "Extended Read Register",
"Security Register", "Flag Status Register" and also have specific
instructions for reading them, such as"2BH/09H/81H/2BH/70H".
2. RDSR2(07H) instruction is not just existed in Spansion "S25FL-L"
family, also existed in Spansion"S79FL-S, S70FS-S, S25FL-S, S25FS-S,
CYRS, CY15B/V-SN" families. We just haven't use it before, it has been
there for a long time.
3. Many manufacturers are using CLSR(30H) instruction, but the functions
are different.
The most common function of "30H" instruction is to resume the
suspended bits, but in many Spansion's products , the function is to
clear P_ERR and E_ERR bits. In some other Spansion's products(such as
S25FS-S), Spansion provided a configure register, which can make "30H"
instruction execute the alternative resuming suspended bits function
like other manufacturers.
4. P_ERR and E_ERR bits are existed in many manufacturers, reside in
specific register, such as "Information Register""Flag Status Register"
and so on, and the instructions for reading those register and clearing
the ERR bits varies according to the different manufacturer.
Based on the facts above, and to resolve the original "sr_ready" problem
within "S25FL-L" family of Spansion, I think the solution is not much
different. We need a "sr_ready" function pointer and maybe a new flag to
mark the twisted status register. Actually I have made a rough fix
based on that(not include fixing spi_nor_xread_sr and
spi_nor_fsr_ready). Would like to learn your suggestions before going
further.
The checked datasheets and related marks are too many, it may not be
appropriate to post them here, but I'd like to share, once needed.
Best Regards,
Yaliang
On 1/26/21 4:51 PM, Tudor.Ambarus@microchip.com wrote:
> Hi, Yaliang,
>
> On 1/26/21 4:06 AM, Wang, Yaliang wrote:
>> or xxh(EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi, Vignesh and Tudor
>>
>> It's really inspiring receiving from you, and
>>
>> On 1/24/2021 2:12 PM, Vignesh Raghavendra wrote:
>>> Hi, Yaliang
>>>
>>> On 1/23/21 11:15 PM, Tudor.Ambarus@microchip.com wrote:
>>>> Hi, Yaliang,
>>>>
>>> [...]
>>>>> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
>>>>> +{
>>>>> + u8 *sr = nor->bouncebuf;
>>>>> + int ret;
>>>>> +
>>>>> + ret = spi_nor_read_sr(nor, sr);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + /**
>>>>> + * P_ERR and E_ERR bits are located in the Status Register 2
>>>>> + * of Cypress FL-L series chips.
>>>>> + */
>>>>> + ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
>>>> If checking other manufacturer datasheets, would you please check if
>>>> CLSR is used by any other manufacturer?
>>>>
>>> This is limited to certain subsets of Cypress/Spansion flashes.
>>>
>>>>> + if (sr[1] & SR_E_ERR)
>>>>> + dev_err(nor->dev, "Erase Error occurred\n");
>>>>> + else
>>>>> + dev_err(nor->dev, "Programming Error occurred\n");
>>>>> +
>>>>> + spi_nor_clear_sr(nor);
>>>>> +
>>>>> + /*
>>>>> + * WEL bit remains set to one when an erase or page program
>>>>> + * error occurs. Issue a Write Disable command to protect
>>>>> + * against inadvertent writes that can possibly corrupt the
>>>>> + * contents of the memory.
>>>>> + */
>>>>> + ret = spi_nor_write_disable(nor);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + return -EIO;
>>>>> + }
>>>>> +
>>>>> + return !(sr[0] & SR_WIP);
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>>>>> * for new commands.
>>>>> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>>>>> */
>>>>> static int spi_nor_sr_ready(struct spi_nor *nor)
>>>>> {
>>>>> - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>>>> + int ret;
>>>>> + const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
>>>>> +
>>>>> + if (IS_ERR_OR_NULL(tmpinfo))
>>>>> + return -ENOENT;
>>>>> +
>>>>> + if (!strcmp(tmpinfo->name, "s25fl064l")
>>>>> + || !strcmp(tmpinfo->name, "s25fl128l")
>>>>> + || !strcmp(tmpinfo->name, "s25fl256l")) {
>>>>> + return spi_nor_s25fl_l_sr_ready(nor);
>>>>> + }
>>>> No, we can't accept flash name comparisons in the SPI NOR core.
>>>> If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
>>>> function pointer that can be filled by spansion flashes. Or some other
>>>> method depending on the CLSR and RDSR2 exposure through other
>>>> manufacturers.
>>>>
>>>> Ideally one would skim through at least 2 - 3 datasheets from each
>>>> manufacturer available in SPI NOR. Preferable each datasheet from
>>>> a different manufacturer family. Unfortunately I'm not aware of any
>>>> standard that describes all the supported SPI NOR commands.
>>>> If you find this overwhelming, I can share the workload with you,
>>>> but at best effort. If you go through this by yourself, please
>>>> save the name of the datasheet flashes that you go through.
>>>>
>>> Agree with Tudor. There is quite a bit of variation even within
>>> Cypress/Spansion devices. S28 series of flashes have these error bits
>>> within SR1 register. See: https://www.cypress.com/file/513996/download
>>>
>>> So, this cannot be in SPI NOR core.
>>>
>>> spi_nor_xread_sr(), spi_nor_fsr_ready() and spi_nor_clear_sr() are all
>>> vendor/family specific and should be moved to appropriate vendor
>>> specific drivers.
>> Have to admit the comparisons and the function here are inappropriate,
>> though it's the fastest way to fix this problem.
>>
>> I'd like to move spansion specific operations out of the core.c first,
>> basing on sr_ready function pointer or other mechanisms mentioned by
>> Tudor, and after that I'll see if I'm capable to cope with the other
>> several vendor/family specific functions.
>>
> The difficulty is not in moving the code in spansion.c, but rather
> the documentation effort, to skim through all the datasheets. So if
> you're going through all the datasheets to check whether RDSR2 is used
> by other vendor or not, please also check the other ops that me and
> Vignesh indicated, so that we don't duplicate the effort.
>
> Cheers,
> ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-02-23 16:36 ` Yaliang.Wang
@ 2021-02-23 17:31 ` Tudor.Ambarus
2021-02-24 16:24 ` Yaliang.Wang
0 siblings, 1 reply; 11+ messages in thread
From: Tudor.Ambarus @ 2021-02-23 17:31 UTC (permalink / raw)
To: Yaliang.Wang, vigneshr, miquel.raynal, richard; +Cc: linux-mtd
On 2/23/21 6:36 PM, Yaliang.Wang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi, Tudor
Hi, Yaliang,
>
> So sorry to reply so late, now I'm committed to solving this problem.
> After checking over sixty datasheets and almost at least one datasheet
> for each family of the manufacturers, I can finally deviler some
> conclusions.
Great! Thanks for doing this!
>
> 1. RDSR2(07H) instruction is just used by Spansion manufacturer.
Then we can define it as
#define SPINOR_OP_SPANSION_SR2 0x07h
and define its related static function in spansion.c.
> As for all other manufacturers, most common instructions for reading SR2
> is "35H", and "15H" is for reading SR3. Some manufactures have specific
> registers that are similar to status register, such as "Information
> Register", "Suspend Status Register", "Extended Read Register",
> "Security Register", "Flag Status Register" and also have specific
> instructions for reading them, such as"2BH/09H/81H/2BH/70H".
Are these common to more manufacturers? If yes, it will help us to inform
us to which manufacturers all these ops are common, it will spare us to
skim through datasheets later on.
>
> 2. RDSR2(07H) instruction is not just existed in Spansion "S25FL-L"
> family, also existed in Spansion"S79FL-S, S70FS-S, S25FL-S, S25FS-S,
> CYRS, CY15B/V-SN" families. We just haven't use it before, it has been
> there for a long time.
Cool, it can reside in spansion.c then.
>
> 3. Many manufacturers are using CLSR(30H) instruction, but the functions
> are different.
> The most common function of "30H" instruction is to resume the
> suspended bits, but in many Spansion's products , the function is to
> clear P_ERR and E_ERR bits. In some other Spansion's products(such as
> S25FS-S), Spansion provided a configure register, which can make "30H"
> instruction execute the alternative resuming suspended bits function
> like other manufacturers.
Which other manufacturers? It will help us later on.
We can define the op that clears the P_ERR and E_ERR bits in spansion.c:
#define SPINOR_OP_SPANSION_CLSR 0x30h
with static function in spansion.c
The op that is used at resume, can be latter added in the SPI NOR core.
>
> 4. P_ERR and E_ERR bits are existed in many manufacturers, reside in
> specific register, such as "Information Register""Flag Status Register"
> and so on, and the instructions for reading those register and clearing
> the ERR bits varies according to the different manufacturer.
Again, if this is fresh in your mind, it would help to say which manufacturer
and what instructions.
>
> Based on the facts above, and to resolve the original "sr_ready" problem
> within "S25FL-L" family of Spansion, I think the solution is not much
> different. We need a "sr_ready" function pointer and maybe a new flag to
> mark the twisted status register. Actually I have made a rough fix
sr_ready function pointer should be enough. The default value for it will
be initialized in the SPI NOR core, and for your use-case, you can update
it in your manufacturer driver.
> based on that(not include fixing spi_nor_xread_sr and
> spi_nor_fsr_ready). Would like to learn your suggestions before going
> further.
we should be good. Try a small patch and air it.
>
> The checked datasheets and related marks are too many, it may not be
> appropriate to post them here, but I'd like to share, once needed.
Would help. Great work, thanks!
ta
>
> Best Regards,
> Yaliang
>
> On 1/26/21 4:51 PM, Tudor.Ambarus@microchip.com wrote:
>> Hi, Yaliang,
>>
>> On 1/26/21 4:06 AM, Wang, Yaliang wrote:
>>> or xxh(EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi, Vignesh and Tudor
>>>
>>> It's really inspiring receiving from you, and
>>>
>>> On 1/24/2021 2:12 PM, Vignesh Raghavendra wrote:
>>>> Hi, Yaliang
>>>>
>>>> On 1/23/21 11:15 PM, Tudor.Ambarus@microchip.com wrote:
>>>>> Hi, Yaliang,
>>>>>
>>>> [...]
>>>>>> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
>>>>>> +{
>>>>>> + u8 *sr = nor->bouncebuf;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = spi_nor_read_sr(nor, sr);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + /**
>>>>>> + * P_ERR and E_ERR bits are located in the Status Register 2
>>>>>> + * of Cypress FL-L series chips.
>>>>>> + */
>>>>>> + ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
>>>>> If checking other manufacturer datasheets, would you please check if
>>>>> CLSR is used by any other manufacturer?
>>>>>
>>>> This is limited to certain subsets of Cypress/Spansion flashes.
>>>>
>>>>>> + if (sr[1] & SR_E_ERR)
>>>>>> + dev_err(nor->dev, "Erase Error occurred\n");
>>>>>> + else
>>>>>> + dev_err(nor->dev, "Programming Error occurred\n");
>>>>>> +
>>>>>> + spi_nor_clear_sr(nor);
>>>>>> +
>>>>>> + /*
>>>>>> + * WEL bit remains set to one when an erase or page program
>>>>>> + * error occurs. Issue a Write Disable command to protect
>>>>>> + * against inadvertent writes that can possibly corrupt the
>>>>>> + * contents of the memory.
>>>>>> + */
>>>>>> + ret = spi_nor_write_disable(nor);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + return -EIO;
>>>>>> + }
>>>>>> +
>>>>>> + return !(sr[0] & SR_WIP);
>>>>>> +}
>>>>>> +
>>>>>> /**
>>>>>> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>>>>>> * for new commands.
>>>>>> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>>>>>> */
>>>>>> static int spi_nor_sr_ready(struct spi_nor *nor)
>>>>>> {
>>>>>> - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>>>>> + int ret;
>>>>>> + const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
>>>>>> +
>>>>>> + if (IS_ERR_OR_NULL(tmpinfo))
>>>>>> + return -ENOENT;
>>>>>> +
>>>>>> + if (!strcmp(tmpinfo->name, "s25fl064l")
>>>>>> + || !strcmp(tmpinfo->name, "s25fl128l")
>>>>>> + || !strcmp(tmpinfo->name, "s25fl256l")) {
>>>>>> + return spi_nor_s25fl_l_sr_ready(nor);
>>>>>> + }
>>>>> No, we can't accept flash name comparisons in the SPI NOR core.
>>>>> If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
>>>>> function pointer that can be filled by spansion flashes. Or some other
>>>>> method depending on the CLSR and RDSR2 exposure through other
>>>>> manufacturers.
>>>>>
>>>>> Ideally one would skim through at least 2 - 3 datasheets from each
>>>>> manufacturer available in SPI NOR. Preferable each datasheet from
>>>>> a different manufacturer family. Unfortunately I'm not aware of any
>>>>> standard that describes all the supported SPI NOR commands.
>>>>> If you find this overwhelming, I can share the workload with you,
>>>>> but at best effort. If you go through this by yourself, please
>>>>> save the name of the datasheet flashes that you go through.
>>>>>
>>>> Agree with Tudor. There is quite a bit of variation even within
>>>> Cypress/Spansion devices. S28 series of flashes have these error bits
>>>> within SR1 register. See: https://www.cypress.com/file/513996/download
>>>>
>>>> So, this cannot be in SPI NOR core.
>>>>
>>>> spi_nor_xread_sr(), spi_nor_fsr_ready() and spi_nor_clear_sr() are all
>>>> vendor/family specific and should be moved to appropriate vendor
>>>> specific drivers.
>>> Have to admit the comparisons and the function here are inappropriate,
>>> though it's the fastest way to fix this problem.
>>>
>>> I'd like to move spansion specific operations out of the core.c first,
>>> basing on sr_ready function pointer or other mechanisms mentioned by
>>> Tudor, and after that I'll see if I'm capable to cope with the other
>>> several vendor/family specific functions.
>>>
>> The difficulty is not in moving the code in spansion.c, but rather
>> the documentation effort, to skim through all the datasheets. So if
>> you're going through all the datasheets to check whether RDSR2 is used
>> by other vendor or not, please also check the other ops that me and
>> Vignesh indicated, so that we don't duplicate the effort.
>>
>> Cheers,
>> ta
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-02-23 17:31 ` Tudor.Ambarus
@ 2021-02-24 16:24 ` Yaliang.Wang
2021-02-24 16:30 ` Tudor.Ambarus
0 siblings, 1 reply; 11+ messages in thread
From: Yaliang.Wang @ 2021-02-24 16:24 UTC (permalink / raw)
To: Tudor.Ambarus, vigneshr, miquel.raynal, richard; +Cc: linux-mtd
Hi, Tudor
On 2/24/21 1:31 AM, Tudor.Ambarus@microchip.com wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On 2/23/21 6:36 PM, Yaliang.Wang wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi, Tudor
> Hi, Yaliang,
>> So sorry to reply so late, now I'm committed to solving this problem.
>> After checking over sixty datasheets and almost at least one datasheet
>> for each family of the manufacturers, I can finally deviler some
>> conclusions.
> Great! Thanks for doing this!
>
>> 1. RDSR2(07H) instruction is just used by Spansion manufacturer.
> Then we can define it as
> #define SPINOR_OP_SPANSION_SR2 0x07h
>
> and define its related static function in spansion.c.
>
>> As for all other manufacturers, most common instructions for reading SR2
>> is "35H", and "15H" is for reading SR3. Some manufactures have specific
>> registers that are similar to status register, such as "Information
>> Register", "Suspend Status Register", "Extended Read Register",
>> "Security Register", "Flag Status Register" and also have specific
>> instructions for reading them, such as"2BH/09H/81H/2BH/70H".
> Are these common to more manufacturers? If yes, it will help us to inform
> us to which manufacturers all these ops are common, it will spare us to
> skim through datasheets later on.
If not considering the number of chips we supported in each manufacture
, "35H" and "15H" are more common instructions used among manufactures .
"35H" to read SR2 is used in "spansion", "atmel", "esmt", "gigadevice",
"winbond" manufactures.
"15H" to read SR3 is used in "gigadevice", "winbond" manufactures.
"33H" to read SR3 is used in "spansion" manufacture.
"09H" to read SR2 and "95H" to read SR3 are used in "xmc" manufacture.
As for SR-similar registers, they always used by manufactures acting as
a supplement of Status Register, but with a different name.
"eon" uses "2BH""09H" to read "Information Register" and "Suspend Status
Register"
"issi" uses "81H" to read "Extended Read Register"
"macronix" uses "2BH" to read "Security Register"
"micron" and "st" uses "70H" to read "Extended Read Register"
>> 2. RDSR2(07H) instruction is not just existed in Spansion "S25FL-L"
>> family, also existed in Spansion"S79FL-S, S70FS-S, S25FL-S, S25FS-S,
>> CYRS, CY15B/V-SN" families. We just haven't use it before, it has been
>> there for a long time.
> Cool, it can reside in spansion.c then.
>
>> 3. Many manufacturers are using CLSR(30H) instruction, but the functions
>> are different.
>> The most common function of "30H" instruction is to resume the
>> suspended bits, but in many Spansion's products , the function is to
>> clear P_ERR and E_ERR bits. In some other Spansion's products(such as
>> S25FS-S), Spansion provided a configure register, which can make "30H"
>> instruction execute the alternative resuming suspended bits function
>> like other manufacturers.
> Which other manufacturers? It will help us later on.
Spansion "S79FS-S""S70FS-S""S25FS-S" families provided the alternative
resuming function for "30H" instruction, it can be configured with
Configuration Register 3.
Spansion "S79FL-S","S25FL-S","S25FL-P","S25FL-L","CYRS16B" families use
"30H" instruction just to clear P_ERR AND E_ERR bits
Spansion "S25FL-K", "S25FL-A", "CY15B/V-SN" families even don't use
"30H" instruction.
>
> We can define the op that clears the P_ERR and E_ERR bits in spansion.c:
> #define SPINOR_OP_SPANSION_CLSR 0x30h
> with static function in spansion.c
>
> The op that is used at resume, can be latter added in the SPI NOR core.
>
>> 4. P_ERR and E_ERR bits are existed in many manufacturers, reside in
>> specific register, such as "Information Register""Flag Status Register"
>> and so on, and the instructions for reading those register and clearing
>> the ERR bits varies according to the different manufacturer.
> Again, if this is fresh in your mind, it would help to say which manufacturer
> and what instructions.
Check above comments.
>> Based on the facts above, and to resolve the original "sr_ready" problem
>> within "S25FL-L" family of Spansion, I think the solution is not much
>> different. We need a "sr_ready" function pointer and maybe a new flag to
>> mark the twisted status register. Actually I have made a rough fix
> sr_ready function pointer should be enough. The default value for it will
> be initialized in the SPI NOR core, and for your use-case, you can update
> it in your manufacturer driver.
>
>> based on that(not include fixing spi_nor_xread_sr and
>> spi_nor_fsr_ready). Would like to learn your suggestions before going
>> further.
> we should be good. Try a small patch and air it.
So, maybe I can continue the fixing work now ? Basing on the macro and
the strategy you suggested above.
Optimizing the original "sr_ready" function won't be too difficult I think.
Regards
Yaliang
>
>> The checked datasheets and related marks are too many, it may not be
>> appropriate to post them here, but I'd like to share, once needed.
> Would help. Great work, thanks!
>
> ta
>
>> Best Regards,
>> Yaliang
>>
>> On 1/26/21 4:51 PM, Tudor.Ambarus@microchip.com wrote:
>>> Hi, Yaliang,
>>>
>>> On 1/26/21 4:06 AM, Wang, Yaliang wrote:
>>>> or xxh(EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Hi, Vignesh and Tudor
>>>>
>>>> It's really inspiring receiving from you, and
>>>>
>>>> On 1/24/2021 2:12 PM, Vignesh Raghavendra wrote:
>>>>> Hi, Yaliang
>>>>>
>>>>> On 1/23/21 11:15 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>> Hi, Yaliang,
>>>>>>
>>>>> [...]
>>>>>>> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
>>>>>>> +{
>>>>>>> + u8 *sr = nor->bouncebuf;
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + ret = spi_nor_read_sr(nor, sr);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + /**
>>>>>>> + * P_ERR and E_ERR bits are located in the Status Register 2
>>>>>>> + * of Cypress FL-L series chips.
>>>>>>> + */
>>>>>>> + ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
>>>>>> If checking other manufacturer datasheets, would you please check if
>>>>>> CLSR is used by any other manufacturer?
>>>>>>
>>>>> This is limited to certain subsets of Cypress/Spansion flashes.
>>>>>
>>>>>>> + if (sr[1] & SR_E_ERR)
>>>>>>> + dev_err(nor->dev, "Erase Error occurred\n");
>>>>>>> + else
>>>>>>> + dev_err(nor->dev, "Programming Error occurred\n");
>>>>>>> +
>>>>>>> + spi_nor_clear_sr(nor);
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * WEL bit remains set to one when an erase or page program
>>>>>>> + * error occurs. Issue a Write Disable command to protect
>>>>>>> + * against inadvertent writes that can possibly corrupt the
>>>>>>> + * contents of the memory.
>>>>>>> + */
>>>>>>> + ret = spi_nor_write_disable(nor);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + return -EIO;
>>>>>>> + }
>>>>>>> +
>>>>>>> + return !(sr[0] & SR_WIP);
>>>>>>> +}
>>>>>>> +
>>>>>>> /**
>>>>>>> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>>>>>>> * for new commands.
>>>>>>> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>>>>>>> */
>>>>>>> static int spi_nor_sr_ready(struct spi_nor *nor)
>>>>>>> {
>>>>>>> - int ret = spi_nor_read_sr(nor, nor->bouncebuf);
>>>>>>> + int ret;
>>>>>>> + const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
>>>>>>> +
>>>>>>> + if (IS_ERR_OR_NULL(tmpinfo))
>>>>>>> + return -ENOENT;
>>>>>>> +
>>>>>>> + if (!strcmp(tmpinfo->name, "s25fl064l")
>>>>>>> + || !strcmp(tmpinfo->name, "s25fl128l")
>>>>>>> + || !strcmp(tmpinfo->name, "s25fl256l")) {
>>>>>>> + return spi_nor_s25fl_l_sr_ready(nor);
>>>>>>> + }
>>>>>> No, we can't accept flash name comparisons in the SPI NOR core.
>>>>>> If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
>>>>>> function pointer that can be filled by spansion flashes. Or some other
>>>>>> method depending on the CLSR and RDSR2 exposure through other
>>>>>> manufacturers.
>>>>>>
>>>>>> Ideally one would skim through at least 2 - 3 datasheets from each
>>>>>> manufacturer available in SPI NOR. Preferable each datasheet from
>>>>>> a different manufacturer family. Unfortunately I'm not aware of any
>>>>>> standard that describes all the supported SPI NOR commands.
>>>>>> If you find this overwhelming, I can share the workload with you,
>>>>>> but at best effort. If you go through this by yourself, please
>>>>>> save the name of the datasheet flashes that you go through.
>>>>>>
>>>>> Agree with Tudor. There is quite a bit of variation even within
>>>>> Cypress/Spansion devices. S28 series of flashes have these error bits
>>>>> within SR1 register. See: https://www.cypress.com/file/513996/download
>>>>>
>>>>> So, this cannot be in SPI NOR core.
>>>>>
>>>>> spi_nor_xread_sr(), spi_nor_fsr_ready() and spi_nor_clear_sr() are all
>>>>> vendor/family specific and should be moved to appropriate vendor
>>>>> specific drivers.
>>>> Have to admit the comparisons and the function here are inappropriate,
>>>> though it's the fastest way to fix this problem.
>>>>
>>>> I'd like to move spansion specific operations out of the core.c first,
>>>> basing on sr_ready function pointer or other mechanisms mentioned by
>>>> Tudor, and after that I'll see if I'm capable to cope with the other
>>>> several vendor/family specific functions.
>>>>
>>> The difficulty is not in moving the code in spansion.c, but rather
>>> the documentation effort, to skim through all the datasheets. So if
>>> you're going through all the datasheets to check whether RDSR2 is used
>>> by other vendor or not, please also check the other ops that me and
>>> Vignesh indicated, so that we don't duplicate the effort.
>>>
>>> Cheers,
>>> ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-02-24 16:24 ` Yaliang.Wang
@ 2021-02-24 16:30 ` Tudor.Ambarus
2021-03-29 13:12 ` Tudor.Ambarus
0 siblings, 1 reply; 11+ messages in thread
From: Tudor.Ambarus @ 2021-02-24 16:30 UTC (permalink / raw)
To: Yaliang.Wang, vigneshr, miquel.raynal, richard; +Cc: linux-mtd
On 2/24/21 6:24 PM, Yaliang.Wang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hi, Tudor
>
> On 2/24/21 1:31 AM, Tudor.Ambarus@microchip.com wrote:
>> [Please note: This e-mail is from an EXTERNAL e-mail address]
>>
>> On 2/23/21 6:36 PM, Yaliang.Wang wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi, Tudor
>> Hi, Yaliang,
>>> So sorry to reply so late, now I'm committed to solving this problem.
>>> After checking over sixty datasheets and almost at least one datasheet
>>> for each family of the manufacturers, I can finally deviler some
>>> conclusions.
>> Great! Thanks for doing this!
>>
>>> 1. RDSR2(07H) instruction is just used by Spansion manufacturer.
>> Then we can define it as
>> #define SPINOR_OP_SPANSION_SR2 0x07h
>>
>> and define its related static function in spansion.c.
>>
>>> As for all other manufacturers, most common instructions for reading SR2
>>> is "35H", and "15H" is for reading SR3. Some manufactures have specific
>>> registers that are similar to status register, such as "Information
>>> Register", "Suspend Status Register", "Extended Read Register",
>>> "Security Register", "Flag Status Register" and also have specific
>>> instructions for reading them, such as"2BH/09H/81H/2BH/70H".
>> Are these common to more manufacturers? If yes, it will help us to inform
>> us to which manufacturers all these ops are common, it will spare us to
>> skim through datasheets later on.
> If not considering the number of chips we supported in each manufacture
> , "35H" and "15H" are more common instructions used among manufactures .
> "35H" to read SR2 is used in "spansion", "atmel", "esmt", "gigadevice",
> "winbond" manufactures.
> "15H" to read SR3 is used in "gigadevice", "winbond" manufactures.
> "33H" to read SR3 is used in "spansion" manufacture.
> "09H" to read SR2 and "95H" to read SR3 are used in "xmc" manufacture.
>
> As for SR-similar registers, they always used by manufactures acting as
> a supplement of Status Register, but with a different name.
> "eon" uses "2BH""09H" to read "Information Register" and "Suspend Status
> Register"
> "issi" uses "81H" to read "Extended Read Register"
> "macronix" uses "2BH" to read "Security Register"
> "micron" and "st" uses "70H" to read "Extended Read Register"
>>> 2. RDSR2(07H) instruction is not just existed in Spansion "S25FL-L"
>>> family, also existed in Spansion"S79FL-S, S70FS-S, S25FL-S, S25FS-S,
>>> CYRS, CY15B/V-SN" families. We just haven't use it before, it has been
>>> there for a long time.
>> Cool, it can reside in spansion.c then.
>>
>>> 3. Many manufacturers are using CLSR(30H) instruction, but the functions
>>> are different.
>>> The most common function of "30H" instruction is to resume the
>>> suspended bits, but in many Spansion's products , the function is to
>>> clear P_ERR and E_ERR bits. In some other Spansion's products(such as
>>> S25FS-S), Spansion provided a configure register, which can make "30H"
>>> instruction execute the alternative resuming suspended bits function
>>> like other manufacturers.
>> Which other manufacturers? It will help us later on.
> Spansion "S79FS-S""S70FS-S""S25FS-S" families provided the alternative
> resuming function for "30H" instruction, it can be configured with
> Configuration Register 3.
> Spansion "S79FL-S","S25FL-S","S25FL-P","S25FL-L","CYRS16B" families use
> "30H" instruction just to clear P_ERR AND E_ERR bits
> Spansion "S25FL-K", "S25FL-A", "CY15B/V-SN" families even don't use
> "30H" instruction.
>>
>> We can define the op that clears the P_ERR and E_ERR bits in spansion.c:
>> #define SPINOR_OP_SPANSION_CLSR 0x30h
>> with static function in spansion.c
>>
>> The op that is used at resume, can be latter added in the SPI NOR core.
>>
>>> 4. P_ERR and E_ERR bits are existed in many manufacturers, reside in
>>> specific register, such as "Information Register""Flag Status Register"
>>> and so on, and the instructions for reading those register and clearing
>>> the ERR bits varies according to the different manufacturer.
>> Again, if this is fresh in your mind, it would help to say which manufacturer
>> and what instructions.
> Check above comments.
Great, thanks!
>>> Based on the facts above, and to resolve the original "sr_ready" problem
>>> within "S25FL-L" family of Spansion, I think the solution is not much
>>> different. We need a "sr_ready" function pointer and maybe a new flag to
>>> mark the twisted status register. Actually I have made a rough fix
>> sr_ready function pointer should be enough. The default value for it will
>> be initialized in the SPI NOR core, and for your use-case, you can update
>> it in your manufacturer driver.
>>
>>> based on that(not include fixing spi_nor_xread_sr and
>>> spi_nor_fsr_ready). Would like to learn your suggestions before going
>>> further.
>> we should be good. Try a small patch and air it.
> So, maybe I can continue the fixing work now ? Basing on the macro and
> the strategy you suggested above.
> Optimizing the original "sr_ready" function won't be too difficult I think.
>
Yes, please. Let us know how it goes.
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-02-24 16:30 ` Tudor.Ambarus
@ 2021-03-29 13:12 ` Tudor.Ambarus
2021-03-29 16:53 ` Wang, Yaliang
0 siblings, 1 reply; 11+ messages in thread
From: Tudor.Ambarus @ 2021-03-29 13:12 UTC (permalink / raw)
To: Yaliang.Wang, vigneshr, miquel.raynal, richard; +Cc: linux-mtd
Hi, Yaliang,
On 2/24/21 6:30 PM, Tudor Ambarus - M18064 wrote:
> Yes, please. Let us know how it goes.
I think I hit the same problem that you're trying to fix, when testing a
s25fl064l. What's the status of this, do you still work on it? Do you
want me to sketch the core patches and you to come on top of it with the
spansion fix?
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips
2021-03-29 13:12 ` Tudor.Ambarus
@ 2021-03-29 16:53 ` Wang, Yaliang
0 siblings, 0 replies; 11+ messages in thread
From: Wang, Yaliang @ 2021-03-29 16:53 UTC (permalink / raw)
To: Tudor.Ambarus, vigneshr, miquel.raynal, richard; +Cc: linux-mtd
Hi, Tudor
Sorry to have kept you waiting so long, I'm still working on it. The
previous version is not so rigorous, so I tried to understand the up
layer codes better and also do some tests.
I've learnt the strategy you mentioned in the comments, I'll prepare the
second version ASAP and send it for reviewing, we'll see if it's more
close to fixing the problem.
Best Regards,
Yaliang
On 2021/3/29 21:12, Tudor.Ambarus@microchip.com wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> Hi, Yaliang,
>
> On 2/24/21 6:30 PM, Tudor Ambarus - M18064 wrote:
>> Yes, please. Let us know how it goes.
>
> I think I hit the same problem that you're trying to fix, when testing a
> s25fl064l. What's the status of this, do you still work on it? Do you
> want me to sketch the core patches and you to come on top of it with the
> spansion fix?
>
> Cheers,
> ta
>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-29 23:34 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-16 15:39 [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips yaliang.wang
2021-01-23 17:45 ` Tudor.Ambarus
2021-01-24 6:12 ` Vignesh Raghavendra
2021-01-26 2:06 ` Wang, Yaliang
2021-01-26 8:51 ` Tudor.Ambarus
2021-02-23 16:36 ` Yaliang.Wang
2021-02-23 17:31 ` Tudor.Ambarus
2021-02-24 16:24 ` Yaliang.Wang
2021-02-24 16:30 ` Tudor.Ambarus
2021-03-29 13:12 ` Tudor.Ambarus
2021-03-29 16:53 ` Wang, Yaliang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.