| Message ID | 8f058fc34c7a5e7baa2a7cb10e92a67518717e04.1722917617.git.Takahiro.Kuwano@infineon.com |
|---|---|
| State | New |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | mtd: spi-nor: add generic flash driver | expand |
On 08.08.2024 09:00, tkuw584924@gmail.com wrote: > From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > > Synchronize set_4byte() with Linux v6.10 as much as possible. Let's aim for spi-nor/for-v6.12. > > Introduce {nor, params}->set_4byte_addr_mode(). > The params->set_4byte_addr_mode is initialized with one of the > manufacturer specific methods, copied to nor->set_4byte_addr_mode, > then it is called from spi_nor_set_4byte_addr_mode() renamed from > set_4byte(). this happens indeed. We need to explain why: we aim to split the manufacturer specific code out of the core, thus introduce set_4byte_addr_mode hook so that the manufacturers/SFDP be empowered to pin point a specific 4byte addr mode method. > > There are still manufacturer checks here and there. unfortunately. > And The set_4byte_addr_mode() method in both nor-> and params-> looks > redundant. Those should be cleaned up separately in another patch set. > then don't introduce it in the first place :) > The following commits in Linux are related to this patch. > 64c160f32235 ("mtd: spi-nor: Create a ->set_4byte() method") > 81924dae5194 ("mtd: spi-nor: Emphasise which is the > genericset_4byte_addr_mode() method") > 076aa4eac8b3 ("mtd: spi-nor: core: Move generic method to core - > micron_st_nor_set_4byte_addr_mode") > 288df4378319 ("mtd: spi-nor: core: Update name and description of > micron_st_nor_set_4byte_addr_mode") > f1f1976224f3 ("mtd: spi-nor: core: Update name and description of > spansion_set_4byte_addr_mode") > b6094ac83dd4 ("mtd: spi-nor: core: Introduce > spi_nor_set_4byte_addr_mode()") yeah, these are good for reference. > > Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > --- > drivers/mtd/spi/spi-nor-core.c | 179 +++++++++++++++++++++++++-------- > include/linux/mtd/spi-nor.h | 3 + > 2 files changed, 138 insertions(+), 44 deletions(-) > > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c > index 8d2afaa0e2..d523c045f4 100644 > --- a/drivers/mtd/spi/spi-nor-core.c > +++ b/drivers/mtd/spi/spi-nor-core.c > @@ -680,54 +680,123 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor, > } > #endif /* !CONFIG_SPI_FLASH_BAR */ > > -/* Enable/disable 4-byte addressing mode. */ > -static int set_4byte(struct spi_nor *nor, const struct flash_info *info, > - int enable) > +/** > + * spi_nor_set_4byte_addr_mode_en4b_ex4b() - Enter/Exit 4-byte address mode > + * using SPINOR_OP_EN4B/SPINOR_OP_EX4B. Typically used by > + * Winbond and Macronix. Cypress also uses SPINOR_OP_EN4B > + * to enter, but not SPINOR_OP_EX4B to exit. > + * @nor: pointer to 'struct spi_nor'. > + * @enable: true to enter the 4-byte address mode, false to exit the 4-byte > + * address mode. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, > + bool enable) > { > - int status; > - bool need_wren = false; > - u8 cmd; > + u8 opcode; > + int ret; > > - switch (JEDEC_MFR(info)) { > - case SNOR_MFR_ST: > - case SNOR_MFR_MICRON: > - /* Some Micron need WREN command; all will accept it */ > - need_wren = true; > - fallthrough; > - case SNOR_MFR_ISSI: > - case SNOR_MFR_MACRONIX: > - case SNOR_MFR_WINBOND: > - if (need_wren) > - write_enable(nor); > + if (enable) > + opcode = SPINOR_OP_EN4B; > + else if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) > + opcode = SPINOR_OP_EX4B_CYPRESS; > + else > + opcode = SPINOR_OP_EX4B; > > - cmd = enable ? SPINOR_OP_EN4B : SPINOR_OP_EX4B; > - status = nor->write_reg(nor, cmd, NULL, 0); > - if (need_wren) > - write_disable(nor); > + ret = nor->write_reg(nor, opcode, NULL, 0); > + if (ret) > + return ret; > > - if (!status && !enable && > - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { > - /* > - * On Winbond W25Q256FV, leaving 4byte mode causes > - * the Extended Address Register to be set to 1, so all > - * 3-byte-address reads come from the second 16M. > - * We must clear the register to enable normal behavior. > - */ > - write_enable(nor); > - nor->cmd_buf[0] = 0; > - nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1); > - write_disable(nor); > - } > + /* > + * On Winbond W25Q256FV, leaving 4byte mode causes the Extended please follow linux, and introduce winbond_nor_set_4byte_addr_mode() for the chunk below. > + * Address Register to be set to 1, so all 3-byte-address reads > + * come from the second 16M. We must clear the register to > + * enable normal behavior. > + */ > + if (!enable && JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) { > + ret = write_enable(nor); > + if (ret) > + return ret; > > - return status; > - case SNOR_MFR_CYPRESS: > - cmd = enable ? SPINOR_OP_EN4B : SPINOR_OP_EX4B_CYPRESS; > - return nor->write_reg(nor, cmd, NULL, 0); > - default: > - /* Spansion style */ > - nor->cmd_buf[0] = enable << 7; > - return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1); > + nor->cmd_buf[0] = 0; > + ret = nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, > + 1); > + if (ret) > + return ret; > + > + ret = write_disable(nor); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +/** > + * spi_nor_set_4byte_addr_mode_wren_en4b_ex4b() - Set 4-byte address mode using > + * SPINOR_OP_WREN followed by SPINOR_OP_EN4B or > + * SPINOR_OP_EX4B. Typically used by ST and Micron flashes. > + * @nor: pointer to 'struct spi_nor'. > + * @enable: true to enter the 4-byte address mode, false to exit the 4-byte > + * address mode. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, > + bool enable) > +{ > + int ret; > + > + ret = write_enable(nor); > + if (ret) > + return ret; > + > + ret = spi_nor_set_4byte_addr_mode_en4b_ex4b(nor, enable); > + if (ret) > + return ret; > + > + return write_disable(nor); > +} > + > +/** > + * spi_nor_set_4byte_addr_mode_brwr() - Set 4-byte address mode using > + * SPINOR_OP_BRWR. Typically used by Spansion flashes. > + * @nor: pointer to 'struct spi_nor'. > + * @enable: true to enter the 4-byte address mode, false to exit the 4-byte > + * address mode. > + * > + * 8-bit volatile bank register used to define A[30:A24] bits. MSB (bit[7]) is > + * used to enable/disable 4-byte address mode. When MSB is set to ‘1’, 4-byte > + * address mode is active and A[30:24] bits are don’t care. Write instruction is > + * SPINOR_OP_BRWR(17h) with 1 byte of data. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +static int spi_nor_set_4byte_addr_mode_brwr(struct spi_nor *nor, bool enable) > +{ > + nor->cmd_buf[0] = enable ? BIT(7) : 0; what's wrong with enable << 7? Keep the code as it was, don't update it on the fly. > + return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1); > +} > + > +/* Enable/disable 4-byte addressing mode. */ > +static int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable) > +{ > + int ret; follow linux and do the if (nor->flags & SNOR_F_BROKEN_RESET) check here. You'll have a good debug message for your flashes too ... > + > + ret = nor->set_4byte_addr_mode(nor, enable); > + if (ret) > + return ret; > + > + if (enable) { > + nor->addr_width = 4; > + nor->addr_mode_nbytes = 4; > + } else { > + nor->addr_width = 3; > + nor->addr_mode_nbytes = 3; > } I don't see where were these done previously. Thus don't introduce new code. If needed, make a dedicated patch and explain why you need these. > + > + return 0; > } > > #ifdef CONFIG_SPI_FLASH_SPANSION > @@ -2884,6 +2953,25 @@ static int spi_nor_init_params(struct spi_nor *nor, > } > } > > + /* Select the procedure to enter/exit 4-byte address mode. */ > + switch (JEDEC_MFR(info)) { > + case SNOR_MFR_ST: > + case SNOR_MFR_MICRON: > + params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_wren_en4b_ex4b; > + break; > + > + case SNOR_MFR_ISSI: > + case SNOR_MFR_MACRONIX: > + case SNOR_MFR_WINBOND: > + case SNOR_MFR_CYPRESS: > + params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_en4b_ex4b; > + break; > + > + default: > + params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr; > + break; > + } > + follow linux, you need to set these after parsing SFDP > /* Override the parameters with data read from SFDP tables. */ > nor->addr_width = 0; > nor->mtd.erasesize = 0; > @@ -3272,6 +3360,9 @@ static int spi_nor_default_setup(struct spi_nor *nor, > else > nor->quad_enable = NULL; > > + /* Enter/Exit 4-byte address mode */ > + nor->set_4byte_addr_mode = params->set_4byte_addr_mode; you won't need nor->set_4byte_addr_mode, don't introduce it. As a general comment, follow linux, just move code, don't introduce new code. No changes in functionality shall be expected with this rework. Looking good, thanks for working on this. Cheers, ta
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 8d2afaa0e2..d523c045f4 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -680,54 +680,123 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor, } #endif /* !CONFIG_SPI_FLASH_BAR */ -/* Enable/disable 4-byte addressing mode. */ -static int set_4byte(struct spi_nor *nor, const struct flash_info *info, - int enable) +/** + * spi_nor_set_4byte_addr_mode_en4b_ex4b() - Enter/Exit 4-byte address mode + * using SPINOR_OP_EN4B/SPINOR_OP_EX4B. Typically used by + * Winbond and Macronix. Cypress also uses SPINOR_OP_EN4B + * to enter, but not SPINOR_OP_EX4B to exit. + * @nor: pointer to 'struct spi_nor'. + * @enable: true to enter the 4-byte address mode, false to exit the 4-byte + * address mode. + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, + bool enable) { - int status; - bool need_wren = false; - u8 cmd; + u8 opcode; + int ret; - switch (JEDEC_MFR(info)) { - case SNOR_MFR_ST: - case SNOR_MFR_MICRON: - /* Some Micron need WREN command; all will accept it */ - need_wren = true; - fallthrough; - case SNOR_MFR_ISSI: - case SNOR_MFR_MACRONIX: - case SNOR_MFR_WINBOND: - if (need_wren) - write_enable(nor); + if (enable) + opcode = SPINOR_OP_EN4B; + else if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) + opcode = SPINOR_OP_EX4B_CYPRESS; + else + opcode = SPINOR_OP_EX4B; - cmd = enable ? SPINOR_OP_EN4B : SPINOR_OP_EX4B; - status = nor->write_reg(nor, cmd, NULL, 0); - if (need_wren) - write_disable(nor); + ret = nor->write_reg(nor, opcode, NULL, 0); + if (ret) + return ret; - if (!status && !enable && - JEDEC_MFR(info) == SNOR_MFR_WINBOND) { - /* - * On Winbond W25Q256FV, leaving 4byte mode causes - * the Extended Address Register to be set to 1, so all - * 3-byte-address reads come from the second 16M. - * We must clear the register to enable normal behavior. - */ - write_enable(nor); - nor->cmd_buf[0] = 0; - nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, 1); - write_disable(nor); - } + /* + * On Winbond W25Q256FV, leaving 4byte mode causes the Extended + * Address Register to be set to 1, so all 3-byte-address reads + * come from the second 16M. We must clear the register to + * enable normal behavior. + */ + if (!enable && JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) { + ret = write_enable(nor); + if (ret) + return ret; - return status; - case SNOR_MFR_CYPRESS: - cmd = enable ? SPINOR_OP_EN4B : SPINOR_OP_EX4B_CYPRESS; - return nor->write_reg(nor, cmd, NULL, 0); - default: - /* Spansion style */ - nor->cmd_buf[0] = enable << 7; - return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1); + nor->cmd_buf[0] = 0; + ret = nor->write_reg(nor, SPINOR_OP_WREAR, nor->cmd_buf, + 1); + if (ret) + return ret; + + ret = write_disable(nor); + if (ret) + return ret; + } + + return 0; +} + +/** + * spi_nor_set_4byte_addr_mode_wren_en4b_ex4b() - Set 4-byte address mode using + * SPINOR_OP_WREN followed by SPINOR_OP_EN4B or + * SPINOR_OP_EX4B. Typically used by ST and Micron flashes. + * @nor: pointer to 'struct spi_nor'. + * @enable: true to enter the 4-byte address mode, false to exit the 4-byte + * address mode. + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, + bool enable) +{ + int ret; + + ret = write_enable(nor); + if (ret) + return ret; + + ret = spi_nor_set_4byte_addr_mode_en4b_ex4b(nor, enable); + if (ret) + return ret; + + return write_disable(nor); +} + +/** + * spi_nor_set_4byte_addr_mode_brwr() - Set 4-byte address mode using + * SPINOR_OP_BRWR. Typically used by Spansion flashes. + * @nor: pointer to 'struct spi_nor'. + * @enable: true to enter the 4-byte address mode, false to exit the 4-byte + * address mode. + * + * 8-bit volatile bank register used to define A[30:A24] bits. MSB (bit[7]) is + * used to enable/disable 4-byte address mode. When MSB is set to ‘1’, 4-byte + * address mode is active and A[30:24] bits are don’t care. Write instruction is + * SPINOR_OP_BRWR(17h) with 1 byte of data. + * + * Return: 0 on success, -errno otherwise. + */ +static int spi_nor_set_4byte_addr_mode_brwr(struct spi_nor *nor, bool enable) +{ + nor->cmd_buf[0] = enable ? BIT(7) : 0; + return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1); +} + +/* Enable/disable 4-byte addressing mode. */ +static int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable) +{ + int ret; + + ret = nor->set_4byte_addr_mode(nor, enable); + if (ret) + return ret; + + if (enable) { + nor->addr_width = 4; + nor->addr_mode_nbytes = 4; + } else { + nor->addr_width = 3; + nor->addr_mode_nbytes = 3; } + + return 0; } #ifdef CONFIG_SPI_FLASH_SPANSION @@ -2884,6 +2953,25 @@ static int spi_nor_init_params(struct spi_nor *nor, } } + /* Select the procedure to enter/exit 4-byte address mode. */ + switch (JEDEC_MFR(info)) { + case SNOR_MFR_ST: + case SNOR_MFR_MICRON: + params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_wren_en4b_ex4b; + break; + + case SNOR_MFR_ISSI: + case SNOR_MFR_MACRONIX: + case SNOR_MFR_WINBOND: + case SNOR_MFR_CYPRESS: + params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_en4b_ex4b; + break; + + default: + params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr; + break; + } + /* Override the parameters with data read from SFDP tables. */ nor->addr_width = 0; nor->mtd.erasesize = 0; @@ -3272,6 +3360,9 @@ static int spi_nor_default_setup(struct spi_nor *nor, else nor->quad_enable = NULL; + /* Enter/Exit 4-byte address mode */ + nor->set_4byte_addr_mode = params->set_4byte_addr_mode; + return 0; } @@ -3504,7 +3595,7 @@ static int s25_s28_post_bfpt_fixup(struct spi_nor *nor, */ if (params->size > SZ_128M) { if (bfpt->dwords[BFPT_DWORD(16)] & BFPT_DWORD16_EX4B_PWRCYC) { - ret = set_4byte(nor, nor->info, 1); + ret = spi_nor_set_4byte_addr_mode(nor, true); if (ret) return ret; } @@ -3923,7 +4014,7 @@ static int spi_nor_init(struct spi_nor *nor) */ if (nor->flags & SNOR_F_BROKEN_RESET) debug("enabling reset hack; may not recover from unexpected reboots\n"); - set_4byte(nor, nor->info, 1); + return spi_nor_set_4byte_addr_mode(nor, true); } return 0; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 0d37a806c4..f5d26447c2 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -446,6 +446,7 @@ struct spi_nor_flash_parameter { struct spi_nor_pp_command page_programs[SNOR_CMD_PP_MAX]; int (*quad_enable)(struct spi_nor *nor); + int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable); }; /** @@ -528,6 +529,7 @@ struct spi_flash { * @flash_is_unlocked: [FLASH-SPECIFIC] check if a region of the SPI NOR is * completely unlocked * @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode + * @set_4byte_addr_mode:[FLASH-SPECIFIC] enter/exit 4-byte address mode * @octal_dtr_enable: [FLASH-SPECIFIC] enables SPI NOR octal DTR mode. * @ready: [FLASH-SPECIFIC] check if the flash is ready * @dirmap: pointers to struct spi_mem_dirmap_desc for reads/writes. @@ -579,6 +581,7 @@ struct spi_nor { int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*flash_is_unlocked)(struct spi_nor *nor, loff_t ofs, uint64_t len); int (*quad_enable)(struct spi_nor *nor); + int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable); int (*octal_dtr_enable)(struct spi_nor *nor); int (*ready)(struct spi_nor *nor);