spi-bcm2835: merge upstream patches allowing DMA transfers

[msperl]

merge of upstream patches allowing for DMA of spi transfers.

Adds an overlay to enable the DMA mode just on request.

See also: #1036 for a discussion.

[pelwell]

Can you add an entry to arch/arm/boot/dts/overlays/README for spi-dma?

[msperl]

added documentation

[popcornmix]

No objection here.

[notro]

I see that the spi node already have the dma properties, so DMA will be enabled by default:
https://github.com/raspberrypi/linux/blob/rpi-4.0.y/arch/arm/boot/dts/bcm2708_common.dtsi#L128

[pelwell]

Not any more, it doesn't.

[msperl]

this did not go into the current 4.1 kernels provided by the foundation - is this on purpose?

[popcornmix]

Not deliberate. I'll see if I can cherry-pick them easily.

[msperl]

In principle with 4.1 you can take the upstream version directly - only thing left behind is that "strange" fix to make CS work when moving SPI to a different place, but which is better handled by configuring the device-tree correctly to use "cs-gpios" instead of native CS...

[popcornmix]

This PR applies with only a trivial conflict. I'm happy to push that (if compile test passes).
I'm also happy to accept a PR if you think there are better versions of the patches.

[pelwell]

The "strange" patch was the one you upstreamed that disables native CS for everyone, rather than using an overlay to enable gpio-cs for your application.

[msperl]

well - take it as is then
4.2 gives you the final patch with DMA included.
Have you any idea when you may want to drop spi-bcm2708 form your shipped kernels?

Because when you do that I would recommend switching to cs-gpios and then we do not need that out of stream "translation" patch any more...

I guess we could even create an overlay for moving things arround...

[msperl]

The "strange" patch from my part that you are refering to was an upstream requirement, where "device-tree" backwards-compatibility is a must - but only for things that are upstream...

I was talking about that additional strange patch that made that also work with the alternate PIN location for SPI for CM, which I guess would be better handled via an overlay that configures cs-gpios plus the alternate location...

I could look into how we could do this in a sane way - the question is if we still need to accomondate the spi-bcm2708 - say in 4.2?

[popcornmix]

spi-bcm2708 is dropped in #1099
It's not merged yet, and we'll need to convince ourselves that dropping it won't cause a loss of functionality for users. But in principle, if we believe switching to an upstream driver will not cause regressions, then the downstream driver should be dropped.

[msperl]

here the "basic" patch for the device-tree (assuming we do not support spi-bcm2708 any longer):

