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

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

[Ranjith Lohithakshan]

This patch adds clock support for the following AM35xx modules
	- Ethernet MAC
	- CAN Controller (HECC)
	- New MUSB OTG Controller with integrated Phy
	- Video Processing Front End (VPFE)
	- Additional UART (UART4)

Signed-off-by: Ranjith Lohithakshan <ranjithl@ti.com>
---
 arch/arm/mach-omap2/clock34xx_data.c  |   93 +++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/cm-regbits-34xx.h |    6 ++
 2 files changed, 99 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/clock34xx_data.c b/arch/arm/mach-omap2/clock34xx_data.c
index 043caed..d7942b3 100644
--- a/arch/arm/mach-omap2/clock34xx_data.c
+++ b/arch/arm/mach-omap2/clock34xx_data.c
@@ -2983,6 +2983,91 @@ static struct clk wdt1_fck = {
 	.recalc		= &followparent_recalc,
 };
 
+/* Clocks for AM35XX */
+static struct clk emac_ick = {
+	.name       = "emac_ick",
+	.ops        = &clkops_omap2_dflt,
+	.parent     = &core_l3_ick,
+	.clkdm_name = "core_l3_clkdm",
+	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
+	.enable_bit = AM35XX_CPGMAC_VBUSP_CLK_SHIFT,
+	.recalc     = &followparent_recalc,
+};
+
+static struct clk emac_fck = {
+	.name       = "emac_fck",
+	.ops        = &clkops_omap2_dflt,
+	.clkdm_name = "core_l3_clkdm",
+	.rate       = 50000000,
+	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
+	.enable_bit = AM35XX_CPGMAC_FCLK_SHIFT,
+	.recalc     = &omap2_clksel_recalc,
+};
+
+static struct clk hsotgusb_ick_am35xx = {
+	.name       = "hsotgusb_ick",
+	.ops        = &clkops_omap2_dflt,
+	.parent     = &core_l3_ick,
+	.clkdm_name = "core_l3_clkdm",
+	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
+	.enable_bit = AM35XX_USBOTG_VBUSP_CLK_SHIFT,
+	.recalc     = &followparent_recalc,
+};
+
+static struct clk hsotgusb_fck_am35xx = {
+	.name       = "hsotgusb_fck",
+	.ops        = &clkops_omap2_dflt,
+	.parent     = &sys_ck,
+	.clkdm_name = "core_l3_clkdm",
+	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
+	.enable_bit = AM35XX_USBOTG_FCLK_SHIFT,
+	.recalc     = &followparent_recalc,
+};
+
+static struct clk hecc_ck = {
+	.name       = "hecc_ck",
+	.ops        = &clkops_omap2_dflt,
+	.parent     = &sys_ck,
+	.clkdm_name = "core_l3_clkdm",
+	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
+	.enable_bit = AM35XX_HECC_VBUSP_CLK_SHIFT,
+	.recalc     = &followparent_recalc,
+};
+
+static struct clk vpfe_ick = {
+	.name       = "vpfe_ick",
+	.ops        = &clkops_omap2_dflt,
+	.parent     = &core_l3_ick,
+	.clkdm_name = "core_l3_clkdm",
+	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
+	.enable_bit = AM35XX_VPFE_VBUSP_CLK_SHIFT,
+	.recalc     = &followparent_recalc,
+};
+
+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,
+};
+
+/*
+ * The UART1/2 functional clock acts as the functional
+ * clock for UART4. No separate fclk control available.
+ */
+static struct clk uart4_ick = {
+	.name       = "uart4_ick",
+	.ops        = &clkops_omap2_dflt_wait,
+	.parent     = &core_l4_ick,
+	.enable_reg = OMAP_CM_REGADDR(CORE_MOD, CM_ICLKEN1),
+	.enable_bit = AM35XX_EN_UART4_SHIFT,
+	.clkdm_name = "core_l4_clkdm",
+	.recalc     = &followparent_recalc,
+};
+
 
 /*
  * clkdev
@@ -3209,6 +3294,14 @@ static struct omap_clk omap3xxx_clks[] = {
 	CLK(NULL,	"secure_32k_fck", &secure_32k_fck, CK_3XXX),
 	CLK(NULL,	"gpt12_fck",	&gpt12_fck,	CK_3XXX),
 	CLK(NULL,	"wdt1_fck",	&wdt1_fck,	CK_3XXX),
+	CLK("davinci_emac", "ick", &emac_ick, CK_AM35XX),
+	CLK("davinci_emac", "fck", &emac_fck, CK_AM35XX),
+	CLK("musb_hdrc", "ick", &hsotgusb_ick_am35xx, CK_AM35XX),
+	CLK("musb_hdrc", "fck", &hsotgusb_fck_am35xx, CK_AM35XX),
+	CLK(NULL, "hecc_ck", &hecc_ck, CK_AM35XX),
+	CLK("vpfe-capture", "master", &vpfe_ick, CK_AM35XX),
+	CLK("vpfe-capture", "slave", &vpfe_fck, CK_AM35XX),
+	CLK(NULL, "uart4_ick", &uart4_ick, CK_AM35XX),
 };
 
 
diff --git a/arch/arm/mach-omap2/cm-regbits-34xx.h b/arch/arm/mach-omap2/cm-regbits-34xx.h
index 6923deb..39caf48 100644
--- a/arch/arm/mach-omap2/cm-regbits-34xx.h
+++ b/arch/arm/mach-omap2/cm-regbits-34xx.h
@@ -168,6 +168,12 @@
 #define OMAP3430_EN_SDRC				(1 << 1)
 #define OMAP3430_EN_SDRC_SHIFT				1
 
+/*
+ * On AM35XX MSPro is replaced by another UART (UART4)
+ */
+#define AM35XX_EN_UART4					(1 << 23)
+#define AM35XX_EN_UART4_SHIFT				23
+
 /* CM_ICLKEN2_CORE */
 #define OMAP3430_EN_PKA					(1 << 4)
 #define OMAP3430_EN_PKA_SHIFT				4
