[v4,4/9] mtd: spi-nor-core: Add support for volatile QE bit

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

Some of Spansion/Cypress chips support volatile version of configuration
registers and 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 patch adds a function to set Quad Enable bit in CFR1
volatile.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Hi,

On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Some of Spansion/Cypress chips support volatile version of configuration
> registers and 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 patch adds a function to set Quad Enable bit in CFR1
> volatile.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 2803536ed5..624e730524 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1576,6 +1576,59 @@ 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'
> + * @addr_base:	base address of register (can be >0 in multi-die parts)
> + * @dummy:	number of dummy cycles for register read
> + *
> + * 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, u32 addr_base,
> +					 u8 dummy)
> +{
> +	u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V;

Why do you OR the register offset with the base? Shouldn't you be adding 
to it?

> +
> +	u8 cr;
> +	int ret;
> +
> +	/* Check current Quad Enable bit value. */
> +	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
> +	if (ret < 0) {
> +		dev_dbg(nor->dev,
> +			"error while reading configuration register\n");
> +		return -EINVAL;
> +	}
> +
> +	if (cr & CR_QUAD_EN_SPAN)
> +		return 0;
> +
> +	cr |= CR_QUAD_EN_SPAN;
> +
> +	write_enable(nor);
> +
> +	ret = spansion_write_any_reg(nor, addr, cr);
> +
> +	if (ret < 0) {
> +		dev_dbg(nor->dev,
> +			"error while writing configuration register\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Read back and check it. */
> +	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
> +	if (ret || !(cr & CR_QUAD_EN_SPAN)) {
> +		dev_dbg(nor->dev, "Spansion Quad bit not set\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

Rest of the patch LGTM.

>  #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
>  /**
>   * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
> -- 
> 2.25.1
>

Hi Pratyush,

On 1/30/2021 3:40 AM, Pratyush Yadav wrote:
> Hi,
> 
> On 28/01/21 01:36PM, tkuw584924@gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Some of Spansion/Cypress chips support volatile version of configuration
>> registers and 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 patch adds a function to set Quad Enable bit in CFR1
>> volatile.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 2803536ed5..624e730524 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -1576,6 +1576,59 @@ 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'
>> + * @addr_base:	base address of register (can be >0 in multi-die parts)
>> + * @dummy:	number of dummy cycles for register read
>> + *
>> + * 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, u32 addr_base,
>> +					 u8 dummy)
>> +{
>> +	u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V;
> 
> Why do you OR the register offset with the base? Shouldn't you be adding 
> to it?
> 
I missed it during review... I will fix.


>> +
>> +	u8 cr;
>> +	int ret;
>> +
>> +	/* Check current Quad Enable bit value. */
>> +	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while reading configuration register\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cr & CR_QUAD_EN_SPAN)
>> +		return 0;
>> +
>> +	cr |= CR_QUAD_EN_SPAN;
>> +
>> +	write_enable(nor);
>> +
>> +	ret = spansion_write_any_reg(nor, addr, cr);
>> +
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while writing configuration register\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Read back and check it. */
>> +	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
>> +	if (ret || !(cr & CR_QUAD_EN_SPAN)) {
>> +		dev_dbg(nor->dev, "Spansion Quad bit not set\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 
> Rest of the patch LGTM.
> 
>>  #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
>>  /**
>>   * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro