Re: [PATCH 1/2] arm64: dts: qcom: sc7180: Switch SPI to use GPIO for CS

From: Doug Anderson
Date: Wed Jun 24 2020 - 23:36:28 EST

Hi,

On Wed, Jun 24, 2020 at 6:25 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Douglas Anderson (2020-06-24 17:08:04)
> > The geni SPI protocol appears to have been designed without taking
> > Linux needs into account. In all the normal flows it takes care of
> > setting chip select itself. However, Linux likes to manage the chip
> > select so it can do fancy things.
> >
> > Back when we first landed the geni SPI driver we worked around this
> > by:
> > - Always setting the FRAGMENTATION bit in transfers, which (I believe)
> > tells the SPI controller not to mess with the chip select during
> > transfers.
> > - Controlling the chip select manually by sending the SPI controller
> > commands to assert or deassert the chip select.
> >
> > Our workaround was fine and it's been working OK, but it's really
> > quite inefficient. A normal SPI transaction now needs to do:
> > 1. Start a command to set the chip select.
> > 2. Wait for an interrupt from controller saying chip select done.
> > 3. Do a transfer.
> > 4. Start a command to unset the chip select.
> > 5. Wait for an interrupt from controller saying chip select done.
>
> I thought the GENI hardware was supposed to be able to queue commands up
> and then plow through them to interrupt the CPU when it finished. If
> that was supported would there be any problems? I assume we could remove
> the wait for CS disable and interrupt on step 5 and also the wait for
> CS/interrupt on step 2 because it is bundled with the transfer in step
> 3.

Do you have any details or pointers to documentation on said ability
to queue commands? I don't think I've ever heard of it.

How exactly would it work, anyway? There's no sequence number or
anything in GENI and there's a single "DONE" interrupt that signals
both "CS Done" and "Transfer Done", so without some really good docs
I'd have a really hard time figuring out how this is supposed to work.


> Where is the delay? On step 2 where we wait to transfer or at step 5
> when we wait for CS to be dropped, or both?

Presumably every CS change takes the same amount of time? ...so I'd
say "both". If it really matters I can try to measure.


> > Things are made a bit worse because the Linux GPIO framework assumes
> > that setting a chip select is quick. Thus the SPI core can be seen to
> > tell us to set our chip select even when it's already in the right
> > state and we were dutifully kicking off commands and waiting for
> > interrupts. While we could optimize that particular case, we'd still
> > be left with the slowness when we actually needed to toggle the chip
> > select.
>
> One thing to note is that the GPIO driver doesn't tell us that it has
> actually asserted/deasserted the GPIO. It writes to the controller and
> moves on so we don't know when it has actually gone into effect.
> Hopefully moving to GPIO mode doesn't mean we get weird problems where
> CS isn't asserted yet and a transfer starts wiggling the lines.

cs-gpios is a pretty normal Linux concept and not something I
invented. It's documented to work just fine and I can't see this as
being a real problem.


> > All the chip select lines can actually be muxed to be GPIOs and
> > there's really no downside in doing so. Now Linux can assert and
> > deassert the chip select at will with a simple MMIO write.
> >
> > The SPI driver will still have the ability to set the chip select, but
> > not we just won't use it.
>
> s/not/now/?

Sigh. I always make that typo. I can spin if need be to fix.


> > With this change I tested reading the firmware off the EC connected to
> > a ChromeOS device (flashrom -p ec -r ...). I saw about a 25% speedup
> > in total runtime of the command and a 30% reduction in interrupts
> > generated (measured by /proc/interrupts).
>
> I see nothing wrong with specifying the CS gpios in DT. Seems like that
> should always be there and then the driver should decide to use GPIO
> mode or not. So
>
> Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
>
> for that part.

I appreciate the tag, but I'm not sure it's working the way you're
thinking it does? See:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml

>From there, when you specify spi-gpios it's saying: definitely use
this GPIO as the chip select, don't use your native one. The bindings
explicitly show how you would specify the native GPIO.

