[PATCH 1/4] mtd: spi-nor: Rework set_4byte()
Tudor Ambarus
tudor.ambarus at linaro.org
Tue Sep 10 09:27:58 CEST 2024
On 08.08.2024 09:00, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano at 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 at 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
More information about the U-Boot
mailing list