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

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

[tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

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

The Altera Arria10 SoC interconnect requires 32 bit write accesses to 
APB 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 (3):
  spi: dw-spi: Single Register read to clear IRQs
  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                             |   31 ++++++++++++----------
 drivers/spi/spi-dw.h                             |   12 +++++++++
 4 files changed, 36 insertions(+), 15 deletions(-)

-- 
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/PATCHv2 1/3] spi: dw-spi: Single Register read to clear IRQs

[tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

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

Instead of clearing the RxU, RxO, and TxO IRQs individually with
3 register reads, a single read of the ICR register will do the
same thing.

Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi-dw.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 950bc50..c5fa2be 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -258,9 +258,7 @@ static irqreturn_t interrupt_transfer(struct dw_spi *dws)
 
 	/* 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);
+		dw_readw(dws, DW_SPI_ICR);
 		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
 		return IRQ_HANDLED;
 	}
-- 
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

Re: [RFC/PATCHv2 1/3] spi: dw-spi: Single Register read to clear IRQs

[Andy Shevchenko]

On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@opensource.altera.com> wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Instead of clearing the RxU, RxO, and TxO IRQs individually with
> 3 register reads, a single read of the ICR register will do the
> same thing.
>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Rather Suggested-by: …

Otherwise,
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> ---
>  drivers/spi/spi-dw.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index 950bc50..c5fa2be 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -258,9 +258,7 @@ static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>
>         /* 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);
> +               dw_readw(dws, DW_SPI_ICR);
>                 int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>                 return IRQ_HANDLED;
>         }
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko

Re: [RFC/PATCHv2 1/3] spi: dw-spi: Single Register read to clear IRQs

[Mark Brown]

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

On Fri, Mar 06, 2015 at 05:46:32PM -0600, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> Instead of clearing the RxU, RxO, and TxO IRQs individually with
> 3 register reads, a single read of the ICR register will do the
> same thing.

Applied, thanks.

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

[RFC/PATCHv2 2/3] 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 write accesses for
APB peripherals. The current spi-dw driver uses 16bit accesses in
some locations. This patch updated the bindings with an optional
field in the devicetree 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

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

[Andy Shevchenko]

On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@opensource.altera.com> wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
>
> Altera's Arria10 architecture requires a 32bit write accesses for
> APB peripherals. The current spi-dw driver uses 16bit accesses in
> some locations. This patch updated the bindings with an optional
> field in the devicetree to select 32bit accesses.
>

One comment below.

> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---
>  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
>

The most big issue for my opinion with DT is an absence of the
standard for naming properties.

So, why underscore? May be it's a time to append a generic description
of this field to Documentation/devicetree/…


-- 
With Best Regards,
Andy Shevchenko

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

[Thor Thayer]

On 03/07/2015 01:58 PM, Andy Shevchenko wrote:
> On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@opensource.altera.com> wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Altera's Arria10 architecture requires a 32bit write accesses for
>> APB peripherals. The current spi-dw driver uses 16bit accesses in
>> some locations. This patch updated the bindings with an optional
>> field in the devicetree to select 32bit accesses.
>>
>
> One comment below.
>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>>   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
>>
>
> The most big issue for my opinion with DT is an absence of the
> standard for naming properties.
>
> So, why underscore? May be it's a time to append a generic description
> of this field to Documentation/devicetree/…
>

Hi Andy,

I'm use the underscore to make it easier to read but I'm open to 
suggestions.

I stay away from dashes because these variable names are problematic to 
parse in languages like Python which may be used as an external tool.

I don't mind making it just 32bit if that is preferable or implementing 
a better field name.

Thanks for reviewing

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

[Mark Brown]

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

On Mon, Mar 09, 2015 at 01:11:28PM -0500, Thor Thayer wrote:

> I stay away from dashes because these variable names are problematic to
> parse in languages like Python which may be used as an external tool.

Dashes are idiomatic for device tree.

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

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

[Thor Thayer]

On 03/09/2015 01:19 PM, Mark Brown wrote:
> On Mon, Mar 09, 2015 at 01:11:28PM -0500, Thor Thayer wrote:
>
>> I stay away from dashes because these variable names are problematic to
>> parse in languages like Python which may be used as an external tool.
>
> Dashes are idiomatic for device tree.
>

Thanks, I will change to using a dash then.
--
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/PATCHv2 3/3] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

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

Altera's Arria10 SoC interconnect requires a 32 bit write for APB
peripherals. The current spi-dw driver uses 16bit accesses in
some locations. Use function pointers to support 32 bit accesses
but retain legacy 16 bit access.

Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
---
 drivers/spi/spi-dw-mmio.c |    7 ++++++-
 drivers/spi/spi-dw.c      |   29 +++++++++++++++++------------
 drivers/spi/spi-dw.h      |   12 ++++++++++++
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index eb03e12..c4fe9e9 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->read_w = dw_readw32;
+			dws->write_w = dw_writew32;
+		}
+	}
 
 	dws->num_cs = num_cs;
 
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index c5fa2be..d008791 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -157,7 +157,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->read_w(dws, DW_SPI_TXFLR);
 
 	/*
 	 * Another concern is about the tx/rx mismatch, we
@@ -178,7 +178,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->read_w(dws, DW_SPI_RXFLR));
 }
 
 static void dw_writer(struct dw_spi *dws)
@@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)
 			else
 				txw = *(u16 *)(dws->tx);
 		}
-		dw_writew(dws, DW_SPI_DR, txw);
+		dws->write_w(dws, DW_SPI_DR, txw);
 		dws->tx += dws->n_bytes;
 	}
 }
@@ -205,7 +205,7 @@ static void dw_reader(struct dw_spi *dws)
 	u16 rxw;
 
 	while (max--) {
-		rxw = dw_readw(dws, DW_SPI_DR);
+		rxw = dws->read_w(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)
@@ -254,11 +254,11 @@ static void int_error_stop(struct dw_spi *dws, const char *msg)
 
 static irqreturn_t interrupt_transfer(struct dw_spi *dws)
 {
-	u16 irq_status = dw_readw(dws, DW_SPI_ISR);
+	u16 irq_status = dws->read_w(dws, DW_SPI_ISR);
 
 	/* Error handling */
 	if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
-		dw_readw(dws, DW_SPI_ICR);
+		dws->read_w(dws, DW_SPI_ICR);
 		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
 		return IRQ_HANDLED;
 	}
