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

[liao jaime]

Hi Michael


>
> 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"?
Thanks, I will change this next patch.

>
> >  {
> >       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.
Got it.

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

Thanks
Jaime