Re: [PATCH v4 02/15] clk: Allow drivers to pass in a regmap

From: Saravana Kannan
Date: Thu Jan 16 2014 - 20:38:50 EST

On 01/15/2014 01:36 AM, Mike Turquette wrote:
Quoting Saravana Kannan (2014-01-13 19:54:42)
On 01/08/2014 05:51 PM, Mike Turquette wrote:
Quoting Stephen Boyd (2013-12-23 17:12:26)
Add support to the clock core so that drivers can pass in a
regmap. If no regmap is specified try to query the device that's
registering the clock for its regmap. This should allow drivers
to use the core regmap helpers. This is based on a similar design
in the regulator framework.

Cc: Mark Brown <broonie@xxxxxxxxxx>
Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
---
drivers/clk/clk.c | 8 ++++++++
include/linux/clk-provider.h | 7 +++++++
2 files changed, 15 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9ad7b71..5e71f5c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -20,6 +20,7 @@
#include <linux/device.h>
#include <linux/init.h>
#include <linux/sched.h>
+#include <linux/regmap.h>

static DEFINE_SPINLOCK(enable_lock);
static DEFINE_MUTEX(prepare_lock);
@@ -1834,6 +1835,13 @@ static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk)
clk->num_parents = hw->init->num_parents;
hw->clk = clk;

+ if (hw->init->regmap)
+ hw->regmap = hw->init->regmap;

Hi Stephen,

The whole series looks good to me except for the placement of the regmap
details inside struct clk_hw. That structure exists only to hide struct
clk from the hardware-specific clock structure and I'd not like to set
the precedent of shoving per-clock data into it.

As an alternative, how about finding a way to put these per-clock regmap
details into the hardware-specific clock structure? I understand that
you want to make these ops available to others, which is why they are in
the public struct clk_hw. I'm just wondering if that is the right way to
do it...

Patch #3 illustrates the sort of struct-member-creep that worries me.
What is to stop someone from putting "unsigned int divider_reg" or
"unsigned int mux_reg", and then the thing just keeps growing.

I agree with Mike here. This definitely encourages struct field creep if
more people want to use it.

I talked to Stephen is person and my recommendation is to not have any
new fields other than struct regmap in clk_hw and remove the above 2
lines of code.

+ else if (dev && dev_get_regmap(dev, NULL))
+ hw->regmap = dev_get_regmap(dev, NULL);

Move "struct regmap *regmap" into struct clk_hw (since it's truly
reusable across clock types and is technically purely HW related) and
update it from the device's regmap like above.

Hi Saravana,

Thanks for your comments. In the paragraph above you mean "struct
clk_hw" or do you mean the hardware-specific structure(s) defined in a
clock driver?


We can then provide __clk_regmap_enable(regmap, offset, enable_mask)
helper functions. Then clock specific functions can use the helper. We
can even a simple macro to generate these wrappers.

#define DEFINE_REGMAP_EN_DIS(clktype) \

int clk_type##_enable(clktype *c, ....) { }
int clk_type##_disable(clktype *c, ....) { }


That to me seems like a reasonable compromise.

Providing common functions for the basic case (e.g. read-modify-write on
a register using a known mask) is reasonable. But that is exactly what
the existing basic clock types do (sans regmap) and they have all become
pretty ugly over time. And the clk-composite implementation just makes
me a sad panda.

I'm not opposed to providing public implementations of clk_ops callbacks
that use regmap, but I will be very mindful of any feature creep in the
future.

I am still unconvinced that adding struct regmap to struct clk_hw is a
good idea. The regmap data is a function of hardware-specific details
and those details always have and always will belong in the clock
driver

The details of the bits inside the register and how it's used would be clock type specific. Meaning, a Vendor X clock gate might have a busy bit and Vendor Y clock gate might not have a busy bit.

Regmap doesn't try to consolidate that. I'm not saying those clock gates should not be consolidated, but that's not what regmap is trying to do.

Regmap is helpful to consolidate clocks that behave exactly the same way, but differ in how the clock registers are accessed. Eg: mem-mapped IO, I2C, CP15 read/writes. So, even with a MSM based board, just because the way to access the registers are different, we could have to implement 3 different clock gate types. Which I think would be the wrong thing to do. Regmap cleanly consolidates this because the clock types just need to store the offsets and operate on them as usual using regmap APIs.

Since the regmap for the clock is assigned based on the device that's registering the enable/disable code for a clock gate can be agnostic of how the registers are accesses. Yes, regmap does provide helpers for read/modify writes, but that's not the main reason for using it.

Btw, this is a very real case on MSMs. The CPU clocks are accesses thru CP15 instructions, and the other clock trees are accessed through mem-mapped IO. But they pretty much behave the same way. That's what reg map is solving.

And I think this is a common problem for ANY clock type that one might implement. Which is why I think it should be in some common clock struct. The clock drivers will need access to the regmap. So, I picked clk_hw (the struct defined by the clock framework) and not struct clk. This also allows for the framework registration code to go populate the regmap pointer for all clocks from the regmap of the device.

Thoughts?

Thanks,
Saravana

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
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/