moving Tegra30 to the common clock framework

moving Tegra30 to the common clock framework

[Peter De Schrijver]

Hi,

I started looking into what would be needed to move our tegra30 clock code
to the common clock framework. The tegra30 clocktree is rather flat. Basically
there are a bunch of sources (13 PLLs, external audio clocks, osc and 32Khz)
and peripheral clocks which have a mux (with 4 or more inputs), a divider and
a gate. So almost every peripheral clock can have multiple parents.

Some questions:

1) should these peripheral clocks be modelled as 3 different clocks
   (mux -> divider -> gate) or would it be better to make a new clock type for
   this?

2) how to define the default parent? in many cases the hw reset value isn't
   a very sensible choice, so the kernel probably needs to set a parent of
   many of them if we don't want to rely on bootloader configuration.

Cheers,

Peter.

moving Tegra30 to the common clock framework

[Mike Turquette]

On 20120503-19:13, Peter De Schrijver wrote:
> Hi,
> 
> I started looking into what would be needed to move our tegra30 clock code
> to the common clock framework. The tegra30 clocktree is rather flat. Basically
> there are a bunch of sources (13 PLLs, external audio clocks, osc and 32Khz)
> and peripheral clocks which have a mux (with 4 or more inputs), a divider and
> a gate. So almost every peripheral clock can have multiple parents.
> 
> Some questions:
> 
> 1) should these peripheral clocks be modelled as 3 different clocks
>    (mux -> divider -> gate) or would it be better to make a new clock type for
>    this?
> 

That is really for you to decide.  If the semantics of the existing mux,
divider and gate in drivers/clk/clk-*.c work well for you then I think
the answer is "yes".  There is infrastructure for register-access
locking in those common types which might help your complex clocks.

Thanks to the parent rate propagation stuff in clk_set_rate it should be
possible for your drivers to only be aware of the gate and call
clk_set_rate on only that clock, which propagates up to the divider and,
if necessary, again propagates up to the mux.

I encourage you to try that first.  But if you find the semantics of
those basic clock types aren't cutting it for you then you must create a
type which is platform-specific.

> 2) how to define the default parent? in many cases the hw reset value isn't
>    a very sensible choice, so the kernel probably needs to set a parent of
>    many of them if we don't want to rely on bootloader configuration.

The only related thing handled at the framework level is _discovery_ of
the parent during clock registration/initialization.  If you don't trust
the bootloader and want to set things up as soon as possible (a wise
move) then I suggest you do so from your platform clock code at the same
time that you register your clocks with the framework.  Something like:

	struct clk *c;
	c = clk_register(...);
	if (IS_ERR(c))
		omg_fail();
	clk_set_parent(c, b);

Where 'b' is a parent of 'c'.  Register your clock tree top-down and you
can re-parent as you go.

Regards,
Mike

moving Tegra30 to the common clock framework

[Stephen Warren]

On 05/06/2012 06:03 PM, Mike Turquette wrote:
> On 20120503-19:13, Peter De Schrijver wrote:
>> Hi,
>>
>> I started looking into what would be needed to move our tegra30 clock code
>> to the common clock framework. The tegra30 clocktree is rather flat. Basically
>> there are a bunch of sources (13 PLLs, external audio clocks, osc and 32Khz)
>> and peripheral clocks which have a mux (with 4 or more inputs), a divider and
>> a gate. So almost every peripheral clock can have multiple parents.
>>
>> Some questions:
>>
>> 1) should these peripheral clocks be modelled as 3 different clocks
>>    (mux -> divider -> gate) or would it be better to make a new clock type for
>>    this?
>>
> 
> That is really for you to decide.  If the semantics of the existing mux,
> divider and gate in drivers/clk/clk-*.c work well for you then I think
> the answer is "yes".  There is infrastructure for register-access
> locking in those common types which might help your complex clocks.
> 
> Thanks to the parent rate propagation stuff in clk_set_rate it should be
> possible for your drivers to only be aware of the gate and call
> clk_set_rate on only that clock, which propagates up to the divider and,
> if necessary, again propagates up to the mux.
> 
> I encourage you to try that first.  But if you find the semantics of
> those basic clock types aren't cutting it for you then you must create a
> type which is platform-specific.

A lot of these mux/divider/gate clocks go out to peripherals, whose
drivers want to call both clk_set_rate() and clk_en/disable() on the clock.

There's only 1 clock reaching the peripheral in HW, so the driver should
only call clk_get() once, and similarly the DT should only provide a
single clock to the driver.

Given the mux->divider->gate clock construction, that clock would
presumably be the gate object. clk_en/disable() clearly work there, but
is clk_set_rate() intended to propagate up the chain until it can be
satisfied, i.e. does the gate clock object's set_rate() op simply call
the parent's set_rate() op?

If the order were instead mux->gate->divider, would it be correct for
enable/disable to propagate from the divider to the gate?

>> 2) how to define the default parent? in many cases the hw reset value isn't
>>    a very sensible choice, so the kernel probably needs to set a parent of
>>    many of them if we don't want to rely on bootloader configuration.
> 
> The only related thing handled at the framework level is _discovery_ of
> the parent during clock registration/initialization.  If you don't trust
> the bootloader and want to set things up as soon as possible (a wise
> move) then I suggest you do so from your platform clock code at the same
> time that you register your clocks with the framework.  Something like:
> 
> 	struct clk *c;
> 	c = clk_register(...);
> 	if (IS_ERR(c))
> 		omg_fail();
> 	clk_set_parent(c, b);
> 
> Where 'b' is a parent of 'c'.  Register your clock tree top-down and you
> can re-parent as you go.

I'm hoping we can represent this in device tree somehow, so that
individual boards can set the clock tree up differently depending on
their needs (e.g. Tegra20 doesn't have quite enough PLLs, so sometimes a
particular PLL will be used to generate the 2nd display's pixel clock,
whereas on other boards it may be used for some peripherals). So, we'd
like to describe this in DT.

It seems like it'd be pretty common to want the kernel to fully
initialize the clock tree, and to do this from device tree, so perhaps
this might evolve into a common (part of) a cross-SoC clock binding, or
some kind of utility function that parsed a clock-tree-init-table from DT?

moving Tegra30 to the common clock framework

[Turquette, Mike]

On Mon, May 7, 2012 at 8:39 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/06/2012 06:03 PM, Mike Turquette wrote:
>> On 20120503-19:13, Peter De Schrijver wrote:
>>> Hi,
>>>
>>> I started looking into what would be needed to move our tegra30 clock code
>>> to the common clock framework. The tegra30 clocktree is rather flat. Basically
>>> there are a bunch of sources (13 PLLs, external audio clocks, osc and 32Khz)
>>> and peripheral clocks which have a mux (with 4 or more inputs), a divider and
>>> a gate. So almost every peripheral clock can have multiple parents.
>>>
>>> Some questions:
>>>
>>> 1) should these peripheral clocks be modelled as 3 different clocks
>>> ? ?(mux -> divider -> gate) or would it be better to make a new clock type for
>>> ? ?this?
>>>
>>
>> That is really for you to decide. ?If the semantics of the existing mux,
>> divider and gate in drivers/clk/clk-*.c work well for you then I think
>> the answer is "yes". ?There is infrastructure for register-access
>> locking in those common types which might help your complex clocks.
>>
>> Thanks to the parent rate propagation stuff in clk_set_rate it should be
>> possible for your drivers to only be aware of the gate and call
>> clk_set_rate on only that clock, which propagates up to the divider and,
>> if necessary, again propagates up to the mux.
>>
>> I encourage you to try that first. ?But if you find the semantics of
>> those basic clock types aren't cutting it for you then you must create a
>> type which is platform-specific.
>
> A lot of these mux/divider/gate clocks go out to peripherals, whose
> drivers want to call both clk_set_rate() and clk_en/disable() on the clock.
>
> There's only 1 clock reaching the peripheral in HW, so the driver should
> only call clk_get() once, and similarly the DT should only provide a
> single clock to the driver.
>

Sounds good so far.

> Given the mux->divider->gate clock construction, that clock would
> presumably be the gate object. clk_en/disable() clearly work there, but
> is clk_set_rate() intended to propagate up the chain until it can be
> satisfied, i.e. does the gate clock object's set_rate() op simply call
> the parent's set_rate() op?
>

Only if you set the CLK_SET_RATE_PARENT flag on the children.  Take
the following example sub-tree:

mux
 |
div
 |
gate

If gate does NOT have CLK_SET_RATE_PARENT set in struct clk->flags
then clk_set_rate will do nothing (since it does not implement a
.set_rate callback).  If the flag is set then the framework will kick
the rate request up to the parent, 'div'.  Div implements a .set_rate
callback and may or may not set the CLK_SET_RATE_PARENT flag.  Having
a .set_rate implemenation does preclude one from propagating the rate
change up the tree, so it is possible to adjust the divider to a new
rate AND still kick the rate change request up to the parent, 'mux'.
In your case 'mux' should probably not set CLK_SET_RATE_PARENT and the
propagation will end there.  'mux' might implement a re-parenting
operation as part of it's .set_rate implementation.

This is covered fairly well in Documentation/clk.txt

> If the order were instead mux->gate->divider, would it be correct for
> enable/disable to propagate from the divider to the gate?
>

Correct.  Order doesn't matter.  Even in the absence of a
.prepare/.enable callback the framework will still propagate these
changes as part of typical use-counting rules.

>>> 2) how to define the default parent? in many cases the hw reset value isn't
>>> ? ?a very sensible choice, so the kernel probably needs to set a parent of
>>> ? ?many of them if we don't want to rely on bootloader configuration.
>>
>> The only related thing handled at the framework level is _discovery_ of
>> the parent during clock registration/initialization. ?If you don't trust
>> the bootloader and want to set things up as soon as possible (a wise
>> move) then I suggest you do so from your platform clock code at the same
>> time that you register your clocks with the framework. ?Something like:
>>
>> ? ? ? struct clk *c;
>> ? ? ? c = clk_register(...);
>> ? ? ? if (IS_ERR(c))
>> ? ? ? ? ? ? ? omg_fail();
>> ? ? ? clk_set_parent(c, b);
>>
>> Where 'b' is a parent of 'c'. ?Register your clock tree top-down and you
>> can re-parent as you go.
>
> I'm hoping we can represent this in device tree somehow, so that
> individual boards can set the clock tree up differently depending on
> their needs (e.g. Tegra20 doesn't have quite enough PLLs, so sometimes a
> particular PLL will be used to generate the 2nd display's pixel clock,
> whereas on other boards it may be used for some peripherals). So, we'd
> like to describe this in DT.
>
> It seems like it'd be pretty common to want the kernel to fully
> initialize the clock tree, and to do this from device tree, so perhaps
> this might evolve into a common (part of) a cross-SoC clock binding, or
> some kind of utility function that parsed a clock-tree-init-table from DT?

