linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv3] spi: dw-spi: Convert to 32-bit register accesses
@ 2015-03-11 19:20 tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
       [not found] ` <1426101644-5816-1-git-send-email-tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
  0 siblings, 1 reply; 4+ messages in thread
From: tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx @ 2015-03-11 19:20 UTC (permalink / raw)
  To: broonie-DgEjT+Ai2ygdnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
  Cc: jkosina-AlSwsSmVLrQ, linux-spi-u79uwXL29TY76Z2rM5mHXA,
	tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx,
	tthayer.linux-Re5JQEeQqe8AvxtiuMwx3w,
	axel.lin-8E1dMatC8ynQT0dZR+AlfA, baruch-NswTu9S1W3P6gbPvEgmw2w,
	andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA,
	jg1.han-Sze3O3UU22JBDgjK7y7TUQ, galak-sgV2jX0FEOL9JmXXK+q4OQ,
	jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
	swarren-3lzwWm7+Weoh9ZMKESR00Q,
	hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR,
	wsa-z923LK4zBo2bacvFa/9K2g, s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ,
	Julia.Lawall-L2FTfq7BK8M

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

Altera's Arria10 SoC requires all write accesses of APB peripherals
are 32-bit. The DesignWare documentation indicates this change
is acceptable.

Request for Testing:
Please test on legacy DesignWare SPI devices. If a problem is
discovered, please reply to this thread.

Additional Documentation To Support this Change:
The DesignWare documentation DW_apb_ssi databook states:
All registers in the DW_apb_ssi are addressed on 32-bit boundaries
to remain consistent with the AHB bus. Where the physical size of
any register is less than 32-bits wide, the upper unused bits of the
32-bit boundary are reserved. Writing to these bits has no effect,
reading from these bits returns 0. [1]

Tested On:
Altera CycloneV Development Kit
Altera Arria10 Development Kit

[1] Section 6.1 of dw_apb_ssi.pdf (version 3.22a)

Thor Thayer (1):
  spi: dw-spi: Convert 16bit accesses to 32bit accesses

 drivers/spi/spi-dw.c |   34 +++++++++++++++++-----------------
 drivers/spi/spi-dw.h |   10 ----------
 2 files changed, 17 insertions(+), 27 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCHv3] spi: dw-spi: Convert 16bit accesses to 32bit accesses
       [not found] ` <1426101644-5816-1-git-send-email-tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
@ 2015-03-11 19:20   ` tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
       [not found]     ` <1426101644-5816-2-git-send-email-tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
  0 siblings, 1 reply; 4+ messages in thread
From: tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx @ 2015-03-11 19:20 UTC (permalink / raw)
  To: broonie-DgEjT+Ai2ygdnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
  Cc: jkosina-AlSwsSmVLrQ, linux-spi-u79uwXL29TY76Z2rM5mHXA,
	tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx,
	tthayer.linux-Re5JQEeQqe8AvxtiuMwx3w,
	axel.lin-8E1dMatC8ynQT0dZR+AlfA, baruch-NswTu9S1W3P6gbPvEgmw2w,
	andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA,
	jg1.han-Sze3O3UU22JBDgjK7y7TUQ, galak-sgV2jX0FEOL9JmXXK+q4OQ,
	jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
	swarren-3lzwWm7+Weoh9ZMKESR00Q,
	hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR,
	wsa-z923LK4zBo2bacvFa/9K2g, s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ,
	Julia.Lawall-L2FTfq7BK8M

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. A review of the Designware SPI IP databook
indicates the registers will support 32b read and writes to
remain consistent with the AHB bus.

Request for test with existing platforms. Currently tested
on Altera CycloneV and Arria10.

Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
---
r1: Use function pointers to select 16b or 32b accesses.

r2: Use 32b version of function pointers for 16b reads and
writes.

