Re: [PATCH v2] dt-bindings: mips: add CPU bindings for MIPS architecture

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 17/09/2022 06:11, Sergio Paracuellos wrote:
> Add the yaml binding for available CPUs in MIPS architecture.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Rebase on some recent Linux kernel tree and use scripts/get_maintainers.pl


> ---
> Changes in v2:
>  - Remove 'bindings/mips/brcm/brcm,bmips.txt'
>  - Include 'mips-hpt-frequency' in cpus YAML schema for bmips CPUS's
>  - Add a BMIPS CPU node sample
> 
>  .../bindings/mips/brcm/brcm,bmips.txt         |   8 --
>  .../devicetree/bindings/mips/cpus.yaml        | 100 ++++++++++++++++++
>  2 files changed, 100 insertions(+), 8 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/mips/brcm/brcm,bmips.txt
>  create mode 100644 Documentation/devicetree/bindings/mips/cpus.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mips/brcm/brcm,bmips.txt b/Documentation/devicetree/bindings/mips/brcm/brcm,bmips.txt
> deleted file mode 100644
> index 8ef71b4085ca..000000000000
> --- a/Documentation/devicetree/bindings/mips/brcm/brcm,bmips.txt
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -* Broadcom MIPS (BMIPS) CPUs
> -
> -Required properties:
> -- compatible: "brcm,bmips3300", "brcm,bmips4350", "brcm,bmips4380",
> -  "brcm,bmips5000"
> -
> -- mips-hpt-frequency: This is common to all CPUs in the system so it lives
> -  under the "cpus" node.
> diff --git a/Documentation/devicetree/bindings/mips/cpus.yaml b/Documentation/devicetree/bindings/mips/cpus.yaml
> new file mode 100644
> index 000000000000..361afde8ce0a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/cpus.yaml
> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mips/cpus.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPS CPUs bindings
> +
> +maintainers:
> +  - Sergio Paracuellos <sergio.paracuellos@gmail.com>

What about existing maintainers?



> +
> +description: |+

|+ seems not needed


> +  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.

s/cpu/CPU/

> +
> +properties:
> +  reg:
> +    maxItems: 1
> +
> +  compatible:

compatible goes first.

> +    enum:
> +      - brcm,bmips3300
> +      - brcm,bmips4350
> +      - brcm,bmips4380
> +      - brcm,bmips5000
> +      - brcm,bmips5200
> +      - ingenic,xburst-mxu1.0
> +      - ingenic,xburst-fpu1.0-mxu1.1
> +      - ingenic,xburst-fpu2.0-mxu2.0
> +      - loongson,gs264
> +      - mips,mips1004Kc
> +      - mips,m14Kc

Maybe keep alphabetical order?

> +      - mips,mips24KEc
> +      - mips,mips4KEc
> +      - mips,mips74Kc
> +      - mips,mips24Kc
> +      - mti,mips24KEc
> +      - mti,mips14KEc
> +      - mti,mips14Kc
> +      - mti,interaptiv
> +
> +if:

Out it in allOf block

> +  properties:
> +    compatible:
> +      enum:
> +        - brcm,bmips3300
> +        - brcm,bmips4350
> +        - brcm,bmips4380
> +        - brcm,bmips5000
> +        - brcm,bmips5200
> +then:
> +  patternProperties:
> +    mips-hpt-frequency:

It's not a pattern. Did you test the bindings?

> +      $ref: /schemas/types.yaml#/definitions/uint32

Missing description.

else mips-hpt-frequency: false

> +
> +required:
> +  - compatible
> +
> +additionalProperties: true

and this is why you did not notice errors...

> +
> +examples:
> +  - |
> +    cpus {
> +      #size-cells = <0>;
> +      #address-cells = <1>;
> +
> +      cpu@0 {
> +        device_type = "cpu";
> +        compatible = "mips,mips1004Kc";
> +        reg = <0>;
> +      };
> +
> +      cpu@1 {
> +        device_type = "cpu";
> +        compatible = "mips,mips1004Kc";
> +        reg = <1>;
> +      };
> +    };
> +
> +  - |
> +    // Example 2 (BMIPS CPU)
> +    cpus {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      mips-hpt-frequency = <150000000>;

Does not match your bindings. Are you sure you tested the patches?

> +
> +      cpu@0 {
> +        compatible = "brcm,bmips4350";
> +        device_type = "cpu";
> +        reg = <0>;
> +      };
> +
> +      cpu@1 {
> +        compatible = "brcm,bmips4350";
> +        device_type = "cpu";
> +        reg = <1>;
> +      };
> +    };


Best regards,
Krzysztof