[PATCH 0/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[PATCH 0/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Yuan Yao]

From: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>

The qSPI controller's endian is independent of the CPU core's endian.

For eg, Core on NXP LS1043A SoC is little endian but DSPI is big endian
whereas Core on LS2080A SoC is little endian and DSPI also is little
endian on the same core.

At first we use regmap to cover this issue.
But the regmap is designed too large and complex.
The issue for regmap often effect the DSPI's stability.

DSPI driver just only need a effective way to R/W the controller register
with BE or LE mode.

So it's better to packaging a sample function.
This will make the DSPI driver more stability and high effective.

Yuan Yao (2):
  spi: spi-fsl-dspi: replace regmap R/W with internal implementation
  spi: spi-fsl-dspi: Update DT binding documentation

 .../devicetree/bindings/spi/spi-fsl-dspi.txt       |   3 +-
 drivers/spi/spi-fsl-dspi.c                         | 101 +++++++++++++--------
 2 files changed, 62 insertions(+), 42 deletions(-)

-- 
2.1.0.27.g96db324

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Yuan Yao]

From: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>

The qSPI controller's endian is independent of the CPU core's endian.

For eg, Core on NXP LS1043A SoC is little endian but DSPI is big endian
whereas Core on LS2080A SoC is little endian and DSPI also is little
endian on the same core.

At first we use regmap to cover this issue.
But the regmap is designed too large and complex.
The issue for regmap often effect the DSPI's stability.

DSPI driver just only need a effective way to R/W the controller register
with BE or LE mode.

So it's better to packaging a sample function.
This will make the DSPI driver more stability and high effective.

Signed-off-by: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>
---
 drivers/spi/spi-fsl-dspi.c | 99 +++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 40 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 9e9dadb..9a61572 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -27,7 +27,6 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
-#include <linux/regmap.h>
 #include <linux/sched.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_bitbang.h>
@@ -143,7 +142,7 @@ struct fsl_dspi {
 	struct spi_master	*master;
 	struct platform_device	*pdev;
 
-	struct regmap		*regmap;
+	void __iomem		*iobase;
 	int			irq;
 	struct clk		*clk;
 
@@ -165,13 +164,46 @@ struct fsl_dspi {
 	u32			waitflags;
 
 	u32			spi_tcnt;
+	bool			big_endian;
 };
 
+/*
+ * R/W functions for big- or little-endian registers:
+ * The qSPI controller's endian is independent of the CPU core's endian.
+ */
+static void dspi_writel(struct fsl_dspi *d, u32 val, u32 offset)
+{
+	if (d->big_endian)
+		iowrite32be(val, d->iobase + offset);
+	else
+		iowrite32(val, d->iobase + offset);
+}
+
+static u32 dspi_readl(struct fsl_dspi *d, u32 offset)
+{
+	if (d->big_endian)
+		return ioread32be(d->iobase + offset);
+	else
+		return ioread32(d->iobase + offset);
+}
+
+static void dspi_updatel(struct fsl_dspi *d, u32 mask, u32 val, u32 offset)
+{
+	u32 tmp, orig;
+
+	orig = dspi_readl(d, offset);
+
+	tmp = orig & ~mask;
+	tmp |= val & mask;
+
+	dspi_writel(d, tmp, offset);
+}
+
 static inline int is_double_byte_mode(struct fsl_dspi *dspi)
 {
 	unsigned int val;
 
-	regmap_read(dspi->regmap, SPI_CTAR(0), &val);
+	val = dspi_readl(dspi, SPI_CTAR(0));
 
 	return ((val & SPI_FRAME_BITS_MASK) == SPI_FRAME_BITS(8)) ? 0 : 1;
 }
@@ -270,7 +302,7 @@ static void dspi_data_from_popr(struct fsl_dspi *dspi, int rx_word)
 	u16 d;
 	unsigned int val;
 
-	regmap_read(dspi->regmap, SPI_POPR, &val);
+	val = dspi_readl(dspi, SPI_POPR);
 	d = SPI_POPR_RXDATA(val);
 
 	if (!(dspi->dataflags & TRAN_STATE_RX_VOID))
@@ -294,8 +326,8 @@ static int dspi_eoq_write(struct fsl_dspi *dspi)
 		 */
 		if (tx_word && (dspi->len == 1)) {
 			dspi->dataflags |= TRAN_STATE_WORD_ODD_NUM;
-			regmap_update_bits(dspi->regmap, SPI_CTAR(0),
-					SPI_FRAME_BITS_MASK, SPI_FRAME_BITS(8));
+			dspi_updatel(dspi, SPI_FRAME_BITS_MASK,
+					SPI_FRAME_BITS(8), SPI_CTAR(0));
 			tx_word = 0;
 		}
 
@@ -309,7 +341,7 @@ static int dspi_eoq_write(struct fsl_dspi *dspi)
 		} else if (tx_word && (dspi->len == 1))
 			dspi_pushr |= SPI_PUSHR_EOQ;
 
-		regmap_write(dspi->regmap, SPI_PUSHR, dspi_pushr);
+		dspi_writel(dspi, dspi_pushr, SPI_PUSHR);
 
 		tx_count++;
 	}
@@ -343,8 +375,8 @@ static int dspi_tcfq_write(struct fsl_dspi *dspi)
 
 	if (tx_word && (dspi->len == 1)) {
 		dspi->dataflags |= TRAN_STATE_WORD_ODD_NUM;
-		regmap_update_bits(dspi->regmap, SPI_CTAR(0),
-				SPI_FRAME_BITS_MASK, SPI_FRAME_BITS(8));
+		dspi_updatel(dspi, SPI_FRAME_BITS_MASK,
+				SPI_FRAME_BITS(8), SPI_CTAR(0));
 		tx_word = 0;
 	}
 
@@ -353,7 +385,7 @@ static int dspi_tcfq_write(struct fsl_dspi *dspi)
 	if ((dspi->cs_change) && (!dspi->len))
 		dspi_pushr &= ~SPI_PUSHR_CONT;
 
-	regmap_write(dspi->regmap, SPI_PUSHR, dspi_pushr);
+	dspi_writel(dspi, dspi_pushr, SPI_PUSHR);
 
 	return tx_word + 1;
 }
