[PATCHv4 11/15] clk: ti: clockdomain: add clock provider support to clockdomains

Tero Kristo t-kristo at ti.com
Fri Oct 28 00:41:54 PDT 2016


On 28/10/16 03:50, Stephen Boyd wrote:
> On 10/18, Tero Kristo wrote:
>> Clockdomains can now be used as clock providers in the system. This
>> patch initializes the provider data during init, and parses the clocks
>> while they are being registered. An xlate function for the provider
>> is also given.
>>
>> Signed-off-by: Tero Kristo <t-kristo at ti.com>
>
> Please Cc dt reviewers on binding updates.

Sorry about missing that...

 > I suppose a PRCM is
> like an MFD that has clocks and resets under it? On other
> platforms we've combined that all into one node and just had
> #clock-cells and #reset-cells in that node. Is there any reason
> we can't do that here?

For OMAPs, there are typically multiple instances of the PRCM around; 
OMAP4 for example has:

cm1 @ 0x4a004000 (clocks + clockdomains)
cm2 @ 0x4a008000 (clocks + clockdomains)
prm @ 0x4a306000 (few clocks + resets + power state handling)
scrm @ 0x4a30a000 (few external clocks + plenty of misc stuff)

These instances are also under different power/voltage domains which 
means their PM behavior is different.

The idea behind having a clockdomain as a provider was mostly to have 
the topology visible : prcm-instance -> clockdomain -> clocks

... but basically I think it would be possible to drop the clockdomain 
representation and just mark the prcm-instance as a clock provider. 
Tony, any thoughts on that?


