spi: dw: Reintroduce optional 16 bit data register I/O
Subsystems (2)
cover spi: dw: Reintroduce optional 16 bit data register I/O
The commit dd11444327ce ("spi: dw-spi: Convert 16bit accesses to 32bit accesses") globally changed all register access in the dw_apb_ssi driver to 32 bit access, which breaks data register (FIFO) access on picoXcell platforms.
This series introduces a boolean to the core spi-dw driver to indicate to the core that 16 bit data register access is appropriate, implements the code that respects that flag and updates the dw-spi-mmio driver to allow setting this boolean from the device tree.
A binding documentation fix is included in this series.
Prior to applying this change the following error presents on a picoCell pc3x3 platform:
spi_master spi32766: interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5
With this series applied:
m25p80 spi32766.0: m25p40 (512 Kbytes)
Michael van der Westhuizen (2):
dt: snps,dw-apb-ssi: Describe 16 bit data register usage limitations
spi: dw: Allow interface drivers to limit data I/O to word sizes
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 3 +++ drivers/spi/spi-dw-mmio.c | 5 +++++ drivers/spi/spi-dw.c | 13 +++++++++++-- drivers/spi/spi-dw.h | 11 +++++++++++ 4 files changed, 30 insertions(+), 2 deletions(-)
Signature · 1 line
2.1.4
PATCH 1/2 spi: dw: Allow interface drivers to limit data I/O to word sizes
The commit dd11444327ce ("spi: dw-spi: Convert 16bit accesses to 32bit accesses") changed all 16bit accesses in the DW_apb_ssi driver to 32bit. This, unfortunately, breaks data register access on picoXcell, where the DW IP needs data register accesses to be word accesses (all other accesses appear to be OK).
This change introduces a new master variable to allow interface drivers to specify that 16bit data transfer I/O is required. This change also introduces the ability to set this variable via device tree bindings in the MMIO interface driver.
Before this change, on a picoXcell pc3x3:
spi_master spi32766: interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5
After this change:
m25p80 spi32766.0: m25p40 (512 Kbytes)
--- drivers/spi/spi-dw-mmio.c | 5 +++++ drivers/spi/spi-dw.c | 13 +++++++++++-- drivers/spi/spi-dw.h | 11 +++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-)
drivers/spi/spi-dw-mmio.c +5 -0
@@ -74,6 +74,11 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) dws->max_freq =clk_get_rate (dwsmmio->clk); +if (pdev->dev.of_node) + dws->data_io_16bit = +of_property_read_bool (pdev->dev.of_node, +"snps,data-io-16bit" ); + num_cs =4 ;if (pdev->dev.of_node)
drivers/spi/spi-dw.c +11 -2
@@ -194,7 +194,12 @@ static void dw_writer(struct dw_spi *dws)else txw = *(u16 *)(dws->tx); } -dw_writel (dws,DW_SPI_DR , txw); + +if (dws->data_io_16bit) +dw_writew (dws,DW_SPI_DR , txw); +else +dw_writel (dws,DW_SPI_DR , txw); + dws->tx += dws->n_bytes; } }
@@ -205,7 +210,11 @@ static void dw_reader(struct dw_spi *dws)u16 rxw;while (max--) { - rxw =dw_readl (dws,DW_SPI_DR ); +if (dws->data_io_16bit) + rxw =dw_readw (dws,DW_SPI_DR ); +else + 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 )
drivers/spi/spi-dw.h +11 -0
@@ -109,6 +109,7 @@ struct dw_spi {u32 fifo_len;/* depth of the FIFO buffer */ u32 max_freq;/* max bus freq supported */ +bool data_io_16bit;/* DR is only 16 bits wide */ u16 bus_num;u16 num_cs;/* supported slave numbers */
@@ -145,11 +146,21 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)return __raw_readl (dws->regs + offset); } +static inline u16 dw_readw (struct dw_spi *dws,u32 offset) +{ +return __raw_readw (dws->regs + offset); +} +static inline void dw_writel (struct dw_spi *dws,u32 offset,u32 val) {__raw_writel (val, 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 ));
Signature · 1 line
2.1.4
PATCH v2 1/2 spi: dw: Allow interface drivers to limit data I/O to word sizes
Quoted 8 lines · Michael van der Westhuizen
The commit
dd11444327ce("spi: dw-spi: Convert 16bit accesses to 32bit
accesses") changed all 16bit accesses in theDW_apb_ssidriver to 32bit.
This, unfortunately, breaks data register access on picoXcell, where the
DW IP needs data register accesses to be word accesses (all other accesses appear to be OK).
Thanks for the patch. My comments below.
Quoted 9 lines
This change introduces a new master variable to allow interface drivers
to specify that 16bit data transfer I/O is required. This change also
introduces the ability to set this variable via device tree bindings in
the MMIO interface driver.
Quoted 8 lines
Before this change, on a picoXcell pc3x3:
spi_masterspi32766:interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5After this change:
m25p80 spi32766.0: m25p40 (512 Kbytes)
Please, add Fixes: tag as well.
Quoted 12 lines
Signed-off-by Michael van der Westhuizen <[email protected]>--- drivers/spi/spi-dw-mmio.c | 5 +++++ drivers/spi/spi-dw.c | 13 +++++++++++-- drivers/spi/spi-dw.h | 11 +++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-)drivers/spi/spi-dw-mmio.c +0
-0
drivers/spi/spi-dw-mmio.c static int dw_spi_mmio_probe(struct · line 79
Quoted 6 lines
drivers/spi/spi-dw-mmio.c +2
-0@@ -74,6 +74,11 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) dws->max_freq = clk_get_rate(dwsmmio->clk); +if (pdev->dev.of_node) + dws->data_io_16bit =
I don't like "flexibility" in the DeviceTree data base and thus I would prefer to see what we already have in naming there, i.e. reg-io-width = 2 or 4.
Quoted 76 lines
drivers/spi/spi-dw-mmio.c +3
-0+of_property_read_bool (pdev->dev.of_node, +"snps,data-io-16bit" ); + num_cs =4 ;drivers/spi/spi-dw-mmio.c +0
-0if (pdev->dev.of_node)drivers/spi/spi-dw.c +11
-2@@ -194,7 +194,12 @@ static void dw_writer(struct dw_spi *dws)else txw = *(u16 *)(dws->tx); } -dw_writel (dws,DW_SPI_DR , txw); + +if (dws->data_io_16bit) +dw_writew (dws,DW_SPI_DR , txw); +else +dw_writel (dws,DW_SPI_DR , txw); + dws->tx += dws->n_bytes; } }@@ -205,7 +210,11 @@ static void dw_reader(struct dw_spi *dws)u16 rxw;while (max--) { - rxw =dw_readl (dws,DW_SPI_DR ); +if (dws->data_io_16bit) + rxw =dw_readw (dws,DW_SPI_DR ); +else + rxw =dw_readl (dws,DW_SPI_DR ); +/* Care rx only if the transfer's original "rx" is not null */
drivers/spi/spi-dw.c +0
-0if (dws->rx_end - dws->len) {if (dws->n_bytes ==1 )drivers/spi/spi-dw.h +0
-0@@ -109,6 +109,7 @@ struct dw_spi {u32 fifo_len;/* depth of the FIFO buffer */
u32max_freq; /* max bus freq
supported */drivers/spi/spi-dw.h +1
-0+bool data_io_16bit;/* DR is only 16 bits wide */
drivers/spi/spi-dw.h +0
-0u16 bus_num;u16 num_cs;/* supported slave numbers */
drivers/spi/spi-dw.h +10
-0@@ -145,11 +146,21 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)return __raw_readl (dws->regs + offset); } +static inline u16 dw_readw (struct dw_spi *dws,u32 offset) +{ +return __raw_readw (dws->regs + offset); +} +static inline void dw_writel (struct dw_spi *dws,u32 offset,u32 val) {__raw_writel (val, dws->regs + offset); } +static inline void dw_writew (struct dw_spi *dws,u32 offset,u16 val) +{ +__raw_writew (val, dws->regs + offset); +} +
Quoted 3 lines
static inline void
spi_enable_chip(struct dw_spi *dws, int enable){
dw_writel(dws, DW_SPI_SSIENR, (enable ? 1 : 0));
Regarding to above comment what about something like
static inline u32 dw_read_reg(dws, offset)
{
switch (reg_io_width)
case 2:
return dw_readw();
case 4:
default:
return dw_readl();
}
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Re: [PATCH 1/2] spi: dw: Allow interface drivers to limit data I/O to word sizes
Quoted 29 lines · Andy Shevchenko
Quoted 8 lines · Michael van der Westhuizen
The commit
dd11444327ce("spi: dw-spi: Convert 16bit accesses to 32bit
accesses") changed all 16bit accesses in theDW_apb_ssidriver to 32bit.
This, unfortunately, breaks data register access on picoXcell, where the
DW IP needs data register accesses to be word accesses (all other accesses appear to be OK).Thanks for the patch. My comments below.
Quoted 14 lines
This change introduces a new master variable to allow interface drivers
to specify that 16bit data transfer I/O is required. This change also
introduces the ability to set this variable via device tree bindings in
the MMIO interface driver.Before this change, on a picoXcell pc3x3:
spi_masterspi32766:interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5
Quoted 5 lines
Quoted 3 lines
After this change:
m25p80 spi32766.0: m25p40 (512 Kbytes)Please, add Fixes: tag as well.
OK.
Quoted 15 lines
Quoted 15 lines
Signed-off-by Michael van der Westhuizen <[email protected]>---
drivers/spi/spi-dw-mmio.c | 5 +++++ drivers/spi/spi-dw.c | 13 +++++++++++-- drivers/spi/spi-dw.h | 11 +++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-)drivers/spi/spi-dw-mmio.c +0
-0@@ -74,6 +74,11 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
Quoted 8 lines
drivers/spi/spi-dw-mmio.c static int dw_spi_mmio_probe(struct platform_device *pdev) · line 78
Quoted 4 lines
drivers/spi/spi-dw-mmio.c +2
-0dws->max_freq =clk_get_rate (dwsmmio->clk); +if (pdev->dev.of_node) + dws->data_io_16bit =I don't like "flexibility" in the DeviceTree data base and thus I would prefer to see what we already have in naming there, i.e. reg-io-width = 2 or 4.
Will update.
<snip>
Quoted 28 lines
Quoted 23 lines
drivers/spi/spi-dw-mmio.c +5
-0@@ -145,11 +146,21 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)return __raw_readl (dws->regs + offset); } +static inline u16 dw_readw (struct dw_spi *dws,u32 offset) +{ +return __raw_readw (dws->regs + offset); +} +static inline void
dw_writel(struct dw_spi *dws, u32 offset, u32 val){ __raw_writel(val, dws->regs + offset); }+static inline void
dw_writew(struct dw_spi *dws, u32 offset, u16 val)drivers/spi/spi-dw-mmio.c +4
-0+{ +__raw_writew (val, dws->regs + offset); +} +static inline void
spi_enable_chip(struct dw_spi *dws, int enable)Quoted 2 lines
{ dw_writel(dws, DW_SPI_SSIENR, (enable ? 1 : 0));Regarding to above comment what about something like
Quoted 9 lines
static inline u32
dw_read_reg(dws, offset)
{
switch (reg_io_width)
case 2:
returndw_readw();
case 4:
default:
returndw_readl();
}
Yes, that would be better. Note that this will only be used for data register access, as the other registers seem fine with 32 bit access.
I’ll update the series and resend.
Michael
PATCH 2/2 dt: snps, dw-apb-ssi: Describe 16 bit data register usage limitations
In reply to Michael van der WesthuizenThis change documents a new property for the snps,dw-apb-ssi device, allowing an implementer to interact with the SPI controller data register in 16 bit mode only. This supports a change that unbreaks this driver on picoXcell platforms.
--- Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 3 +++ 1 file changed, 3 insertions(+)
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt +3 -0
@@ -10,6 +10,9 @@ Required properties: Optional properties: - cs-gpios : Specifies the gpio pis to be used for chipselects. - num-cs : The number of chipselects. If omitted, this will default to 4. +- snps,data-io-16bit : A boolean property that, when present, causes + the driver to interact with the data register using 16 bit I/O instead + of the default 32 bit I/O. Child nodes as per the generic SPI binding.
Signature · 1 line
2.1.4
v2 cover spi: dw: Reintroduce optional 16 bit data register I/O
In reply to Michael van der WesthuizenThe commit dd11444327ce ("spi: dw-spi: Convert 16bit accesses to 32bit accesses") globally changed all register access in the dw_apb_ssi driver to 32 bit access, which breaks data register (FIFO) access on picoXcell platforms.
This series introduces a boolean to the core spi-dw driver to indicate to the core that 16 bit data register access is appropriate, implements the code that respects that flag and updates the dw-spi-mmio driver to allow setting this boolean from the device tree.
A binding documentation fix is included in this series.
Prior to applying this change the following error presents on a picoCell pc3x3 platform:
spi_master spi32766: interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5
With this series applied:
m25p80 spi32766.0: m25p40 (512 Kbytes)
Changes in v2:
- Incorporate review feedback from Andy Shevchenko, reworking the bindings to reflect common practice and adjusting the driver to suit.
- Add a wrapper inline function for accessing the data register using the configured with.
Michael van der Westhuizen (2):
dt: snps,dw-apb-ssi: Document new I/O data register width property
spi: dw: Allow interface drivers to limit data I/O to word sizes
drivers/spi/spi-dw-mmio.c | 4 ++++ drivers/spi/spi-dw.c | 4 ++-- drivers/spi/spi-dw.h | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-)
Signature · 1 line
2.1.4
PATCH v2 1/2 dt: snps, dw-apb-ssi: Document new I/O data register width property
This change documents a new property for the snps,dw-apb-ssi device, allowing an implementer to specify either four byte or two bytes access to the SPI controller data register.
This supports a change that unbreaks this driver on picoXcell platforms.
---
Changes in v2:
- Incorporate review feedback from Andy Shevchenko, reworking the bindings to reflect common practice.
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt +2 -0
@@ -10,6 +10,8 @@ Required properties: Optional properties: - cs-gpios : Specifies the gpio pis to be used for chipselects. - num-cs : The number of chipselects. If omitted, this will default to 4. +- snps,reg-io-width : The I/O register width (in bytes) implemented by + this device. Supported values are 2 or 4 (the default). Child nodes as per the generic SPI binding.
Signature · 1 line
2.1.4
PATCH v2 2/2 spi: dw: Allow interface drivers to limit data I/O to word sizes
In reply to Michael van der WesthuizenThe commit dd11444327ce ("spi: dw-spi: Convert 16bit accesses to 32bit accesses") changed all 16bit accesses in the DW_apb_ssi driver to 32bit. This, unfortunately, breaks data register access on picoXcell, where the DW IP needs data register accesses to be word accesses (all other accesses appear to be OK).
This change introduces a new master variable to allow interface drivers to specify that 16bit data transfer I/O is required. This change also introduces the ability to set this variable via device tree bindings in the MMIO interface driver.
Before this change, on a picoXcell pc3x3:
spi_master spi32766: interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5
After this change:
m25p80 spi32766.0: m25p40 (512 Kbytes)
---
Changes in v2:
- Incorporate review feedback from Andy Shevchenko
- Rework the DT bindings to accept an I/O register width as a number of bytes rather than using a boolean spefifying the width preference to be 16 bits.
- Add data register access wrapper functions and use them when reading and writing the data register.
drivers/spi/spi-dw-mmio.c | 4 ++++ drivers/spi/spi-dw.c | 4 ++-- drivers/spi/spi-dw.h | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-)
drivers/spi/spi-dw-mmio.c +4 -0
@@ -74,6 +74,10 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) dws->max_freq =clk_get_rate (dwsmmio->clk); +if (pdev->dev.of_node) +of_property_read_u32 (pdev->dev.of_node,"snps,reg-io-width" , + &dws->reg_io_width); + num_cs =4 ;if (pdev->dev.of_node)
drivers/spi/spi-dw.c +2 -2
@@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)else txw = *(u16 *)(dws->tx); } -dw_writel (dws,DW_SPI_DR , txw); +dw_write_io_reg (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_readl (dws,DW_SPI_DR ); + rxw =dw_read_io_reg (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 )
drivers/spi/spi-dw.h +35 -0
@@ -109,6 +109,7 @@ struct dw_spi {u32 fifo_len;/* depth of the FIFO buffer */ u32 max_freq;/* max bus freq supported */ +u32 reg_io_width;/* DR I/O width in bytes */ u16 bus_num;u16 num_cs;/* supported slave numbers */
@@ -145,11 +146,45 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)return __raw_readl (dws->regs + offset); } +static inline u16 dw_readw (struct dw_spi *dws,u32 offset) +{ +return __raw_readw (dws->regs + offset); +} +static inline void dw_writel (struct dw_spi *dws,u32 offset,u32 val) {__raw_writel (val, dws->regs + offset); } +static inline void dw_writew (struct dw_spi *dws,u32 offset,u16 val) +{ +__raw_writew (val, dws->regs + offset); +} + +static inline u32 dw_read_io_reg (struct dw_spi *dws,u32 offset) +{ +switch (dws->reg_io_width) { +case 2 : +return dw_readw (dws, offset); +case 4 : +default : +return dw_readl (dws, offset); + } +} + +static inline void dw_write_io_reg (struct dw_spi *dws,u32 offset,u32 val) +{ +switch (dws->reg_io_width) { +case 2 : +dw_writew (dws, offset, val); +break ; +case 4 : +default : +dw_writel (dws, offset, val); +break ; + } +} +static inline void spi_enable_chip (struct dw_spi *dws,int enable) {dw_writel (dws,DW_SPI_SSIENR , (enable ?1 :0 ));
Signature · 1 line
2.1.4
PATCH v2 2/2 spi: dw: Allow interface drivers to limit data I/O to word sizes
Quoted 17 lines · Michael van der Westhuizen
The commit
dd11444327ce("spi: dw-spi: Convert 16bit accesses to 32bit
accesses") changed all 16bit accesses in theDW_apb_ssidriver to 32bit.
This, unfortunately, breaks data register access on picoXcell, where the
DW IP needs data register accesses to be word accesses (all other accesses appear to be OK).This change introduces a new master variable to allow interface drivers
to specify that 16bit data transfer I/O is required. This change also
introduces the ability to set this variable via device tree bindings in
the MMIO interface driver.
Quoted 7 lines
Before this change, on a picoXcell pc3x3:
spi_masterspi32766:interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5After this change:
m25p80 spi32766.0: m25p40 (512 Kbytes)
Quoted 125 lines
Fixes dd11444327ce ("spi: dw-spi: Convert 16bit accesses to 32bitaccesses")
Signed-off-by Michael van der Westhuizen <[email protected]>---
Changes in v2:
- Incorporate review feedback from Andy Shevchenko
- Rework the DT bindings to accept an I/O register width as a number of bytes rather than using a boolean spefifying the width preference to be 16 bits.
- Add data register access wrapper functions and use them when reading and writing the data register.drivers/spi/spi-dw-mmio.c | 4 ++++ drivers/spi/spi-dw.c | 4 ++-- drivers/spi/spi-dw.h | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-)drivers/spi/spi-dw-mmio.c +4
-1@@ -74,6 +74,10 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) dws->max_freq = clk_get_rate(dwsmmio->clk); +if (pdev->dev.of_node) + of_property_read_u32(pdev->dev.of_node, "snps,reg-io -width", + &dws->reg_io_width); + num_cs =4 ;if (pdev->dev.of_node)drivers/spi/spi-dw.c +2
-2@@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)else txw = *(u16 *)(dws->tx); } -dw_writel (dws,DW_SPI_DR , txw); +dw_write_io_reg (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_readl (dws,DW_SPI_DR ); + rxw =dw_read_io_reg (dws,DW_SPI_DR );/* Care rx only if the transfer's original "rx" is not null */
drivers/spi/spi-dw.c +0
-0if (dws->rx_end - dws->len) {if (dws->n_bytes ==1 )drivers/spi/spi-dw.h +0
-0@@ -109,6 +109,7 @@ struct dw_spi {u32 fifo_len;/* depth of the FIFO buffer */
u32max_freq; /* max bus freq
supported */drivers/spi/spi-dw.h +1
-0+u32 reg_io_width;/* DR I/O width in bytes */
drivers/spi/spi-dw.h +0
-0u16 bus_num;u16 num_cs;/* supported slave numbers */
drivers/spi/spi-dw.h +22
-0@@ -145,11 +146,45 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)return __raw_readl (dws->regs + offset); } +static inline u16 dw_readw (struct dw_spi *dws,u32 offset) +{ +return __raw_readw (dws->regs + offset); +} +static inline void dw_writel (struct dw_spi *dws,u32 offset,u32 val) {__raw_writel (val, dws->regs + offset); } +static inline void dw_writew (struct dw_spi *dws,u32 offset,u16 val) +{ +__raw_writew (val, dws->regs + offset); +} + +static inline u32 dw_read_io_reg (struct dw_spi *dws,u32 offset) +{ +switch (dws->reg_io_width) { +case 2 : +return dw_readw (dws, offset); +case 4 : +default : +return dw_readl (dws, offset); + } +} + +static inline void dw_write_io_reg(struct dw_spi *dws, u32 offset,u32 val)
drivers/spi/spi-dw.h +12
-0+{ +switch (dws->reg_io_width) { +case 2 : +dw_writew (dws, offset, val); +break ; +case 4 : +default : +dw_writel (dws, offset, val); +break ; + } +} +static inline void spi_enable_chip (struct dw_spi *dws,int enable) {dw_writel (dws,DW_SPI_SSIENR , (enable ?1 :0 ));
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
v2 cover spi: dw: Reintroduce optional 16 bit data register I/O
In reply to Michael van der WesthuizenQuoted 18 lines · Michael van der Westhuizen
The commit
dd11444327ce("spi: dw-spi: Convert 16bit accesses to 32bit
accesses") globally changed all register access in thedw_apb_ssidriver
to 32 bit access, which breaks data register (FIFO) access on picoXcell
platforms.This series introduces a boolean to the core spi-dw driver to indicate
to the core that 16 bit data register access is appropriate, implements
the code that respects that flag and updates the dw-spi-mmio driver to
allow setting this boolean from the device tree.A binding documentation fix is included in this series.
Quoted 8 lines
Prior to applying this change the following error presents on a picoCell pc3x3 platform:
spi_masterspi32766:interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5With this series applied:
m25p80 spi32766.0: m25p40 (512 Kbytes)
Mark, what do you think about this? For me it looks okay.
Quoted 17 lines
Changes in v2:
- Incorporate review feedback from Andy Shevchenko, reworking the bindings to reflect common practice and adjusting the driver to suit.
- Add a wrapper inline function for accessing the data register using the configured with.Michael van der Westhuizen (2):
dt: snps,dw-apb-ssi: Document new I/O data register width property
spi: dw: Allow interface drivers to limit data I/O to word sizesdrivers/spi/spi-dw-mmio.c | 4 ++++ drivers/spi/spi-dw.c | 4 ++-- drivers/spi/spi-dw.h | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-)
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Mark Brown
maintainerv2 cover spi: dw: Reintroduce optional 16 bit data register I/O
Quoted 6 lines · Andy Shevchenko
Quoted 2 lines · Michael van der Westhuizen
With this series applied:
m25p80 spi32766.0: m25p40 (512 Kbytes)Mark, what do you think about this? For me it looks okay.
Please don't send content free pings, if you want me to review a patch
please send me the patch.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150724/c746fd8c/attachment.sig>
v2 cover spi: dw: Reintroduce optional 16 bit data register I/O
Quoted 4 lines · Mark Brown
Quoted 2 lines · Andy Shevchenko
On Wed, 2015-06-24 at 18:34 +0200, Michael van der Westhuizen wrote:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
With this series applied:
m25p80 spi32766.0: m25p40 (512 Kbytes)Mark, what do you think about this? For me it looks okay.
Please don't send content free pings, if you want me to review a patch
please send me the patch.
Hmm? It's strange since patch series was sent to mailing list (Cc'ed to you) on 24.06 [1].
[1]
https://www.mail-archive.com/linux-spi at vger.kernel.org/msg05530.html
P.S. It has not mine authorship, though I can resend the series if needed.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Mark Brown
maintainerv2 cover spi: dw: Reintroduce optional 16 bit data register I/O
Quoted 6 lines · Andy Shevchenko
Quoted 3 lines · Mark Brown
Please don't send content free pings, if you want me to review a patch
please send me the patch.
Quoted 8 lines
Hmm? It's strange since patch series was sent to mailing list (Cc'ed to you) on 24.06 [1].
[1]
https://www.mail-archive.com/linux-spi at vger.kernel.org/msg05530.html
P.S. It has not mine authorship, though I can resend the series if needed.
It's possible there was some discussion of the patch that made
it look it needed a resubmit, or that it was buried as reply in the
middle of discussion of some other patches (eg, a previous version).
Either way to repeat what I said if you want me to review a patch please
send me the patch.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150727/bf80ac6f/attachment.sig>
v2 cover spi: dw: Reintroduce optional 16 bit data register I/O
Hi Mark,
Quoted 15 lines · Mark Brown
Quoted 11 lines · Andy Shevchenko
Quoted 3 lines · Mark Brown
Please don't send content free pings, if you want me to review a patch
please send me the patch.Hmm? It's strange since patch series was sent to mailing list (Cc'ed to you) on 24.06 [1].
[1]
https://www.mail-archive.com/linux-spi at vger.kernel.org/msg05530.html
Quoted 8 lines
Quoted 2 lines
P.S. It has not mine authorship, though I can resend the series if needed.It's possible there was some discussion of the patch that made it look it needed a resubmit, or that it was buried as reply in the middle of discussion of some other patches (eg, a previous version). Either way to repeat what I said if you want me to review a patch please send me the patch.
I?ve just resent the series.
Michael
Mark Brown
maintainerv2 cover spi: dw: Reintroduce optional 16 bit data register I/O
Quoted 7 lines · Michael van der Westhuizen
Quoted 5 lines
It's possible there was some discussion of the patch that made it look it needed a resubmit, or that it was buried as reply in the middle of discussion of some other patches (eg, a previous version). Either way to repeat what I said if you want me to review a patch please send me the patch.
I?ve just resent the series.
You've sent them as a reply in the middle of another thread which is
something I just indicated above was a bad idea in the message you're
replying to here (it's a great way to either make it hard to figure out
what the current version of the code is or get your message deleted
along with the rest of the thread) and you've decided to label them as
[RESEND instead of [PATCH :(
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150727/b2e3ca89/attachment-0001.sig>
v2 cover spi: dw: Reintroduce optional 16 bit data register I/O
Quoted 8 lines · Mark Brown
Quoted 5 lines · Michael van der Westhuizen
Quoted 5 lines
It's possible there was some discussion of the patch that made it look it needed a resubmit, or that it was buried as reply in the middle of discussion of some other patches (eg, a previous version). Either way to repeat what I said if you want me to review a patch please send me the patch.
Quoted 8 lines
Quoted 1 line
I?ve just resent the series.
You've sent them as a reply in the middle of another thread which is something I just indicated above was a bad idea in the message you're replying to here (it's a great way to either make it hard to figure out what the current version of the code is or get your message deleted along with the rest of the thread) and you've decided to label them as [RESEND instead of [PATCH :(
Michael, I believe you need to resend as a v3 in a separate thread if you aware of this code.
(Though it seems already late for v4.3)
Signature · 2 lines
With Best Regards, Andy Shevchenko
v2 cover spi: dw: Reintroduce optional 16 bit data register I/O
In reply to Michael van der WesthuizenThe commit dd11444327ce ("spi: dw-spi: Convert 16bit accesses to 32bit accesses") globally changed all register access in the dw_apb_ssi driver to 32 bit access, which breaks data register (FIFO) access on picoXcell platforms.
This series introduces a boolean to the core spi-dw driver to indicate to the core that 16 bit data register access is appropriate, implements the code that respects that flag and updates the dw-spi-mmio driver to allow setting this boolean from the device tree.
A binding documentation fix is included in this series.
Prior to applying this change the following error presents on a picoCell pc3x3 platform:
spi_master spi32766: interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5
With this series applied:
m25p80 spi32766.0: m25p40 (512 Kbytes)
(Resend requested by Mark Brown)
Changes in v2:
- Incorporate review feedback from Andy Shevchenko, reworking the bindings to reflect common practice and adjusting the driver to suit.
- Add a wrapper inline function for accessing the data register using the configured with.
Michael van der Westhuizen (2):
dt: snps,dw-apb-ssi: Document new I/O data register width property
spi: dw: Allow interface drivers to limit data I/O to word sizes
drivers/spi/spi-dw-mmio.c | 4 ++++ drivers/spi/spi-dw.c | 4 ++-- drivers/spi/spi-dw.h | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-)
Signature · 1 line
2.1.4
PATCH RESEND v2 1/2 dt: snps, dw-apb-ssi: Document new I/O data register width property
This change documents a new property for the snps,dw-apb-ssi device, allowing an implementer to specify either four byte or two bytes access to the SPI controller data register.
This supports a change that unbreaks this driver on picoXcell platforms.
---
Changes in v2:
- Incorporate review feedback from Andy Shevchenko, reworking the bindings to reflect common practice.
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt +2 -0
@@ -10,6 +10,8 @@ Required properties: Optional properties: - cs-gpios : Specifies the gpio pis to be used for chipselects. - num-cs : The number of chipselects. If omitted, this will default to 4. +- snps,reg-io-width : The I/O register width (in bytes) implemented by + this device. Supported values are 2 or 4 (the default). Child nodes as per the generic SPI binding.
Signature · 1 line
2.1.4
PATCH RESEND v2 1/2 dt: snps, dw-apb-ssi: Document new I/O data register width property
Quoted 22 lines · Michael van der Westhuizen
This change documents a new property for the snps,dw-apb-ssi device, allowing an implementer to specify either four byte or two bytes access to the SPI controller data register.
This supports a change that unbreaks this driver on picoXcell platforms.
Signed-off-by Michael van der Westhuizen <[email protected]>---
Changes in v2:
- Incorporate review feedback from Andy Shevchenko, reworking the bindings to reflect common practice.Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 2 ++ 1 file changed, 2 insertions(+)Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt +0
-0
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt Required properties: · line 14
Quoted 5 lines
Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt +2
-0@@ -10,6 +10,8 @@ Required properties: Optional properties: - cs-gpios : Specifies the gpio pis to be used for chipselects. - num-cs : The number of chipselects. If omitted, this will default to 4. +- snps,reg-io-width : The I/O register width (in bytes) implemented by + this device. Supported values are 2 or 4 (the default).
Drop the snps as this is already a common property (at least for UARTs).
Rob
PATCH RESEND v2 2/2 spi: dw: Allow interface drivers to limit data I/O to word sizes
In reply to Michael van der WesthuizenThe commit dd11444327ce ("spi: dw-spi: Convert 16bit accesses to 32bit accesses") changed all 16bit accesses in the DW_apb_ssi driver to 32bit. This, unfortunately, breaks data register access on picoXcell, where the DW IP needs data register accesses to be word accesses (all other accesses appear to be OK).
This change introduces a new master variable to allow interface drivers to specify that 16bit data transfer I/O is required. This change also introduces the ability to set this variable via device tree bindings in the MMIO interface driver.
Before this change, on a picoXcell pc3x3:
spi_master spi32766: interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5
After this change:
m25p80 spi32766.0: m25p40 (512 Kbytes)
---
Changes in v2:
- Incorporate review feedback from Andy Shevchenko
- Rework the DT bindings to accept an I/O register width as a number of bytes rather than using a boolean spefifying the width preference to be 16 bits.
- Add data register access wrapper functions and use them when reading and writing the data register.
drivers/spi/spi-dw-mmio.c | 4 ++++ drivers/spi/spi-dw.c | 4 ++-- drivers/spi/spi-dw.h | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-)
drivers/spi/spi-dw-mmio.c +4 -0
@@ -74,6 +74,10 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) dws->max_freq =clk_get_rate (dwsmmio->clk); +if (pdev->dev.of_node) +of_property_read_u32 (pdev->dev.of_node,"snps,reg-io-width" , + &dws->reg_io_width); + num_cs =4 ;if (pdev->dev.of_node)
drivers/spi/spi-dw.c +2 -2
@@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)else txw = *(u16 *)(dws->tx); } -dw_writel (dws,DW_SPI_DR , txw); +dw_write_io_reg (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_readl (dws,DW_SPI_DR ); + rxw =dw_read_io_reg (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 )
drivers/spi/spi-dw.h +35 -0
@@ -109,6 +109,7 @@ struct dw_spi {u32 fifo_len;/* depth of the FIFO buffer */ u32 max_freq;/* max bus freq supported */ +u32 reg_io_width;/* DR I/O width in bytes */ u16 bus_num;u16 num_cs;/* supported slave numbers */
@@ -145,11 +146,45 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)return __raw_readl (dws->regs + offset); } +static inline u16 dw_readw (struct dw_spi *dws,u32 offset) +{ +return __raw_readw (dws->regs + offset); +} +static inline void dw_writel (struct dw_spi *dws,u32 offset,u32 val) {__raw_writel (val, dws->regs + offset); } +static inline void dw_writew (struct dw_spi *dws,u32 offset,u16 val) +{ +__raw_writew (val, dws->regs + offset); +} + +static inline u32 dw_read_io_reg (struct dw_spi *dws,u32 offset) +{ +switch (dws->reg_io_width) { +case 2 : +return dw_readw (dws, offset); +case 4 : +default : +return dw_readl (dws, offset); + } +} + +static inline void dw_write_io_reg (struct dw_spi *dws,u32 offset,u32 val) +{ +switch (dws->reg_io_width) { +case 2 : +dw_writew (dws, offset, val); +break ; +case 4 : +default : +dw_writel (dws, offset, val); +break ; + } +} +static inline void spi_enable_chip (struct dw_spi *dws,int enable) {dw_writel (dws,DW_SPI_SSIENR , (enable ?1 :0 ));
Signature · 1 line
2.1.4
PATCH RESEND v2 2/2 spi: dw: Allow interface drivers to limit data I/O to word sizes
Quoted 16 lines · Michael van der Westhuizen
The commit
dd11444327ce("spi: dw-spi: Convert 16bit accesses to 32bit accesses") changed all 16bit accesses in theDW_apb_ssidriver to 32bit. This, unfortunately, breaks data register access on picoXcell, where the DW IP needs data register accesses to be word accesses (all other accesses appear to be OK).This change introduces a new master variable to allow interface drivers to specify that 16bit data transfer I/O is required. This change also introduces the ability to set this variable via device tree bindings in the MMIO interface driver.
Before this change, on a picoXcell pc3x3:
spi_masterspi32766:interrupt_transfer: fifo overrun/underrun m25p80 spi32766.0: error -5 reading 9f
m25p80: probe of spi32766.0 failed with error -5
Quoted 6 lines
After this change:
m25p80 spi32766.0: m25p40 (512 Kbytes)Fixes dd11444327ce ("spi: dw-spi: Convert 16bit accesses to 32bit accesses")Signed-off-by Michael van der Westhuizen <[email protected]>Reviewed-by Andy Shevchenko <[email protected]>
One minor comment below.
Quoted 20 lines
---
Changes in v2:
- Incorporate review feedback from Andy Shevchenko
- Rework the DT bindings to accept an I/O register width as a number of bytes rather than using a boolean spefifying the width preference to be 16 bits.
- Add data register access wrapper functions and use them when reading and writing the data register.drivers/spi/spi-dw-mmio.c | 4 ++++ drivers/spi/spi-dw.c | 4 ++-- drivers/spi/spi-dw.h | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-)drivers/spi/spi-dw-mmio.c +0
-0
drivers/spi/spi-dw-mmio.c static int dw_spi_mmio_probe(struct platform_device *pdev) · line 77
Quoted 4 lines
drivers/spi/spi-dw-mmio.c +1
-0@@ -74,6 +74,10 @@ static int dw_spi_mmio_probe(struct platform_device *pdev) dws->max_freq =clk_get_rate (dwsmmio->clk); +if (pdev->dev.of_node)
of_property_read_u32() is NULL-aware. Since the option is optional we may drop the condition.
drivers/spi/spi-dw-mmio.c static int dw_spi_mmio_probe(struct platform_device *pdev) · line 78
Quoted 1 line
drivers/spi/spi-dw-mmio.c +1
-0+ of_property_read_u32(pdev->dev.of_node, "snps,reg-io-width",
Like Rob said, drop 'snps,' prefix as it is a generic property name.
Quoted 92 lines
drivers/spi/spi-dw-mmio.c +2
-0+ &dws->reg_io_width); + num_cs =4 ;if (pdev->dev.of_node)drivers/spi/spi-dw.c +2
-2@@ -194,7 +194,7 @@ static void dw_writer(struct dw_spi *dws)else txw = *(u16 *)(dws->tx); } -dw_writel (dws,DW_SPI_DR , txw); +dw_write_io_reg (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_readl (dws,DW_SPI_DR ); + rxw =dw_read_io_reg (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 )drivers/spi/spi-dw.h +35
-1@@ -109,6 +109,7 @@ struct dw_spi {u32 fifo_len;/* depth of the FIFO buffer */ u32 max_freq;/* max bus freq supported */ +u32 reg_io_width;/* DR I/O width in bytes */ u16 bus_num;u16 num_cs;/* supported slave numbers */ @@ -145,11 +146,45 @@ static inline u32 dw_readl(struct dw_spi *dws, u32 offset)return __raw_readl (dws->regs + offset); } +static inline u16 dw_readw (struct dw_spi *dws,u32 offset) +{ +return __raw_readw (dws->regs + offset); +} +static inline void dw_writel (struct dw_spi *dws,u32 offset,u32 val) {__raw_writel (val, dws->regs + offset); } +static inline void dw_writew (struct dw_spi *dws,u32 offset,u16 val) +{ +__raw_writew (val, dws->regs + offset); +} + +static inline u32 dw_read_io_reg (struct dw_spi *dws,u32 offset) +{ +switch (dws->reg_io_width) { +case 2 : +return dw_readw (dws, offset); +case 4 : +default : +return dw_readl (dws, offset); + } +} + +static inline void dw_write_io_reg (struct dw_spi *dws,u32 offset,u32 val) +{ +switch (dws->reg_io_width) { +case 2 : +dw_writew (dws, offset, val); +break ; +case 4 : +default : +dw_writel (dws, offset, val); +break ; + } +} +static inline void spi_enable_chip (struct dw_spi *dws,int enable) {dw_writel (dws,DW_SPI_SSIENR , (enable ?1 :0 )); --2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
With Best Regards, Andy Shevchenko