clk: sunxi: Add support for the Audio PLL

Some clocks are using a factor component, however, unlike their mux, gate or divider counterpart, these factors don't have a basic clock implementation.

This leads to code duplication across platforms that want to use that kind of clocks, and the impossibility to use the composite clocks with such a clock without defining your own rate operations.

Create such a driver in order to remove these issues, and hopefully factor the implementations, reducing code size across platforms and consolidating the various implementations.

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/clk/Makefile         |   1 +
 drivers/clk/clk-factor.c     | 176 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  41 ++++++++++
 3 files changed, 218 insertions(+)
 create mode 100644 drivers/clk/clk-factor.c

drivers/clk/Makefile +1 -0

index 3d00c25382c5..d38b71d15ffa 100644
@@ -3,6 +3,7 @@ obj-$(CONFIG_HAVE_CLK)          += clk-devres.o
 obj-$(CONFIG_CLKDEV_LOOKUP)    += clkdev.o
 obj-$(CONFIG_COMMON_CLK)       += clk.o
 obj-$(CONFIG_COMMON_CLK)       += clk-divider.o
+obj-$(CONFIG_COMMON_CLK)       += clk-factor.o
 obj-$(CONFIG_COMMON_CLK)       += clk-fixed-factor.o
 obj-$(CONFIG_COMMON_CLK)       += clk-fixed-rate.o
 obj-$(CONFIG_COMMON_CLK)       += clk-gate.o

drivers/clk/clk-factor.c new file +176 -0

