[PATCH v7 5/7] spi: mxic: Add support for swapping byte

[Michael Walle]

Hi,

> Some SPI-NOR flash swap the bytes on a 16-bit boundary when
> configured in Octal DTR mode. It means data format D0 D1 D2 D3
> would be swapped to D1 D0 D3 D2. So that whether controller
> support swapping bytes should be checked before enable Octal
> DTR mode. Add swap byte support on a 16-bit boundary when
> configured in Octal DTR mode for Macronix xSPI host controller
> dirver.
> 
> According dtr_swab in operation to enable/disable Macronix
> xSPI host controller swap byte feature.
> 
> To make sure swap byte feature is working well, program data in
> 1S-1S-1S mode then read back and compare read data in 8D-8D-8D
> mode.
> 
> This feature have been validated on byte-swap flash and
> non-byte-swap flash.
> 
> Macronix xSPI host controller bit "HC_CFG_DATA_PASS" determine
> the byte swap feature disabled/enabled and swap byte feature is
> working on 8D-8D-8D mode only.
> 
> Signed-off-by: JaimeLiao <jaimeliao at mxic.com.tw>
> ---
>  drivers/spi/spi-mxic.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index 60c9f3048ac9..8dc83adaaa88 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -294,7 +294,8 @@ static void mxic_spi_hw_init(struct mxic_spi *mxic)
>  	       mxic->regs + HC_CFG);
>  }
> 
> -static u32 mxic_spi_prep_hc_cfg(struct spi_device *spi, u32 flags)
> +static u32 mxic_spi_prep_hc_cfg(const struct spi_mem_op *op,
> +				struct spi_device *spi, u32 flags)

Not my driver, but because it caught my eye: I wouldn't pass
spi_mem_op. Maybe just "bool swap16"?

>  {
>  	int nio = 1;
> 
> @@ -305,6 +306,13 @@ static u32 mxic_spi_prep_hc_cfg(struct spi_device 
> *spi, u32 flags)
>  	else if (spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
>  		nio = 2;
> 
> +	if (op->data.dtr) {

Checking this seems to be redundant with checking dtr_swab16.

> +		if (op->data.dtr_swab16)
> +			flags &= ~HC_CFG_DATA_PASS;
> +		else
> +			flags |= HC_CFG_DATA_PASS;

Mhh, this is strange. Given that dtr_swap16 is a new flag means
that you are now setting the HC_CFG_DATA_PASS bit by default.
Something to keep in mind if you have any users which already use
8d8d8d mode nowadays.

Also clearing the flag seems superfluous.

-michael

> +	}
> +
>  	return flags | HC_CFG_NIO(nio) |
>  	       HC_CFG_TYPE(spi_get_chipselect(spi, 0), HC_CFG_TYPE_SPI_NOR) |
>  	       HC_CFG_SLV_ACT(spi_get_chipselect(spi, 0)) | 
> HC_CFG_IDLE_SIO_LVL(1);
> @@ -397,7 +405,8 @@ static ssize_t mxic_spi_mem_dirmap_read(struct 
> spi_mem_dirmap_desc *desc,
>  	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
>  		return -EINVAL;
> 
> -	writel(mxic_spi_prep_hc_cfg(desc->mem->spi, 0), mxic->regs + HC_CFG);
> +	writel(mxic_spi_prep_hc_cfg(&desc->info.op_tmpl,
> +				    desc->mem->spi, 0), mxic->regs + HC_CFG);
> 
>  	writel(mxic_spi_mem_prep_op_cfg(&desc->info.op_tmpl, len),
>  	       mxic->regs + LRD_CFG);
> @@ -441,7 +450,8 @@ static ssize_t mxic_spi_mem_dirmap_write(struct 
> spi_mem_dirmap_desc *desc,
>  	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
>  		return -EINVAL;
> 
> -	writel(mxic_spi_prep_hc_cfg(desc->mem->spi, 0), mxic->regs + HC_CFG);
> +	writel(mxic_spi_prep_hc_cfg(&desc->info.op_tmpl,
> +				    desc->mem->spi, 0), mxic->regs + HC_CFG);
> 
>  	writel(mxic_spi_mem_prep_op_cfg(&desc->info.op_tmpl, len),
>  	       mxic->regs + LWR_CFG);
> @@ -518,7 +528,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem 
> *mem,
>  	if (ret)
>  		return ret;
> 
> -	writel(mxic_spi_prep_hc_cfg(mem->spi, HC_CFG_MAN_CS_EN),
> +	writel(mxic_spi_prep_hc_cfg(op, mem->spi, HC_CFG_MAN_CS_EN),
>  	       mxic->regs + HC_CFG);
> 
>  	writel(HC_EN_BIT, mxic->regs + HC_EN);
> @@ -572,6 +582,7 @@ static const struct spi_controller_mem_ops 
> mxic_spi_mem_ops = {
> 
>  static const struct spi_controller_mem_caps mxic_spi_mem_caps = {
>  	.dtr = true,
> +	.dtr_swab16 = true,
>  	.ecc = true,
>  };