Re: [RFC PATCH v2 26/30] arch/sh: RTS7751R2D Plus DeviceTree.

Hi Sato-san,

On Wed, Sep 13, 2023 at 11:24 AM Yoshinori Sato
<[email protected]> wrote:
> Signed-off-by: Yoshinori Sato <[email protected]>

Thanks for your patch!

> --- /dev/null
> +++ b/arch/sh/boot/dts/rts7751r2dplus.dts
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the Renesas RTS7751R2D Plus
> + */
> +
> +/dts-v1/;
> +
> +#include "sh7751.dtsi"

#include "sh7751r.dtsi"?

To make that work, you can create "sh7751.dtsi" that includes
"sh7751.dtsi" and overrides the parts that are different.

> +
> +/ {
> +       model = "Renesas RTS7715R2D Plus";
> +       compatible = "renesas,r2dplus";

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

And all these compatible values must be documented in the DT
binding documentation.

> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       interrupt-parent = <&shintc>;
> +
> +       aliases {
> +               serial0 = &scif1;
> +       };
> +
> +       chosen {
> +       };
> +
> +       clocks {
> +               xtal: oscillator {
> +                       #clock-cells = <0>;
> +                       compatible = "fixed-clock";
> +                       clock-frequency = <22222222>;
> +               };

Please
> +
> +               cpg: cpg@ffc00000 {

"&cpg"?

> +                       compatible = "renesas,sh7750r-cpg",
> +                                    "renesas,sh7750-cpg";

There should be no need to override the cpg's compatible value.

> +                       renesas,mode = <5>;
> +               };
> +       };
> +
> +       cpus {
> +               cpu@0 {
> +                     clock-frequency = <266666666>;

As the CPU clock is programmable, the "clock-frequency"
property should probably be replaced by a "clocks" property in the
base sh7751.dtsi.

> +               };
> +       };
> +
> +       memory@c000000 {
> +               device_type = "memory";
> +               reg = <0x0c000000 0x4000000>;
> +       };
> +
> +       r2dintc: sh7751irl_encoder@a4000000 {
> +               compatible = "renesas,sh7751-irl-ext";
> +               reg = <0xa4000000 0x02>;
> +               interrupt-controller;
> +               #address-cells = <1>;
> +               #interrupt-cells = <2>;
> +               sh7751irl,width = <16>;
> +               sh7751irl,polarity = <0>;
> +               sh7751irl,irqbit =<11>,         /* PCI INTD */
> +                                 <9>,          /* CF IDE */
> +                                 <8>,          /* CF CD */
> +                                 <12>,         /* PCI INTC */
> +                                 <10>,         /* SM501 */
> +                                 <6>,          /* KEY */
> +                                 <5>,          /* RTC ALARM */
> +                                 <4>,          /* RTC T */
> +                                 <7>,          /* SDCARD */
> +                                 <14>,         /* PCI INTA */
> +                                 <13>,         /* PCI INTB */
> +                                 <0>,          /* EXT */
> +                                 <15>;         /* TP */
> +       };
> +
> +       display@1,0 {
> +               compatible = "smi,sm501";
> +               reg = <0x10000000 0x03e00000
> +                      0x13e00000 0x00200000>;
> +               interrupt-parent = <&r2dintc>;
> +               interrupts = <4 0>;
> +               mode = "640x480-16@60";
> +               little-endian;
> +               sm501,devices = "usb-host","uart0";
> +       };
> +
> +       compact-flash@b4001000 {
> +               compatible = "ata-generic";

compact-flash@b4001000: compatible:0: 'ata-generic' is not one of
['arm,vexpress-cf', 'fsl,mpc8349emitx-pata']
from schema $id: http://devicetree.org/schemas/ata/ata-generic.yaml#

> +               reg = <0xb4001000 0x0e>, <0xb400080c 2>;
> +               reg-shift = <1>;
> +               interrupt-parent = <&r2dintc>;
> +               interrupts = <1 0>;
> +       };
> +
> +       flash@0 {
> +               compatible = "cfi-flash";
> +               reg = <0x00000000 0x02000000>;
> +               device-width = <2>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               partition@0 {
> +                       label = "U-Boot";
> +                       reg = <0x00000000 0x00040000>;
> +               };
> +               partition@1 {
> +                       label = "Environemt";

Environment

> +                       reg = <0x00040000 0x00040000>;
> +               };

Several of the above comments apply to "[RFC PATCH v2 27/30]
arch/sh: LANDISK DeviceTree." and "[RFC PATCH v2 28/30] arch/sh:
USL-5P DeviceTree.", too.

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