new file mode 100644
index 000000000000..17f83852cdb6
@@ -0,0 +1,176 @@
+/*
+ * Copyright (C) 2015 Maxime Ripard <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/module.h>
+#include <linux/clk-provider.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/of.h>
+
+#define to_clk_factor(_hw) container_of(_hw, struct clk_factor, hw)
+
+static unsigned long __get_mult(struct clk_factor *factor,
+                               unsigned long rate,
+                               unsigned long parent_rate)
+{
+       if (factor->flags & CLK_FACTOR_ROUND_CLOSEST)
+               return DIV_ROUND_CLOSEST(rate, parent_rate);
+
+       return rate / parent_rate;
+}
+
+static unsigned long clk_factor_recalc_rate(struct clk_hw *hw,
+                                           unsigned long parent_rate)
+{
+       struct clk_factor *factor = to_clk_factor(hw);
+       unsigned long val;
+
+       val = clk_readl(factor->reg) >> factor->shift;
+       val &= GENMASK(factor->width, 0);
+
+       if (!val && factor->flags & CLK_FACTOR_ZERO_BYPASS)
+               val = 1;
+
+       return parent_rate * val;
+}
+
+static bool __is_best_rate(unsigned long rate, unsigned long new,
+                          unsigned long best, unsigned long flags)
+{
+       if (flags & CLK_FACTOR_ROUND_CLOSEST)
+               return abs(rate - new) < abs(rate - best);
+
+       return new >= rate && new < best;
+}
+
+static unsigned long clk_factor_bestmult(struct clk_hw *hw, unsigned long rate,
+                                        unsigned long *best_parent_rate,
+                                        u8 width, unsigned long flags)
+{
+       unsigned long orig_parent_rate = *best_parent_rate;
+       unsigned long parent_rate, current_rate, best_rate = ~0;
+       unsigned int i, bestmult = 0;
+
+       if (!(__clk_get_flags(hw->clk) & CLK_SET_RATE_PARENT))
+               return rate / *best_parent_rate;
+
+       for (i = 1; i < ((1 << width) - 1); i++) {
+               if (rate * i == orig_parent_rate) {
+                       /*
+                        * This is the best case for us if we have a
+                        * perfect match without changing the parent
+                        * rate.
+                        */
+                       *best_parent_rate = orig_parent_rate;
+                       return i;
+               }
+
+               parent_rate = __clk_round_rate(__clk_get_parent(hw->clk),
+                                              rate / i);
+               current_rate = parent_rate * i;
+
+               if (__is_best_rate(rate, current_rate, best_rate, flags)) {
+                       bestmult = i;
+                       best_rate = current_rate;
+                       *best_parent_rate = parent_rate;
+               }
+       }
+
+       return bestmult;
+}
+
+static long clk_factor_round_rate(struct clk_hw *hw, unsigned long rate,
+                                 unsigned long *parent_rate)
+{
+       struct clk_factor *factor = to_clk_factor(hw);
+       unsigned long mult = clk_factor_bestmult(hw, rate, parent_rate,
+                                                factor->width, factor->flags);
+
+       return *parent_rate * mult;
+}
+
+static int clk_factor_set_rate(struct clk_hw *hw, unsigned long rate,
+                              unsigned long parent_rate)
+{
+       struct clk_factor *factor = to_clk_factor(hw);
+       unsigned long mult = __get_mult(factor, rate, parent_rate);
+       unsigned long uninitialized_var(flags);
+       struct clk *clk = hw->clk;
+       unsigned long val;
+
+       if (factor->lock)
+               spin_lock_irqsave(factor->lock, flags);
+
+       val = clk_readl(factor->reg);
+       val &= ~GENMASK(factor->width + factor->shift, factor->shift);
+       val |= mult << factor->shift;
+       clk_writel(val, factor->reg);
+
+       if (factor->lock)
+               spin_unlock_irqrestore(factor->lock, flags);
+
+       return 0;
+}
+
+const struct clk_ops clk_factor_ops = {
+       .recalc_rate    = clk_factor_recalc_rate,
+       .round_rate     = clk_factor_round_rate,
+       .set_rate       = clk_factor_set_rate,
+};
+EXPORT_SYMBOL_GPL(clk_factor_ops);
+
+struct clk *clk_register_factor(struct device *dev, const char *name,
+                               const char *parent_name, unsigned long flags,
+                               void __iomem *reg, u8 shift, u8 width,
+                               u8 clk_factor_flags, spinlock_t *lock)
+{
+       struct clk_init_data init;
+       struct clk_factor *factor;
+       struct clk *clk;
+
+       factor = kmalloc(sizeof(*factor), GFP_KERNEL);
+       if (!factor) {
+               pr_err("%s: could not allocate fixed factor clk\n", __func__);
+               return ERR_PTR(-ENOMEM);
+       }
+
+       init.name = name;
+       init.ops = &clk_factor_ops;
+       init.flags = flags | CLK_IS_BASIC;
+       init.parent_names = &parent_name;
+       init.num_parents = 1;
+
+       factor->reg = reg;
+       factor->shift = shift;
+       factor->width = width;
+       factor->flags = clk_factor_flags;
+       factor->lock = lock;
+       factor->hw.init = &init;
+
+       clk = clk_register(dev, &factor->hw);
+       if (IS_ERR(clk))
+               kfree(factor);
+
+       return clk;
+}
+EXPORT_SYMBOL_GPL(clk_register_factor);
+
+void clk_unregister_factor(struct clk *clk)
+{
+       struct clk_factor *factor;
+       struct clk_hw *hw;
+
+       hw = __clk_get_hw(clk);
+       if (!hw)
+               return;
+
+       factor = to_clk_factor(hw);
+
+       clk_unregister(clk);
+       kfree(factor);
+}
+EXPORT_SYMBOL_GPL(clk_unregister_factor);

include/linux/clk-provider.h +41 -1

index df695313f975..543918c378f5 100644
@@ -493,6 +493,47 @@ struct clk *clk_register_fractional_divider(struct device *dev,
                void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
                u8 clk_divider_flags, spinlock_t *lock);
 
