[PATCH] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t

Kuwano Takahiro tkuw584924 at gmail.com
Mon Sep 28 09:19:43 CEST 2020


Hi Pratyush,

On 9/25/2020 6:43 PM, Pratyush Yadav wrote:
> Hi,
> 
> On 24/09/20 04:43PM, 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.
>> The datasheet can be found in https://community.cypress.com/docs/DOC-15165
> 
> This link takes me to a registration page where it asks me for my name, 
> email, company name and country, and asks me to consent to a T&C and a 
> Privacy Policy. I don't want to provide all this information just to be 
> able to review this patch. Please provide a link that is not locked 
> behind a login.
>

We don't have such link. I will send the datasheet to your email address.
  
>> This device family can be configured to non-uniform sector layout, while
>> U-Boot does not support it. To handle this, an erase hook emulates uniform
> 
> For my information, why did you not just implement non-uniform erases 
> like Linux does?
> 

In my understanding, only some of Spansion/Cypress flash parts has non-uniform
so I'm not sure non-uniform support is worth the effort. The S25FL-S family
has 4KB sectors that can be erased by large sector erase command at a time.
I selected this erase hook method because it allows the same usage with the
S25FL-S with small code change. 

>> sector layout. To enable quad mode, using volatile register is recommended
>> for safety. And some other fixups for spi_nor_flash_parameter and mtd_info
>> are added.
>>
>> Tested on Xilinx Zynq-7000 FPGA board.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 137 +++++++++++++++++++++++++++++++++
>>  drivers/mtd/spi/spi-nor-ids.c  |  24 ++++++
>>  drivers/mtd/spi/spi-nor-tiny.c |  73 ++++++++++++++++++
>>  include/linux/mtd/spi-nor.h    |   3 +
>>  4 files changed, 237 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 0113e70037..ddb1cb6bcc 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -329,6 +329,7 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>  	case SNOR_MFR_ISSI:
>>  	case SNOR_MFR_MACRONIX:
>>  	case SNOR_MFR_WINBOND:
>> +	case SNOR_MFR_CYPRESS:
>>  		if (need_wren)
>>  			write_enable(nor);
>>  
>> @@ -593,6 +594,54 @@ erase_err:
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/*
>> + * Erase for Spansioin/Cypress Flash devices that has overlaid 4KB sectors at
> 
> Typo. s/Spansioin/Spansion/
> 

Will fix.

>> + * the top and/or bottom.
>> + */
>> +static int spansion_overlaid_erase(struct mtd_info *mtd,
>> +				   struct erase_info *instr)
>> +{
>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +	struct erase_info instr_4k;
>> +	u8 opcode;
>> +	u32 erasesize;
>> +	int ret;
>> +
>> +	/* Perform default erase operation (non-overlaid portion is erased) */
>> +	ret = spi_nor_erase(mtd, instr);
>> +	if (ret)
>> +		return ret;
> 
> Since I don't have access to the datasheet I'm trying to understand how 
> the device behaves by looking at the code.
> 
> So here you issue an erase of the entire region, but with the regular 
> erase command...
> 
>> +
>> +	/* Backup default erase opcode and size */
>> +	opcode = nor->erase_opcode;
>> +	erasesize = mtd->erasesize;
>> +
>> +	/*
>> +	 * Erase 4KB sectors. Use the possible max length of 4KB sector region.
>> +	 * The Flash just ignores the command if the address is not configured
>> +	 * as 4KB sector and reports ready status immediately.
>> +	 */
>> +	instr_4k.len = SZ_128K;
>> +	nor->erase_opcode = SPINOR_OP_BE_4K_4B;
>> +	mtd->erasesize = SZ_4K;
> 
> ... and then you again issue a erase command but 128k in length this 
> time with the 4k erase opcode.
> 
> The first erase will erase the latter half of the erase block and this 
> will erase the former half, correct?
>

Yes, correct.
 
>> +	if (instr->addr < erasesize) {
>> +		instr_4k.addr = 0;
>> +		ret = spi_nor_erase(mtd, &instr_4k);
> 
> If the erase address is less than the erase block size (256k) then you 
> set the address to 0. Since the only erase block that starts before 256k 
> will be the 0th erase block won't the address already be 0 anyway? So in 
> that case wouldn't checking for `if (instr->addr == 0)` be enough? That 
> would certainly be less cryptic than this.
>

You are right. Will fix. Thanks.
 
>> +	}
>> +	if (!ret && instr->addr + instr->len >= mtd->size - erasesize) {
>> +		instr_4k.addr = mtd->size - instr_4k.len;
>> +		ret = spi_nor_erase(mtd, &instr_4k);
> 
> So there is another set of these 4k sectors at the end of the flash. If 
> the erase spans over that region you issue an erase for the last 128k. 
> Ok.
> 
>> +	}
>> +
>> +	/* Restore erase opcode and size */
>> +	nor->erase_opcode = opcode;
>> +	mtd->erasesize = erasesize;
>> +
>> +	return ret;
>> +}
>> +#endif
>> +
>>  #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
>>  /* Write status register and ensure bits in mask match written values */
>>  static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
>> @@ -1413,6 +1462,60 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + *
>> + * It is recommended to update volatile registers in the field application due
>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>> + * function sets Quad Enable bit in CFR1 volatile.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor)
>> +{
>> +	struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),
>> +				   SPI_MEM_OP_ADDR(nor->addr_width,
>> +						   SPINOR_REG_ADDR_CFR1V, 1),
>> +				   SPI_MEM_OP_NO_DUMMY,
>> +				   SPI_MEM_OP_DATA_OUT(1, NULL, 1));
>> +	u8 cr;
>> +	int ret;
>> +
>> +	/* Check current Quad Enable bit value. */
>> +	ret = read_cr(nor);
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while reading configuration register\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (ret & CR_QUAD_EN_SPAN)
>> +		return 0;
>> +
>> +	cr = ret | CR_QUAD_EN_SPAN;
>> +
>> +	write_enable(nor);
>> +
>> +	ret = spi_nor_read_write_reg(nor, &op, &cr);
>> +
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while writing configuration register\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Read back and check it. */
>> +	ret = read_cr(nor);
>> +	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
>> +		dev_dbg(nor->dev, "Spansion Quad bit not set\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 
> Ok. Looks good.
> 
>>  #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
>>  /**
>>   * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
>> @@ -2563,6 +2666,40 @@ int spi_nor_scan(struct spi_nor *nor)
>>  	}
>>  #endif
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +	if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS) {
>> +#ifdef CONFIG_SPI_FLASH_BAR
>> +		return -ENOTSUPP; /* Bank Address Register is not supported */
>> +#endif
>> +		/* 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;
>> +		/* Default page size is 256-byte, but SFDP reports 512-byte */
>> +		params.page_size = 256;
>> +		/* Use volatile register to enable quad */
>> +		params.quad_enable = spansion_quad_enable_volatile;
>> +
>> +		/*
>> +		 * 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.
>> +		 */
>> +		mtd->writesize = params.page_size;
>> +		mtd->flags = MTD_CAP_NORFLASH & ~MTD_BIT_WRITEABLE;
> 
> MTD_CAP_NORFLASH is already set above. Just unset MTD_BIT_WRITEABLE 
> here. In case someone adds another bit to the initial value we won't 
> over-write it here.
> 

Will fix.

>> +
>> +		/* Reset erase size in case it is set to 4K from SFDP */
>> +		mtd->erasesize = 0;
>> +		/* Emulate uniform sector architecure by this erase hook*/
>> +		mtd->_erase = spansion_overlaid_erase;
>> +
>> +		/* Enter 4-byte addressing mode for WRAR used in quad_enable */
>> +		set_4byte(nor, info, true);
> 
> I don't think every Cypress flash would want these settings. For 
> example, I sent out a series for adding S28 support [0] and I certainly 
> don't want to enable 4byte addressing because it will be set in 8D mode 
> later anyway.
> 
> So what you need is a flash-specific fixup mechanism here. I ported it 
> from Linux in the patch [1].
> 

Thanks for the information. Will apply that.
I think I need nor->setup() hook you ported as well.

>> +	}
>> +#endif /* CONFIG_SPI_FLASH_SPANSION */
>> +
>>  	if (info->flags & USE_FSR)
>>  		nor->flags |= SNOR_F_USE_FSR;
>>  	if (info->flags & SPI_NOR_HAS_TB)
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index 114ebacde1..8faa1f5d52 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -215,6 +215,30 @@ const struct flash_info spi_nor_ids[] = {
>>  	{ INFO("s25fl208k",  0x014014,      0,  64 * 1024,  16, SECT_4K | SPI_NOR_DUAL_READ) },
>>  	{ INFO("s25fl064l",  0x016017,      0,  64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>  	{ INFO("s25fl128l",  0x016018,      0,  64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> +
>> +	/* S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
>> +	 * sectors at top and/or bottom, depending on the device configuration.
>> +	 * To support this, an erase hook makes overlaid sectors appear as
>> +	 * uniform sectors.
>> +	 */
>> +	{ INFO6("s25hl256t",  0x342a19, 0x0f0390, 256 * 1024, 128,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hl512t",  0x342a1a, 0x0f0390, 256 * 1024, 256,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hl01gt",  0x342a1b, 0x0f0390, 256 * 1024, 512,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hs256t",  0x342b19, 0x0f0390, 256 * 1024, 128,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hs512t",  0x342b1a, 0x0f0390, 256 * 1024, 256,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hs01gt",  0x342b1b, 0x0f0390, 256 * 1024, 512,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
> 
> Ok. Looks good.
> 
>>  #endif
>>  #ifdef CONFIG_SPI_FLASH_SST		/* SST */
>>  	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
>> diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
>> index fa26ea33c8..e878923387 100644
>> --- a/drivers/mtd/spi/spi-nor-tiny.c
>> +++ b/drivers/mtd/spi/spi-nor-tiny.c
>> @@ -544,6 +544,69 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
>>  }
>>  #endif /* CONFIG_SPI_FLASH_SPANSION */
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + *
>> + * It is recommended to update volatile registers in the field application due
>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>> + * function sets Quad Enable bit in CFR1 volatile.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor)
>> +{
>> +	struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),
>> +				   SPI_MEM_OP_ADDR(4, SPINOR_REG_ADDR_CFR1V, 1),
>> +				   SPI_MEM_OP_NO_DUMMY,
>> +				   SPI_MEM_OP_DATA_OUT(1, NULL, 1));
>> +	u8 cr;
>> +	int ret;
>> +
>> +	/* Check current Quad Enable bit value. */
>> +	ret = read_cr(nor);
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while reading configuration register\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (ret & CR_QUAD_EN_SPAN)
>> +		return 0;
>> +
>> +	cr = ret | CR_QUAD_EN_SPAN;
>> +
>> +	/* Use 4-byte address for WRAR */
>> +	ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
> 
> You enable 4-byte addressing here...
> 
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while enabling 4-byte address\n");
>> +		return ret;
>> +	}
>> +
>> +	write_enable(nor);
>> +
>> +	ret = spi_nor_read_write_reg(nor, &op, &cr);
>> +
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while writing configuration register\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Read back and check it. */
>> +	ret = read_cr(nor);
>> +	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
>> +		dev_dbg(nor->dev, "Spansion Quad bit not set\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
> 
> ... but never disable it. And I don't see this patch touch 
> nor->addr_width anywhere. So won't it break reads because it will use 3 
> byte addresses when the flash expects 4?
> 

Since nor->addr_width is set to 4 and read opcode is converted to 4B version
after spi_nor_setup(), reads use 4 byte addresses.

>> +}
>> +#endif
>> +
>>  struct spi_nor_read_command {
>>  	u8			num_mode_clocks;
>>  	u8			num_wait_states;
>> @@ -594,6 +657,10 @@ static int spi_nor_init_params(struct spi_nor *nor,
>>  		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
>>  					  0, 8, SPINOR_OP_READ_FAST,
>>  					  SNOR_PROTO_1_1_1);
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +		if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS)
>> +			params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
> 
> This again will set it for all Spansion flashes and not just S25. But I 
> don't think we should introduce fixup hooks in spi-nor-tiny. So are you 
> sure this is the right thing to do for _all_ Spansion flashes? If not, 
> maybe you can check the flash ID and only set it for S25 flashes?
> 

This fixup is only applicable for S25. Will add ID check.

>> +#endif
>>  	}
>>  
>>  	if (info->flags & SPI_NOR_QUAD_READ) {
>> @@ -675,6 +742,12 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
>>  		case SNOR_MFR_MICRON:
>>  			break;
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +		case SNOR_MFR_CYPRESS:
>> +			err = spansion_quad_enable_volatile(nor);
>> +			break;
>> +#endif
>> +
>>  		default:
>>  #if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
>>  			/* Kept only for backward compatibility purpose. */
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 233fdc341a..83d13ebe66 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -27,6 +27,7 @@
>>  #define SNOR_MFR_SPANSION	CFI_MFR_AMD
>>  #define SNOR_MFR_SST		CFI_MFR_SST
>>  #define SNOR_MFR_WINBOND	0xef /* Also used by some Spansion */
>> +#define SNOR_MFR_CYPRESS	0x34
>>  
>>  /*
>>   * Note on opcode nomenclature: some opcodes have a format like
>> @@ -120,6 +121,8 @@
>>  #define SPINOR_OP_BRWR		0x17	/* Bank register write */
>>  #define SPINOR_OP_BRRD		0x16	/* Bank register read */
>>  #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
>> +#define SPINOR_OP_WRAR		0x71	/* Write any register */
>> +#define SPINOR_REG_ADDR_CFR1V	0x00800002
>>  
>>  /* Used for Micron flashes only. */
>>  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
>> -- 
>> 2.25.1
>>
> 
> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-21-p.yadav@ti.com/
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav@ti.com/
> 

-- 
Best Regards,
Takahiro Kuwano


More information about the U-Boot mailing list