| Message ID | 6abd3d51cedbc52e25c91eade16fc34e6d188fe3.1611729896.git.Takahiro.Kuwano@infineon.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t | expand |
On 28/01/21 01:37PM, tkuw584924@gmail.com wrote: > From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > > Add nor->setup() and fixup hooks to overwrite: > - volatile QE bit > - the ->ready() hook for dual/quad die package parts > - overlaid erase > - spi_nor_flash_parameter > - mtd_info > > Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > --- > drivers/mtd/spi/spi-nor-core.c | 108 +++++++++++++++++++++++++++++++++ > 1 file changed, 108 insertions(+) > > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c > index ef49328a28..3d8cb9c333 100644 > --- a/drivers/mtd/spi/spi-nor-core.c > +++ b/drivers/mtd/spi/spi-nor-core.c > @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor) > return 0; > } > > +#ifdef CONFIG_SPI_FLASH_SPANSION > +static int s25hx_t_mdp_ready(struct spi_nor *nor) > +{ > + u32 addr; > + int ret; > + > + for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) { > + ret = spansion_sr_ready(nor, addr, 0); > + if (ret != 1) > + return ret; > + } > + > + return 1; > +} > + > +static int s25hx_t_quad_enable(struct spi_nor *nor) > +{ > + u32 addr; > + int ret; > + > + for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) { > + ret = spansion_quad_enable_volatile(nor, addr, 0); So you need to set the QE bit on each die. Ok. Out of curiosity, what will happen if you only set the QE bit on the first die? Will reads from first die work in quad mode and rest in single mode? > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info, > + const struct spi_nor_flash_parameter *params, > + const struct spi_nor_hwcaps *hwcaps) > +{ > +#ifdef CONFIG_SPI_FLASH_BAR > + return -ENOTSUPP; /* Bank Address Register is not supported */ > +#endif > + /* > + * The Cypress Semper family has transparent ECC. To preserve > + * ECC enabled, multi-pass programming within the same 16-byte > + * ECC data unit needs to be avoided. Set writesize to the page > + * size and remove the MTD_BIT_WRITEABLE flag in mtd_info to > + * prevent multi-pass programming. > + */ > + nor->mtd.writesize = params->page_size; The writesize should be set to 16. See [0][1][2]. > + nor->mtd.flags &= ~MTD_BIT_WRITEABLE; Not needed. See discussions pointed to above. > + > + /* Emulate uniform sector architecure by this erase hook*/ > + nor->mtd._erase = spansion_overlaid_erase; > + > + /* For 2Gb (dual die) and 4Gb (quad die) parts */ > + if (nor->mtd.size > SZ_128M) > + nor->ready = s25hx_t_mdp_ready; > + > + /* Enter 4-byte addressing mode for WRAR used in quad_enable */ > + set_4byte(nor, info, true); > + > + return spi_nor_default_setup(nor, info, params, hwcaps); > +} > + > +static void s25hx_t_default_init(struct spi_nor *nor) > +{ > + nor->setup = s25hx_t_setup; > +} > + > +static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor, > + const struct sfdp_parameter_header *header, > + const struct sfdp_bfpt *bfpt, > + struct spi_nor_flash_parameter *params) > +{ > + /* Default page size is 256-byte, but BFPT reports 512-byte */ > + params->page_size = 256; Read the page size from the register, like it is done on Linux for S28 flash family. > + /* Reset erase size in case it is set to 4K from BFPT */ > + nor->mtd.erasesize = 0; What does erasesize of 0 mean? I would take that to mean that the flash does not support erases. I can't find any mention of 0 erase size in the documentation of struct mtd_info. > + > + return 0; > +} > + > +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor, > + struct spi_nor_flash_parameter *params) > +{ > + /* READ_FAST_4B (0Ch) requires mode cycles*/ > + params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8; > + /* PP_1_1_4 is not supported */ > + params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4; > + /* Use volatile register to enable quad */ > + params->quad_enable = s25hx_t_quad_enable; > +} > + > +static struct spi_nor_fixups s25hx_t_fixups = { > + .default_init = s25hx_t_default_init, > + .post_bfpt = s25hx_t_post_bfpt_fixup, > + .post_sfdp = s25hx_t_post_sfdp_fixup, Hmm, I don't think the fixups feature was ever applied to the u-boot tree. I sent a patch for them a while ago [3] but they were never applied due to some issues. I can't find any mentions of "spi_nor_set_fixups" on my 4 day old checkout of Tom's master branch. And that reminds me, the nor->setup() hook you are using is not there either. Your patch series should not even build on upstream u-boot. Please cherry pick the required patches from my series and send them along with yours. Please make sure your patch series builds and works on top of _upstream_ u-boot. Even if it works on your company's fork of U-Boot does not necessarily mean it will work on upstream. > +}; > +#endif > + > static void spi_nor_set_fixups(struct spi_nor *nor) This function is also not present in u-boot master. > { > +#ifdef CONFIG_SPI_FLASH_SPANSION > + if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) { > + switch (nor->info->id[1]) { > + case 0x2a: /* S25HL (QSPI, 3.3V) */ > + case 0x2b: /* S25HS (QSPI, 1.8V) */ > + nor->fixups = &s25hx_t_fixups; > + break; I recall using strcmp() in my series but I guess this should also work just as well. > + > + default: > + break; > + } > + } > +#endif > } > > int spi_nor_scan(struct spi_nor *nor) > -- > 2.25.1 > [0] https://lore.kernel.org/linux-mtd/4c0e3207-72a4-8c1a-5fca-e9f30cc60828@ti.com/ [1] https://lore.kernel.org/linux-mtd/20201201102711.8727-3-p.yadav@ti.com/ [2] https://lore.kernel.org/linux-mtd/20201201102711.8727-4-p.yadav@ti.com/ [3] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav@ti.com/
Hi Pratyush, On 2/2/2021 4:22 AM, Pratyush Yadav wrote: > On 28/01/21 01:37PM, tkuw584924@gmail.com wrote: >> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> >> >> Add nor->setup() and fixup hooks to overwrite: >> - volatile QE bit >> - the ->ready() hook for dual/quad die package parts >> - overlaid erase >> - spi_nor_flash_parameter >> - mtd_info >> >> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> >> --- >> drivers/mtd/spi/spi-nor-core.c | 108 +++++++++++++++++++++++++++++++++ >> 1 file changed, 108 insertions(+) >> >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c >> index ef49328a28..3d8cb9c333 100644 >> --- a/drivers/mtd/spi/spi-nor-core.c >> +++ b/drivers/mtd/spi/spi-nor-core.c >> @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor) >> return 0; >> } >> >> +#ifdef CONFIG_SPI_FLASH_SPANSION >> +static int s25hx_t_mdp_ready(struct spi_nor *nor) >> +{ >> + u32 addr; >> + int ret; >> + >> + for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) { >> + ret = spansion_sr_ready(nor, addr, 0); >> + if (ret != 1) >> + return ret; >> + } >> + >> + return 1; >> +} >> + >> +static int s25hx_t_quad_enable(struct spi_nor *nor) >> +{ >> + u32 addr; >> + int ret; >> + >> + for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) { >> + ret = spansion_quad_enable_volatile(nor, addr, 0); > > So you need to set the QE bit on each die. Ok. > > Out of curiosity, what will happen if you only set the QE bit on the > first die? Will reads from first die work in quad mode and rest in > single mode? > If the host issues quad read command, only the first die works and rest do not respond to the quad read command. >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info, >> + const struct spi_nor_flash_parameter *params, >> + const struct spi_nor_hwcaps *hwcaps) >> +{ >> +#ifdef CONFIG_SPI_FLASH_BAR >> + return -ENOTSUPP; /* Bank Address Register is not supported */ >> +#endif >> + /* >> + * The Cypress Semper family has transparent ECC. To preserve >> + * ECC enabled, multi-pass programming within the same 16-byte >> + * ECC data unit needs to be avoided. Set writesize to the page >> + * size and remove the MTD_BIT_WRITEABLE flag in mtd_info to >> + * prevent multi-pass programming. >> + */ >> + nor->mtd.writesize = params->page_size; > > The writesize should be set to 16. See [0][1][2]. > >> + nor->mtd.flags &= ~MTD_BIT_WRITEABLE; > > Not needed. See discussions pointed to above. > OK, thank you for the information. >> + >> + /* Emulate uniform sector architecure by this erase hook*/ >> + nor->mtd._erase = spansion_overlaid_erase; >> + >> + /* For 2Gb (dual die) and 4Gb (quad die) parts */ >> + if (nor->mtd.size > SZ_128M) >> + nor->ready = s25hx_t_mdp_ready; >> + >> + /* Enter 4-byte addressing mode for WRAR used in quad_enable */ >> + set_4byte(nor, info, true); >> + >> + return spi_nor_default_setup(nor, info, params, hwcaps); >> +} >> + >> +static void s25hx_t_default_init(struct spi_nor *nor) >> +{ >> + nor->setup = s25hx_t_setup; >> +} >> + >> +static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor, >> + const struct sfdp_parameter_header *header, >> + const struct sfdp_bfpt *bfpt, >> + struct spi_nor_flash_parameter *params) >> +{ >> + /* Default page size is 256-byte, but BFPT reports 512-byte */ >> + params->page_size = 256; > > Read the page size from the register, like it is done on Linux for S28 > flash family. > Will fix. >> + /* Reset erase size in case it is set to 4K from BFPT */ >> + nor->mtd.erasesize = 0; > > What does erasesize of 0 mean? I would take that to mean that the flash > does not support erases. I can't find any mention of 0 erase size in the > documentation of struct mtd_info. > In this device, the erasesize is wrongly configured to 4K through BFPT parse. I would reset it to 0 expecting the correct value is set in spi_nor_select_erase() afterwards. But I should simply set correct value in this fixup hook. >> + >> + return 0; >> +} >> + >> +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor, >> + struct spi_nor_flash_parameter *params) >> +{ >> + /* READ_FAST_4B (0Ch) requires mode cycles*/ >> + params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8; >> + /* PP_1_1_4 is not supported */ >> + params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4; >> + /* Use volatile register to enable quad */ >> + params->quad_enable = s25hx_t_quad_enable; >> +} >> + >> +static struct spi_nor_fixups s25hx_t_fixups = { >> + .default_init = s25hx_t_default_init, >> + .post_bfpt = s25hx_t_post_bfpt_fixup, >> + .post_sfdp = s25hx_t_post_sfdp_fixup, > > Hmm, I don't think the fixups feature was ever applied to the u-boot > tree. I sent a patch for them a while ago [3] but they were never > applied due to some issues. I can't find any mentions of > "spi_nor_set_fixups" on my 4 day old checkout of Tom's master branch. > > And that reminds me, the nor->setup() hook you are using is not there > either. Your patch series should not even build on upstream u-boot. > Please cherry pick the required patches from my series and send them > along with yours. > > Please make sure your patch series builds and works on top of _upstream_ > u-boot. Even if it works on your company's fork of U-Boot does not > necessarily mean it will work on upstream. > This patch depends on your patch that introduces the fixups feature. I mentioned it in the changes log section in cover letter only. I will add it into commit description of this patch. >> +}; >> +#endif >> + >> static void spi_nor_set_fixups(struct spi_nor *nor) > > This function is also not present in u-boot master. > >> { >> +#ifdef CONFIG_SPI_FLASH_SPANSION >> + if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) { >> + switch (nor->info->id[1]) { >> + case 0x2a: /* S25HL (QSPI, 3.3V) */ >> + case 0x2b: /* S25HS (QSPI, 1.8V) */ >> + nor->fixups = &s25hx_t_fixups; >> + break; > > I recall using strcmp() in my series but I guess this should also work > just as well. > >> + >> + default: >> + break; >> + } >> + } >> +#endif >> } >> >> int spi_nor_scan(struct spi_nor *nor) >> -- >> 2.25.1 >> > > [0] https://lore.kernel.org/linux-mtd/4c0e3207-72a4-8c1a-5fca-e9f30cc60828@ti.com/ > [1] https://lore.kernel.org/linux-mtd/20201201102711.8727-3-p.yadav@ti.com/ > [2] https://lore.kernel.org/linux-mtd/20201201102711.8727-4-p.yadav@ti.com/ > [3] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav@ti.com/ > Best Regards, Takahiro
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index ef49328a28..3d8cb9c333 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor) return 0; } +#ifdef CONFIG_SPI_FLASH_SPANSION +static int s25hx_t_mdp_ready(struct spi_nor *nor) +{ + u32 addr; + int ret; + + for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) { + ret = spansion_sr_ready(nor, addr, 0); + if (ret != 1) + return ret; + } + + return 1; +} + +static int s25hx_t_quad_enable(struct spi_nor *nor) +{ + u32 addr; + int ret; + + for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) { + ret = spansion_quad_enable_volatile(nor, addr, 0); + if (ret) + return ret; + } + + return 0; +} + +static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info, + const struct spi_nor_flash_parameter *params, + const struct spi_nor_hwcaps *hwcaps) +{ +#ifdef CONFIG_SPI_FLASH_BAR + return -ENOTSUPP; /* Bank Address Register is not supported */ +#endif + /* + * The Cypress Semper family has transparent ECC. To preserve + * ECC enabled, multi-pass programming within the same 16-byte + * ECC data unit needs to be avoided. Set writesize to the page + * size and remove the MTD_BIT_WRITEABLE flag in mtd_info to + * prevent multi-pass programming. + */ + nor->mtd.writesize = params->page_size; + nor->mtd.flags &= ~MTD_BIT_WRITEABLE; + + /* Emulate uniform sector architecure by this erase hook*/ + nor->mtd._erase = spansion_overlaid_erase; + + /* For 2Gb (dual die) and 4Gb (quad die) parts */ + if (nor->mtd.size > SZ_128M) + nor->ready = s25hx_t_mdp_ready; + + /* Enter 4-byte addressing mode for WRAR used in quad_enable */ + set_4byte(nor, info, true); + + return spi_nor_default_setup(nor, info, params, hwcaps); +} + +static void s25hx_t_default_init(struct spi_nor *nor) +{ + nor->setup = s25hx_t_setup; +} + +static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor, + const struct sfdp_parameter_header *header, + const struct sfdp_bfpt *bfpt, + struct spi_nor_flash_parameter *params) +{ + /* Default page size is 256-byte, but BFPT reports 512-byte */ + params->page_size = 256; + /* Reset erase size in case it is set to 4K from BFPT */ + nor->mtd.erasesize = 0; + + return 0; +} + +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) +{ + /* READ_FAST_4B (0Ch) requires mode cycles*/ + params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8; + /* PP_1_1_4 is not supported */ + params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4; + /* Use volatile register to enable quad */ + params->quad_enable = s25hx_t_quad_enable; +} + +static struct spi_nor_fixups s25hx_t_fixups = { + .default_init = s25hx_t_default_init, + .post_bfpt = s25hx_t_post_bfpt_fixup, + .post_sfdp = s25hx_t_post_sfdp_fixup, +}; +#endif + static void spi_nor_set_fixups(struct spi_nor *nor) { +#ifdef CONFIG_SPI_FLASH_SPANSION + if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) { + switch (nor->info->id[1]) { + case 0x2a: /* S25HL (QSPI, 3.3V) */ + case 0x2b: /* S25HS (QSPI, 1.8V) */ + nor->fixups = &s25hx_t_fixups; + break; + + default: + break; + } + } +#endif } int spi_nor_scan(struct spi_nor *nor)