[PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 2/24/2021 9:40 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> 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 <Takahiro.Kuwano@infineon.com>
>> ---
>> 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
>>
>