Re: [RESEND RFC PATCH 11/12] sh: target dts.

Hi Sato-san,

On Fri, Sep 1, 2023 at 12:43 AM Yoshinori Sato
<[email protected]> wrote:
> - rts7751r2dplus - Renesas RTS7751R2D-PLUS board.
> - landisk - IO DATA DEVICE LANDISK
> - usl-5p - IO DATA DECVICE USL-5P
>
> Signed-off-by: Yoshinori Sato <[email protected]>

Thanks for your patch!

>  arch/sh/boot/dts/include/dt-bindings |   1 +
>  arch/sh/boot/dts/landisk.dts         | 142 ++++++++++++++++++++++
>  arch/sh/boot/dts/rts7751r2dplus.dts  | 168 +++++++++++++++++++++++++++
>  arch/sh/boot/dts/usl-5p.dts          | 146 +++++++++++++++++++++++

None of the DTBs are listed in arch/sh/boot/dts/Makefile, so how do you
build them without CONFIG_USE_BUILTIN_DTB/CONFIG_BUILTIN_DTB_SOURCE?

To make "make dtbs" and "make dtbs_check" work, I added:

     dtb-$(CONFIG_CPU_J2) += j2_mimas_v2.dtb
     dtb-$(CONFIG_CPU_SUBTYPE_SH7751R) += landisk.dtb
     dtb-$(CONFIG_CPU_SUBTYPE_SH7751R) += rts7751r2dplus.dtb
     dtb-$(CONFIG_CPU_SUBTYPE_SH7751R) += usl-5p.dtb

> --- /dev/null
> +++ b/arch/sh/boot/dts/landisk.dts
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree source for IO DATA DEVICE LANDISK
> + *
> + * Copyright 2023 Yoshinori Sato
> + */
> +
> +#include <dt-bindings/interrupt-controller/sh_intc.h>
> +
> +/dts-v1/;
> +/ {
> +       model = "IO-DATA Device LANDISK";
> +       compatible = "iodata,landisk";

compatible = "iodata,landisk", "renesas,sh7380", "renesas,sh7751r".

> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       interrupt-parent = <&shintc>;

Empty line between properties and subnodes (everywhere).

> +       chosen {
> +               stdout-path = &sci1;

     stdout-path = "serial0";

or

     stdout-path = "serial0:9600n8";

> +       };

Empty line between subnodes (everywhere).

> +       aliases {
> +               serial1 = &sci1;
> +       };
> +
> +       oclk: oscillator {
> +               #clock-cells = <0>;
> +               compatible = "fixed-clock";
> +               clock-frequency = <22222222>;
> +       };
> +       pllclk: pllclk {
> +               compatible = "renesas,sh7750-pll-clock";
> +               clocks = <&oclk>;
> +               #clock-cells = <0>;
> +               sh7750,md = <5>;
> +               sh7750,rtype = <1>;
> +               reg = <0xffc00000 2>, <0xffc00008 4>;
> +       };
> +       iclk: iclk {
> +               compatible = "renesas,sh7750-div-clock";
> +               clocks = <&pllclk>;
> +               #clock-cells = <0>;
> +               reg = <0xffc00000 2>;
> +               renesas,offset = <6>;
> +               clock-output-names = "ick";
> +       };
> +       bclk: bclk {
> +               compatible = "renesas,sh7750-div-clock";
> +               clocks = <&pllclk>;
> +               #clock-cells = <0>;
> +               reg = <0xffc00000 2>;
> +               renesas,offset = <3>;
> +               clock-output-names = "bck";
> +       };
> +       fclk: fclk {
> +               compatible = "renesas,sh7750-div-clock";
> +               clocks = <&pllclk>;
> +               #clock-cells = <0>;
> +               reg = <0xffc00000 2>;
> +               renesas,offset = <0>;
> +               clock-output-names = "fck";
> +       };

Having individual clock nodes (especially if they use the same register
block) is deprecated.  Please use a single node with compatible value
e.g. "renesas,sh7751r-cpg" and "#clock-cells = <1>", and let the driver
create all clocks itself.

> +       sci1: serial@ffe80000 {

label "scif" (serial@ffe00000 is "sci").

> +               compatible = "renesas,scif";

"renesas,scif-sh7751r", "renesas,scif" ?

> +               reg = <0xffe80000 0x100>;
> +               interrupts = <evt2irq(0x700) 0
> +                             evt2irq(0x720) 0
> +                             evt2irq(0x760) 0
> +                             evt2irq(0x740) 0>;

Please group using angular brackets:

              interrupts = <evt2irq(0x700) 0>,
                            <evt2irq(0x720) 0>,
                            <evt2irq(0x760) 0>,
                            <evt2irq(0x740) 0>;

Missing interrupt-names

> +               clocks = <&fclk>;
> +               clock-names = "fck";
> +       };
> +       tmu: timer@ffd80008 {
> +               compatible = "renesas,tmu";
> +               reg = <0xffd80000 12>;
> +               interrupts = <evt2irq(0x400) 0
> +                             evt2irq(0x420) 0
> +                             evt2irq(0x440) 0>;

Please group using angular brackets

> +               clocks = <&fclk>;
> +               clock-names = "fck";
> +               renesas,channels = <0x03>;

renesas,channels = <5>;

> +       };
> +
> +       pci@fe200000 {
> +               compatible = "renesas,sh7751-pci", "iodata,julian";

Most-specific first, i.e.

    compatible = "iodata,julian", "renesas,sh7751-pci";

(and we do need DT binding documentation!)

However, "iodata,julian" is a too-generic name.

> +               device_type = "pci";
> +               bus-range = <0 0>;
> +               #address-cells = <3>;
> +               #size-cells = <2>;
> +               ranges = <0x02000000 0 0xfd000000 0xfd000000 0 0x01000000>,
> +                        <0x01000000 0 0xfe240000 0xfe240000 0 0x00040000>;
> +               reg = <0xfe200000 0x0400>,
> +                     <0x0c000000 0x04000000>,
> +                     <0xff800000 0x0030>;
> +               #interrupt-cells = <1>;
> +               interrupt-parent = <&julianintc>;
> +               eth@0,0 {
> +                       reg = <0x0000 0 0 0 0>;
> +                       interrupts = <5 0>;
> +               };
> +               ata@1,0 {
> +                       reg = <0x0800 0 0 0 0>;
> +                       interrupts = <6 0>;
> +               };
> +               usb@2,0 {
> +                       reg = <0x1000 0 0 0 0>;
> +                       interrupts = <7 0>;
> +               };
> +               usb@2,1 {
> +                       reg = <0x1100 0 0 0 0>;
> +                       interrupts = <8 0>;
> +               };
> +               usb@2,2 {
> +                       reg = <0x1200 0 0 0 0>;
> +                       interrupts = <5 0>;
> +               };
> +       };
> +};

