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

[Pratyush Yadav]

On 21/02/22 04:14PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> 
> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
> 
> For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
> uniform sector sizes are supported. 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 v8:
>   - Call write_disable in error case only
>   - Use spi_nor_read_reg() helper
>   - Use nor->bouncebuf instead of variable on stack
>   - Update ID table to use FLAGS macro
>   
> Changes in v7:
>   - Add missing device info table in v6
>   
> Changes in v6:
>   - Remove 2Gb multi die pacakge support
> 
> Changes in v5:
>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
> 
> Changes in v4:
>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
> 
> 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 | 103 +++++++++++++++++++++++++++++++++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 5453b89a1c22..4511c0cfd090 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -166,6 +166,93 @@ static int spansion_quad_enable_volatile(struct spi_nor *nor, u8 reg_dummy)
>  	return 0;
>  }
>  
> +static int s25hx_t_quad_enable(struct spi_nor *nor)
> +{
> +	int ret = spansion_quad_enable_volatile(nor, 0);
> +
> +	/* Reset WEL bit in any error cases */
> +	if (ret)
> +		spi_nor_write_disable(nor);
> +
> +	return ret;
> +}
> +
> +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_mem_op op;
> +	int ret;
> +
> +	ret = spi_nor_set_4byte_addr_mode(nor, true);
> +	if (ret)
> +		return ret;
> +	nor->addr_width = 4;

Why do you enable 4 byte address mode here? Why not set 
nor->params->set_4byte_addr_mode and let spi_nor_init() do it?

> +
> +	/* Replace Quad Enable with volatile version */
> +	nor->params->quad_enable = s25hx_t_quad_enable;
> +
> +	/*
> +	 * From BFPT, the page_size is set to 512B(s25hs512t) or 256B(others),
> +	 * but it actually depends on the configuration register. Look up the
> +	 * CFR3V and determine the page_size.
> +	 */
> +	op = (struct spi_mem_op)
> +		SPI_NOR_SPANSION_RD_ANY_REG_OP(nor->addr_width,
> +					       SPINOR_REG_CYPRESS_CFR3V, 0, 1,
> +					       nor->bouncebuf);
> +	ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
> +	if (ret)
> +		return ret;
> +
> +	if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
> +		nor->params->page_size = 512;
> +	else
> +		nor->params->page_size = 256;

Ok.

> +
> +	return 0;
> +}
> +
> +/**
> + * s25hx_t_post_sfdp_fixups() - post SFDP fixups for S25HL-T and S25HS-T
> + * @nor:	pointer to a 'struct spi_nor'
> + *
> + * For the single-die package parts (512Mb and 1Gb), only bottom and uniform
> + * sector maps are correctly populated in the erase_map structure. This is due
> + * to missing or incorrect entries in SMPT. The table below shows all possible
> + * combinations of related register bits and its availability in SMPT. Since
> + * bottom (factorly default) and uniform are commonly used, we don't fix the
> + * erase_map for top and split.
> + *
> + *   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
> + *  -------------------------------------------------------------------
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +void s25hx_t_post_sfdp_fixups(struct spi_nor *nor)
> +{
> +	/* Fast Read 4B requires mode cycles but SFDP does not advertise it */

SFDP does not advertise it as in SFDP table has an entry for this but 
the flash does not populate it correctly? Or does the SFDP table itself 
not supposed to have this information?

This hook should only be used for correcting SFDP information. For 
information that is not defined via SFDP, please use the late_init() 
hook.

> +	nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
> +
> +	/* The writesize should be ECC data unit size */
> +	nor->params->writesize = 16;

Same.

> +}
> +
> +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'
> @@ -353,6 +440,22 @@ static const struct flash_info spansion_parts[] = {
>  	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512)
>  		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>  		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
> +	{ "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256)
> +		FLAGS(USE_CLSR)
> +		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)

This is the old way of specifying flags. We changed this recently [0]. 
Please use just PARSE_SFDP flag, and let SFDP parser discover these 
capabilities. Only use flags for capabilities that can't be discovered 
via SFDP.

Same for other flashes.

> +		.fixups = &s25hx_t_fixups },
> +	{ "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512)
> +		FLAGS(USE_CLSR)
> +		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +		.fixups = &s25hx_t_fixups },
> +	{ "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256)
> +		FLAGS(USE_CLSR)
> +		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +		.fixups = &s25hx_t_fixups },
> +	{ "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512)
> +		FLAGS(USE_CLSR)
> +		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +		.fixups = &s25hx_t_fixups },
>  	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1)
>  		FLAGS(SPI_NOR_NO_ERASE) },
>  	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256)
> -- 
> 2.25.1
> 

[0] https://patchwork.ozlabs.org/project/linux-mtd/patch/20211207140254.87681-7-tudor.ambarus@microchip.com/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.