From: santosh.shilimkar@ti•com (Santosh Shilimkar)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 1/8] clk: keystone: add Keystone PLL clock driver
Date: Tue, 13 Aug 2013 12:58:04 -0400 [thread overview]
Message-ID: <520A659C.3030809@ti.com> (raw)
In-Reply-To: <20130813164730.GT27165@e106331-lin.cambridge.arm.com>
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.
Regards,
Santosh
next prev parent reply other threads:[~2013-08-13 16:58 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-05 16:12 [PATCH 0/8] clk: keystone: Add common clock drivers and PM bus Santosh Shilimkar
2013-08-05 16:12 ` [PATCH 1/8] clk: keystone: add Keystone PLL clock driver Santosh Shilimkar
2013-08-13 15:48 ` Mark Rutland
2013-08-13 16:01 ` Santosh Shilimkar
2013-08-13 16:47 ` Mark Rutland
2013-08-13 16:58 ` Santosh Shilimkar [this message]
2013-08-19 17:42 ` Santosh Shilimkar
2013-08-19 20:33 ` Mike Turquette
2013-08-20 13:41 ` Santosh Shilimkar
2013-08-20 21:23 ` Mike Turquette
2013-08-20 21:46 ` Santosh Shilimkar
2013-08-05 16:12 ` [PATCH 2/8] clk: keystone: Add gate control " Santosh Shilimkar
2013-08-13 16:13 ` Mark Rutland
2013-08-13 16:36 ` Santosh Shilimkar
2013-08-13 16:53 ` Mark Rutland
2013-08-19 20:43 ` Mike Turquette
2013-08-20 13:55 ` Santosh Shilimkar
2013-08-20 21:30 ` Mike Turquette
2013-08-20 21:55 ` Santosh Shilimkar
2013-08-20 22:41 ` Mike Turquette
2013-08-20 22:54 ` Santosh Shilimkar
2013-08-21 2:22 ` Mike Turquette
2013-08-21 13:16 ` Santosh Shilimkar
2013-08-22 8:12 ` Mike Turquette
2013-08-22 14:10 ` Santosh Shilimkar
2013-08-05 16:12 ` [PATCH 3/8] clk: keystone: common clk driver initialization Santosh Shilimkar
2013-08-05 18:54 ` Nishanth Menon
2013-08-05 19:27 ` Santosh Shilimkar
2013-08-05 16:12 ` [PATCH 4/8] clk: keystone: Build Keystone clock drivers Santosh Shilimkar
2013-08-05 16:12 ` [PATCH 5/8] ARM: dts: keystone: Add clock tree data to devicetree Santosh Shilimkar
2013-08-05 16:12 ` [PATCH 6/8] ARM: dts: keystone: Add clock phandle to UART nodes Santosh Shilimkar
2013-08-05 16:12 ` [PATCH 7/8] ARM: keystone: Enable and initialise clock drivers Santosh Shilimkar
2013-08-05 16:12 ` [PATCH 8/8] ARM: keystone: add PM bus support for clock management Santosh Shilimkar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=520A659C.3030809@ti.com \
--to=santosh.shilimkar@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox