by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 2/6] dt-bindings: clock: Bindings for Meson T7 clock controller

On Thu, 15 Jun 2023 19:29:34 +0100, Lucas Tanure wrote:
> Add documentation for T7 the clock controller.
>
> Signed-off-by: Lucas Tanure <[email protected]>
> ---
> .../bindings/clock/amlogic,mesont7.yaml | 69 +++
> include/dt-bindings/clock/mesont7-clkc.h | 487 ++++++++++++++++++
> 2 files changed, 556 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml
> create mode 100644 include/dt-bindings/clock/mesont7-clkc.h
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
$id: http://devicetree.org/schemas/clock/amlogic,mesont7-clk.yaml
file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml
Documentation/devicetree/bindings/clock/amlogic,mesont7.example.dts:18.32-28.11: Warning (unit_address_vs_reg): /example-0/clock-controller: node has a reg or ranges property, but no unit name
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,mesont7.example.dtb: clock-controller: reg: [[0, 0], [0, 1180], [0, 32768], [0, 800], [0, 57408], [0, 188]] is too long
from schema $id: http://devicetree.org/schemas/clock/amlogic,mesont7-clk.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

2023-06-15 22:02:15

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH 2/6] dt-bindings: clock: Bindings for Meson T7 clock controller

Hey Lucas,

On Thu, Jun 15, 2023 at 07:29:34PM +0100, Lucas Tanure wrote:
> Add documentation for T7 the clock controller.

Other than the bot's complaint, few comments for ya.

> Signed-off-by: Lucas Tanure <[email protected]>
> ---
> .../bindings/clock/amlogic,mesont7.yaml | 69 +++
> include/dt-bindings/clock/mesont7-clkc.h | 487 ++++++++++++++++++
> 2 files changed, 556 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml
> create mode 100644 include/dt-bindings/clock/mesont7-clkc.h
>
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml b/Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml
> new file mode 100644
> index 000000000000..18e7cca0c0e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml

The filename should match the compatible - please test the bindings,
dt_binding_check will complain.

> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/amlogic,mesont7-clk.yaml#

and this should match the filename

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Amlogic Meson T7 Clock Controller
> +
> +maintainers:
> + - Lucas Tanure <[email protected]>
> +
> +description: |
> + The Amlogic Meson T7 clock controller generates and supplies clock to
> + various peripherals within the SoC.
> +
> + This binding uses common clock bindings
> + [1] Documentation/devicetree/bindings/clock/clock-bindings.txt

This doesn't add anything as you're adding a yaml binding. I'd drop it
(and the | from description: since you would no longer have formatting
to preserve).

> +
> +properties:
> + compatible:
> + const: amlogic,t7-clkc
> +
> + reg:
> + items:
> + - description: basic registers

What does "basic registers" mean? I think you should be more specific in
your description.

> + - description: pll registers
> + - description: cpu_clk registers
> +
> + reg-names:
> + items:
> + - const: basic
> + - const: pll
> + - const: cpu_clk
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + const: xtal
> +
> + '#clock-cells':
> + const: 1
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> + - clocks
> + - clock-names
> + - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> + # Clock controller node:
> + - |
> + clkc: clock-controller {

The comment above and the node name here can be dropped.
You do however need to change "clock-controller" to
"clock-controller@<addr>".

> + compatible = "amlogic,t7-clkc";
> + #clock-cells = <1>;
> + reg = <0x0 0x0000 0x00 0x49c>,
> + <0x0 0x8000 0x00 0x320>,
> + <0x0 0xe040 0x00 0x0bc>;

Drop the 0x0 stuff from here.

> + reg-names = "basic", "pll", "cpu_clk";
> + clocks = <&xtal>;
> + clock-names = "xtal";
> + status = "okay";

status can be dropped, okay is the default.

Cheers,
Conor.

Attachments:

(No filename) (3.11 kB)
signature.asc (235.00 B)
Download all attachments

2023-06-15 22:02:36

On Fri, Jun 16, 2023 at 1:01 AM Yixun Lan <[email protected]> wrote:
>
> Hi Lucas:
>
> On 19:29 Thu 15 Jun , Lucas Tanure wrote:
> > The Khadas VIM4 uses the Amlogic A311D2 SoC, based on the Amlogic T7 SoC
> > family, on a board with the same form factor as the VIM3 models.
> I'd like to see little bit more verbose messages here, like
> which functionality/driver added here - cpu, gic, timer, uart?
Ok, I add this information in the next version.
Its adding CPU, GIC, and UART.

>
> so, it's capable of booting into a serial console?
yes

>
> >
> > - 8GB LPDDR4X 2016MHz
> > - 32GB eMMC 5.1 storage
> > - 32MB SPI flash
> > - 10/100/1000 Base-T Ethernet
> > - AP6275S Wireless (802.11 a/b/g/n/ac/ax, BT5.1)
> > - HDMI 2.1 video
> > - HDMI Input
> > - 1x USB 2.0 + 1x USB 3.0 ports
> > - 1x USB-C (power) with USB 2.0 OTG
> > - 3x LED's (1x red, 1x blue, 1x white)
> > - 3x buttons (power, function, reset)
> > - M2 socket with PCIe, USB, ADC & I2C
> > - 40pin GPIO Header
> > - 1x micro SD card slot
> >
> > Signed-off-by: Lucas Tanure <[email protected]>
> > ---
> > arch/arm64/boot/dts/amlogic/Makefile | 1 +
> > .../amlogic/meson-t7-a311d2-khadas-vim4.dts | 112 ++++++++++
> > arch/arm64/boot/dts/amlogic/meson-t7.dtsi | 202 ++++++++++++++++++
> > 3 files changed, 315 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/amlogic/meson-t7-a311d2-khadas-vim4.dts
> > create mode 100644 arch/arm64/boot/dts/amlogic/meson-t7.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/amlogic/Makefile b/arch/arm64/boot/dts/amlogic/Makefile
> > index cd1c5b04890a..1c5846bd1ca0 100644
> > --- a/arch/arm64/boot/dts/amlogic/Makefile
> > +++ b/arch/arm64/boot/dts/amlogic/Makefile
> > @@ -74,3 +74,4 @@ dtb-$(CONFIG_ARCH_MESON) += meson-sm1-odroid-hc4.dtb
> > dtb-$(CONFIG_ARCH_MESON) += meson-sm1-sei610.dtb
> > dtb-$(CONFIG_ARCH_MESON) += meson-sm1-x96-air-gbit.dtb
> > dtb-$(CONFIG_ARCH_MESON) += meson-sm1-x96-air.dtb
> > +dtb-$(CONFIG_ARCH_MESON) += meson-t7-a311d2-khadas-vim4.dtb
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-t7-a311d2-khadas-vim4.dts b/arch/arm64/boot/dts/amlogic/meson-t7-a311d2-khadas-vim4.dts
> > new file mode 100644
> > index 000000000000..46e175536edf
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/amlogic/meson-t7-a311d2-khadas-vim4.dts
> > @@ -0,0 +1,112 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (c) 2022 Wesion, Inc. All rights reserved.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "meson-t7.dtsi"
> > +
> > +/ {
> > + model = "Khadas VIM4";
> > +
> > + aliases {
> > + serial0 = &uart_A;
> > + };
> > +
> > + reserved-memory {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + /* 3 MiB reserved for ARM Trusted Firmware (BL31) */
> > + secmon_reserved: secmon@5000000 {
> > + reg = <0x0 0x05000000 0x0 0x300000>;
> > + no-map;
> > + };
> > +
> > + /* 32 MiB reserved for ARM Trusted Firmware (BL32) */
> > + secmon_reserved_bl32: secmon@5300000 {
> > + reg = <0x0 0x05300000 0x0 0x2000000>;
> > + no-map;
> > + };
> > + };
> > +
> > + xtal: xtal-clk {
> > + compatible = "fixed-clock";
> > + clock-frequency = <24000000>;
> > + clock-output-names = "xtal";
> > + #clock-cells = <0>;
> > + };
> > +
> > + vddcpu_a: regulator-vddcpu-a {
> > + /*
> > + * MP8756GD Regulator.
> > + */
> > + compatible = "pwm-regulator";
> > +
> > + regulator-name = "VDDCPU_A";
> > + regulator-min-microvolt = <689000>;
> > + regulator-max-microvolt = <1049000>;
> > +
> > + regulator-boot-on;
> > + regulator-always-on;
> > + };
> > +
> > + vddcpu_b: regulator-vddcpu-a {
> > + /*
> > + * MP8756GD Regulator.
> > + */
> > + compatible = "pwm-regulator";
> > +
> > + regulator-name = "VDDCPU_B";
> > + regulator-min-microvolt = <689000>;
> > + regulator-max-microvolt = <1049000>;
> > +
> > + regulator-boot-on;
> > + regulator-always-on;
> > + };
> > +};
> > +
> > +&clkc{
> > + clocks = <&xtal>;
> > + clock-names = "xtal";
> > + status = "okay";
> > +};
> > +
> > +&uart_A {
> > + status = "okay";
> > +};
> > +
> > +&cpu0 {
> > + cpu-supply = <&vddcpu_a>;
> > +};
> > +
> > +&cpu1 {
> > + cpu-supply = <&vddcpu_a>;
> > +};
> > +
> > +&cpu2 {
> > + cpu-supply = <&vddcpu_a>;
> > +};
> > +
> > +&cpu3 {
> > + cpu-supply = <&vddcpu_a>;
> > +};
> > +
> > +&cpu100 {
> > + cpu-supply = <&vddcpu_b>;
> > +};
> > +
> > +&cpu101 {
> > + cpu-supply = <&vddcpu_b>;
> > +};
> > +
> > +&cpu102 {
> > + cpu-supply = <&vddcpu_b>;
> > +};
> > +
> > +&cpu103 {
> > + cpu-supply = <&vddcpu_b>;
> > +};
> > +
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-t7.dtsi b/arch/arm64/boot/dts/amlogic/meson-t7.dtsi
> > new file mode 100644
> > index 000000000000..453b3d9cb9d8
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/amlogic/meson-t7.dtsi
> > @@ -0,0 +1,202 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> > + */
> > +
> > +#include <dt-bindings/clock/mesont7-clkc.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +/ {
> > + compatible = "amlogic,t7";
> > + interrupt-parent = <&gic>;
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + cpus {
> > + #address-cells = <0x2>;
> > + #size-cells = <0x0>;
> > +
> > + cpu-map {
> > + cluster0 {
> > + core0 {
> > + cpu = <&cpu100>;
> > + };
> > + core1 {
> > + cpu = <&cpu101>;
> > + };
> > + core2 {
> > + cpu = <&cpu102>;
> > + };
> > + core3 {
> > + cpu = <&cpu103>;
> > + };
> > + };
> > +
> > + cluster1 {
> > + core0 {
> > + cpu = <&cpu0>;
> > + };
> > + core1 {
> > + cpu = <&cpu1>;
> > + };
> > + core2 {
> > + cpu = <&cpu2>;
> > + };
> > + core3 {
> > + cpu = <&cpu3>;
> > + };
> > + };
> > + };
> > +
> > + cpu100: cpu@100 {
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a53";
> > + reg = <0x0 0x100>;
> > + enable-method = "psci";
> > + capacity-dmips-mhz = <632>;
> > + dynamic-power-coefficient = <110>;
> > + #cooling-cells = <2>;
> > + };
> > +
> > + cpu101: cpu@101{
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a53";
> > + reg = <0x0 0x101>;
> > + enable-method = "psci";
> > + capacity-dmips-mhz = <632>;
> > + dynamic-power-coefficient = <110>;
> > + #cooling-cells = <2>;
> > + };
> > +
> > + cpu102: cpu@102 {
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a53";
> > + reg = <0x0 0x102>;
> > + enable-method = "psci";
> > + capacity-dmips-mhz = <632>;
> > + dynamic-power-coefficient = <110>;
> > + #cooling-cells = <2>;
> > + };
> > +
> > + cpu103: cpu@103 {
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a53";
> > + reg = <0x0 0x103>;
> > + enable-method = "psci";
> > + capacity-dmips-mhz = <632>;
> > + dynamic-power-coefficient = <110>;
> > + #cooling-cells = <2>;
> > + };
> > +
> > + cpu0: cpu@0 {
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a73";
> > + reg = <0x0 0x0>;
> > + enable-method = "psci";
> > + capacity-dmips-mhz = <1024>;
> > + dynamic-power-coefficient = <550>;
> > + #cooling-cells = <2>;
> > + };
> > +
> > + cpu1: cpu@1 {
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a73";
> > + reg = <0x0 0x1>;
> > + enable-method = "psci";
> > + capacity-dmips-mhz = <1024>;
> > + dynamic-power-coefficient = <550>;
> > + #cooling-cells = <2>;
> > + };
> > +
> > + cpu2: cpu@2 {
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a73";
> > + reg = <0x0 0x2>;
> > + enable-method = "psci";
> > + capacity-dmips-mhz = <1024>;
> > + dynamic-power-coefficient = <550>;
> > + #cooling-cells = <2>;
> > + };
> > +
> > + cpu3: cpu@3 {
> > + device_type = "cpu";
> > + compatible = "arm,cortex-a73";
> > + reg = <0x0 0x3>;
> > + enable-method = "psci";
> > + capacity-dmips-mhz = <1024>;
> > + dynamic-power-coefficient = <550>;
> > + #cooling-cells = <2>;
> > + };
> > + };
> > +
> > + timer {
> > + compatible = "arm,armv8-timer";
> > + interrupts = <GIC_PPI 13 0xff08>,
> > + <GIC_PPI 14 0xff08>,
> > + <GIC_PPI 11 0xff08>,
> > + <GIC_PPI 10 0xff08>;
> > + };
> > +
> > + gic: interrupt-controller@fff01000 {
> > + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
> > + #interrupt-cells = <3>;
> > + #address-cells = <0>;
> > + interrupt-controller;
> > + reg = <0x0 0xfff01000 0 0x1000>,
> > + <0x0 0xfff02000 0 0x0100>;
> > + interrupts = <GIC_PPI 9 0xf04>;
> > + };
> > +
> > + psci {
> > + compatible = "arm,psci-0.2";
> can you double check if it is actual version 0.2?
> most recent Amlogic SoC should support psci-1.0
>
> > + method = "smc";
> > + };
> > +
> > + sm: secure-monitor {
> > + compatible = "amlogic,meson-gxbb-sm";
> > + };
> > +
> > + soc {
> > + compatible = "simple-bus";
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + apb4: apb4@fe000000 {
> > + compatible = "simple-bus";
> > + reg = <0x0 0xfe000000 0x0 0x480000>;
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x480000>;
> > +
> > + clkc: clock-controller {
> > + compatible = "amlogic,t7-clkc";
> > + #clock-cells = <1>;
> > + reg = <0x0 0x0 0x0 0x49c>,
> > + <0x0 0x8000 0x0 0x320>,
> > + <0x0 0xe040 0x0 0xbc>;
> > + reg-names = "basic",
> > + "pll",
> > + "cpu_clk";
> > + };
> > +
> > + ao-secure@140 {
> > + compatible = "amlogic,meson-gx-ao-secure", "syscon";
> > + reg=<0x0 0x10220 0x0 0x140>;
> > + amlogic,has-chip-id;
> > + };
> > + };
> > +
> > + uart_A: serial@fe078000 {
> > + compatible = "amlogic,meson-t7-uart";
> > + reg = <0x0 0xfe078000 0x0 0x18>;
> > + interrupts = <0 168 1>;
> > + status = "disabled";
> > + clocks = <&xtal>, <&clkc CLKID_UART_A>, <&xtal>;
> > + clock-names = "xtal", "pclk", "baud";
> > + fifo-size = < 64 >;
> > + pinctrl-names = "default";
> > + };
> I believe there are more uart ports, it's worth the effort to add them all in one run,
> which sounds more consistent to me, anyway you could also choose to add them
> in later patch series, no problem..

Yeah, I prefer to add those uarts later, when I can propper test them.

>
> > + };
> > +};
> > +
> > --
> > 2.41.0
> >
> >
> > _______________________________________________
> > linux-amlogic mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-amlogic
>
> --
> Yixun Lan (dlan)
> Gentoo Linux Developer
> GPG Key ID AABEFD55

