From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takahiro Kuwano Date: Mon, 8 Mar 2021 17:47:47 +0900 Subject: [PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t In-Reply-To: <20210224124043.4cwamgxb5lt3ibgu@ti.com> References: <20210224124043.4cwamgxb5lt3ibgu@ti.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 2/24/2021 9:40 PM, Pratyush Yadav wrote: > On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote: >> From: Takahiro Kuwano >> >> This patch adds Flash specific fixups and hooks for Cypress >> S25HL-T/S25HS-T family, based on the features introduced in [0][1][2]. > > Instead of linking the patches like this, it would be a better idea to > simply include them in your series. This will make your series > independent from mine and will make it easier for the maintainer to > apply it. > Noted with thanks. >> >> The nor->ready() and spansion_sr_ready() introduced in #5 and #6 in this >> series are used for multi-die package parts. > > Nitpick: Once this patch makes it into the Git history there is no patch > #5 or #6. Probably a better idea to just say "introduced earlier" or > something similar. > OK. >> >> The nor->quad_enable() sets the volatile QE bit on each die. >> >> The mtd._erase() is hooked if the device is single-die package and not >> configured to uniform sectors, assuming it is in factory default >> configuration that has 32 x 4KB sectors overlaid on bottom address. >> Other configurations, top and split, are not supported at this point. >> Will submit additional patches to support it as needed. > > Ah, this patch does check for uniform sector map. Nice. I assume you > have tested with both configurations. > Yes. I tested both. >> >> The post_bfpt/sfdp() fixes the params wrongly advertised in SFDP. >> >> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav at ti.com/ >> [1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav at ti.com/ >> [2] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/ >> >> Signed-off-by: Takahiro Kuwano >> --- >> Changes in v5: >> - Add s25hx_t_erase_non_uniform() >> - Change mtd.writesize and mtd.flags in s25hx_t_setup() >> - Fix page size and erase size issues in s25hx_t_post_bfpt_fixup() >> >> drivers/mtd/spi/spi-nor-core.c | 155 +++++++++++++++++++++++++++++++++ >> include/linux/mtd/spi-nor.h | 3 + >> 2 files changed, 158 insertions(+) >> >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c >> index 8d63681cb3..315e26ab27 100644 >> --- a/drivers/mtd/spi/spi-nor-core.c >> +++ b/drivers/mtd/spi/spi-nor-core.c >> @@ -2677,8 +2677,163 @@ 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) > > Nitpick: Use if (!ret) since spansion_sr_ready() returns a boolean > value. > Will fix. >> + 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_erase_non_uniform(struct mtd_info *mtd, >> + struct erase_info *instr) >> +{ >> + /* Support factory default configuration (32 x 4KB sectors at bottom) */ >> + return spansion_erase_non_uniform(mtd, instr, 0, SZ_128K); >> +} >> + >> +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. >> + */ >> + nor->mtd.writesize = 16; > > Sorry, I forgot what your end goal was with setting the writesize. Which > layer is doing multi-pass writes? You would need to check if those > layers actually respect this value. In Linux some consumers of the MTD > layer simply assumed the writesize for a NOR flash is 1 (like UBI for > example). I had to patch them before everything was back to normal. > My goal so far is enabling access this Flash via cmd/sf that does not respect the writesize. I think I can remove this. >> + >> + if (nor->mtd.size > SZ_128M) { >> + /* >> + * For the multi-die package parts, the ready() hook is needed >> + * to check all dies' status via read any register. >> + */ >> + nor->ready = s25hx_t_mdp_ready; >> + } else { >> + int ret; >> + u8 cfr3v; >> + >> + /* >> + * Read CFR3V to check if uniform sector is selected. If not, >> + * assign an erase hook that supports non-uniform erase, >> + * assuming the part has factory default configuration, >> + * 32 x 4KB sectors at bottom. >> + */ >> + ret = spansion_read_any_reg(nor, SPINOR_REG_ADDR_CFR3V, 0, >> + &cfr3v); >> + if (ret) >> + return ret; >> + >> + if (!(cfr3v & CFR3V_UNHYSA)) >> + nor->mtd._erase = s25hx_t_erase_non_uniform; > > Multi-die parts can also have uniform sector sizes. Why not do this > check for them as well? > Multi-die parts are configured to uniform by default and this patch assumes they are in default sector size setting. But I have realized I can easily add support for bottom in multi-die parts. Will do it in v6. >> + } >> + >> + return spi_nor_default_setup(nor, info, params, hwcaps); >> +} >> + >> +static void s25hx_t_default_init(struct spi_nor *nor) >> +{ >> + nor->setup = s25hx_t_setup; >> +} > > Ok. > >> + >> +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) >> +{ >> + int ret; >> + u32 addr; >> + u8 cfr3v; >> + >> + /* erase size in case it is set to 4K from BFPT */ >> + nor->erase_opcode = SPINOR_OP_SE_4B; >> + nor->mtd.erasesize = nor->info->sector_size; >> + >> + /* Enter 4-byte addressing mode for Read Any Register */ >> + ret = set_4byte(nor, nor->info, 1); >> + if (ret) >> + return ret; > > You enter 4byte addressing but don't exit it before returning. Is this > intentional? > Yes. The nor->addr_width must be 4 for read/write/erase to work correctly. I think device's addressing mode should be in sync with nor->addr_width. >> + nor->addr_width = 4; >> + >> + /* >> + * The page_size is set to 512B from BFPT, but it actually depends on >> + * the configuration register. Look up the CFR3V and determine the >> + * page_size. For multi-die package parts, use 512B only when the all >> + * dies are configured to 512B buffer. >> + */ >> + for (addr = 0; addr < params->size; addr += SZ_128M) { >> + ret = spansion_read_any_reg(nor, addr + SPINOR_REG_ADDR_CFR3V, >> + 0, &cfr3v); >> + if (ret) >> + return ret; >> + >> + if (!(cfr3v & CFR3V_PGMBUF)) { >> + params->page_size = 256; >> + return 0; >> + } >> + } >> + params->page_size = 512; > > Ok. > >> + >> + 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; > > Ok. > >> + >> + default: >> + break; >> + } >> + } >> +#endif >> } >> >> 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 25234177de..cfb2104fee 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -125,6 +125,9 @@ >> #define SPINOR_OP_WRAR 0x71 /* Write any register */ >> #define SPINOR_REG_ADDR_STR1V 0x00800000 >> #define SPINOR_REG_ADDR_CFR1V 0x00800002 >> +#define SPINOR_REG_ADDR_CFR3V 0x00800004 >> +#define CFR3V_UNHYSA BIT(3) /* Uniform sectors or not */ >> +#define CFR3V_PGMBUF BIT(4) /* Program buffer size */ > > Ok. > >> >> /* Used for Micron flashes only. */ >> #define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */ >> -- >> 2.25.1 >> >