Re: [PATCH v3 3/6] mtd: spi-nor: spansion: Add support for Read/Write Any Register

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 3/15/2021 8:27 PM, Pratyush Yadav wrote:
> On 12/03/21 06:44PM, tkuw584924@gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Some of Spansion/Cypress chips support Read/Write Any Register commands.
>> These commands are mainly used to write volatile registers and access to
>> the registers in second and subsequent die for multi-die package parts.
>>
>> The Read Any Register instruction (65h) is followed by register address
>> and dummy cycles, then the selected register byte is returned.
>>
>> The Write Any Register instruction (71h) is followed by register address
>> and register byte to write.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v3:
>>   - Cleanup implementation
>>   
>>  drivers/mtd/spi-nor/spansion.c | 102 +++++++++++++++++++++++++++++++++
>>  1 file changed, 102 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index b0c5521c1e27..1bce95cb7896 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -19,6 +19,108 @@
>>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
>>  #define SPINOR_OP_CYPRESS_RD_FAST		0xee
>>  
>> +/**
>> + * spansion_read_any_reg() - Read Any Register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @reg_addr:	register address
>> + * @reg_dummy:	number of dummy cycles for register read
>> + * @reg_val:	pointer to a buffer where the register value is copied into
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_read_any_reg(struct spi_nor *nor, u32 reg_addr,
>> +				 u8 reg_dummy, u8 *reg_val)
>> +{
>> +	ssize_t ret;
>> +
>> +	if (nor->spimem) {
>> +		struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 0),
>> +				   SPI_MEM_OP_ADDR(nor->addr_width, reg_addr, 0),
>> +				   SPI_MEM_OP_DUMMY((reg_dummy/8), 0),
> 
> This is only correct when dummy buswidth is 1. It will lead to some very 
> nasty and hard-to-catch bugs when someone uses it for a flash that has a 
> different dummy buswidth. See how dummy nbytes are calculated in 
> spi_nor_spimem_read_data().
> 
Yes, I will fix in v4.

>> +				   SPI_MEM_OP_DATA_IN(1, reg_val, 0));
>> +
>> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
>> +
>> +		ret = spi_mem_exec_op(nor->spimem, &op);
>> +	} else {
>> +		enum spi_nor_protocol proto = nor->read_proto;
>> +		u8 opcode = nor->read_opcode;
>> +		u8 dummy = nor->read_dummy;
>> +
>> +		nor->read_opcode = SPINOR_OP_RD_ANY_REG;
>> +		nor->read_dummy = reg_dummy;
>> +		nor->read_proto = nor->reg_proto;
>> +
>> +		ret = nor->controller_ops->read(nor, reg_addr, 1, reg_val);
>> +
>> +		nor->read_opcode = opcode;
>> +		nor->read_dummy = dummy;
>> +		nor->read_proto = proto;
>> +
>> +		if (ret == 1)
>> +			return ret;
>> +		if (ret != 1)
>> +			return -EIO;
>> +
>> +		ret = 0;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * spansion_write_any_reg() - Write Any Register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @reg_addr:	register address
>> + * @reg_val:	register value to be written
>> + *
>> + * Volatile register write will be effective immediately after the operation so
>> + * this function does not poll the status.
> 
> The same opcode can be used to write to non-volatile registers as well, 
> you just need to change the address passed. It is not recommended to 
> write to non-volatile registers in SPI NOR though. Maybe add "(should be 
> a volatile register)" after "register address" above?
> 
Yes, it is better to add such comment.

> As for not polling the status, can a volatile register write cause a 
> program error? If so, you should check the status to know if any error 
> bits were set.
> 
No, writing to volatile registers does not cause a program error.

Best Regards,
Takahiro


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