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

[Stephen Boyd]

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@xxxxxxxxxxxx>
> ---

Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>

> 
> 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",