@@ -283,7 +283,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
 {
 	struct spi_master *master = dev_id;
 	struct dw_spi *dws = spi_master_get_devdata(master);
-	u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
+	u16 irq_status = dws->read_w(dws, DW_SPI_ISR) & 0x3f;
 
 	if (!irq_status)
 		return IRQ_NONE;
@@ -379,7 +379,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
 		cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
 	}
 
-	dw_writew(dws, DW_SPI_CTRL0, cr0);
+	dws->write_w(dws, DW_SPI_CTRL0, cr0);
 
 	/* Check if current transfer is a DMA transaction */
 	dws->dma_mapped = map_dma_buffers(master, spi, transfer);
@@ -393,7 +393,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
 	 */
 	if (!dws->dma_mapped && !chip->poll_mode) {
 		txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
-		dw_writew(dws, DW_SPI_TXFLTR, txlevel);
+		dws->write_w(dws, DW_SPI_TXFLTR, txlevel);
 
 		/* Set the interrupt mask */
 		imask |= SPI_INT_TXEI | SPI_INT_TXOI |
@@ -516,11 +516,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->write_w(dws, DW_SPI_TXFLTR, fifo);
+			if (fifo != dws->read_w(dws, DW_SPI_TXFLTR))
 				break;
 		}
-		dw_writew(dws, DW_SPI_TXFLTR, 0);
+		dws->write_w(dws, DW_SPI_TXFLTR, 0);
 
 		dws->fifo_len = (fifo == 1) ? 0 : fifo;
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
@@ -545,6 +545,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->read_w)
+		dws->read_w = dw_readw;
+	if (!dws->write_w)
+		dws->write_w = dw_writew;
+
 	ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
 			dws->name, master);
 	if (ret < 0) {
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 855bfdd..1df09e2 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -141,6 +141,8 @@ struct dw_spi {
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs;
 #endif
+	u16 (*read_w)(struct dw_spi *dws, u32 offset);
+	void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);
 };
 
 static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
@@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
 	__raw_writew(val, dws->regs + offset);
 }
 
