Re: [PATCH v4 1/2] ARM: keystone: pm: switch to use generic pm domains

[Grygorii Strashko]

Hi Kevin,

On 11/21/2014 09:29 PM, Kevin Hilman wrote:
> Grygorii Strashko <grygorii.strashko@xxxxxx> writes:
> 
>> Hi Kevin,
>> On 11/21/2014 10:06 AM, Geert Uytterhoeven wrote:
>>> On Fri, Nov 21, 2014 at 2:30 AM, Kevin Hilman <khilman@xxxxxxxxxx> wrote:
>>>> Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> writes:
>>>>> On Thu, Nov 20, 2014 at 10:48 PM, Kevin Hilman <khilman@xxxxxxxxxx> wrote:
>>>>>>>> So what exactly are we talking about with "PM" clocks, and why are they
>>>>>>>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>>>>>>>> managed during PM domain transitions for a given device any different
>>>>>>>> than the clocks that need to be managed for a runtime suspend/resume (or
>>>>>>>> system suspend/resume) sequence for the same device?
>>>>>>>
>>>>>>> (Speaking for my case, shmobile)
>>>>>>>
>>>>>>> They're not. The clocks to be managed during PM domain transitions are the
>>>>>>> same as the clocks that need to be managed for a runtime suspend/resume
>>>>>>> (or system suspend/resume) sequence.
>>>>>>>
>>>>>>> The special thing is that this is more a platform than a driver thing: the same
>>>>>>> module may have a "PM/functional" clock (that is documented to enable/disable
>>>>>>> the module) on one Soc, but noet on another.
>>>>>>
>>>>>> So why isn't the presence or absence of the clock described in the .dtsi
>>>>>> for the SoC instead of being handled by special PM domain logic?
>>>>>
>>>>> It is. Cfr. the presence/absence of clocks for renesas,rcar-gpio nodes.
>>>>
>>>> Hmm, OK, Good.
>>>>
>>>> So now I'm confused about why the PM domain has to do anything special
>>>> if the presence/absence of the clocks is already handled by the DT.
>>>
>>> Just adding a clock property to a device node in DT doesn't enable the clock
>>> automatically, nor make it runtime-managed automatically.
>>> Compare this to e.g. pinctrl, where adding pinctrl properties to DT does enable
>>> them automatically, without the driver for the device having to care about it.
>>>
>>> Drivers interfacing external hardware typically do care about clocks, as they
>>> have to program clock generators for the external hardware interface (e.g.
>>> driving spi or i2c buses at specific frequencies).
>>>
>>> Other random drivers don't care about clocks, so they don't handle them.
>>> But as long as they make basic pm_runtime_{enable,get_sync,put}() calls,
>>> the (optional) clocks (and hardware PM domains) will "work" fine, if handled
>>> by the PM (clock) domain.
>>
>> Personally, I don't see problems with "functional" clocks.
>> The problem, in my opinion, is that we can't specify in DT that some clock is
>> "optional", so no one should touch such clock except driver.
>> For example:
>> Keystone 2 NETCP module has 3 clocks:
>> 	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
>> 	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
>>   and CPTS functionality is optional (in general) and can be enabled/disabled.
>> Also, usual example is MMC hosts
>> - OMAP hsmmc has "functional" clock "hsmmc1_fclk", and may have
>> "optional" clock "mmchsdb_fck". As you may remember, PM runtime
>> for OMAP2+ implemented by OMAP PM domain which performs many things,
>> but one thing which is done always is enabling/disabling of "fck"
>> when get/put() are called.
>>
>> Actually, pm_clk is very simple case of OMAP PM domain which should only
>> handle clocks.
>>
>> In non-DT case, we have possibility to divide clocks on "fck" and "opt"
>> (The way it can be done is not convenient, but it is - .con_id).
>>
>> For DT-case - no way now. Also, PM domains are not physically present on
>> Keystone 2 and GPD was selected as glue layer to integrate DT, pm_clk and
>> PM runtime all together (one big-fat-global PM domain :).
>>
>> So, I was able to find only following way to define "fck" clocks in DT:
>> 	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
>> 	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
>> 	fck-clocks = <&papllclk>, <&clkcpgmac>;
>> As you can see - this will lead to data duplication in DT (
>>
>> Any propositions are welcome?
> 
> How about you proposed a new (optional) property for the clock bindings
> called 'clocks-optional' or something like that.  The clock maintainer
> (Mike Turquette, Cc'd) is also very familiar with OMAP and the need for
> these different kinds of clocks.

'clocks-optional' - is not good, as for me. DT ABI:( Many drivers have to get "fck" 
even in case they don't need to manage them - usual case is to get
clock rate (SPI, I2C, MDIO,.. as mentioned by Geert).
Also, CLK core is using clock's names when it is looking for requested clock in DT.
Also, CLK core uses sequential indexing of clock.

Or, may be i don't fully understand, how 'clocks-optional' can be handled :( ?

'clocks-functional'- is better. 
"Optional. List of phandle and functional clock specifier pairs, one pair
 for each clock input to the device. 
 Note: Functional clock is mandatory clock which supplies the functional part
 of a module or a subsystem and which must be operational always when 
 a module or a subsystem is enabled and must be to complete I/O operations as needed.
 Note: it can be empty - then all clocks specified in 'clocks' property will be treated as
 functional"

What do you think?

regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/