| Message ID | 20231221090702.103027-6-jaimeliao.tw@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | Add octal DTR support for Macronix flash | expand |
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@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, > };
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@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
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) { 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) { + if (op->data.dtr_swab16) + flags &= ~HC_CFG_DATA_PASS; + else + flags |= HC_CFG_DATA_PASS; + } + 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, };