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

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

On Mon, 5 Jul 2010, Magnus Damm wrote:

> On Fri, Jul 2, 2010 at 9:33 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Fri, 2 Jul 2010, Magnus Damm wrote:
> >> On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
> >> > other clocks.
> >> >
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> > --- 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.

Hm, is exporting the clock object really better than doing a 
'clk_get(NULL, "name")'?

> >> Also, you don't want to use "name" in struct clk. clkdev is used for
> >> lookup these days.
> >
> > Right, it is not needed, thanks.
> >
> >> >  /* 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.

I'll remove the last entry.

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

Ok, I can re-add the test and just add a line

		clk->rate = pllc2_recalc(clk);

in pllc2_enable()? This even makes sense in a way - if we know, that 
after enabling the ->rate can get out of sync with the actual frequency... 
I'll prepare an updated patch to make it easier to see.

Thanks
Guennadi
---
Guennadi Liakhovetski