[1/8] clk: keystone: add Keystone PLL clock driver

Commit Message

Santosh Shilimkar Aug. 5, 2013, 4:12 p.m. UTC

Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
IP typically has a multiplier, and a divider. The addtional PLL IPs like
ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
as the Main PLL is controlled by a PLL controller and memory map registers.
This difference is handle using 'has_pll_cntrl' property.

Cc: Mike Turquette <[e-mail]>

Signed-off-by: Santosh Shilimkar <[e-mail]>
---
 .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
 drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
 include/linux/clk/keystone.h                       |   18 ++
 3 files changed, 251 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
 create mode 100644 drivers/clk/keystone/pll.c
 create mode 100644 include/linux/clk/keystone.h

Comments

Mark Rutland Aug. 13, 2013, 3:48 p.m. UTC | #1

[Adding dt maintainers]

On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
> IP typically has a multiplier, and a divider. The addtional PLL IPs like
> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
> as the Main PLL is controlled by a PLL controller and memory map registers.
> This difference is handle using 'has_pll_cntrl' property.
> 
> Cc: Mike Turquette <[e-mail]>
> 
> Signed-off-by: Santosh Shilimkar <[e-mail]>
> ---
>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>  include/linux/clk/keystone.h                       |   18 ++
>  3 files changed, 251 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>  create mode 100644 drivers/clk/keystone/pll.c
>  create mode 100644 include/linux/clk/keystone.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> new file mode 100644
> index 0000000..58f6470
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> @@ -0,0 +1,36 @@
> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
> +and PAPLL are controlled by the memory mapped register where as the Main
> +PLL is controlled by a PLL controller. This difference is handle using
> +'pll_has_pllctrl' property.
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : shall be "keystone,pll-clk".

Keystone isn't a vendor, and generally, bindings have used "clock"
rather than "clk".

Perhaps "ti,keystone-pll-clock" ?

> +- #clock-cells : from common clock binding; shall be set to 0.
> +- clocks : parent clock phandle
> +- reg - index 0 -  PLLCTRL PLLM register address
> +- 	index 1 -  MAINPLL_CTL0 register address

Perhaps a reg-names would be useful?

> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register

Huh? I don't understand what that description means. What does the
property tell you? Is having one of the registers optional? If so that
should be described by a reg-names property associated with the reg, and
should be noted in the binding.

> +- pllm_lower_mask - pllm lower bit mask
> +- pllm_upper_mask - pllm upper bit mask
> +- plld_mask - plld mask
> +- fixed_postdiv - fixed post divider value

Please use '-' rather than '_' in property names, it's a standard
convention for dt and going against it just makes things unnecessarily
complicated.

Why are these necessary? Are clocks sharing common registers, are there
some sets of "keystone,pll-clk"s that have the same masks, or does this
just vary wildly?

Also, what types are these (presumably a single cell/u32)?

> +
> +Example:
> +	clock {
> +		#clock-cells = <0>;
> +		compatible = "keystone,pll-clk";
> +		clocks = <&refclk>;
> +		reg = <0x02310110 4	/* PLLCTRL PLLM */
> +			0x02620328 4>;	/* MAINPLL_CTL0 */
> +		pll_has_pllctrl;
> +		pllm_lower_mask	= <0x3f>;
> +		pllm_upper_mask = <0x7f000>;
> +		pllm_upper_shift = <6>;
> +		plld_mask = <0x3f>;
> +		fixed_postdiv = <2>;
> +	};
> diff --git a/drivers/clk/keystone/pll.c b/drivers/clk/keystone/pll.c
> new file mode 100644
> index 0000000..1453eea
> --- /dev/null
> +++ b/drivers/clk/keystone/pll.c
> @@ -0,0 +1,197 @@
> +/*
> + * Main PLL clk driver for Keystone devices
> + *
> + * Copyright (C) 2013 Texas Instruments Inc.
> + *	Murali Karicheri <[e-mail]>
> + *	Santosh Shilimkar <[e-mail]>
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/clk/keystone.h>
> +
> +/**
> + * struct clk_pll_data - pll data structure
> + * @has_pllctrl: If set to non zero, lower 6 bits of multiplier is in pllm
> + *	register of pll controller, else it is in the pll_ctrl0((bit 11-6)

The description in the binding doesn't make that clear at all. The
naming scheme is confusing -- because you've named the PLLCTRL PLLM
register pllm and the MAINPLL_CTL0 register pll_ctl0, it makes it look
like the logic around has_pllctrl is being used backwards throughout the
entire driver.

> + * @phy_pllm: Physical address of PLLM in pll controller. Used when
> + *	has_pllctrl is non zero.
> + * @phy_pll_ctl0: Physical address of PLL ctrl0. This could be that of
> + *	Main PLL or any other PLLs in the device such as ARM PLL, DDR PLL
> + *	or PA PLL available on keystone2. These PLLs are controlled by
> + *	this register. Main PLL is controlled by a PLL controller.
> + * @pllm: PLL register map address
> + * @pll_ctl0: PLL controller map address
> + * @pllm_lower_mask: multiplier lower mask
> + * @pllm_upper_mask: multiplier upper mask
> + * @pllm_upper_shift: multiplier upper shift
> + * @plld_mask: divider mask
> + * @fixed_postdiv: Post divider
> + */
> +struct clk_pll_data {
> +	unsigned char has_pllctrl;

bool?

> +	u32 phy_pllm;
> +	u32 phy_pll_ctl0;
> +	void __iomem *pllm;
> +	void __iomem *pll_ctl0;
> +	u32 pllm_lower_mask;
> +	u32 pllm_upper_mask;
> +	u32 pllm_upper_shift;
> +	u32 plld_mask;
> +	u32 fixed_postdiv;
> +};

[...]

> +/**
> + * of_keystone_pll_clk_init - PLL initialisation via DT
> + * @node: device tree node for this clock
> + */
> +void __init of_keystone_pll_clk_init(struct device_node *node)
> +{
> +	struct clk_pll_data *pll_data;
> +	const char *parent_name;
> +	struct clk *clk;
> +	int temp;
> +
> +	pll_data = kzalloc(sizeof(*pll_data), GFP_KERNEL);
> +	WARN_ON(!pll_data);

Why don't you return here, before dereferencing NULL below?

> +
> +	parent_name = of_clk_get_parent_name(node, 0);
> +
> +	if (of_find_property(node, "pll_has_pllctrl", &temp)) {

of_property_read_bool?

> +		/* PLL is controlled by the pllctrl */
> +		pll_data->has_pllctrl = 1;
> +		pll_data->pllm = of_iomap(node, 0);
> +		WARN_ON(!pll_data->pllm);
> +
> +		pll_data->pll_ctl0 = of_iomap(node, 1);
> +		WARN_ON(!pll_data->pll_ctl0);

Why do you not bail out if you couldn't map the registers you need?

> +
> +		if (of_property_read_u32(node, "pllm_lower_mask",
> +				&pll_data->pllm_lower_mask))
> +			goto out;
> +
> +	} else {
> +		/* PLL is controlled by the ctrl register */
> +		pll_data->has_pllctrl = 0;
> +		pll_data->pll_ctl0 = of_iomap(node, 0);

Huh? That's in a different order to the above (where pll_ctl0 was index
1 in the reg). I think you need to use reg-names for this, and come up
with a more scheme internal to the driver to minimise confusion.

> +	}
> +
> +	if (of_property_read_u32(node, "pllm_upper_mask",
> +			&pll_data->pllm_upper_mask))
> +		goto out;
> +
> +	if (of_property_read_u32(node, "pllm_upper_shift",
> +			&pll_data->pllm_upper_shift))
> +		goto out;
> +
> +	if (of_property_read_u32(node, "plld_mask", &pll_data->plld_mask))
> +		goto out;
> +
> +	if (of_property_read_u32(node, "fixed_postdiv",
> +					&pll_data->fixed_postdiv))
> +		goto out;
> +
> +	clk = clk_register_pll(NULL, node->name, parent_name,
> +					 pll_data);
> +	if (clk) {
> +		of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +		return;
> +	}
> +out:
> +	pr_err("of_keystone_pll_clk_init - error initializing clk %s\n",
> +		 node->name);
> +	kfree(pll_data);

You haven't unmapped either of the registers you may have mapped on your
way out, and you've thrown away your only reference to them.

Thanks,
Mark.

Santosh Shilimkar Aug. 13, 2013, 4:01 p.m. UTC | #2

On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
> [Adding dt maintainers]
> 
> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>> as the Main PLL is controlled by a PLL controller and memory map registers.
>> This difference is handle using 'has_pll_cntrl' property.
>>
>> Cc: Mike Turquette <[e-mail]>
>>
>> Signed-off-by: Santosh Shilimkar <[e-mail]>
>> ---

Thanks for review Mark.

>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>  include/linux/clk/keystone.h                       |   18 ++
>>  3 files changed, 251 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>  create mode 100644 drivers/clk/keystone/pll.c
>>  create mode 100644 include/linux/clk/keystone.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>> new file mode 100644
>> index 0000000..58f6470
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>> @@ -0,0 +1,36 @@
>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>> +and PAPLL are controlled by the memory mapped register where as the Main
>> +PLL is controlled by a PLL controller. This difference is handle using
>> +'pll_has_pllctrl' property.
>> +
>> +This binding uses the common clock binding[1].
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible : shall be "keystone,pll-clk".
> 
> Keystone isn't a vendor, and generally, bindings have used "clock"
> rather than "clk".
> 
> Perhaps "ti,keystone-pll-clock" ?
> 
Agree.

>> +- #clock-cells : from common clock binding; shall be set to 0.
>> +- clocks : parent clock phandle
>> +- reg - index 0 -  PLLCTRL PLLM register address
>> +- 	index 1 -  MAINPLL_CTL0 register address
> 
> Perhaps a reg-names would be useful?
> 
>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
> 
> Huh? I don't understand what that description means. What does the
> property tell you? Is having one of the registers optional? If so that
> should be described by a reg-names property associated with the reg, and
> should be noted in the binding.
> 
After re-reading it, yes I agree it not clear. The point is there
are two different types of IPs and pogramming model changes quite
a bit. Its not just 1 register optional.

>> +- pllm_lower_mask - pllm lower bit mask
>> +- pllm_upper_mask - pllm upper bit mask
>> +- plld_mask - plld mask
>> +- fixed_postdiv - fixed post divider value
> 
> Please use '-' rather than '_' in property names, it's a standard
> convention for dt and going against it just makes things unnecessarily
> complicated.
> 
> Why are these necessary? Are clocks sharing common registers, are there
> some sets of "keystone,pll-clk"s that have the same masks, or does this
> just vary wildly?
> 
This is mainly to take care of the programming model which varies between
IPs. Will try to make that bit more clear.

> Also, what types are these (presumably a single cell/u32)?
> 
u32

>> +
>> +Example:
>> +	clock {
>> +		#clock-cells = <0>;
>> +		compatible = "keystone,pll-clk";
>> +		clocks = <&refclk>;
>> +		reg = <0x02310110 4	/* PLLCTRL PLLM */
>> +			0x02620328 4>;	/* MAINPLL_CTL0 */
>> +		pll_has_pllctrl;
>> +		pllm_lower_mask	= <0x3f>;
>> +		pllm_upper_mask = <0x7f000>;
>> +		pllm_upper_shift = <6>;
>> +		plld_mask = <0x3f>;
>> +		fixed_postdiv = <2>;
>> +	};
>> diff --git a/drivers/clk/keystone/pll.c b/drivers/clk/keystone/pll.c
>> new file mode 100644
>> index 0000000..1453eea
>> --- /dev/null
>> +++ b/drivers/clk/keystone/pll.c
>> @@ -0,0 +1,197 @@
>> +/*
>> + * Main PLL clk driver for Keystone devices
>> + *
>> + * Copyright (C) 2013 Texas Instruments Inc.
>> + *	Murali Karicheri <[e-mail]>
>> + *	Santosh Shilimkar <[e-mail]>
>> + *
>> + * 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; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of.h>
>> +#include <linux/module.h>
>> +#include <linux/clk/keystone.h>
>> +
>> +/**
>> + * struct clk_pll_data - pll data structure
>> + * @has_pllctrl: If set to non zero, lower 6 bits of multiplier is in pllm
>> + *	register of pll controller, else it is in the pll_ctrl0((bit 11-6)
> 
> The description in the binding doesn't make that clear at all. The
> naming scheme is confusing -- because you've named the PLLCTRL PLLM
> register pllm and the MAINPLL_CTL0 register pll_ctl0, it makes it look
> like the logic around has_pllctrl is being used backwards throughout the
> entire driver.
>
This is due to difference in the IPs. Naming scheme can be improved
though. Was bit lazy to not do that firstplace.
 
>> + * @phy_pllm: Physical address of PLLM in pll controller. Used when
>> + *	has_pllctrl is non zero.
>> + * @phy_pll_ctl0: Physical address of PLL ctrl0. This could be that of
>> + *	Main PLL or any other PLLs in the device such as ARM PLL, DDR PLL
>> + *	or PA PLL available on keystone2. These PLLs are controlled by
>> + *	this register. Main PLL is controlled by a PLL controller.
>> + * @pllm: PLL register map address
>> + * @pll_ctl0: PLL controller map address
>> + * @pllm_lower_mask: multiplier lower mask
>> + * @pllm_upper_mask: multiplier upper mask
>> + * @pllm_upper_shift: multiplier upper shift
>> + * @plld_mask: divider mask
>> + * @fixed_postdiv: Post divider
>> + */
>> +struct clk_pll_data {
>> +	unsigned char has_pllctrl;
> 
> bool?
Yes

> 
>> +	u32 phy_pllm;
>> +	u32 phy_pll_ctl0;
>> +	void __iomem *pllm;
>> +	void __iomem *pll_ctl0;
>> +	u32 pllm_lower_mask;
>> +	u32 pllm_upper_mask;
>> +	u32 pllm_upper_shift;
>> +	u32 plld_mask;
>> +	u32 fixed_postdiv;
>> +};
> 
> [...]
> 
>> +/**
>> + * of_keystone_pll_clk_init - PLL initialisation via DT
>> + * @node: device tree node for this clock
>> + */
>> +void __init of_keystone_pll_clk_init(struct device_node *node)
>> +{
>> +	struct clk_pll_data *pll_data;
>> +	const char *parent_name;
>> +	struct clk *clk;
>> +	int temp;
>> +
>> +	pll_data = kzalloc(sizeof(*pll_data), GFP_KERNEL);
>> +	WARN_ON(!pll_data);
> 
> Why don't you return here, before dereferencing NULL below?
> 
>> +
>> +	parent_name = of_clk_get_parent_name(node, 0);
>> +
>> +	if (of_find_property(node, "pll_has_pllctrl", &temp)) {
> 
> of_property_read_bool?
> 
>> +		/* PLL is controlled by the pllctrl */
>> +		pll_data->has_pllctrl = 1;
>> +		pll_data->pllm = of_iomap(node, 0);
>> +		WARN_ON(!pll_data->pllm);
>> +
>> +		pll_data->pll_ctl0 = of_iomap(node, 1);
>> +		WARN_ON(!pll_data->pll_ctl0);
> 
> Why do you not bail out if you couldn't map the registers you need?
> 
>> +
>> +		if (of_property_read_u32(node, "pllm_lower_mask",
>> +				&pll_data->pllm_lower_mask))
>> +			goto out;
>> +
>> +	} else {
>> +		/* PLL is controlled by the ctrl register */
>> +		pll_data->has_pllctrl = 0;
>> +		pll_data->pll_ctl0 = of_iomap(node, 0);
> 
> Huh? That's in a different order to the above (where pll_ctl0 was index
> 1 in the reg). I think you need to use reg-names for this, and come up
> with a more scheme internal to the driver to minimise confusion.
> 
Agree. 

>> +	}
>> +
>> +	if (of_property_read_u32(node, "pllm_upper_mask",
>> +			&pll_data->pllm_upper_mask))
>> +		goto out;
>> +
>> +	if (of_property_read_u32(node, "pllm_upper_shift",
>> +			&pll_data->pllm_upper_shift))
>> +		goto out;
>> +
>> +	if (of_property_read_u32(node, "plld_mask", &pll_data->plld_mask))
>> +		goto out;
>> +
>> +	if (of_property_read_u32(node, "fixed_postdiv",
>> +					&pll_data->fixed_postdiv))
>> +		goto out;
>> +
>> +	clk = clk_register_pll(NULL, node->name, parent_name,
>> +					 pll_data);
>> +	if (clk) {
>> +		of_clk_add_provider(node, of_clk_src_simple_get, clk);
>> +		return;
>> +	}
>> +out:
>> +	pr_err("of_keystone_pll_clk_init - error initializing clk %s\n",
>> +		 node->name);
>> +	kfree(pll_data);
> 
> You haven't unmapped either of the registers you may have mapped on your
> way out, and you've thrown away your only reference to them.
> 
Yep.

