Re: [PATCH v7 4/4] arm64: dts: Add mediatek SoC mt8195 and evaluation board

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2021-12-23 at 17:59 +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Mon, Dec 20, 2021 at 8:20 PM Tinghan Shen <
> tinghan.shen@xxxxxxxxxxxx> wrote:
> > 
> > Add basic chip support for mediatek mt8195.
> > 
> > Signed-off-by: Seiya Wang <seiya.wang@xxxxxxxxxxxx>
> > Signed-off-by: Tinghan Shen <tinghan.shen@xxxxxxxxxxxx>
> > ---
> >  arch/arm64/boot/dts/mediatek/Makefile       |    1 +
> >  arch/arm64/boot/dts/mediatek/mt8195-evb.dts |  209 ++++
> >  arch/arm64/boot/dts/mediatek/mt8195.dtsi    | 1034
> > +++++++++++++++++++
> >  3 files changed, 1244 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8195-evb.dts
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8195.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/Makefile
> > b/arch/arm64/boot/dts/mediatek/Makefile
> > index 4f68ebed2e31..7aa08bb4c078 100644
> > --- a/arch/arm64/boot/dts/mediatek/Makefile
> > +++ b/arch/arm64/boot/dts/mediatek/Makefile
> > @@ -32,4 +32,5 @@ 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) += mt8192-evb.dtb
> > +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8195-evb.dtb
> >  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8516-pumpkin.dtb
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8195-evb.dts
> > b/arch/arm64/boot/dts/mediatek/mt8195-evb.dts
> > new file mode 100644
> > index 000000000000..e581c6bbead6
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8195-evb.dts
> > @@ -0,0 +1,209 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright (C) 2021 MediaTek Inc.
> > + * Author: Seiya Wang <seiya.wang@xxxxxxxxxxxx>
> > + */
> > +/dts-v1/;
> > +#include "mt8195.dtsi"
> > +
> > +/ {
> > +       model = "MediaTek MT8195 evaluation board";
> > +       compatible = "mediatek,mt8195-evb", "mediatek,mt8195";
> > +
> > +       aliases {
> > +               serial0 = &uart0;
> > +       };
> > +
> > +       chosen {
> > +               stdout-path = "serial0:921600n8";
> > +       };
> > +
> > +       memory@40000000 {
> > +               device_type = "memory";
> > +               reg = <0 0x40000000 0 0x80000000>;
> > +       };
> > +};
> > +
> > +&auxadc {
> > +       status = "okay";
> > +};
> > +
> > +&i2c0 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2c0_pin>;
> > +       clock-frequency = <100000>;
> > +       status = "okay";
> > +};
> > +
> > +&i2c1 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2c1_pin>;
> > +       clock-frequency = <400000>;
> > +       status = "okay";
> > +};
> > +
> > +&i2c2 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2c2_pin>;
> > +       status = "disabled";
> > +};
> > +
> > +&i2c3 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2c3_pin>;
> > +       status = "disabled";
> > +};
> 
> Is there any reason in particular to list "disabled" devices here?
> Are they part of some GPIO header? If they are not accessible, then
> it's better to not list them. If they are, please leave a comment
> about it.
> 
> > +
> > +&i2c4 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2c4_pin>;
> > +       clock-frequency = <400000>;
> > +       status = "okay";
> > +};
> > +
> > +&i2c5 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2c5_pin>;
> > +       status = "disabled";
> > +};
> > +
> > +&i2c6 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2c6_pin>;
> > +       clock-frequency = <400000>;
> > +       status = "disabled";
> > +};
> 
> Same here.
> 
> > +
> > +&nor_flash {
> > +       status = "okay";
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&nor_pins_default>;
> 
> Please add an empty line between properties and child device nodes.
> It helps
> with readability and also fits the style of other parts and other DT
> files.
> 
> > +       flash@0 {
> > +               compatible = "jedec,spi-nor";
> > +               reg = <0>;
> > +               spi-max-frequency = <50000000>;
> > +       };
> > +};
> > +
> > +&pio {
> > +       i2c0_pin: i2c0-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO8__FUNC_SDA0>,
> > +                                <PINMUX_GPIO9__FUNC_SCL0>;
> > +                       bias-pull-up = <1>;
> > +                       mediatek,rsel = <7>;
> 
> Please use the MTK_PULL_SET_RSEL_* macros with the bias-pull-*
> properties.
> We spent a lot of time defining those.
> 
> > +                       mediatek,drive-strength-adv = <0>;
> 
> This property is not part of the DT binding.
> 
> > +                       drive-strength = <MTK_DRIVE_6mA>;
> 
> Please just use raw numbers here. MTK_DRIVE_6mA just translates to 6.
> The binding already specifies mA as the unit for "drive-strength".
> 
> > +               };
> > +       };
> 
> Above comments apply to all the other "pins" nodes.
> 
> Please add an empty line between different child device nodes. It
> helps
> with readability and also fits the style of other parts and other DT
> files.
> 
> > +       i2c1_pin: i2c1-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO10__FUNC_SDA1>,
> > +                                <PINMUX_GPIO11__FUNC_SCL1>;
> > +                       bias-pull-up = <1>;
> > +                       mediatek,rsel = <7>;
> > +                       mediatek,drive-strength-adv = <0>;
> > +                       drive-strength = <MTK_DRIVE_6mA>;
> > +               };
> > +       };
> > +       i2c2_pin: i2c2-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO12__FUNC_SDA2>,
> > +                                <PINMUX_GPIO13__FUNC_SCL2>;
> > +                       bias-pull-up = <1>;
> > +                       mediatek,rsel = <7>;
> > +                       mediatek,drive-strength-adv = <7>;
> > +               };
> > +       };
> > +       i2c3_pin: i2c3-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO14__FUNC_SDA3>,
> > +                                <PINMUX_GPIO15__FUNC_SCL3>;
> > +                       bias-pull-up = <1>;
> > +                       mediatek,rsel = <7>;
> > +                       mediatek,drive-strength-adv = <7>;
> > +               };
> > +       };
> > +       i2c4_pin: i2c4-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO16__FUNC_SDA4>,
> > +                                <PINMUX_GPIO17__FUNC_SCL4>;
> > +                       bias-pull-up = <1>;
> > +                       mediatek,rsel = <7>;
> > +                       mediatek,drive-strength-adv = <7>;
> > +               };
> > +       };
> > +       i2c5_pin: i2c5-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO29__FUNC_SCL5>,
> > +                                <PINMUX_GPIO30__FUNC_SDA5>;
> > +                       bias-pull-up = <1>;
> > +                       mediatek,rsel = <7>;
> > +                       mediatek,drive-strength-adv = <7>;
> > +               };
> > +       };
> > +       i2c6_pin: i2c6-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO25__FUNC_SDA6>,
> > +                                <PINMUX_GPIO26__FUNC_SCL6>;
> > +                       bias-pull-up = <1>;
> > +               };
> > +       };
> > +       i2c7_pin: i2c7-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO27__FUNC_SCL7>,
> > +                                <PINMUX_GPIO28__FUNC_SDA7>;
> > +                       bias-pull-up = <1>;
> > +               };
> > +       };
> > +       nor_pins_default: nor-pins {
> > +               pins0 {
> > +                       pinmux = <PINMUX_GPIO142__FUNC_SPINOR_IO0>,
> > +                                        <PINMUX_GPIO141__FUNC_SPIN
> > OR_CK>,
> > +                                        <PINMUX_GPIO143__FUNC_SPIN
> > OR_IO1>;
> > +                       bias-pull-down;
> > +               };
> > +               pins1 {
> > +                       pinmux = <PINMUX_GPIO140__FUNC_SPINOR_CS>,
> > +                                    <PINMUX_GPIO130__FUNC_SPINOR_I
> > O2>,
> > +                                    <PINMUX_GPIO131__FUNC_SPINOR_I
> > O3>;
> > +                       bias-pull-up;
> > +               };
> > +       };
> > +       uart0_pin: uart0-pins {
> > +               pins {
> > +                       pinmux = <PINMUX_GPIO98__FUNC_UTXD0>,
> > +                               <PINMUX_GPIO99__FUNC_URXD0>;
> > +               };
> > +       };
> > +};
> > +
> > +&u2port0 {
> > +       status = "okay";
> > +};
> > +
> > +&u2port1 {
> > +       status = "okay";
> > +};
> > +
> > +&u3phy0 {
> > +       status="okay";
> > +};
> > +
> > +&u3phy1 {
> > +       status="okay";
> > +};
> > +
> > +&u3port0 {
> > +       status = "okay";
> > +};
> > +
> > +&u3port1 {
> > +       status = "okay";
> > +};
> > +
> > +&uart0 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&uart0_pin>;
> > +       status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> > b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> > new file mode 100644
> > index 000000000000..057a8492edec
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
> > @@ -0,0 +1,1034 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Copyright (c) 2021 MediaTek Inc.
> > + * Author: Seiya Wang <seiya.wang@xxxxxxxxxxxx>
> > + */
> > +
> > +/dts-v1/;
> > +#include <dt-bindings/clock/mt8195-clk.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/phy/phy.h>
> > +#include <dt-bindings/pinctrl/mt8195-pinfunc.h>
> > +#include <dt-bindings/reset/ti-syscon.h>
> > +
> > +/ {
> > +       compatible = "mediatek,mt8195";
> > +       interrupt-parent = <&gic>;
> > +       #address-cells = <2>;
> > +       #size-cells = <2>;
> > +
> > +       clk26m: oscillator0 {
> 
> Please order device nodes based on the node name ("oscillator0"),
> not the label. An exception to this would be the "soc" node, which
> most tend to keep as the last node in the list.
> 
> Also, naming them "oscillator-26m" and "oscillator-32k" would make
> more sense than "oscillatorN".
> 
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <26000000>;
> > +               clock-output-names = "clk26m";
> > +       };
> > +
> > +       clk32k: oscillator1 {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <32768>;
> > +               clock-output-names = "clk32k";
> > +       };
> > +
> > +       cpus {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               cpu0: cpu@0 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a55";
> > +                       reg = <0x000>;
> > +                       enable-method = "psci";
> > +                       clock-frequency = <1701000000>;
> > +                       capacity-dmips-mhz = <578>;
> > +                       cpu-idle-states = <&cpuoff_l
> > &clusteroff_l>;
> > +                       next-level-cache = <&l2_0>;
> > +                       #cooling-cells = <2>;
> > +               };
> > +
> > +               cpu1: cpu@100 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a55";
> > +                       reg = <0x100>;
> > +                       enable-method = "psci";
> > +                       clock-frequency = <1701000000>;
> > +                       capacity-dmips-mhz = <578>;
> > +                       cpu-idle-states = <&cpuoff_l
> > &clusteroff_l>;
> > +                       next-level-cache = <&l2_0>;
> > +                       #cooling-cells = <2>;
> > +               };
> > +
> > +               cpu2: cpu@200 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a55";
> > +                       reg = <0x200>;
> > +                       enable-method = "psci";
> > +                       clock-frequency = <1701000000>;
> > +                       capacity-dmips-mhz = <578>;
> > +                       cpu-idle-states = <&cpuoff_l
> > &clusteroff_l>;
> > +                       next-level-cache = <&l2_0>;
> > +                       #cooling-cells = <2>;
> > +               };
> > +
> > +               cpu3: cpu@300 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a55";
> > +                       reg = <0x300>;
> > +                       enable-method = "psci";
> > +                       clock-frequency = <1701000000>;
> > +                       capacity-dmips-mhz = <578>;
> > +                       cpu-idle-states = <&cpuoff_l
> > &clusteroff_l>;
> > +                       next-level-cache = <&l2_0>;
> > +                       #cooling-cells = <2>;
> > +               };
> > +
> > +               cpu4: cpu@400 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a78";
> > +                       reg = <0x400>;
> > +                       enable-method = "psci";
> > +                       clock-frequency = <2171000000>;
> > +                       capacity-dmips-mhz = <1024>;
> > +                       cpu-idle-states = <&cpuoff_b
> > &clusteroff_b>;
> > +                       next-level-cache = <&l2_1>;
> > +                       #cooling-cells = <2>;
> > +               };
> > +
> > +               cpu5: cpu@500 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a78";
> > +                       reg = <0x500>;
> > +                       enable-method = "psci";
> > +                       clock-frequency = <2171000000>;
> > +                       capacity-dmips-mhz = <1024>;
> > +                       cpu-idle-states = <&cpuoff_b
> > &clusteroff_b>;
> > +                       next-level-cache = <&l2_1>;
> > +                       #cooling-cells = <2>;
> > +               };
> > +
> > +               cpu6: cpu@600 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a78";
> > +                       reg = <0x600>;
> > +                       enable-method = "psci";
> > +                       clock-frequency = <2171000000>;
> > +                       capacity-dmips-mhz = <1024>;
> > +                       cpu-idle-states = <&cpuoff_b
> > &clusteroff_b>;
> > +                       next-level-cache = <&l2_1>;
> > +                       #cooling-cells = <2>;
> > +               };
> > +
> > +               cpu7: cpu@700 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a78";
> > +                       reg = <0x700>;
> > +                       enable-method = "psci";
> > +                       clock-frequency = <2171000000>;
> > +                       capacity-dmips-mhz = <1024>;
> > +                       cpu-idle-states = <&cpuoff_b
> > &clusteroff_b>;
> > +                       next-level-cache = <&l2_1>;
> > +                       #cooling-cells = <2>;
> > +               };
> > +
> > +               cpu-map {
> > +                       cluster0 {
> > +                               core0 {
> > +                                       cpu = <&cpu0>;
> > +                               };
> 
> Please add an empty line between child nodes. This aids in
> readability.
> 
> > +                               core1 {
> > +                                       cpu = <&cpu1>;
> > +                               };
> > +                               core2 {
> > +                                       cpu = <&cpu2>;
> > +                               };
> > +                               core3 {
> > +                                       cpu = <&cpu3>;
> > +                               };
> > +                       };
> > +                       cluster1 {
> > +                               core0 {
> > +                                       cpu = <&cpu4>;
> > +                               };
> > +                               core1 {
> > +                                       cpu = <&cpu5>;
> > +                               };
> > +                               core2 {
> > +                                       cpu = <&cpu6>;
> > +                               };
> > +                               core3 {
> > +                                       cpu = <&cpu7>;
> > +                               };
> > +                       };
> > +               };
> > +
> > +               idle-states {
> > +                       entry-method = "arm,psci";
> 
> This should be just "psci".
> 
> Please add empty lines between properties and child device nodes, and
> in between device nodes.
> 
> > +                       cpuoff_l: cpuoff_l {
> 
> Please do not use underscores in device node names. Also, for the
> idle
> states, the binding requires the nodes to be named as "cpu-*" or
> "cluster-*". So {cpu,cluster}-off-{l,b} would work.
> 
> > +                               compatible = "arm,idle-state";
> > +                               arm,psci-suspend-param =
> > <0x00010001>;
> > +                               local-timer-stop;
> > +                               entry-latency-us = <50>;
> > +                               exit-latency-us = <95>;
> > +                               min-residency-us = <580>;
> > +                       };
> > +                       cpuoff_b: cpuoff_b {
> > +                               compatible = "arm,idle-state";
> > +                               arm,psci-suspend-param =
> > <0x00010001>;
> > +                               local-timer-stop;
> > +                               entry-latency-us = <45>;
> > +                               exit-latency-us = <140>;
> > +                               min-residency-us = <740>;
> > +                       };
> > +                       clusteroff_l: clusteroff_l {
> > +                               compatible = "arm,idle-state";
> > +                               arm,psci-suspend-param =
> > <0x01010002>;
> > +                               local-timer-stop;
> > +                               entry-latency-us = <55>;
> > +                               exit-latency-us = <155>;
> > +                               min-residency-us = <840>;
> > +                       };
> > +                       clusteroff_b: clusteroff_b {
> > +                               compatible = "arm,idle-state";
> > +                               arm,psci-suspend-param =
> > <0x01010002>;
> > +                               local-timer-stop;
> > +                               entry-latency-us = <50>;
> > +                               exit-latency-us = <200>;
> > +                               min-residency-us = <1000>;
> > +                       };
> > +               };
> > +
> > +               l2_0: l2-cache0 {
> > +                       compatible = "cache";
> > +                       next-level-cache = <&l3_0>;
> > +               };
> > +
> > +               l2_1: l2-cache1 {
> > +                       compatible = "cache";
> > +                       next-level-cache = <&l3_0>;
> > +               };
> > +
> > +               l3_0: l3-cache {
> > +                       compatible = "cache";
> > +               };
> > +       };
> > +
> > +       dsu-pmu {
> > +               compatible = "arm,dsu-pmu";
> > +               interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH 0>;
> > +               cpus = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>,
> > +                      <&cpu4>, <&cpu5>, <&cpu6>, <&cpu7>;
> > +       };
> > +
> > +       pmu-a55 {
> > +               compatible = "arm,cortex-a55-pmu";
> > +               interrupt-parent = <&gic>;
> > +               interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH
> > &ppi_cluster0>;
> > +       };
> > +
> > +       pmu-a78 {
> > +               compatible = "arm,cortex-a78-pmu";
> > +               interrupt-parent = <&gic>;
> > +               interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH
> > &ppi_cluster1>;
> > +       };
> > +
> > +       psci {
> > +               compatible = "arm,psci-1.0";
> > +               method = "smc";
> > +       };
> > +
> > +       timer: timer {
> > +               compatible = "arm,armv8-timer";
> > +               interrupt-parent = <&gic>;
> > +               interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH 0>,
> > +                            <GIC_PPI 14 IRQ_TYPE_LEVEL_HIGH 0>,
> > +                            <GIC_PPI 11 IRQ_TYPE_LEVEL_HIGH 0>,
> > +                            <GIC_PPI 10 IRQ_TYPE_LEVEL_HIGH 0>;
> > +               clock-frequency = <13000000>;
> 
> IIRC Marc already mentioned before that this should not be specified
> in
> the device tree. The firmware should have filled in the CNTFRQ
> register.
> 
> > +       };
> > +
> > +       soc {
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               compatible = "simple-bus";
> > +               ranges;
> > +
> > +               gic: interrupt-controller@c000000 {
> > +                       compatible = "arm,gic-v3";
> > +                       #interrupt-cells = <4>;
> > +                       #redistributor-regions = <1>;
> > +                       interrupt-parent = <&gic>;
> > +                       interrupt-controller;
> > +                       reg = <0 0x0c000000 0 0x40000>,
> > +                             <0 0x0c040000 0 0x200000>;
> > +                       interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH
> > 0>;
> > +
> > +                       ppi-partitions {
> > +                               ppi_cluster0: interrupt-partition-0 
> > {
> > +                                       affinity = <&cpu0 &cpu1
> > &cpu2 &cpu3>;
> > +                               };
> 
> Please add an extra line for readability.
> 
> > +                               ppi_cluster1: interrupt-partition-1 
> > {
> > +                                       affinity = <&cpu4 &cpu5
> > &cpu6 &cpu7>;
> > +                               };
> > +                       };
> > +               };
> > +
> > +               topckgen: syscon@10000000 {
> > +                       compatible = "mediatek,mt8195-topckgen",
> > "syscon";
> > +                       reg = <0 0x10000000 0 0x1000>;
> > +                       #clock-cells = <1>;
> > +               };
> > +
> > +               infracfg_ao: syscon@10001000 {
> > +                       compatible = "mediatek,mt8195-infracfg_ao", 
> > "syscon", "simple-mfd";
> > +                       reg = <0 0x10001000 0 0x1000>;
> > +                       #clock-cells = <1>;
> 
> Please add an extra line for readability.
> 
> > +                       infracfg_rst: reset-controller {
> > +                               compatible = "ti,syscon-reset";
> > +                               #reset-cells = <1>;
> > +                               ti,reset-bits = <
> > +                                       0x140 18 0x144 18 0 0
> > (ASSERT_SET | DEASSERT_SET | STATUS_NONE)
> > +                                       0x120 0 0x124 0 0
> > 0     (ASSERT_SET | DEASSERT_SET | STATUS_NONE)
> > +                                       0x730 10 0x734 10 0
> > 0     (ASSERT_SET | DEASSERT_SET | STATUS_NONE)
> > +                                       0x150 5 0x154 5 0
> > 0     (ASSERT_SET | DEASSERT_SET | STATUS_NONE)
> 
> This should be 7 cells per entry:
> 
>     ti,reset-bits =
>         <0x140 18 0x144 18 0 0 (ASSERT_SET | DEASSERT_SET |
> STATUS_NONE>,
>         <0x120 0 0x124 0 0 0 (ASSERT_SET | DEASSERT_SET |
> STATUS_NONE)>,
>         <...>,
>         <...>;
> 
> Also, please remove the extra spaces in each line. They aren't
> helping
> with formatting.
> 
> > +                               >;
> > +                       };
> > +               };
> > +
> > +               pio: pinctrl@10005000 {
> > +                       compatible = "mediatek,mt8195-pinctrl";
> > +                       reg = <0 0x10005000 0 0x1000>,
> > +                             <0 0x11d10000 0 0x1000>,
> > +                             <0 0x11d30000 0 0x1000>,
> > +                             <0 0x11d40000 0 0x1000>,
> > +                             <0 0x11e20000 0 0x1000>,
> > +                             <0 0x11eb0000 0 0x1000>,
> > +                             <0 0x11f40000 0 0x1000>,
> > +                             <0 0x1000b000 0 0x1000>;
> > +                       reg-names = "iocfg0", "iocfg_bm",
> > "iocfg_bl",
> > +                                   "iocfg_br", "iocfg_lm",
> > "iocfg_rb",
> > +                                   "iocfg_tl", "eint";
> > +                       gpio-controller;
> > +                       #gpio-cells = <2>;
> > +                       gpio-ranges = <&pio 0 0 144>;
> > +                       interrupt-controller;
> > +                       interrupts = <GIC_SPI 235
> > IRQ_TYPE_LEVEL_HIGH 0>;
> 
> This looks correct now (a comment I raised in v4). Thanks.
> 
> > +                       #interrupt-cells = <2>;
> > +               };
> 
> [...]
> 
> > +               xhci0: usb@11200000 {
> > +                       compatible = "mediatek,mt8195-xhci",
> > +                                    "mediatek,mtk-xhci";
> > +                       reg = <0 0x11200000 0 0x1000>,
> > +                             <0 0x11203e00 0 0x0100>;
> > +                       reg-names = "mac", "ippc";
> > +                       interrupts = <GIC_SPI 129
> > IRQ_TYPE_LEVEL_HIGH 0>;
> > +                       phys = <&u2port0 PHY_TYPE_USB2>,
> > +                              <&u3port0 PHY_TYPE_USB3>;
> > +                       assigned-clocks = <&topckgen
> > CLK_TOP_USB_TOP>,
> > +                                         <&topckgen
> > CLK_TOP_SSUSB_XHCI>;
> > +                       assigned-clock-parents = <&topckgen
> > CLK_TOP_UNIVPLL_D5_D4>,
> > +                                                <&topckgen
> > CLK_TOP_UNIVPLL_D5_D4>;
> > +                       clocks = <&infracfg_ao CLK_INFRA_AO_SSUSB>,
> > +                                <&infracfg_ao
> > CLK_INFRA_AO_SSUSB_XHCI>,
> > +                                <&topckgen CLK_TOP_SSUSB_REF>,
> > +                                <&apmixedsys CLK_APMIXED_USB1PLL>;
> > +                       clock-names = "sys_ck", "xhci_ck",
> > "ref_ck", "mcu_ck";
> 
> The binding for this needs to be fixed. It expects clocks in the
> order
> specified in the binding, and this doesn't match. 

ok

> Also, "dma_ck" is missing
> and will likely cause warnings to be generated.

only sys_ck is required, others are optional as described in binding

> 
> This goes for all the xhci device nodes.
> 
> > +                       status = "disabled";
> > +               };
> > +
> > +               mmc0: mmc@11230000 {
> > +                       compatible = "mediatek,mt8195-mmc",
> > +                                    "mediatek,mt8183-mmc";
> > +                       reg = <0 0x11230000 0 0x10000>,
> > +                             <0 0x11f50000 0 0x1000>;
> 
> The binding only allows one entry. Please fix the binding first.
> This was added with MT8183, and the fix should list the relavent
> commit.
> 
> > +                       interrupts = <GIC_SPI 131
> > IRQ_TYPE_LEVEL_HIGH 0>;
> > +                       clocks = <&topckgen CLK_TOP_MSDC50_0>,
> > +                                <&infracfg_ao CLK_INFRA_AO_MSDC0>,
> > +                                <&infracfg_ao
> > CLK_INFRA_AO_MSDC0_SRC>;
> > +                       clock-names = "source", "hclk",
> > "source_cg";
> > +                       status = "disabled";
> > +               };
> > +
> 
> [...]
> 
> > +
> > +               xhci1: usb@11290000 {
> > +                       compatible = "mediatek,mt8195-xhci",
> > +                                    "mediatek,mtk-xhci";
> > +                       reg = <0 0x11290000 0 0x1000>,
> > +                             <0 0x11293e00 0 0x0100>;
> > +                       reg-names = "mac", "ippc";
> > +                       interrupts = <GIC_SPI 530
> > IRQ_TYPE_LEVEL_HIGH 0>;
> > +                       phys = <&u2port1 PHY_TYPE_USB2>;
> 
> Shouldn't there be a USB3 phy?

currently only enable usb2, usb3 phy is used by pcie.

> 
> > +                       assigned-clocks = <&topckgen
> > CLK_TOP_USB_TOP_1P>,
> > +                                         <&topckgen
> > CLK_TOP_SSUSB_XHCI_1P>;
> > +                       assigned-clock-parents = <&topckgen
> > CLK_TOP_UNIVPLL_D5_D4>,
> > +                                                <&topckgen
> > CLK_TOP_UNIVPLL_D5_D4>;
> > +                       clocks = <&pericfg_ao
> > CLK_PERI_AO_SSUSB_1P_BUS>,
> > +                                <&topckgen CLK_TOP_SSUSB_P1_REF>,
> > +                                <&pericfg_ao
> > CLK_PERI_AO_SSUSB_1P_XHCI>,
> > +                                <&apmixedsys CLK_APMIXED_USB1PLL>;
> > +                       clock-names = "sys_ck", "ref_ck",
> > "xhci_ck", "mcu_ck";
> > +                       status = "disabled";
> > +               };
> > +
> > +               xhci2: usb@112a0000 {
> > +                       compatible = "mediatek,mt8195-xhci",
> > +                                    "mediatek,mtk-xhci";
> > +                       reg = <0 0x112a0000 0 0x1000>,
> > +                             <0 0x112a3e00 0 0x0100>;
> > +                       reg-names = "mac", "ippc";
> > +                       interrupts = <GIC_SPI 533
> > IRQ_TYPE_LEVEL_HIGH 0>;
> > +                       phys = <&u2port2 PHY_TYPE_USB2>;
> > +                       assigned-clocks = <&topckgen
> > CLK_TOP_USB_TOP_2P>,
> > +                                         <&topckgen
> > CLK_TOP_SSUSB_XHCI_2P>;
> > +                       assigned-clock-parents = <&topckgen
> > CLK_TOP_UNIVPLL_D5_D4>,
> > +                                                <&topckgen
> > CLK_TOP_UNIVPLL_D5_D4>;
> > +                       clocks = <&pericfg_ao
> > CLK_PERI_AO_SSUSB_2P_BUS>,
> > +                                <&topckgen CLK_TOP_SSUSB_P2_REF>,
> > +                                <&pericfg_ao
> > CLK_PERI_AO_SSUSB_2P_XHCI>;
> > +                       clock-names = "sys_ck", "ref_ck",
> > "xhci_ck";
> > +                       status = "disabled";
> > +               };
> > +
> > +               xhci3: usb@112b0000 {
> > +                       compatible = "mediatek,mt8195-xhci",
> > +                                    "mediatek,mtk-xhci";
> > +                       reg = <0 0x112b0000 0 0x1000>,
> > +                             <0 0x112b3e00 0 0x0100>;
> > +                       reg-names = "mac", "ippc";
> > +                       interrupts = <GIC_SPI 536
> > IRQ_TYPE_LEVEL_HIGH 0>;
> > +                       phys = <&u2port3 PHY_TYPE_USB2>;
> > +                       assigned-clocks = <&topckgen
> > CLK_TOP_USB_TOP_3P>,
> > +                                         <&topckgen
> > CLK_TOP_SSUSB_XHCI_3P>;
> > +                       assigned-clock-parents = <&topckgen
> > CLK_TOP_UNIVPLL_D5_D4>,
> > +                                                <&topckgen
> > CLK_TOP_UNIVPLL_D5_D4>;
> > +                       clocks = <&pericfg_ao
> > CLK_PERI_AO_SSUSB_3P_BUS>,
> > +                                <&pericfg_ao
> > CLK_PERI_AO_SSUSB_3P_XHCI>,
> > +                                <&topckgen CLK_TOP_SSUSB_P3_REF>;
> > +                       clock-names = "sys_ck", "xhci_ck",
> > "ref_ck";
> > +                       usb2-lpm-disable;
> 
> Could you explain why this is needed only for this controller?

This controller is fixed with a BT, there is something issue when
enable usb2 lpm, so just disabled tmp.

> 
> > +                       status = "disabled";
> > +               };
> > +
> > +               u3phy2: t-phy@11c40000 {
> 
> Just "phy" for the node name. (Or maybe "serdes".) t-phy is not
> generic.

following t-phy’s dt-binding. 
here using t-phy is to avoid dt-check warning, because it has some sub-
phys.

> > +                       compatible = "mediatek,mt8195-tphy",
> > "mediatek,generic-tphy-v3";
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +                       ranges = <0 0 0x11c40000 0x700>;
> > +                       status = "disabled";
> > +
> > +                       u2port2: usb-phy@0 {
> > +                               reg = <0x0 0x700>;
> > +                               clocks = <&topckgen
> > CLK_TOP_SSUSB_PHY_P2_REF>;
> > +                               clock-names = "ref";
> > +                               #phy-cells = <1>;
> > +                       };
> > +               };
> > +
> 
> [...]
> 
> > +               ufsphy: ufs-phy@11fa0000 {
> 
> I would have preferred "phy" for the device node, but this seems
> already
> defined in the binding.
> 
> This IP block is not listed in the datasheet I have, so I am unable
> to
> verify the properties listed here.
> 
> > +                       compatible = "mediatek,mt8195-ufsphy",
> > "mediatek,mt8183-ufsphy";
> > +                       reg = <0 0x11fa0000 0 0xc000>;
> > +                       clocks = <&clk26m>, <&clk26m>;
> > +                       clock-names = "unipro", "mp";
> > +                       #phy-cells = <0>;
> > +                       status = "disabled";
> > +               };
> > +
> 
> Most of the issues I raised in this version were issues with things
> not
> matching the bindings. Please apply your patches on -next and run
> `make dtbs_check`.

ok. I'll apply comments at next version. Thank you.

> 
> 
> ChenYu




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux