[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 <***@arm.com>
Signed-off-by: Rajendra Nayak <***@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.

[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 <***@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);

[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 <***@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 {

[Stanimir Varbanov]

Hi Rajendra,

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

Post by 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.
---
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.

Post by Rajendra Nayak
+{
+ 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.

Post by Rajendra Nayak
+ 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.

Post by Rajendra Nayak
+}
+
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;

Post by Rajendra Nayak
+ 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

Post by Rajendra Nayak
+ return 0;
+}
+

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

[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 <***@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,
},
},

[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 <***@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,
};