[PATCH v1] clk: qcom: Add support for regmap clock dividers

[PATCH v1] clk: qcom: Add support for regmap clock dividers

[Georgi Djakov]

This patch expands the regmap support to allow registration of clock
dividers. It just prepares for the introduction of a clkdiv driver,
that will be in a separate patch.
Such dividers are found in the Qualcomm PMIC chips such as PM8941,
PMA8084 and others.

Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
---
 drivers/clk/qcom/clk-regmap.c |   68 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap.h |   24 +++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/drivers/clk/qcom/clk-regmap.c b/drivers/clk/qcom/clk-regmap.c
index a58ba39..d63b8ca 100644
--- a/drivers/clk/qcom/clk-regmap.c
+++ b/drivers/clk/qcom/clk-regmap.c
@@ -112,3 +112,71 @@ struct clk *devm_clk_register_regmap(struct device *dev,
 	return devm_clk_register(dev, &rclk->hw);
 }
 EXPORT_SYMBOL_GPL(devm_clk_register_regmap);
+
+/**
+ * clkdiv_recalc_rate_regmap - Calculate clock divider output rate
+ */
+unsigned long clkdiv_recalc_rate_regmap(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct clk_regmap *rclk = to_clk_regmap(hw);
+	struct clkdiv_regmap *clkdiv = to_clkdiv_regmap(rclk);
+	unsigned int div, val;
+
+	regmap_read(rclk->regmap, clkdiv->reg, &val);
+	if (!val)
+		return parent_rate;
+
+	div = (val >> clkdiv->shift) & ((1 << clkdiv->width)-1);
+
+	return parent_rate / div;
+}
+EXPORT_SYMBOL_GPL(clkdiv_recalc_rate_regmap);
+
+static const struct clk_ops clkdiv_ops = {
+	.enable = clk_enable_regmap,
+	.disable = clk_disable_regmap,
+	.is_enabled = clk_is_enabled_regmap,
+	.recalc_rate = clkdiv_recalc_rate_regmap,
+};
+EXPORT_SYMBOL_GPL(clkdiv_ops);
+
+/**
+ * devm_clkdiv_register_regmap - register a clk_regmap clock divider
+ *
+ * @dev: device that is registering this clock
+ * @name: name of this clock
+ * @parent_name: name of clock's parent
+ * @flags: framework-specific flags
+ * @clkdiv: clock divider to operate on
+ *
+ * Clocks that use regmap for their register I/O should register their
+ * clk_regmap struct via this function so that the regmap is initialized
+ * and so that the clock is registered with the common clock framework.
+ */
+struct clk *devm_clkdiv_register_regmap(struct device *dev, const char *name,
+					const char *parent_name,
+					unsigned long flags,
+					struct clkdiv_regmap *clkdiv)
+{
+	struct clk_init_data init;
+
+	if (!clkdiv)
+		return ERR_PTR(-ENODEV);
+
+	clkdiv->rclk.regmap = dev_get_regmap(dev->parent, NULL);
+
+	if (!clkdiv->rclk.regmap)
+		return ERR_PTR(-ENXIO);
+
+	init.name = name;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+	init.flags = flags;
+	init.ops = &clkdiv_ops;
+
+	clkdiv->rclk.hw.init = &init;
+
+	return devm_clk_register(dev, &clkdiv->rclk.hw);
+}
+EXPORT_SYMBOL_GPL(devm_clkdiv_register_regmap);
diff --git a/drivers/clk/qcom/clk-regmap.h b/drivers/clk/qcom/clk-regmap.h
index 491a63d..02582cf 100644
--- a/drivers/clk/qcom/clk-regmap.h
+++ b/drivers/clk/qcom/clk-regmap.h
@@ -42,4 +42,28 @@ void clk_disable_regmap(struct clk_hw *hw);
 struct clk *
 devm_clk_register_regmap(struct device *dev, struct clk_regmap *rclk);
 