diff --git a/arch/arm/boot/dts/bcm2709-rpi-2-b.dts b/arch/arm/boot/dts/bcm2709-r
index f25f0a2..d699261 100644
--- a/arch/arm/boot/dts/bcm2709-rpi-2-b.dts
+++ b/arch/arm/boot/dts/bcm2709-rpi-2-b.dts
@@ -9,10 +9,15 @@

 &gpio {
        spi0_pins: spi0_pins {
-               brcm,pins = <7 8 9 10 11>;
+               brcm,pins = <9 10 11>;
                brcm,function = <4>; /* alt0 */
        };

+       spi0_cs_pins: spi0_cs_pins {
+               brcm,pins = <8 7>;
+               brcm,function = <1>; /* output */
+       };
+
        i2c0_pins: i2c0 {
                brcm,pins = <0 1>;
                brcm,function = <4>;
@@ -44,7 +49,8 @@

 &spi0 {
        pinctrl-names = "default";
-       pinctrl-0 = <&spi0_pins>;
+       pinctrl-0 = <&spi0_pins> <&spi0_cs_pins>;
+       cs-gpios = <&gpio 8> <&gpio 7>;

        spidev@0{
                compatible = "spidev";

Needed for all variants that you produce (unless you move these to as dtsi)...
It would NOT work for spi-bcm2708...
Note that spaces instead of tabs due to copy/paste...

Creating an overlay to change the cs-gpios for the CM to a different location would be simple...

[pelwell]

Yes there should be an overlay, but it should be to switch to gpio-cs on the chosen pins. The standard behaviour should be to use native CS, because that's what the DT says, it's what users expect, and because it is sometime necessary (yes, @notro?).

[msperl]

Maybe it would work if the overlay for spi-bcm2708 contains a modifier to set up gpios as they are -

But then spi-bcm2708 still sets the pins as it wants internally in bcm2708_init_pinmode, so that is not an issue...

[pelwell]

But then spi-bcm2708 still sets the pins as it wants internally in bcm2708_init_pinmode, so that is not an issue...

No it doesn't - that code isn't used if DT is enabled.

[msperl]

The thing about native-cs is that there is a bug that just got merged into spi/for-next (ca861dd)

As soon as that is in place that native-cs issue is gone when using reverse polarity clock or data levels.

Here that patch for reference:

From ca861dd0c5e36c4a2cf454049a45a961c855290a Mon Sep 17 00:00:00 2001
From: Martin Sperl <[email protected]>
Date: Tue, 28 Jul 2015 14:03:12 +0000
Subject: [PATCH] spi: bcm2835: set up spi-mode before asserting cs-gpio

When using reverse polarity for clock (spi-cpol) on a device
the clock line gets altered after chip-select has been asserted
resulting in an additional clock beat, which confuses hardware.

This did not show when using native-CS, as the same register
is used to control cs as well as polarity, so the changes came
into effect at the same time. Unfortunately this is not true
with gpio-cs.

To avoid this situation this patch moves the setup of polarity
(spi-cpol and spi-cpha) outside of the chip-select into
prepare_message, which is run prior to asserting chip-select.

Also fixes resetting 3-wire mode after use of rx-mode, so that
a 3-Wire sequence TX, RX, TX works as well (right now it runs
TX, RX, RX instead)

Reported-by: Noralf Tronnes <[email protected]>
Signed-off-by: Martin Sperl <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi-bcm2835.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 59705ab..c9357bb 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -553,13 +553,11 @@ static int bcm2835_spi_transfer_one(struct spi_master *master,
    spi_used_hz = cdiv ? (clk_hz / cdiv) : (clk_hz / 65536);
    bcm2835_wr(bs, BCM2835_SPI_CLK, cdiv);

-   /* handle all the modes */
+   /* handle all the 3-wire mode */
    if ((spi->mode & SPI_3WIRE) && (tfr->rx_buf))
        cs |= BCM2835_SPI_CS_REN;
-   if (spi->mode & SPI_CPOL)
-       cs |= BCM2835_SPI_CS_CPOL;
-   if (spi->mode & SPI_CPHA)
-       cs |= BCM2835_SPI_CS_CPHA;
+   else
+       cs &= ~BCM2835_SPI_CS_REN;

    /* for gpio_cs set dummy CS so that no HW-CS get changed
     * we can not run this in bcm2835_spi_set_cs, as it does
@@ -592,6 +590,25 @@ static int bcm2835_spi_transfer_one(struct spi_master *master,
    return bcm2835_spi_transfer_one_irq(master, spi, tfr, cs);
}

+static int bcm2835_spi_prepare_message(struct spi_master *master,
+                      struct spi_message *msg)
+{
+   struct spi_device *spi = msg->spi;
+   struct bcm2835_spi *bs = spi_master_get_devdata(master);
+   u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
+
+   cs &= ~(BCM2835_SPI_CS_CPOL | BCM2835_SPI_CS_CPHA);
+
+   if (spi->mode & SPI_CPOL)
+       cs |= BCM2835_SPI_CS_CPOL;
+   if (spi->mode & SPI_CPHA)
+       cs |= BCM2835_SPI_CS_CPHA;
+
+   bcm2835_wr(bs, BCM2835_SPI_CS, cs);
+
+   return 0;
+}
+
static void bcm2835_spi_handle_err(struct spi_master *master,
                   struct spi_message *msg)
{
@@ -739,6 +756,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
    master->set_cs = bcm2835_spi_set_cs;
    master->transfer_one = bcm2835_spi_transfer_one;
    master->handle_err = bcm2835_spi_handle_err;
+   master->prepare_message = bcm2835_spi_prepare_message;
    master->dev.of_node = pdev->dev.of_node;

    bs = spi_master_get_devdata(master);
-- 
2.5.0

[msperl]

thinking about it - cs-gpios is not used with the spi-bcm2708 driver as it uses the old transfer interface that builds everything on its own, so it should work transparently.

[pelwell]

Why are we discussing bcm2708-spi? It hasn't been used since rpi-4.0.y.

[msperl]

If it is still there, then you need to support it...

[pelwell]

It's going. Move on.

[msperl]

Here a link to a patch to fix that DT settings that is - as of now - untested (still need to compile the kernel and I can not really test the overlay without a CM):

Please review:
msperl@347ff52

It may also mean we can revert the following patch as it is no longer necessary with cs-gpios set:
6ee3961

[popcornmix]

I've pushed this PR to 4.1 so it should match 4.0. It builds and doesn't break booting for me (but I have no spi device to test).
@msperl would be useful to confirm this works okay for you.

How best to handle the cs/gpio issue can be sorted later.

[msperl]

I can and will - Maybe @notro can also test with his displays...
As soon as the clock thingy is fixed and went into the tree of Linus I will create a merge request

[msperl]

I have created a patch to modify the DT to use gpio-cs.
I have also added an overlay that allows us to move spi0 to gpio 35-39.

#1107

The patch for the issue with regards to polarity is still not merged by Linus, but it went into several trees of the spi branch...

[pelwell]

I disagree with the reasoning behind the patch. Many people have been using SPI with native chip selects without issue. You found a problem with it, then pushed a patch upstream to workaround it for everybody (except it assumed it was using BANK0 GPIOs) when an overlay to provide a workaround for those affected would have been a cleaner solution.

An overlay to switch to GPIO cs seems like a better approach, and we can revert the driver hack.

[msperl]

The point is: spi-bcm2835 does not work with native-cs any longer - the reason behind it is that whenever you are touching the SPI-CS-Register there is a slight chance that the chip select will change for a very short period of time. And all those speed optimizations of the spi driver will write to this register while there is a transfer already happening.

E.g: the "original driver" just enabled interrupts in the CS register and then we had to wait 14us for the interrupt to get really handled by our code. Now we first prefill the FIFO as long as possible (without interrupts enabled) and only when it is full we enable interrupts so that when we reach the HW-configured limits the interrupt will trigger - this means that we need one interrupt less to finish and we start the transfer faster - especially for short transfers, where with the above approach we are polling for short periods of time (<30us) and if we then exceed the timeout (say because of rescheduling of the kernel thread), then we move to interrupt driven to finish the process - and this again requires a modification to the CS register...

As upstream needs backwards compatiblity for existing drivers that the kernel at that time supports, we had to either "fall back to interrupt only" for those "native" cases or to make the change in code.
The patch I had sent as a merge request just makes this "workaround" for backwards compatibility not become active. And that is what we should be going for.

On top with the gpio-cs approach you are NOT limited to 2 Chip-selects any longer - I am running 5 SPI devices on my test setup right now with arbitrary GPIOs as chip selects, which is much of an advantage.

The above device-tree patch also has the advantage of separating out the CS from the SPI pins, so that creating overlays to add additional chip-selects becomes easier.

[HiassofT]

In kernel 4.1 SPI DMA is enabled by default, eating up precious dma channels.
I think 4d317a8 should go into the 4.1 and 4.2 trees as well
ping @popcornmix @pelwell

[msperl]

Dma channels are only used/assigned when the spi driver is loaded.
The corresponding ids 6 and 7 are only describing the DREQs that will trigger further transfers from/to the spi-hw block and they are not related to dma channels that are actually used.

[HiassofT]

The problem I'm facing here is that only 3 of the 5 dma channels are available by default (according to /sys/class/dma/dma0chan?/in_use), and both bcm2708-i2s and spi-bcm2835 each seem to want 2 DMA channels.

In my Cirrus audio card build (which uses SPI for low-bandwidth audio device configuration) this results in a non-working driver since bcm2708-i2s can't allocate the second DMA channel. Either disabling SPI DMA by uncommenting the dma lines in bcm2708_common.dtsi or using spi-bcm2708 fixes this, leaving 1 free dma channel after the I2S module is loaded.

For these cases, where SPI DMA isn't really needed, it would be better to have an option to disable it.

BTW: I tried disabling the legacy DMA API, this results in a total of 7 dma channels but unfortunately also a nonworking framebuffer.

[msperl]

I have now tried the following:

• enable spi and i2s on the DT and rebooted
  • this automatically loaded snd_soc_bcm2708_i2s for me
  • this still shows only 5 DMA channels (3 of which are in use):

==> /sys/class/dma/dma0chan0/in_use <==
1

==> /sys/class/dma/dma0chan1/in_use <==
1

==> /sys/class/dma/dma0chan2/in_use <==
1

==> /sys/class/dma/dma0chan3/in_use <==
0

==> /sys/class/dma/dma0chan4/in_use <==
0

• loaded spi-bcm2835 (which I have put on the blacklist)
  • this crashes the kernel (or at least USB does not work) - but there are NO messages on the serial console!

One thing that is surprizing is that we only have 5 dma channels - even if the mask for DMAs shows that we have more than 5 DMAs that are useable as per parameter:

[    0.728284] bcm2708-dmaengine 3f007000.dma: DMA legacy API manager at f3007000, dmachans=0x7f35
[    0.737685] bcm2708-dmaengine 3f007000.dma: Load BCM2835 DMA engine driver
[    0.744603] bcm2708-dmaengine 3f007000.dma: dma_debug:0

The dmachans=0x7f35 says which ones are useable for the kernel: (the ones with no bit set)
so in this case we got bit 1,3,6,7 and 15 free - so in total 5!
I you remove all the modules then only 2 dma channels are "in_use"
So I start to wonder which devices are making use of DMA when it is NOT defined in the device tree.
Also I wonder why we are only left with 5 dmas for the linux kernel and why the firmware is consuming 11 of the DMA channels...
So this is actually a different issue from that!

[pelwell]

You have the sense of the dmachans parameter inverted - a bit set is a free channel, which makes more sense.

I'm now investigating the channel usage.

[msperl]

OK - I miss-read that - but we still have only 5 channels open (as per /sys/class/dma) and when we claim all then the system falls over without diagnostics...

Maybe there is an off by one bug that does not register 6 channels but only 5?
Note that the upstream kernel DOES show 6 dma channels in /sys/class/dma not 5.

So maybe these are the dma-channels that are configured (and not any other) and for some reason the foundation DMA implementation has a bug?

[pelwell]

See #1113.