r3: Convert 16b reads and writes to 32b reads and writes. The
DW_apb_ssi databook (section 6-1) states:
All registers in the DW_apb_ssi are addressed at 32-bit boundaries
to remain consistent with the AHB bus. Where the physical size of
any register is less than 32-bits wide, the upper unused bits of
the 32-bit boundary are reserved. Writing to these bits has no
effect; reading from these bits returns 0.
---
 drivers/spi/spi-dw.c |   34 +++++++++++++++++-----------------
 drivers/spi/spi-dw.h |   10 ----------
 2 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 0f01069..9451c8a 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 - dw_readl(dws, DW_SPI_TXFLR);
 
 	/*
 	 * Another concern is about the tx/rx mismatch, we
@@ -178,13 +178,13 @@ 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, dw_readl(dws, DW_SPI_RXFLR));
 }
 
 static void dw_writer(struct dw_spi *dws)
 {
 	u32 max = tx_max(dws);
-	u16 txw = 0;
+	u32 txw = 0;
 
 	while (max--) {
 		/* Set the tx word if the transfer's original "tx" is not null */
@@ -194,18 +194,17 @@ static void dw_writer(struct dw_spi *dws)
 			else
 				txw = *(u16 *)(dws->tx);
 		}
-		dw_writew(dws, DW_SPI_DR, txw);
+		dw_writel(dws, DW_SPI_DR, txw);
 		dws->tx += dws->n_bytes;
 	}
 }
 
 static void dw_reader(struct dw_spi *dws)
 {
-	u32 max = rx_max(dws);
-	u16 rxw;
+	u32 rxw, max = rx_max(dws);
 
 	while (max--) {
-		rxw = dw_readw(dws, DW_SPI_DR);
+		rxw = dw_readl(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)
@@ -228,11 +227,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);
+	u32 irq_status = dw_readl(dws, DW_SPI_ISR);
 
 	/* Error handling */
 	if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
-		dw_readw(dws, DW_SPI_ICR);
+		dw_readl(dws, DW_SPI_ICR);
 		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
 		return IRQ_HANDLED;
 	}
@@ -257,7 +256,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;
+	u32 irq_status = dw_readl(dws, DW_SPI_ISR) & 0x3f;
 
 	if (!irq_status)
 		return IRQ_NONE;
@@ -288,7 +287,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
 	struct dw_spi *dws = spi_master_get_devdata(master);
 	struct chip_data *chip = spi_get_ctldata(spi);
 	u8 imask = 0;
-	u16 txlevel = 0;
+	u32 txlevel = 0;
 	u16 clk_div = 0;
 	u32 speed = 0;
 	u32 cr0 = 0;
@@ -354,7 +353,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
 		cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
 	}
 
-	dw_writew(dws, DW_SPI_CTRL0, cr0);
+	dw_writel(dws, DW_SPI_CTRL0, cr0);
 
 	/* Check if current transfer is a DMA transaction */
 	if (master->can_dma && master->can_dma(master, spi, transfer))
@@ -374,8 +373,9 @@ static int dw_spi_transfer_one(struct spi_master *master,
 			return ret;
 		}
 	} else if (!chip->poll_mode) {
-		txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
-		dw_writew(dws, DW_SPI_TXFLTR, txlevel);
+		txlevel = min_t(u32, dws->fifo_len / 2,
+				dws->len / dws->n_bytes);
+		dw_writel(dws, DW_SPI_TXFLTR, txlevel);
 
 		/* Set the interrupt mask */
 		imask |= SPI_INT_TXEI | SPI_INT_TXOI |
@@ -499,11 +499,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))
+			dw_writel(dws, DW_SPI_TXFLTR, fifo);
+			if (fifo != dw_readl(dws, DW_SPI_TXFLTR))
 				break;
 		}