+static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
+{
+	return (u16)__raw_readl(dws->regs + offset);
+}
+
+static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
+{
+	__raw_writel((u32)val, dws->regs + offset);
+}
+
 static inline void spi_enable_chip(struct dw_spi *dws, int enable)
 {
 	dw_writel(dws, DW_SPI_SSIENR, (enable ? 1 : 0));
-- 
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

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

[Andy Shevchenko]

On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>
> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
> peripherals. The current spi-dw driver uses 16bit accesses in
> some locations. Use function pointers to support 32 bit accesses
> but retain legacy 16 bit access.
>

Thanks for this version. My comments below.

> Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> ---
>  drivers/spi/spi-dw-mmio.c |    7 ++++++-
>  drivers/spi/spi-dw.c      |   29 +++++++++++++++++------------
>  drivers/spi/spi-dw.h      |   12 ++++++++++++
>  3 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index eb03e12..c4fe9e9 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->read_w = dw_readw32;
> +                       dws->write_w = dw_writew32;

Can we use just  readw/writew (w/o underscores) as names for the accessors?

> +               }
> +       }
>
>         dws->num_cs = num_cs;
>
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index c5fa2be..d008791 100644
> --- a/drivers/spi/spi-dw.c
> +++ b/drivers/spi/spi-dw.c
> @@ -157,7 +157,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->read_w(dws, DW_SPI_TXFLR);
>
>         /*
>          * Another concern is about the tx/rx mismatch, we
> @@ -178,7 +178,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->read_w(dws, DW_SPI_RXFLR));
>  }
>
>  static void dw_writer(struct dw_spi *dws)
> @@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)
>                         else
>                                 txw = *(u16 *)(dws->tx);
>                 }
> -               dw_writew(dws, DW_SPI_DR, txw);
> +               dws->write_w(dws, DW_SPI_DR, txw);
>                 dws->tx += dws->n_bytes;
>         }
>  }
> @@ -205,7 +205,7 @@ static void dw_reader(struct dw_spi *dws)
>         u16 rxw;
>
>         while (max--) {
> -               rxw = dw_readw(dws, DW_SPI_DR);
> +               rxw = dws->read_w(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)
> @@ -254,11 +254,11 @@ static void int_error_stop(struct dw_spi *dws, const char *msg)
>
>  static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>  {
> -       u16 irq_status = dw_readw(dws, DW_SPI_ISR);
> +       u16 irq_status = dws->read_w(dws, DW_SPI_ISR);
>
>         /* Error handling */
>         if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
> -               dw_readw(dws, DW_SPI_ICR);
> +               dws->read_w(dws, DW_SPI_ICR);
>                 int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>                 return IRQ_HANDLED;
>         }
> @@ -283,7 +283,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
>  {
>         struct spi_master *master = dev_id;
>         struct dw_spi *dws = spi_master_get_devdata(master);
> -       u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
> +       u16 irq_status = dws->read_w(dws, DW_SPI_ISR) & 0x3f;
>
>         if (!irq_status)
>                 return IRQ_NONE;
> @@ -379,7 +379,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>                 cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
>         }
>
> -       dw_writew(dws, DW_SPI_CTRL0, cr0);
> +       dws->write_w(dws, DW_SPI_CTRL0, cr0);
>
>         /* Check if current transfer is a DMA transaction */
>         dws->dma_mapped = map_dma_buffers(master, spi, transfer);
> @@ -393,7 +393,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>          */
>         if (!dws->dma_mapped && !chip->poll_mode) {
>                 txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
> -               dw_writew(dws, DW_SPI_TXFLTR, txlevel);
> +               dws->write_w(dws, DW_SPI_TXFLTR, txlevel);
>
>                 /* Set the interrupt mask */
>                 imask |= SPI_INT_TXEI | SPI_INT_TXOI |
> @@ -516,11 +516,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->write_w(dws, DW_SPI_TXFLTR, fifo);
> +                       if (fifo != dws->read_w(dws, DW_SPI_TXFLTR))
>                                 break;
>                 }
> -               dw_writew(dws, DW_SPI_TXFLTR, 0);
> +               dws->write_w(dws, DW_SPI_TXFLTR, 0);
>
>                 dws->fifo_len = (fifo == 1) ? 0 : fifo;
>                 dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
> @@ -545,6 +545,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->read_w)
> +               dws->read_w = dw_readw;
> +       if (!dws->write_w)
> +               dws->write_w = dw_writew;
> +
>         ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
>                         dws->name, master);
>         if (ret < 0) {
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 855bfdd..1df09e2 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -141,6 +141,8 @@ struct dw_spi {
>  #ifdef CONFIG_DEBUG_FS
>         struct dentry *debugfs;
>  #endif
> +       u16 (*read_w)(struct dw_spi *dws, u32 offset);
> +       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);
>  };
>
>  static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
> @@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>         __raw_writew(val, dws->regs + offset);
>  }
>
> +static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
> +{
> +       return (u16)__raw_readl(dws->regs + offset);
> +}
> +
> +static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
> +{
> +       __raw_writel((u32)val, dws->regs + offset);
> +}
> +

