[PATCH v3 6/6] mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups

[Takahiro Kuwano]

Hi Pratyush,

I will fix the typo and merge the comment blocks.

On 3/15/2021 9:15 PM, Pratyush Yadav wrote:
> On 12/03/21 06:45PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>
>> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
>>
>> For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
>> uniform sector sizes are supported. For the multi-die package parts (2Gb),
>> only uniform sector sizes is supprted. This is due to missing or incorrect
>> entries in SMPT. Fixup for other sector sizes configurations will be
>> followed up as needed.
>>
>> Tested on Xilinx Zynq-7000 FPGA board.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> ---
>> Changes in v3:
>>   - Remove S25HL256T and S25HS256T
>>   - Add S25HL02GT and S25HS02GT 
>>   - Add support for multi-die package parts support
>>   - Remove erase_map fix for top/split sector layout
>>   - Set ECC data unit size (16B) to writesize 
>>
>>  drivers/mtd/spi-nor/spansion.c | 119 +++++++++++++++++++++++++++++++++
>>  1 file changed, 119 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 5742212fde2e..05a4ea00acfc 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -225,6 +225,107 @@ static int spansion_mdp_ready(struct spi_nor *nor, u8 reg_dummy, u32 die_size)
>>  	return 1;
>>  }
>>  
>> +static int s25hx_t_quad_enable(struct spi_nor *nor)
>> +{
>> +	return spansion_quad_enable_volatile(nor, 0, SZ_128M);
>> +}
>> +
>> +static int s25hx_t_mdp_ready(struct spi_nor *nor)
>> +{
>> +	return spansion_mdp_ready(nor, 0, SZ_128M);
>> +}
>> +
>> +static int
>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>> +			 const struct sfdp_parameter_header *bfpt_header,
>> +			 const struct sfdp_bfpt *bfpt,
>> +			 struct spi_nor_flash_parameter *params)
>> +{
>> +	int ret;
>> +	u32 addr;
>> +	u8 cfr3v;
>> +
>> +	ret = spi_nor_set_4byte_addr_mode(nor, true);
>> +	if (ret)
>> +		return ret;
>> +	nor->addr_width = 4;
>> +
>> +	/* Replace Quad Enable with volatile version */
>> +	params->quad_enable = s25hx_t_quad_enable;
>> +
>> +	/*
>> +	 * 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_CYPRESS_CFR3V, 0,
>> +					    &cfr3v);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (!(cfr3v & SPINOR_REG_CYPRESS_CFR3V_PGSZ)) {
>> +			params->page_size = 256;
>> +			return 0;
>> +		}
>> +	}
>> +	params->page_size = 512;
>> +
>> +	return 0;
>> +}
>> +
>> +void s25hx_t_post_sfdp_fixups(struct spi_nor *nor)
>> +{
>> +	/*
>> +	 * For the signle-die package parts (512Mb and 1Gb), bottom 4KB and
> 
> Typo. s/signle/single/
> 
>> +	 * uniform sector maps are correctly populated in the erase_map
>> +	 * structure. The table below shows all possible combinations of related
>> +	 * register bits and its availability in SMPT.
>> +	 *
>> +	 *   CFR3[3] | CFR1[6] | CFR1[2] | Sector Map | Available in SMPT?
>> +	 *  -------------------------------------------------------------------
>> +	 *      0    |    0    |    0    | Bottom     | YES
>> +	 *      0    |    0    |    1    | Top        | NO (decoded as Split)
>> +	 *      0    |    1    |    0    | Split      | NO
>> +	 *      0    |    1    |    1    | Split      | NO (decoded as Top)
>> +	 *      1    |    0    |    0    | Uniform    | YES
>> +	 *      1    |    0    |    1    | Uniform    | NO
>> +	 *      1    |    1    |    0    | Uniform    | NO
>> +	 *      1    |    1    |    1    | Uniform    | NO
>> +	 *  -------------------------------------------------------------------
> 
> Neat! This would be very helpful for anyone looking at this flash's SMPT 
> table in the future.
> 
>> +	 */
> 
> Nitpick: It would be good to merge this
> 
>> +
>> +	/* Fast Read 4B requires mode cycles */
>> +	nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>> +
>> +	/* The writesize should be ECC data unit size */
>> +	nor->params->writesize = 16;
>> +
>> +	/* Fixup for multi-die package parts */
>> +	if (nor->params->size > SZ_128M) {
>> +		/*
>> +		 * For the dual-die package parts (2Gb), SMPT parse fails due to
>> +		 * incorrect SMPT entries and the erase map is populated as 4K
>> +		 * uniform that does not supported the parts. So it needs to be
>> +		 * rolled back to 256K uniform that is the factory default of
>> +		 * multi-die package parts.
>> +		 */
> 
> ... and this comment. If the table can't fit due to indentation, at 
> least move it to above the "if" so it is at least somewhat close to 
> where the SMPT is actually being fixed up.
> 
>> +		spi_nor_init_uniform_erase_map(&nor->params->erase_map,
>> +					       BIT(SNOR_ERASE_TYPE_MAX - 1),
>> +					       nor->params->size);
>> +
>> +		/* Need to check status of each die via RDAR command */
>> +		nor->params->ready = s25hx_t_mdp_ready;
>> +	}
>> +}
>> +
>> +static struct spi_nor_fixups s25hx_t_fixups = {
>> +	.post_bfpt = s25hx_t_post_bfpt_fixups,
>> +	.post_sfdp = s25hx_t_post_sfdp_fixups
>> +};
>> +
>>  /**
>>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>   * @nor:		pointer to a 'struct spi_nor'
>> @@ -475,6 +576,24 @@ static const struct flash_info spansion_parts[] = {
>>  	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
>>  			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>  			     SPI_NOR_4B_OPCODES) },
>> +	{ "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hl02gt",  INFO6(0x342a1c, 0x0f0090, 256 * 1024, 1024,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hs02gt",  INFO6(0x342b1c, 0x0f0090, 256 * 1024, 1024,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>>  	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
>>  			      SPI_NOR_NO_ERASE) },
>>  	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,
> 
> Other than the comments above,
> 
> Reviewed-by: Pratyush Yadav <p.yadav at ti.com>
> 

Best Regards,
Takahiro