[PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Rajendra Nayak]

On OMAP4+, the clkdm association is moved to hwmod while on older OMAPs'
its associated with a clk.
Hence look for a clkdm in both clk and hwmod and warn only when
its missing in both. Also fix the pr_warning() to print the correct
hwmod name.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 2c27fdb..83d56df 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -566,9 +566,9 @@ static int _init_main_clk(struct omap_hwmod *oh)
 		return -EINVAL;
 	}
 
-	if (!oh->_clk->clkdm)
+	if (!oh->_clk->clkdm && !oh->clkdm_name)
 		pr_warning("omap_hwmod: %s: missing clockdomain for %s.\n",
-			   oh->main_clk, oh->_clk->name);
+			   oh->name, oh->_clk->name);
 
 	return ret;
 }
-- 
1.7.1

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Paul Walmsley]

Hi

On Thu, 12 Apr 2012, Rajendra Nayak wrote:

> On OMAP4+, the clkdm association is moved to hwmod while on older OMAPs'
> its associated with a clk.

Sounds like this should be conditional based on the platform, then, 
rather than weakening the warning for all platforms ?

> Hence look for a clkdm in both clk and hwmod and warn only when
> its missing in both. Also fix the pr_warning() to print the correct
> hwmod name.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 2c27fdb..83d56df 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -566,9 +566,9 @@ static int _init_main_clk(struct omap_hwmod *oh)
>  		return -EINVAL;
>  	}
>  
> -	if (!oh->_clk->clkdm)
> +	if (!oh->_clk->clkdm && !oh->clkdm_name)
>  		pr_warning("omap_hwmod: %s: missing clockdomain for %s.\n",
> -			   oh->main_clk, oh->_clk->name);
> +			   oh->name, oh->_clk->name);
>  
>  	return ret;
>  }
> -- 
> 1.7.1
> 


- Paul

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Cousson, Benoit]

Hi Paul,

On 4/12/2012 7:06 PM, Paul Walmsley wrote:
> Hi
>
> On Thu, 12 Apr 2012, Rajendra Nayak wrote:
>
>> On OMAP4+, the clkdm association is moved to hwmod while on older OMAPs'
>> its associated with a clk.
>
> Sounds like this should be conditional based on the platform, then,
> rather than weakening the warning for all platforms ?

Well, as already discussed the clockdomain information is mostly useless 
for most clock nodes because the HW is taking care of the dependencies, 
so highlighting only the ones missing in hwmod is far from enough and 
will avoid scaring people with something that is normal.

Considering that OMAP4 clock domain partition is way more complex than 
it was on OMAP2 & 3, if OMAP4 can leave without that, I doubt OMAP2 & 3 
clock nodes will ever need it, except HW bugs, as usual.

But still, it is an exception more than the regular case.

>> Hence look for a clkdm in both clk and hwmod and warn only when
>> its missing in both. Also fix the pr_warning() to print the correct
>> hwmod name.
>>
>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>

Acked-by: Benoit Cousson <b-cousson@ti.com>

Regards,
Benoit

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Paul Walmsley]

On Wed, 18 Apr 2012, Cousson, Benoit wrote:

> On 4/12/2012 7:06 PM, Paul Walmsley wrote:
> > On Thu, 12 Apr 2012, Rajendra Nayak wrote:
> > 
> > > On OMAP4+, the clkdm association is moved to hwmod while on older OMAPs'
> > > its associated with a clk.
> > 
> > Sounds like this should be conditional based on the platform, then,
> > rather than weakening the warning for all platforms ?
> 
> Well, as already discussed the clockdomain information is mostly useless for
> most clock nodes because the HW is taking care of the dependencies, so
> highlighting only the ones missing in hwmod is far from enough and will avoid
> scaring people with something that is normal.

Sounds to me like the right time to make this change for OMAP4 is when the 
call to omap2_clk_disable_clkdm_control() is removed from 
clock44xx_data.c.  Hopefully that can happen as soon as someone can finish 
the analysis work that we discussed to figure out what drivers still 
aren't converted to using runtime PM, backed with omap_device & 
omap_hwmod.

And at that point, there shouldn't be any reason to test oh->_clk->clkdm 
at all on OMAP4, no?  OMAP4 should only warn if oh->clkdm_name is missing 
or can't be resolved.