Will address your comments in next version.

Regards,
Santosh

Mark Rutland Aug. 13, 2013, 4:47 p.m. UTC | #3

On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
> > [Adding dt maintainers]
> > 
> > On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
> >> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
> >> IP typically has a multiplier, and a divider. The addtional PLL IPs like
> >> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
> >> as the Main PLL is controlled by a PLL controller and memory map registers.
> >> This difference is handle using 'has_pll_cntrl' property.
> >>
> >> Cc: Mike Turquette <[e-mail]>
> >>
> >> Signed-off-by: Santosh Shilimkar <[e-mail]>
> >> ---
> 
> Thanks for review Mark.
> 
> >>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
> >>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
> >>  include/linux/clk/keystone.h                       |   18 ++
> >>  3 files changed, 251 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>  create mode 100644 drivers/clk/keystone/pll.c
> >>  create mode 100644 include/linux/clk/keystone.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >> new file mode 100644
> >> index 0000000..58f6470
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >> @@ -0,0 +1,36 @@
> >> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
> >> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
> >> +and PAPLL are controlled by the memory mapped register where as the Main
> >> +PLL is controlled by a PLL controller. This difference is handle using
> >> +'pll_has_pllctrl' property.
> >> +
> >> +This binding uses the common clock binding[1].
> >> +
> >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +
> >> +Required properties:
> >> +- compatible : shall be "keystone,pll-clk".
> > 
> > Keystone isn't a vendor, and generally, bindings have used "clock"
> > rather than "clk".
> > 
> > Perhaps "ti,keystone-pll-clock" ?
> > 
> Agree.
> 
> >> +- #clock-cells : from common clock binding; shall be set to 0.
> >> +- clocks : parent clock phandle
> >> +- reg - index 0 -  PLLCTRL PLLM register address
> >> +- 	index 1 -  MAINPLL_CTL0 register address
> > 
> > Perhaps a reg-names would be useful?
> > 
> >> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
> > 
> > Huh? I don't understand what that description means. What does the
> > property tell you? Is having one of the registers optional? If so that
> > should be described by a reg-names property associated with the reg, and
> > should be noted in the binding.
> > 
> After re-reading it, yes I agree it not clear. The point is there
> are two different types of IPs and pogramming model changes quite
> a bit. Its not just 1 register optional.

If that's the case, then having different compatible strings would make
sense.

> 
> >> +- pllm_lower_mask - pllm lower bit mask
> >> +- pllm_upper_mask - pllm upper bit mask
> >> +- plld_mask - plld mask
> >> +- fixed_postdiv - fixed post divider value
> > 
> > Please use '-' rather than '_' in property names, it's a standard
> > convention for dt and going against it just makes things unnecessarily
> > complicated.
> > 
> > Why are these necessary? Are clocks sharing common registers, are there
> > some sets of "keystone,pll-clk"s that have the same masks, or does this
> > just vary wildly?
> > 
> This is mainly to take care of the programming model which varies between
> IPs. Will try to make that bit more clear.

Are there more than the two IPs mentioned above? If they had separate
compatible strings, would that give enough information implicitly,
without the precise masks needing to be in the dt?

> 
> > Also, what types are these (presumably a single cell/u32)?
> > 
> u32

Ok, please could you document that in the binding?

Cheers,
Mark.

Santosh Shilimkar Aug. 13, 2013, 4:58 p.m. UTC | #4

On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
>>> [Adding dt maintainers]
>>>
>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
>>>> This difference is handle using 'has_pll_cntrl' property.
>>>>
>>>> Cc: Mike Turquette <[e-mail]>
>>>>
>>>> Signed-off-by: Santosh Shilimkar <[e-mail]>
>>>> ---
>>
>> Thanks for review Mark.
>>
>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>>>  include/linux/clk/keystone.h                       |   18 ++
>>>>  3 files changed, 251 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>  create mode 100644 drivers/clk/keystone/pll.c
>>>>  create mode 100644 include/linux/clk/keystone.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>> new file mode 100644
>>>> index 0000000..58f6470
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>> @@ -0,0 +1,36 @@
>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>>>> +and PAPLL are controlled by the memory mapped register where as the Main
>>>> +PLL is controlled by a PLL controller. This difference is handle using
>>>> +'pll_has_pllctrl' property.
>>>> +
>>>> +This binding uses the common clock binding[1].
>>>> +
>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> +
>>>> +Required properties:
>>>> +- compatible : shall be "keystone,pll-clk".
>>>
>>> Keystone isn't a vendor, and generally, bindings have used "clock"
>>> rather than "clk".
>>>
>>> Perhaps "ti,keystone-pll-clock" ?
>>>
>> Agree.
>>
>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>> +- clocks : parent clock phandle
>>>> +- reg - index 0 -  PLLCTRL PLLM register address
>>>> +- 	index 1 -  MAINPLL_CTL0 register address
>>>
>>> Perhaps a reg-names would be useful?
>>>
>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
>>>
>>> Huh? I don't understand what that description means. What does the
>>> property tell you? Is having one of the registers optional? If so that
>>> should be described by a reg-names property associated with the reg, and
>>> should be noted in the binding.
>>>
>> After re-reading it, yes I agree it not clear. The point is there
>> are two different types of IPs and pogramming model changes quite
>> a bit. Its not just 1 register optional.
> 
> If that's the case, then having different compatible strings would make
> sense.
> 
>>
>>>> +- pllm_lower_mask - pllm lower bit mask
>>>> +- pllm_upper_mask - pllm upper bit mask
>>>> +- plld_mask - plld mask
>>>> +- fixed_postdiv - fixed post divider value
>>>
>>> Please use '-' rather than '_' in property names, it's a standard
>>> convention for dt and going against it just makes things unnecessarily
>>> complicated.
>>>
>>> Why are these necessary? Are clocks sharing common registers, are there
>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
>>> just vary wildly?
>>>
>> This is mainly to take care of the programming model which varies between
>> IPs. Will try to make that bit more clear.
> 
> Are there more than the two IPs mentioned above? If they had separate
> compatible strings, would that give enough information implicitly,
> without the precise masks needing to be in the dt?
> 
I will explore the separate compatible option. Thanks for suggestion.