So, does simple
dws->readw = dw_readl;
dws->writew = dw_writel;

work for you?

>  static inline void spi_enable_chip(struct dw_spi *dws, int enable)
>  {
>         dw_writel(dws, DW_SPI_SSIENR, (enable ? 1 : 0));
> --
> 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



-- 
With Best Regards,
Andy Shevchenko
--
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/PATCHv2 3/3] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[Thor Thayer]

On 03/07/2015 01:52 PM, Andy Shevchenko wrote:
> On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer@opensource.altera.com> wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
>> peripherals. The current spi-dw driver uses 16bit accesses in
>> some locations. Use function pointers to support 32 bit accesses
>> but retain legacy 16 bit access.
>>
>
> Thanks for this version. My comments below.
>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>>   drivers/spi/spi-dw-mmio.c |    7 ++++++-
>>   drivers/spi/spi-dw.c      |   29 +++++++++++++++++------------
>>   drivers/spi/spi-dw.h      |   12 ++++++++++++
>>   3 files changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>> index eb03e12..c4fe9e9 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->read_w = dw_readw32;
>> +                       dws->write_w = dw_writew32;
>
> Can we use just  readw/writew (w/o underscores) as names for the accessors?
>

I tried this initially and got a namespace conflict with the readw & 
write2 macros.
macro writew passed 3 arguments, but takes just 2
macro readw passed 2 arguments, but takes just 1

