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

On Fri, Jul 2, 2010 at 9:33 PM, Guennadi Liakhovetski
[off-list ref] wrote:

On Fri, 2 Jul 2010, Magnus Damm wrote:

quoted

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>

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

You're not supposed to set that clock to any special frequency at all.
It's totally up to the board designer. On the AP4EVB board it happens
to be set to 27 MHz. I suggest that you make the clock non-static and
use clk_set_rate() in the board code.

quoted

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

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?

Is it needed by your other patches at this point?

So far the clock routing has been static, so no clocks have got their
parents updated over time. This patch adds a much needed parent
selection support, but only implementing some of the parents does not
make so much sense to me.

quoted

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?

I'm not sure which way is the best, but I think this patch breaks the
case when the PLL is turned off.

/ magnus