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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c5ba2cc-b3c1-44df-99f2-a9146e8685f7@linaro.org>
Date: Thu, 11 Apr 2024 17:02:40 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
 Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
 Linus Walleij <linus.walleij@...aro.org>
Cc: linux-mips@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-gpio@...r.kernel.org,
 Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
 Gregory CLEMENT <gregory.clement@...tlin.com>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Tawfik Bayouk <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH 02/11] dt-bindings: clock: mobileye,eyeq5-clk: add EyeQ6L
 and EyeQ6H

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@...000 {
>                         clocks: clock-controller@...02c {
>                                 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@...000:
>   clock-controller@...02c: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