Regards,
Santosh

Santosh Shilimkar Aug. 19, 2013, 5:42 p.m. UTC | #5

Mark,

On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
> On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
>> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
>>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
>>>> [Adding dt maintainers]
>>>>
>>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
>>>>> This difference is handle using 'has_pll_cntrl' property.
>>>>>
>>>>> Cc: Mike Turquette <[e-mail]>
>>>>>
>>>>> Signed-off-by: Santosh Shilimkar <[e-mail]>
>>>>> ---
>>>
>>> Thanks for review Mark.
>>>
>>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>>>>  include/linux/clk/keystone.h                       |   18 ++
>>>>>  3 files changed, 251 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>  create mode 100644 drivers/clk/keystone/pll.c
>>>>>  create mode 100644 include/linux/clk/keystone.h
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>> new file mode 100644
>>>>> index 0000000..58f6470
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>> @@ -0,0 +1,36 @@
>>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>>>>> +and PAPLL are controlled by the memory mapped register where as the Main
>>>>> +PLL is controlled by a PLL controller. This difference is handle using
>>>>> +'pll_has_pllctrl' property.
>>>>> +
>>>>> +This binding uses the common clock binding[1].
>>>>> +
>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : shall be "keystone,pll-clk".
>>>>
>>>> Keystone isn't a vendor, and generally, bindings have used "clock"
>>>> rather than "clk".
>>>>
>>>> Perhaps "ti,keystone-pll-clock" ?
>>>>
>>> Agree.
>>>
>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>> +- clocks : parent clock phandle
>>>>> +- reg - index 0 -  PLLCTRL PLLM register address
>>>>> +- 	index 1 -  MAINPLL_CTL0 register address
>>>>
>>>> Perhaps a reg-names would be useful?
>>>>
>>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
>>>>
>>>> Huh? I don't understand what that description means. What does the
>>>> property tell you? Is having one of the registers optional? If so that
>>>> should be described by a reg-names property associated with the reg, and
>>>> should be noted in the binding.
>>>>
>>> After re-reading it, yes I agree it not clear. The point is there
>>> are two different types of IPs and pogramming model changes quite
>>> a bit. Its not just 1 register optional.
>>
>> If that's the case, then having different compatible strings would make
>> sense.
>>
>>>
>>>>> +- pllm_lower_mask - pllm lower bit mask
>>>>> +- pllm_upper_mask - pllm upper bit mask
>>>>> +- plld_mask - plld mask
>>>>> +- fixed_postdiv - fixed post divider value
>>>>
>>>> Please use '-' rather than '_' in property names, it's a standard
>>>> convention for dt and going against it just makes things unnecessarily
>>>> complicated.
>>>>
>>>> Why are these necessary? Are clocks sharing common registers, are there
>>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
>>>> just vary wildly?
>>>>
>>> This is mainly to take care of the programming model which varies between
>>> IPs. Will try to make that bit more clear.
>>
>> Are there more than the two IPs mentioned above? If they had separate
>> compatible strings, would that give enough information implicitly,
>> without the precise masks needing to be in the dt?
>>
> I will explore the separate compatible option. Thanks for suggestion.
> 
I looked at further into separate compatible option or reg-names
to check if it can help to reduce some additional dt information.
Actually it doesn't help much. The base programming model is shared
between both types of PLL IPs. The PLLs which has PLL controller along
with MMRs, has to take into account additional bit-fields for the
multiplier and divider along with the base model multiplier and divider
registers.

Having said that, there are few parameters which are fixed like
plld_mask, pllm_upper_shift etc need not come from DT. I am going
to remove them from dt bindings and also make the reg index consistent
as it should have been in first place.

Rest of the comments are fine and will be addressed in next version.

Regards,
Santosh

Mike Turquette Aug. 19, 2013, 8:33 p.m. UTC | #6

Quoting Santosh Shilimkar (2013-08-19 10:42:19)
> Mark,
> 
> On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
> > On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
> >> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
> >>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
> >>>> [Adding dt maintainers]
> >>>>
> >>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
> >>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
> >>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
> >>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
> >>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
> >>>>> This difference is handle using 'has_pll_cntrl' property.
> >>>>>
> >>>>> Cc: Mike Turquette <[e-mail]>
> >>>>>
> >>>>> Signed-off-by: Santosh Shilimkar <[e-mail]>
> >>>>> ---
> >>>
> >>> Thanks for review Mark.
> >>>
> >>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
> >>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
> >>>>>  include/linux/clk/keystone.h                       |   18 ++
> >>>>>  3 files changed, 251 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>>  create mode 100644 drivers/clk/keystone/pll.c
> >>>>>  create mode 100644 include/linux/clk/keystone.h
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>> new file mode 100644
> >>>>> index 0000000..58f6470
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>> @@ -0,0 +1,36 @@
> >>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
> >>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
> >>>>> +and PAPLL are controlled by the memory mapped register where as the Main
> >>>>> +PLL is controlled by a PLL controller. This difference is handle using
> >>>>> +'pll_has_pllctrl' property.
> >>>>> +
> >>>>> +This binding uses the common clock binding[1].
> >>>>> +
> >>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>> +
> >>>>> +Required properties:
> >>>>> +- compatible : shall be "keystone,pll-clk".
> >>>>
> >>>> Keystone isn't a vendor, and generally, bindings have used "clock"
> >>>> rather than "clk".
> >>>>
> >>>> Perhaps "ti,keystone-pll-clock" ?
> >>>>
> >>> Agree.
> >>>
> >>>>> +- #clock-cells : from common clock binding; shall be set to 0.
> >>>>> +- clocks : parent clock phandle
> >>>>> +- reg - index 0 -  PLLCTRL PLLM register address
> >>>>> +-        index 1 -  MAINPLL_CTL0 register address
> >>>>
> >>>> Perhaps a reg-names would be useful?
> >>>>
> >>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
> >>>>
> >>>> Huh? I don't understand what that description means. What does the
> >>>> property tell you? Is having one of the registers optional? If so that
> >>>> should be described by a reg-names property associated with the reg, and
> >>>> should be noted in the binding.
> >>>>
> >>> After re-reading it, yes I agree it not clear. The point is there
> >>> are two different types of IPs and pogramming model changes quite
> >>> a bit. Its not just 1 register optional.
> >>
> >> If that's the case, then having different compatible strings would make
> >> sense.
> >>
> >>>
> >>>>> +- pllm_lower_mask - pllm lower bit mask
> >>>>> +- pllm_upper_mask - pllm upper bit mask
> >>>>> +- plld_mask - plld mask
> >>>>> +- fixed_postdiv - fixed post divider value
> >>>>
> >>>> Please use '-' rather than '_' in property names, it's a standard
> >>>> convention for dt and going against it just makes things unnecessarily
> >>>> complicated.
> >>>>
> >>>> Why are these necessary? Are clocks sharing common registers, are there
> >>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
> >>>> just vary wildly?
> >>>>
> >>> This is mainly to take care of the programming model which varies between
> >>> IPs. Will try to make that bit more clear.
> >>
> >> Are there more than the two IPs mentioned above? If they had separate
> >> compatible strings, would that give enough information implicitly,
> >> without the precise masks needing to be in the dt?
> >>
> > I will explore the separate compatible option. Thanks for suggestion.
> > 
> I looked at further into separate compatible option or reg-names
> to check if it can help to reduce some additional dt information.
> Actually it doesn't help much. The base programming model is shared
> between both types of PLL IPs. The PLLs which has PLL controller along
> with MMRs, has to take into account additional bit-fields for the
> multiplier and divider along with the base model multiplier and divider
> registers.

