[PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Dmitry Baryshkov]

GENI SPI controller shows several issues if it manages the CS on its own
(see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to
use GPIO for CS")) for the details. Provide pinctrl entries for SPI
controllers using the same CS pin but in GPIO mode.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 380 +++++++++++++++++++++++++++
 1 file changed, 380 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 3cea28058a91..03015174ec06 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -3046,6 +3046,25 @@ config {
 				};
 			};
 
+			qup_spi0_cs_gpio: qup-spi0-cs-gpio {
+				mux {
+					pins = "gpio28", "gpio29",
+					       "gpio30";
+					function = "qup0";
+				};
+
+				cs {
+					pins = "gpio31";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio28", "gpio29", "gpio30", "gpio31";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi0_default: qup-spi0-default {
 				mux {
 					pins = "gpio28", "gpio29",
@@ -3061,6 +3080,25 @@ config {
 				};
 			};
 
+			qup_spi1_cs_gpio: qup-spi1-cs-gpio {
+				mux {
+					pins = "gpio4", "gpio5",
+					       "gpio6";
+					function = "qup1";
+				};
+
+				cs {
+					pins = "gpio7";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio4", "gpio5", "gpio6", "gpio7";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi1_default: qup-spi1-default {
 				mux {
 					pins = "gpio4", "gpio5",
@@ -3076,6 +3114,25 @@ config {
 				};
 			};
 
+			qup_spi2_cs_gpio: qup-spi2-cs-gpio {
+				mux {
+					pins = "gpio115", "gpio116",
+					       "gpio117";
+					function = "qup2";
+				};
+
+				cs {
+					pins = "gpio118";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio115", "gpio116", "gpio117", "gpio118";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi2_default: qup-spi2-default {
 				mux {
 					pins = "gpio115", "gpio116",
@@ -3091,6 +3148,25 @@ config {
 				};
 			};
 
+			qup_spi3_cs_gpio: qup-spi3-cs-gpio {
+				mux {
+					pins = "gpio119", "gpio120",
+					       "gpio121";
+					function = "qup3";
+				};
+
+				cs {
+					pins = "gpio122";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio119", "gpio120", "gpio121", "gpio122";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi3_default: qup-spi3-default {
 				mux {
 					pins = "gpio119", "gpio120",
@@ -3106,6 +3182,25 @@ config {
 				};
 			};
 
+			qup_spi4_cs_gpio: qup-spi4-cs-gpio {
+				mux {
+					pins = "gpio8", "gpio9",
+					       "gpio10";
+					function = "qup4";
+				};
+
+				cs {
+					pins = "gpio11";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio8", "gpio9", "gpio10", "gpio11";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi4_default: qup-spi4-default {
 				mux {
 					pins = "gpio8", "gpio9",
@@ -3121,6 +3216,25 @@ config {
 				};
 			};
 
+			qup_spi5_cs_gpio: qup-spi5-cs-gpio {
+				mux {
+					pins = "gpio12", "gpio13",
+					       "gpio14";
+					function = "qup5";
+				};
+
+				cs {
+					pins = "gpio15";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio12", "gpio13", "gpio14", "gpio15";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi5_default: qup-spi5-default {
 				mux {
 					pins = "gpio12", "gpio13",
@@ -3136,6 +3250,25 @@ config {
 				};
 			};
 
+			qup_spi6_cs_gpio: qup-spi6-cs-gpio {
+				mux {
+					pins = "gpio16", "gpio17",
+					       "gpio18";
+					function = "qup6";
+				};
+
+				cs {
+					pins = "gpio19";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio16", "gpio17", "gpio18", "gpio19";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi6_default: qup-spi6-default {
 				mux {
 					pins = "gpio16", "gpio17",
@@ -3151,6 +3284,25 @@ config {
 				};
 			};
 
+			qup_spi7_cs_gpio: qup-spi7-cs-gpio {
+				mux {
+					pins = "gpio20", "gpio21",
+					       "gpio22";
+					function = "qup7";
+				};
+
+				cs {
+					pins = "gpio23";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio20", "gpio21", "gpio22", "gpio23";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi7_default: qup-spi7-default {
 				mux {
 					pins = "gpio20", "gpio21",
@@ -3166,6 +3318,25 @@ config {
 				};
 			};
 
+			qup_spi8_cs_gpio: qup-spi8-cs-gpio {
+				mux {
+					pins = "gpio24", "gpio25",
+					       "gpio26";
+					function = "qup8";
+				};
+
+				cs {
+					pins = "gpio27";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio24", "gpio25", "gpio26", "gpio27";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi8_default: qup-spi8-default {
 				mux {
 					pins = "gpio24", "gpio25",
@@ -3181,6 +3352,25 @@ config {
 				};
 			};
 
+			qup_spi9_cs_gpio: qup-spi9-cs-gpio {
+				mux {
+					pins = "gpio125", "gpio126",
+					       "gpio127";
+					function = "qup9";
+				};
+
+				cs {
+					pins = "gpio128";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio125", "gpio126", "gpio127", "gpio128";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi9_default: qup-spi9-default {
 				mux {
 					pins = "gpio125", "gpio126",
@@ -3196,6 +3386,25 @@ config {
 				};
 			};
 
+			qup_spi10_cs_gpio: qup-spi10-cs-gpio {
+				mux {
+					pins = "gpio129", "gpio130",
+					       "gpio131";
+					function = "qup10";
+				};
+
+				cs {
+					pins = "gpio132";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio129", "gpio130", "gpio131", "gpio132";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi10_default: qup-spi10-default {
 				mux {
 					pins = "gpio129", "gpio130",
@@ -3211,6 +3420,25 @@ config {
 				};
 			};
 
+			qup_spi11_cs_gpio: qup-spi11-cs-gpio {
+				mux {
+					pins = "gpio60", "gpio61",
+					       "gpio62";
+					function = "qup11";
+				};
+
+				cs {
+					pins = "gpio63";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio60", "gpio61", "gpio62", "gpio63";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi11_default: qup-spi11-default {
 				mux {
 					pins = "gpio60", "gpio61",
@@ -3226,6 +3454,25 @@ config {
 				};
 			};
 
+			qup_spi12_cs_gpio: qup-spi12-cs-gpio {
+				mux {
+					pins = "gpio32", "gpio33",
+					       "gpio34";
+					function = "qup12";
+				};
+
+				cs {
+					pins = "gpio35";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio32", "gpio33", "gpio34", "gpio35";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi12_default: qup-spi12-default {
 				mux {
 					pins = "gpio32", "gpio33",
@@ -3241,6 +3488,25 @@ config {
 				};
 			};
 
+			qup_spi13_cs_gpio: qup-spi13-cs-gpio {
+				mux {
+					pins = "gpio36", "gpio37",
+					       "gpio38";
+					function = "qup13";
+				};
+
+				cs {
+					pins = "gpio39";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio36", "gpio37", "gpio38", "gpio39";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi13_default: qup-spi13-default {
 				mux {
 					pins = "gpio36", "gpio37",
@@ -3256,6 +3522,25 @@ config {
 				};
 			};
 
+			qup_spi14_cs_gpio: qup-spi14-cs-gpio {
+				mux {
+					pins = "gpio40", "gpio41",
+					       "gpio42";
+					function = "qup14";
+				};
+
+				cs {
+					pins = "gpio43";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio40", "gpio41", "gpio42", "gpio43";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi14_default: qup-spi14-default {
 				mux {
 					pins = "gpio40", "gpio41",
@@ -3271,6 +3556,25 @@ config {
 				};
 			};
 
+			qup_spi15_cs_gpio: qup-spi15-cs-gpio {
+				mux {
+					pins = "gpio44", "gpio45",
+					       "gpio46";
+					function = "qup15";
+				};
+
+				cs {
+					pins = "gpio47";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio44", "gpio45", "gpio46", "gpio47";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi15_default: qup-spi15-default {
 				mux {
 					pins = "gpio44", "gpio45",
@@ -3286,6 +3590,25 @@ config {
 				};
 			};
 
+			qup_spi16_cs_gpio: qup-spi16-cs-gpio {
+				mux {
+					pins = "gpio48", "gpio49",
+					       "gpio50";
+					function = "qup16";
+				};
+
+				cs {
+					pins = "gpio51";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio48", "gpio49", "gpio50", "gpio51";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi16_default: qup-spi16-default {
 				mux {
 					pins = "gpio48", "gpio49",
@@ -3301,6 +3624,25 @@ config {
 				};
 			};
 
+			qup_spi17_cs_gpio: qup-spi17-cs-gpio {
+				mux {
+					pins = "gpio52", "gpio53",
+					       "gpio54";
+					function = "qup17";
+				};
+
+				cs {
+					pins = "gpio55";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio52", "gpio53", "gpio54", "gpio55";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi17_default: qup-spi17-default {
 				mux {
 					pins = "gpio52", "gpio53",
@@ -3316,6 +3658,25 @@ config {
 				};
 			};
 
+			qup_spi18_cs_gpio: qup-spi18-cs-gpio {
+				mux {
+					pins = "gpio56", "gpio57",
+					       "gpio58";
+					function = "qup18";
+				};
+
+				cs {
+					pins = "gpio59";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio56", "gpio57", "gpio58", "gpio59";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi18_default: qup-spi18-default {
 				mux {
 					pins = "gpio56", "gpio57",
@@ -3331,6 +3692,25 @@ config {
 				};
 			};
 
+			qup_spi19_cs_gpio: qup-spi19-cs-gpio {
+				mux {
+					pins = "gpio0", "gpio1",
+					       "gpio2";
+					function = "qup19";
+				};
+
+				cs {
+					pins = "gpio3";
+					function = "gpio";
+				};
+
+				config {
+					pins = "gpio0", "gpio1", "gpio2", "gpio3";
+					drive-strength = <6>;
+					bias-disable;
+				};
+			};
+
 			qup_spi19_default: qup-spi19-default {
 				mux {
 					pins = "gpio0", "gpio1",
-- 
2.30.0

[PATCH 2/2] arm64: dts: qcom: qrb5165-rb5: switch into using GPIO for SPI0 CS

[Dmitry Baryshkov]

GENI SPI controller shows several issues if it manages the CS on its own
(see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to
use GPIO for CS")) for the details. Configure SPI0 CS pin as a GPIO.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
index 50f94460c970..6098ccba85de 100644
--- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
+++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
@@ -1129,6 +1129,8 @@ &slpi {
 /* CAN */
 &spi0 {
 	status = "okay";
+	pinctrl-0 = <&qup_spi0_cs_gpio>;
+	cs-gpios = <&tlmm 31 GPIO_ACTIVE_LOW>;
 
 	can@0 {
 		compatible = "microchip,mcp2518fd";
-- 
2.30.0

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Bjorn Andersson]

On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote:

> GENI SPI controller shows several issues if it manages the CS on its own
> (see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to
> use GPIO for CS")) for the details. Provide pinctrl entries for SPI
> controllers using the same CS pin but in GPIO mode.
> 

Doug, can you confirm that this is the final (or at least current)
verdict?

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 380 +++++++++++++++++++++++++++
>  1 file changed, 380 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index 3cea28058a91..03015174ec06 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -3046,6 +3046,25 @@ config {
>  				};
>  			};
>  
> +			qup_spi0_cs_gpio: qup-spi0-cs-gpio {

There might be others who need the same states, but I would prefer if we
move this to the device's dts.

> +				mux {

Rather than splitting the properties in {mux, cs, config} I think it
makes more sense to split them in {spi, cs} or something like that.

Regards,
Bjorn

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Doug Anderson]

Hi,

On Thu, Feb 4, 2021 at 3:07 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote:
>
> > GENI SPI controller shows several issues if it manages the CS on its own
> > (see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to
> > use GPIO for CS")) for the details. Provide pinctrl entries for SPI
> > controllers using the same CS pin but in GPIO mode.

I'm curious: were you seeing real problems or are you just trying to
optimize things more?  The only known problem (other than performance)
that I'm aware of is that if you get really high interrupt latency
then setting the chip select can appear to "fail" (we timeout waiting
for the interrupt saying that the chip select command was done).  The
SPI framework doesn't expect setting the chip select to be something
that can fail so error handling isn't the most amazing in this case,
though at least it shouldn't crash after the most recent fixes I sent
to the SPI driver.


> Doug, can you confirm that this is the final (or at least current)
> verdict?

As far as I know using a GPIO chip select is always superior on
Qualcomm hardware unless you're forced into GPI/GSI mode.  In GPI/GSI
mode (I think) you lose full control of the chip select anyway.  I
wrote this long-winded explanation recently to explain it to someone
else.  Pasting here in case it's useful, though there's overlap w/ the
commit message that Dmitry pointed at.

---

In general the Linux kernel supports using any GPIO as a SPI chip
select.  However, it also supports the concept that a SPI controller
may have its own way of controlling chip select.  You can freely mix
and match these approaches.  For instance maybe you've got 4 devices
on a single SPI "bus" (we never do this on Chrome OS designs but it is
possible).  The SPI controller itself might have the ability to
control two chip selects.  You could still make things work by hooking
two of the peripherals up to GPIOs and two up to the SPI controller's
native chip selects and it'd all be hunky dory.  Or you could choose
to not use any of the builtin chip selects and use all 4 on GPIOs.
...or 3 and 1.

When a SPI controller has a builtin chip select, it can usually be
configured in two ways: something more automatic where the controller
asserts the chip select and deasserts it automatically around
transfers or in a fully manual mode.  In general Linux prefers the
fully manual mode.  The Linux API to SPI endpoint drivers is super
flexible and allows them to assert/deassert chip select at will and
it's hard to honor that flexibility when it's not manually controlled.

If a SPI chip select is manually controlled it's not at all different
from a GPIO configured in "output mode".  Thus using a GPIO instead of
a builtin chip select has no real downsides.

On many SoCs, sc7180 included, pinmuxing allows you to reconfigure
almost any pin as a GPIO.  So it turns out that on sc7180 boards it's
just a different way of looking at it to say whether we're hooked up
to a GPIO or we're hooked up to the native chip select logic.  Both
are valid ways to look at it.

On Qualcomm SoCs it's incredibly inefficient to control the native SPI
chip select in "manual mode".  It involves sending a packet (a
command) to the SPI controller and waiting for it to respond that it
set the chip select.  However, it is plenty efficient to control
GPIOs.  Thus it is more efficient (with no real downsides) to envision
that the chip select is hooked up to a GPIO instead of the native chip
select.

---

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8250.dtsi | 380 +++++++++++++++++++++++++++
> >  1 file changed, 380 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > index 3cea28058a91..03015174ec06 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > @@ -3046,6 +3046,25 @@ config {
> >                               };
> >                       };
> >
> > +                     qup_spi0_cs_gpio: qup-spi0-cs-gpio {
>
> There might be others who need the same states, but I would prefer if we
> move this to the device's dts.

This is opposite to what Stephen requested, though it was in a review
on our gerrit and not on lists [1].  :-P

It definitely feels like a 6 of one half dozen of the other.  Unless
you're dead set on moving them to the board dts my bias would be
towards keeping consistent with what was done on sc7180.  If you
really want this moved to the board file we should do it for sc7180
too.


> > +                             mux {
>
> Rather than splitting the properties in {mux, cs, config} I think it
> makes more sense to split them in {spi, cs} or something like that.

In general pinconf doesn't belong in the SoC dts file.  If there's no
reason to change it seems like this should match what sc7180 did.


[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2406557/1/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Dmitry Baryshkov]

On 05/02/2021 02:31, Doug Anderson wrote:
> Hi,
> 
> On Thu, Feb 4, 2021 at 3:07 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
>>
>> On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote:
>>
>>> GENI SPI controller shows several issues if it manages the CS on its own
>>> (see 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to
>>> use GPIO for CS")) for the details. Provide pinctrl entries for SPI
>>> controllers using the same CS pin but in GPIO mode.
> 
> I'm curious: were you seeing real problems or are you just trying to
> optimize things more?  The only known problem (other than performance)
> that I'm aware of is that if you get really high interrupt latency
> then setting the chip select can appear to "fail" (we timeout waiting
> for the interrupt saying that the chip select command was done).  The
> SPI framework doesn't expect setting the chip select to be something
> that can fail so error handling isn't the most amazing in this case,
> though at least it shouldn't crash after the most recent fixes I sent
> to the SPI driver.

I have been observing strange behaviour from the SPI CAN interface. This 
change allowed me to narrow down the issue (with the GPI support for 
SPI) we had in our tree.

[skipped the explanation.

>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   arch/arm64/boot/dts/qcom/sm8250.dtsi | 380 +++++++++++++++++++++++++++
>>>   1 file changed, 380 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
>>> index 3cea28058a91..03015174ec06 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
>>> @@ -3046,6 +3046,25 @@ config {
>>>                                };
>>>                        };
>>>
>>> +                     qup_spi0_cs_gpio: qup-spi0-cs-gpio {
>>
>> There might be others who need the same states, but I would prefer if we
>> move this to the device's dts.
> 
> This is opposite to what Stephen requested, though it was in a review
> on our gerrit and not on lists [1].  :-P
> 
> It definitely feels like a 6 of one half dozen of the other.  Unless
> you're dead set on moving them to the board dts my bias would be
> towards keeping consistent with what was done on sc7180.  If you
> really want this moved to the board file we should do it for sc7180
> too.
> 
> 
>>> +                             mux {
>>
>> Rather than splitting the properties in {mux, cs, config} I think it
>> makes more sense to split them in {spi, cs} or something like that.

That was my original idea, with qup-spi-nocs config being in sm8250.dtsi 
and spi0_cs (defining only CS) belonging to the board dts.

> 
> In general pinconf doesn't belong in the SoC dts file.  If there's no
> reason to change it seems like this should match what sc7180 did.
> 
> 
> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2406557/1/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi


-- 
With best wishes
Dmitry

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Bjorn Andersson]

On Thu 04 Feb 17:31 CST 2021, Doug Anderson wrote:
> On Thu, Feb 4, 2021 at 3:07 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Thu 04 Feb 14:49 CST 2021, Dmitry Baryshkov wrote:
[..]
> > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > index 3cea28058a91..03015174ec06 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> > > @@ -3046,6 +3046,25 @@ config {
> > >                               };
> > >                       };
> > >
> > > +                     qup_spi0_cs_gpio: qup-spi0-cs-gpio {
> >
> > There might be others who need the same states, but I would prefer if we
> > move this to the device's dts.
> 
> This is opposite to what Stephen requested, though it was in a review
> on our gerrit and not on lists [1].  :-P
> 
> It definitely feels like a 6 of one half dozen of the other.  Unless
> you're dead set on moving them to the board dts my bias would be
> towards keeping consistent with what was done on sc7180.  If you
> really want this moved to the board file we should do it for sc7180
> too.
> 

What I dislike is the fact that we have a huge amount of these unused in
the platform.dtsi, but let's align with what I agreed to on
sc7180...Sorry for the short memory.

> 
> > > +                             mux {
> >
> > Rather than splitting the properties in {mux, cs, config} I think it
> > makes more sense to split them in {spi, cs} or something like that.
> 
> In general pinconf doesn't belong in the SoC dts file.  If there's no
> reason to change it seems like this should match what sc7180 did.
> 

Right, but I still would prefer the pinctrl state to be split by
function/pins, rather than pinmux vs pinconf. That way it's natural to
add pinconf properties to the various functional parts (i.e. bias or
drive-strength for the spi pins vs cs).

Do you have any concerns with this?

Regards,
Bjorn

> 
> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2406557/1/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Doug Anderson]

Hi,

On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> > > > +                             mux {
> > >
> > > Rather than splitting the properties in {mux, cs, config} I think it
> > > makes more sense to split them in {spi, cs} or something like that.
> >
> > In general pinconf doesn't belong in the SoC dts file.  If there's no
> > reason to change it seems like this should match what sc7180 did.
> >
>
> Right, but I still would prefer the pinctrl state to be split by
> function/pins, rather than pinmux vs pinconf. That way it's natural to
> add pinconf properties to the various functional parts (i.e. bias or
> drive-strength for the spi pins vs cs).
>
> Do you have any concerns with this?

I read this a few times and I'm not exactly sure what you're
proposing.  Can you provide an example of what you want it to look
like in this case?

-Doug

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Bjorn Andersson]

On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote:

> Hi,
> 
> On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > > > > +                             mux {
> > > >
> > > > Rather than splitting the properties in {mux, cs, config} I think it
> > > > makes more sense to split them in {spi, cs} or something like that.
> > >
> > > In general pinconf doesn't belong in the SoC dts file.  If there's no
> > > reason to change it seems like this should match what sc7180 did.
> > >
> >
> > Right, but I still would prefer the pinctrl state to be split by
> > function/pins, rather than pinmux vs pinconf. That way it's natural to
> > add pinconf properties to the various functional parts (i.e. bias or
> > drive-strength for the spi pins vs cs).
> >
> > Do you have any concerns with this?
> 
> I read this a few times and I'm not exactly sure what you're
> proposing.  Can you provide an example of what you want it to look
> like in this case?
> 

Today in most cases we group pinctrl properties by being a "conf" of a
"mux" property, so we end up with:

the_state: spi-state {
	all-the-mux-properties {
		pins = "gpio40", gpio41", "gpio42", "gpio43";
		function = qup14";
	};

	repeat-pins-and-add-all-conf-properties {
		pins = "gpio40", gpio41", "gpio42", "gpio43";
		drive-strength = <6>;
		bias-disable;
	};
};

This made sense to me after implementing the driver, there's muxing to
be done and there's electrical configuration to configure.

But what's actually trying to describe is a hardware state; i.e. that
miso, mosi, clk and cs should be acting in a particular fashion.

In particular this lends itself useful when the hardware state consists
of different functions, a good example being the Bluetooth UART, or in
the SPI-with-separate-GPIO:

the_state: spi-state {
	miso-mosi-clk {
		pins = "gpio40", gpio41", "gpio42"
		function = qup14";
		drive-strength = <6>;
		bias-disable;
	};

	cs {
		pins = "gpio43";
		function = "gpio";
		drive-strength = <6>;
		bias-disable;
	};
};


For the case of uniform configuration across the state we've come to
sprinkle a few different synonyms for "pinconf" and "pinmux" in the
state nodes. But a few years ago someone updated the state parser to
handle cases either directly in the state or in subnodes. So we can
avoid these boilerplate nodes with a simple:

the_state: spi-state {
	pins = "gpio40", gpio41", "gpio42", "gpio43";
	function = qup14";
	drive-strength = <6>;
	bias-disable;
};

Regards,
Bjorn

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Doug Anderson]

Hi,

On Fri, Feb 5, 2021 at 8:48 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > > > > +                             mux {
> > > > >
> > > > > Rather than splitting the properties in {mux, cs, config} I think it
> > > > > makes more sense to split them in {spi, cs} or something like that.
> > > >
> > > > In general pinconf doesn't belong in the SoC dts file.  If there's no
> > > > reason to change it seems like this should match what sc7180 did.
> > > >
> > >
> > > Right, but I still would prefer the pinctrl state to be split by
> > > function/pins, rather than pinmux vs pinconf. That way it's natural to
> > > add pinconf properties to the various functional parts (i.e. bias or
> > > drive-strength for the spi pins vs cs).
> > >
> > > Do you have any concerns with this?
> >
> > I read this a few times and I'm not exactly sure what you're
> > proposing.  Can you provide an example of what you want it to look
> > like in this case?
> >
>
> Today in most cases we group pinctrl properties by being a "conf" of a
> "mux" property, so we end up with:
>
> the_state: spi-state {
>         all-the-mux-properties {
>                 pins = "gpio40", gpio41", "gpio42", "gpio43";
>                 function = qup14";
>         };
>
>         repeat-pins-and-add-all-conf-properties {
>                 pins = "gpio40", gpio41", "gpio42", "gpio43";
>                 drive-strength = <6>;
>                 bias-disable;
>         };
> };
>
> This made sense to me after implementing the driver, there's muxing to
> be done and there's electrical configuration to configure.
>
> But what's actually trying to describe is a hardware state; i.e. that
> miso, mosi, clk and cs should be acting in a particular fashion.
>
> In particular this lends itself useful when the hardware state consists
> of different functions, a good example being the Bluetooth UART, or in
> the SPI-with-separate-GPIO:
>
> the_state: spi-state {
>         miso-mosi-clk {
>                 pins = "gpio40", gpio41", "gpio42"
>                 function = qup14";
>                 drive-strength = <6>;
>                 bias-disable;
>         };
>
>         cs {
>                 pins = "gpio43";
>                 function = "gpio";
>                 drive-strength = <6>;
>                 bias-disable;
>         };
> };
>
>
> For the case of uniform configuration across the state we've come to
> sprinkle a few different synonyms for "pinconf" and "pinmux" in the
> state nodes. But a few years ago someone updated the state parser to
> handle cases either directly in the state or in subnodes. So we can
> avoid these boilerplate nodes with a simple:
>
> the_state: spi-state {
>         pins = "gpio40", gpio41", "gpio42", "gpio43";
>         function = qup14";
>         drive-strength = <6>;
>         bias-disable;
> };

OK, this makes sense to me.  I always felt like the extra "pinconf" /
"pinmux" made things awkward.  I guess someone should try to convert
some SoC dtsi fully over so we can see how it looks?  In this case I
think you'd have something like this, right?

SoC dtsi:

tlmm: pinctrl@... {
  qup_spi0_data_clk: qup-spi0-data-clk {
    pins = "gpio28", "gpio29", "gpio30";
    function = "qup0";
  };

  qup_spi0_cs: qup-spi0-cs {
    pins = "gpio31";
    function = "qup0";
  };

  qup_spi0_cs_gpio: qup-spi0-cs-gpio {
    pins = "gpio31";
    function = "gpio";
  };
};

Board dts:

&spi0 {
  pinctrl-0 = <&qup_spi0_data_clk>, &<qup_spi0_cs_gpio>;
};

&qup_spi0_data_clk {
  drive-strength = <6>;
  bias-disable;
};

&qup_spi0_cs_gpio {
  drive-strength = <6>;
  bias-disable;
};

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Bjorn Andersson]

On Mon 08 Feb 09:58 CST 2021, Doug Anderson wrote:

> Hi,
> 
> On Fri, Feb 5, 2021 at 8:48 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > >
> > > > > > > +                             mux {
> > > > > >
> > > > > > Rather than splitting the properties in {mux, cs, config} I think it
> > > > > > makes more sense to split them in {spi, cs} or something like that.
> > > > >
> > > > > In general pinconf doesn't belong in the SoC dts file.  If there's no
> > > > > reason to change it seems like this should match what sc7180 did.
> > > > >
> > > >
> > > > Right, but I still would prefer the pinctrl state to be split by
> > > > function/pins, rather than pinmux vs pinconf. That way it's natural to
> > > > add pinconf properties to the various functional parts (i.e. bias or
> > > > drive-strength for the spi pins vs cs).
> > > >
> > > > Do you have any concerns with this?
> > >
> > > I read this a few times and I'm not exactly sure what you're
> > > proposing.  Can you provide an example of what you want it to look
> > > like in this case?
> > >
> >
> > Today in most cases we group pinctrl properties by being a "conf" of a
> > "mux" property, so we end up with:
> >
> > the_state: spi-state {
> >         all-the-mux-properties {
> >                 pins = "gpio40", gpio41", "gpio42", "gpio43";
> >                 function = qup14";
> >         };
> >
> >         repeat-pins-and-add-all-conf-properties {
> >                 pins = "gpio40", gpio41", "gpio42", "gpio43";
> >                 drive-strength = <6>;
> >                 bias-disable;
> >         };
> > };
> >
> > This made sense to me after implementing the driver, there's muxing to
> > be done and there's electrical configuration to configure.
> >
> > But what's actually trying to describe is a hardware state; i.e. that
> > miso, mosi, clk and cs should be acting in a particular fashion.
> >
> > In particular this lends itself useful when the hardware state consists
> > of different functions, a good example being the Bluetooth UART, or in
> > the SPI-with-separate-GPIO:
> >
> > the_state: spi-state {
> >         miso-mosi-clk {
> >                 pins = "gpio40", gpio41", "gpio42"
> >                 function = qup14";
> >                 drive-strength = <6>;
> >                 bias-disable;
> >         };
> >
> >         cs {
> >                 pins = "gpio43";
> >                 function = "gpio";
> >                 drive-strength = <6>;
> >                 bias-disable;
> >         };
> > };
> >
> >
> > For the case of uniform configuration across the state we've come to
> > sprinkle a few different synonyms for "pinconf" and "pinmux" in the
> > state nodes. But a few years ago someone updated the state parser to
> > handle cases either directly in the state or in subnodes. So we can
> > avoid these boilerplate nodes with a simple:
> >
> > the_state: spi-state {
> >         pins = "gpio40", gpio41", "gpio42", "gpio43";
> >         function = qup14";
> >         drive-strength = <6>;
> >         bias-disable;
> > };
> 
> OK, this makes sense to me.  I always felt like the extra "pinconf" /
> "pinmux" made things awkward.

I'm happy to hear that :)

> I guess someone should try to convert some SoC dtsi fully over so we
> can see how it looks?

Sounds good. I feel fairly confident, so let's pick SM8250 and aim to
land this patch in the "new" form.

> In this case I think you'd have something like this, right?
> 
> SoC dtsi:
> 
> tlmm: pinctrl@... {
>   qup_spi0_data_clk: qup-spi0-data-clk {
>     pins = "gpio28", "gpio29", "gpio30";
>     function = "qup0";
>   };
> 
>   qup_spi0_cs: qup-spi0-cs {
>     pins = "gpio31";
>     function = "qup0";
>   };
> 
>   qup_spi0_cs_gpio: qup-spi0-cs-gpio {
>     pins = "gpio31";
>     function = "gpio";
>   };
> };
> 
> Board dts:
> 
> &spi0 {
>   pinctrl-0 = <&qup_spi0_data_clk>, &<qup_spi0_cs_gpio>;
> };
> 
> &qup_spi0_data_clk {
>   drive-strength = <6>;
>   bias-disable;
> };
> 
> &qup_spi0_cs_gpio {
>   drive-strength = <6>;
>   bias-disable;
> };

Correct. And providing a common state for the 3 non-cs pins and using
the pinctrl-0 to select which kind of cs we want looks even better.

Regards,
Bjorn

Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Dmitry Baryshkov]

On Mon, 8 Feb 2021 at 21:04, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>
> On Mon 08 Feb 09:58 CST 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Fri, Feb 5, 2021 at 8:48 AM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote:
> > >
> > > > Hi,
> > > >
> > > > On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson
> > > > <bjorn.andersson@linaro.org> wrote:
> > > > >
> > > > > > > > +                             mux {
> > > > > > >
> > > > > > > Rather than splitting the properties in {mux, cs, config} I think it
> > > > > > > makes more sense to split them in {spi, cs} or something like that.
> > > > > >
> > > > > > In general pinconf doesn't belong in the SoC dts file.  If there's no
> > > > > > reason to change it seems like this should match what sc7180 did.
> > > > > >
> > > > >
> > > > > Right, but I still would prefer the pinctrl state to be split by
> > > > > function/pins, rather than pinmux vs pinconf. That way it's natural to
> > > > > add pinconf properties to the various functional parts (i.e. bias or
> > > > > drive-strength for the spi pins vs cs).
> > > > >
> > > > > Do you have any concerns with this?
> > > >
> > > > I read this a few times and I'm not exactly sure what you're
> > > > proposing.  Can you provide an example of what you want it to look
> > > > like in this case?
> > > >
> > >
> > > Today in most cases we group pinctrl properties by being a "conf" of a
> > > "mux" property, so we end up with:
> > >
> > > the_state: spi-state {
> > >         all-the-mux-properties {
> > >                 pins = "gpio40", gpio41", "gpio42", "gpio43";
> > >                 function = qup14";
> > >         };
> > >
> > >         repeat-pins-and-add-all-conf-properties {
> > >                 pins = "gpio40", gpio41", "gpio42", "gpio43";
> > >                 drive-strength = <6>;
> > >                 bias-disable;
> > >         };
> > > };
> > >
> > > This made sense to me after implementing the driver, there's muxing to
> > > be done and there's electrical configuration to configure.
> > >
> > > But what's actually trying to describe is a hardware state; i.e. that
> > > miso, mosi, clk and cs should be acting in a particular fashion.
> > >
> > > In particular this lends itself useful when the hardware state consists
> > > of different functions, a good example being the Bluetooth UART, or in
> > > the SPI-with-separate-GPIO:
> > >
> > > the_state: spi-state {
> > >         miso-mosi-clk {
> > >                 pins = "gpio40", gpio41", "gpio42"
> > >                 function = qup14";
> > >                 drive-strength = <6>;
> > >                 bias-disable;
> > >         };
> > >
> > >         cs {
> > >                 pins = "gpio43";
> > >                 function = "gpio";
> > >                 drive-strength = <6>;
> > >                 bias-disable;
> > >         };
> > > };
> > >
> > >
> > > For the case of uniform configuration across the state we've come to
> > > sprinkle a few different synonyms for "pinconf" and "pinmux" in the
> > > state nodes. But a few years ago someone updated the state parser to
> > > handle cases either directly in the state or in subnodes. So we can
> > > avoid these boilerplate nodes with a simple:
> > >
> > > the_state: spi-state {
> > >         pins = "gpio40", gpio41", "gpio42", "gpio43";
> > >         function = qup14";
> > >         drive-strength = <6>;
> > >         bias-disable;
> > > };
> >
> > OK, this makes sense to me.  I always felt like the extra "pinconf" /
> > "pinmux" made things awkward.
>
> I'm happy to hear that :)
>
> > I guess someone should try to convert some SoC dtsi fully over so we
> > can see how it looks?
>
> Sounds good. I feel fairly confident, so let's pick SM8250 and aim to
> land this patch in the "new" form.

OK. As a starting bit I will convert SPI pinctrl for now with the rest
of pins to follow.

>
> > In this case I think you'd have something like this, right?
> >
> > SoC dtsi:
> >
> > tlmm: pinctrl@... {
> >   qup_spi0_data_clk: qup-spi0-data-clk {
> >     pins = "gpio28", "gpio29", "gpio30";
> >     function = "qup0";
> >   };
> >
> >   qup_spi0_cs: qup-spi0-cs {
> >     pins = "gpio31";
> >     function = "qup0";
> >   };
> >
> >   qup_spi0_cs_gpio: qup-spi0-cs-gpio {
> >     pins = "gpio31";
> >     function = "gpio";
> >   };
> > };
> >
> > Board dts:
> >
> > &spi0 {
> >   pinctrl-0 = <&qup_spi0_data_clk>, &<qup_spi0_cs_gpio>;
> > };
> >
> > &qup_spi0_data_clk {
> >   drive-strength = <6>;
> >   bias-disable;
> > };
> >
> > &qup_spi0_cs_gpio {
> >   drive-strength = <6>;
> >   bias-disable;
> > };
>
> Correct. And providing a common state for the 3 non-cs pins and using
> the pinctrl-0 to select which kind of cs we want looks even better.
>
> Regards,
> Bjorn



-- 
With best wishes
Dmitry