Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Matthijs van Duin]

On Thu, Dec 10, 2015 at 06:26:32PM -0800, Tony Lindgren wrote:
> +- compatible : shall be one of "ti,dm814-adpll-s-clock" or
> +  "ti,dm814-adpll-j-clock" depending on the type of the ADPLL

There's still a j -> lj you missed.

Also, since the device series almost always referred to as dm814x, any 
reason the x is omitted here?  Based on a quick google, dm814 mostly seems 
to be a moose hunting area in alaska ;-)

> +- clocks : link phandles of parent clocks (clk-ref and clk-bypass)

The documentation and your later example calls these clkinp and clkinpulow. 
In theory adpll-s has a third input (clkinphif).

Calling the input clock "clk-ref" is potentially misleading since the 
documentation uses the name "refclk" for clkinp/(N+1). Also, while I'll 
admit "clkinpulow" is an awful name, "clk-bypass" is also misleading since 
it is merely an optional alternative bypass clock, the default being 
clkinp/(N2+1).

Are you aware of any dm814x-sibling that actually uses clkinpulow, or are 
you just including it for completeness or consistency with DPLL instances 
in other devices like the omap4 or am335x?


> + clock-output-names = "481c5040.adpll.dcoclkldo",
> ...
> + clock-output-names = "481c5080.adpll.clkdcoldo",

I know this inconsistency comes straight out of the TRM, but may I vote for 
picking one of them and sticking to it? :-)


> +#define ADPLL_CLKINPHIFSEL_ADPLL_S   19  /* REVISIT: which bit? */

Impossible to say unless a dm814x sibling shows up that actually uses it; 
pll_mpu lacks the HIF clocks entirely.  Note that it's quite possible bit 19 
is indeed officially assigned to it, CLKINPHIFSEL and CLKOUTLDOEN do not 
conflict since they apply to different PLL types.


> +static void ti_adpll_set_bypass(struct ti_adpll_data *d)
> +static void ti_adpll_clear_bypass(struct ti_adpll_data *d)

These functions relate to a specific "Idle Bypass" mode of the PLL,
which gates the refclk and sort of freezes the PLL state.  Various other
control/status bits become unresponsive in this mode as a result.

I would suggest reflecting this in the name, or at least a comment,
since "bypass" refers to a much more general state.  Clearing the IDLE
bit is necessary to take the PLL out of bypass, but other conditions
need to be satisfied as well.  Hence, "clear_bypass" does not not
literally do what its name would seem to suggest.

