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

Tue Aug 20 09:41:21 EDT 2013

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.

Regards,
Santosh