[PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Talel Shenhar]

Update the dw-apb-ssi binding document, to add an optional 'cs-override'
devicetree property.

This property adds the ability for dw spi controller driver to work with
the dw spi controller found on Alpine chips.

The dw spi controller has an auto-deselect of Chip-Select, in case there is
no data inside the Tx FIFO. While working on platforms with Alpine chips,
auto-deselect mode causes an issue for some spi devices that can't handle
the Chip-Select deselect in the middle of a transaction. It is a normal
behavior for a Tx FIFO to be empty in the middle of a transaction, due to
busy cpu. In the Alpine chip family an option to change the default
behavior was added to the original dw spi controller to prevent this issue
of de-asserting Chip-Select once TX FIFO is empty. The change was to allow
SW to force the Chip-Select. With this change, as long as the Slave Enable
Register is asserted, the Chip-Select will be asserted. As a result, it is
necessary to deselect the Slave Select Register once the transaction is
done. This feature is enabled via a device property called 'cs-override'.
Once the driver identifies the 'cs-override' property, it enables the hw
fixup logic, by writing to a dedicated register found in the IP reserved
area and will starts deselecting the Slave Select Register when the
transfer ends.

Signed-off-by: Talel Shenhar <talel@amazon.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
index 642d3fb..dd366f5 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
@@ -14,6 +14,8 @@ Optional properties:
 - num-cs : The number of chipselects. If omitted, this will default to 4.
 - reg-io-width : The I/O register width (in bytes) implemented by this
   device.  Supported values are 2 or 4 (the default).
+- cs-override: Enable explicit Chip-Select deselect during transfer instead of
+  default auto-deselect upon tx FIFO becoming empty.
 
 Child nodes as per the generic SPI binding.
 
-- 
2.7.4

[PATCH 2/2] dw: spi: add 'cs-override' DT property support

[Talel Shenhar]

Add support for an optional devicetree property called 'cs-override', which
is necessary for the Amazon Alpine spi controller. 'cs-override' is used in
the dw spi driver if specified in the devicetree.  Otherwise, fall back to
driver default behavior, i.e., original dw IP hw driver behavior.

Signed-off-by: Talel Shenhar <talel@amazon.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 drivers/spi/spi-dw-mmio.c | 2 ++
 drivers/spi/spi-dw.c      | 6 ++++++
 drivers/spi/spi-dw.h      | 2 ++
 3 files changed, 10 insertions(+)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index a768461..a2f7359 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -195,6 +195,8 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 					goto out;
 			}
 		}
+		dws->cs_override = of_property_read_bool(pdev->dev.of_node,
+							 "cs-override");
 	}
 
 	init_func = device_get_match_data(&pdev->dev);
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 3e205ab..eb4c3d1 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -144,6 +144,8 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
 
 	if (!enable)
 		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
+	else
+		dw_writel(dws, DW_SPI_SER, 0);
 }
 EXPORT_SYMBOL_GPL(dw_spi_set_cs);
 
@@ -463,6 +465,10 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
 		dws->fifo_len = (fifo == 1) ? 0 : fifo;
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
 	}
+
+	/* enable HW fixup for explicit CS deselect for Amazon's alpine chip */
+	if (dws->cs_override)
+		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
 }
 
 int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 0168b08..c9c1588 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -32,6 +32,7 @@
 #define DW_SPI_IDR			0x58
 #define DW_SPI_VERSION			0x5c
 #define DW_SPI_DR			0x60
+#define DW_SPI_CS_OVERRIDE		0xf4
 
 /* Bit fields in CTRLR0 */
 #define SPI_DFS_OFFSET			0
@@ -109,6 +110,7 @@ struct dw_spi {
 	u32			fifo_len;	/* depth of the FIFO buffer */
 	u32			max_freq;	/* max bus freq supported */
 
+	int			cs_override;
 	u32			reg_io_width;	/* DR I/O width in bytes */
 	u16			bus_num;
 	u16			num_cs;		/* supported slave numbers */
-- 
2.7.4

Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Mark Brown]

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

On Wed, Oct 10, 2018 at 10:08:12AM +0300, Talel Shenhar wrote:

> The dw spi controller has an auto-deselect of Chip-Select, in case there is
> no data inside the Tx FIFO. While working on platforms with Alpine chips,

Why would we ever want to use this behaviour?  It will be broken for any
non-trivial SPI message such as those made with multiple transfers
anyway.  Why not just unconditionally control it manually?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Talel Shenhar]

On 10/10/2018 01:18 PM, Mark Brown wrote:
> On Wed, Oct 10, 2018 at 10:08:12AM +0300, Talel Shenhar wrote:
>
>> The dw spi controller has an auto-deselect of Chip-Select, in case there is
>> no data inside the Tx FIFO. While working on platforms with Alpine chips,
> Why would we ever want to use this behaviour?  It will be broken for any
> non-trivial SPI message such as those made with multiple transfers
> anyway.  Why not just unconditionally control it manually?

This behavior (auto-deselect of Chip-Select) is the default behavior of dw spi controller hw.
On Alpine chip there is additional behavior added to the dw spi controller hw that allows the sw to disable this behavior.
This patch allows the dw driver to enable this hw workaround and add the needed sw manual control for it.

Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Mark Brown]

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

On Wed, Oct 10, 2018 at 02:23:40PM +0300, Talel Shenhar wrote:
> On 10/10/2018 01:18 PM, Mark Brown wrote:
> > On Wed, Oct 10, 2018 at 10:08:12AM +0300, Talel Shenhar wrote:

> >> The dw spi controller has an auto-deselect of Chip-Select, in case there is
> >> no data inside the Tx FIFO. While working on platforms with Alpine chips,

> > Why would we ever want to use this behaviour?  It will be broken for any
> > non-trivial SPI message such as those made with multiple transfers
> > anyway.  Why not just unconditionally control it manually?

> This behavior (auto-deselect of Chip-Select) is the default behavior of dw spi controller hw.
> On Alpine chip there is additional behavior added to the dw spi controller hw that allows the sw to disable this behavior.
> This patch allows the dw driver to enable this hw workaround and add the needed sw manual control for it.

If this is a modified IP with additional features then it should be
given a new compatible string rather than having a property - it's not
just configuration of the existing IP, it's a new thing and we may find
there are other quirks that have to be taken care of for it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Mark Brown]

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

On Wed, Oct 10, 2018 at 11:58:40AM +0000, Woodhouse, David wrote:
> On Wed, 2018-10-10 at 12:27 +0100, Mark Brown wrote:

> > If this is a modified IP with additional features then it should be
> > given a new compatible string rather than having a property - it's not
> > just configuration of the existing IP, it's a new thing and we may find
> > there are other quirks that have to be taken care of for it.

> No.

> It's an extension of the existing IP which is explicitly designed to be
> compatible with existing drivers via an opt-in feature.

That's totally normal and something that there's already infrastructure
to handle, plenty of other IPs have new versions with new features - you
can just add a new compatible string and use that to decide which
features to enable, and if it's backwards compatible with old versions
of the driver supporting older hardware then you can list multiple
compatibles.  As we just discussed on IRC this is partly a policy thing
now I've been told that it's a modified version of the hardware rather
than a configuration or integration option, it's easier than having
implementations of new features trickle out from the vendor and needing
to enable them all one by one in device trees (which does happen).

> Which is exactly what we've spent the last decade or two trying to beat
> hardware designers into doing, instead of randomly breaking
> compatibility for no good reason.

That's great and we get to reuse all the driver code with a quirk (a
quirk which fixes the hardware to be more compatible with devices, this
is a really good hardware change).  Ideally we'd be able to enumerate
things like IP versions and options from hardware but that's a more
entertaining problem.

Having said all this if there are production systems using this
property, especially production systems where people other than the
system integrator can realistically deploy their own kernel separate to
the device tree, then supporting those existing DTs even if they're not
doing the ideal thing might be the best thing.  You mentioned that this
might be the case, can you check what the status is there please?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/2] dt-bindings: spi: dw: add compatible for Alpine spi controller

[Talel Shenhar]

This compatible adds the ability for dw spi controller driver to work with
the dw spi controller found on Alpine chips.

The dw spi controller has an auto-deselect of Chip-Select, in case there is
no data inside the Tx FIFO. While working on platforms with Alpine chips,
auto-deselect mode causes an issue for some spi devices that can't handle
the Chip-Select deselect in the middle of a transaction. It is a normal
behavior for a Tx FIFO to be empty in the middle of a transaction, due to
busy cpu. In the Alpine chip family an option to change the default
behavior was added to the original dw spi controller to prevent this issue
of de-asserting Chip-Select once TX FIFO is empty. The change was to allow
SW manual control of the Chip-Select. With this change, as long as the
Slave Enable Register is asserted, the Chip-Select will be asserted. As a
result, it is necessary to deselect the Slave Select Register once the
transaction is done. This feature is enabled via a new device compatible
string called 'al,alpine-dw-apb-ssi'.  Once the driver identifies the new
compatible string, it enables the hw fixup logic, by writing to a dedicated
register found in the IP reserved area and will start manual deselecting
the Slave Select Register when the transfer ends.

Signed-off-by: Talel Shenhar <talel@amazon.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
index 642d3fb..d25b1f8 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
@@ -2,7 +2,7 @@ Synopsys DesignWare AMBA 2.0 Synchronous Serial Interface.
 
 Required properties:
 - compatible : "snps,dw-apb-ssi" or "mscc,<soc>-spi", where soc is "ocelot" or
-  "jaguar2"
+  "jaguar2", or "al,alpine-dw-apb-ssi"
 - reg : The register base for the controller. For "mscc,<soc>-spi", a second
   register set is required (named ICPU_CFG:SPI_MST)
 - interrupts : One interrupt, used by the controller.
-- 
2.7.4

[PATCH 2/2] dw: spi: add support for Alpine spi controller

[Talel Shenhar]

Add support for a new devicetree compatible string called
'al,alpine-apb-ssi', which is necessary for the Amazon Alpine spi
controller. 'al,alpine-dw-apb-ssi' is used in the dw spi driver if
specified in the devicetree.  Otherwise, fall back to driver default
behavior, i.e. original dw IP hw driver behavior.

Signed-off-by: Talel Shenhar <talel@amazon.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 drivers/spi/spi-dw-mmio.c | 9 +++++++++
 drivers/spi/spi-dw.c      | 6 ++++++
 drivers/spi/spi-dw.h      | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index a768461..88b04d2 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -126,6 +126,14 @@ static int dw_spi_mscc_jaguar2_init(struct platform_device *pdev,
 				JAGUAR2_IF_SI_OWNER_OFFSET);
 }
 
+static int dw_spi_alpine_init(struct platform_device *pdev,
+			      struct dw_spi_mmio *dwsmmio)
+{
+	dwsmmio->dws.cs_override = 1;
+
+	return 0;
+}
+
 static int dw_spi_mmio_probe(struct platform_device *pdev)
 {
 	int (*init_func)(struct platform_device *pdev,
@@ -230,6 +238,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "snps,dw-apb-ssi", },
 	{ .compatible = "mscc,ocelot-spi", .data = dw_spi_mscc_ocelot_init},
 	{ .compatible = "mscc,jaguar2-spi", .data = dw_spi_mscc_jaguar2_init},
+	{ .compatible = "al,alpine-dw-apb-ssi", .data = dw_spi_alpine_init},
 	{ /* end of table */}
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 3e205ab..b705f2b 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -144,6 +144,8 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
 
 	if (!enable)
 		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
+	else if (dws->cs_override)
+		dw_writel(dws, DW_SPI_SER, 0);
 }
 EXPORT_SYMBOL_GPL(dw_spi_set_cs);
 
@@ -463,6 +465,10 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
 		dws->fifo_len = (fifo == 1) ? 0 : fifo;
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
 	}
+
+	/* enable HW fixup for explicit CS deselect for Amazon's alpine chip */
+	if (dws->cs_override)
+		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
 }
 
 int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 0168b08..c9c1588 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -32,6 +32,7 @@
 #define DW_SPI_IDR			0x58
 #define DW_SPI_VERSION			0x5c
 #define DW_SPI_DR			0x60
+#define DW_SPI_CS_OVERRIDE		0xf4
 
 /* Bit fields in CTRLR0 */
 #define SPI_DFS_OFFSET			0
@@ -109,6 +110,7 @@ struct dw_spi {
 	u32			fifo_len;	/* depth of the FIFO buffer */
 	u32			max_freq;	/* max bus freq supported */
 
+	int			cs_override;
 	u32			reg_io_width;	/* DR I/O width in bytes */
 	u16			bus_num;
 	u16			num_cs;		/* supported slave numbers */
-- 
2.7.4

Re: [PATCH 2/2] dw: spi: add support for Alpine spi controller

[Trent Piepho]

On Wed, 2018-10-10 at 18:15 +0300, Talel Shenhar wrote:
> Add support for a new devicetree compatible string called
> 'al,alpine-apb-ssi', which is necessary for the Amazon Alpine spi
> controller. 'al,alpine-dw-apb-ssi' is used in the dw spi driver if
> specified in the devicetree.  Otherwise, fall back to driver default
> behavior, i.e. original dw IP hw driver behavior.
> 
> Signed-off-by: Talel Shenhar <talel@amazon.com>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Shouldn't this be "amzn,alpine-apb-ssi"?  The convention, and this IS
documented, is to the use company's stock ticker symbol as the prefix,
so as to have something grounded in the real world.  I don't know of
anyone else using a product line name as the company name in their dt
bindings.  Example, "snps,dw-apb-ssi", is named for Synopsis, not the
Designware line.

Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Trent Piepho]

On Wed, 2018-10-10 at 10:08 +0300, Talel Shenhar wrote:
> 
> The dw spi controller has an auto-deselect of Chip-Select, in case there is
> no data inside the Tx FIFO. While working on platforms with Alpine chips,
> auto-deselect mode causes an issue for some spi devices that can't handle
> the Chip-Select deselect in the middle of a transaction. It is a normal
> behavior for a Tx FIFO to be empty in the middle of a transaction, due to
> 

So that's the problem!  I, like everyone else I suspect, switched to
using GPIO chip selects with this driver because of this.  I narrowed
it down to a CS de-assert when the bus switched from TX to RX, which of
course makes a SPI register read fail on most devices.  The TX FIFO
would empty at this point, so that would explain it.

Did the designers of this IP ever read a SPI device datasheet???

Got to agree with Mark Brown, why would anyone ever want to NOT have it
work properly?  The previous behavior is not "alternate correct", it's
Broken.

Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Trent Piepho]

On Wed, 2018-10-10 at 13:27 +0100, Mark Brown wrote:
> 
> That's great and we get to reuse all the driver code with a quirk (a
> quirk which fixes the hardware to be more compatible with devices, this
> is a really good hardware change).  Ideally we'd be able to enumerate
> things like IP versions and options from hardware but that's a more
> entertaining problem.

Might well be possible here.  There are an id code and version
registers that could be used.  Did anyone have the foresight to change
them would modifying the IP is the question.


> Having said all this if there are production systems using this
> property, especially production systems where people other than the
> system integrator can realistically deploy their own kernel separate to
> the device tree, then supporting those existing DTs even if they're not
> doing the ideal thing might be the best thing.  You mentioned that this
> might be the case, can you check what the status is there please?