> +static int ti_adpll_wait_lock(struct ti_adpll_data *d)
> ...
> + while (ti_adpll_clock_is_bypass(d)) {

Locked and bypass are not actually mutually exclusive:  if you only care
about the DCO clock and not CLKOUT you can clear M2PWDNZ before enabling
the PLL, resulting in status (FREQLOCK | PHASELOCK | BYPASS) after lock.

I don't know if there's any reason to take this obscure configuration
into consideration, but I just wanted to make you aware of it.


> +static int ti_adpll_is_prepared(struct clk_hw *hw)
> ...
> + return (v & ADPLL_STATUS_PREPARED_MASK) == ADPLL_STATUS_PREPARED_MASK;

Yet here you do actually test whether the PLL is locked... why the use a
different condition here than in ti_adpll_wait_lock?


> +static unsigned long ti_adpll_recalc_rate(struct clk_hw *hw,
> ...
> + if (ti_adpll_clock_is_bypass(d))
> + return clk_get_rate(d->clocks[TI_ADPLL_N2].clk);

Bypass refers to clkout. This function calculates the DCO clock, which
is never subject to bypass: if the PLL is off, dcoclk is low.


Although I understand the reasoning behind using abstractions like
readl_relaxed() for I/O access to allow portability to platforms where
I/O is not simply memory-mapped, this concern does not exist for
platform-specific devices like this.  There's really no reason in my
humble opinion this code could not simply do

/* read predivider and multiplier, shifted left 18 bits to
 * accomodate the fractional multiplier */
spin_lock_irqsave(>lock, flags);
divider = (pll->n + 1) << 18;
rate = (pll->m << 18) + pll->frac_m;
spin_unlock_irqrestore(>lock, flags);

do_div(rate, divider);

instead of spending a whole paragraph of code on reading each individual
field, which I think just hurts readability.


Note btw that PLLSS is entirely memory-like and is perfectly okay with
16-bit reads/writes.  Grouping the  u16 n, m2, m, n2;  into two 32-bit
registers in the documentation is just TI being silly.


> + frac_m += 1;

que?


> + /* Internal mux, sources from divider N2 or clkinpulow */
> + err = ti_adpll_init_mux(d, TI_ADPLL_ULOW, "ulow",

Shouldn't this just be called "bypass", since it is the bypass clock
after all.  I would associate the name "ulow" only with clkinpulow.


Matthijs
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Tony Lindgren]

* Matthijs van Duin  [151214 01:16]:
> On Thu, Dec 10, 2015 at 06:26:32PM -0800, Tony Lindgren wrote:
> > +- compatible : shall be one of "ti,dm814-adpll-s-clock" or
> > +  "ti,dm814-adpll-j-clock" depending on the type of the ADPLL
> 
> There's still a j -> lj you missed.

Oops thanks will fix.

> Also, since the device series almost always referred to as dm814x, any 
> reason the x is omitted here?  Based on a quick google, dm814 mostly seems 
> to be a moose hunting area in alaska ;-)

Well we don't want any any wild cards in the dts compatible names.
Typically the first piece of hardware is selected, but that causes
confusion too. So we're using comaptible names like dra7 and dm814.

> > +- clocks : link phandles of parent clocks (clk-ref and clk-bypass)
> 
> The documentation and your later example calls these clkinp and clkinpulow. 
> In theory adpll-s has a third input (clkinphif).
> 
> Calling the input clock "clk-ref" is potentially misleading since the 
> documentation uses the name "refclk" for clkinp/(N+1). Also, while I'll 
> admit "clkinpulow" is an awful name, "clk-bypass" is also misleading since 
> it is merely an optional alternative bypass clock, the default being 
> clkinp/(N2+1).
> 
> Are you aware of any dm814x-sibling that actually uses clkinpulow, or are 
> you just including it for completeness or consistency with DPLL instances 
> in other devices like the omap4 or am335x?

I don't know of any instances using it.. I'll take a look but seems like we
could just leave it out.

> > +   clock-output-names = "481c5040.adpll.dcoclkldo",
> > ...
> > +   clock-output-names = "481c5080.adpll.clkdcoldo",
> 
> I know this inconsistency comes straight out of the TRM, but may I vote for 
> picking one of them and sticking to it? :-)

I vote for dcoclkldo then :) The "cold" name embedded in the other one just
causes me confusion reading it every time..

> > +#define ADPLL_CLKINPHIFSEL_ADPLL_S 19  /* REVISIT: which bit? */
> 
> Impossible to say unless a dm814x sibling shows up that actually uses it; 
> pll_mpu lacks the HIF clocks entirely.  Note that it's quite possible bit 19 
> is indeed officially assigned to it, CLKINPHIFSEL and CLKOUTLDOEN do not 
> conflict since they apply to different PLL types.

Yup.

