Re: [PATCH RFC 02/11] clk: sunxi: factors: Make struct clk_factors_config table const

Re: [PATCH RFC 02/11] clk: sunxi: factors: Make struct clk_factors_config table const

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

On Mon, Jan 25, 2016 at 09:15:38PM +0800, Chen-Yu Tsai wrote:
> struct clk_factors_config contains shifts/widths for the factors of
> the factors clk. This is used to read out the factors from the register
> value. In no case is it written to, so make it const.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 03/11] clk: sunxi: factors: Add clk cleanup in sunxi_factors_register() error path

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 384 bytes --]

On Mon, Jan 25, 2016 at 09:15:39PM +0800, Chen-Yu Tsai wrote:
> sunxi_factors_register() does not check for failures or cleanup after
> clk_register_composite() or other clk-related calls.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 04/11] clk: sunxi: factors: Add unregister function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On Mon, Jan 25, 2016 at 09:15:40PM +0800, Chen-Yu Tsai wrote:
> sunxi's factors clk did not have an unregister function. This means
> multiple structs were leaked whenever a factors clk was unregistered.
> 
> Add an unregister function for it. Also keep pointers to the mux and
> gate structs so they can be freed.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 04/11] clk: sunxi: factors: Add unregister function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On Wed, Jan 27, 2016 at 04:52:10PM +0100, Maxime Ripard wrote:
> On Mon, Jan 25, 2016 at 09:15:40PM +0800, Chen-Yu Tsai wrote:
> > sunxi's factors clk did not have an unregister function. This means
> > multiple structs were leaked whenever a factors clk was unregistered.
> > 
> > Add an unregister function for it. Also keep pointers to the mux and
> > gate structs so they can be freed.
> > 
> > Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> 
> Applied, thanks!
> Maxime

So, yeah, that obviously didn't work, since clk_unregister_composite
wasn't in.

I want to give that as much linux-next coverage as possible, so I
still pulled 3, 5 and 11, replacing the call to
clk_unregister_composite by clk_unregister, adding a todo note on top.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 05/11] clk: sunxi: unmap registers in sunxi_factors_clk_setup if register call fails

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On Mon, Jan 25, 2016 at 09:15:41PM +0800, Chen-Yu Tsai wrote:
> sunxi_factors_clk_setup() does not unmap registers when
> sunxi_factors_register() fails.
> 
> This patch adds proper error handling, and also an error message
> when sunxi_factors_register() fails. Also use the full DT node name
> in error messages, as the node name is often just "clk", which is
> useless.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

APplied, thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 06/11] clk: sunxi: factors: Consolidate get_factors parameters into a struct

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Mon, Jan 25, 2016 at 09:15:42PM +0800, Chen-Yu Tsai wrote:
> The .get_factors callback of factors_clk has 6 parameters. To extend
> factors_clk in any way that requires adding parameters to .get_factors
> would make that list even longer, not to mention changing all the
> function declarations.
> 
> Do this once now and consolidate all the parameters into a struct.
> Also drop the space before function pointer arguments, since checkpatch
> complains.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 07/11] clk: sunxi: factors: Support custom formulas

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

On Mon, Jan 25, 2016 at 09:15:43PM +0800, Chen-Yu Tsai wrote:
> Some clocks cannot be modelled using the standard factors clk formula,
> such as clocks with special pre-dividers on one parent, or clocks
> with all power-of-two dividers.
> 
> Add support for a custom .recalc callback for factors clk. Also pass
> the current parent index to the .get_factor and .recalc callbacks.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 08/11] clk: sunxi: factors: Drop round_rate from clk ops

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On Mon, Jan 25, 2016 at 09:15:44PM +0800, Chen-Yu Tsai wrote:
> The common clock framework requires either determine_rate or round_rate
> to be implemented. We use determine_rate so we can pass the parent index
> to the get_factors callback. This cannot be done easily with round_rate,
> so just drop it.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 09/11] clk: sunxi: rewrite sun6i-a31-ahb1-clk using factors clk with custom recalc

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On Mon, Jan 25, 2016 at 09:15:45PM +0800, Chen-Yu Tsai wrote:
> The factors clk implementation has been extended to support custom
> recalc callbacks to support clocks that use one factor for certain
> parents only, like a pre-divider.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 10/11] clk: sunxi: rewrite sun6i-ar100 using factors clk

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On Mon, Jan 25, 2016 at 09:15:46PM +0800, Chen-Yu Tsai wrote:
> sun6i's AR100 clock is a classic factors clk case:
> 
> AR100 = ((parent mux) >> p) / (m + 1)
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 10/11] clk: sunxi: rewrite sun6i-ar100 using factors clk

[Paul Gortmaker]

On Mon, Jan 25, 2016 at 8:15 AM, Chen-Yu Tsai <wens@csie.org> wrote:
> sun6i's AR100 clock is a classic factors clk case:
>
> AR100 = ((parent mux) >> p) / (m + 1)
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

This patch adds a ".remove" function to a driver that is controlled by
a bool Kconfig, and hence the remove code can't be called unless
one explicitly goes into sysfs and muddles with the unbind as root
(which normally has no sane use case for builtin, non-modular code).

Since I'm trying to cut back on the amount of dead code we have
in .remove functions for built in drivers, is there a valid use case
for this one here, or can I just stage a delete commit for it too?

Normally I've set the set the disallow-unbind flag when deleting
a .remove function to make it clear there is no sane use case
for wandering into sysfs and unhooking the builtin driver.

Thanks,
Paul.
--

> ---
>  drivers/clk/sunxi/clk-sun6i-ar100.c | 235 ++++++++++--------------------------
>  1 file changed, 61 insertions(+), 174 deletions(-)
>
> diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c b/drivers/clk/sunxi/clk-sun6i-ar100.c
> index 20887686bdbe..a7f5777834eb 100644
> --- a/drivers/clk/sunxi/clk-sun6i-ar100.c
> +++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
> @@ -8,211 +8,97 @@
>   *
>   */
>
> +#include <linux/bitops.h>
>  #include <linux/clk-provider.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/spinlock.h>
>
> -#define SUN6I_AR100_MAX_PARENTS                4
> -#define SUN6I_AR100_SHIFT_MASK         0x3
> -#define SUN6I_AR100_SHIFT_MAX          SUN6I_AR100_SHIFT_MASK
> -#define SUN6I_AR100_SHIFT_SHIFT                4
> -#define SUN6I_AR100_DIV_MASK           0x1f
> -#define SUN6I_AR100_DIV_MAX            (SUN6I_AR100_DIV_MASK + 1)
> -#define SUN6I_AR100_DIV_SHIFT          8
> -#define SUN6I_AR100_MUX_MASK           0x3
> -#define SUN6I_AR100_MUX_SHIFT          16
> -
> -struct ar100_clk {
> -       struct clk_hw hw;
> -       void __iomem *reg;
> -};
> -
> -static inline struct ar100_clk *to_ar100_clk(struct clk_hw *hw)
> -{
> -       return container_of(hw, struct ar100_clk, hw);
> -}
> -
> -static unsigned long ar100_recalc_rate(struct clk_hw *hw,
> -                                      unsigned long parent_rate)
> -{
> -       struct ar100_clk *clk = to_ar100_clk(hw);
> -       u32 val = readl(clk->reg);
> -       int shift = (val >> SUN6I_AR100_SHIFT_SHIFT) & SUN6I_AR100_SHIFT_MASK;
> -       int div = (val >> SUN6I_AR100_DIV_SHIFT) & SUN6I_AR100_DIV_MASK;
> -
> -       return (parent_rate >> shift) / (div + 1);
> -}
> -
> -static int ar100_determine_rate(struct clk_hw *hw,
> -                               struct clk_rate_request *req)
> -{
> -       int nparents = clk_hw_get_num_parents(hw);
> -       long best_rate = -EINVAL;
> -       int i;
> -
> -       req->best_parent_hw = NULL;
> -
> -       for (i = 0; i < nparents; i++) {
> -               unsigned long parent_rate;
> -               unsigned long tmp_rate;
> -               struct clk_hw *parent;
> -               unsigned long div;
> -               int shift;
> -
> -               parent = clk_hw_get_parent_by_index(hw, i);
> -               parent_rate = clk_hw_get_rate(parent);
> -               div = DIV_ROUND_UP(parent_rate, req->rate);
> -
> -               /*
> -                * The AR100 clk contains 2 divisors:
> -                * - one power of 2 divisor
> -                * - one regular divisor
> -                *
> -                * First check if we can safely shift (or divide by a power
> -                * of 2) without losing precision on the requested rate.
> -                */
> -               shift = ffs(div) - 1;
> -               if (shift > SUN6I_AR100_SHIFT_MAX)
> -                       shift = SUN6I_AR100_SHIFT_MAX;
> -
> -               div >>= shift;
> -
> -               /*
> -                * Then if the divisor is still bigger than what the HW
> -                * actually supports, use a bigger shift (or power of 2
> -                * divider) value and accept to lose some precision.
> -                */
> -               while (div > SUN6I_AR100_DIV_MAX) {
> -                       shift++;
> -                       div >>= 1;
> -                       if (shift > SUN6I_AR100_SHIFT_MAX)
> -                               break;
> -               }
> -
> -               /*
> -                * If the shift value (or power of 2 divider) is bigger
> -                * than what the HW actually support, skip this parent.
> -                */
> -               if (shift > SUN6I_AR100_SHIFT_MAX)
> -                       continue;
> -
> -               tmp_rate = (parent_rate >> shift) / div;
> -               if (!req->best_parent_hw || tmp_rate > best_rate) {
> -                       req->best_parent_hw = parent;
> -                       req->best_parent_rate = parent_rate;
> -                       best_rate = tmp_rate;
> -               }
> -       }
> -
> -       if (best_rate < 0)
> -               return best_rate;
> -
> -       req->rate = best_rate;
> -
> -       return 0;
> -}
> -
> -static int ar100_set_parent(struct clk_hw *hw, u8 index)
> -{
> -       struct ar100_clk *clk = to_ar100_clk(hw);
> -       u32 val = readl(clk->reg);
> -
> -       if (index >= SUN6I_AR100_MAX_PARENTS)
> -               return -EINVAL;
> -
> -       val &= ~(SUN6I_AR100_MUX_MASK << SUN6I_AR100_MUX_SHIFT);
> -       val |= (index << SUN6I_AR100_MUX_SHIFT);
> -       writel(val, clk->reg);
> -
> -       return 0;
> -}
> +#include "clk-factors.h"
>
> -static u8 ar100_get_parent(struct clk_hw *hw)
> -{
> -       struct ar100_clk *clk = to_ar100_clk(hw);
> -       return (readl(clk->reg) >> SUN6I_AR100_MUX_SHIFT) &
> -              SUN6I_AR100_MUX_MASK;
> -}
> -
> -static int ar100_set_rate(struct clk_hw *hw, unsigned long rate,
> -                         unsigned long parent_rate)
> +/**
> + * sun6i_get_ar100_factors - Calculates factors p, m for AR100
> + *
> + * AR100 rate is calculated as follows
> + * rate = (parent_rate >> p) / (m + 1);
> + */
> +static void sun6i_get_ar100_factors(struct factors_request *req)
>  {
> -       unsigned long div = parent_rate / rate;
> -       struct ar100_clk *clk = to_ar100_clk(hw);
> -       u32 val = readl(clk->reg);
> +       unsigned long div;
>         int shift;
>
> -       if (parent_rate % rate)
> -               return -EINVAL;
> +       /* clock only divides */
> +       if (req->rate > req->parent_rate)
> +               req->rate = req->parent_rate;
>
> -       shift = ffs(div) - 1;
> -       if (shift > SUN6I_AR100_SHIFT_MAX)
> -               shift = SUN6I_AR100_SHIFT_MAX;
> +       div = DIV_ROUND_UP(req->parent_rate, req->rate);
>
> -       div >>= shift;
> +       if (div < 32)
> +               shift = 0;
> +       else if (div >> 1 < 32)
> +               shift = 1;
> +       else if (div >> 2 < 32)
> +               shift = 2;
> +       else
> +               shift = 3;
>
> -       if (div > SUN6I_AR100_DIV_MAX)
> -               return -EINVAL;
> +       div >>= shift;
>
> -       val &= ~((SUN6I_AR100_SHIFT_MASK << SUN6I_AR100_SHIFT_SHIFT) |
> -                (SUN6I_AR100_DIV_MASK << SUN6I_AR100_DIV_SHIFT));
> -       val |= (shift << SUN6I_AR100_SHIFT_SHIFT) |
> -              (div << SUN6I_AR100_DIV_SHIFT);
> -       writel(val, clk->reg);
> +       if (div > 32)
> +               div = 32;
>
> -       return 0;
> +       req->rate = (req->parent_rate >> shift) / div;
> +       req->m = div - 1;
> +       req->p = shift;
>  }
>
> -static struct clk_ops ar100_ops = {
> -       .recalc_rate = ar100_recalc_rate,
> -       .determine_rate = ar100_determine_rate,
> -       .set_parent = ar100_set_parent,
> -       .get_parent = ar100_get_parent,
> -       .set_rate = ar100_set_rate,
> +static const struct clk_factors_config sun6i_ar100_config = {
> +       .mwidth = 5,
> +       .mshift = 8,
> +       .pwidth = 2,
> +       .pshift = 4,
>  };
>
> +static const struct factors_data sun6i_ar100_data __initconst = {
> +       .mux = 16,
> +       .muxmask = GENMASK(1, 0),
> +       .table = &sun6i_ar100_config,
> +       .getter = sun6i_get_ar100_factors,
> +};
> +
> +static DEFINE_SPINLOCK(sun6i_ar100_lock);
> +
>  static int sun6i_a31_ar100_clk_probe(struct platform_device *pdev)
>  {
> -       const char *parents[SUN6I_AR100_MAX_PARENTS];
>         struct device_node *np = pdev->dev.of_node;
> -       const char *clk_name = np->name;
> -       struct clk_init_data init;
> -       struct ar100_clk *ar100;
>         struct resource *r;
> +       void __iomem *reg;
>         struct clk *clk;
> -       int nparents;
> -
> -       ar100 = devm_kzalloc(&pdev->dev, sizeof(*ar100), GFP_KERNEL);
> -       if (!ar100)
> -               return -ENOMEM;
>
>         r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -       ar100->reg = devm_ioremap_resource(&pdev->dev, r);
> -       if (IS_ERR(ar100->reg))
> -               return PTR_ERR(ar100->reg);
> +       reg = devm_ioremap_resource(&pdev->dev, r);
> +       if (IS_ERR(reg))
> +               return PTR_ERR(reg);
>
> -       nparents = of_clk_get_parent_count(np);
> -       if (nparents > SUN6I_AR100_MAX_PARENTS)
> -               nparents = SUN6I_AR100_MAX_PARENTS;
> -
> -       of_clk_parent_fill(np, parents, nparents);
> +       clk = sunxi_factors_register(np, &sun6i_ar100_data, &sun6i_ar100_lock,
> +                                    reg);
> +       if (!clk)
> +               return -ENOMEM;
>
> -       of_property_read_string(np, "clock-output-names", &clk_name);
> +       platform_set_drvdata(pdev, clk);
>
> -       init.name = clk_name;
> -       init.ops = &ar100_ops;
> -       init.parent_names = parents;
> -       init.num_parents = nparents;
> -       init.flags = 0;
> +       return 0;
> +}
>
> -       ar100->hw.init = &init;
> +static int sun6i_a31_ar100_clk_remove(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct clk *clk = platform_get_drvdata(pdev);
>
> -       clk = clk_register(&pdev->dev, &ar100->hw);
> -       if (IS_ERR(clk))
> -               return PTR_ERR(clk);
> +       sunxi_factors_unregister(np, clk);
>
> -       return of_clk_add_provider(np, of_clk_src_simple_get, clk);
> +       return 0;
>  }
>
>  static const struct of_device_id sun6i_a31_ar100_clk_dt_ids[] = {
> @@ -227,6 +113,7 @@ static struct platform_driver sun6i_a31_ar100_clk_driver = {
>                 .of_match_table = sun6i_a31_ar100_clk_dt_ids,
>         },
>         .probe = sun6i_a31_ar100_clk_probe,
> +       .remove = sun6i_a31_ar100_clk_remove,
>  };
>  module_platform_driver(sun6i_a31_ar100_clk_driver);
>
> --
> 2.7.0
>

[PATCH] clk: sunxi: don't mark sun6i_ar100_data __initconst

[Arnd Bergmann]

The clk-sun6i-ar100 clk driver is a platform driver that may use
deferred probing, so its probe function must not access
__init symbols. Kbuild warns about this:

WARNING: drivers/clk/sunxi/built-in.o(.text+0x15f0): Section mismatch in reference from the function sun6i_a31_ar100_clk_probe() to the (unknown reference) .init.rodata:(unknown)
The function sun6i_a31_ar100_clk_probe() references
the (unknown reference) __initconst (unknown).
This is often because sun6i_a31_ar100_clk_probe lacks a __initconst
annotation or the annotation of (unknown) is wrong.

Removing the __initconst annotation avoids the warning and makes
deferred probing work.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 3ca2377b6fed ("clk: sunxi: rewrite sun6i-ar100 using factors clk")
---
 drivers/clk/sunxi/clk-sun6i-ar100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c b/drivers/clk/sunxi/clk-sun6i-ar100.c
index a7f5777834eb..84a187e55360 100644
--- a/drivers/clk/sunxi/clk-sun6i-ar100.c
+++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
@@ -60,7 +60,7 @@ static const struct clk_factors_config sun6i_ar100_config = {
 	.pshift = 4,
 };
 
-static const struct factors_data sun6i_ar100_data __initconst = {
+static const struct factors_data sun6i_ar100_data = {
 	.mux = 16,
 	.muxmask = GENMASK(1, 0),
 	.table = &sun6i_ar100_config,
-- 
2.7.0

Re: [PATCH] clk: sunxi: don't mark sun6i_ar100_data __initconst

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

On Tue, Feb 02, 2016 at 04:55:30PM +0100, Arnd Bergmann wrote:
> The clk-sun6i-ar100 clk driver is a platform driver that may use
> deferred probing, so its probe function must not access
> __init symbols. Kbuild warns about this:
> 
> WARNING: drivers/clk/sunxi/built-in.o(.text+0x15f0): Section mismatch in reference from the function sun6i_a31_ar100_clk_probe() to the (unknown reference) .init.rodata:(unknown)
> The function sun6i_a31_ar100_clk_probe() references
> the (unknown reference) __initconst (unknown).
> This is often because sun6i_a31_ar100_clk_probe lacks a __initconst
> annotation or the annotation of (unknown) is wrong.
> 
> Removing the __initconst annotation avoids the warning and makes
> deferred probing work.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 3ca2377b6fed ("clk: sunxi: rewrite sun6i-ar100 using factors clk")

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 11/11] clk: sunxi: rewrite sun8i-a23-mbus-clk using the simpler composite clk

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

On Mon, Jan 25, 2016 at 09:15:47PM +0800, Chen-Yu Tsai wrote:
> sun8i-a23-mbus-clk used sunxi's factors clk, which is nice for very
> complicated clocks, but is not really needed here.
> 
> Convert sun8i-a23-mbus-clk to use clk_composite, as it is a gate + mux
> + divider. This makes the code easier to understand.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 11/11] clk: sunxi: rewrite sun8i-a23-mbus-clk using the simpler composite clk

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

Hi,

On Thu, Jan 28, 2016 at 10:41:33AM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Thu, Jan 28, 2016 at 1:49 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Mon, Jan 25, 2016 at 09:15:47PM +0800, Chen-Yu Tsai wrote:
> >> sun8i-a23-mbus-clk used sunxi's factors clk, which is nice for very
> >> complicated clocks, but is not really needed here.
> >>
> >> Convert sun8i-a23-mbus-clk to use clk_composite, as it is a gate + mux
> >> + divider. This makes the code easier to understand.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >
> > Applied, thanks!
> > Maxime
> 
> Given your suggestion for extending and generalizing factors clk
> clock rate calculations, maybe we just leave this one out and use
> the new stuff later?

My suggestion was only about the core of the clk-factors, the
"external" API would not change, so I'm guessing it's still relevant.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH RFC 00/11] clk: sunxi: factors clk clean up and refactor

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4779 bytes --]