I've developed systems that used chips with this SPI master.  I used
GPIO chip selects to work around the existing behavior and would not be
negatively affected if someone fixed this bug.

There's also another quirk where certain phase/polarity combination
make it generate a CS pulse between each word.

IMHO, Linux SPI has a specification for how it works.  Setup an xfer a
certain way and you get a certain waveform from the master.  If the
master generates the wrong waveform then it's a bug.  Designers should
know better than to design a system that depends on a bug never being
fixed and be caught off guard when it is.  If you want CS to turn off
whenever the FIFO is empty (why???) then extend the spec to encompass
the behavior you want.  Like how "pulse between words" can be specified
now.

Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Talel Shenhar]

On 10/11/2018 01:21 AM, Trent Piepho wrote:
> On Wed, 2018-10-10 at 10:08 +0300, Talel Shenhar wrote:
>> The dw spi controller has an auto-deselect of Chip-Select, in case there is
>> no data inside the Tx FIFO. While working on platforms with Alpine chips,
>> auto-deselect mode causes an issue for some spi devices that can't handle
>> the Chip-Select deselect in the middle of a transaction. It is a normal
>> behavior for a Tx FIFO to be empty in the middle of a transaction, due to
>>
> So that's the problem!  I, like everyone else I suspect, switched to
> using GPIO chip selects with this driver because of this.  I narrowed
> it down to a CS de-assert when the bus switched from TX to RX, which of
> course makes a SPI register read fail on most devices.  The TX FIFO
> would empty at this point, so that would explain it.
>
> Did the designers of this IP ever read a SPI device datasheet???
>
> Got to agree with Mark Brown, why would anyone ever want to NOT have it
> work properly?  The previous behavior is not "alternate correct", it's
> Broken.
This patch allow the Amazon changed hw to work in a correct way.
Unfortunately, the original hw doesn't support auto-deselect disable.
auto-deselect disable is a hw fixup Amazon hw engineers added on top
of the original dw IP.
The fix was to disable the auto-deselect and to allow sw
to manually control the chip-select.
This patch enables the above described Amazon hw fixup and
adds manual control over chip-select.

[PATCH 1/2] dt-bindings: spi: dw: add compatible for Amazon's Alpine spi controller

[Talel Shenhar]

This compatible adds the ability for dw spi controller driver to work with
the dw spi controller found on Alpine chips.

The dw spi controller has an auto-deselect of Chip-Select, in case there is
no data inside the Tx FIFO. While working on platforms with Alpine chips,
auto-deselect mode causes an issue for some spi devices that can't handle
the Chip-Select deselect in the middle of a transaction. It is a normal
behavior for a Tx FIFO to be empty in the middle of a transaction, due to
busy cpu. In the Alpine chip family an option to change the default
behavior was added to the original dw spi controller to prevent this issue
of de-asserting Chip-Select once TX FIFO is empty. The change was to allow
SW manual control of the Chip-Select. With this change, as long as the
Slave Enable Register is asserted, the Chip-Select will be asserted. As a
result, it is necessary to deselect the Slave Select Register once the
transaction is done. This feature is enabled via a new device compatible
string called 'amazon,alpine-dw-apb-ssi'.  Once the driver identifies the
new compatible string, it enables the hw fixup logic, by writing to a
dedicated register found in the IP reserved area and will start manual
deselecting the Slave Select Register when the transfer ends.

Signed-off-by: Talel Shenhar <talel@amazon.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
index 642d3fb..2864bc6 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
@@ -2,7 +2,7 @@ Synopsys DesignWare AMBA 2.0 Synchronous Serial Interface.
 
 Required properties:
 - compatible : "snps,dw-apb-ssi" or "mscc,<soc>-spi", where soc is "ocelot" or
-  "jaguar2"
+  "jaguar2", or "amazon,alpine-dw-apb-ssi"
 - reg : The register base for the controller. For "mscc,<soc>-spi", a second
   register set is required (named ICPU_CFG:SPI_MST)
 - interrupts : One interrupt, used by the controller.
-- 
2.7.4

[PATCH 2/2] dw: spi: add support for Amazon's Alpine spi controller

[Talel Shenhar]

Add support for a new devicetree compatible string called
'amazon,alpine-apb-ssi', which is necessary for the Amazon Alpine spi
controller. 'amazon,alpine-dw-apb-ssi' is used in the dw spi driver if
specified in the devicetree.  Otherwise, fall back to driver default
behavior, i.e. original dw IP hw driver behavior.

Signed-off-by: Talel Shenhar <talel@amazon.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 drivers/spi/spi-dw-mmio.c | 9 +++++++++
 drivers/spi/spi-dw.c      | 6 ++++++
 drivers/spi/spi-dw.h      | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index a768461..3ffb6a40 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -126,6 +126,14 @@ static int dw_spi_mscc_jaguar2_init(struct platform_device *pdev,
 				JAGUAR2_IF_SI_OWNER_OFFSET);
 }
 
+static int dw_spi_alpine_init(struct platform_device *pdev,
+			      struct dw_spi_mmio *dwsmmio)
+{
+	dwsmmio->dws.cs_override = 1;
+
+	return 0;
+}
+
 static int dw_spi_mmio_probe(struct platform_device *pdev)
 {
 	int (*init_func)(struct platform_device *pdev,
@@ -230,6 +238,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "snps,dw-apb-ssi", },
 	{ .compatible = "mscc,ocelot-spi", .data = dw_spi_mscc_ocelot_init},
 	{ .compatible = "mscc,jaguar2-spi", .data = dw_spi_mscc_jaguar2_init},
+	{ .compatible = "amazon,alpine-dw-apb-ssi", .data = dw_spi_alpine_init},
 	{ /* end of table */}
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 3e205ab..b705f2b 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -144,6 +144,8 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
 
 	if (!enable)
 		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
+	else if (dws->cs_override)
+		dw_writel(dws, DW_SPI_SER, 0);
 }
 EXPORT_SYMBOL_GPL(dw_spi_set_cs);
 
@@ -463,6 +465,10 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
 		dws->fifo_len = (fifo == 1) ? 0 : fifo;
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
 	}
+
+	/* enable HW fixup for explicit CS deselect for Amazon's alpine chip */
+	if (dws->cs_override)
+		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
 }
 
 int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 0168b08..c9c1588 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -32,6 +32,7 @@
 #define DW_SPI_IDR			0x58
 #define DW_SPI_VERSION			0x5c
 #define DW_SPI_DR			0x60
+#define DW_SPI_CS_OVERRIDE		0xf4
 
 /* Bit fields in CTRLR0 */
 #define SPI_DFS_OFFSET			0
@@ -109,6 +110,7 @@ struct dw_spi {
 	u32			fifo_len;	/* depth of the FIFO buffer */
 	u32			max_freq;	/* max bus freq supported */
 
+	int			cs_override;
 	u32			reg_io_width;	/* DR I/O width in bytes */
 	u16			bus_num;
 	u16			num_cs;		/* supported slave numbers */
-- 
2.7.4

Re: [PATCH 2/2] dw: spi: add support for Alpine spi controller

[Talel Shenhar]

On 10/11/2018 01:08 AM, Trent Piepho wrote:
> On Wed, 2018-10-10 at 18:15 +0300, Talel Shenhar wrote:
>> Add support for a new devicetree compatible string called
>> 'al,alpine-apb-ssi', which is necessary for the Amazon Alpine spi
>> controller. 'al,alpine-dw-apb-ssi' is used in the dw spi driver if
>> specified in the devicetree.  Otherwise, fall back to driver default
>> behavior, i.e. original dw IP hw driver behavior.
>>
>> Signed-off-by: Talel Shenhar <talel@amazon.com>
>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Shouldn't this be "amzn,alpine-apb-ssi"?  The convention, and this IS
> documented, is to the use company's stock ticker symbol as the prefix,
> so as to have something grounded in the real world.  I don't know of
> anyone else using a product line name as the company name in their dt
> bindings.  Example, "snps,dw-apb-ssi", is named for Synopsis, not the
> Designware line.
>
updated the vendor prefix to "amazon" as described in bindings/vendor-prefixes.txt.

Re: [PATCH 1/2] dt-bindings: spi: dw: add compatible for Amazon's Alpine spi controller

[Mark Brown]

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

On Thu, Oct 11, 2018 at 02:20:06PM +0300, Talel Shenhar wrote:
> This compatible adds the ability for dw spi controller driver to work with
> the dw spi controller found on Alpine chips.

Please don't send new series in reply to old ones, especially if you're
not versioning things, as it makes it much harder to track what's going
on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] dt-bindings: spi: dw: add cs-override property

