[permalink] [raw]

Subject: Re: [PATCH V2 7/8] arm64: dts: Add ipq5018 SoC and MP03 board support

Thanks Konrad for the review.

On 6/27/2022 12:02 AM, Konrad Dybcio wrote:
>
> On 21.06.2022 18:11, Sricharan R wrote:
>> From: Varadarajan Narayanan <[email protected]>
>>
>> Add initial device tree support for the Qualcomm IPQ5018 SoC and
>> MP03.1-C2 board.
>>
>> Co-developed-by: Sricharan R <[email protected]>
>> Signed-off-by: Sricharan R <[email protected]>
>> Signed-off-by: Varadarajan Narayanan <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/Makefile | 1 +
>> .../arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts | 29 +++
>> arch/arm64/boot/dts/qcom/ipq5018.dtsi | 221 ++++++++++++++++++
>> 3 files changed, 251 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts
>> create mode 100644 arch/arm64/boot/dts/qcom/ipq5018.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>> index f9e6343acd03..c44e701f093c 100644
>> --- a/arch/arm64/boot/dts/qcom/Makefile
>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>> @@ -10,6 +10,7 @@ dtb-$(CONFIG_ARCH_QCOM) += ipq8074-hk10-c2.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += msm8916-alcatel-idol347.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += msm8916-asus-z00l.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += msm8916-huawei-g7.dtb
>> +dtb-$(CONFIG_ARCH_QCOM) += ipq5018-mp03.1-c2.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += msm8916-longcheer-l8150.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += msm8916-longcheer-l8910.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += msm8916-mtp.dtb
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts
>> new file mode 100644
>> index 000000000000..d1cd080ec3db
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts
>> @@ -0,0 +1,29 @@
>> +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause
>> +/*
>> + * IPQ5018 CP01 board device tree source
>> + *
>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "ipq5018.dtsi"
>> +
>> +/ {
>> + model = "Qualcomm Technologies, Inc. IPQ5018/AP-MP03-C2";
>> + compatible = "qcom,ipq5018-mp03", "qcom,ipq5018";
>> +
>> + aliases {
>> + serial0 = &blsp1_uart1;
>> + };
>> +
>> + chosen {
>> + stdout-path = "serial0:115200n8";
>> + };
>> +};
>> +
>> +&blsp1_uart1 {
>> + pinctrl-0 = <&serial_1_pins>;
>> + pinctrl-names = "default";
>> + status = "ok";
>> +};
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> new file mode 100644
>> index 000000000000..084fb7b30dfd
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>> @@ -0,0 +1,221 @@
>> +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause
>> +/*
>> + * Copyright (c) 2017-2021, The Linux Foundation. All rights reserved.
>> + */
>> +/*
>> + * IPQ5018 SoC device tree source
>> + *
>> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +#include <dt-bindings/clock/qcom,gcc-ipq5018.h>
>> +#include <dt-bindings/reset/qcom,gcc-ipq5018.h>
>> +
>> +/ {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + interrupt-parent = <&intc>;
> Hi!
>
> interrupt-parent could go first.

ok.

>> +
>> + sleep_clk: sleep-clk {
>> + compatible = "fixed-clock";
>> + clock-frequency = <32000>;
>> + #clock-cells = <0>;
>> + };
>> +
>> + xo: xo {
>> + compatible = "fixed-clock";
>> + clock-frequency = <24000000>;
>> + #clock-cells = <0>;
>> + };
>> +
>> + gen2clk0: gen2clk0 {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <125000000>;
>> + clock-output-names = "pcie20_phy0_pipe_clk";
>> + };
>> +
>> + gen2clk1: gen2clk1 {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <125000000>;
>> + clock-output-names = "pcie20_phy1_pipe_clk";
>> + };
> I am not sure what's the current stance on this, but previously clock nodes
> used to be wrapped in a clocks {} node, as currently they are not sorted
> properly.
>
hmm ok, yeah, see the clocks { node in some recent dts as well, will
add the wrapper.

>> +
>> + cpus: cpus {
> Is this label going to be used?
hmm, not used, will remove.
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + CPU0: cpu@0 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a53";
>> + reg = <0x0>;
>> + enable-method = "psci";
>> + next-level-cache = <&L2_0>;
>> + };
>> +
>> + CPU1: cpu@1 {
>> + device_type = "cpu";
>> + compatible = "arm,cortex-a53";
>> + enable-method = "psci";
>> + reg = <0x1>;
>> + next-level-cache = <&L2_0>;
>> + };
>> +
>> + L2_0: l2-cache {
>> + compatible = "cache";
>> + cache-level = <0x2>;
> This should probably be dec, as it's not a register.

'dec' ? Sorry, i did not get that.

>> + };
>> + };
>> +
>> + pmuv8: pmu {
> Are there any other PMUs? And is this label going to be used?

ok, will remove the label.

>> + compatible = "arm,cortex-a53-pmu";
>> + interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) |
>> + IRQ_TYPE_LEVEL_HIGH)>;
> I think this can fit in a single 100-char line.
>
ok.

>> + };
>> +
>> + psci: psci {
>> + compatible = "arm,psci-1.0";
>> + method = "smc";
>> + };
>> +
>> + memory@40000000 {
> This node is not sorted properly.

ok, will sort for all nodes.

>> + device_type = "memory";
>> + /* We expect the bootloader to fill in the size */
>> + reg = <0x0 0x40000000 0x0 0x0>;
>> + };
>> +
>> + reserved-memory {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + ranges;
>> +
>> + tz: tz@4ac00000 {
> Please rename it to tz_memory or tz_region or something more
> descriptive. Also, memory@

ok.

>> + reg = <0x0 0x4ac00000 0x0 0x00400000>;
> I don't think we tend to pad size to 8 hex digits.

ok.

>> + no-map;
>> + };
>> + };
>> +
>> + timer {
> This is not sorted properly.

ok.

>> + compatible = "arm,armv8-timer";
>> + interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> + <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> + };
>> +
>> + soc: soc@0 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0 0 0 0xffffffff>;
> Isn't that the default value?

ok, if omitting this would be retain the default, then will do it.

>> + compatible = "simple-bus";
> Please sort the properties.

ok.

>> +
>> + tlmm: pinctrl@1000000 {
>> + compatible = "qcom,ipq5018-pinctrl";
>> + reg = <0x01000000 0x300000>;
>> + interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + gpio-ranges = <&tlmm 0 80>;
>> + interrupt-controller;
>> + #interrupt-cells = <2>;
>> +
>> + serial_1_pins: serial1-pinmux {
> Other DTs call it uart pins, also this is the default/poweron
> state but the hardware provides support for sleep/poweroff.
> Please rename this accordingly.

ok.

>> + pins = "gpio31", "gpio32", "gpio33", "gpio34";
>> + function = "blsp1_uart1";
>> + drive-strength = <8>;
>> + bias-pull-down;
>> + };
>> + };
>> +
>> + gcc: clock-controller@1800000 {
>> + compatible = "qcom,gcc-ipq5018";
>> + reg = <0x01800000 0x80000>;
>> + clocks = <&xo>, <&sleep_clk>;
>> + clock-names = "xo", "sleep_clk";
>> + #clock-cells = <1>;
>> + #reset-cells = <1>;
>> + };
>> +
>> + blsp1_uart1: serial@78af000 {
>> + compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
>> + reg = <0x078af000 0x200>;
>> + interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&gcc GCC_BLSP1_UART1_APPS_CLK>,
>> + <&gcc GCC_BLSP1_AHB_CLK>;
> Not sure whether thunderbird is dumb again, or the indentation
> is off by 1 space.
indentation issue, will fix.
>> + clock-names = "core", "iface";
>> + status = "disabled";
>> + };
>> +
>> + intc: interrupt-controller@b000000 {
>> + compatible = "qcom,msm-qgic2";
> Please sort the properties (compatible and reg gotta go first, etc.).

ok.

>> + interrupt-controller;
>> + #interrupt-cells = <0x3>;
> *-cells should be decimal.
>
ok.

>> + reg = <0x0b000000 0x1000>, /*GICD*/
> /* GICD */, please (and the next ones too)

ok.

>> + <0x0b002000 0x1000>, /*GICC*/
>> + <0x0b001000 0x1000>, /*GICH*/
>> + <0x0b004000 0x1000>; /*GICV*/
>> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
>> + };
>> +
>> + timer@b120000 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> + compatible = "arm,armv7-timer-mem";
>> + reg = <0x0b120000 0x1000>;
>> + clock-frequency = <19200000>;
> Please sort the properties, also in subnodes.

