Re: [PATCH] mtd: spi-nor: Use CLSR command for FL-L chips

[Vignesh Raghavendra <vigneshr@ti.com>]

 Hi, Yaliang

On 1/23/21 11:15 PM, Tudor.Ambarus@microchip.com wrote:
> Hi, Yaliang,
> 
[...]
>> +static int spi_nor_s25fl_l_sr_ready(struct spi_nor *nor)
>> +{
>> +	u8 *sr = nor->bouncebuf;
>> +	int ret;
>> +
>> +	ret = spi_nor_read_sr(nor, sr);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/**
>> +	 * P_ERR and E_ERR bits are located in the Status Register 2
>> +	 * of Cypress FL-L series chips.
>> +	 */
>> +	ret = spi_nor_s25fl_l_read_sr2(nor, &sr[1]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (nor->flags & SNOR_F_USE_CLSR && sr[1] & (SR_E_ERR | SR_P_ERR)) {
> If checking other manufacturer datasheets, would you please check if
> CLSR is used by any other manufacturer?
> 

This is limited to certain subsets of Cypress/Spansion flashes.

>> +		if (sr[1] & SR_E_ERR)
>> +			dev_err(nor->dev, "Erase Error occurred\n");
>> +		else
>> +			dev_err(nor->dev, "Programming Error occurred\n");
>> +
>> +		spi_nor_clear_sr(nor);
>> +
>> +		/*
>> +		 * WEL bit remains set to one when an erase or page program
>> +		 * error occurs. Issue a Write Disable command to protect
>> +		 * against inadvertent writes that can possibly corrupt the
>> +		 * contents of the memory.
>> +		 */
>> +		ret = spi_nor_write_disable(nor);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return -EIO;
>> +	}
>> +
>> +	return !(sr[0] & SR_WIP);
>> +}
>> +
>>  /**
>>   * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>>   * for new commands.
>> @@ -546,7 +628,19 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>>   */
>>  static int spi_nor_sr_ready(struct spi_nor *nor)
>>  {
>> -	int ret = spi_nor_read_sr(nor, nor->bouncebuf);
>> +	int ret;
>> +	const struct flash_info *tmpinfo = nor->info ? nor->info : spi_nor_read_id(nor);
>> +
>> +	if (IS_ERR_OR_NULL(tmpinfo))
>> +		return -ENOENT;
>> +
>> +	if (!strcmp(tmpinfo->name, "s25fl064l")
>> +			|| !strcmp(tmpinfo->name, "s25fl128l")
>> +			|| !strcmp(tmpinfo->name, "s25fl256l")) {
>> +		return spi_nor_s25fl_l_sr_ready(nor);
>> +	}
> 
> No, we can't accept flash name comparisons in the SPI NOR core.
> If CLSR and RDSR2 are just spansion specific, we can provide a sr_ready
> function pointer that can be filled by spansion flashes. Or some other
> method depending on the CLSR and RDSR2 exposure through other
> manufacturers.
> 
> Ideally one would skim through at least 2 - 3 datasheets from each
> manufacturer available in SPI NOR. Preferable each datasheet from
> a different manufacturer family. Unfortunately I'm not aware of any
> standard that describes all the supported SPI NOR commands.
> If you find this overwhelming, I can share the workload with you,
> but at best effort. If you go through this by yourself, please
> save the name of the datasheet flashes that you go through.
> 

Agree with Tudor. There is quite a bit of variation even within
Cypress/Spansion devices. S28 series of flashes have these error bits
within SR1 register. See: https://www.cypress.com/file/513996/download

So, this cannot be in SPI NOR core.

spi_nor_xread_sr(), spi_nor_fsr_ready() and spi_nor_clear_sr() are all
vendor/family specific and should be moved to appropriate vendor
specific drivers.

Would highly appreciate it, if you could fix these up as part of your
current work. Thanks!

Regards
Vignesh

[...]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/