Re: [PATCH] spi: bcm2835: Convert to use CS GPIO descriptors

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Sun, Apr 21, 2019 at 7:17 PM Martin Sperl <kernel@xxxxxxxxxxxxxxxx> wrote:
> > On 21.04.2019, at 14:27, Stefan Wahren <stefan.wahren@xxxxxxxx> wrote:
> >
> >> Am 20.04.19 um 17:46 schrieb Martin Sperl:
> >>
> >>> On 20.04.2019, at 13:08, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> >>>
> >>> This converts the BCM2835 SPI master driver to use GPIO
> >>> descriptors for chip select handling.
> >>>
> >>> The BCM2835 driver was relying on the core to drive the
> >>> CS high/low so very small changes were needed for this
> >>> part. If it managed to request the CS from the device tree
> >>> node, all is pretty straight forward.
> >>>
> >>> However for native GPIOs this driver has a quite unorthodox
> >>> loopback to request some GPIOs from the SoC GPIO chip by
> >>> looking it up from the device tree using gpiochip_find()
> >>> and then offseting hard into its numberspace. This has
> >>> been augmented a bit by using gpiochip_request_own_desc()
> >>> but this code really needs to be verified. If "native CS"
> >>> is actually an SoC GPIO, why is it even done this way?
> >>> Should this GPIO not just be defined in the device tree
> >>> like any other CS GPIO? I'm confused.
> >> It has been done to keep dt-backward compatibility.
> >>
> >> When the driver was written dt had configured native-cs.
> >
> > This is done in a very opaque way. The bcm2835-rpi.dtsi defines a pinmux
> > group alt0 which init SPI pins (including native CS). So we better use
> > spi0_gpio7 (including native CS) or even better create new groups
> > without native CS and use them.
>
> I remember that there was a patch of mine to achieve this but it was
> turned down because it would not describe the the hw and the altx
> Pin up still needed to get at least documented that way. And that
> when enabling the spi Block the corresponding  gpio Block would
> need to get configured properly.
>
> At that point I left the discussion as the effect of using gpio mode
> was implemented by the driver. I can search for the patch
>  from a few years ago...
>
> > Even worse the alt0 group is applied for all the RPi Zero, 1 and 2
> > boards, but not for the RPi 3 ones.
> >
> >> A native-cs implementation comes with lots of
> >> limitations and some of which - especially dma mode
> >> has some quirks with regards to native-cs (glitches,
> >> deasserts after each finished dma transfer,...)
> >>
> >> The only way to make this work properly was to use
> >> Gpio.
> >
> > So we better extend the current binding with cs-gpios as optional
> > property in order to point the user the right direction. And gave the
> > same warnings like in bcm2835-spi-aux.
>
> The difference is that aux has different issues and this driver
> Is not working properly without gpio cs giving unexpected results
>  and resulting in regressions.
>
>  Keeping gpio cs being mandatory is the only way to avoid
> regressions everything else is just relying on the legacy code
> and thus avoiding the discussion.

While all this is great and I am sorry if things are not what they
should ideally be, the patch as it stands actually doesn't change
the hack, combined with patch 1 it tries to preserve it and put
us in a better position for more cleanups later.

It would be great if someone could test patches 1+2 on
hardware and see if it works as intended!

Yours,
Linus Walleij