> > +static void ti_adpll_set_bypass(struct ti_adpll_data *d)
> > +static void ti_adpll_clear_bypass(struct ti_adpll_data *d)
> 
> These functions relate to a specific "Idle Bypass" mode of the PLL,
> which gates the refclk and sort of freezes the PLL state.  Various other
> control/status bits become unresponsive in this mode as a result.
> 
> I would suggest reflecting this in the name, or at least a comment,
> since "bypass" refers to a much more general state.  Clearing the IDLE
> bit is necessary to take the PLL out of bypass, but other conditions
> need to be satisfied as well.  Hence, "clear_bypass" does not not
> literally do what its name would seem to suggest.
> 
> > +static int ti_adpll_wait_lock(struct ti_adpll_data *d)
> > ...
> > +   while (ti_adpll_clock_is_bypass(d)) {

Yeah I have to take a closer look at this whole bypass thing..

> Locked and bypass are not actually mutually exclusive:  if you only care
> about the DCO clock and not CLKOUT you can clear M2PWDNZ before enabling
> the PLL, resulting in status (FREQLOCK | PHASELOCK | BYPASS) after lock.
> 
> I don't know if there's any reason to take this obscure configuration
> into consideration, but I just wanted to make you aware of it.

OK good to know thanks.

> > +static int ti_adpll_is_prepared(struct clk_hw *hw)
> > ...
> > +   return (v & ADPLL_STATUS_PREPARED_MASK) == ADPLL_STATUS_PREPARED_MASK;
> 
> Yet here you do actually test whether the PLL is locked... why the use a
> different condition here than in ti_adpll_wait_lock?
> 
> 
> > +static unsigned long ti_adpll_recalc_rate(struct clk_hw *hw,
> > ...
> > +   if (ti_adpll_clock_is_bypass(d))
> > +   return clk_get_rate(d->clocks[TI_ADPLL_N2].clk);
> 
> Bypass refers to clkout. This function calculates the DCO clock, which
> is never subject to bypass: if the PLL is off, dcoclk is low.

Thanks will check.

> 
> Although I understand the reasoning behind using abstractions like
> readl_relaxed() for I/O access to allow portability to platforms where
> I/O is not simply memory-mapped, this concern does not exist for
> platform-specific devices like this.  There's really no reason in my
> humble opinion this code could not simply do
> 
>   /* read predivider and multiplier, shifted left 18 bits to
>* accomodate the fractional multiplier */
>   spin_lock_irqsave(>lock, flags);
>   divider = (pll->n + 1) << 18;
>   rate = (pll->m << 18) + pll->frac_m;
>   spin_unlock_irqrestore(>lock, flags);
> 
>   do_div(rate, divider);
> 
> instead of spending a whole paragraph of code on reading each individual
> field, which I think just hurts readability.
> 
>
> Note btw that PLLSS is 

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Tony Lindgren]

* Tero Kristo  [151210 23:45]:
> On 12/11/2015 04:26 AM, Tony Lindgren wrote:
> 
> Looks mostly good to me, added some minor comments inline below. Sorry again
> for latencies in my replies.

No problme, thanks for looking.

> >+static bool ti_adpll_clock_is_bypass(struct ti_adpll_data *d)
> >+{
> >+unsigned long flags;
> >+u32 v;
> >+
> >+spin_lock_irqsave(>lock, flags);
> >+v = readl_relaxed(d->status);
> >+spin_unlock_irqrestore(>lock, flags);
> 
> What do you need the lock for in here?

Yeah seems pointless, will remove.

> >+static int ti_adpll_clkout_set_parent(struct clk_hw *hw, u8 index)
> >+{
> >+struct ti_adpll_clkout_data *co = to_clkout(hw);
> >+struct ti_adpll_data *d = co->adpll;
> >+
> >+return ti_adpll_clock_is_bypass(d) == index;

Hmm yeah that seems broken. I need to check what all really goes
to bypass mode with ulowclken signal.

> What is the point of this? It doesn't seem to set anything.

> >+/* Internal mux, sources sources from DCO and clkinphf */
> 
> Double "sources" in comment?
> 
> >+/* Output clkout clkout, sources M2 or ulow */
> 
> Double "clkout" in comment?

Oops will fix.

> >+d->pwrctrl = d->base + register_offset + ADPLL_PWRCTRL_OFFSET;
> >+d->clkctrl = d->base + register_offset + ADPLL_CLKCTRL_OFFSET;
> >+d->tenable = d->base + register_offset + ADPLL_TENABLE_OFFSET;
> >+d->tenablediv = d->base + register_offset + ADPLL_TENABLEDIV_OFFSET;
> >+d->m2ndiv = d->base + register_offset + ADPLL_M2NDIV_OFFSET;
> >+d->mn2div = d->base + register_offset + ADPLL_MN2DIV_OFFSET;
> >+d->fracdiv = d->base + register_offset + ADPLL_FRACDIV_OFFSET;
> >+d->bwctrl = d->base + register_offset + ADPLL_BWCTRL_OFFSET;
> >+d->status = d->base + register_offset + ADPLL_STATUS_OFFSET;
> >+d->m3div = d->base + register_offset + ADPLL_M3DIV_OFFSET;
> >+d->rampctrl = d->base + register_offset + ADPLL_RAMPCTRL_OFFSET;
> 
> Do you need the individual pointers to each of these registers within the
> struct? Seems the offsets are pretty static so could just use d->base +
> offset + MAGIC calculation where used.
> 
> Most of these registers are not used for anything either at the moment it
> seems.

Well I guess I was initially thinking that we can set up separate instances
for the children and don't need locking for the registers.. But all of them
really share the clkctl register so that did not work out.

Yeah I can change these to use offsets no problem.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Russell King - ARM Linux]

On Fri, Dec 11, 2015 at 07:48:54AM -0800, Tony Lindgren wrote:
> There's a problem with MAX_CON_ID 16 hardcoded allocated name length.

Absolutely right...

> In this case I have 13 instances of plls with 3 - 4 outputs each and I'd
> like to use "481c5040.adpll.clkout" style naming starting with the instance
> address. Also "adpll5.clkdcoldo" style naming would work,

Because it's a connection ID, not a clock _name_.  I see that we're still
making all the same mistakes with clocks that were made years ago, and
which lead down the path of amazingly complex drivers having conditional
clock names and the like.

Is there a reason why you can't split this into separate device and input
names, and use clk_get_sys() rather than passing NULL as the device to
clk_get().  This is exactly why we've ended up with clk_get_sys(), to
cater for the cases where there is no struct device to associate with the
connection ID: the idea behind clk_get_sys() is that you pass the device
name explicitly, along with the connection ID.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Tony Lindgren]

* Russell King - ARM Linux  [151211 08:16]:
> On Fri, Dec 11, 2015 at 07:48:54AM -0800, Tony Lindgren wrote:
> > There's a problem with MAX_CON_ID 16 hardcoded allocated name length.
> 
> Absolutely right...

Well adding the warning there allows people to at least see what's going
on.

> > In this case I have 13 instances of plls with 3 - 4 outputs each and I'd
> > like to use "481c5040.adpll.clkout" style naming starting with the instance
> > address. Also "adpll5.clkdcoldo" style naming would work,
> 
> Because it's a connection ID, not a clock _name_.  I see that we're still
> making all the same mistakes with clocks that were made years ago, and
> which lead down the path of amazingly complex drivers having conditional
> clock names and the like.

Yeah we certainly want to get out of the conditional name business.

> Is there a reason why you can't split this into separate device and input
> names, and use clk_get_sys() rather than passing NULL as the device to
> clk_get().  This is exactly why we've ended up with clk_get_sys(), to
> cater for the cases where there is no struct device to associate with the
> connection ID: the idea behind clk_get_sys() is that you pass the device
> name explicitly, along with the connection ID.

For most code yes, there's still a pile of shared legacy code relying on
the clock connection ID only.

So what do you envision people using for the connection ID in cases like
this? It needs to be unique, should relate to the documentation and fit
MAX_CON_ID. Something like "pll5.clkdcoldo" will still overflow for pll13
unless also the dot is left out :)

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Tony Lindgren]

