linux-kernel - Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control

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]

Date:   Fri, 19 Aug 2022 16:28:38 +0300
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Conor.Dooley@...rochip.com, mturquette@...libre.com,
        sboyd@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, palmer@...belt.com,
        Daire.McNamara@...rochip.com
Cc:     paul.walmsley@...ive.com, aou@...s.berkeley.edu,
        linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock
 control

On 19/08/2022 16:15, Conor.Dooley@...rochip.com wrote:
>>>
>>> +             ccc_se: cccseclk@...10000 {
>>
>> Although you call it "Clock Conditioning Circuitry", but the role of
>> this device is a clock-controller, isn't it? If so, node names should be
>> generic, so "clock-controller".
> 
> Thanks for the prompt reply Krzysztof!
> I suspected that this is what I was going to hear back. The reason I
> had used the non-generic node name is that I wanted to use it for the
> "name" of the clocks in the clock framework. As you can see, there are
> four instances of the same clock, and I am using the of_node's name to
> generate the unique names the clock framework requires, like so:
> 
> # cat clk_summary
>     clock
> -------------------------
>   cccrefclk
>      cccnwclk_pll1
>         cccnwclk_pll1_out3
>         cccnwclk_pll1_out2
>         cccnwclk_pll1_out1
>         cccnwclk_pll1_out0
>      cccnwclk_pll0
>         cccnwclk_pll0_out3
>         cccnwclk_pll0_out2
>         cccnwclk_pll0_out1
>         cccnwclk_pll0_out0
>      cccswclk_pll1
>         cccswclk_pll1_out3
>         cccswclk_pll1_out2
>         cccswclk_pll1_out1
>         cccswclk_pll1_out0
>      cccnsclk_pll0
>         cccswclk_pll0_out3
>         cccswclk_pll0_out2
>         cccswclk_pll0_out1
>         cccswclk_pll0_out0
> 
> Maybe that is me exploiting the "should", but I was not sure how to
> include the location in the devicetree.

Neither node names nor clock names are considered an ABI, but some
pieces like to rely on them. Now you created such dependency so imagine
someone prepares a DTSI/DTS with "clock-controller" names for all four
blocks. How you driver would behave?

The DTS would be perfectly valid but driver would not accept it
(conflicting names) or behave incorrect.

I think what you need is the clock-output-names property. The core
schema dtschema/schemas/clock/clock.yaml recommends unified
interpretation of it - list of names for all the clocks - but accepts
other uses, e.g. as a prefix.

> 
> I had experimented with a "microchip,ordinal" or "microchip,location"
> string property to do the same thing but I thought you/Rob might not
> like that - is location/placement on the chip a relevant property of the
> hardware? I'd argue that for an FPGA, where the user is the one deciding
> what clocks what, it could be relevant to some degree.
> 
> Knowing if a CCC is the north-west one has some extra benefits as it
> is co-located with the PLLs for the processor & has a reduced input
> mux range.
> 
> Any suggestions would be appreciated, even if it is just a NAK to all of
> the above!


Best regards,
Krzysztof