RFC: [PATCH] ARM: Kirkwood: clk_register_gate_fn: add fn assignment

RFC: [PATCH] ARM: Kirkwood: clk_register_gate_fn: add fn assignment

[Marc Kleine-Budde]

In commit:
    98d9986 ARM: Kirkwood: Replace clock gating
the kirkwood clock gating has been reworked. A custom variant of
clock gating, that calls a custom function before gating the clock
off, has been introduced. However in clk_register_gate_fn() this
custom function "fn" is never assigned.

This patch adds the missing fn assignment.

Signed-off-by: Marc Kleine-Budde <mkl@blackshift.org>
---
Hi Andrew,

just stumbled over this one. I'm not sure if I missed something in the code
(it's time to go to bed here). Neither the original version or the patch
has been tested on hardware.

regards, Marc

 arch/arm/mach-kirkwood/common.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index 25fb3fd..855e37b 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -159,6 +159,7 @@ static struct clk __init *clk_register_gate_fn(struct device *dev,
 	gate_fn->gate.flags = clk_gate_flags;
 	gate_fn->gate.lock = lock;
 	gate_fn->gate.hw.init = &init;
+	gate_fn->fn = fn;
 
 	/* ops is the gate ops, but with our disable function */
 	if (clk_gate_fn_ops.disable != clk_gate_fn_disable) {
-- 
1.7.4.1

RFC: [PATCH] ARM: Kirkwood: clk_register_gate_fn: add fn assignment

[Andrew Lunn]

On Mon, Jun 18, 2012 at 02:07:34AM +0200, Marc Kleine-Budde wrote:
> In commit:
>     98d9986 ARM: Kirkwood: Replace clock gating
> the kirkwood clock gating has been reworked. A custom variant of
> clock gating, that calls a custom function before gating the clock
> off, has been introduced. However in clk_register_gate_fn() this
> custom function "fn" is never assigned.
> 
> This patch adds the missing fn assignment.
> 
> Signed-off-by: Marc Kleine-Budde <mkl@blackshift.org>
> ---
> Hi Andrew,
> 
> just stumbled over this one. I'm not sure if I missed something in the code
> (it's time to go to bed here). Neither the original version or the patch
> has been tested on hardware.
> 
> regards, Marc
> 
>  arch/arm/mach-kirkwood/common.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
> index 25fb3fd..855e37b 100644
> --- a/arch/arm/mach-kirkwood/common.c
> +++ b/arch/arm/mach-kirkwood/common.c
> @@ -159,6 +159,7 @@ static struct clk __init *clk_register_gate_fn(struct device *dev,
>  	gate_fn->gate.flags = clk_gate_flags;
>  	gate_fn->gate.lock = lock;
>  	gate_fn->gate.hw.init = &init;
> +	gate_fn->fn = fn;
>  
>  	/* ops is the gate ops, but with our disable function */
>  	if (clk_gate_fn_ops.disable != clk_gate_fn_disable) {
> -- 
> 1.7.4.1

Hi Marc

Thanks for the patch.

Tested-by: Andrew Lunn
Acked-by: Andrew Lunn

I will pass it on for inclusion.

  Andrew

RFC: [PATCH] ARM: Kirkwood: clk_register_gate_fn: add fn assignment

[Marc Kleine-Budde]

On 06/18/2012 09:42 AM, Andrew Lunn wrote:
> On Mon, Jun 18, 2012 at 02:07:34AM +0200, Marc Kleine-Budde wrote:
>> In commit:
>>     98d9986 ARM: Kirkwood: Replace clock gating
>> the kirkwood clock gating has been reworked. A custom variant of
>> clock gating, that calls a custom function before gating the clock
>> off, has been introduced. However in clk_register_gate_fn() this
>> custom function "fn" is never assigned.
>>
>> This patch adds the missing fn assignment.
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@blackshift.org>
>> ---
>> Hi Andrew,
>>
>> just stumbled over this one. I'm not sure if I missed something in the code
>> (it's time to go to bed here). Neither the original version or the patch
>> has been tested on hardware.
>>
>> regards, Marc
>>
>>  arch/arm/mach-kirkwood/common.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
>> index 25fb3fd..855e37b 100644
>> --- a/arch/arm/mach-kirkwood/common.c
>> +++ b/arch/arm/mach-kirkwood/common.c
>> @@ -159,6 +159,7 @@ static struct clk __init *clk_register_gate_fn(struct device *dev,
>>  	gate_fn->gate.flags = clk_gate_flags;
>>  	gate_fn->gate.lock = lock;
>>  	gate_fn->gate.hw.init = &init;
>> +	gate_fn->fn = fn;
>>  
>>  	/* ops is the gate ops, but with our disable function */
>>  	if (clk_gate_fn_ops.disable != clk_gate_fn_disable) {
>> -- 
>> 1.7.4.1
> 
> Hi Marc
> 
> Thanks for the patch.
> 
> Tested-by: Andrew Lunn
> Acked-by: Andrew Lunn
> 
> I will pass it on for inclusion.

Thanks, this should go into 3.5.

BTW: I'm hacking on the dove clock support, I noticed that dove can make
use of the same ethernet and pci phy shutdown functions than kirkwood.
What about moving kirkwood_register_gate_fn and subfunctions to plat-orion?

regards, Marc

RFC: [PATCH] ARM: Kirkwood: clk_register_gate_fn: add fn assignment

[Andrew Lunn]

> Thanks, this should go into 3.5.

Yes.

> BTW: I'm hacking on the dove clock support, I noticed that dove can make
> use of the same ethernet and pci phy shutdown functions than kirkwood.
> What about moving kirkwood_register_gate_fn and subfunctions to plat-orion?

Please coordinate with Sebastian.hesselbarth
<Sebastian.hesselbarth@googlemail.com> He has done some work in the
same area.

I would probably move the kirkwood_register_gate_fn() stuff to
drivers/clk. Also, the pcie and sata shutdown functions need
generalizing if they are the be re-used. The address map on Dove is
different to Kirkwood, so the registers are in different places.

	  Andrew

Dove clock support (was: Re: RFC: [PATCH] ARM: Kirkwood: clk_register_gate_fn: add fn assignment)

[Marc Kleine-Budde]

Hello Sebastian,

On 06/18/2012 10:04 AM, Andrew Lunn wrote:
>> BTW: I'm hacking on the dove clock support, I noticed that dove can make
>> use of the same ethernet and pci phy shutdown functions than kirkwood.
>> What about moving kirkwood_register_gate_fn and subfunctions to plat-orion?
> 
> Please coordinate with Sebastian.hesselbarth
> <Sebastian.hesselbarth@googlemail.com> He has done some work in the
> same area.
> 
> I would probably move the kirkwood_register_gate_fn() stuff to
> drivers/clk. Also, the pcie and sata shutdown functions need
> generalizing if they are the be re-used. The address map on Dove is
> different to Kirkwood, so the registers are in different places.

Sure, the address layout is different, but that can be made generic in a
second step. Maybe we need a private pointer in the gate_fn struct.

BTW: who will enable the clocks that have been disabled via the
sata/pcie shutdown functions?

cheers, Marc

Dove clock support (was: Re: RFC: [PATCH] ARM: Kirkwood: clk_register_gate_fn: add fn assignment)

[Andrew Lunn]

> Sure, the address layout is different, but that can be made generic in a
> second step. Maybe we need a private pointer in the gate_fn struct.

Yes, something like that.
 
> BTW: who will enable the clocks that have been disabled via the
> sata/pcie shutdown functions?

This is potentially a problem when the SATA driver is built as a
kernel module. There is no code that i know of to turn the SATA PHYs
back on again. I think this has been broken like this for a long
time...

For PCIE it is not a problem, that code cannot be build as a module.

    Andrew

Dove clock support

[Sebastian Hesselbarh]

On 06/18/2012 10:43 AM, Andrew Lunn wrote:
>> Sure, the address layout is different, but that can be made generic in a
>> second step. Maybe we need a private pointer in the gate_fn struct.
>
> Yes, something like that.

You need to pass at least the controllers base address. Everything else
is common - IIRC kirkwood has two SATA, dove only one. Moreover the base
addresses for the second are not defined, yet.

Also ge-phy has to be connected with ge-clk, too. But for dove this is
a clk gate while kirkwood can shut it down somewhere else. I guess it
can be handled like sata/pcie on kirkwood.

>> BTW: who will enable the clocks that have been disabled via the
>> sata/pcie shutdown functions?
>
> This is potentially a problem when the SATA driver is built as a
> kernel module. There is no code that i know of to turn the SATA PHYs
> back on again. I think this has been broken like this for a long
> time...

IMHO the driver should take care of enabling clk and PHYs. In my
understanding of the common clock framework both will be disabled
if no driver requests it.

Sebastian

Dove clock support

[Marc Kleine-Budde]

On 06/18/2012 11:42 AM, Sebastian Hesselbarh wrote:
> On 06/18/2012 10:43 AM, Andrew Lunn wrote:
>>> Sure, the address layout is different, but that can be made generic in a
>>> second step. Maybe we need a private pointer in the gate_fn struct.
>>
>> Yes, something like that.
> 
> You need to pass at least the controllers base address. Everything else
> is common - IIRC kirkwood has two SATA, dove only one. Moreover the base
> addresses for the second are not defined, yet.
> 
> Also ge-phy has to be connected with ge-clk, too. But for dove this is
> a clk gate while kirkwood can shut it down somewhere else. I guess it
> can be handled like sata/pcie on kirkwood.

The PHY is a clock gate so I'm handling it via:

> ge = dove_register_gate("ge0", CLOCK_GATING_GBE_BIT | CLOCK_GATING_GIGA_PHY_BIT);

So no gate_fn needed.

>>> BTW: who will enable the clocks that have been disabled via the
>>> sata/pcie shutdown functions?
>>
>> This is potentially a problem when the SATA driver is built as a
>> kernel module. There is no code that i know of to turn the SATA PHYs
>> back on again. I think this has been broken like this for a long
>> time...
> 
> IMHO the driver should take care of enabling clk and PHYs. In my
> understanding of the common clock framework both will be disabled
> if no driver requests it.

But the current drivers don't enable clk and PHYs? But if I understand
Andrew correct, this was the case all the time.

Cheers, Marc

Dove clock support

[Sebastian Hesselbarh]

On 06/18/2012 11:54 AM, Marc Kleine-Budde wrote:
>> Also ge-phy has to be connected with ge-clk, too. But for dove this is
>> a clk gate while kirkwood can shut it down somewhere else. I guess it
>> can be handled like sata/pcie on kirkwood.
>
> The PHY is a clock gate so I'm handling it via:
>> ge = dove_register_gate("ge0", CLOCK_GATING_GBE_BIT | CLOCK_GATING_GIGA_PHY_BIT);
> So no gate_fn needed.

There is no fn needed for dove, but kirkwood will need one IIRC.
Moreover, you could hook-up PHY-gate as a parent of corresponding
clk-gate and they will be enabled/disabled simultaneously.

But I'd prefer the driver to take care of clks _and_ PHYs.

One more: I suggest to clean the clk names of orion platforms,
they are a mess ;) And IMHO clks should always have both strings
set, e.g. kirkwood-i2s has an extclk input besides it on-chip clk.

Sebastian

Dove clock support

[Andrew Lunn]

On Mon, Jun 18, 2012 at 12:01:14PM +0200, Sebastian Hesselbarh wrote:
> On 06/18/2012 11:54 AM, Marc Kleine-Budde wrote:
> >>Also ge-phy has to be connected with ge-clk, too. But for dove this is
> >>a clk gate while kirkwood can shut it down somewhere else. I guess it
> >>can be handled like sata/pcie on kirkwood.
> >
> >The PHY is a clock gate so I'm handling it via:
> >>ge = dove_register_gate("ge0", CLOCK_GATING_GBE_BIT | CLOCK_GATING_GIGA_PHY_BIT);
> >So no gate_fn needed.
> 
> There is no fn needed for dove, but kirkwood will need one IIRC.
> Moreover, you could hook-up PHY-gate as a parent of corresponding
> clk-gate and they will be enabled/disabled simultaneously.
> 
> But I'd prefer the driver to take care of clks _and_ PHYs.

That would be nice, but we have to remember that the driver is used
for a number of different Marvell SoC and discreet devices. Only a few
have the ability to turn on/off there clocks and PHYs. So you need to
abstract this in such a way it does not break older chipsets.

> One more: I suggest to clean the clk names of orion platforms,
> they are a mess

Do you have a proposal?

   Thanks
	Andrew

Dove clock support

[Marc Kleine-Budde]

On 06/18/2012 12:01 PM, Sebastian Hesselbarh wrote:
> On 06/18/2012 11:54 AM, Marc Kleine-Budde wrote:
>>> Also ge-phy has to be connected with ge-clk, too. But for dove this is
>>> a clk gate while kirkwood can shut it down somewhere else. I guess it
>>> can be handled like sata/pcie on kirkwood.
>>
>> The PHY is a clock gate so I'm handling it via:
>>> ge = dove_register_gate("ge0", CLOCK_GATING_GBE_BIT |
>>> CLOCK_GATING_GIGA_PHY_BIT);
>> So no gate_fn needed.

BTW: This code doesn't work, btw. It's bits, not masks.

> There is no fn needed for dove, but kirkwood will need one IIRC.
> Moreover, you could hook-up PHY-gate as a parent of corresponding
> clk-gate and they will be enabled/disabled simultaneously.

My solution doesn't wrk. We need either a custom fn or your proposed
parent-clock trick.

> But I'd prefer the driver to take care of clks _and_ PHYs.

You mean clk and PHY individually? I'm not that familiar with the new
clock framework. Is it possible without registering a dummy clock on the
systems with doesn't have individual clocks?

> One more: I suggest to clean the clk names of orion platforms,
> they are a mess ;) And IMHO clks should always have both strings
> set, e.g. kirkwood-i2s has an extclk input besides it on-chip clk.

I'm not (yet) deep into the different orion archs, can you elaborate more?

regards, Marc
Marc

Dove clock support

[Sebastian Hesselbarh]

On 06/18/2012 12:11 PM, Andrew Lunn wrote:
 >> But I'd prefer the driver to take care of clks _and_ PHYs.
 >
 > That would be nice, but we have to remember that the driver is used
 > for a number of different Marvell SoC and discreet devices. Only a few
 > have the ability to turn on/off there clocks and PHYs. So you need to
 > abstract this in such a way it does not break older chipsets.

Well, I'd suggest to find a good name for clk and PHY, e.g.
"pcie.0","clk" and "pcie.0","phy", and let the driver decide if it
enables/disables clk/PHY when it gets a valid struct clk back.

Although struct clk is meant for clocks there is no difference
between clocks and PHYs. Moreover, dove handles PHY control in
it's clock control register sometimes. (Well, it is maybe external
clock to PHY so it's ok)

But first I suggest to have a drivers/clk/clk-orion (which I have
somehow) and I already sent you a proposal some weeks ago.

I wasn't sure how to hook up drivers/clk/clk-orion with the platform
itself but now that there are some other clock drivers available I'll
have another look.

I have the datasheets for dove and kirkwood but with other orion-based
platforms I cannot tell the differences. Maybe someone can comment on
the different other platforms.

 >> One more: I suggest to clean the clk names of orion platforms,
 >> they are a mess
 >
 > Do you have a proposal?

Hmm, well if you look at mach-kirkwood/common.c there is:
- orion_spi.0/NULL (underscore)
- orion-ehci.0/NULL (dash)
- pcie/0
- kirkwood-i2s/NULL

First, I suggest to choose either underscore or dash. Second, pcie/0
should be kirkwood-pcie.0/NULL (or orion-pcie.0) and if we consider
having PHY "clocks" it should be kirkwood-pcie.0/clk.

With kirkwood-i2s/NULL there is mach-dove that has two i2s controllers
so it should be kirkwood-i2s.0. Moreover, the i2s controller can have
an external clk instead of internal clock divider, so the clock names
should be kirkwood-i2s.0/clk and kirkwood-i2s.0/extclk.

With external i2s clock it _could_ be possible to have high bitrate
audio on spdif by overclocking the i2s controller.

Sebastian

Dove clock support (was: Re: RFC: [PATCH] ARM: Kirkwood: clk_register_gate_fn: add fn assignment)

[Simon Baatz]

On Mon, Jun 18, 2012 at 10:43:00AM +0200, Andrew Lunn wrote:
> > Sure, the address layout is different, but that can be made generic in a
> > second step. Maybe we need a private pointer in the gate_fn struct.
> 
> Yes, something like that.
>  
> > BTW: who will enable the clocks that have been disabled via the
> > sata/pcie shutdown functions?
> 
> This is potentially a problem when the SATA driver is built as a
> kernel module. There is no code that i know of to turn the SATA PHYs
> back on again. I think this has been broken like this for a long
> time...
> 

This is not potentially a problem, this is definitively a problem (on kirkwood):

[   12.280783] sata_mv sata_mv.0: cannot get optional clkdev                    
[   12.286314] sata_mv sata_mv.0: slots 32 ports 2                              
[   12.304443] scsi0 : sata_mv                                                  
[   12.307836] scsi1 : sata_mv                                                  
[   12.310804] ata1: SATA max UDMA/133 irq 21                                   
[   12.314920] ata2: SATA max UDMA/133 irq 21                                   
[   12.665882] ata1: SATA link down (SStatus 0 SControl F300)                   
[   13.015881] ata2: SATA link down (SStatus 0 SControl F300)                   


- Simon

Dove clock support

[Sebastian Hesselbarh]

On 06/18/2012 11:41 PM, Simon Baatz wrote:
> This is not potentially a problem, this is definitively a problem (on kirkwood):
>
> [   12.280783] sata_mv sata_mv.0: cannot get optional clkdev
> [   12.286314] sata_mv sata_mv.0: slots 32 ports 2
> ...
> [   12.665882] ata1: SATA link down (SStatus 0 SControl F300)
> [   13.015881] ata2: SATA link down (SStatus 0 SControl F300)

Is this because of some bug in kirkwood soc or missing code in
sata_mv.c?

According to the functional specification of kirkwood there is a
PhyShutdown bit in it's registers. Dove also has this register bit.

Up to now it looks like the SATA PHY can only be disabled in code,
but there should be an phy_enable, too.

What repository your kernel is based on?

Sebastian

Dove clock support

[Andrew Lunn]

On Mon, Jun 18, 2012 at 10:38:41PM +0200, Sebastian Hesselbarh wrote:
> On 06/18/2012 12:11 PM, Andrew Lunn wrote:
> >> But I'd prefer the driver to take care of clks _and_ PHYs.
> >
> > That would be nice, but we have to remember that the driver is used
> > for a number of different Marvell SoC and discreet devices. Only a few
> > have the ability to turn on/off there clocks and PHYs. So you need to
> > abstract this in such a way it does not break older chipsets.
> 
> Well, I'd suggest to find a good name for clk and PHY, e.g.
> "pcie.0","clk" and "pcie.0","phy", and let the driver decide if it
> enables/disables clk/PHY when it gets a valid struct clk back.
> 
> Although struct clk is meant for clocks there is no difference
> between clocks and PHYs. Moreover, dove handles PHY control in
> it's clock control register sometimes. (Well, it is maybe external
> clock to PHY so it's ok)
> 
> But first I suggest to have a drivers/clk/clk-orion (which I have
> somehow) and I already sent you a proposal some weeks ago.
> 
> I wasn't sure how to hook up drivers/clk/clk-orion with the platform
> itself but now that there are some other clock drivers available I'll
> have another look.
> 
> I have the datasheets for dove and kirkwood but with other orion-based
> platforms I cannot tell the differences. Maybe someone can comment on
> the different other platforms.
> 
> >> One more: I suggest to clean the clk names of orion platforms,
> >> they are a mess
> >
> > Do you have a proposal?

Hi Sebastian

> Hmm, well if you look at mach-kirkwood/common.c there is:
> - orion_spi.0/NULL (underscore)
> - orion-ehci.0/NULL (dash)
> - pcie/0
> - kirkwood-i2s/NULL
> 
> First, I suggest to choose either underscore or dash. Second, pcie/0
> should be kirkwood-pcie.0/NULL (or orion-pcie.0) and if we consider
> having PHY "clocks" it should be kirkwood-pcie.0/clk.

Are you aware these names are not free choice.

The code does:

 hpriv->clk = clk_get(&pdev->dev, NULL);

meaning the name needs to match the name in the platform device
structure. This platform device structure is created in
plat-orion/common.c. The driver also uses this name in its
platform_driver structure. Boards which make use of device tree will
also use this name in the auxdata to translate the DT name.

So, it would be nice to have uniform names, but it is a lot of work.

    Andrew

Dove clock support

[Sebastian Hesselbarh]

On 06/19/2012 09:25 PM, Andrew Lunn wrote:
> So, it would be nice to have uniform names, but it is a lot of work.

Hi Andrew,

I was aware that the names come from the corresponding driver name. And
I guess the names were chosen right when the driver was created. But
with more and more orion-based SoCs and Dove besides Kirkwood some names
should be changed.

Of course, this renaming can be postponed after the gory details of DT
have been finished. But yes, it would be nice and a lot of work.

Sebastian

Dove clock support

[Andrew Lunn]

On Tue, Jun 19, 2012 at 12:10:16AM +0200, Sebastian Hesselbarh wrote:
> On 06/18/2012 11:41 PM, Simon Baatz wrote:
> >This is not potentially a problem, this is definitively a problem (on kirkwood):
> >
> >[   12.280783] sata_mv sata_mv.0: cannot get optional clkdev
> >[   12.286314] sata_mv sata_mv.0: slots 32 ports 2
> >...
> >[   12.665882] ata1: SATA link down (SStatus 0 SControl F300)
> >[   13.015881] ata2: SATA link down (SStatus 0 SControl F300)
> 
> Is this because of some bug in kirkwood soc or missing code in
> sata_mv.c?
> 
> According to the functional specification of kirkwood there is a
> PhyShutdown bit in it's registers. Dove also has this register bit.
> 
> Up to now it looks like the SATA PHY can only be disabled in code,
> but there should be an phy_enable, too.

Correct.

When moving from kirkwoods old clock management to the generic clock
framework, i kept the functionality the same. The old code would turn
the PHYs off, but have no way to turn them back on again. The new code
is the same.

	Andrew

Dove clock support

[Andrew Lunn]

> Of course, this renaming can be postponed after the gory details of DT
> have been finished. But yes, it would be nice and a lot of work.

Actually, i would suggest doing it before DT is complete,
i.e. ASAP. At the moment there are very few auxdata entries, meaning
there is less to change.

      Andrew

Dove clock support

[Simon Baatz]

On Tue, Jun 19, 2012 at 09:32:51PM +0200, Andrew Lunn wrote:
> On Tue, Jun 19, 2012 at 12:10:16AM +0200, Sebastian Hesselbarh wrote:
> > On 06/18/2012 11:41 PM, Simon Baatz wrote:
> > >This is not potentially a problem, this is definitively a problem (on kirkwood):
> > >
> > >[   12.280783] sata_mv sata_mv.0: cannot get optional clkdev
> > >[   12.286314] sata_mv sata_mv.0: slots 32 ports 2
> > >...
> > >[   12.665882] ata1: SATA link down (SStatus 0 SControl F300)
> > >[   13.015881] ata2: SATA link down (SStatus 0 SControl F300)
> > 
> > Is this because of some bug in kirkwood soc or missing code in
> > sata_mv.c?
> > 
> > According to the functional specification of kirkwood there is a
> > PhyShutdown bit in it's registers. Dove also has this register bit.
> > 
> > Up to now it looks like the SATA PHY can only be disabled in code,
> > but there should be an phy_enable, too.
> 
> Correct.
> 
> When moving from kirkwoods old clock management to the generic clock
> framework, i kept the functionality the same. The old code would turn
> the PHYs off, but have no way to turn them back on again. The new code
> is the same.

No, not exactly the same. In the old code, it was sufficient to call
the respective kirkwood_..._init function to keep the clock and the
PHY alive. Now, the respective driver needs to enable the clock in
order to prevent that the clock and the PHY are shut down. 

If the driver is loaded as a module, the clock and the PHY are
already shut down at init of the module, which normally would not
happen previously.

Should we make this symmetric and add an enable function to gate_fn?

- Simon

Dove clock support

[Marc Kleine-Budde]

On 06/19/2012 10:42 PM, Simon Baatz wrote:
> On Tue, Jun 19, 2012 at 09:32:51PM +0200, Andrew Lunn wrote:
>> On Tue, Jun 19, 2012 at 12:10:16AM +0200, Sebastian Hesselbarh wrote:
>>> On 06/18/2012 11:41 PM, Simon Baatz wrote:
>>>> This is not potentially a problem, this is definitively a problem (on kirkwood):
>>>>
>>>> [   12.280783] sata_mv sata_mv.0: cannot get optional clkdev
>>>> [   12.286314] sata_mv sata_mv.0: slots 32 ports 2
>>>> ...
>>>> [   12.665882] ata1: SATA link down (SStatus 0 SControl F300)
>>>> [   13.015881] ata2: SATA link down (SStatus 0 SControl F300)
>>>
>>> Is this because of some bug in kirkwood soc or missing code in
>>> sata_mv.c?
>>>
>>> According to the functional specification of kirkwood there is a
>>> PhyShutdown bit in it's registers. Dove also has this register bit.
>>>
>>> Up to now it looks like the SATA PHY can only be disabled in code,
>>> but there should be an phy_enable, too.
>>
>> Correct.
>>
>> When moving from kirkwoods old clock management to the generic clock
>> framework, i kept the functionality the same. The old code would turn
>> the PHYs off, but have no way to turn them back on again. The new code
>> is the same.
> 
> No, not exactly the same. In the old code, it was sufficient to call
> the respective kirkwood_..._init function to keep the clock and the
> PHY alive. Now, the respective driver needs to enable the clock in
> order to prevent that the clock and the PHY are shut down. 
> 
> If the driver is loaded as a module, the clock and the PHY are
> already shut down at init of the module, which normally would not
> happen previously.
> 
> Should we make this symmetric and add an enable function to gate_fn?
+1

Marc

Dove clock support

[Sebastian Hesselbarh]

On 06/19/2012 10:42 PM, Simon Baatz wrote:
> Should we make this symmetric and add an enable function to gate_fn?

I also thought about that issue and I think that as long as PHY is
controlled by controller specific registers it should be handled
by the driver and not by common clock framework.

This is true for SATA and PCIe and will also remove the need for
gate_fn - as long as it doen't break other orion-based SoCs. With
ge00 on dove this is different as ge PHY is not part of the SoC
but an external chip. To turn off the external clock a clk-gate
will be required for the ge PHY bit in dove's clock control
register. This should also be a child of ge controller clock to
turn both off.

Sebastian

Dove clock support

[Simon Baatz]

On Tue, Jun 19, 2012 at 10:55:47PM +0200, Sebastian Hesselbarh wrote:
> On 06/19/2012 10:42 PM, Simon Baatz wrote:
> >Should we make this symmetric and add an enable function to gate_fn?
> 
> I also thought about that issue and I think that as long as PHY is
> controlled by controller specific registers it should be handled
> by the driver and not by common clock framework.
> 
> This is true for SATA and PCIe and will also remove the need for
> gate_fn - as long as it doen't break other orion-based SoCs. ...

If PHY control is part of the driver, where do we turn off PHYs that
are on by default and that we don't use?

Don't we still need something like gate_fn or the clk gate/phy gate
parent/child mechanism you propose?

- Simon

Dove clock support

[Andrew Lunn]

On Wed, Jun 20, 2012 at 01:06:10AM +0200, Simon Baatz wrote:
> On Tue, Jun 19, 2012 at 10:55:47PM +0200, Sebastian Hesselbarh wrote:
> > On 06/19/2012 10:42 PM, Simon Baatz wrote:
> > >Should we make this symmetric and add an enable function to gate_fn?
> > 
> > I also thought about that issue and I think that as long as PHY is
> > controlled by controller specific registers it should be handled
> > by the driver and not by common clock framework.
> > 
> > This is true for SATA and PCIe and will also remove the need for
> > gate_fn - as long as it doen't break other orion-based SoCs. ...
> 
> If PHY control is part of the driver, where do we turn off PHYs that
> are on by default and that we don't use?
> 
> Don't we still need something like gate_fn or the clk gate/phy gate
> parent/child mechanism you propose?

Hi Simon

This is the interesting part to the puzzle.

Probably the correct way for the driver to turn the PHY clk on/off is
via PM runtime functions. This interface should allow a good level of
abstraction such that Dove can do what it needs, while one older
devices, which do not require anything will just have a NOP.

The problem with runtime PM functions is that you need a device
structure. However, when turning off unused clks at boot, it is unused
because we don't have a device driver using the hardware and hence
there is no device structure we can pass to the runtime PM functions.
The same applies for device where there is a kernel module for the
hardware, but it has not been loaded yet.

We probably need both, the extended gate_fn to turn off unused
hardware, and runtime pm to control used hardware.

      Andrew

Dove clock support

[Andrew Lunn]

> > When moving from kirkwoods old clock management to the generic clock
> > framework, i kept the functionality the same. The old code would turn
> > the PHYs off, but have no way to turn them back on again. The new code
> > is the same.
> 
> No, not exactly the same. In the old code, it was sufficient to call
> the respective kirkwood_..._init function to keep the clock and the
> PHY alive. Now, the respective driver needs to enable the clock in
> order to prevent that the clock and the PHY are shut down. 

Ah yes, you are right.

I must admit, i never thought much about systems using kernel modules.
My experience so far, is that systems using kirkwood tend to have
nearly all drivers built in. The exception seems to be wifi, IPv6, and
all plug+play drivers for USB.

However, now i start thinking about it, with the move to one kernel
per ARM architectures, we are probably going to have more and more
systems using modules, since for example it makes little sense to have
the sata_mv driver built in, when run on an AT91 system...

    Andrew

Dove clock support

[Simon Baatz]

Hi Andrew,

On Wed, Jun 20, 2012 at 07:51:39AM +0200, Andrew Lunn wrote:
> > > When moving from kirkwoods old clock management to the generic clock
> > > framework, i kept the functionality the same. The old code would turn
> > > the PHYs off, but have no way to turn them back on again. The new code
> > > is the same.
> > 
> > No, not exactly the same. In the old code, it was sufficient to call
> > the respective kirkwood_..._init function to keep the clock and the
> > PHY alive. Now, the respective driver needs to enable the clock in
> > order to prevent that the clock and the PHY are shut down. 
> 
> Ah yes, you are right.
> 
> I must admit, i never thought much about systems using kernel modules.
> My experience so far, is that systems using kirkwood tend to have
> nearly all drivers built in. The exception seems to be wifi, IPv6, and
> all plug+play drivers for USB.
> 
> However, now i start thinking about it, with the move to one kernel
> per ARM architectures, we are probably going to have more and more
> systems using modules, since for example it makes little sense to have
> the sata_mv driver built in, when run on an AT91 system...

Interesting, my experience is just the other way around. 
Distribution kernels typically are highly modularized.  For example,
the Debian kirkwood kernel uses most of the drivers as modules
(sata_mv, mv_cesa, mv643xx_eth).  This is where my current kernel
configuration originally came from.

After the patch to fix gate_fn, the Ethernet driver works as a
module, since its clocks and thus its PHY is not turned off. 
However, the SATA driver does not work and reports (not surprisingly)
all SATA links as down.

I think we cannot leave this that way. I still like the idea of
adding an "enable PHY" mechanism when the clock is enabled.  Of
course, it is not nice to fiddle around with registers which rather
belong to the driver.  However, we need to do this anyway for the
disable case. But if so, we can also do it symmetrically
(irrespective of adding PM code to drivers).


- Simon