>
>> ---
>>  .../devicetree/bindings/arm/omap/prcm.txt          |  13 ++
>>  .../devicetree/bindings/clock/ti/clockdomain.txt   |  12 +-
>>  arch/arm/mach-omap2/io.c                           |   2 +
>>  drivers/clk/ti/clock.h                             |   1 +
>>  drivers/clk/ti/clockdomain.c                       | 147 +++++++++++++++++++++
>>  include/linux/clk/ti.h                             |   3 +
>>  6 files changed, 177 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/omap/prcm.txt b/Documentation/devicetree/bindings/arm/omap/prcm.txt
>> index 3eb6d7a..301f576 100644
>> --- a/Documentation/devicetree/bindings/arm/omap/prcm.txt
>> +++ b/Documentation/devicetree/bindings/arm/omap/prcm.txt
>> @@ -47,6 +47,19 @@ cm: cm at 48004000 {
>>  	};
>>  }
>>
>> +cm2: cm2 at 8000 {
>> +	compatible = "ti,omap4-cm2";
>> +	reg = <0x8000 0x3000>;
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +	ranges = <0 0x8000 0x3000>;
>> +
>> +	l4_per_clkdm: l4_per_clkdm {
>> +		compatible = "ti,clockdomain";
>> +		reg = <0x1400 0x200>;
>
> Should there be #clock-cells = <1> here? Also node name should
> have an @1400 after it?

Yeah both should be there. I had the #clock-cells in my test data in 
place already but the address portion I seem to have completely forgot.

>
>> +	};
>> +};
>> +
>>  &cm_clocks {
>>  	omap2_32k_fck: omap_32k_fck {
>>  		#clock-cells = <0>;
>> diff --git a/Documentation/devicetree/bindings/clock/ti/clockdomain.txt b/Documentation/devicetree/bindings/clock/ti/clockdomain.txt
>> index cb76b3f..5d8ca61 100644
>> --- a/Documentation/devicetree/bindings/clock/ti/clockdomain.txt
>> +++ b/Documentation/devicetree/bindings/clock/ti/clockdomain.txt
>> @@ -14,11 +14,21 @@ hardware hierarchy.
>>
>>  Required properties:
>>  - compatible : shall be "ti,clockdomain"
>> -- #clock-cells : from common clock binding; shall be set to 0.
>> +- #clock-cells : from common clock binding; shall be set to 1 if this
>> +		 clockdomain acts as a clock provider.
>> +
>> +Optional properties:
>>  - clocks : link phandles of clocks within this domain
>> +- reg : address for the clockdomain
>>
>>  Examples:
>>  	dss_clkdm: dss_clkdm {
>>  		compatible = "ti,clockdomain";
>>  		clocks = <&dss1_alwon_fck_3430es2>, <&dss_ick_3430es2>;
>>  	};
>> +
>> +	l4_per_clkdm: l4_per_clkdm {
>
> add an @1400?

Yea will do, unless we decide to go for prcm-instance provider approach.

>
>> +		compatible = "ti,clockdomain";
>> +		#clock-cells = <1>;
>> +		reg = <0x1400 0x200>;
>> +	};
>> diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
>> index 0e9acdd..c1a5cfb 100644
>> --- a/arch/arm/mach-omap2/io.c
>> +++ b/arch/arm/mach-omap2/io.c
>> @@ -794,6 +794,8 @@ int __init omap_clk_init(void)
>>  		if (ret)
>>  			return ret;
>>
>> +		ti_dt_clockdomains_early_setup();
>> +
>>  		of_clk_init(NULL);
>>
>>  		ti_dt_clk_init_retry_clks();
>> diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h
>> index 9b8a5f2..f6383ab 100644
>> --- a/drivers/clk/ti/clock.h
>> +++ b/drivers/clk/ti/clock.h
>> @@ -205,6 +205,7 @@ struct clk *ti_clk_register(struct device *dev, struct clk_hw *hw,
>>
>>  int ti_clk_get_memmap_index(struct device_node *node);
>>  void __iomem *ti_clk_get_reg_addr(struct device_node *node, int index);
>> +void __iomem *ti_clk_get_reg_addr_clkdm(const char *clkdm_name, u16 offset);
>>  void ti_dt_clocks_register(struct ti_dt_clk *oclks);
>>  int ti_clk_retry_init(struct device_node *node, struct clk_hw *hw,
>>  		      ti_of_clk_init_cb_t func);
>> diff --git a/drivers/clk/ti/clockdomain.c b/drivers/clk/ti/clockdomain.c
>> index 704157d..7b0a6c3 100644
>> --- a/drivers/clk/ti/clockdomain.c
>> +++ b/drivers/clk/ti/clockdomain.c
>> @@ -28,6 +28,23 @@
>>  #define pr_fmt(fmt) "%s: " fmt, __func__
>>
>>  /**
>> + * struct ti_clkdm - TI clockdomain data structure
>> + * @name: name of the clockdomain
>> + * @index: index of the clk_iomap struct for this clkdm
>> + * @offset: clockdomain offset from the beginning of the iomap
>> + * @link: link to the list
>> + */
>> +struct ti_clkdm {
>> +	const char *name;
>> +	int index;
>> +	u32 offset;
>> +	struct list_head link;
>> +	struct list_head clocks;
>> +};
>> +
>> +static LIST_HEAD(clkdms);
>> +
>> +/**
>>   * omap2_clkops_enable_clkdm - increment usecount on clkdm of @hw
>>   * @hw: struct clk_hw * of the clock being enabled
>>   *
>> @@ -116,6 +133,8 @@ void omap2_init_clk_clkdm(struct clk_hw *hw)
>>  	struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>>  	struct clockdomain *clkdm;
>>  	const char *clk_name;
>> +	struct ti_clkdm *ti_clkdm;
>> +	bool match = false;
>>
>>  	if (!clk->clkdm_name)
>>  		return;
>> @@ -130,7 +149,21 @@ void omap2_init_clk_clkdm(struct clk_hw *hw)
>>  	} else {
>>  		pr_debug("clock: could not associate clk %s to clkdm %s\n",
>>  			 clk_name, clk->clkdm_name);
>> +		return;
>>  	}
>> +
>> +	list_for_each_entry(ti_clkdm, &clkdms, link) {
>> +		if (!strcmp(ti_clkdm->name, clk->clkdm_name)) {
>> +			match = true;
>> +			break;
>
> Or just goto found and then drop the match bool thing.

That will be cleaner yes. Will change.

>
>> +		}
>> +	}
>> +
>> +	if (!match)
>> +		return;
>> +
>> +	/* Add clock to the list of provided clocks */
>> +	list_add(&clk->clkdm_link, &ti_clkdm->clocks);
>>  }
>>
>>  static void __init of_ti_clockdomain_setup(struct device_node *node)
>> @@ -161,11 +194,125 @@ static void __init of_ti_clockdomain_setup(struct device_node *node)
>>  	}
>>  }
>>
>> +static struct clk_hw *clkdm_clk_xlate(struct of_phandle_args *clkspec,
>> +				      void *data)
>> +{
>> +	struct ti_clkdm *clkdm = data;
>> +	struct clk_hw_omap *clk;
>> +	u16 offset = clkspec->args[0];
>> +
>> +	list_for_each_entry(clk, &clkdm->clocks, clkdm_link)
>> +		if (((u32)clk->enable_reg & 0xffff) - clkdm->offset == offset)
>
> This looks scary.

I think I need to add a separate cleanup patch for the address handling 
before this series... There are a few nasty looking address casts around 
at the moment under the TI clock driver.

>
>> +			return &clk->hw;
>> +
>> +	return ERR_PTR(-EINVAL);
>> +}
>> +
>> +static int ti_clk_register_clkdm(struct device_node *node)
>> +{
>> +	u64 clkdm_addr;
>> +	u64 inst_addr;
>> +	const __be32 *reg;
>> +	u32 offset;
>> +	int idx;
>> +	struct ti_clkdm *clkdm;
>> +	int ret;
>> +
>> +	reg = of_get_address(node, 0, NULL, NULL);
>> +	if (!reg)
>> +		return -ENOENT;
>> +
>> +	clkdm_addr = of_translate_address(node, reg);
>> +
>> +	reg = of_get_address(node->parent, 0, NULL, NULL);
>> +	if (!reg)
>> +		return -EINVAL;
>> +
>> +	inst_addr = of_translate_address(node->parent, reg);
>> +
>> +	offset = clkdm_addr - inst_addr;
>> +
>
> I guess the usual offset tricks are still going on in the TI clk
> driver? Is there a plan to stop doing that at some point?

I can have a look at that with the addressing cleanup I mentioned above. 
I'll see if I can reduce the amount of trickery involved.

>
>> +	idx = ti_clk_get_memmap_index(node->parent);
>> +
>> +	if (idx < 0) {
>> +		pr_err("bad memmap index for %s\n", node->name);
>> +		return idx;
>> +	}
>> +
>> +	clkdm = kzalloc(sizeof(*clkdm), GFP_KERNEL);
>> +	if (!clkdm)
>> +		return -ENOMEM;
>> +
>> +	clkdm->name = node->name;
>> +	clkdm->index = idx;
>> +	clkdm->offset = offset;
>> +
>> +	INIT_LIST_HEAD(&clkdm->clocks);
>> +
>> +	list_add(&clkdm->link, &clkdms);
>> +
>> +	ret = of_clk_add_hw_provider(node, clkdm_clk_xlate, clkdm);
>> +	if (ret) {
>> +		list_del(&clkdm->link);
>> +		kfree(clkdm);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ti_clk_get_reg_addr_clkdm - get register address relative to clockdomain
>> + * @clkdm_name: parent clockdomain
>> + * @offset: offset from the clockdomain
>> + *
>> + * Gets a register address relative to parent clockdomain. Searches the
>> + * list of available clockdomain, and if match is found, calculates the
>> + * register address from the iomap relative to the clockdomain.
>> + * Returns the register address, or NULL if not found.
>> + */
>> +void __iomem *ti_clk_get_reg_addr_clkdm(const char *clkdm_name, u16 offset)
>> +{
>> +	u32 reg;
>> +	struct clk_omap_reg *reg_setup;
>> +	struct ti_clkdm *clkdm;
>> +	bool match = false;
>> +
>> +	reg_setup = (struct clk_omap_reg *)®
>> +
>> +	/* XXX: get offset from clkdm, get base for instance */
>> +	list_for_each_entry(clkdm, &clkdms, link) {
>> +		if (!strcmp(clkdm->name, clkdm_name)) {
>> +			match = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!match) {
>> +		pr_err("%s: no entry for %s\n", __func__, clkdm_name);
>> +		return NULL;
>> +	}
>> +
>> +	reg_setup->offset = clkdm->offset + offset;
>> +	reg_setup->index = clkdm->index;
>> +
>> +	return (void __iomem *)reg;
>> +}
>> +
>>  static const struct of_device_id ti_clkdm_match_table[] __initconst = {
>>  	{ .compatible = "ti,clockdomain" },
>>  	{ }
>>  };
>>
>> +void __init ti_dt_clockdomains_early_setup(void)
>> +{
>> +	struct device_node *np;
>> +
>> +	for_each_matching_node(np, ti_clkdm_match_table) {
>> +		ti_clk_register_clkdm(np);
>> +	}
>
> Nitpick: drop braces please.

True, will do that.

-Tero

>
>> +}
>> +
>>  /**
>>   * ti_dt_clockdomains_setup - setup device tree clockdomains
>>   *
>> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
>> index 626ae94..afccb48 100644
>> --- a/include/linux/clk/ti.h
>> +++ b/include/linux/clk/ti.h
>> @@ -125,6 +125,7 @@ struct clk_hw_omap_ops {
>>  /**
>>   * struct clk_hw_omap - OMAP struct clk
>>   * @node: list_head connecting this clock into the full clock list
>> + * @clkdm_link: list_head connecting this clock into the clockdomain
>>   * @enable_reg: register to write to enable the clock (see @enable_bit)
>>   * @enable_bit: bitshift to write to enable/disable the clock (see @enable_reg)
>>   * @flags: see "struct clk.flags possibilities" above
>> @@ -137,6 +138,7 @@ struct clk_hw_omap_ops {
>>  struct clk_hw_omap {
>>  	struct clk_hw		hw;
>>  	struct list_head	node;
>> +	struct list_head	clkdm_link;
>>  	unsigned long		fixed_rate;
>>  	u8			fixed_div;
>>  	void __iomem		*enable_reg;
>> @@ -251,6 +253,7 @@ int omap2_reprogram_dpllcore(struct clk_hw *clk, unsigned long rate,
>>  unsigned long omap2_get_dpll_rate(struct clk_hw_omap *clk);
>>
>>  void ti_dt_clk_init_retry_clks(void);
>> +void ti_dt_clockdomains_early_setup(void);
>>  void ti_dt_clockdomains_setup(void);
>>  int ti_clk_setup_ll_ops(struct ti_clk_ll_ops *ops);
>>
>> --
>> 1.9.1
>>
>




More information about the linux-arm-kernel mailing list