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@xxxxxxxxxx> >>>>>>>>> >>>>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@xxxxxx> >>>>>>>>> --- >>>>>>> >>>>>>> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
