Add support for MSM's mmio clock/reset controller

If a user of <linux/reset-controller.h> doesn't include <linux/of.h> before including reset-controller.h they'll get a warning as follows:

include/linux/reset-controller.h:44:17:
warning: 'struct of_phandle_args' declared inside parameter list

This is because of_phandle_args is not forward declared. Add the declaration to silence this warning.

Cc Philipp Zabel <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 include/linux/reset-controller.h | 1 +
 1 file changed, 1 insertion(+)

include/linux/reset-controller.h +0 -0

index 2f61311..41a4695 100644

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.

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 <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/clk/clk.c            | 8 ++++++++
 include/linux/clk-provider.h | 7 +++++++
 2 files changed, 15 insertions(+)

drivers/clk/clk.c +1 -0

index 9ad7b71..5e71f5c 100644
@@ -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);

drivers/clk/clk.c +5 -0

+       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);

include/linux/clk-provider.h +7 -0

index 7e59253..31f2890 100644
@@ -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;
 };
 
 /*

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Quoting Stephen Boyd (2013-12-23 17:12:26)

Quoted 26 lines

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 <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/clk/clk.c            | 8 ++++++++
 include/linux/clk-provider.h | 7 +++++++
 2 files changed, 15 insertions(+)

drivers/clk/clk.c +1 -0

index 9ad7b71..5e71f5c 100644
@@ -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);

drivers/clk/clk.c static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) · line 1839

Quoted 5 lines

drivers/clk/clk.c +2 -0

@@ -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.

Quoted 4 lines · Mike Turquette

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,
                },
        },
};

Quoted 39 lines

Quoting Stephen Boyd (2013-12-23 17:12:26)

Quoted 26 lines

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 <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/clk/clk.c            | 8 ++++++++
 include/linux/clk-provider.h | 7 +++++++
 2 files changed, 15 insertions(+)

drivers/clk/clk.c +1 -0

index 9ad7b71..5e71f5c 100644
@@ -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);

drivers/clk/clk.c static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) · line 1839

Quoted 5 lines

drivers/clk/clk.c +2 -0

@@ -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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Signature · 3 lines

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

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.

Quoted 51 lines

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,
                },
        },
};

Quoted 2 lines

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?

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.

Quoted 4 lines

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.

Quoted 83 lines

Quoted 52 lines

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,
                },
        },
};

Quoted 2 lines

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:

Quoted 7 lines

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.

Signature · 3 lines

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

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.

Quoting Stephen Boyd (2013-12-23 17:12:26)

Quoted 26 lines

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 <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
  drivers/clk/clk.c            | 8 ++++++++
  include/linux/clk-provider.h | 7 +++++++
  2 files changed, 15 insertions(+)

drivers/clk/clk.c +1 -0

index 9ad7b71..5e71f5c 100644
@@ -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);

drivers/clk/clk.c static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) · line 1839

Quoted 5 lines

drivers/clk/clk.c +2 -0

@@ -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.

drivers/clk/clk.c static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) · line 1841

Quoted 2 lines

drivers/clk/clk.c +2 -0

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

Quoted 46 lines

Quoting Stephen Boyd (2013-12-23 17:12:26)

Quoted 26 lines

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 <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
  drivers/clk/clk.c            | 8 ++++++++
  include/linux/clk-provider.h | 7 +++++++
  2 files changed, 15 insertions(+)

drivers/clk/clk.c +1 -0

index 9ad7b71..5e71f5c 100644
@@ -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);

drivers/clk/clk.c static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) · line 1839

Quoted 5 lines

drivers/clk/clk.c +2 -0

@@ -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...

Quoted 3 lines

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.

Quoted 2 lines

drivers/clk/clk.c static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) · line 1841

Quoted 2 lines

drivers/clk/clk.c +2 -0

+       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

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

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.

Quoting Saravana Kannan (2014-01-13 19:54:42)

Quoted 58 lines · Mike Turquette

Quoted 46 lines

Quoting Stephen Boyd (2013-12-23 17:12:26)

Quoted 26 lines

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 <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
   drivers/clk/clk.c            | 8 ++++++++
   include/linux/clk-provider.h | 7 +++++++
   2 files changed, 15 insertions(+)

drivers/clk/clk.c +1 -0

index 9ad7b71..5e71f5c 100644
@@ -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);

drivers/clk/clk.c static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) · line 1839

Quoted 5 lines

drivers/clk/clk.c +2 -0

@@ -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...

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

drivers/clk/clk.c static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) · line 1841

Quoted 2 lines

drivers/clk/clk.c +2 -0

+       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?

Quoted 5 lines

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.

Quoted 7 lines

#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 clock framework already has support for simple gate clocks but if drivers want to use the gate clock functionality they need to wrap the gate clock in another struct and chain the ops by calling the gate ops from their own custom ops. Plus the gate

The clock framework already has support for simple gate clocks but if drivers want to use the gate clock functionality they need to wrap the gate clock in another struct and chain the ops by calling the gate ops from their own custom ops. Plus the gate clock implementation only supports MMIO accessors so other bus type clocks don't benefit from the potential code reuse. Add some simple regmap helpers for enable/disable/is_enabled that drivers can use as drop in replacements for their clock ops or as simple functions they call from their own custom ops. This is based on similar helps in the regulator framework.

Quoted 11 lines · Stephen Boyd

The clock framework already has support for simple gate clocks but if drivers want to use the gate clock functionality they need to wrap the gate clock in another struct and chain the ops by calling the gate ops from their own custom ops. Plus the gate clock implementation only supports MMIO accessors so other bus type clocks don't benefit from the potential code reuse. Add some simple regmap helpers for enable/disable/is_enabled that drivers can use as drop in replacements for their clock ops or as simple functions they call from their own custom ops. This is based on similar helps in the regulator framework.

The same comment applies as to the previous version. Is it useful to introduce copies of the gate handling while the difference in only in how the hardware registers get accessed?

Quoted 7 lines

include/linux/clk-provider.h +0 -0

@@ -177,11 +177,21 @@ struct clk_init_data {
[ ... ]

@@ -447,6 +457,9 @@ struct clk *__clk_lookup(const char *name);
 long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
 			      unsigned long *best_parent_rate,

include/linux/clk-provider.h struct clk *__clk_lookup(const char *name); · line 462

Quoted 4 lines

include/linux/clk-provider.h +3 -0

 			      struct clk **best_parent_p);
+int clk_is_enabled_regmap(struct clk_hw *hw);
+int clk_enable_regmap(struct clk_hw *hw);
+void clk_disable_regmap(struct clk_hw *hw);

Looking at the patch: Do you expect callers to remember whether a clock gate is backed by mmio or by regmap access, to call a different set of routines?

Should this not be hidden behind the
API and be transparent after clock registration?

I'd suggest to fold regmap support into Tero Kristo's ll_ops approach, and to discuss this in his v12 thread.

Quoted 13 lines · Gerhard Sittig

Quoted 11 lines · Stephen Boyd

The clock framework already has support for simple gate clocks but if drivers want to use the gate clock functionality they need to wrap the gate clock in another struct and chain the ops by calling the gate ops from their own custom ops. Plus the gate clock implementation only supports MMIO accessors so other bus type clocks don't benefit from the potential code reuse. Add some simple regmap helpers for enable/disable/is_enabled that drivers can use as drop in replacements for their clock ops or as simple functions they call from their own custom ops. This is based on similar helps in the regulator framework.

Quoted 4 lines

The same comment applies as to the previous version. Is it useful to introduce copies of the gate handling while the difference in only in how the hardware registers get accessed?

I don't plan to use the clk-gate.c implementation because I need more than just a bit toggling clock. We can easily make clk-gate.c use these helpers if you're worried about the very small amount of code duplication between the two. I'd be glad to do that, I just didn't include it here because I don't have a use for it.

Quoted 7 lines

Quoted 7 lines

include/linux/clk-provider.h +0 -0

@@ -177,11 +177,21 @@ struct clk_init_data {
[ ... ]

@@ -447,6 +457,9 @@ struct clk *__clk_lookup(const char *name);
 long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
 			      unsigned long *best_parent_rate,

Quoted 8 lines

include/linux/clk-provider.h struct clk *__clk_lookup(const char *name); · line 462

Quoted 4 lines

include/linux/clk-provider.h +3 -0

 			      struct clk **best_parent_p);
+int clk_is_enabled_regmap(struct clk_hw *hw);
+int clk_enable_regmap(struct clk_hw *hw);
+void clk_disable_regmap(struct clk_hw *hw);

Looking at the patch: Do you expect callers to remember whether a clock gate is backed by mmio or by regmap access, to call a different set of routines?

There are only regmap functions. I'm not sure where the choice is, but I expect the callers to know what they're doing. If you look at the rest of this series you'll see that I assign these functions directly to the clk_ops, or I call them from the enable/disable functions that need to do some status bit polling after the clock is enabled or disabled.

Quoted 2 lines

Should this not be hidden behind the
API and be transparent after clock registration?

I don't really understand what you mean by hiding it behind the API? What API? If we're talking about clk_register_gate() I think we would need to add a clk_register_regmap_gate() function because the reg argument is an __iomem pointer. It doesn't look like it can be transparent unless that pointer is reused as an offset. I don't attempt to do anything about that here though because I don't use the clk-gate.c code.