Date:   Tue, 30 Jul 2019 23:34:20 +0530
From:   Vignesh Raghavendra <vigneshr@...com>
To:     <Tudor.Ambarus@...rochip.com>, <miquel.raynal@...tlin.com>,
        <richard@....at>, <marek.vasut@...il.com>
CC:     <bbrezillon@...nel.org>, <yogeshnarayan.gaur@....com>,
        <linux-kernel@...r.kernel.org>, <linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH v2 1/2] mtd: spi-nor: Move m25p80 code in spi-nor.c

Hi Tudor,

On 25-Jul-19 4:49 PM, Tudor.Ambarus@...rochip.com wrote:
> 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:
>

I will move the code introducing bounce buffer into separate patch at
the beginning of this series and switch over all read/write regs
functions to use bounce buffer instead of cmd_buf. cmd_buf will be dropped.
And then simplify this patch to spi_nor_spimem_xfer_reg() to you pointed
out below. Does that sound good?

> 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.
> 

I prefer reverse xmas and hope compilers are intelligent enough to
reorder allocation to save 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.

OK.

> 
>> +					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.
> 

Ok, my idea was to factor out all common code b/w
spi_nor_spimem_read_data() and spi_nor_spimem_write_data() in
spi_nor_spimem_xfer_data(). But, I am fine with your idea.

>> +}
> 
> 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
> 

Will drop

> 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.
> 

Will update when dropping spi_nor_data_op()

>> +
>> +		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

Agreed

> 
>> -	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.
> 

Will fix along with dropping spi_nor_data_op()

> Cheers,
> ta
> 

Regards
Vignesh

Powered by blists - more mailing lists