| Message ID | 1387847559-18330-3-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Dec 23, 2013 at 05:12:26PM -0800, Stephen Boyd wrote: > 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. Reviewed-by: Mark Brown <broonie@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/08/14 17:51, 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@kernel.org> >> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> >> --- >> 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... The regulator framework has gone this way. It seemed like a similar approach in the clock framework would be the right way to go too. > > 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 see two ways forward if you don't want these members in struct clk_hw. 1) Inheritance: struct clk_regmap wrapper struct and clk_register_regmap() and devm_clk_register_regmap() and then another wrapper struct around that. example: struct clk_regmap { struct clk_hw hw; struct regmap *regmap; unsigned int enable_reg; unsigned int enable_mask; bool enable_is_inverted; }; struct clk_branch { u32 hwcg_reg; u32 halt_reg; u8 hwcg_bit; u8 halt_bit; u8 halt_check; struct clk_regmap clkr; }; static struct clk_branch gsbi1_uart_clk = { .halt_reg = 0x2fcc, .halt_bit = 10, .clkr = { .enable_reg = 0x29d4, .enable_mask = BIT(9), .hw.init = &(struct clk_init_data){ .name = "gsbi1_uart_clk", .parent_names = (const char *[]){ "gsbi1_uart_src", }, .num_parents = 1, .ops = &clk_branch_ops, .flags = CLK_SET_RATE_PARENT, }, }, }; 2) Interfaces: Add a void *data in struct clk_hw that can point to whatever I want and still have the same clk_regmap_register() and devm_clk_regmap_register() Example: struct clk_hw { struct clk *clk; const struct clk_init_data *init; void *data; }; struct clk_regmap { struct regmap *regmap; unsigned int enable_reg; unsigned int enable_mask; bool enable_is_inverted; }; struct clk_branch { u32 hwcg_reg; u32 halt_reg; u8 hwcg_bit; u8 halt_bit; u8 halt_check; struct clk_hw; }; static struct clk_branch gsbi1_uart_clk = { .halt_reg = 0x2fcc, .halt_bit = 10, .hw = { .data = &(struct clk_regmap){ .enable_reg = 0x29d4, .enable_mask = BIT(9), }; .init = &(struct clk_init_data){ .name = "gsbi1_uart_clk", .parent_names = (const char *[]){ "gsbi1_uart_src", }, .num_parents = 1, .ops = &clk_branch_ops, .flags = CLK_SET_RATE_PARENT, }, }, }; I guess option 2 is less likely given your comment about clk_hw being nothing more than a traversal mechanism.
On 01/08/14 18:11, Stephen Boyd wrote: > On 01/08/14 17:51, Mike Turquette wrote: >> 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 see two ways forward if you don't want these members in struct clk_hw. > > 1) Inheritance: struct clk_regmap wrapper struct and > clk_register_regmap() and devm_clk_register_regmap() and then another > wrapper struct around that. > > example: > > struct clk_regmap { > struct clk_hw hw; > struct regmap *regmap; > unsigned int enable_reg; > unsigned int enable_mask; > bool enable_is_inverted; > }; > > struct clk_branch { > u32 hwcg_reg; > u32 halt_reg; > u8 hwcg_bit; > u8 halt_bit; > u8 halt_check; > > struct clk_regmap clkr; > }; > > static struct clk_branch gsbi1_uart_clk = { > .halt_reg = 0x2fcc, > .halt_bit = 10, > .clkr = { > .enable_reg = 0x29d4, > .enable_mask = BIT(9), > .hw.init = &(struct clk_init_data){ > .name = "gsbi1_uart_clk", > .parent_names = (const char *[]){ > "gsbi1_uart_src", > }, > .num_parents = 1, > .ops = &clk_branch_ops, > .flags = CLK_SET_RATE_PARENT, > }, > }, > }; The downside to this approach is that we have to have two similar structs for struct clk_gate if we want to support regmap in that code path. If we put the regmap inside struct clk_hw we don't have two different structs, we would just assign different ops. struct clk_gate { struct clk_hw hw; void __iomem *reg; u8 bit_idx; u8 flags; spinlock_t *lock; }; and struct clk_gate_regmap { struct clk_regmap hw; u8 flags; spinlock_t *lock; }; Do you have any preference on which way we move forward here? I have the wrapper method all finished and ready to send if you agree with that approach.
On 01/09, Mike Turquette wrote: > If we're going to use these wrappers, why make it regmap specific? The > struct clk_desc patches[1][2] can achieve this, but in a more generic > way. > I think you're suggesting a way to avoid adding a clk_register_regmap() function? But won't we need to write the same code: if (dev && dev_get_regmap(dev, NULL)) [clk_type]->regmap = dev_get_regmap(dev, NULL); else if (dev && dev->parent) [clk_type]->regmap = dev_get_regmap(dev->parent, NULL); everytime we want to assign the regmap pointer to a different clock type? A macro might work for this little snippet, but it wouldn't have any type safety. > > > > > > 2) Interfaces: Add a void *data in struct clk_hw that can point to > > whatever I want and still have the same clk_regmap_register() and > > devm_clk_regmap_register() > > > > Example: > > > > struct clk_hw { > > struct clk *clk; > > const struct clk_init_data *init; > > void *data; > > }; > > > > struct clk_regmap { > > struct regmap *regmap; > > unsigned int enable_reg; > > unsigned int enable_mask; > > bool enable_is_inverted; > > }; > > > > struct clk_branch { > > u32 hwcg_reg; > > u32 halt_reg; > > u8 hwcg_bit; > > u8 halt_bit; > > u8 halt_check; > > > > struct clk_hw; > > }; > > > > static struct clk_branch gsbi1_uart_clk = { > > .halt_reg = 0x2fcc, > > .halt_bit = 10, > > .hw = { > > .data = &(struct clk_regmap){ > > .enable_reg = 0x29d4, > > .enable_mask = BIT(9), > > }; > > .init = &(struct clk_init_data){ > > .name = "gsbi1_uart_clk", > > .parent_names = (const char *[]){ > > "gsbi1_uart_src", > > }, > > .num_parents = 1, > > .ops = &clk_branch_ops, > > .flags = CLK_SET_RATE_PARENT, > > }, > > }, > > }; > > > > I guess option 2 is less likely given your comment about clk_hw being > > nothing more than a traversal mechanism. > > Instead of private data, how about a .register() callback function that > can point to anything you like? The clk_desc patches implement this and > it would suffice for registering regmap ops or anything else, without > polluting struct clk_hw. > > [1] http://www.spinics.net/lists/linux-omap/msg101822.html > [2] http://www.spinics.net/lists/linux-omap/msg101698.html > > So you could statically define gsbi1_uart_clk with: > > static struct clk_branch_desc gsbi1_uart_clk_desc = { > .halt_reg = 0x2fcc, > .halt_bit = 10, > .enable_reg = 0x29d4, > .enable_mask = BIT(9), > .desc = { > .name = "gsbi1_uart_clk", > .parent_names = (const char *[]){ > "gsbi1_uart_src", > }, > .num_parents = 1, > .ops = &clk_branch_ops, > .flags = CLK_SET_RATE_PARENT, > }, > }; > > And then register it with: > > clk_register_desc(NULL, &gsbi1_uart_clk_desc.desc); > > This is very analogous to the way that you use use &gsbi1_uart_clk.hw > but it is more generic and also doesn't pollute clk_hw any further. I > also think your static data is quite a bit prettier using this method. > > Thoughts? Is the plan to allocate a struct clk_branch at runtime and then copy all the fields over one by one? I'm trying to avoid that because it takes more time and more runtime memory. If I had to go the descriptor route I would probably avoid copying any fields and just point to the descriptor from struct clk_branch, i.e. struct clk_branch { struct clk_branch_desc *desc; struct clk_hw; }; but that still seems wasteful to allocate a bunch of little pointer wrappers when I could have just embedded the clk_hw struct inside the clk_branch struct from the start. It feels another key point is being missed though. The regmap pointer and the enable_reg/enable_mask is embedded in clk_hw to allow the same code to be used by different types of surrounding structs. Each struct: clk_pll, clk_rcg, and clk_branch in this series use the regmap interface to enable/disable the clock and they can easily do so by passing something that's always available from struct clk_hw (be it via a wrapper struct, private data member, or addition of new fields to clk_hw). If the regmap members move into each specific type of clock we can't just pass a single pointer to the enable/disable regmap functions anymore. This is the reason why I suggested a driver data pointer or container struct so that everything regmap related is contained within one type.
On 01/09/14 23:05, Stephen Boyd wrote: > It feels another key point is being missed though. The regmap > pointer and the enable_reg/enable_mask is embedded in clk_hw to > allow the same code to be used by different types of surrounding > structs. Each struct: clk_pll, clk_rcg, and clk_branch in this > series use the regmap interface to enable/disable the clock and > they can easily do so by passing something that's always > available from struct clk_hw (be it via a wrapper struct, private > data member, or addition of new fields to clk_hw). If the regmap > members move into each specific type of clock we can't just pass > a single pointer to the enable/disable regmap functions anymore. > This is the reason why I suggested a driver data pointer or > container struct so that everything regmap related is contained > within one type. > Any thoughts?
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@kernel.org> >> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> >> --- >> 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. 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. Thanks, Saravana
On Wed, Jan 15, 2014 at 01:36:41AM -0800, Mike Turquette wrote: > 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. What we've tried to do with regulators is just have multiple helper functions in the core rather than trying to merge their implementation, the drivers pick the right one for their usage, and in cases where there aren't many drivers doing the same thing we just have the driver either copy the bits of the implementation they reuse or call the helper function from within the implementation.
On 01/15, Mike Turquette wrote: > > So in summary: consolidation is over-rated. You can put your regmap > functions into drivers/clk/msm/ and not convert over to struct clk_desc > by the way, just to remove any confusion on that point. > Ok I've made the wrapper struct clk_regmap and posted the patches. I think I've addressed all your concerns.
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@kernel.org> >>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> >>>> --- >>>> 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
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; + else if (dev && dev_get_regmap(dev, NULL)) + hw->regmap = dev_get_regmap(dev, NULL); + else if (dev && dev->parent) + hw->regmap = dev_get_regmap(dev->parent, NULL); + /* allocate local copy in case parent_names is __initdata */ clk->parent_names = kcalloc(clk->num_parents, sizeof(char *), GFP_KERNEL); diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 7e59253..31f2890 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -142,6 +142,8 @@ struct clk_ops { void (*init)(struct clk_hw *hw); }; +struct regmap; + /** * struct clk_init_data - holds init data that's common to all clocks and is * shared between the clock provider and the common clock framework. @@ -151,6 +153,7 @@ struct clk_ops { * @parent_names: array of string names for all possible parents * @num_parents: number of possible parents * @flags: framework-level hints and quirks + * @regmap: regmap to use for regmap helpers and/or by providers */ struct clk_init_data { const char *name; @@ -158,6 +161,7 @@ struct clk_init_data { const char **parent_names; u8 num_parents; unsigned long flags; + struct regmap *regmap; }; /** @@ -171,10 +175,13 @@ struct clk_init_data { * * @init: pointer to struct clk_init_data that contains the init data shared * with the common clock framework. + * + * @regmap: regmap to use for regmap helpers and/or by providers */ struct clk_hw { struct clk *clk; const struct clk_init_data *init; + struct regmap *regmap; }; /*
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@kernel.org> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/clk/clk.c | 8 ++++++++ include/linux/clk-provider.h | 7 +++++++ 2 files changed, 15 insertions(+)