2023-06-18 19:20:08

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH 2/6] dt-bindings: clock: Bindings for Meson T7 clock controller

On Sun, Jun 18, 2023 at 06:39:17PM +0100, Lucas Tanure wrote:
> On Thu, Jun 15, 2023 at 10:41 PM Conor Dooley <[email protected]> wrote:
> >
> > Hey Lucas,
> >
> > On Thu, Jun 15, 2023 at 07:29:34PM +0100, Lucas Tanure wrote:
> > > Add documentation for T7 the clock controller.
> >
> > Other than the bot's complaint, few comments for ya.
> >
> > > Signed-off-by: Lucas Tanure <[email protected]>
> > > ---
> > > .../bindings/clock/amlogic,mesont7.yaml | 69 +++
> > > include/dt-bindings/clock/mesont7-clkc.h | 487 ++++++++++++++++++
> > > 2 files changed, 556 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml
> > > create mode 100644 include/dt-bindings/clock/mesont7-clkc.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml b/Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml
> > > new file mode 100644
> > > index 000000000000..18e7cca0c0e1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/amlogic,mesont7.yaml
> >
> > The filename should match the compatible - please test the bindings,
> > dt_binding_check will complain.
> >
> > > @@ -0,0 +1,69 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/clock/amlogic,mesont7-clk.yaml#
> >
> > and this should match the filename
> >
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Amlogic Meson T7 Clock Controller
> > > +
> > > +maintainers:
> > > + - Lucas Tanure <[email protected]>
> > > +
> > > +description: |
> > > + The Amlogic Meson T7 clock controller generates and supplies clock to
> > > + various peripherals within the SoC.
> > > +
> > > + This binding uses common clock bindings
> > > + [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >
> > This doesn't add anything as you're adding a yaml binding. I'd drop it
> > (and the | from description: since you would no longer have formatting
> > to preserve).
> >
> > > +
> > > +properties:
> > > + compatible:
> > > + const: amlogic,t7-clkc
> > > +
> > > + reg:
> > > + items:
> > > + - description: basic registers
> >
> > What does "basic registers" mean? I think you should be more specific in
> > your description.
> >
> > > + - description: pll registers
> > > + - description: cpu_clk registers
> > > +
> > > + reg-names:
> > > + items:
> > > + - const: basic
> > > + - const: pll
> > > + - const: cpu_clk
> > > +
> > > + clocks:
> > > + maxItems: 1
> > > +
> > > + clock-names:
> > > + const: xtal
> > > +
> > > + '#clock-cells':
> > > + const: 1
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - reg-names
> > > + - clocks
> > > + - clock-names
> > > + - '#clock-cells'
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + # Clock controller node:
> > > + - |
> > > + clkc: clock-controller {
> >
> > The comment above and the node name here can be dropped.
> > You do however need to change "clock-controller" to
> > "clock-controller@<addr>".
> >
> > > + compatible = "amlogic,t7-clkc";
> > > + #clock-cells = <1>;
> > > + reg = <0x0 0x0000 0x00 0x49c>,
> > > + <0x0 0x8000 0x00 0x320>,
> > > + <0x0 0xe040 0x00 0x0bc>;
> >
> > Drop the 0x0 stuff from here.
> >
> > > + reg-names = "basic", "pll", "cpu_clk";
> > > + clocks = <&xtal>;
> > > + clock-names = "xtal";
> > > + status = "okay";
> >
> > status can be dropped, okay is the default.
> >
> > Cheers,
> > Conor.
> >
> I will drop this patch for now, wait for the S4 clock driver to be
> upstreamed, and use that as a base.

Why? Is the t7-clkc IP the same or very similar to the S4?

Attachments:

(No filename) (3.86 kB)
signature.asc (235.00 B)
Download all attachments

2023-06-18 19:24:25

by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH 5/6] tty: serial: meson: Added T7 SOC compatibility

On Sun, Jun 18, 2023 at 06:40:12PM +0100, Lucas Tanure wrote:
> On Fri, Jun 16, 2023 at 12:31 AM Yixun Lan <[email protected]> wrote:
> >
> > Hi Lucas:
> >
> > On 19:29 Thu 15 Jun , Lucas Tanure wrote:
> > > Make UART driver compatible with T7 SOC UART.
> > >
> > > Signed-off-by: Lucas Tanure <[email protected]>
> > > ---
> > > drivers/tty/serial/meson_uart.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> > > index 2501db5a7aaf..0208f9a6ba7e 100644
> > > --- a/drivers/tty/serial/meson_uart.c
> > > +++ b/drivers/tty/serial/meson_uart.c
> > > @@ -796,6 +796,10 @@ static const struct of_device_id meson_uart_dt_match[] = {
> > > .compatible = "amlogic,meson-s4-uart",
> > > .data = (void *)&meson_g12a_uart_data,
> > > },
> > > + {
> > > + .compatible = "amlogic,meson-t7-uart",
> > > + .data = (void *)&meson_g12a_uart_data,
> > I think you are trying to follow previous s4 scheme - to introduce a new
> > compatible string, while I think it's not necessary or even wrong, this will just
> > make the dt_match_list longer but without obvious benefits..
> >
> > as Conor already raised this question in previous dt-binding patch[4/6],
> > how about just using 'amlogic,meson-g12a-uart' which is the first compatible
> > introduced.
> >
> > if people agree, we could also drop 'amlogic,meson-s4-uart' since it use same
> > compatible data as gl12a, anyway it should be separated into another patch..

> Yes, I am dropping this patch and using s4 and g12a compatible lines.

If you drop the "amlogic,meson-s4-uart" from here it will break
backwards compatibility, because the binding permits
"amlogic,meson-s4-uart" in isolation. Please do not make that change.

When you introduce the new compatible for the t7, it can fall back to
the s4 (or g12a), doesn't really matter, but the existing one for the s4
should not be touched.

Cheers,
Conor.

Attachments:

(No filename) (2.01 kB)
signature.asc (235.00 B)
Download all attachments

2023-06-21 05:19:34

by kernel test robot

[permalink] [raw]