DT clk binding support
Mike,
Please pull devicetree clock binding support for 3.5. It's a bit late I know, but this shouldn't affect anyone except for highbank which uses it. It will be good to get it in so other platforms can start using it.
If you prefer, perhaps Arnd/Olof can pull this directly if you don't have any other pulls planned for 3.5-rc1.
Rob
The following changes since commit f0948f59dbc8e725a96ba16da666e8f5cdd43ba8:
clk: add a fixed factor clock (2012-05-08 14:13:25 -0700)
are available in the git repository at:
git://sources.calxeda.com/kernel/linux.git clk-for-3.5
for you to fetch changes up to 9dca208955ffba16adc99d982fd7432b37f76faa:
clk: add highbank clock support (2012-05-15 19:14:10 -0500)
----------------------------------------------------------------
Grant Likely (2):
clk: add DT clock binding support
clk: add DT fixed-clock binding support
Rob Herring (2):
dt: add clock binding doc to primecell bindings
clk: add highbank clock support
.../devicetree/bindings/arm/primecell.txt | 6 + .../devicetree/bindings/clock/calxeda.txt | 17 + .../devicetree/bindings/clock/clock-bindings.txt | 117 +++++++ .../devicetree/bindings/clock/fixed-clock.txt | 21 ++ arch/arm/Kconfig | 1 + arch/arm/boot/dts/highbank.dts | 91 +++++- arch/arm/mach-highbank/Makefile | 2 +- arch/arm/mach-highbank/clock.c | 62 ---- arch/arm/mach-highbank/highbank.c | 7 + drivers/clk/Makefile | 4 +- drivers/clk/clk-fixed-rate.c | 23 ++ drivers/clk/clk-highbank.c | 345 ++++++++++++++++++++ drivers/clk/clk.c | 140 ++++++++ drivers/clk/clkdev.c | 77 +++++ include/linux/clk-provider.h | 16 + include/linux/clk.h | 19 ++ 16 files changed, 883 insertions(+), 65 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/calxeda.txt create mode 100644 Documentation/devicetree/bindings/clock/clock-bindings.txt create mode 100644 Documentation/devicetree/bindings/clock/fixed-clock.txt delete mode 100644 arch/arm/mach-highbank/clock.c create mode 100644 drivers/clk/clk-highbank.c
Quoted 2 lines · Rob Herring
Grant Likely (2):
clk: add DT clock binding support
I just checked your branch and found that a couple of comments that I put on this patch haven't get addressed. The most notable one would be the "clocks" property of clock consumers.
+==Clock consumers==
+
+Required properties:
+clocks: List of phandle and clock specifier pairs, one pair
+ for each clock input to the device. Note: if the + clock provider specifies '0' for #clock-cells, then + only the phandle portion of the pair will appear. + +Optional properties: +clock-names: List of clock input name strings sorted in the same + order as the clocks property. Consumers drivers + will use clock-names to match clock input names + with clocks specifiers.
As I said, any clock in the clock tree except root clock is not only a clock provider but also a consumer. If you define "clocks" as a required property for clock consumers, you are essentially asking users to either define the whole clock tree in the device tree or stay away from device tree completely.
Are you sure this is the right thing to do? If I remember correctly, Grant's position is it should be pretty reasonable to have most of the clock tree defined in clock driver and only define those leaf clocks which are very likely to become the clock providers for other peripherals.
Let me put a terrible example here. Since clock tree is actually SoC specific, I can reasonably choose to define the entire imx6q clock tree and all the clk lookups for imx6q peripherals in clk-imx6q driver. On imx6q-sabrelite board, the audio codec sgtl5000 uses cko (an imx6q clock available on pad) as the clock source. That said, I need a board specific clk lookup here, which should be the best user of clock DT bindings. But sadly, with the current bindings, I can not give the required "clocks" property for sgtl5000 node.
Signature · 2 lines
Regards, Shawn
Quoted 22 lines · Shawn Guo
Quoted 2 lines · Rob Herring
Grant Likely (2):
clk: add DT clock binding supportI just checked your branch and found that a couple of comments that I put on this patch haven't get addressed. The most notable one would be the "clocks" property of clock consumers.
+==Clock consumers==
(quoted diff) +12
-0+ +Required properties: +clocks: List of phandle and clock specifier pairs, one pair + for each clock input to the device. Note: if the + clock provider specifies '0' for #clock-cells, then + only the phandle portion of the pair will appear. + +Optional properties: +clock-names: List of clock input name strings sorted in the same + order as the clocks property. Consumers drivers + will use clock-names to match clock input names + with clocks specifiers.
Quoted 5 lines
As I said, any clock in the clock tree except root clock is not only a clock provider but also a consumer. If you define "clocks" as a required property for clock consumers, you are essentially asking users to either define the whole clock tree in the device tree or stay away from device tree completely.
So what are you proposing that a clock consumer have? The very definition of a clock consumer is that it has a clocks property.
Quoted 6 lines
Are you sure this is the right thing to do? If I remember correctly, Grant's position is it should be pretty reasonable to have most of the clock tree defined in clock driver and only define those leaf clocks which are very likely to become the clock providers for other peripherals.
The minimum is you have to have a provider and consumer. It may be a single provider that provides all clocks for a chip. If you don't want a provider, then just define a clock-frequency property.
Quoted 9 lines
Let me put a terrible example here. Since clock tree is actually SoC specific, I can reasonably choose to define the entire imx6q clock tree and all the clk lookups for imx6q peripherals in clk-imx6q driver. On imx6q-sabrelite board, the audio codec sgtl5000 uses cko (an imx6q clock available on pad) as the clock source. That said, I need a board specific clk lookup here, which should be the best user of clock DT bindings. But sadly, with the current bindings, I can not give the required "clocks" property for sgtl5000 node.
I don't understand your example. For the sgtl5000 on the sabrelite, you would provide a phandle and cell entry that is interpreted as the cko pin. Is your concern the node itself has to change from board to board? That's normal for all bindings. If you have a module, its interrupts binding is dependent on the interrupt controller's binding (i.e. a GIC has 3 cells and VIC has 1 cell) which can vary.
Rob
Quoted 9 lines · Rob Herring
Quoted 5 lines
As I said, any clock in the clock tree except root clock is not only a clock provider but also a consumer. If you define "clocks" as a required property for clock consumers, you are essentially asking users to either define the whole clock tree in the device tree or stay away from device tree completely.
So what are you proposing that a clock consumer have? The very definition of a clock consumer is that it has a clocks property.
To support the cases that the clock tree is defined by clock driver and device tree together, the "clocks" property could be reasonably absent in case the parent clock (provider) is being defined in driver than DT. And for such clocks, the "clock-names" than "clocks" should be required to find the clock provider/parent.
I do not like the idea to look for clock with name too much, but I do not see other way around to support those platforms that have clock tree definition split in clock driver and device tree.
Quoted 20 lines
Quoted 6 lines
Are you sure this is the right thing to do? If I remember correctly, Grant's position is it should be pretty reasonable to have most of the clock tree defined in clock driver and only define those leaf clocks which are very likely to become the clock providers for other peripherals.
The minimum is you have to have a provider and consumer. It may be a single provider that provides all clocks for a chip. If you don't want a provider, then just define a clock-frequency property.
Quoted 9 lines
Let me put a terrible example here. Since clock tree is actually SoC specific, I can reasonably choose to define the entire imx6q clock tree and all the clk lookups for imx6q peripherals in clk-imx6q driver. On imx6q-sabrelite board, the audio codec sgtl5000 uses cko (an imx6q clock available on pad) as the clock source. That said, I need a board specific clk lookup here, which should be the best user of clock DT bindings. But sadly, with the current bindings, I can not give the required "clocks" property for sgtl5000 node.
Quoted 3 lines
I don't understand your example. For the sgtl5000 on the sabrelite, you would provide a phandle and cell entry that is interpreted as the cko pin.
With the bindings here, I need something like below in device tree to replace the clk lookup registration that is currently done in imx6q sabrelite specific setup code. However the problem here is I have cko defined in clock driver, and thus I can not give phandle to cko in device tree. What I suggest is for such cases, we could require clock-names = "cko" than clocks = <&cko>, and of_clk_get() should also be able to find the clock with looking for the clk name.
imx6q-sabrelite.dts:
codec: sgtl5000@0a {
compatible = "fsl,sgtl5000";
reg = <0x0a>;
clocks = <&cko>;
};mach-imx6q.c, imx6q_sabrelite_cko1_setup():
cko1 = clk_get_sys(NULL, "cko1");
clk_register_clkdev(cko1, NULL, "0-000a");Regards,
Shawn
Quoted 60 lines · Shawn Guo
Quoted 9 lines · Rob Herring
Quoted 5 lines
As I said, any clock in the clock tree except root clock is not only a clock provider but also a consumer. If you define "clocks" as a required property for clock consumers, you are essentially asking users to either define the whole clock tree in the device tree or stay away from device tree completely.
So what are you proposing that a clock consumer have? The very definition of a clock consumer is that it has a clocks property.
To support the cases that the clock tree is defined by clock driver and device tree together, the "clocks" property could be reasonably absent in case the parent clock (provider) is being defined in driver than DT. And for such clocks, the "clock-names" than "clocks" should be required to find the clock provider/parent.
I do not like the idea to look for clock with name too much, but I do not see other way around to support those platforms that have clock tree definition split in clock driver and device tree.
Quoted 20 lines
Quoted 6 lines
Are you sure this is the right thing to do? If I remember correctly, Grant's position is it should be pretty reasonable to have most of the clock tree defined in clock driver and only define those leaf clocks which are very likely to become the clock providers for other peripherals.
The minimum is you have to have a provider and consumer. It may be a single provider that provides all clocks for a chip. If you don't want a provider, then just define a clock-frequency property.
Quoted 9 lines
Let me put a terrible example here. Since clock tree is actually SoC specific, I can reasonably choose to define the entire imx6q clock tree and all the clk lookups for imx6q peripherals in clk-imx6q driver. On imx6q-sabrelite board, the audio codec sgtl5000 uses cko (an imx6q clock available on pad) as the clock source. That said, I need a board specific clk lookup here, which should be the best user of clock DT bindings. But sadly, with the current bindings, I can not give the required "clocks" property for sgtl5000 node.
Quoted 3 lines
I don't understand your example. For the sgtl5000 on the sabrelite, you would provide a phandle and cell entry that is interpreted as the cko pin.
With the bindings here, I need something like below in device tree to replace the clk lookup registration that is currently done in imx6q sabrelite specific setup code. However the problem here is I have cko defined in clock driver, and thus I can not give phandle to cko in device tree. What I suggest is for such cases, we could require clock-names = "cko" than clocks = <&cko>, and
of_clk_get()should also be able to find the clock with looking for the clk name.
imx6q-sabrelite.dts:codec: sgtl5000 at 0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; clocks = <&cko>; };
Quoted 5 lines
mach-imx6q.c,imx6q_sabrelite_cko1_setup():cko1 = clk_get_sys(NULL, "cko1"); clk_register_clkdev(cko1, NULL, "0-000a");
What!? This is a terrible abuse/hack of the clock binding and is in no way what was intended. You cannot use half of the clock binding. You have to have a provider. The primary binding is a phandle reference. clock-names is just auxiliary data.
Rob
Quoted 18 lines · Rob Herring
Quoted 18 lines
With the bindings here, I need something like below in device tree to replace the clk lookup registration that is currently done in imx6q sabrelite specific setup code. However the problem here is I have cko defined in clock driver, and thus I can not give phandle to cko in device tree. What I suggest is for such cases, we could require clock-names = "cko" than clocks = <&cko>, and
of_clk_get()should also be able to find the clock with looking for the clk name.
imx6q-sabrelite.dts:codec: sgtl5000@0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; clocks = <&cko>; };
mach-imx6q.c,imx6q_sabrelite_cko1_setup():
Quoted 9 lines
Quoted 3 lines
cko1 = clk_get_sys(NULL, "cko1"); clk_register_clkdev(cko1, NULL, "0-000a");What!? This is a terrible abuse/hack of the clock binding and is in no way what was intended. You cannot use half of the clock binding. You have to have a provider. The primary binding is a phandle reference. clock-names is just auxiliary data.
Well, on conversation [1], it seems people agreed that for those huge clock tree, only leaf clocks should be exposed in device tree. Then please help me understand how we could do that with the current binding design, considering those leaf clocks are consumers of their parent clocks while being provider to the leaf clock the parent clock is not exposed in device tree.
Regards,
Shawn
[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/139414/focus=1216423
Quoted 28 lines · Shawn Guo
Quoted 18 lines · Rob Herring
Quoted 18 lines
With the bindings here, I need something like below in device tree to replace the clk lookup registration that is currently done in imx6q sabrelite specific setup code. However the problem here is I have cko defined in clock driver, and thus I can not give phandle to cko in device tree. What I suggest is for such cases, we could require clock-names = "cko" than clocks = <&cko>, and
of_clk_get()should also be able to find the clock with looking for the clk name.
imx6q-sabrelite.dts:codec: sgtl5000 at 0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; clocks = <&cko>; };
mach-imx6q.c,imx6q_sabrelite_cko1_setup():Quoted 9 lines
Quoted 3 lines
cko1 = clk_get_sys(NULL, "cko1"); clk_register_clkdev(cko1, NULL, "0-000a");What!? This is a terrible abuse/hack of the clock binding and is in no way what was intended. You cannot use half of the clock binding. You have to have a provider. The primary binding is a phandle reference. clock-names is just auxiliary data.
Quoted 6 lines
Well, on conversation [1], it seems people agreed that for those huge clock tree, only leaf clocks should be exposed in device tree. Then please help me understand how we could do that with the current binding design, considering those leaf clocks are consumers of their parent clocks while being provider to the leaf clock the parent clock is not exposed in device tree.
You are mis-interpreting things.
As Grant states: "This proposed binding is only about one thing:
attaching clock providers to clock consumers." This means you have to have at least a single provider and a single consumer defined in the DT.
By only exposing leaf nodes of the clocks, that means only exposing a single DT node for the SOC clocks with a whole bunch of outputs (i.e. the leaf clocks). In the imx case, this would be a single node for the CCM with the dozens clocks the CCM outputs. In this case you would not expose all the individual muxes, gates, and dividers of the CCM in the DT. You still have to describe the connection between the CCM and a h/w block. I'm not sure what you want here. Based on our prior conversations, I thought you wanted to break out every single clock as a separate DT node and have generic divider, mux, and gate bindings. Now you are arguing for the opposite.
Rob
Quoted 4 lines
Regards,
Shawn[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/139414/focus=1216423
Quoted 4 lines · Rob Herring
As Grant states: "This proposed binding is only about one thing:
attaching clock providers to clock consumers." This means you have to have at least a single provider and a single consumer defined in the DT.
I just read through Grant's comments over again. I agree with the statement which implicitly requires the clk provider defined in DT. However, for some case, this provider in DT is just a skeleton which is backed by clock driver where the provider is actually defined.
Looking at Grant's comment below, the second option is also to match the clock in driver just using name. The only difference to my proposal is the name here is given by the argument of phandle pointing to that skeleton provider node.
I'm fine with that. So go ahead with your bindings.
Regards,
Shawn
Quoted 20 lines · Grant Likely
I'm really not convinced that it is a good idea to break out the entire clock tree into one node per struct clk. To begin with, that looks to be very centric around the current 'struct clk' Linux abstraction, which is potentially in flux. Also, looking at Sascha's initial RFC for describing the clock tree, I see cases where it looks like a clock nexus node really makes sense. For instance, the 'divider-ipg <at>
0x53fd4014' node which has a list of child nodes which merely provide a register offset and shift value (reg=0x53fd4068..0x53fd4084, shift=0x0..0xf). It would be natural to instead encode that as part of the clock reference, or map it directly from the clock reference (ie, assign names to each of the clocks, and let the clock provider driver match up the name to the reg offset & shift values).I had originally thought that it would be better to use names directly for references to clocks (ie. clock = <phandle>,"name") , but after actually playing with it and looking at the existing DT conventions, I've reverted to cell values for the arguments and a separate set of clock-{input,output}-name properties for attaching meaningful names, just like we decided to do for assigning names to 'reg' properties.
Quoted 10 lines · Shawn Guo
Quoted 4 lines · Rob Herring
As Grant states: "This proposed binding is only about one thing:
attaching clock providers to clock consumers." This means you have to have at least a single provider and a single consumer defined in the DT.I just read through Grant's comments over again. I agree with the statement which implicitly requires the clk provider defined in DT. However, for some case, this provider in DT is just a skeleton which is backed by clock driver where the provider is actually defined.
Quoted 7 lines
Looking at Grant's comment below, the second option is also to match the clock in driver just using name. The only difference to my proposal is the name here is given by the argument of phandle pointing to that skeleton provider node.
I'm fine with that. So go ahead with your bindings.
Can we do what the regulator framework has done and have a common binding of <connection_name>-clk = <&phandle>? Something like:
core-clk = <&uart3_clk>
and then have clk_get() use the of node of the device passed in to find a property named %s-clk and find the clock with the matching phandle.
This looks like it's trying to cover both the end consumers (uart uses uart3_clk) and the internal clock tree consumers (a crystal oscillator connects to a PLL or a mux has multiple parents). We can certainly use these bindings for muxes and internal parent-child relationships but I would prefer we use different bindings for consumer bindings that match what regulators do today.
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 19 lines · Stephen Boyd
Quoted 10 lines · Shawn Guo
Quoted 4 lines · Rob Herring
As Grant states: "This proposed binding is only about one thing:
attaching clock providers to clock consumers." This means you have to have at least a single provider and a single consumer defined in the DT.I just read through Grant's comments over again. I agree with the statement which implicitly requires the clk provider defined in DT. However, for some case, this provider in DT is just a skeleton which is backed by clock driver where the provider is actually defined.
Quoted 7 lines
Looking at Grant's comment below, the second option is also to match the clock in driver just using name. The only difference to my proposal is the name here is given by the argument of phandle pointing to that skeleton provider node.
I'm fine with that. So go ahead with your bindings.
Quoted 7 lines
Can we do what the regulator framework has done and have a common binding of <connection_name>-clk = <&phandle>? Something like:
core-clk = <&uart3_clk>and then have
clk_get()use the of node of the device passed in to find a property named %s-clk and find the clock with the matching phandle.
Sigh... That is what we had in previous versions from over a year ago and we moved away from that approach. The current binding has been reviewed multiple times in the last 6 months...
The current approach is aligned with how interrupts are handled (with the addition of a phandle). I think not having per clock property names is easier to parse and easier to document.
Quoted 6 lines
This looks like it's trying to cover both the end consumers (uart uses
uart3_clk) and the internal clock tree consumers (a crystal oscillator connects to a PLL or a mux has multiple parents). We can certainly use these bindings for muxes and internal parent-child relationships but I would prefer we use different bindings for consumer bindings that match what regulators do today.
The binding supports either defining every last internal clock or just the leaf clocks. I took the former route on highbank since I don't have a lot of clocks. If I was doing imx or omap for example, I'd probably just define all the clock controller outputs.
Rob
Quoted 44 lines · Rob Herring
Quoted 20 lines · Stephen Boyd
Quoted 11 lines · Shawn Guo
Quoted 5 lines · Rob Herring
As Grant states: "This proposed binding is only about one thing:
attaching clock providers to clock consumers." This means you have to have at least a single provider and a single consumer defined in the DT.I just read through Grant's comments over again. I agree with the statement which implicitly requires the clk provider defined in DT. However, for some case, this provider in DT is just a skeleton which is backed by clock driver where the provider is actually defined.
Quoted 7 lines
Looking at Grant's comment below, the second option is also to match the clock in driver just using name. The only difference to my proposal is the name here is given by the argument of phandle pointing to that skeleton provider node.
I'm fine with that. So go ahead with your bindings.
Quoted 7 lines
Can we do what the regulator framework has done and have a common binding of <connection_name>-clk = <&phandle>? Something like:
core-clk = <&uart3_clk>and then have
clk_get()use the of node of the device passed in to find a property named %s-clk and find the clock with the matching phandle.Sigh... That is what we had in previous versions from over a year ago and we moved away from that approach. The current binding has been reviewed multiple times in the last 6 months...
The current approach is aligned with how interrupts are handled (with the addition of a phandle). I think not having per clock property names is easier to parse and easier to document.
Quoted 6 lines
This looks like it's trying to cover both the end consumers (uart uses
uart3_clk) and the internal clock tree consumers (a crystal oscillator connects to a PLL or a mux has multiple parents). We can certainly use these bindings for muxes and internal parent-child relationships but I would prefer we use different bindings for consumer bindings that match what regulators do today.
Quoted 5 lines
The binding supports either defining every last internal clock or just the leaf clocks. I took the former route on highbank since I don't have a lot of clocks. If I was doing imx or omap for example, I'd probably just define all the clock controller outputs.
If only the leaf nodes are defined in DT, then how is the clock platform driver implementer supposed to instantiate the rest of the tree and connect it up with the partial list of clocks in DT? So, they have to switch back and forth between DT and the .c file which defines the rest and make sure the parent<->child names match?
To me it looks that it might better to decouple the description of the clock HW from the mapping of a clock leaf to a consumer device. If we just use a string to identify the clock that's consumed by a device, we can achieve this decoupling at a clean boundary -- clock consumers devices (UART) vs clock producer devices (clock controller in the SoC, in a PMIC, audio codec, etc).
With the decoupling, we don't have the inconsistency of having some of the clocks of a clock producer device incompletely defined in DT and the rest of the clocks of the same clock producer device hard coded in the kernel. So, you either put your entire clock tree in the SoC in the DT or put all of it in the kernel but you aren't forced to put just some of them in the DT just to get DT working. I see no benefit in defining only some of the clocks in DT -- it just adds more confusion in the clock tree definition. What am I missing?
Thanks,
Saravana
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 65 lines · Saravana Kannan
Quoted 44 lines · Rob Herring
Quoted 20 lines · Stephen Boyd
Quoted 11 lines · Shawn Guo
Quoted 5 lines · Rob Herring
As Grant states: "This proposed binding is only about one thing:
attaching clock providers to clock consumers." This means you have to have at least a single provider and a single consumer defined in the DT.I just read through Grant's comments over again. I agree with the statement which implicitly requires the clk provider defined in DT. However, for some case, this provider in DT is just a skeleton which is backed by clock driver where the provider is actually defined.
Quoted 7 lines
Looking at Grant's comment below, the second option is also to match the clock in driver just using name. The only difference to my proposal is the name here is given by the argument of phandle pointing to that skeleton provider node.
I'm fine with that. So go ahead with your bindings.
Quoted 7 lines
Can we do what the regulator framework has done and have a common binding of <connection_name>-clk = <&phandle>? Something like:
core-clk = <&uart3_clk>and then have
clk_get()use the of node of the device passed in to find a property named %s-clk and find the clock with the matching phandle.Sigh... That is what we had in previous versions from over a year ago and we moved away from that approach. The current binding has been reviewed multiple times in the last 6 months...
The current approach is aligned with how interrupts are handled (with the addition of a phandle). I think not having per clock property names is easier to parse and easier to document.
Quoted 6 lines
This looks like it's trying to cover both the end consumers (uart uses
uart3_clk) and the internal clock tree consumers (a crystal oscillator connects to a PLL or a mux has multiple parents). We can certainly use these bindings for muxes and internal parent-child relationships but I would prefer we use different bindings for consumer bindings that match what regulators do today.Quoted 5 lines
The binding supports either defining every last internal clock or just the leaf clocks. I took the former route on highbank since I don't have a lot of clocks. If I was doing imx or omap for example, I'd probably just define all the clock controller outputs.
If only the leaf nodes are defined in DT, then how is the clock platform driver implementer supposed to instantiate the rest of the tree and connect it up with the partial list of clocks in DT? So, they have to switch back and forth between DT and the .c file which defines the rest and make sure the parent<->child names match?
To me it looks that it might better to decouple the description of the clock HW from the mapping of a clock leaf to a consumer device. If we just use a string to identify the clock that's consumed by a device, we can achieve this decoupling at a clean boundary -- clock consumers devices (UART) vs clock producer devices (clock controller in the SoC, in a PMIC, audio codec, etc).
Quoted 8 lines
With the decoupling, we don't have the inconsistency of having some of the clocks of a clock producer device incompletely defined in DT and the rest of the clocks of the same clock producer device hard coded in the kernel. So, you either put your entire clock tree in the SoC in the DT or put all of it in the kernel but you aren't forced to put just some of them in the DT just to get DT working. I see no benefit in defining only some of the clocks in DT -- it just adds more confusion in the clock tree definition. What am I missing?
I fail to see what would need changing in the binding itself. The binding just describes connections. Whether that is a connection to a clock controller node to a device or a clock gate/mux/divider node to a device is really beyond the clock binding. This is really just policy. You are free to put no clocks in DT, all clocks, or a nexus of clocks.
Rob
Quoted 75 lines · Rob Herring
Quoted 65 lines · Saravana Kannan
Quoted 44 lines · Rob Herring
Quoted 20 lines · Stephen Boyd
Quoted 11 lines · Shawn Guo
Quoted 5 lines · Rob Herring
As Grant states: "This proposed binding is only about one thing:
attaching clock providers to clock consumers." This means you have to have at least a single provider and a single consumer defined in the DT.I just read through Grant's comments over again. I agree with the statement which implicitly requires the clk provider defined in DT. However, for some case, this provider in DT is just a skeleton which is backed by clock driver where the provider is actually defined.
Quoted 7 lines
Looking at Grant's comment below, the second option is also to match the clock in driver just using name. The only difference to my proposal is the name here is given by the argument of phandle pointing to that skeleton provider node.
I'm fine with that. So go ahead with your bindings.
Quoted 7 lines
Can we do what the regulator framework has done and have a common binding of<connection_name>-clk =<&phandle>? Something like:
core-clk =<&uart3_clk>and then have
clk_get()use the of node of the device passed in to find a property named %s-clk and find the clock with the matching phandle.Sigh... That is what we had in previous versions from over a year ago and we moved away from that approach. The current binding has been reviewed multiple times in the last 6 months...
The current approach is aligned with how interrupts are handled (with the addition of a phandle). I think not having per clock property names is easier to parse and easier to document.
Quoted 6 lines
This looks like it's trying to cover both the end consumers (uart uses
uart3_clk) and the internal clock tree consumers (a crystal oscillator connects to a PLL or a mux has multiple parents). We can certainly use these bindings for muxes and internal parent-child relationships but I would prefer we use different bindings for consumer bindings that match what regulators do today.Quoted 5 lines
The binding supports either defining every last internal clock or just the leaf clocks. I took the former route on highbank since I don't have a lot of clocks. If I was doing imx or omap for example, I'd probably just define all the clock controller outputs.
If only the leaf nodes are defined in DT, then how is the clock platform driver implementer supposed to instantiate the rest of the tree and connect it up with the partial list of clocks in DT? So, they have to switch back and forth between DT and the .c file which defines the rest and make sure the parent<->child names match?
To me it looks that it might better to decouple the description of the clock HW from the mapping of a clock leaf to a consumer device. If we just use a string to identify the clock that's consumed by a device, we can achieve this decoupling at a clean boundary -- clock consumers devices (UART) vs clock producer devices (clock controller in the SoC, in a PMIC, audio codec, etc).
Quoted 8 lines
With the decoupling, we don't have the inconsistency of having some of the clocks of a clock producer device incompletely defined in DT and the rest of the clocks of the same clock producer device hard coded in the kernel. So, you either put your entire clock tree in the SoC in the DT or put all of it in the kernel but you aren't forced to put just some of them in the DT just to get DT working. I see no benefit in defining only some of the clocks in DT -- it just adds more confusion in the clock tree definition. What am I missing?
Quoted 5 lines
I fail to see what would need changing in the binding itself. The binding just describes connections. Whether that is a connection to a clock controller node to a device or a clock gate/mux/divider node to a device is really beyond the clock binding. This is really just policy. You are free to put no clocks in DT, all clocks, or a nexus of clocks.
With the current approach you are taking can you please give an example of how a random device described in DT would hook itself up with a leaf clock if that leaf clock is not described in DT? So that it can do a call a DT version of clk_get() to get the clock it cares for.
And no, there is a huge difference between binding a clock controller node (by which I mean the block that provides many clocks) to a device vs. binding a clock leaf to a device. The former is useless wrt to clk_get() and similar functions. The latter is very useful to handle that.
Thanks,
Saravana
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 2 lines · Saravana Kannan
Quoted 1 line · Rob Herring
On 05/22/2012 08:38 PM, Saravana Kannan wrote:
snip
Quoted 36 lines
Quoted 30 lines
Quoted 15 lines
If only the leaf nodes are defined in DT, then how is the clock platform driver implementer supposed to instantiate the rest of the tree and connect it up with the partial list of clocks in DT? So, they have to switch back and forth between DT and the .c file which defines the rest and make sure the parent<->child names match?
To me it looks that it might better to decouple the description of the clock HW from the mapping of a clock leaf to a consumer device. If we just
use a string to identify the clock that's consumed by a device, we can achieve this decoupling at a clean boundary -- clock consumers devices (UART) vs clock producer devices (clock controller in the SoC, in a PMIC,
audio codec, etc).Quoted 14 lines
With the decoupling, we don't have the inconsistency of having some of the
clocks of a clock producer device incompletely defined in DT and the rest
of the clocks of the same clock producer device hard coded in the kernel.
So, you either put your entire clock tree in the SoC in the DT or put all
of it in the kernel but you aren't forced to put just some of them in the
DT just to get DT working. I see no benefit in defining only some of the clocks in DT -- it just adds more confusion in the clock tree definition.
What am I missing?Quoted 5 lines
I fail to see what would need changing in the binding itself. The binding just describes connections. Whether that is a connection to a clock controller node to a device or a clock gate/mux/divider node to a device is really beyond the clock binding. This is really just policy. You are free to put no clocks in DT, all clocks, or a nexus of clocks.
Quoted 4 lines
With the current approach you are taking can you please give an example of how a random device described in DT would hook itself up with a leaf clock if that leaf clock is not described in DT? So that it can do a call a DT version of
clk_get()to get the clock it cares for.
No, because that's impossible with any binding. The only way that would work is provide a string with a clock name and matching to the struct clk name string. That means putting linux clock names into the h/w description. That is the wrong direction and not how bindings work. Defining bindings should not get tangled up with how the OS implementation is done.
Quoted 4 lines
And no, there is a huge difference between binding a clock controller node (by which I mean the block that provides many clocks) to a device vs. binding a clock leaf to a device. The former is useless wrt to
clk_get()and similar functions. The latter is very useful to handle that.
The binding and clkdev changes support clk_get fully. Drivers don't have to change at all. There is not a DT version of clk_get that all drivers have to adopt. It's all handled within clk_get and should be transparent to drivers. The only thing that has to change is callers of clk_get_sys to use of_clk_get, but that's a small fraction of clocks.
Rob
Quoted 42 lines · Rob Herring
Quoted 2 lines · Saravana Kannan
Quoted 1 line · Rob Herring
On 05/22/2012 08:38 PM, Saravana Kannan wrote:
snip
Quoted 36 lines
Quoted 35 lines
Quoted 15 lines
If only the leaf nodes are defined in DT, then how is the clock platform driver implementer supposed to instantiate the rest of the tree and connect it up with the partial list of clocks in DT? So, they have to switch back and forth between DT and the .c file which defines the rest and make sure the parent<->child names match?
To me it looks that it might better to decouple the description of the clock HW from the mapping of a clock leaf to a consumer device. If we just
use a string to identify the clock that's consumed by a device, we can achieve this decoupling at a clean boundary -- clock consumers devices (UART) vs clock producer devices (clock controller in the SoC, in a PMIC,
audio codec, etc).Quoted 14 lines
With the decoupling, we don't have the inconsistency of having some of the
clocks of a clock producer device incompletely defined in DT and the rest
of the clocks of the same clock producer device hard coded in the kernel.
So, you either put your entire clock tree in the SoC in the DT or put all
of it in the kernel but you aren't forced to put just some of them in the
DT just to get DT working. I see no benefit in defining only some of the clocks in DT -- it just adds more confusion in the clock tree definition.
What am I missing?I fail to see what would need changing in the binding itself. The binding just describes connections. Whether that is a connection to a clock controller node to a device or a clock gate/mux/divider node to a device is really beyond the clock binding. This is really just policy. You are free to put no clocks in DT, all clocks, or a nexus of clocks.
Quoted 6 lines
Quoted 4 lines
With the current approach you are taking can you please give an example of how a random device described in DT would hook itself up with a leaf clock if that leaf clock is not described in DT? So that it can do a call a DT version of
clk_get()to get the clock it cares for.No, because that's impossible with any binding.
So, this is really forcing everyone moving rest of their devices to DT to put part/all of the clocks in DT. Either that or deal with the "aux data" that's supposed to be temporary.
Quoted 4 lines
The only way that would
work is provide a string with a clock name and matching to the struct clk name string. That means putting linux clock names into the h/w description.
No, the name of the clocks stored in clk->name should be the name of the clock in HW.
Quoted 3 lines
That is the wrong direction and not how bindings work. Defining bindings should not get tangled up with how the OS implementation is done.
So, I'm not asking to bind to the name of a clock as defined by the OS implementation. I'm just asking to bind to the name of the clock in HW instead of the name binding to an actual DT clock node. And I'm only asking this because we seem to want to give an option to NOT have the clocks in DT.
A developer might choose to abbreviate the clock name in DT and in clk->name field in the kernel, but there's nothing wrong with it. Nor can DT do anything about it for ANY (not just clock) DT device descriptions.
Quoted 10 lines
Quoted 4 lines
And no, there is a huge difference between binding a clock controller node (by which I mean the block that provides many clocks) to a device vs. binding a clock leaf to a device. The former is useless wrt to
clk_get()and similar functions. The latter is very useful to handle that.The binding and clkdev changes support
clk_getfully. Drivers don't have to change at all. There is not a DT version ofclk_getthat all drivers have to adopt. It's all handled withinclk_getand should be transparent to drivers. The only thing that has to change is callers ofclk_get_systo useof_clk_get, but that's a small fraction of clocks.
I understand there are changes to have clk_get() work on clocks added through DT. But, you said earlier that it doesn't matter if a general DT device binds to a clock controller DT device or to an actual clock node (say, a leaf in the clock tree) defined in DT. I'm just pointing out that it's not true. If a general DT device binds to a clock controller DT device, there is no way to figure out what clock node/leaf from the clock controller is actually consumed by this general device.
-Saravana
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 51 lines · Saravana Kannan
Quoted 50 lines · Rob Herring
Quoted 2 lines · Saravana Kannan
Quoted 1 line · Rob Herring
On 05/22/2012 08:38 PM, Saravana Kannan wrote:
snip
Quoted 43 lines
Quoted 33 lines
Quoted 17 lines
If only the leaf nodes are defined in DT, then how is the clock platform
driver implementer supposed to instantiate the rest of the tree and connect it up with the partial list of clocks in DT? So, they have to switch back and forth between DT and the .c file which defines the rest
and make sure the parent<->child names match?To me it looks that it might better to decouple the description of the clock HW from the mapping of a clock leaf to a consumer device. If we just
use a string to identify the clock that's consumed by a device, we can achieve this decoupling at a clean boundary -- clock consumers devices (UART) vs clock producer devices (clock controller in the SoC, in a PMIC,
audio codec, etc).Quoted 15 lines
With the decoupling, we don't have the inconsistency of having some of the
clocks of a clock producer device incompletely defined in DT and the rest
of the clocks of the same clock producer device hard coded in the kernel.
So, you either put your entire clock tree in the SoC in the DT or put all
of it in the kernel but you aren't forced to put just some of them in the
DT just to get DT working. I see no benefit in defining only some of the
clocks in DT -- it just adds more confusion in the clock tree definition.
What am I missing?Quoted 5 lines
I fail to see what would need changing in the binding itself. The binding just describes connections. Whether that is a connection to a clock controller node to a device or a clock gate/mux/divider node to a device is really beyond the clock binding. This is really just policy. You are free to put no clocks in DT, all clocks, or a nexus of clocks.
With the current approach you are taking can you please give an example of how a random device described in DT would hook itself up with a leaf clock if that leaf clock is not described in DT? So that it can do a call a DT version of
clk_get()to get the clock it cares for.
Quoted 6 lines
Quoted 1 line
No, because that's impossible with any binding.
So, this is really forcing everyone moving rest of their devices to DT to put part/all of the clocks in DT. Either that or deal with the "aux data" that's supposed to be temporary.
No. You can still use clkdev lookups if clock lookup is the only thing you need to hook up. In other words, I would only use aux_data if you need to set platform_data. If you only need clk lookups, then you can use clkdev. This is what most of the DT conversions I've reviewed have done.
Quoted 8 lines
Quoted 4 lines
The only way that would
work is provide a string with a clock name and matching to the struct clk name string. That means putting linux clock names into the h/w description.No, the name of the clocks stored in
clk->nameshould be the name of the clock in HW.
True, but it had been debated whether a name was needed for anything but debug. We're trying to get data like this out of the kernel with DT. For example, say you have a new derivative SOC that adds 1 additional PLL. Adding support for that PLL could be as simple as adding it to DT and the kernel will instantiate it. If you are matching with strings, then you have to add the new PLL name string to the kernel.
Quoted 27 lines
Quoted 3 lines
That is the wrong direction and not how bindings work. Defining bindings should not get tangled up with how the OS implementation is done.
So, I'm not asking to bind to the name of a clock as defined by the OS implementation. I'm just asking to bind to the name of the clock in HW instead of the name binding to an actual DT clock node. And I'm only asking this because we seem to want to give an option to NOT have the clocks in DT.
A developer might choose to abbreviate the clock name in DT and in
clk->namefield in the kernel, but there's nothing wrong with it. Nor can DT do anything about it for ANY (not just clock) DT device descriptions.Quoted 11 lines
Quoted 5 lines
And no, there is a huge difference between binding a clock controller node (by which I mean the block that provides many clocks) to a device vs. binding a clock leaf to a device. The former is useless wrt to
clk_get()and similar functions. The latter is very useful to handle that.The binding and clkdev changes support
clk_getfully. Drivers don't have to change at all. There is not a DT version ofclk_getthat all drivers have to adopt. It's all handled withinclk_getand should be transparent to drivers. The only thing that has to change is callers ofclk_get_systo useof_clk_get, but that's a small fraction of clocks.
Quoted 7 lines
I understand there are changes to have
clk_get()work on clocks added through DT. But, you said earlier that it doesn't matter if a general DT device binds to a clock controller DT device or to an actual clock node (say, a leaf in the clock tree) defined in DT. I'm just pointing out that it's not true. If a general DT device binds to a clock controller DT device, there is no way to figure out what clock node/leaf from the clock controller is actually consumed by this general device.
of_clk_get_parent_name (or some variation that gives you parent phandle too) doesn't do what you want?
The fact that a driver needs to know details about possible parents is a problem independent of DT. We do need a better way to abstract that.
Rob
Quoted 3 lines
-Saravana