>> +               }
>> +       }
>>
>>          dws->num_cs = num_cs;
>>
>> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
>> index c5fa2be..d008791 100644
>> --- a/drivers/spi/spi-dw.c
>> +++ b/drivers/spi/spi-dw.c
>> @@ -157,7 +157,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->read_w(dws, DW_SPI_TXFLR);
>>
>>          /*
>>           * Another concern is about the tx/rx mismatch, we
>> @@ -178,7 +178,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->read_w(dws, DW_SPI_RXFLR));
>>   }
>>
>>   static void dw_writer(struct dw_spi *dws)
>> @@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)
>>                          else
>>                                  txw = *(u16 *)(dws->tx);
>>                  }
>> -               dw_writew(dws, DW_SPI_DR, txw);
>> +               dws->write_w(dws, DW_SPI_DR, txw);
>>                  dws->tx += dws->n_bytes;
>>          }
>>   }
>> @@ -205,7 +205,7 @@ static void dw_reader(struct dw_spi *dws)
>>          u16 rxw;
>>
>>          while (max--) {
>> -               rxw = dw_readw(dws, DW_SPI_DR);
>> +               rxw = dws->read_w(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)
>> @@ -254,11 +254,11 @@ static void int_error_stop(struct dw_spi *dws, const char *msg)
>>
>>   static irqreturn_t interrupt_transfer(struct dw_spi *dws)
>>   {
>> -       u16 irq_status = dw_readw(dws, DW_SPI_ISR);
>> +       u16 irq_status = dws->read_w(dws, DW_SPI_ISR);
>>
>>          /* Error handling */
>>          if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
>> -               dw_readw(dws, DW_SPI_ICR);
>> +               dws->read_w(dws, DW_SPI_ICR);
>>                  int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>>                  return IRQ_HANDLED;
>>          }
>> @@ -283,7 +283,7 @@ static irqreturn_t dw_spi_irq(int irq, void *dev_id)
>>   {
>>          struct spi_master *master = dev_id;
>>          struct dw_spi *dws = spi_master_get_devdata(master);
>> -       u16 irq_status = dw_readw(dws, DW_SPI_ISR) & 0x3f;
>> +       u16 irq_status = dws->read_w(dws, DW_SPI_ISR) & 0x3f;
>>
>>          if (!irq_status)
>>                  return IRQ_NONE;
>> @@ -379,7 +379,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>>                  cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
>>          }
>>
>> -       dw_writew(dws, DW_SPI_CTRL0, cr0);
>> +       dws->write_w(dws, DW_SPI_CTRL0, cr0);
>>
>>          /* Check if current transfer is a DMA transaction */
>>          dws->dma_mapped = map_dma_buffers(master, spi, transfer);
>> @@ -393,7 +393,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>>           */
>>          if (!dws->dma_mapped && !chip->poll_mode) {
>>                  txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
>> -               dw_writew(dws, DW_SPI_TXFLTR, txlevel);
>> +               dws->write_w(dws, DW_SPI_TXFLTR, txlevel);
>>
>>                  /* Set the interrupt mask */
>>                  imask |= SPI_INT_TXEI | SPI_INT_TXOI |
>> @@ -516,11 +516,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->write_w(dws, DW_SPI_TXFLTR, fifo);
>> +                       if (fifo != dws->read_w(dws, DW_SPI_TXFLTR))
>>                                  break;
>>                  }
>> -               dw_writew(dws, DW_SPI_TXFLTR, 0);
>> +               dws->write_w(dws, DW_SPI_TXFLTR, 0);
>>
>>                  dws->fifo_len = (fifo == 1) ? 0 : fifo;
>>                  dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
>> @@ -545,6 +545,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->read_w)
>> +               dws->read_w = dw_readw;
>> +       if (!dws->write_w)
>> +               dws->write_w = dw_writew;
>> +
>>          ret = devm_request_irq(dev, dws->irq, dw_spi_irq, IRQF_SHARED,
>>                          dws->name, master);
>>          if (ret < 0) {
>> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
>> index 855bfdd..1df09e2 100644
>> --- a/drivers/spi/spi-dw.h
>> +++ b/drivers/spi/spi-dw.h
>> @@ -141,6 +141,8 @@ struct dw_spi {
>>   #ifdef CONFIG_DEBUG_FS
>>          struct dentry *debugfs;
>>   #endif
>> +       u16 (*read_w)(struct dw_spi *dws, u32 offset);
>> +       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);
>>   };
>>
>>   static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>> @@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>>          __raw_writew(val, dws->regs + offset);
>>   }
>>
>> +static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
>> +{
>> +       return (u16)__raw_readl(dws->regs + offset);
>> +}
>> +
>> +static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
>> +{
>> +       __raw_writel((u32)val, dws->regs + offset);
>> +}
>> +
>
> So, does simple
> dws->readw = dw_readl;
> dws->writew = dw_writel;
>
> work for you?
>

Yes, but I get the macro conflict shown above and "assignment from 
incompatible pointer type" warnings. If I use the dws->read_w and 
dws->write_w names, I get the incompatible pointer type warnings but it 
works.

Thanks for reviewing.

>>   static inline void spi_enable_chip(struct dw_spi *dws, int enable)
>>   {
>>          dw_writel(dws, DW_SPI_SSIENR, (enable ? 1 : 0));
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

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

[Andy Shevchenko]

On Mon, Mar 9, 2015 at 8:01 PM, Thor Thayer
<tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
>
>
> On 03/07/2015 01:52 PM, Andy Shevchenko wrote:
>>
>> On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
>>>
>>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>>
>>> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
>>> peripherals. The current spi-dw driver uses 16bit accesses in
>>> some locations. Use function pointers to support 32 bit accesses
>>> but retain legacy 16 bit access.

>>> --- 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")) {

"32bit-access"

>>> +                       dws->read_w = dw_readw32;
>>> +                       dws->write_w = dw_writew32;
>>
>>
>> Can we use just  readw/writew (w/o underscores) as names for the
>> accessors?
>>
>
> I tried this initially and got a namespace conflict with the readw & write2
> macros.
> macro writew passed 3 arguments, but takes just 2
> macro readw passed 2 arguments, but takes just 1

Might be I wasn't clear enough. I meant

dws->readw = …
dws->writew = …

>>> --- a/drivers/spi/spi-dw.h
>>> +++ b/drivers/spi/spi-dw.h
>>> @@ -141,6 +141,8 @@ struct dw_spi {
>>>   #ifdef CONFIG_DEBUG_FS
>>>          struct dentry *debugfs;
>>>   #endif
>>> +       u16 (*read_w)(struct dw_spi *dws, u32 offset);
>>> +       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);

readw
writew

As I can see there are no such field names in struct dw_spi.

>>>   };
>>>
>>>   static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>>> @@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32
>>> offset, u16 val)
>>>          __raw_writew(val, dws->regs + offset);
>>>   }
>>>
>>> +static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
>>> +{
>>> +       return (u16)__raw_readl(dws->regs + offset)

Maybe return dw_readl(dws, offset);

>>> +}
>>> +
>>> +static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
>>> +{
>>> +       __raw_writel((u32)val, dws->regs + offset);

dw_writel(dws, offset, val);

>>> +}
>>> +
>>
>> So, does simple
>> dws->readw = dw_readl;
>> dws->writew = dw_writel;
>>
>> work for you?
>>
>
> Yes, but I get the macro conflict shown above and "assignment from
> incompatible pointer type" warnings. If I use the dws->read_w and
> dws->write_w names, I get the incompatible pointer type warnings but it
> works.
>
> Thanks for reviewing.

Okay, I guess there are two variants:
a) replace u16 by u32 in existing functions;
b) introduce new ones like you did.

If no other opinions I think we better go b), but see above comments.

And one more thing. If dw_readw() works for maybe we leave them as is for now?

-- 
With Best Regards,
Andy Shevchenko
--
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/PATCHv2 3/3] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[Thor Thayer]

On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
> On Mon, Mar 9, 2015 at 8:01 PM, Thor Thayer
> <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
>>
>>
>> On 03/07/2015 01:52 PM, Andy Shevchenko wrote:
>>>
>>> On Sat, Mar 7, 2015 at 1:46 AM,  <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
>>>>
>>>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>>>
>>>> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
>>>> peripherals. The current spi-dw driver uses 16bit accesses in
>>>> some locations. Use function pointers to support 32 bit accesses
>>>> but retain legacy 16 bit access.
>
>>>> --- 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")) {
>
> "32bit-access"
>

Thanks. I will make the change.

>>>> +                       dws->read_w = dw_readw32;
>>>> +                       dws->write_w = dw_writew32;
>>>
>>>
>>> Can we use just  readw/writew (w/o underscores) as names for the
>>> accessors?
>>>
>>
>> I tried this initially and got a namespace conflict with the readw & write2
>> macros.
>> macro writew passed 3 arguments, but takes just 2
>> macro readw passed 2 arguments, but takes just 1
>
> Might be I wasn't clear enough. I meant
>
> dws->readw = …
> dws->writew = …
>

Yes, we agree. These are exactly what I was using when I saw the error.

>>>> --- a/drivers/spi/spi-dw.h
>>>> +++ b/drivers/spi/spi-dw.h
>>>> @@ -141,6 +141,8 @@ struct dw_spi {
>>>>    #ifdef CONFIG_DEBUG_FS
>>>>           struct dentry *debugfs;
>>>>    #endif
>>>> +       u16 (*read_w)(struct dw_spi *dws, u32 offset);
>>>> +       void (*write_w)(struct dw_spi *dws, u32 offset, u16 val);
>
> readw
> writew
>
> As I can see there are no such field names in struct dw_spi.
>

Yes. This seems safe but I still see the macro conflict which is strange 
to me. Like you, I initially used the dws->readw and dws->writew because 
I didn't see any conflicting field names.

Are readw & writew reserved variable names?

>>>>    };
>>>>
>>>>    static inline u32 dw_readl(struct dw_spi *dws, u32 offset)
>>>> @@ -163,6 +165,16 @@ static inline void dw_writew(struct dw_spi *dws, u32
>>>> offset, u16 val)
>>>>           __raw_writew(val, dws->regs + offset);
>>>>    }
>>>>
>>>> +static inline u16 dw_readw32(struct dw_spi *dws, u32 offset)
>>>> +{
>>>> +       return (u16)__raw_readl(dws->regs + offset)
>
> Maybe return dw_readl(dws, offset);
>
>>>> +}
>>>> +
>>>> +static inline void dw_writew32(struct dw_spi *dws, u32 offset, u16 val)
>>>> +{
>>>> +       __raw_writel((u32)val, dws->regs + offset);
>
> dw_writel(dws, offset, val);
>

Yes, I'll make these two changes - at least the write change. Based on 
feedback from your suggestion below, the dw_readw32 may be removed.

>>>> +}
>>>> +
>>>
>>> So, does simple
>>> dws->readw = dw_readl;
>>> dws->writew = dw_writel;
>>>
>>> work for you?
>>>
>>
>> Yes, but I get the macro conflict shown above and "assignment from
>> incompatible pointer type" warnings. If I use the dws->read_w and
>> dws->write_w names, I get the incompatible pointer type warnings but it
>> works.
>>
>> Thanks for reviewing.
>
> Okay, I guess there are two variants:
> a) replace u16 by u32 in existing functions;
> b) introduce new ones like you did.
>
> If no other opinions I think we better go b), but see above comments.
>
> And one more thing. If dw_readw() works for maybe we leave them as is for now?
>

