diff mbox series

[v2,1/2] arm64: dts: qcom: sc7180: Provide pinconf for SPI to use GPIO for CS

Message ID 20200918174511.v2.1.I997a428f58ef9d48b37a27a028360f34e66c00ec@changeid (mailing list archive)
State Superseded
Headers show
Series [v2,1/2] arm64: dts: qcom: sc7180: Provide pinconf for SPI to use GPIO for CS | expand

Commit Message

Doug Anderson Sept. 19, 2020, 12:45 a.m. UTC
When the chip select line is controlled by the QUP, changing CS is a
time consuming operation.  We have to send a command over to the geni
and wait for it to Ack us every time we want to change (both making it
high and low).  To send this command we have to make a choice in
software when we want to control the chip select, we have to either:
A) Wait for the Ack via interrupt which slows down all SPI transfers
   (and incurrs extra processing associated with interrupts).
B) Sit in a loop and poll, waiting for the Ack.

Neither A) nor B) is a great option.

We can avoid all of this by realizing that, at least on some boards,
there is no advantage of considering this line to be a geni line.
While it's true that geni _can_ control the line, it's also true that
the line can be a GPIO and there is no downside of viewing it that
way.  Setting a GPIO is a simple MMIO operation.

This patch provides definitions so a board can easily select the GPIO
mode.

NOTE: apparently, it's possible to run the geni in "GSI" mode.  In GSI
the SPI port is allowed to be controlled by more than one user (like
firmware and Linux) and also the port can operate sequences of
operations in one go.  In GSI mode it _would_ be invalid to look at
the chip select as a GPIO because that would prevent other users from
using it.  In theory GSI mode would also avoid some overhead by
allowing us to sequence the chip select better.  However, I'll argue
GSI is not relevant for all boards (and certainly not any boards
supported by mainline today).  Why?
- Apparently to run a SPI chip in GSI mode you need to initialize it
  (in the bootloader) with a different firmware and then it will
  always run in GSI mode.  Since there is no support for GSI mode in
  the current Linux driver, it must be that existing boards don't have
  firmware that's doing that.  Note that the kernel device tree
  describes hardware but also firmware, so it is legitimate to make
  the assumption that we don't have GSI firmware in a given dts file.
- Some boards with sc7180 have SPI connected to the Chrome OS EC or
  security chip (Cr50).  The protocols for talking to cros_ec and cr50
  are extremely complex.  Both drivers in Linux fully lock the bus
  across several distinct SPI transfers.  While I am not an expert on
  GSI mode it feels highly unlikely to me that we'd ever be able to
  enable GSI mode for these devices.

From a testing perspective, running "flashrom -p ec -r /tmp/foo.bin"
in a loop after this patch shows almost no reduction in time, but the
number of interrupts per command goes from 32357 down to 30611 (about
a 5% reduction).

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Now just add the pinctrl; let a board use it.

 arch/arm64/boot/dts/qcom/sc7180.dtsi | 96 ++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

Comments

Stephen Boyd Sept. 21, 2020, 8:06 p.m. UTC | #1
Quoting Douglas Anderson (2020-09-18 17:45:27)
> When the chip select line is controlled by the QUP, changing CS is a
> time consuming operation.  We have to send a command over to the geni
> and wait for it to Ack us every time we want to change (both making it
> high and low).  To send this command we have to make a choice in
> software when we want to control the chip select, we have to either:
> A) Wait for the Ack via interrupt which slows down all SPI transfers
>    (and incurrs extra processing associated with interrupts).
> B) Sit in a loop and poll, waiting for the Ack.
> 
> Neither A) nor B) is a great option.
> 
> We can avoid all of this by realizing that, at least on some boards,
> there is no advantage of considering this line to be a geni line.
> While it's true that geni _can_ control the line, it's also true that
> the line can be a GPIO and there is no downside of viewing it that
> way.  Setting a GPIO is a simple MMIO operation.
> 
> This patch provides definitions so a board can easily select the GPIO
> mode.
> 
> NOTE: apparently, it's possible to run the geni in "GSI" mode.  In GSI
> the SPI port is allowed to be controlled by more than one user (like
> firmware and Linux) and also the port can operate sequences of
> operations in one go.  In GSI mode it _would_ be invalid to look at
> the chip select as a GPIO because that would prevent other users from
> using it.  In theory GSI mode would also avoid some overhead by
> allowing us to sequence the chip select better.  However, I'll argue
> GSI is not relevant for all boards (and certainly not any boards
> supported by mainline today).  Why?
> - Apparently to run a SPI chip in GSI mode you need to initialize it
>   (in the bootloader) with a different firmware and then it will
>   always run in GSI mode.  Since there is no support for GSI mode in
>   the current Linux driver, it must be that existing boards don't have
>   firmware that's doing that.  Note that the kernel device tree
>   describes hardware but also firmware, so it is legitimate to make
>   the assumption that we don't have GSI firmware in a given dts file.
> - Some boards with sc7180 have SPI connected to the Chrome OS EC or
>   security chip (Cr50).  The protocols for talking to cros_ec and cr50
>   are extremely complex.  Both drivers in Linux fully lock the bus
>   across several distinct SPI transfers.  While I am not an expert on
>   GSI mode it feels highly unlikely to me that we'd ever be able to
>   enable GSI mode for these devices.
> 
> From a testing perspective, running "flashrom -p ec -r /tmp/foo.bin"
> in a loop after this patch shows almost no reduction in time, but the
> number of interrupts per command goes from 32357 down to 30611 (about
> a 5% reduction).
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