@@ -378,7 +410,7 @@ static int dspi_transfer_one_message(struct spi_master *master,
 	enum dspi_trans_mode trans_mode;
 	u32 spi_tcr;
 
-	regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
+	spi_tcr = dspi_readl(dspi, SPI_TCR);
 	dspi->spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
 
 	message->actual_length = 0;
@@ -407,21 +439,19 @@ static int dspi_transfer_one_message(struct spi_master *master,
 		if (!dspi->tx)
 			dspi->dataflags |= TRAN_STATE_TX_VOID;
 
-		regmap_write(dspi->regmap, SPI_MCR, dspi->cur_chip->mcr_val);
-		regmap_update_bits(dspi->regmap, SPI_MCR,
-				SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF,
-				SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF);
-		regmap_write(dspi->regmap, SPI_CTAR(0),
-				dspi->cur_chip->ctar_val);
+		dspi_writel(dspi, dspi->cur_chip->mcr_val, SPI_MCR);
+		dspi_updatel(dspi, SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF,
+				   SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF, SPI_MCR);
+		dspi_writel(dspi, dspi->cur_chip->ctar_val, SPI_CTAR(0));
 
 		trans_mode = dspi->devtype_data->trans_mode;
 		switch (trans_mode) {
 		case DSPI_EOQ_MODE:
-			regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_EOQFE);
+			dspi_writel(dspi, SPI_RSER_EOQFE, SPI_RSER);
 			dspi_eoq_write(dspi);
 			break;
 		case DSPI_TCFQ_MODE:
-			regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_TCFQE);
+			dspi_writel(dspi, SPI_RSER_TCFQE, SPI_RSER);
 			dspi_tcfq_write(dspi);
 			break;
 		default:
@@ -525,14 +555,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 	u32 spi_tcnt, tcnt_diff;
 	int tx_word;
 
-	regmap_read(dspi->regmap, SPI_SR, &spi_sr);
-	regmap_write(dspi->regmap, SPI_SR, spi_sr);
+	spi_sr = dspi_readl(dspi, SPI_SR);
+	dspi_writel(dspi, spi_sr, SPI_SR);
 
 
 	if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)) {
 		tx_word = is_double_byte_mode(dspi);
 
-		regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
+		spi_tcr = dspi_readl(dspi, SPI_TCR);
 		spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
 		/*
 		 * The width of SPI Transfer Counter in SPI_TCR is 16bits,
@@ -569,10 +599,10 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 
 		if (!dspi->len) {
 			if (dspi->dataflags & TRAN_STATE_WORD_ODD_NUM) {
-				regmap_update_bits(dspi->regmap,
-						   SPI_CTAR(0),
-						   SPI_FRAME_BITS_MASK,
-						   SPI_FRAME_BITS(16));
+				dspi_updatel(dspi,
+					     SPI_FRAME_BITS_MASK,
+					     SPI_FRAME_BITS(16),
+					     SPI_CTAR(0));
 				dspi->dataflags &= ~TRAN_STATE_WORD_ODD_NUM;
 			}
 
@@ -636,13 +666,6 @@ static int dspi_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(dspi_pm, dspi_suspend, dspi_resume);
 
-static const struct regmap_config dspi_regmap_config = {
-	.reg_bits = 32,
-	.val_bits = 32,
-	.reg_stride = 4,
-	.max_register = 0x88,
-};
-
 static int dspi_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -700,13 +723,7 @@ static int dspi_probe(struct platform_device *pdev)
 		goto out_master_put;
 	}
 
-	dspi->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
-						&dspi_regmap_config);
-	if (IS_ERR(dspi->regmap)) {
-		dev_err(&pdev->dev, "failed to init regmap: %ld\n",
-				PTR_ERR(dspi->regmap));
-		return PTR_ERR(dspi->regmap);
-	}
+	dspi->iobase = base;
 
 	dspi->irq = platform_get_irq(pdev, 0);
 	if (dspi->irq < 0) {
@@ -730,6 +747,8 @@ static int dspi_probe(struct platform_device *pdev)
 	}
 	clk_prepare_enable(dspi->clk);
 
+	dspi->big_endian = of_property_read_bool(np, "big-endian");
+
 	master->max_speed_hz =
 		clk_get_rate(dspi->clk) / dspi->devtype_data->max_clock_factor;
 
-- 
2.1.0.27.g96db324

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

On Wed, Apr 27, 2016 at 04:12:35PM +0800, Yuan Yao wrote:

> At first we use regmap to cover this issue.
> But the regmap is designed too large and complex.
> The issue for regmap often effect the DSPI's stability.

I'm not convinced that open coding things is going to make life much
easier here.  If there's problems in the underlying code then fixing
those seems like a more generally useful approach to things.  What are
the actual problems this is intended to fix?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Yao Yuan]

Hi Mark Brown,

There are two problems we have found for use regmap up to now.

1, Regmap will read device value depends on endian in .dtsi(DSPI in .dtsi file is big-endian) and readl()(readl() in arm64 is le32_to_cpu read) in system.  
That is the value read out is fixed regardless the kernel were compiled with CONFIG_CPU_BIG_ENDIAN yes or no. That makes the value(read from DSPI)can't be recognized in one endian mode(CONFIG_CPU_BIG_ENDIAN be set or not).
The result is: DSPI can't working with CONFIG_CPU_BIG_ENDIAN set.

2, Sometimes regmap will generate some issues. Some regmap issues will cause the DSPI do not working.
At 2016.4 we find the DSPI was not work, then we found that the regmap R/W the controller register sometimes not right. 
The result is: DSPI can't working with REGMAP API.
Although the regmap issues was fast fixed by some giving person. But I'm still tend to use the reliable way.

Regmap is nice and Maybe I should try to fix regmap instead of replacing regmap in my driver.
But regmap is really very large and complex for DSPI driver.

I think maybe the suitable way is the best way even the regmap is so good as everyone says.

So I strongly suggest to use the dspi_writel / dspi_readl  to instead of the regmap in DSPI driver.

Best Regards,
Yuan Yao


> -----Original Message-----
> From: Mark Brown [mailto:broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Wednesday, April 27, 2016 11:26 PM
> To: Yuan Yao <yao.yuan-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; pawel.moll-5wv7dgnIgG8@public.gmane.org; shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
> devel-XDVM779Km55Y1YpKYGMr2+TW4wlIGRCZ@public.gmane.org; linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Yao Yuan <yao.yuan-3arQi8VN3Tc@public.gmane.org>; Yang-Leo Li
> <leoyang.li-3arQi8VN3Tc@public.gmane.org>; Scott Wood <scott.wood-3arQi8VN3Tc@public.gmane.org>
> Subject: Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal
> implementation
> 
> On Wed, Apr 27, 2016 at 04:12:35PM +0800, Yuan Yao wrote:
> 
> > At first we use regmap to cover this issue.
> > But the regmap is designed too large and complex.
> > The issue for regmap often effect the DSPI's stability.
> 
> I'm not convinced that open coding things is going to make life much easier
> here.  If there's problems in the underlying code then fixing those seems like a
> more generally useful approach to things.  What are the actual problems this is
> intended to fix?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On Mon, May 09, 2016 at 08:48:59AM +0000, Yao Yuan wrote:
> Hi Mark Brown,

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> There are two problems we have found for use regmap up to now.

> 1, Regmap will read device value depends on endian in .dtsi(DSPI in .dtsi file is big-endian) and readl()(readl() in arm64 is le32_to_cpu read) in system.  
> That is the value read out is fixed regardless the kernel were compiled with CONFIG_CPU_BIG_ENDIAN yes or no. That makes the value(read from DSPI)can't be recognized in one endian mode(CONFIG_CPU_BIG_ENDIAN be set or not).
> The result is: DSPI can't working with CONFIG_CPU_BIG_ENDIAN set.

This doesn't make sense.  If the endianness of the device is specified
in DT then the endianness of the CPU won't make a difference.

> 2, Sometimes regmap will generate some issues. Some regmap issues will cause the DSPI do not working.
> At 2016.4 we find the DSPI was not work, then we found that the regmap R/W the controller register sometimes not right. 
> The result is: DSPI can't working with REGMAP API.
> Although the regmap issues was fast fixed by some giving person. But I'm still tend to use the reliable way.

> Regmap is nice and Maybe I should try to fix regmap instead of replacing regmap in my driver.
> But regmap is really very large and complex for DSPI driver.

Yes, if you find bugs in generic frameworks you should fix them rather
than just open coding something.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Yao Yuan]

On Mon, May 09, 2016 at 06:29:59AM +0800, Mark Brown wrote:
> On Mon, May 09, 2016 at 08:48:59AM +0000, Yao Yuan wrote:
> > Hi Mark Brown,
> 
> Please don't top post, reply in line with needed context.  This allows readers to
> readily follow the flow of conversation and understand what you are talking
> about and also helps ensure that everything in the discussion is being
> addressed.
> 
> Please fix your mail client to word wrap within paragraphs at something
> substantially less than 80 columns.  Doing this makes your messages much
> easier to read and reply to.
> 

Ok, Thanks.

> > There are two problems we have found for use regmap up to now.
> 
> > 1, Regmap will read device value depends on endian in .dtsi(DSPI in .dtsi file is
> big-endian) and readl()(readl() in arm64 is le32_to_cpu read) in system.
> > That is the value read out is fixed regardless the kernel were compiled with
> CONFIG_CPU_BIG_ENDIAN yes or no. That makes the value(read from
> DSPI)can't be recognized in one endian mode(CONFIG_CPU_BIG_ENDIAN be
> set or not).
> > The result is: DSPI can't working with CONFIG_CPU_BIG_ENDIAN set.
> 
> This doesn't make sense.  If the endianness of the device is specified in DT then
> the endianness of the CPU won't make a difference.
> 

For example, if DSPI controller register is BE, so I set big-endian in DT.
That means we should R/W the DSPI controller register with big-endian.
Then I can think all of the value we R/W to/from DSPI controller register, we can
Think they are the BE.
So that we can understand it easy.

That means no matter core is LE or BE, we both need get the value from DSPI register
with big endian.

But from regmap is not.
When the core is little endian, I can get the big endian value form register like:0xaabbccdd.
Then we can analysis and process this value.
But once config the core to big endian, I get the value like:0xddccbbaa. It's the little endian.
That's terrible for my driver to understand the value.

> > 2, Sometimes regmap will generate some issues. Some regmap issues will
> cause the DSPI do not working.
> > At 2016.4 we find the DSPI was not work, then we found that the regmap
> R/W the controller register sometimes not right.
> > The result is: DSPI can't working with REGMAP API.
> > Although the regmap issues was fast fixed by some giving person. But I'm still
> tend to use the reliable way.
> 
> > Regmap is nice and Maybe I should try to fix regmap instead of replacing
> regmap in my driver.
> > But regmap is really very large and complex for DSPI driver.
> 
> Yes, if you find bugs in generic frameworks you should fix them rather than just
> open coding something. 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Leo Li]

On Mon, May 9, 2016 at 5:58 AM, Yao Yuan <yao.yuan-3arQi8VN3Tc@public.gmane.org> wrote:
> On Mon, May 09, 2016 at 06:29:59AM +0800, Mark Brown wrote:
>> On Mon, May 09, 2016 at 08:48:59AM +0000, Yao Yuan wrote:
>> > Hi Mark Brown,
>>
>> Please don't top post, reply in line with needed context.  This allows readers to
>> readily follow the flow of conversation and understand what you are talking
>> about and also helps ensure that everything in the discussion is being
>> addressed.
>>
>> Please fix your mail client to word wrap within paragraphs at something
>> substantially less than 80 columns.  Doing this makes your messages much
>> easier to read and reply to.
>>
>
> Ok, Thanks.
>
>> > There are two problems we have found for use regmap up to now.
>>
>> > 1, Regmap will read device value depends on endian in .dtsi(DSPI in .dtsi file is
>> big-endian) and readl()(readl() in arm64 is le32_to_cpu read) in system.
>> > That is the value read out is fixed regardless the kernel were compiled with
>> CONFIG_CPU_BIG_ENDIAN yes or no. That makes the value(read from
>> DSPI)can't be recognized in one endian mode(CONFIG_CPU_BIG_ENDIAN be
>> set or not).
>> > The result is: DSPI can't working with CONFIG_CPU_BIG_ENDIAN set.
>>
>> This doesn't make sense.  If the endianness of the device is specified in DT then
>> the endianness of the CPU won't make a difference.
>>
>
> For example, if DSPI controller register is BE, so I set big-endian in DT.
> That means we should R/W the DSPI controller register with big-endian.
> Then I can think all of the value we R/W to/from DSPI controller register, we can
> Think they are the BE.
> So that we can understand it easy.
>
> That means no matter core is LE or BE, we both need get the value from DSPI register
> with big endian.
>
> But from regmap is not.
> When the core is little endian, I can get the big endian value form register like:0xaabbccdd.
> Then we can analysis and process this value.
> But once config the core to big endian, I get the value like:0xddccbbaa. It's the little endian.
> That's terrible for my driver to understand the value.

This is just the result.  Are you able to dig deeper to explain why
this is happening?

I checked the LS1043 reference manual, the register definition looks
like little endian instead of the big endian defined in the LS1043
device tree.  Can you confirm if the block is actually having big
endian registers or little endian registers.

Regards,
Leo
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [linux-devel] [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Po Liu]

>  -----Original Message-----
>  From: linux-devel-bounces@gforge.freescale.net [mailto:linux-devel-
>  bounces@gforge.freescale.net] On Behalf Of Leo Li
>  Sent: Thursday, May 12, 2016 6:20 AM
>  To: Yao Yuan
>  Cc: devicetree@vger.kernel.org; pawel.moll@arm.com; robh+dt@kernel.org;
>  linux-spi@vger.kernel.org; Mark Brown; linux-devel@gforge.freescale.net;
>  Scott Wood; Yang-Leo Li; shawnguo@kernel.org
>  Subject: Re: [linux-devel] [PATCH 1/2] spi: spi-fsl-dspi: replace regmap
>  R/W with internal implementation
>  
>  On Mon, May 9, 2016 at 5:58 AM, Yao Yuan <yao.yuan@nxp.com> wrote:
>  > On Mon, May 09, 2016 at 06:29:59AM +0800, Mark Brown wrote:
>  >> On Mon, May 09, 2016 at 08:48:59AM +0000, Yao Yuan wrote:
>  >> > Hi Mark Brown,
>  >>
>  >> Please don't top post, reply in line with needed context.  This
>  >> allows readers to readily follow the flow of conversation and
>  >> understand what you are talking about and also helps ensure that
>  >> everything in the discussion is being addressed.
>  >>
>  >> Please fix your mail client to word wrap within paragraphs at
>  >> something substantially less than 80 columns.  Doing this makes your
>  >> messages much easier to read and reply to.
>  >>
>  >
>  > Ok, Thanks.
>  >
>  >> > There are two problems we have found for use regmap up to now.
>  >>
>  >> > 1, Regmap will read device value depends on endian in .dtsi(DSPI in
>  >> > .dtsi file is
>  >> big-endian) and readl()(readl() in arm64 is le32_to_cpu read) in
>  system.
>  >> > That is the value read out is fixed regardless the kernel were
>  >> > compiled with
>  >> CONFIG_CPU_BIG_ENDIAN yes or no. That makes the value(read from
>  >> DSPI)can't be recognized in one endian mode(CONFIG_CPU_BIG_ENDIAN be
>  >> set or not).
>  >> > The result is: DSPI can't working with CONFIG_CPU_BIG_ENDIAN set.
>  >>
>  >> This doesn't make sense.  If the endianness of the device is
>  >> specified in DT then the endianness of the CPU won't make a
>  difference.
>  >>
>  >
>  > For example, if DSPI controller register is BE, so I set big-endian in
>  DT.
>  > That means we should R/W the DSPI controller register with big-endian.
>  > Then I can think all of the value we R/W to/from DSPI controller
>  > register, we can Think they are the BE.
>  > So that we can understand it easy.
>  >
>  > That means no matter core is LE or BE, we both need get the value from
>  > DSPI register with big endian.
>  >
>  > But from regmap is not.
>  > When the core is little endian, I can get the big endian value form
>  register like:0xaabbccdd.
>  > Then we can analysis and process this value.
>  > But once config the core to big endian, I get the value
>  like:0xddccbbaa. It's the little endian.
>  > That's terrible for my driver to understand the value.
>  
>  This is just the result.  Are you able to dig deeper to explain why this
>  is happening?
spi/spi-fsl-dspi.c use the devm_regmap_init_mmio_clk() get regmap which will use regmap_mmio for operation the registers.

static struct regmap_bus regmap_mmio = {
	...
	.write = regmap_mmio_write,
	.read = regmap_mmio_read,
}

Anyway, the whole process register read operation will include regmap_mmio_read and format.parse_val(map->work_buf) then get the value.
	regmap_mmio_read() will use le32_to_cpu() value, 
	format.parse_val() will use cpu_to_be32() (if .dtsi file config the dspi with big_endian, else use cpu_to_le32() if set little_endian )

So for kernel we get the value will always le32_to_cpu() + cpu_to_be32(), in short, the value always get value from register le32_to_be32 read. This operation could not be recognized for both big_endian and little_endian core setting. Founded that it will hang in one type of CPU ENDIAN mode.

>  
>  I checked the LS1043 reference manual, the register definition looks
>  like little endian instead of the big endian defined in the LS1043
>  device tree.  Can you confirm if the block is actually having big endian
>  registers or little endian registers.
>  
>  Regards,
>  Leo
>  _______________________________________________
>  linux-devel mailing list
>  linux-devel@gforge.freescale.net
>  http://gforge.freescale.net/mailman/listinfo/linux-devel

RE: [linux-devel] [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Yao Yuan]

On Thu, May 12, 2016 at 09:41 OM, Po Liu wrote:
> On Thu, May12, 2016 at 06:20 AM, Leo Li Wrote:
> >  On Mon, May 9, 2016 at 5:58 AM, Yao Yuan <yao.yuan@nxp.com> wrote:
> >  > On Mon, May 09, 2016 at 06:29:59AM +0800, Mark Brown wrote:
> >  >> On Mon, May 09, 2016 at 08:48:59AM +0000, Yao Yuan wrote:
> >  >> > Hi Mark Brown,
> >  >>
> >  >> Please don't top post, reply in line with needed context.  This
> > >> allows readers to readily follow the flow of conversation and  >>
> > understand what you are talking about and also helps ensure that  >>
> > everything in the discussion is being addressed.
> >  >>
> >  >> Please fix your mail client to word wrap within paragraphs at  >>
> > something substantially less than 80 columns.  Doing this makes your
> > >> messages much easier to read and reply to.
> >  >>
> >  >
> >  > Ok, Thanks.
> >  >
> >  >> > There are two problems we have found for use regmap up to now.
> >  >>
> >  >> > 1, Regmap will read device value depends on endian in .dtsi(DSPI
> > in  >> > .dtsi file is  >> big-endian) and readl()(readl() in arm64 is
> > le32_to_cpu read) in  system.
> >  >> > That is the value read out is fixed regardless the kernel were
> > >> > compiled with  >> CONFIG_CPU_BIG_ENDIAN yes or no. That makes the
> > value(read from  >> DSPI)can't be recognized in one endian
> > mode(CONFIG_CPU_BIG_ENDIAN be  >> set or not).
> >  >> > The result is: DSPI can't working with CONFIG_CPU_BIG_ENDIAN set.
> >  >>
> >  >> This doesn't make sense.  If the endianness of the device is  >>
> > specified in DT then the endianness of the CPU won't make a
> > difference.
> >  >>
> >  >
> >  > For example, if DSPI controller register is BE, so I set big-endian
> > in  DT.
> >  > That means we should R/W the DSPI controller register with big-endian.
> >  > Then I can think all of the value we R/W to/from DSPI controller  >
> > register, we can Think they are the BE.
> >  > So that we can understand it easy.
> >  >
> >  > That means no matter core is LE or BE, we both need get the value
> > from  > DSPI register with big endian.
> >  >
> >  > But from regmap is not.
> >  > When the core is little endian, I can get the big endian value form
> > register like:0xaabbccdd.
> >  > Then we can analysis and process this value.
> >  > But once config the core to big endian, I get the value
> > like:0xddccbbaa. It's the little endian.
> >  > That's terrible for my driver to understand the value.
> >
> >  This is just the result.  Are you able to dig deeper to explain why
> > this  is happening?
> spi/spi-fsl-dspi.c use the devm_regmap_init_mmio_clk() get regmap which will
> use regmap_mmio for operation the registers.
> 
> static struct regmap_bus regmap_mmio = {
> 	...
> 	.write = regmap_mmio_write,
> 	.read = regmap_mmio_read,
> }
> 
> Anyway, the whole process register read operation will include
> regmap_mmio_read and format.parse_val(map->work_buf) then get the
> value.
> 	regmap_mmio_read() will use le32_to_cpu() value,
> 	format.parse_val() will use cpu_to_be32() (if .dtsi file config the dspi
> with big_endian, else use cpu_to_le32() if set little_endian )
> 
> So for kernel we get the value will always le32_to_cpu() + cpu_to_be32(), in
> short, the value always get value from register le32_to_be32 read. This
> operation could not be recognized for both big_endian and little_endian core
> setting. Founded that it will hang in one type of CPU ENDIAN mode.

Yes, That's the reason, Thanks for you to explain.

> 
> >
> >  I checked the LS1043 reference manual, the register definition looks
> > like little endian instead of the big endian defined in the LS1043
> > device tree.  Can you confirm if the block is actually having big
> > endian  registers or little endian registers.

No, it should be the document issue.
The register for DSPI on LS1043A is still big-endian.
Maybe will changed to little-endian in some later new SOC.

> >
> >  Regards,
> >  Leo
> >  _______________________________________________
> >  linux-devel mailing list
> >  linux-devel@gforge.freescale.net
> >  http://gforge.freescale.net/mailman/listinfo/linux-devel

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On Mon, May 09, 2016 at 10:58:08AM +0000, Yao Yuan wrote:

> For example, if DSPI controller register is BE, so I set big-endian in DT.
> That means we should R/W the DSPI controller register with big-endian.
> Then I can think all of the value we R/W to/from DSPI controller register, we can
> Think they are the BE.
> So that we can understand it easy.

> That means no matter core is LE or BE, we both need get the value from DSPI register
> with big endian.

That's not how regmap is intended to be used, the intention is that the
driver will interact with native endian values and regmap will hide the
endianness changes.  You could mask this with some cpu_to_be() and
be_to_cpu() usage but I'm not sure it's a great idea or what it's buying
you.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Leo Li]

On Thu, May 12, 2016 at 11:31 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, May 09, 2016 at 10:58:08AM +0000, Yao Yuan wrote:
>
>> For example, if DSPI controller register is BE, so I set big-endian in DT.
>> That means we should R/W the DSPI controller register with big-endian.
>> Then I can think all of the value we R/W to/from DSPI controller register, we can
>> Think they are the BE.
>> So that we can understand it easy.
>
>> That means no matter core is LE or BE, we both need get the value from DSPI register
>> with big endian.
>
> That's not how regmap is intended to be used, the intention is that the
> driver will interact with native endian values and regmap will hide the
> endianness changes.  You could mask this with some cpu_to_be() and
> be_to_cpu() usage but I'm not sure it's a great idea or what it's buying
> you.

So you do agree that the DSPI driver shouldn't use the regmap APIs?
It is still confusing to me on what is the intended behavior for the
endianness property in device tree.  Previously for regmap binding the
default endian is the native-endian if no endian property is not
present.  But for bindings of many devices including the
Documentation/devicetree/bindings/common-properties.txt file, the
default endian is the endian when the device is first used in the dts
binding.  For example, the FSL/NXP IFC device was first used as a
big-endian device for PowerPC SoCs.  The default endian should be
big-endian no matter if it is used on PowerPC or ARM later.  It
shouldn't be default to little-endian on an ARM SoC.

Regards,
Leo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

On Thu, May 12, 2016 at 06:55:38PM -0500, Leo Li wrote:
> On Thu, May 12, 2016 at 11:31 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Mon, May 09, 2016 at 10:58:08AM +0000, Yao Yuan wrote:

> > That's not how regmap is intended to be used, the intention is that the
> > driver will interact with native endian values and regmap will hide the
> > endianness changes.  You could mask this with some cpu_to_be() and
> > be_to_cpu() usage but I'm not sure it's a great idea or what it's buying
> > you.

> So you do agree that the DSPI driver shouldn't use the regmap APIs?

No, of course not!  The driver's usage just appears to be very confused.

> It is still confusing to me on what is the intended behavior for the
> endianness property in device tree.  Previously for regmap binding the

What is confusing?  If the device is big endian the driver should
specify big endian.  If the device is little endian the driver should
specify little endian.  If the device is native endian the driver should
specify native endian.  Regardless of what the endianness of the device
is the driver should always use native endian when talking to the regmap
APIs.

> default endian is the native-endian if no endian property is not
> present.  But for bindings of many devices including the

No, the default is little endian (it always has been, though for a long
time only by accident and now the documentation matches that).  If this
was causing problems you should have reported it rather than trying to
hack around it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Scott Wood]

On 05/12/2016 11:31 AM, Mark Brown wrote:
> On Mon, May 09, 2016 at 10:58:08AM +0000, Yao Yuan wrote:
> 
>> For example, if DSPI controller register is BE, so I set big-endian in DT.
>> That means we should R/W the DSPI controller register with big-endian.
>> Then I can think all of the value we R/W to/from DSPI controller register, we can
>> Think they are the BE.
>> So that we can understand it easy.
> 
>> That means no matter core is LE or BE, we both need get the value from DSPI register
>> with big endian.
> 
> That's not how regmap is intended to be used, the intention is that the
> driver will interact with native endian values and regmap will hide the
> endianness changes.  You could mask this with some cpu_to_be() and
> be_to_cpu() usage but I'm not sure it's a great idea or what it's buying
> you.

I don't think that's what Yao Yuan meant.  The driver wants the I/O
accessor to return native-endian values, just like any other driver.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Fri, May 13, 2016 at 02:29:03AM +0000, Scott Wood wrote:
> On 05/12/2016 11:31 AM, Mark Brown wrote:

> >> That means no matter core is LE or BE, we both need get the value from DSPI register
> >> with big endian.

> > That's not how regmap is intended to be used, the intention is that the
> > driver will interact with native endian values and regmap will hide the
> > endianness changes.  You could mask this with some cpu_to_be() and
> > be_to_cpu() usage but I'm not sure it's a great idea or what it's buying
> > you.

> I don't think that's what Yao Yuan meant.  The driver wants the I/O
> accessor to return native-endian values, just like any other driver.

This really isn't clear to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Scott Wood]

On 05/09/2016 05:29 AM, Mark Brown wrote:
> On Mon, May 09, 2016 at 08:48:59AM +0000, Yao Yuan wrote:
>> 2, Sometimes regmap will generate some issues. Some regmap issues will cause the DSPI do not working.
>> At 2016.4 we find the DSPI was not work, then we found that the regmap R/W the controller register sometimes not right. 
>> The result is: DSPI can't working with REGMAP API.
>> Although the regmap issues was fast fixed by some giving person. But I'm still tend to use the reliable way.
> 
>> Regmap is nice and Maybe I should try to fix regmap instead of replacing regmap in my driver.
>> But regmap is really very large and complex for DSPI driver.
> 
> Yes, if you find bugs in generic frameworks you should fix them rather
> than just open coding something.  

That's often true, but using regmap to handle endianness is like
swatting flies with a sledgehammer.  Regmap's code is quite (over?)
complicated (e.g. the (user-visible!) difference between
reg_format_endian and val_format_endian is quite confusing) and ideally
people who aren't getting actual value from a buggy and/or awkward
subsystem shouldn't be forced to use/fix it.  We're not talking about
massive duplication here.  We're talking about a set of simple I/O
wrappers (something that many drivers do) with an if-statement.

That said, Yao Yuan, can you try linux-next to see if the regmap
problems have been fixed?  Looking at Linus's tree I don't see how
regmap-mmio would ever give you anything but little-endian if the driver
doesn't specify an endianness (the device tree is only looked at by
regmap_get_val_endian() which wasn't called for regmap-mmio), but that
appears to be fixed in linux-next.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

On Fri, May 13, 2016 at 03:08:55AM +0000, Scott Wood wrote:
> On 05/09/2016 05:29 AM, Mark Brown wrote:

> > Yes, if you find bugs in generic frameworks you should fix them rather
> > than just open coding something.  

> That's often true, but using regmap to handle endianness is like
> swatting flies with a sledgehammer.  Regmap's code is quite (over?)
> complicated (e.g. the (user-visible!) difference between
> reg_format_endian and val_format_endian is quite confusing) and ideally

This is the register map API, not the register API - many buses,
including MMIO, have different endiannesses for their values and
addresses so of course we need to let users control that.

> people who aren't getting actual value from a buggy and/or awkward
> subsystem shouldn't be forced to use/fix it.  We're not talking about
> massive duplication here.  We're talking about a set of simple I/O
> wrappers (something that many drivers do) with an if-statement.

As far as I am aware any issues have been fixed by other users who
reported the problems they were seeing (which hasn't happened here).
When we encounter problems it is not OK to just bodge around the issue
in a driver without reporting them.  That doesn't help other users and
makes everything more fragile.

If there were some history or other indication of real problems here the
story would be different but if that's been happening it hasn't ever
been communicated.  As it is it looks like people have been using the
diagnostic tools (max_register is specified) and clock management
provided by regmap and the way this has been communicated is causing me
concern.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Li Yang]

On Wed, Apr 27, 2016 at 3:12 AM, Yuan Yao <yao.yuan-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> From: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>
>
> The qSPI controller's endian is independent of the CPU core's endian.

DSPI?

>
> For eg, Core on NXP LS1043A SoC is little endian but DSPI is big endian
> whereas Core on LS2080A SoC is little endian and DSPI also is little
> endian on the same core.

It's not correct to say the SoC is little endian for LS1043 or LS2080.
Not all devices on the SoC are sharing the same endianness and the
core endianness is configurable.

>
> At first we use regmap to cover this issue.
> But the regmap is designed too large and complex.
> The issue for regmap often effect the DSPI's stability.

This is not stability issue but functional issue.

>
> DSPI driver just only need a effective way to R/W the controller register
> with BE or LE mode.
>
> So it's better to packaging a sample function.
> This will make the DSPI driver more stability and high effective.
>
> Signed-off-by: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 99 +++++++++++++++++++++++++++-------------------
>  1 file changed, 59 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 9e9dadb..9a61572 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -27,7 +27,6 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/regmap.h>
>  #include <linux/sched.h>
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi_bitbang.h>
> @@ -143,7 +142,7 @@ struct fsl_dspi {
>         struct spi_master       *master;
>         struct platform_device  *pdev;
>
> -       struct regmap           *regmap;
> +       void __iomem            *iobase;
>         int                     irq;
>         struct clk              *clk;
>
> @@ -165,13 +164,46 @@ struct fsl_dspi {
>         u32                     waitflags;
>
>         u32                     spi_tcnt;
> +       bool                    big_endian;
>  };
>
> +/*
> + * R/W functions for big- or little-endian registers:
> + * The qSPI controller's endian is independent of the CPU core's endian.

DSPI?

> + */
> +static void dspi_writel(struct fsl_dspi *d, u32 val, u32 offset)
> +{
> +       if (d->big_endian)
> +               iowrite32be(val, d->iobase + offset);
> +       else
> +               iowrite32(val, d->iobase + offset);
> +}
> +
> +static u32 dspi_readl(struct fsl_dspi *d, u32 offset)
> +{
> +       if (d->big_endian)
> +               return ioread32be(d->iobase + offset);
> +       else
> +               return ioread32(d->iobase + offset);
> +}
> +
> +static void dspi_updatel(struct fsl_dspi *d, u32 mask, u32 val, u32 offset)
> +{
> +       u32 tmp, orig;
> +
> +       orig = dspi_readl(d, offset);
> +
> +       tmp = orig & ~mask;
> +       tmp |= val & mask;
> +
> +       dspi_writel(d, tmp, offset);
> +}
> +
>  static inline int is_double_byte_mode(struct fsl_dspi *dspi)
>  {
>         unsigned int val;
>
> -       regmap_read(dspi->regmap, SPI_CTAR(0), &val);
> +       val = dspi_readl(dspi, SPI_CTAR(0));
>
>         return ((val & SPI_FRAME_BITS_MASK) == SPI_FRAME_BITS(8)) ? 0 : 1;
>  }
> @@ -270,7 +302,7 @@ static void dspi_data_from_popr(struct fsl_dspi *dspi, int rx_word)
>         u16 d;
>         unsigned int val;
>
> -       regmap_read(dspi->regmap, SPI_POPR, &val);
> +       val = dspi_readl(dspi, SPI_POPR);
>         d = SPI_POPR_RXDATA(val);
>
>         if (!(dspi->dataflags & TRAN_STATE_RX_VOID))
> @@ -294,8 +326,8 @@ static int dspi_eoq_write(struct fsl_dspi *dspi)
>                  */
>                 if (tx_word && (dspi->len == 1)) {
>                         dspi->dataflags |= TRAN_STATE_WORD_ODD_NUM;
> -                       regmap_update_bits(dspi->regmap, SPI_CTAR(0),
> -                                       SPI_FRAME_BITS_MASK, SPI_FRAME_BITS(8));
> +                       dspi_updatel(dspi, SPI_FRAME_BITS_MASK,
> +                                       SPI_FRAME_BITS(8), SPI_CTAR(0));
>                         tx_word = 0;
>                 }
>
> @@ -309,7 +341,7 @@ static int dspi_eoq_write(struct fsl_dspi *dspi)
>                 } else if (tx_word && (dspi->len == 1))
>                         dspi_pushr |= SPI_PUSHR_EOQ;
>
> -               regmap_write(dspi->regmap, SPI_PUSHR, dspi_pushr);
> +               dspi_writel(dspi, dspi_pushr, SPI_PUSHR);
>
>                 tx_count++;
>         }
> @@ -343,8 +375,8 @@ static int dspi_tcfq_write(struct fsl_dspi *dspi)
>
>         if (tx_word && (dspi->len == 1)) {
>                 dspi->dataflags |= TRAN_STATE_WORD_ODD_NUM;
> -               regmap_update_bits(dspi->regmap, SPI_CTAR(0),
> -                               SPI_FRAME_BITS_MASK, SPI_FRAME_BITS(8));
> +               dspi_updatel(dspi, SPI_FRAME_BITS_MASK,
> +                               SPI_FRAME_BITS(8), SPI_CTAR(0));
>                 tx_word = 0;
>         }
>
> @@ -353,7 +385,7 @@ static int dspi_tcfq_write(struct fsl_dspi *dspi)
>         if ((dspi->cs_change) && (!dspi->len))
>                 dspi_pushr &= ~SPI_PUSHR_CONT;
>
> -       regmap_write(dspi->regmap, SPI_PUSHR, dspi_pushr);
> +       dspi_writel(dspi, dspi_pushr, SPI_PUSHR);
>
>         return tx_word + 1;
>  }
> @@ -378,7 +410,7 @@ static int dspi_transfer_one_message(struct spi_master *master,
>         enum dspi_trans_mode trans_mode;
>         u32 spi_tcr;
>
> -       regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
> +       spi_tcr = dspi_readl(dspi, SPI_TCR);
>         dspi->spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
>
>         message->actual_length = 0;
> @@ -407,21 +439,19 @@ static int dspi_transfer_one_message(struct spi_master *master,
>                 if (!dspi->tx)
>                         dspi->dataflags |= TRAN_STATE_TX_VOID;
>
> -               regmap_write(dspi->regmap, SPI_MCR, dspi->cur_chip->mcr_val);
> -               regmap_update_bits(dspi->regmap, SPI_MCR,
> -                               SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF,
> -                               SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF);
> -               regmap_write(dspi->regmap, SPI_CTAR(0),
> -                               dspi->cur_chip->ctar_val);
> +               dspi_writel(dspi, dspi->cur_chip->mcr_val, SPI_MCR);
> +               dspi_updatel(dspi, SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF,
> +                                  SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF, SPI_MCR);
> +               dspi_writel(dspi, dspi->cur_chip->ctar_val, SPI_CTAR(0));
>
>                 trans_mode = dspi->devtype_data->trans_mode;
>                 switch (trans_mode) {
>                 case DSPI_EOQ_MODE:
> -                       regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_EOQFE);
> +                       dspi_writel(dspi, SPI_RSER_EOQFE, SPI_RSER);
>                         dspi_eoq_write(dspi);
>                         break;
>                 case DSPI_TCFQ_MODE:
> -                       regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_TCFQE);
> +                       dspi_writel(dspi, SPI_RSER_TCFQE, SPI_RSER);
>                         dspi_tcfq_write(dspi);
>                         break;
>                 default:
> @@ -525,14 +555,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>         u32 spi_tcnt, tcnt_diff;
>         int tx_word;
>
> -       regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> -       regmap_write(dspi->regmap, SPI_SR, spi_sr);
> +       spi_sr = dspi_readl(dspi, SPI_SR);
> +       dspi_writel(dspi, spi_sr, SPI_SR);
>
>
>         if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)) {
>                 tx_word = is_double_byte_mode(dspi);
>
> -               regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
> +               spi_tcr = dspi_readl(dspi, SPI_TCR);
>                 spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
>                 /*
>                  * The width of SPI Transfer Counter in SPI_TCR is 16bits,
> @@ -569,10 +599,10 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>
>                 if (!dspi->len) {
>                         if (dspi->dataflags & TRAN_STATE_WORD_ODD_NUM) {
> -                               regmap_update_bits(dspi->regmap,
> -                                                  SPI_CTAR(0),
> -                                                  SPI_FRAME_BITS_MASK,
> -                                                  SPI_FRAME_BITS(16));
> +                               dspi_updatel(dspi,
> +                                            SPI_FRAME_BITS_MASK,
> +                                            SPI_FRAME_BITS(16),
> +                                            SPI_CTAR(0));
>                                 dspi->dataflags &= ~TRAN_STATE_WORD_ODD_NUM;
>                         }
>
> @@ -636,13 +666,6 @@ static int dspi_resume(struct device *dev)
>
>  static SIMPLE_DEV_PM_OPS(dspi_pm, dspi_suspend, dspi_resume);
>
> -static const struct regmap_config dspi_regmap_config = {
> -       .reg_bits = 32,
> -       .val_bits = 32,
> -       .reg_stride = 4,
> -       .max_register = 0x88,
> -};
> -
>  static int dspi_probe(struct platform_device *pdev)
>  {
>         struct device_node *np = pdev->dev.of_node;
> @@ -700,13 +723,7 @@ static int dspi_probe(struct platform_device *pdev)
>                 goto out_master_put;
>         }
>
> -       dspi->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
> -                                               &dspi_regmap_config);
> -       if (IS_ERR(dspi->regmap)) {
> -               dev_err(&pdev->dev, "failed to init regmap: %ld\n",
> -                               PTR_ERR(dspi->regmap));
> -               return PTR_ERR(dspi->regmap);
> -       }
> +       dspi->iobase = base;
>
>         dspi->irq = platform_get_irq(pdev, 0);
>         if (dspi->irq < 0) {
> @@ -730,6 +747,8 @@ static int dspi_probe(struct platform_device *pdev)
>         }
>         clk_prepare_enable(dspi->clk);
>
> +       dspi->big_endian = of_property_read_bool(np, "big-endian");
> +
>         master->max_speed_hz =
>                 clk_get_rate(dspi->clk) / dspi->devtype_data->max_clock_factor;
>
> --
> 2.1.0.27.g96db324
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] spi: spi-fsl-dspi: replace regmap R/W with internal implementation

[Yao Yuan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11553 bytes --]

On Wed, May 04, 2016 at 5:33 AM, pku.leo@gmail.com wrote:
> On Wed, Apr 27, 2016 at 3:12 AM, Yuan Yao <yao.yuan@freescale.com> wrote:
> > From: Yuan Yao <yao.yuan@nxp.com>
> >
> > The qSPI controller's endian is independent of the CPU core's endian.
> 
> DSPI?

Thanks for your review.
Sorry for the typo, I will update in the next version.
 
> >
> > For eg, Core on NXP LS1043A SoC is little endian but DSPI is big
> > endian whereas Core on LS2080A SoC is little endian and DSPI also is
> > little endian on the same core.
> 
> It's not correct to say the SoC is little endian for LS1043 or LS2080.
> Not all devices on the SoC are sharing the same endianness and the core
> endianness is configurable.

Ok, I will update it.
 

> 
> >
> > At first we use regmap to cover this issue.
> > But the regmap is designed too large and complex.
> > The issue for regmap often effect the DSPI's stability.
> 
> This is not stability issue but functional issue.

Ok, I will update it.

> 
> >
> > DSPI driver just only need a effective way to R/W the controller
> > register with BE or LE mode.
> >
> > So it's better to packaging a sample function.
> > This will make the DSPI driver more stability and high effective.
> >
> > Signed-off-by: Yuan Yao <yao.yuan@nxp.com>
> > ---
> >  drivers/spi/spi-fsl-dspi.c | 99
> > +++++++++++++++++++++++++++-------------------
> >  1 file changed, 59 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > index 9e9dadb..9a61572 100644
> > --- a/drivers/spi/spi-fsl-dspi.c
> > +++ b/drivers/spi/spi-fsl-dspi.c
> > @@ -27,7 +27,6 @@
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > -#include <linux/regmap.h>
> >  #include <linux/sched.h>
> >  #include <linux/spi/spi.h>
> >  #include <linux/spi/spi_bitbang.h>
> > @@ -143,7 +142,7 @@ struct fsl_dspi {
> >         struct spi_master       *master;
> >         struct platform_device  *pdev;
> >
> > -       struct regmap           *regmap;
> > +       void __iomem            *iobase;
> >         int                     irq;
> >         struct clk              *clk;
> >
> > @@ -165,13 +164,46 @@ struct fsl_dspi {
> >         u32                     waitflags;
> >
> >         u32                     spi_tcnt;
> > +       bool                    big_endian;
> >  };
> >
> > +/*
> > + * R/W functions for big- or little-endian registers:
> > + * The qSPI controller's endian is independent of the CPU core's endian.
> 
> DSPI?

Yes, DSPI, I will update it.
> 
> > + */
> > +static void dspi_writel(struct fsl_dspi *d, u32 val, u32 offset) {
> > +       if (d->big_endian)
> > +               iowrite32be(val, d->iobase + offset);
> > +       else
> > +               iowrite32(val, d->iobase + offset); }
> > +
> > +static u32 dspi_readl(struct fsl_dspi *d, u32 offset) {
> > +       if (d->big_endian)
> > +               return ioread32be(d->iobase + offset);
> > +       else
> > +               return ioread32(d->iobase + offset); }
> > +
> > +static void dspi_updatel(struct fsl_dspi *d, u32 mask, u32 val, u32
> > +offset) {
> > +       u32 tmp, orig;
> > +
> > +       orig = dspi_readl(d, offset);
> > +
> > +       tmp = orig & ~mask;
> > +       tmp |= val & mask;
> > +
> > +       dspi_writel(d, tmp, offset);
> > +}
> > +
> >  static inline int is_double_byte_mode(struct fsl_dspi *dspi)  {
> >         unsigned int val;
> >
> > -       regmap_read(dspi->regmap, SPI_CTAR(0), &val);
> > +       val = dspi_readl(dspi, SPI_CTAR(0));
> >
> >         return ((val & SPI_FRAME_BITS_MASK) == SPI_FRAME_BITS(8)) ? 0
> > : 1;  } @@ -270,7 +302,7 @@ static void dspi_data_from_popr(struct
> > fsl_dspi *dspi, int rx_word)
> >         u16 d;
> >         unsigned int val;
> >
> > -       regmap_read(dspi->regmap, SPI_POPR, &val);
> > +       val = dspi_readl(dspi, SPI_POPR);
> >         d = SPI_POPR_RXDATA(val);
> >
> >         if (!(dspi->dataflags & TRAN_STATE_RX_VOID)) @@ -294,8 +326,8
> > @@ static int dspi_eoq_write(struct fsl_dspi *dspi)
> >                  */
> >                 if (tx_word && (dspi->len == 1)) {
> >                         dspi->dataflags |= TRAN_STATE_WORD_ODD_NUM;
> > -                       regmap_update_bits(dspi->regmap, SPI_CTAR(0),
> > -                                       SPI_FRAME_BITS_MASK, SPI_FRAME_BITS(8));
> > +                       dspi_updatel(dspi, SPI_FRAME_BITS_MASK,
> > +                                       SPI_FRAME_BITS(8),
> > + SPI_CTAR(0));
> >                         tx_word = 0;
> >                 }
> >
> > @@ -309,7 +341,7 @@ static int dspi_eoq_write(struct fsl_dspi *dspi)
> >                 } else if (tx_word && (dspi->len == 1))
> >                         dspi_pushr |= SPI_PUSHR_EOQ;
> >
> > -               regmap_write(dspi->regmap, SPI_PUSHR, dspi_pushr);
> > +               dspi_writel(dspi, dspi_pushr, SPI_PUSHR);
> >
> >                 tx_count++;
> >         }
> > @@ -343,8 +375,8 @@ static int dspi_tcfq_write(struct fsl_dspi *dspi)
> >
> >         if (tx_word && (dspi->len == 1)) {
> >                 dspi->dataflags |= TRAN_STATE_WORD_ODD_NUM;
> > -               regmap_update_bits(dspi->regmap, SPI_CTAR(0),
> > -                               SPI_FRAME_BITS_MASK, SPI_FRAME_BITS(8));
> > +               dspi_updatel(dspi, SPI_FRAME_BITS_MASK,
> > +                               SPI_FRAME_BITS(8), SPI_CTAR(0));
> >                 tx_word = 0;
> >         }
> >
> > @@ -353,7 +385,7 @@ static int dspi_tcfq_write(struct fsl_dspi *dspi)
> >         if ((dspi->cs_change) && (!dspi->len))
> >                 dspi_pushr &= ~SPI_PUSHR_CONT;
> >
> > -       regmap_write(dspi->regmap, SPI_PUSHR, dspi_pushr);
> > +       dspi_writel(dspi, dspi_pushr, SPI_PUSHR);
> >
> >         return tx_word + 1;
> >  }
> > @@ -378,7 +410,7 @@ static int dspi_transfer_one_message(struct
> spi_master *master,
> >         enum dspi_trans_mode trans_mode;
> >         u32 spi_tcr;
> >
> > -       regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
> > +       spi_tcr = dspi_readl(dspi, SPI_TCR);
> >         dspi->spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
> >
> >         message->actual_length = 0;
> > @@ -407,21 +439,19 @@ static int dspi_transfer_one_message(struct
> spi_master *master,
> >                 if (!dspi->tx)
> >                         dspi->dataflags |= TRAN_STATE_TX_VOID;
> >
> > -               regmap_write(dspi->regmap, SPI_MCR, dspi->cur_chip->mcr_val);
> > -               regmap_update_bits(dspi->regmap, SPI_MCR,
> > -                               SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF,
> > -                               SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF);
> > -               regmap_write(dspi->regmap, SPI_CTAR(0),
> > -                               dspi->cur_chip->ctar_val);
> > +               dspi_writel(dspi, dspi->cur_chip->mcr_val, SPI_MCR);
> > +               dspi_updatel(dspi, SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF,
> > +                                  SPI_MCR_CLR_TXF | SPI_MCR_CLR_RXF, SPI_MCR);
> > +               dspi_writel(dspi, dspi->cur_chip->ctar_val,
> > + SPI_CTAR(0));
> >
> >                 trans_mode = dspi->devtype_data->trans_mode;
> >                 switch (trans_mode) {
> >                 case DSPI_EOQ_MODE:
> > -                       regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_EOQFE);
> > +                       dspi_writel(dspi, SPI_RSER_EOQFE, SPI_RSER);
> >                         dspi_eoq_write(dspi);
> >                         break;
> >                 case DSPI_TCFQ_MODE:
> > -                       regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_TCFQE);
> > +                       dspi_writel(dspi, SPI_RSER_TCFQE, SPI_RSER);
> >                         dspi_tcfq_write(dspi);
> >                         break;
> >                 default:
> > @@ -525,14 +555,14 @@ static irqreturn_t dspi_interrupt(int irq, void
> *dev_id)
> >         u32 spi_tcnt, tcnt_diff;
> >         int tx_word;
> >
> > -       regmap_read(dspi->regmap, SPI_SR, &spi_sr);
> > -       regmap_write(dspi->regmap, SPI_SR, spi_sr);
> > +       spi_sr = dspi_readl(dspi, SPI_SR);
> > +       dspi_writel(dspi, spi_sr, SPI_SR);
> >
> >
> >         if (spi_sr & (SPI_SR_EOQF | SPI_SR_TCFQF)) {
> >                 tx_word = is_double_byte_mode(dspi);
> >
> > -               regmap_read(dspi->regmap, SPI_TCR, &spi_tcr);
> > +               spi_tcr = dspi_readl(dspi, SPI_TCR);
> >                 spi_tcnt = SPI_TCR_GET_TCNT(spi_tcr);
> >                 /*
> >                  * The width of SPI Transfer Counter in SPI_TCR is
> > 16bits, @@ -569,10 +599,10 @@ static irqreturn_t dspi_interrupt(int
> > irq, void *dev_id)
> >
> >                 if (!dspi->len) {
> >                         if (dspi->dataflags & TRAN_STATE_WORD_ODD_NUM) {
> > -                               regmap_update_bits(dspi->regmap,
> > -                                                  SPI_CTAR(0),
> > -                                                  SPI_FRAME_BITS_MASK,
> > -                                                  SPI_FRAME_BITS(16));
> > +                               dspi_updatel(dspi,
> > +                                            SPI_FRAME_BITS_MASK,
> > +                                            SPI_FRAME_BITS(16),
> > +                                            SPI_CTAR(0));
> >                                 dspi->dataflags &= ~TRAN_STATE_WORD_ODD_NUM;
> >                         }
> >
> > @@ -636,13 +666,6 @@ static int dspi_resume(struct device *dev)
> >
> >  static SIMPLE_DEV_PM_OPS(dspi_pm, dspi_suspend, dspi_resume);
> >
> > -static const struct regmap_config dspi_regmap_config = {
> > -       .reg_bits = 32,
> > -       .val_bits = 32,
> > -       .reg_stride = 4,
> > -       .max_register = 0x88,
> > -};
> > -
> >  static int dspi_probe(struct platform_device *pdev)  {
> >         struct device_node *np = pdev->dev.of_node; @@ -700,13 +723,7
> > @@ static int dspi_probe(struct platform_device *pdev)
> >                 goto out_master_put;
> >         }
> >
> > -       dspi->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
> > -                                               &dspi_regmap_config);
> > -       if (IS_ERR(dspi->regmap)) {
> > -               dev_err(&pdev->dev, "failed to init regmap: %ld\n",
> > -                               PTR_ERR(dspi->regmap));
> > -               return PTR_ERR(dspi->regmap);
> > -       }
> > +       dspi->iobase = base;
> >
> >         dspi->irq = platform_get_irq(pdev, 0);
> >         if (dspi->irq < 0) {
> > @@ -730,6 +747,8 @@ static int dspi_probe(struct platform_device *pdev)
> >         }
> >         clk_prepare_enable(dspi->clk);
> >
> > +       dspi->big_endian = of_property_read_bool(np, "big-endian");
> > +
> >         master->max_speed_hz =
> >                 clk_get_rate(dspi->clk) /
> > dspi->devtype_data->max_clock_factor;
> >
> > --
> > 2.1.0.27.g96db324
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

[PATCH 2/2] spi: spi-fsl-dspi: Update DT binding documentation

[Yuan Yao]

From: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>

DSPI driver support the register on the controller with big endian
mode R/W. But not use the regmap API.
So update the binding documentation for "big-endian".

Signed-off-by: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
index 1ad0fe3..ff5893d 100644
--- a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
+++ b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
@@ -16,8 +16,7 @@ Required properties:
 
 Optional property:
 - big-endian: If present the dspi device's registers are implemented
-  in big endian mode, otherwise in native mode(same with CPU), for more
-  detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
+  in big endian mode.
 
 Optional SPI slave node properties:
 - fsl,spi-cs-sck-delay: a delay in nanoseconds between activating chip
-- 
2.1.0.27.g96db324

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Applied "spi: spi-fsl-dspi: Update DT binding documentation" to the spi tree

[Mark Brown]

The patch

   spi: spi-fsl-dspi: Update DT binding documentation

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From c145344763192042135ef6172b779baaf4a16c1e Mon Sep 17 00:00:00 2001
From: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>
Date: Wed, 27 Apr 2016 16:12:36 +0800
Subject: [PATCH] spi: spi-fsl-dspi: Update DT binding documentation

DSPI driver support the register on the controller with big endian
mode R/W. But not use the regmap API.
So update the binding documentation for "big-endian".

Signed-off-by: Yuan Yao <yao.yuan-3arQi8VN3Tc@public.gmane.org>
Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
index fa77f874e321..d2969d2ceb4b 100644
--- a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
+++ b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
@@ -13,8 +13,7 @@ Required properties:
 
 Optional property:
 - big-endian: If present the dspi device's registers are implemented
-  in big endian mode, otherwise in native mode(same with CPU), for more
-  detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
+  in big endian mode.
 
 Optional SPI slave node properties:
 - fsl,spi-cs-sck-delay: a delay in nanoseconds between activating chip
-- 
2.8.0.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html