Re: [PATCH] AM35xx: Add clock support for new modules on AM35xx

[Ranjith Lohithakshan <ranjithl@ti.com>]

Hello Paul,

On Wed, 06-Jan-10 2:51 AM +0530, Paul Walmsley wrote:
> On Fri, 18 Dec 2009, Ranjith Lohithakshan wrote:
> 
>> On Fri, 18-Dec-09 7:12 AM +0530, Paul Walmsley wrote:
>>
>>> +/* Clocks for AM35XX */
>>> +static struct clk emac_ick = {
>>> +    .name       = "emac_ick",
>>> +    .ops        = &clkops_omap2_dflt,
>>> Shouldn't this clock use &clkops_omap2_dflt_wait (or rather, some custom
>>> version that knows how to read the _ACK bits in IPSS_CLK_CTRL) and test
>>> CPGMAC_VBUSP_CLK_EN_ACK?  Same for the other IPSS VBUSP clocks?   (I
>> guess
>>> "VBUSP" is a synonym for "interface"?)
>> I will ad a custom clkops to address this. Do we need a find_companion
>> to be defined?  The VBUSP clock ACK's do not depend on functional clocks
>> and the functional clocks themselves don't have ant ACK or status bits.
>> EMAC, VPFE amd MUSBOTG are initiators, but the their ACK bits are just
>> for the target idle status.
> 
> I see four ACK bits marked as "clock status bits" in the TRM.  Are these 
> interface clock status bits for initiator interfaces, or are they module 
> target IdleAck bits?  If they are the former, then they are useless and 
> there is no point waiting for these clocks.  If they are the latter, then 
> yes, you'll need a find_companion, because the clock code must ensure that 
> both the interface clock and main functional clock are both enabled before 
> checking the module's target idle status, and the AM35xx IPSS does this in 
> a completely different way than the OMAP35xx.  I guess the IPSS was just 
> 'bolted on' to the chip, rather than really integrated into the OMAP2 PRCM 
> design.

These ACK bits are for the target IdleAck status. I will add a custom
find_companion code for AM35xx. As you rightly said, the IPSS was kind
of bolted together and not properly integrated into the PRCM

>> One of the issues on AM35xx is regarding the status indicator for new
>> clocks. Here 1 indicates as enabled and 0 as not ready state, similar to
>> 24xx case but just opposite to 34xx clocks. And now we have a mix of
>> these two on AM35xx. 
> 
> Wow, that's really craptacular.
> 
>> In omap2_cm_wait_idlest, I do not get a per clock granularity to
>> decide what should be the status check for that particular
>> clock. One of the approach I am thinking is to define a new clock
>> flag (say INVERT_ENABLE_STATUS) and set it for the new AM35xx
>> clocks. And in omap2_module_wait_ready I will do the cpu check and
>> flag check on a per clock basis and determine the status that need
>> to be checked and pass it to omap2_module_wait_ready to check what
>> we want. omap2_module_wait_ready will not be doing any cpu checks as
>> its done today. Let me know what you think.
> 
> (I guess you mean  omap2_cm_wait_module_ready() in the last two sentences?)

I am actually referring to omap2_module_wait_ready defined in
mach-omap2/clock.c

> How about extending find_idlest to pass back whether to test against 0 or 
> 1?  The cpu code will then move into the find_idlest code, and you can 
> just create a few new find_idlest functions for the AM35xx clocks.  Let's 
> try to avoid creating new clock flags for this, I'm trying to move all of 
> this module IDLEST testing code out of the clock code and into the hwmod 
> code where it belongs.

OK. I will extend the existing find_idlest to pass back what value needs
to be checked as you suggested. I will make this change as a separate patch.

>>>> +static struct clk emac_fck = {
>>>> +	.name       = "emac_fck",
>>>> +	.ops        = &clkops_omap2_dflt,
>>>> +	.clkdm_name = "core_l3_clkdm",
>>> Is this the correct clockdomain for this clock?  It seems unlikely that a
>>> 50MHz functional clock would be in core_l3_clkdm.  Usually these are all
>>> interface clocks.  This applies for all the other functional clocks
>> listed
>>> in this file also.
>> Would it be OK if we just don't set clkdm_name. I am not sure whether we
>> can map it to any existing OMAP3 clock domains that way
> 
> I'd like to see all new clocks added with a clockdomain, even if it is not 
> a software-controllable clockdomain.  That makes it possible for others to 
> understand what is really going on from a power management point of view.  
> What is the clockdomain structure for these new clocks?

All the VBUSP (interface) clocks are derived from core_l3_clk and I am
making them as part of core_l3_clk domain. The rmii_ck/emac_fck and
vpfe_fck are sourced from external clock sources. (AM35XX TRM section
4.7.7.12)

>>>> +	.rate       = 50000000,
>>> What's the parent of this clock?  Looking at TRM section 4.7.7.12 it
>>> appears that it gets this from an off-chip source, "rmii_clk"?  Guess
>> that
>>> should probably be defined as the fixed source clock, and emac_fck should
>>> list rmii_clk as its parent?
>>>
>>>> +	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
>>>> +	.enable_bit = AM35XX_CPGMAC_FCLK_SHIFT,
>>>> +	.recalc     = &omap2_clksel_recalc,
>>> This .recalc field is wrong, since there's no .clksel field defined.  If
>>> you define that parent, then this should be followparent_recalc.  The
>>> parent should have .flags = RATE_FIXED and no .recalc.
>> Do we really need to define an rmii_ck? Can we make emac_fck itself as
>> the root clock with no parent? Here is what I was thinking of. Let me
>> know if you see any issues with this
> 
> Yes, please define the parent clocks appropriately.  They will probably be 
> in different clockdomains from the downstream clocks.  Where does rmii_clk 
> come from?  Is this an off-chip clock?  Is this coming from one of the 
> on-chip DPLLs?  

rmii_ck and vpfe_fck are from off-chip sources. These are fixed rate
clocks being fed to the chip. Do we need to associate a dummy or virtual
clock domain for these clocks or is it OK if we treat it similar to the
way we currently treat 32K timer clock (RATE_FIXED, clockops_null, no
clock domain and having no parent)?

>> static struct clk emac_fck = {
>>     .name       = "emac_fck",
>>     .ops        = &clkops_omap2_dflt,
>>     .flags      = RATE_FIXED,
>>     .rate       = 50000000,
>>     .enable_reg = OMAP343X_CTRL_REGADDR(OMAP3517_CONTROL_IP_CLK_CTRL),
>>     .enable_bit = OMAP3517_CPGMAC_FCLK_SHIFT,
>> };
>>
>>>> +static struct clk vpfe_fck = {
>>>> +	.name       = "vpfe_fck",
>>>> +	.ops        = &clkops_omap2_dflt,
>>>> +	.clkdm_name = "core_l3_clkdm",
>>>> +	.rate       = 27000000,
>>>> +	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
>>>> +	.enable_bit = AM35XX_VPFE_FCLK_SHIFT,
>>>> +	.recalc     = &omap2_clksel_recalc,
>>> This fixed rate clock isn't right, for the same reasons as described
>>> above.
>> Would make it similar to emac_fck depending on what we decide.
> 

- Ranjith