| Message ID | 20220819122259.183600-7-conor.dooley@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add PolarFire SoC Fabric Clock Conditioning Circuitry Support | expand |
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". https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Best regards, Krzysztof
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.
On 19/08/2022 16:15, Conor.Dooley@microchip.com wrote: >>> >>> + 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. 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
On 19/08/2022 14:28, Krzysztof Kozlowski wrote: >> 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? -EEXIST, registration fails in the core. > 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. So could I do `clock-output-names = "ccc_nw";`. That would work for me, with one question: How would I enforce the unique-ness of this property, since it would be a per CCC/clock-controller property? Maybe I missed something, but I gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check did not complain. Up to me to explain the restriction in the dt-bindings description? FWIW I would then have: ccc_sw: clock-controller@38400000 { compatible = "microchip,mpfs-ccc"; reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>, <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>; #clock-cells = <1>; clock-output-names = "ccc_sw"; status = "disabled"; }; & in the binding: clock-output-names: pattern: ^ccc_[ns][ew]$ As always, thanks for your help. I did look at output names earlier in the process, but didn't realise I could use it as a prefix. Conor.
On 19/08/2022 16:48, Conor.Dooley@microchip.com wrote: > On 19/08/2022 14:28, Krzysztof Kozlowski wrote: >>> 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? > > -EEXIST, registration fails in the core. > >> 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. > > So could I do `clock-output-names = "ccc_nw";`. That would work for me, > with one question: > How would I enforce the unique-ness of this property, since it would be > a per CCC/clock-controller property? Maybe I missed something, but I > gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check > did not complain. Up to me to explain the restriction in the dt-bindings > description? Uniqueness among entire DTS? I don't think you can, except of course mentioning it in description. Your driver should handle such DTS - minimally by gracefully failing but better behaving in some default way. > > FWIW I would then have: > ccc_sw: clock-controller@38400000 { > compatible = "microchip,mpfs-ccc"; > reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>, > <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>; > #clock-cells = <1>; > clock-output-names = "ccc_sw"; > status = "disabled"; > }; > > & in the binding: > clock-output-names: > pattern: ^ccc_[ns][ew]$ Yes, although this won't enforce uniqueness. Best regards, Krzysztof
On 19/08/2022 15:06, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 19/08/2022 16:48, Conor.Dooley@microchip.com wrote: >> On 19/08/2022 14:28, Krzysztof Kozlowski wrote: >>>> 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? >> >> -EEXIST, registration fails in the core. >> >>> 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. >> >> So could I do `clock-output-names = "ccc_nw";`. That would work for me, >> with one question: >> How would I enforce the unique-ness of this property, since it would be >> a per CCC/clock-controller property? Maybe I missed something, but I >> gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check >> did not complain. Up to me to explain the restriction in the dt-bindings >> description? > > Uniqueness among entire DTS? I don't think you can, except of course > mentioning it in description. Your driver should handle such DTS - > minimally by gracefully failing but better behaving in some default way. It fails not-too-gracefully at the moment, but that could easily be changed. Truncated base address I suppose would be a meaningful thing to fall back to afterwards. > >> >> FWIW I would then have: >> ccc_sw: clock-controller@38400000 { >> compatible = "microchip,mpfs-ccc"; >> reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>, >> <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>; >> #clock-cells = <1>; >> clock-output-names = "ccc_sw"; >> status = "disabled"; >> }; >> >> & in the binding: >> clock-output-names: >> pattern: ^ccc_[ns][ew]$ > > Yes, although this won't enforce uniqueness. I know :( I'll respin next week I guess, thanks again. Conor.
On 19/08/2022 17:14, Conor.Dooley@microchip.com wrote: > On 19/08/2022 15:06, Krzysztof Kozlowski wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 19/08/2022 16:48, Conor.Dooley@microchip.com wrote: >>> On 19/08/2022 14:28, Krzysztof Kozlowski wrote: >>>>> 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? >>> >>> -EEXIST, registration fails in the core. >>> >>>> 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. >>> >>> So could I do `clock-output-names = "ccc_nw";`. That would work for me, >>> with one question: >>> How would I enforce the unique-ness of this property, since it would be >>> a per CCC/clock-controller property? Maybe I missed something, but I >>> gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check >>> did not complain. Up to me to explain the restriction in the dt-bindings >>> description? >> >> Uniqueness among entire DTS? I don't think you can, except of course >> mentioning it in description. Your driver should handle such DTS - >> minimally by gracefully failing but better behaving in some default way. > > It fails not-too-gracefully at the moment, but that could easily be > changed. Truncated base address I suppose would be a meaningful thing > to fall back to afterwards. > >> >>> >>> FWIW I would then have: >>> ccc_sw: clock-controller@38400000 { >>> compatible = "microchip,mpfs-ccc"; >>> reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>, >>> <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>; >>> #clock-cells = <1>; >>> clock-output-names = "ccc_sw"; >>> status = "disabled"; >>> }; >>> >>> & in the binding: >>> clock-output-names: >>> pattern: ^ccc_[ns][ew]$ >> >> Yes, although this won't enforce uniqueness. > > I know :( I'll respin next week I guess, thanks again. The issue with this is that we are getting close to tying bindings with your Linux implementation. What if other implementation does not use these names as prefix and instead creates some other clock names (as clock names are not considered ABI)? Your binding would still enforce such property with a specific pattern. The clock names should not really matter, so if you have conflict of names among multiple controllers, I think driver should embed unit address in the name (as fallback of clock-output-name) and the binding should not enforce specific pattern. I can easily imagine a real hardware board design with "sexy_duck_ccc_pll1_out3" clock names. :) Best regards, Krzysztof
On 19/08/2022 15:22, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 19/08/2022 17:14, Conor.Dooley@microchip.com wrote: >> On 19/08/2022 15:06, Krzysztof Kozlowski wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On 19/08/2022 16:48, Conor.Dooley@microchip.com wrote: >>>> On 19/08/2022 14:28, Krzysztof Kozlowski wrote: >>>>>> 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? >>>> >>>> -EEXIST, registration fails in the core. >>>> >>>>> 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. >>>> >>>> So could I do `clock-output-names = "ccc_nw";`. That would work for me, >>>> with one question: >>>> How would I enforce the unique-ness of this property, since it would be >>>> a per CCC/clock-controller property? Maybe I missed something, but I >>>> gave it a shot with two different CCC nodes having "ccc_nw" & dtbs_check >>>> did not complain. Up to me to explain the restriction in the dt-bindings >>>> description? >>> >>> Uniqueness among entire DTS? I don't think you can, except of course >>> mentioning it in description. Your driver should handle such DTS - >>> minimally by gracefully failing but better behaving in some default way. >> >> It fails not-too-gracefully at the moment, but that could easily be >> changed. Truncated base address I suppose would be a meaningful thing >> to fall back to afterwards. >> >>> >>>> >>>> FWIW I would then have: >>>> ccc_sw: clock-controller@38400000 { >>>> compatible = "microchip,mpfs-ccc"; >>>> reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>, >>>> <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>; >>>> #clock-cells = <1>; >>>> clock-output-names = "ccc_sw"; >>>> status = "disabled"; >>>> }; >>>> >>>> & in the binding: >>>> clock-output-names: >>>> pattern: ^ccc_[ns][ew]$ >>> >>> Yes, although this won't enforce uniqueness. >> >> I know :( I'll respin next week I guess, thanks again. > > > The issue with this is that we are getting close to tying bindings with > your Linux implementation. What if other implementation does not use > these names as prefix and instead creates some other clock names (as > clock names are not considered ABI)? Your binding would still enforce > such property with a specific pattern. > > The clock names should not really matter, so if you have conflict of > names among multiple controllers, I think driver should embed unit > address in the name (as fallback of clock-output-name) and the binding > should not enforce specific pattern. Not sure if you just passed over it, but I agree: >> Truncated base address I suppose would be a meaningful thing >> to fall back to afterwards. But if the names aren't an ABI, then either there's not much point in having the regex at all for clock-output-names or failing the check for it does not matter. I'll have a think over the weekend about what exactly to do, but I think the driver side of this is clear to me now & what not to do in the binding is too. > I can easily imagine a real hardware board design with > "sexy_duck_ccc_pll1_out3" clock names. :) If Alestorm made a board with our FPGA, I could see that.. I'd buy the t-shirt too!
On 19/08/2022 17:32, Conor.Dooley@microchip.com wrote: >> The clock names should not really matter, so if you have conflict of >> names among multiple controllers, I think driver should embed unit >> address in the name (as fallback of clock-output-name) and the binding >> should not enforce specific pattern. > > Not sure if you just passed over it, but I agree: >>> Truncated base address I suppose would be a meaningful thing >>> to fall back to afterwards. Yeah, indeed, you mentioned it. > > But if the names aren't an ABI, then either there's not much point in > having the regex at all for clock-output-names or failing the check for > it does not matter. I'll have a think over the weekend about what > exactly to do, but I think the driver side of this is clear to me now & > what not to do in the binding is too. Yes. > >> I can easily imagine a real hardware board design with >> "sexy_duck_ccc_pll1_out3" clock names. :) > > If Alestorm made a board with our FPGA, I could see that.. > I'd buy the t-shirt too! > Best regards, Krzysztof
diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi index 0d28858b83f2..f17cb00df467 100644 --- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi +++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi @@ -2,14 +2,14 @@ /* Copyright (c) 2020-2021 Microchip Technology Inc */ / { - compatible = "microchip,mpfs-icicle-reference-rtlv2203", "microchip,mpfs"; + compatible = "microchip,mpfs-icicle-reference-rtlv2209", "microchip,mpfs"; core_pwm0: pwm@41000000 { compatible = "microchip,corepwm-rtl-v4"; reg = <0x0 0x41000000 0x0 0xF0>; microchip,sync-update-mask = /bits/ 32 <0>; #pwm-cells = <2>; - clocks = <&fabric_clk3>; + clocks = <&ccc_nw CLK_CCC_PLL0_OUT0>; status = "disabled"; }; @@ -18,22 +18,29 @@ i2c2: i2c@44000000 { reg = <0x0 0x44000000 0x0 0x1000>; #address-cells = <1>; #size-cells = <0>; - clocks = <&fabric_clk3>; + clocks = <&ccc_nw CLK_CCC_PLL0_OUT3>; interrupt-parent = <&plic>; interrupts = <122>; clock-frequency = <100000>; status = "disabled"; }; - fabric_clk3: fabric-clk3 { + refclk_ccc: cccrefclk { compatible = "fixed-clock"; #clock-cells = <0>; - clock-frequency = <62500000>; }; +}; - fabric_clk1: fabric-clk1 { - compatible = "fixed-clock"; - #clock-cells = <0>; - clock-frequency = <125000000>; - }; +&ccc_nw { + clocks = <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>, + <&refclk_ccc>, <&refclk_ccc>; + clock-names = "pll0_ref0", "pll0_ref1", "pll1_ref0", "pll1_ref1", + "dll0_ref", "dll1_ref"; + status = "okay"; +}; + +&pcie { + clocks = <&ccc_nw CLK_CCC_PLL0_OUT0>, <&ccc_nw CLK_CCC_PLL0_OUT1>, + <&ccc_nw CLK_CCC_PLL0_OUT3>; + clock-names = "fic0", "fic1", "fic3"; }; diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts index 044982a11df5..d361d1e38b16 100644 --- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts +++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts @@ -140,6 +140,10 @@ &refclk { clock-frequency = <125000000>; }; +&refclk_ccc { + clock-frequency = <50000000>; +}; + &rtc { status = "okay"; }; diff --git a/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi index 49380c428ec9..3beb450b4259 100644 --- a/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi +++ b/arch/riscv/boot/dts/microchip/mpfs-polarberry-fabric.dtsi @@ -14,3 +14,8 @@ fabric_clk1: fabric-clk1 { clock-frequency = <125000000>; }; }; + +&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 { + compatible = "microchip,mpfs-ccc"; + reg = <0x0 0x38010000 0x0 0x1000>, <0x0 0x38020000 0x0 0x1000>, + <0x0 0x39010000 0x0 0x1000>, <0x0 0x39020000 0x0 0x1000>; + #clock-cells = <1>; + status = "disabled"; + }; + + ccc_ne: cccneclk@38040000 { + compatible = "microchip,mpfs-ccc"; + reg = <0x0 0x38040000 0x0 0x1000>, <0x0 0x38080000 0x0 0x1000>, + <0x0 0x39040000 0x0 0x1000>, <0x0 0x39080000 0x0 0x1000>; + #clock-cells = <1>; + status = "disabled"; + }; + + ccc_nw: cccnwclk@38100000 { + compatible = "microchip,mpfs-ccc"; + reg = <0x0 0x38100000 0x0 0x1000>, <0x0 0x38200000 0x0 0x1000>, + <0x0 0x39100000 0x0 0x1000>, <0x0 0x39200000 0x0 0x1000>; + #clock-cells = <1>; + status = "disabled"; + }; + + ccc_sw: cccswclk@38400000 { + compatible = "microchip,mpfs-ccc"; + reg = <0x0 0x38400000 0x0 0x1000>, <0x0 0x38800000 0x0 0x1000>, + <0x0 0x39400000 0x0 0x1000>, <0x0 0x39800000 0x0 0x1000>; + #clock-cells = <1>; + status = "disabled"; + }; + mmuart0: serial@20000000 { compatible = "ns16550a"; reg = <0x0 0x20000000 0x0 0x400>; @@ -480,8 +512,6 @@ pcie: pcie@2000000000 { <0 0 0 3 &pcie_intc 2>, <0 0 0 4 &pcie_intc 3>; interrupt-map-mask = <0 0 0 7>; - clocks = <&fabric_clk1>, <&fabric_clk1>, <&fabric_clk3>; - clock-names = "fic0", "fic1", "fic3"; ranges = <0x3000000 0x0 0x8000000 0x20 0x8000000 0x0 0x80000000>; msi-parent = <&pcie>; msi-controller;
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. In the v2209 reference design a single CCC (north-west corner) is enabled, using a 50 MHz off-chip oscillator as its reference. Updating to the v2209 reference design is required. Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- .../dts/microchip/mpfs-icicle-kit-fabric.dtsi | 27 +++++++++------ .../boot/dts/microchip/mpfs-icicle-kit.dts | 4 +++ .../dts/microchip/mpfs-polarberry-fabric.dtsi | 5 +++ arch/riscv/boot/dts/microchip/mpfs.dtsi | 34 +++++++++++++++++-- 4 files changed, 58 insertions(+), 12 deletions(-)