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

[PATCH 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.

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         | 48 +++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h         |  8 +++++++
 drivers/clk/qcom/mmcc-msm8996.c | 25 +++++++++++++++++++++
 include/linux/clk-provider.h    |  5 +++++
 7 files changed, 126 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 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.

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         | 48 +++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h         |  8 +++++++
 drivers/clk/qcom/mmcc-msm8996.c | 25 +++++++++++++++++++++
 include/linux/clk-provider.h    |  5 +++++
 7 files changed, 126 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 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.

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         | 48 +++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h         |  8 +++++++
 drivers/clk/qcom/mmcc-msm8996.c | 25 +++++++++++++++++++++
 include/linux/clk-provider.h    |  5 +++++
 7 files changed, 126 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 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 129cc5a..c5ae2cf 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -115,6 +115,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 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 129cc5a..c5ae2cf 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -115,6 +115,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 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 129cc5a..c5ae2cf 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -115,6 +115,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 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 0fb39fe..a6ba77c 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 a428aec..feddf95 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -729,6 +729,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 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 0fb39fe..a6ba77c 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 a428aec..feddf95 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -729,6 +729,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 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h |  8 ++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a4f3580..e9e7442 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -12,15 +12,19 @@
  */
 
 #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>
 #include <linux/kernel.h>
 #include <linux/ktime.h>
+#include <linux/pm_clock.h>
 #include <linux/pm_domain.h>
 #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 +170,27 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
 			   GMEM_CLAMP_IO_MASK, 1);
 }
 
+static 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)
+			pr_err("Failed to enable clock: %s\n",
+			       __clk_get_name(sc->clks[i]));
+	}
+	return ret;
+}
+
+static 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 +218,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	 */
 	udelay(1);
 
+	if (sc->clk_count)
+		gdsc_clk_enable(sc);
+
 	/* Turn on HW trigger mode if supported */
 	if (sc->flags & HW_CTRL) {
 		ret = gdsc_hwctrl(sc, true);
@@ -241,6 +269,9 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 			return ret;
 	}
 
+	if (sc->clk_count)
+		gdsc_clk_disable(sc);
+
 	if (sc->pwrsts & PWRSTS_OFF)
 		gdsc_clear_mem_on(sc);
 
@@ -254,7 +285,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	return 0;
 }
 
-static int gdsc_init(struct gdsc *sc)
+static int gdsc_init(struct device *dev, struct gdsc *sc)
 {
 	u32 mask, val;
 	int on, ret;
@@ -284,6 +315,19 @@ static int gdsc_init(struct gdsc *sc)
 	if (on < 0)
 		return on;
 
+	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);
+	}
+
 	/*
 	 * 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 +371,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 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h |  8 ++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a4f3580..e9e7442 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -12,15 +12,19 @@
  */
 
 #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>
 #include <linux/kernel.h>
 #include <linux/ktime.h>
+#include <linux/pm_clock.h>
 #include <linux/pm_domain.h>
 #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 +170,27 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
 			   GMEM_CLAMP_IO_MASK, 1);
 }
 
+static 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)
+			pr_err("Failed to enable clock: %s\n",
+			       __clk_get_name(sc->clks[i]));
+	}
+	return ret;
+}
+
+static 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 +218,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	 */
 	udelay(1);
 
+	if (sc->clk_count)
+		gdsc_clk_enable(sc);
+
 	/* Turn on HW trigger mode if supported */
 	if (sc->flags & HW_CTRL) {
 		ret = gdsc_hwctrl(sc, true);
@@ -241,6 +269,9 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 			return ret;
 	}
 
+	if (sc->clk_count)
+		gdsc_clk_disable(sc);
+
 	if (sc->pwrsts & PWRSTS_OFF)
 		gdsc_clear_mem_on(sc);
 
@@ -254,7 +285,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	return 0;
 }
 
-static int gdsc_init(struct gdsc *sc)
+static int gdsc_init(struct device *dev, struct gdsc *sc)
 {
 	u32 mask, val;
 	int on, ret;
@@ -284,6 +315,19 @@ static int gdsc_init(struct gdsc *sc)
 	if (on < 0)
 		return on;
 
+	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);
+	}
+
 	/*
 	 * 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 +371,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 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h |  8 ++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a4f3580..e9e7442 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -12,15 +12,19 @@
  */
 
 #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>
 #include <linux/kernel.h>
 #include <linux/ktime.h>
+#include <linux/pm_clock.h>
 #include <linux/pm_domain.h>
 #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 +170,27 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
 			   GMEM_CLAMP_IO_MASK, 1);
 }
 