If we wanted the SPI controller to decide one way or the other on its
own, I guess we'd need an entirely new property saying that if it
wanted to control its chip select with GPIO then here's the GPIO and
then we'd need to provide a pinmux config for that. That feels
overkill to me since I really see no reason not to use it as a GPIO.

It's kinda like saying: if an SoC provided two different ways to set a
pin, one of which always delayed 50 us to assert and one that asserted
instantly, do we really need to write drivers to support both modes?
No. The mode which delays 50 us is useless and there was no good
reason for the SoC to invent that mode and no reason to try to support
it in software.


> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > ---
> >
> > arch/arm64/boot/dts/qcom/sc7180.dtsi | 57 ++++++++++++++++++++++++----
> > 1 file changed, 49 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > index 3a8076c8bdbf..74c8503b560e 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > @@ -1204,65 +1213,97 @@ pinmux {
> > qup_spi0_default: qup-spi0-default {
> > pinmux {
> > pins = "gpio34", "gpio35",
> > - "gpio36", "gpio37";
> > + "gpio36";
> > function = "qup00";
> > };
> > + pinmux-cs {
> > + pins = "gpio37";
> > + function = "gpio";
> > + };
> > };
> >
> > qup_spi1_default: qup-spi1-default {
> > pinmux {
> > pins = "gpio0", "gpio1",
> > - "gpio2", "gpio3";
> > + "gpio2";
> > function = "qup01";
> > };
> > + pinmux-cs {
> > + pins = "gpio3";
> > + function = "gpio";
> > + };
> > };
> >
> > qup_spi3_default: qup-spi3-default {
> > pinmux {
> > pins = "gpio38", "gpio39",
> > - "gpio40", "gpio41";
> > + "gpio40";
> > function = "qup03";
> > };
> > + pinmux-cs {
> > + pins = "gpio41";
> > + function = "gpio";
> > + };
> > };
> >
> > qup_spi5_default: qup-spi5-default {
> > pinmux {
> > pins = "gpio25", "gpio26",
> > - "gpio27", "gpio28";
> > + "gpio27";
> > function = "qup05";
> > };
> > + pinmux-cs {
> > + pins = "gpio28";
> > + function = "gpio";
> > + };
> > };
> >
> > qup_spi6_default: qup-spi6-default {
> > pinmux {
> > pins = "gpio59", "gpio60",
> > - "gpio61", "gpio62";
> > + "gpio61";
> > function = "qup10";
> > };
> > + pinmux-cs {
> > + pins = "gpio62";
> > + function = "gpio";
> > + };
> > };
> >
> > qup_spi8_default: qup-spi8-default {
> > pinmux {
> > pins = "gpio42", "gpio43",
> > - "gpio44", "gpio45";
> > + "gpio44";
> > function = "qup12";
> > };
> > + pinmux-cs {
> > + pins = "gpio45";
> > + function = "gpio";
> > + };
> > };
> >
> > qup_spi10_default: qup-spi10-default {
> > pinmux {
> > pins = "gpio86", "gpio87",
> > - "gpio88", "gpio89";
> > + "gpio88";
> > function = "qup14";
> > };
> > + pinmux-cs {
> > + pins = "gpio89";
> > + function = "gpio";
> > + };
> > };
> >
> > qup_spi11_default: qup-spi11-default {
> > pinmux {
> > pins = "gpio53", "gpio54",
> > - "gpio55", "gpio56";
> > + "gpio55";
> > function = "qup15";
> > };
> > + pinmux-cs {
> > + pins = "gpio56";
> > + function = "gpio";
> > + };
> > };
>
> Perhaps we should have qup-spiN-default and qup-spiN-cs-gpio though?
> That way the driver can properly mux the pin to be gpio mode if it wants
> to. I don't see a way to mux the pin to be "qup" until the driver
> decides it doesn't want that.

This seems like extra complexity. Why would the SPI controller ever
need to control the GPIO itself. If we ever do come up with a reason
we can solve it then?

-Doug