As long as the configuration of your clocks can be expressed by the
existing clk api, e.g. clk_prepare / clk_enable / clk_set_rate /
clk_set_parent, then I think it would be easy to define a format
whereby the clock is configured immediately after being registered.
Essentially it would do what I outline in my original email above, but
the configuration could be encoded into DT.

Any platform-specific clock configuration (bits that aren't obviously
expressed by the clk.h api) will be harder to configure through
generic DT bindings.

Regards,
Mike

moving Tegra30 to the common clock framework

[zhoujie wu]

Hi Mike,
Could you please explain more details about how to implement a
re-parenting operation as part of it's .set_rate implementation?
As far as I know, we can not call clk_set_parent in .set_rate function
directly, since clk_set_rate and clk_set_parent are using the same
prepare_lock.
Any other interface can be used to implement it?
Thanks.

2012/5/8 Turquette, Mike <mturquette@ti.com>:
> On Mon, May 7, 2012 at 8:39 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/06/2012 06:03 PM, Mike Turquette wrote:
>>> On 20120503-19:13, Peter De Schrijver wrote:
>>>> Hi,
>>>>
>>>> I started looking into what would be needed to move our tegra30 clock code
>>>> to the common clock framework. The tegra30 clocktree is rather flat. Basically
>>>> there are a bunch of sources (13 PLLs, external audio clocks, osc and 32Khz)
>>>> and peripheral clocks which have a mux (with 4 or more inputs), a divider and
>>>> a gate. So almost every peripheral clock can have multiple parents.
>>>>
>>>> Some questions:
>>>>
>>>> 1) should these peripheral clocks be modelled as 3 different clocks
>>>> ? ?(mux -> divider -> gate) or would it be better to make a new clock type for
>>>> ? ?this?
>>>>
>>>
>>> That is really for you to decide. ?If the semantics of the existing mux,
>>> divider and gate in drivers/clk/clk-*.c work well for you then I think
>>> the answer is "yes". ?There is infrastructure for register-access
>>> locking in those common types which might help your complex clocks.
>>>
>>> Thanks to the parent rate propagation stuff in clk_set_rate it should be
>>> possible for your drivers to only be aware of the gate and call
>>> clk_set_rate on only that clock, which propagates up to the divider and,
>>> if necessary, again propagates up to the mux.
>>>
>>> I encourage you to try that first. ?But if you find the semantics of
>>> those basic clock types aren't cutting it for you then you must create a
>>> type which is platform-specific.
>>
>> A lot of these mux/divider/gate clocks go out to peripherals, whose
>> drivers want to call both clk_set_rate() and clk_en/disable() on the clock.
>>
>> There's only 1 clock reaching the peripheral in HW, so the driver should
>> only call clk_get() once, and similarly the DT should only provide a
>> single clock to the driver.
>>
>
> Sounds good so far.
>
>> Given the mux->divider->gate clock construction, that clock would
>> presumably be the gate object. clk_en/disable() clearly work there, but
>> is clk_set_rate() intended to propagate up the chain until it can be
>> satisfied, i.e. does the gate clock object's set_rate() op simply call
>> the parent's set_rate() op?
>>
>
> Only if you set the CLK_SET_RATE_PARENT flag on the children. ?Take
> the following example sub-tree:
>
> mux
> ?|
> div
> ?|
> gate
>
> If gate does NOT have CLK_SET_RATE_PARENT set in struct clk->flags
> then clk_set_rate will do nothing (since it does not implement a
> .set_rate callback). ?If the flag is set then the framework will kick
> the rate request up to the parent, 'div'. ?Div implements a .set_rate
> callback and may or may not set the CLK_SET_RATE_PARENT flag. ?Having
> a .set_rate implemenation does preclude one from propagating the rate
> change up the tree, so it is possible to adjust the divider to a new
> rate AND still kick the rate change request up to the parent, 'mux'.
> In your case 'mux' should probably not set CLK_SET_RATE_PARENT and the
> propagation will end there. ?'mux' might implement a re-parenting
> operation as part of it's .set_rate implementation.
>
> This is covered fairly well in Documentation/clk.txt
>
>> If the order were instead mux->gate->divider, would it be correct for
>> enable/disable to propagate from the divider to the gate?
>>
>
> Correct. ?Order doesn't matter. ?Even in the absence of a
> .prepare/.enable callback the framework will still propagate these
> changes as part of typical use-counting rules.
>
>>>> 2) how to define the default parent? in many cases the hw reset value isn't
>>>> ? ?a very sensible choice, so the kernel probably needs to set a parent of
>>>> ? ?many of them if we don't want to rely on bootloader configuration.
>>>
>>> The only related thing handled at the framework level is _discovery_ of
>>> the parent during clock registration/initialization. ?If you don't trust
>>> the bootloader and want to set things up as soon as possible (a wise
>>> move) then I suggest you do so from your platform clock code at the same
>>> time that you register your clocks with the framework. ?Something like:
>>>
>>> ? ? ? struct clk *c;
>>> ? ? ? c = clk_register(...);
>>> ? ? ? if (IS_ERR(c))
>>> ? ? ? ? ? ? ? omg_fail();
>>> ? ? ? clk_set_parent(c, b);
>>>
>>> Where 'b' is a parent of 'c'. ?Register your clock tree top-down and you
>>> can re-parent as you go.
>>
>> I'm hoping we can represent this in device tree somehow, so that
>> individual boards can set the clock tree up differently depending on
>> their needs (e.g. Tegra20 doesn't have quite enough PLLs, so sometimes a
>> particular PLL will be used to generate the 2nd display's pixel clock,
>> whereas on other boards it may be used for some peripherals). So, we'd
>> like to describe this in DT.
>>
>> It seems like it'd be pretty common to want the kernel to fully
>> initialize the clock tree, and to do this from device tree, so perhaps
>> this might evolve into a common (part of) a cross-SoC clock binding, or
>> some kind of utility function that parsed a clock-tree-init-table from DT?
>
> As long as the configuration of your clocks can be expressed by the
> existing clk api, e.g. clk_prepare / clk_enable / clk_set_rate /
> clk_set_parent, then I think it would be easy to define a format
> whereby the clock is configured immediately after being registered.
> Essentially it would do what I outline in my original email above, but
> the configuration could be encoded into DT.
>
> Any platform-specific clock configuration (bits that aren't obviously
> expressed by the clk.h api) will be harder to configure through
> generic DT bindings.
>
> Regards,
> Mike
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Zhoujie Wu

moving Tegra30 to the common clock framework

[Turquette, Mike]

On Mon, May 7, 2012 at 10:07 PM, zhoujie wu <zhoujiewu@gmail.com> wrote:
> Hi Mike,
> Could you please explain more details about how to implement a
> re-parenting operation as part of it's .set_rate implementation?

Sure.

> As far as I know, we can not call clk_set_parent in .set_rate function
> directly, since clk_set_rate and clk_set_parent are using the same
> prepare_lock.

That is correct.

> Any other interface can be used to implement it?

You have two options available to you.

1) __clk_reparent can be used from your .set_rate callback today to
reflect changes made to the tree topology.  OMAP uses this in our PLL
.set_rate implementation: depending on the re-lock frequency the PLL
may switch parents dynamically.  __clk_reparent does the
framework-level cleanup needed for this (that function is also used
when populating the clock tree with new clock nodes).

2) __clk_set_parent could be made non-static if you needed this (I've
been meaning to talk to Saravana about this since I think MSM needs
something like this).  This is not done today but perhaps you can
experiment with it and let me know what you find?

If you stare at the code for a while I'm sure you'll see how it comes
together.  If any infrastructure that you require is missing please
let me know.

Regards,
Mike

> Thanks.
>
> 2012/5/8 Turquette, Mike <mturquette@ti.com>:
>> On Mon, May 7, 2012 at 8:39 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 05/06/2012 06:03 PM, Mike Turquette wrote:
>>>> On 20120503-19:13, Peter De Schrijver wrote:
>>>>> Hi,
>>>>>
>>>>> I started looking into what would be needed to move our tegra30 clock code
>>>>> to the common clock framework. The tegra30 clocktree is rather flat. Basically
>>>>> there are a bunch of sources (13 PLLs, external audio clocks, osc and 32Khz)
>>>>> and peripheral clocks which have a mux (with 4 or more inputs), a divider and
>>>>> a gate. So almost every peripheral clock can have multiple parents.
>>>>>
>>>>> Some questions:
>>>>>
>>>>> 1) should these peripheral clocks be modelled as 3 different clocks
>>>>> ? ?(mux -> divider -> gate) or would it be better to make a new clock type for
>>>>> ? ?this?
>>>>>
>>>>
>>>> That is really for you to decide. ?If the semantics of the existing mux,
>>>> divider and gate in drivers/clk/clk-*.c work well for you then I think
>>>> the answer is "yes". ?There is infrastructure for register-access
>>>> locking in those common types which might help your complex clocks.
>>>>
>>>> Thanks to the parent rate propagation stuff in clk_set_rate it should be
>>>> possible for your drivers to only be aware of the gate and call
>>>> clk_set_rate on only that clock, which propagates up to the divider and,
>>>> if necessary, again propagates up to the mux.
>>>>
>>>> I encourage you to try that first. ?But if you find the semantics of
>>>> those basic clock types aren't cutting it for you then you must create a
>>>> type which is platform-specific.
>>>
>>> A lot of these mux/divider/gate clocks go out to peripherals, whose
>>> drivers want to call both clk_set_rate() and clk_en/disable() on the clock.
>>>
>>> There's only 1 clock reaching the peripheral in HW, so the driver should
>>> only call clk_get() once, and similarly the DT should only provide a
>>> single clock to the driver.
>>>
>>
>> Sounds good so far.
>>
>>> Given the mux->divider->gate clock construction, that clock would
>>> presumably be the gate object. clk_en/disable() clearly work there, but
>>> is clk_set_rate() intended to propagate up the chain until it can be
>>> satisfied, i.e. does the gate clock object's set_rate() op simply call
>>> the parent's set_rate() op?
>>>
>>
>> Only if you set the CLK_SET_RATE_PARENT flag on the children. ?Take
>> the following example sub-tree:
>>
>> mux
>> ?|
>> div
>> ?|
>> gate
>>
>> If gate does NOT have CLK_SET_RATE_PARENT set in struct clk->flags
>> then clk_set_rate will do nothing (since it does not implement a
>> .set_rate callback). ?If the flag is set then the framework will kick
>> the rate request up to the parent, 'div'. ?Div implements a .set_rate
>> callback and may or may not set the CLK_SET_RATE_PARENT flag. ?Having
>> a .set_rate implemenation does preclude one from propagating the rate
>> change up the tree, so it is possible to adjust the divider to a new
>> rate AND still kick the rate change request up to the parent, 'mux'.
>> In your case 'mux' should probably not set CLK_SET_RATE_PARENT and the
>> propagation will end there. ?'mux' might implement a re-parenting
>> operation as part of it's .set_rate implementation.
>>
>> This is covered fairly well in Documentation/clk.txt
>>
>>> If the order were instead mux->gate->divider, would it be correct for
>>> enable/disable to propagate from the divider to the gate?
>>>
>>
>> Correct. ?Order doesn't matter. ?Even in the absence of a
>> .prepare/.enable callback the framework will still propagate these
>> changes as part of typical use-counting rules.
>>
>>>>> 2) how to define the default parent? in many cases the hw reset value isn't
>>>>> ? ?a very sensible choice, so the kernel probably needs to set a parent of
>>>>> ? ?many of them if we don't want to rely on bootloader configuration.
>>>>
>>>> The only related thing handled at the framework level is _discovery_ of
>>>> the parent during clock registration/initialization. ?If you don't trust
>>>> the bootloader and want to set things up as soon as possible (a wise
>>>> move) then I suggest you do so from your platform clock code at the same
>>>> time that you register your clocks with the framework. ?Something like:
>>>>
>>>> ? ? ? struct clk *c;
>>>> ? ? ? c = clk_register(...);
>>>> ? ? ? if (IS_ERR(c))
>>>> ? ? ? ? ? ? ? omg_fail();
>>>> ? ? ? clk_set_parent(c, b);
>>>>
>>>> Where 'b' is a parent of 'c'. ?Register your clock tree top-down and you
>>>> can re-parent as you go.
>>>
>>> I'm hoping we can represent this in device tree somehow, so that
>>> individual boards can set the clock tree up differently depending on
>>> their needs (e.g. Tegra20 doesn't have quite enough PLLs, so sometimes a
>>> particular PLL will be used to generate the 2nd display's pixel clock,
>>> whereas on other boards it may be used for some peripherals). So, we'd
>>> like to describe this in DT.
>>>
>>> It seems like it'd be pretty common to want the kernel to fully
>>> initialize the clock tree, and to do this from device tree, so perhaps
>>> this might evolve into a common (part of) a cross-SoC clock binding, or
>>> some kind of utility function that parsed a clock-tree-init-table from DT?
>>
>> As long as the configuration of your clocks can be expressed by the
>> existing clk api, e.g. clk_prepare / clk_enable / clk_set_rate /
>> clk_set_parent, then I think it would be easy to define a format
>> whereby the clock is configured immediately after being registered.
>> Essentially it would do what I outline in my original email above, but
>> the configuration could be encoded into DT.
>>
>> Any platform-specific clock configuration (bits that aren't obviously
>> expressed by the clk.h api) will be harder to configure through
>> generic DT bindings.
>>
>> Regards,
>> Mike
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> --
> Zhoujie Wu

moving Tegra30 to the common clock framework

[Saravana Kannan]

On 05/08/2012 10:15 AM, Turquette, Mike wrote:
> On Mon, May 7, 2012 at 10:07 PM, zhoujie wu<zhoujiewu@gmail.com>  wrote:
>> Hi Mike,
>> Could you please explain more details about how to implement a
>> re-parenting operation as part of it's .set_rate implementation?
>
> Sure.
>
>> As far as I know, we can not call clk_set_parent in .set_rate function
>> directly, since clk_set_rate and clk_set_parent are using the same
>> prepare_lock.
>
> That is correct.
>
>> Any other interface can be used to implement it?
>
> You have two options available to you.
>
> 1) __clk_reparent can be used from your .set_rate callback today to
> reflect changes made to the tree topology.  OMAP uses this in our PLL
> .set_rate implementation: depending on the re-lock frequency the PLL
> may switch parents dynamically.  __clk_reparent does the
> framework-level cleanup needed for this (that function is also used
> when populating the clock tree with new clock nodes).
>
> 2) __clk_set_parent could be made non-static if you needed this (I've
> been meaning to talk to Saravana about this since I think MSM needs
> something like this).

Thanks!

I don't think I need (2). But I don't think I can use (1) as is either. 
I can use (1) with some additional code in my set rate op.

While set rate is in progress, both the parents might need to stay 
enabled for a short duration. So, in my internal set rate, I need to 
check if my clock is prepared/enabled and call prepare/enable on the 
"old parent", call __clk_reparent (which will reduce the ref count for 
the old parents and increase it for the new parents), finish the 
reparent in HW and then unprepare/disable the old parent if I have 
prepared/enabled them earlier.

It might be beneficial to provide something like a 
__clk_reparent_start(new_parent, *scratch_pointer) and 
__clk_reparent_finish(*scratch_pointer) if it will be useful for more 
than just MSM. Based on this email, I would guess that Tegra would want 
something similar too.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

moving Tegra30 to the common clock framework

[skannan at codeaurora.org]

> On 05/08/2012 10:15 AM, Turquette, Mike wrote:
>> On Mon, May 7, 2012 at 10:07 PM, zhoujie wu<zhoujiewu@gmail.com>  wrote:
>>> Hi Mike,
>>> Could you please explain more details about how to implement a
>>> re-parenting operation as part of it's .set_rate implementation?
>>
>> Sure.
>>
>>> As far as I know, we can not call clk_set_parent in .set_rate function
>>> directly, since clk_set_rate and clk_set_parent are using the same
>>> prepare_lock.
>>
>> That is correct.
>>
>>> Any other interface can be used to implement it?
>>
>> You have two options available to you.
>>
>> 1) __clk_reparent can be used from your .set_rate callback today to
>> reflect changes made to the tree topology.  OMAP uses this in our PLL
>> .set_rate implementation: depending on the re-lock frequency the PLL
>> may switch parents dynamically.  __clk_reparent does the
>> framework-level cleanup needed for this (that function is also used
>> when populating the clock tree with new clock nodes).
>>
>> 2) __clk_set_parent could be made non-static if you needed this (I've
>> been meaning to talk to Saravana about this since I think MSM needs
>> something like this).
>
> Thanks!
>
> I don't think I need (2). But I don't think I can use (1) as is either.
> I can use (1) with some additional code in my set rate op.
>
> While set rate is in progress, both the parents might need to stay
> enabled for a short duration. So, in my internal set rate, I need to
> check if my clock is prepared/enabled and call prepare/enable on the
> "old parent", call __clk_reparent (which will reduce the ref count for
> the old parents and increase it for the new parents), finish the
> reparent in HW and then unprepare/disable the old parent if I have
> prepared/enabled them earlier.
>
> It might be beneficial to provide something like a
> __clk_reparent_start(new_parent, *scratch_pointer) and
> __clk_reparent_finish(*scratch_pointer) if it will be useful for more
> than just MSM. Based on this email, I would guess that Tegra would want
> something similar too.

Thinking more about this, I think this is how any clk op that might change
the parent should operate. I will try to write up an RFC patch for this
and send it out soon. I'm in a hurry, so will explain more in the RFC
patch or in a later email.

Thanks,
Saravana

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

moving Tegra30 to the common clock framework

[Turquette, Mike]

On 20120508-19:20, skannan at codeaurora.org wrote:
> > While set rate is in progress, both the parents might need to stay
> > enabled for a short duration. So, in my internal set rate, I need to
> > check if my clock is prepared/enabled and call prepare/enable on the
> > "old parent", call __clk_reparent (which will reduce the ref count for
> > the old parents and increase it for the new parents), finish the
> > reparent in HW and then unprepare/disable the old parent if I have
> > prepared/enabled them earlier.
> >

The .set_rate implemenation for OMAP's PLLs has the same requirement and
this is handled by nested calls to __clk_prepare and clk_enable.  Please
refer the omap3_noncore_dpll_set_rate definition:

http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/dpll3xxx.c;h=63e92e2d4bb80ea702572b919c17b0ac9f93cc0e;hb=clk-next-omap#l425

> > It might be beneficial to provide something like a
> > __clk_reparent_start(new_parent, *scratch_pointer) and
> > __clk_reparent_finish(*scratch_pointer) if it will be useful for more
> > than just MSM. Based on this email, I would guess that Tegra would want
> > something similar too.
>
> Thinking more about this, I think this is how any clk op that might change
> the parent should operate. I will try to write up an RFC patch for this
> and send it out soon. I'm in a hurry, so will explain more in the RFC
> patch or in a later email.

I'm interested to see your patch, as I might not be fully understanding
your requirements.  Is there any reason why making nested calls to
__clk_prepare and clk_enable from your .set_rate callback isn't enough?

Thanks,
Mike

moving Tegra30 to the common clock framework

[Peter De Schrijver]

On Wed, May 09, 2012 at 02:41:37AM +0200, Saravana Kannan wrote:
> On 05/08/2012 10:15 AM, Turquette, Mike wrote:
> > On Mon, May 7, 2012 at 10:07 PM, zhoujie wu<zhoujiewu@gmail.com>  wrote:
> >> Hi Mike,
> >> Could you please explain more details about how to implement a
> >> re-parenting operation as part of it's .set_rate implementation?
> >
> > Sure.
> >
> >> As far as I know, we can not call clk_set_parent in .set_rate function
> >> directly, since clk_set_rate and clk_set_parent are using the same
> >> prepare_lock.
> >
> > That is correct.
> >
> >> Any other interface can be used to implement it?
> >
> > You have two options available to you.
> >
> > 1) __clk_reparent can be used from your .set_rate callback today to
> > reflect changes made to the tree topology.  OMAP uses this in our PLL
> > .set_rate implementation: depending on the re-lock frequency the PLL
> > may switch parents dynamically.  __clk_reparent does the
> > framework-level cleanup needed for this (that function is also used
> > when populating the clock tree with new clock nodes).
> >
> > 2) __clk_set_parent could be made non-static if you needed this (I've
> > been meaning to talk to Saravana about this since I think MSM needs
> > something like this).
> 
> Thanks!
> 
> I don't think I need (2). But I don't think I can use (1) as is either. 
> I can use (1) with some additional code in my set rate op.
> 
> While set rate is in progress, both the parents might need to stay 
> enabled for a short duration. So, in my internal set rate, I need to 
> check if my clock is prepared/enabled and call prepare/enable on the 
> "old parent", call __clk_reparent (which will reduce the ref count for 
> the old parents and increase it for the new parents), finish the 
> reparent in HW and then unprepare/disable the old parent if I have 
> prepared/enabled them earlier.
> 
> It might be beneficial to provide something like a 
> __clk_reparent_start(new_parent, *scratch_pointer) and 
> __clk_reparent_finish(*scratch_pointer) if it will be useful for more 
> than just MSM. Based on this email, I would guess that Tegra would want 
> something similar too.
> 

We also need to reparent clocks using a pll if we want to change the PLLs rate
while the users are active.

Cheers,

Peter.

moving Tegra30 to the common clock framework

[Peter De Schrijver]

On Mon, May 07, 2012 at 02:03:29AM +0200, Mike Turquette wrote:
> On 20120503-19:13, Peter De Schrijver wrote:
> > Hi,
> > 
> > I started looking into what would be needed to move our tegra30 clock code
> > to the common clock framework. The tegra30 clocktree is rather flat. Basically
> > there are a bunch of sources (13 PLLs, external audio clocks, osc and 32Khz)
> > and peripheral clocks which have a mux (with 4 or more inputs), a divider and
> > a gate. So almost every peripheral clock can have multiple parents.
> > 
> > Some questions:
> > 
> > 1) should these peripheral clocks be modelled as 3 different clocks
> >    (mux -> divider -> gate) or would it be better to make a new clock type for
> >    this?
> > 
> 
> That is really for you to decide.  If the semantics of the existing mux,
> divider and gate in drivers/clk/clk-*.c work well for you then I think
> the answer is "yes".  There is infrastructure for register-access
> locking in those common types which might help your complex clocks.
> 
> Thanks to the parent rate propagation stuff in clk_set_rate it should be
> possible for your drivers to only be aware of the gate and call
> clk_set_rate on only that clock, which propagates up to the divider and,
> if necessary, again propagates up to the mux.
> 
> I encourage you to try that first.  But if you find the semantics of
> those basic clock types aren't cutting it for you then you must create a
> type which is platform-specific.
> 
> > 2) how to define the default parent? in many cases the hw reset value isn't
> >    a very sensible choice, so the kernel probably needs to set a parent of
> >    many of them if we don't want to rely on bootloader configuration.
> 
> The only related thing handled at the framework level is _discovery_ of
> the parent during clock registration/initialization.  If you don't trust
> the bootloader and want to set things up as soon as possible (a wise
> move) then I suggest you do so from your platform clock code at the same
> time that you register your clocks with the framework.  Something like:
> 
> 	struct clk *c;
> 	c = clk_register(...);
> 	if (IS_ERR(c))
> 		omg_fail();
> 	clk_set_parent(c, b);
> 
> Where 'b' is a parent of 'c'.  Register your clock tree top-down and you
> can re-parent as you go.

Ok. Thanks. One more question. We have some clocks with special features
such as request lines for clock outputs, delays for clocks recovered from
an external source or several divisors which are used based on the state
of the module which is served by the clock (eg. an idle divisor and an active
divisor). How should these be modelled?

Thanks,

Peter.

moving Tegra30 to the common clock framework

[Mike Turquette]

On 20120509-14:13, Peter De Schrijver wrote:
> Ok. Thanks. One more question. We have some clocks with special features
> such as request lines for clock outputs, delays for clocks recovered from
> an external source or several divisors which are used based on the state
> of the module which is served by the clock (eg. an idle divisor and an active
> divisor). How should these be modelled?

My platform also has some "special" clocks.  The worst case is that you
must create some platform clock types and eschew the common basic types.
A common example of this is that everyone has their own PLL
implementation.

However it is possible to subclass some existing basic types if your
needs aren't too complicated.  Check out the patch from Andrew that
builds on top of the basic gate:
http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=commitdiff;h=98d9986cb8bf65f8316b45244fdafc1d12c303be;hp=e919c71665d2386eec6dc2ecd58d01bae69fc0fd

In fact the basic clock types were originally discussed as targeting
only "simple platforms".  I don't think anyone thought that we would get
as much use out of them as we have had for complex SoCs.

Regards,
Mike

moving Tegra30 to the common clock framework

[Saravana Kannan]

On 05/08/2012 11:21 PM, Turquette, Mike wrote:
> On 20120508-19:20, skannan at codeaurora.org wrote:
>>> It might be beneficial to provide something like a
>>> __clk_reparent_start(new_parent, *scratch_pointer) and
>>> __clk_reparent_finish(*scratch_pointer) if it will be useful for more
>>> than just MSM. Based on this email, I would guess that Tegra would want
>>> something similar too.
>>
>> Thinking more about this, I think this is how any clk op that might change
>> the parent should operate. I will try to write up an RFC patch for this
>> and send it out soon. I'm in a hurry, so will explain more in the RFC
>> patch or in a later email.
>
> I'm interested to see your patch, as I might not be fully understanding
> your requirements.  Is there any reason why making nested calls to
> __clk_prepare and clk_enable from your .set_rate callback isn't enough?
>

Sorry if I wasn't clear -- my plan with the current framework IS to make 
nested (as in, inside the set rate op) calls to __clk_prepare() and 
enable() as needed. But my point is that everyone really wants to do the 
same, so we might as well make it better by putting it in the common 
code and having everyone use it instead of having to redo the same code 
all over the place.

Will send out the patch soon.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

moving Tegra30 to the common clock framework

[Peter De Schrijver]

On Wed, May 09, 2012 at 06:49:15PM +0200, Mike Turquette wrote:
> On 20120509-14:13, Peter De Schrijver wrote:
> > Ok. Thanks. One more question. We have some clocks with special features
> > such as request lines for clock outputs, delays for clocks recovered from
> > an external source or several divisors which are used based on the state
> > of the module which is served by the clock (eg. an idle divisor and an active
> > divisor). How should these be modelled?
> 
> My platform also has some "special" clocks.  The worst case is that you
> must create some platform clock types and eschew the common basic types.
> A common example of this is that everyone has their own PLL
> implementation.
> 

Ok. I was more wondering on how to expose those features which don't fit into
the normal clock API. Currently we have tegra_clk_cfg_ex() which is some kind
of ioctl for clocks.

> However it is possible to subclass some existing basic types if your
> needs aren't too complicated.  Check out the patch from Andrew that
> builds on top of the basic gate:
> http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=commitdiff;h=98d9986cb8bf65f8316b45244fdafc1d12c303be;hp=e919c71665d2386eec6dc2ecd58d01bae69fc0fd
> 
> In fact the basic clock types were originally discussed as targeting
> only "simple platforms".  I don't think anyone thought that we would get
> as much use out of them as we have had for complex SoCs.

Ok. Might be useful at some point.

Cheers,

Peter.

moving Tegra30 to the common clock framework

[Saravana Kannan]

On 05/09/2012 03:36 AM, Peter De Schrijver wrote:
> On Wed, May 09, 2012 at 02:41:37AM +0200, Saravana Kannan wrote:
>> On 05/08/2012 10:15 AM, Turquette, Mike wrote:
>>> On Mon, May 7, 2012 at 10:07 PM, zhoujie wu<zhoujiewu@gmail.com>   wrote:
>>>> Hi Mike,
>>>> Could you please explain more details about how to implement a
>>>> re-parenting operation as part of it's .set_rate implementation?
>>>
>>> Sure.
>>>
>>>> As far as I know, we can not call clk_set_parent in .set_rate function
>>>> directly, since clk_set_rate and clk_set_parent are using the same
>>>> prepare_lock.
>>>
>>> That is correct.
>>>
>>>> Any other interface can be used to implement it?
>>>
>>> You have two options available to you.
>>>
>>> 1) __clk_reparent can be used from your .set_rate callback today to
>>> reflect changes made to the tree topology.  OMAP uses this in our PLL
>>> .set_rate implementation: depending on the re-lock frequency the PLL
>>> may switch parents dynamically.  __clk_reparent does the
>>> framework-level cleanup needed for this (that function is also used
>>> when populating the clock tree with new clock nodes).
>>>
>>> 2) __clk_set_parent could be made non-static if you needed this (I've
>>> been meaning to talk to Saravana about this since I think MSM needs
>>> something like this).
>>
>> Thanks!
>>
>> I don't think I need (2). But I don't think I can use (1) as is either.
>> I can use (1) with some additional code in my set rate op.
>>
>> While set rate is in progress, both the parents might need to stay
>> enabled for a short duration. So, in my internal set rate, I need to
>> check if my clock is prepared/enabled and call prepare/enable on the
>> "old parent", call __clk_reparent (which will reduce the ref count for
>> the old parents and increase it for the new parents), finish the
>> reparent in HW and then unprepare/disable the old parent if I have
>> prepared/enabled them earlier.
>>
>> It might be beneficial to provide something like a
>> __clk_reparent_start(new_parent, *scratch_pointer) and
>> __clk_reparent_finish(*scratch_pointer) if it will be useful for more
>> than just MSM. Based on this email, I would guess that Tegra would want
>> something similar too.
>>
>
> We also need to reparent clocks using a pll if we want to change the PLLs rate
> while the users are active.

Peter,

Is this reparent permanent (as in, stays reparented when you return from 
clk_set_rate()) or is it a reparenting for just a short duration inside 
the set_rate ops?

If it's the latter, I don't think you would need any helper code in 
clock framework other than the already existing __clk_prepare() and 
clk_enable(). Just turn on the temp parent, reparent to it, do whatever 
you need to do, and go back to your original parent.

If it's the former (permanent reparent), can I assume that the 
CLK_SET_RATE_PARENT flag won't be set for this clock? Otherwise, it's 
going to be quite yucky/convoluted. I'm not sure how well a reparent 
would work with the code in common clock framwework that walks up the 
parents to propagate the rate change to them. I wouldn't say that it's 
wrong to only want to propagate the rate for certain rates and for 
certain parents, but I will have to stare at the common clock code for a 
while.

Mike,

I was looking at the code to make the changes and I noticed this snippet 
(reformatted for email) in clk_change_rate():

        if (clk->ops->set_rate)
                 clk->ops->set_rate(clk->hw, clk->new_rate,
					clk->parent->rate);


         if (clk->ops->recalc_rate)
                 clk->rate = clk->ops->recalc_rate(clk->hw,
                                 clk->parent->rate);
         else
                 clk->rate = clk->parent->rate;

I'm a bit confused. I thought recalc_rates was optional. But if I don't 
implement it, the clocks rate will get set to parent's rate? Or is that 
a bug in the code?

Also, if the clock's rate was just set with set_rate, why do we need to 
recalc the rate by reading hardware? I'm a bit confused. Can you please 
clarify what's going on here?

Would you mind adding more comments inside clk_calc_new_rates() and 
clk_change_rate() trying to explain what cases you are trying to account 
for?

Also, in clk_calc_new_rates(),

         if (!clk->ops->round_rate) {
                 top = clk_calc_new_rates(clk->parent, rate);
                 new_rate = clk->parent->new_rate;

                 goto out;
         }

Is the code assuming that if there is no round rate ops that that clock 
node is only a gating clock (as in, can't change frequency the input 
freq)? Just trying to understand the assumptions made in the code.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

moving Tegra30 to the common clock framework

[Mark Brown]

On Wed, May 09, 2012 at 02:13:35PM +0300, Peter De Schrijver wrote:

> Ok. Thanks. One more question. We have some clocks with special features
> such as request lines for clock outputs, delays for clocks recovered from
> an external source or several divisors which are used based on the state
> of the module which is served by the clock (eg. an idle divisor and an active
> divisor). How should these be modelled?

For the idle/active divisors could they be represented as separate
clocks?

moving Tegra30 to the common clock framework

[Stephen Warren]

On 05/11/2012 08:58 PM, Saravana Kannan wrote:
> On 05/09/2012 03:36 AM, Peter De Schrijver wrote:
>> On Wed, May 09, 2012 at 02:41:37AM +0200, Saravana Kannan wrote:
>>> On 05/08/2012 10:15 AM, Turquette, Mike wrote:
>>>> On Mon, May 7, 2012 at 10:07 PM, zhoujie wu<zhoujiewu@gmail.com>  
>>>> wrote:
>>>>> Hi Mike,
>>>>> Could you please explain more details about how to implement a
>>>>> re-parenting operation as part of it's .set_rate implementation?
>>>>
>>>> Sure.
>>>>
>>>>> As far as I know, we can not call clk_set_parent in .set_rate function
>>>>> directly, since clk_set_rate and clk_set_parent are using the same
>>>>> prepare_lock.
>>>>
>>>> That is correct.
>>>>
>>>>> Any other interface can be used to implement it?
>>>>
>>>> You have two options available to you.
>>>>
>>>> 1) __clk_reparent can be used from your .set_rate callback today to
>>>> reflect changes made to the tree topology.  OMAP uses this in our PLL
>>>> .set_rate implementation: depending on the re-lock frequency the PLL
>>>> may switch parents dynamically.  __clk_reparent does the
>>>> framework-level cleanup needed for this (that function is also used
>>>> when populating the clock tree with new clock nodes).
>>>>
>>>> 2) __clk_set_parent could be made non-static if you needed this (I've
>>>> been meaning to talk to Saravana about this since I think MSM needs
>>>> something like this).
>>>
>>> Thanks!
>>>
>>> I don't think I need (2). But I don't think I can use (1) as is either.
>>> I can use (1) with some additional code in my set rate op.
>>>
>>> While set rate is in progress, both the parents might need to stay
>>> enabled for a short duration. So, in my internal set rate, I need to
>>> check if my clock is prepared/enabled and call prepare/enable on the
>>> "old parent", call __clk_reparent (which will reduce the ref count for
>>> the old parents and increase it for the new parents), finish the
>>> reparent in HW and then unprepare/disable the old parent if I have
>>> prepared/enabled them earlier.
>>>
>>> It might be beneficial to provide something like a
>>> __clk_reparent_start(new_parent, *scratch_pointer) and
>>> __clk_reparent_finish(*scratch_pointer) if it will be useful for more
>>> than just MSM. Based on this email, I would guess that Tegra would want
>>> something similar too.
>>>
>>
>> We also need to reparent clocks using a pll if we want to change the
>> PLLs rate
>> while the users are active.
> 
> Peter,
> 
> Is this reparent permanent (as in, stays reparented when you return from
> clk_set_rate()) or is it a reparenting for just a short duration inside
> the set_rate ops?

I've seen both cases, and indeed the case sometimes depends on the
target rate of the clock.

For example, when the CPU clock changes, we basically do the following
within set_rate:

* Set CPU clock parent to some "backup" PLL
* Change the CPU PLL to the desired rate
* Set CPU clock parent to the CPU PLL

However, the lowest CPU clock rate we support is actually the rate that
the backup PLL runs at, so if we're targeting that rate, the CPU clock
set_rate /just/ does:

* Set CPU clock parent to some "backup" PLL

and leaves it there, until a different CPU clock rate is requested, at
which time the CPU clock will be re-parented back to the CPU PLL.

moving Tegra30 to the common clock framework

[Peter De Schrijver]

On Sun, May 13, 2012 at 06:31:42AM +0200, Stephen Warren wrote:
> On 05/11/2012 08:58 PM, Saravana Kannan wrote:
> > On 05/09/2012 03:36 AM, Peter De Schrijver wrote:
> >> On Wed, May 09, 2012 at 02:41:37AM +0200, Saravana Kannan wrote:
> >>> On 05/08/2012 10:15 AM, Turquette, Mike wrote:
> >>>> On Mon, May 7, 2012 at 10:07 PM, zhoujie wu<zhoujiewu@gmail.com>  
> >>>> wrote:
> >>>>> Hi Mike,
> >>>>> Could you please explain more details about how to implement a
> >>>>> re-parenting operation as part of it's .set_rate implementation?
> >>>>
> >>>> Sure.
> >>>>
> >>>>> As far as I know, we can not call clk_set_parent in .set_rate function
> >>>>> directly, since clk_set_rate and clk_set_parent are using the same
> >>>>> prepare_lock.
> >>>>
> >>>> That is correct.
> >>>>
> >>>>> Any other interface can be used to implement it?
> >>>>
> >>>> You have two options available to you.
> >>>>
> >>>> 1) __clk_reparent can be used from your .set_rate callback today to
> >>>> reflect changes made to the tree topology.  OMAP uses this in our PLL
> >>>> .set_rate implementation: depending on the re-lock frequency the PLL
> >>>> may switch parents dynamically.  __clk_reparent does the
> >>>> framework-level cleanup needed for this (that function is also used
> >>>> when populating the clock tree with new clock nodes).
> >>>>
> >>>> 2) __clk_set_parent could be made non-static if you needed this (I've
> >>>> been meaning to talk to Saravana about this since I think MSM needs
> >>>> something like this).
> >>>
> >>> Thanks!
> >>>
> >>> I don't think I need (2). But I don't think I can use (1) as is either.
> >>> I can use (1) with some additional code in my set rate op.
> >>>
> >>> While set rate is in progress, both the parents might need to stay
> >>> enabled for a short duration. So, in my internal set rate, I need to
> >>> check if my clock is prepared/enabled and call prepare/enable on the
> >>> "old parent", call __clk_reparent (which will reduce the ref count for
> >>> the old parents and increase it for the new parents), finish the
> >>> reparent in HW and then unprepare/disable the old parent if I have
> >>> prepared/enabled them earlier.
> >>>
> >>> It might be beneficial to provide something like a
> >>> __clk_reparent_start(new_parent, *scratch_pointer) and
> >>> __clk_reparent_finish(*scratch_pointer) if it will be useful for more
> >>> than just MSM. Based on this email, I would guess that Tegra would want
> >>> something similar too.
> >>>
> >>
> >> We also need to reparent clocks using a pll if we want to change the
> >> PLLs rate
> >> while the users are active.
> > 
> > Peter,
> > 
> > Is this reparent permanent (as in, stays reparented when you return from
> > clk_set_rate()) or is it a reparenting for just a short duration inside
> > the set_rate ops?
> 
> I've seen both cases, and indeed the case sometimes depends on the
> target rate of the clock.
> 
> For example, when the CPU clock changes, we basically do the following
> within set_rate:
> 
> * Set CPU clock parent to some "backup" PLL
> * Change the CPU PLL to the desired rate
> * Set CPU clock parent to the CPU PLL
> 
> However, the lowest CPU clock rate we support is actually the rate that
> the backup PLL runs at, so if we're targeting that rate, the CPU clock
> set_rate /just/ does:
> 
> * Set CPU clock parent to some "backup" PLL
> 
> and leaves it there, until a different CPU clock rate is requested, at
> which time the CPU clock will be re-parented back to the CPU PLL.

The backup PLL and rate are statically defined however. It's not chosen at
runtime.

Cheers,

Peter.

moving Tegra30 to the common clock framework

[Peter De Schrijver]

On Sat, May 12, 2012 at 08:04:41PM +0200, Mark Brown wrote:
> On Wed, May 09, 2012 at 02:13:35PM +0300, Peter De Schrijver wrote:
> 
> > Ok. Thanks. One more question. We have some clocks with special features
> > such as request lines for clock outputs, delays for clocks recovered from
> > an external source or several divisors which are used based on the state
> > of the module which is served by the clock (eg. an idle divisor and an active
> > divisor). How should these be modelled?
> 
> For the idle/active divisors could they be represented as separate
> clocks?

I guess that might be possible. Need to look into it.

Cheers,

Peter.

moving Tegra30 to the common clock framework

[Peter De Schrijver]

