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 13:15:17 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <krzysztof.kozlowski@...aro.org>, <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 13:47, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 19/08/2022 15:23, Conor Dooley wrote:
>> The "fabric clocks" in current PolarFire SoC device trees are not
>> really fixed clocks. Their frequency is set by the bitstream, so having
>> them located in -fabric.dtsi is not a problem - they're just as "fixed"
>> as the IP blocks etc used in the FPGA fabric.
>> However, their configuration can be read at runtime (and to an extent
>> they can be controlled, although the intended usage is static
>> configurations set by the bitstream) through the system controller bus.
>>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
>> +&pcie {
>> +     clocks = <&fabric_clk1>, <&fabric_clk1>, <&fabric_clk3>;
>> +     clock-names = "fic0", "fic1", "fic3";
>> +};
>> diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
>> index 499c2e63ad35..dd15b6d1a3c9 100644
>> --- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
>> +++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
>> @@ -236,6 +236,38 @@ clkcfg: clkcfg@...02000 {
>>                        #clock-cells = <1>;
>>                };
>>
>> +             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.

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!

Thanks,
Conor.