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

[Magnus Damm <magnus.damm@gmail.com>]

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.

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

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

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

Cheers,

/ magnus