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

[Conor.Dooley]

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@20002000 {
>>                        #clock-cells = <1>;
>>                };
>>
>> +             ccc_se: cccseclk@38010000 {
> 
> 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.