Re: [PATCH v6 4/4] arm64: dts: Add MediaTek MT8188 dts and evaluation board and Makefile

From: Chen-Yu Tsai
Date: Tue Oct 24 2023 - 05:37:12 EST

Hi,

On Mon, Oct 23, 2023 at 6:55 PM Alexandre Mergnat <amergnat@xxxxxxxxxxxx> wrote:
>
>
>
> On 23/10/2023 10:38, Jason-ch Chen wrote:
> > From: jason-ch chen <Jason-ch.Chen@xxxxxxxxxxxx>
> >
> > MT8188 is a SoC based on 64bit ARMv8 architecture. It contains 6 CA55
> > and 2 CA78 cores. MT8188 share many HW IP with MT65xx series.
> >
> > We add basic chip support for MediaTek MT8188 on evaluation board.
> >
> > Signed-off-by: jason-ch chen <Jason-ch.Chen@xxxxxxxxxxxx>
> > ---
> > arch/arm64/boot/dts/mediatek/Makefile | 1 +
> > arch/arm64/boot/dts/mediatek/mt8188-evb.dts | 387 ++++++++
> > arch/arm64/boot/dts/mediatek/mt8188.dtsi | 956 ++++++++++++++++++++
> > 3 files changed, 1344 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/mediatek/mt8188-evb.dts
> > create mode 100644 arch/arm64/boot/dts/mediatek/mt8188.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> > index e6e7592a3645..8900b939ed52 100644
> > --- a/arch/arm64/boot/dts/mediatek/Makefile
> > +++ b/arch/arm64/boot/dts/mediatek/Makefile
> > @@ -44,6 +44,7 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-krane-sku0.dtb
> > dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-kukui-krane-sku176.dtb
> > dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-pumpkin.dtb
> > dtb-$(CONFIG_ARCH_MEDIATEK) += mt8186-evb.dtb
> > +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8188-evb.dtb
> > dtb-$(CONFIG_ARCH_MEDIATEK) += mt8192-asurada-hayato-r1.dtb
> > dtb-$(CONFIG_ARCH_MEDIATEK) += mt8192-asurada-hayato-r5-sku2.dtb
> > dtb-$(CONFIG_ARCH_MEDIATEK) += mt8192-asurada-spherion-r0.dtb
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8188-evb.dts b/arch/arm64/boot/dts/mediatek/mt8188-evb.dts
> > new file mode 100644
> > index 000000000000..68a82b49f7a3
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8188-evb.dts
> > @@ -0,0 +1,387 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright (C) 2023 MediaTek Inc.
> > + */
> > +/dts-v1/;
> > +#include "mt8188.dtsi"
> > +#include "mt6359.dtsi"
> > +
> > +/ {
> > + model = "MediaTek MT8188 evaluation board";
> > + compatible = "mediatek,mt8188-evb", "mediatek,mt8188";
> > +
> > + aliases {
> > + serial0 = &uart0;
> > + i2c0 = &i2c0;
> > + i2c1 = &i2c1;
> > + i2c2 = &i2c2;
> > + i2c3 = &i2c3;
> > + i2c4 = &i2c4;
> > + i2c5 = &i2c5;
> > + i2c6 = &i2c6;
> > + mmc0 = &mmc0;
> > + };
> > +
> > + chosen: chosen {
> > + stdout-path = "serial0:115200n8";
> > + };
> > +
> > + memory@40000000 {
> > + device_type = "memory";
> > + reg = <0 0x40000000 0 0x80000000>;
> > + };
> > +
> > + reserved_memory: reserved-memory {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + scp_mem_reserved: memory@50000000 {
> > + compatible = "shared-dma-pool";
> > + reg = <0 0x50000000 0 0x2900000>;
> > + no-map;
> > + };
> > + };
> > +};
> > +
> > +&auxadc {
> > + status = "okay";
> > +};
> > +
> > +&i2c0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c0_pins>;
> > + clock-frequency = <400000>;
> > + status = "okay";
>
> IMO, the order should be
>
> clock-frequency = <400000>;
> pinctrl-0 = <&i2c0_pins>;
> pinctrl-names = "default";
> status = "okay";
>
> Please apply this to other nodes

Within the MediaTek platform device trees, this ordering seems to be all
over. The one here seems to be the ordering MediaTek likes. The device
trees upstreamed by Collabora folks have:

{
status = "okay";

clock-frequency = <400000>;
pinctrl-names = "default";
pinctrl-0 = <&i2c3_pins>;
}

