Hi Pratyush, On 9/25/2020 6:43 PM, Pratyush Yadav wrote: > Hi, > > On 24/09/20 04:43PM, [email protected] wrote: >> From: Takahiro Kuwano <[email protected]> >> >> 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 <[email protected]> >> --- >> 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/[email protected]/ > [1] > https://patchwork.ozlabs.org/project/uboot/patch/[email protected]/ > -- Best Regards, Takahiro Kuwano