* Russell King - ARM Linux  [151211 05:53]:
> On Thu, Dec 10, 2015 at 06:26:32PM -0800, Tony Lindgren wrote:
> > +   /* Released with kfree() by clkdev_drop() */
> > +   cl = kzalloc(sizeof(*cl), GFP_KERNEL);
> > +   if (!cl)
> > +   return -ENOMEM;
> > +
> > +   /* Use clkdev_add, clk_register_clkdev limits length to MAX_CON_ID */
> > +   cl->con_id = name;
> > +   cl->clk = clock;
> > +   cl->clk_hw = __clk_get_hw(clock);
> > +   clkdev_add(cl);
> > +   d->clocks[index].cl = cl;
> 
> NAK.  I've no idea why you're open-coding the clkdev internals (which
> seems to have been a historical habbit in OMAP code.)  Please stop
> doing this.
>
> You are provided with clkdev_alloc() which will allocate the structure
> and initialise it for you, and clkdev_add() which will add the allocated
> and initialised struct to the list of lookups.  Everything you're doing
> above can be done with clkdev_alloc() + clkdev_add() which have been
> there for a _very_ long time.  They're even documented (thanks for
> providing me with more proof that documentation is nothing but a waste
> of time. :))

I tried, but I was seeing mysterious silent failures for some clocks.

> Even better is clkdev_create() which eliminates the two step clkdev_alloc()
> and clkdev_add() process.
> 
> So, the whole of the above can be reduced down to:
> 
>   cl = clkdev_create(clock, name, NULL);
>   if (!cl)
>   return -ENOMEM;
> 

There's a problem with MAX_CON_ID 16 hardcoded allocated name length.

