[PATCH v2 0/5] clk: qcom: gdsc: Add support for clk control

[PATCH v2 0/5] clk: qcom: gdsc: Add support for clk control

[Rajendra Nayak]

This series allows clocks associated with GDSCs to be controlled
along with the powerdomains. To start with, some of the mmagic
clocks are assocaited with the mmagic gdscs on msm8996 so the
devices in these mmagic domains (vidc, mdss, their smmu instances etc)
don't have to put these in their resp. DT bindings.

changes in v2:
* Fixed review comments from Stan
* Added mmagic clocks needed by gpu

Rajendra Nayak (5):
  arm64: qcom: Select PM_GENERIC_DOMAINS
  clk: Add clk_hw_get_clk() helper API to be used by clk providers
  clk: qcom: gdsc: Add support to control associated clks
  clk: qcom: gcc-msm8996: Mark gcc_mmss_noc_cfg_ahb_clk as a critical
    clock
  clk: qcom: mmcc-8996: Associate all mmagic clks with mmagic gdscs

 arch/arm64/Kconfig.platforms    |  2 ++
 drivers/clk/clk.c               | 39 +++++++++++++++++++++++++++
 drivers/clk/qcom/gcc-msm8996.c  |  2 +-
 drivers/clk/qcom/gdsc.c         | 60 +++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h         |  8 ++++++
 drivers/clk/qcom/mmcc-msm8996.c | 26 ++++++++++++++++++
 include/linux/clk-provider.h    |  5 ++++
 7 files changed, 139 insertions(+), 3 deletions(-)

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

[PATCH v2 1/5] arm64: qcom: Select PM_GENERIC_DOMAINS

[Rajendra Nayak]

Enable PM_GENERIC_DOMAINS for 64-bit qualcomm platforms. This is required
to ensure devices which are dependent on some gdscs (powerdomains) to be
powered on before they can be probed, work as expected.

CC: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 arch/arm64/Kconfig.platforms | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index f5f0c81..e3de82d 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -133,6 +133,8 @@ config ARCH_QCOM
 	bool "Qualcomm Platforms"
 	select GPIOLIB
 	select PINCTRL
+	select PM
+	select PM_GENERIC_DOMAINS
 	help
 	  This enables support for the ARMv8 based Qualcomm chipsets.
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 2/5] clk: Add clk_hw_get_clk() helper API to be used by clk providers

[Rajendra Nayak]

As we move towards a cleaner split to have clock providers use clk_hw
for all clock operations, while consumers operate on the (per-user)
struct clk handles, we still have cases where in a clock provider
might want to call into high level clk apis which only operate on a
struct clk handle.
To facilitate such needs, have a clk_hw_get_clk() api which can be
used from within clock providers to get access to struct clk handles.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/clk.c            | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h |  5 +++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index fc58c52..c9bbfb3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -186,6 +186,45 @@ const char *clk_hw_get_name(const struct clk_hw *hw)
 }
 EXPORT_SYMBOL_GPL(clk_hw_get_name);
 
+struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *dev_id,
+			   const char *con_id)
+{
+	return __clk_create_clk(hw, dev_id, con_id);
+}
+EXPORT_SYMBOL_GPL(clk_hw_get_clk);
+
+void clk_hw_put_clk(struct clk *clk)
+{
+	__clk_free_clk(clk);
+}
+EXPORT_SYMBOL_GPL(clk_hw_put_clk);
+
+static void devm_clk_hw_put(struct device *dev, void *res)
+{
+	clk_hw_put_clk(*(struct clk **)res);
+}
+
+struct clk *devm_clk_hw_get_clk(struct device *dev, struct clk_hw *hw,
+				const char *con_id)
+{
+	struct clk **ptr, *clk;
+
+	ptr = devres_alloc(devm_clk_hw_put, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	clk = clk_hw_get_clk(hw, dev_name(dev), con_id);
+	if (!IS_ERR(clk)) {
+		*ptr = clk;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return clk;
+}
+EXPORT_SYMBOL_GPL(devm_clk_hw_get_clk);
+
 struct clk_hw *__clk_get_hw(struct clk *clk)
 {
 	return !clk ? NULL : clk->core->hw;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index c59c625..c85bfed 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -730,6 +730,11 @@ struct clk_hw *clk_hw_register_gpio_mux(struct device *dev, const char *name,
 /* helper functions */
 const char *__clk_get_name(const struct clk *clk);
 const char *clk_hw_get_name(const struct clk_hw *hw);
+struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *dev_id,
+			   const char *con_id);
+void clk_hw_put_clk(struct clk *clk);
+struct clk *devm_clk_hw_get_clk(struct device *dev, struct clk_hw *hw,
+				const char *con_id);
 struct clk_hw *__clk_get_hw(struct clk *clk);
 unsigned int clk_hw_get_num_parents(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 3/5] clk: qcom: gdsc: Add support to control associated clks

[Rajendra Nayak]

The devices within a gdsc power domain, quite often have additional
clocks to be turned on/off along with the power domain itself.
Add support for this by specifying a list of clk_hw pointers
per gdsc which would be the clocks turned on/off along with the
powerdomain on/off callbacks.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h |  8 +++++++
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a4f3580..7e7c051 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -12,6 +12,8 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/jiffies.h>
@@ -21,6 +23,7 @@
 #include <linux/regmap.h>
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
+#include "common.h"
 #include "gdsc.h"
 
 #define PWR_ON_MASK		BIT(31)
@@ -166,6 +169,29 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
 			   GMEM_CLAMP_IO_MASK, 1);
 }
 
+static inline int gdsc_clk_enable(struct gdsc *sc)
+{
+	int i, ret;
+
+	for (i = 0; i < sc->clk_count; i++) {
+		ret = clk_prepare_enable(sc->clks[i]);
+		if (ret) {
+			for (i--; i >= 0; i--)
+				clk_disable_unprepare(sc->clks[i]);
+			return ret;
+		}
+	}
+	return 0;
+}
+
+static inline void gdsc_clk_disable(struct gdsc *sc)
+{
+	int i;
+
+	for (i = 0; i < sc->clk_count; i++)
+		clk_disable_unprepare(sc->clks[i]);
+}
+
 static int gdsc_enable(struct generic_pm_domain *domain)
 {
 	struct gdsc *sc = domain_to_gdsc(domain);
@@ -193,6 +219,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	 */
 	udelay(1);
 
+	ret = gdsc_clk_enable(sc);
+	if (ret)
+		return ret;
+
 	/* Turn on HW trigger mode if supported */
 	if (sc->flags & HW_CTRL) {
 		ret = gdsc_hwctrl(sc, true);
@@ -241,6 +271,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 			return ret;
 	}
 
+	gdsc_clk_disable(sc);
+
 	if (sc->pwrsts & PWRSTS_OFF)
 		gdsc_clear_mem_on(sc);
 
@@ -254,7 +286,27 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	return 0;
 }
 
-static int gdsc_init(struct gdsc *sc)
+static inline int gdsc_clk_get(struct device *dev, struct gdsc *sc)
+{
+	if (sc->clk_count) {
+		int i;
+
+		sc->clks = devm_kcalloc(dev, sc->clk_count, sizeof(*sc->clks),
+					GFP_KERNEL);
+		if (!sc->clks)
+			return -ENOMEM;
+
+		for (i = 0; i < sc->clk_count; i++) {
+			sc->clks[i] = devm_clk_hw_get_clk(dev, sc->clk_hws[i],
+							  NULL);
+			if (IS_ERR(sc->clks[i]))
+				return PTR_ERR(sc->clks[i]);
+		}
+	}
+	return 0;
+}
+
+static int gdsc_init(struct device *dev, struct gdsc *sc)
 {
 	u32 mask, val;
 	int on, ret;
@@ -284,6 +336,10 @@ static int gdsc_init(struct gdsc *sc)
 	if (on < 0)
 		return on;
 
+	ret = gdsc_clk_get(dev, sc);
+	if (ret)
+		return ret;
+
 	/*
 	 * Votable GDSCs can be ON due to Vote from other masters.
 	 * If a Votable GDSC is ON, make sure we have a Vote.
@@ -327,7 +383,7 @@ int gdsc_register(struct gdsc_desc *desc,
 			continue;
 		scs[i]->regmap = regmap;
 		scs[i]->rcdev = rcdev;
-		ret = gdsc_init(scs[i]);
+		ret = gdsc_init(dev, scs[i]);
 		if (ret)
 			return ret;
 		data->domains[i] = &scs[i]->pd;
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 3964834..a7fd51b 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -17,6 +17,8 @@
 #include <linux/err.h>
 #include <linux/pm_domain.h>
 
+struct clk;
+struct clk_hw;
 struct regmap;
 struct reset_controller_dev;
 
@@ -32,6 +34,9 @@
  * @resets: ids of resets associated with this gdsc
  * @reset_count: number of @resets
  * @rcdev: reset controller
+ * @clk_count: number of gdsc clocks
+ * @clks: clk pointers for gdsc clocks
+ * @clk_hws: clk_hw pointers for gdsc clocks
  */
 struct gdsc {
 	struct generic_pm_domain	pd;
@@ -56,6 +61,9 @@ struct gdsc {
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
+	unsigned int			clk_count;
+	struct clk			**clks;
+	struct clk_hw			*clk_hws[];
 };
 
 struct gdsc_desc {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 4/5] clk: qcom: gcc-msm8996: Mark gcc_mmss_noc_cfg_ahb_clk as a critical clock

[Rajendra Nayak]

we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
this can prevent it from being disabled while its unused, it does not
prevent it from being disabled when a child derived from the clock calls
an explicit enable/disable.
Mark this with a CLK_IS_CRITICAL and remove the CLK_IGNORE_UNUSED flag
so its never disabled.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/gcc-msm8996.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
index 8abc200..8872985 100644
--- a/drivers/clk/qcom/gcc-msm8996.c
+++ b/drivers/clk/qcom/gcc-msm8996.c
@@ -1333,7 +1333,7 @@ enum {
 			.name = "gcc_mmss_noc_cfg_ahb_clk",
 			.parent_names = (const char *[]){ "config_noc_clk_src" },
 			.num_parents = 1,
-			.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
 			.ops = &clk_branch2_ops,
 		},
 	},
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 5/5] clk: qcom: mmcc-8996: Associate all mmagic clks with mmagic gdscs

[Rajendra Nayak]

With support added to control clocks associated with a gdsc from within
the gdsc driver, associate all mmagic clock instances with the respective
mmagic gdscs so devices inside these mmagic domains do not have to control
them individually.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/clk/qcom/mmcc-msm8996.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index 352394d..9d2d34b 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -2902,6 +2902,13 @@ enum {
 	.pd = {
 		.name = "mmagic_video",
 	},
+	.clk_hws = {
+		&mmss_mmagic_ahb_clk.clkr.hw,
+		&mmss_mmagic_cfg_ahb_clk.clkr.hw,
+		&mmagic_video_axi_clk.clkr.hw,
+		&mmagic_video_noc_cfg_ahb_clk.clkr.hw,
+	},
+	.clk_count = 4,
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = VOTABLE,
 };
@@ -2912,6 +2919,13 @@ enum {
 	.pd = {
 		.name = "mmagic_mdss",
 	},
+	.clk_hws = {
+		&mmss_mmagic_ahb_clk.clkr.hw,
+		&mmss_mmagic_cfg_ahb_clk.clkr.hw,
+		&mmagic_mdss_axi_clk.clkr.hw,
+		&mmagic_mdss_noc_cfg_ahb_clk.clkr.hw,
+	},
+	.clk_count = 4,
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = VOTABLE,
 };
@@ -2922,6 +2936,13 @@ enum {
 	.pd = {
 		.name = "mmagic_camss",
 	},
+	.clk_hws = {
+		&mmss_mmagic_ahb_clk.clkr.hw,
+		&mmss_mmagic_cfg_ahb_clk.clkr.hw,
+		&mmagic_camss_axi_clk.clkr.hw,
+		&mmagic_camss_noc_cfg_ahb_clk.clkr.hw,
+	},
+	.clk_count = 4,
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = VOTABLE,
 };
@@ -3056,6 +3077,11 @@ enum {
 	.pd = {
 		.name = "gpu_gx",
 	},
+	.clk_hws = {
+		&mmss_mmagic_ahb_clk.clkr.hw,
+		&mmss_mmagic_cfg_ahb_clk.clkr.hw,
+	},
+	.clk_count = 2,
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = CLAMP_IO,
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 3/5] clk: qcom: gdsc: Add support to control associated clks

[Stanimir Varbanov]

Hi Rajendra,

few small comments to get code closer to perfection...

On 07/20/2017 07:48 AM, Rajendra Nayak wrote:
> The devices within a gdsc power domain, quite often have additional
> clocks to be turned on/off along with the power domain itself.
> Add support for this by specifying a list of clk_hw pointers
> per gdsc which would be the clocks turned on/off along with the
> powerdomain on/off callbacks.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/gdsc.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/clk/qcom/gdsc.h |  8 +++++++
>  2 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index a4f3580..7e7c051 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -12,6 +12,8 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/jiffies.h>
> @@ -21,6 +23,7 @@
>  #include <linux/regmap.h>
>  #include <linux/reset-controller.h>
>  #include <linux/slab.h>
> +#include "common.h"
>  #include "gdsc.h"
>  
>  #define PWR_ON_MASK		BIT(31)
> @@ -166,6 +169,29 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
>  			   GMEM_CLAMP_IO_MASK, 1);
>  }
>  
> +static inline int gdsc_clk_enable(struct gdsc *sc)

