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

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

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>
> 
> Hi Guennadi,
> 
> Thanks for your work on this.
> 
> > 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.

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

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

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