ok, will do.

Regards,
Sricharan

2022-06-28 13:57:05

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH V2 0/8] Add minimal boot support for IPQ5018

On Tue, Jun 21, 2022 at 6:11 PM Sricharan R <[email protected]> wrote:

> The IPQ5018 is Qualcomm's 802.11ax SoC for Routers,
> Gateways and Access Points.
>
> This series adds minimal board boot support for ipq5018-mp03.1-c2 board.

Pretty cool!

> dt-bindings: pinctrl: qcom: Add ipq5018 pinctrl bindings
> pinctrl: qcom: Add IPQ5018 pinctrl driver

I'm happy to merge the two pinctrl patches separately to the pinctrl
tree if I can get a review from Bjorn or Krzysztof.

Yours,
Linus Walleij

2022-06-29 06:58:35

by Sricharan Ramabadhran

[permalink] [raw]

Subject: Re: [PATCH V2 0/8] Add minimal boot support for IPQ5018

Hi Linus,

On 6/28/2022 6:25 PM, Linus Walleij wrote:
> On Tue, Jun 21, 2022 at 6:11 PM Sricharan R <[email protected]> wrote:
>
>> The IPQ5018 is Qualcomm's 802.11ax SoC for Routers,
>> Gateways and Access Points.
>>
>> This series adds minimal board boot support for ipq5018-mp03.1-c2 board.
> Pretty cool!
>
>> dt-bindings: pinctrl: qcom: Add ipq5018 pinctrl bindings
>> pinctrl: qcom: Add IPQ5018 pinctrl driver
> I'm happy to merge the two pinctrl patches separately to the pinctrl
> tree if I can get a review from Bjorn or Krzysztof.

Thanks. Have few review comments from Bjorn and Krzysztof on the
bindings.
Will post V3 and hopefully that should have their acks on that.

Regards,
Sricharan

2022-07-01 07:45:10

by Konrad Dybcio

[permalink] [raw]

Subject: Re: [PATCH V2 7/8] arm64: dts: Add ipq5018 SoC and MP03 board support

