Re: [PATCH v2 1/2] mtd: spi-nor: Move m25p80 code in spi-nor.c

[<Tudor.Ambarus@microchip.com>]

All,

I want this in 5.4, please review/test the soonest.

On 07/20/2019 11:00 AM, Vignesh Raghavendra wrote:

> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 03cc788511d5..f428a6d4022b 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -19,6 +19,7 @@
>  
>  #include <linux/mtd/mtd.h>
>  #include <linux/of_platform.h>
> +#include <linux/sched/task_stack.h>
>  #include <linux/spi/flash.h>
>  #include <linux/mtd/spi-nor.h>
>  
> @@ -288,6 +289,232 @@ struct flash_info {
>  
>  #define JEDEC_MFR(info)	((info)->id[0])
>  
> +/**
> + * spi_nor_exec_op() - helper function to read/write flash registers

the function name can easily get confused with spi_mem_exec_op(). How about
renaming it to spi_nor_spimem_xfer_reg(), it will be in concordance with
spi_nor_spimem_xfer_data().

> + * @nor:        pointer to 'struct spi_nor'
> + * @op:         pointer to 'struct spi_mem_op' template for transfer
> + * @addr:       pointer to offset within flash
> + * @buf:        pointer to data buffer into which data is read/written
> + *              into

                   ^ drop second into

> + * @len:        length of the transfer
> + *
> + * Return: 0 on success, non-zero otherwise

                            ^ s/non-zero/-errno?

> + */
> +static int spi_nor_exec_op(struct spi_nor *nor, struct spi_mem_op *op,
> +			   u64 *addr, void *buf, size_t len)
> +{
> +	int ret;
> +	bool usebouncebuf = false;

I don't think we need a bounce buffer for regs. What is the maximum size that we
read/write regs, SPI_NOR_MAX_CMD_SIZE(8)?

In spi-nor.c the maximum length that we pass to nor->read_reg()/write_reg() is
SPI_NOR_MAX_ID_LEN(6).

I can provide a patch to always use nor->cmd_buf when reading/writing regs so
you respin the series on top of it, if you feel the same.

With nor->cmd_buf this function will be reduced to the following:

static int spi_nor_spimem_xfer_reg(struct spi_nor *nor, struct spi_mem_op *op)
{
	if (!op || (op->data.nbytes && !nor->cmd_buf))
		return -EINVAL;

	return spi_mem_exec_op(nor->spimem, op);
}

spi_nor_exec_op() always received a NULL addr, let's get rid of it. We won't
need buf anymore and you can retrieve the length from op->data.nbytes. Now that
we trimmed the arguments, I think I would get rid of the
spi_nor_data/nodata_op() wrappers and use spi_nor_spimem_xfer_reg() directly.

> +
> +	if (!op || (len && !buf))
> +		return -EINVAL;
> +
> +	if (op->addr.nbytes && addr)
> +		op->addr.val = *addr;
> +
> +	op->data.nbytes = len;
> +
> +	if (object_is_on_stack(buf) || !virt_addr_valid(buf))
> +		usebouncebuf = true;
> +	if (len && usebouncebuf) {
> +		if (len > nor->bouncebuf_size)
> +			return -ENOTSUPP;
> +
> +		if (op->data.dir == SPI_MEM_DATA_IN) {
> +			op->data.buf.in = nor->bouncebuf;
> +		} else {
> +			op->data.buf.out = nor->bouncebuf;
> +			memcpy(nor->bouncebuf, buf, len);
> +		}
> +	} else {
> +		op->data.buf.out = buf;
> +	}
> +
> +	ret = spi_mem_exec_op(nor->spimem, op);
> +	if (ret)
> +		return ret;
> +
> +	if (usebouncebuf && len && op->data.dir == SPI_MEM_DATA_IN)
> +		memcpy(buf, nor->bouncebuf, len);
> +
> +	return 0;
> +}

cut

> +
> +/**
> + * spi_nor_spimem_xfer_data() - helper function to read/write data to
> + *                              flash's memory region
> + * @nor:        pointer to 'struct spi_nor'
> + * @op:         pointer to 'struct spi_mem_op' template for transfer
> + * @proto:      protocol to be used for transfer
> + *
> + * Return: number of bytes transferred on success, -errno otherwise
> + */
> +static ssize_t spi_nor_spimem_xfer_data(struct spi_nor *nor,
> +					struct spi_mem_op *op,
> +					enum spi_nor_protocol proto)
> +{
> +	bool usebouncebuf = false;

declare bool at the end to avoid stack padding.

> +	void *rdbuf = NULL;
> +	const void *buf;

you can get rid of rdbuf and buf if you pass buf as argument.

> +	int ret;
> +
> +	/* get transfer protocols. */
> +	op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto);
> +	op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> +	op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
> +
> +	if (op->data.dir == SPI_MEM_DATA_IN)
> +		buf = op->data.buf.in;
> +	else
> +		buf = op->data.buf.out;
> +
> +	if (object_is_on_stack(buf) || !virt_addr_valid(buf))
> +		usebouncebuf = true;
> +
> +	if (usebouncebuf) {
> +		if (op->data.nbytes > nor->bouncebuf_size)
> +			op->data.nbytes = nor->bouncebuf_size;
> +
> +		if (op->data.dir == SPI_MEM_DATA_IN) {
> +			rdbuf = op->data.buf.in;
> +			op->data.buf.in = nor->bouncebuf;
> +		} else {
> +			op->data.buf.out = nor->bouncebuf;
> +			memcpy(nor->bouncebuf, buf,
> +			       op->data.nbytes);
> +		}
> +	}
> +
> +	ret = spi_mem_adjust_op_size(nor->spimem, op);
> +	if (ret)
> +		return ret;
> +
> +	ret = spi_mem_exec_op(nor->spimem, op);
> +	if (ret)
> +		return ret;
> +
> +	if (usebouncebuf && op->data.dir == SPI_MEM_DATA_IN)
> +		memcpy(rdbuf, nor->bouncebuf, op->data.nbytes);
> +
> +	return op->data.nbytes;
> +}
> +
> +/**
> + * spi_nor_spimem_read_data() - read data from flash's memory region via
> + *                              spi-mem
> + * @nor:        pointer to 'struct spi_nor'
> + * @ofs:        offset to read from
> + * @len:        number of bytes to read
> + * @buf:        pointer to dst buffer
> + *
> + * Return: number of bytes read successfully, -errno otherwise
> + */
> +static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t ofs,

