[PATCHv5 02/31] CLK: TI: Add DPLL clock support

[mturquette@linaro.org (Mike Turquette)]

Quoting Tero Kristo (2013-08-19 10:06:39)
> On 08/19/2013 07:24 PM, Mark Rutland wrote:
> > On Mon, Aug 19, 2013 at 04:09:37PM +0100, Tero Kristo wrote:
> >> On 08/19/2013 05:18 PM, Mark Rutland wrote:
> >>> On Mon, Aug 19, 2013 at 02:34:45PM +0100, Tero Kristo wrote:
> >>>> On 08/13/2013 01:50 PM, Mark Rutland wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Fri, Aug 02, 2013 at 05:25:21PM +0100, Tero Kristo wrote:
> >>>>>> The OMAP clock driver now supports DPLL clock type. This patch also
> >>>>>> adds support for DT DPLL nodes.
> >>>>>>
> >>>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> >>>>>> ---
> >>>>>>     .../devicetree/bindings/clock/ti/dpll.txt          |   70 +++
> >>>>>>     arch/arm/mach-omap2/clock.h                        |  144 +-----
> >>>>>>     arch/arm/mach-omap2/clock3xxx.h                    |    2 -
> >>>>>>     drivers/clk/Makefile                               |    1 +
> >>>>>>     drivers/clk/ti/Makefile                            |    3 +
> >>>>>>     drivers/clk/ti/dpll.c                              |  488 ++++++++++++++++++++
> >>>>>>     include/linux/clk/ti.h                             |  164 +++++++
> >>>>>>     7 files changed, 727 insertions(+), 145 deletions(-)
> >>>>>>     create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt
> >>>>>>     create mode 100644 drivers/clk/ti/Makefile
> >>>>>>     create mode 100644 drivers/clk/ti/dpll.c
> >>>>>>     create mode 100644 include/linux/clk/ti.h
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/clock/ti/dpll.txt b/Documentation/devicetree/bindings/clock/ti/dpll.txt
> >>>>>> new file mode 100644
> >>>>>> index 0000000..dcd6e63
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/clock/ti/dpll.txt
> >>>>>> @@ -0,0 +1,70 @@
> >>>>>> +Binding for Texas Instruments DPLL clock.
> >>>>>> +
> >>>>>> +This binding uses the common clock binding[1].  It assumes a
> >>>>>> +register-mapped DPLL with usually two selectable input clocks
> >>>>>> +(reference clock and bypass clock), with digital phase locked
> >>>>>> +loop logic for multiplying the input clock to a desired output
> >>>>>> +clock. This clock also typically supports different operation
> >>>>>> +modes (locked, low power stop etc.) This binding has several
> >>>>>> +sub-types, which effectively result in slightly different setup
> >>>>>> +for the actual DPLL clock.
> >>>>>> +
> >>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>>> +
> >>>>>> +Required properties:
> >>>>>> +- compatible : shall be one of:
> >>>>>> +               "ti,omap4-dpll-x2-clock",
> >>>>>> +               "ti,omap3-dpll-clock",
> >>>>>> +               "ti,omap3-dpll-core-clock",
> >>>>>> +               "ti,omap3-dpll-per-clock",
> >>>>>> +               "ti,omap3-dpll-per-j-type-clock",
> >>>>>> +               "ti,omap4-dpll-clock",
> >>>>>> +               "ti,omap4-dpll-core-clock",
> >>>>>> +               "ti,omap4-dpll-m4xen-clock",
> >>>>>> +               "ti,omap4-dpll-j-type-clock",
> >>>>>> +               "ti,omap4-dpll-no-gate-clock",
> >>>>>> +               "ti,omap4-dpll-no-gate-j-type-clock",
> >>>>>> +
> >>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
> >>>>>> +- clocks : link phandles of parent clocks (clk-ref and clk-bypass)
> >>>>>
> >>>>> It might be a good idea to use clock-names to clarify which clocks are
> >>>>> present (I notice your examples contain only one clock input).
> >>>>
> >>>> All DPLLs have both bypass and reference clocks, but these can be the
> >>>> same. Is it better to just list both always (and hence drop
> >>>> clock-names), or provide clock-names always?
> >>>
> >>> If they're separate inputs, it's worth listing both (even if they're fed
> >>> by the same provider). If it's possible one input might not be wired up,
> >>> use clock-names.
> >>
> >> Ref and bypass clocks are used currently by all DPLLs (also the APLL) so
> >> I guess I just enforce both to be specified.
> >
> > Ok. It's always possible to add clock-names later if a platform doesn't
> > wire an input. We lose the possibility of future compatibility, but
> > backwards compatibility is easy enough to maintain.
> >
> >>>>>> +- ti,clkdm-name : clockdomain name for the DPLL
> >>>>>
> >>>>> Could you elaborate on what this is for? What constitutes a valid name?
> >>>>>
> >>>>> I'm not sure a string is the best way to define the linkage of several
> >>>>> elements to a domain...
> >>>>
> >>>> Well, I am not sure if we can do this any better at this point, we don't
> >>>> have DT nodes for clockdomain at the moment (I am not sure if we will
> >>>> have those either as there are only a handful of those per SoC) but I'll
> >>>> add some more documentation for this.
> >>>
> >>> I'll have to see the documentation, but I'd be very wary of putting that
> >>> in as-is. Does having the clock domain string link this up to domains in
> >>> platform data?
> >>>
> >>> I'm not sure how well we'll be able to maintain support for that in
> >>> future if it requires other platform code to use now, and we're not sure
> >>> how the domains themselves will be represented in dt.
> >>
> >> Hmm so, should I add a stub representation for the clockdomains to the
> >> DT then for now or how should this be handled? The stubs could then be
> >> mapped to the actual clock domains based on their node names.

I'm not sure that this binding should know about the clock domain at
all. Maybe the clock domain binding should list the clocks that are
within the domain?

> >>
> >
> > I unfortunately don't have a good answer here, because I'm not that
> > familiar with how we handle clockdomains for power management purposes.
> >
> > As I understand it, each clock domain is essentially a clock gate
> > controlling multiple clock signals, so it's possible to describe that
> > with the common clock bindings, with a domain's clocks all coming from a
> > "domain-gate-clock" node (or something like that). That would make the
> > wiring of clocks to a domain explicit and in line with the rest of the
> > common clock bindings. But perhaps I've simplified things a bit too
> > far.
> 
> You got it basically right, but maybe oversimplified it a bit. The 
> root/parent clocks can still cross clockdomain boundaries, so mapping 
> everything under a simple clockdomain gate would not work. Basically 
> each clock has a mapping on the SoC for both its parent clock signal and 
> the clockdomain association, kind of having two parents at the same 
> time. In OMAP case, most of the clockdomains support hardware autoidle 
> type functionality, which puts the domain to idle once all the clocks on 
> it are disabled.

I always thought that OMAP clock domains were poorly named. Seems to me
that they had more to do with the IP/module programming than clocks per
se.  Again, I'm not sure that putting clkdm data into this binding is
correct.

Is it because you want a call to clk_enable to program the clock domain
in the .enable callback? I always thought that this was better left to a
pm_runtime_get callback...

Regards,
Mike

> 
> -Tero
> 
> > I'm not sure how easy it would be to use that information for power
> > management. I don't know what the kernel logic for clock domain power
> > management needs to know about consumers of the clocks and how it would
> > need to poke those consumers.
> 
> >
> > Cheers,
> > Mark.
> >