-		dw_writew(dws, DW_SPI_TXFLTR, 0);
+		dw_writel(dws, DW_SPI_TXFLTR, 0);
 
 		dws->fifo_len = (fifo == 1) ? 0 : fifo;
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 41f77e2..6c91391 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -150,16 +150,6 @@ static inline void dw_writel(struct dw_spi *dws, u32 offset, u32 val)
 	__raw_writel(val, dws->regs + offset);
 }
 
-static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
-{
-	return __raw_readw(dws->regs + offset);
-}
-
-static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
-{
-	__raw_writew(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 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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCHv3] spi: dw-spi: Convert 16bit accesses to 32bit accesses
       [not found]     ` <1426101644-5816-2-git-send-email-tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
@ 2015-03-11 19:28       ` Andy Shevchenko
       [not found]         ` <1426102087.14897.279.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
  0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2015-03-11 19:28 UTC (permalink / raw)
  To: tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
  Cc: broonie-DgEjT+Ai2ygdnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx, jkosina-AlSwsSmVLrQ,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	tthayer.linux-Re5JQEeQqe8AvxtiuMwx3w,
	axel.lin-8E1dMatC8ynQT0dZR+AlfA, baruch-NswTu9S1W3P6gbPvEgmw2w,
	jg1.han-Sze3O3UU22JBDgjK7y7TUQ, galak-sgV2jX0FEOL9JmXXK+q4OQ,
	jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
	swarren-3lzwWm7+Weoh9ZMKESR00Q,
	hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR,
	wsa-z923LK4zBo2bacvFa/9K2g, s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ,
	Julia.Lawall-L2FTfq7BK8M

On Wed, 2015-03-11 at 14:20 -0500, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>

Thanks my couple of comments below. And I will test your next version
tomorrow if everything okay on your side (regarding to my comments)

> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
> peripherals. The current spi-dw driver uses 16bit accesses in
> some locations. A review of the Designware SPI IP databook
> indicates the registers will support 32b read and writes to
> remain consistent with the AHB bus.

Better to exchange this with what you put on cover letter, i.e. cite
documentation here, but describe in common there.

> 
> Request for test with existing platforms. Currently tested
> on Altera CycloneV and Arria10.

> 
> Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> ---
> r1: Use function pointers to select 16b or 32b accesses.
> 
> r2: Use 32b version of function pointers for 16b reads and
> writes.
> 
> r3: Convert 16b reads and writes to 32b reads and writes. The
> DW_apb_ssi databook (section 6-1) states:
> All registers in the DW_apb_ssi are addressed at 32-bit boundaries
> to remain consistent with the AHB bus. Where the physical size of
> any register is less than 32-bits wide, the upper unused bits of
> the 32-bit boundary are reserved. Writing to these bits has no
> effect; reading from these bits returns 0.
> ---
>  drivers/spi/spi-dw.c |   34 +++++++++++++++++-----------------
>  drivers/spi/spi-dw.h |   10 ----------
>  2 files changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> index 0f01069..9451c8a 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 - dw_readl(dws, DW_SPI_TXFLR);
>  
>  	/*
>  	 * Another concern is about the tx/rx mismatch, we
> @@ -178,13 +178,13 @@ 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, dw_readl(dws, DW_SPI_RXFLR));
>  }
>  
>  static void dw_writer(struct dw_spi *dws)
>  {
>  	u32 max = tx_max(dws);
> -	u16 txw = 0;
> +	u32 txw = 0;

Do we really need to touch types? Can you try without this kind of
changes?

>  
>  	while (max--) {
>  		/* Set the tx word if the transfer's original "tx" is not null */
> @@ -194,18 +194,17 @@ static void dw_writer(struct dw_spi *dws)
>  			else
>  				txw = *(u16 *)(dws->tx);
>  		}
> -		dw_writew(dws, DW_SPI_DR, txw);
> +		dw_writel(dws, DW_SPI_DR, txw);
>  		dws->tx += dws->n_bytes;
>  	}
>  }
>  
>  static void dw_reader(struct dw_spi *dws)
>  {
> -	u32 max = rx_max(dws);
> -	u16 rxw;
> +	u32 rxw, max = rx_max(dws);
>  
>  	while (max--) {
> -		rxw = dw_readw(dws, DW_SPI_DR);
> +		rxw = dw_readl(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)
> @@ -228,11 +227,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);
> +	u32 irq_status = dw_readl(dws, DW_SPI_ISR);
>  
>  	/* Error handling */
>  	if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
> -		dw_readw(dws, DW_SPI_ICR);
> +		dw_readl(dws, DW_SPI_ICR);
>  		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>  		return IRQ_HANDLED;
>  	}
> @@ -257,7 +256,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;
> +	u32 irq_status = dw_readl(dws, DW_SPI_ISR) & 0x3f;
>  
>  	if (!irq_status)
>  		return IRQ_NONE;
> @@ -288,7 +287,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>  	struct dw_spi *dws = spi_master_get_devdata(master);
>  	struct chip_data *chip = spi_get_ctldata(spi);
>  	u8 imask = 0;
> -	u16 txlevel = 0;
> +	u32 txlevel = 0;
>  	u16 clk_div = 0;
>  	u32 speed = 0;
>  	u32 cr0 = 0;
> @@ -354,7 +353,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>  		cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
>  	}
>  
> -	dw_writew(dws, DW_SPI_CTRL0, cr0);
> +	dw_writel(dws, DW_SPI_CTRL0, cr0);
>  
>  	/* Check if current transfer is a DMA transaction */
>  	if (master->can_dma && master->can_dma(master, spi, transfer))
> @@ -374,8 +373,9 @@ static int dw_spi_transfer_one(struct spi_master *master,
>  			return ret;
>  		}
>  	} else if (!chip->poll_mode) {
> -		txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
> -		dw_writew(dws, DW_SPI_TXFLTR, txlevel);
> +		txlevel = min_t(u32, dws->fifo_len / 2,
> +				dws->len / dws->n_bytes);
> +		dw_writel(dws, DW_SPI_TXFLTR, txlevel);
>  
>  		/* Set the interrupt mask */
>  		imask |= SPI_INT_TXEI | SPI_INT_TXOI |
> @@ -499,11 +499,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))
> +			dw_writel(dws, DW_SPI_TXFLTR, fifo);
> +			if (fifo != dw_readl(dws, DW_SPI_TXFLTR))
>  				break;
>  		}
> -		dw_writew(dws, DW_SPI_TXFLTR, 0);
> +		dw_writel(dws, DW_SPI_TXFLTR, 0);
>  
>  		dws->fifo_len = (fifo == 1) ? 0 : fifo;
>  		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 41f77e2..6c91391 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -150,16 +150,6 @@ static inline void dw_writel(struct dw_spi *dws, u32 offset, u32 val)
>  	__raw_writel(val, dws->regs + offset);
>  }
>  
> -static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
> -{
> -	return __raw_readw(dws->regs + offset);
> -}
> -
> -static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
> -{
> -	__raw_writew(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));


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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCHv3] spi: dw-spi: Convert 16bit accesses to 32bit accesses
       [not found]         ` <1426102087.14897.279.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
@ 2015-03-11 21:11           ` Thor Thayer
  0 siblings, 0 replies; 4+ messages in thread
From: Thor Thayer @ 2015-03-11 21:11 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: broonie-DgEjT+Ai2ygdnm+yROfE0A,
	grant.likely-QSEj5FYQhm4dnm+yROfE0A,
	dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx, jkosina-AlSwsSmVLrQ,
	linux-spi-u79uwXL29TY76Z2rM5mHXA,
	tthayer.linux-Re5JQEeQqe8AvxtiuMwx3w,
	axel.lin-8E1dMatC8ynQT0dZR+AlfA, baruch-NswTu9S1W3P6gbPvEgmw2w,
	jg1.han-Sze3O3UU22JBDgjK7y7TUQ, galak-sgV2jX0FEOL9JmXXK+q4OQ,
	jarkko.nikula-VuQAYsv1563Yd54FQh9/CA,
	swarren-3lzwWm7+Weoh9ZMKESR00Q,
	hartleys-3FF4nKcrg1dE2c76skzGb0EOCMrvLtNR,
	wsa-z923LK4zBo2bacvFa/9K2g, s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ,
	Julia.Lawall-L2FTfq7BK8M

Hi Andy,

On 03/11/2015 02:28 PM, Andy Shevchenko wrote:
> On Wed, 2015-03-11 at 14:20 -0500, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
>> From: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>
> Thanks my couple of comments below. And I will test your next version
> tomorrow if everything okay on your side (regarding to my comments)
>

Yes, I will make these all these changes and send out an update shortly. 
Thanks for reviewing!

>> Altera's Arria10 SoC interconnect requires a 32 bit write for APB
>> peripherals. The current spi-dw driver uses 16bit accesses in
>> some locations. A review of the Designware SPI IP databook
>> indicates the registers will support 32b read and writes to
>> remain consistent with the AHB bus.
>
> Better to exchange this with what you put on cover letter, i.e. cite
> documentation here, but describe in common there.
>
>>
>> Request for test with existing platforms. Currently tested
>> on Altera CycloneV and Arria10.
>
>>
>> Signed-off-by: Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>> ---
>> r1: Use function pointers to select 16b or 32b accesses.
>>
>> r2: Use 32b version of function pointers for 16b reads and
>> writes.
>>
>> r3: Convert 16b reads and writes to 32b reads and writes. The
>> DW_apb_ssi databook (section 6-1) states:
>> All registers in the DW_apb_ssi are addressed at 32-bit boundaries
>> to remain consistent with the AHB bus. Where the physical size of
>> any register is less than 32-bits wide, the upper unused bits of
>> the 32-bit boundary are reserved. Writing to these bits has no
>> effect; reading from these bits returns 0.
>> ---
>>   drivers/spi/spi-dw.c |   34 +++++++++++++++++-----------------
>>   drivers/spi/spi-dw.h |   10 ----------
>>   2 files changed, 17 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
>> index 0f01069..9451c8a 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 - dw_readl(dws, DW_SPI_TXFLR);
>>
>>   	/*
>>   	 * Another concern is about the tx/rx mismatch, we
>> @@ -178,13 +178,13 @@ 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, dw_readl(dws, DW_SPI_RXFLR));
>>   }
>>
>>   static void dw_writer(struct dw_spi *dws)
>>   {
>>   	u32 max = tx_max(dws);
>> -	u16 txw = 0;
>> +	u32 txw = 0;
>
> Do we really need to touch types? Can you try without this kind of
> changes?
>

Yes. It compiles and runs without issues. I changed the types because 
truncating/extending doesn't seem as efficient but I'm fine with leaving 
them - fewer changes.

>>
>>   	while (max--) {
>>   		/* Set the tx word if the transfer's original "tx" is not null */
>> @@ -194,18 +194,17 @@ static void dw_writer(struct dw_spi *dws)
>>   			else
>>   				txw = *(u16 *)(dws->tx);
>>   		}
>> -		dw_writew(dws, DW_SPI_DR, txw);
>> +		dw_writel(dws, DW_SPI_DR, txw);
>>   		dws->tx += dws->n_bytes;
>>   	}
>>   }
>>
>>   static void dw_reader(struct dw_spi *dws)
>>   {
>> -	u32 max = rx_max(dws);
>> -	u16 rxw;
>> +	u32 rxw, max = rx_max(dws);
>>
>>   	while (max--) {
>> -		rxw = dw_readw(dws, DW_SPI_DR);
>> +		rxw = dw_readl(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)
>> @@ -228,11 +227,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);
>> +	u32 irq_status = dw_readl(dws, DW_SPI_ISR);
>>
>>   	/* Error handling */
>>   	if (irq_status & (SPI_INT_TXOI | SPI_INT_RXOI | SPI_INT_RXUI)) {
>> -		dw_readw(dws, DW_SPI_ICR);
>> +		dw_readl(dws, DW_SPI_ICR);
>>   		int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun");
>>   		return IRQ_HANDLED;
>>   	}
>> @@ -257,7 +256,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;
>> +	u32 irq_status = dw_readl(dws, DW_SPI_ISR) & 0x3f;
>>
>>   	if (!irq_status)
>>   		return IRQ_NONE;
>> @@ -288,7 +287,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>>   	struct dw_spi *dws = spi_master_get_devdata(master);
>>   	struct chip_data *chip = spi_get_ctldata(spi);
>>   	u8 imask = 0;
>> -	u16 txlevel = 0;
>> +	u32 txlevel = 0;
>>   	u16 clk_div = 0;
>>   	u32 speed = 0;
>>   	u32 cr0 = 0;
>> @@ -354,7 +353,7 @@ static int dw_spi_transfer_one(struct spi_master *master,
>>   		cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
>>   	}
>>
>> -	dw_writew(dws, DW_SPI_CTRL0, cr0);
>> +	dw_writel(dws, DW_SPI_CTRL0, cr0);
>>
>>   	/* Check if current transfer is a DMA transaction */
>>   	if (master->can_dma && master->can_dma(master, spi, transfer))
>> @@ -374,8 +373,9 @@ static int dw_spi_transfer_one(struct spi_master *master,
>>   			return ret;
>>   		}
>>   	} else if (!chip->poll_mode) {
>> -		txlevel = min_t(u16, dws->fifo_len / 2, dws->len / dws->n_bytes);
>> -		dw_writew(dws, DW_SPI_TXFLTR, txlevel);
>> +		txlevel = min_t(u32, dws->fifo_len / 2,
>> +				dws->len / dws->n_bytes);
>> +		dw_writel(dws, DW_SPI_TXFLTR, txlevel);
>>
>>   		/* Set the interrupt mask */
>>   		imask |= SPI_INT_TXEI | SPI_INT_TXOI |
>> @@ -499,11 +499,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))
>> +			dw_writel(dws, DW_SPI_TXFLTR, fifo);
>> +			if (fifo != dw_readl(dws, DW_SPI_TXFLTR))
>>   				break;
>>   		}
>> -		dw_writew(dws, DW_SPI_TXFLTR, 0);
>> +		dw_writel(dws, DW_SPI_TXFLTR, 0);
>>
>>   		dws->fifo_len = (fifo == 1) ? 0 : fifo;
>>   		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
>> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
>> index 41f77e2..6c91391 100644
>> --- a/drivers/spi/spi-dw.h
>> +++ b/drivers/spi/spi-dw.h
>> @@ -150,16 +150,6 @@ static inline void dw_writel(struct dw_spi *dws, u32 offset, u32 val)
>>   	__raw_writel(val, dws->regs + offset);
>>   }
>>
>> -static inline u16 dw_readw(struct dw_spi *dws, u32 offset)
>> -{
>> -	return __raw_readw(dws->regs + offset);
>> -}
>> -
>> -static inline void dw_writew(struct dw_spi *dws, u32 offset, u16 val)
>> -{
>> -	__raw_writew(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));
>
>
--
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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-03-11 21:11 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-11 19:20 [PATCHv3] spi: dw-spi: Convert to 32-bit register accesses tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
     [not found] ` <1426101644-5816-1-git-send-email-tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
2015-03-11 19:20   ` [PATCHv3] spi: dw-spi: Convert 16bit accesses to 32bit accesses tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx
     [not found]     ` <1426101644-5816-2-git-send-email-tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
2015-03-11 19:28       ` Andy Shevchenko
     [not found]         ` <1426102087.14897.279.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
2015-03-11 21:11           ` Thor Thayer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).