In this case I have 13 instances of plls with 3 - 4 outputs each and I'd
like to use "481c5040.adpll.clkout" style naming starting with the instance
address. Also "adpll5.clkdcoldo" style naming would work,

Below is a patch we should probably apply as a band aid to produce a proper
warning.

Naturally we don't want to allocate longer names for all the clocks.. And
we already do have some const names coming from device tree we could use.

What if we optionally allow passing a name to clkdev and add some flag
that tells clkdev code not to attempt to free it? Or do you have better
ideas in mind to fix the issue?

Regards,

Tony

8< 
From: Tony Lindgren 
Date: Fri, 11 Dec 2015 07:28:36 -0800
Subject: [PATCH] clkdev: Make silent failures of clk_get() produce a proper
 warning

Otherwise we can see mysterious failures with some clocks where
the name is longer than MAX_CON_ID 16. This can easily happen with
clock names coming from device tree for example in the form of
"481c5040.adpll.clkout" or "adpll1.clkdcoldo".

Let's make things a bit easier to debug by adding a warning for
the clock name. Note that we don't want to error out here as the
string matching still probably does the right thing for most
clocks.

Also note that we should also remove the hardcoded name allocation
in clkdev by adding functions that allow passing a name to clkdev
optionally.

Signed-off-by: Tony Lindgren 

--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -256,6 +256,10 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const 
char *dev_fmt,
 {
struct clk_lookup_alloc *cla;
 
+   if (strlen(con_id) > MAX_CON_ID)
+   pr_warn("%s length of %s > %i, clock lookup can fail\n",
+   __func__, con_id, MAX_CON_ID);
+
cla = __clkdev_alloc(sizeof(*cla));
if (!cla)
return NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Russell King - ARM Linux]

On Thu, Dec 10, 2015 at 06:26:32PM -0800, Tony Lindgren wrote:
> + /* Released with kfree() by clkdev_drop() */
> + cl = kzalloc(sizeof(*cl), GFP_KERNEL);
> + if (!cl)
> + return -ENOMEM;
> +
> + /* Use clkdev_add, clk_register_clkdev limits length to MAX_CON_ID */
> + cl->con_id = name;
> + cl->clk = clock;
> + cl->clk_hw = __clk_get_hw(clock);
> + clkdev_add(cl);
> + d->clocks[index].cl = cl;

NAK.  I've no idea why you're open-coding the clkdev internals (which
seems to have been a historical habbit in OMAP code.)  Please stop
doing this.

You are provided with clkdev_alloc() which will allocate the structure
and initialise it for you, and clkdev_add() which will add the allocated
and initialised struct to the list of lookups.  Everything you're doing
above can be done with clkdev_alloc() + clkdev_add() which have been
there for a _very_ long time.  They're even documented (thanks for
providing me with more proof that documentation is nothing but a waste
of time. :))

Even better is clkdev_create() which eliminates the two step clkdev_alloc()
and clkdev_add() process.

So, the whole of the above can be reduced down to:

cl = clkdev_create(clock, name, NULL);
if (!cl)
return -ENOMEM;

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] clk: ti: Add support for dm814x ADPLL

[Tony Lindgren]

On dm814x we have 13 ADPLLs with 3 to 4 outputs on each. The
ADPLLs have several dividers and muxes controlled by a shared
control register for each PLL.

Note that for the clocks to work as device drivers for booting on
dm814x, this patch depends on "ARM: OMAP2+: Change core_initcall
levels to postcore_initcall".

Also note that this patch does not implement clk_set_rate,
that will be posted later on when available.

Signed-off-by: Tony Lindgren 
---

Updated to use adpll_lj and adpll_s naming and s/FAPLL/ADPLL/ in the
documentation as suggested by Matthijs.

If no other comments, I'd like to have this patch alone in an immutable
branch againt v4.4-rc1 that I can merge in too. Maybe Tero wants to do
that and merge this along with the other omap clock patches?

---
 .../devicetree/bindings/clock/ti/adpll.txt |   42 +
 drivers/clk/Kconfig|1 +
 drivers/clk/ti/Kconfig |6 +
 drivers/clk/ti/Makefile|2 +
 drivers/clk/ti/adpll.c | 1028 
 drivers/clk/ti/clk-814x.c  |   53 +
 6 files changed, 1132 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/ti/adpll.txt
 create mode 100644 drivers/clk/ti/Kconfig
 create mode 100644 drivers/clk/ti/adpll.c

diff --git a/Documentation/devicetree/bindings/clock/ti/adpll.txt 
b/Documentation/devicetree/bindings/clock/ti/adpll.txt
new file mode 100644
index 000..8d951de
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti/adpll.txt
@@ -0,0 +1,42 @@
+Binding for Texas Instruments ADPLL clock.
+
+Binding status: Unstable - ABI compatibility may be broken in the future
+
+This binding uses the common clock binding[1]. It assumes a
+register-mapped ADPLL with two to three selectable input clocks
+and three to four children..
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of "ti,dm814-adpll-s-clock" or
+  "ti,dm814-adpll-j-clock" depending on the type of the ADPLL
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)
+- reg : address and length of the register set for controlling the ADPLL.
+
+Examples:
+   adpll_mpu_ck: adpll@40 {
+   #clock-cells = <1>;
+   compatible = "ti,dm814-adpll-s-clock";
+   reg = <0x40 0x40>;
+   clocks = <_ck _ck _ck>;
+   clock-names = "clkinp", "clkinpulow", "clkinphif";
+   clock-indices = <0>, <1>, <2>, <3>;
+   clock-output-names = "481c5040.adpll.dcoclkldo",
+"481c5040.adpll.clkout",
+"481c5040.adpll.clkoutx2",
+"481c5040.adpll.clkouthif";
+   };
+
+   adpll_dsp_ck: adpll@80 {
+   #clock-cells = <1>;
+   compatible = "ti,dm814-adpll-lj-clock";
+   reg = <0x80 0x30>;
+   clocks = <_ck _ck>;
+   clock-names = "clkinp", "clkinpulow";
+   clock-indices = <0>, <1>, <2>;
+   clock-output-names = "481c5080.adpll.clkdcoldo",
+"481c5080.adpll.clkout",
+"481c5080.adpll.clkoutldo";
+   };
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c3e3a02f..c0c9868 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -190,6 +190,7 @@ config COMMON_CLK_CDCE706
 
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
+source "drivers/clk/ti/Kconfig"
 source "drivers/clk/qcom/Kconfig"
 
 endmenu
diff --git a/drivers/clk/ti/Kconfig b/drivers/clk/ti/Kconfig
new file mode 100644
index 000..a9d5474
--- /dev/null
+++ b/drivers/clk/ti/Kconfig
@@ -0,0 +1,6 @@
+config COMMON_CLK_TI_ADPLL
+   tristate "Clock driver for dm814x ADPLL"
+   depends on ARCH_OMAP2PLUS
+   default y if SOC_TI81XX
+   ---help---
+ ADPLL clock driver for the dm814x SoC using common clock framework.
diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
index d4ac960..dfe91d7 100644
--- a/drivers/clk/ti/Makefile
+++ b/drivers/clk/ti/Makefile
@@ -18,3 +18,5 @@ obj-$(CONFIG_SOC_AM43XX)  += $(clk-common) 
dpll3xxx.o clk-43xx.o
 ifdef CONFIG_ATAGS
 obj-$(CONFIG_ARCH_OMAP3)+= clk-3xxx-legacy.o
 endif
+
+obj-$(CONFIG_COMMON_CLK_TI_ADPLL)  += adpll.o
diff --git a/drivers/clk/ti/adpll.c b/drivers/clk/ti/adpll.c
new file mode 100644
index 000..2c75c55
--- /dev/null
+++ b/drivers/clk/ti/adpll.c
@@ -0,0 +1,1028 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ 

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Tero Kristo]

On 12/11/2015 04:26 AM, Tony Lindgren wrote:

On dm814x we have 13 ADPLLs with 3 to 4 outputs on each. The
ADPLLs have several dividers and muxes controlled by a shared
control register for each PLL.

Note that for the clocks to work as device drivers for booting on
dm814x, this patch depends on "ARM: OMAP2+: Change core_initcall
levels to postcore_initcall".

Also note that this patch does not implement clk_set_rate,
that will be posted later on when available.

Signed-off-by: Tony Lindgren 


Hi Tony,

Looks mostly good to me, added some minor comments inline below. Sorry 
again for latencies in my replies.


-Tero


---

Updated to use adpll_lj and adpll_s naming and s/FAPLL/ADPLL/ in the
documentation as suggested by Matthijs.

