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

Message ID 1539155293-21750-1-git-send-email-talel@amazon.com
State New
Headers show
Series
  • [1/2] dt-bindings: spi: dw: add cs-override property
Related show

Commit Message

Talel Shenhar Oct. 10, 2018, 7:08 a.m.
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(+)

Comments

Mark Brown Oct. 10, 2018, 10:18 a.m. | #1
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?
Talel Shenhar Oct. 10, 2018, 10:34 a.m. | #2
> 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?
Thank you for your prompt response.

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.
Talel Shenhar Oct. 10, 2018, 11:23 a.m. | #3
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.
Mark Brown Oct. 10, 2018, 11:27 a.m. | #4
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.
David Woodhouse Oct. 10, 2018, 11:29 a.m. | #5
On Wed, 2018-10-10 at 13:34 +0300, Talel Shenhar 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?
> 
> Thank you for your prompt response.
> 
> 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.

In particular, that would have been an incompatible change and would
render the hardware non-function with existing drivers. By making it
opt-in, old drivers work.... as long as the FIFO doesn't stall in the
middle of a transaction, which no worse than with the "standard" DW
implementation anyway.
Mark Brown Oct. 10, 2018, 12:27 p.m. | #6
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?
Trent Piepho Oct. 10, 2018, 10:21 p.m. | #7
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.
Trent Piepho Oct. 10, 2018, 10:52 p.m. | #8
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.
Talel Shenhar Oct. 11, 2018, 7:39 a.m. | #9
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.
Mark Brown Oct. 11, 2018, 1:46 p.m. | #10
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.

Patch

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.