[Mark Brown]

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

On Wed, Oct 10, 2018 at 10:21:51PM +0000, Trent Piepho wrote:

> So that's the problem!  I, like everyone else I suspect, switched to
> using GPIO chip selects with this driver because of this.  I narrowed

That's generally a good idea.

> it down to a CS de-assert when the bus switched from TX to RX, which of
> course makes a SPI register read fail on most devices.  The TX FIFO
> would empty at this point, so that would explain it.

> Did the designers of this IP ever read a SPI device datasheet???

> Got to agree with Mark Brown, why would anyone ever want to NOT have it
> work properly?  The previous behavior is not "alternate correct", it's
> Broken.

This isn't even that unusual an innovation for hardware to have, for
some reason automatic chip select management is *really* popular and
rarely helpful.  It's far from the most entertaining thing I've seen
hardware do.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Applied "dw: spi: add support for Amazon's Alpine spi controller" to the spi tree

[Mark Brown]

The patch

   dw: spi: add support for Amazon's Alpine spi controller

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From f2d704794864a4bb486f2a0eaed40f25dd87303f Mon Sep 17 00:00:00 2001
From: Talel Shenhar <talel@amazon.com>
Date: Thu, 11 Oct 2018 14:20:07 +0300
Subject: [PATCH] dw: spi: add support for Amazon's Alpine spi controller

Add support for a new devicetree compatible string called
'amazon,alpine-apb-ssi', which is necessary for the Amazon Alpine spi
controller. 'amazon,alpine-dw-apb-ssi' is used in the dw spi driver if
specified in the devicetree.  Otherwise, fall back to driver default
behavior, i.e. original dw IP hw driver behavior.

Signed-off-by: Talel Shenhar <talel@amazon.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-dw-mmio.c | 9 +++++++++
 drivers/spi/spi-dw.c      | 6 ++++++
 drivers/spi/spi-dw.h      | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index a768461614a0..3ffb6a40fe0c 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -126,6 +126,14 @@ static int dw_spi_mscc_jaguar2_init(struct platform_device *pdev,
 				JAGUAR2_IF_SI_OWNER_OFFSET);
 }
 
+static int dw_spi_alpine_init(struct platform_device *pdev,
+			      struct dw_spi_mmio *dwsmmio)
+{
+	dwsmmio->dws.cs_override = 1;
+
+	return 0;
+}
+
 static int dw_spi_mmio_probe(struct platform_device *pdev)
 {
 	int (*init_func)(struct platform_device *pdev,
@@ -230,6 +238,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = {
 	{ .compatible = "snps,dw-apb-ssi", },
 	{ .compatible = "mscc,ocelot-spi", .data = dw_spi_mscc_ocelot_init},
 	{ .compatible = "mscc,jaguar2-spi", .data = dw_spi_mscc_jaguar2_init},
+	{ .compatible = "amazon,alpine-dw-apb-ssi", .data = dw_spi_alpine_init},
 	{ /* end of table */}
 };
 MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match);
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 3e205ab60cd4..b705f2bdb8b9 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -144,6 +144,8 @@ void dw_spi_set_cs(struct spi_device *spi, bool enable)
 
 	if (!enable)
 		dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));
