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

[Takahiro Kuwano]

Hi Pratyush,

On 2/2/2021 4:22 AM, Pratyush Yadav wrote:
> 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 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