[PATCH v4 8/9] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

On 28/01/21 01:37PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano at 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 at 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/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.