-- 
1.6.2.4

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

[Paul Walmsley]

Hello Ranjith,

some comments:

On Wed, 16 Dec 2009, Ranjith Lohithakshan wrote:

> This patch adds clock support for the following AM35xx modules
> 	- Ethernet MAC
> 	- CAN Controller (HECC)
> 	- New MUSB OTG Controller with integrated Phy
> 	- Video Processing Front End (VPFE)
> 	- Additional UART (UART4)
> 
> Signed-off-by: Ranjith Lohithakshan <ranjithl@ti.com>
> ---
>  arch/arm/mach-omap2/clock34xx_data.c  |   93 +++++++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/cm-regbits-34xx.h |    6 ++
>  2 files changed, 99 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/clock34xx_data.c b/arch/arm/mach-omap2/clock34xx_data.c
> index 043caed..d7942b3 100644
> --- a/arch/arm/mach-omap2/clock34xx_data.c
> +++ b/arch/arm/mach-omap2/clock34xx_data.c
> @@ -2983,6 +2983,91 @@ static struct clk wdt1_fck = {
>  	.recalc		= &followparent_recalc,
>  };
>  
> +/* 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"?)

> +	.parent     = &core_l3_ick,
> +	.clkdm_name = "core_l3_clkdm",
> +	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
> +	.enable_bit = AM35XX_CPGMAC_VBUSP_CLK_SHIFT,
> +	.recalc     = &followparent_recalc,
> +};
> +
> +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.

> +	.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.

> +};
> +
> +static struct clk hsotgusb_ick_am35xx = {
> +	.name       = "hsotgusb_ick",
> +	.ops        = &clkops_omap2_dflt,
> +	.parent     = &core_l3_ick,
> +	.clkdm_name = "core_l3_clkdm",
> +	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
> +	.enable_bit = AM35XX_USBOTG_VBUSP_CLK_SHIFT,
> +	.recalc     = &followparent_recalc,
> +};
> +
> +static struct clk hsotgusb_fck_am35xx = {
> +	.name       = "hsotgusb_fck",
> +	.ops        = &clkops_omap2_dflt,
> +	.parent     = &sys_ck,
> +	.clkdm_name = "core_l3_clkdm",
> +	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
> +	.enable_bit = AM35XX_USBOTG_FCLK_SHIFT,
> +	.recalc     = &followparent_recalc,
> +};
> +
> +static struct clk hecc_ck = {
> +	.name       = "hecc_ck",
> +	.ops        = &clkops_omap2_dflt,
> +	.parent     = &sys_ck,
> +	.clkdm_name = "core_l3_clkdm",
> +	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
> +	.enable_bit = AM35XX_HECC_VBUSP_CLK_SHIFT,
> +	.recalc     = &followparent_recalc,
> +};
> +
> +static struct clk vpfe_ick = {
> +	.name       = "vpfe_ick",
> +	.ops        = &clkops_omap2_dflt,
> +	.parent     = &core_l3_ick,
> +	.clkdm_name = "core_l3_clkdm",
> +	.enable_reg = OMAP343X_CTRL_REGADDR(AM35XX_CONTROL_IPSS_CLK_CTRL),
> +	.enable_bit = AM35XX_VPFE_VBUSP_CLK_SHIFT,
> +	.recalc     = &followparent_recalc,
> +};
> +
> +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.

> +};
> +
> +/*
> + * The UART1/2 functional clock acts as the functional
> + * clock for UART4. No separate fclk control available.
> + */
> +static struct clk uart4_ick = {
> +	.name       = "uart4_ick",
> +	.ops        = &clkops_omap2_dflt_wait,
> +	.parent     = &core_l4_ick,
> +	.enable_reg = OMAP_CM_REGADDR(CORE_MOD, CM_ICLKEN1),
> +	.enable_bit = AM35XX_EN_UART4_SHIFT,
> +	.clkdm_name = "core_l4_clkdm",
> +	.recalc     = &followparent_recalc,
> +};
> +
>  
>  /*
>   * clkdev
> @@ -3209,6 +3294,14 @@ static struct omap_clk omap3xxx_clks[] = {
>  	CLK(NULL,	"secure_32k_fck", &secure_32k_fck, CK_3XXX),
>  	CLK(NULL,	"gpt12_fck",	&gpt12_fck,	CK_3XXX),
>  	CLK(NULL,	"wdt1_fck",	&wdt1_fck,	CK_3XXX),
> +	CLK("davinci_emac", "ick", &emac_ick, CK_AM35XX),
> +	CLK("davinci_emac", "fck", &emac_fck, CK_AM35XX),
> +	CLK("musb_hdrc", "ick", &hsotgusb_ick_am35xx, CK_AM35XX),
> +	CLK("musb_hdrc", "fck", &hsotgusb_fck_am35xx, CK_AM35XX),
> +	CLK(NULL, "hecc_ck", &hecc_ck, CK_AM35XX),
> +	CLK("vpfe-capture", "master", &vpfe_ick, CK_AM35XX),
> +	CLK("vpfe-capture", "slave", &vpfe_fck, CK_AM35XX),
> +	CLK(NULL, "uart4_ick", &uart4_ick, CK_AM35XX),

Please align the new structure entries with the previous entries.  

>  };
>  
>  
> diff --git a/arch/arm/mach-omap2/cm-regbits-34xx.h b/arch/arm/mach-omap2/cm-regbits-34xx.h
> index 6923deb..39caf48 100644
> --- a/arch/arm/mach-omap2/cm-regbits-34xx.h
> +++ b/arch/arm/mach-omap2/cm-regbits-34xx.h
> @@ -168,6 +168,12 @@
>  #define OMAP3430_EN_SDRC				(1 << 1)
>  #define OMAP3430_EN_SDRC_SHIFT				1
>  
> +/*
> + * On AM35XX MSPro is replaced by another UART (UART4)
> + */
> +#define AM35XX_EN_UART4					(1 << 23)
> +#define AM35XX_EN_UART4_SHIFT				23
> +
>  /* CM_ICLKEN2_CORE */
>  #define OMAP3430_EN_PKA					(1 << 4)
>  #define OMAP3430_EN_PKA_SHIFT				4
> -- 
> 1.6.2.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


