On 02/14/2018 06:02 AM, Doug Anderson wrote:
> Hi,
>
> On Sun, Feb 11, 2018 at 10:28 PM, Rajendra Nayak <rnayak@xxxxxxxxxxxxxx> wrote:
>> Add the qup uart node and geni se instance needed to
>> support the serial console on the MTP.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
>> ---
>> 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 = &qup_uart2;
>> + };
>> +
>> + chosen {
>> + stdout-path = "serial0";
>> + };
>> +};
>> +
>> +&soc {
>> + geni-se@ac0000 {
>> + 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):
>
> &qup_uart2 {
> status = "okay";
> };
>
> ...then you don't need to replicate all the hierarchy.
>
>> + pinctrl@3400000 {
>
> Similar here. This could be:
>
> &qup_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 <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/clock/qcom,gcc-sdm845.h>
>>
>> / {
>> interrupt-parent = <&intc>;
>> @@ -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@17c90000 {
>> @@ -271,5 +286,29 @@
>> #interrupt-cells = <4>;
>> cell-index = <0>;
>> };
>> +
>> + qup_1: geni-se@ac0000 {
>
> 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?
So FWIK, its one QUP with 8 SE instances, and we have 2 such QUP instances
in SDM845. So yes, I should probably name it qupv3_id_1 to avoid confusion.
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
