Hi Matthias, thanks for the review
On 10/22/2019 5:38 AM, Matthias Kaehlcke wrote:
Hi Rajendra,
I don't have all the hardware documentation for a full review, but
find a few comments inline.
[]..
+#include "sc7180.dtsi"
+
+/ {
+ model = "Qualcomm Technologies, Inc. SC7180 IDP";
+ compatible = "qcom,sc7180-idp";
+
+ aliases {
+ serial0 = &uart2;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+};
+
+&qupv3_id_0 {
+ status = "okay";
+};
+
+&uart2 {
+ status = "okay";
+};
+
+/* PINCTRL - additions to nodes defined in sc7180.dtsi */
+
+&qup_uart2_default {
+ pinconf-tx {
+ pins = "gpio44";
+ drive-strength = <2>;
+ bias-disable;
+ };
+
+ pinconf-rx {
+ pins = "gpio45";
+ drive-strength = <2>;
+ bias-pull-up;
+ };
+};
This config seems reasonable as default for a UART in general.
Would it make sense to configure these in the SoC .dtsi?
I think the general rule of thumb that was followed was to have
all pinmux configurations in soc file and all pinconf setting in
the board, even though it meant a bit of duplication in some cases.
See [1] for some discussions around it that happened in the past.
[1] https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg1603693.html
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
new file mode 100644
index 000000000000..82bf7cdce6b8
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -0,0 +1,300 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * SC7180 SoC device tree source
+ *
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include <dt-bindings/clock/qcom,gcc-sc7180.h>
Note: depends on "Add Global Clock controller (GCC) driver for SC7180"
(https://patchwork.kernel.org/project/linux-arm-msm/list/?submitter=179717)
which isn't merged yet.
Right, I did mention it in the cover letter, perhaps I should have mentioned it
as part of this patch as well.
[]..
+
+ soc: soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges = <0 0 0 0 0x10 0>;
+ dma-ranges = <0 0 0 0 0x10 0>;
+ compatible = "simple-bus";
+
+ gcc: clock-controller@100000 {
+ compatible = "qcom,gcc-sc7180";
+ reg = <0 0x00100000 0 0x1f0000>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ #power-domain-cells = <1>;
+ };
+
+ qupv3_id_0: geniqup@ac0000 {
The QUP enumeration is a bit confusing. The Hardware Register
Description has QUPV3_0_QUPV3_ID_0 at 0x00800000 and
QUPV3_1_QUPV3_ID_0 at 0x00a00000. This QUP apparently is
the latter. In the SDM845 DT the QUP @ac0000 has the label
'qupv3_id_1', I guess this should be the same here.
I had a re-look at the documentation again and yes, you are
right, this seems exactly same as on sdm845 except that on
sdm845 the 2 blocks were named QUPV3_0_QUPV3_ID_1 at 0x00800000
and QUPV3_1_QUPV3_ID_1 at 0x00a00000.
I will match this up with the labeling approach we followed on
sdm845. Thanks.
+ compatible = "qcom,geni-se-qup";
+ reg = <0 0x00ac0000 0 0x6000>;
+ clock-names = "m-ahb", "s-ahb";
+ clocks = <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>,
+ <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+ status = "disabled";
+
+ uart2: serial@a88000 {
+ compatible = "qcom,geni-debug-uart";
+ reg = <0 0x00a88000 0 0x4000>;
Related to the comment above: on SDM845 this UART has the label
'uart10'. I understand these are different SoCs, but could you
please clarify the enumeration of the SC7180 QUPs and their ports?
I will move this to uart10 once I have the qup instance marked with id_1.
On sdm845 the qup_id_0 had SE instances from 0 to 7 and qup_id_1 had it
from 8 to 15. I will follow the same here so this uart instance would
remain the same as on sdm845, which is uart10.
thanks,
Rajendra
+ clock-names = "se";
+ clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&qup_uart2_default>;
+ interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
+ status = "disabled";
+ };
+ };
+
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