If no other comments, I'd like to have this patch alone in an immutable
branch againt v4.4-rc1 that I can merge in too. Maybe Tero wants to do
that and merge this along with the other omap clock patches?

---
  .../devicetree/bindings/clock/ti/adpll.txt |   42 +
  drivers/clk/Kconfig|1 +
  drivers/clk/ti/Kconfig |6 +
  drivers/clk/ti/Makefile|2 +
  drivers/clk/ti/adpll.c | 1028 
  drivers/clk/ti/clk-814x.c  |   53 +
  6 files changed, 1132 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/clock/ti/adpll.txt
  create mode 100644 drivers/clk/ti/Kconfig
  create mode 100644 drivers/clk/ti/adpll.c

diff --git a/Documentation/devicetree/bindings/clock/ti/adpll.txt 
b/Documentation/devicetree/bindings/clock/ti/adpll.txt
new file mode 100644
index 000..8d951de
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti/adpll.txt
@@ -0,0 +1,42 @@
+Binding for Texas Instruments ADPLL clock.
+
+Binding status: Unstable - ABI compatibility may be broken in the future
+
+This binding uses the common clock binding[1]. It assumes a
+register-mapped ADPLL with two to three selectable input clocks
+and three to four children..
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be one of "ti,dm814-adpll-s-clock" or
+  "ti,dm814-adpll-j-clock" depending on the type of the ADPLL
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : link phandles of parent clocks (clk-ref and clk-bypass)
+- reg : address and length of the register set for controlling the ADPLL.
+
+Examples:
+   adpll_mpu_ck: adpll@40 {
+   #clock-cells = <1>;
+   compatible = "ti,dm814-adpll-s-clock";
+   reg = <0x40 0x40>;
+   clocks = <_ck _ck _ck>;
+   clock-names = "clkinp", "clkinpulow", "clkinphif";
+   clock-indices = <0>, <1>, <2>, <3>;
+   clock-output-names = "481c5040.adpll.dcoclkldo",
+"481c5040.adpll.clkout",
+"481c5040.adpll.clkoutx2",
+"481c5040.adpll.clkouthif";
+   };
+
+   adpll_dsp_ck: adpll@80 {
+   #clock-cells = <1>;
+   compatible = "ti,dm814-adpll-lj-clock";
+   reg = <0x80 0x30>;
+   clocks = <_ck _ck>;
+   clock-names = "clkinp", "clkinpulow";
+   clock-indices = <0>, <1>, <2>;
+   clock-output-names = "481c5080.adpll.clkdcoldo",
+"481c5080.adpll.clkout",
+"481c5080.adpll.clkoutldo";
+   };
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c3e3a02f..c0c9868 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -190,6 +190,7 @@ config COMMON_CLK_CDCE706

  source "drivers/clk/bcm/Kconfig"
  source "drivers/clk/hisilicon/Kconfig"
+source "drivers/clk/ti/Kconfig"
  source "drivers/clk/qcom/Kconfig"

  endmenu
diff --git a/drivers/clk/ti/Kconfig b/drivers/clk/ti/Kconfig
new file mode 100644
index 000..a9d5474
--- /dev/null
+++ b/drivers/clk/ti/Kconfig
@@ -0,0 +1,6 @@
+config COMMON_CLK_TI_ADPLL
+   tristate "Clock driver for dm814x ADPLL"
+   depends on ARCH_OMAP2PLUS
+   default y if SOC_TI81XX
+   ---help---
+ ADPLL clock driver for the dm814x SoC using common clock framework.
diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
index d4ac960..dfe91d7 100644
--- a/drivers/clk/ti/Makefile
+++ b/drivers/clk/ti/Makefile
@@ -18,3 +18,5 @@ obj-$(CONFIG_SOC_AM43XX)  += $(clk-common) 
dpll3xxx.o clk-43xx.o
  ifdef CONFIG_ATAGS
  obj-$(CONFIG_ARCH_OMAP3)+= clk-3xxx-legacy.o
  endif
+
+obj-$(CONFIG_COMMON_CLK_TI_ADPLL)  += adpll.o
diff --git a/drivers/clk/ti/adpll.c b/drivers/clk/ti/adpll.c
new file mode 100644
index 000..2c75c55
--- /dev/null
+++ b/drivers/clk/ti/adpll.c
@@ -0,0 +1,1028 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ *