[PATCH v2 1/2] spi:fsl-dspi:add support of DSPI IP in big endian

[PATCH v2 1/2] spi:fsl-dspi:add support of DSPI IP in big endian

[Chao Fu]

From: Chao Fu <B44548-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

Freescale DSPI module will have two endianess in diffrente platform,
but ARM is little endian. So when DSPI in big endian, core in little endian,
readl and writel can not adjust R/W register in this condition.
This patch will provide a new io R/W method for the device in two endianess.

Add big/little endian judgement macros:
  "#define DSPI_BITWISE16(d, v) (d->big_endian ? cpu_to_be16(v) : cpu_to_le16(v))
  "#define DSPI_BITWISE32(d, v) (d->big_endian ? cpu_to_be32(v) : cpu_to_le32(v))

Add functions:
  dspi_readb dspi_readw dspi_readl
  dspi_writeb dspi_writew dspi_writel

And remove some coding style not in standard.

Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Chao Fu      <b44548-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
Change in v2:
Make dspi_readx dspi_writex as inline functions.
Modify the issue of indentation in function is_double_byte_mode.
Add description about big endian in bindings document.
 
 .../devicetree/bindings/spi/spi-fsl-dspi.txt       |  2 +
 drivers/spi/spi-fsl-dspi.c                         | 87 +++++++++++++++++-----
 2 files changed, 71 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
index a1fb303..5376de4 100644
--- a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
+++ b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
@@ -10,6 +10,7 @@ Required properties:
 - pinctrl-names: must contain a "default" entry.
 - spi-num-chipselects : the number of the chipselect signals.
 - bus-num : the slave chip chipselect signal number.
+- big-endian : if DSPI modudle is big endian, the bool will be set in node.
 Example:
 
 dspi0@4002c000 {
@@ -24,6 +25,7 @@ dspi0@4002c000 {
 	bus-num = <0>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_dspi0_1>;
+	big-endian;
 	status = "okay";
 
 	sflash: at26df081a@0 {
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 8641b03..1cd6d79 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -31,6 +31,9 @@
 
 #define DRIVER_NAME "fsl-dspi"
 
+#define DSPI_BITWISE16(d, v)	(d->big_endian ? cpu_to_be16(v) : cpu_to_le16(v))
+#define DSPI_BITWISE32(d, v)	(d->big_endian ? cpu_to_be32(v) : cpu_to_le32(v))
+
 #define TRAN_STATE_RX_VOID		0x01
 #define TRAN_STATE_TX_VOID		0x02
 #define TRAN_STATE_WORD_ODD_NUM	0x04
@@ -112,6 +115,7 @@ struct fsl_dspi {
 	int			irq;
 	struct clk 		*clk;
 
+	bool			big_endian;
 	struct spi_transfer 	*cur_transfer;
 	struct chip_data	*cur_chip;
 	size_t			len;
@@ -127,20 +131,60 @@ struct fsl_dspi {
 	u32 			waitflags;
 };
 
+static inline u8 dspi_readb(void __iomem *addr)
+{
+	return readb(addr);
+}
+
+static inline u16 dspi_readw(void __iomem *addr)
+{
+	u16 __v = (__force u16) __raw_readw(addr);
+	__iormb();
+
+	return __v;
+}
+
+static inline u32 dspi_readl(void __iomem *addr)
+{
+	u32 __v = (__force u32) __raw_readl(addr);
+	__iormb();
+
+	return __v;
+}
+
+static inline void dspi_writeb(u8 val, void __iomem *addr)
+{
+	writeb(val, addr);
+}
+
+static inline void dspi_writew(u16 val, void __iomem *addr)
+{
+	__iowmb();
+	__raw_writew((__force u16) val, addr);
+}
+
+static inline void dspi_writel(u32 val, void __iomem *addr)
+{
+	__iowmb();
+	__raw_writel((__force u32) val, addr);
+}
+
+
 static inline int is_double_byte_mode(struct fsl_dspi *dspi)
 {
-	return ((readl(dspi->base + SPI_CTAR(dspi->cs)) & SPI_FRAME_BITS_MASK)
-			== SPI_FRAME_BITS(8)) ? 0 : 1;
+	u32 val;
+	val = DSPI_BITWISE32(dspi, dspi_readl(dspi->base + SPI_CTAR(dspi->cs)));
+	return ((val & SPI_FRAME_BITS_MASK) == SPI_FRAME_BITS(8)) ? 0 : 1;
 }
 
 static void set_bit_mode(struct fsl_dspi *dspi, unsigned char bits)
 {
-	u32 temp;
+	u32 tmp;
 
-	temp = readl(dspi->base + SPI_CTAR(dspi->cs));
-	temp &= ~SPI_FRAME_BITS_MASK;
-	temp |= SPI_FRAME_BITS(bits);
-	writel(temp, dspi->base + SPI_CTAR(dspi->cs));
+	tmp = DSPI_BITWISE32(dspi, dspi_readl(dspi->base + SPI_CTAR(dspi->cs)));
+	tmp &= ~SPI_FRAME_BITS_MASK;
+	tmp |= SPI_FRAME_BITS(bits);
+	dspi_writel(DSPI_BITWISE32(dspi, tmp), dspi->base + SPI_CTAR(dspi->cs));
 }
 
 static void hz_to_spi_baud(char *pbr, char *br, int speed_hz,
@@ -212,7 +256,6 @@ static int dspi_transfer_write(struct fsl_dspi *dspi)
 			dspi->len -= 2;
 		} else {
 			if (!(dspi->dataflags & TRAN_STATE_TX_VOID)) {
-
 				d8 = *(u8 *)dspi->tx;
 				dspi->tx++;
 			} else {
@@ -238,7 +281,8 @@ static int dspi_transfer_write(struct fsl_dspi *dspi)
 			dspi_pushr |= SPI_PUSHR_CTCNT; /* clear counter */
 		}
 
-		writel(dspi_pushr, dspi->base + SPI_PUSHR);
+		dspi_writel(DSPI_BITWISE32(dspi, dspi_pushr),
+				dspi->base + SPI_PUSHR);
 		tx_count++;
 	}
 
@@ -256,14 +300,15 @@ static int dspi_transfer_read(struct fsl_dspi *dspi)
 			if ((dspi->rx_end - dspi->rx) == 1)
 				break;
 
-			d = SPI_POPR_RXDATA(readl(dspi->base + SPI_POPR));
+			d = DSPI_BITWISE16(dspi,
+					dspi_readl(dspi->base + SPI_POPR));
 
 			if (!(dspi->dataflags & TRAN_STATE_RX_VOID))
 				*(u16 *)dspi->rx = d;
 			dspi->rx += 2;
 
 		} else {
-			d = SPI_POPR_RXDATA(readl(dspi->base + SPI_POPR));
+			d = dspi_readb(dspi->base + SPI_POPR);
 			if (!(dspi->dataflags & TRAN_STATE_RX_VOID))
 				*(u8 *)dspi->rx = d;
 			dspi->rx++;
@@ -295,12 +340,15 @@ static int dspi_txrx_transfer(struct spi_device *spi, struct spi_transfer *t)
 	if (!dspi->tx)
 		dspi->dataflags |= TRAN_STATE_TX_VOID;
 
-	writel(dspi->cur_chip->mcr_val, dspi->base + SPI_MCR);
-	writel(dspi->cur_chip->ctar_val, dspi->base + SPI_CTAR(dspi->cs));
-	writel(SPI_RSER_EOQFE, dspi->base + SPI_RSER);
+	dspi_writel(DSPI_BITWISE32(dspi, dspi->cur_chip->mcr_val),
+			dspi->base + SPI_MCR);
+	dspi_writel(DSPI_BITWISE32(dspi, dspi->cur_chip->ctar_val),
+			dspi->base + SPI_CTAR(dspi->cs));
+	dspi_writel(DSPI_BITWISE32(dspi, SPI_RSER_EOQFE),
+			dspi->base + SPI_RSER);
 
 	if (t->speed_hz)
-		writel(dspi->cur_chip->ctar_val,
+		dspi_writel(DSPI_BITWISE32(dspi, dspi->cur_chip->ctar_val),
 				dspi->base + SPI_CTAR(dspi->cs));
 
 	dspi_transfer_write(dspi);
@@ -315,7 +363,7 @@ static int dspi_txrx_transfer(struct spi_device *spi, struct spi_transfer *t)
 static void dspi_chipselect(struct spi_device *spi, int value)
 {
 	struct fsl_dspi *dspi = spi_master_get_devdata(spi->master);
-	u32 pushr = readl(dspi->base + SPI_PUSHR);
+	u32 pushr = DSPI_BITWISE32(dspi, dspi_readl(dspi->base + SPI_PUSHR));
 
 	switch (value) {
 	case BITBANG_CS_ACTIVE:
@@ -324,7 +372,7 @@ static void dspi_chipselect(struct spi_device *spi, int value)
 		pushr &= ~SPI_PUSHR_CONT;
 	}
 
-	writel(pushr, dspi->base + SPI_PUSHR);
+	dspi_writel(DSPI_BITWISE32(dspi, pushr), dspi->base + SPI_PUSHR);
 }
 
 static int dspi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
@@ -383,13 +431,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 {
 	struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
 
-	writel(SPI_SR_EOQF, dspi->base + SPI_SR);
+	dspi_writel(DSPI_BITWISE32(dspi, SPI_SR_EOQF), dspi->base + SPI_SR);
 
 	dspi_transfer_read(dspi);
 
 	if (!dspi->len) {
 		if (dspi->dataflags & TRAN_STATE_WORD_ODD_NUM)
 			set_bit_mode(dspi, 16);
+
 		dspi->waitflags = 1;
 		wake_up_interruptible(&dspi->waitq);
 	} else {
@@ -507,6 +556,8 @@ static int dspi_probe(struct platform_device *pdev)
 	init_waitqueue_head(&dspi->waitq);
 	platform_set_drvdata(pdev, dspi);
 
+	dspi->big_endian = of_property_read_bool(np, "big-endian");
+
 	ret = spi_bitbang_start(&dspi->bitbang);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Problem registering DSPI master\n");
-- 
1.8.4


--
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

[PATCH v2 2/2] spi:dspi:Remove some coding sytle not in standard

[Chao Fu]

From: Chao Fu <B44548-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

Remove some coding sytle not in standard in former code.

Signed-off-by: Chao Fu      <b44548-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
Change in v2:
	New.

 drivers/spi/spi-fsl-dspi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 1cd6d79..76cd521 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -113,10 +113,10 @@ struct fsl_dspi {
 
 	void __iomem		*base;
 	int			irq;
-	struct clk 		*clk;
+	struct clk		*clk;
 
 	bool			big_endian;
-	struct spi_transfer 	*cur_transfer;
+	struct spi_transfer	*cur_transfer;
 	struct chip_data	*cur_chip;
 	size_t			len;
 	void			*tx;
@@ -127,8 +127,8 @@ struct fsl_dspi {
 	u8			cs;
 	u16			void_write_data;
 
-	wait_queue_head_t 	waitq;
-	u32 			waitflags;
+	wait_queue_head_t	waitq;
+	u32			waitflags;
 };
 
 static inline u8 dspi_readb(void __iomem *addr)
-- 
1.8.4


--
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 v2 1/2] spi:fsl-dspi:add support of DSPI IP in big endian

[Mark Brown]

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

On Wed, Jan 08, 2014 at 02:14:14PM +0800, Chao Fu wrote:

This looks a lot nicer - a few things below though.

> +#define DSPI_BITWISE16(d, v)	(d->big_endian ? cpu_to_be16(v) : cpu_to_le16(v))
> +#define DSPI_BITWISE32(d, v)	(d->big_endian ? cpu_to_be32(v) : cpu_to_le32(v))

These should probably be inline for the same reason as the I/O
functions, perhaps even being done as part of the I/O functions.

> +static inline u16 dspi_readw(void __iomem *addr)
> +{
> +	u16 __v = (__force u16) __raw_readw(addr);
> +	__iormb();
> +
> +	return __v;
> +}

Why does this need a barrier and why does it not take care of the
endianness translation?  A quick glance through shows most if not all of
the callers doing the translation.

> +static inline void dspi_writew(u16 val, void __iomem *addr)
> +{
> +	__iowmb();
> +	__raw_writew((__force u16) val, addr);
> +}

Again the memory barrier seems odd, especially the barrier *before*
doing the write.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] spi:dspi:Remove some coding sytle not in standard

[Mark Brown]

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

On Wed, Jan 08, 2014 at 02:14:15PM +0800, Chao Fu wrote:
> From: Chao Fu <B44548-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> 
> Remove some coding sytle not in standard in former code.

This is OK but depends on your previous patch so can't be applied just
yet.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH v2 1/2] spi:fsl-dspi:add support of DSPI IP in big endian

[Chao Fu]

> This looks a lot nicer - a few things below though.
> 
> > +#define DSPI_BITWISE16(d, v)	(d->big_endian ? cpu_to_be16(v) :
> cpu_to_le16(v))
> > +#define DSPI_BITWISE32(d, v)	(d->big_endian ? cpu_to_be32(v) :
> cpu_to_le32(v))
> 
> These should probably be inline for the same reason as the I/O functions,
> perhaps even being done as part of the I/O functions.
> 
[Chao Fu] Thank you, Mark! I wiil put them into inline functions.

> > +static inline u16 dspi_readw(void __iomem *addr) {
> > +	u16 __v = (__force u16) __raw_readw(addr);
> > +	__iormb();
> > +
> > +	return __v;
> > +}
> 
> Why does this need a barrier and why does it not take care of the
> endianness translation?  A quick glance through shows most if not all of
> the callers doing the translation.
>
[Chao Fu] Our CPUs are working on only ARM architecture. But DSPI have two endianness 
In different series CPU, so we use __raw_read that do not take of endianness.
We need observe ARM io methods  make sure avoid instructions executing reorder .
ARM IO methods :
#define readb(c)                ({ u8  __v = readb_relaxed(c); __iormb(); __v; })
#define readw(c)                ({ u16 __v = readw_relaxed(c); __iormb(); __v; })
#define readl(c)                ({ u32 __v = readl_relaxed(c); __iormb(); __v; })

#define writeb(v,c)             ({ __iowmb(); writeb_relaxed(v,c); })
#define writew(v,c)             ({ __iowmb(); writew_relaxed(v,c); })
#define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })

So add barrier here. Could you give some suggestions? Many thanks!
 
> > +static inline void dspi_writew(u16 val, void __iomem *addr) {
> > +	__iowmb();
> > +	__raw_writew((__force u16) val, addr); }
> 
> Again the memory barrier seems odd, especially the barrier *before* doing
> the write.
--
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 v2 1/2] spi:fsl-dspi:add support of DSPI IP in big endian

[Mark Brown]

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

On Fri, Jan 10, 2014 at 08:11:33AM +0000, Chao Fu wrote:

> [Chao Fu] Our CPUs are working on only ARM architecture. But DSPI have two endianness 
> In different series CPU, so we use __raw_read that do not take of endianness.
> We need observe ARM io methods  make sure avoid instructions executing reorder .
> ARM IO methods :
> #define readb(c)                ({ u8  __v = readb_relaxed(c); __iormb(); __v; })
> #define readw(c)                ({ u16 __v = readw_relaxed(c); __iormb(); __v; })
> #define readl(c)                ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
> 
> #define writeb(v,c)             ({ __iowmb(); writeb_relaxed(v,c); })
> #define writew(v,c)             ({ __iowmb(); writew_relaxed(v,c); })
> #define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })
> 
> So add barrier here. Could you give some suggestions? Many thanks!

OK that makes sense, please add comments explaining that this is due to
the endinaness translation.  Given that people are starting to use big
endian more it might be sensible to have these factored out into generic
code but that can wait.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 1/2] spi:fsl-dspi:add support of DSPI IP in big endian

[Russell King - ARM Linux]

On Fri, Jan 10, 2014 at 12:40:14PM +0000, Mark Brown wrote:
> On Fri, Jan 10, 2014 at 08:11:33AM +0000, Chao Fu wrote:
> 
> > [Chao Fu] Our CPUs are working on only ARM architecture. But DSPI have two endianness 
> > In different series CPU, so we use __raw_read that do not take of endianness.
> > We need observe ARM io methods  make sure avoid instructions executing reorder .
> > ARM IO methods :
> > #define readb(c)                ({ u8  __v = readb_relaxed(c); __iormb(); __v; })
> > #define readw(c)                ({ u16 __v = readw_relaxed(c); __iormb(); __v; })
> > #define readl(c)                ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
> > 
> > #define writeb(v,c)             ({ __iowmb(); writeb_relaxed(v,c); })
> > #define writew(v,c)             ({ __iowmb(); writew_relaxed(v,c); })
> > #define writel(v,c)             ({ __iowmb(); writel_relaxed(v,c); })
> > 
> > So add barrier here. Could you give some suggestions? Many thanks!
> 
> OK that makes sense, please add comments explaining that this is due to
> the endinaness translation.  Given that people are starting to use big
> endian more it might be sensible to have these factored out into generic
> code but that can wait.

Accesses to device memory are guaranteed by the architecture to be in
program order when they're within the same 1K block of memory.  Larger
blocks are permissible, and depends on the SoC.  (The ARM ARM is a little
unclear on this statement, and I believe the statement is/has been fixed.)

The barriers above are not about ensuring correct program order (we have
that anyway), they're about ensuring the visibility externally given the
ARM ARM mess-up, and _primerily_ ensuring proper order between DMA
coherent memory and a DMA agent being enabled, or the DMA agent status
being read vs DMA coherent memory.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
--
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