[PATCH v7 1/3] mtd: spi-nor: spansion: Add support for Read/Write Any Register

[Tudor.Ambarus at microchip.com]

On 7/19/21 11:03, tkuw584924 at gmail.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> 
> Some of Spansion/Cypress chips support Read/Write Any Register commands.
> These commands are mainly used to write volatile registers.
> 
> 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 at infineon.com>
> ---
> Changes in v7:
>   - No change
> 
> Changes in v6:
>   - Add helper functions for controller_ops
>   - Add 'reg_addr_width' parameter to spansion_read/write_any_reg()
>   - Remove spi_nor_write_enable() from spansion_write_any_reg() and modified
>     function header comment
> 
> 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 | 142 +++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index ee82dcd75310..5e7e62704e11 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -19,6 +19,148 @@
>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS    0
>  #define SPINOR_OP_CYPRESS_RD_FAST              0xee
> 
> +/**
> + * controller_ops_read_any_reg() - Read Any Register using controller_ops.
> + * @nor:               pointer to a 'struct spi_nor'
> + * @reg_addr:          register address
> + * @reg_addr_width:    number of address bytes
> + * @reg_dummy:         number of dummy cycles for register read
> + * @reg_val:           pointer to a buffer where the register value is copied
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int controller_ops_read_any_reg(struct spi_nor *nor, u32 reg_addr,
> +                                      u8 reg_addr_width, u8 reg_dummy,
> +                                      u8 *reg_val)
> +{
> +       ssize_t ret;
> +       enum spi_nor_protocol proto = nor->read_proto;
> +       u8 opcode = nor->read_opcode;
> +       u8 dummy = nor->read_dummy;
> +       u8 addr_width = nor->addr_width;
> +
> +       nor->read_opcode = SPINOR_OP_RD_ANY_REG;
> +       nor->read_dummy = reg_dummy;
> +       nor->read_proto = nor->reg_proto;
> +       nor->addr_width = reg_addr_width;
> +
> +       ret = nor->controller_ops->read(nor, reg_addr, 1, reg_val);
> +
> +       nor->read_opcode = opcode;
> +       nor->read_dummy = dummy;
> +       nor->read_proto = proto;
> +       nor->addr_width = addr_width;
> +
> +       if (ret < 0)
> +               return ret;
> +       if (ret != 1)
> +               return -EIO;
> +
> +       return 0;
> +}
> +
> +/**
> + * controller_ops_write_any_reg() - Write Any Register using controller_ops.
> + * @nor:               pointer to a 'struct spi_nor'
> + * @reg_addr:          register address
> + * @reg_addr_width:    number of address bytes
> + * @reg_val:           register value to be written
> + * *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int controller_ops_write_any_reg(struct spi_nor *nor, u32 reg_addr,
> +                                       u8 reg_addr_width, u8 reg_val)
> +{
> +       ssize_t ret;
> +       enum spi_nor_protocol proto = nor->write_proto;
> +       u8 opcode = nor->program_opcode;
> +       u8 addr_width = nor->addr_width;
> +
> +       nor->program_opcode = SPINOR_OP_WR_ANY_REG;
> +       nor->write_proto = nor->reg_proto;
> +       nor->addr_width = reg_addr_width;
> +
> +       ret = nor->controller_ops->write(nor, reg_addr, 1, &reg_val);
> +
> +       nor->program_opcode = opcode;
> +       nor->write_proto = proto;
> +       nor->addr_width = addr_width;
> +
> +       if (ret < 0)
> +               return ret;
> +       if (ret != 1)
> +               return -EIO;
> +
> +       return 0;
> +}
> +
> +/**
> + * spansion_read_any_reg() - Read Any Register.
> + * @nor:               pointer to a 'struct spi_nor'
> + * @reg_addr:          register address
> + * @reg_addr_width:    number of address bytes
> + * @reg_dummy:         number of dummy cycles for register read
> + * @reg_val:           pointer to a buffer where the register value is copied
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_read_any_reg(struct spi_nor *nor, u32 reg_addr,
> +                                u8 reg_addr_width, u8 reg_dummy, u8 *reg_val)

how about making this a generic core method? I'm sure there are other SPI NOR
vendors that use registers indexed by address. How about introducing:

struct spi_nor_reg {
	u32 addr;
	u8 addr_width;
	u8 dummy;
	u8 opcode;
	u8 val;
};

and passing a pointer to this struct as argument.

> +{
> +       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(reg_addr_width, reg_addr, 0),
> +                                  SPI_MEM_OP_DUMMY(reg_dummy, 0),
> +                                  SPI_MEM_OP_DATA_IN(1, reg_val, 0));
> +
> +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +               /* convert the dummy cycles to the number of bytes */
> +               op.dummy.nbytes = (reg_dummy * op.dummy.buswidth) / 8;
> +               if (spi_nor_protocol_is_dtr(nor->reg_proto))
> +                       op.dummy.nbytes *= 2;
> +
> +               return spi_mem_exec_op(nor->spimem, &op);
> +       }
> +
else
	return -EOPNOTSUPP;

we'd like to move all the SPI NOR controller drivers under SPI MEM, let's
not add support for them.

> +       return controller_ops_read_any_reg(nor, reg_addr, reg_addr_width,
> +                                          reg_dummy, reg_val);
> +}
> +
> +/**
> + * spansion_write_any_reg() - Write Any Register.
> + * @nor:               pointer to a 'struct spi_nor'
> + * @reg_addr:          register address
> + * @reg_addr_width:    number of address bytes
> + * @reg_val:           register value to be written
> + *
> + * spi_nor_write_enable() and spi_nor_write_disable() need to be called before
> + * and after this function. Caller also need to poll status for non-volatile
> + * register. No need to poll status for volatile registers since volatile
> + * register write will be effective immediately after the operation.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr,
> +                                 u8 reg_addr_width, u8 reg_val)
> +{
> +       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(reg_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);
> +
> +               return spi_mem_exec_op(nor->spimem, &op);
> +       }
> +
> +       return controller_ops_write_any_reg(nor, reg_addr, reg_addr_width,
> +                                           reg_val);
> +}
> +
>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:               pointer to a 'struct spi_nor'
> --
> 2.25.1
>