Re: [PATCH] ARM: sa1100: convert to common clock framework

[Stephen Boyd <sboyd@kernel.org>]

Quoting Russell King (2019-06-29 03:14:10)
> Convert sa1100 to use the common clock framework.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> Please ack; this is part of a larger series.  Thanks.

Just a few minor comments but otherwise looks good to me.

> diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
> index 6199e87447ca..523ef25618f7 100644
> --- a/arch/arm/mach-sa1100/clock.c
> +++ b/arch/arm/mach-sa1100/clock.c
> +static const char * const clk_tucr_parents[] = {
> +       "clk32768", "clk3686400",
>  };

It would be great if you used the new way of specifying clk parents with
direct pointers instead of strings. See commit fc0c209c147f ("clk: Allow
parents to be specified without string names") for some details.

>  
> -struct clk {
> -       const struct clkops     *ops;
> -       unsigned int            enabled;
> -};
> -
> -#define DEFINE_CLK(_name, _ops)                                \
> -struct clk clk_##_name = {                             \
> -               .ops    = _ops,                         \
> -       }
> -
> -static DEFINE_SPINLOCK(clocks_lock);
> -
> -/* Dummy clk routine to build generic kernel parts that may be using them */
> -long clk_round_rate(struct clk *clk, unsigned long rate)
> -{
> -       return clk_get_rate(clk);
> -}
> -EXPORT_SYMBOL(clk_round_rate);
> -
> -int clk_set_rate(struct clk *clk, unsigned long rate)
> -{
> -       return 0;
> -}
> -EXPORT_SYMBOL(clk_set_rate);
> -
> -int clk_set_parent(struct clk *clk, struct clk *parent)
> -{
> -       return 0;
> -}
> -EXPORT_SYMBOL(clk_set_parent);
> +static DEFINE_SPINLOCK(tucr_lock);
>  
> -struct clk *clk_get_parent(struct clk *clk)
> +static int clk_gpio27_enable(struct clk_hw *hw)
>  {
> -       return NULL;
> -}
> -EXPORT_SYMBOL(clk_get_parent);
> +       unsigned long flags;
>  
> -static void clk_gpio27_enable(struct clk *clk)
> -{
>         /*
>          * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
>          * (SA-1110 Developer's Manual, section 9.1.2.1)
>          */
> +       local_irq_save(flags);
>         GAFR |= GPIO_32_768kHz;
>         GPDR |= GPIO_32_768kHz;
> -       TUCR = TUCR_3_6864MHz;
> +       local_irq_restore(flags);
> +
> +       return 0;
>  }
>  
> -static void clk_gpio27_disable(struct clk *clk)
> +static void clk_gpio27_disable(struct clk_hw *hw)
>  {
> -       TUCR = 0;
> +       unsigned long flags;
> +
> +       local_irq_save(flags);

Why just disable irqs here?

>         GPDR &= ~GPIO_32_768kHz;
>         GAFR &= ~GPIO_32_768kHz;
> +       local_irq_restore(flags);
>  }
>  
> -static void clk_cpu_enable(struct clk *clk)
> -{
> -}
> +static const struct clk_ops clk_gpio27_ops = {
> +       .enable = clk_gpio27_enable,
> +       .disable = clk_gpio27_disable,
> +};
>  
> -static void clk_cpu_disable(struct clk *clk)
> -{
> -}
> +static const char * const clk_gpio27_parents[] = {
> +       "tucr-mux",
> +};
>  
> -static unsigned long clk_cpu_get_rate(struct clk *clk)
> +static const struct clk_init_data clk_gpio27_init_data __initconst = {
> +       .name = "gpio27",
> +       .ops = &clk_gpio27_ops,
> +       .parent_names = clk_gpio27_parents,
> +       .num_parents = ARRAY_SIZE(clk_gpio27_parents),
> +       .flags = CLK_IS_BASIC,

CLK_IS_BASIC is gone. Please don't use it.

> +};
> +
> +/*
> + * Derived from the table 8-1 in the SA1110 manual, the MPLL appears to
> + * multiply its input rate by 4 x (4 + PPCR).  This calculation gives
> + * the exact rate.  The figures given in the table are the rates rounded
> + * to 100kHz.  Stick with sa11x0_getspeed() for the time being.
[...]
> +static const struct clk_init_data clk_mpll_init_data __initconst = {
> +       .name = "mpll",
> +       .ops = &clk_mpll_ops,
> +       .parent_names = clk_mpll_parents,
> +       .num_parents = ARRAY_SIZE(clk_mpll_parents),
> +       .flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE | CLK_IS_CRITICAL,

Please add a comment about these last two flags so we know why the rate
can't be cached and the clk is critical.