+static 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)
+			pr_err("Failed to enable clock: %s\n",
+			       __clk_get_name(sc->clks[i]));
+	}
+	return ret;
+}
+
+static 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 +218,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	 */
 	udelay(1);
 
+	if (sc->clk_count)
+		gdsc_clk_enable(sc);
+
 	/* Turn on HW trigger mode if supported */
 	if (sc->flags & HW_CTRL) {
 		ret = gdsc_hwctrl(sc, true);
@@ -241,6 +269,9 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 			return ret;
 	}
 
+	if (sc->clk_count)
+		gdsc_clk_disable(sc);
+
 	if (sc->pwrsts & PWRSTS_OFF)
 		gdsc_clear_mem_on(sc);
 
@@ -254,7 +285,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	return 0;
 }
 
-static int gdsc_init(struct gdsc *sc)
+static int gdsc_init(struct device *dev, struct gdsc *sc)
 {
 	u32 mask, val;
 	int on, ret;
@@ -284,6 +315,19 @@ static int gdsc_init(struct gdsc *sc)
 	if (on < 0)
 		return on;
 
+	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);
+	}
+
 	/*
 	 * 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 +371,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 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 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 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 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index 9b97246..7c0ffb5 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -2893,6 +2893,10 @@ enum {
 	.pd = {
 		.name = "mmagic_bimc",
 	},
+	.clk_hws = {
+		&mmagic_bimc_noc_cfg_ahb_clk.clkr.hw,
+	},
+	.clk_count = 1,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -2902,6 +2906,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 +2923,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 +2940,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,
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 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 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index 9b97246..7c0ffb5 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -2893,6 +2893,10 @@ enum {
 	.pd = {
 		.name = "mmagic_bimc",
 	},
+	.clk_hws = {
+		&mmagic_bimc_noc_cfg_ahb_clk.clkr.hw,
+	},
+	.clk_count = 1,
 	.pwrsts = PWRSTS_OFF_ON,
 };
 
@@ -2902,6 +2906,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 +2923,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 +2940,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,
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

[Stanimir Varbanov]

Hi Rajendra,

Thanks for the patches!

On 03/21/2017 08:15 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/clk/qcom/gdsc.h |  8 ++++++++
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index a4f3580..e9e7442 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -12,15 +12,19 @@
>   */
>  
>  #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>
>  #include <linux/kernel.h>
>  #include <linux/ktime.h>
> +#include <linux/pm_clock.h>

this is not needed

>  #include <linux/pm_domain.h>
>  #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 +170,27 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
>  			   GMEM_CLAMP_IO_MASK, 1);
>  }
>  
> +static 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)
> +			pr_err("Failed to enable clock: %s\n",
> +			       __clk_get_name(sc->clks[i]));
		
I think the error message can be removed. And the already enabled clocks
should be disabled on error.

> +	}
> +	return ret;
> +}
> +
> +static 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 +218,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	 */
>  	udelay(1);
>  
> +	if (sc->clk_count)
> +		gdsc_clk_enable(sc);

could you add error handling.

> +
>  	/* Turn on HW trigger mode if supported */
>  	if (sc->flags & HW_CTRL) {
>  		ret = gdsc_hwctrl(sc, true);
> @@ -241,6 +269,9 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  			return ret;
>  	}
>  
> +	if (sc->clk_count)
> +		gdsc_clk_disable(sc);

IMO sc->clk_count check could be moved in gdsc_clk_disable. This is also
valid for all clk_count checks.

> +
>  	if (sc->pwrsts & PWRSTS_OFF)
>  		gdsc_clear_mem_on(sc);
>  
> @@ -254,7 +285,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  	return 0;
>  }
>  
> -static int gdsc_init(struct gdsc *sc)
> +static int gdsc_init(struct device *dev, struct gdsc *sc)
>  {
>  	u32 mask, val;
>  	int on, ret;
> @@ -284,6 +315,19 @@ static int gdsc_init(struct gdsc *sc)
>  	if (on < 0)
>  		return on;
>  
> +	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);

error handling?

Also I think it will be more readable if you above chunk in separate
function like gdsc_clk_get and call it unconditionally?

> +	}
> +

<snip>

-- 
regards,
Stan

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

[Stanimir Varbanov]

Hi Rajendra,

Thanks for the patches!

On 03/21/2017 08:15 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/clk/qcom/gdsc.h |  8 ++++++++
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index a4f3580..e9e7442 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -12,15 +12,19 @@
>   */
>  
>  #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>
>  #include <linux/kernel.h>
>  #include <linux/ktime.h>
> +#include <linux/pm_clock.h>

this is not needed

