[PATCH] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t
Kuwano Takahiro
tkuw584924 at gmail.com
Mon Sep 28 09:19:43 CEST 2020
Hi Pratyush,
On 9/25/2020 6:43 PM, Pratyush Yadav wrote:
> 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.
>
We don't have such link. I will send the datasheet to your email address.
>> 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?
>
In my understanding, only some of Spansion/Cypress flash parts has non-uniform
so I'm not sure non-uniform support is worth the effort. The S25FL-S family
has 4KB sectors that can be erased by large sector erase command at a time.
I selected this erase hook method because it allows the same usage with the
S25FL-S with small code change.
>> 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/
>
Will fix.
>> + * 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?
>
Yes, 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.
>
You are right. Will fix. Thanks.
>> + }
>> + 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.
>
Will fix.
>> +
>> + /* 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].
>
Thanks for the information. Will apply that.
I think I need nor->setup() hook you ported as well.
>> + }
>> +#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?
>
Since nor->addr_width is set to 4 and read opcode is converted to 4B version
after spi_nor_setup(), reads use 4 byte addresses.
>> +}
>> +#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?
>
This fixup is only applicable for S25. Will add ID check.
>> +#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/
>
--
Best Regards,
Takahiro Kuwano
More information about the U-Boot
mailing list