| Message ID | 1539155293-21750-1-git-send-email-talel@amazon.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-bindings: spi: dw: add cs-override property | expand |
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?
> 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.
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.
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.
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.
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?
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.
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.
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.
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.
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.