[PATCH 1/3] arm64: dts: qcom: sdm630: fix UART1 pin bias

[PATCH 1/3] arm64: dts: qcom: sdm630: fix UART1 pin bias

[Krzysztof Kozlowski]

There is no "bias-no-pull" property.  Assume intentions were disabling
bias.

Fixes: b190fb010664 ("arm64: dts: qcom: sdm630: Add sdm630 dts file")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Not tested on hardware.
---
 arch/arm64/boot/dts/qcom/sdm630.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
index b51b85f583e5..e119060ac56c 100644
--- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
@@ -779,7 +779,7 @@ rx-cts-rts {
 					pins = "gpio17", "gpio18", "gpio19";
 					function = "gpio";
 					drive-strength = <2>;
-					bias-no-pull;
+					bias-disable;
 				};
 			};
 
-- 
2.34.1

[PATCH 2/3] arm64: dts: qcom: sdm845-db845c: correct SPI2 pins drive strength

[Krzysztof Kozlowski]

The pin configuration (done with generic pin controller helpers and
as expressed by bindings) requires children nodes with either:
1. "pins" property and the actual configuration,
2. another set of nodes with above point.

The qup_spi2_default pin configuration used second method - with a
"pinmux" child.

Fixes: 8d23a0040475 ("arm64: dts: qcom: db845c: add Low speed expansion i2c and spi nodes")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Not tested on hardware.
---
 arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index 132417e2d11e..a157eab66dee 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -1123,7 +1123,9 @@ &wifi {
 
 /* PINCTRL - additions to nodes defined in sdm845.dtsi */
 &qup_spi2_default {
-	drive-strength = <16>;
+	pinmux {
+		drive-strength = <16>;
+	};
 };
 
 &qup_uart3_default{
-- 
2.34.1

Re: [PATCH 2/3] arm64: dts: qcom: sdm845-db845c: correct SPI2 pins drive strength

[Doug Anderson]

Hi,

On Fri, Sep 30, 2022 at 11:22 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> The pin configuration (done with generic pin controller helpers and
> as expressed by bindings) requires children nodes with either:
> 1. "pins" property and the actual configuration,
> 2. another set of nodes with above point.
>
> The qup_spi2_default pin configuration used second method - with a
> "pinmux" child.
>
> Fixes: 8d23a0040475 ("arm64: dts: qcom: db845c: add Low speed expansion i2c and spi nodes")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Not tested on hardware.
> ---
>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> index 132417e2d11e..a157eab66dee 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> @@ -1123,7 +1123,9 @@ &wifi {
>
>  /* PINCTRL - additions to nodes defined in sdm845.dtsi */
>  &qup_spi2_default {
> -       drive-strength = <16>;
> +       pinmux {
> +               drive-strength = <16>;
> +       };

The convention on Qualcomm boards of this era is that muxing (setting
the function) is done under a "pinmux" node and, unless some of the
pins need to be treated differently like for the UARTs, configuration
(bias, drive strength, etc) is done under a "pinconf" subnode. I
believe that the "pinconf" subnode also needs to replicate the list of
pins, or at least that's what we did everywhere else on sdm845 /
sc7180.

Thus to match conventions, I assume you'd do:

&qup_spi2_default {
  pinconf {
    pins = "gpio27", "gpio28", "gpio29", "gpio30";
    drive-strength = <16>;
  };
};

We've since moved away from this to a less cumbersome approach, but
for "older" boards like db845c we should probably match the existing
convention, or have a flag day and change all sdm845 boards over to
the new convention.

-Doug

Re: [PATCH 2/3] arm64: dts: qcom: sdm845-db845c: correct SPI2 pins drive strength

[Krzysztof Kozlowski]

On 30/09/2022 22:12, Doug Anderson wrote:
> Hi,
> 
> On Fri, Sep 30, 2022 at 11:22 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> The pin configuration (done with generic pin controller helpers and
>> as expressed by bindings) requires children nodes with either:
>> 1. "pins" property and the actual configuration,
>> 2. another set of nodes with above point.
>>
>> The qup_spi2_default pin configuration used second method - with a
>> "pinmux" child.
>>
>> Fixes: 8d23a0040475 ("arm64: dts: qcom: db845c: add Low speed expansion i2c and spi nodes")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Not tested on hardware.
>> ---
>>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
>> index 132417e2d11e..a157eab66dee 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
>> @@ -1123,7 +1123,9 @@ &wifi {
>>
>>  /* PINCTRL - additions to nodes defined in sdm845.dtsi */
>>  &qup_spi2_default {
>> -       drive-strength = <16>;
>> +       pinmux {
>> +               drive-strength = <16>;
>> +       };
> 
> The convention on Qualcomm boards of this era is that muxing (setting
> the function) is done under a "pinmux" node and, unless some of the
> pins need to be treated differently like for the UARTs, configuration
> (bias, drive strength, etc) is done under a "pinconf" subnode.

Yes, although this was not expressed in bindings.

> I
> believe that the "pinconf" subnode also needs to replicate the list of
> pins, or at least that's what we did everywhere else on sdm845 /
> sc7180.

Yes.

> 
> Thus to match conventions, I assume you'd do:
> 
> &qup_spi2_default {
>   pinconf {

No, because I want a convention of all pinctrl bindings and drivers, not
convention of old pinctrl ones. The new ones are already moved or being
moved to "-state" and "-pins". In the same time I am also unifying the
requirement of "function" property - enforcing it in each node, thus
"pinconf" will not be valid anymore.

>     pins = "gpio27", "gpio28", "gpio29", "gpio30";
>     drive-strength = <16>;
>   };
> };
> 
> We've since moved away from this to a less cumbersome approach, but
> for "older" boards like db845c we should probably match the existing
> convention, or have a flag day and change all sdm845 boards over to
> the new convention.

That's what my next patchset from yesterday was doing. Unifying the
bindings with modern bindings and converting DTS to match them.

https://lore.kernel.org/linux-devicetree/20220930200529.331223-1-krzysztof.kozlowski@linaro.org/T/#t


Best regards,
Krzysztof

Re: [PATCH 2/3] arm64: dts: qcom: sdm845-db845c: correct SPI2 pins drive strength

[Doug Anderson]

Hi,

On Sat, Oct 1, 2022 at 3:01 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 30/09/2022 22:12, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Sep 30, 2022 at 11:22 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> The pin configuration (done with generic pin controller helpers and
> >> as expressed by bindings) requires children nodes with either:
> >> 1. "pins" property and the actual configuration,
> >> 2. another set of nodes with above point.
> >>
> >> The qup_spi2_default pin configuration used second method - with a
> >> "pinmux" child.
> >>
> >> Fixes: 8d23a0040475 ("arm64: dts: qcom: db845c: add Low speed expansion i2c and spi nodes")
> >> Cc: <stable@vger.kernel.org>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>
> >> ---
> >>
> >> Not tested on hardware.
> >> ---
> >>  arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >> index 132417e2d11e..a157eab66dee 100644
> >> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >> @@ -1123,7 +1123,9 @@ &wifi {
> >>
> >>  /* PINCTRL - additions to nodes defined in sdm845.dtsi */
> >>  &qup_spi2_default {
> >> -       drive-strength = <16>;
> >> +       pinmux {
> >> +               drive-strength = <16>;
> >> +       };
> >
> > The convention on Qualcomm boards of this era is that muxing (setting
> > the function) is done under a "pinmux" node and, unless some of the
> > pins need to be treated differently like for the UARTs, configuration
> > (bias, drive strength, etc) is done under a "pinconf" subnode.
>
> Yes, although this was not expressed in bindings.
>
> > I
> > believe that the "pinconf" subnode also needs to replicate the list of
> > pins, or at least that's what we did everywhere else on sdm845 /
> > sc7180.
>
> Yes.
>
> >
> > Thus to match conventions, I assume you'd do:
> >
> > &qup_spi2_default {
> >   pinconf {
>
> No, because I want a convention of all pinctrl bindings and drivers, not
> convention of old pinctrl ones. The new ones are already moved or being
> moved to "-state" and "-pins". In the same time I am also unifying the
> requirement of "function" property - enforcing it in each node, thus
> "pinconf" will not be valid anymore.

Regardless of where we want to end up, it feels like as of ${SUBJECT}
patch this should match existing conventions in this file. If a later
patch wants to change the conventions in this file then it can, but
having just this one patch leaving things in an inconsistent state
isn't great IMO...

If this really has to be one-off then the subnode shouldn't be called
"pinmux". A subnode called "pinmux" implies that it just has muxing
information in it. After your patch this is called "pinmux" but has
_configuration_ in it.


> >     pins = "gpio27", "gpio28", "gpio29", "gpio30";
> >     drive-strength = <16>;
> >   };
> > };
> >
> > We've since moved away from this to a less cumbersome approach, but
> > for "older" boards like db845c we should probably match the existing
> > convention, or have a flag day and change all sdm845 boards over to
> > the new convention.
>
> That's what my next patchset from yesterday was doing. Unifying the
> bindings with modern bindings and converting DTS to match them.
>
> https://lore.kernel.org/linux-devicetree/20220930200529.331223-1-krzysztof.kozlowski@linaro.org/T/#t

I wasn't CCed on that patch series. A few things jump out as not quite
right to me. I'll try to do a review.

-Doug

Re: [PATCH 2/3] arm64: dts: qcom: sdm845-db845c: correct SPI2 pins drive strength

[Krzysztof Kozlowski]

On 03/10/2022 17:40, Doug Anderson wrote:
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
>>>> index 132417e2d11e..a157eab66dee 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
>>>> @@ -1123,7 +1123,9 @@ &wifi {
>>>>
>>>>  /* PINCTRL - additions to nodes defined in sdm845.dtsi */
>>>>  &qup_spi2_default {
>>>> -       drive-strength = <16>;
>>>> +       pinmux {
>>>> +               drive-strength = <16>;
>>>> +       };
>>>
>>> The convention on Qualcomm boards of this era is that muxing (setting
>>> the function) is done under a "pinmux" node and, unless some of the
>>> pins need to be treated differently like for the UARTs, configuration
>>> (bias, drive strength, etc) is done under a "pinconf" subnode.
>>
>> Yes, although this was not expressed in bindings.
>>
>>> I
>>> believe that the "pinconf" subnode also needs to replicate the list of
>>> pins, or at least that's what we did everywhere else on sdm845 /
>>> sc7180.
>>
>> Yes.
>>
>>>
>>> Thus to match conventions, I assume you'd do:
>>>
>>> &qup_spi2_default {
>>>   pinconf {
>>
>> No, because I want a convention of all pinctrl bindings and drivers, not
>> convention of old pinctrl ones. The new ones are already moved or being
>> moved to "-state" and "-pins". In the same time I am also unifying the
>> requirement of "function" property - enforcing it in each node, thus
>> "pinconf" will not be valid anymore.
> 
> Regardless of where we want to end up, it feels like as of ${SUBJECT}
> patch this should match existing conventions in this file. If a later
> patch wants to change the conventions in this file then it can, but
> having just this one patch leaving things in an inconsistent state
> isn't great IMO...
> 
> If this really has to be one-off then the subnode shouldn't be called
> "pinmux". A subnode called "pinmux" implies that it just has muxing
> information in it. After your patch this is called "pinmux" but has
> _configuration_ in it.
> 

It is a poor argument to keep some convention which is both
undocumented, not kept in this file and known only to some folks
(although that's effect of lack of documentation). Even the bindings do
not say it should be "pinconf" but they mention "config" in example. The
existing sdm845.dts uses config - so why now there should be "pinconf"?
By this "convention" we have both "pinmux" and "mux", perfect. Several
other pins do not have pinmux/mux/config at all.

This convention was never implemented, so there is nothing to keep/match.

Changing it to "config" (because this is the most used "convention" in
the file and bindings) would also mean to add useless "pins" which will
be in next patch removed.

Best regards,
Krzysztof

Re: [PATCH 2/3] arm64: dts: qcom: sdm845-db845c: correct SPI2 pins drive strength

[Doug Anderson]

Hi,

On Mon, Oct 3, 2022 at 10:57 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 03/10/2022 17:40, Doug Anderson wrote:
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >>>> index 132417e2d11e..a157eab66dee 100644
> >>>> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> >>>> @@ -1123,7 +1123,9 @@ &wifi {
> >>>>
> >>>>  /* PINCTRL - additions to nodes defined in sdm845.dtsi */
> >>>>  &qup_spi2_default {
> >>>> -       drive-strength = <16>;
> >>>> +       pinmux {
> >>>> +               drive-strength = <16>;
> >>>> +       };
> >>>
> >>> The convention on Qualcomm boards of this era is that muxing (setting
> >>> the function) is done under a "pinmux" node and, unless some of the
> >>> pins need to be treated differently like for the UARTs, configuration
> >>> (bias, drive strength, etc) is done under a "pinconf" subnode.
> >>
> >> Yes, although this was not expressed in bindings.
> >>
> >>> I
> >>> believe that the "pinconf" subnode also needs to replicate the list of
> >>> pins, or at least that's what we did everywhere else on sdm845 /
> >>> sc7180.
> >>
> >> Yes.
> >>
> >>>
> >>> Thus to match conventions, I assume you'd do:
> >>>
> >>> &qup_spi2_default {
> >>>   pinconf {
> >>
> >> No, because I want a convention of all pinctrl bindings and drivers, not
> >> convention of old pinctrl ones. The new ones are already moved or being
> >> moved to "-state" and "-pins". In the same time I am also unifying the
> >> requirement of "function" property - enforcing it in each node, thus
> >> "pinconf" will not be valid anymore.
> >
> > Regardless of where we want to end up, it feels like as of ${SUBJECT}
> > patch this should match existing conventions in this file. If a later
> > patch wants to change the conventions in this file then it can, but
> > having just this one patch leaving things in an inconsistent state
> > isn't great IMO...
> >
> > If this really has to be one-off then the subnode shouldn't be called
> > "pinmux". A subnode called "pinmux" implies that it just has muxing
> > information in it. After your patch this is called "pinmux" but has
> > _configuration_ in it.
> >
>
> It is a poor argument to keep some convention which is both
> undocumented, not kept in this file and known only to some folks
> (although that's effect of lack of documentation). Even the bindings do
> not say it should be "pinconf" but they mention "config" in example. The
> existing sdm845.dts uses config - so why now there should be "pinconf"?
> By this "convention" we have both "pinmux" and "mux", perfect. Several
> other pins do not have pinmux/mux/config at all.
>
> This convention was never implemented, so there is nothing to keep/match.
>
> Changing it to "config" (because this is the most used "convention" in
> the file and bindings) would also mean to add useless "pins" which will
> be in next patch removed.

I certainly won't make the argument that the old convention was great
or even that consistently followed. That's why it changed. ...but to
me it's more that a patch should stand on its own and not only make
sense in the context of future patches. After applying ${SUBJECT}
patch you end up with a node called "pinmux" that has more than just
muxing information in it. That seems less than ideal.

-Doug

Re: [PATCH 2/3] arm64: dts: qcom: sdm845-db845c: correct SPI2 pins drive strength

[Krzysztof Kozlowski]

On 04/10/2022 01:03, Doug Anderson wrote:
>>> If this really has to be one-off then the subnode shouldn't be called
>>> "pinmux". A subnode called "pinmux" implies that it just has muxing
>>> information in it. After your patch this is called "pinmux" but has
>>> _configuration_ in it.
>>>
>>
>> It is a poor argument to keep some convention which is both
>> undocumented, not kept in this file and known only to some folks
>> (although that's effect of lack of documentation). Even the bindings do
>> not say it should be "pinconf" but they mention "config" in example. The
>> existing sdm845.dts uses config - so why now there should be "pinconf"?
>> By this "convention" we have both "pinmux" and "mux", perfect. Several
>> other pins do not have pinmux/mux/config at all.
>>
>> This convention was never implemented, so there is nothing to keep/match.
>>
>> Changing it to "config" (because this is the most used "convention" in
>> the file and bindings) would also mean to add useless "pins" which will
>> be in next patch removed.
> 
> I certainly won't make the argument that the old convention was great
> or even that consistently followed. That's why it changed. ...but to
> me it's more that a patch should stand on its own and not only make
> sense in the context of future patches. After applying ${SUBJECT}
> patch you end up with a node called "pinmux" that has more than just
> muxing information in it. That seems less than ideal.

OK, let me make it part of "config" then to match other nodes from DTSI.

Best regards,
Krzysztof

[PATCH 3/3] arm64: dts: qcom: sdm845-cheza: fix AP suspend pin bias

[Krzysztof Kozlowski]

There is no "bias-no-pull" property.  Assume intentions were disabling
bias.

Fixes: 79e7739f7b87 ("arm64: dts: qcom: sdm845-cheza: add initial cheza dt")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Not tested on hardware.
---
 arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
index b5eb8f7eca1d..b5f11fbcc300 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
@@ -1436,7 +1436,7 @@ ap_suspend_l_assert: ap_suspend_l_assert {
 		config {
 			pins = "gpio126";
 			function = "gpio";
-			bias-no-pull;
+			bias-disable;
 			drive-strength = <2>;
 			output-low;
 		};
@@ -1446,7 +1446,7 @@ ap_suspend_l_deassert: ap_suspend_l_deassert {
 		config {
 			pins = "gpio126";
 			function = "gpio";
-			bias-no-pull;
+			bias-disable;
 			drive-strength = <2>;
 			output-high;
 		};
-- 
2.34.1

Re: [PATCH 3/3] arm64: dts: qcom: sdm845-cheza: fix AP suspend pin bias

[Doug Anderson]

Hi,

On Fri, Sep 30, 2022 at 11:22 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> There is no "bias-no-pull" property.  Assume intentions were disabling
> bias.
>
> Fixes: 79e7739f7b87 ("arm64: dts: qcom: sdm845-cheza: add initial cheza dt")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Not tested on hardware.
> ---
>  arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

Re: [PATCH 1/3] arm64: dts: qcom: sdm630: fix UART1 pin bias

[Doug Anderson]

Hi,

On Fri, Sep 30, 2022 at 11:22 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> There is no "bias-no-pull" property.  Assume intentions were disabling
> bias.
>
> Fixes: b190fb010664 ("arm64: dts: qcom: sdm630: Add sdm630 dts file")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Not tested on hardware.
> ---
>  arch/arm64/boot/dts/qcom/sdm630.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This does change behavior and has the potential to break someone.
Thus, without a bug report or someone to give a tested-by I'm at least
moderately worried about this going to stable@

I would also note that convention on Qualcomm SoCs that I've worked on
was that bias shouldn't be specified in the SoC dtsi file and should
be left to board files. This is talked a bit about in a previous email
thread [1].

That being said, it does look like this was the intention of the
original commit, so thus:

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

[1] https://lore.kernel.org/lkml/CAD=FV=VUL4GmjaibAMhKNdpEso_Hg_R=XeMaqah1LSj_9-Ce4Q@mail.gmail.com/

Re: [PATCH 1/3] arm64: dts: qcom: sdm630: fix UART1 pin bias

[Krzysztof Kozlowski]

On 30/09/2022 22:19, Doug Anderson wrote:
> Hi,
> 
> On Fri, Sep 30, 2022 at 11:22 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> There is no "bias-no-pull" property.  Assume intentions were disabling
>> bias.
>>
>> Fixes: b190fb010664 ("arm64: dts: qcom: sdm630: Add sdm630 dts file")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Not tested on hardware.
>> ---
>>  arch/arm64/boot/dts/qcom/sdm630.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> This does change behavior and has the potential to break someone.
> Thus, without a bug report or someone to give a tested-by I'm at least
> moderately worried about this going to stable@

Indeed. I can drop Cc-stable, but AUTOSEL can still pick it up because
of Fixes tag. Fixes tag is here important to indicate we are having a
bug before.

> 
> I would also note that convention on Qualcomm SoCs that I've worked on
> was that bias shouldn't be specified in the SoC dtsi file and should
> be left to board files. This is talked a bit about in a previous email
> thread [1].

Uh, that makes a lot of sense. It is almost always a property of a board.

> 
> That being said, it does look like this was the intention of the
> original commit, so thus:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks.

I can also drop the property entirely to match existing behavior (not
the intention).

> 
> [1] https://lore.kernel.org/lkml/CAD=FV=VUL4GmjaibAMhKNdpEso_Hg_R=XeMaqah1LSj_9-Ce4Q@mail.gmail.com/

Best regards,
Krzysztof

Re: [PATCH 1/3] arm64: dts: qcom: sdm630: fix UART1 pin bias

[Doug Anderson]

Hi,

On Sat, Oct 1, 2022 at 2:58 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> > I would also note that convention on Qualcomm SoCs that I've worked on
> > was that bias shouldn't be specified in the SoC dtsi file and should
> > be left to board files. This is talked a bit about in a previous email
> > thread [1].
>
> Uh, that makes a lot of sense. It is almost always a property of a board.

Right, though it can make sense to have a "default" in the SoC
sometimes. For instance, for i2c you almost always want external
pullups so you can tune them to the speed/trace lengths. Thus having a
default in the SoC file to disable i2c pullups would make a lot of
sense. The problem is the ugly / non-obvious "delete-property" we need
to put in the board.dts file if we ever need to override the SoC's
pull. :(

I actually remember this not being a problem in Rockchip SoCs. I guess
it's because they end up having an extra level of indirection. I guess
there's no great way to do that for Qualcomm without changing the
bindings.


> > That being said, it does look like this was the intention of the
> > original commit, so thus:
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> Thanks.
>
> I can also drop the property entirely to match existing behavior (not
> the intention).

Hopefully someone who cares about this board can test and let you know
either way.


> > [1] https://lore.kernel.org/lkml/CAD=FV=VUL4GmjaibAMhKNdpEso_Hg_R=XeMaqah1LSj_9-Ce4Q@mail.gmail.com/

Re: [PATCH 1/3] arm64: dts: qcom: sdm630: fix UART1 pin bias

[Krzysztof Kozlowski]

On 03/10/2022 17:29, Doug Anderson wrote:
> Hi,
> 
> On Sat, Oct 1, 2022 at 2:58 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>>> I would also note that convention on Qualcomm SoCs that I've worked on
>>> was that bias shouldn't be specified in the SoC dtsi file and should
>>> be left to board files. This is talked a bit about in a previous email
>>> thread [1].
>>
>> Uh, that makes a lot of sense. It is almost always a property of a board.
> 
> Right, though it can make sense to have a "default" in the SoC
> sometimes. 

If the default is safe, then could be. But it is still causing a risk of
developer just forgetting to configure the configs for his board.
Bringup of DTS should be a conscious decision, not just "copy and hope
it works", therefore recommendation is to configure per-board properties
in board. Even if it means duplication. The same was for board-provided
clocks or aliases.

> For instance, for i2c you almost always want external
> pullups so you can tune them to the speed/trace lengths. Thus having a
> default in the SoC file to disable i2c pullups would make a lot of
> sense. The problem is the ugly / non-obvious "delete-property" we need
> to put in the board.dts file if we ever need to override the SoC's
> pull. :(

Which might not help in reducing amount of code...



Best regards,
Krzysztof