| Message ID | 20191021065522.24511-1-rnayak@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add device tree support for sc7180 | expand |
On 2019-10-21 07:55, Rajendra Nayak wrote: > From: Maulik Shah <mkshah@codeaurora.org> > > Add sc7180 pdc irqchip > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > Cc: Lina Iyer <ilina@codeaurora.org> > Cc: Marc Zyngier <maz@kernel.org> > --- > v2: No change > > drivers/irqchip/qcom-pdc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index faa7d61b9d6c..954fb599fa9c 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > @@ -310,3 +310,4 @@ static int qcom_pdc_init(struct device_node > *node, struct device_node *parent) > } > > IRQCHIP_DECLARE(pdc_sdm845, "qcom,sdm845-pdc", qcom_pdc_init); > +IRQCHIP_DECLARE(pdc_sc7180, "qcom,sc7180-pdc", qcom_pdc_init); What I gather from these 3 irq-related patches is that as far as the PDC is concerned, SDM845/850 and SC7180 are strictly identical. Why the churn? M.
Hi Rajendra, I don't have all the hardware documentation for a full review, but find a few comments inline. On Mon, Oct 21, 2019 at 12:25:11PM +0530, Rajendra Nayak wrote: > Add skeletal sc7180 SoC dtsi and idp board dts files. > > Co-developed-by: Taniya Das <tdas@codeaurora.org> > Signed-off-by: Taniya Das <tdas@codeaurora.org> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > v2: > * Sorted the nodes in alphabetical order > * Dropped the bi_tcxo/bi_tcxo_ao nodes > > arch/arm64/boot/dts/qcom/Makefile | 1 + > arch/arm64/boot/dts/qcom/sc7180-idp.dts | 47 ++++ > arch/arm64/boot/dts/qcom/sc7180.dtsi | 300 ++++++++++++++++++++++++ > 3 files changed, 348 insertions(+) > create mode 100644 arch/arm64/boot/dts/qcom/sc7180-idp.dts > create mode 100644 arch/arm64/boot/dts/qcom/sc7180.dtsi > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > index 6498a1ec893f..7a5c2f7fe37f 100644 > --- a/arch/arm64/boot/dts/qcom/Makefile > +++ b/arch/arm64/boot/dts/qcom/Makefile > @@ -13,6 +13,7 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8998-asus-novago-tp370ql.dtb > dtb-$(CONFIG_ARCH_QCOM) += msm8998-hp-envy-x2.dtb > dtb-$(CONFIG_ARCH_QCOM) += msm8998-lenovo-miix-630.dtb > dtb-$(CONFIG_ARCH_QCOM) += msm8998-mtp.dtb > +dtb-$(CONFIG_ARCH_QCOM) += sc7180-idp.dtb > dtb-$(CONFIG_ARCH_QCOM) += sdm845-cheza-r1.dtb > dtb-$(CONFIG_ARCH_QCOM) += sdm845-cheza-r2.dtb > dtb-$(CONFIG_ARCH_QCOM) += sdm845-cheza-r3.dtb > diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > new file mode 100644 > index 000000000000..f8b7e098f5b4 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts > @@ -0,0 +1,47 @@ > +// SPDX-License-Identifier: BSD-3-Clause > +/* > + * SC7180 IDP board device tree source > + * > + * Copyright (c) 2019, The Linux Foundation. All rights reserved. > + */ > + > +/dts-v1/; > + > +#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? > 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. > +#include <dt-bindings/interrupt-controller/arm-gic.h> > + > +/ { > + interrupt-parent = <&intc>; > + > + #address-cells = <2>; > + #size-cells = <2>; > + > + chosen { }; > + > + clocks { > + xo_board: xo-board { > + compatible = "fixed-clock"; > + clock-frequency = <38400000>; > + clock-output-names = "xo_board"; > + #clock-cells = <0>; > + }; > + > + sleep_clk: sleep-clk { > + compatible = "fixed-clock"; > + clock-frequency = <32764>; > + clock-output-names = "sleep_clk"; > + #clock-cells = <0>; > + }; > + }; > + > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + CPU0: cpu@0 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x0>; > + enable-method = "psci"; > + next-level-cache = <&L2_0>; > + L2_0: l2-cache { > + compatible = "cache"; > + next-level-cache = <&L3_0>; > + L3_0: l3-cache { > + compatible = "cache"; > + }; > + }; > + }; > + > + CPU1: cpu@100 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x100>; > + enable-method = "psci"; > + next-level-cache = <&L2_100>; > + L2_100: l2-cache { > + compatible = "cache"; > + next-level-cache = <&L3_0>; > + }; > + }; > + > + CPU2: cpu@200 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x200>; > + enable-method = "psci"; > + next-level-cache = <&L2_200>; > + L2_200: l2-cache { > + compatible = "cache"; > + next-level-cache = <&L3_0>; > + }; > + }; > + > + CPU3: cpu@300 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x300>; > + enable-method = "psci"; > + next-level-cache = <&L2_300>; > + L2_300: l2-cache { > + compatible = "cache"; > + next-level-cache = <&L3_0>; > + }; > + }; > + > + CPU4: cpu@400 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x400>; > + enable-method = "psci"; > + next-level-cache = <&L2_400>; > + L2_400: l2-cache { > + compatible = "cache"; > + next-level-cache = <&L3_0>; > + }; > + }; > + > + CPU5: cpu@500 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x500>; > + enable-method = "psci"; > + next-level-cache = <&L2_500>; > + L2_500: l2-cache { > + compatible = "cache"; > + next-level-cache = <&L3_0>; > + }; > + }; > + > + CPU6: cpu@600 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x600>; > + enable-method = "psci"; > + next-level-cache = <&L2_600>; > + L2_600: l2-cache { > + compatible = "cache"; > + next-level-cache = <&L3_0>; > + }; > + }; > + > + CPU7: cpu@700 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x700>; > + enable-method = "psci"; > + next-level-cache = <&L2_700>; > + L2_700: l2-cache { > + compatible = "cache"; > + next-level-cache = <&L3_0>; > + }; > + }; > + }; > + > + memory@80000000 { > + device_type = "memory"; > + /* We expect the bootloader to fill in the size */ > + reg = <0 0x80000000 0 0>; > + }; > + > + pmu { > + compatible = "arm,armv8-pmuv3"; > + interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_HIGH>; > + }; > + > + psci { > + compatible = "arm,psci-1.0"; > + method = "smc"; > + }; > + > + 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. > + 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? > + 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"; > + }; > + }; > + > + tlmm: pinctrl@3500000 { > + compatible = "qcom,sc7180-pinctrl"; > + reg = <0 0x03500000 0 0x300000>, > + <0 0x03900000 0 0x300000>, > + <0 0x03d00000 0 0x300000>; > + reg-names = "west", "north", "south"; > + interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-controller; > + #interrupt-cells = <2>; > + gpio-ranges = <&tlmm 0 0 120>; > + > + qup_uart2_default: qup-uart2-default { > + pinmux { > + pins = "gpio44", "gpio45"; > + function = "qup12"; > + }; > + }; > + }; > + > + intc: interrupt-controller@17a00000 { > + compatible = "arm,gic-v3"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + #interrupt-cells = <3>; > + interrupt-controller; > + reg = <0 0x17a00000 0 0x10000>, /* GICD */ > + <0 0x17a60000 0 0x100000>; /* GICR * 8 */ > + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>; > + > + gic-its@17a40000 { > + compatible = "arm,gic-v3-its"; > + msi-controller; > + #msi-cells = <1>; > + reg = <0 0x17a40000 0 0x20000>; > + status = "disabled"; > + }; > + }; > + > + timer@17c20000{ > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + compatible = "arm,armv7-timer-mem"; > + reg = <0 0x17c20000 0 0x1000>; > + > + frame@17c21000 { > + frame-number = <0>; > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0 0x17c21000 0 0x1000>, > + <0 0x17c22000 0 0x1000>; > + }; > + > + frame@17c23000 { > + frame-number = <1>; > + interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0 0x17c23000 0 0x1000>; > + status = "disabled"; > + }; > + > + frame@17c25000 { > + frame-number = <2>; > + interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0 0x17c25000 0 0x1000>; > + status = "disabled"; > + }; > + > + frame@17c27000 { > + frame-number = <3>; > + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0 0x17c27000 0 0x1000>; > + status = "disabled"; > + }; > + > + frame@17c29000 { > + frame-number = <4>; > + interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0 0x17c29000 0 0x1000>; > + status = "disabled"; > + }; > + > + frame@17c2b000 { > + frame-number = <5>; > + interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0 0x17c2b000 0 0x1000>; > + status = "disabled"; > + }; > + > + frame@17c2d000 { > + frame-number = <6>; > + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0 0x17c2d000 0 0x1000>; > + status = "disabled"; > + }; > + }; > + }; > + > + timer { > + compatible = "arm,armv8-timer"; > + interrupts = <GIC_PPI 1 IRQ_TYPE_LEVEL_LOW>, > + <GIC_PPI 2 IRQ_TYPE_LEVEL_LOW>, > + <GIC_PPI 3 IRQ_TYPE_LEVEL_LOW>, > + <GIC_PPI 0 IRQ_TYPE_LEVEL_LOW>; > + }; > +}; > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
On 10/21/2019 1:40 PM, Marc Zyngier wrote: > On 2019-10-21 07:55, Rajendra Nayak wrote: >> From: Maulik Shah <mkshah@codeaurora.org> >> >> Add sc7180 pdc irqchip >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> Cc: Lina Iyer <ilina@codeaurora.org> >> Cc: Marc Zyngier <maz@kernel.org> >> --- >> v2: No change >> >> drivers/irqchip/qcom-pdc.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c >> index faa7d61b9d6c..954fb599fa9c 100644 >> --- a/drivers/irqchip/qcom-pdc.c >> +++ b/drivers/irqchip/qcom-pdc.c >> @@ -310,3 +310,4 @@ static int qcom_pdc_init(struct device_node >> *node, struct device_node *parent) >> } >> >> IRQCHIP_DECLARE(pdc_sdm845, "qcom,sdm845-pdc", qcom_pdc_init); >> +IRQCHIP_DECLARE(pdc_sc7180, "qcom,sc7180-pdc", qcom_pdc_init); > > What I gather from these 3 irq-related patches is that as far as > the PDC is concerned, SDM845/850 and SC7180 are strictly identical. > > Why the churn? > > M. Hi Marc, Different compatible were used to distinguish since interrupt mapping (PDC to GIC) was earlier kept in driver. Now since mapping is kept in DTSI, we can continue to use qcom,sdm845-pdc for sc7180. i will submit another patch to make it more generic for all SoCs like "qcom,pdc". Thanks, Maulik
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@vger.kernel.org/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"; >> + }; >> + }; >> +