+/**
+ * struct clkdiv_regmap - regmap supporting clock divider
+ * @rclk:   regmap supporting clock struct
+ * @reg:    offset into regmap for the control register
+ * @shift:  bit position for divider value
+ * @width:  number of bits used for divider value
+ * @table:  pointer to table containing an array of divider/value pairs
+ */
+struct clkdiv_regmap {
+	struct clk_regmap rclk;
+	unsigned int reg;
+	unsigned int shift;
+	unsigned int width;
+	struct clk_div_table *table;
+};
+
+#define to_clkdiv_regmap(_rclk) container_of(_rclk, struct clkdiv_regmap, rclk)
+
+unsigned long
+clkdiv_recalc_rate_regmap(struct clk_hw *hw, unsigned long parent_rate);
+struct clk *
+devm_clkdiv_register_regmap(struct device *dev, const char *name,
+			    const char *parent_name, unsigned long flags,
+			    struct clkdiv_regmap *clkdiv);
 #endif
-- 
1.7.9.5

Re: [PATCH v1] clk: qcom: Add support for regmap clock dividers

[Stephen Boyd]

On 09/30/14 10:20, Georgi Djakov wrote:
> This patch expands the regmap support to allow registration of clock
> dividers. It just prepares for the introduction of a clkdiv driver,
> that will be in a separate patch.
> Such dividers are found in the Qualcomm PMIC chips such as PM8941,
> PMA8084 and others.

We're going to need to rework the Makefile in clk/qcom so that we only
build certain pieces of the "library" when we need them. Right now the
directory is focused entirely on mmio clock controllers and if we put
the pmic clocks in there then we need to figure out a way to only build
the pmic pieces if only the pmic driver is selected and only build the
mmio pieces if the mmio drivers are selected.

>
> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
> ---
>  drivers/clk/qcom/clk-regmap.c |   68 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap.h |   24 +++++++++++++++
>  2 files changed, 92 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-regmap.c b/drivers/clk/qcom/clk-regmap.c
> index a58ba39..d63b8ca 100644
> --- a/drivers/clk/qcom/clk-regmap.c
> +++ b/drivers/clk/qcom/clk-regmap.c
> @@ -112,3 +112,71 @@ struct clk *devm_clk_register_regmap(struct device *dev,
>  	return devm_clk_register(dev, &rclk->hw);
>  }
>  EXPORT_SYMBOL_GPL(devm_clk_register_regmap);
> +
> +/**
> + * clkdiv_recalc_rate_regmap - Calculate clock divider output rate

Arguments should be documented?
> + */
> +unsigned long clkdiv_recalc_rate_regmap(struct clk_hw *hw,

Or this should be static.

> +					unsigned long parent_rate)
> +{
> +	struct clk_regmap *rclk = to_clk_regmap(hw);
> +	struct clkdiv_regmap *clkdiv = to_clkdiv_regmap(rclk);
> +	unsigned int div, val;
> +
> +	regmap_read(rclk->regmap, clkdiv->reg, &val);
> +	if (!val)
> +		return parent_rate;
> +
> +	div = (val >> clkdiv->shift) & ((1 << clkdiv->width)-1);
> +
> +	return parent_rate / div;

I don't know if you saw the patch to split out the clk-divider.c logic
from the readl/writel patch I sent[1]? That could make this slightly
smaller.
tabl
> +}
> +EXPORT_SYMBOL_GPL(clkdiv_recalc_rate_regmap);
> +
> +static const struct clk_ops clkdiv_ops = {
> +	.enable = clk_enable_regmap,
> +	.disable = clk_disable_regmap,

This isn't exactly a divider if it also has enable/disable control. At
which point this starts to look like a composite clock. Perhaps call
this clk_div_gate_ops?

> +	.is_enabled = clk_is_enabled_regmap,
> +	.recalc_rate = clkdiv_recalc_rate_regmap,

No .set_rate? So call it clk_fixed_div_gate_ops?

> +};
> +EXPORT_SYMBOL_GPL(clkdiv_ops);
> +
> +/**
> + * devm_clkdiv_register_regmap - register a clk_regmap clock divider
> + *
> + * @dev: device that is registering this clock
> + * @name: name of this clock
> + * @parent_name: name of clock's parent
> + * @flags: framework-specific flags
> + * @clkdiv: clock divider to operate on
> + *
> + * Clocks that use regmap for their register I/O should register their
> + * clk_regmap struct via this function so that the regmap is initialized
> + * and so that the clock is registered with the common clock framework.
> + */
> +struct clk *devm_clkdiv_register_regmap(struct device *dev, const char *name,
> +					const char *parent_name,
> +					unsigned long flags,
> +					struct clkdiv_regmap *clkdiv)
> +{
> +	struct clk_init_data init;
> +
> +	if (!clkdiv)
> +		return ERR_PTR(-ENODEV);

Looks like a useless check. Just blow up instead so we don't have to
tolerate bad code.

> +
> +	clkdiv->rclk.regmap = dev_get_regmap(dev->parent, NULL);
> +
> +	if (!clkdiv->rclk.regmap)
> +		return ERR_PTR(-ENXIO);
> +
> +	init.name = name;
> +	init.parent_names = (parent_name ? &parent_name : NULL);
> +	init.num_parents = (parent_name ? 1 : 0);
> +	init.flags = flags;
> +	init.ops = &clkdiv_ops;
> +
> +	clkdiv->rclk.hw.init = &init;
> +
> +	return devm_clk_register(dev, &clkdiv->rclk.hw);
> +}
> +EXPORT_SYMBOL_GPL(devm_clkdiv_register_regmap);
> diff --git a/drivers/clk/qcom/clk-regmap.h b/drivers/clk/qcom/clk-regmap.h
> index 491a63d..02582cf 100644
> --- a/drivers/clk/qcom/clk-regmap.h
> +++ b/drivers/clk/qcom/clk-regmap.h
> @@ -42,4 +42,28 @@ void clk_disable_regmap(struct clk_hw *hw);
>  struct clk *
>  devm_clk_register_regmap(struct device *dev, struct clk_regmap *rclk);
>  
> +/**
> + * struct clkdiv_regmap - regmap supporting clock divider
> + * @rclk:   regmap supporting clock struct
> + * @reg:    offset into regmap for the control register
> + * @shift:  bit position for divider value
> + * @width:  number of bits used for divider value
> + * @table:  pointer to table containing an array of divider/value pairs
> + */
> +struct clkdiv_regmap {
> +	struct clk_regmap rclk;
> +	unsigned int reg;
> +	unsigned int shift;
> +	unsigned int width;
> +	struct clk_div_table *table;

Is this used?

> +};
> +
> +#define to_clkdiv_regmap(_rclk) container_of(_rclk, struct clkdiv_regmap, rclk)
> +
> +unsigned long
> +clkdiv_recalc_rate_regmap(struct clk_hw *hw, unsigned long parent_rate);
> +struct clk *
> +devm_clkdiv_register_regmap(struct device *dev, const char *name,
> +			    const char *parent_name, unsigned long flags,
> +			    struct clkdiv_regmap *clkdiv);
>  #endif


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

