On Mon, 5 Jul 2010, Magnus Damm wrote:
> On Fri, Jul 2, 2010 at 9:33 PM, Guennadi Liakhovetski
> <g.liakhovetski@xxxxxx> wrote:
> > On Fri, 2 Jul 2010, Magnus Damm wrote:
> >> On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
> >> <g.liakhovetski@xxxxxx> wrote:
> >> > Add definitions for DV_CLKI and HDMI clocks, extend support for PLLC2 and some
> >> > other clocks.
> >> >
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> >> > --- 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