It is common for the base programming model to be similar or shared
between two different compatible strings. You can use the same clk_ops
for clk_prepare, clk_enable, etc.

The point is to use the different compatible strings to encode the
differences in *data* between the two clock types. That way you have
fewer properties to list in the binding since two separate setup
functions can implicitly handle the differences in initializing the
per-clock data.

> 
> Having said that, there are few parameters which are fixed like
> plld_mask, pllm_upper_shift etc need not come from DT. I am going
> to remove them from dt bindings and also make the reg index consistent
> as it should have been in first place.

This is nice. Note that these things may change in future Keystone
versions because hardware people hate you and want to make your life
hard. So the compatible string might benefit from including the first
keystone part number that uses this binding (e.g.
ti,keystone-2420-pll-clock, which I just made up).

In the future when the register layout, offsets and masks change for
absolutely no reason (but the programming model is the same) then you
can just write a new binding that setups up your private clk_pll_data
struct without having to jam all of those details into DT (e.g.
ti,keystone-3430-pll-clock, which I also just made up).

Regards,
Mike

> 
> Rest of the comments are fine and will be addressed in next version.
> 
> Regards,
> Santosh

Santosh Shilimkar Aug. 20, 2013, 1:41 p.m. UTC | #7

On Monday 19 August 2013 04:33 PM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-19 10:42:19)
>> Mark,
>>
>> On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
>>> On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
>>>> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
>>>>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
>>>>>> [Adding dt maintainers]
>>>>>>
>>>>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>>>>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>>>>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>>>>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>>>>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
>>>>>>> This difference is handle using 'has_pll_cntrl' property.
>>>>>>>
>>>>>>> Cc: Mike Turquette <[e-mail]>
>>>>>>>
>>>>>>> Signed-off-by: Santosh Shilimkar <[e-mail]>
>>>>>>> ---
>>>>>
>>>>> Thanks for review Mark.
>>>>>
>>>>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>>>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>>>>>>  include/linux/clk/keystone.h                       |   18 ++
>>>>>>>  3 files changed, 251 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>>  create mode 100644 drivers/clk/keystone/pll.c
>>>>>>>  create mode 100644 include/linux/clk/keystone.h
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..58f6470
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>> @@ -0,0 +1,36 @@
>>>>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>>>>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>>>>>>> +and PAPLL are controlled by the memory mapped register where as the Main
>>>>>>> +PLL is controlled by a PLL controller. This difference is handle using
>>>>>>> +'pll_has_pllctrl' property.
>>>>>>> +
>>>>>>> +This binding uses the common clock binding[1].
>>>>>>> +
>>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible : shall be "keystone,pll-clk".
>>>>>>
>>>>>> Keystone isn't a vendor, and generally, bindings have used "clock"
>>>>>> rather than "clk".
>>>>>>
>>>>>> Perhaps "ti,keystone-pll-clock" ?
>>>>>>
>>>>> Agree.
>>>>>
>>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>>>> +- clocks : parent clock phandle
>>>>>>> +- reg - index 0 -  PLLCTRL PLLM register address
>>>>>>> +-        index 1 -  MAINPLL_CTL0 register address
>>>>>>
>>>>>> Perhaps a reg-names would be useful?
>>>>>>
>>>>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
>>>>>>
>>>>>> Huh? I don't understand what that description means. What does the
>>>>>> property tell you? Is having one of the registers optional? If so that
>>>>>> should be described by a reg-names property associated with the reg, and
>>>>>> should be noted in the binding.
>>>>>>
>>>>> After re-reading it, yes I agree it not clear. The point is there
>>>>> are two different types of IPs and pogramming model changes quite
>>>>> a bit. Its not just 1 register optional.
>>>>
>>>> If that's the case, then having different compatible strings would make
>>>> sense.
>>>>
>>>>>
>>>>>>> +- pllm_lower_mask - pllm lower bit mask
>>>>>>> +- pllm_upper_mask - pllm upper bit mask
>>>>>>> +- plld_mask - plld mask
>>>>>>> +- fixed_postdiv - fixed post divider value
>>>>>>
>>>>>> Please use '-' rather than '_' in property names, it's a standard
>>>>>> convention for dt and going against it just makes things unnecessarily
>>>>>> complicated.
>>>>>>
>>>>>> Why are these necessary? Are clocks sharing common registers, are there
>>>>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
>>>>>> just vary wildly?
>>>>>>
>>>>> This is mainly to take care of the programming model which varies between
>>>>> IPs. Will try to make that bit more clear.
>>>>
>>>> Are there more than the two IPs mentioned above? If they had separate
>>>> compatible strings, would that give enough information implicitly,
>>>> without the precise masks needing to be in the dt?
>>>>
>>> I will explore the separate compatible option. Thanks for suggestion.
>>>
>> I looked at further into separate compatible option or reg-names
>> to check if it can help to reduce some additional dt information.
>> Actually it doesn't help much. The base programming model is shared
>> between both types of PLL IPs. The PLLs which has PLL controller along
>> with MMRs, has to take into account additional bit-fields for the
>> multiplier and divider along with the base model multiplier and divider
>> registers.
> 
> It is common for the base programming model to be similar or shared
> between two different compatible strings. You can use the same clk_ops
> for clk_prepare, clk_enable, etc.
>
> The point is to use the different compatible strings to encode the
> differences in *data* between the two clock types. That way you have
> fewer properties to list in the binding since two separate setup
> functions can implicitly handle the differences in initializing the
> per-clock data.
> 
Thats the point I came back. Both PLL share the properties and
the main PLL brings in couple of properties to extend the divider
field and that was handled through the flag. As mentioned below,
some properties like plld_mask, pllm_upper_shift etc can be
dropped since they are static values.

>>
>> Having said that, there are few parameters which are fixed like
>> plld_mask, pllm_upper_shift etc need not come from DT. I am going
>> to remove them from dt bindings and also make the reg index consistent
>> as it should have been in first place.
> 
> This is nice. Note that these things may change in future Keystone
> versions because hardware people hate you and want to make your life
> hard. So the compatible string might benefit from including the first
> keystone part number that uses this binding (e.g.
> ti,keystone-2420-pll-clock, which I just made up).
> 
> In the future when the register layout, offsets and masks change for
> absolutely no reason (but the programming model is the same) then you
> can just write a new binding that setups up your private clk_pll_data
> struct without having to jam all of those details into DT (e.g.
> ti,keystone-3430-pll-clock, which I also just made up).
> 
So thats the good part so far with the keystone where the compatibility
is maintained pretty well. I know background of your comments ;-)
I will keep the bindings without any direct device association for
now considering I don't foresee any thing changing in that aspect.

Regards,
Santosh

Mike Turquette Aug. 20, 2013, 9:23 p.m. UTC | #8

