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

[mturquette@linaro.org (Mike Turquette)]

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 <mturquette@linaro.org>
> >>>>>
> >>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >>>>> ---
> >>>
> >>> 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