Hi Tudor
> > Hi, > > On 12/29/21 7:56 AM, JaimeLiao wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > Follow patch "f6adec1af4b2f5d3012480c6cdce7743b74a6156" for adding > > Macronix flash in Octal DTR mode. > > > > Enable Octal DTR mode with 20 dummy cycles to allow running at the > > maximum supported frequency. > > > > -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7841/MX25LM51245G,%203V,%20512Mb,%20v1.1.pdf > > > > Signed-off-by: JaimeLiao <jaimeliao...@gmail.com> > > --- > > drivers/mtd/spi/spi-nor-core.c | 83 ++++++++++++++++++++++++++++++++++ > > include/linux/mtd/spi-nor.h | 12 ++++- > > 2 files changed, 93 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c > > index d5d905fa5a..0a6550984b 100644 > > --- a/drivers/mtd/spi/spi-nor-core.c > > +++ b/drivers/mtd/spi/spi-nor-core.c > > @@ -3489,6 +3489,85 @@ static struct spi_nor_fixups mt35xu512aba_fixups = { > > }; > > #endif /* CONFIG_SPI_FLASH_MT35XU */ > > > > +#if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX) > > +/** > > + * spi_nor_macronix_octal_dtr_enable() - set DTR OPI Enable bit in > > Configuration Register 2. > > + * @nor: pointer to a 'struct spi_nor' > > + * > > + * Set the DTR OPI Enable (DOPI) bit in Configuration Register 2. > > + * Bit 2 of Configuration Register 2 is the DOPI bit for Macronix like > > OPI memories. > > + * > > + * Return: 0 on success, -errno otherwise. > > + */ > > +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor) > > This method should be called just for the flashes that do not define the > JESD216 "Command Sequences to Change to Octal DDR (8D-8D-8D) mode" table. > Or where SFDP is skipped intentionally. I think Octal DTR enable method have been defined by manufacturers. It would be better if the method could be parce from SFDP. But I prefer to follow the existing rules for adding the flashes which have been output but not be support in uboot. > > > +{ > > + struct spi_mem_op op; > > + int ret; > > + u8 buf; > > + > > + ret = write_enable(nor); > > + if (ret) > > + return ret; > > + > > + buf = SPINOR_REG_MXIC_DC_20; > > + op = (struct spi_mem_op) > > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1), > > + SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_DC, 1), > > + SPI_MEM_OP_NO_DUMMY, > > + SPI_MEM_OP_DATA_OUT(1, &buf, 1)); > > + > > + ret = spi_mem_exec_op(nor->spi, &op); > > + if (ret) > > + return ret; > > + > > + ret = spi_nor_wait_till_ready(nor); > > + if (ret) > > + return ret; > > + > > + nor->read_dummy = MXIC_MAX_DC; > > + ret = write_enable(nor); > > + if (ret) > > + return ret; > > + > > + buf = SPINOR_REG_MXIC_OPI_DTR_EN; > > + op = (struct spi_mem_op) > > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1), > > + SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1), > > + SPI_MEM_OP_NO_DUMMY, > > + SPI_MEM_OP_DATA_OUT(1, &buf, 1)); > > + > > + ret = spi_mem_exec_op(nor->spi, &op); > > + if (ret) { > > + dev_err(nor->dev, "Failed to enable octal DTR mode\n"); > > + return ret; > > + } > > + nor->reg_proto = SNOR_PROTO_8_8_8_DTR; > > + > > + return 0; > > +} > > + > > +static void macronix_octal_default_init(struct spi_nor *nor) > > I think we should get rid of the default_init() hook, it messes how > the params are initialized, there's a spaghetti right now, it's very hard to > determine who initializes which params solely by reading the code. I would > advise to follow linux and use a late_init() hook where explicit octal dtr > enable method is required. Sure it is a good idea. I hope U-boot could have more similar architecture with Linux kernel. > > > > +{ > > + nor->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable; > > +} > > + > > +static void macronix_octal_post_sfdp_fixup(struct spi_nor *nor, > > + struct spi_nor_flash_parameter > > *params) > > +{ > > + /* > > + * Adding SNOR_HWCAPS_PP_8_8_8_DTR in hwcaps.mask when > > + * SPI_NOR_OCTAL_DTR_READ flag exists. > > + */ > > + if (params->hwcaps.mask & SNOR_HWCAPS_READ_8_8_8_DTR) > > + params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR; > > This confirms the spaghetti theory. This is not SFDP related, why do you use > the post_sfdp hook? Because of SPI_NOR_OCTAL_DTR_PP is not existing in U-boot. I will take other suitable method to enable it next patch. > > > +} > > + > > +static struct spi_nor_fixups macronix_octal_fixups = { > > + .default_init = macronix_octal_default_init, > > + .post_sfdp = macronix_octal_post_sfdp_fixup, > > +}; > > +#endif /* CONFIG_SPI_FLASH_MACRONIX */ > > + > > /** spi_nor_octal_dtr_enable() - enable Octal DTR I/O if needed > > * @nor: pointer to a 'struct spi_nor' > > * > > @@ -3655,6 +3734,10 @@ void spi_nor_set_fixups(struct spi_nor *nor) > > if (!strcmp(nor->info->name, "mt35xu512aba")) > > nor->fixups = &mt35xu512aba_fixups; > > #endif > > + > > +#if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX) > Not related to your patch, but ideally all the macronix code should reside > in a dedicated macronix.c driver. Sure, but all manufacturer IDs are gather in spi-nor-ids.c now. > > Cheers, > ta > > > + nor->fixups = ¯onix_octal_fixups; > > +#endif /* SPI_FLASH_MACRONIX */ > > } > > > > int spi_nor_scan(struct spi_nor *nor) > > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > > index 7ddc4ba2bf..8682368f2f 100644 > > --- a/include/linux/mtd/spi-nor.h > > +++ b/include/linux/mtd/spi-nor.h > > @@ -116,8 +116,16 @@ > > #define XSR_RDY BIT(7) /* Ready */ > > > > /* Used for Macronix and Winbond flashes. */ > > -#define SPINOR_OP_EN4B 0xb7 /* Enter 4-byte mode */ > > -#define SPINOR_OP_EX4B 0xe9 /* Exit 4-byte mode */ > > +#define SPINOR_OP_EN4B 0xb7 /* Enter 4-byte > > mode */ > > +#define SPINOR_OP_EX4B 0xe9 /* Exit 4-byte mode > > */ > > +#define SPINOR_OP_RD_CR2 0x71 /* Read > > configuration register 2 */ > > +#define SPINOR_OP_WR_CR2 0x72 /* Write > > configuration register 2 */ > > +#define SPINOR_OP_MXIC_DTR_RD 0xee /* Fast Read opcode > > in DTR mode */ > > +#define SPINOR_REG_MXIC_CR2_MODE 0x00000000 /* For setting > > octal DTR mode */ > > +#define SPINOR_REG_MXIC_OPI_DTR_EN 0x2 /* Enable Octal DTR > > */ > > +#define SPINOR_REG_MXIC_CR2_DC 0x00000300 /* For setting > > dummy cycles */ > > +#define SPINOR_REG_MXIC_DC_20 0x0 /* Setting dummy > > cycles to 20 */ > > +#define MXIC_MAX_DC 20 /* Maximum value of > > dummy cycles */ > > > > /* Used for Spansion flashes only. */ > > #define SPINOR_OP_BRWR 0x17 /* Bank register write */ > > -- > > 2.17.1 > > > Thanks Jaime