Re: [RFC PATCH v2 23/30] arch/sh: Add SH7751 SoC Internal periphreal 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/sh7751.dtsi
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the SH7751 SoC
> + */
> +
> +#include <dt-bindings/interrupt-controller/sh_intc.h>
> +#include <dt-bindings/clock/sh7750.h>
> +
> +/ {

Missing top-level compatible property:

    compatible = "renesas,sh7751r";

And of course this compatible value should be documented in
Documentation/devicetree/bindings/soc/renesas/renesas.yaml

> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;

Please add a blank line between properties and subnodes.

> +               cpu@0 {
> +                     compatible = "renesas,sh7751r","renesas,sh4", "renesas,sh";
> +               };
> +       };
> +
> +       clocks {

s/clocks/soc/, and move everything below inside the "soc" container.

> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
> +
> +               cpg: cpg@ffc00000 {

Please use generic node names: clock-controller@ffc00000;

> +                       #clock-cells = <1>;
> +                       compatible = "renesas,sh7750-cpg";
> +                       clocks = <&xtal>;
> +                       reg = <0xffc00000 2>, <0xffc00008 4>;
> +               };
> +
> +       };
> +
> +       shintc: interrupt-controller@ffd00000 {
> +               compatible = "renesas,sh7751-intc";
> +               #interrupt-cells = <2>;
> +               #address-cells = <0>;
> +               interrupt-controller;
> +               reg = <0xffd00000 14>, <0xfe080000 128>;
> +       };
> +
> +       /* sci0 is rarely used, so it is not defined here. */
> +       scif1: serial@ffe80000 {
> +               compatible = "renesas,scif";

Please add (and document!) "renesas,scif-sh7751".

> +               reg = <0xffe80000 0x100>;
> +               interrupts = <evt2irq(0x700) 0>,
> +                            <evt2irq(0x720) 0>,
> +                            <evt2irq(0x760) 0>,
> +                            <evt2irq(0x740) 0>;
> +               interrupt-names = "eri", "rxi", "bri", "txi";

make dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/serial/renesas,scif.yaml

    serial@ffe80000: interrupt-names: 'oneOf' conditional failed, one
must be fixed:
        ['eri', 'rxi', 'bri', 'txi'] is too short
        'txi' was expected
        'bri' was expected

The "bri" and "txi" entries must be exchanged, both in the
"interrupts" and "interrupt-names" properties.

> +               clocks = <&cpg SH7750_CPG_FCK>;

clock-names = "fck";

> +       };
> +
> +       /* Normally ch0 and ch1 are used, so we will define ch0 to ch2 here. */
> +       tmu0: timer@ffd80008 {
> +               compatible = "renesas,tmu";

Please add (and document!) "renesas,tmu-sh7751".

> +               reg = <0xffd80000 12>;
> +               interrupts = <evt2irq(0x400) 0>,
> +                            <evt2irq(0x420) 0>,
> +                            <evt2irq(0x440) 0>,
> +                            <evt2irq(0x460) 0>;
> +               interrupt-names = "tuni0", "tuni1", "tuni2", "ticpi2";

This will need an update to the bindings, and the driver, too.

> +               clocks = <&cpg SH7750_CPG_FCK>;

clock-names = "fck";

> +               renesas,channels = <3>;
> +       };

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