+	else if (dws->cs_override)
+		dw_writel(dws, DW_SPI_SER, 0);
 }
 EXPORT_SYMBOL_GPL(dw_spi_set_cs);
 
@@ -463,6 +465,10 @@ static void spi_hw_init(struct device *dev, struct dw_spi *dws)
 		dws->fifo_len = (fifo == 1) ? 0 : fifo;
 		dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
 	}
+
+	/* enable HW fixup for explicit CS deselect for Amazon's alpine chip */
+	if (dws->cs_override)
+		dw_writel(dws, DW_SPI_CS_OVERRIDE, 0xF);
 }
 
 int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 0168b08364d5..c9c15881e982 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -32,6 +32,7 @@
 #define DW_SPI_IDR			0x58
 #define DW_SPI_VERSION			0x5c
 #define DW_SPI_DR			0x60
+#define DW_SPI_CS_OVERRIDE		0xf4
 
 /* Bit fields in CTRLR0 */
 #define SPI_DFS_OFFSET			0
@@ -109,6 +110,7 @@ struct dw_spi {
 	u32			fifo_len;	/* depth of the FIFO buffer */
 	u32			max_freq;	/* max bus freq supported */
 
+	int			cs_override;
 	u32			reg_io_width;	/* DR I/O width in bytes */
 	u16			bus_num;
 	u16			num_cs;		/* supported slave numbers */
-- 
2.19.0.rc2

Applied "spi: dw: add compatible for Amazon's Alpine spi controller" to the spi tree

[Mark Brown]

The patch

   spi: dw: add compatible for Amazon's Alpine spi controller

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From d49a30366793210cd64759edcdf7099a2e32efd6 Mon Sep 17 00:00:00 2001
From: Talel Shenhar <talel@amazon.com>
Date: Thu, 11 Oct 2018 14:20:06 +0300
Subject: [PATCH] spi: dw: add compatible for Amazon's Alpine spi controller

This compatible adds the ability for dw spi controller driver to work with
the dw spi controller found on Alpine chips.

The dw spi controller has an auto-deselect of Chip-Select, in case there is
no data inside the Tx FIFO. While working on platforms with Alpine chips,
auto-deselect mode causes an issue for some spi devices that can't handle
the Chip-Select deselect in the middle of a transaction. It is a normal
behavior for a Tx FIFO to be empty in the middle of a transaction, due to
busy cpu. In the Alpine chip family an option to change the default
behavior was added to the original dw spi controller to prevent this issue
of de-asserting Chip-Select once TX FIFO is empty. The change was to allow
SW manual control of the Chip-Select. With this change, as long as the
Slave Enable Register is asserted, the Chip-Select will be asserted. As a
result, it is necessary to deselect the Slave Select Register once the
transaction is done. This feature is enabled via a new device compatible
string called 'amazon,alpine-dw-apb-ssi'.  Once the driver identifies the
new compatible string, it enables the hw fixup logic, by writing to a
dedicated register found in the IP reserved area and will start manual
deselecting the Slave Select Register when the transfer ends.

Signed-off-by: Talel Shenhar <talel@amazon.com>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
index 642d3fb1ef85..2864bc6b659c 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.txt
@@ -2,7 +2,7 @@ Synopsys DesignWare AMBA 2.0 Synchronous Serial Interface.
 
 Required properties:
 - compatible : "snps,dw-apb-ssi" or "mscc,<soc>-spi", where soc is "ocelot" or
-  "jaguar2"
+  "jaguar2", or "amazon,alpine-dw-apb-ssi"
 - reg : The register base for the controller. For "mscc,<soc>-spi", a second
   register set is required (named ICPU_CFG:SPI_MST)
 - interrupts : One interrupt, used by the controller.
-- 
2.19.0.rc2