[PATCH] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t
Pratyush Yadav
p.yadav at ti.com
Fri Sep 25 11:43:26 CEST 2020
Hi,
On 24/09/20 04:43PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>
> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
> The datasheet can be found in https://community.cypress.com/docs/DOC-15165
This link takes me to a registration page where it asks me for my name,
email, company name and country, and asks me to consent to a T&C and a
Privacy Policy. I don't want to provide all this information just to be
able to review this patch. Please provide a link that is not locked
behind a login.
> This device family can be configured to non-uniform sector layout, while
> U-Boot does not support it. To handle this, an erase hook emulates uniform
For my information, why did you not just implement non-uniform erases
like Linux does?
> sector layout. To enable quad mode, using volatile register is recommended
> for safety. And some other fixups for spi_nor_flash_parameter and mtd_info
> are added.
>
> Tested on Xilinx Zynq-7000 FPGA board.
>
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> ---
> drivers/mtd/spi/spi-nor-core.c | 137 +++++++++++++++++++++++++++++++++
> drivers/mtd/spi/spi-nor-ids.c | 24 ++++++
> drivers/mtd/spi/spi-nor-tiny.c | 73 ++++++++++++++++++
> include/linux/mtd/spi-nor.h | 3 +
> 4 files changed, 237 insertions(+)
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 0113e70037..ddb1cb6bcc 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -329,6 +329,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);
>
> @@ -593,6 +594,54 @@ erase_err:
> return ret;
> }
>
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +/*
> + * Erase for Spansioin/Cypress Flash devices that has overlaid 4KB sectors at
Typo. s/Spansioin/Spansion/
> + * 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;
Since I don't have access to the datasheet I'm trying to understand how
the device behaves by looking at the code.
So here you issue an erase of the entire region, but with the regular
erase command...
> +
> + /* 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;
... and then you again issue a erase command but 128k in length this
time with the 4k erase opcode.
The first erase will erase the latter half of the erase block and this
will erase the former half, correct?
> + if (instr->addr < erasesize) {
> + instr_4k.addr = 0;
> + ret = spi_nor_erase(mtd, &instr_4k);
If the erase address is less than the erase block size (256k) then you
set the address to 0. Since the only erase block that starts before 256k
will be the 0th erase block won't the address already be 0 anyway? So in
that case wouldn't checking for `if (instr->addr == 0)` be enough? That
would certainly be less cryptic than this.
> + }
> + if (!ret && instr->addr + instr->len >= mtd->size - erasesize) {
> + instr_4k.addr = mtd->size - instr_4k.len;
> + ret = spi_nor_erase(mtd, &instr_4k);
So there is another set of these 4k sectors at the end of the flash. If
the erase spans over that region you issue an erase for the last 128k.
Ok.
> + }
> +
> + /* 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)
> @@ -1413,6 +1462,60 @@ 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'
> + *
> + * 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)
> +{
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),
> + SPI_MEM_OP_ADDR(nor->addr_width,
> + SPINOR_REG_ADDR_CFR1V, 1),
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_OUT(1, NULL, 1));
> + u8 cr;
> + int ret;
> +
> + /* Check current Quad Enable bit value. */
> + ret = read_cr(nor);
> + if (ret < 0) {
> + dev_dbg(nor->dev,
> + "error while reading configuration register\n");
> + return -EINVAL;
> + }
> +
> + if (ret & CR_QUAD_EN_SPAN)
> + return 0;
> +
> + cr = ret | CR_QUAD_EN_SPAN;
> +
> + 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. */
> + ret = read_cr(nor);
> + if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
> + dev_dbg(nor->dev, "Spansion Quad bit not set\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
Ok. Looks good.
> #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
> /**
> * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
> @@ -2563,6 +2666,40 @@ int spi_nor_scan(struct spi_nor *nor)
> }
> #endif
>
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> + if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS) {
> +#ifdef CONFIG_SPI_FLASH_BAR
> + return -ENOTSUPP; /* Bank Address Register is not supported */
> +#endif
> + /* 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;
> + /* Default page size is 256-byte, but SFDP reports 512-byte */
> + params.page_size = 256;
> + /* Use volatile register to enable quad */
> + params.quad_enable = spansion_quad_enable_volatile;
> +
> + /*
> + * 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.
> + */
> + mtd->writesize = params.page_size;
> + mtd->flags = MTD_CAP_NORFLASH & ~MTD_BIT_WRITEABLE;
MTD_CAP_NORFLASH is already set above. Just unset MTD_BIT_WRITEABLE
here. In case someone adds another bit to the initial value we won't
over-write it here.
> +
> + /* Reset erase size in case it is set to 4K from SFDP */
> + mtd->erasesize = 0;
> + /* Emulate uniform sector architecure by this erase hook*/
> + mtd->_erase = spansion_overlaid_erase;
> +
> + /* Enter 4-byte addressing mode for WRAR used in quad_enable */
> + set_4byte(nor, info, true);
I don't think every Cypress flash would want these settings. For
example, I sent out a series for adding S28 support [0] and I certainly
don't want to enable 4byte addressing because it will be set in 8D mode
later anyway.
So what you need is a flash-specific fixup mechanism here. I ported it
from Linux in the patch [1].
> + }
> +#endif /* CONFIG_SPI_FLASH_SPANSION */
> +
> if (info->flags & USE_FSR)
> nor->flags |= SNOR_F_USE_FSR;
> if (info->flags & SPI_NOR_HAS_TB)
> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
> index 114ebacde1..8faa1f5d52 100644
> --- a/drivers/mtd/spi/spi-nor-ids.c
> +++ b/drivers/mtd/spi/spi-nor-ids.c
> @@ -215,6 +215,30 @@ 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("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) },
Ok. Looks good.
> #endif
> #ifdef CONFIG_SPI_FLASH_SST /* SST */
> /* SST -- large erase sizes are "overlays", "sectors" are 4K */
> diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
> index fa26ea33c8..e878923387 100644
> --- a/drivers/mtd/spi/spi-nor-tiny.c
> +++ b/drivers/mtd/spi/spi-nor-tiny.c
> @@ -544,6 +544,69 @@ 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'
> + *
> + * 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)
> +{
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),
> + SPI_MEM_OP_ADDR(4, SPINOR_REG_ADDR_CFR1V, 1),
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_OUT(1, NULL, 1));
> + u8 cr;
> + int ret;
> +
> + /* Check current Quad Enable bit value. */
> + ret = read_cr(nor);
> + if (ret < 0) {
> + dev_dbg(nor->dev,
> + "error while reading configuration register\n");
> + return -EINVAL;
> + }
> +
> + if (ret & CR_QUAD_EN_SPAN)
> + return 0;
> +
> + cr = ret | CR_QUAD_EN_SPAN;
> +
> + /* Use 4-byte address for WRAR */
> + ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
You enable 4-byte addressing here...
> + if (ret < 0) {
> + dev_dbg(nor->dev,
> + "error while enabling 4-byte address\n");
> + return ret;
> + }
> +
> + 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. */
> + ret = read_cr(nor);
> + if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
> + dev_dbg(nor->dev, "Spansion Quad bit not set\n");
> + return -EINVAL;
> + }
> +
> + return 0;
... but never disable it. And I don't see this patch touch
nor->addr_width anywhere. So won't it break reads because it will use 3
byte addresses when the flash expects 4?
> +}
> +#endif
> +
> struct spi_nor_read_command {
> u8 num_mode_clocks;
> u8 num_wait_states;
> @@ -594,6 +657,10 @@ 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)
> + params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
This again will set it for all Spansion flashes and not just S25. But I
don't think we should introduce fixup hooks in spi-nor-tiny. So are you
sure this is the right thing to do for _all_ Spansion flashes? If not,
maybe you can check the flash ID and only set it for S25 flashes?
> +#endif
> }
>
> if (info->flags & SPI_NOR_QUAD_READ) {
> @@ -675,6 +742,12 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
> case SNOR_MFR_MICRON:
> break;
>
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> + case SNOR_MFR_CYPRESS:
> + err = spansion_quad_enable_volatile(nor);
> + break;
> +#endif
> +
> default:
> #if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
> /* Kept only for backward compatibility purpose. */
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 233fdc341a..83d13ebe66 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
> @@ -120,6 +121,8 @@
> #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_WRAR 0x71 /* Write any register */
> +#define SPINOR_REG_ADDR_CFR1V 0x00800002
>
> /* Used for Micron flashes only. */
> #define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */
> --
> 2.25.1
>
[0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-21-p.yadav@ti.com/
[1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav@ti.com/
--
Regards,
Pratyush Yadav
Texas Instruments India
More information about the U-Boot
mailing list