Hi Chen-Yu,

On Mon, Jan 25, 2016 at 09:15:36PM +0800, Chen-Yu Tsai wrote:
> Hi everyone,
> 
> This series cleans up and reworks parts of sunxi's factors clk. The goal
> is to support non-standard formulas for clock rate calculation, such as
> pre-dividers on some parents, or all power-of-2 dividers. One such clock
> is the AHB1 clock on A31/A31s.
> 
> Patch 1 is Maxime's patch adding an unregister function for composite
> clocks. Patches 3 and 4 use this, so it is included for completeness.
> 
> Patch 2 makes the config tables for factors clk constant. These contain
> the shift and width for the factors. They are used to manipulate the
> clk register values. There should be no reason to change them in-flight.
> 
> Patch 3 adds a proper error path for the factors clk register function(),
> so we don't leak memory when a call fails.
> 
> Patch 4 adds an unregister function for factors clks.
> 
> Patch 5 adds an error patch to sunxi_factors_clk_setup()
> 
> Patch 6 packs the parameters passed to get_factors callbacks in a struct.
> This makes it easier to extend factors clk without having to edit all
> the function definitions, and also makes the lines shorter.
> 
> Patch 7 makes factors clk support custom formulas for calculating clock
> rates. On the clock rounding/setting side, we only need to teach
> get_factors about different parent clocks. On the recalc side, we add
> support for custom .recalc callbacks for clocks that need them.
> 
> Patch 8 drops .round_rate from factors clk ops. Since only one of
> .round_rate and .determine_rate is needed, and the clk core prefers the
> latter, remove .round_rate.
> 
> Patch 9 rewrites sun6i-a31-ahb1-clk using factors clk with the new custom
> formula support. sun6i-a31-ahb1 has a pre-divider on one of its parents.
> 
> Patch 10 rewrite sun6i-ar100 using factors clk.
> 
> Patch 11 rewrites sun8i-a23-mbus-clk using the simpler composite clk.
> While this patch is doing the reverse, i.e. rewriting a factors clk into
> a composite clk, it is included because some changes overlap. I'm not
> sure whether this approach is worthwhile, as it actually adds more code,
> though it might make it easier to understand.

Thanks a lot for working on this.

I'm guessing we could even take a step further, since most of the
clocks are re-using a variation of the factor calculation code. We
roughly end up in a handful of cases (the clocks are just from a quick
look at the A10 and A31 datasheet and the source code, which might
leave a few clocks that we don't support yet in the newer SoCs)

  * A single factor:
    + These ones are trivial to handle, a simple division gives us
      directly the divisor to use.
    + Clocks in this case:
      - A13 AHB (p)
      - A80 AHB (p)
      - A10 PLL3 (m)
      - A31 AHB (m)
      - A80 GT (m)

  * Two factors:
    + These ones might be a bit more difficult to handle. One case is
      quite trivial too, it's the n and m case, where we can use
      directly rational_best_approximation() that handles this just
      fine.
      The other cases are a bit more tricky, but we can always brute
      force it, it shouldn't be very difficult to implement or very
      long to run.

    + Clocks in the (p + m) case
      - A10 APB1
      - A20 CLK OUT
      - A10 MOD0
      - A31 AR100
      - A80 APB1

    + Clocks in the (n + k) case
      - A10 PLL5
      - A31 PLL6

    + Clocks in the (n + m) case
      - A10 PLL2
      - A31 PLL3
      - A31 PLL4
      - A31 PLL8
      - A31 PLL9
      - A31 PLL10

  * Three factors
    + There's probably some consolidation that can be done here too,
      or to consider brute-forcing the whole thing again. The number
      of combinations would probably rise quite a lot, which might
      have a quite significant performance hit. I'm not really sure we
      care though.

    + Clocks in the (n, k and m) case
      - A31 PLL1
      - A31 PLL5
      - A31 MIPI PLL

    + Clocks in the (n, p and m) case
      - A31 pll2
      - A80 pll4

  * All factors (n, k, p and m)
    + I'm not sure it's worth it in this case. I'd expect the code to
      be quite complex and slow to evaluate all the cases.
    + Clocks
      - A10 PLL1
      - A10 PLL4
      - A23 PLL1
    

So, I guess we could have a default (and overridable) function that
would cover at least the cases where we have a single or two
factors. I think we already have everything we need in the clk_factors
structure, so we shouldn't need to modify each and every clocks.

What do you think about it?
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]