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

[Paul Cercueil <paul@crapouillou.net>]

Hi,

Le mer., févr. 26, 2020 at 10:29, Rob Herring <robh@kernel.org> 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@goldelico.com>
>>  Tested-by: Paul Boddie <paul@boddie.org.uk>
>>  Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>  ---
>> 
>>  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@wanyeetech.com>
> 
> 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@wanyeetech.com>
> 
> 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