On 28.06.2022 09:14, Sricharan Ramabadhran wrote:
> Thanks Konrad for the review.
>
> On 6/27/2022 12:02 AM, Konrad Dybcio wrote:
>>
>> On 21.06.2022 18:11, Sricharan R wrote:
>>> From: Varadarajan Narayanan <[email protected]>
>>>
>>> Add initial device tree support for the Qualcomm IPQ5018 SoC and
>>> MP03.1-C2 board.
>>>
>>> Co-developed-by: Sricharan R <[email protected]>
>>> Signed-off-by: Sricharan R <[email protected]>
>>> Signed-off-by: Varadarajan Narayanan <[email protected]>
>>> ---
>>> arch/arm64/boot/dts/qcom/Makefile             |   1 +
>>> .../arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts | 29 +++
>>> arch/arm64/boot/dts/qcom/ipq5018.dtsi         | 221 ++++++++++++++++++
>>> 3 files changed, 251 insertions(+)
>>> create mode 100644 arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts
>>> create mode 100644 arch/arm64/boot/dts/qcom/ipq5018.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>>> index f9e6343acd03..c44e701f093c 100644
>>> --- a/arch/arm64/boot/dts/qcom/Makefile
>>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>>> @@ -10,6 +10,7 @@ dtb-$(CONFIG_ARCH_QCOM)    += ipq8074-hk10-c2.dtb
>>> dtb-$(CONFIG_ARCH_QCOM)    += msm8916-alcatel-idol347.dtb
>>> dtb-$(CONFIG_ARCH_QCOM)    += msm8916-asus-z00l.dtb
>>> dtb-$(CONFIG_ARCH_QCOM)    += msm8916-huawei-g7.dtb
>>> +dtb-$(CONFIG_ARCH_QCOM)    += ipq5018-mp03.1-c2.dtb
>>> dtb-$(CONFIG_ARCH_QCOM)    += msm8916-longcheer-l8150.dtb
>>> dtb-$(CONFIG_ARCH_QCOM)    += msm8916-longcheer-l8910.dtb
>>> dtb-$(CONFIG_ARCH_QCOM)    += msm8916-mtp.dtb
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts b/arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts
>>> new file mode 100644
>>> index 000000000000..d1cd080ec3db
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts
>>> @@ -0,0 +1,29 @@
>>> +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause
>>> +/*
>>> + * IPQ5018 CP01 board device tree source
>>> + *
>>> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
>>> + */
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include "ipq5018.dtsi"
>>> +
>>> +/ {
>>> +    model = "Qualcomm Technologies, Inc. IPQ5018/AP-MP03-C2";
>>> +    compatible = "qcom,ipq5018-mp03", "qcom,ipq5018";
>>> +
>>> +    aliases {
>>> +        serial0 = &blsp1_uart1;
>>> +    };
>>> +
>>> +    chosen {
>>> +        stdout-path = "serial0:115200n8";
>>> +    };
>>> +};
>>> +
>>> +&blsp1_uart1 {
>>> +    pinctrl-0 = <&serial_1_pins>;
>>> +    pinctrl-names = "default";
>>> +    status = "ok";
>>> +};
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>>> new file mode 100644
>>> index 000000000000..084fb7b30dfd
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
>>> @@ -0,0 +1,221 @@
>>> +// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause
>>> +/*
>>> + * Copyright (c) 2017-2021, The Linux Foundation. All rights reserved.
>>> + */
>>> +/*
>>> + * IPQ5018 SoC device tree source
>>> + *
>>> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
>>> + */
>>> +
>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +#include <dt-bindings/clock/qcom,gcc-ipq5018.h>
>>> +#include <dt-bindings/reset/qcom,gcc-ipq5018.h>
>>> +
>>> +/ {
>>> +    #address-cells = <2>;
>>> +    #size-cells = <2>;
>>> +    interrupt-parent = <&intc>;
>> Hi!
>>
>> interrupt-parent could go first.
>
> ok.
>
>
>>> +
>>> +    sleep_clk: sleep-clk {
>>> +        compatible = "fixed-clock";
>>> +        clock-frequency = <32000>;
>>> +        #clock-cells = <0>;
>>> +    };
>>> +
>>> +    xo: xo {
>>> +        compatible = "fixed-clock";
>>> +        clock-frequency = <24000000>;
>>> +        #clock-cells = <0>;
>>> +    };
>>> +
>>> +    gen2clk0: gen2clk0 {
>>> +        compatible = "fixed-clock";
>>> +        #clock-cells = <0>;
>>> +        clock-frequency = <125000000>;
>>> +        clock-output-names = "pcie20_phy0_pipe_clk";
>>> +    };
>>> +
>>> +    gen2clk1: gen2clk1 {
>>> +        compatible = "fixed-clock";
>>> +        #clock-cells = <0>;
>>> +        clock-frequency = <125000000>;
>>> +        clock-output-names = "pcie20_phy1_pipe_clk";
>>> +    };
>> I am not sure what's the current stance on this, but previously clock nodes
>> used to be wrapped in a clocks {} node, as currently they are not sorted
>> properly.
>>
> hmm ok, yeah, see the clocks { node in some recent dts as well, will add the wrapper.
>
>
>>> +
>>> +    cpus: cpus {
>> Is this label going to be used?
> hmm, not used, will remove.
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        CPU0: cpu@0 {
>>> +            device_type = "cpu";
>>> +            compatible = "arm,cortex-a53";
>>> +            reg = <0x0>;
>>> +            enable-method = "psci";
>>> +            next-level-cache = <&L2_0>;
>>> +        };
>>> +
>>> +        CPU1: cpu@1 {
>>> +            device_type = "cpu";
>>> +            compatible = "arm,cortex-a53";
>>> +            enable-method = "psci";
>>> +            reg = <0x1>;
>>> +            next-level-cache = <&L2_0>;
>>> +        };
>>> +
>>> +        L2_0: l2-cache {
>>> +            compatible = "cache";
>>> +            cache-level = <0x2>;
>> This should probably be dec, as it's not a register.
>
>    'dec' ? Sorry, i did not get that.
Short for decimal.

Konrad

[snip]