[PATCH v4 0/9] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. The datasheets can be found in the following links.
https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die) https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
Tested on Xilinx Zynq-7000 FPGA board.
Takahiro Kuwano (9): mtd: spi-nor: Add Cypress manufacturer ID mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t mtd: spi-nor-core: Add support for Read/Write Any Register mtd: spi-nor-core: Add support for volatile QE bit mtd: spi-nor-core: Add the ->ready() hook mtd: spi-nor-core: Add overlaid sector erase feature mtd: spi-nor-core: Add Cypress manufacturer ID in set_4byte mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t mtd: spi-nor-tiny: Add fixups for Cypress s25hl-t/s25hs-t
drivers/mtd/spi/spi-nor-core.c | 267 +++++++++++++++++++++++++++++++++ drivers/mtd/spi/spi-nor-ids.c | 36 +++++ drivers/mtd/spi/spi-nor-tiny.c | 89 +++++++++++ include/linux/mtd/spi-nor.h | 9 +- 4 files changed, 400 insertions(+), 1 deletion(-)
--- Changes since v4: - Added Read/Write Any Register support - Added the ->ready() hook to support multi-die package parts - Added S25HL02GT/S25HL04GT/S25HS02GT/S25HS04GT support
Changes since v3: - Split into multiple patches
Changes since v2: - Fixed typo in comment for spansion_overlaid_erase() - Fixed expressions for addr and len check in spansion_overlaid_erase() - Added device ID check to make the changes effective for S25 only - Added nor->setup() and fixup hooks based on the following patches https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yad... https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yad... https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yad...
-- 2.25.1
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
This patch adds Cypress manufacturer ID (34h) definition.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- include/linux/mtd/spi-nor.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 5842e9d6ee..89e7a4fdcd 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -27,6 +27,7 @@ #define SNOR_MFR_SPANSION CFI_MFR_AMD #define SNOR_MFR_SST CFI_MFR_SST #define SNOR_MFR_WINBOND 0xef /* Also used by some Spansion */ +#define SNOR_MFR_CYPRESS 0x34
/* * Note on opcode nomenclature: some opcodes have a format like
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
This patch adds Cypress manufacturer ID (34h) definition.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Reviewed-by: Pratyush Yadav p.yadav@ti.com
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. The datasheets can be found in the following links.
https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die) https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
Tested 512Mb/1Gb/2Gb parts on Xilinx Zynq-7000 FPGA board.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-ids.c | 36 +++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c index 5bd5dd3003..b78d13e980 100644 --- a/drivers/mtd/spi/spi-nor-ids.c +++ b/drivers/mtd/spi/spi-nor-ids.c @@ -217,6 +217,42 @@ const struct flash_info spi_nor_ids[] = { { INFO("s25fl208k", 0x014014, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_DUAL_READ) }, { INFO("s25fl064l", 0x016017, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) }, { INFO("s25fl128l", 0x016018, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) }, + + /* S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB + * sectors at top and/or bottom, depending on the device configuration. + * To support this, an erase hook makes overlaid sectors appear as + * uniform sectors. + */ + { INFO6("s25hl256t", 0x342a19, 0x0f0390, 256 * 1024, 128, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hl512t", 0x342a1a, 0x0f0390, 256 * 1024, 256, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hl01gt", 0x342a1b, 0x0f0390, 256 * 1024, 512, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hl02gt", 0x342a1c, 0x0f0090, 256 * 1024, 1024, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hl04gt", 0x342a1d, 0x0f0090, 256 * 1024, 2048, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hs256t", 0x342b19, 0x0f0390, 256 * 1024, 128, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hs512t", 0x342b1a, 0x0f0390, 256 * 1024, 256, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hs01gt", 0x342b1b, 0x0f0390, 256 * 1024, 512, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hs02gt", 0x342b1c, 0x0f0090, 256 * 1024, 1024, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, + { INFO6("s25hs04gt", 0x342b1d, 0x0f0090, 256 * 1024, 2048, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES | + USE_CLSR) }, #endif #ifdef CONFIG_SPI_FLASH_SST /* SST */ /* SST -- large erase sizes are "overlays", "sectors" are 4K */
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. The datasheets can be found in the following links.
https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die) https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
Tested 512Mb/1Gb/2Gb parts on Xilinx Zynq-7000 FPGA board.
Why not test the 256 Mb and 4 Gb parts as well? They might work perfectly well but adding support for untested hardware sounds wrong to me.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-ids.c | 36 +++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c index 5bd5dd3003..b78d13e980 100644 --- a/drivers/mtd/spi/spi-nor-ids.c +++ b/drivers/mtd/spi/spi-nor-ids.c @@ -217,6 +217,42 @@ const struct flash_info spi_nor_ids[] = { { INFO("s25fl208k", 0x014014, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_DUAL_READ) }, { INFO("s25fl064l", 0x016017, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) }, { INFO("s25fl128l", 0x016018, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
- /* S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
Nitpick: Please leave first line of multi-line comments blank like so:
/* * S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB * ... */
* sectors at top and/or bottom, depending on the device configuration.* To support this, an erase hook makes overlaid sectors appear as* uniform sectors.*/- { INFO6("s25hl256t", 0x342a19, 0x0f0390, 256 * 1024, 128,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hl512t", 0x342a1a, 0x0f0390, 256 * 1024, 256,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hl01gt", 0x342a1b, 0x0f0390, 256 * 1024, 512,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hl02gt", 0x342a1c, 0x0f0090, 256 * 1024, 1024,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hl04gt", 0x342a1d, 0x0f0090, 256 * 1024, 2048,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs256t", 0x342b19, 0x0f0390, 256 * 1024, 128,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs512t", 0x342b1a, 0x0f0390, 256 * 1024, 256,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs01gt", 0x342b1b, 0x0f0390, 256 * 1024, 512,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs02gt", 0x342b1c, 0x0f0090, 256 * 1024, 1024,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs04gt", 0x342b1d, 0x0f0090, 256 * 1024, 2048,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },
The datasheets you have linked do not specify a mapping between device ID and part number. So I can't verify that you are using the correct IDs here. But I'll trust you to get them right :-)
With the above comment addressed and 256 Mb and 4 Gb parts tested, please add
Reviewed-by: Pratyush Yadav p.yadav@ti.com
#endif #ifdef CONFIG_SPI_FLASH_SST /* SST */ /* SST -- large erase sizes are "overlays", "sectors" are 4K */ -- 2.25.1
Hi Pratyush,
On 1/30/2021 3:08 AM, Pratyush Yadav wrote:
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI. The datasheets can be found in the following links.
https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die) https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
Tested 512Mb/1Gb/2Gb parts on Xilinx Zynq-7000 FPGA board.
Why not test the 256 Mb and 4 Gb parts as well? They might work perfectly well but adding support for untested hardware sounds wrong to me.
The 256Mb and 4Gb parts are not yet officially sampled, although they are described in the datasheet. They should work like other tested ones, but as you commented, only tested devices should be added. I will remove 256Mb and 4Gb in v5.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-ids.c | 36 +++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c index 5bd5dd3003..b78d13e980 100644 --- a/drivers/mtd/spi/spi-nor-ids.c +++ b/drivers/mtd/spi/spi-nor-ids.c @@ -217,6 +217,42 @@ const struct flash_info spi_nor_ids[] = { { INFO("s25fl208k", 0x014014, 0, 64 * 1024, 16, SECT_4K | SPI_NOR_DUAL_READ) }, { INFO("s25fl064l", 0x016017, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) }, { INFO("s25fl128l", 0x016018, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
- /* S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
Nitpick: Please leave first line of multi-line comments blank like so:
/*
- S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
- ...
*/
I will fix it.
* sectors at top and/or bottom, depending on the device configuration.* To support this, an erase hook makes overlaid sectors appear as* uniform sectors.*/- { INFO6("s25hl256t", 0x342a19, 0x0f0390, 256 * 1024, 128,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hl512t", 0x342a1a, 0x0f0390, 256 * 1024, 256,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hl01gt", 0x342a1b, 0x0f0390, 256 * 1024, 512,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hl02gt", 0x342a1c, 0x0f0090, 256 * 1024, 1024,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hl04gt", 0x342a1d, 0x0f0090, 256 * 1024, 2048,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs256t", 0x342b19, 0x0f0390, 256 * 1024, 128,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs512t", 0x342b1a, 0x0f0390, 256 * 1024, 256,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs01gt", 0x342b1b, 0x0f0390, 256 * 1024, 512,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs02gt", 0x342b1c, 0x0f0090, 256 * 1024, 1024,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },- { INFO6("s25hs04gt", 0x342b1d, 0x0f0090, 256 * 1024, 2048,
SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |USE_CLSR) },The datasheets you have linked do not specify a mapping between device ID and part number. So I can't verify that you are using the correct IDs here. But I'll trust you to get them right :-)
Thank you :-)
With the above comment addressed and 256 Mb and 4 Gb parts tested, please add
Reviewed-by: Pratyush Yadav p.yadav@ti.com
#endif #ifdef CONFIG_SPI_FLASH_SST /* SST */ /* SST -- large erase sizes are "overlays", "sectors" are 4K */ -- 2.25.1
Best Regards, Takahiro
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips support Read/Write Any Register commands. These commands are mainly used to write volatile registers and access to the registers in second and subsequent die for multi-die package parts.
The Read Any Register instruction (65h) is followed by register address and dummy cycles, then the selected register byte is returned.
The Write Any Register instruction (71h) is followed by register address and register byte to write.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 25 +++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 4 ++++ 2 files changed, 29 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 34c15f1561..2803536ed5 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -211,6 +211,31 @@ static int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) return spi_nor_read_write_reg(nor, &op, buf); }
+#ifdef CONFIG_SPI_FLASH_SPANSION +static int spansion_read_any_reg(struct spi_nor *nor, u32 addr, u8 dummy, + u8 *val) +{ + struct spi_mem_op op = + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDAR, 1), + SPI_MEM_OP_ADDR(nor->addr_width, addr, 1), + SPI_MEM_OP_DUMMY(dummy / 8, 1), + SPI_MEM_OP_DATA_IN(1, NULL, 1)); + + return spi_nor_read_write_reg(nor, &op, val); +} + +static int spansion_write_any_reg(struct spi_nor *nor, u32 addr, u8 val) +{ + struct spi_mem_op op = + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1), + SPI_MEM_OP_ADDR(nor->addr_width, addr, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_OUT(1, NULL, 1)); + + return spi_nor_read_write_reg(nor, &op, &val); +} +#endif + static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len, u_char *buf) { diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 89e7a4fdcd..e31073eb24 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -121,6 +121,10 @@ #define SPINOR_OP_BRWR 0x17 /* Bank register write */ #define SPINOR_OP_BRRD 0x16 /* Bank register read */ #define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */ +#define SPINOR_OP_RDAR 0x65 /* Read any register */ +#define SPINOR_OP_WRAR 0x71 /* Write any register */ +#define SPINOR_REG_ADDR_STR1V 0x00800000 +#define SPINOR_REG_ADDR_CFR1V 0x00800002
/* Used for Micron flashes only. */ #define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips support Read/Write Any Register commands. These commands are mainly used to write volatile registers and access to the registers in second and subsequent die for multi-die package parts.
The Read Any Register instruction (65h) is followed by register address and dummy cycles, then the selected register byte is returned.
The Write Any Register instruction (71h) is followed by register address and register byte to write.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 25 +++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 4 ++++ 2 files changed, 29 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 34c15f1561..2803536ed5 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -211,6 +211,31 @@ static int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) return spi_nor_read_write_reg(nor, &op, buf); }
+#ifdef CONFIG_SPI_FLASH_SPANSION +static int spansion_read_any_reg(struct spi_nor *nor, u32 addr, u8 dummy,
u8 *val)+{
- struct spi_mem_op op =
SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDAR, 1),SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),SPI_MEM_OP_DUMMY(dummy / 8, 1),SPI_MEM_OP_DATA_IN(1, NULL, 1));- return spi_nor_read_write_reg(nor, &op, val);
+}
+static int spansion_write_any_reg(struct spi_nor *nor, u32 addr, u8 val) +{
- struct spi_mem_op op =
SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),SPI_MEM_OP_NO_DUMMY,SPI_MEM_OP_DATA_OUT(1, NULL, 1));- return spi_nor_read_write_reg(nor, &op, &val);
+} +#endif
static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len, u_char *buf) { diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 89e7a4fdcd..e31073eb24 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -121,6 +121,10 @@ #define SPINOR_OP_BRWR 0x17 /* Bank register write */ #define SPINOR_OP_BRRD 0x16 /* Bank register read */ #define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */ +#define SPINOR_OP_RDAR 0x65 /* Read any register */ +#define SPINOR_OP_WRAR 0x71 /* Write any register */ +#define SPINOR_REG_ADDR_STR1V 0x00800000 +#define SPINOR_REG_ADDR_CFR1V 0x00800002
These two defines are not used by this patch. Remove them from this one and add them to the one that actually uses them for the first time.
With this fixed,
Reviewed-by: Pratyush Yadav p.yadav@ti.com
/* Used for Micron flashes only. */
#define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */
2.25.1
On 1/30/2021 3:17 AM, Pratyush Yadav wrote:
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips support Read/Write Any Register commands. These commands are mainly used to write volatile registers and access to the registers in second and subsequent die for multi-die package parts.
The Read Any Register instruction (65h) is followed by register address and dummy cycles, then the selected register byte is returned.
The Write Any Register instruction (71h) is followed by register address and register byte to write.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 25 +++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 4 ++++ 2 files changed, 29 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 34c15f1561..2803536ed5 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -211,6 +211,31 @@ static int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) return spi_nor_read_write_reg(nor, &op, buf); }
+#ifdef CONFIG_SPI_FLASH_SPANSION +static int spansion_read_any_reg(struct spi_nor *nor, u32 addr, u8 dummy,
u8 *val)+{
- struct spi_mem_op op =
SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDAR, 1),SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),SPI_MEM_OP_DUMMY(dummy / 8, 1),SPI_MEM_OP_DATA_IN(1, NULL, 1));- return spi_nor_read_write_reg(nor, &op, val);
+}
+static int spansion_write_any_reg(struct spi_nor *nor, u32 addr, u8 val) +{
- struct spi_mem_op op =
SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),SPI_MEM_OP_NO_DUMMY,SPI_MEM_OP_DATA_OUT(1, NULL, 1));- return spi_nor_read_write_reg(nor, &op, &val);
+} +#endif
static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len, u_char *buf) { diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 89e7a4fdcd..e31073eb24 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -121,6 +121,10 @@ #define SPINOR_OP_BRWR 0x17 /* Bank register write */ #define SPINOR_OP_BRRD 0x16 /* Bank register read */ #define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */ +#define SPINOR_OP_RDAR 0x65 /* Read any register */ +#define SPINOR_OP_WRAR 0x71 /* Write any register */ +#define SPINOR_REG_ADDR_STR1V 0x00800000 +#define SPINOR_REG_ADDR_CFR1V 0x00800002
These two defines are not used by this patch. Remove them from this one and add them to the one that actually uses them for the first time.
I will fix it, thank you.
With this fixed,
Reviewed-by: Pratyush Yadav p.yadav@ti.com
/* Used for Micron flashes only. */
#define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */
2.25.1
Best Regards, Takahiro
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips support volatile version of configuration registers and it is recommended to update volatile registers in the field application due to a risk of the non-volatile registers corruption by power interrupt. This patch adds a function to set Quad Enable bit in CFR1 volatile.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 2803536ed5..624e730524 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -1576,6 +1576,59 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) return 0; }
+/** + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register. + * @nor: pointer to a 'struct spi_nor' + * @addr_base: base address of register (can be >0 in multi-die parts) + * @dummy: number of dummy cycles for register read + * + * It is recommended to update volatile registers in the field application due + * to a risk of the non-volatile registers corruption by power interrupt. This + * function sets Quad Enable bit in CFR1 volatile. + * + * Return: 0 on success, -errno otherwise. + */ +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 addr_base, + u8 dummy) +{ + u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V; + + u8 cr; + int ret; + + /* Check current Quad Enable bit value. */ + ret = spansion_read_any_reg(nor, addr, dummy, &cr); + if (ret < 0) { + dev_dbg(nor->dev, + "error while reading configuration register\n"); + return -EINVAL; + } + + if (cr & CR_QUAD_EN_SPAN) + return 0; + + cr |= CR_QUAD_EN_SPAN; + + write_enable(nor); + + ret = spansion_write_any_reg(nor, addr, cr); + + if (ret < 0) { + dev_dbg(nor->dev, + "error while writing configuration register\n"); + return -EINVAL; + } + + /* Read back and check it. */ + ret = spansion_read_any_reg(nor, addr, dummy, &cr); + if (ret || !(cr & CR_QUAD_EN_SPAN)) { + dev_dbg(nor->dev, "Spansion Quad bit not set\n"); + return -EINVAL; + } + + return 0; +} + #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT) /** * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
Hi,
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips support volatile version of configuration registers and it is recommended to update volatile registers in the field application due to a risk of the non-volatile registers corruption by power interrupt. This patch adds a function to set Quad Enable bit in CFR1 volatile.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 2803536ed5..624e730524 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -1576,6 +1576,59 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) return 0; }
+/**
- spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
- @nor: pointer to a 'struct spi_nor'
- @addr_base: base address of register (can be >0 in multi-die parts)
- @dummy: number of dummy cycles for register read
- It is recommended to update volatile registers in the field application due
- to a risk of the non-volatile registers corruption by power interrupt. This
- function sets Quad Enable bit in CFR1 volatile.
- Return: 0 on success, -errno otherwise.
- */
+static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 addr_base,
u8 dummy)+{
- u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V;
Why do you OR the register offset with the base? Shouldn't you be adding to it?
- u8 cr;
- int ret;
- /* Check current Quad Enable bit value. */
- ret = spansion_read_any_reg(nor, addr, dummy, &cr);
- if (ret < 0) {
dev_dbg(nor->dev,"error while reading configuration register\n");return -EINVAL;- }
- if (cr & CR_QUAD_EN_SPAN)
return 0;- cr |= CR_QUAD_EN_SPAN;
- write_enable(nor);
- ret = spansion_write_any_reg(nor, addr, cr);
- if (ret < 0) {
dev_dbg(nor->dev,"error while writing configuration register\n");return -EINVAL;- }
- /* Read back and check it. */
- ret = spansion_read_any_reg(nor, addr, dummy, &cr);
- if (ret || !(cr & CR_QUAD_EN_SPAN)) {
dev_dbg(nor->dev, "Spansion Quad bit not set\n");return -EINVAL;- }
- return 0;
+}
Rest of the patch LGTM.
#if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT) /**
- spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
-- 2.25.1
Hi Pratyush,
On 1/30/2021 3:40 AM, Pratyush Yadav wrote:
Hi,
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips support volatile version of configuration registers and it is recommended to update volatile registers in the field application due to a risk of the non-volatile registers corruption by power interrupt. This patch adds a function to set Quad Enable bit in CFR1 volatile.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 2803536ed5..624e730524 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -1576,6 +1576,59 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) return 0; }
+/**
- spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
- @nor: pointer to a 'struct spi_nor'
- @addr_base: base address of register (can be >0 in multi-die parts)
- @dummy: number of dummy cycles for register read
- It is recommended to update volatile registers in the field application due
- to a risk of the non-volatile registers corruption by power interrupt. This
- function sets Quad Enable bit in CFR1 volatile.
- Return: 0 on success, -errno otherwise.
- */
+static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 addr_base,
u8 dummy)+{
- u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V;
Why do you OR the register offset with the base? Shouldn't you be adding to it?
I missed it during review... I will fix.
- u8 cr;
- int ret;
- /* Check current Quad Enable bit value. */
- ret = spansion_read_any_reg(nor, addr, dummy, &cr);
- if (ret < 0) {
dev_dbg(nor->dev,"error while reading configuration register\n");return -EINVAL;- }
- if (cr & CR_QUAD_EN_SPAN)
return 0;- cr |= CR_QUAD_EN_SPAN;
- write_enable(nor);
- ret = spansion_write_any_reg(nor, addr, cr);
- if (ret < 0) {
dev_dbg(nor->dev,"error while writing configuration register\n");return -EINVAL;- }
- /* Read back and check it. */
- ret = spansion_read_any_reg(nor, addr, dummy, &cr);
- if (ret || !(cr & CR_QUAD_EN_SPAN)) {
dev_dbg(nor->dev, "Spansion Quad bit not set\n");return -EINVAL;- }
- return 0;
+}
Rest of the patch LGTM.
#if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT) /**
- spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
-- 2.25.1
Best Regards, Takahiro
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
For dual/quad die package devices from Spansion/Cypress, the device's status needs to be checked by reading status registers in all dies, by using Read Any Register command. To support this, a Flash specific hook that can overwrite the legacy status check is needed.
The spansion_sr_ready() reads status register 1 by Read Any Register commnad. This function is called from Flash specific hook with die address and dummy cycles.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 32 ++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 4 +++- 2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 624e730524..1c0ba5abf9 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -522,6 +522,35 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info, } }
+#ifdef CONFIG_SPI_FLASH_SPANSION +/* + * Read status register 1 by using Read Any Register command to support multi + * die package parts. + */ +static int spansion_sr_ready(struct spi_nor *nor, u32 addr_base, u8 dummy) +{ + u32 reg_addr = addr_base + SPINOR_REG_ADDR_STR1V; + u8 sr; + int ret; + + ret = spansion_read_any_reg(nor, reg_addr, dummy, &sr); + if (ret < 0) + return ret; + + if (sr & (SR_E_ERR | SR_P_ERR)) { + if (sr & SR_E_ERR) + dev_dbg(nor->dev, "Erase Error occurred\n"); + else + dev_dbg(nor->dev, "Programming Error occurred\n"); + + nor->write_reg(nor, SPINOR_OP_CLSR, NULL, 0); + return -EIO; + } + + return !(sr & SR_WIP); +} +#endif + static int spi_nor_sr_ready(struct spi_nor *nor) { int sr = read_sr(nor); @@ -570,6 +599,9 @@ static int spi_nor_ready(struct spi_nor *nor) { int sr, fsr;
+ if (nor->ready) + return nor->ready(nor); + sr = spi_nor_sr_ready(nor); if (sr < 0) return sr; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index e31073eb24..25234177de 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -434,8 +434,9 @@ struct flash_info; * @flash_lock: [FLASH-SPECIFIC] lock a region of the SPI NOR * @flash_unlock: [FLASH-SPECIFIC] unlock a region of the SPI NOR * @flash_is_locked: [FLASH-SPECIFIC] check if a region of the SPI NOR is - * @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode * completely locked + * @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode + * @ready: [FLASH-SPECIFIC] check if the flash is ready * @priv: the private data */ struct spi_nor { @@ -481,6 +482,7 @@ struct spi_nor { int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*quad_enable)(struct spi_nor *nor); + int (*ready)(struct spi_nor *nor);
void *priv; /* Compatibility for spi_flash, remove once sf layer is merged with mtd */
Hi,
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
For dual/quad die package devices from Spansion/Cypress, the device's status needs to be checked by reading status registers in all dies, by using Read Any Register command. To support this, a Flash specific hook that can overwrite the legacy status check is needed.
The spansion_sr_ready() reads status register 1 by Read Any Register commnad. This function is called from Flash specific hook with die address and dummy cycles.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 32 ++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 4 +++- 2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 624e730524..1c0ba5abf9 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -522,6 +522,35 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info, } }
+#ifdef CONFIG_SPI_FLASH_SPANSION +/*
- Read status register 1 by using Read Any Register command to support multi
- die package parts.
- */
+static int spansion_sr_ready(struct spi_nor *nor, u32 addr_base, u8 dummy) +{
- u32 reg_addr = addr_base + SPINOR_REG_ADDR_STR1V;
- u8 sr;
- int ret;
- ret = spansion_read_any_reg(nor, reg_addr, dummy, &sr);
- if (ret < 0)
return ret;- if (sr & (SR_E_ERR | SR_P_ERR)) {
if (sr & SR_E_ERR)dev_dbg(nor->dev, "Erase Error occurred\n");elsedev_dbg(nor->dev, "Programming Error occurred\n");nor->write_reg(nor, SPINOR_OP_CLSR, NULL, 0);return -EIO;- }
- return !(sr & SR_WIP);
+} +#endif
Do not add this function in this patch. Just add the hook here. Add it in the patch that actually uses it.
static int spi_nor_sr_ready(struct spi_nor *nor) { int sr = read_sr(nor); @@ -570,6 +599,9 @@ static int spi_nor_ready(struct spi_nor *nor) { int sr, fsr;
- if (nor->ready)
return nor->ready(nor);
Move the code below in a separate function and call it something like spi_nor_default_ready(). Then call that function from here.
sr = spi_nor_sr_ready(nor); if (sr < 0) return sr; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index e31073eb24..25234177de 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -434,8 +434,9 @@ struct flash_info;
- @flash_lock: [FLASH-SPECIFIC] lock a region of the SPI NOR
- @flash_unlock: [FLASH-SPECIFIC] unlock a region of the SPI NOR
- @flash_is_locked: [FLASH-SPECIFIC] check if a region of the SPI NOR is
- @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode
completely locked
- @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode
*/
- @ready: [FLASH-SPECIFIC] check if the flash is ready
- @priv: the private data
struct spi_nor { @@ -481,6 +482,7 @@ struct spi_nor { int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*quad_enable)(struct spi_nor *nor);
int (*ready)(struct spi_nor *nor);
void *priv;
/* Compatibility for spi_flash, remove once sf layer is merged with mtd */
2.25.1
Hi Pratyush,
Thank you for the feedback. I will address this in v5.
On 1/30/2021 3:49 AM, Pratyush Yadav wrote:
Hi,
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
For dual/quad die package devices from Spansion/Cypress, the device's status needs to be checked by reading status registers in all dies, by using Read Any Register command. To support this, a Flash specific hook that can overwrite the legacy status check is needed.
The spansion_sr_ready() reads status register 1 by Read Any Register commnad. This function is called from Flash specific hook with die address and dummy cycles.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 32 ++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 4 +++- 2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 624e730524..1c0ba5abf9 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -522,6 +522,35 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info, } }
+#ifdef CONFIG_SPI_FLASH_SPANSION +/*
- Read status register 1 by using Read Any Register command to support multi
- die package parts.
- */
+static int spansion_sr_ready(struct spi_nor *nor, u32 addr_base, u8 dummy) +{
- u32 reg_addr = addr_base + SPINOR_REG_ADDR_STR1V;
- u8 sr;
- int ret;
- ret = spansion_read_any_reg(nor, reg_addr, dummy, &sr);
- if (ret < 0)
return ret;- if (sr & (SR_E_ERR | SR_P_ERR)) {
if (sr & SR_E_ERR)dev_dbg(nor->dev, "Erase Error occurred\n");elsedev_dbg(nor->dev, "Programming Error occurred\n");nor->write_reg(nor, SPINOR_OP_CLSR, NULL, 0);return -EIO;- }
- return !(sr & SR_WIP);
+} +#endif
Do not add this function in this patch. Just add the hook here. Add it in the patch that actually uses it.
static int spi_nor_sr_ready(struct spi_nor *nor) { int sr = read_sr(nor); @@ -570,6 +599,9 @@ static int spi_nor_ready(struct spi_nor *nor) { int sr, fsr;
- if (nor->ready)
return nor->ready(nor);Move the code below in a separate function and call it something like spi_nor_default_ready(). Then call that function from here.
sr = spi_nor_sr_ready(nor); if (sr < 0) return sr; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index e31073eb24..25234177de 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -434,8 +434,9 @@ struct flash_info;
- @flash_lock: [FLASH-SPECIFIC] lock a region of the SPI NOR
- @flash_unlock: [FLASH-SPECIFIC] unlock a region of the SPI NOR
- @flash_is_locked: [FLASH-SPECIFIC] check if a region of the SPI NOR is
- @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode
completely locked
- @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode
*/
- @ready: [FLASH-SPECIFIC] check if the flash is ready
- @priv: the private data
struct spi_nor { @@ -481,6 +482,7 @@ struct spi_nor { int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*quad_enable)(struct spi_nor *nor);
int (*ready)(struct spi_nor *nor);
void *priv;
/* Compatibility for spi_flash, remove once sf layer is merged with mtd */
2.25.1
Best Regards, Takahiro
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or bottom, depending on the device configuration, while U-Boot supports uniform sector layout only. This patch adds an erase hook that emulates uniform sector layout.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 48 ++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 1c0ba5abf9..70da0081b6 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -788,6 +788,54 @@ erase_err: return ret; }
+#ifdef CONFIG_SPI_FLASH_SPANSION +/* + * Erase for Spansion/Cypress Flash devices that has overlaid 4KB sectors at + * the top and/or bottom. + */ +static int spansion_overlaid_erase(struct mtd_info *mtd, + struct erase_info *instr) +{ + struct spi_nor *nor = mtd_to_spi_nor(mtd); + struct erase_info instr_4k; + u8 opcode; + u32 erasesize; + int ret; + + /* Perform default erase operation (non-overlaid portion is erased) */ + ret = spi_nor_erase(mtd, instr); + if (ret) + return ret; + + /* Backup default erase opcode and size */ + opcode = nor->erase_opcode; + erasesize = mtd->erasesize; + + /* + * Erase 4KB sectors. Use the possible max length of 4KB sector region. + * The Flash just ignores the command if the address is not configured + * as 4KB sector and reports ready status immediately. + */ + instr_4k.len = SZ_128K; + nor->erase_opcode = SPINOR_OP_BE_4K_4B; + mtd->erasesize = SZ_4K; + if (instr->addr == 0) { + instr_4k.addr = 0; + ret = spi_nor_erase(mtd, &instr_4k); + } + if (!ret && instr->addr + instr->len == mtd->size) { + instr_4k.addr = mtd->size - instr_4k.len; + ret = spi_nor_erase(mtd, &instr_4k); + } + + /* Restore erase opcode and size */ + nor->erase_opcode = opcode; + mtd->erasesize = erasesize; + + return ret; +} +#endif + #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) /* Write status register and ensure bits in mask match written values */ static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
Hi Takahiro,
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or bottom, depending on the device configuration, while U-Boot supports uniform sector layout only. This patch adds an erase hook that emulates uniform sector layout.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 48 ++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 1c0ba5abf9..70da0081b6 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -788,6 +788,54 @@ erase_err: return ret; }
+#ifdef CONFIG_SPI_FLASH_SPANSION +/*
- Erase for Spansion/Cypress Flash devices that has overlaid 4KB sectors at
- the top and/or bottom.
- */
+static int spansion_overlaid_erase(struct mtd_info *mtd,
struct erase_info *instr)+{
- struct spi_nor *nor = mtd_to_spi_nor(mtd);
- struct erase_info instr_4k;
- u8 opcode;
- u32 erasesize;
- int ret;
- /* Perform default erase operation (non-overlaid portion is erased) */
- ret = spi_nor_erase(mtd, instr);
- if (ret)
return ret;- /* Backup default erase opcode and size */
- opcode = nor->erase_opcode;
- erasesize = mtd->erasesize;
- /*
* Erase 4KB sectors. Use the possible max length of 4KB sector region.* The Flash just ignores the command if the address is not configured* as 4KB sector and reports ready status immediately.*/- instr_4k.len = SZ_128K;
- nor->erase_opcode = SPINOR_OP_BE_4K_4B;
- mtd->erasesize = SZ_4K;
- if (instr->addr == 0) {
instr_4k.addr = 0;ret = spi_nor_erase(mtd, &instr_4k);- }
- if (!ret && instr->addr + instr->len == mtd->size) {
instr_4k.addr = mtd->size - instr_4k.len;ret = spi_nor_erase(mtd, &instr_4k);- }
This feels like a hack to me. Does the flash datasheet explicitly say that erasing the overlaid area with the "normal" erase opcode is a no-op?
I don't see a big reason to run this hack. You are already in a flash-specific erase hook. Why not just directly issue the correct erase commands to the sectors? That is, why not issue 4k erase to overlaid sectors and normal erase to the rest? Why do you need to emulate uniform erase?
- /* Restore erase opcode and size */
- nor->erase_opcode = opcode;
- mtd->erasesize = erasesize;
- return ret;
+} +#endif
#if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) /* Write status register and ensure bits in mask match written values */ static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask) -- 2.25.1
Hi Pratyush,
On 2/2/2021 3:56 AM, Pratyush Yadav wrote:
Hi Takahiro,
On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or bottom, depending on the device configuration, while U-Boot supports uniform sector layout only. This patch adds an erase hook that emulates uniform sector layout.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 48 ++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 1c0ba5abf9..70da0081b6 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -788,6 +788,54 @@ erase_err: return ret; }
+#ifdef CONFIG_SPI_FLASH_SPANSION +/*
- Erase for Spansion/Cypress Flash devices that has overlaid 4KB sectors at
- the top and/or bottom.
- */
+static int spansion_overlaid_erase(struct mtd_info *mtd,
struct erase_info *instr)+{
- struct spi_nor *nor = mtd_to_spi_nor(mtd);
- struct erase_info instr_4k;
- u8 opcode;
- u32 erasesize;
- int ret;
- /* Perform default erase operation (non-overlaid portion is erased) */
- ret = spi_nor_erase(mtd, instr);
- if (ret)
return ret;- /* Backup default erase opcode and size */
- opcode = nor->erase_opcode;
- erasesize = mtd->erasesize;
- /*
* Erase 4KB sectors. Use the possible max length of 4KB sector region.* The Flash just ignores the command if the address is not configured* as 4KB sector and reports ready status immediately.*/- instr_4k.len = SZ_128K;
- nor->erase_opcode = SPINOR_OP_BE_4K_4B;
- mtd->erasesize = SZ_4K;
- if (instr->addr == 0) {
instr_4k.addr = 0;ret = spi_nor_erase(mtd, &instr_4k);- }
- if (!ret && instr->addr + instr->len == mtd->size) {
instr_4k.addr = mtd->size - instr_4k.len;ret = spi_nor_erase(mtd, &instr_4k);- }
This feels like a hack to me. Does the flash datasheet explicitly say that erasing the overlaid area with the "normal" erase opcode is a no-op?
Sorry, I have noticed the datasheet I mentioned in the cover letter is a summary version. Here is the link to he full version, but you need registration to access. https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Sempe...
So, let me quote the description about erasing overlaid area:
"If a sector erase transaction is applied to a 256 KB sector that is overlaid by 4 KB sectors, the overlaid 4 KB sectors are not affected by the erase. Only the visible (non-overlaid) portion of the 128 KB or 192 KB sector is erased."
And about 4 KB sector erase:
"This transaction is ignored when the device is configured for uniform sector only (CFR3V[3] = 1). If the Erase 4K B sector transaction is issued to a non-4 KB sector address, the device will abort the operation and will not set the ERSERR status fail bit."
I don't see a big reason to run this hack. You are already in a flash-specific erase hook. Why not just directly issue the correct erase commands to the sectors? That is, why not issue 4k erase to overlaid sectors and normal erase to the rest? Why do you need to emulate uniform erase?
Thanks for pointing this out. I should probably hook nor->erase() instead of mtd->_erase(), then issue 4k or normal erase depending on the address. I will introduce that in v5.
- /* Restore erase opcode and size */
- nor->erase_opcode = opcode;
- mtd->erasesize = erasesize;
- return ret;
+} +#endif
#if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST) /* Write status register and ensure bits in mask match written values */ static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask) -- 2.25.1
Best Regards, Takahiro
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Cypress chips support SPINOR_OP_EN4B(B7h)/SPINOR_OP_EX4B(E9h) to enable/disable 4-byte addressing mode.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 70da0081b6..ef49328a28 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -492,6 +492,7 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info, case SNOR_MFR_ISSI: case SNOR_MFR_MACRONIX: case SNOR_MFR_WINBOND: + case SNOR_MFR_CYPRESS: if (need_wren) write_enable(nor);
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Add nor->setup() and fixup hooks to overwrite: - volatile QE bit - the ->ready() hook for dual/quad die package parts - overlaid erase - spi_nor_flash_parameter - mtd_info
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 108 +++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index ef49328a28..3d8cb9c333 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor) return 0; }
+#ifdef CONFIG_SPI_FLASH_SPANSION +static int s25hx_t_mdp_ready(struct spi_nor *nor) +{ + u32 addr; + int ret; + + for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) { + ret = spansion_sr_ready(nor, addr, 0); + if (ret != 1) + return ret; + } + + return 1; +} + +static int s25hx_t_quad_enable(struct spi_nor *nor) +{ + u32 addr; + int ret; + + for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) { + ret = spansion_quad_enable_volatile(nor, addr, 0); + if (ret) + return ret; + } + + return 0; +} + +static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info, + const struct spi_nor_flash_parameter *params, + const struct spi_nor_hwcaps *hwcaps) +{ +#ifdef CONFIG_SPI_FLASH_BAR + return -ENOTSUPP; /* Bank Address Register is not supported */ +#endif + /* + * The Cypress Semper family has transparent ECC. To preserve + * ECC enabled, multi-pass programming within the same 16-byte + * ECC data unit needs to be avoided. Set writesize to the page + * size and remove the MTD_BIT_WRITEABLE flag in mtd_info to + * prevent multi-pass programming. + */ + nor->mtd.writesize = params->page_size; + nor->mtd.flags &= ~MTD_BIT_WRITEABLE; + + /* Emulate uniform sector architecure by this erase hook*/ + nor->mtd._erase = spansion_overlaid_erase; + + /* For 2Gb (dual die) and 4Gb (quad die) parts */ + if (nor->mtd.size > SZ_128M) + nor->ready = s25hx_t_mdp_ready; + + /* Enter 4-byte addressing mode for WRAR used in quad_enable */ + set_4byte(nor, info, true); + + return spi_nor_default_setup(nor, info, params, hwcaps); +} + +static void s25hx_t_default_init(struct spi_nor *nor) +{ + nor->setup = s25hx_t_setup; +} + +static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor, + const struct sfdp_parameter_header *header, + const struct sfdp_bfpt *bfpt, + struct spi_nor_flash_parameter *params) +{ + /* Default page size is 256-byte, but BFPT reports 512-byte */ + params->page_size = 256; + /* Reset erase size in case it is set to 4K from BFPT */ + nor->mtd.erasesize = 0; + + return 0; +} + +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) +{ + /* READ_FAST_4B (0Ch) requires mode cycles*/ + params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8; + /* PP_1_1_4 is not supported */ + params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4; + /* Use volatile register to enable quad */ + params->quad_enable = s25hx_t_quad_enable; +} + +static struct spi_nor_fixups s25hx_t_fixups = { + .default_init = s25hx_t_default_init, + .post_bfpt = s25hx_t_post_bfpt_fixup, + .post_sfdp = s25hx_t_post_sfdp_fixup, +}; +#endif + static void spi_nor_set_fixups(struct spi_nor *nor) { +#ifdef CONFIG_SPI_FLASH_SPANSION + if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) { + switch (nor->info->id[1]) { + case 0x2a: /* S25HL (QSPI, 3.3V) */ + case 0x2b: /* S25HS (QSPI, 1.8V) */ + nor->fixups = &s25hx_t_fixups; + break; + + default: + break; + } + } +#endif }
int spi_nor_scan(struct spi_nor *nor)
On 28/01/21 01:37PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Add nor->setup() and fixup hooks to overwrite:
- volatile QE bit
- the ->ready() hook for dual/quad die package parts
- overlaid erase
- spi_nor_flash_parameter
- mtd_info
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 108 +++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index ef49328a28..3d8cb9c333 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor) return 0; }
+#ifdef CONFIG_SPI_FLASH_SPANSION +static int s25hx_t_mdp_ready(struct spi_nor *nor) +{
- u32 addr;
- int ret;
- for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
ret = spansion_sr_ready(nor, addr, 0);if (ret != 1)return ret;- }
- return 1;
+}
+static int s25hx_t_quad_enable(struct spi_nor *nor) +{
- u32 addr;
- int ret;
- for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
ret = spansion_quad_enable_volatile(nor, addr, 0);
So you need to set the QE bit on each die. Ok.
Out of curiosity, what will happen if you only set the QE bit on the first die? Will reads from first die work in quad mode and rest in single mode?
if (ret)return ret;- }
- return 0;
+}
+static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info,
const struct spi_nor_flash_parameter *params,const struct spi_nor_hwcaps *hwcaps)+{ +#ifdef CONFIG_SPI_FLASH_BAR
- return -ENOTSUPP; /* Bank Address Register is not supported */
+#endif
- /*
* The Cypress Semper family has transparent ECC. To preserve* ECC enabled, multi-pass programming within the same 16-byte* ECC data unit needs to be avoided. Set writesize to the page* size and remove the MTD_BIT_WRITEABLE flag in mtd_info to* prevent multi-pass programming.*/- nor->mtd.writesize = params->page_size;
The writesize should be set to 16. See [0][1][2].
- nor->mtd.flags &= ~MTD_BIT_WRITEABLE;
Not needed. See discussions pointed to above.
- /* Emulate uniform sector architecure by this erase hook*/
- nor->mtd._erase = spansion_overlaid_erase;
- /* For 2Gb (dual die) and 4Gb (quad die) parts */
- if (nor->mtd.size > SZ_128M)
nor->ready = s25hx_t_mdp_ready;- /* Enter 4-byte addressing mode for WRAR used in quad_enable */
- set_4byte(nor, info, true);
- return spi_nor_default_setup(nor, info, params, hwcaps);
+}
+static void s25hx_t_default_init(struct spi_nor *nor) +{
- nor->setup = s25hx_t_setup;
+}
+static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
const struct sfdp_parameter_header *header,const struct sfdp_bfpt *bfpt,struct spi_nor_flash_parameter *params)+{
- /* Default page size is 256-byte, but BFPT reports 512-byte */
- params->page_size = 256;
Read the page size from the register, like it is done on Linux for S28 flash family.
- /* Reset erase size in case it is set to 4K from BFPT */
- nor->mtd.erasesize = 0;
What does erasesize of 0 mean? I would take that to mean that the flash does not support erases. I can't find any mention of 0 erase size in the documentation of struct mtd_info.
- return 0;
+}
+static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor,
struct spi_nor_flash_parameter *params)+{
- /* READ_FAST_4B (0Ch) requires mode cycles*/
- params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
- /* PP_1_1_4 is not supported */
- params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
- /* Use volatile register to enable quad */
- params->quad_enable = s25hx_t_quad_enable;
+}
+static struct spi_nor_fixups s25hx_t_fixups = {
- .default_init = s25hx_t_default_init,
- .post_bfpt = s25hx_t_post_bfpt_fixup,
- .post_sfdp = s25hx_t_post_sfdp_fixup,
Hmm, I don't think the fixups feature was ever applied to the u-boot tree. I sent a patch for them a while ago [3] but they were never applied due to some issues. I can't find any mentions of "spi_nor_set_fixups" on my 4 day old checkout of Tom's master branch.
And that reminds me, the nor->setup() hook you are using is not there either. Your patch series should not even build on upstream u-boot. Please cherry pick the required patches from my series and send them along with yours.
Please make sure your patch series builds and works on top of _upstream_ u-boot. Even if it works on your company's fork of U-Boot does not necessarily mean it will work on upstream.
+}; +#endif
static void spi_nor_set_fixups(struct spi_nor *nor)
This function is also not present in u-boot master.
{ +#ifdef CONFIG_SPI_FLASH_SPANSION
- if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
switch (nor->info->id[1]) {case 0x2a: /* S25HL (QSPI, 3.3V) */case 0x2b: /* S25HS (QSPI, 1.8V) */nor->fixups = &s25hx_t_fixups;break;
I recall using strcmp() in my series but I guess this should also work just as well.
default:break;}- }
+#endif }
int spi_nor_scan(struct spi_nor *nor)
2.25.1
[0] https://lore.kernel.org/linux-mtd/4c0e3207-72a4-8c1a-5fca-e9f30cc60828@ti.co... [1] https://lore.kernel.org/linux-mtd/20201201102711.8727-3-p.yadav@ti.com/ [2] https://lore.kernel.org/linux-mtd/20201201102711.8727-4-p.yadav@ti.com/ [3] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yad...
Hi Pratyush,
On 2/2/2021 4:22 AM, Pratyush Yadav wrote:
On 28/01/21 01:37PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Add nor->setup() and fixup hooks to overwrite:
- volatile QE bit
- the ->ready() hook for dual/quad die package parts
- overlaid erase
- spi_nor_flash_parameter
- mtd_info
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-core.c | 108 +++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index ef49328a28..3d8cb9c333 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor) return 0; }
+#ifdef CONFIG_SPI_FLASH_SPANSION +static int s25hx_t_mdp_ready(struct spi_nor *nor) +{
- u32 addr;
- int ret;
- for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
ret = spansion_sr_ready(nor, addr, 0);if (ret != 1)return ret;- }
- return 1;
+}
+static int s25hx_t_quad_enable(struct spi_nor *nor) +{
- u32 addr;
- int ret;
- for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
ret = spansion_quad_enable_volatile(nor, addr, 0);So you need to set the QE bit on each die. Ok.
Out of curiosity, what will happen if you only set the QE bit on the first die? Will reads from first die work in quad mode and rest in single mode?
If the host issues quad read command, only the first die works and rest do not respond to the quad read command.
if (ret)return ret;- }
- return 0;
+}
+static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info,
const struct spi_nor_flash_parameter *params,const struct spi_nor_hwcaps *hwcaps)+{ +#ifdef CONFIG_SPI_FLASH_BAR
- return -ENOTSUPP; /* Bank Address Register is not supported */
+#endif
- /*
* The Cypress Semper family has transparent ECC. To preserve* ECC enabled, multi-pass programming within the same 16-byte* ECC data unit needs to be avoided. Set writesize to the page* size and remove the MTD_BIT_WRITEABLE flag in mtd_info to* prevent multi-pass programming.*/- nor->mtd.writesize = params->page_size;
The writesize should be set to 16. See [0][1][2].
- nor->mtd.flags &= ~MTD_BIT_WRITEABLE;
Not needed. See discussions pointed to above.
OK, thank you for the information.
- /* Emulate uniform sector architecure by this erase hook*/
- nor->mtd._erase = spansion_overlaid_erase;
- /* For 2Gb (dual die) and 4Gb (quad die) parts */
- if (nor->mtd.size > SZ_128M)
nor->ready = s25hx_t_mdp_ready;- /* Enter 4-byte addressing mode for WRAR used in quad_enable */
- set_4byte(nor, info, true);
- return spi_nor_default_setup(nor, info, params, hwcaps);
+}
+static void s25hx_t_default_init(struct spi_nor *nor) +{
- nor->setup = s25hx_t_setup;
+}
+static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
const struct sfdp_parameter_header *header,const struct sfdp_bfpt *bfpt,struct spi_nor_flash_parameter *params)+{
- /* Default page size is 256-byte, but BFPT reports 512-byte */
- params->page_size = 256;
Read the page size from the register, like it is done on Linux for S28 flash family.
Will fix.
- /* Reset erase size in case it is set to 4K from BFPT */
- nor->mtd.erasesize = 0;
What does erasesize of 0 mean? I would take that to mean that the flash does not support erases. I can't find any mention of 0 erase size in the documentation of struct mtd_info.
In this device, the erasesize is wrongly configured to 4K through BFPT parse. I would reset it to 0 expecting the correct value is set in spi_nor_select_erase() afterwards. But I should simply set correct value in this fixup hook.
- return 0;
+}
+static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor,
struct spi_nor_flash_parameter *params)+{
- /* READ_FAST_4B (0Ch) requires mode cycles*/
- params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
- /* PP_1_1_4 is not supported */
- params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
- /* Use volatile register to enable quad */
- params->quad_enable = s25hx_t_quad_enable;
+}
+static struct spi_nor_fixups s25hx_t_fixups = {
- .default_init = s25hx_t_default_init,
- .post_bfpt = s25hx_t_post_bfpt_fixup,
- .post_sfdp = s25hx_t_post_sfdp_fixup,
Hmm, I don't think the fixups feature was ever applied to the u-boot tree. I sent a patch for them a while ago [3] but they were never applied due to some issues. I can't find any mentions of "spi_nor_set_fixups" on my 4 day old checkout of Tom's master branch.
And that reminds me, the nor->setup() hook you are using is not there either. Your patch series should not even build on upstream u-boot. Please cherry pick the required patches from my series and send them along with yours.
Please make sure your patch series builds and works on top of _upstream_ u-boot. Even if it works on your company's fork of U-Boot does not necessarily mean it will work on upstream.
This patch depends on your patch that introduces the fixups feature. I mentioned it in the changes log section in cover letter only. I will add it into commit description of this patch.
+}; +#endif
static void spi_nor_set_fixups(struct spi_nor *nor)
This function is also not present in u-boot master.
{ +#ifdef CONFIG_SPI_FLASH_SPANSION
- if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
switch (nor->info->id[1]) {case 0x2a: /* S25HL (QSPI, 3.3V) */case 0x2b: /* S25HS (QSPI, 1.8V) */nor->fixups = &s25hx_t_fixups;break;I recall using strcmp() in my series but I guess this should also work just as well.
default:break;}- }
+#endif }
int spi_nor_scan(struct spi_nor *nor)
2.25.1
[0] https://lore.kernel.org/linux-mtd/4c0e3207-72a4-8c1a-5fca-e9f30cc60828@ti.co... [1] https://lore.kernel.org/linux-mtd/20201201102711.8727-3-p.yadav@ti.com/ [2] https://lore.kernel.org/linux-mtd/20201201102711.8727-4-p.yadav@ti.com/ [3] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yad...
Best Regards, Takahiro
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny. The volatile QE bit function, spansion_quad_enable_volatile() supports dual/quad die package parts, by taking 'die_size' parameter that is used to iterate register update for all dies in the device.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-tiny.c | 89 ++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c index 5cc2b7d996..66680df5a9 100644 --- a/drivers/mtd/spi/spi-nor-tiny.c +++ b/drivers/mtd/spi/spi-nor-tiny.c @@ -555,6 +555,85 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) } #endif /* CONFIG_SPI_FLASH_SPANSION */
+#ifdef CONFIG_SPI_FLASH_SPANSION +/** + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register. + * @nor: pointer to a 'struct spi_nor' + * @die_size: maximum number of bytes per die ('mtd.size' > 'die_size' in + * multi die package parts). + * @dummy: number of dummy cycles for register read + * + * It is recommended to update volatile registers in the field application due + * to a risk of the non-volatile registers corruption by power interrupt. This + * function sets Quad Enable bit in CFR1 volatile. + * + * Return: 0 on success, -errno otherwise. + */ +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 die_size, + u8 dummy) +{ + struct spi_mem_op op = + SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1), + SPI_MEM_OP_ADDR(4, 0, 1), + SPI_MEM_OP_DUMMY(0, 1), + SPI_MEM_OP_DATA_IN(1, NULL, 1)); + u32 addr; + u8 cr; + int ret; + + /* Use 4-byte address for RDAR/WRAR */ + ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0); + if (ret < 0) { + dev_dbg(nor->dev, + "error while enabling 4-byte address\n"); + return ret; + } + + for (addr = 0; addr < nor->mtd.size; addr += die_size) { + op.addr.val = addr + SPINOR_REG_ADDR_CFR1V; + + /* Check current Quad Enable bit value. */ + op.cmd.opcode = SPINOR_OP_RDAR; + op.dummy.nbytes = dummy / 8; + op.data.dir = SPI_MEM_DATA_IN; + ret = spi_nor_read_write_reg(nor, &op, &cr); + if (ret < 0) { + dev_dbg(nor->dev, + "error while reading configuration register\n"); + return -EINVAL; + } + + if (cr & CR_QUAD_EN_SPAN) + return 0; + + /* Write new value. */ + cr |= CR_QUAD_EN_SPAN; + op.cmd.opcode = SPINOR_OP_WRAR; + op.dummy.nbytes = 0; + op.data.dir = SPI_MEM_DATA_OUT; + write_enable(nor); + ret = spi_nor_read_write_reg(nor, &op, &cr); + if (ret < 0) { + dev_dbg(nor->dev, + "error while writing configuration register\n"); + return -EINVAL; + } + + /* Read back and check it. */ + op.cmd.opcode = SPINOR_OP_RDAR; + op.dummy.nbytes = dummy / 8; + op.data.dir = SPI_MEM_DATA_IN; + ret = spi_nor_read_write_reg(nor, &op, &cr); + if (ret || !(cr & CR_QUAD_EN_SPAN)) { + dev_dbg(nor->dev, "Spansion Quad bit not set\n"); + return -EINVAL; + } + } + + return 0; +} +#endif + static void spi_nor_set_read_settings(struct spi_nor_read_command *read, u8 num_mode_clocks, @@ -583,6 +662,11 @@ static int spi_nor_init_params(struct spi_nor *nor, spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], 0, 8, SPINOR_OP_READ_FAST, SNOR_PROTO_1_1_1); +#ifdef CONFIG_SPI_FLASH_SPANSION + if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS && + (info->id[1] == 0x2a || info->id[1] == 0x2b)) + params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8; +#endif }
if (info->flags & SPI_NOR_QUAD_READ) { @@ -659,6 +743,11 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info, case SNOR_MFR_MACRONIX: err = macronix_quad_enable(nor); break; +#endif +#ifdef CONFIG_SPI_FLASH_SPANSION + case SNOR_MFR_CYPRESS: + err = spansion_quad_enable_volatile(nor, SZ_128M, 0); + break; #endif case SNOR_MFR_ST: case SNOR_MFR_MICRON:
On 28/01/21 01:37PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny. The volatile QE bit function, spansion_quad_enable_volatile() supports dual/quad die package parts, by taking 'die_size' parameter that is used to iterate register update for all dies in the device.
I'm not so sure if this is a good idea. spi-nor-tiny should be the minimal set of functionality to get the bootloader to the next stage. 1S-1S-1S mode is sufficient for that. Adding quad enable functions of all the flashes will increase the size quite a bit. I know that some flashes already have their quad enable hooks, and I don't think they should be there either.
Of course, the maintainers have the final call, but from my side,
Nacked-by: Pratyush Yadav p.yadav@ti.com
Anyway, comments below in case the maintainers do plan on picking this patch up.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-tiny.c | 89 ++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c index 5cc2b7d996..66680df5a9 100644 --- a/drivers/mtd/spi/spi-nor-tiny.c +++ b/drivers/mtd/spi/spi-nor-tiny.c @@ -555,6 +555,85 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) } #endif /* CONFIG_SPI_FLASH_SPANSION */
+#ifdef CONFIG_SPI_FLASH_SPANSION +/**
- spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
- @nor: pointer to a 'struct spi_nor'
- @die_size: maximum number of bytes per die ('mtd.size' > 'die_size' in
multi die package parts).
- @dummy: number of dummy cycles for register read
- It is recommended to update volatile registers in the field application due
- to a risk of the non-volatile registers corruption by power interrupt. This
- function sets Quad Enable bit in CFR1 volatile.
- Return: 0 on success, -errno otherwise.
- */
+static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 die_size,
u8 dummy)+{
- struct spi_mem_op op =
SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1),SPI_MEM_OP_ADDR(4, 0, 1),SPI_MEM_OP_DUMMY(0, 1),SPI_MEM_OP_DATA_IN(1, NULL, 1));- u32 addr;
- u8 cr;
- int ret;
- /* Use 4-byte address for RDAR/WRAR */
- ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
- if (ret < 0) {
dev_dbg(nor->dev,"error while enabling 4-byte address\n");return ret;- }
- for (addr = 0; addr < nor->mtd.size; addr += die_size) {
op.addr.val = addr + SPINOR_REG_ADDR_CFR1V;
So here you add the register offset to the base address, instead of ORing it. Ok.
/* Check current Quad Enable bit value. */op.cmd.opcode = SPINOR_OP_RDAR;op.dummy.nbytes = dummy / 8;op.data.dir = SPI_MEM_DATA_IN;ret = spi_nor_read_write_reg(nor, &op, &cr);if (ret < 0) {dev_dbg(nor->dev,"error while reading configuration register\n");return -EINVAL;}if (cr & CR_QUAD_EN_SPAN)return 0;/* Write new value. */cr |= CR_QUAD_EN_SPAN;op.cmd.opcode = SPINOR_OP_WRAR;op.dummy.nbytes = 0;op.data.dir = SPI_MEM_DATA_OUT;write_enable(nor);ret = spi_nor_read_write_reg(nor, &op, &cr);if (ret < 0) {dev_dbg(nor->dev,"error while writing configuration register\n");return -EINVAL;}/* Read back and check it. */op.data.dir = SPI_MEM_DATA_IN;ret = spi_nor_read_write_reg(nor, &op, &cr);if (ret || !(cr & CR_QUAD_EN_SPAN)) {dev_dbg(nor->dev, "Spansion Quad bit not set\n");return -EINVAL;}- }
- return 0;
+} +#endif
LGTM.
static void spi_nor_set_read_settings(struct spi_nor_read_command *read, u8 num_mode_clocks, @@ -583,6 +662,11 @@ static int spi_nor_init_params(struct spi_nor *nor, spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], 0, 8, SPINOR_OP_READ_FAST, SNOR_PROTO_1_1_1); +#ifdef CONFIG_SPI_FLASH_SPANSION
if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS &&(info->id[1] == 0x2a || info->id[1] == 0x2b))
Add a comment about which flash models these two are.
params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;+#endif }
if (info->flags & SPI_NOR_QUAD_READ) { @@ -659,6 +743,11 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info, case SNOR_MFR_MACRONIX: err = macronix_quad_enable(nor); break; +#endif +#ifdef CONFIG_SPI_FLASH_SPANSION
case SNOR_MFR_CYPRESS:err = spansion_quad_enable_volatile(nor, SZ_128M, 0);break;#endif case SNOR_MFR_ST: case SNOR_MFR_MICRON: -- 2.25.1
Hi Pratyush,
On 2/2/2021 4:40 AM, Pratyush Yadav wrote:
On 28/01/21 01:37PM, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny. The volatile QE bit function, spansion_quad_enable_volatile() supports dual/quad die package parts, by taking 'die_size' parameter that is used to iterate register update for all dies in the device.
I'm not so sure if this is a good idea. spi-nor-tiny should be the minimal set of functionality to get the bootloader to the next stage. 1S-1S-1S mode is sufficient for that. Adding quad enable functions of all the flashes will increase the size quite a bit. I know that some flashes already have their quad enable hooks, and I don't think they should be there either.
Of course, the maintainers have the final call, but from my side,
Nacked-by: Pratyush Yadav p.yadav@ti.com
I understand your point. Let's leave the decision up to the maintainers.
Anyway, comments below in case the maintainers do plan on picking this patch up.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
drivers/mtd/spi/spi-nor-tiny.c | 89 ++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c index 5cc2b7d996..66680df5a9 100644 --- a/drivers/mtd/spi/spi-nor-tiny.c +++ b/drivers/mtd/spi/spi-nor-tiny.c @@ -555,6 +555,85 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor) } #endif /* CONFIG_SPI_FLASH_SPANSION */
+#ifdef CONFIG_SPI_FLASH_SPANSION +/**
- spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
- @nor: pointer to a 'struct spi_nor'
- @die_size: maximum number of bytes per die ('mtd.size' > 'die_size' in
multi die package parts).
- @dummy: number of dummy cycles for register read
- It is recommended to update volatile registers in the field application due
- to a risk of the non-volatile registers corruption by power interrupt. This
- function sets Quad Enable bit in CFR1 volatile.
- Return: 0 on success, -errno otherwise.
- */
+static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 die_size,
u8 dummy)+{
- struct spi_mem_op op =
SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1),SPI_MEM_OP_ADDR(4, 0, 1),SPI_MEM_OP_DUMMY(0, 1),SPI_MEM_OP_DATA_IN(1, NULL, 1));- u32 addr;
- u8 cr;
- int ret;
- /* Use 4-byte address for RDAR/WRAR */
- ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
- if (ret < 0) {
dev_dbg(nor->dev,"error while enabling 4-byte address\n");return ret;- }
- for (addr = 0; addr < nor->mtd.size; addr += die_size) {
op.addr.val = addr + SPINOR_REG_ADDR_CFR1V;So here you add the register offset to the base address, instead of ORing it. Ok.
/* Check current Quad Enable bit value. */op.cmd.opcode = SPINOR_OP_RDAR;op.dummy.nbytes = dummy / 8;op.data.dir = SPI_MEM_DATA_IN;ret = spi_nor_read_write_reg(nor, &op, &cr);if (ret < 0) {dev_dbg(nor->dev,"error while reading configuration register\n");return -EINVAL;}if (cr & CR_QUAD_EN_SPAN)return 0;/* Write new value. */cr |= CR_QUAD_EN_SPAN;op.cmd.opcode = SPINOR_OP_WRAR;op.dummy.nbytes = 0;op.data.dir = SPI_MEM_DATA_OUT;write_enable(nor);ret = spi_nor_read_write_reg(nor, &op, &cr);if (ret < 0) {dev_dbg(nor->dev,"error while writing configuration register\n");return -EINVAL;}/* Read back and check it. */op.data.dir = SPI_MEM_DATA_IN;ret = spi_nor_read_write_reg(nor, &op, &cr);if (ret || !(cr & CR_QUAD_EN_SPAN)) {dev_dbg(nor->dev, "Spansion Quad bit not set\n");return -EINVAL;}- }
- return 0;
+} +#endif
LGTM.
static void spi_nor_set_read_settings(struct spi_nor_read_command *read, u8 num_mode_clocks, @@ -583,6 +662,11 @@ static int spi_nor_init_params(struct spi_nor *nor, spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST], 0, 8, SPINOR_OP_READ_FAST, SNOR_PROTO_1_1_1); +#ifdef CONFIG_SPI_FLASH_SPANSION
if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS &&(info->id[1] == 0x2a || info->id[1] == 0x2b))Add a comment about which flash models these two are.
Will do.
params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;+#endif }
if (info->flags & SPI_NOR_QUAD_READ) { @@ -659,6 +743,11 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info, case SNOR_MFR_MACRONIX: err = macronix_quad_enable(nor); break; +#endif +#ifdef CONFIG_SPI_FLASH_SPANSION
case SNOR_MFR_CYPRESS:err = spansion_quad_enable_volatile(nor, SZ_128M, 0);break;#endif case SNOR_MFR_ST: case SNOR_MFR_MICRON: -- 2.25.1
Best Regards, Takahiro
participants (3)
-
Pratyush Yadav -
Takahiro Kuwano -
tkuw584924＠gmail.com