Re: [PATCH 3/3] ARM: dts: Prepare Realtek RTD1195 and MeLE X1000

[Rob Herring]

On Tue, Oct 29, 2019 at 10:52 AM Andreas FÃrber <afaerber@xxxxxxx> wrote:
>
> Am 29.10.19 um 16:41 schrieb Rob Herring:
> > On Mon, Oct 21, 2019 at 04:10:35AM +0200, Andreas FÃrber wrote:
> >> Add Device Trees for Realtek RTD1195 SoC and MeLE X1000 TV box.
> >>
> >> Reuse the existing RTD1295 watchdog compatible for now.
> >>
> >> Signed-off-by: Andreas FÃrber <afaerber@xxxxxxx>
> >> ---
> >>  arch/arm/boot/dts/Makefile               |   2 +
> >>  arch/arm/boot/dts/rtd1195-mele-x1000.dts |  30 ++++++++
> >>  arch/arm/boot/dts/rtd1195.dtsi           | 128 +++++++++++++++++++++++++++++++
> >>  3 files changed, 160 insertions(+)
> >>  create mode 100644 arch/arm/boot/dts/rtd1195-mele-x1000.dts
> >>  create mode 100644 arch/arm/boot/dts/rtd1195.dtsi
> >>
> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> >> index 73d33611c372..89a951485da8 100644
> >> --- a/arch/arm/boot/dts/Makefile
> >> +++ b/arch/arm/boot/dts/Makefile
> >> @@ -858,6 +858,8 @@ dtb-$(CONFIG_ARCH_QCOM) += \
> >>  dtb-$(CONFIG_ARCH_RDA) += \
> >>      rda8810pl-orangepi-2g-iot.dtb \
> >>      rda8810pl-orangepi-i96.dtb
> >> +dtb-$(CONFIG_ARCH_REALTEK) += \
> >> +    rtd1195-mele-x1000.dtb
> >>  dtb-$(CONFIG_ARCH_REALVIEW) += \
> >>      arm-realview-pb1176.dtb \
> >>      arm-realview-pb11mp.dtb \
> >> diff --git a/arch/arm/boot/dts/rtd1195-mele-x1000.dts b/arch/arm/boot/dts/rtd1195-mele-x1000.dts
> >> new file mode 100644
> >> index 000000000000..ce9a255950d3
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/rtd1195-mele-x1000.dts
> >> @@ -0,0 +1,30 @@
> >> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> >> +/*
> >> + * Copyright (c) 2017 Andreas FÃrber
> >
> > 2019?
>
> Nope, I am flushing out old queues, and updating SPDX line does not
> really warrant a copyright bump IMO. The changes below would though.
>
> >> + */
> >> +
> >> +/dts-v1/;
> >> +
> >> +#include "rtd1195.dtsi"
> >> +
> >> +/ {
> >> +    compatible = "mele,x1000", "realtek,rtd1195";
> >> +    model = "MeLE X1000";
> >> +
> >> +    aliases {
> >> +            serial0 = &uart0;
> >> +    };
> >> +
> >> +    chosen {
> >> +            stdout-path = "serial0:115200n8";
> >> +    };
> >> +
> >> +    memory {
> >
> > memory@0
>
> Will test.
>
> >
> >> +            device_type = "memory";
> >> +            reg = <0x0 0x40000000>;
> >> +    };
> >> +};
> >> +
> >> +&uart0 {
> >> +    status = "okay";
> >> +};
> >> diff --git a/arch/arm/boot/dts/rtd1195.dtsi b/arch/arm/boot/dts/rtd1195.dtsi
> >> new file mode 100644
> >> index 000000000000..475740c67d26
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/rtd1195.dtsi
> >> @@ -0,0 +1,128 @@
> >> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> >> +/*
> >> + * Copyright (c) 2017 Andreas FÃrber
> >> + */
> >> +
> >> +/memreserve/ 0x00000000 0x0000c000; /* boot code */
> >> +/memreserve/ 0x0000c000 0x000f4000;
> >> +/memreserve/ 0x01b00000 0x00400000; /* audio */
> >> +/memreserve/ 0x01ffe000 0x00004000; /* rpc ringbuf */
> >> +/memreserve/ 0x10000000 0x00100000; /* secure */
> >> +/memreserve/ 0x17fff000 0x00001000;
> >> +/memreserve/ 0x18000000 0x00100000; /* rbus */
> >> +/memreserve/ 0x18100000 0x01000000; /* nor */
> >
> > You shouldn't have the same entries here and in /reserved-memory. There
> > was a time before /reserved-memory was fully supported, but we should be
> > well past that now.
>
> I am dealing with a v2012.07 based downstream U-Boot that I do not have
> sources for, so I wouldn't be so sure there... It will only respect
> memreserve I think, whereas reserved-memory below is for the kernel, no?

Sigh... Well, that may be too old. :(

I could be wrong too and no one ever added /reserved-memory support
for u-boot. The intent was never that one was for u-boot and the other
for the kernel. The kernel handles both. reserved-memory was to
overcome the limitations of memreserve.

> >> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +
> >> +/ {
> >> +    compatible = "realtek,rtd1195";
> >> +    interrupt-parent = <&gic>;
> >> +    #address-cells = <1>;
> >> +    #size-cells = <1>;
> >> +
> >> +    cpus {
> >> +            #address-cells = <1>;
> >> +            #size-cells = <0>;
> >> +
> >> +            cpu0: cpu@0 {
> >> +                    device_type = "cpu";
> >> +                    compatible = "arm,cortex-a7";
> >> +                    reg = <0x0>;
> >> +                    clock-frequency = <1000000000>;
> >> +            };
> >> +
> >> +            cpu1: cpu@1 {
> >> +                    device_type = "cpu";
> >> +                    compatible = "arm,cortex-a7";
> >> +                    reg = <0x1>;
> >> +                    clock-frequency = <1000000000>;
> >> +            };
> >> +    };
> >> +
> >> +    reserved-memory {
> >> +            #address-cells = <1>;
> >> +            #size-cells = <1>;
> >> +            ranges;
> >> +
> >> +            secure@10000000 {
> >> +                    reg = <0x10000000 0x100000>;
> >> +                    no-map;
> >> +            };
> >> +
> >> +            rbus@18000000 {
> >> +                    reg = <0x18000000 0x100000>;
> >> +                    no-map;
> >
> > This doesn't look right as it overlaps the register space.
>
> Will try dropping it. James?
>
> >> +            };
> >> +
> >> +            nor@18100000 {
> >> +                    reg = <0x18100000 0x1000000>;
> >> +                    no-map;
> >> +            };
>
> Same issue here, I guess?

Yes.

Rob