On Wed, May 09, 2012 at 01:13:35PM +0200, Peter De Schrijver wrote:
> 
> Ok. Thanks. One more question. We have some clocks with special features
> such as request lines for clock outputs, delays for clocks recovered from
> an external source or several divisors which are used based on the state
> of the module which is served by the clock (eg. an idle divisor and an active
> divisor). How should these be modelled?

One more thing: we have muxes where not all possible register values are
valid. I could make a special mux type which contains a mapping array, but
that doesn't seem like a good solution to me. Is there a reason why we don't
have a struct clk_parent like this:

struct clk_parents {
	char *name;
	struct clk *clk;
	int index;
}

where index is the value to be written to the hw to select that clock as a
parent. It would also avoid having to pass 2 arrays (name and clk pointer).

Cheers,

Peter.

moving Tegra30 to the common clock framework

[Turquette, Mike]

On Fri, May 11, 2012 at 7:58 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> Mike,
>
> I was looking at the code to make the changes and I noticed this snippet
> (reformatted for email) in clk_change_rate():
>
> ? ? ? if (clk->ops->set_rate)
> ? ? ? ? ? ? ? ?clk->ops->set_rate(clk->hw, clk->new_rate,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?clk->parent->rate);
>
>
> ? ? ? ?if (clk->ops->recalc_rate)
> ? ? ? ? ? ? ? ?clk->rate = clk->ops->recalc_rate(clk->hw,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?clk->parent->rate);
> ? ? ? ?else
> ? ? ? ? ? ? ? ?clk->rate = clk->parent->rate;
>
> I'm a bit confused. I thought recalc_rates was optional. But if I don't
> implement it, the clocks rate will get set to parent's rate? Or is that a
> bug in the code?
>

It is not a bug.  The handsome ascii art in Documentation/clk.txt covers
this requirement.  I have copy/pasted the relevant bits below for
convenience:

                           clock hardware characteristics
	     -----------------------------------------------------------
             | gate | change rate | single parent | multiplexer | root |
             |------|-------------|---------------|-------------|------|
	     				...
             |      |             |               |             |      |
.recalc_rate |      | y           |               |             |      |
.round_rate  |      | y           |               |             |      |
.set_rate    |      | y           |               |             |      |
					...
             |      |             |               |             |      |
	     -----------------------------------------------------------

The take-away is that a clock that can adjust its rate (eg: implements a
.set_rate callback) must also implement .recalc_rate and .round_rate
callbacks.

> Also, if the clock's rate was just set with set_rate, why do we need to
> recalc the rate by reading hardware? I'm a bit confused. Can you please
> clarify what's going on here?
>

This is simply being very cautious.  For platforms adjusting dividers
with direct register writes this might feel unnecessary.  However this
strict checking is in anticipation of clock hardware that might not
actually output the precise rate passed into .set_rate.  In principal
this isn't different from how CPUfreq and devfreq drivers inspect rates
after requesting them.

> Would you mind adding more comments inside clk_calc_new_rates() and
> clk_change_rate() trying to explain what cases you are trying to account
> for?
>

Someday I'll have a comment/kerneldoc cleanup session.  Things in the
clock core are likely to change in the next couple of release cycles
which will deprecate some of the kerneldoc making a big cleanup
unavoidable.

In the mean time are there any other specific question you have for
clk_change_rate?

> Also, in clk_calc_new_rates(),
>
> ? ? ? ?if (!clk->ops->round_rate) {
> ? ? ? ? ? ? ? ?top = clk_calc_new_rates(clk->parent, rate);
> ? ? ? ? ? ? ? ?new_rate = clk->parent->new_rate;
>
> ? ? ? ? ? ? ? ?goto out;
> ? ? ? ?}
>
> Is the code assuming that if there is no round rate ops that that clock node
> is only a gating clock (as in, can't change frequency the input freq)? Just
> trying to understand the assumptions made in the code.
>

This is also covered by the ascii art above.  There is no assumption
about gating, per se.  However if a clock can adjust it's rate (more
specifically, if the input rate for a clock differs from the output
rate) then a .round_rate callback must exist.

The code above reads like it does because in the absence of a
.round_rate callback it is implied that the clock cannot set it's own
rate and should thus rely on its parent's rate.

Regards,
Mike

>
> Thanks,
> Saravana
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

moving Tegra30 to the common clock framework

[Saravana Kannan]

On 05/14/2012 02:36 PM, Turquette, Mike wrote:
> On Fri, May 11, 2012 at 7:58 PM, Saravana Kannan<skannan@codeaurora.org>  wrote:
>> Mike,
>>
>> I was looking at the code to make the changes and I noticed this snippet
>> (reformatted for email) in clk_change_rate():
>>
>>        if (clk->ops->set_rate)
>>                 clk->ops->set_rate(clk->hw, clk->new_rate,
>>                                         clk->parent->rate);
>>
>>
>>         if (clk->ops->recalc_rate)
>>                 clk->rate = clk->ops->recalc_rate(clk->hw,
>>                                 clk->parent->rate);
>>         else
>>                 clk->rate = clk->parent->rate;
>>
>> I'm a bit confused. I thought recalc_rates was optional. But if I don't
>> implement it, the clocks rate will get set to parent's rate? Or is that a
>> bug in the code?
>>
>
> It is not a bug.  The handsome ascii art in Documentation/clk.txt covers
> this requirement.  I have copy/pasted the relevant bits below for
> convenience:
>
>                             clock hardware characteristics
> 	     -----------------------------------------------------------
>               | gate | change rate | single parent | multiplexer | root |
>               |------|-------------|---------------|-------------|------|
> 	     				...
>               |      |             |               |             |      |
> .recalc_rate |      | y           |               |             |      |
> .round_rate  |      | y           |               |             |      |
> .set_rate    |      | y           |               |             |      |
> 					...
>               |      |             |               |             |      |
> 	     -----------------------------------------------------------
>
> The take-away is that a clock that can adjust its rate (eg: implements a
> .set_rate callback) must also implement .recalc_rate and .round_rate
> callbacks.

I get the round_rate ops part. But I don't see a need to force a 
recalc_rate ops. Can we just check for the existence of .set_rate() to 
figure out if the clock will take up the rate of the parent or will 
output a different rate?

>
>> Also, if the clock's rate was just set with set_rate, why do we need to
>> recalc the rate by reading hardware? I'm a bit confused. Can you please
>> clarify what's going on here?
>>
>
> This is simply being very cautious.  For platforms adjusting dividers
> with direct register writes this might feel unnecessary.  However this
> strict checking is in anticipation of clock hardware that might not
> actually output the precise rate passed into .set_rate.  In principal
> this isn't different from how CPUfreq and devfreq drivers inspect rates
> after requesting them.

Sorry, this still doesn't make much sense to me. This essentially means 
we can't trust the HW to do what we are asking it to do?

CPUfreq and devfreq are clients of external clock APIs. That's different 
from whether the HW will do what the platform driver will ask it to.

Even if this unusual HW exists, I certainly don't want to deal with 
recalc_rate(). It's also quite a bit of register reads that I would like 
to avoid. I want to keep the clock APIs as fast as I can since it 
affects power.

Can we do something like this in clk_change_rate()?

if (ops->set_rate) {
	ops->set_rate(clk->new_rate,...);
	clk->rate = clk->new_rate;
} else {
	clk->rate = clk->parent->rate;
}

if (ops->recalc_rate()) {
	clk->rate = ops->recalc_rate(...);
}

This code also makes the assumptions more intuitive and easy to understand.

>> Would you mind adding more comments inside clk_calc_new_rates() and
>> clk_change_rate() trying to explain what cases you are trying to account
>> for?
>>
>
> Someday I'll have a comment/kerneldoc cleanup session.  Things in the
> clock core are likely to change in the next couple of release cycles
> which will deprecate some of the kerneldoc making a big cleanup
> unavoidable.
>
> In the mean time are there any other specific question you have for
> clk_change_rate?
>
>> Also, in clk_calc_new_rates(),
>>
>>         if (!clk->ops->round_rate) {
>>                 top = clk_calc_new_rates(clk->parent, rate);
>>                 new_rate = clk->parent->new_rate;
>>
>>                 goto out;
>>         }
>>
>> Is the code assuming that if there is no round rate ops that that clock node
>> is only a gating clock (as in, can't change frequency the input freq)? Just
>> trying to understand the assumptions made in the code.
>>
>
> This is also covered by the ascii art above.  There is no assumption
> about gating, per se.  However if a clock can adjust it's rate (more
> specifically, if the input rate for a clock differs from the output
> rate) then a .round_rate callback must exist.
>
> The code above reads like it does because in the absence of a
> .round_rate callback it is implied that the clock cannot set it's own
> rate and should thus rely on its parent's rate.

I agree that anyone implementing .set_rate should also provide 
.round_rate. May be we should enforce that in clk_init/clk_register?

But can we please the above "if" check more explicit? "Your rate has to 
be same as the parent rate since you don't implement set rate" is 
clearer than "Your rate has to be the same as the parent rate since you 
don't implement round rate".

	if (!clk->ops->set_rate) {
		top = clk_calc_new_rates(clk->parent, rate);
		new_rate = clk->parent->new_rate;

		goto out;
	}

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

moving Tegra30 to the common clock framework

[Saravana Kannan]

On 05/14/2012 04:08 AM, Peter De Schrijver wrote:
> On Sun, May 13, 2012 at 06:31:42AM +0200, Stephen Warren wrote:
>> On 05/11/2012 08:58 PM, Saravana Kannan wrote:
>>> On 05/09/2012 03:36 AM, Peter De Schrijver wrote:
>>>> On Wed, May 09, 2012 at 02:41:37AM +0200, Saravana Kannan wrote:
>>>>> On 05/08/2012 10:15 AM, Turquette, Mike wrote:
>>>>>> On Mon, May 7, 2012 at 10:07 PM, zhoujie wu<zhoujiewu@gmail.com>
>>>>>> wrote:
>>>>>>> Hi Mike,
>>>>>>> Could you please explain more details about how to implement a
>>>>>>> re-parenting operation as part of it's .set_rate implementation?
>>>>>>
>>>>>> Sure.
>>>>>>
>>>>>>> As far as I know, we can not call clk_set_parent in .set_rate function
>>>>>>> directly, since clk_set_rate and clk_set_parent are using the same
>>>>>>> prepare_lock.
>>>>>>
>>>>>> That is correct.
>>>>>>
>>>>>>> Any other interface can be used to implement it?
>>>>>>
>>>>>> You have two options available to you.
>>>>>>
>>>>>> 1) __clk_reparent can be used from your .set_rate callback today to
>>>>>> reflect changes made to the tree topology.  OMAP uses this in our PLL
>>>>>> .set_rate implementation: depending on the re-lock frequency the PLL
>>>>>> may switch parents dynamically.  __clk_reparent does the
>>>>>> framework-level cleanup needed for this (that function is also used
>>>>>> when populating the clock tree with new clock nodes).
>>>>>>
>>>>>> 2) __clk_set_parent could be made non-static if you needed this (I've
>>>>>> been meaning to talk to Saravana about this since I think MSM needs
>>>>>> something like this).
>>>>>
>>>>> Thanks!
>>>>>
>>>>> I don't think I need (2). But I don't think I can use (1) as is either.
>>>>> I can use (1) with some additional code in my set rate op.
>>>>>
>>>>> While set rate is in progress, both the parents might need to stay
>>>>> enabled for a short duration. So, in my internal set rate, I need to
>>>>> check if my clock is prepared/enabled and call prepare/enable on the
>>>>> "old parent", call __clk_reparent (which will reduce the ref count for
>>>>> the old parents and increase it for the new parents), finish the
>>>>> reparent in HW and then unprepare/disable the old parent if I have
>>>>> prepared/enabled them earlier.
>>>>>
>>>>> It might be beneficial to provide something like a
>>>>> __clk_reparent_start(new_parent, *scratch_pointer) and
>>>>> __clk_reparent_finish(*scratch_pointer) if it will be useful for more
>>>>> than just MSM. Based on this email, I would guess that Tegra would want
>>>>> something similar too.
>>>>>
>>>>
>>>> We also need to reparent clocks using a pll if we want to change the
>>>> PLLs rate
>>>> while the users are active.
>>>
>>> Peter,
>>>
>>> Is this reparent permanent (as in, stays reparented when you return from
>>> clk_set_rate()) or is it a reparenting for just a short duration inside
>>> the set_rate ops?
>>
>> I've seen both cases, and indeed the case sometimes depends on the
>> target rate of the clock.
>>
>> For example, when the CPU clock changes, we basically do the following
>> within set_rate:
>>
>> * Set CPU clock parent to some "backup" PLL
>> * Change the CPU PLL to the desired rate
>> * Set CPU clock parent to the CPU PLL
>>
>> However, the lowest CPU clock rate we support is actually the rate that
>> the backup PLL runs at, so if we're targeting that rate, the CPU clock
>> set_rate /just/ does:
>>
>> * Set CPU clock parent to some "backup" PLL
>>
>> and leaves it there, until a different CPU clock rate is requested, at
>> which time the CPU clock will be re-parented back to the CPU PLL.
>
> The backup PLL and rate are statically defined however. It's not chosen at
> runtime.

Yeah, this is pretty much what MSM has been doing for a while for our 
CPU clocks too. But in MSM we made the decision of not to control the 
CPU clocks through the clock APIs. It makes your life so much more 
easier. Really, the CPU clocks are very different from the rest of the 
clocks -- the set rate operation of the clock needs the clock to be 
running. We also never disable the CPU clock we are running on. Just 
that it might not be as useful as it might appear. It was especially 
hard when we didn't have clk_prepare/_unprepare().

Anyway, not saying this is the only way to proceed or using the clock 
APIs to control the CPU clock is wrong. Just some food for thought.

Anyway, getting back to the point of this thread, you might look at the 
other email I sent out on Friday [1]. Sorry I forgot to cc you on that. 
I have a list of people I copy paste when sending clock patches. I can 
add you to that list if you want (or remove anyone who is annoyed by it).

If you are doing the parent management inside the .set_rate ops, I 
believe you will have a race condition with the current clock framework. 
We need to update the clock framework to provide more APIs to implement 
this correctly. I was confused by some code and pinged Mike about it. 
Now that I understand what the code was trying to do, I think I might be 
able to clean it up a bit and then fix up set rate support.

Thanks,
Saravana

[1] http://thread.gmane.org/gmane.linux.ports.arm.msm/2655

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

moving Tegra30 to the common clock framework

[Mike Turquette]

On 20120514-16:48, Saravana Kannan wrote:
> On 05/14/2012 02:36 PM, Turquette, Mike wrote:
> >On Fri, May 11, 2012 at 7:58 PM, Saravana Kannan<skannan@codeaurora.org>  wrote:
> >>Mike,
> >>
> >>I was looking at the code to make the changes and I noticed this snippet
> >>(reformatted for email) in clk_change_rate():
> >>
> >>       if (clk->ops->set_rate)
> >>                clk->ops->set_rate(clk->hw, clk->new_rate,
> >>                                        clk->parent->rate);
> >>
> >>
> >>        if (clk->ops->recalc_rate)
> >>                clk->rate = clk->ops->recalc_rate(clk->hw,
> >>                                clk->parent->rate);
> >>        else
> >>                clk->rate = clk->parent->rate;
> >>
> >>I'm a bit confused. I thought recalc_rates was optional. But if I don't
> >>implement it, the clocks rate will get set to parent's rate? Or is that a
> >>bug in the code?
> >>
> >
> >It is not a bug.  The handsome ascii art in Documentation/clk.txt covers
> >this requirement.  I have copy/pasted the relevant bits below for
> >convenience:
> >
> >                            clock hardware characteristics
> >	     -----------------------------------------------------------
> >              | gate | change rate | single parent | multiplexer | root |
> >              |------|-------------|---------------|-------------|------|
> >	     				...
> >              |      |             |               |             |      |
> >.recalc_rate |      | y           |               |             |      |
> >.round_rate  |      | y           |               |             |      |
> >.set_rate    |      | y           |               |             |      |
> >					...
> >              |      |             |               |             |      |
> >	     -----------------------------------------------------------
> >
> >The take-away is that a clock that can adjust its rate (eg: implements a
> >.set_rate callback) must also implement .recalc_rate and .round_rate
> >callbacks.
> 
> I get the round_rate ops part. But I don't see a need to force a
> recalc_rate ops. Can we just check for the existence of .set_rate()
> to figure out if the clock will take up the rate of the parent or
> will output a different rate?
> 

I think you are forgetting the case where a clock can adjust its output
rate but doesn't always have its .set_rate callback called.  The trivial
example of this is an adjustable divider that is downstream from some
parent clock whose rate is being changed.  In any such case it is quite
necessary to have a .recalc_rate callback on the downstream clock.

The sanity checks in __clk_init (and the documenation) make it clear
that .recalc_rate callbacks must exist due to this exact scenario.  I
guess we could leave it up to the implementor to "get it right" and only
provide .recalc_rate callbacks for clocks whose parents' rates might
change, but why take that risk?

> >
> >>Also, if the clock's rate was just set with set_rate, why do we need to
> >>recalc the rate by reading hardware? I'm a bit confused. Can you please
> >>clarify what's going on here?
> >>
> >
> >This is simply being very cautious.  For platforms adjusting dividers
> >with direct register writes this might feel unnecessary.  However this
> >strict checking is in anticipation of clock hardware that might not
> >actually output the precise rate passed into .set_rate.  In principal
> >this isn't different from how CPUfreq and devfreq drivers inspect rates
> >after requesting them.
> 
> Sorry, this still doesn't make much sense to me. This essentially
> means we can't trust the HW to do what we are asking it to do?
> 

My bit about being "cautious" above is not the driving force behind the
requirement for implementing .recalc_rate.  I thought that you were
specifically complaining about calling .recalc_rate AFTER we called
.set_rate, in which case my point above stands from the perspective of
being future-proof.

Your hardware even has a feature to sample it's own frequency at
run-time... does that mean your hardware doesn't trust itself?

Let's move past the "I don't trust the hardware" bit for a second.  It
is clear to me in your latest email that you not only think that we
should not call .recalc_rate immediately following a call to .set_rate
(which is an interesting point to discuss) but you also feel that we
should not have a requirement to implement .recalc_rate on clocks that
can adjust their rate again.

I'll only repeat what I said at the top of this email: for the scenario
where an adjustable-rate clock can have a parents' rate change it is
absolutely necessary to implement .recalc_rate to correctly determine
the chain of rates as we propagate down the tree.  That we force this at
the framework level is simply good design.

I'm willing to discuss removing the (sometimes) redundant .recalc_rate
calls immediately following .set_rate (since we basically perform a
preemptive .recalc_rate in clk_calc_subtree, called by clk_calc_rates).
But to assert that we can entirely remove the requirement to implement
.recalc_rates on clocks that support .set_rate sounds insane to me.

Please let me know if I've misunderstood what you meant by the
statement, "I don't see a need to force a recalc_rate ops".

> Can we do something like this in clk_change_rate()?
> 
> if (ops->set_rate) {
> 	ops->set_rate(clk->new_rate,...);
> 	clk->rate = clk->new_rate;
> } else {
> 	clk->rate = clk->parent->rate;
> }
> 
> if (ops->recalc_rate()) {
> 	clk->rate = ops->recalc_rate(...);
> }
> 
> This code also makes the assumptions more intuitive and easy to understand.
> 

What does this code buy us?

There is absolutely no difference in this code and it is misleading.
For the case where we do not implement .set_rate but we do implement
.recalc_rate (consider a fixed divide-by-2) this code would assign
clk->rate twice.

Furthermore we must still implement .recalc_rate in an adjustable-rate
clock for the case where a parent rate can change.  Thus every
adjustable-rate clock that implements .set_rate will also implement
.recalc_rate and both will get called in the code above, so we've gained
nothing here.

Your code above is functionally equivalent to the current
clk_change_rate but assigns clk->rate in three different places and is
more confusing.

> >>Would you mind adding more comments inside clk_calc_new_rates() and
> >>clk_change_rate() trying to explain what cases you are trying to account
> >>for?
> >>
> >
> >Someday I'll have a comment/kerneldoc cleanup session.  Things in the
> >clock core are likely to change in the next couple of release cycles
> >which will deprecate some of the kerneldoc making a big cleanup
> >unavoidable.
> >
> >In the mean time are there any other specific question you have for
> >clk_change_rate?
> >
> >>Also, in clk_calc_new_rates(),
> >>
> >>        if (!clk->ops->round_rate) {
> >>                top = clk_calc_new_rates(clk->parent, rate);
> >>                new_rate = clk->parent->new_rate;
> >>
> >>                goto out;
> >>        }
> >>
> >>Is the code assuming that if there is no round rate ops that that clock node
> >>is only a gating clock (as in, can't change frequency the input freq)? Just
> >>trying to understand the assumptions made in the code.
> >>
> >
> >This is also covered by the ascii art above.  There is no assumption
> >about gating, per se.  However if a clock can adjust it's rate (more
> >specifically, if the input rate for a clock differs from the output
> >rate) then a .round_rate callback must exist.
> >
> >The code above reads like it does because in the absence of a
> >.round_rate callback it is implied that the clock cannot set it's own
> >rate and should thus rely on its parent's rate.
> 
> I agree that anyone implementing .set_rate should also provide
> .round_rate. May be we should enforce that in clk_init/clk_register?
> 

It is enforced already.  Please review __clk_init.  I hope that after
reading this email you agree that anyone implementing .set_rate must
also implement .recalc_rate.

> But can we please the above "if" check more explicit? "Your rate has
> to be same as the parent rate since you don't implement set rate" is
> clearer than "Your rate has to be the same as the parent rate since
> you don't implement round rate".
> 
> 	if (!clk->ops->set_rate) {
> 		top = clk_calc_new_rates(clk->parent, rate);
> 		new_rate = clk->parent->new_rate;
> 
> 		goto out;
> 	}

I like that we check the function pointer before calling it.  That is
sane.

Anyone that has read Documentation/clk.txt or reviewed the sanity checks
in __clk_init should realize the relationship between .set_rate,
.round_rate and .recalc_rate.  I see no reason to make the change above.

Regards,
Mike

moving Tegra30 to the common clock framework

[Saravana Kannan]

On 05/14/2012 07:00 PM, Mike Turquette wrote:
> On 20120514-16:48, Saravana Kannan wrote:
>> On 05/14/2012 02:36 PM, Turquette, Mike wrote:
>>> On Fri, May 11, 2012 at 7:58 PM, Saravana Kannan<skannan@codeaurora.org>   wrote:
>>>> Mike,
>>>>

<snip>

>>> The take-away is that a clock that can adjust its rate (eg: implements a
>>> .set_rate callback) must also implement .recalc_rate and .round_rate
>>> callbacks.
>>
>> I get the round_rate ops part. But I don't see a need to force a
>> recalc_rate ops. Can we just check for the existence of .set_rate()
>> to figure out if the clock will take up the rate of the parent or
>> will output a different rate?
>>
>
> I think you are forgetting the case where a clock can adjust its output
> rate but doesn't always have its .set_rate callback called.  The trivial
> example of this is an adjustable divider that is downstream from some
> parent clock whose rate is being changed.  In any such case it is quite
> necessary to have a .recalc_rate callback on the downstream clock.
 >
> The sanity checks in __clk_init (and the documenation) make it clear
> that .recalc_rate callbacks must exist due to this exact scenario.  I
> guess we could leave it up to the implementor to "get it right" and only
> provide .recalc_rate callbacks for clocks whose parents' rates might
> change, but why take that risk?

My comment about forcing recalc rates is limited to "a clock that can 
adjust its rate (eg: implements a .set_rate callback) must also 
implement .recalc_rate.". As you mention, this is really a function of 
whether the parent rate can change or not.

I stand corrected on my orthogonal point that "if .set_rate is not 
implemented, we can assume the rate matches the parent".

>>>> Also, if the clock's rate was just set with set_rate, why do we need to
>>>> recalc the rate by reading hardware? I'm a bit confused. Can you please
>>>> clarify what's going on here?
>>>>
>>>
>>> This is simply being very cautious.  For platforms adjusting dividers
>>> with direct register writes this might feel unnecessary.  However this
>>> strict checking is in anticipation of clock hardware that might not
>>> actually output the precise rate passed into .set_rate.  In principal
>>> this isn't different from how CPUfreq and devfreq drivers inspect rates
>>> after requesting them.
>>
>> Sorry, this still doesn't make much sense to me. This essentially
>> means we can't trust the HW to do what we are asking it to do?
>>
>
> My bit about being "cautious" above is not the driving force behind the
> requirement for implementing .recalc_rate.  I thought that you were
> specifically complaining about calling .recalc_rate AFTER we called
> .set_rate, in which case my point above stands from the perspective of
> being future-proof.

I agree the with the need for recalc_rates you mentioned above (parent 
changes rate and it will affect children). But I still don't get why you 
have to recalc the rate of the clock that you just set. Can you please 
give an example?

> Your hardware even has a feature to sample it's own frequency at
> run-time... does that mean your hardware doesn't trust itself?

No. This is for debugging the software.

<snip>

> but you also feel that we
> should not have a requirement to implement .recalc_rate on clocks that
> can adjust their rate again.

I never questioned the need for recalc rates. I also didn't make the 
above blanket statement -- in fact, I was saying the opposite.

My point was whether we needed to check for .recalc_rates or if we can 
just look for .set_rate to figure out if the clock will take the 
parent's rate. Your fixed factor div-2 example corrected me on that point.

> I'm willing to discuss removing the (sometimes) redundant .recalc_rate
> calls immediately following .set_rate

Yes, please. This was one of the main points of my previous email.

> (since we basically perform a
> preemptive .recalc_rate in clk_calc_subtree, called by clk_calc_rates).

Do you do a recalc on the clock that the clk_set_rate() is called on? 
You only seem to make the preemptive calls on the children. Which makes 
sense (But I have one concern. I will get to it at the end).

> But to assert that we can entirely remove the requirement to implement
> .recalc_rates on clocks that support .set_rate sounds insane to me.
>
> Please let me know if I've misunderstood what you meant by the
> statement, "I don't see a need to force a recalc_rate ops".

Yeah, I think you mistook my comment that was specific to clocks that 
implement set rate as applying to all clocks.

<snip>

> I hope that after
> reading this email you agree that anyone implementing .set_rate must
> also implement .recalc_rate.

This is the original point I raised. To be clear, I'm not denying the 
need for .recalc_rate. I'm just saying that it's not related to 
.set_rate(). As you mentioned in your response above, this is not really 
a function of whether a clock can set it's rate or not. It's a function 
of whether a parent's rate can change. So, I don't see why we should 
arbitrarily tie it to .set_rate.

For example, there are several PLLs in MSM that get their input from a 
fixed crystal oscillator. There's no point in implementing recalc_rate 
for them (except for figuring out the rate during init).

Which brings me to another point:
I think we should split out the "figure out the clock's rate during 
boot/init" to a separate op. That operation by definition has to go 
through many registers, and if it's a rate settable clock, go through 
all the possible frequencies to figure out the rate. That seems too 
expensive for something that's done often like .recalc_rate. In pretty 
much every other call to .recalc_rate, it doesn't really need to check 
the hardware. It just needs to recompute the rate based on the software 
model of that clock. If we do add this new op (say, .sync), the expense 
of calling .recalc_rate after calling .set_rate would be much much lower 
and I won't really complain much about it (would still be nice to not do 
it).

Regards,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

moving Tegra30 to the common clock framework

[Turquette, Mike]

On Mon, May 14, 2012 at 9:20 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 05/14/2012 07:00 PM, Mike Turquette wrote:
> I agree the with the need for recalc_rates you mentioned above (parent
> changes rate and it will affect children). But I still don't get why you
> have to recalc the rate of the clock that you just set. Can you please give
> an example?

I don't have a clock that needs "immediate recalculation after setting
it's rate".  However the code is not incorrect as-is, just suboptimal.
 I'm not currently looking into improving this since I have bigger
problems to solve.  Patches are welcome, otherwise I'll toss it on the
back-burner and fix it up some day (not for initial 3.5 merge window).

>> I'm willing to discuss removing the (sometimes) redundant .recalc_rate
>> calls immediately following .set_rate
>
> Yes, please. This was one of the main points of my previous email.

Same as above.

>> (since we basically perform a
>> preemptive .recalc_rate in clk_calc_subtree, called by clk_calc_rates).
>
>
> Do you do a recalc on the clock that the clk_set_rate() is called on? You
> only seem to make the preemptive calls on the children. Which makes sense
> (But I have one concern. I will get to it at the end).

Preemptive calls happen once only, in a cascade starting from the
"top" clock (which may or may not be the original clock in the
clk_set_rate call due to upwards parent propagation).

However we do recalc the rate for each clock (even after calling its
.set_rate callback) in clk_change_rate.  This could be improved, but
I'm not looking at it right now.

...
> For example, there are several PLLs in MSM that get their input from a fixed
> crystal oscillator. There's no point in implementing recalc_rate for them
> (except for figuring out the rate during init).

I still feel that, fundamentally, any clock which can output a
different rate than is fed into must implement .recalc_rates.  The set
of clocks which can output a different rate than their input often
exceeds the set of clocks that implement .set_rate (as evidenced by my
fixed-divide-by-2 example).  But forcing a check for .recalc_rate on
clocks that implement .set_rate at least helps us catch some which
should implement and do not.

For your PLLs which are fed from a fixed input clock, we can optimize
the code to not _call_ .recalc_rates unnecessarily.  But some day when
your chip changes and you have multiple inputs to your PLLs or an
adjustable rate parent you'll be glad you had .recalc_rates
implemented and ready to go.  It's simply a matter of correctness.

> Which brings me to another point:
> I think we should split out the "figure out the clock's rate during
> boot/init" to a separate op. That operation by definition has to go through
> many registers, and if it's a rate settable clock, go through all the
> possible frequencies to figure out the rate. That seems too expensive for
> something that's done often like .recalc_rate. In pretty much every other
> call to .recalc_rate, it doesn't really need to check the hardware. It just
> needs to recompute the rate based on the software model of that clock. If we
> do add this new op (say, .sync), the expense of calling .recalc_rate after
> calling .set_rate would be much much lower and I won't really complain much
> about it (would still be nice to not do it).

This is also worth thinking about, but it changes the existing
interfaces for clock code to implement and it's yet another
optimization.  So patches are welcome, otherwise I'll toss it onto the
stack somewhere.

Regards,
Mike