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

[Tudor.Ambarus]

Hi, Boris,

On 07/25/2019 03:37 PM, Boris Brezillon wrote:
> External E-Mail
> 
> 
> On Thu, 25 Jul 2019 11:19:06 +0000
> <Tudor.Ambarus@xxxxxxxxxxxxx> wrote:
> 
>>> + */
>>> +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);
>> }
> 
> Well, I don't think that's a good idea. ->cmd_buf is an array in the
> middle of the spi_nor struct, which means it won't be aligned on a
> cache line and you'll have to be extra careful not to touch the spi_nor
> fields when calling spi_mem_exec_op(). Might work, but I wouldn't take
> the risk if I were you.
> 

u8 cmd_buf[SPI_NOR_MAX_CMD_SIZE] ____cacheline_aligned;

Does this help?

> Another option would be to allocate ->cmd_buf with kmalloc() instead of
> having it defined as a static array.
> 
>>
>> 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.
> 
> I think I added the addr param for a good reason (probably to support
> Octo mode cmds that take an address parameter). This being said, I
> agree with you, we should just pass everything through the op parameter
> (including the address if we ever need to add one).
> 
> 
>>> +
>>> +/**
>>> + * 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.
> 
> But it breaks the reverse-xmas-tree formatting :-).
> 
>>
>>> +	void *rdbuf = NULL;
>>> +	const void *buf;  
>>
>> you can get rid of rdbuf and buf if you pass buf as argument.
> 
> Hm, passing the buffer to send data from/receive data into is already
> part of the spi_mem_op definition process (which is done in the caller
> of this func) so why bother passing an extra arg to the function.
> Note that you had the exact opposite argument for the
> spi_nor_spimem_xfer_reg() prototype you suggested above (which I
> agree with BTW) :P.

In order to avoid if clauses like "if (op->data.dir == SPI_MEM_DATA_IN)". You
can't use op->data.buf directly, the *out const qualifier can be discarded.

pointer to buf was not needed in spi_nor_spimem_xfer_reg(), we could use
nor->cmd_buf.

> 
>>
>>> +	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.
> 
> The semantic is well documented in the doc just above the function, but
> I have the feeling that you're in 'nitpick' mode, so I'll just let you
> pick the one you prefer :).

Not my intention. struct mtd_info and struct spi_nor use "from" when referring
to the offset from where to read in the read() calls. Just consistency reasons.

Cheers,
ta