Re: [PATCH] clk-divider: Fix READ_ONLY when divider > 1

[Mike Turquette]

Quoting James Hogan (2014-11-14 07:32:09)
> Commit 79c6ab509558 (clk: divider: add CLK_DIVIDER_READ_ONLY flag) in
> v3.16 introduced the CLK_DIVIDER_READ_ONLY flag which caused the
> recalc_rate() and round_rate() clock callbacks to be omitted.
> 
> However using this flag has the unfortunate side effect of causing the
> clock recalculation code when a clock rate change is attempted to always
> treat it as a pass-through clock, i.e. with a fixed divide of 1, which
> may not be the case. Child clock rates are then recalculated using the
> wrong parent rate.
> 
> Therefore instead of dropping the recalc_rate() and round_rate()
> callbacks, alter clk_divider_bestdiv() to always report the current
> divider as the best divider so that it is never altered.
> 
> For me the read only clock was the system clock, which divided the PLL
> rate by 2, from which both the UART and the SPI clocks were divided.
> Initial setting of the UART rate set it correctly, but when the SPI
> clock was set, the other child clocks were miscalculated. The UART clock
> was recalculated using the PLL rate as the parent rate, resulting in a
> UART new_rate of double what it should be, and a UART which spewed forth
> garbage when the rate changes were propagated.

Applied to clk-fixes towards -rc6.

Thanks,
Mike

> 
> Signed-off-by: James Hogan <james.hogan@xxxxxxxxxx>
> Cc: Mike Turquette <mturquette@xxxxxxxxxx>
> Cc: Heiko Stuebner <heiko@xxxxxxxxx>
> Cc: Thomas Abraham <thomas.ab@xxxxxxxxxxx>
> Cc: Tomasz Figa <t.figa@xxxxxxxxxxx>
> Cc: Max Schwarz <max.schwarz@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v3.16+
> ---
>  drivers/clk/clk-divider.c    | 18 +++++++++---------
>  drivers/clk/rockchip/clk.c   |  4 +---
>  include/linux/clk-provider.h |  1 -
>  3 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 18a9de29df0e..c0a842b335c5 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -263,6 +263,14 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate,
>         if (!rate)
>                 rate = 1;
>  
> +       /* if read only, just return current value */
> +       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> +               bestdiv = readl(divider->reg) >> divider->shift;
> +               bestdiv &= div_mask(divider);
> +               bestdiv = _get_div(divider, bestdiv);
> +               return bestdiv;
> +       }
> +
>         maxdiv = _get_maxdiv(divider);
>  
>         if (!(__clk_get_flags(hw->clk) & CLK_SET_RATE_PARENT)) {
> @@ -361,11 +369,6 @@ const struct clk_ops clk_divider_ops = {
>  };
>  EXPORT_SYMBOL_GPL(clk_divider_ops);
>  
> -const struct clk_ops clk_divider_ro_ops = {
> -       .recalc_rate = clk_divider_recalc_rate,
> -};
> -EXPORT_SYMBOL_GPL(clk_divider_ro_ops);
> -
>  static struct clk *_register_divider(struct device *dev, const char *name,
>                 const char *parent_name, unsigned long flags,
>                 void __iomem *reg, u8 shift, u8 width,
> @@ -391,10 +394,7 @@ static struct clk *_register_divider(struct device *dev, const char *name,
>         }
>  
>         init.name = name;
> -       if (clk_divider_flags & CLK_DIVIDER_READ_ONLY)
> -               init.ops = &clk_divider_ro_ops;
> -       else
> -               init.ops = &clk_divider_ops;
> +       init.ops = &clk_divider_ops;
>         init.flags = flags | CLK_IS_BASIC;
>         init.parent_names = (parent_name ? &parent_name: NULL);
>         init.num_parents = (parent_name ? 1 : 0);
> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
> index 1e68bff481b8..880a266f0143 100644
> --- a/drivers/clk/rockchip/clk.c
> +++ b/drivers/clk/rockchip/clk.c
> @@ -90,9 +90,7 @@ static struct clk *rockchip_clk_register_branch(const char *name,
>                 div->width = div_width;
>                 div->lock = lock;
>                 div->table = div_table;
> -               div_ops = (div_flags & CLK_DIVIDER_READ_ONLY)
> -                                               ? &clk_divider_ro_ops
> -                                               : &clk_divider_ops;
> +               div_ops = &clk_divider_ops;
>         }
>  
>         clk = clk_register_composite(NULL, name, parent_names, num_parents,
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index be21af149f11..2839c639f092 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -352,7 +352,6 @@ struct clk_divider {
>  #define CLK_DIVIDER_READ_ONLY          BIT(5)
>  
>  extern const struct clk_ops clk_divider_ops;
> -extern const struct clk_ops clk_divider_ro_ops;
>  struct clk *clk_register_divider(struct device *dev, const char *name,
>                 const char *parent_name, unsigned long flags,
>                 void __iomem *reg, u8 shift, u8 width,
> -- 
> 2.0.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/