>  #include <linux/pm_domain.h>
>  #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 +170,27 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
>  			   GMEM_CLAMP_IO_MASK, 1);
>  }
>  
> +static 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)
> +			pr_err("Failed to enable clock: %s\n",
> +			       __clk_get_name(sc->clks[i]));
		
I think the error message can be removed. And the already enabled clocks
should be disabled on error.

> +	}
> +	return ret;
> +}
> +
> +static 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 +218,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	 */
>  	udelay(1);
>  
> +	if (sc->clk_count)
> +		gdsc_clk_enable(sc);

could you add error handling.

> +
>  	/* Turn on HW trigger mode if supported */
>  	if (sc->flags & HW_CTRL) {
>  		ret = gdsc_hwctrl(sc, true);
> @@ -241,6 +269,9 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  			return ret;
>  	}
>  
> +	if (sc->clk_count)
> +		gdsc_clk_disable(sc);

IMO sc->clk_count check could be moved in gdsc_clk_disable. This is also
valid for all clk_count checks.

> +
>  	if (sc->pwrsts & PWRSTS_OFF)
>  		gdsc_clear_mem_on(sc);
>  
> @@ -254,7 +285,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  	return 0;
>  }
>  
> -static int gdsc_init(struct gdsc *sc)
> +static int gdsc_init(struct device *dev, struct gdsc *sc)
>  {
>  	u32 mask, val;
>  	int on, ret;
> @@ -284,6 +315,19 @@ static int gdsc_init(struct gdsc *sc)
>  	if (on < 0)
>  		return on;
>  
> +	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);

error handling?

Also I think it will be more readable if you above chunk in separate
function like gdsc_clk_get and call it unconditionally?

> +	}
> +

<snip>

-- 
regards,
Stan

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

[Rajendra Nayak]

Hey Stan,

On 06/14/2017 05:10 PM, Stanimir Varbanov wrote:
> Hi Rajendra,
> 
> Thanks for the patches!

thanks for the review, do you plan to use this series to get rid of the mmagic
clock handling for vidc, or something else?
Is this a dependency to get your Video driver patches to work?
These patches have been on the list for a while and I had asked Stephen to leave
these out for now till we find someone using them.

I will fixup based on your review and repost in case its useful for something
else on the way upstream.

regards,
Rajendra

> 
> On 03/21/2017 08:15 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
>>  drivers/clk/qcom/gdsc.h |  8 ++++++++
>>  2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index a4f3580..e9e7442 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -12,15 +12,19 @@
>>   */
>>  
>>  #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>
>>  #include <linux/kernel.h>
>>  #include <linux/ktime.h>
>> +#include <linux/pm_clock.h>
> 
> this is not needed
> 
>>  #include <linux/pm_domain.h>
>>  #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 +170,27 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
>>  			   GMEM_CLAMP_IO_MASK, 1);
>>  }
>>  
>> +static 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)
>> +			pr_err("Failed to enable clock: %s\n",
>> +			       __clk_get_name(sc->clks[i]));
> 		
> I think the error message can be removed. And the already enabled clocks
> should be disabled on error.
> 
>> +	}
>> +	return ret;
>> +}
>> +
>> +static 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 +218,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>  	 */
>>  	udelay(1);
>>  
>> +	if (sc->clk_count)
>> +		gdsc_clk_enable(sc);
> 
> could you add error handling.
> 
>> +
>>  	/* Turn on HW trigger mode if supported */
>>  	if (sc->flags & HW_CTRL) {
>>  		ret = gdsc_hwctrl(sc, true);
>> @@ -241,6 +269,9 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>  			return ret;
>>  	}
>>  
>> +	if (sc->clk_count)
>> +		gdsc_clk_disable(sc);
> 
> IMO sc->clk_count check could be moved in gdsc_clk_disable. This is also
> valid for all clk_count checks.
> 
>> +
>>  	if (sc->pwrsts & PWRSTS_OFF)
>>  		gdsc_clear_mem_on(sc);
>>  
>> @@ -254,7 +285,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>  	return 0;
>>  }
>>  
>> -static int gdsc_init(struct gdsc *sc)
>> +static int gdsc_init(struct device *dev, struct gdsc *sc)
>>  {
>>  	u32 mask, val;
>>  	int on, ret;
>> @@ -284,6 +315,19 @@ static int gdsc_init(struct gdsc *sc)
>>  	if (on < 0)
>>  		return on;
>>  
>> +	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);
> 
> error handling?
> 
> Also I think it will be more readable if you above chunk in separate
> function like gdsc_clk_get and call it unconditionally?
> 
>> +	}
>> +
> 
> <snip>
> 

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

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