> Considering that OMAP4 clock domain partition is way more complex than it was
> on OMAP2 & 3, if OMAP4 can leave without that, I doubt OMAP2 & 3 clock nodes
> will ever need it, except HW bugs, as usual.
> 
> But still, it is an exception more than the regular case.

As far as OMAP2/3 goes, when OMAP2/3 is converted to use the hwmod 
clockdomain enable sequence, that seems like a good time to drop the 
oh->_clk->clkdm test for main clocks.

I'm also wondering if we should be checking clockdomains for the optional 
clocks in _init_opt_clks()... at least on OMAP2/3, unsure about OMAP4.  
Right now we don't implement any hwmod clockdomain enable sequence for the 
optional clocks.


- Paul

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Paul Walmsley]

On Wed, 18 Apr 2012, Paul Walmsley wrote:

> Sounds to me like the right time to make this change for OMAP4 is when the 
> call to omap2_clk_disable_clkdm_control() is removed from 
> clock44xx_data.c.

Sorry, this should have read "when the call to 
omap2_clk_disable_clkdm_control() is _uncommented_"


- Paul

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Cousson, Benoit]

On 4/18/2012 10:52 AM, Paul Walmsley wrote:
> On Wed, 18 Apr 2012, Cousson, Benoit wrote:
> 
>> On 4/12/2012 7:06 PM, Paul Walmsley wrote:
>>> On Thu, 12 Apr 2012, Rajendra Nayak wrote:
>>>
>>>> On OMAP4+, the clkdm association is moved to hwmod while on older OMAPs'
>>>> its associated with a clk.
>>>
>>> Sounds like this should be conditional based on the platform, then,
>>> rather than weakening the warning for all platforms ?
>>
>> Well, as already discussed the clockdomain information is mostly useless for
>> most clock nodes because the HW is taking care of the dependencies, so
>> highlighting only the ones missing in hwmod is far from enough and will avoid
>> scaring people with something that is normal.
> 
> Sounds to me like the right time to make this change for OMAP4 is when the
> call to omap2_clk_disable_clkdm_control() is removed from
> clock44xx_data.c.  Hopefully that can happen as soon as someone can finish
> the analysis work that we discussed to figure out what drivers still
> aren't converted to using runtime PM, backed with omap_device&
> omap_hwmod.

Well, the point is that we do not need this warning even for that.
This is something we have to ensure by reviewing carefully the code.
If you look today, the warning is complaining about clocks that are perfectly fine. So keeping it will just add some noise and not necessarily highlighting the real issue.

FYI here is what we have after the clock cleanup on OMAP4 (i.e removal of modulemode clock nodes):

[    0.287292] omap_hwmod: l3_div_ck: missing clockdomain for l3_div_ck.
[    0.294250] omap_hwmod: func_dmic_abe_gfclk: missing clockdomain for func_dmic_abe_gfclk.
[    0.302795] omap_hwmod: dpll_iva_m4x2_ck: missing clockdomain for dpll_iva_m4x2_ck.
[    0.310821] omap_hwmod: l4_wkup_clk_mux: missing clockdomain for l4_wkup_clk_mux.
[    0.318603] omap_hwmod: l4_div_ck: missing clockdomain for l4_div_ck.
[    0.325317] omap_hwmod: l4_div_ck: missing clockdomain for l4_div_ck.
[    0.332031] omap_hwmod: l4_div_ck: missing clockdomain for l4_div_ck.
[    0.338745] omap_hwmod: l4_div_ck: missing clockdomain for l4_div_ck.
[    0.345458] omap_hwmod: l4_div_ck: missing clockdomain for l4_div_ck.
[    0.352172] omap_hwmod: func_96m_fclk: missing clockdomain for func_96m_fclk.
[    0.359588] omap_hwmod: func_96m_fclk: missing clockdomain for func_96m_fclk.
[    0.367034] omap_hwmod: func_96m_fclk: missing clockdomain for func_96m_fclk.
[    0.374450] omap_hwmod: func_96m_fclk: missing clockdomain for func_96m_fclk.
[    0.381866] omap_hwmod: ducati_clk_mux: missing clockdomain for ducati_clk_mux.
[    0.389465] omap_hwmod: dpll_iva_m5x2_ck: missing clockdomain for dpll_iva_m5x2_ck.
[    0.397460] omap_hwmod: sys_32k_ck: missing clockdomain for sys_32k_ck.
[    0.404357] omap_hwmod: l4_div_ck: missing clockdomain for l4_div_ck.
[    0.411041] omap_hwmod: func_mcbsp1_gfclk: missing clockdomain for func_mcbsp1_gfclk.
[    0.419219] omap_hwmod: func_mcbsp2_gfclk: missing clockdomain for func_mcbsp2_gfclk.
[    0.427337] omap_hwmod: func_mcbsp3_gfclk: missing clockdomain for func_mcbsp3_gfclk.
[    0.435485] omap_hwmod: per_mcbsp4_gfclk: missing clockdomain for per_mcbsp4_gfclk.
[    0.443481] omap_hwmod: pad_clks: missing clockdomain for pad_clks.
[    0.449981] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.457427] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.464843] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.472259] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.479705] omap_hwmod: hsmmc1_fclk: missing clockdomain for hsmmc1_fclk.
[    0.486755] omap_hwmod: hsmmc2_fclk: missing clockdomain for hsmmc2_fclk.
[    0.493835] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.501251] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.508666] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.516113] omap_hwmod: l4_wkup_clk_mux: missing clockdomain for l4_wkup_clk_mux.
[    0.523895] omap_hwmod: l4_wkup_clk_mux: missing clockdomain for l4_wkup_clk_mux.
[    0.531677] omap_hwmod: l4_wkup_clk_mux: missing clockdomain for l4_wkup_clk_mux.
[    0.539459] omap_hwmod: l4_div_ck: missing clockdomain for l4_div_ck.
[    0.546173] omap_hwmod: cm2_dm2_mux_ck: missing clockdomain for cm2_dm2_mux_ck.
[    0.553771] omap_hwmod: cm2_dm3_mux_ck: missing clockdomain for cm2_dm3_mux_ck.
[    0.561370] omap_hwmod: cm2_dm4_mux_ck: missing clockdomain for cm2_dm4_mux_ck.
[    0.568969] omap_hwmod: timer5_sync_mux_ck: missing clockdomain for timer5_sync_mux_ck.
[    0.577270] omap_hwmod: timer6_sync_mux_ck: missing clockdomain for timer6_sync_mux_ck.
[    0.585601] omap_hwmod: timer7_sync_mux_ck: missing clockdomain for timer7_sync_mux_ck.
[    0.593902] omap_hwmod: timer8_sync_mux_ck: missing clockdomain for timer8_sync_mux_ck.
[    0.602233] omap_hwmod: cm2_dm9_mux_ck: missing clockdomain for cm2_dm9_mux_ck.
[    0.609832] omap_hwmod: cm2_dm10_mux_ck: missing clockdomain for cm2_dm10_mux_ck.
[    0.617614] omap_hwmod: cm2_dm11_mux_ck: missing clockdomain for cm2_dm11_mux_ck.
[    0.625396] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.632843] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.640258] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.647674] omap_hwmod: func_48m_fclk: missing clockdomain for func_48m_fclk.
[    0.655120] omap_hwmod: l4_div_ck: missing clockdomain for l4_div_ck.
[    0.661804] omap_hwmod: l3_div_ck: missing clockdomain for l3_div_ck.
[    0.668518] omap_hwmod: sys_32k_ck: missing clockdomain for sys_32k_ck.
[    0.675415] omap_hwmod: sys_32k_ck: missing clockdomain for sys_32k_ck.

That's a lot of noise for nothing. That's why Rajendra's patch is needed now.

> And at that point, there shouldn't be any reason to test oh->_clk->clkdm
> at all on OMAP4, no?  OMAP4 should only warn if oh->clkdm_name is missing
> or can't be resolved.

Yes, that's right.

>> Considering that OMAP4 clock domain partition is way more complex than it was
>> on OMAP2&  3, if OMAP4 can leave without that, I doubt OMAP2&  3 clock nodes
>> will ever need it, except HW bugs, as usual.
>>
>> But still, it is an exception more than the regular case.
> 
> As far as OMAP2/3 goes, when OMAP2/3 is converted to use the hwmod
> clockdomain enable sequence, that seems like a good time to drop the
> oh->_clk->clkdm test for main clocks.

Yes, indeed.

> I'm also wondering if we should be checking clockdomains for the optional
> clocks in _init_opt_clks()... at least on OMAP2/3, unsure about OMAP4.
> Right now we don't implement any hwmod clockdomain enable sequence for the
> optional clocks.

This will not be necessary as soon as we ensure that we always have one main_clk.

The issue was mainly with the DSS, but since we upgraded the dss_dss_fck to become the main_clk, we do not have any opt_clk issue.

Regards,
Benoit  

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Paul Walmsley]

On Wed, 18 Apr 2012, Cousson, Benoit wrote:

> Well, the point is that we do not need this warning even for that. This 
> is something we have to ensure by reviewing carefully the code. 

My perspective is slightly different.

Until we're sure that we don't need clock framework-based clockdomain 
control in mainline for main clocks, we shouldn't remove that warning.  
That is because clock framework-based clockdomain control works via the 
struct clk's clkdm field.  So until we switch away from that model, we 
should ensure that each clock is associated with the clockdomain that its 
clock control FSM is associated with (in OMAP3 terms).

I don't have a problem with switching away from that model, or switching 
individual SoCs away from that model, as long as regressions aren't 
introduced.  But until that switch happens, it seems wise to avoid 
weakening its consistency assumptions.

> If you look today, the warning is complaining about clocks that are 
> perfectly fine.

Actually, mainline is only complaining about one clock:

http://www.pwsan.com/omap/bootlogs/20120417/sparse_cppcheck_cleanup_3.5__0b93afd5d945a8c002f4d380a88b5d7a61c49289/4430es2panda_bootlog.txt

In the current model, the right fix for that clock is to associate it with 
a CM clockdomain (see for example the 4430 TRM vX Figure 3-70 "CD_DMA 
Overview").

> So keeping it will just add some noise and not necessarily highlighting 
> the real issue.
> 
> FYI here is what we have after the clock cleanup on OMAP4 (i.e removal of modulemode clock nodes):
> 

[ lots of warnings elided ]

> That's a lot of noise for nothing. That's why Rajendra's patch is needed now.

Sounds like the patch to alter the warnings should be associated with this 
clock cleanup series, then, since it sounds like it changes the 
clockdomain control model.  And if the model change is only affecting 
OMAP4+, then the warning change should also only apply to OMAP4+.


- Paul

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Cousson, Benoit]

On 4/18/2012 11:40 AM, Paul Walmsley wrote:
> On Wed, 18 Apr 2012, Cousson, Benoit wrote:
>
>> Well, the point is that we do not need this warning even for that. This
>> is something we have to ensure by reviewing carefully the code.
>
> My perspective is slightly different.
>
> Until we're sure that we don't need clock framework-based clockdomain
> control in mainline for main clocks, we shouldn't remove that warning.
> That is because clock framework-based clockdomain control works via the
> struct clk's clkdm field.  So until we switch away from that model, we
> should ensure that each clock is associated with the clockdomain that its
> clock control FSM is associated with (in OMAP3 terms).
>
> I don't have a problem with switching away from that model, or switching
> individual SoCs away from that model, as long as regressions aren't
> introduced.  But until that switch happens, it seems wise to avoid
> weakening its consistency assumptions.
>
>> If you look today, the warning is complaining about clocks that are
>> perfectly fine.
>
> Actually, mainline is only complaining about one clock:

Hehe, sure, this is because we still have a bunch of clock nodes that 
should not be there at the first place... But once you are trying to 
remove them...

> http://www.pwsan.com/omap/bootlogs/20120417/sparse_cppcheck_cleanup_3.5__0b93afd5d945a8c002f4d380a88b5d7a61c49289/4430es2panda_bootlog.txt
>
> In the current model, the right fix for that clock is to associate it with
> a CM clockdomain (see for example the 4430 TRM vX Figure 3-70 "CD_DMA
> Overview").
>
>> So keeping it will just add some noise and not necessarily highlighting
>> the real issue.
>>
>> FYI here is what we have after the clock cleanup on OMAP4 (i.e removal of modulemode clock nodes):
>>
>
> [ lots of warnings elided ]
>
>> That's a lot of noise for nothing. That's why Rajendra's patch is needed now.
>
> Sounds like the patch to alter the warnings should be associated with this
> clock cleanup series, then, since it sounds like it changes the
> clockdomain control model.

It just removes the modulemode clock nodes we were using so far. And 
since these nodes were the only ones with a clkdm on OMAP4, it is now 
complaining, because their parents clock does not have a clkdm.

> And if the model change is only affecting
> OMAP4+, then the warning change should also only apply to OMAP4+.

OK, fair enough. Let's reduce the scope of that change to OMAP4+ only.

Regards,
Benoit

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Paul Walmsley]

On Wed, 18 Apr 2012, Cousson, Benoit wrote:

> On 4/18/2012 11:40 AM, Paul Walmsley wrote:
> 
> > Sounds like the patch to alter the warnings should be associated with this
> > clock cleanup series, then, since it sounds like it changes the
> > clockdomain control model.
> 
> It just removes the modulemode clock nodes we were using so far. And since
> these nodes were the only ones with a clkdm on OMAP4, it is now complaining,
> because their parents clock does not have a clkdm.

If that series doesn't change the model, then maybe the right fix is just 
to associate those clocks with clockdomains in clock44xx_data.c, until the 
model changes?

I looked up some of those clocks in the OMAP4430 TRM vX.  Most of them 
would be associated with either a CM clockdomain or a PRM clockdomain. The 
TRM does actually mention these two clockdomains.  Two examples (out of 
several) that mention CD_PRM and CD_CM are Figure 3-58 "CD_L4_PER 
Overview" and Figure 3-59 "CD_L3_INIT Overview".

Of course, from the software's perspective, these associations would 
effectively be no-ops, as they are on OMAP2/3; but they seem to match the 
view of the hardware as described in the TRM, so at least they make sense.


- Paul

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Cousson, Benoit]

On 4/18/2012 12:12 PM, Paul Walmsley wrote:
> On Wed, 18 Apr 2012, Cousson, Benoit wrote:
>
>> On 4/18/2012 11:40 AM, Paul Walmsley wrote:
>>
>>> Sounds like the patch to alter the warnings should be associated with this
>>> clock cleanup series, then, since it sounds like it changes the
>>> clockdomain control model.
>>
>> It just removes the modulemode clock nodes we were using so far. And since
>> these nodes were the only ones with a clkdm on OMAP4, it is now complaining,
>> because their parents clock does not have a clkdm.
>
> If that series doesn't change the model, then maybe the right fix is just
> to associate those clocks with clockdomains in clock44xx_data.c, until the
> model changes?

But the model is already different for OMAP4. Since the clkdm is 
associated with the hwmod, we already ensure that the clkdm is properly 
enabled before enabling the modulemode.
In fact the clkdm associated with the fake modulemode clk node was 
already useless for OMAP4 as soon as we had introduced the modulemode 
support in hwmod.

The only need for that today is for the few broken drivers that does use 
the clock fmwk to enable the modulemode instead of using pm_runtime.

> I looked up some of those clocks in the OMAP4430 TRM vX.  Most of them
> would be associated with either a CM clockdomain or a PRM clockdomain. The
> TRM does actually mention these two clockdomains.  Two examples (out of
> several) that mention CD_PRM and CD_CM are Figure 3-58 "CD_L4_PER
> Overview" and Figure 3-59 "CD_L3_INIT Overview".
>
> Of course, from the software's perspective, these associations would
> effectively be no-ops, as they are on OMAP2/3; but they seem to match the
> view of the hardware as described in the TRM, so at least they make sense.

Yeah, I'm just reluctant to add some more data in an already big file, 
knowing that this information will be useless, just because a couple of 
broken drivers does need that.

What we plan to do with the migration to CCF is to keep these specific 
nodes only for the broken drivers to highlight that these ones need to 
be fixed. And thus only these nodes will need the clkdm information.

Regards,
Benoit

Re: [PATCH] ARM: omap: hwmod: warn only when clkdm is missing from both clk and hwmod

[Paul Walmsley]

On Wed, 18 Apr 2012, Cousson, Benoit wrote:

> But the model is already different for OMAP4.

We're running both models at the same time in mainline for OMAP4, 
effectively.

> Yeah, I'm just reluctant to add some more data in an already big file, 
> knowing that this information will be useless, just because a couple of 
> broken drivers does need that.
> 
> What we plan to do with the migration to CCF is to keep these specific 
> nodes only for the broken drivers to highlight that these ones need to 
> be fixed. And thus only these nodes will need the clkdm information.

Sounds like after your series, we'll only take both approaches for the 
broken drivers, and then just do the hwmod clockdomain sequence for the 
majority?  That sounds reasonable to me.  We should probably log a warning 
for those cases too.


regards,

- Paul