spi: spi-dw: Select 16b or 32b register access
Subsystems (2)
cover spi: spi-dw: Select 16b or 32b register access
From: Thor Thayer <[email protected]>
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(-)
Signature · 6 lines
1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1425501075-17081-1-git-send-email-tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org
PATCH 1/2 dt-binding: spi: spi-dw: Select 16b or 32b access for Designware SPI
From: Thor Thayer <[email protected]>
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.
--- Documentation/devicetree/bindings/spi/spi-dw.txt | 1 + 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/spi/spi-dw.txt +1 -0
@@ -11,6 +11,7 @@ Required properties: Optional properties: - cs-gpios: see spi-bus.txt +- 32bit_access : use 32 bit register accesses Example:
Signature · 6 lines
1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 6 lines · On Wed, 2015-03-04 at 14:31 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@pub…
From: Thor Thayer <[email protected]>
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?
Quoted 11 lines
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 accessDocumentation/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(-)
Signature · 7 lines
Andy Shevchenko <[email protected]> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy,
Quoted 12 lines · Andy Shevchenko
Quoted 6 lines · On Wed, 2015-03-04 at 14:31 -0600, [email protected] wrote:
From: Thor Thayer <[email protected]>
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.
Quoted 13 lines
Quoted 11 lines
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 accessDocumentation/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(-)
A message that is not in the archive (sent off-list or not yet received): 54F7809E.2010307-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org
Quoted 12 lines · Thor Thayer
Hi Andy,
Quoted 9 lines · Andy Shevchenko
Quoted 6 lines · On Wed, 2015-03-04 at 14:31 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@pub…
From: Thor Thayer <[email protected]>
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.
Quoted 7 lines
Quoted 3 lines
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?
Quoted 5 lines
In reply to your other email, yes it does support the
DW_apb_ssibut 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:
drivers/spi/spi-dw.h +2 -0
@@ -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)
Signature · 7 lines
Andy Shevchenko <[email protected]> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1425552233.14897.189.camel-VuQAYsv1563Yd54FQh9/[email protected]
Hi Andy,
Quoted 17 lines · Andy Shevchenko
Quoted 16 lines · Thor Thayer
Hi Andy,
Quoted 12 lines · Andy Shevchenko
Quoted 6 lines · On Wed, 2015-03-04 at 14:31 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@pub…
From: Thor Thayer <[email protected]>
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?
Quoted 6 lines
Quoted 3 lines
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.
Quoted 12 lines
Quoted 5 lines
In reply to your other email, yes it does support the
DW_apb_ssibut 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:
drivers/spi/spi-dw.h static inline u16 dw_readw(struct dw_spi *dws, u32 offset) · line 177
Quoted 8 lines
drivers/spi/spi-dw.h +2
-0@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 24 lines · Thor Thayer
Hi Andy,
Quoted 21 lines · Andy Shevchenko
Quoted 19 lines · Thor Thayer
Hi Andy,
Quoted 12 lines · Andy Shevchenko
Quoted 6 lines · On Wed, 2015-03-04 at 14:31 -0600, [email protected] wrote:
From: Thor Thayer <[email protected]>
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.
Quoted 6 lines
Quoted 2 lines
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?
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 21 lines
Quoted 21 lines
Quoted 6 lines
In reply to your other email, yes it does support the
DW_apb_ssibut 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:
drivers/spi/spi-dw.h +2
-0@@ -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)
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Quoted 28 lines · Andy Shevchenko
Quoted 27 lines · Thor Thayer
Hi Andy,
Quoted 23 lines · Andy Shevchenko
Quoted 12 lines · Thor Thayer
Hi Andy,
Quoted 9 lines · Andy Shevchenko
Quoted 6 lines · On Wed, 2015-03-04 at 14:31 -0600, [email protected] wrote:
From: Thor Thayer <[email protected]>
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.
Quoted 7 lines
Quoted 3 lines
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?
Quoted 7 lines
Quoted 3 lines
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
Quoted 35 lines
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 21 lines
Quoted 21 lines
Quoted 6 lines
In reply to your other email, yes it does support the
DW_apb_ssibut 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:
drivers/spi/spi-dw.h +2
-0@@ -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)
PATCH 2/2 spi: dw-spi: Pointers select 16b vs. 32b DesignWare access
In reply to [email protected]From: Thor Thayer <[email protected]>
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.
--- 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(-)
drivers/spi/spi-dw-mmio.c +6 -1
@@ -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;
drivers/spi/spi-dw.c +22 -16
@@ -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 ) {
drivers/spi/spi-dw.h +6 -4
@@ -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)
Signature · 1 line
1.7.9.5
Re: [RFC/PATCH 2/2] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access
Quoted 6 lines · On Wed, 2015-03-04 at 14:31 -0600, [email protected] wrote:
From: Thor Thayer <[email protected]>
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.
Quoted 76 lines
Signed-off-by Thor Thayer <[email protected]>--- 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(-)drivers/spi/spi-dw-mmio.c +6
-1@@ -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;drivers/spi/spi-dw.c +5
-6@@ -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 );
drivers/spi/spi-dw.c EXPORT_SYMBOL_GPL(dw_spi_xfer_done); · line 308
Quoted 10 lines
drivers/spi/spi-dw.c +4
-3+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);
Quoted 16 lines
drivers/spi/spi-dw.c +1
-1int_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 */
drivers/spi/spi-dw.c static void pump_transfers(unsigned long data) · line 483
Quoted 10 lines
drivers/spi/spi-dw.c +3
-2-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?
Quoted 41 lines
drivers/spi/spi-dw.c +9
-4@@ -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 ) {drivers/spi/spi-dw.h +0
-0
drivers/spi/spi-dw.h struct dw_spi { · line 154
Quoted 5 lines
drivers/spi/spi-dw.h +2
-0@@ -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?!
drivers/spi/spi-dw.h static inline u32 dw_readl(struct dw_spi *dws, u32 offset) · line 162
Quoted 9 lines
drivers/spi/spi-dw.h +1
-1};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?
drivers/spi/spi-dw.h static inline u32 dw_readl(struct dw_spi *dws, u32 offset) · line 164
Quoted 3 lines
drivers/spi/spi-dw.h +1
-1{ -__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?
drivers/spi/spi-dw.h static inline u32 dw_readl(struct dw_spi *dws, u32 offset) · line 169
Quoted 7 lines
drivers/spi/spi-dw.h +2
-2} -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?
drivers/spi/spi-dw.h static inline u32 dw_readl(struct dw_spi *dws, u32 offset) · line 172
Quoted 3 lines
drivers/spi/spi-dw.h +0
-0}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.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
A message that is not in the archive (sent off-list or not yet received): 1425502525.14897.185.camel-VuQAYsv1563Yd54FQh9/[email protected]
Re: [RFC/PATCH 2/2] spi: dw-spi: Pointers select 16b vs. 32b DesignWare access
Hi Andy,
Quoted 12 lines · Andy Shevchenko
Quoted 6 lines · On Wed, 2015-03-04 at 14:31 -0600, tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@pub…
From: Thor Thayer <[email protected]>
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.
Quoted 2 lines
Signed-off-by Thor Thayer <[email protected]>
<snip>
Quoted 17 lines
Quoted 4 lines
static irqreturn_t interrupt_transfer(struct dw_spi *dws) {drivers/spi/spi-dw.c +0
-1-u16 irq_status =dw_readw (dws,DW_SPI_ISR );drivers/spi/spi-dw.c EXPORT_SYMBOL_GPL(dw_spi_xfer_done); · line 308
Quoted 10 lines
drivers/spi/spi-dw.c +4
-3+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
Quoted 2 lines
dw_readw(dws, DW_SPI_ICR);
Yes, I'll do that.
Quoted 25 lines
Quoted 25 lines
int_error_stop(dws, "interrupt_transfer: fifo overrun/underrun"); return IRQ_HANDLED; }drivers/spi/spi-dw.c +4
-3@@ -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);
Quoted 7 lines
drivers/spi/spi-dw.c static void pump_transfers(unsigned long data) · line 483
Quoted 2 lines
drivers/spi/spi-dw.c +0
-0spi_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?
Yes. I can do this. I tested the 16 bit accesses on our Cyclone5 board and the 32 bit accesses on the Arria10 board.
Quoted 40 lines
Quoted 40 lines
drivers/spi/spi-dw.c +9
-4@@ -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 ) {drivers/spi/spi-dw.h +0
-0
Quoted 8 lines
drivers/spi/spi-dw.h struct dw_spi { · line 154
Quoted 6 lines
drivers/spi/spi-dw.h +2
-0@@ -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.
Quoted 12 lines
drivers/spi/spi-dw.h static inline u32 dw_readl(struct dw_spi *dws, u32 offset) · line 162
Quoted 9 lines
drivers/spi/spi-dw.h +1
-1};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?
Quoted 6 lines
drivers/spi/spi-dw.h static inline u32 dw_readl(struct dw_spi *dws, u32 offset) · line 164
Quoted 3 lines
drivers/spi/spi-dw.h +1
-1{ -__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.
Quoted 11 lines
drivers/spi/spi-dw.h static inline u32 dw_readl(struct dw_spi *dws, u32 offset) · line 169
Quoted 8 lines
drivers/spi/spi-dw.h +2
-2} -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.
Quoted 7 lines
drivers/spi/spi-dw.h static inline u32 dw_readl(struct dw_spi *dws, u32 offset) · line 172
Quoted 3 lines
drivers/spi/spi-dw.h +0
-0}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.
Quoted 3 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 3 lines · On Wed, 2015-03-04 at 14:31 -0600, [email protected] wrote:
From: Thor Thayer <[email protected]>
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
Quoted 15 lines
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 accessDocumentation/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(-)
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy