| Message ID | 1426657522-2473-2-git-send-email-rjui@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Ray Jui (2015-03-17 22:45:17) > Document the device tree binding for Broadcom iProc architecture based > clock controller > > Signed-off-by: Ray Jui <rjui@broadcom.com> > Reviewed-by: Scott Branden <sbranden@broadcom.com> > --- > .../bindings/clock/brcm,iproc-clocks.txt | 171 ++++++++++++++++++++ > 1 file changed, 171 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > > diff --git a/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > new file mode 100644 > index 0000000..bf2316b > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > @@ -0,0 +1,171 @@ > +Broadcom iProc Family Clocks > + > +This binding uses the common clock binding: > + Documentation/devicetree/bindings/clock/clock-bindings.txt > + > +The iProc clock controller manages clocks that are common to the iProc family. > +An SoC from the iProc family may have several PPLs, e.g., ARMPLL, GENPLL, > +LCPLL0, MIPIPLL, and etc., all derived from an onboard crystal. Each PLL > +comprises of several leaf clocks > + > +Required properties for PLLs: > +- compatible: > + Should have a value of the form "brcm,<soc>-<pll>". For example, GENPLL on > +Cygnus has a compatible string of "brcm,cygnus-genpll" > + > +- #clock-cells: > + Must be <0> > + > +- reg: > + Define the base and range of the I/O address space that contain the iProc > +clock control registers required for the PLL > + > +- clocks: > + The input parent clock phandle for the PLL. For all iProc PLLs, this is an > +onboard crystal with a fixed rate > + > +Example: > + > + osc: oscillator { > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <25000000>; > + }; > + > + genpll: genpll { > + #clock-cells = <0>; > + compatible = "brcm,cygnus-genpll"; > + reg = <0x0301d000 0x2c>, > + <0x0301c020 0x4>; > + clocks = <&osc>; > + }; > + > +Required properties for leaf clocks of a PLL: > + > +- compatible: > + Should have a value of the form "brcm,<soc>-<pll>-clk". For example, leaf > +clocks derived from the GENPLL on Cygnus SoC have a compatible string of > +"brcm,cygnus-genpll-clk" > + > +- #clock-cells: > + Have a value of <1> since there are more than 1 leaf clock of a > +given PLL > + > +- reg: > + Define the base and range of the I/O address space that contain the iProc > +clock control registers required for the PLL leaf clocks > + > +- clocks: > + The input parent PLL phandle for the leaf clock > + > +- clock-output-names: > + An ordered list of strings defining the names of the leaf clocks > + > +Example: > + > + genpll: genpll { > + #clock-cells = <0>; > + compatible = "brcm,cygnus-genpll"; > + reg = <0x0301d000 0x2c>, > + <0x0301c020 0x4>; > + clocks = <&osc>; > + }; > + > + genpll_clks: genpll_clks { > + #clock-cells = <1>; > + compatible = "brcm,cygnus-genpll-clk"; > + reg = <0x0301d000 0x2c>; > + clocks = <&genpll>; > + clock-output-names = "axi21", "250mhz", "ihost_sys", > + "enet_sw", "audio_125", "can"; > + }; Hi Ray, Thanks for submitting the patch. It was nice meeting you at ELC as well. This binding doesn't conform to the norms for clock bindings. It looks like for each type of controllable clock node (e.g. pll, leaf clock, etc) you have a dts node. Looking at the above example it seems that those two nodes (genpll and genpll_clks) share the same register. /me checks patch #5 Yup, you re-use the same register address for the *pll and *pll_clks nodes. I'm not a DT expert but I think this is considered Wrong. More generally your clock dt binding should be modeling the hardware in terms of IP blocks. If you have a clock generator IP block it may control many clock bits and output many clock signals. E.g. for your hardware (based only on the addresses in patch #5 and not having seen any data manual) I will hazard a guess that the genpll, lcpll and asiu clocks are all part of the same IP block. If my guess is correct then these clocks should all be represented by a single node int DT. Lets say that the clock controller IP block that manages the genpll, lcpll and asiu clocks (including their child/leaf clocks) is called "foobar" in your data manual. You should have a dts node with a compatible string such as "brcm,cygnus-foobar" or "brcm,cygnus-foobar-clk". Your clock driver should be responsible for registering all of the clocks controlled by this IP block, regardless of the "type" of clock node. I think part of the reason for your approach is that the previous (deprecated) binding/dts had a bunch of fixed-rate clocks in it. This is a hack that we do every now and then to get a new platform up and running with a mainline kernel, but we don't do per-clock nodes in dts any more, we do them by clock controller ip block instead. There are plenty of good examples of this for Exynos and QCOM SoCs if you want an example. Regards, Mike
On 4/10/2015 5:12 PM, Michael Turquette wrote: > Quoting Ray Jui (2015-03-17 22:45:17) >> Document the device tree binding for Broadcom iProc architecture based >> clock controller >> >> Signed-off-by: Ray Jui <rjui@broadcom.com> >> Reviewed-by: Scott Branden <sbranden@broadcom.com> >> --- >> .../bindings/clock/brcm,iproc-clocks.txt | 171 ++++++++++++++++++++ >> 1 file changed, 171 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >> >> diff --git a/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >> new file mode 100644 >> index 0000000..bf2316b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >> @@ -0,0 +1,171 @@ >> +Broadcom iProc Family Clocks >> + >> +This binding uses the common clock binding: >> + Documentation/devicetree/bindings/clock/clock-bindings.txt >> + >> +The iProc clock controller manages clocks that are common to the iProc family. >> +An SoC from the iProc family may have several PPLs, e.g., ARMPLL, GENPLL, >> +LCPLL0, MIPIPLL, and etc., all derived from an onboard crystal. Each PLL >> +comprises of several leaf clocks >> + >> +Required properties for PLLs: >> +- compatible: >> + Should have a value of the form "brcm,<soc>-<pll>". For example, GENPLL on >> +Cygnus has a compatible string of "brcm,cygnus-genpll" >> + >> +- #clock-cells: >> + Must be <0> >> + >> +- reg: >> + Define the base and range of the I/O address space that contain the iProc >> +clock control registers required for the PLL >> + >> +- clocks: >> + The input parent clock phandle for the PLL. For all iProc PLLs, this is an >> +onboard crystal with a fixed rate >> + >> +Example: >> + >> + osc: oscillator { >> + #clock-cells = <0>; >> + compatible = "fixed-clock"; >> + clock-frequency = <25000000>; >> + }; >> + >> + genpll: genpll { >> + #clock-cells = <0>; >> + compatible = "brcm,cygnus-genpll"; >> + reg = <0x0301d000 0x2c>, >> + <0x0301c020 0x4>; >> + clocks = <&osc>; >> + }; >> + >> +Required properties for leaf clocks of a PLL: >> + >> +- compatible: >> + Should have a value of the form "brcm,<soc>-<pll>-clk". For example, leaf >> +clocks derived from the GENPLL on Cygnus SoC have a compatible string of >> +"brcm,cygnus-genpll-clk" >> + >> +- #clock-cells: >> + Have a value of <1> since there are more than 1 leaf clock of a >> +given PLL >> + >> +- reg: >> + Define the base and range of the I/O address space that contain the iProc >> +clock control registers required for the PLL leaf clocks >> + >> +- clocks: >> + The input parent PLL phandle for the leaf clock >> + >> +- clock-output-names: >> + An ordered list of strings defining the names of the leaf clocks >> + >> +Example: >> + >> + genpll: genpll { >> + #clock-cells = <0>; >> + compatible = "brcm,cygnus-genpll"; >> + reg = <0x0301d000 0x2c>, >> + <0x0301c020 0x4>; >> + clocks = <&osc>; >> + }; >> + >> + genpll_clks: genpll_clks { >> + #clock-cells = <1>; >> + compatible = "brcm,cygnus-genpll-clk"; >> + reg = <0x0301d000 0x2c>; >> + clocks = <&genpll>; >> + clock-output-names = "axi21", "250mhz", "ihost_sys", >> + "enet_sw", "audio_125", "can"; >> + }; > > Hi Ray, > > Thanks for submitting the patch. It was nice meeting you at ELC as well. > > This binding doesn't conform to the norms for clock bindings. It looks > like for each type of controllable clock node (e.g. pll, leaf clock, > etc) you have a dts node. Looking at the above example it seems that > those two nodes (genpll and genpll_clks) share the same register. > > /me checks patch #5 > > Yup, you re-use the same register address for the *pll and *pll_clks > nodes. I'm not a DT expert but I think this is considered Wrong. > > More generally your clock dt binding should be modeling the hardware in > terms of IP blocks. If you have a clock generator IP block it may > control many clock bits and output many clock signals. E.g. for your > hardware (based only on the addresses in patch #5 and not having seen > any data manual) I will hazard a guess that the genpll, lcpll and asiu > clocks are all part of the same IP block. Hi Mike, In fact, lcpll, genpll, mipipll are similar but DIFFERENT IP blocks, and asiu is completely different from any of them. All of these plls are unique and have their own register banks, as you can see from the bcm-cygnus-clock.dtsi file. Therefore, I think it's totally correct and actually necessary to represent each of them with a separate device node. Regarding the relationship between a PLL clock and its leaf clocks: Taking the genpll as an example, physically they are connected as: xtal -> genpll -> axi21, 250mhz, ihost_sys, ... The 25 MHz crystal feeds to the genpll, and the genpll generates 6 different leaf clocks including axi21, 250mhz, ihost_sys, and etc. One can choose to set the genpll's vco to one frequency, and based on that frequency, different leaf clock frequencies can be generated with their own post divider. Therefore, I think it makes sense to represent xtal, genpll, genpll_clks (including axi21, 250mhz, ihost_sys, and etc) each with a unique device node, and genpll is the parent of these leaf clocks. Basically the device nodes and the way how the "clocks" phandle is used represent the hierarchy of the clock architecture within Cygnus (and in the future other iProc SoCs). Does that make sense? Regarding the register address overlapping, again, taking genpll as an example, the genpll and the genpll_clks actually never access the same set of registers, but the registers are sort of scattered within one bank, i.e., on Cygnus, genpll uses registers at offset 0x0, 0x8 ~ 0x1c, and genpll_clks uses registers at offset 0x4, 0x20 - 0x24. Thanks, Ray > > If my guess is correct then these clocks should all be represented by a > single node int DT. Lets say that the clock controller IP block that > manages the genpll, lcpll and asiu clocks (including their child/leaf > clocks) is called "foobar" in your data manual. You should have a dts > node with a compatible string such as "brcm,cygnus-foobar" or > "brcm,cygnus-foobar-clk". > > Your clock driver should be responsible for registering all of the > clocks controlled by this IP block, regardless of the "type" of clock > node. > > I think part of the reason for your approach is that the previous > (deprecated) binding/dts had a bunch of fixed-rate clocks in it. This is > a hack that we do every now and then to get a new platform up and > running with a mainline kernel, but we don't do per-clock nodes in dts > any more, we do them by clock controller ip block instead. > > There are plenty of good examples of this for Exynos and QCOM SoCs if > you want an example. > > Regards, > Mike >
Quoting Ray Jui (2015-04-12 21:08:32) > > > On 4/10/2015 5:12 PM, Michael Turquette wrote: > > Quoting Ray Jui (2015-03-17 22:45:17) > >> Document the device tree binding for Broadcom iProc architecture based > >> clock controller > >> > >> Signed-off-by: Ray Jui <rjui@broadcom.com> > >> Reviewed-by: Scott Branden <sbranden@broadcom.com> > >> --- > >> .../bindings/clock/brcm,iproc-clocks.txt | 171 ++++++++++++++++++++ > >> 1 file changed, 171 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > >> > >> diff --git a/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > >> new file mode 100644 > >> index 0000000..bf2316b > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > >> @@ -0,0 +1,171 @@ > >> +Broadcom iProc Family Clocks > >> + > >> +This binding uses the common clock binding: > >> + Documentation/devicetree/bindings/clock/clock-bindings.txt > >> + > >> +The iProc clock controller manages clocks that are common to the iProc family. > >> +An SoC from the iProc family may have several PPLs, e.g., ARMPLL, GENPLL, > >> +LCPLL0, MIPIPLL, and etc., all derived from an onboard crystal. Each PLL > >> +comprises of several leaf clocks > >> + > >> +Required properties for PLLs: > >> +- compatible: > >> + Should have a value of the form "brcm,<soc>-<pll>". For example, GENPLL on > >> +Cygnus has a compatible string of "brcm,cygnus-genpll" > >> + > >> +- #clock-cells: > >> + Must be <0> > >> + > >> +- reg: > >> + Define the base and range of the I/O address space that contain the iProc > >> +clock control registers required for the PLL > >> + > >> +- clocks: > >> + The input parent clock phandle for the PLL. For all iProc PLLs, this is an > >> +onboard crystal with a fixed rate > >> + > >> +Example: > >> + > >> + osc: oscillator { > >> + #clock-cells = <0>; > >> + compatible = "fixed-clock"; > >> + clock-frequency = <25000000>; > >> + }; > >> + > >> + genpll: genpll { > >> + #clock-cells = <0>; > >> + compatible = "brcm,cygnus-genpll"; > >> + reg = <0x0301d000 0x2c>, > >> + <0x0301c020 0x4>; > >> + clocks = <&osc>; > >> + }; > >> + > >> +Required properties for leaf clocks of a PLL: > >> + > >> +- compatible: > >> + Should have a value of the form "brcm,<soc>-<pll>-clk". For example, leaf > >> +clocks derived from the GENPLL on Cygnus SoC have a compatible string of > >> +"brcm,cygnus-genpll-clk" > >> + > >> +- #clock-cells: > >> + Have a value of <1> since there are more than 1 leaf clock of a > >> +given PLL > >> + > >> +- reg: > >> + Define the base and range of the I/O address space that contain the iProc > >> +clock control registers required for the PLL leaf clocks > >> + > >> +- clocks: > >> + The input parent PLL phandle for the leaf clock > >> + > >> +- clock-output-names: > >> + An ordered list of strings defining the names of the leaf clocks > >> + > >> +Example: > >> + > >> + genpll: genpll { > >> + #clock-cells = <0>; > >> + compatible = "brcm,cygnus-genpll"; > >> + reg = <0x0301d000 0x2c>, > >> + <0x0301c020 0x4>; > >> + clocks = <&osc>; > >> + }; > >> + > >> + genpll_clks: genpll_clks { > >> + #clock-cells = <1>; > >> + compatible = "brcm,cygnus-genpll-clk"; > >> + reg = <0x0301d000 0x2c>; > >> + clocks = <&genpll>; > >> + clock-output-names = "axi21", "250mhz", "ihost_sys", > >> + "enet_sw", "audio_125", "can"; > >> + }; > > > > Hi Ray, > > > > Thanks for submitting the patch. It was nice meeting you at ELC as well. > > > > This binding doesn't conform to the norms for clock bindings. It looks > > like for each type of controllable clock node (e.g. pll, leaf clock, > > etc) you have a dts node. Looking at the above example it seems that > > those two nodes (genpll and genpll_clks) share the same register. > > > > /me checks patch #5 > > > > Yup, you re-use the same register address for the *pll and *pll_clks > > nodes. I'm not a DT expert but I think this is considered Wrong. > > > > More generally your clock dt binding should be modeling the hardware in > > terms of IP blocks. If you have a clock generator IP block it may > > control many clock bits and output many clock signals. E.g. for your > > hardware (based only on the addresses in patch #5 and not having seen > > any data manual) I will hazard a guess that the genpll, lcpll and asiu > > clocks are all part of the same IP block. > > Hi Mike, > > In fact, lcpll, genpll, mipipll are similar but DIFFERENT IP blocks, and > asiu is completely different from any of them. All of these plls are > unique and have their own register banks, as you can see from the > bcm-cygnus-clock.dtsi file. Therefore, I think it's totally correct and > actually necessary to represent each of them with a separate device node. OK, that makes sense to me, if those registers live in addresses ranges which correspond to different IP blocks. > > Regarding the relationship between a PLL clock and its leaf clocks: > Taking the genpll as an example, physically they are connected as: > > xtal -> genpll -> axi21, 250mhz, ihost_sys, ... > > The 25 MHz crystal feeds to the genpll, and the genpll generates 6 > different leaf clocks including axi21, 250mhz, ihost_sys, and etc. One > can choose to set the genpll's vco to one frequency, and based on that > frequency, different leaf clock frequencies can be generated with their > own post divider. > > Therefore, I think it makes sense to represent xtal, genpll, genpll_clks > (including axi21, 250mhz, ihost_sys, and etc) each with a unique device > node, and genpll is the parent of these leaf clocks. Basically the > device nodes and the way how the "clocks" phandle is used represent the > hierarchy of the clock architecture within Cygnus (and in the future > other iProc SoCs). Does that make sense? This doesn't make sense to me. If I understand correctly, the register range that controls the post-divider clock (e.g. axi21) is the same register range that control's genpll. This is a reasonable indicator to me that the leaf clocks are part of the same clock generator IP block as the PLL controls. As such they should be on node. > > Regarding the register address overlapping, again, taking genpll as an > example, the genpll and the genpll_clks actually never access the same > set of registers, but the registers are sort of scattered within one > bank, i.e., on Cygnus, genpll uses registers at offset 0x0, 0x8 ~ 0x1c, > and genpll_clks uses registers at offset 0x4, 0x20 - 0x24. Sure, my argument above doesn't hinge on the fact that the pll and child clocks use the exact same register. It still looks to me like *pll and it's child dividers belong in the same IP block. Is there an open data sheet or technical reference manual I can look at for this part? That is the best way to put my concerns at ease ;-) Looking over your binding again, it looks like your nodes are divided conveniently for the different clock types (e.g. pll versus post-divider), but our goal with DT is to accurately describe the hardware, not the C structures. Regards, Mike > > Thanks, > > Ray > > > > > If my guess is correct then these clocks should all be represented by a > > single node int DT. Lets say that the clock controller IP block that > > manages the genpll, lcpll and asiu clocks (including their child/leaf > > clocks) is called "foobar" in your data manual. You should have a dts > > node with a compatible string such as "brcm,cygnus-foobar" or > > "brcm,cygnus-foobar-clk". > > > > Your clock driver should be responsible for registering all of the > > clocks controlled by this IP block, regardless of the "type" of clock > > node. > > > > I think part of the reason for your approach is that the previous > > (deprecated) binding/dts had a bunch of fixed-rate clocks in it. This is > > a hack that we do every now and then to get a new platform up and > > running with a mainline kernel, but we don't do per-clock nodes in dts > > any more, we do them by clock controller ip block instead. > > > > There are plenty of good examples of this for Exynos and QCOM SoCs if > > you want an example. > > > > Regards, > > Mike > >
Hi Mike, On 4/12/2015 11:02 PM, Michael Turquette wrote: > Quoting Ray Jui (2015-04-12 21:08:32) >> >> >> On 4/10/2015 5:12 PM, Michael Turquette wrote: >>> Quoting Ray Jui (2015-03-17 22:45:17) >>>> Document the device tree binding for Broadcom iProc architecture based >>>> clock controller >>>> >>>> Signed-off-by: Ray Jui <rjui@broadcom.com> >>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>>> --- >>>> .../bindings/clock/brcm,iproc-clocks.txt | 171 ++++++++++++++++++++ >>>> 1 file changed, 171 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>> >>>> diff --git a/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>> new file mode 100644 >>>> index 0000000..bf2316b >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>> @@ -0,0 +1,171 @@ >>>> +Broadcom iProc Family Clocks >>>> + >>>> +This binding uses the common clock binding: >>>> + Documentation/devicetree/bindings/clock/clock-bindings.txt >>>> + >>>> +The iProc clock controller manages clocks that are common to the iProc family. >>>> +An SoC from the iProc family may have several PPLs, e.g., ARMPLL, GENPLL, >>>> +LCPLL0, MIPIPLL, and etc., all derived from an onboard crystal. Each PLL >>>> +comprises of several leaf clocks >>>> + >>>> +Required properties for PLLs: >>>> +- compatible: >>>> + Should have a value of the form "brcm,<soc>-<pll>". For example, GENPLL on >>>> +Cygnus has a compatible string of "brcm,cygnus-genpll" >>>> + >>>> +- #clock-cells: >>>> + Must be <0> >>>> + >>>> +- reg: >>>> + Define the base and range of the I/O address space that contain the iProc >>>> +clock control registers required for the PLL >>>> + >>>> +- clocks: >>>> + The input parent clock phandle for the PLL. For all iProc PLLs, this is an >>>> +onboard crystal with a fixed rate >>>> + >>>> +Example: >>>> + >>>> + osc: oscillator { >>>> + #clock-cells = <0>; >>>> + compatible = "fixed-clock"; >>>> + clock-frequency = <25000000>; >>>> + }; >>>> + >>>> + genpll: genpll { >>>> + #clock-cells = <0>; >>>> + compatible = "brcm,cygnus-genpll"; >>>> + reg = <0x0301d000 0x2c>, >>>> + <0x0301c020 0x4>; >>>> + clocks = <&osc>; >>>> + }; >>>> + >>>> +Required properties for leaf clocks of a PLL: >>>> + >>>> +- compatible: >>>> + Should have a value of the form "brcm,<soc>-<pll>-clk". For example, leaf >>>> +clocks derived from the GENPLL on Cygnus SoC have a compatible string of >>>> +"brcm,cygnus-genpll-clk" >>>> + >>>> +- #clock-cells: >>>> + Have a value of <1> since there are more than 1 leaf clock of a >>>> +given PLL >>>> + >>>> +- reg: >>>> + Define the base and range of the I/O address space that contain the iProc >>>> +clock control registers required for the PLL leaf clocks >>>> + >>>> +- clocks: >>>> + The input parent PLL phandle for the leaf clock >>>> + >>>> +- clock-output-names: >>>> + An ordered list of strings defining the names of the leaf clocks >>>> + >>>> +Example: >>>> + >>>> + genpll: genpll { >>>> + #clock-cells = <0>; >>>> + compatible = "brcm,cygnus-genpll"; >>>> + reg = <0x0301d000 0x2c>, >>>> + <0x0301c020 0x4>; >>>> + clocks = <&osc>; >>>> + }; >>>> + >>>> + genpll_clks: genpll_clks { >>>> + #clock-cells = <1>; >>>> + compatible = "brcm,cygnus-genpll-clk"; >>>> + reg = <0x0301d000 0x2c>; >>>> + clocks = <&genpll>; >>>> + clock-output-names = "axi21", "250mhz", "ihost_sys", >>>> + "enet_sw", "audio_125", "can"; >>>> + }; >>> >>> Hi Ray, >>> >>> Thanks for submitting the patch. It was nice meeting you at ELC as well. >>> >>> This binding doesn't conform to the norms for clock bindings. It looks >>> like for each type of controllable clock node (e.g. pll, leaf clock, >>> etc) you have a dts node. Looking at the above example it seems that >>> those two nodes (genpll and genpll_clks) share the same register. >>> >>> /me checks patch #5 >>> >>> Yup, you re-use the same register address for the *pll and *pll_clks >>> nodes. I'm not a DT expert but I think this is considered Wrong. >>> >>> More generally your clock dt binding should be modeling the hardware in >>> terms of IP blocks. If you have a clock generator IP block it may >>> control many clock bits and output many clock signals. E.g. for your >>> hardware (based only on the addresses in patch #5 and not having seen >>> any data manual) I will hazard a guess that the genpll, lcpll and asiu >>> clocks are all part of the same IP block. >> >> Hi Mike, >> >> In fact, lcpll, genpll, mipipll are similar but DIFFERENT IP blocks, and >> asiu is completely different from any of them. All of these plls are >> unique and have their own register banks, as you can see from the >> bcm-cygnus-clock.dtsi file. Therefore, I think it's totally correct and >> actually necessary to represent each of them with a separate device node. > > OK, that makes sense to me, if those registers live in addresses ranges > which correspond to different IP blocks. > >> >> Regarding the relationship between a PLL clock and its leaf clocks: >> Taking the genpll as an example, physically they are connected as: >> >> xtal -> genpll -> axi21, 250mhz, ihost_sys, ... >> >> The 25 MHz crystal feeds to the genpll, and the genpll generates 6 >> different leaf clocks including axi21, 250mhz, ihost_sys, and etc. One >> can choose to set the genpll's vco to one frequency, and based on that >> frequency, different leaf clock frequencies can be generated with their >> own post divider. >> >> Therefore, I think it makes sense to represent xtal, genpll, genpll_clks >> (including axi21, 250mhz, ihost_sys, and etc) each with a unique device >> node, and genpll is the parent of these leaf clocks. Basically the >> device nodes and the way how the "clocks" phandle is used represent the >> hierarchy of the clock architecture within Cygnus (and in the future >> other iProc SoCs). Does that make sense? > > This doesn't make sense to me. If I understand correctly, the register > range that controls the post-divider clock (e.g. axi21) is the same > register range that control's genpll. This is a reasonable indicator to > me that the leaf clocks are part of the same clock generator IP block as > the PLL controls. As such they should be on node. > >> >> Regarding the register address overlapping, again, taking genpll as an >> example, the genpll and the genpll_clks actually never access the same >> set of registers, but the registers are sort of scattered within one >> bank, i.e., on Cygnus, genpll uses registers at offset 0x0, 0x8 ~ 0x1c, >> and genpll_clks uses registers at offset 0x4, 0x20 - 0x24. > > Sure, my argument above doesn't hinge on the fact that the pll and child > clocks use the exact same register. It still looks to me like *pll and > it's child dividers belong in the same IP block. Is there an open data > sheet or technical reference manual I can look at for this part? That is > the best way to put my concerns at ease ;-) > > Looking over your binding again, it looks like your nodes are divided > conveniently for the different clock types (e.g. pll versus > post-divider), but our goal with DT is to accurately describe the > hardware, not the C structures. > > Regards, > Mike > Yes, the genpll and genpll_clks are controlled by the same IP block. I can make the change to combine them to use one DT node. But before doing that, I just need to get one thing clarified with you. In SW, the pll and its leaf clocks will still be registered as separate two clock providers, since we need to be able to configure the PLL vco frequency and its leaf clock frequencies separately. The pll will be the parent of its leaf clocks, and the leaf clocks will be used by various peripherals. I plan to have the combined DT looks like this: genpll_clks: genpll_clks { #clock-cells = <1>; compatible = "brcm,cygnus-genpll-clk"; reg = <0x0301d000 0x2c>; assigned-clocks = <&genpll_clks>; /* this sets the PLL rate at the time when the PLL clock is registered */ assigned-clock-rates = <4000000000>; clocks = <&osc>; clock-output-names = "axi21", "250mhz", "ihost_sys", "enet_sw", "audio_125", "can"; }; peripheralA: peripheralA { /* use the first leaf clock of genpll */ clocks = <&genpll_clks 0>; clock-frequency = <100000000>; }; If we register both the genpll and its leaf clocks to the clock framework as two separate clock providers, would the above DT entries still work? For example, peripheralA refers to the phandle of genpll_clks, but how does the clock framework know to link it to the pll clock or the leaf clock? Thanks, Ray
Hi Mike, On 4/13/2015 12:40 PM, Ray Jui wrote: > Hi Mike, > > On 4/12/2015 11:02 PM, Michael Turquette wrote: >> Quoting Ray Jui (2015-04-12 21:08:32) >>> >>> >>> On 4/10/2015 5:12 PM, Michael Turquette wrote: >>>> Quoting Ray Jui (2015-03-17 22:45:17) >>>>> Document the device tree binding for Broadcom iProc architecture based >>>>> clock controller >>>>> >>>>> Signed-off-by: Ray Jui <rjui@broadcom.com> >>>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>>>> --- >>>>> .../bindings/clock/brcm,iproc-clocks.txt | 171 ++++++++++++++++++++ >>>>> 1 file changed, 171 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>>> new file mode 100644 >>>>> index 0000000..bf2316b >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>>> @@ -0,0 +1,171 @@ >>>>> +Broadcom iProc Family Clocks >>>>> + >>>>> +This binding uses the common clock binding: >>>>> + Documentation/devicetree/bindings/clock/clock-bindings.txt >>>>> + >>>>> +The iProc clock controller manages clocks that are common to the iProc family. >>>>> +An SoC from the iProc family may have several PPLs, e.g., ARMPLL, GENPLL, >>>>> +LCPLL0, MIPIPLL, and etc., all derived from an onboard crystal. Each PLL >>>>> +comprises of several leaf clocks >>>>> + >>>>> +Required properties for PLLs: >>>>> +- compatible: >>>>> + Should have a value of the form "brcm,<soc>-<pll>". For example, GENPLL on >>>>> +Cygnus has a compatible string of "brcm,cygnus-genpll" >>>>> + >>>>> +- #clock-cells: >>>>> + Must be <0> >>>>> + >>>>> +- reg: >>>>> + Define the base and range of the I/O address space that contain the iProc >>>>> +clock control registers required for the PLL >>>>> + >>>>> +- clocks: >>>>> + The input parent clock phandle for the PLL. For all iProc PLLs, this is an >>>>> +onboard crystal with a fixed rate >>>>> + >>>>> +Example: >>>>> + >>>>> + osc: oscillator { >>>>> + #clock-cells = <0>; >>>>> + compatible = "fixed-clock"; >>>>> + clock-frequency = <25000000>; >>>>> + }; >>>>> + >>>>> + genpll: genpll { >>>>> + #clock-cells = <0>; >>>>> + compatible = "brcm,cygnus-genpll"; >>>>> + reg = <0x0301d000 0x2c>, >>>>> + <0x0301c020 0x4>; >>>>> + clocks = <&osc>; >>>>> + }; >>>>> + >>>>> +Required properties for leaf clocks of a PLL: >>>>> + >>>>> +- compatible: >>>>> + Should have a value of the form "brcm,<soc>-<pll>-clk". For example, leaf >>>>> +clocks derived from the GENPLL on Cygnus SoC have a compatible string of >>>>> +"brcm,cygnus-genpll-clk" >>>>> + >>>>> +- #clock-cells: >>>>> + Have a value of <1> since there are more than 1 leaf clock of a >>>>> +given PLL >>>>> + >>>>> +- reg: >>>>> + Define the base and range of the I/O address space that contain the iProc >>>>> +clock control registers required for the PLL leaf clocks >>>>> + >>>>> +- clocks: >>>>> + The input parent PLL phandle for the leaf clock >>>>> + >>>>> +- clock-output-names: >>>>> + An ordered list of strings defining the names of the leaf clocks >>>>> + >>>>> +Example: >>>>> + >>>>> + genpll: genpll { >>>>> + #clock-cells = <0>; >>>>> + compatible = "brcm,cygnus-genpll"; >>>>> + reg = <0x0301d000 0x2c>, >>>>> + <0x0301c020 0x4>; >>>>> + clocks = <&osc>; >>>>> + }; >>>>> + >>>>> + genpll_clks: genpll_clks { >>>>> + #clock-cells = <1>; >>>>> + compatible = "brcm,cygnus-genpll-clk"; >>>>> + reg = <0x0301d000 0x2c>; >>>>> + clocks = <&genpll>; >>>>> + clock-output-names = "axi21", "250mhz", "ihost_sys", >>>>> + "enet_sw", "audio_125", "can"; >>>>> + }; >>>> >>>> Hi Ray, >>>> >>>> Thanks for submitting the patch. It was nice meeting you at ELC as well. >>>> >>>> This binding doesn't conform to the norms for clock bindings. It looks >>>> like for each type of controllable clock node (e.g. pll, leaf clock, >>>> etc) you have a dts node. Looking at the above example it seems that >>>> those two nodes (genpll and genpll_clks) share the same register. >>>> >>>> /me checks patch #5 >>>> >>>> Yup, you re-use the same register address for the *pll and *pll_clks >>>> nodes. I'm not a DT expert but I think this is considered Wrong. >>>> >>>> More generally your clock dt binding should be modeling the hardware in >>>> terms of IP blocks. If you have a clock generator IP block it may >>>> control many clock bits and output many clock signals. E.g. for your >>>> hardware (based only on the addresses in patch #5 and not having seen >>>> any data manual) I will hazard a guess that the genpll, lcpll and asiu >>>> clocks are all part of the same IP block. >>> >>> Hi Mike, >>> >>> In fact, lcpll, genpll, mipipll are similar but DIFFERENT IP blocks, and >>> asiu is completely different from any of them. All of these plls are >>> unique and have their own register banks, as you can see from the >>> bcm-cygnus-clock.dtsi file. Therefore, I think it's totally correct and >>> actually necessary to represent each of them with a separate device node. >> >> OK, that makes sense to me, if those registers live in addresses ranges >> which correspond to different IP blocks. >> >>> >>> Regarding the relationship between a PLL clock and its leaf clocks: >>> Taking the genpll as an example, physically they are connected as: >>> >>> xtal -> genpll -> axi21, 250mhz, ihost_sys, ... >>> >>> The 25 MHz crystal feeds to the genpll, and the genpll generates 6 >>> different leaf clocks including axi21, 250mhz, ihost_sys, and etc. One >>> can choose to set the genpll's vco to one frequency, and based on that >>> frequency, different leaf clock frequencies can be generated with their >>> own post divider. >>> >>> Therefore, I think it makes sense to represent xtal, genpll, genpll_clks >>> (including axi21, 250mhz, ihost_sys, and etc) each with a unique device >>> node, and genpll is the parent of these leaf clocks. Basically the >>> device nodes and the way how the "clocks" phandle is used represent the >>> hierarchy of the clock architecture within Cygnus (and in the future >>> other iProc SoCs). Does that make sense? >> >> This doesn't make sense to me. If I understand correctly, the register >> range that controls the post-divider clock (e.g. axi21) is the same >> register range that control's genpll. This is a reasonable indicator to >> me that the leaf clocks are part of the same clock generator IP block as >> the PLL controls. As such they should be on node. >> >>> >>> Regarding the register address overlapping, again, taking genpll as an >>> example, the genpll and the genpll_clks actually never access the same >>> set of registers, but the registers are sort of scattered within one >>> bank, i.e., on Cygnus, genpll uses registers at offset 0x0, 0x8 ~ 0x1c, >>> and genpll_clks uses registers at offset 0x4, 0x20 - 0x24. >> >> Sure, my argument above doesn't hinge on the fact that the pll and child >> clocks use the exact same register. It still looks to me like *pll and >> it's child dividers belong in the same IP block. Is there an open data >> sheet or technical reference manual I can look at for this part? That is >> the best way to put my concerns at ease ;-) >> >> Looking over your binding again, it looks like your nodes are divided >> conveniently for the different clock types (e.g. pll versus >> post-divider), but our goal with DT is to accurately describe the >> hardware, not the C structures. >> >> Regards, >> Mike >> > > Yes, the genpll and genpll_clks are controlled by the same IP block. I > can make the change to combine them to use one DT node. But before doing > that, I just need to get one thing clarified with you. In SW, the pll > and its leaf clocks will still be registered as separate two clock > providers, since we need to be able to configure the PLL vco frequency > and its leaf clock frequencies separately. The pll will be the parent of > its leaf clocks, and the leaf clocks will be used by various peripherals. > > I plan to have the combined DT looks like this: > > genpll_clks: genpll_clks { > #clock-cells = <1>; > compatible = "brcm,cygnus-genpll-clk"; > reg = <0x0301d000 0x2c>; > > assigned-clocks = <&genpll_clks>; > /* this sets the PLL rate at the time when the PLL clock is > registered */ > assigned-clock-rates = <4000000000>; > > clocks = <&osc>; > > clock-output-names = "axi21", "250mhz", "ihost_sys", "enet_sw", > "audio_125", "can"; > }; > > peripheralA: peripheralA { > /* use the first leaf clock of genpll */ > clocks = <&genpll_clks 0>; > clock-frequency = <100000000>; > }; > > If we register both the genpll and its leaf clocks to the clock > framework as two separate clock providers, would the above DT entries > still work? For example, peripheralA refers to the phandle of > genpll_clks, but how does the clock framework know to link it to the pll > clock or the leaf clock? > > Thanks, > > Ray > So I've changed the code to merge pll and pll_clks nodes and have played around with it a bit. Let me try to explain the issue I'm having here, and see if you can help to shed some light: As far as I know, the correct way to configure a clock's rate at the time of registering to the clock framework, as you mentioned previously, is to use the "assigned-clock" and "assigned-clock-rates" properties. Assuming we have a pll clock called "pll", with 6 leaf clocks derived from the pll called "clk-0", "clk-1", etc., and a peripheral using the first leaf clock: "clk-0". pll: pll { #clock-cell = <1>; compatible = "brcm,pll"; clocks = <&osc>; clock-output-names = "clk-0", "clk-1", ...; }; peripheral: peripheral { clocks = <&pll 0>; }; With your proposed way of constructing the above clock device tree, i.e., to combine "pll" and "clk-x" to use the same DT node, now how can one configure the frequency of the "pll" through device tree at the time of clock registration? There's now no access to "pll" through device tree but only access to its leaf clocks, e.g., clocks = <&pll 0>. The requirement comes from the fact that we need the peripheral to run at a certain clock rate off the leaf clock, and to achieve that we need the pll to run at a specific rate, since we only have an integer based post-divider between the pll and its leaf clocks. I was able to achieve this previously by separating the pll and its leaf clocks into two DT nodes, but I am really not sure how one can do this when merging pll and its leaf clocks into the same node. Any idea how this can be done cleanly with a single DT node representing the pll and its leaf clocks? Thanks, Ray
Quoting Ray Jui (2015-04-14 12:10:35) > Hi Mike, > > On 4/13/2015 12:40 PM, Ray Jui wrote: > > Hi Mike, > > > > On 4/12/2015 11:02 PM, Michael Turquette wrote: > >> Quoting Ray Jui (2015-04-12 21:08:32) > >>> > >>> > >>> On 4/10/2015 5:12 PM, Michael Turquette wrote: > >>>> Quoting Ray Jui (2015-03-17 22:45:17) > >>>>> Document the device tree binding for Broadcom iProc architecture based > >>>>> clock controller > >>>>> > >>>>> Signed-off-by: Ray Jui <rjui@broadcom.com> > >>>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> > >>>>> --- > >>>>> .../bindings/clock/brcm,iproc-clocks.txt | 171 ++++++++++++++++++++ > >>>>> 1 file changed, 171 insertions(+) > >>>>> create mode 100644 Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > >>>>> new file mode 100644 > >>>>> index 0000000..bf2316b > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt > >>>>> @@ -0,0 +1,171 @@ > >>>>> +Broadcom iProc Family Clocks > >>>>> + > >>>>> +This binding uses the common clock binding: > >>>>> + Documentation/devicetree/bindings/clock/clock-bindings.txt > >>>>> + > >>>>> +The iProc clock controller manages clocks that are common to the iProc family. > >>>>> +An SoC from the iProc family may have several PPLs, e.g., ARMPLL, GENPLL, > >>>>> +LCPLL0, MIPIPLL, and etc., all derived from an onboard crystal. Each PLL > >>>>> +comprises of several leaf clocks > >>>>> + > >>>>> +Required properties for PLLs: > >>>>> +- compatible: > >>>>> + Should have a value of the form "brcm,<soc>-<pll>". For example, GENPLL on > >>>>> +Cygnus has a compatible string of "brcm,cygnus-genpll" > >>>>> + > >>>>> +- #clock-cells: > >>>>> + Must be <0> > >>>>> + > >>>>> +- reg: > >>>>> + Define the base and range of the I/O address space that contain the iProc > >>>>> +clock control registers required for the PLL > >>>>> + > >>>>> +- clocks: > >>>>> + The input parent clock phandle for the PLL. For all iProc PLLs, this is an > >>>>> +onboard crystal with a fixed rate > >>>>> + > >>>>> +Example: > >>>>> + > >>>>> + osc: oscillator { > >>>>> + #clock-cells = <0>; > >>>>> + compatible = "fixed-clock"; > >>>>> + clock-frequency = <25000000>; > >>>>> + }; > >>>>> + > >>>>> + genpll: genpll { > >>>>> + #clock-cells = <0>; > >>>>> + compatible = "brcm,cygnus-genpll"; > >>>>> + reg = <0x0301d000 0x2c>, > >>>>> + <0x0301c020 0x4>; > >>>>> + clocks = <&osc>; > >>>>> + }; > >>>>> + > >>>>> +Required properties for leaf clocks of a PLL: > >>>>> + > >>>>> +- compatible: > >>>>> + Should have a value of the form "brcm,<soc>-<pll>-clk". For example, leaf > >>>>> +clocks derived from the GENPLL on Cygnus SoC have a compatible string of > >>>>> +"brcm,cygnus-genpll-clk" > >>>>> + > >>>>> +- #clock-cells: > >>>>> + Have a value of <1> since there are more than 1 leaf clock of a > >>>>> +given PLL > >>>>> + > >>>>> +- reg: > >>>>> + Define the base and range of the I/O address space that contain the iProc > >>>>> +clock control registers required for the PLL leaf clocks > >>>>> + > >>>>> +- clocks: > >>>>> + The input parent PLL phandle for the leaf clock > >>>>> + > >>>>> +- clock-output-names: > >>>>> + An ordered list of strings defining the names of the leaf clocks > >>>>> + > >>>>> +Example: > >>>>> + > >>>>> + genpll: genpll { > >>>>> + #clock-cells = <0>; > >>>>> + compatible = "brcm,cygnus-genpll"; > >>>>> + reg = <0x0301d000 0x2c>, > >>>>> + <0x0301c020 0x4>; > >>>>> + clocks = <&osc>; > >>>>> + }; > >>>>> + > >>>>> + genpll_clks: genpll_clks { > >>>>> + #clock-cells = <1>; > >>>>> + compatible = "brcm,cygnus-genpll-clk"; > >>>>> + reg = <0x0301d000 0x2c>; > >>>>> + clocks = <&genpll>; > >>>>> + clock-output-names = "axi21", "250mhz", "ihost_sys", > >>>>> + "enet_sw", "audio_125", "can"; > >>>>> + }; > >>>> > >>>> Hi Ray, > >>>> > >>>> Thanks for submitting the patch. It was nice meeting you at ELC as well. > >>>> > >>>> This binding doesn't conform to the norms for clock bindings. It looks > >>>> like for each type of controllable clock node (e.g. pll, leaf clock, > >>>> etc) you have a dts node. Looking at the above example it seems that > >>>> those two nodes (genpll and genpll_clks) share the same register. > >>>> > >>>> /me checks patch #5 > >>>> > >>>> Yup, you re-use the same register address for the *pll and *pll_clks > >>>> nodes. I'm not a DT expert but I think this is considered Wrong. > >>>> > >>>> More generally your clock dt binding should be modeling the hardware in > >>>> terms of IP blocks. If you have a clock generator IP block it may > >>>> control many clock bits and output many clock signals. E.g. for your > >>>> hardware (based only on the addresses in patch #5 and not having seen > >>>> any data manual) I will hazard a guess that the genpll, lcpll and asiu > >>>> clocks are all part of the same IP block. > >>> > >>> Hi Mike, > >>> > >>> In fact, lcpll, genpll, mipipll are similar but DIFFERENT IP blocks, and > >>> asiu is completely different from any of them. All of these plls are > >>> unique and have their own register banks, as you can see from the > >>> bcm-cygnus-clock.dtsi file. Therefore, I think it's totally correct and > >>> actually necessary to represent each of them with a separate device node. > >> > >> OK, that makes sense to me, if those registers live in addresses ranges > >> which correspond to different IP blocks. > >> > >>> > >>> Regarding the relationship between a PLL clock and its leaf clocks: > >>> Taking the genpll as an example, physically they are connected as: > >>> > >>> xtal -> genpll -> axi21, 250mhz, ihost_sys, ... > >>> > >>> The 25 MHz crystal feeds to the genpll, and the genpll generates 6 > >>> different leaf clocks including axi21, 250mhz, ihost_sys, and etc. One > >>> can choose to set the genpll's vco to one frequency, and based on that > >>> frequency, different leaf clock frequencies can be generated with their > >>> own post divider. > >>> > >>> Therefore, I think it makes sense to represent xtal, genpll, genpll_clks > >>> (including axi21, 250mhz, ihost_sys, and etc) each with a unique device > >>> node, and genpll is the parent of these leaf clocks. Basically the > >>> device nodes and the way how the "clocks" phandle is used represent the > >>> hierarchy of the clock architecture within Cygnus (and in the future > >>> other iProc SoCs). Does that make sense? > >> > >> This doesn't make sense to me. If I understand correctly, the register > >> range that controls the post-divider clock (e.g. axi21) is the same > >> register range that control's genpll. This is a reasonable indicator to > >> me that the leaf clocks are part of the same clock generator IP block as > >> the PLL controls. As such they should be on node. > >> > >>> > >>> Regarding the register address overlapping, again, taking genpll as an > >>> example, the genpll and the genpll_clks actually never access the same > >>> set of registers, but the registers are sort of scattered within one > >>> bank, i.e., on Cygnus, genpll uses registers at offset 0x0, 0x8 ~ 0x1c, > >>> and genpll_clks uses registers at offset 0x4, 0x20 - 0x24. > >> > >> Sure, my argument above doesn't hinge on the fact that the pll and child > >> clocks use the exact same register. It still looks to me like *pll and > >> it's child dividers belong in the same IP block. Is there an open data > >> sheet or technical reference manual I can look at for this part? That is > >> the best way to put my concerns at ease ;-) > >> > >> Looking over your binding again, it looks like your nodes are divided > >> conveniently for the different clock types (e.g. pll versus > >> post-divider), but our goal with DT is to accurately describe the > >> hardware, not the C structures. > >> > >> Regards, > >> Mike > >> > > > > Yes, the genpll and genpll_clks are controlled by the same IP block. I > > can make the change to combine them to use one DT node. But before doing > > that, I just need to get one thing clarified with you. In SW, the pll > > and its leaf clocks will still be registered as separate two clock > > providers, since we need to be able to configure the PLL vco frequency > > and its leaf clock frequencies separately. The pll will be the parent of > > its leaf clocks, and the leaf clocks will be used by various peripherals. > > > > I plan to have the combined DT looks like this: > > > > genpll_clks: genpll_clks { > > #clock-cells = <1>; > > compatible = "brcm,cygnus-genpll-clk"; > > reg = <0x0301d000 0x2c>; > > > > assigned-clocks = <&genpll_clks>; > > /* this sets the PLL rate at the time when the PLL clock is > > registered */ > > assigned-clock-rates = <4000000000>; > > > > clocks = <&osc>; > > > > clock-output-names = "axi21", "250mhz", "ihost_sys", "enet_sw", > > "audio_125", "can"; > > }; > > > > peripheralA: peripheralA { > > /* use the first leaf clock of genpll */ > > clocks = <&genpll_clks 0>; > > clock-frequency = <100000000>; > > }; > > > > If we register both the genpll and its leaf clocks to the clock > > framework as two separate clock providers, would the above DT entries > > still work? For example, peripheralA refers to the phandle of > > genpll_clks, but how does the clock framework know to link it to the pll > > clock or the leaf clock? Hi Ray, There are two options here: using clock-output-names or not using clock-output-names. I would recommend against using clock-output-names. If both your clock driver and the peripheral devices that consume these clocks are all represented in DT then you can skip clock-output-names and instead create phandle linkage between the clocks inside of your clock provider and the consumer devices. String names are still important here, but now you only need to specify the string name as an *input* to the consumer device, and not as an *output* from the clock provider node. Let's look at how the qcom bindings do it: gcc is a clock generator ip block. arch/arm/boot/dts/qcom-apq8084.dtsi: gcc: clock-controller@fc400000 { compatible = "qcom,gcc-apq8084"; #clock-cells = <1>; #reset-cells = <1>; reg = <0xfc400000 0x4000>; }; This same file includes this header: #include <dt-bindings/clock/qcom,gcc-apq8084.h> That header creates a map of clocks by numbers. An example: #define GCC_BLSP2_UART2_APPS_CLK 142 We can see how it used by the serial controller in arch/arm/boot/dts/qcom-apq8084.dtsi: serial@f995e000 { compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm"; reg = <0xf995e000 0x1000>; interrupts = <0 114 0x0>; clocks = <&gcc GCC_BLSP2_UART2_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>; clock-names = "core", "iface"; status = "disabled"; }; Note the 'clock-names' property. This lists a string name which is a property of the *consuming* device (uart controller), not the *providing* device (gcc clock generator). Finally, it all comes together in the driver simply using clk_get: msm_port->clk = devm_clk_get(&pdev->dev, "core"); if (IS_ERR(msm_port->clk)) return PTR_ERR(msm_port->clk); if (msm_port->is_uartdm) { msm_port->pclk = devm_clk_get(&pdev->dev, "iface"); if (IS_ERR(msm_port->pclk)) return PTR_ERR(msm_port->pclk); clk_set_rate(msm_port->clk, 1843200); } This works because we have created linkage between the clock phandle and the consuming device node. The shared header is used by the Linux kernel to look up the clock as well as the consumer node to map the string name onto a clock input. Back to clock-output-names: If you decide to keep using clock-output-names you can still represent the pll clock in your "clock-output-names". In a perfect world DT nodes that use clock-output-names would only expose the leaf clocks that your peripheral devices consume, but there is no technical reason why your pll can't be a part of the clock-output-names array. It is simply an array of string names that maps to an index. There is no sense of parent-child hierarchy in the DT binding, so there is no problem adding a parent pll clock to the array of clock-output-names. I think that this will tactically solve your problem in the short term, but I encourage you to inspect the qcom binding example I gave above to see if it a better fit for you long term. Regards, Mike > > > > Thanks, > > > > Ray > > > > So I've changed the code to merge pll and pll_clks nodes and have played > around with it a bit. Let me try to explain the issue I'm having here, > and see if you can help to shed some light: > > As far as I know, the correct way to configure a clock's rate at the > time of registering to the clock framework, as you mentioned previously, > is to use the "assigned-clock" and "assigned-clock-rates" properties. > > Assuming we have a pll clock called "pll", with 6 leaf clocks derived > from the pll called "clk-0", "clk-1", etc., and a peripheral using the > first leaf clock: "clk-0". > > pll: pll { > #clock-cell = <1>; > compatible = "brcm,pll"; > clocks = <&osc>; > clock-output-names = "clk-0", "clk-1", ...; > }; > > peripheral: peripheral { > clocks = <&pll 0>; > }; > > With your proposed way of constructing the above clock device tree, > i.e., to combine "pll" and "clk-x" to use the same DT node, now how can > one configure the frequency of the "pll" through device tree at the time > of clock registration? There's now no access to "pll" through device > tree but only access to its leaf clocks, e.g., clocks = <&pll 0>. > > The requirement comes from the fact that we need the peripheral to run > at a certain clock rate off the leaf clock, and to achieve that we need > the pll to run at a specific rate, since we only have an integer based > post-divider between the pll and its leaf clocks. I was able to achieve > this previously by separating the pll and its leaf clocks into two DT > nodes, but I am really not sure how one can do this when merging pll and > its leaf clocks into the same node. > > Any idea how this can be done cleanly with a single DT node representing > the pll and its leaf clocks? > > Thanks, > > Ray
Hi Mike, On 4/16/2015 12:20 PM, Michael Turquette wrote: > Quoting Ray Jui (2015-04-14 12:10:35) >> Hi Mike, >> >> On 4/13/2015 12:40 PM, Ray Jui wrote: >>> Hi Mike, >>> >>> On 4/12/2015 11:02 PM, Michael Turquette wrote: >>>> Quoting Ray Jui (2015-04-12 21:08:32) >>>>> >>>>> >>>>> On 4/10/2015 5:12 PM, Michael Turquette wrote: >>>>>> Quoting Ray Jui (2015-03-17 22:45:17) >>>>>>> Document the device tree binding for Broadcom iProc architecture based >>>>>>> clock controller >>>>>>> >>>>>>> Signed-off-by: Ray Jui <rjui@broadcom.com> >>>>>>> Reviewed-by: Scott Branden <sbranden@broadcom.com> >>>>>>> --- >>>>>>> .../bindings/clock/brcm,iproc-clocks.txt | 171 ++++++++++++++++++++ >>>>>>> 1 file changed, 171 insertions(+) >>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>>>>> new file mode 100644 >>>>>>> index 0000000..bf2316b >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt >>>>>>> @@ -0,0 +1,171 @@ >>>>>>> +Broadcom iProc Family Clocks >>>>>>> + >>>>>>> +This binding uses the common clock binding: >>>>>>> + Documentation/devicetree/bindings/clock/clock-bindings.txt >>>>>>> + >>>>>>> +The iProc clock controller manages clocks that are common to the iProc family. >>>>>>> +An SoC from the iProc family may have several PPLs, e.g., ARMPLL, GENPLL, >>>>>>> +LCPLL0, MIPIPLL, and etc., all derived from an onboard crystal. Each PLL >>>>>>> +comprises of several leaf clocks >>>>>>> + >>>>>>> +Required properties for PLLs: >>>>>>> +- compatible: >>>>>>> + Should have a value of the form "brcm,<soc>-<pll>". For example, GENPLL on >>>>>>> +Cygnus has a compatible string of "brcm,cygnus-genpll" >>>>>>> + >>>>>>> +- #clock-cells: >>>>>>> + Must be <0> >>>>>>> + >>>>>>> +- reg: >>>>>>> + Define the base and range of the I/O address space that contain the iProc >>>>>>> +clock control registers required for the PLL >>>>>>> + >>>>>>> +- clocks: >>>>>>> + The input parent clock phandle for the PLL. For all iProc PLLs, this is an >>>>>>> +onboard crystal with a fixed rate >>>>>>> + >>>>>>> +Example: >>>>>>> + >>>>>>> + osc: oscillator { >>>>>>> + #clock-cells = <0>; >>>>>>> + compatible = "fixed-clock"; >>>>>>> + clock-frequency = <25000000>; >>>>>>> + }; >>>>>>> + >>>>>>> + genpll: genpll { >>>>>>> + #clock-cells = <0>; >>>>>>> + compatible = "brcm,cygnus-genpll"; >>>>>>> + reg = <0x0301d000 0x2c>, >>>>>>> + <0x0301c020 0x4>; >>>>>>> + clocks = <&osc>; >>>>>>> + }; >>>>>>> + >>>>>>> +Required properties for leaf clocks of a PLL: >>>>>>> + >>>>>>> +- compatible: >>>>>>> + Should have a value of the form "brcm,<soc>-<pll>-clk". For example, leaf >>>>>>> +clocks derived from the GENPLL on Cygnus SoC have a compatible string of >>>>>>> +"brcm,cygnus-genpll-clk" >>>>>>> + >>>>>>> +- #clock-cells: >>>>>>> + Have a value of <1> since there are more than 1 leaf clock of a >>>>>>> +given PLL >>>>>>> + >>>>>>> +- reg: >>>>>>> + Define the base and range of the I/O address space that contain the iProc >>>>>>> +clock control registers required for the PLL leaf clocks >>>>>>> + >>>>>>> +- clocks: >>>>>>> + The input parent PLL phandle for the leaf clock >>>>>>> + >>>>>>> +- clock-output-names: >>>>>>> + An ordered list of strings defining the names of the leaf clocks >>>>>>> + >>>>>>> +Example: >>>>>>> + >>>>>>> + genpll: genpll { >>>>>>> + #clock-cells = <0>; >>>>>>> + compatible = "brcm,cygnus-genpll"; >>>>>>> + reg = <0x0301d000 0x2c>, >>>>>>> + <0x0301c020 0x4>; >>>>>>> + clocks = <&osc>; >>>>>>> + }; >>>>>>> + >>>>>>> + genpll_clks: genpll_clks { >>>>>>> + #clock-cells = <1>; >>>>>>> + compatible = "brcm,cygnus-genpll-clk"; >>>>>>> + reg = <0x0301d000 0x2c>; >>>>>>> + clocks = <&genpll>; >>>>>>> + clock-output-names = "axi21", "250mhz", "ihost_sys", >>>>>>> + "enet_sw", "audio_125", "can"; >>>>>>> + }; >>>>>> >>>>>> Hi Ray, >>>>>> >>>>>> Thanks for submitting the patch. It was nice meeting you at ELC as well. >>>>>> >>>>>> This binding doesn't conform to the norms for clock bindings. It looks >>>>>> like for each type of controllable clock node (e.g. pll, leaf clock, >>>>>> etc) you have a dts node. Looking at the above example it seems that >>>>>> those two nodes (genpll and genpll_clks) share the same register. >>>>>> >>>>>> /me checks patch #5 >>>>>> >>>>>> Yup, you re-use the same register address for the *pll and *pll_clks >>>>>> nodes. I'm not a DT expert but I think this is considered Wrong. >>>>>> >>>>>> More generally your clock dt binding should be modeling the hardware in >>>>>> terms of IP blocks. If you have a clock generator IP block it may >>>>>> control many clock bits and output many clock signals. E.g. for your >>>>>> hardware (based only on the addresses in patch #5 and not having seen >>>>>> any data manual) I will hazard a guess that the genpll, lcpll and asiu >>>>>> clocks are all part of the same IP block. >>>>> >>>>> Hi Mike, >>>>> >>>>> In fact, lcpll, genpll, mipipll are similar but DIFFERENT IP blocks, and >>>>> asiu is completely different from any of them. All of these plls are >>>>> unique and have their own register banks, as you can see from the >>>>> bcm-cygnus-clock.dtsi file. Therefore, I think it's totally correct and >>>>> actually necessary to represent each of them with a separate device node. >>>> >>>> OK, that makes sense to me, if those registers live in addresses ranges >>>> which correspond to different IP blocks. >>>> >>>>> >>>>> Regarding the relationship between a PLL clock and its leaf clocks: >>>>> Taking the genpll as an example, physically they are connected as: >>>>> >>>>> xtal -> genpll -> axi21, 250mhz, ihost_sys, ... >>>>> >>>>> The 25 MHz crystal feeds to the genpll, and the genpll generates 6 >>>>> different leaf clocks including axi21, 250mhz, ihost_sys, and etc. One >>>>> can choose to set the genpll's vco to one frequency, and based on that >>>>> frequency, different leaf clock frequencies can be generated with their >>>>> own post divider. >>>>> >>>>> Therefore, I think it makes sense to represent xtal, genpll, genpll_clks >>>>> (including axi21, 250mhz, ihost_sys, and etc) each with a unique device >>>>> node, and genpll is the parent of these leaf clocks. Basically the >>>>> device nodes and the way how the "clocks" phandle is used represent the >>>>> hierarchy of the clock architecture within Cygnus (and in the future >>>>> other iProc SoCs). Does that make sense? >>>> >>>> This doesn't make sense to me. If I understand correctly, the register >>>> range that controls the post-divider clock (e.g. axi21) is the same >>>> register range that control's genpll. This is a reasonable indicator to >>>> me that the leaf clocks are part of the same clock generator IP block as >>>> the PLL controls. As such they should be on node. >>>> >>>>> >>>>> Regarding the register address overlapping, again, taking genpll as an >>>>> example, the genpll and the genpll_clks actually never access the same >>>>> set of registers, but the registers are sort of scattered within one >>>>> bank, i.e., on Cygnus, genpll uses registers at offset 0x0, 0x8 ~ 0x1c, >>>>> and genpll_clks uses registers at offset 0x4, 0x20 - 0x24. >>>> >>>> Sure, my argument above doesn't hinge on the fact that the pll and child >>>> clocks use the exact same register. It still looks to me like *pll and >>>> it's child dividers belong in the same IP block. Is there an open data >>>> sheet or technical reference manual I can look at for this part? That is >>>> the best way to put my concerns at ease ;-) >>>> >>>> Looking over your binding again, it looks like your nodes are divided >>>> conveniently for the different clock types (e.g. pll versus >>>> post-divider), but our goal with DT is to accurately describe the >>>> hardware, not the C structures. >>>> >>>> Regards, >>>> Mike >>>> >>> >>> Yes, the genpll and genpll_clks are controlled by the same IP block. I >>> can make the change to combine them to use one DT node. But before doing >>> that, I just need to get one thing clarified with you. In SW, the pll >>> and its leaf clocks will still be registered as separate two clock >>> providers, since we need to be able to configure the PLL vco frequency >>> and its leaf clock frequencies separately. The pll will be the parent of >>> its leaf clocks, and the leaf clocks will be used by various peripherals. >>> >>> I plan to have the combined DT looks like this: >>> >>> genpll_clks: genpll_clks { >>> #clock-cells = <1>; >>> compatible = "brcm,cygnus-genpll-clk"; >>> reg = <0x0301d000 0x2c>; >>> >>> assigned-clocks = <&genpll_clks>; >>> /* this sets the PLL rate at the time when the PLL clock is >>> registered */ >>> assigned-clock-rates = <4000000000>; >>> >>> clocks = <&osc>; >>> >>> clock-output-names = "axi21", "250mhz", "ihost_sys", "enet_sw", >>> "audio_125", "can"; >>> }; >>> >>> peripheralA: peripheralA { >>> /* use the first leaf clock of genpll */ >>> clocks = <&genpll_clks 0>; >>> clock-frequency = <100000000>; >>> }; >>> >>> If we register both the genpll and its leaf clocks to the clock >>> framework as two separate clock providers, would the above DT entries >>> still work? For example, peripheralA refers to the phandle of >>> genpll_clks, but how does the clock framework know to link it to the pll >>> clock or the leaf clock? > > Hi Ray, > > There are two options here: using clock-output-names or not using > clock-output-names. > > I would recommend against using clock-output-names. If both your > clock driver and the peripheral devices that consume these clocks are > all represented in DT then you can skip clock-output-names and instead > create phandle linkage between the clocks inside of your clock provider > and the consumer devices. String names are still important here, but now > you only need to specify the string name as an *input* to the consumer > device, and not as an *output* from the clock provider node. Let's look > at how the qcom bindings do it: > > gcc is a clock generator ip block. > > arch/arm/boot/dts/qcom-apq8084.dtsi: > gcc: clock-controller@fc400000 { > compatible = "qcom,gcc-apq8084"; > #clock-cells = <1>; > #reset-cells = <1>; > reg = <0xfc400000 0x4000>; > }; > > This same file includes this header: > #include <dt-bindings/clock/qcom,gcc-apq8084.h> > > That header creates a map of clocks by numbers. An example: > #define GCC_BLSP2_UART2_APPS_CLK 142 > > We can see how it used by the serial controller in > arch/arm/boot/dts/qcom-apq8084.dtsi: > serial@f995e000 { > compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm"; > reg = <0xf995e000 0x1000>; > interrupts = <0 114 0x0>; > clocks = <&gcc GCC_BLSP2_UART2_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>; > clock-names = "core", "iface"; > status = "disabled"; > }; > > Note the 'clock-names' property. This lists a string name which is a property > of the *consuming* device (uart controller), not the *providing* device (gcc > clock generator). > > Finally, it all comes together in the driver simply using clk_get: > msm_port->clk = devm_clk_get(&pdev->dev, "core"); > if (IS_ERR(msm_port->clk)) > return PTR_ERR(msm_port->clk); > > if (msm_port->is_uartdm) { > msm_port->pclk = devm_clk_get(&pdev->dev, "iface"); > if (IS_ERR(msm_port->pclk)) > return PTR_ERR(msm_port->pclk); > > clk_set_rate(msm_port->clk, 1843200); > } > > This works because we have created linkage between the clock phandle and > the consuming device node. The shared header is used by the Linux kernel > to look up the clock as well as the consumer node to map the string name > onto a clock input. > > Back to clock-output-names: > > If you decide to keep using clock-output-names you can still represent > the pll clock in your "clock-output-names". In a perfect world DT nodes > that use clock-output-names would only expose the leaf clocks that your > peripheral devices consume, but there is no technical reason why your > pll can't be a part of the clock-output-names array. It is simply an > array of string names that maps to an index. There is no sense of > parent-child hierarchy in the DT binding, so there is no problem adding > a parent pll clock to the array of clock-output-names. I think that this > will tactically solve your problem in the short term, but I encourage > you to inspect the qcom binding example I gave above to see if it a > better fit for you long term. > > Regards, > Mike > Thanks for your thorough explanation. You clarified that there does not need to be a clock parent-child hierarchy in the DT binding. I think that's where I got really confused at. Yes, by exposing both the pll (parent) and its leaf clocks (children) with individual indices under the same node, that should solve the problem I have. I will investigate and work out a new patch. Thanks, Ray
diff --git a/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt new file mode 100644 index 0000000..bf2316b --- /dev/null +++ b/Documentation/devicetree/bindings/clock/brcm,iproc-clocks.txt @@ -0,0 +1,171 @@ +Broadcom iProc Family Clocks + +This binding uses the common clock binding: + Documentation/devicetree/bindings/clock/clock-bindings.txt + +The iProc clock controller manages clocks that are common to the iProc family. +An SoC from the iProc family may have several PPLs, e.g., ARMPLL, GENPLL, +LCPLL0, MIPIPLL, and etc., all derived from an onboard crystal. Each PLL +comprises of several leaf clocks + +Required properties for PLLs: +- compatible: + Should have a value of the form "brcm,<soc>-<pll>". For example, GENPLL on +Cygnus has a compatible string of "brcm,cygnus-genpll" + +- #clock-cells: + Must be <0> + +- reg: + Define the base and range of the I/O address space that contain the iProc +clock control registers required for the PLL + +- clocks: + The input parent clock phandle for the PLL. For all iProc PLLs, this is an +onboard crystal with a fixed rate + +Example: + + osc: oscillator { + #clock-cells = <0>; + compatible = "fixed-clock"; + clock-frequency = <25000000>; + }; + + genpll: genpll { + #clock-cells = <0>; + compatible = "brcm,cygnus-genpll"; + reg = <0x0301d000 0x2c>, + <0x0301c020 0x4>; + clocks = <&osc>; + }; + +Required properties for leaf clocks of a PLL: + +- compatible: + Should have a value of the form "brcm,<soc>-<pll>-clk". For example, leaf +clocks derived from the GENPLL on Cygnus SoC have a compatible string of +"brcm,cygnus-genpll-clk" + +- #clock-cells: + Have a value of <1> since there are more than 1 leaf clock of a +given PLL + +- reg: + Define the base and range of the I/O address space that contain the iProc +clock control registers required for the PLL leaf clocks + +- clocks: + The input parent PLL phandle for the leaf clock + +- clock-output-names: + An ordered list of strings defining the names of the leaf clocks + +Example: + + genpll: genpll { + #clock-cells = <0>; + compatible = "brcm,cygnus-genpll"; + reg = <0x0301d000 0x2c>, + <0x0301c020 0x4>; + clocks = <&osc>; + }; + + genpll_clks: genpll_clks { + #clock-cells = <1>; + compatible = "brcm,cygnus-genpll-clk"; + reg = <0x0301d000 0x2c>; + clocks = <&genpll>; + clock-output-names = "axi21", "250mhz", "ihost_sys", + "enet_sw", "audio_125", "can"; + }; + +Required properties for ASIU clocks: + +ASIU clocks are a special case. These clocks are derived directly from the +reference clock of the onboard crystal + +- compatible: + Should have a value of the form "brcm,<soc>-asiu-clk". For example, ASIU +clocks for Cygnus have a compatible string of "brcm,cygnus-asiu-clk" + +- #clock-cells: + Have a value of <1> since there are more than 1 ASIU clocks + +- reg: + Define the base and range of the I/O address space that contain the iProc +clock control registers required for ASIU clocks + +- clocks: + The input parent clock phandle for the ASIU clock, i.e., the onboard +crystal + +- clock-output-names: + An ordered list of strings defining the names of the ASIU clocks + +Example: + + osc: oscillator { + #clock-cells = <0>; + compatible = "fixed-clock"; + clock-frequency = <25000000>; + }; + + asiu_clks: asiu_clks { + #clock-cells = <1>; + compatible = "brcm,cygnus-asiu-clk"; + reg = <0x0301d048 0xc>, + <0x180aa024 0x4>; + clocks = <&osc>; + clock-output-names = "keypad", "adc/touch", "pwm"; + }; + +Cygnus +------ +PLL and leaf clock compatible strings for Cygnus are: + "brcm,cygnus-armpll" + "brcm,cygnus-genpll" + "brcm,cygnus-lcpll0" + "brcm,cygnus-mipipll" + "brcm,cygnus-genpll-clk" + "brcm,cygnus-lcpll0-clk" + "brcm,cygnus-mipipll-clk" + "brcm,cygnus-asiu-clk" + +The following table defines the set of PLL/clock index and ID for Cygnus. +These clock IDs are defined in: + "include/dt-bindings/clock/bcm-cygnus.h" + + Clock Source Index ID + --- ----- ----- --------- + crystal N/A N/A N/A + + armpll crystal N/A N/A + genpll crystal N/A N/A + lcpll0 crystal N/A N/A + mipipll crystal N/A N/A + + keypad crystal (ASIU) 0 BCM_CYGNUS_ASIU_KEYPAD_CLK + adc/tsc crystal (ASIU) 1 BCM_CYGNUS_ASIU_ADC_CLK + pwm crystal (ASIU) 2 BCM_CYGNUS_ASIU_PWM_CLK + + axi21 genpll 0 BCM_CYGNUS_GENPLL_AXI21_CLK + 250mhz genpll 1 BCM_CYGNUS_GENPLL_250MHZ_CLK + ihost_sys genpll 2 BCM_CYGNUS_GENPLL_IHOST_SYS_CLK + enet_sw genpll 3 BCM_CYGNUS_GENPLL_ENET_SW_CLK + audio_125 genpll 4 BCM_CYGNUS_GENPLL_AUDIO_125_CLK + can genpll 5 BCM_CYGNUS_GENPLL_CAN_CLK + + pcie_phy lcpll0 0 BCM_CYGNUS_LCPLL0_PCIE_PHY_REF_CLK + ddr_phy lcpll0 1 BCM_CYGNUS_LCPLL0_DDR_PHY_CLK + sdio lcpll0 2 BCM_CYGNUS_LCPLL0_SDIO_CLK + usb_phy lcpll0 3 BCM_CYGNUS_LCPLL0_USB_PHY_REF_CLK + smart_card lcpll0 4 BCM_CYGNUS_LCPLL0_SMART_CARD_CLK + ch5_unused lcpll0 5 BCM_CYGNUS_LCPLL0_CH5_UNUSED + + ch0_unused mipipll 0 BCM_CYGNUS_MIPIPLL_CH0_UNUSED + ch1_lcd mipipll 1 BCM_CYGNUS_MIPIPLL_CH1_LCD + ch2_v3d mipipll 2 BCM_CYGNUS_MIPIPLL_CH2_V3D + ch3_unused mipipll 3 BCM_CYGNUS_MIPIPLL_CH3_UNUSED + ch4_unused mipipll 4 BCM_CYGNUS_MIPIPLL_CH4_UNUSED + ch5_unused mipipll 5 BCM_CYGNUS_MIPIPLL_CH5_UNUSED