Quoting Santosh Shilimkar (2013-08-20 06:41:21)
> On Monday 19 August 2013 04:33 PM, Mike Turquette wrote:
> > Quoting Santosh Shilimkar (2013-08-19 10:42:19)
> >> Mark,
> >>
> >> On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
> >>> On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
> >>>> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
> >>>>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
> >>>>>> [Adding dt maintainers]
> >>>>>>
> >>>>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
> >>>>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
> >>>>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
> >>>>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
> >>>>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
> >>>>>>> This difference is handle using 'has_pll_cntrl' property.
> >>>>>>>
> >>>>>>> Cc: Mike Turquette <[e-mail]>
> >>>>>>>
> >>>>>>> Signed-off-by: Santosh Shilimkar <[e-mail]>
> >>>>>>> ---
> >>>>>
> >>>>> Thanks for review Mark.
> >>>>>
> >>>>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
> >>>>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
> >>>>>>>  include/linux/clk/keystone.h                       |   18 ++
> >>>>>>>  3 files changed, 251 insertions(+)
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>>>>  create mode 100644 drivers/clk/keystone/pll.c
> >>>>>>>  create mode 100644 include/linux/clk/keystone.h
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000..58f6470
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
> >>>>>>> @@ -0,0 +1,36 @@
> >>>>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
> >>>>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
> >>>>>>> +and PAPLL are controlled by the memory mapped register where as the Main
> >>>>>>> +PLL is controlled by a PLL controller. This difference is handle using
> >>>>>>> +'pll_has_pllctrl' property.
> >>>>>>> +
> >>>>>>> +This binding uses the common clock binding[1].
> >>>>>>> +
> >>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>>>>> +
> >>>>>>> +Required properties:
> >>>>>>> +- compatible : shall be "keystone,pll-clk".
> >>>>>>
> >>>>>> Keystone isn't a vendor, and generally, bindings have used "clock"
> >>>>>> rather than "clk".
> >>>>>>
> >>>>>> Perhaps "ti,keystone-pll-clock" ?
> >>>>>>
> >>>>> Agree.
> >>>>>
> >>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
> >>>>>>> +- clocks : parent clock phandle
> >>>>>>> +- reg - index 0 -  PLLCTRL PLLM register address
> >>>>>>> +-        index 1 -  MAINPLL_CTL0 register address
> >>>>>>
> >>>>>> Perhaps a reg-names would be useful?
> >>>>>>
> >>>>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
> >>>>>>
> >>>>>> Huh? I don't understand what that description means. What does the
> >>>>>> property tell you? Is having one of the registers optional? If so that
> >>>>>> should be described by a reg-names property associated with the reg, and
> >>>>>> should be noted in the binding.
> >>>>>>
> >>>>> After re-reading it, yes I agree it not clear. The point is there
> >>>>> are two different types of IPs and pogramming model changes quite
> >>>>> a bit. Its not just 1 register optional.
> >>>>
> >>>> If that's the case, then having different compatible strings would make
> >>>> sense.
> >>>>
> >>>>>
> >>>>>>> +- pllm_lower_mask - pllm lower bit mask
> >>>>>>> +- pllm_upper_mask - pllm upper bit mask
> >>>>>>> +- plld_mask - plld mask
> >>>>>>> +- fixed_postdiv - fixed post divider value
> >>>>>>
> >>>>>> Please use '-' rather than '_' in property names, it's a standard
> >>>>>> convention for dt and going against it just makes things unnecessarily
> >>>>>> complicated.
> >>>>>>
> >>>>>> Why are these necessary? Are clocks sharing common registers, are there
> >>>>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
> >>>>>> just vary wildly?
> >>>>>>
> >>>>> This is mainly to take care of the programming model which varies between
> >>>>> IPs. Will try to make that bit more clear.
> >>>>
> >>>> Are there more than the two IPs mentioned above? If they had separate
> >>>> compatible strings, would that give enough information implicitly,
> >>>> without the precise masks needing to be in the dt?
> >>>>
> >>> I will explore the separate compatible option. Thanks for suggestion.
> >>>
> >> I looked at further into separate compatible option or reg-names
> >> to check if it can help to reduce some additional dt information.
> >> Actually it doesn't help much. The base programming model is shared
> >> between both types of PLL IPs. The PLLs which has PLL controller along
> >> with MMRs, has to take into account additional bit-fields for the
> >> multiplier and divider along with the base model multiplier and divider
> >> registers.
> > 
> > It is common for the base programming model to be similar or shared
> > between two different compatible strings. You can use the same clk_ops
> > for clk_prepare, clk_enable, etc.
> >
> > The point is to use the different compatible strings to encode the
> > differences in *data* between the two clock types. That way you have
> > fewer properties to list in the binding since two separate setup
> > functions can implicitly handle the differences in initializing the
> > per-clock data.
> > 
> Thats the point I came back. Both PLL share the properties and
> the main PLL brings in couple of properties to extend the divider
> field and that was handled through the flag. As mentioned below,
> some properties like plld_mask, pllm_upper_shift etc can be
> dropped since they are static values.
> 
> >>
> >> Having said that, there are few parameters which are fixed like
> >> plld_mask, pllm_upper_shift etc need not come from DT. I am going
> >> to remove them from dt bindings and also make the reg index consistent
> >> as it should have been in first place.
> > 
> > This is nice. Note that these things may change in future Keystone
> > versions because hardware people hate you and want to make your life
> > hard. So the compatible string might benefit from including the first
> > keystone part number that uses this binding (e.g.
> > ti,keystone-2420-pll-clock, which I just made up).
> > 
> > In the future when the register layout, offsets and masks change for
> > absolutely no reason (but the programming model is the same) then you
> > can just write a new binding that setups up your private clk_pll_data
> > struct without having to jam all of those details into DT (e.g.
> > ti,keystone-3430-pll-clock, which I also just made up).
> > 
> So thats the good part so far with the keystone where the compatibility
> is maintained pretty well. I know background of your comments ;-)
> I will keep the bindings without any direct device association for
> now considering I don't foresee any thing changing in that aspect.

Sure. It's just a suggestion. Once the ABI is set it isn't supposed to
change so I'm just trying to think ahead.

Also there has been discussion on marking bindings as "unstable". I
don't know if you are interested in that but maybe putting a line at the
top of the binding stating something like:

Status: Unstable - ABI compatibility may be broken in the future

Regards,
Mike

> 
> Regards,
> Santosh

Santosh Shilimkar Aug. 20, 2013, 9:46 p.m. UTC | #9

