Re: [RFC PATCH v3 25/35] Documentation/devicetree/bindings/sh/cpus.yaml: Add SH CPU.

[Yoshinori Sato <ysato@users.sourceforge.jp>]

On Wed, 18 Oct 2023 23:27:43 +0900,
Geert Uytterhoeven wrote:
> 
> Hi Sato-san,
> 
> On Sat, Oct 14, 2023 at 4:54 PM Yoshinori Sato
> <ysato@users.sourceforge.jp> wrote:
> > Renesas SuperH binding definition.
> >
> > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sh/cpus.yaml
> > @@ -0,0 +1,45 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sh/cpus.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas SuperH CPUs
> > +
> > +maintainers:
> > +  - Yoshinori Sato <ysato@users.sourceforge.jp>
> > +
> > +description: |+
> > +  The device tree allows to describe the layout of CPUs in a system through
> > +  the "cpus" node, which in turn contains a number of subnodes (ie "cpu")
> > +  defining properties for every cpu.
> > +
> > +  Bindings for CPU nodes follow the Devicetree Specification, available from:
> > +
> > +  https://www.devicetree.org/specifications/
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> 
> Missing
> 
>     - jcore,j2
> 
> > +          - renesas,sh4
> 
> 
> > +      - const: renesas,sh
> 
> I see arch/sh/boot/dts/j2_mimas_v2.dts lacks the fallback to
> "renesas,sh", though.
> Is there a common base of instructions that are available on all SH cores?

The base instruction set is sh2.
Before that, there is sh1, but this is not compatible with Linux.
I think it would be a good idea to change this to "renesas,sh2",
but the SH7619 (SH2 CPU) would look like the following.
cpus {
	cpu: cpu@0 {
        	compatible = "renesas,sh2", "renesas,sh2";
        };
};

> Missing reg property.
> Missing "device_type: true".
> 
> > +
> > +  clock-frequency:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      CPU core clock freqency.
> 
> Perhaps a "clocks" property instead, or as an alternative?
> 
> On sh7750, you do have
> 
>     clocks = <&cpg SH7750_CPG_ICK>;
> 
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
> > +        cpus {
> 
> make dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/sh/cpus.yaml:
> 
> Documentation/devicetree/bindings/sh/cpus.example.dtb: cpus:
> '#address-cells' is a required property
>         from schema $id: http://devicetree.org/schemas/cpus.yaml#
>         Documentation/devicetree/bindings/sh/cpus.example.dtb: cpus:
> '#size-cells' is a required property
> from schema $id: http://devicetree.org/schemas/cpus.yaml#
> 
> > +                cpu: cpu@0 {
> > +                      compatible = "renesas,sh4", "renesas,sh";
> 
> Documentation/devicetree/bindings/sh/cpus.example.dts:19.28-21.19:
> Warning (unit_address_vs_reg): /example-0/cpus/cpu@0: node has a unit
> name, but no reg or ranges property
> Documentation/devicetree/bindings/sh/cpus.example.dtb: cpus: cpu@0:
> 'cache-level' is a required property
>         from schema $id: http://devicetree.org/schemas/cpus.yaml#
> 
> > +                };
> > +        };
> > +...
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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