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

From: Stephen Boyd
Date: Wed Jun 24 2020 - 21:25:52 EST

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.

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?

>
> 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.

>
> 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/?

>
> 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.

>
> 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.