Yes, I just need the 32 bit write. I was trying to remain consistent but 
I agree that only changing only writes would minimize the changes.

Thanks!
--
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/PATCHv2 3/3] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[Andy Shevchenko]

On Mon, Mar 9, 2015 at 9:47 PM, Thor Thayer
<tthayer@opensource.altera.com> wrote:
> On 03/09/2015 01:54 PM, Andy Shevchenko wrote:

> Yes, I just need the 32 bit write. I was trying to remain consistent but I
> agree that only changing only writes would minimize the changes.

Which is still makes me anxious.

I have briefly read a chapter for SPI in pdf you sent link to. I
didn't find anything except SPI supports 32 bit data width.

It would be nice if you ping your HW engineers and clarify what
exactly is happening there.

-- 
With Best Regards,
Andy Shevchenko

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

[Thor Thayer]

On 03/09/2015 03:02 PM, Andy Shevchenko wrote:
> On Mon, Mar 9, 2015 at 9:47 PM, Thor Thayer
> <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
>> On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
>
>> Yes, I just need the 32 bit write. I was trying to remain consistent but I
>> agree that only changing only writes would minimize the changes.
>
> Which is still makes me anxious.
>
> I have briefly read a chapter for SPI in pdf you sent link to. I
> didn't find anything except SPI supports 32 bit data width.
>
> It would be nice if you ping your HW engineers and clarify what
> exactly is happening there.
>

I confirmed with our HW engineer that writes are required to be 32 bits. 
We are in the process of updating our documentation to explicitly say this.

Since there are no byte enables on our 32 bit APB interface, writing 
only 1 or 2 bytes could corrupt the other bytes in the 32 bit word. The 
32 bit write enforces requiring an explicit read/modify/write for less 
than 32 bit writes.

Since reads are non-destructive (nothing is being written back into the 
register), it is safe for all reads to get promoted to 32 bit reads.

Is it preferable to have both 32 bit reads and writes for consistency?

Or is it preferable to make the minimal number of changes which would be 
to only add a 32 bit write function?
--
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/PATCHv2 3/3] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[Mark Brown]

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

On Tue, Mar 10, 2015 at 03:34:50PM -0500, Thor Thayer wrote:

> Is it preferable to have both 32 bit reads and writes for consistency?

> Or is it preferable to make the minimal number of changes which would be to
> only add a 32 bit write function?

It's probably preferable to be symmetric, if only to help anyone else
who has a similar system that generates errors instead of silently
handling, but unless someone has any great reason it's not a blocker for
me.

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

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

[Andy Shevchenko]

On Tue, Mar 10, 2015 at 10:34 PM, Thor Thayer
<tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
> On 03/09/2015 03:02 PM, Andy Shevchenko wrote:
>>
>> On Mon, Mar 9, 2015 at 9:47 PM, Thor Thayer
>> <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org> wrote:
>>>
>>> On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
>>
>>
>>> Yes, I just need the 32 bit write. I was trying to remain consistent but
>>> I
>>> agree that only changing only writes would minimize the changes.
>>
>>
>> Which is still makes me anxious.
>>
>> I have briefly read a chapter for SPI in pdf you sent link to. I
>> didn't find anything except SPI supports 32 bit data width.
>>
>> It would be nice if you ping your HW engineers and clarify what
>> exactly is happening there.
>>
>
> I confirmed with our HW engineer that writes are required to be 32 bits. We
> are in the process of updating our documentation to explicitly say this.
>
> Since there are no byte enables on our 32 bit APB interface, writing only 1
> or 2 bytes could corrupt the other bytes in the 32 bit word. The 32 bit
> write enforces requiring an explicit read/modify/write for less than 32 bit
> writes.
>
> Since reads are non-destructive (nothing is being written back into the
> register), it is safe for all reads to get promoted to 32 bit reads.

