On 28/01/21 01:37PM, tkuw584...@gmail.com wrote:
> From: Takahiro Kuwano <takahiro.kuw...@infineon.com>
>
> Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny.
> The volatile QE bit function, spansion_quad_enable_volatile() supports
> dual/quad die package parts, by taking 'die_size' parameter that is used
> to iterate register update for all dies in the device.
I'm not so sure if this is a good idea. spi-nor-tiny should be the
minimal set of functionality to get the bootloader to the next stage.
1S-1S-1S mode is sufficient for that. Adding quad enable functions of
all the flashes will increase the size quite a bit. I know that some
flashes already have their quad enable hooks, and I don't think they
should be there either.
Of course, the maintainers have the final call, but from my side,
Nacked-by: Pratyush Yadav <p.ya...@ti.com>
Anyway, comments below in case the maintainers do plan on picking this
patch up.
> Signed-off-by: Takahiro Kuwano <takahiro.kuw...@infineon.com>
> ---
> drivers/mtd/spi/spi-nor-tiny.c | 89 ++++++++++++++++++++++++++++++++++
> 1 file changed, 89 insertions(+)
>
> diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
> index 5cc2b7d996..66680df5a9 100644
> --- a/drivers/mtd/spi/spi-nor-tiny.c
> +++ b/drivers/mtd/spi/spi-nor-tiny.c
> @@ -555,6 +555,85 @@ 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'
> + * @die_size: maximum number of bytes per die ('mtd.size' >
> 'die_size' in
> + * multi die package parts).
> + * @dummy: number of dummy cycles for register read
> + *
> + * 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, u32 die_size,
> + u8 dummy)
> +{
> + struct spi_mem_op op =
> + SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1),
> + SPI_MEM_OP_ADDR(4, 0, 1),
> + SPI_MEM_OP_DUMMY(0, 1),
> + SPI_MEM_OP_DATA_IN(1, NULL, 1));
> + u32 addr;
> + u8 cr;
> + int ret;
> +
> + /* Use 4-byte address for RDAR/WRAR */
> + ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
> + if (ret < 0) {
> + dev_dbg(nor->dev,
> + "error while enabling 4-byte address\n");
> + return ret;
> + }
> +
> + for (addr = 0; addr < nor->mtd.size; addr += die_size) {
> + op.addr.val = addr + SPINOR_REG_ADDR_CFR1V;
So here you add the register offset to the base address, instead of
ORing it. Ok.
> +
> + /* Check current Quad Enable bit value. */
> + op.cmd.opcode = SPINOR_OP_RDAR;
> + op.dummy.nbytes = dummy / 8;
> + op.data.dir = SPI_MEM_DATA_IN;
> + ret = spi_nor_read_write_reg(nor, &op, &cr);
> + if (ret < 0) {
> + dev_dbg(nor->dev,
> + "error while reading configuration register\n");
> + return -EINVAL;
> + }
> +
> + if (cr & CR_QUAD_EN_SPAN)
> + return 0;
> +
> + /* Write new value. */
> + cr |= CR_QUAD_EN_SPAN;
> + op.cmd.opcode = SPINOR_OP_WRAR;
> + op.dummy.nbytes = 0;
> + op.data.dir = SPI_MEM_DATA_OUT;
> + 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. */
> + op.data.dir = SPI_MEM_DATA_IN;
> + ret = spi_nor_read_write_reg(nor, &op, &cr);
> + if (ret || !(cr & CR_QUAD_EN_SPAN)) {
> + dev_dbg(nor->dev, "Spansion Quad bit not set\n");
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +#endif
> +
LGTM.
> static void
> spi_nor_set_read_settings(struct spi_nor_read_command *read,
> u8 num_mode_clocks,
> @@ -583,6 +662,11 @@ 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 &&
> + (info->id[1] == 0x2a || info->id[1] == 0x2b))
Add a comment about which flash models these two are.
> + params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
> +#endif
> }
>
> if (info->flags & SPI_NOR_QUAD_READ) {
> @@ -659,6 +743,11 @@ static int spi_nor_setup(struct spi_nor *nor, const
> struct flash_info *info,
> case SNOR_MFR_MACRONIX:
> err = macronix_quad_enable(nor);
> break;
> +#endif
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> + case SNOR_MFR_CYPRESS:
> + err = spansion_quad_enable_volatile(nor, SZ_128M, 0);
> + break;
> #endif
> case SNOR_MFR_ST:
> case SNOR_MFR_MICRON:
> --
> 2.25.1
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
