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

On Tue, 19 Sep 2023 22:25:19 +0900,
Geert Uytterhoeven wrote:
> 
> 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.

The only difference between 7751 and 7751R is CPG,
so I don't differentiate between them.

Shall we write the CPG differences in sh7751r.dtsi?

> > +
> > +/ {
> > +       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

-- 
Yosinori Sato