diff mbox

[v6,03/13] clk: qcom: gdsc: Use PM clocks to control gdsc clocks

Message ID 1437549069-29655-4-git-send-email-rnayak@codeaurora.org (mailing list archive)
State New, archived
Delegated to: Andy Gross
Headers show

Commit Message

Rajendra Nayak July 22, 2015, 7:10 a.m. UTC
The devices within a gdsc power domain, quite often have additional
clocks to be turned on/off along with the power domain itself.
Once the drivers for these devices are converted to use runtime PM,
it would be possible to remove all clock handling from the drivers if
the gdsc driver can handle it.
Use PM clocks to add support for this. A list of con_ids[] specified
per gdsc would be the clocks turned on/off on every device start/stop
callbacks.

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

Comments

Stephen Boyd July 23, 2015, 1:01 a.m. UTC | #1
On 07/22/2015 12:10 AM, Rajendra Nayak wrote:
> @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>   	return gdsc_toggle_logic(sc, false);
>   }
>   
> +static int gdsc_attach(struct generic_pm_domain *domain, struct device *dev)
> +{
> +	int ret;
> +	struct gdsc *sc = domain_to_gdsc(domain);
> +	char **con_id, *con_ids[] = { "core", "iface", NULL };

const?

This is where I get scared of sniffing too much SoC glue. What's to 
enforce the "core", and "iface" naming scheme? What's to enforce there 
being two clocks vs. one? Maybe a better approach would be to use 
of_clk_get() and iterate through all clocks of the device, or to encode 
the clock names in the gdsc structure.

The problem I'm getting at is that we're going through the consumer 
struct device's mapping of names to clks when we're inside this SoC glue 
code that has to know about what the consumer has decided to do. This 
code becomes tightly coupled with that decision that doesn't seem to be 
under the glue code's control. Using of_clk_get() sidesteps that 
problem, with the loss of flexibility of deciding which clock does what 
so at least it's a step in the right direction. But if we want to 
control individual clocks then we have to know which clock is which. 
Maybe we should associate clk_hw pointers with the gdscs and then export 
__clk_hw_create_clk() so that drivers can turn clk_hw pointers into clocks?

> +
> +	ret = pm_clk_create(dev);
> +	if (ret) {
> +		dev_err(dev, "pm_clk_create failed %d\n", ret);

Who cares? debug?

> +		return ret;
> +	}
> +
> +	for (con_id = con_ids; *con_id; con_id++) {
> +		ret = pm_clk_add(dev, *con_id);
> +		if (ret) {
> +			dev_err(dev, "pm_clk_add failed %d\n", ret);

Who cares? debug?

> +			goto fail;
> +		}
> +	}
> +	return 0;
> +fail:
> +	pm_clk_destroy(dev);
> +	return ret;
> +};
> +
> +static void gdsc_detach(struct generic_pm_domain *domain, struct device *dev)
> +{
> +	pm_clk_destroy(dev);
> +	return;

useless return
Rajendra Nayak July 23, 2015, 8:34 a.m. UTC | #2
On 07/23/2015 06:31 AM, Stephen Boyd wrote:
> On 07/22/2015 12:10 AM, Rajendra Nayak wrote:
>> @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain
>> *domain)
>>       return gdsc_toggle_logic(sc, false);
>>   }
>> +static int gdsc_attach(struct generic_pm_domain *domain, struct
>> device *dev)
>> +{
>> +    int ret;
>> +    struct gdsc *sc = domain_to_gdsc(domain);
>> +    char **con_id, *con_ids[] = { "core", "iface", NULL };
>
> const?
>
> This is where I get scared of sniffing too much SoC glue. What's to
> enforce the "core", and "iface" naming scheme? What's to enforce there
> being two clocks vs. one? Maybe a better approach would be to use
> of_clk_get() and iterate through all clocks of the device, or to encode
> the clock names in the gdsc structure.

I had the clock names in the gdsc structure in v5. I should probably go
back to having it that way.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanimir Varbanov July 23, 2015, 9:22 a.m. UTC | #3
On 07/23/2015 11:34 AM, Rajendra Nayak wrote:
> On 07/23/2015 06:31 AM, Stephen Boyd wrote:
>> On 07/22/2015 12:10 AM, Rajendra Nayak wrote:
>>> @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain
>>> *domain)
>>>       return gdsc_toggle_logic(sc, false);
>>>   }
>>> +static int gdsc_attach(struct generic_pm_domain *domain, struct
>>> device *dev)
>>> +{
>>> +    int ret;
>>> +    struct gdsc *sc = domain_to_gdsc(domain);
>>> +    char **con_id, *con_ids[] = { "core", "iface", NULL };
>>
>> const?
>>
>> This is where I get scared of sniffing too much SoC glue. What's to
>> enforce the "core", and "iface" naming scheme? What's to enforce there
>> being two clocks vs. one? Maybe a better approach would be to use
>> of_clk_get() and iterate through all clocks of the device, or to encode
>> the clock names in the gdsc structure.
> 
> I had the clock names in the gdsc structure in v5. I should probably go
> back to having it that way.

Rajendra, If you decide to go back please look at that comment [1], as well.
Rajendra Nayak July 23, 2015, 10:28 a.m. UTC | #4
On 07/23/2015 02:52 PM, Stanimir Varbanov wrote:
> On 07/23/2015 11:34 AM, Rajendra Nayak wrote:
>> On 07/23/2015 06:31 AM, Stephen Boyd wrote:
>>> On 07/22/2015 12:10 AM, Rajendra Nayak wrote:
>>>> @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain
>>>> *domain)
>>>>        return gdsc_toggle_logic(sc, false);
>>>>    }
>>>> +static int gdsc_attach(struct generic_pm_domain *domain, struct
>>>> device *dev)
>>>> +{
>>>> +    int ret;
>>>> +    struct gdsc *sc = domain_to_gdsc(domain);
>>>> +    char **con_id, *con_ids[] = { "core", "iface", NULL };
>>>
>>> const?
>>>
>>> This is where I get scared of sniffing too much SoC glue. What's to
>>> enforce the "core", and "iface" naming scheme? What's to enforce there
>>> being two clocks vs. one? Maybe a better approach would be to use
>>> of_clk_get() and iterate through all clocks of the device, or to encode
>>> the clock names in the gdsc structure.
>>
>> I had the clock names in the gdsc structure in v5. I should probably go
>> back to having it that way.
>
> Rajendra, If you decide to go back please look at that comment [1], as well.

yes, thanks, I will.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a59655b..3125809 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -14,6 +14,7 @@ 
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/jiffies.h>
+#include <linux/pm_clock.h>
 #include <linux/slab.h>
 #include "gdsc.h"
 
@@ -104,6 +105,37 @@  static int gdsc_disable(struct generic_pm_domain *domain)
 	return gdsc_toggle_logic(sc, false);
 }
 
+static int gdsc_attach(struct generic_pm_domain *domain, struct device *dev)
+{
+	int ret;
+	struct gdsc *sc = domain_to_gdsc(domain);
+	char **con_id, *con_ids[] = { "core", "iface", NULL };
+
+	ret = pm_clk_create(dev);
+	if (ret) {
+		dev_err(dev, "pm_clk_create failed %d\n", ret);
+		return ret;
+	}
+
+	for (con_id = con_ids; *con_id; con_id++) {
+		ret = pm_clk_add(dev, *con_id);
+		if (ret) {
+			dev_err(dev, "pm_clk_add failed %d\n", ret);
+			goto fail;
+		}
+	}
+	return 0;
+fail:
+	pm_clk_destroy(dev);
+	return ret;
+};
+
+static void gdsc_detach(struct generic_pm_domain *domain, struct device *dev)
+{
+	pm_clk_destroy(dev);
+	return;
+};
+
 static int gdsc_init(struct gdsc *sc)
 {
 	u32 mask, val;
@@ -127,6 +159,9 @@  static int gdsc_init(struct gdsc *sc)
 
 	sc->pd.power_off = gdsc_disable;
 	sc->pd.power_on = gdsc_enable;
+	sc->pd.attach_dev = gdsc_attach;
+	sc->pd.detach_dev = gdsc_detach;
+	sc->pd.flags = GENPD_FLAG_PM_CLK;
 	pm_genpd_init(&sc->pd, NULL, !on);
 
 	return 0;