lkml.org 
[lkml]   [2022]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 6/6] riscv: dts: microchip: add the mpfs' fabric clock control
Date
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.




\
 
 \ /
  Last update: 2022-08-19 15:18    [W:0.089 / U:1.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site