On Tuesday 20 August 2013 05:23 PM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-20 06:41:21)
>> On Monday 19 August 2013 04:33 PM, Mike Turquette wrote:
>>> Quoting Santosh Shilimkar (2013-08-19 10:42:19)
>>>> Mark,
>>>>
>>>> On Tuesday 13 August 2013 12:58 PM, Santosh Shilimkar wrote:
>>>>> On Tuesday 13 August 2013 12:47 PM, Mark Rutland wrote:
>>>>>> On Tue, Aug 13, 2013 at 05:01:59PM +0100, Santosh Shilimkar wrote:
>>>>>>> On Tuesday 13 August 2013 11:48 AM, Mark Rutland wrote:
>>>>>>>> [Adding dt maintainers]
>>>>>>>>
>>>>>>>> On Mon, Aug 05, 2013 at 05:12:20PM +0100, Santosh Shilimkar wrote:
>>>>>>>>> Add the driver for the PLL IPs found on Keystone 2 devices. The main PLL
>>>>>>>>> IP typically has a multiplier, and a divider. The addtional PLL IPs like
>>>>>>>>> ARMPLL, DDRPLL and PAPLL are controlled by the memory mapped register where
>>>>>>>>> as the Main PLL is controlled by a PLL controller and memory map registers.
>>>>>>>>> This difference is handle using 'has_pll_cntrl' property.
>>>>>>>>>
>>>>>>>>> Cc: Mike Turquette <[e-mail]>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Santosh Shilimkar <[e-mail]>
>>>>>>>>> ---
>>>>>>>
>>>>>>> Thanks for review Mark.
>>>>>>>
>>>>>>>>>  .../devicetree/bindings/clock/keystone-pll.txt     |   36 ++++
>>>>>>>>>  drivers/clk/keystone/pll.c                         |  197 ++++++++++++++++++++
>>>>>>>>>  include/linux/clk/keystone.h                       |   18 ++
>>>>>>>>>  3 files changed, 251 insertions(+)
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>>>>  create mode 100644 drivers/clk/keystone/pll.c
>>>>>>>>>  create mode 100644 include/linux/clk/keystone.h
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..58f6470
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
>>>>>>>>> @@ -0,0 +1,36 @@
>>>>>>>>> +Binding for keystone PLLs. The main PLL IP typically has a multiplier,
>>>>>>>>> +a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
>>>>>>>>> +and PAPLL are controlled by the memory mapped register where as the Main
>>>>>>>>> +PLL is controlled by a PLL controller. This difference is handle using
>>>>>>>>> +'pll_has_pllctrl' property.
>>>>>>>>> +
>>>>>>>>> +This binding uses the common clock binding[1].
>>>>>>>>> +
>>>>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible : shall be "keystone,pll-clk".
>>>>>>>>
>>>>>>>> Keystone isn't a vendor, and generally, bindings have used "clock"
>>>>>>>> rather than "clk".
>>>>>>>>
>>>>>>>> Perhaps "ti,keystone-pll-clock" ?
>>>>>>>>
>>>>>>> Agree.
>>>>>>>
>>>>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>>>>>> +- clocks : parent clock phandle
>>>>>>>>> +- reg - index 0 -  PLLCTRL PLLM register address
>>>>>>>>> +-        index 1 -  MAINPLL_CTL0 register address
>>>>>>>>
>>>>>>>> Perhaps a reg-names would be useful?
>>>>>>>>
>>>>>>>>> +- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
>>>>>>>>
>>>>>>>> Huh? I don't understand what that description means. What does the
>>>>>>>> property tell you? Is having one of the registers optional? If so that
>>>>>>>> should be described by a reg-names property associated with the reg, and
>>>>>>>> should be noted in the binding.
>>>>>>>>
>>>>>>> After re-reading it, yes I agree it not clear. The point is there
>>>>>>> are two different types of IPs and pogramming model changes quite
>>>>>>> a bit. Its not just 1 register optional.
>>>>>>
>>>>>> If that's the case, then having different compatible strings would make
>>>>>> sense.
>>>>>>
>>>>>>>
>>>>>>>>> +- pllm_lower_mask - pllm lower bit mask
>>>>>>>>> +- pllm_upper_mask - pllm upper bit mask
>>>>>>>>> +- plld_mask - plld mask
>>>>>>>>> +- fixed_postdiv - fixed post divider value
>>>>>>>>
>>>>>>>> Please use '-' rather than '_' in property names, it's a standard
>>>>>>>> convention for dt and going against it just makes things unnecessarily
>>>>>>>> complicated.
>>>>>>>>
>>>>>>>> Why are these necessary? Are clocks sharing common registers, are there
>>>>>>>> some sets of "keystone,pll-clk"s that have the same masks, or does this
>>>>>>>> just vary wildly?
>>>>>>>>
>>>>>>> This is mainly to take care of the programming model which varies between
>>>>>>> IPs. Will try to make that bit more clear.
>>>>>>
>>>>>> Are there more than the two IPs mentioned above? If they had separate
>>>>>> compatible strings, would that give enough information implicitly,
>>>>>> without the precise masks needing to be in the dt?
>>>>>>
>>>>> I will explore the separate compatible option. Thanks for suggestion.
>>>>>
>>>> I looked at further into separate compatible option or reg-names
>>>> to check if it can help to reduce some additional dt information.
>>>> Actually it doesn't help much. The base programming model is shared
>>>> between both types of PLL IPs. The PLLs which has PLL controller along
>>>> with MMRs, has to take into account additional bit-fields for the
>>>> multiplier and divider along with the base model multiplier and divider
>>>> registers.
>>>
>>> It is common for the base programming model to be similar or shared
>>> between two different compatible strings. You can use the same clk_ops
>>> for clk_prepare, clk_enable, etc.
>>>
>>> The point is to use the different compatible strings to encode the
>>> differences in *data* between the two clock types. That way you have
>>> fewer properties to list in the binding since two separate setup
>>> functions can implicitly handle the differences in initializing the
>>> per-clock data.
>>>
>> Thats the point I came back. Both PLL share the properties and
>> the main PLL brings in couple of properties to extend the divider
>> field and that was handled through the flag. As mentioned below,
>> some properties like plld_mask, pllm_upper_shift etc can be
>> dropped since they are static values.
>>
>>>>
>>>> Having said that, there are few parameters which are fixed like
>>>> plld_mask, pllm_upper_shift etc need not come from DT. I am going
>>>> to remove them from dt bindings and also make the reg index consistent
>>>> as it should have been in first place.
>>>
>>> This is nice. Note that these things may change in future Keystone
>>> versions because hardware people hate you and want to make your life
>>> hard. So the compatible string might benefit from including the first
>>> keystone part number that uses this binding (e.g.
>>> ti,keystone-2420-pll-clock, which I just made up).
>>>
>>> In the future when the register layout, offsets and masks change for
>>> absolutely no reason (but the programming model is the same) then you
>>> can just write a new binding that setups up your private clk_pll_data
>>> struct without having to jam all of those details into DT (e.g.
>>> ti,keystone-3430-pll-clock, which I also just made up).
>>>
>> So thats the good part so far with the keystone where the compatibility
>> is maintained pretty well. I know background of your comments ;-)
>> I will keep the bindings without any direct device association for
>> now considering I don't foresee any thing changing in that aspect.
> 
> Sure. It's just a suggestion. Once the ABI is set it isn't supposed to
> change so I'm just trying to think ahead.
> 
I will also spend some more time on binding so that we don't miss anything.

> Also there has been discussion on marking bindings as "unstable". I
> don't know if you are interested in that but maybe putting a line at the
> top of the binding stating something like:
> 
> Status: Unstable - ABI compatibility may be broken in the future
> 
OK. Will do that.

regards,
Santosh

Patch

