Re: [PATCH 3/6] ARM: mach-shmobile: extend clock definitions on

On Fri, 2 Jul 2010, Magnus Damm wrote:

On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
[off-list ref] wrote:

quoted

Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
other clocks.

Signed-off-by: Guennadi Liakhovetski <redacted>

Hi Guennadi,

Thanks for your work on this.

quoted

diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
index 26521a7..21cb629 100644
--- a/arch/arm/mach-shmobile/clock-sh7372.c
+++ b/arch/arm/mach-shmobile/clock-sh7372.c

@@ -50,6 +50,12 @@

 #define SMSTPCR3       0xe615013c
 #define SMSTPCR4       0xe6150140

+/* Fixed 27 MHz video clock from DV_CLKI pin */
+static struct clk dv_clki_clk = {
+       .name           = "dv_clki",
+       .rate           = 27000000,
+};

Hm, 27MHz is a board property, not a cpu property.

Well, yes, but I think somewhere something suggested, that 27MHz is what 
you're actually supposed to provide there. Otherwise your board can always 
just call clk_set_rate(), right? Or what's the preferred way to let 
platforms set up clocks?

Also, you don't want to use "name" in struct clk. clkdev is used for
lookup these days.

Right, it is not needed, thanks.

quoted

 /* PLLC2 */
+
+/* Indices are important - they are the actual src selecting values */
+static struct clk *pllc2_parent[] = {
+       [0] = &extal1_div2_clk,
+       [1] = &extal2_div2_clk,
+       [2] = &dv_clki_div2_clk,
+       [3] = NULL,     /* extal2_div4 not implemented yet*/
+};

Why not implemented yet?

Because it's not used yet. Should I implement it anyway?

quoted

+       /*
+        * TODO: If the PLL is off, mult should be = 1, but the clock must be
+        * stopped during re-parenting, a better solution to this conflict
+        * should be found.
+        */
+       mult = (((__raw_readl(PLLC2CR) >> 24) & 0x3f) + 1) * 2;

Yes, this needs to be fixed.

You mean now or in the future? If now - I don't see a reasonable fix so 
far... Do you?

quoted

+enum { DIV6_HDMI, DIV6_REPARENT_NR };
+
+/* Indices are important - they are the actual src selecting values */
+static struct clk *hdmi_parent[] = {
+       [0] = &pllc1_div2_clk,
+       [1] = &pllc2_clk,
+       [2] = &dv_clki_clk,
+       [3] = NULL,     /* pllc2_div4 not implemented yet */
+};
+
+static struct clk div6_reparent_clks[DIV6_REPARENT_NR] = {
+       [DIV6_HDMI] = SH_CLK_DIV6_EXT(&pllc1_div2_clk, HDMICKCR, 0,
+                                     hdmi_parent, ARRAY_SIZE(hdmi_parent), 6, 2),
+};

This part looks nice and clean IMO. Thanks!

Thanks for the review!
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/