Hi Pratyush, On 2/2/2021 4:40 AM, Pratyush Yadav wrote: > 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> > I understand your point. Let's leave the decision up to the maintainers.
> 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. > Will do. >> + 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 >> > Best Regards, Takahiro