Re: [PATCH v8 2/9] spi: spi-mem: Allow specifying the byte order in Octal DTR mode

[Michael Walle <mwalle@kernel.org>]

Hi,

> From: JaimeLiao <jaimeliao@mxic.com.tw>

I think Tudor told you to keep his name/mail here.

> There are NOR flashes (Macronix) that swap the bytes on a 16-bit
> boundary when configured in Octal DTR mode. The byte order of
> 16-bit words is swapped when read or written in Octal Double
> Transfer Rate (DTR) mode compared to Single Transfer Rate (STR)
> modes. If one writes D0 D1 D2 D3 bytes using 1-1-1 mode, and uses
> 8D-8D-8D SPI mode for reading, it will read back D1 D0 D3 D2.
> Swapping the bytes may introduce some endianness problems. It can
> affect the boot sequence if the entire boot sequence is not handled
> in either 8D-8D-8D mode or 1-1-1 mode. So we must swap the bytes
> back to have the same byte order as in STR modes. Fortunately there
> are controllers that could swap the bytes back at runtime,
> addressing the flash's endiannesses requirements. Provide a way for
> the upper layers to specify the byte order in Octal DTR mode.
> 
> Merge Tudor's patch and add modifications for suiting newer version
> of Linux kernel.
> 
> Suggested-by: Michael Walle <mwalle@kernel.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> ---
>  drivers/spi/spi-mem.c       | 4 ++++
>  include/linux/spi/spi-mem.h | 6 ++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 2dc8ceb85374..f8120f6b288f 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -172,6 +172,10 @@ bool spi_mem_default_supports_op(struct spi_mem 
> *mem,
>  		if (!spi_mem_controller_is_capable(ctlr, dtr))
>  			return false;
> 
> +		if (op->data.swap16 &&
> +		    !spi_mem_controller_is_capable(ctlr, swap16))

Since you need to redo this anyway (see below):
This can now be one line. Please keep in mind that the 80char limit
was extended to 100 chars some time ago. And I think this reads
better if its just one line.

> +			return false;
> +
>  		if (op->cmd.nbytes != 2)
>  			return false;
>  	} else {
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index f866d5c8ed32..8df44fbc9d99 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -89,6 +89,8 @@ enum spi_mem_data_dir {
>   * @dummy.dtr: whether the dummy bytes should be sent in DTR mode or 
> not
>   * @data.buswidth: number of IO lanes used to send/receive the data
>   * @data.dtr: whether the data should be sent in DTR mode or not
> + * @data.swap16: whether the byte order of 16-bit words is swapped 
> when read
> + *		 or written in Octal DTR mode compared to STR mode.

I think this was ordered alphabetically in the former patch, but since
you've renamed it to swap16 now the order would change. I don't have
any preference here, just wanted to point that out. I just noticed
because the new member "swap16" in spi_controller_mem_caps was added in
the middle, which looked odd.

>   * @data.ecc: whether error correction is required or not
>   * @data.dir: direction of the transfer
>   * @data.nbytes: number of data bytes to send/receive. Can be zero if 
> the
> @@ -123,6 +125,7 @@ struct spi_mem_op {
>  	struct {
>  		u8 buswidth;
>  		u8 dtr : 1;
> +		u8 swap16 : 1;
>  		u8 ecc : 1;
>  		u8 __pad : 6;

Still wrong, please go over all the previous remarks, to be clear you 
have
to use "__pad : 5" here.

Otherwise looks good. So with the above fixed:

Reviewed-by: Michael Walle <mwalle@kernel.org>

-michael

>  		enum spi_mem_data_dir dir;
> @@ -296,10 +299,13 @@ struct spi_controller_mem_ops {
>  /**
>   * struct spi_controller_mem_caps - SPI memory controller capabilities
>   * @dtr: Supports DTR operations
> + * @swap16: Supports swapping bytes on a 16 bit boundary when 
> configured in
> + *	    Octal DTR
>   * @ecc: Supports operations with error correction
>   */
>  struct spi_controller_mem_caps {
>  	bool dtr;
> +	bool swap16;
>  	bool ecc;
>  };

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