Re: [PATCH v6 5/7] dt-bindings: MIPS: Document Ingenic SoCs binding.

[Paul Cercueil]

Hi,

Le mer., fÃvr. 26, 2020 at 10:29, Rob Herring <robh@xxxxxxxxxx> a 
Ãcrit :
> On Fri, Feb 21, 2020 at 12:24:47AM +0800, åçæ (Zhou Yanjie) 
> wrote:
> >  Document the available properties for the SoC root node and the
> >  CPU nodes of the devicetree for the Ingenic XBurst SoCs.
> >
> >  Tested-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
> >  Tested-by: Paul Boddie <paul@xxxxxxxxxxxxx>
> >  Signed-off-by: åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> >  ---
> >
> >  Notes:
> >      v1->v2:
> >      Change the two Document from txt to yaml.
> >
> >      v2->v3:
> >      Fix formatting errors.
> >
> >      v3->v4:
> >      Fix bugs in the two yaml files.
> >
> >      v4->v5:
> >      No change.
> >
> >      v5->v6:
> >      Rewrite the two yaml files.
> >
> >   .../bindings/mips/ingenic/ingenic,cpu.yaml         | 61 
> > ++++++++++++++++++++++
> >   .../bindings/mips/ingenic/ingenic,soc.yaml         | 34 
> > ++++++++++++
> >   2 files changed, 95 insertions(+)
> >   create mode 100644 
> > Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
> >   create mode 100644 
> > Documentation/devicetree/bindings/mips/ingenic/ingenic,soc.yaml
> >
> >  diff --git 
> > a/Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml 
> > b/Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
> >  new file mode 100644
> >  index 00000000..ad1fd86
> >  --- /dev/null
> >  +++ 
> > b/Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
> >  @@ -0,0 +1,61 @@
> >  +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >  +%YAML 1.2
> >  +---
> >  +$id: http://devicetree.org/schemas/mips/ingenic/ingenic,cpu.yaml#
> >  +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >  +
> >  +title: Bindings for Ingenic XBurst family CPUs
> >  +
> >  +maintainers:
> >  +  - åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
>
> Blank line here.
>
> >  +description: |
>
> Drop the '|'.
>
> >  +  Ingenic XBurst family CPUs shall have the following properties.
> >  +
> >  +properties:
> >  +  compatible:
> >  +    oneOf:
> >  +
> >  +      - description: Ingenic XBurstÂ1 CPU Core
> >  +        items:
> >  +          - const: ingenic,xburst
> >  +
> >  +      - description: Ingenic XBurstÂ2 CPU Core
> >  +        items:
> >  +          - const: ingenic,xburst2
>
> enum:
>   - ingenic,xburst  # Ingenic XBurstÂ1 CPU Core
>   - ingenic,xburst2 # Ingenic XBurstÂ2 CPU Core
>
> Though I don't find the description really adds much.

About the enum values: shouldn't they be a bit more descriptive? There 
has been various versions of the Xburst1 chip, with slightly different 
instruction sets and hardware (FPU).

-Paul

> >  +
> >  +  reg:
> >  +    description: |
> >  +      The number of the CPU.
>
> Drop this.
>
> Add:
>
> maxItems: 1
>
> >  +
> >  +required:
> >  +  - device_type
> >  +  - compatible
> >  +  - reg
> >  +
> >  +examples:
> >  +  - |
> >  +    #include <dt-bindings/clock/jz4780-cgu.h>
> >  +
> >  +    cpus {
> >  +    	#address-cells = <1>;
> >  +    	#size-cells = <0>;
> >  +
> >  +    	cpu0: cpu@0 {
> >  +    		device_type = "cpu";
> >  +    		compatible = "ingenic,xburst";
> >  +    		reg = <0>;
> >  +
>
> >  +    		clocks = <&cgu JZ4780_CLK_CPU>;
> >  +    		clock-names = "cpu";
>
> Not documented.
>
> >  +    	};
> >  +
> >  +    	cpu1: cpu@1 {
> >  +    		device_type = "cpu";
> >  +    		compatible = "ingenic,xburst";
> >  +    		reg = <1>;
> >  +
> >  +    		clocks = <&cgu JZ4780_CLK_CORE1>;
> >  +    		clock-names = "cpu";
> >  +    	};
> >  +    };
> >  +...
> >  diff --git 
> > a/Documentation/devicetree/bindings/mips/ingenic/ingenic,soc.yaml 
> > b/Documentation/devicetree/bindings/mips/ingenic/ingenic,soc.yaml
> >  new file mode 100644
> >  index 00000000..8943e73
> >  --- /dev/null
> >  +++ 
> > b/Documentation/devicetree/bindings/mips/ingenic/ingenic,soc.yaml
> >  @@ -0,0 +1,34 @@
> >  +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >  +%YAML 1.2
> >  +---
> >  +$id: http://devicetree.org/schemas/mips/ingenic/ingenic,soc.yaml#
> >  +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >  +
> >  +title: Bindings for Ingenic SoCs with XBurst CPU inside.
> >  +
> >  +maintainers:
> >  +  - åçæ (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
>
> Blank line.
>
> >  +description: |
> >  +  Ingenic SoCs with XBurst CPU inside shall have the following 
> > properties.
> >  +
> >  +properties:
> >  +  $nodename:
> >  +    const: '/'
> >  +  compatible:
> >  +    oneOf:
> >  +
> >  +      - description: Ingenic JZ47 Series Mobile Application 
> > Processor
> >  +        items:
> >  +          - const: ingenic,jz4740
> >  +          - const: ingenic,jz4725b
> >  +          - const: ingenic,jz4760
> >  +          - const: ingenic,jz4760b
> >  +          - const: ingenic,jz4770
> >  +          - const: ingenic,jz4780
>
> This is defining the root compatible is 6 strings. You want a enum 
> here
> I think.
>
> >  +
> >  +      - description: Ingenic X Series IoT Application Processor
> >  +        items:
> >  +          - const: ingenic,x1000
> >  +          - const: ingenic,x1000e
> >  +          - const: ingenic,x1500
>
> Same here.
>
> Did you validate your dts file with this schema using 'make 
> dtbs_check'?
>
> Rob