Re: [PATCH v1] clk: qcom: Add support for regmap clock dividers

[Kumar Gala]

On Oct 2, 2014, at 1:11 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> On 09/30/14 10:20, Georgi Djakov wrote:
>> This patch expands the regmap support to allow registration of clock
>> dividers. It just prepares for the introduction of a clkdiv driver,
>> that will be in a separate patch.
>> Such dividers are found in the Qualcomm PMIC chips such as PM8941,
>> PMA8084 and others.
> 
> We're going to need to rework the Makefile in clk/qcom so that we only
> build certain pieces of the "library" when we need them. Right now the
> directory is focused entirely on mmio clock controllers and if we put
> the pmic clocks in there then we need to figure out a way to only build
> the pmic pieces if only the pmic driver is selected and only build the
> mmio pieces if the mmio drivers are selected.

Would we ever not build the mmio drivers or do you mean something else?

- k

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

Re: [PATCH v1] clk: qcom: Add support for regmap clock dividers

[Stephen Boyd]

On 10/02/14 12:51, Kumar Gala wrote:
> On Oct 2, 2014, at 1:11 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
>> On 09/30/14 10:20, Georgi Djakov wrote:
>>> This patch expands the regmap support to allow registration of clock
>>> dividers. It just prepares for the introduction of a clkdiv driver,
>>> that will be in a separate patch.
>>> Such dividers are found in the Qualcomm PMIC chips such as PM8941,
>>> PMA8084 and others.
>> We're going to need to rework the Makefile in clk/qcom so that we only
>> build certain pieces of the "library" when we need them. Right now the
>> directory is focused entirely on mmio clock controllers and if we put
>> the pmic clocks in there then we need to figure out a way to only build
>> the pmic pieces if only the pmic driver is selected and only build the
>> mmio pieces if the mmio drivers are selected.
> Would we ever not build the mmio drivers or do you mean something else?
>
>

Yes I mean we may not build the mmio drivers.

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

Re: [PATCH v1] clk: qcom: Add support for regmap clock dividers

[Georgi Djakov]

On 10/02/2014 09:11 PM, Stephen Boyd wrote:
> On 09/30/14 10:20, Georgi Djakov wrote:
>> This patch expands the regmap support to allow registration of clock
>> dividers. It just prepares for the introduction of a clkdiv driver,
>> that will be in a separate patch.
>> Such dividers are found in the Qualcomm PMIC chips such as PM8941,
>> PMA8084 and others.
> 
> We're going to need to rework the Makefile in clk/qcom so that we only
> build certain pieces of the "library" when we need them. Right now the
> directory is focused entirely on mmio clock controllers and if we put
> the pmic clocks in there then we need to figure out a way to only build
> the pmic pieces if only the pmic driver is selected and only build the
> mmio pieces if the mmio drivers are selected.
> 

Ok, the pmic clocks currently depend only on clk-regmap.o, but if we prefer
to separate this part of the "library", it could be moved into a separate
file. Will update.

>>
>> Signed-off-by: Georgi Djakov <gdjakov@mm-sol.com>
>> ---
>>  drivers/clk/qcom/clk-regmap.c |   68 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/qcom/clk-regmap.h |   24 +++++++++++++++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-regmap.c b/drivers/clk/qcom/clk-regmap.c
>> index a58ba39..d63b8ca 100644
>> --- a/drivers/clk/qcom/clk-regmap.c
>> +++ b/drivers/clk/qcom/clk-regmap.c
>> @@ -112,3 +112,71 @@ struct clk *devm_clk_register_regmap(struct device *dev,
>>  	return devm_clk_register(dev, &rclk->hw);
>>  }
>>  EXPORT_SYMBOL_GPL(devm_clk_register_regmap);
>> +
>> +/**
>> + * clkdiv_recalc_rate_regmap - Calculate clock divider output rate
> 
> Arguments should be documented?
>> + */
>> +unsigned long clkdiv_recalc_rate_regmap(struct clk_hw *hw,
> 
> Or this should be static.
> 
>> +					unsigned long parent_rate)
>> +{
>> +	struct clk_regmap *rclk = to_clk_regmap(hw);
>> +	struct clkdiv_regmap *clkdiv = to_clkdiv_regmap(rclk);
>> +	unsigned int div, val;
>> +
>> +	regmap_read(rclk->regmap, clkdiv->reg, &val);
>> +	if (!val)
>> +		return parent_rate;
>> +
>> +	div = (val >> clkdiv->shift) & ((1 << clkdiv->width)-1);
>> +
>> +	return parent_rate / div;
> 
> I don't know if you saw the patch to split out the clk-divider.c logic
> from the readl/writel patch I sent[1]? That could make this slightly
> smaller.
> tabl

Could you please provide a link to that patch?

>> +}
>> +EXPORT_SYMBOL_GPL(clkdiv_recalc_rate_regmap);
>> +
>> +static const struct clk_ops clkdiv_ops = {
>> +	.enable = clk_enable_regmap,
>> +	.disable = clk_disable_regmap,
> 
> This isn't exactly a divider if it also has enable/disable control. At
> which point this starts to look like a composite clock. Perhaps call
> this clk_div_gate_ops?
> 
>> +	.is_enabled = clk_is_enabled_regmap,
>> +	.recalc_rate = clkdiv_recalc_rate_regmap,
> 
> No .set_rate? So call it clk_fixed_div_gate_ops?
> 

Actually there is a set_rate that i somehow missed here, it looks this way:

int clkdiv_set_rate_regmap(struct clk_hw *hw, unsigned long rate,
                           unsigned long parent_rate)
{
        struct clk_regmap *rclk = to_clk_regmap(hw);
        struct clkdiv_regmap *clkdiv = to_clkdiv_regmap(rclk);
        struct clk_div_table *table = clkdiv->table;
        unsigned long div;

        if (rate > parent_rate)
                return -EINVAL;

        div = DIV_ROUND_UP(parent_rate, rate);
                                                                                                                                                                                             
        for(; table->div; table++)
                if (table->div == div)
                        return regmap_update_bits(rclk->regmap, clkdiv->reg,
                                                  rclk->enable_mask,
                                                  table->val);
        return -EINVAL;
}

>> +};
>> +EXPORT_SYMBOL_GPL(clkdiv_ops);
>> +
>> +/**
>> + * devm_clkdiv_register_regmap - register a clk_regmap clock divider
>> + *
>> + * @dev: device that is registering this clock
>> + * @name: name of this clock
>> + * @parent_name: name of clock's parent
>> + * @flags: framework-specific flags
>> + * @clkdiv: clock divider to operate on
>> + *
>> + * Clocks that use regmap for their register I/O should register their
>> + * clk_regmap struct via this function so that the regmap is initialized
>> + * and so that the clock is registered with the common clock framework.
>> + */
>> +struct clk *devm_clkdiv_register_regmap(struct device *dev, const char *name,
>> +					const char *parent_name,
>> +					unsigned long flags,
>> +					struct clkdiv_regmap *clkdiv)
>> +{
>> +	struct clk_init_data init;
>> +
>> +	if (!clkdiv)
>> +		return ERR_PTR(-ENODEV);
> 
> Looks like a useless check. Just blow up instead so we don't have to
> tolerate bad code.
> 

Ok, sure!

>> +
>> +	clkdiv->rclk.regmap = dev_get_regmap(dev->parent, NULL);
>> +
>> +	if (!clkdiv->rclk.regmap)
>> +		return ERR_PTR(-ENXIO);
>> +
>> +	init.name = name;
>> +	init.parent_names = (parent_name ? &parent_name : NULL);
>> +	init.num_parents = (parent_name ? 1 : 0);
>> +	init.flags = flags;
>> +	init.ops = &clkdiv_ops;
>> +
>> +	clkdiv->rclk.hw.init = &init;
>> +
>> +	return devm_clk_register(dev, &clkdiv->rclk.hw);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_clkdiv_register_regmap);
>> diff --git a/drivers/clk/qcom/clk-regmap.h b/drivers/clk/qcom/clk-regmap.h
>> index 491a63d..02582cf 100644
>> --- a/drivers/clk/qcom/clk-regmap.h
>> +++ b/drivers/clk/qcom/clk-regmap.h
>> @@ -42,4 +42,28 @@ void clk_disable_regmap(struct clk_hw *hw);
>>  struct clk *
>>  devm_clk_register_regmap(struct device *dev, struct clk_regmap *rclk);
>>  
>> +/**
>> + * struct clkdiv_regmap - regmap supporting clock divider
>> + * @rclk:   regmap supporting clock struct
>> + * @reg:    offset into regmap for the control register
>> + * @shift:  bit position for divider value
>> + * @width:  number of bits used for divider value
>> + * @table:  pointer to table containing an array of divider/value pairs
>> + */
>> +struct clkdiv_regmap {
>> +	struct clk_regmap rclk;
>> +	unsigned int reg;
>> +	unsigned int shift;
>> +	unsigned int width;
>> +	struct clk_div_table *table;
> 
> Is this used?
> 

Yes, its passed from the driver that will be registered by devm_clkdiv_register_regmap().
Its just a divider/value pairs table. Will be used in set_rate() and round_rate().

Thank you for the comments!

BR,
Georgi

Re: [PATCH v1] clk: qcom: Add support for regmap clock dividers

[Stephen Boyd]

On 10/03/14 08:13, Georgi Djakov wrote:
> On 10/02/2014 09:11 PM, Stephen Boyd wrote:
>> On 09/30/14 10:20, Georgi Djakov wrote:
>> +					unsigned long parent_rate)
>> +{
>> +	struct clk_regmap *rclk = to_clk_regmap(hw);
>> +	struct clkdiv_regmap *clkdiv = to_clkdiv_regmap(rclk);
>> +	unsigned int div, val;
>> +
>> +	regmap_read(rclk->regmap, clkdiv->reg, &val);
>> +	if (!val)
>> +		return parent_rate;
>> +
>> +	div = (val >> clkdiv->shift) & ((1 << clkdiv->width)-1);
>> +
>> +	return parent_rate / div;
>> I don't know if you saw the patch to split out the clk-divider.c logic
>> from the readl/writel patch I sent[1]? That could make this slightly
>> smaller.
>> tabl
> Could you please provide a link to that patch?

Doh, here it is: https://lkml.org/lkml/2014/9/5/762


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