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

Commit Message

Tero Kristo Oct. 18, 2016, 3:46 p.m. UTC

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 <[e-mail]>
---
 .../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(-)

Comments

Tony Lindgren Oct. 20, 2016, 1:06 p.m. UTC | #1

* Tero Kristo <[e-mail]> [161018 08:47]:
> 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.

Good to see this happen:

Acked-by: Tony Lindgren <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd Oct. 28, 2016, 12:50 a.m. UTC | #2

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 <[e-mail]>

Please Cc dt reviewers on binding updates. 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?

> ---
>  .../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@48004000 {
>  	};
>  }
>  
> +cm2: cm2@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?

> +	};
> +};
> +
>  &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?

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

> +		}
> +	}
> +
> +	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.

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

> +	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 *)&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.

> +}
> +
>  /**
>   * 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
>

Tero Kristo Oct. 28, 2016, 7:41 a.m. UTC | #3

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 <[e-mail]>
>
> 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@48004000 {
>>  	};
>>  }
>>
>> +cm2: cm2@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 *)&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
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren Oct. 28, 2016, 12:51 p.m. UTC | #4

* Tero Kristo <[e-mail]> [161028 00:43]:
> On 28/10/16 03:50, Stephen Boyd wrote:
> > 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

Yeah that's needed to get the interconnect hierarchy right for
genpd :)

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

No let's not drop the clockdomains as those will be needed when we
move things into proper hierarchy within the interconnect instances.
This will then help with getting things right with genpd.

In the long run we just want to specify clockdomain and the offset of
the clock instance within the clockdomain in the dts files.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd Oct. 28, 2016, 11:36 p.m. UTC | #5

On 10/28, Tony Lindgren wrote:
> * Tero Kristo <[e-mail]> [161028 00:43]:
> > On 28/10/16 03:50, Stephen Boyd wrote:
> > > 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
> 
> Yeah that's needed to get the interconnect hierarchy right for
> genpd :)
> 
> > ... 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?
> 
> No let's not drop the clockdomains as those will be needed when we
> move things into proper hierarchy within the interconnect instances.
> This will then help with getting things right with genpd.
> 
> In the long run we just want to specify clockdomain and the offset of
> the clock instance within the clockdomain in the dts files.
> 

Sorry, I have very little idea how OMAP hardware works. Do you
mean that you will have different nodes for each clockdomain so
that genpd can map 1:1 to the node in dts? But in hardware
there's a prcm that allows us to control many clock domains
through register read/writes? How is the interconnect involved?

Tony Lindgren Oct. 28, 2016, 11:54 p.m. UTC | #6

* Stephen Boyd <[e-mail]> [161028 16:37]:
> On 10/28, Tony Lindgren wrote:
> > * Tero Kristo <[e-mail]> [161028 00:43]:
> > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > 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
> > 
> > Yeah that's needed to get the interconnect hierarchy right for
> > genpd :)
> > 
> > > ... 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?
> > 
> > No let's not drop the clockdomains as those will be needed when we
> > move things into proper hierarchy within the interconnect instances.
> > This will then help with getting things right with genpd.
> > 
> > In the long run we just want to specify clockdomain and the offset of
> > the clock instance within the clockdomain in the dts files.
> > 
> 
> Sorry, I have very little idea how OMAP hardware works. Do you
> mean that you will have different nodes for each clockdomain so
> that genpd can map 1:1 to the node in dts? But in hardware
> there's a prcm that allows us to control many clock domains
> through register read/writes? How is the interconnect involved?

There are multiple clockdomains, at least one for each interconnect
instance. Once a clockdomain is idle, the related interconnect can
idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.

There's more info in for example omap4 TRM section "3.4.1 Device
Power-Management Layout" that shows the voltage/power/clock domains.
The interconnect instances are mostly named there too looking at
the L4/L3 naming.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Turquette Dec. 2, 2016, 10:33 p.m. UTC | #7

Quoting Tony Lindgren (2016-10-28 16:54:48)
> * Stephen Boyd <[e-mail]> [161028 16:37]:
> > On 10/28, Tony Lindgren wrote:
> > > * Tero Kristo <[e-mail]> [161028 00:43]:
> > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > 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
> > > 
> > > Yeah that's needed to get the interconnect hierarchy right for
> > > genpd :)
> > > 
> > > > ... 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?
> > > 
> > > No let's not drop the clockdomains as those will be needed when we
> > > move things into proper hierarchy within the interconnect instances.
> > > This will then help with getting things right with genpd.
> > > 
> > > In the long run we just want to specify clockdomain and the offset of
> > > the clock instance within the clockdomain in the dts files.
> > > 
> > 
> > Sorry, I have very little idea how OMAP hardware works. Do you
> > mean that you will have different nodes for each clockdomain so
> > that genpd can map 1:1 to the node in dts? But in hardware
> > there's a prcm that allows us to control many clock domains
> > through register read/writes? How is the interconnect involved?
> 
> There are multiple clockdomains, at least one for each interconnect
> instance. Once a clockdomain is idle, the related interconnect can
> idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
> 
> There's more info in for example omap4 TRM section "3.4.1 Device
> Power-Management Layout" that shows the voltage/power/clock domains.
> The interconnect instances are mostly named there too looking at
> the L4/L3 naming.

I'm confused on two points:

1) why are the clkdm's acting as clock providers? I've always hated the
name "clock domain" since those bits are for managing module state, not
clock state. The PRM, CM1 and CM2 provide the clocks, not the
clockdomains.

2) why aren't the clock domains modeled as genpds with their associated
devices attached to them? Note that it is possible to "nest" genpd
objects. This would also allow for the "Clockdomain Dependency"
relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
Dependency in the OMAP4 TRM).

Regards,
Mike

> 
> Regards,
> 
> Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren Dec. 2, 2016, 11:12 p.m. UTC | #8

* Michael Turquette <[e-mail]> [161202 14:34]:
> Quoting Tony Lindgren (2016-10-28 16:54:48)
> > * Stephen Boyd <[e-mail]> [161028 16:37]:
> > > On 10/28, Tony Lindgren wrote:
> > > > * Tero Kristo <[e-mail]> [161028 00:43]:
> > > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > > 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
> > > > 
> > > > Yeah that's needed to get the interconnect hierarchy right for
> > > > genpd :)
> > > > 
> > > > > ... 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?
> > > > 
> > > > No let's not drop the clockdomains as those will be needed when we
> > > > move things into proper hierarchy within the interconnect instances.
> > > > This will then help with getting things right with genpd.
> > > > 
> > > > In the long run we just want to specify clockdomain and the offset of
> > > > the clock instance within the clockdomain in the dts files.
> > > > 
> > > 
> > > Sorry, I have very little idea how OMAP hardware works. Do you
> > > mean that you will have different nodes for each clockdomain so
> > > that genpd can map 1:1 to the node in dts? But in hardware
> > > there's a prcm that allows us to control many clock domains
> > > through register read/writes? How is the interconnect involved?
> > 
> > There are multiple clockdomains, at least one for each interconnect
> > instance. Once a clockdomain is idle, the related interconnect can
> > idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
> > 
> > There's more info in for example omap4 TRM section "3.4.1 Device
> > Power-Management Layout" that shows the voltage/power/clock domains.
> > The interconnect instances are mostly named there too looking at
> > the L4/L3 naming.
> 
> I'm confused on two points:
> 
> 1) why are the clkdm's acting as clock providers? I've always hated the
> name "clock domain" since those bits are for managing module state, not
> clock state. The PRM, CM1 and CM2 provide the clocks, not the
> clockdomains.

The clock domains have multiple clock inputs that are routed to multiple
child clocks. So it is a clock :)

See for example omap4430 TRM "3.6.4 CD_WKUP Clock Domain" on page
393 in my revision here.

On that page "Figure 3-48" shows CD_WKUP with the four input clocks.
And then "Table 3-84. CD_WKUP Control and Status Parameters" shows
the CD_WKUP clock domain specific registers. These registers show
the status, I think they are all read-only registers. Then CD_WKUP
has multiple child clocks with configurable registers.

From hardware register point of view, each clock domain has:

- Read-only clockdomain status registers in the beginning of
  the address space

- Multiple similar clock instances register instances each
  mapping to a specific interconnect target module

These are documented in "3.11.16.1 WKUP_CM Register Summary".

From hardware point of view, we ideally want to map interconnect
target modules to the clock instance offset from the clock domain
for that interconnect segment. For example gptimer1 clocks would
be just:

clocks = <&cd_wkup 0x40>;

> 2) why aren't the clock domains modeled as genpds with their associated
> devices attached to them? Note that it is possible to "nest" genpd
> objects. This would also allow for the "Clockdomain Dependency"
> relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
> Dependency in the OMAP4 TRM).

Clock domains only route clocks to child clocks. Power domains
are different registers. The power domains map roughly to
interconnect instances, there we have registers to disable the
whole interconnect when idle.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren Dec. 3, 2016, 12:18 a.m. UTC | #9

* Michael Turquette <[e-mail]> [161202 15:52]:
> Quoting Tony Lindgren (2016-12-02 15:12:40)
> > * Michael Turquette <[e-mail]> [161202 14:34]:
> > > Quoting Tony Lindgren (2016-10-28 16:54:48)
> > > > * Stephen Boyd <[e-mail]> [161028 16:37]:
> > > > > On 10/28, Tony Lindgren wrote:
> > > > > > * Tero Kristo <[e-mail]> [161028 00:43]:
> > > > > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > > > > 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
> > > > > > 
> > > > > > Yeah that's needed to get the interconnect hierarchy right for
> > > > > > genpd :)
> > > > > > 
> > > > > > > ... 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?
> > > > > > 
> > > > > > No let's not drop the clockdomains as those will be needed when we
> > > > > > move things into proper hierarchy within the interconnect instances.
> > > > > > This will then help with getting things right with genpd.
> > > > > > 
> > > > > > In the long run we just want to specify clockdomain and the offset of
> > > > > > the clock instance within the clockdomain in the dts files.
> > > > > > 
> > > > > 
> > > > > Sorry, I have very little idea how OMAP hardware works. Do you
> > > > > mean that you will have different nodes for each clockdomain so
> > > > > that genpd can map 1:1 to the node in dts? But in hardware
> > > > > there's a prcm that allows us to control many clock domains
> > > > > through register read/writes? How is the interconnect involved?
> > > > 
> > > > There are multiple clockdomains, at least one for each interconnect
> > > > instance. Once a clockdomain is idle, the related interconnect can
> > > > idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
> > > > 
> > > > There's more info in for example omap4 TRM section "3.4.1 Device
> > > > Power-Management Layout" that shows the voltage/power/clock domains.
> > > > The interconnect instances are mostly named there too looking at
> > > > the L4/L3 naming.
> > > 
> > > I'm confused on two points:
> > > 
> > > 1) why are the clkdm's acting as clock providers? I've always hated the
> > > name "clock domain" since those bits are for managing module state, not
> > > clock state. The PRM, CM1 and CM2 provide the clocks, not the
> > > clockdomains.
> > 
> > The clock domains have multiple clock inputs that are routed to multiple
> > child clocks. So it is a clock :)
> > 
> > See for example omap4430 TRM "3.6.4 CD_WKUP Clock Domain" on page
> > 393 in my revision here.
> > 
> > On that page "Figure 3-48" shows CD_WKUP with the four input clocks.
> > And then "Table 3-84. CD_WKUP Control and Status Parameters" shows
> > the CD_WKUP clock domain specific registers. These registers show
> > the status, I think they are all read-only registers. Then CD_WKUP
> > has multiple child clocks with configurable registers.
> > 
> > From hardware register point of view, each clock domain has:
> > 
> > - Read-only clockdomain status registers in the beginning of
> >   the address space
> > 
> > - Multiple similar clock instances register instances each
> >   mapping to a specific interconnect target module
> > 
> > These are documented in "3.11.16.1 WKUP_CM Register Summary".
> 
> Oh, this is because you are treating the MODULEMODE bits like gate
> clocks. I never really figured out if this was the best way to model
> those bits since they do more than control a line toggling at a rate.
> For instance this bit will affect the master/slave IDLE protocol between
> the module and the PRCM.

Yes seems like there is some negotiation going on there with the
target module. But from practical point of view the CLKCTRL
register is the gate for a module functional clock.

> > From hardware point of view, we ideally want to map interconnect
> > target modules to the clock instance offset from the clock domain
> > for that interconnect segment. For example gptimer1 clocks would
> > be just:
> > 
> > clocks = <&cd_wkup 0x40>;
> > 
> > > 2) why aren't the clock domains modeled as genpds with their associated
> > > devices attached to them? Note that it is possible to "nest" genpd
> > > objects. This would also allow for the "Clockdomain Dependency"
> > > relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
> > > Dependency in the OMAP4 TRM).
> > 
> > Clock domains only route clocks to child clocks. Power domains
> > are different registers. The power domains map roughly to
> > interconnect instances, there we have registers to disable the
> > whole interconnect when idle.
> 
> I'm not talking about power islands at all, but the genpd object in
> Linux. For instance, if we treat each clock domain like a clock
> provider, how could the functional dependency between clkdm_A and
> clkdm_B be asserted?

To me it seems that some output of a clockdomain is just a input
of another clockdomain? So it's just the usual parent child
relationship once we treat a clockdomain just as a clock. Tero
probably has some input here.

> There is certainly no API for that in the clock framework, but for genpd
> your runtime_pm_get() callback for clkdm_A could call runtime_pm_get
> against clkdm_B, which would satisfy the requirement. See section
> 3.1.1.1.7 Clock Domain Dependency in the OMAP4 TRM, version AB.

To me it seems the API is just clk_get() :) Do you have some
specific example we can use to check? My guess is that the
TRM "Clock Domain Dependency" is just the usual parent child
relationship between clocks that are the clockdomains..

If there is something more magical there certainly that should
be considered though.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tero Kristo Dec. 5, 2016, 10:08 a.m. UTC | #10

On 03/12/16 02:18, Tony Lindgren wrote:
> * Michael Turquette <[e-mail]> [161202 15:52]:
>> Quoting Tony Lindgren (2016-12-02 15:12:40)
>>> * Michael Turquette <[e-mail]> [161202 14:34]:
>>>> Quoting Tony Lindgren (2016-10-28 16:54:48)
>>>>> * Stephen Boyd <[e-mail]> [161028 16:37]:
>>>>>> On 10/28, Tony Lindgren wrote:
>>>>>>> * Tero Kristo <[e-mail]> [161028 00:43]:
>>>>>>>> On 28/10/16 03:50, Stephen Boyd wrote:
>>>>>>>>> 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
>>>>>>>
>>>>>>> Yeah that's needed to get the interconnect hierarchy right for
>>>>>>> genpd :)
>>>>>>>
>>>>>>>> ... 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?
>>>>>>>
>>>>>>> No let's not drop the clockdomains as those will be needed when we
>>>>>>> move things into proper hierarchy within the interconnect instances.
>>>>>>> This will then help with getting things right with genpd.
>>>>>>>
>>>>>>> In the long run we just want to specify clockdomain and the offset of
>>>>>>> the clock instance within the clockdomain in the dts files.
>>>>>>>
>>>>>>
>>>>>> Sorry, I have very little idea how OMAP hardware works. Do you
>>>>>> mean that you will have different nodes for each clockdomain so
>>>>>> that genpd can map 1:1 to the node in dts? But in hardware
>>>>>> there's a prcm that allows us to control many clock domains
>>>>>> through register read/writes? How is the interconnect involved?
>>>>>
>>>>> There are multiple clockdomains, at least one for each interconnect
>>>>> instance. Once a clockdomain is idle, the related interconnect can
>>>>> idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
>>>>>
>>>>> There's more info in for example omap4 TRM section "3.4.1 Device
>>>>> Power-Management Layout" that shows the voltage/power/clock domains.
>>>>> The interconnect instances are mostly named there too looking at
>>>>> the L4/L3 naming.
>>>>
>>>> I'm confused on two points:
>>>>
>>>> 1) why are the clkdm's acting as clock providers? I've always hated the
>>>> name "clock domain" since those bits are for managing module state, not
>>>> clock state. The PRM, CM1 and CM2 provide the clocks, not the
>>>> clockdomains.
>>>
>>> The clock domains have multiple clock inputs that are routed to multiple
>>> child clocks. So it is a clock :)
>>>
>>> See for example omap4430 TRM "3.6.4 CD_WKUP Clock Domain" on page
>>> 393 in my revision here.
>>>
>>> On that page "Figure 3-48" shows CD_WKUP with the four input clocks.
>>> And then "Table 3-84. CD_WKUP Control and Status Parameters" shows
>>> the CD_WKUP clock domain specific registers. These registers show
>>> the status, I think they are all read-only registers. Then CD_WKUP
>>> has multiple child clocks with configurable registers.
>>>
>>> From hardware register point of view, each clock domain has:
>>>
>>> - Read-only clockdomain status registers in the beginning of
>>>   the address space
>>>
>>> - Multiple similar clock instances register instances each
>>>   mapping to a specific interconnect target module
>>>
>>> These are documented in "3.11.16.1 WKUP_CM Register Summary".
>>
>> Oh, this is because you are treating the MODULEMODE bits like gate
>> clocks. I never really figured out if this was the best way to model
>> those bits since they do more than control a line toggling at a rate.
>> For instance this bit will affect the master/slave IDLE protocol between
>> the module and the PRCM.
>
> Yes seems like there is some negotiation going on there with the
> target module. But from practical point of view the CLKCTRL
> register is the gate for a module functional clock.

There's some confusion on this, clockdomain is effectively a collection 
of clocks, and can be used to force control that collection if needed. 
Chapter "3.1.1.1.3 Clock Domain" in some OMAP4 TRM shows the 
relationship neatly.

>
>>> From hardware point of view, we ideally want to map interconnect
>>> target modules to the clock instance offset from the clock domain
>>> for that interconnect segment. For example gptimer1 clocks would
>>> be just:
>>>
>>> clocks = <&cd_wkup 0x40>;
>>>
>>>> 2) why aren't the clock domains modeled as genpds with their associated
>>>> devices attached to them? Note that it is possible to "nest" genpd
>>>> objects. This would also allow for the "Clockdomain Dependency"
>>>> relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
>>>> Dependency in the OMAP4 TRM).
>>>
>>> Clock domains only route clocks to child clocks. Power domains
>>> are different registers. The power domains map roughly to
>>> interconnect instances, there we have registers to disable the
>>> whole interconnect when idle.
>>
>> I'm not talking about power islands at all, but the genpd object in
>> Linux. For instance, if we treat each clock domain like a clock
>> provider, how could the functional dependency between clkdm_A and
>> clkdm_B be asserted?
>
> To me it seems that some output of a clockdomain is just a input
> of another clockdomain? So it's just the usual parent child
> relationship once we treat a clockdomain just as a clock. Tero
> probably has some input here.

A clockdomain should be modelled as a genpd, that I agree. However, it 
doesn't prevent it from being a clock provider also, or does it?

>> There is certainly no API for that in the clock framework, but for genpd
>> your runtime_pm_get() callback for clkdm_A could call runtime_pm_get
>> against clkdm_B, which would satisfy the requirement. See section
>> 3.1.1.1.7 Clock Domain Dependency in the OMAP4 TRM, version AB.

For static dependencies the apis genpd_add/remove_subdomain could 
probably be used.

> To me it seems the API is just clk_get() :) Do you have some
> specific example we can use to check? My guess is that the
> TRM "Clock Domain Dependency" is just the usual parent child
> relationship between clocks that are the clockdomains..
>
> If there is something more magical there certainly that should
> be considered though.

The hwmods could be transformed to individual genpds also I guess. On DT 
level though, we would still need a clock pointer to the main clock and 
a genpd pointer in addition to that.

Tony, any thoughts on that? Would this break up the plans for the 
interconnect completely?

-Tero
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren Dec. 5, 2016, 3:25 p.m. UTC | #11

* Tero Kristo <[e-mail]> [161205 02:09]:
> On 03/12/16 02:18, Tony Lindgren wrote:
> > * Michael Turquette <[e-mail]> [161202 15:52]:
> > > Quoting Tony Lindgren (2016-12-02 15:12:40)
> > > > * Michael Turquette <[e-mail]> [161202 14:34]:
> > > > > Quoting Tony Lindgren (2016-10-28 16:54:48)
> > > > > > * Stephen Boyd <[e-mail]> [161028 16:37]:
> > > > > > > On 10/28, Tony Lindgren wrote:
> > > > > > > > * Tero Kristo <[e-mail]> [161028 00:43]:
> > > > > > > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > > > > > > 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
> > > > > > > > 
> > > > > > > > Yeah that's needed to get the interconnect hierarchy right for
> > > > > > > > genpd :)
> > > > > > > > 
> > > > > > > > > ... 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?
> > > > > > > > 
> > > > > > > > No let's not drop the clockdomains as those will be needed when we
> > > > > > > > move things into proper hierarchy within the interconnect instances.
> > > > > > > > This will then help with getting things right with genpd.
> > > > > > > > 
> > > > > > > > In the long run we just want to specify clockdomain and the offset of
> > > > > > > > the clock instance within the clockdomain in the dts files.
> > > > > > > > 
> > > > > > > 
> > > > > > > Sorry, I have very little idea how OMAP hardware works. Do you
> > > > > > > mean that you will have different nodes for each clockdomain so
> > > > > > > that genpd can map 1:1 to the node in dts? But in hardware
> > > > > > > there's a prcm that allows us to control many clock domains
> > > > > > > through register read/writes? How is the interconnect involved?
> > > > > > 
> > > > > > There are multiple clockdomains, at least one for each interconnect
> > > > > > instance. Once a clockdomain is idle, the related interconnect can
> > > > > > idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
> > > > > > 
> > > > > > There's more info in for example omap4 TRM section "3.4.1 Device
> > > > > > Power-Management Layout" that shows the voltage/power/clock domains.
> > > > > > The interconnect instances are mostly named there too looking at
> > > > > > the L4/L3 naming.
> > > > > 
> > > > > I'm confused on two points:
> > > > > 
> > > > > 1) why are the clkdm's acting as clock providers? I've always hated the
> > > > > name "clock domain" since those bits are for managing module state, not
> > > > > clock state. The PRM, CM1 and CM2 provide the clocks, not the
> > > > > clockdomains.
> > > > 
> > > > The clock domains have multiple clock inputs that are routed to multiple
> > > > child clocks. So it is a clock :)
> > > > 
> > > > See for example omap4430 TRM "3.6.4 CD_WKUP Clock Domain" on page
> > > > 393 in my revision here.
> > > > 
> > > > On that page "Figure 3-48" shows CD_WKUP with the four input clocks.
> > > > And then "Table 3-84. CD_WKUP Control and Status Parameters" shows
> > > > the CD_WKUP clock domain specific registers. These registers show
> > > > the status, I think they are all read-only registers. Then CD_WKUP
> > > > has multiple child clocks with configurable registers.
> > > > 
> > > > From hardware register point of view, each clock domain has:
> > > > 
> > > > - Read-only clockdomain status registers in the beginning of
> > > >   the address space
> > > > 
> > > > - Multiple similar clock instances register instances each
> > > >   mapping to a specific interconnect target module
> > > > 
> > > > These are documented in "3.11.16.1 WKUP_CM Register Summary".
> > > 
> > > Oh, this is because you are treating the MODULEMODE bits like gate
> > > clocks. I never really figured out if this was the best way to model
> > > those bits since they do more than control a line toggling at a rate.
> > > For instance this bit will affect the master/slave IDLE protocol between
> > > the module and the PRCM.
> > 
> > Yes seems like there is some negotiation going on there with the
> > target module. But from practical point of view the CLKCTRL
> > register is the gate for a module functional clock.
> 
> There's some confusion on this, clockdomain is effectively a collection of
> clocks, and can be used to force control that collection if needed. Chapter
> "3.1.1.1.3 Clock Domain" in some OMAP4 TRM shows the relationship neatly.

Yeah that's my understanding too.

> > > > From hardware point of view, we ideally want to map interconnect
> > > > target modules to the clock instance offset from the clock domain
> > > > for that interconnect segment. For example gptimer1 clocks would
> > > > be just:
> > > > 
> > > > clocks = <&cd_wkup 0x40>;
> > > > 
> > > > > 2) why aren't the clock domains modeled as genpds with their associated
> > > > > devices attached to them? Note that it is possible to "nest" genpd
> > > > > objects. This would also allow for the "Clockdomain Dependency"
> > > > > relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
> > > > > Dependency in the OMAP4 TRM).
> > > > 
> > > > Clock domains only route clocks to child clocks. Power domains
> > > > are different registers. The power domains map roughly to
> > > > interconnect instances, there we have registers to disable the
> > > > whole interconnect when idle.
> > > 
> > > I'm not talking about power islands at all, but the genpd object in
> > > Linux. For instance, if we treat each clock domain like a clock
> > > provider, how could the functional dependency between clkdm_A and
> > > clkdm_B be asserted?
> > 
> > To me it seems that some output of a clockdomain is just a input
> > of another clockdomain? So it's just the usual parent child
> > relationship once we treat a clockdomain just as a clock. Tero
> > probably has some input here.
> 
> A clockdomain should be modelled as a genpd, that I agree. However, it
> doesn't prevent it from being a clock provider also, or does it?
> 
> > > There is certainly no API for that in the clock framework, but for genpd
> > > your runtime_pm_get() callback for clkdm_A could call runtime_pm_get
> > > against clkdm_B, which would satisfy the requirement. See section
> > > 3.1.1.1.7 Clock Domain Dependency in the OMAP4 TRM, version AB.
> 
> For static dependencies the apis genpd_add/remove_subdomain could probably
> be used.
> 
> > To me it seems the API is just clk_get() :) Do you have some
> > specific example we can use to check? My guess is that the
> > TRM "Clock Domain Dependency" is just the usual parent child
> > relationship between clocks that are the clockdomains..
> > 
> > If there is something more magical there certainly that should
> > be considered though.
> 
> The hwmods could be transformed to individual genpds also I guess. On DT
> level though, we would still need a clock pointer to the main clock and a
> genpd pointer in addition to that.

Hmm a genpd pointer to where exactly? AFAIK each interconnect
instance should be a genpd provider, and the individual interconnect
target modules should be consumers for that genpd.

> Tony, any thoughts on that? Would this break up the plans for the
> interconnect completely?

Does using genpd for clockdomains cause issues for using genpd for
interconnect instances and the target modules?

The thing I'd be worried about there is that the clockdomains and
their child clocks are just devices sitting on the interconnect,
so we could easily end up with genpd modeling something that does
not represent the hardware.

For example, on 4430 we have:

l4_cfg interconnect
       ...
       segment@0
		...
		target_module@4000
			cm1: cm1@0
			     ...
		...
		target_module@8000
			cm2: cm2@0
		...


l4_wkup interonnect
	...
	segment@0
		...
		target_module@6000
			prm: prm@0
		...
		target_module@a000
			scrm: scrm@0
		...

So what do you guys have in mind for using genpd in the above
example for the clockdomains?

To me it seems that the interconnect instances like l4_cfg and
l4_wkup above should be genpd providers. I don't at least yet
follow what we need to do with the clockdomains with genpd :)

Wouldn't just doing clk_get() from one clockdomain clock to
another clockdomain clock (or it's output) be enough to
represent the clockdomain dependencies?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Turquette Dec. 9, 2016, 8:02 p.m. UTC | #12

Quoting Tony Lindgren (2016-12-05 07:25:34)
> * Tero Kristo <[e-mail]> [161205 02:09]:
> > On 03/12/16 02:18, Tony Lindgren wrote:
> > > * Michael Turquette <[e-mail]> [161202 15:52]:
> > > > Quoting Tony Lindgren (2016-12-02 15:12:40)
> > > > > * Michael Turquette <[e-mail]> [161202 14:34]:
> > > > > > Quoting Tony Lindgren (2016-10-28 16:54:48)
> > > > > > > * Stephen Boyd <[e-mail]> [161028 16:37]:
> > > > > > > > On 10/28, Tony Lindgren wrote:
> > > > > > > > > * Tero Kristo <[e-mail]> [161028 00:43]:
> > > > > > > > > > On 28/10/16 03:50, Stephen Boyd wrote:
> > > > > > > > > > > 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
> > > > > > > > > 
> > > > > > > > > Yeah that's needed to get the interconnect hierarchy right for
> > > > > > > > > genpd :)
> > > > > > > > > 
> > > > > > > > > > ... 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?
> > > > > > > > > 
> > > > > > > > > No let's not drop the clockdomains as those will be needed when we
> > > > > > > > > move things into proper hierarchy within the interconnect instances.
> > > > > > > > > This will then help with getting things right with genpd.
> > > > > > > > > 
> > > > > > > > > In the long run we just want to specify clockdomain and the offset of
> > > > > > > > > the clock instance within the clockdomain in the dts files.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Sorry, I have very little idea how OMAP hardware works. Do you
> > > > > > > > mean that you will have different nodes for each clockdomain so
> > > > > > > > that genpd can map 1:1 to the node in dts? But in hardware
> > > > > > > > there's a prcm that allows us to control many clock domains
> > > > > > > > through register read/writes? How is the interconnect involved?
> > > > > > > 
> > > > > > > There are multiple clockdomains, at least one for each interconnect
> > > > > > > instance. Once a clockdomain is idle, the related interconnect can
> > > > > > > idle too. So yeah genpd pretty much maps 1:1 with the clockdomains.
> > > > > > > 
> > > > > > > There's more info in for example omap4 TRM section "3.4.1 Device
> > > > > > > Power-Management Layout" that shows the voltage/power/clock domains.
> > > > > > > The interconnect instances are mostly named there too looking at
> > > > > > > the L4/L3 naming.
> > > > > > 
> > > > > > I'm confused on two points:
> > > > > > 
> > > > > > 1) why are the clkdm's acting as clock providers? I've always hated the
> > > > > > name "clock domain" since those bits are for managing module state, not
> > > > > > clock state. The PRM, CM1 and CM2 provide the clocks, not the
> > > > > > clockdomains.
> > > > > 
> > > > > The clock domains have multiple clock inputs that are routed to multiple
> > > > > child clocks. So it is a clock :)
> > > > > 
> > > > > See for example omap4430 TRM "3.6.4 CD_WKUP Clock Domain" on page
> > > > > 393 in my revision here.
> > > > > 
> > > > > On that page "Figure 3-48" shows CD_WKUP with the four input clocks.
> > > > > And then "Table 3-84. CD_WKUP Control and Status Parameters" shows
> > > > > the CD_WKUP clock domain specific registers. These registers show
> > > > > the status, I think they are all read-only registers. Then CD_WKUP
> > > > > has multiple child clocks with configurable registers.
> > > > > 
> > > > > From hardware register point of view, each clock domain has:
> > > > > 
> > > > > - Read-only clockdomain status registers in the beginning of
> > > > >   the address space
> > > > > 
> > > > > - Multiple similar clock instances register instances each
> > > > >   mapping to a specific interconnect target module
> > > > > 
> > > > > These are documented in "3.11.16.1 WKUP_CM Register Summary".
> > > > 
> > > > Oh, this is because you are treating the MODULEMODE bits like gate
> > > > clocks. I never really figured out if this was the best way to model
> > > > those bits since they do more than control a line toggling at a rate.
> > > > For instance this bit will affect the master/slave IDLE protocol between
> > > > the module and the PRCM.
> > > 
> > > Yes seems like there is some negotiation going on there with the
> > > target module. But from practical point of view the CLKCTRL
> > > register is the gate for a module functional clock.
> > 
> > There's some confusion on this, clockdomain is effectively a collection of
> > clocks, and can be used to force control that collection if needed. Chapter
> > "3.1.1.1.3 Clock Domain" in some OMAP4 TRM shows the relationship neatly.
> 
> Yeah that's my understanding too.

There is no clk api for this type of behavior. We keep clocks enabled so
long as consumers have a positive prepare_count or enable_count. The
concept of force-idle doesn't work very well here, unless any calls to
force the clkdm to idle also use a usecount.

> 
> > > > > From hardware point of view, we ideally want to map interconnect
> > > > > target modules to the clock instance offset from the clock domain
> > > > > for that interconnect segment. For example gptimer1 clocks would
> > > > > be just:
> > > > > 
> > > > > clocks = <&cd_wkup 0x40>;
> > > > > 
> > > > > > 2) why aren't the clock domains modeled as genpds with their associated
> > > > > > devices attached to them? Note that it is possible to "nest" genpd
> > > > > > objects. This would also allow for the "Clockdomain Dependency"
> > > > > > relationships to be properly modeled (see section 3.1.1.1.7 Clock Domain
> > > > > > Dependency in the OMAP4 TRM).
> > > > > 
> > > > > Clock domains only route clocks to child clocks. Power domains
> > > > > are different registers. The power domains map roughly to
> > > > > interconnect instances, there we have registers to disable the
> > > > > whole interconnect when idle.
> > > > 
> > > > I'm not talking about power islands at all, but the genpd object in
> > > > Linux. For instance, if we treat each clock domain like a clock
> > > > provider, how could the functional dependency between clkdm_A and
> > > > clkdm_B be asserted?
> > > 
> > > To me it seems that some output of a clockdomain is just a input
> > > of another clockdomain? So it's just the usual parent child
> > > relationship once we treat a clockdomain just as a clock. Tero
> > > probably has some input here.
> > 
> > A clockdomain should be modelled as a genpd, that I agree. However, it
> > doesn't prevent it from being a clock provider also, or does it?

It does not prevent it, but I don't understand why you would want both.

I had a recent conversation with Kevin Hilman about a related issue
(we were not discussing this thread or this series) and we both agreed
that most drivers don't even need to managed their clocks directly, so
much as they need to manage their on/off resources. Clocks are just one
part of that, and if we can hide that stuff inside of an attached genpd
then it would be better than having the driver manage clocks explicitly.

Obviously some devices such as audio codec or uart will need to manage
clocks directly, but this is mostly the exception, not the rule.

> > 
> > > > There is certainly no API for that in the clock framework, but for genpd
> > > > your runtime_pm_get() callback for clkdm_A could call runtime_pm_get
> > > > against clkdm_B, which would satisfy the requirement. See section
> > > > 3.1.1.1.7 Clock Domain Dependency in the OMAP4 TRM, version AB.
> > 
> > For static dependencies the apis genpd_add/remove_subdomain could probably
> > be used.
> > 
> > > To me it seems the API is just clk_get() :) Do you have some
> > > specific example we can use to check? My guess is that the
> > > TRM "Clock Domain Dependency" is just the usual parent child
> > > relationship between clocks that are the clockdomains..

clk_get() only fetches a pointer to the clk. I guess you mean
clk_prepare_enable() to actually increment the use count?

If we used the clk framework here is that it would look something like
this:

clk_enable(clk_a)
-> .enable(clk_a_hw)
   -> clk_enable(clk_b)

However, clk_a and clk_b do not have a parent-child relationship in the
clock tree. This is purely a functional relationship between IP blocks.
Modeling this sort of thing in the clk framework would be wrong, and
genpd is a much better place to establish these arbitrary relationships.

> > > 
> > > If there is something more magical there certainly that should
> > > be considered though.
> > 
> > The hwmods could be transformed to individual genpds also I guess. On DT
> > level though, we would still need a clock pointer to the main clock and a
> > genpd pointer in addition to that.
> 
> Hmm a genpd pointer to where exactly? AFAIK each interconnect
> instance should be a genpd provider, and the individual interconnect
> target modules should be consumers for that genpd.

I was thinking that the clock domains would be modeled as genpd objects
with the interconnect target modules attached as struct devices.

> 
> > Tony, any thoughts on that? Would this break up the plans for the
> > interconnect completely?
> 
> Does using genpd for clockdomains cause issues for using genpd for
> interconnect instances and the target modules?

Can they be the same object in Linux? If there is a one-to-one mapping
between clock domains and the interconnect port then maybe you can just
model them together.

> 
> The thing I'd be worried about there is that the clockdomains and
> their child clocks are just devices sitting on the interconnect,
> so we could easily end up with genpd modeling something that does
> not represent the hardware.
> 
> For example, on 4430 we have:
> 
> l4_cfg interconnect
>        ...
>        segment@0
>                 ...
>                 target_module@4000
>                         cm1: cm1@0

How about:

l4_cfg interconnect
       ...
       segment@0
                ...
                cm1@4000
                	module: foo_module@0

I don't know much about the segments. Do they map one-to-one with the
clock domains?

>                              ...
>                 ...
>                 target_module@8000
>                         cm2: cm2@0
>                 ...
> 
> 
> l4_wkup interonnect
>         ...
>         segment@0
>                 ...
>                 target_module@6000
>                         prm: prm@0
>                 ...
>                 target_module@a000
>                         scrm: scrm@0
>                 ...
> 
> So what do you guys have in mind for using genpd in the above
> example for the clockdomains?

If my quick-and-dirty DT above makes sense, then the target modules
(e.g. io controller) would not get clocks anymore, but just
pm_runtime_get(). The genpd backing object would call clk_enable/disable
as needed.

If fine grained control of a clock is needed (e.g. for clk_set_rate)
then the driver can still clk_get it. Whether or not the clockdomain
provides that clock or if it comes from the clock generator (e.g. cm1,
cm2, prm, etc) isn't as important to me, but I prefer for the
clockdomain to not be a clock provider if possible.

> 
> To me it seems that the interconnect instances like l4_cfg and
> l4_wkup above should be genpd providers.

Agreed.

> I don't at least yet
> follow what we need to do with the clockdomains with genpd :)

Use the clockdomain genpd to call clk_enable/disable under the hood.
Don't use them as clock providers to the target modules. Clockdomain
genpds would be the clock consumers.

> 
> Wouldn't just doing clk_get() from one clockdomain clock to
> another clockdomain clock (or it's output) be enough to
> represent the clockdomain dependencies?

s/clk_get/clk_prepare_enable/

Yes, but you're stuffing functional dependencies into the clock tree,
which sucks. genpd was created to model these arbitrary dependencies.

Regards,
Mike

> 
> Regards,
> 
> Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren Dec. 9, 2016, 8:40 p.m. UTC | #13

* Michael Turquette <[e-mail]> [161209 12:02]:
> Quoting Tony Lindgren (2016-12-05 07:25:34)
> > * Tero Kristo <[e-mail]> [161205 02:09]:
...
<snip>

> I had a recent conversation with Kevin Hilman about a related issue
> (we were not discussing this thread or this series) and we both agreed
> that most drivers don't even need to managed their clocks directly, so
> much as they need to manage their on/off resources. Clocks are just one
> part of that, and if we can hide that stuff inside of an attached genpd
> then it would be better than having the driver manage clocks explicitly.
> 
> Obviously some devices such as audio codec or uart will need to manage
> clocks directly, but this is mostly the exception, not the rule.

Yes. And we do that already for clkctrl clocks via PM runtime where
hwmod manages them. Tero's series still has hwmod manage the clocks,
but the clock register handling is moved to live under drivers/clock.

> > > > > There is certainly no API for that in the clock framework, but for genpd
> > > > > your runtime_pm_get() callback for clkdm_A could call runtime_pm_get
> > > > > against clkdm_B, which would satisfy the requirement. See section
> > > > > 3.1.1.1.7 Clock Domain Dependency in the OMAP4 TRM, version AB.
> > > 
> > > For static dependencies the apis genpd_add/remove_subdomain could probably
> > > be used.
> > > 
> > > > To me it seems the API is just clk_get() :) Do you have some
> > > > specific example we can use to check? My guess is that the
> > > > TRM "Clock Domain Dependency" is just the usual parent child
> > > > relationship between clocks that are the clockdomains..
> 
> clk_get() only fetches a pointer to the clk. I guess you mean
> clk_prepare_enable() to actually increment the use count?

Right, with clocks that's all we should need to do :)

> If we used the clk framework here is that it would look something like
> this:
> 
> clk_enable(clk_a)
> -> .enable(clk_a_hw)
>    -> clk_enable(clk_b)
> 
> However, clk_a and clk_b do not have a parent-child relationship in the
> clock tree. This is purely a functional relationship between IP blocks.
> Modeling this sort of thing in the clk framework would be wrong, and
> genpd is a much better place to establish these arbitrary relationships.

Hmm yes, and I don't mean the clock framework should do anything more
complex beyond what it already does.

We just want to represent the clocks as clocks, then have the
interconnect code manage those clocks. That's currently hwmod, eventually
it will be genpd.

> > > > If there is something more magical there certainly that should
> > > > be considered though.
> > > 
> > > The hwmods could be transformed to individual genpds also I guess. On DT
> > > level though, we would still need a clock pointer to the main clock and a
> > > genpd pointer in addition to that.
> > 
> > Hmm a genpd pointer to where exactly? AFAIK each interconnect
> > instance should be a genpd provider, and the individual interconnect
> > target modules should be consumers for that genpd.
> 
> I was thinking that the clock domains would be modeled as genpd objects
> with the interconnect target modules attached as struct devices.

I think clock domains should be just clocks, then we let the interconnect
code and eventually genpd manage them.

> > > Tony, any thoughts on that? Would this break up the plans for the
> > > interconnect completely?
> > 
> > Does using genpd for clockdomains cause issues for using genpd for
> > interconnect instances and the target modules?
> 
> Can they be the same object in Linux? If there is a one-to-one mapping
> between clock domains and the interconnect port then maybe you can just
> model them together.

I'm thinking that it should be the interconnect code implementing
genpd, and use clk_request_enable().

> > The thing I'd be worried about there is that the clockdomains and
> > their child clocks are just devices sitting on the interconnect,
> > so we could easily end up with genpd modeling something that does
> > not represent the hardware.
> > 
> > For example, on 4430 we have:
> > 
> > l4_cfg interconnect
> >        ...
> >        segment@0
> >                 ...
> >                 target_module@4000
> >                         cm1: cm1@0
> 
> How about:
> 
> l4_cfg interconnect
>        ...
>        segment@0
>                 ...
>                 cm1@4000
>                 	module: foo_module@0

That's the wrong way around from hardware point of view. There's
a generic interconnect wrapper module with it's own registers,
then cm1 (and possibly other devices) are children of that target
module.

> I don't know much about the segments. Do they map one-to-one with the
> clock domains?

I need to check, it's been a while, but I recall some interconnects
are partioned to segments based on voltages or clocks.

> If my quick-and-dirty DT above makes sense, then the target modules
> (e.g. io controller) would not get clocks anymore, but just
> pm_runtime_get(). The genpd backing object would call clk_enable/disable
> as needed.

Yeah that's what we already have with hwmod and PM runtime for the
clockctrl register. But hwmod currently directly manages the clkctrl
register, we just want to move that part to be a clock driver.

The children of the interconnect target modules just need to use
PM runtime, but the interconnect target module driver needs to know
it's clkctrl clock.

> If fine grained control of a clock is needed (e.g. for clk_set_rate)
> then the driver can still clk_get it. Whether or not the clockdomain
> provides that clock or if it comes from the clock generator (e.g. cm1,
> cm2, prm, etc) isn't as important to me, but I prefer for the
> clockdomain to not be a clock provider if possible.

Yeah I totally agree with that, and that's already what we mostly
have.

> > I don't at least yet
> > follow what we need to do with the clockdomains with genpd :)
> 
> Use the clockdomain genpd to call clk_enable/disable under the hood.
> Don't use them as clock providers to the target modules. Clockdomain
> genpds would be the clock consumers.

I don't think the clockdomain should be a genpd provider because
that creates a genpd network of dependencies instead of a tree
structure. If we end up setting the clockdomains with genpd, then
only the other clockdomains should use them, but I don't know how
we ever keep drivers from directly tinkering with them..

IMO, the clockdomain clock driver should just provides clocks, then
we can have the interconnect target module driver deal with the
clockdomain dependencies.

> > Wouldn't just doing clk_get() from one clockdomain clock to
> > another clockdomain clock (or it's output) be enough to
> > represent the clockdomain dependencies?
> 
> s/clk_get/clk_prepare_enable/
> 
> Yes, but you're stuffing functional dependencies into the clock tree,
> which sucks. genpd was created to model these arbitrary dependencies.

Well let's not stuff anything beyond clock framework to the
clockdomain clock drivers. We already have the clockdomain
dependencies handled by the interconnect code (hwmod), and there
should be no problem moving those to be handled by genpd and the
interconnect target driver instances.

Care to take another look at Tero's patches with the assumption
that the clockdomain clocks stay just as a clocks?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren Dec. 9, 2016, 9:58 p.m. UTC | #14

* Michael Turquette <[e-mail]> [161209 13:28]:
> Quoting Tony Lindgren (2016-12-09 12:40:16)
> > Yeah that's what we already have with hwmod and PM runtime for the
> > clockctrl register. But hwmod currently directly manages the clkctrl
> > register, we just want to move that part to be a clock driver.
> > 
> > The children of the interconnect target modules just need to use
> > PM runtime, but the interconnect target module driver needs to know
> > it's clkctrl clock.
> 
> OK, that sounds good to me but I'm not quite sure about the difference
> between "children of interconnect target modules" versus just the
> "interconnect target module".

The interconnect target module specific registers are "sysc", "syss"
and "revision". Those are usually stuffed to random addresses, and
currently managed by hwmod. Each interconnect target module has a
module gate clock "clkctrl" in some clockdomain. Each clockdomain
has multiple similar "clckctrl" clocks.

> Can you give an example on each? I just want to understand for my own
> curiosity.

For example, musb on am335x has these as children of a single
interconnect target module:

- Two instances of musb wrapper IP
- Two instances of musb IP
- One instance of CPP41 DMA

The on omap4, the whole system control module is just a single
interconnect target module with tens of devices in it like padconf,
clocks, pbias regulator and whatever random devices.

> > I don't think the clockdomain should be a genpd provider because
> > that creates a genpd network of dependencies instead of a tree
> > structure. If we end up setting the clockdomains with genpd, then
> > only the other clockdomains should use them, but I don't know how
> > we ever keep drivers from directly tinkering with them..
> 
> Genpd is set up as an arbitrary graph, not strictly a tree, so these
> types of dependencies should be OK.

Well maybe that works then, worth checking for sure.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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@48004000 {
 	};
 }
 
+cm2: cm2@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>;
+	};
+};
+
 &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 {
+		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;
+		}
+	}
+
+	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)
+			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;
+
+	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 *)&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);
+	}
+}
+
 /**
  * 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);