+/**
+ * struct clk_factor - adjustable factor clock
+ *
+ * @hw:                handle between common and hardware-specific interfaces
+ * @reg:       register containing the factor
+ * @shift:     shift to the factor bit field
+ * @width:     width of the factor bit field
+ * @lock:      register lock
+ *
+ * Clock with an adjustable factor affecting its output frequency.
+ * Implements .recalc_rate, .set_rate and .round_rate
+ *
+ * Flags:
+ * CLK_FACTOR_ZERO_BYPASS - By default, the factor is the value read
+ *     from the register, with 0 being a valid value effectively
+ *     zeroing the output clock rate. If CLK_FACTOR_ZERO_BYPASS is
+ *     set, then a null factor will be considered as a bypass,
+ *     leaving the parent rate unmodified.
+ * CLK_FACTOR_ROUND_CLOSEST - Makes the best calculated divider to be
+ *     rounded to the closest integer instead of the down one.
+ */
+struct clk_factor {
+       struct clk_hw   hw;
+       void __iomem    *reg;
+       u8              shift;
+       u8              width;
+       u8              flags;
+       spinlock_t      *lock;
+};
+
+#define CLK_FACTOR_ZERO_BYPASS         BIT(0)
+#define CLK_FACTOR_ROUND_CLOSEST       BIT(1)
+
+extern const struct clk_ops clk_factor_ops;
+
+struct clk *clk_register_factor(struct device *dev, const char *name,
+                               const char *parent_name, unsigned long flags,
+                               void __iomem *reg, u8 shift, u8 width,
+                               u8 clk_factor_flags, spinlock_t *lock);
+void clk_unregister_factor(struct clk *clk);
+
 /***
  * struct clk_composite - aggregate clock of mux, divider and gate clocks
  *
--

2.4.1

Hi,

Quoted 3 lines · Maxime Ripard

Some clocks are using a factor component, however, unlike their mux, gate or divider counterpart, these factors don't have a basic clock implementation.

I think "multiplier" would be a better name here, considering it is the counterpart of "divider". "factor" implies you can multiply and/or divide the clock rate.

Unless of course you plan to add that? :)

Quoted 8 lines

Hi,

Quoted 3 lines · Maxime Ripard

Some clocks are using a factor component, however, unlike their mux, gate or divider counterpart, these factors don't have a basic clock implementation.

Quoted 3 lines

I think "multiplier" would be a better name here, considering it is the counterpart of "divider". "factor" implies you can multiply and/or divide the clock rate.

You're probably right, I didn't though of it that way :)

Quoted 2 lines

Unless of course you plan to add that? :)

Hmmm, no, not really.

Maxime

Signature · 3 lines

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

Quoting Maxime Ripard (2015-05-23 00:49:30)

Quoted 9 lines · Chen-Yu Tsai

Quoted 8 lines

Hi,

Quoted 3 lines · Maxime Ripard

Some clocks are using a factor component, however, unlike their mux, gate or divider counterpart, these factors don't have a basic clock implementation.

Quoted 5 lines

Quoted 3 lines

I think "multiplier" would be a better name here, considering it is the counterpart of "divider". "factor" implies you can multiply and/or divide the clock rate.

You're probably right, I didn't though of it that way :)

I also prefer multiplier over factor.

Jim Quinlan submitted a similar patch. See here:

http://www.spinics.net/lists/linux-clk/msg00691.html

I nacked that patch because Stephen and I are trying to figure out how the basic clock types should work going forward. Code reuse is good, but they are not very maintainable.

From a pure maintainance point of view, refusing it would mean that

Since there are two potential users of this code, I should reconsider.

Can you two come up with a common implementation that works for both of you?

Please do not put any CLK_OF_DECLARE stuff in there (there isn't any now, but I wanted to be clear).

Hi Mike,

Quoted 14 lines · Michael Turquette

Quoting Maxime Ripard (2015-05-23 00:49:30)

Quoted 13 lines · Chen-Yu Tsai

Quoted 11 lines

Hi,

Quoted 3 lines · Maxime Ripard

Some clocks are using a factor component, however, unlike their mux, gate or divider counterpart, these factors don't have a basic clock implementation.

I think "multiplier" would be a better name here, considering it is the counterpart of "divider". "factor" implies you can multiply and/or divide the clock rate.

Quoted 3 lines

Quoted 1 line

You're probably right, I didn't though of it that way :)

I also prefer multiplier over factor.

Ok. Note that I re-submitted it some time this week and forgot to change it, but I'm perfectly fine with the name.

Quoted 4 lines

Jim Quinlan submitted a similar patch. See here:

http://www.spinics.net/lists/linux-clk/msg00691.html

It looks very similar indeed.

Quoted 3 lines

I nacked that patch because Stephen and I are trying to figure out how the basic clock types should work going forward. Code reuse is good, but they are not very maintainable.

What are the issues with maintaining them? The only drawback I'm seeing with introducing such a driver is that you can't really have a clock that is both a divider and a multiplier, but that can be solved by splitting it into two sub-clocks.

From a pure maintainance point of view, refusing it would mean that each and every platform would have to come up with its own implementation. For example, we do have clk-factors.c for sunxi that does just that, and implies some cooperation from each clock driver that have to provide some code to determine the various components of the output formula. This can prove to be very challenging (and bug prone) for clocks like the audio one we have where we have 1 multiplier and 2 dividers that needs some adjusting.

Splitting it into sub-clocks for each of these components would allow to have less bugs, while keeping the whole thing very simple, and the implementation on the driver side very trivial.

Overall, the clk-factors code we have (client side) is approximately a thousand lines of code logic that could be replaced by (less) trivial probing code for such a driver.

Quoted 1 line

Since there are two potential users of this code, I should reconsider.

Like I said, eventually, I'd like to leverage that code a lot more than for the single clock alone, and I think it could benefit other platforms too (like Jim has proven).

Quoted 2 lines

Can you two come up with a common implementation that works for both of you?

Yes, sure. Jim, how do you want to go with this? Do you want to resubmit your patch on top of 4.2-rc something so that I could give it a try? Otherwise, I posted such a patch this week

https://lkml.kernel.org/r/1437235304-2208-1-git-send-email-maxime.ripard at free-electrons.com

Can you give it a try and your feedback?

Quoted 2 lines

Please do not put any CLK_OF_DECLARE stuff in there (there isn't any now, but I wanted to be clear).

That's never been my intention ;)

Thanks!
Maxime

Signature · 9 lines

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 4 lines

What are the issues with maintaining them? The only drawback I'm seeing with introducing such a driver is that you can't really have a clock that is both a divider and a multiplier, but that can be solved by splitting it into two sub-clocks.

There are a bunch of problems with the basic clock types. First is that there is some feature creep every merge window that subtly breaks an existing user (e.g. the round_rate stuff in clk-divider.c), and then there are the growing number of flags to handle corner cases that are one-off quirks for a single chip.

These make it harder to maintain, but it is possible.

The real problem with these basic clock types is that they are an abstraction layer at the wrong level. Each clock type implements both the policy of a given clock, as well as the machine-specific details. For example clk-divider.c has made some assumptions in the past about rounding the rate, or how to calculate the best divider; this is a matter of policy and is useful on its own. But additionally that same policy is glued to a specific implementation: memory-mapped register controls for a clock divider.

The I/O accessor stuff needs to be addressed at some point. Currently the basic clock types assume specific patterns of access to memory-mapped clock registers. There are lots of other clock controls out there that talk to firmware, or over i2c, or whatever. The amount of code that has to be copy/pasted for each different type of access is 100%; i.e. we do not have abstractions at the right level such as .get_best_div(struct clk_hw *hw, unsigned long rate).

What I would like to see in time is a re-usable layer for clock policy (e.g. common rules for how dividers or multipliers should behave), and then have that sit on top of the machine-specific callbacks that directly touch the hardware, such as the .get_best_div callback above.

For this reason I like to limit the number of new basic clock types. If there is a single user then I'm inclined to have the author put it with the machine-specific code. But in this case since there are two users, I see the value in making a new basic clock type.

Hi Mike,

Quoted 5 lines · Michael Turquette

Quoted 4 lines

What are the issues with maintaining them? The only drawback I'm seeing with introducing such a driver is that you can't really have a clock that is both a divider and a multiplier, but that can be solved by splitting it into two sub-clocks.

Quoted 7 lines

There are a bunch of problems with the basic clock types. First is that there is some feature creep every merge window that subtly breaks an existing user (e.g. the round_rate stuff in clk-divider.c), and then there are the growing number of flags to handle corner cases that are one-off quirks for a single chip.

These make it harder to maintain, but it is possible.

Ok. It's the downside of having common code I guess, everyone wants to use it, and the clocks are just more subject to it than other drivers :/

Quoted 17 lines

The real problem with these basic clock types is that they are an abstraction layer at the wrong level. Each clock type implements both the policy of a given clock, as well as the machine-specific details. For example clk-divider.c has made some assumptions in the past about rounding the rate, or how to calculate the best divider; this is a matter of policy and is useful on its own. But additionally that same policy is glued to a specific implementation: memory-mapped register controls for a clock divider.

The I/O accessor stuff needs to be addressed at some point. Currently the basic clock types assume specific patterns of access to memory-mapped clock registers. There are lots of other clock controls out there that talk to firmware, or over i2c, or whatever. The amount of code that has to be copy/pasted for each different type of access is 100%; i.e. we do not have abstractions at the right level such as .get_best_div(struct clk_hw *hw, unsigned long rate).

Quoted 4 lines

What I would like to see in time is a re-usable layer for clock policy (e.g. common rules for how dividers or multipliers should behave), and then have that sit on top of the machine-specific callbacks that directly touch the hardware, such as the .get_best_div callback above.

Can't that be solved by moving to regmap using Matthias' patches, or at least the IO method abstraction?

We would then have to only provide an additional callback then for providers that have specific requirements about the divider calculation.

So far, I haven't had any usecase where I needed anything but having the as-close-as-possible rate, so I would expect to not provide anything beside whether I'd like to round down or round up, but I do understand that some other might have different requirements.

Quoted 4 lines

For this reason I like to limit the number of new basic clock types. If there is a single user then I'm inclined to have the author put it with the machine-specific code. But in this case since there are two users, I see the value in making a new basic clock type.

That makes sense.

Maxime

Signature · 9 lines

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 22 lines

Quoted 21 lines

The real problem with these basic clock types is that they are an abstraction layer at the wrong level. Each clock type implements both the policy of a given clock, as well as the machine-specific details. For example clk-divider.c has made some assumptions in the past about rounding the rate, or how to calculate the best divider; this is a matter of policy and is useful on its own. But additionally that same policy is glued to a specific implementation: memory-mapped register controls for a clock divider.

The I/O accessor stuff needs to be addressed at some point. Currently the basic clock types assume specific patterns of access to memory-mapped clock registers. There are lots of other clock controls out there that talk to firmware, or over i2c, or whatever. The amount of code that has to be copy/pasted for each different type of access is 100%; i.e. we do not have abstractions at the right level such as .get_best_div(struct clk_hw *hw, unsigned long rate).

What I would like to see in time is a re-usable layer for clock policy (e.g. common rules for how dividers or multipliers should behave), and then have that sit on top of the machine-specific callbacks that directly touch the hardware, such as the .get_best_div callback above.

Quoted 2 lines

Can't that be solved by moving to regmap using Matthias' patches, or at least the IO method abstraction?

I need to look at those patches again, but I do not think they address the problem I described above: abstraction at the wrong level (or more specifically, combining two abstract layers into one).

I'll have more bandwidth to look at this problem after the next merge window. The clk framework is slowly be rewritten in bits and pieces here on the list (shh don't tell anybody) and this is on my radar and Stephen's.

Quoted 3 lines

Jim Quinlan submitted a similar patch. See here:

http://www.spinics.net/lists/linux-clk/msg00691.html

It looks very similar indeed.

Quoted 3 lines

I nacked that patch because Stephen and I are trying to figure out how the basic clock types should work going forward. Code reuse is good, but they are not very maintainable.

What are the issues with maintaining them? The only drawback I'm seeing with introducing such a driver is that you can't really have a clock that is both a divider and a multiplier, but that can be solved by splitting it into two sub-clocks.

From a pure maintainance point of view, refusing it would mean that each and every platform would have to come up with its own implementation. For example, we do have clk-factors.c for sunxi that does just that, and implies some cooperation from each clock driver that have to provide some code to determine the various components of the output formula. This can prove to be very challenging (and bug prone) for clocks like the audio one we have where we have 1 multiplier and 2 dividers that needs some adjusting.

Splitting it into sub-clocks for each of these components would allow to have less bugs, while keeping the whole thing very simple, and the implementation on the driver side very trivial.

Overall, the clk-factors code we have (client side) is approximately a thousand lines of code logic that could be replaced by (less) trivial probing code for such a driver.

Quoted 1 line

Since there are two potential users of this code, I should reconsider.

Like I said, eventually, I'd like to leverage that code a lot more than for the single clock alone, and I think it could benefit other platforms too (like Jim has proven).

Quoted 2 lines

Can you two come up with a common implementation that works for both of you?

Yes, sure. Jim, how do you want to go with this? Do you want to resubmit your patch on top of 4.2-rc something so that I could give it a try? Otherwise, I posted such a patch this week

https://lkml.kernel.org/r/1437235304-2208-1-git-send-email-maxime.ripard at free-electrons.com

Can you give it a try and your feedback?

Hi,

This serie adds support for the PLL2 aka the Audio PLL on the Allwinner A10 and the later SoCs.

This is the first stepping stone to get the audio support merged.

This serie is built on top of a generic clk-factor driver to handle clock that multiply their parent clock rate (mostly PLL's), in order to provide the driver for the PLL2 base clock, and then adds the drivers for the clock that derive from the Audio PLL.

Thanks!
Maxime