All SoC-specific parts should be extracted into sh7751r.dtsi, so it can be
shared by all board DTS files.

> diff --git a/arch/sh/boot/dts/rts7751r2dplus.dts b/arch/sh/boot/dts/rts7751r2dplus.dts
> new file mode 100644
> index 000000000000..1d64753f47a2
> --- /dev/null
> +++ b/arch/sh/boot/dts/rts7751r2dplus.dts
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree source for Renesas RTS7751R2D Plus
> + *
> + * Copyright 2023 Yoshinori Sato
> + */
> +
> +#include <dt-bindings/interrupt-controller/sh_intc.h>
> +
> +/dts-v1/;
> +/ {
> +       model = "Renesas RTS7715R2D Plus";
> +       compatible = "renesas,r2dplus";

compatible = "renesas,r2dplus", "renesas,sh7751r".

> +       display@1,0 {
> +               compatible = "smi,sm501";
> +               reg = <0x10000000 0x03e00000
> +                      0x13e00000 0x00200000>;

Please group using angular brackets.

> +               interrupt-parent = <&r2dintc>;
> +               interrupts = <4 0>;
> +               mode = "640x480-16@60";
> +               little-endian;
> +               sm501,devices = "usb-host,uart0";
> +       };

> +       flash@0 {
> +               compatible = "cfi-flash";
> +               reg = <0x00000000 0x02000000>;
> +               device-width = <2>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               partition@0 {

Please wrap all partitions inside a "partitions" subnode.

> +                       label = "U-Boot";
> +                       reg = <0x00000000 0x00040000>;
> +               };
> +               partition@1 {

partition@40000

> +                       label = "Environemt";
> +                       reg = <0x00040000 0x00040000>;
> +               };
> +               partition@2 {

partition@80000

> +                       label = "Kernel";
> +                       reg = <0x00080000 0x001c0000>;
> +               };
> +               partition@3 {

partition@240000

> +                       label = "Flash_FS";
> +                       reg = <0x00240000 0x00dc0000>;
> +               };
> +       };
> +
> +       pci@fe200000 {
> +               compatible = "renesas,sh7751-pci", "renesas,r2d";

compatible = "renesas,r2d", "renesas,sh7751-pci";

However, "renesas,r2d" is a too-generic name.

> +               device_type = "pci";
> +               bus-range = <0 0>;
> +               #address-cells = <3>;
> +               #size-cells = <2>;
> +               ranges = <0x02000000 0 0xfd000000 0xfd000000 0 0x01000000>,
> +                        <0x01000000 0 0xfe240000 0xfe240000 0 0x00040000>;
> +               reg = <0xfe200000 0x0400>,
> +                     <0x0c000000 0x04000000>,
> +                     <0xff800000 0x0030>;
> +               #interrupt-cells = <1>;
> +               interrupt-parent = <&r2dintc>;
> +               eth@2,0 {
> +                       reg = <0x1000 0 0 0 0>;
> +                       interrupts = <3 0>;
> +               };
> +       };
> +};

> --- /dev/null
> +++ b/arch/sh/boot/dts/usl-5p.dts
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree source for IO DATA DEVICE USL-5P
> + *
> + * Copyright 2023 Yoshinori Sato
> + */
> +
> +#include <dt-bindings/interrupt-controller/sh_intc.h>
> +
> +/dts-v1/;
> +/ {
> +       model = "IO-DATA Device USL-5P";
> +       compatible = "iodata,usl-5p";

compatible = "iodata,usl-5p", "renesas,sh7380", "renesas,sh7751r".

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds