[RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>

The Altera Arria10 SoC requires 32 bit accesses to peripherals. The
DesignWare SPI peripheral registers are on 32bit boundaries so this
patch is minimal. Function pointers are used to select 32bit access
or 16bit accesses.

Thor Thayer (2):
  dt-binding: spi: spi-dw: Select 16b or 32b access for Designware SPI
  spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

 Documentation/devicetree/bindings/spi/spi-dw.txt |    1 +
 drivers/spi/spi-dw-mmio.c                        |    7 +++-
 drivers/spi/spi-dw.c                             |   38 +++++++++++++---------
 drivers/spi/spi-dw.h                             |   10 +++---
 4 files changed, 35 insertions(+), 21 deletions(-)

-- 
1.7.9.5

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

[RFC/PATCH 1/2] dt-binding: spi: spi-dw: Select 16b or 32b access for Designware SPI

[tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>

Altera's Arria10 architecture requires a 32bit accesses for
peripherals. The current spi-dw driver uses 16bit accesses in
some locations. This patch updated the bindings with an optional
field the devicetree can use to select 32bit accesses.

Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/spi-dw.txt |    1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-dw.txt b/Documentation/devicetree/bindings/spi/spi-dw.txt
index 7b63ed6..034dbdd 100644
--- a/Documentation/devicetree/bindings/spi/spi-dw.txt
+++ b/Documentation/devicetree/bindings/spi/spi-dw.txt
@@ -11,6 +11,7 @@ Required properties:
 
 Optional properties:
 - cs-gpios: see spi-bus.txt
+- 32bit_access : use 32 bit register accesses
 
 Example:
 
-- 
1.7.9.5

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

[RFC/PATCH 2/2] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[tthayer]

From: Thor Thayer <tthayer@opensource.altera.com>

Altera's Arria10 SoC requires a 32 bit access for peripherals.
The current spi-dw driver uses 16bit accesses in some locations.
Use function pointers to support 32 bit accesses and not break
legacy 16 bit access.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
 drivers/spi/spi-dw-mmio.c |    7 ++++++-
 drivers/spi/spi-dw.c      |   38 ++++++++++++++++++++++----------------
 drivers/spi/spi-dw.h      |   10 ++++++----
 3 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index eb03e12..ee77005 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -76,8 +76,13 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 
 	num_cs = 4;
 
-	if (pdev->dev.of_node)
+	if (pdev->dev.of_node) {
 		of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs);
+		if (of_property_read_bool(pdev->dev.of_node, "32bit_access")) {
+			dws->dwread = dw_readl;
+			dws->dwwrite = dw_writel;
+		}
+	}
 
 	dws->num_cs = num_cs;
 
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 05af817..614de7f 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -149,7 +149,7 @@ static inline u32 tx_max(struct dw_spi *dws)
 	u32 tx_left, tx_room, rxtx_gap;
 
 	tx_left = (dws->tx_end - dws->tx) / dws->n_bytes;
-	tx_room = dws->fifo_len - dw_readw(dws, DW_SPI_TXFLR);
+	tx_room = dws->fifo_len - dws->dwread(dws, DW_SPI_TXFLR);
 
 	/*
 	 * Another concern is about the tx/rx mismatch, we
@@ -170,7 +170,7 @@ static inline u32 rx_max(struct dw_spi *dws)
 {
 	u32 rx_left = (dws->rx_end - dws->rx) / dws->n_bytes;
 
-	return min_t(u32, rx_left, dw_readw(dws, DW_SPI_RXFLR));
+	return min_t(u32, rx_left, dws->dwread(dws, DW_SPI_RXFLR));
 }
 
 static void dw_writer(struct dw_spi *dws)
@@ -186,7 +186,7 @@ static void dw_writer(struct dw_spi *dws)
 			else
 				txw = *(u16 *)(dws->tx);
 		}
-		dw_writew(dws, DW_SPI_DR, txw);
+		dws->dwwrite(dws, DW_SPI_DR, txw);
 		dws->tx += dws->n_bytes;
 	}
 }
@@ -194,10 +194,10 @@ static void dw_writer(struct dw_spi *dws)
 static void dw_reader(struct dw_spi *dws)
 {
 	u32 max = rx_max(dws);
-	u16 rxw;
+	u32 rxw;
 
 	while (max--) {
-		rxw = dw_readw(dws, DW_SPI_DR);
+		rxw = dws->dwread(dws, DW_SPI_DR);
 		/* Care rx only if the transfer's original "rx" is not null */
 		if (dws->rx_end - dws->len) {
 			if (dws->n_bytes == 1)
@@ -299,13 +299,13 @@ EXPORT_SYMBOL_GPL(dw_spi_xfer_done);
 
 static irqreturn_t interrupt_transfer(struct dw_spi *dws)
 {
-	u16 irq_status = dw_readw(dws, DW_SPI_ISR);
+	u32 irq_status = dws->dwread(dws, DW_SPI_ISR);
 
 	/* Error handling */
 	if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
-		dw_readw(dws, DW_SPI_TXOICR);
-		dw_readw(dws, DW_SPI_RXOICR);
-		dw_readw(dws, DW_SPI_RXUICR);
+		dws->dwread(dws, DW_SPI_TXOICR);
+		dws->dwread(dws, DW_SPI_RXOICR);
+		dws->dwread(dws, DW_SPI_RXUICR);
 		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
 		return IRQ_HANDLED;
 	}
@@ -329,7 +329,7 @@ static irqreturn_t interrupt_transfer(struct dw_spi *dws)
 static irqreturn_t dw_spi_irq(int irq, void *dev_id)
 {
 	struct dw_spi *dws = dev_id;
-	u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
+	u32 irq_status = dws->dwread(dws, DW_SPI_ISR) & 0x3f;
 
 	if (!irq_status)
 		return IRQ_NONE;
@@ -473,10 +473,11 @@ static void pump_transfers(unsigned long data)
 	 *	2. clk_div is changed
 	 *	3. control value changes
 	 */
-	if (dw_readw(dws, DW_SPI_CTRL0) != cr0 || cs_change || clk_div || imask) {
+	if (dws->dwread(dws, DW_SPI_CTRL0) != cr0 ||
+	    cs_change || clk_div || imask) {
 		spi_enable_chip(dws, 0);
 
-		dw_writew(dws, DW_SPI_CTRL0, cr0);
+		dws->dwwrite(dws, DW_SPI_CTRL0, cr0);
 
 		spi_set_clk(dws, chip->clk_div);
 		spi_chip_sel(dws, spi, 1);
@@ -486,7 +487,7 @@ static void pump_transfers(unsigned long data)
 		if (imask)
 			spi_umask_intr(dws, imask);
 		if (txlevel)
-			dw_writew(dws, DW_SPI_TXFLTR, txlevel);
+			dws->dwwrite(dws, DW_SPI_TXFLTR, txlevel);
 
 		spi_enable_chip(dws, 1);
 	}
@@ -618,11 +619,11 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
 		u32 fifo;
 
 		for (fifo = 1; fifo < 256; fifo++) {
-			dw_writew(dws, DW_SPI_TXFLTR, fifo);
-			if (fifo != dw_readw(dws, DW_SPI_TXFLTR))
+			dws->dwwrite(dws, DW_SPI_TXFLTR, fifo);
+			if (fifo != dws->dwread(dws, DW_SPI_TXFLTR))
 				break;
 		}
-		dw_writew(dws, DW_SPI_TXFLTR, 0);
+		dws->dwwrite(dws, DW_SPI_TXFLTR, 0);
 
 		dws->fifo_len = (fifo == 1) ? 0 : fifo;
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
@@ -647,6 +648,11 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 	dws->dma_addr = (dma_addr_t)(dws->paddr + 0x60);
 	snprintf(dws->name, sizeof(dws->name), "dw_spi%d", dws->bus_num);
 
+	if (!dws->dwread)
+		dws->dwread = dw_readw;
+	if (!dws->dwwrite)
+		dws->dwwrite = dw_writew;
+
 	ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
 			dws->name, dws);
 	if (ret < 0) {
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 3d32be6..5ca2407 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -150,6 +150,8 @@ struct dw_spi {
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
 #endif
+	u32 (*dwread)(struct dw_spi *dws, u32 offset);
+	void (*dwwrite)(struct dw_spi *dws, u32 offset, u16 val);
 };
 
 static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
@@ -157,14 +159,14 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
 	return __raw_readl(dws->regs + offset);
 }
 
-static inline void dw_writel(struct dw_spi *dws, u32 offset, u32 val)
+static inline void dw_writel(struct dw_spi *dws, u32 offset, u16 val)
 {
-	__raw_writel(val, dws->regs + offset);
+	__raw_writel((u32)val, dws->regs + offset);
 }
 
-static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
+static inline u32 dw_readw(struct dw_spi *dws, u32 offset)
 {
-	return __raw_readw(dws->regs + offset);
+	return (u32)__raw_readw(dws->regs + offset);
 }
 
 static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
-- 
1.7.9.5

Re: [RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[Andy Shevchenko]

On Wed, 2015-03-04 at 14:31 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> 
> The Altera Arria10 SoC requires 32 bit accesses to peripherals. The
> DesignWare SPI peripheral registers are on 32bit boundaries so this
> patch is minimal. Function pointers are used to select 32bit access
> or 16bit accesses.


So, what is exactly the issue when we read only half of the register?
Bus lock, or what?

> 
> Thor Thayer (2):
>   dt-binding: spi: spi-dw: Select 16b or 32b access for Designware SPI
>   spi: dw-spi: Pointers select 16b vs. 32b DesignWare access
> 
>  Documentation/devicetree/bindings/spi/spi-dw.txt |    1 +
>  drivers/spi/spi-dw-mmio.c                        |    7 +++-
>  drivers/spi/spi-dw.c                             |   38 +++++++++++++---------
>  drivers/spi/spi-dw.h                             |   10 +++---
>  4 files changed, 35 insertions(+), 21 deletions(-)
> 


-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
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: [RFC/PATCH 2/2] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[Andy Shevchenko]

On Wed, 2015-03-04 at 14:31 -0600, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> Altera's Arria10 SoC requires a 32 bit access for peripherals.
> The current spi-dw driver uses 16bit accesses in some locations.
> Use function pointers to support 32 bit accesses and not break
> legacy 16 bit access.

Besides comment to cover letter few more here.

> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
>  drivers/spi/spi-dw-mmio.c |    7 ++++++-
>  drivers/spi/spi-dw.c      |   38 ++++++++++++++++++++++----------------
>  drivers/spi/spi-dw.h      |   10 ++++++----
>  3 files changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index eb03e12..ee77005 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
> @@ -76,8 +76,13 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
>  
>  	num_cs = 4;
>  
> -	if (pdev->dev.of_node)
> +	if (pdev->dev.of_node) {
>  		of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs);
> +		if (of_property_read_bool(pdev->dev.of_node, "32bit_access")) {
> +			dws->dwread = dw_readl;
> +			dws->dwwrite = dw_writel;
> +		}
> +	}
>  
>  	dws->num_cs = num_cs;
>  
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index 05af817..614de7f 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -149,7 +149,7 @@ static inline u32 tx_max(struct dw_spi *dws)
>  	u32 tx_left, tx_room, rxtx_gap;
>  
>  	tx_left = (dws->tx_end - dws->tx) / dws->n_bytes;
> -	tx_room = dws->fifo_len - dw_readw(dws, DW_SPI_TXFLR);
> +	tx_room = dws->fifo_len - dws->dwread(dws, DW_SPI_TXFLR);
>  
>  	/*
>  	 * Another concern is about the tx/rx mismatch, we
> @@ -170,7 +170,7 @@ static inline u32 rx_max(struct dw_spi *dws)
>  {
>  	u32 rx_left = (dws->rx_end - dws->rx) / dws->n_bytes;
>  
> -	return min_t(u32, rx_left, dw_readw(dws, DW_SPI_RXFLR));
> +	return min_t(u32, rx_left, dws->dwread(dws, DW_SPI_RXFLR));
>  }
>  
>  static void dw_writer(struct dw_spi *dws)
> @@ -186,7 +186,7 @@ static void dw_writer(struct dw_spi *dws)
>  			else
>  				txw = *(u16 *)(dws->tx);
>  		}
> -		dw_writew(dws, DW_SPI_DR, txw);
> +		dws->dwwrite(dws, DW_SPI_DR, txw);
>  		dws->tx += dws->n_bytes;
>  	}
>  }
> @@ -194,10 +194,10 @@ static void dw_writer(struct dw_spi *dws)
>  static void dw_reader(struct dw_spi *dws)
>  {
>  	u32 max = rx_max(dws);
> -	u16 rxw;
> +	u32 rxw;
>  
>  	while (max--) {
> -		rxw = dw_readw(dws, DW_SPI_DR);
> +		rxw = dws->dwread(dws, DW_SPI_DR);
>  		/* Care rx only if the transfer's original "rx" is not null */
>  		if (dws->rx_end - dws->len) {
>  			if (dws->n_bytes == 1)
> @@ -299,13 +299,13 @@ EXPORT_SYMBOL_GPL(dw_spi_xfer_done);
>  
>  static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>  {
> -	u16 irq_status = dw_readw(dws, DW_SPI_ISR);
> +	u32 irq_status = dws->dwread(dws, DW_SPI_ISR);
>  
>  	/* Error handling */
>  	if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
> -		dw_readw(dws, DW_SPI_TXOICR);
> -		dw_readw(dws, DW_SPI_RXOICR);
> -		dw_readw(dws, DW_SPI_RXUICR);
> +		dws->dwread(dws, DW_SPI_TXOICR);
> +		dws->dwread(dws, DW_SPI_RXOICR);
> +		dws->dwread(dws, DW_SPI_RXUICR);

Better to issue separate patch first which substitutes those 3 by 1

dw_readw(dws, DW_SPI_ICR);


>  		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>  		return IRQ_HANDLED;
>  	}
> @@ -329,7 +329,7 @@ static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>  static irqreturn_t dw_spi_irq(int irq, void *dev_id)
>  {
>  	struct dw_spi *dws = dev_id;
> -	u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
> +	u32 irq_status = dws->dwread(dws, DW_SPI_ISR) & 0x3f;
>  
>  	if (!irq_status)
>  		return IRQ_NONE;
> @@ -473,10 +473,11 @@ static void pump_transfers(unsigned long data)
>  	 *	2. clk_div is changed
>  	 *	3. control value changes
>  	 */
> -	if (dw_readw(dws, DW_SPI_CTRL0) != cr0 || cs_change || clk_div || imask) {
> +	if (dws->dwread(dws, DW_SPI_CTRL0) != cr0 ||
> +	    cs_change || clk_div || imask) {
>  		spi_enable_chip(dws, 0);
>  
> -		dw_writew(dws, DW_SPI_CTRL0, cr0);
> +		dws->dwwrite(dws, DW_SPI_CTRL0, cr0);
>  
>  		spi_set_clk(dws, chip->clk_div);
>  		spi_chip_sel(dws, spi, 1);

If possible, can you re-base on top of my patchset an re-test?

http://www.spinics.net/lists/linux-spi/msg03004.html


> @@ -486,7 +487,7 @@ static void pump_transfers(unsigned long data)
>  		if (imask)
>  			spi_umask_intr(dws, imask);
>  		if (txlevel)
> -			dw_writew(dws, DW_SPI_TXFLTR, txlevel);
> +			dws->dwwrite(dws, DW_SPI_TXFLTR, txlevel);
>  
>  		spi_enable_chip(dws, 1);
>  	}
> @@ -618,11 +619,11 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
>  		u32 fifo;
>  
>  		for (fifo = 1; fifo < 256; fifo++) {
> -			dw_writew(dws, DW_SPI_TXFLTR, fifo);
> -			if (fifo != dw_readw(dws, DW_SPI_TXFLTR))
> +			dws->dwwrite(dws, DW_SPI_TXFLTR, fifo);
> +			if (fifo != dws->dwread(dws, DW_SPI_TXFLTR))
>  				break;
>  		}
> -		dw_writew(dws, DW_SPI_TXFLTR, 0);
> +		dws->dwwrite(dws, DW_SPI_TXFLTR, 0);
>  
>  		dws->fifo_len = (fifo == 1) ? 0 : fifo;
>  		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
> @@ -647,6 +648,11 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>  	dws->dma_addr = (dma_addr_t)(dws->paddr + 0x60);
>  	snprintf(dws->name, sizeof(dws->name), "dw_spi%d", dws->bus_num);
>  
> +	if (!dws->dwread)
> +		dws->dwread = dw_readw;
> +	if (!dws->dwwrite)
> +		dws->dwwrite = dw_writew;
> +
>  	ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
>  			dws->name, dws);
>  	if (ret < 0) {
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 3d32be6..5ca2407 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -150,6 +150,8 @@ struct dw_spi {
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *debugfs;
>  #endif
> +	u32 (*dwread)(struct dw_spi *dws, u32 offset);
> +	void (*dwwrite)(struct dw_spi *dws, u32 offset, u16 val);

u16?!

>  };
>  
>  static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
> @@ -157,14 +159,14 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>  	return __raw_readl(dws->regs + offset);
>  }
>  
> -static inline void dw_writel(struct dw_spi *dws, u32 offset, u32 val)
> +static inline void dw_writel(struct dw_spi *dws, u32 offset, u16 val)

So, why u16?

>  {
> -	__raw_writel(val, dws->regs + offset);
> +	__raw_writel((u32)val, dws->regs + offset);

Seems like a mess here. Could you figure out, please, what should be
done?

>  }
>  
> -static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
> +static inline u32 dw_readw(struct dw_spi *dws, u32 offset)
>  {
> -	return __raw_readw(dws->regs + offset);
> +	return (u32)__raw_readw(dws->regs + offset);

why readw?

>  }
>  
>  static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)


We have exactly 4 functions. What is wrong to use proper ones?

So, I would appreciate to hear an answer to the question in cover letter
reply and if you reorganizes code to use proper functions.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[Andy Shevchenko]

On Wed, 2015-03-04 at 14:31 -0600, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> The Altera Arria10 SoC requires 32 bit accesses to peripherals.

Accordingly to what I can read here [1] the IP block is DW_apb_ssi. So,
I don't see any needs for those patches.

The thing which might be needed is to implement DMA support for that
chip.

[1] http://www.altera.com/literature/hb/arria-10/hps.html


>  The
> DesignWare SPI peripheral registers are on 32bit boundaries so this
> patch is minimal. Function pointers are used to select 32bit access
> or 16bit accesses.
> 
> Thor Thayer (2):
>   dt-binding: spi: spi-dw: Select 16b or 32b access for Designware SPI
>   spi: dw-spi: Pointers select 16b vs. 32b DesignWare access
> 
>  Documentation/devicetree/bindings/spi/spi-dw.txt |    1 +
>  drivers/spi/spi-dw-mmio.c                        |    7 +++-
>  drivers/spi/spi-dw.c                             |   38 +++++++++++++---------
>  drivers/spi/spi-dw.h                             |   10 +++---
>  4 files changed, 35 insertions(+), 21 deletions(-)
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[Thor Thayer]

Hi Andy,

On 03/04/2015 02:44 PM, Andy Shevchenko wrote:
> On Wed, 2015-03-04 at 14:31 -0600, tthayer@opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> The Altera Arria10 SoC requires 32 bit accesses to peripherals. The
>> DesignWare SPI peripheral registers are on 32bit boundaries so this
>> patch is minimal. Function pointers are used to select 32bit access
>> or 16bit accesses.
>
>
> So, what is exactly the issue when we read only half of the register?
> Bus lock, or what?
>

The read actually works on our chip but I changed both read and write to 
be consistent. For Arria10, on a 16 bit write the data isn't written 
into the DesignWare register.

In reply to your other email, yes it does support the DW_apb_ssi but the 
Arria10 architecture requires 32 bit access (actually as you point out, 
32 bit writes). We're using the original driver on our older chips but 
Arria10 requires upstream changes.

>>
>> Thor Thayer (2):
>>    dt-binding: spi: spi-dw: Select 16b or 32b access for Designware SPI
>>    spi: dw-spi: Pointers select 16b vs. 32b DesignWare access
>>
>>   Documentation/devicetree/bindings/spi/spi-dw.txt |    1 +
>>   drivers/spi/spi-dw-mmio.c                        |    7 +++-
>>   drivers/spi/spi-dw.c                             |   38 +++++++++++++---------
>>   drivers/spi/spi-dw.h                             |   10 +++---
>>   4 files changed, 35 insertions(+), 21 deletions(-)
>>
>
>

Re: [RFC/PATCH 2/2] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[Thor Thayer]

Hi Andy,

On 03/04/2015 02:55 PM, Andy Shevchenko wrote:
> On Wed, 2015-03-04 at 14:31 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>
>> Altera's Arria10 SoC requires a 32 bit access for peripherals.
>> The current spi-dw driver uses 16bit accesses in some locations.
>> Use function pointers to support 32 bit accesses and not break
>> legacy 16 bit access.
>
> Besides comment to cover letter few more here.
>
>>
>> Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>

<snip>

>>
>>   static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>>   {
>> -	u16 irq_status = dw_readw(dws, DW_SPI_ISR);
>> +	u32 irq_status = dws->dwread(dws, DW_SPI_ISR);
>>
>>   	/* Error handling */
>>   	if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
>> -		dw_readw(dws, DW_SPI_TXOICR);
>> -		dw_readw(dws, DW_SPI_RXOICR);
>> -		dw_readw(dws, DW_SPI_RXUICR);
>> +		dws->dwread(dws, DW_SPI_TXOICR);
>> +		dws->dwread(dws, DW_SPI_RXOICR);
>> +		dws->dwread(dws, DW_SPI_RXUICR);
>
> Better to issue separate patch first which substitutes those 3 by 1
>
> dw_readw(dws, DW_SPI_ICR);
>

Yes, I'll do that.

>
>>   		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>>   		return IRQ_HANDLED;
>>   	}
>> @@ -329,7 +329,7 @@ static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>>   static irqreturn_t dw_spi_irq(int irq, void *dev_id)
>>   {
>>   	struct dw_spi *dws = dev_id;
>> -	u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
>> +	u32 irq_status = dws->dwread(dws, DW_SPI_ISR) & 0x3f;
>>
>>   	if (!irq_status)
>>   		return IRQ_NONE;
>> @@ -473,10 +473,11 @@ static void pump_transfers(unsigned long data)
>>   	 *	2. clk_div is changed
>>   	 *	3. control value changes
>>   	 */
>> -	if (dw_readw(dws, DW_SPI_CTRL0) != cr0 || cs_change || clk_div || imask) {
>> +	if (dws->dwread(dws, DW_SPI_CTRL0) != cr0 ||
>> +	    cs_change || clk_div || imask) {
>>   		spi_enable_chip(dws, 0);
>>
>> -		dw_writew(dws, DW_SPI_CTRL0, cr0);
>> +		dws->dwwrite(dws, DW_SPI_CTRL0, cr0);
>>
>>   		spi_set_clk(dws, chip->clk_div);
>>   		spi_chip_sel(dws, spi, 1);
>
> If possible, can you re-base on top of my patchset an re-test?
>
> http://www.spinics.net/lists/linux-spi/msg03004.html
>

Yes. I can do this. I tested the 16 bit accesses on our Cyclone5 board 
and the 32 bit accesses on the Arria10 board.

>> @@ -486,7 +487,7 @@ static void pump_transfers(unsigned long data)
>>   		if (imask)
>>   			spi_umask_intr(dws, imask);
>>   		if (txlevel)
>> -			dw_writew(dws, DW_SPI_TXFLTR, txlevel);
>> +			dws->dwwrite(dws, DW_SPI_TXFLTR, txlevel);
>>
>>   		spi_enable_chip(dws, 1);
>>   	}
>> @@ -618,11 +619,11 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
>>   		u32 fifo;
>>
>>   		for (fifo = 1; fifo < 256; fifo++) {
>> -			dw_writew(dws, DW_SPI_TXFLTR, fifo);
>> -			if (fifo != dw_readw(dws, DW_SPI_TXFLTR))
>> +			dws->dwwrite(dws, DW_SPI_TXFLTR, fifo);
>> +			if (fifo != dws->dwread(dws, DW_SPI_TXFLTR))
>>   				break;
>>   		}
>> -		dw_writew(dws, DW_SPI_TXFLTR, 0);
>> +		dws->dwwrite(dws, DW_SPI_TXFLTR, 0);
>>
>>   		dws->fifo_len = (fifo == 1) ? 0 : fifo;
>>   		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
>> @@ -647,6 +648,11 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
>>   	dws->dma_addr = (dma_addr_t)(dws->paddr + 0x60);
>>   	snprintf(dws->name, sizeof(dws->name), "dw_spi%d", dws->bus_num);
>>
>> +	if (!dws->dwread)
>> +		dws->dwread = dw_readw;
>> +	if (!dws->dwwrite)
>> +		dws->dwwrite = dw_writew;
>> +
>>   	ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
>>   			dws->name, dws);
>>   	if (ret < 0) {
>> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
>> index 3d32be6..5ca2407 100644
>> --- a/drivers/spi/spi-dw.h
>> +++ b/drivers/spi/spi-dw.h
>> @@ -150,6 +150,8 @@ struct dw_spi {
>>   #ifdef CONFIG_DEBUG_FS
>>   	struct dentry *debugfs;
>>   #endif
>> +	u32 (*dwread)(struct dw_spi *dws, u32 offset);
>> +	void (*dwwrite)(struct dw_spi *dws, u32 offset, u16 val);
>
> u16?!

Yes, I was trying to minimize legacy code changes to the variables in 
spi-dw.c.  Also the function pointer parameters match for both 16 bit 
and 32 bit. They're promoted in the 32 bit functions.

>>   };
>>
>>   static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>> @@ -157,14 +159,14 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>>   	return __raw_readl(dws->regs + offset);
>>   }
>>
>> -static inline void dw_writel(struct dw_spi *dws, u32 offset, u32 val)
>> +static inline void dw_writel(struct dw_spi *dws, u32 offset, u16 val)
>
> So, why u16?
>
>>   {
>> -	__raw_writel(val, dws->regs + offset);
>> +	__raw_writel((u32)val, dws->regs + offset);
>
> Seems like a mess here. Could you figure out, please, what should be
> done?

This is the 32 bit promotion I mentioned above.

>
>>   }
>>
>> -static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
>> +static inline u32 dw_readw(struct dw_spi *dws, u32 offset)
>>   {
>> -	return __raw_readw(dws->regs + offset);
>> +	return (u32)__raw_readw(dws->regs + offset);
>
> why readw?
>

This change meant the function pointer parameters matched.

>>   }
>>
>>   static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>
>
> We have exactly 4 functions. What is wrong to use proper ones?
>

I will revisit the function pointer parameters.

> So, I would appreciate to hear an answer to the question in cover letter
> reply and if you reorganizes code to use proper functions.
>
--
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: [RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[Andy Shevchenko]

On Wed, 2015-03-04 at 16:01 -0600, Thor Thayer wrote:
> Hi Andy,
> 
> On 03/04/2015 02:44 PM, Andy Shevchenko wrote:
> > On Wed, 2015-03-04 at 14:31 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
> >> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> >>
> >> The Altera Arria10 SoC requires 32 bit accesses to peripherals. The
> >> DesignWare SPI peripheral registers are on 32bit boundaries so this
> >> patch is minimal. Function pointers are used to select 32bit access
> >> or 16bit accesses.
> >
> >
> > So, what is exactly the issue when we read only half of the register?
> > Bus lock, or what?
> >
> 
> The read actually works on our chip but I changed both read and write to 
> be consistent. For Arria10, on a 16 bit write the data isn't written 
> into the DesignWare register.

How did you exactly check this?

> 
> In reply to your other email, yes it does support the DW_apb_ssi but the 
> Arria10 architecture requires 32 bit access (actually as you point out, 
> 32 bit writes). We're using the original driver on our older chips but 
> Arria10 requires upstream changes.

Can you check if the following helps in your case:

--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -170,6 +170,8 @@ static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
 static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
 {
        __raw_writew(val, dws->regs + offset);
+       mmiowb():
+       __raw_readw(dws->regs + offset);
 }
 
 static inline void spi_enable_chip(struct dw_spi *dws, int enable)

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
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: [RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[Thor Thayer]

Hi Andy,

On 03/05/2015 04:43 AM, Andy Shevchenko wrote:
> On Wed, 2015-03-04 at 16:01 -0600, Thor Thayer wrote:
>> Hi Andy,
>>
>> On 03/04/2015 02:44 PM, Andy Shevchenko wrote:
>>> On Wed, 2015-03-04 at 14:31 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>>>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>>>
>>>> The Altera Arria10 SoC requires 32 bit accesses to peripherals. The
>>>> DesignWare SPI peripheral registers are on 32bit boundaries so this
>>>> patch is minimal. Function pointers are used to select 32bit access
>>>> or 16bit accesses.
>>>
>>>
>>> So, what is exactly the issue when we read only half of the register?
>>> Bus lock, or what?
>>>
>>
>> The read actually works on our chip but I changed both read and write to
>> be consistent. For Arria10, on a 16 bit write the data isn't written
>> into the DesignWare register.
>
> How did you exactly check this?
>

Sorry, I should have been more clear. The architecture of the Arria10 
SoC enforces 32 bit writes - even for the APB bus. The Arria10's 
interconnect only allows 32 bit writes.

As a result, the solution below doesn't work for us.

I do have a much cleaner patch to re-submit but I haven't applied it on 
top of your patches yet & tested yet.

>>
>> In reply to your other email, yes it does support the DW_apb_ssi but the
>> Arria10 architecture requires 32 bit access (actually as you point out,
>> 32 bit writes). We're using the original driver on our older chips but
>> Arria10 requires upstream changes.
>
> Can you check if the following helps in your case:
>
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -170,6 +170,8 @@ static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
>   static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>   {
>          __raw_writew(val, dws->regs + offset);
> +       mmiowb():
> +       __raw_readw(dws->regs + offset);
>   }
>
>   static inline void spi_enable_chip(struct dw_spi *dws, int enable)
>
--
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: [RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[Andy Shevchenko]

On Thu, 2015-03-05 at 14:41 -0600, Thor Thayer wrote:
> Hi Andy,
> 
> On 03/05/2015 04:43 AM, Andy Shevchenko wrote:
> > On Wed, 2015-03-04 at 16:01 -0600, Thor Thayer wrote:
> >> Hi Andy,
> >>
> >> On 03/04/2015 02:44 PM, Andy Shevchenko wrote:
> >>> On Wed, 2015-03-04 at 14:31 -0600, tthayer@opensource.altera.com wrote:
> >>>> From: Thor Thayer <tthayer@opensource.altera.com>
> >>>>
> >>>> The Altera Arria10 SoC requires 32 bit accesses to peripherals. The
> >>>> DesignWare SPI peripheral registers are on 32bit boundaries so this
> >>>> patch is minimal. Function pointers are used to select 32bit access
> >>>> or 16bit accesses.
> >>>
> >>>
> >>> So, what is exactly the issue when we read only half of the register?
> >>> Bus lock, or what?
> >>>
> >>
> >> The read actually works on our chip but I changed both read and write to
> >> be consistent. For Arria10, on a 16 bit write the data isn't written
> >> into the DesignWare register.
> >
> > How did you exactly check this?
> >
> 
> Sorry, I should have been more clear. The architecture of the Arria10 
> SoC enforces 32 bit writes - even for the APB bus. The Arria10's 
> interconnect only allows 32 bit writes.

Hmm… So, reads are okay, but writes are 32 bit only? Have you any link
to the documentation where I could read this?

> As a result, the solution below doesn't work for us.

My thought was about post writes. That's why I was wondering and still
wonder if something to clarify in the documentation. 

> 
> I do have a much cleaner patch to re-submit but I haven't applied it on 
> top of your patches yet & tested yet.

Okay, please, apply the above as a part of commit message.
I also would like to test it on Intel Medfield.

> 
> >>
> >> In reply to your other email, yes it does support the DW_apb_ssi but the
> >> Arria10 architecture requires 32 bit access (actually as you point out,
> >> 32 bit writes). We're using the original driver on our older chips but
> >> Arria10 requires upstream changes.
> >
> > Can you check if the following helps in your case:
> >
> > --- a/drivers/spi/spi-dw.h
> > +++ b/drivers/spi/spi-dw.h
> > @@ -170,6 +170,8 @@ static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
> >   static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
> >   {
> >          __raw_writew(val, dws->regs + offset);
> > +       mmiowb():
> > +       __raw_readw(dws->regs + offset);
> >   }
> >
> >   static inline void spi_enable_chip(struct dw_spi *dws, int enable)
> >


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [RFC/PATCH 0/2] spi: spi-dw: Select 16b or 32b register access

[Thor Thayer]

On 03/05/2015 03:54 PM, Andy Shevchenko wrote:
> On Thu, 2015-03-05 at 14:41 -0600, Thor Thayer wrote:
>> Hi Andy,
>>
>> On 03/05/2015 04:43 AM, Andy Shevchenko wrote:
>>> On Wed, 2015-03-04 at 16:01 -0600, Thor Thayer wrote:
>>>> Hi Andy,
>>>>
>>>> On 03/04/2015 02:44 PM, Andy Shevchenko wrote:
>>>>> On Wed, 2015-03-04 at 14:31 -0600, tthayer@opensource.altera.com wrote:
>>>>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>>>>
>>>>>> The Altera Arria10 SoC requires 32 bit accesses to peripherals. The
>>>>>> DesignWare SPI peripheral registers are on 32bit boundaries so this
>>>>>> patch is minimal. Function pointers are used to select 32bit access
>>>>>> or 16bit accesses.
>>>>>
>>>>>
>>>>> So, what is exactly the issue when we read only half of the register?
>>>>> Bus lock, or what?
>>>>>
>>>>
>>>> The read actually works on our chip but I changed both read and write to
>>>> be consistent. For Arria10, on a 16 bit write the data isn't written
>>>> into the DesignWare register.
>>>
>>> How did you exactly check this?
>>>
>>
>> Sorry, I should have been more clear. The architecture of the Arria10
>> SoC enforces 32 bit writes - even for the APB bus. The Arria10's
>> interconnect only allows 32 bit writes.
>
> Hmm… So, reads are okay, but writes are 32 bit only? Have you any link
> to the documentation where I could read this?
>

Currently Table 7-6 in the Arria10 Technical Reference Manual [1] on 
page 141 shows SPI Master as a 32 bit wide interface but it doesn't 
explicitly note the 32bit write requirement. We're adding a note to 
table 7-6 as a result of this discussion.

[1] http://www.altera.com/literature/hb/arria-10/a10_5v4.pdf

>> As a result, the solution below doesn't work for us.
>
> My thought was about post writes. That's why I was wondering and still
> wonder if something to clarify in the documentation.
>
>>
>> I do have a much cleaner patch to re-submit but I haven't applied it on
>> top of your patches yet & tested yet.
>
> Okay, please, apply the above as a part of commit message.
> I also would like to test it on Intel Medfield.
>
>>
>>>>
>>>> In reply to your other email, yes it does support the DW_apb_ssi but the
>>>> Arria10 architecture requires 32 bit access (actually as you point out,
>>>> 32 bit writes). We're using the original driver on our older chips but
>>>> Arria10 requires upstream changes.
>>>
>>> Can you check if the following helps in your case:
>>>
>>> --- a/drivers/spi/spi-dw.h
>>> +++ b/drivers/spi/spi-dw.h
>>> @@ -170,6 +170,8 @@ static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
>>>    static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>>>    {
>>>           __raw_writew(val, dws->regs + offset);
>>> +       mmiowb():
>>> +       __raw_readw(dws->regs + offset);
>>>    }
>>>
>>>    static inline void spi_enable_chip(struct dw_spi *dws, int enable)
>>>
>
>