I think the compiler is smart enough so 'inline' can dropped here and below.

> +{
> +	int i, ret;
> +
> +	for (i = 0; i < sc->clk_count; i++) {
> +		ret = clk_prepare_enable(sc->clks[i]);
> +		if (ret) {
> +			for (i--; i >= 0; i--)
> +				clk_disable_unprepare(sc->clks[i]);
> +			return ret;
> +		}
> +	}

blank line please.

> +	return 0;
> +}
> +
> +static inline void gdsc_clk_disable(struct gdsc *sc)
> +{
> +	int i;
> +
> +	for (i = 0; i < sc->clk_count; i++)
> +		clk_disable_unprepare(sc->clks[i]);

can we disable clocks in reverse order, not sure that will make any
sense but I've seen issues with some of the clocks in the past.

> +}
> +
>  static int gdsc_enable(struct generic_pm_domain *domain)
>  {
>  	struct gdsc *sc = domain_to_gdsc(domain);
> @@ -193,6 +219,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	 */
>  	udelay(1);
>  
> +	ret = gdsc_clk_enable(sc);
> +	if (ret)
> +		return ret;
> +
>  	/* Turn on HW trigger mode if supported */
>  	if (sc->flags & HW_CTRL) {
>  		ret = gdsc_hwctrl(sc, true);
> @@ -241,6 +271,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  			return ret;
>  	}
>  
> +	gdsc_clk_disable(sc);
> +
>  	if (sc->pwrsts & PWRSTS_OFF)
>  		gdsc_clear_mem_on(sc);
>  
> @@ -254,7 +286,27 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  	return 0;
>  }
>  
> -static int gdsc_init(struct gdsc *sc)
> +static inline int gdsc_clk_get(struct device *dev, struct gdsc *sc)
> +{
> +	if (sc->clk_count) {

could you inverse the logic

if (!sc->clk_count)
	return 0;

> +		int i;
> +
> +		sc->clks = devm_kcalloc(dev, sc->clk_count, sizeof(*sc->clks),
> +					GFP_KERNEL);
> +		if (!sc->clks)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < sc->clk_count; i++) {
> +			sc->clks[i] = devm_clk_hw_get_clk(dev, sc->clk_hws[i],
> +							  NULL);
> +			if (IS_ERR(sc->clks[i]))
> +				return PTR_ERR(sc->clks[i]);
> +		}
> +	}

add blank line please

> +	return 0;
> +}
> +

I will make some tests with venus driver on mainline soon.

-- 
regards,
Stan

[PATCH v2 0/5] clk: qcom: gdsc: Add support for clk control

[Vivek Gautam]

On Thu, Jul 20, 2017 at 10:18 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> This series allows clocks associated with GDSCs to be controlled
> along with the powerdomains. To start with, some of the mmagic
> clocks are assocaited with the mmagic gdscs on msm8996 so the
> devices in these mmagic domains (vidc, mdss, their smmu instances etc)
> don't have to put these in their resp. DT bindings.
>
> changes in v2:
> * Fixed review comments from Stan
> * Added mmagic clocks needed by gpu

Tested the series on db820c with necessary iommu and display/gpu
patches.

Tested-by: Vivek Gautam <vivek.gautam@codeaurora.org>


Regards
Vivek

