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

[Takahiro Kuwano <tkuw584924@gmail.com>]

Hi Vignesh,

On 5/27/2021 8:06 PM, Vignesh Raghavendra wrote:
> Hi,
> 
> On 4/27/21 12:38 PM, 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>
>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>> Changes in v5:
>>   - Fix 'if (ret == 1)' to 'if (ret < 0)' in spansion_read_any_reg()
>>
>> Changes in v4:
>>   - Fix dummy cycle calculation in spansion_read_any_reg()
>>   - Modify comment for spansion_write_any_reg()
>>   
>> Changes in v3:
>>   - Cleanup implementation
>>
>>  drivers/mtd/spi-nor/spansion.c | 106 +++++++++++++++++++++++++++++++++
>>  1 file changed, 106 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index b0c5521c1e27..436db8933dcf 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -19,6 +19,112 @@
>>  #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, 0),
>> +				   SPI_MEM_OP_DATA_IN(1, reg_val, 0));
> 
> This isn't compatible with DDR mode. We should able to use
> spansion_read_any_reg() helper in place of some of the open coding being
> done today in spi_nor_cypress_octal_dtr_enable(). Same applies for
> spansion_write_any_reg().
> 
I don't see why this isn't compatible with DDR mode.
The spi_nor_spimem_setup_op() and dummy adjustment take care of DDR.

To use this helper in spi_nor_cypress_octal_dtr_enable(), addr_width needs
to be passed by parameter instead of using 'nor->addr_width'. Please let
me know if any other changes are required.

>> +
>> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
>> +
> 
> 
>> +		op.dummy.nbytes = (reg_dummy * op.dummy.buswidth) / 8;
>> +		if (spi_nor_protocol_is_dtr(nor->reg_proto))
>> +			op.dummy.nbytes *= 2;
> 
> spi_nor_spimem_setup_op() should care of above right?
> 
This is same as spi_nor_spimem_read_data() in core.c. We need to convert
the dummy 'cycles' to the dummy 'bytes' after spi_nor_spimem_setup_op().
I will add a comment about it.

>> +
>> +		ret = spi_mem_exec_op(nor->spimem, &op);
> 
> 		return spi_mem_exec_op(nor->spimem, &op);
> 
> Thus avoid extra level indentation below.
> 
>> +	} 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 < 0)
>> +			return ret;
>> +		if (ret != 1)
>> +			return -EIO;
>> +
> 
> 
>> +		ret = 0;
> 
> 		return 0;
> 
For better readability, how about adding a helper of controller ops?

	if (nor->spimem) {
		[...]
		return spi_mem_exec_op(nor->spimem, &op);
	}

	return spansion_controller_ops_read_any_reg(nor, reg_addr, reg_dummy, reg_val);


>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * spansion_write_any_reg() - Write Any Register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @reg_addr:	register address (should be a volatile register)
> 
> 
> To be safe, lets explicitly reject writes to non-volatile register
> addresses.
> 
OK.

>> + * @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.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr, u8 reg_val)
>> +{
>> +	ssize_t ret;
>> +
>> +	ret = spi_nor_write_enable(nor);
>> +	if (ret)
>> +		return ret;
> 
> 
> Hmm, I don't see spi_nor_write_disable().
> 
> Either both spi_nor_write_enable() and spi_nor_write_disable() should be
> responsibility of caller or both needs to be taken care of here.
> 
> And the same needs to be documented in the function description
> 
OK. I will call both from outside.

>> +
>> +	if (nor->spimem) {
>> +		struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 0),
>> +				   SPI_MEM_OP_ADDR(nor->addr_width, reg_addr, 0),
>> +				   SPI_MEM_OP_NO_DUMMY,
>> +				   SPI_MEM_OP_DATA_OUT(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->write_proto;
>> +		u8 opcode = nor->program_opcode;
>> +
>> +		nor->program_opcode = SPINOR_OP_WR_ANY_REG;
>> +		nor->write_proto = nor->reg_proto;
>> +
>> +		ret = nor->controller_ops->write(nor, reg_addr, 1, &reg_val);
>> +
>> +		nor->program_opcode = opcode;
>> +		nor->write_proto = proto;
>> +
>> +		if (ret < 0)
>> +			return ret;
>> +		if (ret != 1)
>> +			return -EIO;
>> +
>> +		ret = 0;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  /**
>>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>   * @nor:		pointer to a 'struct spi_nor'
>>

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