[Rajendra Nayak]

Hey Stan,

On 06/14/2017 05:10 PM, Stanimir Varbanov wrote:
> Hi Rajendra,
> 
> Thanks for the patches!

thanks for the review, do you plan to use this series to get rid of the mmagic
clock handling for vidc, or something else?
Is this a dependency to get your Video driver patches to work?
These patches have been on the list for a while and I had asked Stephen to leave
these out for now till we find someone using them.

I will fixup based on your review and repost in case its useful for something
else on the way upstream.

regards,
Rajendra

> 
> On 03/21/2017 08:15 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
>>  drivers/clk/qcom/gdsc.h |  8 ++++++++
>>  2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index a4f3580..e9e7442 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -12,15 +12,19 @@
>>   */
>>  
>>  #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>
>>  #include <linux/kernel.h>
>>  #include <linux/ktime.h>
>> +#include <linux/pm_clock.h>
> 
> this is not needed
> 
>>  #include <linux/pm_domain.h>
>>  #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 +170,27 @@ static inline void gdsc_assert_clamp_io(struct gdsc *sc)
>>  			   GMEM_CLAMP_IO_MASK, 1);
>>  }
>>  
>> +static 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)
>> +			pr_err("Failed to enable clock: %s\n",
>> +			       __clk_get_name(sc->clks[i]));
> 		
> I think the error message can be removed. And the already enabled clocks
> should be disabled on error.
> 
>> +	}
>> +	return ret;
>> +}
>> +
>> +static 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 +218,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>  	 */
>>  	udelay(1);
>>  
>> +	if (sc->clk_count)
>> +		gdsc_clk_enable(sc);
> 
> could you add error handling.
> 
>> +
>>  	/* Turn on HW trigger mode if supported */
>>  	if (sc->flags & HW_CTRL) {
>>  		ret = gdsc_hwctrl(sc, true);
>> @@ -241,6 +269,9 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>  			return ret;
>>  	}
>>  
>> +	if (sc->clk_count)
>> +		gdsc_clk_disable(sc);
> 
> IMO sc->clk_count check could be moved in gdsc_clk_disable. This is also
> valid for all clk_count checks.
> 
>> +
>>  	if (sc->pwrsts & PWRSTS_OFF)
>>  		gdsc_clear_mem_on(sc);
>>  
>> @@ -254,7 +285,7 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>  	return 0;
>>  }
>>  
>> -static int gdsc_init(struct gdsc *sc)
>> +static int gdsc_init(struct device *dev, struct gdsc *sc)
>>  {
>>  	u32 mask, val;
>>  	int on, ret;
>> @@ -284,6 +315,19 @@ static int gdsc_init(struct gdsc *sc)
>>  	if (on < 0)
>>  		return on;
>>  
>> +	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);
> 
> error handling?
> 
> Also I think it will be more readable if you above chunk in separate
> function like gdsc_clk_get and call it unconditionally?
> 
>> +	}
>> +
> 
> <snip>
> 

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

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

[Stanimir Varbanov]

Hi Rajendra,

On 06/21/2017 09:11 AM, Rajendra Nayak wrote:
> Hey Stan,
> 
> On 06/14/2017 05:10 PM, Stanimir Varbanov wrote:
>> Hi Rajendra,
>>
>> Thanks for the patches!
> 
> thanks for the review, do you plan to use this series to get rid of the mmagic
> clock handling for vidc, or something else?

yes, I have used those patches in the linaro integration branch based on
4.11. So far those patches are working as expected.

> Is this a dependency to get your Video driver patches to work?
> These patches have been on the list for a while and I had asked Stephen to leave
> these out for now till we find someone using them.
> 
> I will fixup based on your review and repost in case its useful for something
> else on the way upstream.

yes, please do.

-- 
regards,
Stan

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

[Stanimir Varbanov]

Hi Rajendra,

On 06/21/2017 09:11 AM, Rajendra Nayak wrote:
> Hey Stan,
> 
> On 06/14/2017 05:10 PM, Stanimir Varbanov wrote:
>> Hi Rajendra,
>>
>> Thanks for the patches!
> 
> thanks for the review, do you plan to use this series to get rid of the mmagic
> clock handling for vidc, or something else?

yes, I have used those patches in the linaro integration branch based on
4.11. So far those patches are working as expected.

> Is this a dependency to get your Video driver patches to work?
> These patches have been on the list for a while and I had asked Stephen to leave
> these out for now till we find someone using them.
> 
> I will fixup based on your review and repost in case its useful for something
> else on the way upstream.

yes, please do.

-- 
regards,
Stan