>
> Rajendra Nayak (5):
>   arm64: qcom: Select PM_GENERIC_DOMAINS
>   clk: Add clk_hw_get_clk() helper API to be used by clk providers
>   clk: qcom: gdsc: Add support to control associated clks
>   clk: qcom: gcc-msm8996: Mark gcc_mmss_noc_cfg_ahb_clk as a critical
>     clock
>   clk: qcom: mmcc-8996: Associate all mmagic clks with mmagic gdscs
>
>  arch/arm64/Kconfig.platforms    |  2 ++
>  drivers/clk/clk.c               | 39 +++++++++++++++++++++++++++
>  drivers/clk/qcom/gcc-msm8996.c  |  2 +-
>  drivers/clk/qcom/gdsc.c         | 60 +++++++++++++++++++++++++++++++++++++++--
>  drivers/clk/qcom/gdsc.h         |  8 ++++++
>  drivers/clk/qcom/mmcc-msm8996.c | 26 ++++++++++++++++++
>  include/linux/clk-provider.h    |  5 ++++
>  7 files changed, 139 insertions(+), 3 deletions(-)
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/5] clk: Add clk_hw_get_clk() helper API to be used by clk providers

[Stephen Boyd]

On 07/20, Rajendra Nayak wrote:
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fc58c52..c9bbfb3 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -186,6 +186,45 @@ const char *clk_hw_get_name(const struct clk_hw *hw)
>  }
>  EXPORT_SYMBOL_GPL(clk_hw_get_name);
>  
> +struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *dev_id,
> +			   const char *con_id)
> +{
> +	return __clk_create_clk(hw, dev_id, con_id);
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_get_clk);
> +
> +void clk_hw_put_clk(struct clk *clk)
> +{
> +	__clk_free_clk(clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_put_clk);

Isn't this just clk_put()? Not sure why we need this API.

> +
> +static void devm_clk_hw_put(struct device *dev, void *res)
> +{
> +	clk_hw_put_clk(*(struct clk **)res);
> +}

Same comment.

> +
> +struct clk *devm_clk_hw_get_clk(struct device *dev, struct clk_hw *hw,
> +				const char *con_id)
> +{
> +	struct clk **ptr, *clk;
> +
> +	ptr = devres_alloc(devm_clk_hw_put, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	clk = clk_hw_get_clk(hw, dev_name(dev), con_id);
> +	if (!IS_ERR(clk)) {
> +		*ptr = clk;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return clk;
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_hw_get_clk);

Hm.. ok. Wasn't expecting us to need this API, but I guess it is
needed.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/5] clk: qcom: gcc-msm8996: Mark gcc_mmss_noc_cfg_ahb_clk as a critical clock

[Stephen Boyd]

On 07/20, Rajendra Nayak wrote:
> we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
> this can prevent it from being disabled while its unused, it does not
> prevent it from being disabled when a child derived from the clock calls
> an explicit enable/disable.

Do you mean the config_noc_clk_src is being disabled? Who is
getting this clk? The bus driver? In the downstream sources I
don't even see this clk exposed, so maybe we should just remove
support for it instead.

> Mark this with a CLK_IS_CRITICAL and remove the CLK_IGNORE_UNUSED flag
> so its never disabled.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/gcc-msm8996.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
> index 8abc200..8872985 100644
> --- a/drivers/clk/qcom/gcc-msm8996.c
> +++ b/drivers/clk/qcom/gcc-msm8996.c
> @@ -1333,7 +1333,7 @@ enum {
>  			.name = "gcc_mmss_noc_cfg_ahb_clk",
>  			.parent_names = (const char *[]){ "config_noc_clk_src" },
>  			.num_parents = 1,
> -			.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>  			.ops = &clk_branch2_ops,
>  		},
>  	},
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/5] clk: qcom: gdsc: Add support to control associated clks

[Stephen Boyd]