- Paul

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

[Paul Walmsley]

On Thu, 17 Dec 2009, Paul Walmsley wrote:

> On Wed, 16 Dec 2009, Ranjith Lohithakshan wrote:
> 
> > This patch adds clock support for the following AM35xx modules
> > 	- Ethernet MAC
> > 	- CAN Controller (HECC)
> > 	- New MUSB OTG Controller with integrated Phy
> > 	- Video Processing Front End (VPFE)
> > 	- Additional UART (UART4)
> > 
> > Signed-off-by: Ranjith Lohithakshan <ranjithl@ti.com>
> > ---
> >  arch/arm/mach-omap2/clock34xx_data.c  |   93 +++++++++++++++++++++++++++++++++
> >  arch/arm/mach-omap2/cm-regbits-34xx.h |    6 ++
> >  2 files changed, 99 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/clock34xx_data.c b/arch/arm/mach-omap2/clock34xx_data.c
> > index 043caed..d7942b3 100644
> > --- a/arch/arm/mach-omap2/clock34xx_data.c
> > +++ b/arch/arm/mach-omap2/clock34xx_data.c
> > @@ -2983,6 +2983,91 @@ static struct clk wdt1_fck = {
> >  	.recalc		= &followparent_recalc,
> >  };
> >  
> > +/* 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"?)

Hmmm.  Thinking about this further, I suppose this depends on whether the 
modules are initiators or targets.  Do you know this?


- Paul

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

[Ranjith Lohithakshan]

Hello Paul,

Thanks for the review comments. My replies below.

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.

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. 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.

>> +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

>
>> +	.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
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.

>> +	CLK("musb_hdrc", "ick", &hsotgusb_ick_am35xx, CK_AM35XX),
>> +	CLK("musb_hdrc", "fck", &hsotgusb_fck_am35xx, CK_AM35XX),
>> +	CLK(NULL, "hecc_ck", &hecc_ck, CK_AM35XX),
>> +	CLK("vpfe-capture", "master", &vpfe_ick, CK_AM35XX),
>> +	CLK("vpfe-capture", "slave", &vpfe_fck, CK_AM35XX),
>> +	CLK(NULL, "uart4_ick", &uart4_ick, CK_AM35XX),
>
> Please align the new structure entries with the previous entries.

Will do that.


 - Ranjith

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

[Lohithakshan, Ranjith]

Hello Paul,
Any comments on the approaches listed below? Let me know your thoughts and I will issue a new version of this patch accordingly

Thanks,
Ranjith 

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of 
> Lohithakshan, Ranjith
> Sent: Friday, December 18, 2009 4:02 PM
> To: Paul Walmsley
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] AM35xx: Add clock support for new 
> modules on AM35xx
> 
> Hello Paul,
> 
> Thanks for the review comments. My replies below.
> 
> 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.
> 
> 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. 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.
> 
> >> +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
> 
> >
> >> +	.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
> 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.
> 
> >> +	CLK("musb_hdrc", "ick", &hsotgusb_ick_am35xx, CK_AM35XX),
> >> +	CLK("musb_hdrc", "fck", &hsotgusb_fck_am35xx, CK_AM35XX),
> >> +	CLK(NULL, "hecc_ck", &hecc_ck, CK_AM35XX),
> >> +	CLK("vpfe-capture", "master", &vpfe_ick, CK_AM35XX),
> >> +	CLK("vpfe-capture", "slave", &vpfe_fck, CK_AM35XX),
> >> +	CLK(NULL, "uart4_ick", &uart4_ick, CK_AM35XX),
> >
> > Please align the new structure entries with the previous entries.
> 
> Will do that.
> 
> 
>  - Ranjith
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Paul Walmsley]

Hi Ranjith,

(please keep attribution lines in message replies, otherwise it becomes 
very difficult to keep track of who said what)

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.

> 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?)

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.

> >> +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?

> >> +	.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?  

> 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.

- Paul

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

[Ranjith Lohithakshan]

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

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

[Paul Walmsley]

Hello Ranjith,

On Fri, 8 Jan 2010, Ranjith Lohithakshan wrote:

> These ACK bits are for the target IdleAck status. I will add a custom
> find_companion code for AM35xx.

...

> 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.

Both of the above sound good.

> 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)

...

> 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)?

I guess it will be fine to add these with no clockdomain until we add an 
external clockdomain.  One last question - do you know if these external 
clocks require the CORE powerdomain to be powered for them to be routed?  
I believe this is the case for some of the external clocks that are routed 
through the CM on OMAP3.


- Paul

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

[Ranjith Lohithakshan]

Hi Paul,

On Sat, 09-Jan-10 4:37 AM +0530, Paul Walmsley wrote:
> Hello Ranjith,
> 
> On Fri, 8 Jan 2010, Ranjith Lohithakshan wrote:
> 
>> These ACK bits are for the target IdleAck status. I will add a custom
>> find_companion code for AM35xx.
> 
> ...
> 
>> 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.
> 
> Both of the above sound good.
> 
>> 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)
> 
> ...
> 
>> 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)?
> 
> I guess it will be fine to add these with no clockdomain until we add an 
> external clockdomain.  One last question - do you know if these external 
> clocks require the CORE powerdomain to be powered for them to be routed?  
> I believe this is the case for some of the external clocks that are routed 
> through the CM on OMAP3.

These external clocks are routed directly from IO to the respective
modules. They are not controlled by CM.

 - Ranjith