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