> 
> Changes in v2:
> - Now just add the pinctrl; let a board use it.
> 
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 96 ++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 6678f1e8e395..0534122b9a3c 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -1595,6 +1595,18 @@ pinmux {
>                                 };
>                         };
>  
> +                       qup_spi0_cs_gpio: qup-spi0-cs-gpio {
> +                               pinmux {
> +                                       pins = "gpio34", "gpio35",
> +                                              "gpio36";
> +                                       function = "qup00";
> +                               };
> +                               pinmux-cs {

Style nit: Add a newline between nodes?

> +                                       pins = "gpio37";
> +                                       function = "gpio";
> +                               };
> +                       };
> +
>                         qup_spi1_default: qup-spi1-default {
>                                 pinmux {
>                                         pins = "gpio0", "gpio1",
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 6678f1e8e395..0534122b9a3c 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -1595,6 +1595,18 @@  pinmux {
 				};
 			};
 
+			qup_spi0_cs_gpio: qup-spi0-cs-gpio {
+				pinmux {
+					pins = "gpio34", "gpio35",
+					       "gpio36";
+					function = "qup00";
+				};
+				pinmux-cs {
+					pins = "gpio37";
+					function = "gpio";
+				};
+			};
+
 			qup_spi1_default: qup-spi1-default {
 				pinmux {
 					pins = "gpio0", "gpio1",
@@ -1603,6 +1615,18 @@  pinmux {
 				};
 			};
 
+			qup_spi1_cs_gpio: qup-spi1-cs-gpio {
+				pinmux {
+					pins = "gpio0", "gpio1",
+					       "gpio2";
+					function = "qup01";
+				};
+				pinmux-cs {
+					pins = "gpio3";
+					function = "gpio";
+				};
+			};
+
 			qup_spi3_default: qup-spi3-default {
 				pinmux {
 					pins = "gpio38", "gpio39",
@@ -1611,6 +1635,18 @@  pinmux {
 				};
 			};
 
+			qup_spi3_cs_gpio: qup-spi3-cs-gpio {
+				pinmux {
+					pins = "gpio38", "gpio39",
+					       "gpio40";
+					function = "qup03";
+				};
+				pinmux-cs {
+					pins = "gpio41";
+					function = "gpio";
+				};
+			};
+
 			qup_spi5_default: qup-spi5-default {
 				pinmux {
 					pins = "gpio25", "gpio26",
@@ -1619,6 +1655,18 @@  pinmux {
 				};
 			};
 
+			qup_spi5_cs_gpio: qup-spi5-cs-gpio {
+				pinmux {
+					pins = "gpio25", "gpio26",
+					       "gpio27";
+					function = "qup05";
+				};
+				pinmux-cs {
+					pins = "gpio28";
+					function = "gpio";
+				};
+			};
+
 			qup_spi6_default: qup-spi6-default {
 				pinmux {
 					pins = "gpio59", "gpio60",
@@ -1627,6 +1675,18 @@  pinmux {
 				};
 			};
 
+			qup_spi6_cs_gpio: qup-spi6-cs-gpio {
+				pinmux {
+					pins = "gpio59", "gpio60",
+					       "gpio61";
+					function = "qup10";
+				};
+				pinmux-cs {
+					pins = "gpio62";
+					function = "gpio";
+				};
+			};
+
 			qup_spi8_default: qup-spi8-default {
 				pinmux {
 					pins = "gpio42", "gpio43",
@@ -1635,6 +1695,18 @@  pinmux {
 				};
 			};
 
+			qup_spi8_cs_gpio: qup-spi8-cs-gpio {
+				pinmux {
+					pins = "gpio42", "gpio43",
+					       "gpio44";
+					function = "qup12";
+				};
+				pinmux-cs {
+					pins = "gpio45";
+					function = "gpio";
+				};
+			};
+
 			qup_spi10_default: qup-spi10-default {
 				pinmux {
 					pins = "gpio86", "gpio87",
@@ -1643,6 +1715,18 @@  pinmux {
 				};
 			};
 
+			qup_spi10_cs_gpio: qup-spi10-cs-gpio {
+				pinmux {
+					pins = "gpio86", "gpio87",
+					       "gpio88";
+					function = "qup14";
+				};
+				pinmux-cs {
+					pins = "gpio89";
+					function = "gpio";
+				};
+			};
+
 			qup_spi11_default: qup-spi11-default {
 				pinmux {
 					pins = "gpio53", "gpio54",
@@ -1651,6 +1735,18 @@  pinmux {
 				};
 			};
 
+			qup_spi11_cs_gpio: qup-spi11-cs-gpio {
+				pinmux {
+					pins = "gpio53", "gpio54",
+					       "gpio55";
+					function = "qup15";
+				};
+				pinmux-cs {
+					pins = "gpio56";
+					function = "gpio";
+				};
+			};
+
 			qup_uart0_default: qup-uart0-default {
 				pinmux {
 					pins = "gpio34", "gpio35",