s/ofs/from? both flash and buf may have offsets, "from" better indicates that
the offset is associated with the flash.

> +					size_t len, u8 *buf)
> +{
> +	struct spi_mem_op op =
> +		SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1),
> +			   SPI_MEM_OP_ADDR(nor->addr_width, ofs, 1),
> +			   SPI_MEM_OP_DUMMY(nor->read_dummy, 1),
> +			   SPI_MEM_OP_DATA_IN(len, buf, 1));
> +
> +	op.dummy.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
> +
> +	/* convert the dummy cycles to the number of bytes */
> +	op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> +
> +	return spi_nor_spimem_xfer_data(nor, &op, nor->read_proto);

stop passing nor->read_proto and do all buswidth initialization here. This way
we'll keep the inits all gathered together, and will have the xfer() that will
do just the transfer (with bouncebuffer if needed). Function that does a single
thing.

> +}

cut

> @@ -459,7 +749,6 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor)
>  		struct spi_nor_erase_map *map = &nor->erase_map;
>  		struct spi_nor_erase_type *erase;
>  		int i;
> -

keep the blank line

cut

> @@ -1406,7 +1807,18 @@ static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
>  
>  	write_enable(nor);
>  
> -	ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRSR, 1),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_OUT(0, NULL, 1));

nbytes is 2.

> +
> +		ret = spi_nor_data_op(nor, &op, sr_cr, 2);
> +	} else {
> +		ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
> +	}

cut

> @@ -1626,8 +2068,7 @@ static int sr2_bit7_quad_enable(struct spi_nor *nor)
>  		return ret;
>  	}
>  
> -	/* Read back and check it. */

don't drop the comment

> -	ret = nor->read_reg(nor, SPINOR_OP_RDSR2, &sr2, 1);
> +	ret = spi_nor_read_sr2(nor, &sr2);
>  	if (!(ret > 0 && (sr2 & SR2_QUAD_EN_BIT7))) {
>  		dev_err(nor->dev, "SR2 Quad bit not set\n");
>  		return -EINVAL;
> @@ -2180,7 +2621,18 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>  	u8			id[SPI_NOR_MAX_ID_LEN];
>  	const struct flash_info	*info;
>  
> -	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN);
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_IN(0, NULL, 1));

nbytes is SPI_NOR_MAX_ID_LEN and not 1.

Cheers,
ta