[PATCH 2/8] clk: keystone: Add gate control clock driver

[santosh.shilimkar@ti.com (Santosh Shilimkar)]

On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote:
> Quoting Santosh Shilimkar (2013-08-20 06:55:54)
>> On Monday 19 August 2013 04:43 PM, Mike Turquette wrote:
>>> Quoting Mark Rutland (2013-08-13 09:53:44)
>>>> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote:
>>>>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote:
>>>>>> [adding dt maintainers]
>>>>>>
>>>>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote:
>>>>>>> Add the driver for the clock gate control which uses PSC (Power Sleep
>>>>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and
>>>>>>> disabling of the clocks for different IPs present in the SoC.
>>>>>>>
>>>>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>>>>
>>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/clock/keystone-gate.txt    |   30 ++
>>>>>>>  drivers/clk/keystone/gate.c                        |  306 ++++++++++++++++++++
>>>>>>>  include/linux/clk/keystone.h                       |    1 +
>>>>>>>  3 files changed, 337 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>>>>  create mode 100644 drivers/clk/keystone/gate.c
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..40afef5
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt
>>>>>>> @@ -0,0 +1,30 @@
>>>>>>> +Binding for Keystone gate control driver which uses PSC controller IP.
>>>>>>> +
>>>>>>> +This binding uses the common clock binding[1].
>>>>>>> +
>>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible : shall be "keystone,psc-clk".
>>>>>>
>>>>>> Similarly to my comments on patch 1, this should probably be something
>>>>>> more like "ti,keystone-psc-clock"
>>>>>>
>>>>>>> +- #clock-cells : from common clock binding; shall be set to 0.
>>>>>>> +- clocks : parent clock phandle
>>>>>>> +- reg :        psc address address space
>>>>>>> +
>>>>>>> +Optional properties:
>>>>>>> +- clock-output-names : From common clock binding to override the
>>>>>>> +                       default output clock name
>>>>>>> +- status : "enabled" if clock is always enabled
>>>>>>
>>>>>> Huh? This is a standard property, for which ePAPR defines "okay",
>>>>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't
>>>>>> seem to follow the standard meaning judging by the code.
>>>>>>
>>>>>> It looks like this could be a boolean property
>>>>>> ("clock-always-enabled"?), but that assumes it's necessary.
>>>>>>
>>>>>> Why do you need this?
>>>>>>
>>>>> I dropped this already. Forgot to update the documentation.
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>>>> +- lpsc : lpsc module id, if not set defaults to zero
>>>>>>
>>>>>> What's that needed for? Are there multiple clocks sharing a common
>>>>>> register bank?
>>>>>>
>>>>>>> +- pd : power domain number, if not set defaults to zero (always ON)
>>>>>>
>>>>>> Please don't use abbreviations unnecessarily. "power-domain-id" or
>>>>>> similar would be far better. What exactly does this represent? Does the
>>>>>> clock IP itself handle power domains, or is there some external unit
>>>>>> that controls power domains?
>>>>>>
>>>>> pd is commonly used but I can expand it. This represent the power
>>>>> domain number.
>>>>
>>>> Does the the clock IP have some internal logic for handling power
>>>> domains only covering its clocks, or is there some external power
>>>> controller involved (i.e. do we possibly need to describe an external
>>>> unit and a linkage to it?).
>>>
>>> Hmm, does the clock own the power domain or does the power domain own
>>> the clock? As you well know on OMAP the clock resides within the power
>>> domain. So it seems to me that the better way to do this would be for
>>> the power domain to have it's own binding and node, and then reference
>>> the clock:
>>>
>>> power-domain {
>>>       ...
>>>       clocks = <&chipclk3>, <&chipclk4>;
>>>       clock-names = "perclk", "uartclk";
>>>       ...
>>> };
>>>
>>> Now maybe things are different on Keystone, but if not then I don't see
>>> why the clock binding should have anything to do with the power domain.
>>>
>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the
>> IP. The LPSC number in the bindings is actually the specific number which
>> is used to reach to the address space of the clock control. One can view
>> that one as clock control register index.
> 
> Thanks for the information. I have a further question about then: are
> the LPSC clocks really module clocks that belong to the IP that they are
> gating?
> 
LPSC controls the clock enable/disable to the IP/module so answer is yes.
In certain cases LPSC controls clock to more than one IP as well.

> If so then they could be defined within the node defining their parent
> IP. That might be enough to get rid of the LPSC index value. Again I
> might be over-engineering it. Just trying to get an understanding.
> 
Am not sure I follow you here on not having the LPSC index. Sorry. 

>>
>> The power domain as such are above the lpsc but the clock enable/disable needs
>> to follow a recommended sequence which needs the access to those registers.
>> As such there is no major validation done on dynamically switching off PDs
>> in current generation devices.
>>
>> As I mentioned you to on IRC, the PD was the only odd part I have to keep
>> around to address the sequencing need which is kind of interlinked.
> 
> Right. Well maybe some day that part can go away but I understand the
> need for it now.
> 
right. Thanks

regards,
Santosh