Re: [PATCH 02/11] dt-bindings: clock: mobileye,eyeq5-clk: add EyeQ6L and EyeQ6H

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

On 11/04/2024 15:49, Théo Lebrun wrote:
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 2
>>> +          maxItems: 2
>>> +        reg-names:
>>> +          minItems: 2
>>> +          maxItems: 2
>>
>> So any name is now valid? Like "yellow-pony"?
> 
> I do not understand what implies this. Below "items: enum: [...]"
> ensures only two allowed values. dtbs_check agrees:
> 
> ⟩ git diff
> diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
>            b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
> index 8d4f65ec912d..5031eb8b4270 100644
> --- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
> +++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
> @@ -126,7 +126,7 @@ reset: reset-controller@e00000 {
>                         clocks: clock-controller@e0002c {
>                                 compatible = "mobileye,eyeq5-clk";
>                                 reg = <0x02c 0x50>, <0x11c 0x04>;
> -                               reg-names = "plls", "ospi";
> +                               reg-names = "plls", "yellow-pony";
>                                 #clock-cells = <1>;
>                                 clocks = <&xtal>;
>                                 clock-names = "ref";
> 
> ⟩ make dtbs_check DT_SCHEMA_FILES=mobileye DT_CHECKER_FLAGS=-m
>   UPD     include/config/kernel.release
>   DTC_CHK arch/mips/boot/dts/mobileye/eyeq5-epm5.dtb
> arch/mips/boot/dts/mobileye/eyeq5-epm5.dtb: system-controller@e00000:
>   clock-controller@e0002c:reg-names:1:
>   'yellow-pony' is not one of ['plls', 'ospi']
>   from schema $id:
>     http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#

Ah, so you defined the items but made them an random order? No, please
keep same syntax which is what we always recommend anyway:

https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132

...

>>> +
>>> +  # Some compatibles provide a single clock; they do not take a clock cell.
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          enum:
>>> +            - mobileye,eyeq6h-central-clk
>>> +            - mobileye,eyeq6h-west-clk
>>> +            - mobileye,eyeq6h-east-clk
>>> +            - mobileye,eyeq6h-ddr0-clk
>>> +            - mobileye,eyeq6h-ddr1-clk
>>> +    then:
>>> +      properties:
>>> +        "#clock-cells":
>>> +          const: 0
>>
>> Wait, so you define device-per-clock? That's a terrible idea. We also
>> discussed it many times and it was rejected many times.
>>
>> You have one device, not 5.
> 
> Each region must be a syscon to make its various registers accessible to
> drivers that'll need it. Following that, I have a hard time seeing what
> would be the DT structure of 7 OLB system-controllers but a single
> clock node?

I assumed all these are in one syscon. Lack of DTS (example is quite
limited, which is expected) does not help. Please link full DTS so we
can see what you want to achieve.

Best regards,
Krzysztof