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

Tue Aug 20 17:46:05 EDT 2013

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