Thanks for detailed explanation!

> Is it preferable to have both 32 bit reads and writes for consistency?
>
> Or is it preferable to make the minimal number of changes which would be to
> only add a 32 bit write function?

Both I think just to be more clear.

However, I'm now considering what if we just replace
dw_{write/read}w() by l-variants without any additional DT property
and accessor functions?
Would it work for both your cases (old chip, new chip)?
On my side I may test this on Intel MID.

-- 
With Best Regards,
Andy Shevchenko
--
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/PATCHv2 3/3] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access

[Thor Thayer]

On 03/10/2015 03:44 PM, Andy Shevchenko wrote:
> On Tue, Mar 10, 2015 at 10:34 PM, Thor Thayer
> <tthayer@opensource.altera.com> wrote:
>> On 03/09/2015 03:02 PM, Andy Shevchenko wrote:
>>>
>>> On Mon, Mar 9, 2015 at 9:47 PM, Thor Thayer
>>> <tthayer@opensource.altera.com> wrote:
>>>>
>>>> On 03/09/2015 01:54 PM, Andy Shevchenko wrote:
>>>
>>>
>>>> Yes, I just need the 32 bit write. I was trying to remain consistent but
>>>> I
>>>> agree that only changing only writes would minimize the changes.
>>>
>>>
>>> Which is still makes me anxious.
>>>
>>> I have briefly read a chapter for SPI in pdf you sent link to. I
>>> didn't find anything except SPI supports 32 bit data width.
>>>
>>> It would be nice if you ping your HW engineers and clarify what
>>> exactly is happening there.
>>>
>>
>> I confirmed with our HW engineer that writes are required to be 32 bits. We
>> are in the process of updating our documentation to explicitly say this.
>>
>> Since there are no byte enables on our 32 bit APB interface, writing only 1
>> or 2 bytes could corrupt the other bytes in the 32 bit word. The 32 bit
>> write enforces requiring an explicit read/modify/write for less than 32 bit
>> writes.
>>
>> Since reads are non-destructive (nothing is being written back into the
>> register), it is safe for all reads to get promoted to 32 bit reads.
>
> Thanks for detailed explanation!
>
>> Is it preferable to have both 32 bit reads and writes for consistency?
>>
>> Or is it preferable to make the minimal number of changes which would be to
>> only add a 32 bit write function?
>
> Both I think just to be more clear.
>
> However, I'm now considering what if we just replace
> dw_{write/read}w() by l-variants without any additional DT property
> and accessor functions?
> Would it work for both your cases (old chip, new chip)?
> On my side I may test this on Intel MID.
>

Yes, this would be the simplest solution. The l-variant certainly works 
on our legacy SoCs. I'll be curious to hear your testing results.

The data sheet mentions that registers are addressed at 32-bit 
boundaries to remain consistent with the AHB bus (Section 6.1 of 
dw_apb_ssi_db.pdf).  Additionally unused bits are reserved for writes 
and 0 for reads so this seems like a good solution.

My concern is the presence of legacy devices that I have no way of 
testing. Is a Request For Test in the body of the patch sufficient?

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

[Andy Shevchenko]

On Tue, 2015-03-10 at 17:22 -0500, Thor Thayer wrote:


> > However, I'm now considering what if we just replace
> > dw_{write/read}w() by l-variants without any additional DT property
> > and accessor functions?
> > Would it work for both your cases (old chip, new chip)?
> > On my side I may test this on Intel MID.
> >
> 
> Yes, this would be the simplest solution. The l-variant certainly works 
> on our legacy SoCs. I'll be curious to hear your testing results.

Whenever you send a new version of patch.

> The data sheet mentions that registers are addressed at 32-bit 
> boundaries to remain consistent with the AHB bus (Section 6.1 of 
> dw_apb_ssi_db.pdf).  Additionally unused bits are reserved for writes 
> and 0 for reads so this seems like a good solution.
> 
> My concern is the presence of legacy devices that I have no way of 
> testing. Is a Request For Test in the body of the patch sufficient?

Better to write this wider in cover letter or (in case of one patch) in
additional description usually located after '---' line.

Since I fixed couple of bugs in core I'm not sure we have a lot of
users. Nevertheless, can you check who was recent and / or active
contributor to spi-dw-mmio.c and put him / her to Cc list.

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