Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <db8bc4e8-3b41-43d5-6bc9-fb958635ce90@linaro.org>
Date:   Fri, 12 May 2023 12:12:31 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
From:   neil.armstrong@linaro.org
Reply-To: neil.armstrong@linaro.org
Subject: Re: [PATCH V4] arm64: dts: add support for C3 based Amlogic AW409
Content-Language: en-US
To:     "xianwei.zhao" <xianwei.zhao@amlogic.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-amlogic@lists.infradead.org, devicetree@vger.kernel.org
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
References: <20230510091129.151669-1-xianwei.zhao@amlogic.com>
 <b2bdb00f-63de-40a9-ebe6-32f83607b5d8@linaro.org>
 <d588b62f-970d-a974-814d-5b116bc547f9@amlogic.com>
Organization: Linaro Developer Services
In-Reply-To: <d588b62f-970d-a974-814d-5b116bc547f9@amlogic.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 12/05/2023 09:03, xianwei.zhao wrote:
> Hi Neil,
>       Thank you for your reply.
> 
> On 2023/5/11 16:39, Neil Armstrong wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hi,
>>
>> On 10/05/2023 11:11, Xianwei Zhao wrote:
>>> Amlogic C3 is an advanced edge AI processor designed for smart IP camera
>>> applications.
>>>
>>> Add basic support for the C3 based Amlogic AW409 board, which describes
>>> the following components: CPU, GIC, IRQ, Timer, UART. It's capable of
>>> booting up into the serial console.
>>>
>>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>> ---
>>> Link: https://lore.kernel.org/all/20230407102704.1055152-1-kelvin.zhang@amlogic.com
>>> Link: https://lore.kernel.org/all/20230307222651.2106615-2-martin.blumenstingl@googlemail.com
>>>
>>> V3 -> V4: Move Link under the --- before the changelog.
>>> V2 -> V3: Remove '256m' from filename;
>>>            Keep alphabetical order of Makefile.
>>> V1 -> V2: Remove new arch, and use ARCH_MESON;
>>>            Modify node name, and delete superfluous blank line.
>>> ---
>>>   arch/arm64/boot/dts/amlogic/Makefile          |  1 +
>>>   .../dts/amlogic/amlogic-c3-c302x-aw409.dts    | 29 +++++++
>>>   arch/arm64/boot/dts/amlogic/amlogic-c3.dtsi   | 86 +++++++++++++++++++
>>>   3 files changed, 116 insertions(+)
>>>   create mode 100644 arch/arm64/boot/dts/amlogic/amlogic-c3-c302x-aw409.dts
>>>   create mode 100644 arch/arm64/boot/dts/amlogic/amlogic-c3.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/amlogic/Makefile b/arch/arm64/boot/dts/amlogic/Makefile
>>> index cd1c5b04890a..6f61798a109f 100644
>>> --- a/arch/arm64/boot/dts/amlogic/Makefile
>>> +++ b/arch/arm64/boot/dts/amlogic/Makefile
>>> @@ -1,4 +1,5 @@
>>>   # SPDX-License-Identifier: GPL-2.0
>>> +dtb-$(CONFIG_ARCH_MESON) += amlogic-c3-c302x-aw409.dtb
>>>   dtb-$(CONFIG_ARCH_MESON) += meson-a1-ad401.dtb
>>>   dtb-$(CONFIG_ARCH_MESON) += meson-axg-jethome-jethub-j100.dtb
>>>   dtb-$(CONFIG_ARCH_MESON) += meson-axg-jethome-jethub-j110-rev-2.dtb
>>> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-c3-c302x-aw409.dts b/arch/arm64/boot/dts/amlogic/amlogic-c3-c302x-aw409.dts
>>> new file mode 100644
>>> index 000000000000..edce8850b338
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/amlogic/amlogic-c3-c302x-aw409.dts
>>> @@ -0,0 +1,29 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +/*
>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
>>> + */
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include "amlogic-c3.dtsi"
>>> +
>>> +/ {
>>> +     model = "Amlogic C302 aw409 Development Board";
>>> +     compatible = "amlogic,aw409", "amlogic,c3";
>>> +     interrupt-parent = <&gic>;
>>> +     #address-cells = <2>;
>>> +     #size-cells = <2>;
>>> +
>>> +     aliases {
>>> +             serial0 = &uart_b;
>>> +     };
>>> +
>>> +     memory@0 {
>>> +             device_type = "memory";
>>> +             reg = <0x0 0x0 0x0 0x10000000>;
>>> +     };
>>> +};
>>> +
>>> +&uart_b {
>>> +     status = "okay";
>>> +};
>>> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-c3.dtsi b/arch/arm64/boot/dts/amlogic/amlogic-c3.dtsi
>>> new file mode 100644
>>> index 000000000000..93b335aef605
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/amlogic/amlogic-c3.dtsi
>>> @@ -0,0 +1,86 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +/*
>>> + * Copyright (c) 2021 Amlogic, Inc. All rights reserved.
>>> + */
>>> +
>>> +#include <dt-bindings/interrupt-controller/irq.h>
>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +#include <dt-bindings/gpio/gpio.h>
>>> +
>>> +/ {
>>> +     cpus {
>>> +             #address-cells = <2>;
>>> +             #size-cells = <0>;
>>> +
>>> +             cpu0: cpu@0 {
>>> +                     device_type = "cpu";
>>> +                     compatible = "arm,cortex-a35";
>>> +                     reg = <0x0 0x0>;
>>> +                     enable-method = "psci";
>>> +             };
>>> +
>>> +             cpu1: cpu@1 {
>>> +                     device_type = "cpu";
>>> +                     compatible = "arm,cortex-a35";
>>> +                     reg = <0x0 0x1>;
>>> +                     enable-method = "psci";
>>> +             };
>>> +     };
>>> +
>>> +     timer {
>>> +             compatible = "arm,armv8-timer";
>>> +             interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>> +                          <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>> +                          <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>> +                          <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>>> +     };
>>> +
>>> +     psci {
>>> +             compatible = "arm,psci-1.0";
>>> +             method = "smc";
>>> +     };
>>> +
>>> +     xtal: xtal-clk {
>>> +             compatible = "fixed-clock";
>>> +             clock-frequency = <24000000>;
>>> +             clock-output-names = "xtal";
>>> +             #clock-cells = <0>;
>>> +     };
>>> +
>>> +     soc {
>>> +             compatible = "simple-bus";
>>> +             #address-cells = <2>;
>>> +             #size-cells = <2>;
>>> +             ranges;
>>> +
>>> +             gic: interrupt-controller@fff01000 {
>>> +                     compatible = "arm,gic-400";
>>> +                     #interrupt-cells = <3>;
>>> +                     #address-cells = <0>;
>>> +                     interrupt-controller;
>>> +                     reg = <0x0 0xfff01000 0 0x1000>,
>>> +                           <0x0 0xfff02000 0 0x2000>,
>>> +                           <0x0 0xfff04000 0 0x2000>,
>>> +                           <0x0 0xfff06000 0 0x2000>;
>>> +                     interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
>>> +             };
>>> +
>>> +             apb4: bus@fe000000 {
>>> +                     compatible = "simple-bus";
>>> +                     reg = <0x0 0xfe000000 0x0 0x480000>;
>>> +                     #address-cells = <2>;
>>> +                     #size-cells = <2>;
>>> +                     ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x480000>;
>>> +
>>> +                     uart_b: serial@7a000 {
>>> +                             compatible = "amlogic,meson-g12a-uart";
>>
>> This should be:
>>
>>         compatible = "amlogic,meson-g12a-uart", "amlogic,meson-gx-uart"
>>
>> to match the amlogic,meson-uart.yaml bindings.
>>
> 
> I will use strings 'amlogic,meson-s4-uart' instead of these, because of board C3 is only use 12M clock source for serial, the same as s4 series.

Sure.

Neil

> 
>> Please run :
>> make CHECK_DTBS=1 amlogic/amlogic-c3-c302x-aw409.dtb
>>
>> With that fixed, please add on v5:
> 
> Will do.
> 
>>
>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
>>
>>> +                             reg = <0x0 0x7a000 0x0 0x18>;
>>> +                             interrupts = <GIC_SPI 169 IRQ_TYPE_EDGE_RISING>;
>>> +                             status = "disabled";
>>> +                             clocks = <&xtal>, <&xtal>, <&xtal>;
>>> +                             clock-names = "xtal", "pclk", "baud";
>>> +                     };
>>> +
>>> +             };
>>> +     };
>>> +};
>>>
>>> base-commit: ae68fb187b59bc8645974320808ab2d7c41b1833
>>