On 07/20, Rajendra Nayak wrote:
> @@ -166,6 +169,29 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
>  			   GMEM_CLAMP_IO_MASK, 1);
>  }
>  
> +static inline int gdsc_clk_enable(struct gdsc *sc)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < sc->clk_count; i++) {
> +		ret = clk_prepare_enable(sc->clks[i]);
> +		if (ret) {
> +			for (i--; i >= 0; i--)

while (--i >= 0) ?

> +				clk_disable_unprepare(sc->clks[i]);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static inline void gdsc_clk_disable(struct gdsc *sc)
> +{
> +	int i;
> +
> +	for (i = 0; i < sc->clk_count; i++)

Could also be the same while loop.

> +		clk_disable_unprepare(sc->clks[i]);
> +}
> +
>  static int gdsc_enable(struct generic_pm_domain *domain)
>  {
>  	struct gdsc *sc = domain_to_gdsc(domain);
> @@ -193,6 +219,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	 */
>  	udelay(1);
>  
> +	ret = gdsc_clk_enable(sc);
> +	if (ret)
> +		return ret;
> +
>  	/* Turn on HW trigger mode if supported */
>  	if (sc->flags & HW_CTRL) {
>  		ret = gdsc_hwctrl(sc, true);
> @@ -241,6 +271,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  			return ret;
>  	}
>  
> +	gdsc_clk_disable(sc);

Can we call sleeping APIs (i.e. prepare/unprepare) from within
the power domains? I thought that didn't work generically because
someone could set their runtime PM configuration to be atomic
context friendly. Is there some way we can block that from
happening if we hook up a power domain that is no longer safe to
call from atomic context?

> +
>  	if (sc->pwrsts & PWRSTS_OFF)
>  		gdsc_clear_mem_on(sc);
>  
> @@ -254,7 +286,27 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  	return 0;
>  }
>  
> -static int gdsc_init(struct gdsc *sc)
> +static inline int gdsc_clk_get(struct device *dev, struct gdsc *sc)
> +{
> +	if (sc->clk_count) {

We could drop this check. kmalloc with size zero is OK as long as
we don't use the returned pointer value. We shouldn't use it
assuming the for loop is maintained.

> +		int i;
> +
> +		sc->clks = devm_kcalloc(dev, sc->clk_count, sizeof(*sc->clks),
> +					GFP_KERNEL);
> +		if (!sc->clks)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < sc->clk_count; i++) {
> +			sc->clks[i] = devm_clk_hw_get_clk(dev, sc->clk_hws[i],
> +							  NULL);
> +			if (IS_ERR(sc->clks[i]))
> +				return PTR_ERR(sc->clks[i]);
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int gdsc_init(struct device *dev, struct gdsc *sc)
>  {
>  	u32 mask, val;
>  	int on, ret;
> @@ -284,6 +336,10 @@ static int gdsc_init(struct gdsc *sc)
>  	if (on < 0)
>  		return on;
>  
> +	ret = gdsc_clk_get(dev, sc);
> +	if (ret)
> +		return ret;
> +

This concerns me if we do probe defer on orphan clks. We may get
into some situation where the gdsc is setup by a clk driver that
is trying to probe before some parent clk has registered for the
clks we're "getting" here. For example, this could easily happen
if we insert XO into the tree at the top and everything defers.

I suppose this is not a problem because in this case we don't
have any clk here that could be orphaned even if RPM clks are
inserted into the clk tree for XO? I mean to say that we won't
get into a probe defer due to orphan clk loop. I'm always afraid
someone will make hardware that send a clk from one controller to
another and then back again (we did that with pcie) and then
we'll be unable to get out of the defer loop because we'll be
deferred on orphan status.


>  	/*
>  	 * Votable GDSCs can be ON due to Vote from other masters.
>  	 * If a Votable GDSC is ON, make sure we have a Vote.
> @@ -327,7 +383,7 @@ int gdsc_register(struct gdsc_desc *desc,
>  			continue;
>  		scs[i]->regmap = regmap;
>  		scs[i]->rcdev = rcdev;
> -		ret = gdsc_init(scs[i]);
> +		ret = gdsc_init(dev, scs[i]);
>  		if (ret)
>  			return ret;
>  		data->domains[i] = &scs[i]->pd;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/5] clk: Add clk_hw_get_clk() helper API to be used by clk providers

[Rajendra Nayak]

On 07/28/2017 04:17 AM, Stephen Boyd wrote:
> On 07/20, Rajendra Nayak wrote:
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index fc58c52..c9bbfb3 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -186,6 +186,45 @@ const char *clk_hw_get_name(const struct clk_hw *hw)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_hw_get_name);
>>  
>> +struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *dev_id,
>> +			   const char *con_id)
>> +{
>> +	return __clk_create_clk(hw, dev_id, con_id);
>> +}
>> +EXPORT_SYMBOL_GPL(clk_hw_get_clk);
>> +
>> +void clk_hw_put_clk(struct clk *clk)
>> +{
>> +	__clk_free_clk(clk);
>> +}
>> +EXPORT_SYMBOL_GPL(clk_hw_put_clk);
> 
> Isn't this just clk_put()? Not sure why we need this API.
> 
>> +
>> +static void devm_clk_hw_put(struct device *dev, void *res)
>> +{
>> +	clk_hw_put_clk(*(struct clk **)res);
>> +}
> 
> Same comment.

yes, I guess we don;t need these.

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

[PATCH v2 4/5] clk: qcom: gcc-msm8996: Mark gcc_mmss_noc_cfg_ahb_clk as a critical clock

[Rajendra Nayak]

On 07/28/2017 04:21 AM, Stephen Boyd wrote:
> On 07/20, Rajendra Nayak wrote:
>> we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
>> this can prevent it from being disabled while its unused, it does not
>> prevent it from being disabled when a child derived from the clock calls
>> an explicit enable/disable.
> 
> Do you mean the config_noc_clk_src is being disabled? Who is

I think the issue I saw was after I switched the parent from
config_noc_clk_src to the RPM controlled cnoc_clk. That had to
be kept voted, else some other ahb clock derived from it being
disabled was causing the vote on cnoc_clk to be dropped.

> getting this clk? The bus driver? In the downstream sources I
> don't even see this clk exposed, so maybe we should just remove
> support for it instead.
> 
>> Mark this with a CLK_IS_CRITICAL and remove the CLK_IGNORE_UNUSED flag
>> so its never disabled.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>  drivers/clk/qcom/gcc-msm8996.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
>> index 8abc200..8872985 100644
>> --- a/drivers/clk/qcom/gcc-msm8996.c
>> +++ b/drivers/clk/qcom/gcc-msm8996.c
>> @@ -1333,7 +1333,7 @@ enum {
>>  			.name = "gcc_mmss_noc_cfg_ahb_clk",
>>  			.parent_names = (const char *[]){ "config_noc_clk_src" },
>>  			.num_parents = 1,
>> -			.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> +			.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>>  			.ops = &clk_branch2_ops,
>>  		},
>>  	},
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
> 

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

[PATCH v2 3/5] clk: qcom: gdsc: Add support to control associated clks

[Rajendra Nayak]

On 07/28/2017 04:32 AM, Stephen Boyd wrote:
> On 07/20, Rajendra Nayak wrote:
>> @@ -166,6 +169,29 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
>>  			   GMEM_CLAMP_IO_MASK, 1);
>>  }
>>  
>> +static inline int gdsc_clk_enable(struct gdsc *sc)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0; i < sc->clk_count; i++) {
>> +		ret = clk_prepare_enable(sc->clks[i]);
>> +		if (ret) {
>> +			for (i--; i >= 0; i--)
> 
> while (--i >= 0) ?
> 
>> +				clk_disable_unprepare(sc->clks[i]);
>> +			return ret;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static inline void gdsc_clk_disable(struct gdsc *sc)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < sc->clk_count; i++)
> 
> Could also be the same while loop.
> 
>> +		clk_disable_unprepare(sc->clks[i]);
>> +}
>> +
>>  static int gdsc_enable(struct generic_pm_domain *domain)
>>  {
>>  	struct gdsc *sc = domain_to_gdsc(domain);
>> @@ -193,6 +219,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>  	 */
>>  	udelay(1);
>>  
>> +	ret = gdsc_clk_enable(sc);
>> +	if (ret)
>> +		return ret;
>> +
>>  	/* Turn on HW trigger mode if supported */
>>  	if (sc->flags & HW_CTRL) {
>>  		ret = gdsc_hwctrl(sc, true);
>> @@ -241,6 +271,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>  			return ret;
>>  	}
>>  
>> +	gdsc_clk_disable(sc);
> 
> Can we call sleeping APIs (i.e. prepare/unprepare) from within
> the power domains? I thought that didn't work generically because
> someone could set their runtime PM configuration to be atomic
> context friendly. Is there some way we can block that from
> happening if we hook up a power domain that is no longer safe to
> call from atomic context?

hmm, I don't see a way to do that. Perhaps we could prepare/unprepare
these as part of the pm_domain attach/detach to a device and then
only enable/disable them as part of the gdsc_enable/disable?

> 
>> +
>>  	if (sc->pwrsts & PWRSTS_OFF)
>>  		gdsc_clear_mem_on(sc);
>>  
>> @@ -254,7 +286,27 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>  	return 0;
>>  }
>>  
>> -static int gdsc_init(struct gdsc *sc)
>> +static inline int gdsc_clk_get(struct device *dev, struct gdsc *sc)
>> +{
>> +	if (sc->clk_count) {
> 
> We could drop this check. kmalloc with size zero is OK as long as
> we don't use the returned pointer value. We shouldn't use it
> assuming the for loop is maintained.
> 
>> +		int i;
>> +
>> +		sc->clks = devm_kcalloc(dev, sc->clk_count, sizeof(*sc->clks),
>> +					GFP_KERNEL);
>> +		if (!sc->clks)
>> +			return -ENOMEM;
>> +
>> +		for (i = 0; i < sc->clk_count; i++) {
>> +			sc->clks[i] = devm_clk_hw_get_clk(dev, sc->clk_hws[i],
>> +							  NULL);
>> +			if (IS_ERR(sc->clks[i]))
>> +				return PTR_ERR(sc->clks[i]);
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int gdsc_init(struct device *dev, struct gdsc *sc)
>>  {
>>  	u32 mask, val;
>>  	int on, ret;
>> @@ -284,6 +336,10 @@ static int gdsc_init(struct gdsc *sc)
>>  	if (on < 0)
>>  		return on;
>>  
>> +	ret = gdsc_clk_get(dev, sc);
>> +	if (ret)
>> +		return ret;
>> +
> 
> This concerns me if we do probe defer on orphan clks. We may get
> into some situation where the gdsc is setup by a clk driver that
> is trying to probe before some parent clk has registered for the
> clks we're "getting" here. For example, this could easily happen
> if we insert XO into the tree at the top and everything defers.
> 
> I suppose this is not a problem because in this case we don't
> have any clk here that could be orphaned even if RPM clks are
> inserted into the clk tree for XO? I mean to say that we won't
> get into a probe defer due to orphan clk loop. I'm always afraid
> someone will make hardware that send a clk from one controller to
> another and then back again (we did that with pcie) and then
> we'll be unable to get out of the defer loop because we'll be
> deferred on orphan status.

Yes, we would probably run into these issues with probe defer for
orphan clks. One way to handle it could be to decouple the probing
of the clocks and powerdomain parts of the controller. Like the clock
driver can probe and then register a dummy device for powerdomains
(like is done for tsens on 8960) so it could probe independently?

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

[PATCH v2 3/5] clk: qcom: gdsc: Add support to control associated clks

[Stephen Boyd]

On 07/28, Rajendra Nayak wrote:
> 
> 
> On 07/28/2017 04:32 AM, Stephen Boyd wrote:
> > On 07/20, Rajendra Nayak wrote:
> > 
> >> +		clk_disable_unprepare(sc->clks[i]);
> >> +}
> >> +
> >>  static int gdsc_enable(struct generic_pm_domain *domain)
> >>  {
> >>  	struct gdsc *sc = domain_to_gdsc(domain);
> >> @@ -193,6 +219,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> >>  	 */
> >>  	udelay(1);
> >>  
> >> +	ret = gdsc_clk_enable(sc);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>  	/* Turn on HW trigger mode if supported */
> >>  	if (sc->flags & HW_CTRL) {
> >>  		ret = gdsc_hwctrl(sc, true);
> >> @@ -241,6 +271,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
> >>  			return ret;
> >>  	}
> >>  
> >> +	gdsc_clk_disable(sc);
> > 
> > Can we call sleeping APIs (i.e. prepare/unprepare) from within
> > the power domains? I thought that didn't work generically because
> > someone could set their runtime PM configuration to be atomic
> > context friendly. Is there some way we can block that from
> > happening if we hook up a power domain that is no longer safe to
> > call from atomic context?
> 
> hmm, I don't see a way to do that. Perhaps we could prepare/unprepare
> these as part of the pm_domain attach/detach to a device and then
> only enable/disable them as part of the gdsc_enable/disable?
> 

The problem there is if we keep these clks prepared while the
domain is attached to a device I don't see how we can ever turn
off the XO clk and achieve XO shutdown in low power modes. A pm
domain is basically never detached, right? This is where we need
different power levels in PM domains if I understand correctly.

> > 
> > This concerns me if we do probe defer on orphan clks. We may get
> > into some situation where the gdsc is setup by a clk driver that
> > is trying to probe before some parent clk has registered for the
> > clks we're "getting" here. For example, this could easily happen
> > if we insert XO into the tree at the top and everything defers.
> > 
> > I suppose this is not a problem because in this case we don't
> > have any clk here that could be orphaned even if RPM clks are
> > inserted into the clk tree for XO? I mean to say that we won't
> > get into a probe defer due to orphan clk loop. I'm always afraid
> > someone will make hardware that send a clk from one controller to
> > another and then back again (we did that with pcie) and then
> > we'll be unable to get out of the defer loop because we'll be
> > deferred on orphan status.
> 
> Yes, we would probably run into these issues with probe defer for
> orphan clks. One way to handle it could be to decouple the probing
> of the clocks and powerdomain parts of the controller. Like the clock
> driver can probe and then register a dummy device for powerdomains
> (like is done for tsens on 8960) so it could probe independently?
> 

Well is it a problem right now? I think we're OK here because we
don't have a "cycle" between clk providers in the clk provider
hierarchy. Can we clk_get() during attach and then fail attach if
we can't get clks at that point? If this works, we have a backup
plan should something go wrong, but we can ignore this concern
for now until it becomes a real problem.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/5] clk: qcom: gcc-msm8996: Mark gcc_mmss_noc_cfg_ahb_clk as a critical clock

[Stephen Boyd]

On 07/28, Rajendra Nayak wrote:
> 
> On 07/28/2017 04:21 AM, Stephen Boyd wrote:
> > On 07/20, Rajendra Nayak wrote:
> >> we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
> >> this can prevent it from being disabled while its unused, it does not
> >> prevent it from being disabled when a child derived from the clock calls
> >> an explicit enable/disable.
> > 
> > Do you mean the config_noc_clk_src is being disabled? Who is
> 
> I think the issue I saw was after I switched the parent from
> config_noc_clk_src to the RPM controlled cnoc_clk. That had to
> be kept voted, else some other ahb clock derived from it being
> disabled was causing the vote on cnoc_clk to be dropped.

Hmm ok. Maybe we have some sort of bus driver vote from the
multimedia clk driver in downstream code? Let me check.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 1/5] arm64: qcom: Select PM_GENERIC_DOMAINS

[Stephen Boyd]

On 07/20, Rajendra Nayak wrote:
> Enable PM_GENERIC_DOMAINS for 64-bit qualcomm platforms. This is required
> to ensure devices which are dependent on some gdscs (powerdomains) to be
> powered on before they can be probed, work as expected.
> 
> CC: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---

Maybe Andy can ack this? It's ARCH_QCOM specific anyway.

>  arch/arm64/Kconfig.platforms | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index f5f0c81..e3de82d 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -133,6 +133,8 @@ config ARCH_QCOM
>  	bool "Qualcomm Platforms"
>  	select GPIOLIB
>  	select PINCTRL
> +	select PM
> +	select PM_GENERIC_DOMAINS
>  	help
>  	  This enables support for the ARMv8 based Qualcomm chipsets.
>  

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/5] clk: qcom: gcc-msm8996: Mark gcc_mmss_noc_cfg_ahb_clk as a critical clock

[Stephen Boyd]

On 07/28, Stephen Boyd wrote:
> On 07/28, Rajendra Nayak wrote:
> > 
> > On 07/28/2017 04:21 AM, Stephen Boyd wrote:
> > > On 07/20, Rajendra Nayak wrote:
> > >> we have gcc_mmss_noc_cfg_ahb_clk marked with a CLK_IGNORE_UNUSED. While
> > >> this can prevent it from being disabled while its unused, it does not
> > >> prevent it from being disabled when a child derived from the clock calls
> > >> an explicit enable/disable.
> > > 
> > > Do you mean the config_noc_clk_src is being disabled? Who is
> > 
> > I think the issue I saw was after I switched the parent from
> > config_noc_clk_src to the RPM controlled cnoc_clk. That had to
> > be kept voted, else some other ahb clock derived from it being
> > disabled was causing the vote on cnoc_clk to be dropped.
> 
> Hmm ok. Maybe we have some sort of bus driver vote from the
> multimedia clk driver in downstream code? Let me check.
> 

Ah I was looking at the mmss file when I should have been looking
at the gcc file. I see it downstream too, where we call
clk_prepare_enable() during gcc boot. This patch looks fine.
Eventually, it would make more sense to have this handled by the
bus driver, but until we get there this is fine.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/5] clk: qcom: gdsc: Add support to control associated clks

[Rajendra Nayak]

On 07/28/2017 10:07 PM, Stephen Boyd wrote:
> On 07/28, Rajendra Nayak wrote:
>>
>>
>> On 07/28/2017 04:32 AM, Stephen Boyd wrote:
>>> On 07/20, Rajendra Nayak wrote:
>>>
>>>> +		clk_disable_unprepare(sc->clks[i]);
>>>> +}
>>>> +
>>>>  static int gdsc_enable(struct generic_pm_domain *domain)
>>>>  {
>>>>  	struct gdsc *sc = domain_to_gdsc(domain);
>>>> @@ -193,6 +219,10 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>>>  	 */
>>>>  	udelay(1);
>>>>  
>>>> +	ret = gdsc_clk_enable(sc);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>  	/* Turn on HW trigger mode if supported */
>>>>  	if (sc->flags & HW_CTRL) {
>>>>  		ret = gdsc_hwctrl(sc, true);
>>>> @@ -241,6 +271,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>>>  			return ret;
>>>>  	}
>>>>  
>>>> +	gdsc_clk_disable(sc);
>>>
>>> Can we call sleeping APIs (i.e. prepare/unprepare) from within
>>> the power domains? I thought that didn't work generically because
>>> someone could set their runtime PM configuration to be atomic
>>> context friendly. Is there some way we can block that from
>>> happening if we hook up a power domain that is no longer safe to
>>> call from atomic context?
>>
>> hmm, I don't see a way to do that. Perhaps we could prepare/unprepare
>> these as part of the pm_domain attach/detach to a device and then
>> only enable/disable them as part of the gdsc_enable/disable?
>>
> 
> The problem there is if we keep these clks prepared while the
> domain is attached to a device I don't see how we can ever turn
> off the XO clk and achieve XO shutdown in low power modes. A pm
> domain is basically never detached, right? This is where we need
> different power levels in PM domains if I understand correctly.

right, I did not realize the XO shutdown would happen via a unprepare
callback and not disable.
So looking some more into this, genpd does have a GENPD_FLAG_IRQ_SAFE
used to indicate an IRQ safe domain. We don't set this today for any
of the gdscs so genpd uses mutex locking anyway.
When we do end up supporting/have a need to support IRQ safe domains
we might end up putting a check to make sure we don't associate any
clocks with those gdscs and leave it to the devices to handle it from
within the drivers.

> 
>>>
>>> This concerns me if we do probe defer on orphan clks. We may get
>>> into some situation where the gdsc is setup by a clk driver that
>>> is trying to probe before some parent clk has registered for the
>>> clks we're "getting" here. For example, this could easily happen
>>> if we insert XO into the tree at the top and everything defers.
>>>
>>> I suppose this is not a problem because in this case we don't
>>> have any clk here that could be orphaned even if RPM clks are
>>> inserted into the clk tree for XO? I mean to say that we won't
>>> get into a probe defer due to orphan clk loop. I'm always afraid
>>> someone will make hardware that send a clk from one controller to
>>> another and then back again (we did that with pcie) and then
>>> we'll be unable to get out of the defer loop because we'll be
>>> deferred on orphan status.
>>
>> Yes, we would probably run into these issues with probe defer for
>> orphan clks. One way to handle it could be to decouple the probing
>> of the clocks and powerdomain parts of the controller. Like the clock
>> driver can probe and then register a dummy device for powerdomains
>> (like is done for tsens on 8960) so it could probe independently?
>>
> 
> Well is it a problem right now? I think we're OK here because we
> don't have a "cycle" between clk providers in the clk provider
> hierarchy. Can we clk_get() during attach and then fail attach if
> we can't get clks at that point? If this works, we have a backup
> plan should something go wrong, but we can ignore this concern
> for now until it becomes a real problem.

we don't have a cycle but I was worried about gcc being deferred until
RPM clocks are probed and MMCC getting probed in between, and MMCC init
before GCC init would have us run into issues.
But I should be able to defer the clk_get()'s to during pm domain attach
and it should all work in that case.

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

[PATCH v2 3/5] clk: qcom: gdsc: Add support to control associated clks

[Rajendra Nayak]

On 07/31/2017 01:55 PM, Rajendra Nayak wrote:
[]..

>>>> This concerns me if we do probe defer on orphan clks. We may get
>>>> into some situation where the gdsc is setup by a clk driver that
>>>> is trying to probe before some parent clk has registered for the
>>>> clks we're "getting" here. For example, this could easily happen
>>>> if we insert XO into the tree at the top and everything defers.
>>>>
>>>> I suppose this is not a problem because in this case we don't
>>>> have any clk here that could be orphaned even if RPM clks are
>>>> inserted into the clk tree for XO? I mean to say that we won't
>>>> get into a probe defer due to orphan clk loop. I'm always afraid
>>>> someone will make hardware that send a clk from one controller to
>>>> another and then back again (we did that with pcie) and then
>>>> we'll be unable to get out of the defer loop because we'll be
>>>> deferred on orphan status.
>>>
>>> Yes, we would probably run into these issues with probe defer for
>>> orphan clks. One way to handle it could be to decouple the probing
>>> of the clocks and powerdomain parts of the controller. Like the clock
>>> driver can probe and then register a dummy device for powerdomains
>>> (like is done for tsens on 8960) so it could probe independently?
>>>
>>
>> Well is it a problem right now? I think we're OK here because we
>> don't have a "cycle" between clk providers in the clk provider
>> hierarchy. Can we clk_get() during attach and then fail attach if
>> we can't get clks at that point? If this works, we have a backup
>> plan should something go wrong, but we can ignore this concern
>> for now until it becomes a real problem.
> 
> we don't have a cycle but I was worried about gcc being deferred until
> RPM clocks are probed and MMCC getting probed in between, and MMCC init
> before GCC init would have us run into issues.
> But I should be able to defer the clk_get()'s to during pm domain attach
> and it should all work in that case.

So one issue I run into trying to move the clk_get() to during attach is,
most of these GDSCs to which we are associating the mmagic clocks are
votable and some of them happen to be ON when the gdsc driver probes.

And we have this to handle the situation..

        /*
         * Votable GDSCs can be ON due to Vote from other masters.
         * If a Votable GDSC is ON, make sure we have a Vote.
         */
        if ((sc->flags & VOTABLE) && on)
                gdsc_enable(&sc->pd);

And now since we defer clk_get() to a later point, the gdsc
can't really be enabled along with the enable/voting of the 
associated clocks.

I was thinking we do something like this instead,

	/*
	 * Votable GDSCs can be ON due to Vote from other masters.
	 * If *we* haven't Voted for it, tell genpd its actually OFF.
	 */
	if ((sc->flags & VOTABLE) && on)
		on = !on;

What do you think?

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

[PATCH v2 1/5] arm64: qcom: Select PM_GENERIC_DOMAINS

[Andy Gross]

On Thu, Jul 20, 2017 at 10:18:15AM +0530, Rajendra Nayak wrote:
> Enable PM_GENERIC_DOMAINS for 64-bit qualcomm platforms. This is required
> to ensure devices which are dependent on some gdscs (powerdomains) to be
> powered on before they can be probed, work as expected.
> 
> CC: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>

Go ahead and queue this with the related changes.

Acked-by: Andy Gross <andy.gross@linaro.org>