| Message ID | 4FB80F32.5090309@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, May 19, 2012 at 04:22:58PM -0500, Rob Herring wrote: > 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.
On 05/19/2012 10:06 PM, Shawn Guo wrote: > On Sat, May 19, 2012 at 04:22:58PM -0500, Rob Herring wrote: >> 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. So what are you proposing that a clock consumer have? The very definition of a clock consumer is that it has a clocks property. > > 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. > > 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
On Sun, May 20, 2012 at 09:18:15PM -0500, Rob Herring wrote: > > 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. > > > > 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. > > > > 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. 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
On 05/21/2012 01:49 AM, Shawn Guo wrote: > On Sun, May 20, 2012 at 09:18:15PM -0500, Rob Herring wrote: >>> 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. > >>> >>> 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. >>> >>> 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. > > 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"); > 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
On Mon, May 21, 2012 at 01:30:59PM -0500, Rob Herring wrote: > > 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"); > > > > 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
On 05/21/2012 06:26 PM, Shawn Guo wrote: > On Mon, May 21, 2012 at 01:30:59PM -0500, Rob Herring wrote: >>> 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"); >>> >> >> 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. 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 > Regards, > Shawn > > [1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/139414/focus=1216423
On Mon, May 21, 2012 at 06:52:37PM -0500, Rob Herring wrote: > 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 On Wed, Nov 16, 2011 at 03:12:50PM -0700, Grant Likely wrote: > 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.
On 05/21/12 19:15, Shawn Guo wrote: > On Mon, May 21, 2012 at 06:52:37PM -0500, Rob Herring wrote: >> 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. > 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.
On 05/21/2012 11:17 PM, Stephen Boyd wrote: > On 05/21/12 19:15, Shawn Guo wrote: >> On Mon, May 21, 2012 at 06:52:37PM -0500, Rob Herring wrote: >>> 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. >> > > 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. > 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
On Tue, May 22, 2012 6:52 am, Rob Herring wrote: > On 05/21/2012 11:17 PM, Stephen Boyd wrote: >> On 05/21/12 19:15, Shawn Guo wrote: >>> On Mon, May 21, 2012 at 06:52:37PM -0500, Rob Herring wrote: >>>> 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. >>> >> >> 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. > >> 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. > 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
On 05/22/2012 08:38 PM, Saravana Kannan wrote: > > On Tue, May 22, 2012 6:52 am, Rob Herring wrote: >> On 05/21/2012 11:17 PM, Stephen Boyd wrote: >>> On 05/21/12 19:15, Shawn Guo wrote: >>>> On Mon, May 21, 2012 at 06:52:37PM -0500, Rob Herring wrote: >>>>> 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. >>>> >>> >>> 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. >> >>> 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. >> > > 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? 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
On 05/23/2012 06:59 AM, Rob Herring wrote: > On 05/22/2012 08:38 PM, Saravana Kannan wrote: >> >> On Tue, May 22, 2012 6:52 am, Rob Herring wrote: >>> On 05/21/2012 11:17 PM, Stephen Boyd wrote: >>>> On 05/21/12 19:15, Shawn Guo wrote: >>>>> On Mon, May 21, 2012 at 06:52:37PM -0500, Rob Herring wrote: >>>>>> 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. >>>>> >>>> >>>> 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. >>> >>>> 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. >>> >> >> 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? > > 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
On 05/24/2012 04:16 PM, Saravana Kannan wrote: > On 05/23/2012 06:59 AM, Rob Herring wrote: >> On 05/22/2012 08:38 PM, Saravana Kannan wrote: snip >>> 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? >> >> 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. 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. > 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
On 05/24/2012 02:54 PM, Rob Herring wrote: > On 05/24/2012 04:16 PM, Saravana Kannan wrote: >> On 05/23/2012 06:59 AM, Rob Herring wrote: >>> On 05/22/2012 08:38 PM, Saravana Kannan wrote: > > snip > >>>> 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? >>> >>> 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. > > 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. > 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. > 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. >> 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. 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
On 05/24/2012 10:33 PM, Saravana Kannan wrote: > On 05/24/2012 02:54 PM, Rob Herring wrote: >> On 05/24/2012 04:16 PM, Saravana Kannan wrote: >>> On 05/23/2012 06:59 AM, Rob Herring wrote: >>>> On 05/22/2012 08:38 PM, Saravana Kannan wrote: >> >> snip >> >>>>> 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? >>>> >>>> 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. >> >> 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. >> 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. > 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. >> 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. > >>> 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. > > 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 > > -Saravana >
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