As long as the ordering sort of makes sense (external bus properties come
after internal properties), I probably wouldn't argue either way.

> > +};
> > +
> > +&i2c1 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c1_pins>;
> > + clock-frequency = <400000>;
> > + status = "okay";
> > +};
> > +
> > +&i2c2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c2_pins>;
> > + clock-frequency = <400000>;
> > + status = "okay";
> > +};
> > +
> > +&i2c3 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c3_pins>;
> > + clock-frequency = <400000>;
> > + status = "okay";
> > +};
> > +
> > +&i2c4 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c4_pins>;
> > + clock-frequency = <400000>;
> > + status = "okay";
> > +};
> > +
> > +&i2c5 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c5_pins>;
> > + clock-frequency = <400000>;
> > + status = "okay";
> > +};
> > +
> > +&i2c6 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&i2c6_pins>;
> > + clock-frequency = <400000>;
> > + status = "okay";
> > +};
> > +
> > +&mmc0 {
> > + bus-width = <8>;
> > + hs400-ds-delay = <0x1481b>;
> > + max-frequency = <200000000>;
> > +
> > + cap-mmc-highspeed;
> > + mmc-hs200-1_8v;
> > + mmc-hs400-1_8v;
> > + supports-cqe;
> > + cap-mmc-hw-reset;
> > + no-sdio;
> > + no-sd;
> > + non-removable;
> > +
> > + vmmc-supply = <&mt6359_vemc_1_ldo_reg>;
> > + vqmmc-supply = <&mt6359_vufs_ldo_reg>;
> > +
> > + pinctrl-names = "default", "state_uhs";
> > + pinctrl-0 = <&mmc0_default_pins>;
> > + pinctrl-1 = <&mmc0_uhs_pins>;
> > +
> > + status = "okay";
> > +};
> > +
> > +&mt6359_vcore_buck_reg {
> > + regulator-always-on;
> > +};
> > +
> > +&mt6359_vgpu11_buck_reg {
> > + regulator-always-on;
> > +};
> > +
> > +&mt6359_vpu_buck_reg {
> > + regulator-always-on;
> > +};
> > +
> > +&mt6359_vrf12_ldo_reg {
> > + regulator-always-on;
> > +};
> > +
> > +&nor_flash {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&nor_pins_default>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + status = "okay";
>
> Order:
>
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> pinctrl-0 = <&nor_pins_default>;
> pinctrl-names = "default";

I think pinctrl-names before pinctrl-* makes more sense. We declare the
names and by extension how many pinctrl-N entries are needed first. The
vast majority of the arm64 device tree files have pinctrl-names before
pinctrl-N. The only platform that exclusively has pinctrl-N before
pinctrl-names is amlogic.

If there's a preference for a particular order platform-wide or tree-wide
then it should probably be documented somewhere?

> status = "okay";

I think #address-cells and #size-cells belong at the end of the list,
even after "status", just before any child nodes. They describe
properties or requirements for the child nodes, not for the node they
sit in.

> > +
> > + flash@0 {
> > + compatible = "jedec,spi-nor";
> > + reg = <0>;
> > + spi-max-frequency = <52000000>;
> > + };
> > +};
> > +
>
> ..snip..
>
> > +
> > +&pmic {
> > + interrupts-extended = <&pio 222 IRQ_TYPE_LEVEL_HIGH>;
> > +};
> > +
> > +&scp {
> > + memory-region = <&scp_mem_reserved>;
> > + status = "okay";
> > +};
> > +
> > +&spi0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&spi0_pins>;
>
> Order:
>
> pinctrl-0 = <&spi0_pins>;
> pinctrl-names = "default";
>
> Please apply this to other nodes

See above.

ChenYu

> > + status = "okay";
> > +};
> > +
> > +&spi1 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&spi1_pins>;
> > + status = "okay";
> > +};
> > +
> > +&spi2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&spi2_pins>;
> > + status = "okay";
> > +};
> > +
> > +&u3phy0 {
> > + status = "okay";
> > +};
> > +
> > +&u3phy1 {
> > + status = "okay";
> > +};
> > +
> > +&u3phy2 {
> > + status = "okay";
> > +};
> > +
> > +&uart0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart0_pins>;
> > + status = "okay";
> > +};
> > +
>
> ..snip..
>
> > + };
> > + };
> > +};
>
> After that:
> Reviewed-by: Alexandre Mergnat <amergnat@xxxxxxxxxxxx>
>
> --
> Regards,
> Alexandre