| Message ID | 5481F79D.4010504@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Stephen Boyd <sboyd@codeaurora.org> [141205 10:23]: > On 12/05/2014 08:55 AM, Tony Lindgren wrote: > > Hi, > > > > Looks like commit 646cafc6aa4d ("clk: Change clk_ops->determine_rate > > to return a clk_hw as the best parent") breaks booting at least for > > omap4. > > Do you get a compilation warning in arch/arm/mach-omap2/dpll3xxx.c ? Yes so it seems. > From what I can tell omap3_noncore_dpll_determine_rate() hasn't been > updated to take a clk_hw pointer instead of clk pointer. It was there in > the original patch and I'm not sure why Mike dropped that part while > applying. OK that makes sense, Mike should apply that part too. Note that also include/linux/clk/ti.h needs changed accordingly for struct clk_hw, which you probably had in your orignal patch too. Assuming that's there, please feel free to add: Acked-by: Tony Lindgren <tony@atomide.com> > diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c > index 20e120d..c2da2a0 100644 > --- a/arch/arm/mach-omap2/dpll3xxx.c > +++ b/arch/arm/mach-omap2/dpll3xxx.c > @@ -474,7 +474,7 @@ void omap3_noncore_dpll_disable(struct clk_hw *hw) > */ > long omap3_noncore_dpll_determine_rate(struct clk_hw *hw, unsigned long rate, > unsigned long *best_parent_rate, > - struct clk **best_parent_clk) > + struct clk_hw **best_parent_clk) > { > struct clk_hw_omap *clk = to_clk_hw_omap(hw); > struct dpll_data *dd; > @@ -488,10 +488,10 @@ long omap3_noncore_dpll_determine_rate(struct clk_hw *hw, unsigned long rate, > > if (__clk_get_rate(dd->clk_bypass) == rate && > (dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) { > - *best_parent_clk = dd->clk_bypass; > + *best_parent_clk = __clk_get_hw(dd->clk_bypass); > } else { > rate = omap2_dpll_round_rate(hw, rate, best_parent_rate); > - *best_parent_clk = dd->clk_ref; > + *best_parent_clk = __clk_get_hw(dd->clk_ref); > } > > *best_parent_rate = rate; > diff --git a/arch/arm/mach-omap2/dpll44xx.c b/arch/arm/mach-omap2/dpll44xx.c > index 535822f..0e58e5a 100644 > --- a/arch/arm/mach-omap2/dpll44xx.c > +++ b/arch/arm/mach-omap2/dpll44xx.c > @@ -223,7 +223,7 @@ out: > */ > long omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw, unsigned long rate, > unsigned long *best_parent_rate, > - struct clk **best_parent_clk) > + struct clk_hw **best_parent_clk) > { > struct clk_hw_omap *clk = to_clk_hw_omap(hw); > struct dpll_data *dd; > @@ -237,11 +237,11 @@ long omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw, unsigned long rate, > > if (__clk_get_rate(dd->clk_bypass) == rate && > (dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) { > - *best_parent_clk = dd->clk_bypass; > + *best_parent_clk = __clk_get_hw(dd->clk_bypass); > } else { > rate = omap4_dpll_regm4xen_round_rate(hw, rate, > best_parent_rate); > - *best_parent_clk = dd->clk_ref; > + *best_parent_clk = __clk_get_hw(dd->clk_ref); > } > > *best_parent_rate = rate; > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project >
On 12/15/2014 03:02 PM, Mike Turquette wrote: > Quoting Paul Walmsley (2014-12-12 15:28:32) >> On Fri, 12 Dec 2014, Mike Turquette wrote: >> >>> Quoting Tony Lindgren (2014-12-05 10:38:49) >>>> * Stephen Boyd <sboyd@codeaurora.org> [141205 10:23]: >>>>> On 12/05/2014 08:55 AM, Tony Lindgren wrote: >>>>>> Hi, >>>>>> >>>>>> Looks like commit 646cafc6aa4d ("clk: Change clk_ops->determine_rate >>>>>> to return a clk_hw as the best parent") breaks booting at least for >>>>>> omap4. >>>>> Do you get a compilation warning in arch/arm/mach-omap2/dpll3xxx.c ? >>>> Yes so it seems. >>>> >>>>> From what I can tell omap3_noncore_dpll_determine_rate() hasn't been >>>>> updated to take a clk_hw pointer instead of clk pointer. It was there in >>>>> the original patch and I'm not sure why Mike dropped that part while >>>>> applying. >>>> OK that makes sense, Mike should apply that part too. Note that also >>>> include/linux/clk/ti.h needs changed accordingly for struct clk_hw, >>>> which you probably had in your orignal patch too. Assuming that's there, >>>> please feel free to add: >>>> >>>> Acked-by: Tony Lindgren <tony@atomide.com> >>> I figured out what went wrong here. When I applied Tomeu's changes the >>> OMAP stuff did not apply at all. In fact the .determine_rate callbacks >>> did not exist in clk-next. Paul merged that stuff through his tree[0]. I >>> don't know why I didn't follow up on that at the time. >>> >>> So we need to come up with a solution. Paul can take the OMAP portion of >>> Tomeu's changes[1] for OMAP, but I believe compilation will be broken in >>> his tree until it meets up with mine in linux-next. Or we could set up a >>> shared immutable branch that provides the needed changes. >>> >>> I could either set up a shared branch that includes Tomeu's changes that >>> Paul could merge (will require a rebase of the tip of my tree) or Paul >>> could provide a shared branch of the changes to dpll3xxx.c and >>> dpll4xxx.c that I could merge in. >>> >>> Or I could remove Tomeu's patches from my tree since we're right up >>> against the merge window but I would rather not do that since he has >>> worked tirelessly on this topic. >>> >>> [0] http://www.spinics.net/lists/arm-kernel/msg377288.html >>> [1] https://lkml.org/lkml/2014/12/2/70 >> I don't think there's much that I can do at this point. My tree is quite >> topologically distant from Linus's tree (pjw/omap-pending -> >> tmlind/linux-omap -> arm/arm-soc -> torvalds/linux). So anything I do is >> high-latency. My pull request for Tero's patches was sent to Tony a month >> ago. > Paul, > > I identified the patch in your tree that is missing in mine and, with > Tony's help, applied your for-v3.19/omap-a signed tag to my tree. With > these 5 patches in place I have applied Tero's two patches from > Friday[0]. > > Paul & Tony, are you OK for me to take both of Tero's patches? I'm > already taking stuff in late so it is no trouble for me to pick up "ARM: > OMAP3: clock: fix boot breakage in legacy mode" while I'm at it. > > I'm going to let this get at least one cycle in linux-next before > sending my PR late this week. Hopefully Kevin (Cc'd) can check on the > omap boards in his autobuilder once my tree hits -next? > > Let me know if I missed anything. Thanks for the great teamwork, gang. > > [0] http://lkml.kernel.org/r/<1418390521-7541-1-git-send-email-t-kristo@ti.com> > > Regards, > Mike I think Tony's taking the second patch from Tero. - Paul
* Paul Walmsley <pwalmsley@nvidia.com> [141215 16:23]: > On 12/15/2014 03:02 PM, Mike Turquette wrote: > >Quoting Paul Walmsley (2014-12-12 15:28:32) > >>On Fri, 12 Dec 2014, Mike Turquette wrote: > >> > >>>Quoting Tony Lindgren (2014-12-05 10:38:49) > >>>>* Stephen Boyd <sboyd@codeaurora.org> [141205 10:23]: > >>>>>On 12/05/2014 08:55 AM, Tony Lindgren wrote: > >>>>>>Hi, > >>>>>> > >>>>>>Looks like commit 646cafc6aa4d ("clk: Change clk_ops->determine_rate > >>>>>>to return a clk_hw as the best parent") breaks booting at least for > >>>>>>omap4. > >>>>>Do you get a compilation warning in arch/arm/mach-omap2/dpll3xxx.c ? > >>>>Yes so it seems. > >>>> > >>>>> From what I can tell omap3_noncore_dpll_determine_rate() hasn't been > >>>>>updated to take a clk_hw pointer instead of clk pointer. It was there in > >>>>>the original patch and I'm not sure why Mike dropped that part while > >>>>>applying. > >>>>OK that makes sense, Mike should apply that part too. Note that also > >>>>include/linux/clk/ti.h needs changed accordingly for struct clk_hw, > >>>>which you probably had in your orignal patch too. Assuming that's there, > >>>>please feel free to add: > >>>> > >>>>Acked-by: Tony Lindgren <tony@atomide.com> > >>>I figured out what went wrong here. When I applied Tomeu's changes the > >>>OMAP stuff did not apply at all. In fact the .determine_rate callbacks > >>>did not exist in clk-next. Paul merged that stuff through his tree[0]. I > >>>don't know why I didn't follow up on that at the time. > >>> > >>>So we need to come up with a solution. Paul can take the OMAP portion of > >>>Tomeu's changes[1] for OMAP, but I believe compilation will be broken in > >>>his tree until it meets up with mine in linux-next. Or we could set up a > >>>shared immutable branch that provides the needed changes. > >>> > >>>I could either set up a shared branch that includes Tomeu's changes that > >>>Paul could merge (will require a rebase of the tip of my tree) or Paul > >>>could provide a shared branch of the changes to dpll3xxx.c and > >>>dpll4xxx.c that I could merge in. > >>> > >>>Or I could remove Tomeu's patches from my tree since we're right up > >>>against the merge window but I would rather not do that since he has > >>>worked tirelessly on this topic. > >>> > >>>[0] http://www.spinics.net/lists/arm-kernel/msg377288.html > >>>[1] https://lkml.org/lkml/2014/12/2/70 > >>I don't think there's much that I can do at this point. My tree is quite > >>topologically distant from Linus's tree (pjw/omap-pending -> > >>tmlind/linux-omap -> arm/arm-soc -> torvalds/linux). So anything I do is > >>high-latency. My pull request for Tero's patches was sent to Tony a month > >>ago. > >Paul, > > > >I identified the patch in your tree that is missing in mine and, with > >Tony's help, applied your for-v3.19/omap-a signed tag to my tree. With > >these 5 patches in place I have applied Tero's two patches from > >Friday[0]. > > > >Paul & Tony, are you OK for me to take both of Tero's patches? I'm > >already taking stuff in late so it is no trouble for me to pick up "ARM: > >OMAP3: clock: fix boot breakage in legacy mode" while I'm at it. > > > >I'm going to let this get at least one cycle in linux-next before > >sending my PR late this week. Hopefully Kevin (Cc'd) can check on the > >omap boards in his autobuilder once my tree hits -next? > > > >Let me know if I missed anything. Thanks for the great teamwork, gang. > > > >[0] http://lkml.kernel.org/r/<1418390521-7541-1-git-send-email-t-kristo@ti.com> > > > >Regards, > >Mike > > I think Tony's taking the second patch from Tero. If it applies to what Mike has now queued, please take that too Mike: Acked-by: Tony Lindgren <tony@atomide.com>
On 12/15/2014 05:38 PM, Tony Lindgren wrote: > * Paul Walmsley <pwalmsley@nvidia.com> [141215 16:23]: >> On 12/15/2014 03:02 PM, Mike Turquette wrote: >>> Quoting Paul Walmsley (2014-12-12 15:28:32) >>>> On Fri, 12 Dec 2014, Mike Turquette wrote: >>>> >>>>> Quoting Tony Lindgren (2014-12-05 10:38:49) >>>>>> * Stephen Boyd <sboyd@codeaurora.org> [141205 10:23]: >>>>>>> On 12/05/2014 08:55 AM, Tony Lindgren wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> Looks like commit 646cafc6aa4d ("clk: Change clk_ops->determine_rate >>>>>>>> to return a clk_hw as the best parent") breaks booting at least for >>>>>>>> omap4. >>>>>>> Do you get a compilation warning in arch/arm/mach-omap2/dpll3xxx.c ? >>>>>> Yes so it seems. >>>>>> >>>>>>> From what I can tell omap3_noncore_dpll_determine_rate() hasn't been >>>>>>> updated to take a clk_hw pointer instead of clk pointer. It was there in >>>>>>> the original patch and I'm not sure why Mike dropped that part while >>>>>>> applying. >>>>>> OK that makes sense, Mike should apply that part too. Note that also >>>>>> include/linux/clk/ti.h needs changed accordingly for struct clk_hw, >>>>>> which you probably had in your orignal patch too. Assuming that's there, >>>>>> please feel free to add: >>>>>> >>>>>> Acked-by: Tony Lindgren <tony@atomide.com> >>>>> I figured out what went wrong here. When I applied Tomeu's changes the >>>>> OMAP stuff did not apply at all. In fact the .determine_rate callbacks >>>>> did not exist in clk-next. Paul merged that stuff through his tree[0]. I >>>>> don't know why I didn't follow up on that at the time. >>>>> >>>>> So we need to come up with a solution. Paul can take the OMAP portion of >>>>> Tomeu's changes[1] for OMAP, but I believe compilation will be broken in >>>>> his tree until it meets up with mine in linux-next. Or we could set up a >>>>> shared immutable branch that provides the needed changes. >>>>> >>>>> I could either set up a shared branch that includes Tomeu's changes that >>>>> Paul could merge (will require a rebase of the tip of my tree) or Paul >>>>> could provide a shared branch of the changes to dpll3xxx.c and >>>>> dpll4xxx.c that I could merge in. >>>>> >>>>> Or I could remove Tomeu's patches from my tree since we're right up >>>>> against the merge window but I would rather not do that since he has >>>>> worked tirelessly on this topic. >>>>> >>>>> [0] http://www.spinics.net/lists/arm-kernel/msg377288.html >>>>> [1] https://lkml.org/lkml/2014/12/2/70 >>>> I don't think there's much that I can do at this point. My tree is quite >>>> topologically distant from Linus's tree (pjw/omap-pending -> >>>> tmlind/linux-omap -> arm/arm-soc -> torvalds/linux). So anything I do is >>>> high-latency. My pull request for Tero's patches was sent to Tony a month >>>> ago. >>> Paul, >>> >>> I identified the patch in your tree that is missing in mine and, with >>> Tony's help, applied your for-v3.19/omap-a signed tag to my tree. With >>> these 5 patches in place I have applied Tero's two patches from >>> Friday[0]. >>> >>> Paul & Tony, are you OK for me to take both of Tero's patches? I'm >>> already taking stuff in late so it is no trouble for me to pick up "ARM: >>> OMAP3: clock: fix boot breakage in legacy mode" while I'm at it. >>> >>> I'm going to let this get at least one cycle in linux-next before >>> sending my PR late this week. Hopefully Kevin (Cc'd) can check on the >>> omap boards in his autobuilder once my tree hits -next? >>> >>> Let me know if I missed anything. Thanks for the great teamwork, gang. >>> >>> [0] http://lkml.kernel.org/r/<1418390521-7541-1-git-send-email-t-kristo@ti.com> >>> >>> Regards, >>> Mike >> I think Tony's taking the second patch from Tero. > If it applies to what Mike has now queued, please take that too Mike: > > Acked-by: Tony Lindgren <tony@atomide.com> I just took a quick glance at Tero's second patch, and it looks like a hack to me. Better to fix the problem in the core CCF code if possible. I don't think there's any reason why a PLL couldn't have just one parent clock. But I'm fine with merging it as a short-term fix if fixing the core code is difficult or risky. - Paul
On 12/15/2014 05:31 PM, Paul Walmsley wrote: > > I just took a quick glance at Tero's second patch, and it looks like a > hack to me. Better to fix the problem in the core CCF code if > possible. I don't think there's any reason why a PLL couldn't have > just one parent clock. But I'm fine with merging it as a short-term > fix if fixing the core code is difficult or risky. Can you describe what's wrong? Does the PLL have a mux with two inputs that map to the same clock?
+ Tero On 12/16/2014 12:01 PM, Stephen Boyd wrote: > On 12/15/2014 05:31 PM, Paul Walmsley wrote: >> I just took a quick glance at Tero's second patch, and it looks like a >> hack to me. Better to fix the problem in the core CCF code if >> possible. I don't think there's any reason why a PLL couldn't have >> just one parent clock. But I'm fine with merging it as a short-term >> fix if fixing the core code is difficult or risky. > Can you describe what's wrong? Took a closer look at it, at your prompting. The first observation is that the issue seems to be limited to TI-specific clock code in drivers/clk/ti. So nothing to worry about in terms of the CCF core, it looks. The second observation is that this appears to only be a problem due to the current DT data in arch/arm/boot/dts/omap3xxx-clocks.dtsi: dpll3_ck: dpll3_ck { #clock-cells = <0>; compatible = "ti,omap3-dpll-core-clock"; clocks = <&sys_ck>, <&sys_ck>; reg = <0x0d00>, <0x0d20>, <0x0d40>, <0x0d30>; }; It lists two entries for the "clocks" node for some reason. of_ti_dpll_setup() only seems to require one. So unless there's some good reason to list two of them in the DTS file, the right fix would probably have been to fix the DT data. > Does the PLL have a mux with two inputs that map to the same clock? > I don't think so. ... Generally speaking, I suspect the way we model PLLs isn't quite right (although it's been a while since I've looked at that particular code). This was probably my fault, in the final analysis, for taking a shortcut here a long time ago. Unless one wishes to implement support for multiple clock parents, it's probably not quite right to state that a PLL's reference clock is a "parent" in a clock tree sense. When a PLL is active, the output clock originates from a VCO of some kind that is internal to the PLL. The reference clock is just used by the PLL internal logic to close the control loop. Some PLL-like clock sources use a third clock that is used to clock some of the PLL's internal logic (let's call this a "functional clock.") So the reference clock and functional clock are (usually) required by the PLL to operate, and should therefore be required by the PLL clock driver code in the kernel; but one could claim that they aren't technically parent clocks of the PLL in a clock tree sense, since the downstream output clock isn't directly derived from either of those clocks. Otherwise if we want to represent those clocks accurately in the Linux clock tree, we probably need to state that clocks can have multiple "parents" that often need to be active simultaneously, and we should propagate usecount changes accordingly up all of the parent paths. ... Anyway, thanks for following up on this, - Paul
On Tue, Dec 16, 2014 at 01:01:17PM -0700, Paul Walmsley wrote: > So the reference clock and functional clock are (usually) required by the > PLL to operate, and should therefore be required by the PLL clock driver > code in the kernel; but one could claim that they aren't technically parent > clocks of the PLL in a clock tree sense, since the downstream output clock > isn't directly derived from either of those clocks. The reference clock is the parent clock for a PLL, and the output clock is a derivative of the reference clock. The PLL maths show that very clearly.
On Tue, 16 Dec 2014, Russell King - ARM Linux wrote: > On Tue, Dec 16, 2014 at 01:01:17PM -0700, Paul Walmsley wrote: > > So the reference clock and functional clock are (usually) required by the > > PLL to operate, and should therefore be required by the PLL clock driver > > code in the kernel; but one could claim that they aren't technically parent > > clocks of the PLL in a clock tree sense, since the downstream output clock > > isn't directly derived from either of those clocks. > > The reference clock is the parent clock for a PLL, and the output clock > is a derivative of the reference clock. The PLL maths show that very > clearly. The PLL's internal oscillator is the electrical source of the PLL's output clock. The reference clock is just used to discipline that internal oscillator. Even that's not necessary - the reference clock can be optional on some clock source implementations, which can run the internal oscillator in open-loop mode. The question of what the appropriate "parent" clock(s) should be is really a question of how the software chooses to model it, and has more to do with use-counts, constraints, etc. I don't believe we have documented a precise definition for what a "parent clock" is, in the Linux clock framework context. - Paul
On Tue, Dec 16, 2014 at 08:45:25PM +0000, Paul Walmsley wrote: > On Tue, 16 Dec 2014, Russell King - ARM Linux wrote: > > > On Tue, Dec 16, 2014 at 01:01:17PM -0700, Paul Walmsley wrote: > > > So the reference clock and functional clock are (usually) required by the > > > PLL to operate, and should therefore be required by the PLL clock driver > > > code in the kernel; but one could claim that they aren't technically parent > > > clocks of the PLL in a clock tree sense, since the downstream output clock > > > isn't directly derived from either of those clocks. > > > > The reference clock is the parent clock for a PLL, and the output clock > > is a derivative of the reference clock. The PLL maths show that very > > clearly. > > The PLL's internal oscillator is the electrical source of the PLL's output > clock. The reference clock is just used to discipline that internal > oscillator. Even that's not necessary - the reference clock can be > optional on some clock source implementations, which can run the internal > oscillator in open-loop mode. I would argue that running a PLL in open-loop mode is an exceptional circumstance: in such scenarios, many PLLs will head towards a certain frequency (depending on their design) and some may be rather unstable in open loop mode. Only those which have been explicitly designed to be stable in open loop mode should be run that way. And arguably, a PLL without a reference clock is not a phase *locked* loop at all - the clue is in the name. A PLL without a reference clock is merely an oscillator. There is no "phase lock" what so ever. > The question of what the appropriate "parent" clock(s) should be is really > a question of how the software chooses to model it, and has more to do > with use-counts, constraints, etc. I don't believe we have documented a > precise definition for what a "parent clock" is, in the Linux clock > framework context. If a PLL is designed to be operated in open loop mode, then the parent clock should be optional, otherwise the parent clock is rather fundamental to the PLLs operation, in the same way that the parent clock is fundamental to a divider's operation. You can argue that taking away the "reference" clock to a divider gives a known output (which happens to be 0Hz) but a steady state can still be a perfectly valid clock signal too if it changes state at some point. :) In much the same way, some PLLs are designed to disable their output when they are not in lock. Much like that divider above with no reference input. This really is a question about how you view a PLL, and it's one which can be debated for a very long time, and everyone will have perfectly valid but conflicting ideas on it.
+ paul@pwsan.com (I prefer not having to deal with MS Exchange when possible :-) ) On Tue, 16 Dec 2014, Russell King - ARM Linux wrote: > On Tue, Dec 16, 2014 at 08:45:25PM +0000, Paul Walmsley wrote: > > On Tue, 16 Dec 2014, Russell King - ARM Linux wrote: > > > > > On Tue, Dec 16, 2014 at 01:01:17PM -0700, Paul Walmsley wrote: > > > > So the reference clock and functional clock are (usually) required by the > > > > PLL to operate, and should therefore be required by the PLL clock driver > > > > code in the kernel; but one could claim that they aren't technically parent > > > > clocks of the PLL in a clock tree sense, since the downstream output clock > > > > isn't directly derived from either of those clocks. > > > > > > The reference clock is the parent clock for a PLL, and the output clock > > > is a derivative of the reference clock. The PLL maths show that very > > > clearly. > > > > The PLL's internal oscillator is the electrical source of the PLL's output > > clock. The reference clock is just used to discipline that internal > > oscillator. Even that's not necessary - the reference clock can be > > optional on some clock source implementations, which can run the internal > > oscillator in open-loop mode. > > I would argue that running a PLL in open-loop mode is an exceptional > circumstance: in such scenarios, many PLLs will head towards a certain > frequency (depending on their design) and some may be rather unstable > in open loop mode. Only those which have been explicitly designed to > be stable in open loop mode should be run that way. > > And arguably, a PLL without a reference clock is not a phase *locked* > loop at all - the clue is in the name. A PLL without a reference clock > is merely an oscillator. There is no "phase lock" what so ever. ... > This really is a question about how you view a PLL, and it's one which > can be debated for a very long time, and everyone will have perfectly > valid but conflicting ideas on it. Yep, I agree with most of what you wrote. I was mostly thinking about our Linux data structures and software abstraction for clocks that have multiple input clocks that all need to be simultaneously active for the IP block to work. PLLs and PLL-like clock sources which take multiple input clocks just happened to be the examples of this phenomenon that came to mind. Those multiple input clocks could all be considered "parents" from a use-counting point of view, and we could expect the core clock code to deal with them. Or one could take the electrical point of view and consider only one of the input clocks to be the "parent" -- whatever's directly driving the output clock -- and fix up the gating and constraint details in the driver code for that clock source. I don't think this pot is boiling over at the moment; but, just something for us to reflect on. - Paul
Am Dienstag, den 16.12.2014, 22:25 +0000 schrieb Paul Walmsley: > + paul@pwsan.com (I prefer not having to deal with MS Exchange when > possible :-) ) > > On Tue, 16 Dec 2014, Russell King - ARM Linux wrote: > > > On Tue, Dec 16, 2014 at 08:45:25PM +0000, Paul Walmsley wrote: > > > On Tue, 16 Dec 2014, Russell King - ARM Linux wrote: > > > > > > > On Tue, Dec 16, 2014 at 01:01:17PM -0700, Paul Walmsley wrote: > > > > > So the reference clock and functional clock are (usually) required by the > > > > > PLL to operate, and should therefore be required by the PLL clock driver > > > > > code in the kernel; but one could claim that they aren't technically parent > > > > > clocks of the PLL in a clock tree sense, since the downstream output clock > > > > > isn't directly derived from either of those clocks. > > > > > > > > The reference clock is the parent clock for a PLL, and the output clock > > > > is a derivative of the reference clock. The PLL maths show that very > > > > clearly. > > > > > > The PLL's internal oscillator is the electrical source of the PLL's output > > > clock. The reference clock is just used to discipline that internal > > > oscillator. Even that's not necessary - the reference clock can be > > > optional on some clock source implementations, which can run the internal > > > oscillator in open-loop mode. > > > > I would argue that running a PLL in open-loop mode is an exceptional > > circumstance: in such scenarios, many PLLs will head towards a certain > > frequency (depending on their design) and some may be rather unstable > > in open loop mode. Only those which have been explicitly designed to > > be stable in open loop mode should be run that way. > > > > And arguably, a PLL without a reference clock is not a phase *locked* > > loop at all - the clue is in the name. A PLL without a reference clock > > is merely an oscillator. There is no "phase lock" what so ever. > > ... > > > This really is a question about how you view a PLL, and it's one which > > can be debated for a very long time, and everyone will have perfectly > > valid but conflicting ideas on it. > > Yep, I agree with most of what you wrote. I was mostly thinking about our > Linux data structures and software abstraction for clocks that have > multiple input clocks that all need to be simultaneously active for the IP > block to work. PLLs and PLL-like clock sources which take multiple input > clocks just happened to be the examples of this phenomenon that came to > mind. > > Those multiple input clocks could all be considered "parents" from a > use-counting point of view, and we could expect the core clock code to > deal with them. Or one could take the electrical point of view and > consider only one of the input clocks to be the "parent" -- whatever's > directly driving the output clock -- and fix up the gating and constraint > details in the driver code for that clock source. > > I don't think this pot is boiling over at the moment; but, just something > for us to reflect on. > Maybe I'm thinking about this too lightly, but I think we already have the right abstractions in place. The clock tree in Linux is always modeled along the flow of reference clocks. So typically the root of the tree is something like an oscillator and everything spreads out from this in a parent/child relation. So clearly the reference clock of a PLL is the PLLs parent. If the PLL is running in open-loop mode (isn't this rather direct frequency synthesis?) is has no parent and is the root of a tree itself. If a PLL needs a functional clock to drive some internal logic, that doesn't directly influence the clock synthesis from reference to resulting clock, that clock would be no parent to the PLL. The PLL is merely a consumer of this clock just like every other device in the system. Regards, Lucas
On 12/17/2014 11:52 AM, Lucas Stach wrote: > Am Dienstag, den 16.12.2014, 22:25 +0000 schrieb Paul Walmsley: >> + paul@pwsan.com (I prefer not having to deal with MS Exchange when >> possible :-) ) >> >> On Tue, 16 Dec 2014, Russell King - ARM Linux wrote: >> >>> On Tue, Dec 16, 2014 at 08:45:25PM +0000, Paul Walmsley wrote: >>>> On Tue, 16 Dec 2014, Russell King - ARM Linux wrote: >>>> >>>>> On Tue, Dec 16, 2014 at 01:01:17PM -0700, Paul Walmsley wrote: >>>>>> So the reference clock and functional clock are (usually) required by the >>>>>> PLL to operate, and should therefore be required by the PLL clock driver >>>>>> code in the kernel; but one could claim that they aren't technically parent >>>>>> clocks of the PLL in a clock tree sense, since the downstream output clock >>>>>> isn't directly derived from either of those clocks. >>>>> >>>>> The reference clock is the parent clock for a PLL, and the output clock >>>>> is a derivative of the reference clock. The PLL maths show that very >>>>> clearly. >>>> >>>> The PLL's internal oscillator is the electrical source of the PLL's output >>>> clock. The reference clock is just used to discipline that internal >>>> oscillator. Even that's not necessary - the reference clock can be >>>> optional on some clock source implementations, which can run the internal >>>> oscillator in open-loop mode. >>> >>> I would argue that running a PLL in open-loop mode is an exceptional >>> circumstance: in such scenarios, many PLLs will head towards a certain >>> frequency (depending on their design) and some may be rather unstable >>> in open loop mode. Only those which have been explicitly designed to >>> be stable in open loop mode should be run that way. >>> >>> And arguably, a PLL without a reference clock is not a phase *locked* >>> loop at all - the clue is in the name. A PLL without a reference clock >>> is merely an oscillator. There is no "phase lock" what so ever. >> >> ... >> >>> This really is a question about how you view a PLL, and it's one which >>> can be debated for a very long time, and everyone will have perfectly >>> valid but conflicting ideas on it. >> >> Yep, I agree with most of what you wrote. I was mostly thinking about our >> Linux data structures and software abstraction for clocks that have >> multiple input clocks that all need to be simultaneously active for the IP >> block to work. PLLs and PLL-like clock sources which take multiple input >> clocks just happened to be the examples of this phenomenon that came to >> mind. >> >> Those multiple input clocks could all be considered "parents" from a >> use-counting point of view, and we could expect the core clock code to >> deal with them. Or one could take the electrical point of view and >> consider only one of the input clocks to be the "parent" -- whatever's >> directly driving the output clock -- and fix up the gating and constraint >> details in the driver code for that clock source. >> >> I don't think this pot is boiling over at the moment; but, just something >> for us to reflect on. >> > Maybe I'm thinking about this too lightly, but I think we already have > the right abstractions in place. > > The clock tree in Linux is always modeled along the flow of reference > clocks. So typically the root of the tree is something like an > oscillator and everything spreads out from this in a parent/child > relation. So clearly the reference clock of a PLL is the PLLs parent. If > the PLL is running in open-loop mode (isn't this rather direct frequency > synthesis?) is has no parent and is the root of a tree itself. > > If a PLL needs a functional clock to drive some internal logic, that > doesn't directly influence the clock synthesis from reference to > resulting clock, that clock would be no parent to the PLL. The PLL is > merely a consumer of this clock just like every other device in the > system. > > Regards, > Lucas The dual parent issue required by the DPLL code is caused by the introduction of determine-rate / set-parent / set-rate-and-parent logic to OMAP DPPLs. I took a short-cut here, making the assumption that every DPLL has both of these clocks defined (which basically they do have, as every DPLL technically has both reference and bypass clocks, but which can be the same clock => thus the declaration itself is missing in some cases.) The clock data is modelled like this for every other TI SoC (which use DT clock data), except for omap3 legacy clock data, thus the patch to tweak the clock data itself. It would be much harder to modify the clock code itself to take this into account, rather than just add the missing parents to the clock data, thus the approach taken in the patch. We can of course discuss whether it is okay to have DPLLs modelled as they are right now. I think them as composite clocks containing mux, gate and divider/multiplier logic within a single black box, we could split them up, but I don't see much need for that. If it works, don't break it. Regarding the omap3 clock data patch, I have also just posted new patch set that will move the omap3 legacy clock data under clock driver, until we figure out what to finally do with this (use the DT clocks, or introduce loadable clock data modules or something.) Thus, the data patch is kind of a temporary one, or thats the intention at least, but I wouldn't call it a hack. It is there to fix the issues introduced by the set-parent / set-rate-and-parent logic, which was required by the changes to the core clock set-rate. -Tero
On Wed, 17 Dec 2014, Lucas Stach wrote: > Maybe I'm thinking about this too lightly, but I think we already have > the right abstractions in place. > > The clock tree in Linux is always modeled along the flow of reference > clocks. So typically the root of the tree is something like an > oscillator and everything spreads out from this in a parent/child > relation. So clearly the reference clock of a PLL is the PLLs parent. If > the PLL is running in open-loop mode (isn't this rather direct frequency > synthesis?) is has no parent and is the root of a tree itself. > > If a PLL needs a functional clock to drive some internal logic, that > doesn't directly influence the clock synthesis from reference to > resulting clock, that clock would be no parent to the PLL. The PLL is > merely a consumer of this clock just like every other device in the > system. I suspect we're using different terminology. From my point of view, a reference clock is an input clock signal that is used by another clock generator IP block or clock measurement IP block. The use is to measure and (optionally) discipline another oscillator. Thus the clock generator/measurement IP block generally does not electrically propagate the reference clock outside of the block. (Although some of the reference clock's characteristics, like phase noise or frequency stability, may be propagated indirectly.) Only a small number of clock tree entities makes use of reference clocks. By contrast, we could use the term "source clock" in the SoC context to mean a clock signal that directly drives a downstream IP block, in an electrical sense. These clocks are directly propagated through dividers and muxes. If you're willing to play along with this terminology, then a PLL's "source clock" would be its internal VCO/DCO, which thus would be the root clock of the PLL. Flipping over from hardware to software, in the Linux clock tree, there's usually not much point to modeling the VCO/DCO directly because it tends not to be under the direct control of the OS, and it is usually tightly integrated into the PLLs from a hardware point of view. But unlike most of the Linux clock tree nodes that are downstream from clock sources, which use clocks that are electrically driven by the clock source, the clock signal that PLLs and PLL-like clocks generate is not directly driven by the reference clock. So why is a reference clock listed as a Linux parent clock of an OMAP PLL? At least for OMAP, it was sort of shoehorned in to represent a gating dependency. It was a way of stating that the reference clock had to be enabled for the PLL to generate an output clock. (The off-chip crystal oscillator that drives most of the PLL reference clocks could be disabled when all of its users were idle, to save energy.) But this type of gating dependency is equally true for, say, a separate functional clock that the clock source requires in order to operate. (OMAP PLLs didn't have a separate functional clock, at least not that I was aware of; but that's not true for some similar clock sources used in other SoCs.) The clock framework wasn't re-entrant back then, and we wanted to avoid implementing re-entrancy because we were worried that it could turn into a mess. So we just wound up listing the reference clock as the PLL's Linux parent clock. My original comment was just intended to be a reflection on what it means to be a "parent clock" in the Linux clock tree sense. If it's intended to represent "source clocks" as they are defined above, then PLLs and PLL-like clocks should be root nodes, except for the few cases where it's useful to add a node for the underlying source oscillator directly (for example, if open-loop mode is supported). This might be the right way forward for the time being, and I like the idea of stating that Linux parent clocks should represent electrical "source clock" relationships. That raises the question of what to do about the additional gating relationships. At the moment, the clock type code needs to be responsible for calling clk_enable() etc. on all of the reference and functional clocks that need to be ungated for the clock source to work. But it has always seemed preferable to me to represent fundamental hardware structural constraints in data. If the data structures are well-defined, then the data should be relatively easy to analyze, reason about, generate, validate, share across platforms, and operate upon iteratively; unlike custom per-clocktype code, which often isn't. - Paul
Quoting Paul Walmsley (2014-12-18 11:23:07) > On Wed, 17 Dec 2014, Lucas Stach wrote: > > > Maybe I'm thinking about this too lightly, but I think we already have > > the right abstractions in place. > > > > The clock tree in Linux is always modeled along the flow of reference > > clocks. So typically the root of the tree is something like an > > oscillator and everything spreads out from this in a parent/child > > relation. So clearly the reference clock of a PLL is the PLLs parent. If > > the PLL is running in open-loop mode (isn't this rather direct frequency > > synthesis?) is has no parent and is the root of a tree itself. > > > > If a PLL needs a functional clock to drive some internal logic, that > > doesn't directly influence the clock synthesis from reference to > > resulting clock, that clock would be no parent to the PLL. The PLL is > > merely a consumer of this clock just like every other device in the > > system. > > I suspect we're using different terminology. > > From my point of view, a reference clock is an input clock signal that is > used by another clock generator IP block or clock measurement IP block. > The use is to measure and (optionally) discipline another oscillator. > Thus the clock generator/measurement IP block generally does not > electrically propagate the reference clock outside of the block. > (Although some of the reference clock's characteristics, like phase noise > or frequency stability, may be propagated indirectly.) Only a small > number of clock tree entities makes use of reference clocks. > > By contrast, we could use the term "source clock" in the SoC context to > mean a clock signal that directly drives a downstream IP block, in an > electrical sense. These clocks are directly propagated through dividers > and muxes. > > If you're willing to play along with this terminology, then a PLL's > "source clock" would be its internal VCO/DCO, which thus would be the root > clock of the PLL. > > Flipping over from hardware to software, in the Linux clock tree, there's > usually not much point to modeling the VCO/DCO directly because it tends > not to be under the direct control of the OS, and it is usually tightly > integrated into the PLLs from a hardware point of view. But unlike most > of the Linux clock tree nodes that are downstream from clock sources, > which use clocks that are electrically driven by the clock source, the > clock signal that PLLs and PLL-like clocks generate is not directly driven > by the reference clock. Hi Paul, This is debatable given that the formula to determine the OMAP DPLL output rate directly depends upon the rate of the input clock (osc, 32Khz, hsd, bypass, etc). I'm not speaking for all PLLs, just for the OMAP ones that I am familiar with. > > So why is a reference clock listed as a Linux parent clock of an OMAP PLL? > At least for OMAP, it was sort of shoehorned in to represent a gating > dependency. It was a way of stating that the reference clock had to be > enabled for the PLL to generate an output clock. (The off-chip crystal > oscillator that drives most of the PLL reference clocks could be disabled > when all of its users were idle, to save energy.) But this type of gating > dependency is equally true for, say, a separate functional clock that the > clock source requires in order to operate. (OMAP PLLs didn't have a > separate functional clock, at least not that I was aware of; but that's > not true for some similar clock sources used in other SoCs.) Using your terminology above, it is possible to do what you want to do today without any change to the clock framework. Simply make each DPLL a device. That device calls clk_get, clk_prepare_enable on the input reference clock. Coincidentally these new DPLL device are clock providers themselves and they provide root clocks of their own which are the DPLLs and corresponding Mx outputs. This model would treat the reference clock as something that drives the DPLL device's logic, but not as a part of the clock signal chain. I don't personally agree with the idea that the reference clock is not a parent of the DPLL, but I don't oppose any change to the OMAP clock driver along those lines so long as it is done in a sane way. It seems there is never just one correct way to look at these things. > > The clock framework wasn't re-entrant back then, and we wanted to avoid > implementing re-entrancy because we were worried that it could turn into a > mess. So we just wound up listing the reference clock as the PLL's Linux > parent clock. > > My original comment was just intended to be a reflection on what it means > to be a "parent clock" in the Linux clock tree sense. If it's intended to > represent "source clocks" as they are defined above, then PLLs and > PLL-like clocks should be root nodes, except for the few cases where it's > useful to add a node for the underlying source oscillator directly (for > example, if open-loop mode is supported). > > This might be the right way forward for the time being, and I like the > idea of stating that Linux parent clocks should represent electrical > "source clock" relationships. > > That raises the question of what to do about the additional gating > relationships. At the moment, the clock type code needs to be responsible > for calling clk_enable() etc. on all of the reference and functional > clocks that need to be ungated for the clock source to work. But it has > always seemed preferable to me to represent fundamental hardware > structural constraints in data. If the data structures are well-defined, > then the data should be relatively easy to analyze, reason about, > generate, validate, share across platforms, and operate upon iteratively; > unlike custom per-clocktype code, which often isn't. I completely agree that the interfaces and abstractions in the clock framework do not scale well. As an example, there could be much more reuse if callbacks such as .get_best_div() existed and the large variety of .round_rate() implementations could be replaced by a single generic one. Easier mixing and matching of callbacks would be great as well. We don't quite have polymorphism but something better could be achieved than the complex clock type. Namely the ability to combine various clock hardware ops at run-time without having to always generate unique struct clk_ops per platform. I think there are lots of ideas out there on how to improve this stuff. Regards, Mike > > > - Paul
On Thu, 18 Dec 2014, Mike Turquette wrote: > Quoting Paul Walmsley (2014-12-18 11:23:07) > > > Flipping over from hardware to software, in the Linux clock tree, there's > > usually not much point to modeling the VCO/DCO directly because it tends > > not to be under the direct control of the OS, and it is usually tightly > > integrated into the PLLs from a hardware point of view. But unlike most > > of the Linux clock tree nodes that are downstream from clock sources, > > which use clocks that are electrically driven by the clock source, the > > clock signal that PLLs and PLL-like clocks generate is not directly driven > > by the reference clock. > > Hi Paul, Hi Mike, > This is debatable given that the formula to determine the OMAP DPLL > output rate directly depends upon the rate of the input clock (osc, > 32Khz, hsd, bypass, etc). I'm not speaking for all PLLs, just for the > OMAP ones that I am familiar with. The frequency and phase noise characteristics of the PLL's output signal are partially a function of the reference clock. But that's only because the reference clock is used to steer the VCO. It's not because the reference clock itself appears on the PLL output. Even if the reference clock dividers and VCO loop dividers were both set to divide-by-one, the reference clock still would not appear on the PLL output - merely a simulacrum of it, originating by the VCO. The PLL's output clock is, electrically, directly driven by the VCO. Page 5 of this PDF has a nice frequency synthesis PLL block diagram with integer dividers: http://www.ti.com/lit/an/swra029/swra029.pdf To rephrase, if, in the block diagram above, one were to remove everything from the TCXO reference clock on the left to the triangle to the left of the VCO, and to replace all that with a constant voltage input to the VCO, you'd still get a clock signal out of the IP block, even though the reference clock would no longer exist. > > So why is a reference clock listed as a Linux parent clock of an OMAP PLL? > > At least for OMAP, it was sort of shoehorned in to represent a gating > > dependency. It was a way of stating that the reference clock had to be > > enabled for the PLL to generate an output clock. (The off-chip crystal > > oscillator that drives most of the PLL reference clocks could be disabled > > when all of its users were idle, to save energy.) But this type of gating > > dependency is equally true for, say, a separate functional clock that the > > clock source requires in order to operate. (OMAP PLLs didn't have a > > separate functional clock, at least not that I was aware of; but that's > > not true for some similar clock sources used in other SoCs.) > > Using your terminology above, it is possible to do what you want to do > today without any change to the clock framework. Simply make each DPLL a > device. That device calls clk_get, clk_prepare_enable on the input > reference clock. I think I did something like that with the Tegra DFLL driver last year. Except the clock provider device was the DFLL IP block itself, with its own registers, etc., and not an additional abstraction, if I recall correctly. My interest is mostly to determine whether some of that special-case code can be converted into data that the CCF core can operate on, in a consistent manner that would be useful across platforms, and in a way that several people could potentially agree on. > Coincidentally these new DPLL device are clock providers themselves and > they provide root clocks of their own which are the DPLLs and > corresponding Mx outputs. This model would treat the reference clock as > something that drives the DPLL device's logic, but not as a part of the > clock signal chain. > > I don't personally agree with the idea that the reference clock is not a > parent of the DPLL, I'd be interested to know how you would define a parent clock, if it differs from the electrical "source clock" definition that I mentioned. The Linux clock tree is a software abstraction. So one could define "parent clock" however one wishes. (That's not to say that all definitions are equally useful...) One could state, for example, that a parent clock is any clock that has some effect on the output of the child clock. That would be one definition which might be consistent with your comments. The problem with that is that it means that a clock can have multiple simultaneously-active parents, which I'd naïvely think we'd want to avoid, if we could. I also have to admit that my bias is towards definitions with some kind of relationship to the hardware, which helps avoid the "utter relativism" problem that you mention below. > but I don't oppose any change to the OMAP clock driver along those lines > so long as it is done in a sane way. It seems there is never just one > correct way to look at these things. I'd say that to the extent that one could come up with concrete definitions for terms like "parent clocks," it allows others to reason about how the CCF should work, helps distinguish between data and code changes that do and don't make sense, and helps others understand which changes might be acceptable to maintainers. > > The clock framework wasn't re-entrant back then, and we wanted to avoid > > implementing re-entrancy because we were worried that it could turn into a > > mess. So we just wound up listing the reference clock as the PLL's Linux > > parent clock. > > > > My original comment was just intended to be a reflection on what it means > > to be a "parent clock" in the Linux clock tree sense. If it's intended to > > represent "source clocks" as they are defined above, then PLLs and > > PLL-like clocks should be root nodes, except for the few cases where it's > > useful to add a node for the underlying source oscillator directly (for > > example, if open-loop mode is supported). > > > > This might be the right way forward for the time being, and I like the > > idea of stating that Linux parent clocks should represent electrical > > "source clock" relationships. > > > > That raises the question of what to do about the additional gating > > relationships. At the moment, the clock type code needs to be responsible > > for calling clk_enable() etc. on all of the reference and functional > > clocks that need to be ungated for the clock source to work. But it has > > always seemed preferable to me to represent fundamental hardware > > structural constraints in data. If the data structures are well-defined, > > then the data should be relatively easy to analyze, reason about, > > generate, validate, share across platforms, and operate upon iteratively; > > unlike custom per-clocktype code, which often isn't. > > I completely agree that the interfaces and abstractions in the clock > framework do not scale well. As an example, there could be much more > reuse if callbacks such as .get_best_div() existed and the large variety > of .round_rate() implementations could be replaced by a single generic > one. > > Easier mixing and matching of callbacks would be great as well. We don't > quite have polymorphism but something better could be achieved than the > complex clock type. Namely the ability to combine various clock hardware > ops at run-time without having to always generate unique struct clk_ops > per platform. > > I think there are lots of ideas out there on how to improve this stuff. Oh, just in case it isn't clear, my objective wasn't to criticize the existing CCF. Just thinking that if it was possible to come to an agreement on how to define some of these entities, it might make it easier to reason together about ways to work on the core code and data structures. I'd hate to spend a bunch of time working on changes that you would eventually nack because we might not agree on what a parent clock is :-) - Paul
On Wed, 17 Dec 2014, Tero Kristo wrote: > The dual parent issue required by the DPLL code is caused by the introduction > of determine-rate / set-parent / set-rate-and-parent logic to OMAP DPPLs. I > took a short-cut here, making the assumption that every DPLL has both of these > clocks defined (which basically they do have, as every DPLL technically has > both reference and bypass clocks, but which can be the same clock => thus the > declaration itself is missing in some cases.) The clock data is modelled like > this for every other TI SoC (which use DT clock data), except for omap3 legacy > clock data, thus the patch to tweak the clock data itself. It would be much > harder to modify the clock code itself to take this into account, rather than > just add the missing parents to the clock data, thus the approach taken in the > patch. We can of course discuss whether it is okay to have DPLLs modelled as > they are right now. I think them as composite clocks containing mux, gate and > divider/multiplier logic within a single black box, we could split them up, > but I don't see much need for that. If it works, don't break it. > > Regarding the omap3 clock data patch, I have also just posted new patch set > that will move the omap3 legacy clock data under clock driver, until we figure > out what to finally do with this (use the DT clocks, or introduce loadable > clock data modules or something.) Thus, the data patch is kind of a temporary > one, or thats the intention at least, but I wouldn't call it a hack. It is > there to fix the issues introduced by the set-parent / set-rate-and-parent > logic, which was required by the changes to the core clock set-rate. OK, I understand now why the OMAP3 clock DT data takes two of the same clock - one is for the reference and one is for bypass. So I retract my earlier statement about it being a hack, and am fine with Tero's original patch. I suppose I should resuscitate the uImage booter for the OMAP3 boards here at least. I dropped those out of the testbed thinking that we'd switch over to DT-only fairly quickly; that turned out not to be the case. - Paul
Quoting Paul Walmsley (2014-12-18 18:15:59) > On Thu, 18 Dec 2014, Mike Turquette wrote: > > > Quoting Paul Walmsley (2014-12-18 11:23:07) > > > > > Flipping over from hardware to software, in the Linux clock tree, there's > > > usually not much point to modeling the VCO/DCO directly because it tends > > > not to be under the direct control of the OS, and it is usually tightly > > > integrated into the PLLs from a hardware point of view. But unlike most > > > of the Linux clock tree nodes that are downstream from clock sources, > > > which use clocks that are electrically driven by the clock source, the > > > clock signal that PLLs and PLL-like clocks generate is not directly driven > > > by the reference clock. > > > > Hi Paul, > > Hi Mike, > > > This is debatable given that the formula to determine the OMAP DPLL > > output rate directly depends upon the rate of the input clock (osc, > > 32Khz, hsd, bypass, etc). I'm not speaking for all PLLs, just for the > > OMAP ones that I am familiar with. > > The frequency and phase noise characteristics of the PLL's output signal > are partially a function of the reference clock. But that's only because > the reference clock is used to steer the VCO. It's not because the > reference clock itself appears on the PLL output. Even if the reference > clock dividers and VCO loop dividers were both set to divide-by-one, the > reference clock still would not appear on the PLL output - merely a > simulacrum of it, originating by the VCO. The PLL's output clock is, > electrically, directly driven by the VCO. > > Page 5 of this PDF has a nice frequency synthesis PLL block diagram with > integer dividers: > > http://www.ti.com/lit/an/swra029/swra029.pdf > > To rephrase, if, in the block diagram above, one were to remove everything > from the TCXO reference clock on the left to the triangle to the left of > the VCO, and to replace all that with a constant voltage input to the VCO, > you'd still get a clock signal out of the IP block, even though the > reference clock would no longer exist. Yes, your point is completely valid. But TI's own clock tree diagrams (which sadly do not make it into any TRM that I know of) show a cascade starting at sys_clk, continuing through each DPLL, the Mx output and so on and so forth. I would link it if it was public :-/ Samsung's Exynos TRMs do a great job of illustrating the clock tree paths and my feedback to the TRM team at TI never seemed to take root. In addition the clock tree tool from TI[0] follows the same trend. This tool is developed by an independent team that has no relationship to the folks working on SoC operating system software. In short, the manufacturer's visualization of the clock signal chain treats the system clock as a parent of the DPLLs. > > > > So why is a reference clock listed as a Linux parent clock of an OMAP PLL? > > > At least for OMAP, it was sort of shoehorned in to represent a gating > > > dependency. It was a way of stating that the reference clock had to be > > > enabled for the PLL to generate an output clock. (The off-chip crystal > > > oscillator that drives most of the PLL reference clocks could be disabled > > > when all of its users were idle, to save energy.) But this type of gating > > > dependency is equally true for, say, a separate functional clock that the > > > clock source requires in order to operate. (OMAP PLLs didn't have a > > > separate functional clock, at least not that I was aware of; but that's > > > not true for some similar clock sources used in other SoCs.) > > > > Using your terminology above, it is possible to do what you want to do > > today without any change to the clock framework. Simply make each DPLL a > > device. That device calls clk_get, clk_prepare_enable on the input > > reference clock. > > I think I did something like that with the Tegra DFLL driver last year. > Except the clock provider device was the DFLL IP block itself, with its > own registers, etc., and not an additional abstraction, if I recall > correctly. This sounds like what I had in mind, if I was not clear above. > > My interest is mostly to determine whether some of that special-case code > can be converted into data that the CCF core can operate on, in a > consistent manner that would be useful across platforms, and in a way that > several people could potentially agree on. > > > Coincidentally these new DPLL device are clock providers themselves and > > they provide root clocks of their own which are the DPLLs and > > corresponding Mx outputs. This model would treat the reference clock as > > something that drives the DPLL device's logic, but not as a part of the > > clock signal chain. > > > > I don't personally agree with the idea that the reference clock is not a > > parent of the DPLL, > > I'd be interested to know how you would define a parent clock, if it > differs from the electrical "source clock" definition that I mentioned. My less-rigorous definition is a clock signal which is fed into some sink (in this case another clock generator IP) and has a tangential relationship or effect on its output rate. In the event of more than one active clock signal going into the IP block, then the current parent is the one that has the most reasonable affect on the clock signal rate of the downstream clock generator. In short, the parent is the one that you think, "oh gee, that's the parent clock" the first time that you look at the schematic. > > The Linux clock tree is a software abstraction. So one could define > "parent clock" however one wishes. (That's not to say that all > definitions are equally useful...) > > One could state, for example, that a parent clock is any clock that has > some effect on the output of the child clock. That would be one > definition which might be consistent with your comments. The problem with > that is that it means that a clock can have multiple simultaneously-active > parents, which I'd naïvely think we'd want to avoid, if we could. You're right to say that we're talking about a software abstraction. In general we should model this abstraction after the hardware as much as possible to keep debugging and our understanding of the platform sane. Though there are limits to how closely we want to model the hardware (please imagine a clever joke about "struct spinning_electron" here). I'm happy to update the documentation with guidelines for how driver writers should partition things to stay in line with their hardware once we have that discussion and settle on some guidelines. Understanding of the hardware varies wildly though, often due to poor or zero documentation. And additionally I don't want to create any new hoops for people to jump through if it brings no tangible gain to the software (better debugability, decreased code size, better power savings, more awesomer apis exposed to device drivers, etc). Finally, I know I continue to argue the point, but I would not have any problem merging changes to remove sys_clk as the parent of the DPLLs. I guess I'm just feeling argumentative. If you and the TI folks feel like that is the right way to go then by all means. [0] http://www.ti.com/general/docs/wtbu/wtbudocumentcenter.tsp?templateId=6123&navigationId=12804 Regards, Mike > > I also have to admit that my bias is towards definitions with some kind of > relationship to the hardware, which helps avoid the "utter relativism" > problem that you mention below. > > > but I don't oppose any change to the OMAP clock driver along those lines > > so long as it is done in a sane way. It seems there is never just one > > correct way to look at these things. > > I'd say that to the extent that one could come up with concrete > definitions for terms like "parent clocks," it allows others to reason > about how the CCF should work, helps distinguish between data and code > changes that do and don't make sense, and helps others understand which > changes might be acceptable to maintainers. > > > > The clock framework wasn't re-entrant back then, and we wanted to avoid > > > implementing re-entrancy because we were worried that it could turn into a > > > mess. So we just wound up listing the reference clock as the PLL's Linux > > > parent clock. > > > > > > My original comment was just intended to be a reflection on what it means > > > to be a "parent clock" in the Linux clock tree sense. If it's intended to > > > represent "source clocks" as they are defined above, then PLLs and > > > PLL-like clocks should be root nodes, except for the few cases where it's > > > useful to add a node for the underlying source oscillator directly (for > > > example, if open-loop mode is supported). > > > > > > This might be the right way forward for the time being, and I like the > > > idea of stating that Linux parent clocks should represent electrical > > > "source clock" relationships. > > > > > > That raises the question of what to do about the additional gating > > > relationships. At the moment, the clock type code needs to be responsible > > > for calling clk_enable() etc. on all of the reference and functional > > > clocks that need to be ungated for the clock source to work. But it has > > > always seemed preferable to me to represent fundamental hardware > > > structural constraints in data. If the data structures are well-defined, > > > then the data should be relatively easy to analyze, reason about, > > > generate, validate, share across platforms, and operate upon iteratively; > > > unlike custom per-clocktype code, which often isn't. > > > > I completely agree that the interfaces and abstractions in the clock > > framework do not scale well. As an example, there could be much more > > reuse if callbacks such as .get_best_div() existed and the large variety > > of .round_rate() implementations could be replaced by a single generic > > one. > > > > Easier mixing and matching of callbacks would be great as well. We don't > > quite have polymorphism but something better could be achieved than the > > complex clock type. Namely the ability to combine various clock hardware > > ops at run-time without having to always generate unique struct clk_ops > > per platform. > > > > I think there are lots of ideas out there on how to improve this stuff. > > Oh, just in case it isn't clear, my objective wasn't to criticize the > existing CCF. Just thinking that if it was possible to come to an > agreement on how to define some of these entities, it might make it easier > to reason together about ways to work on the core code and data > structures. I'd hate to spend a bunch of time working on changes that you > would eventually nack because we might not agree on what a parent clock is > :-) > > > - Paul
diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c index 20e120d..c2da2a0 100644 --- a/arch/arm/mach-omap2/dpll3xxx.c +++ b/arch/arm/mach-omap2/dpll3xxx.c @@ -474,7 +474,7 @@ void omap3_noncore_dpll_disable(struct clk_hw *hw) */ long omap3_noncore_dpll_determine_rate(struct clk_hw *hw, unsigned long rate, unsigned long *best_parent_rate, - struct clk **best_parent_clk) + struct clk_hw **best_parent_clk) { struct clk_hw_omap *clk = to_clk_hw_omap(hw); struct dpll_data *dd; @@ -488,10 +488,10 @@ long omap3_noncore_dpll_determine_rate(struct clk_hw *hw, unsigned long rate, if (__clk_get_rate(dd->clk_bypass) == rate && (dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) { - *best_parent_clk = dd->clk_bypass; + *best_parent_clk = __clk_get_hw(dd->clk_bypass); } else { rate = omap2_dpll_round_rate(hw, rate, best_parent_rate); - *best_parent_clk = dd->clk_ref; + *best_parent_clk = __clk_get_hw(dd->clk_ref); } *best_parent_rate = rate; diff --git a/arch/arm/mach-omap2/dpll44xx.c b/arch/arm/mach-omap2/dpll44xx.c index 535822f..0e58e5a 100644 --- a/arch/arm/mach-omap2/dpll44xx.c +++ b/arch/arm/mach-omap2/dpll44xx.c @@ -223,7 +223,7 @@ out: */ long omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw, unsigned long rate, unsigned long *best_parent_rate, - struct clk **best_parent_clk) + struct clk_hw **best_parent_clk) { struct clk_hw_omap *clk = to_clk_hw_omap(hw); struct dpll_data *dd; @@ -237,11 +237,11 @@ long omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw, unsigned long rate, if (__clk_get_rate(dd->clk_bypass) == rate && (dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) { - *best_parent_clk = dd->clk_bypass; + *best_parent_clk = __clk_get_hw(dd->clk_bypass); } else { rate = omap4_dpll_regm4xen_round_rate(hw, rate, best_parent_rate); - *best_parent_clk = dd->clk_ref; + *best_parent_clk = __clk_get_hw(dd->clk_ref); } *best_parent_rate = rate;