Add DTS for SDM845 SoC and MTP
v3 cover Add DTS for SDM845 SoC and MTP
These are basic device tree files needed to boot a SDM845 MTP board to a ramfs based serial console shell
Bindings are based on whats proposed for pinctrl/serial/clock drivers for SDM845 SoC
pinctrl: https://patchwork.kernel.org/patch/10157143/ (This is now pulled in by Linus Walleij for 4.17)
clocks: https://lkml.org/lkml/2018/1/31/209 (under review)
serial: https://patchwork.ozlabs.org/cover/860251/ (under review)
'PATCH 3/3' is based on v2 of serial patches, will need an update if v3 (still in the works) has further binding updates
Since 'PATCH 2/3' also adds an ITS node and keeps it disabled, we also depend on https://lkml.org/lkml/2018/1/29/383
changes in v3:
* split the pinmux/pinconf nodes across SoC/Board files
* Fixes for issues reported with 'make dtbs W=2'
* other minor fixes based on review
changes in v2:
* dropped cpu-map
* dropped GIC_CPU_MASK_SIMPLE()
* Added new cpu compatible for kryo385
* added ITS node, marked as disabled
* other cosmetic fixes
Rajendra Nayak (3):
dt-bindings: arm: Document kryo385 cpu
arm64: dts: sdm845: Add minimal dts files for sdm845 SoC/MTP
arm64: dts: sdm845: Add serial console support
Documentation/devicetree/bindings/arm/cpus.txt | 1 + arch/arm64/boot/dts/qcom/Makefile | 1 + arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 47 ++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 314 +++++++++++++++++++++++++ 4 files changed, 363 insertions(+) create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsi
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v3 1/3 dt-bindings: arm: Document kryo385 cpu
Document the compatible string for the Kryo385 cpus found in qualcomm SoCs.
--- Documentation/devicetree/bindings/arm/cpus.txt | 1 + 1 file changed, 1 insertion(+)
Documentation/devicetree/bindings/arm/cpus.txt +1 -0
@@ -184,6 +184,7 @@ described below. "nvidia,tegra186-denver" "qcom,krait" "qcom,kryo" + "qcom,kryo385" "qcom,scorpion" - enable-method Value type: <stringlist>
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v3 2/3 arm64: dts: sdm845: Add minimal dts files for sdm845 SoC/MTP
In reply to Rajendra NayakAdd a skeletal sdm845 SoC dtsi and MTP board dts/dtsi files
--- arch/arm64/boot/dts/qcom/Makefile | 1 + arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 13 ++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 275 ++++++++++++++++++++++++++++++++ 3 files changed, 289 insertions(+) create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsi
arch/arm64/boot/dts/qcom/Makefile +1 -0
@@ -6,3 +6,4 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8916-mtp.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8992-bullhead-rev-101.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8994-angler-rev-101.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb +dtb-$(CONFIG_ARCH_QCOM) += sdm845-mtp.dtb
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +13 -0
@@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018, The Linux Foundation. All rights reserved. + */ + +/dts-v1/; + +#include "sdm845.dtsi" + +/ { + model = "Qualcomm Technologies, Inc. SDM845 MTP"; + compatible = "qcom,sdm845-mtp"; +};
arch/arm64/boot/dts/qcom/sdm845.dtsi new file +275 -0
275 lines not on this page · raw message
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v3 2/3] arm64: dts: sdm845: Add minimal dts files for sdm845 SoC/MTP
Quoted 26 lines · Rajendra Nayak
Add a skeletal sdm845 SoC dtsi and MTP board dts/dtsi files
Signed-off-by Rajendra Nayak <[email protected]>--- arch/arm64/boot/dts/qcom/Makefile | 1 + arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 13 ++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 275 ++++++++++++++++++++++++++++++++ 3 files changed, 289 insertions(+) create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsiarch/arm64/boot/dts/qcom/Makefile +1
-0@@ -6,3 +6,4 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8916-mtp.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8992-bullhead-rev-101.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8994-angler-rev-101.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb +dtb-$(CONFIG_ARCH_QCOM) += sdm845-mtp.dtbarch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +0
-0
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 4
Quoted 4 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +4
-0@@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018, The Linux Foundation. All rights reserved. + */
This is more cosmetic, but since there is only a single line of copyright statement and no other comments, it would make sense to use C++ style // for that line too IMHO (and other similar cases)
Signature · 2 lines
Cordially Philippe Ombredanne
Re: [PATCH v3 2/3] arm64: dts: sdm845: Add minimal dts files for sdm845 SoC/MTP
In reply to Rajendra NayakHi,
Quoted 26 lines · Rajendra Nayak
Add a skeletal sdm845 SoC dtsi and MTP board dts/dtsi files
Signed-off-by Rajendra Nayak <[email protected]>--- arch/arm64/boot/dts/qcom/Makefile | 1 + arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 13 ++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 275 ++++++++++++++++++++++++++++++++ 3 files changed, 289 insertions(+) create mode 100644 arch/arm64/boot/dts/qcom/sdm845-mtp.dts create mode 100644 arch/arm64/boot/dts/qcom/sdm845.dtsiarch/arm64/boot/dts/qcom/Makefile +1
-0@@ -6,3 +6,4 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8916-mtp.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8992-bullhead-rev-101.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8994-angler-rev-101.dtb dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb +dtb-$(CONFIG_ARCH_QCOM) += sdm845-mtp.dtbarch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +0
-0
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 1
Quoted 1 line
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +1
-0@@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0
It would, of course, be up to Qualcomm. ...but might I suggest instead:
// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
The device tree files really don't have any special secret sauce in them and IIRC allowing them to have a more permissive MIT license _or_ a GPL allowed people to run other operating systems on these boards. This kind of thing is better to fix now so we don't have to go and get everyone's permission later on.
In the past we went through this with Rockchip SoCs in commit b1772506206f ("ARM: dts: rockchip: relicense rk3288.dtsi under GPLv2/X11").
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 4
Quoted 3 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +3
-0+/* + * Copyright (c) 2018, The Linux Foundation. All rights reserved. + */
IMHO add an extra line to this comment with a description to avoid the bike shedding of how we're supposed to do 1-line comments in device tree files. AKA:
/*
* SDM845 MTP board device tree source
*
* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 12
Quoted 8 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +8
-0+ +/dts-v1/; + +#include "sdm845.dtsi" + +/ { + model = "Qualcomm Technologies, Inc. SDM845 MTP"; + compatible = "qcom,sdm845-mtp";
For me checkpatch complains about this. It looks like the file "Documentation/devicetree/bindings/arm/qcom.txt" needs to be updated with "sdm845". I don't think that will make checkpatch be quiet (since that file doesn't have a full list of every board), but it still should be the correct thing to do.
arch/arm64/boot/dts/qcom/sdm845.dtsi line 1
Quoted 8 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +1
-0+};arch/arm64/boot/dts/qcom/sdm845.dtsi new file +1
-0@@ -0,0 +1,275 @@ +// SPDX-License-Identifier: GPL-2.0
As per above, suggest dual licensed?
arch/arm64/boot/dts/qcom/sdm845.dtsi line 3
Quoted 2 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi new file +2
-0+/* + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
As per above, suggest adding an extra line to avoid the bikeshed.
SDM845 SoC device tree source
Besides those things, everything looks good as far as I can see. I'm not an expert on every one of the devices used in this file, but reading through bindings docs and looking at other users of them, it looks sane enough. Thus, with the above nits fixed you can feel free to add my Reviewed-by.
-Doug
Re: [PATCH v3 2/3] arm64: dts: sdm845: Add minimal dts files for sdm845 SoC/MTP
[]..
Quoted 10 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 1
Quoted 7 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +1
-0@@ -0,0 +1,13 @@ +// SPDX-License-Identifier: GPL-2.0It would, of course, be up to Qualcomm. ...but might I suggest instead:
Quoted 7 lines
// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
The device tree files really don't have any special secret sauce in them and IIRC allowing them to have a more permissive MIT license _or_ a GPL allowed people to run other operating systems on these boards. This kind of thing is better to fix now so we don't have to go and get everyone's permission later on.
sure, sounds reasonable, but I am told I need to get this reviewed once by qualcomm legal before we go ahead and use the dual licensing copyright. I will update based on what I hear.
[]..
Quoted 9 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 4
Quoted 4 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +3
-0+/* + * Copyright (c) 2018, The Linux Foundation. All rights reserved. + */IMHO add an extra line to this comment with a description to avoid the bike shedding of how we're supposed to do 1-line comments in device tree files. AKA:
Quoted 5 lines
/*
* SDM845 MTP board device tree source
*
* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
sure will update.
Quoted 16 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 12
Quoted 10 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +8
-0+ +/dts-v1/; + +#include "sdm845.dtsi" + +/ { + model = "Qualcomm Technologies, Inc. SDM845 MTP"; + compatible = "qcom,sdm845-mtp";For me checkpatch complains about this. It looks like the file "
Documentation/devicetree/bindings/arm/qcom.txt" needs to be updated with "sdm845". I don't think that will make checkpatch be quiet (since that file doesn't have a full list of every board), but it still should be the correct thing to do.
sure, I missed updating it, will fix.
Quoted 19 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi line 1
Quoted 10 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts new file +1
-0+};arch/arm64/boot/dts/qcom/sdm845.dtsi new file +1
-0@@ -0,0 +1,275 @@ +// SPDX-License-Identifier: GPL-2.0As per above, suggest dual licensed?
arch/arm64/boot/dts/qcom/sdm845.dtsi line 3
Quoted 2 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi new file +2
-0+/* + * Copyright (c) 2018, The Linux Foundation. All rights reserved.As per above, suggest adding an extra line to avoid the bikeshed.
Quoted 8 lines
SDM845 SoC device tree source
Besides those things, everything looks good as far as I can see. I'm not an expert on every one of the devices used in this file, but reading through bindings docs and looking at other users of them, it looks sane enough. Thus, with the above nits fixed you can feel free to add my Reviewed-by.
Thanks, the only thing I need to wait for before I respin this is to get a go ahead from legal for the dual licensing copyright header.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v3 3/3 arm64: dts: sdm845: Add serial console support
In reply to Rajendra NayakAdd the qup uart node and geni se instance needed to support the serial console on the MTP.
--- arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 34 ++++++++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 39 +++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+)
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +34 -0
@@ -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 at ac0000 { + serial at a84000 { + status = "okay"; + }; + }; + + pinctrl at 3400000 { + qup-uart2-default { + pinconf { + pins = "gpio4", "gpio5"; + drive-strength = <2>; + bias-disable; + }; + }; + + qup-uart2-sleep { + pinconf { + pins = "gpio4", "gpio5"; + drive-strength = <2>; + bias-disable; + }; + }; + }; };
arch/arm64/boot/dts/qcom/sdm845.dtsi +39 -0
@@ -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 at 17c90000 {
@@ -271,5 +286,29 @@ #interrupt-cells = <4>; cell-index = <0>; }; + + qup_1: geni-se at ac0000 { + compatible = "qcom,geni-se-qup"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + reg = <0xac0000 0x6000>; + clocks = <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, + <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>; + clock-names = "m-ahb", "s-ahb"; + + qup_uart2: serial at a84000 { + compatible = "qcom,geni-debug-uart"; + reg = <0xa84000 0x4000>; + reg-names = "se-phys"; + clock-names = "se-clk"; + clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>; + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&qup_uart2_default>; + pinctrl-1 = <&qup_uart2_sleep>; + interrupts = <GIC_SPI 354 IRQ_TYPE_NONE>; + status = "disabled"; + }; + }; }; };
Signature · 2 lines
QUALCOMM INDIA, on behalf of 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
Hi,
Quoted 23 lines · Rajendra Nayak
Add the qup uart node and geni se instance needed to support the serial console on the MTP.
Signed-off-by Rajendra Nayak <[email protected]>--- arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 34 ++++++++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 39 +++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+)arch/arm64/boot/dts/qcom/sdm845-mtp.dts +6
-0@@ -10,4 +10,38 @@ / { model = "Qualcomm Technologies, Inc. SDM845 MTP"; compatible = "qcom,sdm845-mtp"; + + aliases { + serial0 = &qup_uart2; + }; + + chosen {
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 28
Quoted 10 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +10
-0+ stdout-path = "serial0"; + }; +}; + +&soc { + geni-se at ac0000 { + serial at 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.
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 30
Quoted 1 line
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +1
-0+ pinctrl at 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.
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 35
Quoted 5 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +5
-0+ 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?
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 42
Quoted 7 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +7
-0+ }; + }; + + 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?
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 42
Quoted 1 line
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +1
-0+ 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.
Quoted 38 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +3
-0+ }; + }; + }; };arch/arm64/boot/dts/qcom/sdm845.dtsi +15
-0@@ -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 at 17c90000 {
arch/arm64/boot/dts/qcom/sdm845.dtsi line 290
Quoted 5 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi +2
-0@@ -271,5 +286,29 @@ #interrupt-cells = <4>; cell-index = <0>; }; + + qup_1: geni-se at 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?
Re: [PATCH v3 3/3] arm64: dts: sdm845: Add serial console support
Quoted 53 lines · Doug Anderson
Hi,
Quoted 23 lines · Rajendra Nayak
Add the qup uart node and geni se instance needed to support the serial console on the MTP.
Signed-off-by Rajendra Nayak <[email protected]>--- arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 34 ++++++++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 39 +++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+)arch/arm64/boot/dts/qcom/sdm845-mtp.dts +6
-0@@ -10,4 +10,38 @@ / { model = "Qualcomm Technologies, Inc. SDM845 MTP"; compatible = "qcom,sdm845-mtp"; + + aliases { + serial0 = &qup_uart2; + }; + + chosen {arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 28
Quoted 10 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +10
-0+ stdout-path = "serial0"; + }; +}; + +&soc { + geni-se at ac0000 { + serial at 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.
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 30
Quoted 1 line
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +1
-0+ pinctrl at 3400000 {Similar here. This could be:
&qup_uart2_default{pinconf { ...
Quoted 7 lines
}
};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?
Quoted 35 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 35
Quoted 7 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +5
-0+ 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?
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 42
Quoted 7 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +7
-0+ }; + }; + + 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?
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 42
Quoted 1 line
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +1
-0+ 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?
Quoted 6 lines
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.
Quoted 50 lines
Quoted 40 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +3
-0+ }; + }; + }; };arch/arm64/boot/dts/qcom/sdm845.dtsi +15
-0@@ -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 at 17c90000 {arch/arm64/boot/dts/qcom/sdm845.dtsi line 290
Quoted 5 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi +2
-0@@ -271,5 +286,29 @@ #interrupt-cells = <4>; cell-index = <0>; }; + + qup_1: geni-se at 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"?
Quoted 8 lines
...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.
Quoted 1 line
Signature · 2 lines
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
In reply to Doug AndersonQuoted 1 line · Doug Anderson
On Sun, Feb 11, 2018 at 10:28 PM, Rajendra Nayak <[email protected]> wrote:
[..]
Quoted 23 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 28
Quoted 6 lines
+&soc {
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +5
-0+ geni-se at ac0000 { + serial at 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.
Quoted 1 line
+ pinctrl at 3400000 {
Similar here. This could be:
&qup_uart2_default{pinconf { ...
Quoted 8 lines
}
};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.
Quoted 7 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts line 35
Quoted 6 lines
arch/arm64/boot/dts/qcom/sdm845-mtp.dts +5
-0+ qup-uart2-default { + pinconf { + pins = "gpio4", "gpio5"; + drive-strength = <2>; + bias-disable;
Quoted 6 lines
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.
[..]
Quoted 7 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi line 290
Quoted 2 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi +2
-0+ + qup_1: geni-se at 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"?
Quoted 8 lines
...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
Quoted 15 lines · Bjorn Andersson
[..]
Quoted 6 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi line 290
Quoted 2 lines
arch/arm64/boot/dts/qcom/sdm845.dtsi +2
-0+ + qup_1: geni-se at 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"?Quoted 8 lines
...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?
Quoted 7 lines
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.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation