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

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

On 21/09/2022 09:18, Sergio Paracuellos wrote:
> Hi Krzysztof,
> 
> On Wed, Sep 21, 2022 at 8:42 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 20/09/2022 07:51, Sergio Paracuellos wrote:
>>> Hi Krzysztof,
>>>
>>> On Mon, Sep 19, 2022 at 6:08 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 19/09/2022 15:41, Sergio Paracuellos wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On Mon, Sep 19, 2022 at 2:48 PM Krzysztof Kozlowski
>>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>>
>>>>>> On 19/09/2022 14:29, Sergio Paracuellos wrote:
>>>>>>>>
>>>>>>>> else mips-hpt-frequency: false
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +required:
>>>>>>>>> +  - compatible
>>>>>>>>> +
>>>>>>>>> +additionalProperties: true
>>>>>>>>
>>>>>>>> and this is why you did not notice errors...
>>>>>>>
>>>>>>> Current arch/mips/boot/dts folder dts files are a mess for cpu nodes,
>>>>>>> so I set additionalProperties to true and only make required for
>>>>>>> 'compatible'. What should be the correct approach?
>>>>>>
>>>>>> This is okay, but it caused 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?
>>>>>>>
>>>>>>> Yes I did:
>>>>>>>
>>>>>>> $ make dt_binding_check
>>>>>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/mips/cpus.yaml
>>>>>>>   LINT    Documentation/devicetree/bindings
>>>>>>>   CHKDT   Documentation/devicetree/bindings/processed-schema.json
>>>>>>>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>>>>>>>   DTEX    Documentation/devicetree/bindings/mips/cpus.example.dts
>>>>>>>   DTC     Documentation/devicetree/bindings/mips/cpus.example.dtb
>>>>>>> ' CHECK   Documentation/devicetree/bindings/mips/cpus.example.dtb
>>>>>>>
>>>>>>> Can you please point me to a sample of how to make required in a
>>>>>>> parent node of cpu@X property 'mips-hpt-frequency' only for some
>>>>>>> compatible strings inside the node? What can this be properly
>>>>>>> expressed using schema??
>>>>>>> I was looking and testing different things for a while without success at all.
>>>>>>
>>>>>> You either define new schema for /cpus node (and match by name, define
>>>>>> children etc) or include it in schema for top-level properties. The
>>>>>> first is tricky, because the cpus node does not have compatible (like
>>>>>> nvidia,tegra194-ccplex.yaml).
>>>
>>> Ok so if I am understanding correctly having two schemas is a way to go:
>>>
>>> One for brcm,bmips-cpus.yaml (since there is no compatible, should
>>> this be a valid name for this?) containing something like:
>>>
>>> properties:
>>>   $nodename:
>>>      const: cpus
>>>
>>>      mips-hpt-frequency:
>>>         $ref: /schemas/types.yaml#/definitions/uint32
>>>         description: |
>>>            This is common to all CPUs in the system so it lives
>>>             under the "cpus" node.
>>>
>>> additionalProperties: true
>>
>> Almost. Such schema will allow mips-hpt-frequency in each cpus node,
>> everywhere. On every board and architecture.
> 
> Yes, that is what I thought since no compatible to match this is
> included in current node.
> 
>>
>> You need to limit it per top-level compatibles.
> 
> Any sample of how to do this? So this bmips SoCs use compatible
> strings that are described in:
> https://elixir.bootlin.com/linux/v6.0-rc5/source/Documentation/devicetree/bindings/mips/brcm/soc.txt

Could be something like this:
https://lore.kernel.org/all/20220830065744.161163-2-krzysztof.kozlowski@linaro.org/
which is a part of top-level schema or add a new one. The new one will
duplicate the compatibles and parts from that one there, so maybe better
to keep it in top-level?

I am not sure, any suggestions are welcome. Also platform/architecture
maintainers might have their preference to organize it.

Anyway, you did not Cc the actual platform maintainers (Rafał and Hauke).

> 
> Can the top level compatible string be used in some way to filter this
> easily from this new 'brcm,bmips-cpus.yaml'

Yes. If schema matches the top level compatible, then in allOf:if:then
you can add restriction to disallow it for other variants:

For example:
https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212

https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml#L152


Best regards,
Krzysztof