Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support

[Rajendra Nayak]

On 02/15/2018 12:41 AM, Bjorn Andersson wrote:
> [..]
>>> +
>>> +   qup_1: geni-se@ac {
>> Color me confused.  So you're saying here that this is "qup_1".
>> ...but above you turn the pinmux for pins "GPIO4" and "GPIO5" to
>> "qup9", right?  So UART2 is on "qup 1" and "qup 9"?
>>
>> ...OK, so I stared at manuals a bunch more, and _maybe_ I get it.
>> Maybe there are 3 "QUP v3 modules" each of which handles up to 8
>> "QUP"s.  So QUP 9 is on "QUP module 1", is that right?  If everyone
>> understands this already and it's just me that's confused then I guess
>> you can just ignore this comment.  However, if you can think of any
>> better alias than "qup_1" that makes this less confusing then that
>> would make me extra happy.  Like maybe "qupv3_id_1" to match the
>> manual?
> This is indeed a source of confusion, in particular since there tend to
> be different numbering depending on what part of the puzzle you look at.
> But you're right that each QUP has a number of engines, each one being a
> UART/I2C/SPI controller.
> 
> I don't see a reason for labeling this particular node though, aliases
> references individual engines, not the wrapper.

makes sense, I'll just drop the label.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support

[Rajendra Nayak]

On 02/15/2018 12:41 AM, Bjorn Andersson wrote:
> [..]
>>> +
>>> +   qup_1: geni-se@ac {
>> Color me confused.  So you're saying here that this is "qup_1".
>> ...but above you turn the pinmux for pins "GPIO4" and "GPIO5" to
>> "qup9", right?  So UART2 is on "qup 1" and "qup 9"?
>>
>> ...OK, so I stared at manuals a bunch more, and _maybe_ I get it.
>> Maybe there are 3 "QUP v3 modules" each of which handles up to 8
>> "QUP"s.  So QUP 9 is on "QUP module 1", is that right?  If everyone
>> understands this already and it's just me that's confused then I guess
>> you can just ignore this comment.  However, if you can think of any
>> better alias than "qup_1" that makes this less confusing then that
>> would make me extra happy.  Like maybe "qupv3_id_1" to match the
>> manual?
> This is indeed a source of confusion, in particular since there tend to
> be different numbering depending on what part of the puzzle you look at.
> But you're right that each QUP has a number of engines, each one being a
> UART/I2C/SPI controller.
> 
> I don't see a reason for labeling this particular node though, aliases
> references individual engines, not the wrapper.

makes sense, I'll just drop the label.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support

[Bjorn Andersson]

On Tue 13 Feb 16:32 PST 2018, Doug Anderson wrote:
> On Sun, Feb 11, 2018 at 10:28 PM, Rajendra Nayak  
> wrote:
[..]
> > + {
> > +   geni-se@ac {
> > +   serial@a84000 {
> > +   status = "okay";
> > +   };
> > +   };
> 
> If others at QC have already decided that they like the style above
> then it's OK with me, but I'd prefer instead (at the top level):
> 
> _uart2 {
>   status = "okay";
> };
> 
> ...then you don't need to replicate all the hierarchy.
> 
> > +   pinctrl@340 {
> 
> Similar here.  This could be:
> 
> _uart2_default {
>   pinconf {
> ...
>   }
> };
> 
> If you're upset about things being in a "random" order at the top
> level, you can still create commented sections in the "dts" file to
> organize things, but the above means that you don't end up tabbed in
> several levels of indentation for no reason.
> 

I prefer using the hierarchy to describe the relationship between
sibling nodes, in favour of using comments and code review to keep
things in order.

This also mean that nodes that are not references by other parts of the
tree does not need a label.

That said, I've promised to write some patches to convert the prior
platforms to this structure, so let's see how that turns out in
practice - although it's still just an indication of what a fully
described board would look like.

> 
> > +   qup-uart2-default {
> > +   pinconf {
> > +   pins = "gpio4", "gpio5";
> > +   drive-strength = <2>;
> > +   bias-disable;
> 
> Possibly you'd want some sort of pull on the "receive" pin unless
> you're guaranteed that on this board that the other side will always
> be driving the pin.  As far as I can tell this UART goes to a debug
> connector.  If that debug connector is not populated this pin will be
> floating, no?
> 

The rx pin is typically bias-pull-up and tx bias-disable, so I would
expect the same.

[..]
> > +
> > +   qup_1: geni-se@ac {
> 
> Color me confused.  So you're saying here that this is "qup_1".
> ...but above you turn the pinmux for pins "GPIO4" and "GPIO5" to
> "qup9", right?  So UART2 is on "qup 1" and "qup 9"?
> 
> ...OK, so I stared at manuals a bunch more, and _maybe_ I get it.
> Maybe there are 3 "QUP v3 modules" each of which handles up to 8
> "QUP"s.  So QUP 9 is on "QUP module 1", is that right?  If everyone
> understands this already and it's just me that's confused then I guess
> you can just ignore this comment.  However, if you can think of any
> better alias than "qup_1" that makes this less confusing then that
> would make me extra happy.  Like maybe "qupv3_id_1" to match the
> manual?

This is indeed a source of confusion, in particular since there tend to
be different numbering depending on what part of the puzzle you look at.
But you're right that each QUP has a number of engines, each one being a
UART/I2C/SPI controller.

I don't see a reason for labeling this particular node though, aliases
references individual engines, not the wrapper.

Regards,
Bjorn

Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support

[Bjorn Andersson]

On Tue 13 Feb 16:32 PST 2018, Doug Anderson wrote:
> On Sun, Feb 11, 2018 at 10:28 PM, Rajendra Nayak  
> wrote:
[..]
> > + {
> > +   geni-se@ac {
> > +   serial@a84000 {
> > +   status = "okay";
> > +   };
> > +   };
> 
> If others at QC have already decided that they like the style above
> then it's OK with me, but I'd prefer instead (at the top level):
> 
> _uart2 {
>   status = "okay";
> };
> 
> ...then you don't need to replicate all the hierarchy.
> 
> > +   pinctrl@340 {
> 
> Similar here.  This could be:
> 
> _uart2_default {
>   pinconf {
> ...
>   }
> };
> 
> If you're upset about things being in a "random" order at the top
> level, you can still create commented sections in the "dts" file to
> organize things, but the above means that you don't end up tabbed in
> several levels of indentation for no reason.
> 

I prefer using the hierarchy to describe the relationship between
sibling nodes, in favour of using comments and code review to keep
things in order.

This also mean that nodes that are not references by other parts of the
tree does not need a label.

That said, I've promised to write some patches to convert the prior
platforms to this structure, so let's see how that turns out in
practice - although it's still just an indication of what a fully
described board would look like.

> 
> > +   qup-uart2-default {
> > +   pinconf {
> > +   pins = "gpio4", "gpio5";
> > +   drive-strength = <2>;
> > +   bias-disable;
> 
> Possibly you'd want some sort of pull on the "receive" pin unless
> you're guaranteed that on this board that the other side will always
> be driving the pin.  As far as I can tell this UART goes to a debug
> connector.  If that debug connector is not populated this pin will be
> floating, no?
> 

The rx pin is typically bias-pull-up and tx bias-disable, so I would
expect the same.

[..]
> > +
> > +   qup_1: geni-se@ac {
> 
> Color me confused.  So you're saying here that this is "qup_1".
> ...but above you turn the pinmux for pins "GPIO4" and "GPIO5" to
> "qup9", right?  So UART2 is on "qup 1" and "qup 9"?
> 
> ...OK, so I stared at manuals a bunch more, and _maybe_ I get it.
> Maybe there are 3 "QUP v3 modules" each of which handles up to 8
> "QUP"s.  So QUP 9 is on "QUP module 1", is that right?  If everyone
> understands this already and it's just me that's confused then I guess
> you can just ignore this comment.  However, if you can think of any
> better alias than "qup_1" that makes this less confusing then that
> would make me extra happy.  Like maybe "qupv3_id_1" to match the
> manual?

This is indeed a source of confusion, in particular since there tend to
be different numbering depending on what part of the puzzle you look at.
But you're right that each QUP has a number of engines, each one being a
UART/I2C/SPI controller.

I don't see a reason for labeling this particular node though, aliases
references individual engines, not the wrapper.

Regards,
Bjorn

Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support

[Rajendra Nayak]

On 02/14/2018 06:02 AM, Doug Anderson wrote:
> Hi,
> 
> On Sun, Feb 11, 2018 at 10:28 PM, Rajendra Nayak  
> wrote:
>> Add the qup uart node and geni se instance needed to
>> support the serial console on the MTP.
>>
>> Signed-off-by: Rajendra Nayak 
>> ---
>>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 34 
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi| 39 
>> +
>>  2 files changed, 73 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts 
>> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> index 617c7bb25fb1..9eab2b815e0d 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> @@ -10,4 +10,38 @@
>>  / {
>> model = "Qualcomm Technologies, Inc. SDM845 MTP";
>> compatible = "qcom,sdm845-mtp";
>> +
>> +   aliases {
>> +   serial0 = _uart2;
>> +   };
>> +
>> +   chosen {
>> +   stdout-path = "serial0";
>> +   };
>> +};
>> +
>> + {
>> +   geni-se@ac {
>> +   serial@a84000 {
>> +   status = "okay";
>> +   };
>> +   };
> 
> If others at QC have already decided that they like the style above
> then it's OK with me, but I'd prefer instead (at the top level):
> 
> _uart2 {
>   status = "okay";
> };
> 
> ...then you don't need to replicate all the hierarchy.
> 
>> +   pinctrl@340 {
> 
> Similar here.  This could be:
> 
> _uart2_default {
>   pinconf {
> ...
>   }
> };
> 
> If you're upset about things being in a "random" order at the top
> level, you can still create commented sections in the "dts" file to
> organize things, but the above means that you don't end up tabbed in
> several levels of indentation for no reason.

Yes, I kept it this way mainly because of Bjorn's concerns about things
being in random order.
Bjorn/Andy, any thoughts?

> 
> 
>> +   qup-uart2-default {
>> +   pinconf {
>> +   pins = "gpio4", "gpio5";
>> +   drive-strength = <2>;
>> +   bias-disable;
> 
> Possibly you'd want some sort of pull on the "receive" pin unless
> you're guaranteed that on this board that the other side will always
> be driving the pin.  As far as I can tell this UART goes to a debug
> connector.  If that debug connector is not populated this pin will be
> floating, no?
> 
> 
>> +   };
>> +   };
>> +
>> +   qup-uart2-sleep {
>> +   pinconf {
>> +   pins = "gpio4", "gpio5";
>> +   drive-strength = <2>;
> 
> Does specifying the "drive-strength" in this case actually do anything
> useful?  If not can we leave it out?
> 
> 
>> +   bias-disable;
> 
> Feel free to ignore if I'm being ignorant, but I would have expected a
> "pull" of some sort to be turned on for the "transmit" pin when you're
> in sleep mode.  Otherwise the line will be left floating which could
> generate noise to the other side, no?  ...or is there some sort of
> external pull present on this board?
> 
> Depending on the board you might also want a pull on the "receive" pin
> (assuming we wanted one in the "default" state--see above).  With my
> extremely limited EE understanding I have the impression that
> transitions on a line can still cause power draw even if software
> isn't paying attention to them, so it's best to prevent them by adding
> a pull.

What you are suggesting seems to make sense, however, given I blindly
pulled these in from the internal kernels, I am looping in Karthik/Girish
if they have anything to chime in.

> 
> 
>> +   };
>> +   };
>> +   };
>>  };
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
>> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 55a7e0b454e1..8cf8df25b06d 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -4,6 +4,7 @@
>>   */
>>
>>  #include 
>> +#include 
>>
>>  / {
>> interrupt-parent = <>;
>> @@ -193,6 +194,20 @@
>> #gpio-cells = <2>;
>> interrupt-controller;
>> #interrupt-cells = <2>;
>> +
>> +   qup_uart2_default: qup-uart2-default {
>> +   pinmux {
>> +   function = "qup9";
>> +   pins = "gpio4", "gpio5";
>> +   };
>> +   };
>> +
>> +   qup_uart2_sleep: qup-uart2-sleep {
>> +   pinmux {
>> +   function = "gpio";
>> +   pins = "gpio4", "gpio5";
>> +   };
>> + 

Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support

[Rajendra Nayak]

On 02/14/2018 06:02 AM, Doug Anderson wrote:
> Hi,
> 
> On Sun, Feb 11, 2018 at 10:28 PM, Rajendra Nayak  
> wrote:
>> Add the qup uart node and geni se instance needed to
>> support the serial console on the MTP.
>>
>> Signed-off-by: Rajendra Nayak 
>> ---
>>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 34 
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi| 39 
>> +
>>  2 files changed, 73 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts 
>> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> index 617c7bb25fb1..9eab2b815e0d 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
>> @@ -10,4 +10,38 @@
>>  / {
>> model = "Qualcomm Technologies, Inc. SDM845 MTP";
>> compatible = "qcom,sdm845-mtp";
>> +
>> +   aliases {
>> +   serial0 = _uart2;
>> +   };
>> +
>> +   chosen {
>> +   stdout-path = "serial0";
>> +   };
>> +};
>> +
>> + {
>> +   geni-se@ac {
>> +   serial@a84000 {
>> +   status = "okay";
>> +   };
>> +   };
> 
> If others at QC have already decided that they like the style above
> then it's OK with me, but I'd prefer instead (at the top level):
> 
> _uart2 {
>   status = "okay";
> };
> 
> ...then you don't need to replicate all the hierarchy.
> 
>> +   pinctrl@340 {
> 
> Similar here.  This could be:
> 
> _uart2_default {
>   pinconf {
> ...
>   }
> };
> 
> If you're upset about things being in a "random" order at the top
> level, you can still create commented sections in the "dts" file to
> organize things, but the above means that you don't end up tabbed in
> several levels of indentation for no reason.

Yes, I kept it this way mainly because of Bjorn's concerns about things
being in random order.
Bjorn/Andy, any thoughts?

> 
> 
>> +   qup-uart2-default {
>> +   pinconf {
>> +   pins = "gpio4", "gpio5";
>> +   drive-strength = <2>;
>> +   bias-disable;
> 
> Possibly you'd want some sort of pull on the "receive" pin unless
> you're guaranteed that on this board that the other side will always
> be driving the pin.  As far as I can tell this UART goes to a debug
> connector.  If that debug connector is not populated this pin will be
> floating, no?
> 
> 
>> +   };
>> +   };
>> +
>> +   qup-uart2-sleep {
>> +   pinconf {
>> +   pins = "gpio4", "gpio5";
>> +   drive-strength = <2>;
> 
> Does specifying the "drive-strength" in this case actually do anything
> useful?  If not can we leave it out?
> 
> 
>> +   bias-disable;
> 
> Feel free to ignore if I'm being ignorant, but I would have expected a
> "pull" of some sort to be turned on for the "transmit" pin when you're
> in sleep mode.  Otherwise the line will be left floating which could
> generate noise to the other side, no?  ...or is there some sort of
> external pull present on this board?
> 
> Depending on the board you might also want a pull on the "receive" pin
> (assuming we wanted one in the "default" state--see above).  With my
> extremely limited EE understanding I have the impression that
> transitions on a line can still cause power draw even if software
> isn't paying attention to them, so it's best to prevent them by adding
> a pull.

What you are suggesting seems to make sense, however, given I blindly
pulled these in from the internal kernels, I am looping in Karthik/Girish
if they have anything to chime in.

> 
> 
>> +   };
>> +   };
>> +   };
>>  };
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
>> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index 55a7e0b454e1..8cf8df25b06d 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -4,6 +4,7 @@
>>   */
>>
>>  #include 
>> +#include 
>>
>>  / {
>> interrupt-parent = <>;
>> @@ -193,6 +194,20 @@
>> #gpio-cells = <2>;
>> interrupt-controller;
>> #interrupt-cells = <2>;
>> +
>> +   qup_uart2_default: qup-uart2-default {
>> +   pinmux {
>> +   function = "qup9";
>> +   pins = "gpio4", "gpio5";
>> +   };
>> +   };
>> +
>> +   qup_uart2_sleep: qup-uart2-sleep {
>> +   pinmux {
>> +   function = "gpio";
>> +   pins = "gpio4", "gpio5";
>> +   };
>> +   };
>> };
>>
>>  

Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support

[Doug Anderson]

Hi,

On Sun, Feb 11, 2018 at 10:28 PM, Rajendra Nayak  wrote:
> Add the qup uart node and geni se instance needed to
> support the serial console on the MTP.
>
> Signed-off-by: Rajendra Nayak 
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 34 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi| 39 
> +
>  2 files changed, 73 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts 
> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 617c7bb25fb1..9eab2b815e0d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -10,4 +10,38 @@
>  / {
> model = "Qualcomm Technologies, Inc. SDM845 MTP";
> compatible = "qcom,sdm845-mtp";
> +
> +   aliases {
> +   serial0 = _uart2;
> +   };
> +
> +   chosen {
> +   stdout-path = "serial0";
> +   };
> +};
> +
> + {
> +   geni-se@ac {
> +   serial@a84000 {
> +   status = "okay";
> +   };
> +   };

If others at QC have already decided that they like the style above
then it's OK with me, but I'd prefer instead (at the top level):

_uart2 {
  status = "okay";
};

...then you don't need to replicate all the hierarchy.

> +   pinctrl@340 {

Similar here.  This could be:

_uart2_default {
  pinconf {
...
  }
};

If you're upset about things being in a "random" order at the top
level, you can still create commented sections in the "dts" file to
organize things, but the above means that you don't end up tabbed in
several levels of indentation for no reason.


> +   qup-uart2-default {
> +   pinconf {
> +   pins = "gpio4", "gpio5";
> +   drive-strength = <2>;
> +   bias-disable;

Possibly you'd want some sort of pull on the "receive" pin unless
you're guaranteed that on this board that the other side will always
be driving the pin.  As far as I can tell this UART goes to a debug
connector.  If that debug connector is not populated this pin will be
floating, no?


> +   };
> +   };
> +
> +   qup-uart2-sleep {
> +   pinconf {
> +   pins = "gpio4", "gpio5";
> +   drive-strength = <2>;

Does specifying the "drive-strength" in this case actually do anything
useful?  If not can we leave it out?


> +   bias-disable;

Feel free to ignore if I'm being ignorant, but I would have expected a
"pull" of some sort to be turned on for the "transmit" pin when you're
in sleep mode.  Otherwise the line will be left floating which could
generate noise to the other side, no?  ...or is there some sort of
external pull present on this board?

Depending on the board you might also want a pull on the "receive" pin
(assuming we wanted one in the "default" state--see above).  With my
extremely limited EE understanding I have the impression that
transitions on a line can still cause power draw even if software
isn't paying attention to them, so it's best to prevent them by adding
a pull.


> +   };
> +   };
> +   };
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 55a7e0b454e1..8cf8df25b06d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -4,6 +4,7 @@
>   */
>
>  #include 
> +#include 
>
>  / {
> interrupt-parent = <>;
> @@ -193,6 +194,20 @@
> #gpio-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
> +
> +   qup_uart2_default: qup-uart2-default {
> +   pinmux {
> +   function = "qup9";
> +   pins = "gpio4", "gpio5";
> +   };
> +   };
> +
> +   qup_uart2_sleep: qup-uart2-sleep {
> +   pinmux {
> +   function = "gpio";
> +   pins = "gpio4", "gpio5";
> +   };
> +   };
> };
>
> timer@17c9 {
> @@ -271,5 +286,29 @@
> #interrupt-cells = <4>;
> cell-index = <0>;
> };
> +
> +   qup_1: geni-se@ac {

Color me confused.  So you're saying here that this is "qup_1".
...but above you turn the pinmux for pins "GPIO4" and "GPIO5" to
"qup9", right?  So UART2 is on "qup 1" and "qup 9"?

...OK, so I stared at manuals a bunch more, and _maybe_ I get it.
Maybe there are 3 "QUP v3 modules" each of which 

Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support

[Doug Anderson]

Hi,

On Sun, Feb 11, 2018 at 10:28 PM, Rajendra Nayak  wrote:
> Add the qup uart node and geni se instance needed to
> support the serial console on the MTP.
>
> Signed-off-by: Rajendra Nayak 
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 34 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi| 39 
> +
>  2 files changed, 73 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts 
> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 617c7bb25fb1..9eab2b815e0d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -10,4 +10,38 @@
>  / {
> model = "Qualcomm Technologies, Inc. SDM845 MTP";
> compatible = "qcom,sdm845-mtp";
> +
> +   aliases {
> +   serial0 = _uart2;
> +   };
> +
> +   chosen {
> +   stdout-path = "serial0";
> +   };
> +};
> +
> + {
> +   geni-se@ac {
> +   serial@a84000 {
> +   status = "okay";
> +   };
> +   };

If others at QC have already decided that they like the style above
then it's OK with me, but I'd prefer instead (at the top level):

_uart2 {
  status = "okay";
};

...then you don't need to replicate all the hierarchy.

> +   pinctrl@340 {

Similar here.  This could be:

_uart2_default {
  pinconf {
...
  }
};

If you're upset about things being in a "random" order at the top
level, you can still create commented sections in the "dts" file to
organize things, but the above means that you don't end up tabbed in
several levels of indentation for no reason.


> +   qup-uart2-default {
> +   pinconf {
> +   pins = "gpio4", "gpio5";
> +   drive-strength = <2>;
> +   bias-disable;

Possibly you'd want some sort of pull on the "receive" pin unless
you're guaranteed that on this board that the other side will always
be driving the pin.  As far as I can tell this UART goes to a debug
connector.  If that debug connector is not populated this pin will be
floating, no?


> +   };
> +   };
> +
> +   qup-uart2-sleep {
> +   pinconf {
> +   pins = "gpio4", "gpio5";
> +   drive-strength = <2>;

Does specifying the "drive-strength" in this case actually do anything
useful?  If not can we leave it out?


> +   bias-disable;

Feel free to ignore if I'm being ignorant, but I would have expected a
"pull" of some sort to be turned on for the "transmit" pin when you're
in sleep mode.  Otherwise the line will be left floating which could
generate noise to the other side, no?  ...or is there some sort of
external pull present on this board?

Depending on the board you might also want a pull on the "receive" pin
(assuming we wanted one in the "default" state--see above).  With my
extremely limited EE understanding I have the impression that
transitions on a line can still cause power draw even if software
isn't paying attention to them, so it's best to prevent them by adding
a pull.


> +   };
> +   };
> +   };
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 55a7e0b454e1..8cf8df25b06d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -4,6 +4,7 @@
>   */
>
>  #include 
> +#include 
>
>  / {
> interrupt-parent = <>;
> @@ -193,6 +194,20 @@
> #gpio-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
> +
> +   qup_uart2_default: qup-uart2-default {
> +   pinmux {
> +   function = "qup9";
> +   pins = "gpio4", "gpio5";
> +   };
> +   };
> +
> +   qup_uart2_sleep: qup-uart2-sleep {
> +   pinmux {
> +   function = "gpio";
> +   pins = "gpio4", "gpio5";
> +   };
> +   };
> };
>
> timer@17c9 {
> @@ -271,5 +286,29 @@
> #interrupt-cells = <4>;
> cell-index = <0>;
> };
> +
> +   qup_1: geni-se@ac {

Color me confused.  So you're saying here that this is "qup_1".
...but above you turn the pinmux for pins "GPIO4" and "GPIO5" to
"qup9", right?  So UART2 is on "qup 1" and "qup 9"?

...OK, so I stared at manuals a bunch more, and _maybe_ I get it.
Maybe there are 3 "QUP v3 modules" each of which handles up to 8
"QUP"s.  So QUP 9 is on "QUP