Re: [PATCH 1/2] arm64: dts: qcom: sc7180: Switch SPI to use GPIO for CS
From: Stephen Boyd <hidden>
Date: 2020-06-25 01:25:14
Also in:
linux-arm-msm, lkml
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 <redacted> for that part.
quoted hunk ↗ jump to hunk
Signed-off-by: Douglas Anderson <dianders@chromium.org> --- 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.