diff --git a/Documentation/devicetree/bindings/clock/keystone-pll.txt b/Documentation/devicetree/bindings/clock/keystone-pll.txt
new file mode 100644
index 0000000..58f6470
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/keystone-pll.txt
@@ -0,0 +1,36 @@ 
+Binding for keystone PLLs. The main PLL IP typically has a multiplier,
+a divider and a post divider. The additional PLL IPs like ARMPLL, DDRPLL
+and PAPLL are controlled by the memory mapped register where as the Main
+PLL is controlled by a PLL controller. This difference is handle using
+'pll_has_pllctrl' property.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "keystone,pll-clk".
+- #clock-cells : from common clock binding; shall be set to 0.
+- clocks : parent clock phandle
+- reg - index 0 -  PLLCTRL PLLM register address
+- 	index 1 -  MAINPLL_CTL0 register address
+- pll_has_pllctrl - PLL is controlled by controller or memory mapped register
+- pllm_lower_mask - pllm lower bit mask
+- pllm_upper_mask - pllm upper bit mask
+- plld_mask - plld mask
+- fixed_postdiv - fixed post divider value
+
+Example:
+	clock {
+		#clock-cells = <0>;
+		compatible = "keystone,pll-clk";
+		clocks = <&refclk>;
+		reg = <0x02310110 4	/* PLLCTRL PLLM */
+			0x02620328 4>;	/* MAINPLL_CTL0 */
+		pll_has_pllctrl;
+		pllm_lower_mask	= <0x3f>;
+		pllm_upper_mask = <0x7f000>;
+		pllm_upper_shift = <6>;
+		plld_mask = <0x3f>;
+		fixed_postdiv = <2>;
+	};
diff --git a/drivers/clk/keystone/pll.c b/drivers/clk/keystone/pll.c
new file mode 100644
index 0000000..1453eea
--- /dev/null
+++ b/drivers/clk/keystone/pll.c
@@ -0,0 +1,197 @@ 
+/*
+ * Main PLL clk driver for Keystone devices
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *	Murali Karicheri <[e-mail]>
+ *	Santosh Shilimkar <[e-mail]>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/clk/keystone.h>
+
+/**
+ * struct clk_pll_data - pll data structure
+ * @has_pllctrl: If set to non zero, lower 6 bits of multiplier is in pllm
+ *	register of pll controller, else it is in the pll_ctrl0((bit 11-6)
+ * @phy_pllm: Physical address of PLLM in pll controller. Used when
+ *	has_pllctrl is non zero.
+ * @phy_pll_ctl0: Physical address of PLL ctrl0. This could be that of
+ *	Main PLL or any other PLLs in the device such as ARM PLL, DDR PLL
+ *	or PA PLL available on keystone2. These PLLs are controlled by
+ *	this register. Main PLL is controlled by a PLL controller.
+ * @pllm: PLL register map address
+ * @pll_ctl0: PLL controller map address
+ * @pllm_lower_mask: multiplier lower mask
+ * @pllm_upper_mask: multiplier upper mask
+ * @pllm_upper_shift: multiplier upper shift
+ * @plld_mask: divider mask
+ * @fixed_postdiv: Post divider
+ */
+struct clk_pll_data {
+	unsigned char has_pllctrl;
+	u32 phy_pllm;
+	u32 phy_pll_ctl0;
+	void __iomem *pllm;
+	void __iomem *pll_ctl0;
+	u32 pllm_lower_mask;
+	u32 pllm_upper_mask;
+	u32 pllm_upper_shift;
+	u32 plld_mask;
+	u32 fixed_postdiv;
+};
+
+/**
+ * struct clk_pll - Main pll clock
+ * @hw: clk_hw for the pll
+ * @pll_data: PLL driver specific data
+ */
+struct clk_pll {
+	struct clk_hw hw;
+	struct clk_pll_data *pll_data;
+};
+
+#define to_clk_pll(_hw) container_of(_hw, struct clk_pll, hw)
+
+static unsigned long clk_pllclk_recalc(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct clk_pll *pll = to_clk_pll(hw);
+	struct clk_pll_data *pll_data = pll->pll_data;
+	unsigned long rate = parent_rate;
+	u32  mult = 0, prediv, postdiv, val;
+
+	/*
+	 * get bits 0-5 of multiplier from pllctrl PLLM register
+	 * if has_pllctrl is non zero */
+	if (pll_data->has_pllctrl) {
+		val = readl(pll_data->pllm);
+		mult = (val & pll_data->pllm_lower_mask);
+	}
+
+	/* bit6-12 of PLLM is in Main PLL control register */
+	val = readl(pll_data->pll_ctl0);
+	mult |= ((val & pll_data->pllm_upper_mask)
+			>> pll_data->pllm_upper_shift);
+	prediv = (val & pll_data->plld_mask);
+	postdiv = pll_data->fixed_postdiv;
+
+	rate /= (prediv + 1);
+	rate = (rate * (mult + 1));
+	rate /= postdiv;
+
+	return rate;
+}
+
+static const struct clk_ops clk_pll_ops = {
+	.recalc_rate = clk_pllclk_recalc,
+};
+
+static struct clk *clk_register_pll(struct device *dev,
+			const char *name,
+			const char *parent_name,
+			struct clk_pll_data *pll_data)
+{
+	struct clk_init_data init;
+	struct clk_pll *pll;
+	struct clk *clk;
+
+	if (!pll_data)
+		return ERR_PTR(-ENODEV);
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &clk_pll_ops;
+	init.flags = 0;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	pll->pll_data	= pll_data;
+	pll->hw.init = &init;
+
+	clk = clk_register(NULL, &pll->hw);
+	if (IS_ERR(clk))
+		goto out;
+
+	return clk;
+out:
+	kfree(pll);
+	return NULL;
+}
+
+/**
+ * of_keystone_pll_clk_init - PLL initialisation via DT
+ * @node: device tree node for this clock
+ */
+void __init of_keystone_pll_clk_init(struct device_node *node)
+{
+	struct clk_pll_data *pll_data;
+	const char *parent_name;
+	struct clk *clk;
+	int temp;
+
+	pll_data = kzalloc(sizeof(*pll_data), GFP_KERNEL);
+	WARN_ON(!pll_data);
+
+	parent_name = of_clk_get_parent_name(node, 0);
+
+	if (of_find_property(node, "pll_has_pllctrl", &temp)) {
+		/* PLL is controlled by the pllctrl */
+		pll_data->has_pllctrl = 1;
+		pll_data->pllm = of_iomap(node, 0);
+		WARN_ON(!pll_data->pllm);
+
+		pll_data->pll_ctl0 = of_iomap(node, 1);
+		WARN_ON(!pll_data->pll_ctl0);
+
+		if (of_property_read_u32(node, "pllm_lower_mask",
+				&pll_data->pllm_lower_mask))
+			goto out;
+
+	} else {
+		/* PLL is controlled by the ctrl register */
+		pll_data->has_pllctrl = 0;
+		pll_data->pll_ctl0 = of_iomap(node, 0);
+	}
+
+	if (of_property_read_u32(node, "pllm_upper_mask",
+			&pll_data->pllm_upper_mask))
+		goto out;
+
+	if (of_property_read_u32(node, "pllm_upper_shift",
+			&pll_data->pllm_upper_shift))
+		goto out;
+
+	if (of_property_read_u32(node, "plld_mask", &pll_data->plld_mask))
+		goto out;
+
+	if (of_property_read_u32(node, "fixed_postdiv",
+					&pll_data->fixed_postdiv))
+		goto out;
+
+	clk = clk_register_pll(NULL, node->name, parent_name,
+					 pll_data);
+	if (clk) {
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+		return;
+	}
+out:
+	pr_err("of_keystone_pll_clk_init - error initializing clk %s\n",
+		 node->name);
+	kfree(pll_data);
+}
+EXPORT_SYMBOL_GPL(of_keystone_pll_clk_init);
+CLK_OF_DECLARE(keystone_pll_clk, "keystone,pll-clk", of_keystone_pll_clk_init);
diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h
new file mode 100644
index 0000000..1ade95d
--- /dev/null
+++ b/include/linux/clk/keystone.h
@@ -0,0 +1,18 @@ 
+/*
+ * Keystone Clock driver header
+ *
+ * Copyright (C) 2013 Texas Instruments Inc.
+ *	Santosh Shilimkar <[e-mail]>
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_CLK_KEYSTONE_H_
+#define __LINUX_CLK_KEYSTONE_H_
+
+extern void of_keystone_pll_clk_init(struct device_node *node);
+
+#endif /* __LINUX_CLK_KEYSTONE_H_ */