* [PATCH v3 1/5] PM / clock_ops: Add pm_clk_add_clk()
2014-10-23 14:22 [PATCH v3 0/5] ARM: keystone: pm: switch to use generic pm domains Grygorii Strashko
@ 2014-10-23 14:22 ` Grygorii Strashko
2014-10-23 15:49 ` Dmitry Torokhov
2014-10-23 14:22 ` [PATCH v3 2/5] PM / clock_ops: make __pm_clk_enable more generic Grygorii Strashko
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Grygorii Strashko @ 2014-10-23 14:22 UTC (permalink / raw)
To: ssantosh, Rafael J. Wysocki, khilman
Cc: linux-pm, Rob Herring, grant.likely, linux-arm-kernel,
linux-kernel, devicetree, Geert Uytterhoeven, Ulf Hansson,
Dmitry Torokhov, Grygorii Strashko
From: Geert Uytterhoeven <geert+renesas@glider.be>
The existing pm_clk_add() allows to pass a clock by con_id. However,
when referring to a specific clock from DT, no con_id is available.
Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
include/linux/pm_clock.h | 8 ++++++++
2 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 7836930..f14b767 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
*/
static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
{
- ce->clk = clk_get(dev, ce->con_id);
+ if (!ce->clk)
+ ce->clk = clk_get(dev, ce->con_id);
if (IS_ERR(ce->clk)) {
ce->status = PCE_STATUS_ERROR;
} else {
@@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
}
}
-/**
- * pm_clk_add - Start using a device clock for power management.
- * @dev: Device whose clock is going to be used for power management.
- * @con_id: Connection ID of the clock.
- *
- * Add the clock represented by @con_id to the list of clocks used for
- * the power management of @dev.
- */
-int pm_clk_add(struct device *dev, const char *con_id)
+static int __pm_clk_add(struct device *dev, const char *con_id,
+ struct clk *clk)
{
struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
@@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
kfree(ce);
return -ENOMEM;
}
+ } else {
+ ce->clk = clk;
}
pm_clk_acquire(dev, ce);
@@ -104,6 +100,31 @@ int pm_clk_add(struct device *dev, const char *con_id)
}
/**
+ * pm_clk_add - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @con_id: Connection ID of the clock.
+ *
+ * Add the clock represented by @con_id to the list of clocks used for
+ * the power management of @dev.
+ */
+int pm_clk_add(struct device *dev, const char *con_id)
+{
+ return __pm_clk_add(dev, con_id, NULL);
+}
+
+/**
+ * pm_clk_add_clk - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @clk: Clock pointer
+ *
+ * Add the clock to the list of clocks used for the power management of @dev.
+ */
+int pm_clk_add_clk(struct device *dev, struct clk *clk)
+{
+ return __pm_clk_add(dev, NULL, clk);
+}
+
+/**
* __pm_clk_remove - Destroy PM clock entry.
* @ce: PM clock entry to destroy.
*/
diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h
index 8348866..0b00396 100644
--- a/include/linux/pm_clock.h
+++ b/include/linux/pm_clock.h
@@ -18,6 +18,8 @@ struct pm_clk_notifier_block {
char *con_ids[];
};
+struct clk;
+
#ifdef CONFIG_PM_CLK
static inline bool pm_clk_no_clocks(struct device *dev)
{
@@ -29,6 +31,7 @@ extern void pm_clk_init(struct device *dev);
extern int pm_clk_create(struct device *dev);
extern void pm_clk_destroy(struct device *dev);
extern int pm_clk_add(struct device *dev, const char *con_id);
+extern int pm_clk_add_clk(struct device *dev, struct clk *clk);
extern void pm_clk_remove(struct device *dev, const char *con_id);
extern int pm_clk_suspend(struct device *dev);
extern int pm_clk_resume(struct device *dev);
@@ -51,6 +54,11 @@ static inline int pm_clk_add(struct device *dev, const char *con_id)
{
return -EINVAL;
}
+
+static inline int pm_clk_add_clk(struct device *dev, struct clk *clk)
+{
+ return -EINVAL;
+}
static inline void pm_clk_remove(struct device *dev, const char *con_id)
{
}
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/5] PM / clock_ops: Add pm_clk_add_clk()
2014-10-23 14:22 ` [PATCH v3 1/5] PM / clock_ops: Add pm_clk_add_clk() Grygorii Strashko
@ 2014-10-23 15:49 ` Dmitry Torokhov
2014-10-23 16:18 ` Grygorii Strashko
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2014-10-23 15:49 UTC (permalink / raw)
To: Grygorii Strashko
Cc: ssantosh, Rafael J. Wysocki, khilman, linux-pm, Rob Herring,
grant.likely, linux-arm-kernel, linux-kernel, devicetree,
Geert Uytterhoeven, Ulf Hansson
On Thu, Oct 23, 2014 at 05:22:58PM +0300, Grygorii Strashko wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
>
> The existing pm_clk_add() allows to pass a clock by con_id. However,
> when referring to a specific clock from DT, no con_id is available.
>
> Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
>
> CC: Ulf Hansson <ulf.hansson@linaro.org>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
> include/linux/pm_clock.h | 8 ++++++++
> 2 files changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> index 7836930..f14b767 100644
> --- a/drivers/base/power/clock_ops.c
> +++ b/drivers/base/power/clock_ops.c
> @@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
> */
> static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> {
> - ce->clk = clk_get(dev, ce->con_id);
> + if (!ce->clk)
> + ce->clk = clk_get(dev, ce->con_id);
> if (IS_ERR(ce->clk)) {
> ce->status = PCE_STATUS_ERROR;
> } else {
> @@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> }
> }
>
> -/**
> - * pm_clk_add - Start using a device clock for power management.
> - * @dev: Device whose clock is going to be used for power management.
> - * @con_id: Connection ID of the clock.
> - *
> - * Add the clock represented by @con_id to the list of clocks used for
> - * the power management of @dev.
> - */
> -int pm_clk_add(struct device *dev, const char *con_id)
> +static int __pm_clk_add(struct device *dev, const char *con_id,
> + struct clk *clk)
> {
> struct pm_subsys_data *psd = dev_to_psd(dev);
> struct pm_clock_entry *ce;
> @@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
> kfree(ce);
> return -ENOMEM;
> }
> + } else {
You need __clk_get(clk) here, otherwise when you will be removing clocks
you'll be dropping a reference that does not belong to you (as yiou have
not taken it).
> + ce->clk = clk;
> }
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/5] PM / clock_ops: Add pm_clk_add_clk()
2014-10-23 15:49 ` Dmitry Torokhov
@ 2014-10-23 16:18 ` Grygorii Strashko
2014-10-23 17:13 ` Dmitry Torokhov
0 siblings, 1 reply; 12+ messages in thread
From: Grygorii Strashko @ 2014-10-23 16:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: ssantosh, Rafael J. Wysocki, khilman, linux-pm, Rob Herring,
grant.likely, linux-arm-kernel, linux-kernel, devicetree,
Geert Uytterhoeven, Ulf Hansson
On 10/23/2014 06:49 PM, Dmitry Torokhov wrote:
> On Thu, Oct 23, 2014 at 05:22:58PM +0300, Grygorii Strashko wrote:
>> From: Geert Uytterhoeven <geert+renesas@glider.be>
>>
>> The existing pm_clk_add() allows to pass a clock by con_id. However,
>> when referring to a specific clock from DT, no con_id is available.
>>
>> Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
>>
>> CC: Ulf Hansson <ulf.hansson@linaro.org>
>> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Reviewed-by: Kevin Hilman <khilman@linaro.org>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
>> include/linux/pm_clock.h | 8 ++++++++
>> 2 files changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
>> index 7836930..f14b767 100644
>> --- a/drivers/base/power/clock_ops.c
>> +++ b/drivers/base/power/clock_ops.c
>> @@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
>> */
>> static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>> {
>> - ce->clk = clk_get(dev, ce->con_id);
>> + if (!ce->clk)
>> + ce->clk = clk_get(dev, ce->con_id);
>> if (IS_ERR(ce->clk)) {
>> ce->status = PCE_STATUS_ERROR;
>> } else {
>> @@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>> }
>> }
>>
>> -/**
>> - * pm_clk_add - Start using a device clock for power management.
>> - * @dev: Device whose clock is going to be used for power management.
>> - * @con_id: Connection ID of the clock.
>> - *
>> - * Add the clock represented by @con_id to the list of clocks used for
>> - * the power management of @dev.
>> - */
>> -int pm_clk_add(struct device *dev, const char *con_id)
>> +static int __pm_clk_add(struct device *dev, const char *con_id,
>> + struct clk *clk)
>> {
>> struct pm_subsys_data *psd = dev_to_psd(dev);
>> struct pm_clock_entry *ce;
>> @@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
>> kfree(ce);
>> return -ENOMEM;
>> }
>> + } else {
>
> You need __clk_get(clk) here, otherwise when you will be removing clocks
> you'll be dropping a reference that does not belong to you (as yiou have
> not taken it).
No, I don't ;)
pm_clk_add_clk is intended to be used with valid pointer on clk,
which means clk_get() has been called already. In our case:
clk = of_clk_get(dev->of_node, i));
ret = pm_clk_add_clk(dev, clk);
It's just a different way to fill list of clocks for device,
once pm_clk_add_clk() is called - PM clock framework will be the owner
of clock.
>
>> + ce->clk = clk;
>> }
>>
>
regards,
-grygorii
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/5] PM / clock_ops: Add pm_clk_add_clk()
2014-10-23 16:18 ` Grygorii Strashko
@ 2014-10-23 17:13 ` Dmitry Torokhov
2014-10-23 18:04 ` Grygorii Strashko
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2014-10-23 17:13 UTC (permalink / raw)
To: Grygorii Strashko
Cc: ssantosh, Rafael J. Wysocki, khilman, linux-pm, Rob Herring,
grant.likely, linux-arm-kernel, linux-kernel, devicetree,
Geert Uytterhoeven, Ulf Hansson
On Thu, Oct 23, 2014 at 07:18:49PM +0300, Grygorii Strashko wrote:
> On 10/23/2014 06:49 PM, Dmitry Torokhov wrote:
> >On Thu, Oct 23, 2014 at 05:22:58PM +0300, Grygorii Strashko wrote:
> >>From: Geert Uytterhoeven <geert+renesas@glider.be>
> >>
> >>The existing pm_clk_add() allows to pass a clock by con_id. However,
> >>when referring to a specific clock from DT, no con_id is available.
> >>
> >>Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
> >>
> >>CC: Ulf Hansson <ulf.hansson@linaro.org>
> >>CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >>Reviewed-by: Kevin Hilman <khilman@linaro.org>
> >>Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >>Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >>---
> >> drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
> >> include/linux/pm_clock.h | 8 ++++++++
> >> 2 files changed, 39 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> >>index 7836930..f14b767 100644
> >>--- a/drivers/base/power/clock_ops.c
> >>+++ b/drivers/base/power/clock_ops.c
> >>@@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
> >> */
> >> static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> >> {
> >>- ce->clk = clk_get(dev, ce->con_id);
> >>+ if (!ce->clk)
> >>+ ce->clk = clk_get(dev, ce->con_id);
> >> if (IS_ERR(ce->clk)) {
> >> ce->status = PCE_STATUS_ERROR;
> >> } else {
> >>@@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> >> }
> >> }
> >>
> >>-/**
> >>- * pm_clk_add - Start using a device clock for power management.
> >>- * @dev: Device whose clock is going to be used for power management.
> >>- * @con_id: Connection ID of the clock.
> >>- *
> >>- * Add the clock represented by @con_id to the list of clocks used for
> >>- * the power management of @dev.
> >>- */
> >>-int pm_clk_add(struct device *dev, const char *con_id)
> >>+static int __pm_clk_add(struct device *dev, const char *con_id,
> >>+ struct clk *clk)
> >> {
> >> struct pm_subsys_data *psd = dev_to_psd(dev);
> >> struct pm_clock_entry *ce;
> >>@@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
> >> kfree(ce);
> >> return -ENOMEM;
> >> }
> >>+ } else {
> >
> >You need __clk_get(clk) here, otherwise when you will be removing clocks
> >you'll be dropping a reference that does not belong to you (as yiou have
> >not taken it).
>
> No, I don't ;)
>
> pm_clk_add_clk is intended to be used with valid pointer on clk,
> which means clk_get() has been called already. In our case:
> clk = of_clk_get(dev->of_node, i));
> ret = pm_clk_add_clk(dev, clk);
>
> It's just a different way to fill list of clocks for device,
> once pm_clk_add_clk() is called - PM clock framework will be the owner
> of clock.
No, this is not how the rest of refcounted APIs work in the kernel. You
do not take over other's references and if you need to store pointer to
refcounted object you are responsible for taking your own reference. See
how we work with device structures, OF nodes, etc, etc.
The proper pattern here should be (unless the driver wants to hold on to
the clock for its own purposes):
clk = of_clk_get(dev->of_node, i));
ret = pm_clk_add_clk(dev, clk);
clk_put(clk);
with pm_clk_add_clk() taking and managing _its own_ reference and the
caller (driver) managing _its_ reference.
So FWIW strong NAK here from me.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/5] PM / clock_ops: Add pm_clk_add_clk()
2014-10-23 17:13 ` Dmitry Torokhov
@ 2014-10-23 18:04 ` Grygorii Strashko
0 siblings, 0 replies; 12+ messages in thread
From: Grygorii Strashko @ 2014-10-23 18:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: ssantosh, Rafael J. Wysocki, khilman, linux-pm, Rob Herring,
grant.likely, linux-arm-kernel, linux-kernel, devicetree,
Geert Uytterhoeven, Ulf Hansson
On 10/23/2014 08:13 PM, Dmitry Torokhov wrote:
> On Thu, Oct 23, 2014 at 07:18:49PM +0300, Grygorii Strashko wrote:
>> On 10/23/2014 06:49 PM, Dmitry Torokhov wrote:
>>> On Thu, Oct 23, 2014 at 05:22:58PM +0300, Grygorii Strashko wrote:
>>>> From: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>
>>>> The existing pm_clk_add() allows to pass a clock by con_id. However,
>>>> when referring to a specific clock from DT, no con_id is available.
>>>>
>>>> Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
>>>>
>>>> CC: Ulf Hansson <ulf.hansson@linaro.org>
>>>> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>> Reviewed-by: Kevin Hilman <khilman@linaro.org>
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>>> ---
>>>> drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
>>>> include/linux/pm_clock.h | 8 ++++++++
>>>> 2 files changed, 39 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
>>>> index 7836930..f14b767 100644
>>>> --- a/drivers/base/power/clock_ops.c
>>>> +++ b/drivers/base/power/clock_ops.c
>>>> @@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
>>>> */
>>>> static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>>>> {
>>>> - ce->clk = clk_get(dev, ce->con_id);
>>>> + if (!ce->clk)
>>>> + ce->clk = clk_get(dev, ce->con_id);
>>>> if (IS_ERR(ce->clk)) {
>>>> ce->status = PCE_STATUS_ERROR;
>>>> } else {
>>>> @@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>>>> }
>>>> }
>>>>
>>>> -/**
>>>> - * pm_clk_add - Start using a device clock for power management.
>>>> - * @dev: Device whose clock is going to be used for power management.
>>>> - * @con_id: Connection ID of the clock.
>>>> - *
>>>> - * Add the clock represented by @con_id to the list of clocks used for
>>>> - * the power management of @dev.
>>>> - */
>>>> -int pm_clk_add(struct device *dev, const char *con_id)
>>>> +static int __pm_clk_add(struct device *dev, const char *con_id,
>>>> + struct clk *clk)
>>>> {
>>>> struct pm_subsys_data *psd = dev_to_psd(dev);
>>>> struct pm_clock_entry *ce;
>>>> @@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
>>>> kfree(ce);
>>>> return -ENOMEM;
>>>> }
>>>> + } else {
>>>
>>> You need __clk_get(clk) here, otherwise when you will be removing clocks
>>> you'll be dropping a reference that does not belong to you (as yiou have
>>> not taken it).
>>
>> No, I don't ;)
>>
>> pm_clk_add_clk is intended to be used with valid pointer on clk,
>> which means clk_get() has been called already. In our case:
>> clk = of_clk_get(dev->of_node, i));
>> ret = pm_clk_add_clk(dev, clk);
>>
>> It's just a different way to fill list of clocks for device,
>> once pm_clk_add_clk() is called - PM clock framework will be the owner
>> of clock.
>
> No, this is not how the rest of refcounted APIs work in the kernel. You
> do not take over other's references and if you need to store pointer to
> refcounted object you are responsible for taking your own reference. See
> how we work with device structures, OF nodes, etc, etc.
>
> The proper pattern here should be (unless the driver wants to hold on to
> the clock for its own purposes):
>
> clk = of_clk_get(dev->of_node, i));
> ret = pm_clk_add_clk(dev, clk);
> clk_put(clk);
>
> with pm_clk_add_clk() taking and managing _its own_ reference and the
> caller (driver) managing _its_ reference.
>
> So FWIW strong NAK here from me.
Ok. I'll update and resend.
regards,
-grygorii
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 2/5] PM / clock_ops: make __pm_clk_enable more generic
2014-10-23 14:22 [PATCH v3 0/5] ARM: keystone: pm: switch to use generic pm domains Grygorii Strashko
2014-10-23 14:22 ` [PATCH v3 1/5] PM / clock_ops: Add pm_clk_add_clk() Grygorii Strashko
@ 2014-10-23 14:22 ` Grygorii Strashko
2014-10-23 14:23 ` [PATCH v3 3/5] PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME Grygorii Strashko
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Grygorii Strashko @ 2014-10-23 14:22 UTC (permalink / raw)
To: ssantosh, Rafael J. Wysocki, khilman
Cc: linux-pm, Rob Herring, grant.likely, linux-arm-kernel,
linux-kernel, devicetree, Grygorii Strashko, Ulf Hansson,
Geert Uytterhoeven, Dmitry Torokhov
Now there are two places in code which do the same things,
so allow __pm_clk_enable() to accept pointer on pm_clock_entry
structure as second parameter instead of pointer on clock and
remove duplicated code.
Also, updated function intended to be used by the following patch.
CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Kevin Hilman <khilman@linaro.org>
CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/base/power/clock_ops.c | 38 ++++++++++++++++----------------------
1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index f14b767..c3b44c1 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -34,14 +34,20 @@ struct pm_clock_entry {
/**
* pm_clk_enable - Enable a clock, reporting any errors
* @dev: The device for the given clock
- * @clk: The clock being enabled.
+ * @ce: PM clock entry corresponding to the clock.
*/
-static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
+static inline int __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce)
{
- int ret = clk_enable(clk);
- if (ret)
- dev_err(dev, "%s: failed to enable clk %p, error %d\n",
- __func__, clk, ret);
+ int ret;
+
+ if (ce->status < PCE_STATUS_ERROR) {
+ ret = clk_enable(ce->clk);
+ if (!ret)
+ ce->status = PCE_STATUS_ENABLED;
+ else
+ dev_err(dev, "%s: failed to enable clk %p, error %d\n",
+ __func__, ce->clk, ret);
+ }
return ret;
}
@@ -287,7 +293,6 @@ int pm_clk_resume(struct device *dev)
struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
unsigned long flags;
- int ret;
dev_dbg(dev, "%s()\n", __func__);
@@ -296,13 +301,8 @@ int pm_clk_resume(struct device *dev)
spin_lock_irqsave(&psd->lock, flags);
- list_for_each_entry(ce, &psd->clock_list, node) {
- if (ce->status < PCE_STATUS_ERROR) {
- ret = __pm_clk_enable(dev, ce->clk);
- if (!ret)
- ce->status = PCE_STATUS_ENABLED;
- }
- }
+ list_for_each_entry(ce, &psd->clock_list, node)
+ __pm_clk_enable(dev, ce);
spin_unlock_irqrestore(&psd->lock, flags);
@@ -411,7 +411,6 @@ int pm_clk_resume(struct device *dev)
struct pm_subsys_data *psd = dev_to_psd(dev);
struct pm_clock_entry *ce;
unsigned long flags;
- int ret;
dev_dbg(dev, "%s()\n", __func__);
@@ -421,13 +420,8 @@ int pm_clk_resume(struct device *dev)
spin_lock_irqsave(&psd->lock, flags);
- list_for_each_entry(ce, &psd->clock_list, node) {
- if (ce->status < PCE_STATUS_ERROR) {
- ret = __pm_clk_enable(dev, ce->clk);
- if (!ret)
- ce->status = PCE_STATUS_ENABLED;
- }
- }
+ list_for_each_entry(ce, &psd->clock_list, node)
+ __pm_clk_enable(dev, ce);
spin_unlock_irqrestore(&psd->lock, flags);
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v3 3/5] PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME
2014-10-23 14:22 [PATCH v3 0/5] ARM: keystone: pm: switch to use generic pm domains Grygorii Strashko
2014-10-23 14:22 ` [PATCH v3 1/5] PM / clock_ops: Add pm_clk_add_clk() Grygorii Strashko
2014-10-23 14:22 ` [PATCH v3 2/5] PM / clock_ops: make __pm_clk_enable more generic Grygorii Strashko
@ 2014-10-23 14:23 ` Grygorii Strashko
2014-10-23 14:23 ` [PATCH v3 4/5] ARM: keystone: pm: switch to use generic pm domains Grygorii Strashko
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Grygorii Strashko @ 2014-10-23 14:23 UTC (permalink / raw)
To: ssantosh, Rafael J. Wysocki, khilman
Cc: linux-pm, Rob Herring, grant.likely, linux-arm-kernel,
linux-kernel, devicetree, Grygorii Strashko, Ulf Hansson,
Geert Uytterhoeven, Dmitry Torokhov
Device's clocks need to be enabled always at probing time
if !CONFIG_PM_RUNTIME - in that way, device will become accessible
and, later, its clocks can be disabled/enabled during system
suspend/resume by using pm_clk_suspend/pm_clk_resume APIs.
But now, the clocks management code doesn't enable clocks when
clocks are being registered per-device by using pm_clk_add/_clk().
So, update pm_clk_acquire(), which is called from pm_clk_add/_clk(),
to enable clocks always if !CONFIG_PM_RUNTIME.
Also, Platform PM domains drivers will not need to add code like below
each time they need to handle the case when !CONFIG_PM_RUNTIME [1 - 3]:
if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
ret = pm_clk_resume(dev);
if (ret) {
dev_err(dev, "pm_clk_resume failed %d\n", ret);
goto clk_err;
};
}
[1] https://lkml.org/lkml/2014/10/3/306
[2] https://lkml.org/lkml/2014/10/16/305
[3] https://lkml.org/lkml/2014/10/20/249
CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Kevin Hilman <khilman@linaro.org>
CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/base/power/clock_ops.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index c3b44c1..87d07dc 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -68,6 +68,10 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
ce->status = PCE_STATUS_ACQUIRED;
dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
}
+
+ if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
+ __pm_clk_enable(dev, ce);
+ }
}
static int __pm_clk_add(struct device *dev, const char *con_id,
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v3 4/5] ARM: keystone: pm: switch to use generic pm domains
2014-10-23 14:22 [PATCH v3 0/5] ARM: keystone: pm: switch to use generic pm domains Grygorii Strashko
` (2 preceding siblings ...)
2014-10-23 14:23 ` [PATCH v3 3/5] PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME Grygorii Strashko
@ 2014-10-23 14:23 ` Grygorii Strashko
[not found] ` <1414074182-25980-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>
2014-10-24 18:25 ` [PATCH v3 0/5] ARM: keystone: pm: switch to use generic pm domains Santosh Shilimkar
5 siblings, 0 replies; 12+ messages in thread
From: Grygorii Strashko @ 2014-10-23 14:23 UTC (permalink / raw)
To: ssantosh, Rafael J. Wysocki, khilman
Cc: linux-pm, Rob Herring, grant.likely, linux-arm-kernel,
linux-kernel, devicetree, Grygorii Strashko, Ulf Hansson,
Geert Uytterhoeven, Dmitry Torokhov
This patch switches Keystone 2 PM code to use Generic PM domains
instead of PM clock domains because of the lack of DT support
for the last.
CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
.../bindings/power/ti,keystone-powerdomain.txt | 31 ++++++
arch/arm/mach-keystone/Kconfig | 1 +
arch/arm/mach-keystone/pm_domain.c | 105 +++++++++++++--------
3 files changed, 100 insertions(+), 37 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
diff --git a/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
new file mode 100644
index 0000000..d4e24d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
@@ -0,0 +1,31 @@
+* TI Keystone 2 Generic PM Controller
+
+The TI Keystone 2 Generic PM Controller is responsible for Clock gating
+for each controlled IP module.
+
+Required properties:
+- compatible: Should be "ti,keystone-powerdomain"
+- #power-domain-cells: Should be 0, see below:
+
+The PM Controller node is a PM domain as documented in
+Documentation/devicetree/bindings/power/power_domain.txt.
+
+Example:
+
+ pm_controller: pm-controller {
+ compatible = "ti,keystone-powerdomain";
+ #power-domain-cells = <0>;
+ };
+
+ netcp: netcp@2090000 {
+ reg = <0x2620110 0x8>;
+ reg-names = "efuse";
+ ...
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ power-domains = <&pm_controller>;
+
+ clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
+ dma-coherent;
+ }
diff --git a/arch/arm/mach-keystone/Kconfig b/arch/arm/mach-keystone/Kconfig
index 98a156a..de43107 100644
--- a/arch/arm/mach-keystone/Kconfig
+++ b/arch/arm/mach-keystone/Kconfig
@@ -9,6 +9,7 @@ config ARCH_KEYSTONE
select COMMON_CLK_KEYSTONE
select ARCH_SUPPORTS_BIG_ENDIAN
select ZONE_DMA if ARM_LPAE
+ select PM_GENERIC_DOMAINS if PM
help
Support for boards based on the Texas Instruments Keystone family of
SoCs.
diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c
index ca79dda..3341da1 100644
--- a/arch/arm/mach-keystone/pm_domain.c
+++ b/arch/arm/mach-keystone/pm_domain.c
@@ -12,69 +12,100 @@
* version 2, as published by the Free Software Foundation.
*/
+#include <linux/clk.h>
#include <linux/init.h>
-#include <linux/pm_runtime.h>
#include <linux/pm_clock.h>
+#include <linux/pm_domain.h>
#include <linux/platform_device.h>
-#include <linux/clk-provider.h>
#include <linux/of.h>
-#ifdef CONFIG_PM_RUNTIME
-static int keystone_pm_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_GENERIC_DOMAINS
+
+struct keystone_domain {
+ struct generic_pm_domain genpd;
+ struct device *dev;
+};
+
+void keystone_pm_domain_attach_dev(struct device *dev)
{
+ struct clk *clk;
int ret;
+ int i = 0;
dev_dbg(dev, "%s\n", __func__);
- ret = pm_generic_runtime_suspend(dev);
- if (ret)
- return ret;
-
- ret = pm_clk_suspend(dev);
+ ret = pm_clk_create(dev);
if (ret) {
- pm_generic_runtime_resume(dev);
- return ret;
+ dev_err(dev, "pm_clk_create failed %d\n", ret);
+ return;
+ };
+
+ while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
+ ret = pm_clk_add_clk(dev, clk);
+ if (ret) {
+ dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
+ goto clk_err;
+ };
}
- return 0;
+ return;
+
+clk_err:
+ pm_clk_destroy(dev);
}
-static int keystone_pm_runtime_resume(struct device *dev)
+void keystone_pm_domain_detach_dev(struct device *dev)
{
dev_dbg(dev, "%s\n", __func__);
-
- pm_clk_resume(dev);
-
- return pm_generic_runtime_resume(dev);
+ pm_clk_destroy(dev);
}
-#endif
-static struct dev_pm_domain keystone_pm_domain = {
- .ops = {
- SET_RUNTIME_PM_OPS(keystone_pm_runtime_suspend,
- keystone_pm_runtime_resume, NULL)
- USE_PLATFORM_PM_SLEEP_OPS
+static const struct keystone_domain keystone_domain = {
+ .genpd = {
+ .name = "keystone",
+ .attach_dev = keystone_pm_domain_attach_dev,
+ .detach_dev = keystone_pm_domain_detach_dev,
+ .dev_ops = {
+ .stop = pm_clk_suspend,
+ .start = pm_clk_resume,
+ },
},
};
-static struct pm_clk_notifier_block platform_domain_notifier = {
- .pm_domain = &keystone_pm_domain,
+static int keystone_pm_domain_probe(struct platform_device *pdev)
+{
+ struct keystone_domain *domain;
+
+ domain = devm_kzalloc(&pdev->dev,
+ sizeof(struct keystone_domain), GFP_KERNEL);
+ if (!domain)
+ return -ENOMEM;
+
+ domain->genpd = keystone_domain.genpd;
+ domain->dev = &pdev->dev;
+
+ pm_genpd_init(&domain->genpd, NULL, false);
+ return of_genpd_add_provider_simple(pdev->dev.of_node, &domain->genpd);
+}
+
+static struct of_device_id keystone_pm_domain_dt_ids[] = {
+ { .compatible = "ti,keystone-powerdomain" },
+ { }
};
-static struct of_device_id of_keystone_table[] = {
- {.compatible = "ti,keystone"},
- { /* end of list */ },
+static struct platform_driver keystone_pm_domain_driver = {
+ .driver = {
+ .name = "ti,keystone-powerdomain",
+ .owner = THIS_MODULE,
+ .of_match_table = keystone_pm_domain_dt_ids,
+ },
+ .probe = keystone_pm_domain_probe,
};
int __init keystone_pm_runtime_init(void)
{
- struct device_node *np;
-
- np = of_find_matching_node(NULL, of_keystone_table);
- if (!np)
- return 0;
-
- pm_clk_add_notifier(&platform_bus_type, &platform_domain_notifier);
-
- return 0;
+ return platform_driver_register(&keystone_pm_domain_driver);
}
+#else
+int __init keystone_pm_runtime_init(void) { return 0; }
+#endif /* CONFIG_PM_GENERIC_DOMAINS */
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread[parent not found: <1414074182-25980-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>]
* [PATCH v3 5/5] ARM: dts: keystone: add generic pm controller node
[not found] ` <1414074182-25980-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>
@ 2014-10-23 14:23 ` Grygorii Strashko
0 siblings, 0 replies; 12+ messages in thread
From: Grygorii Strashko @ 2014-10-23 14:23 UTC (permalink / raw)
To: ssantosh-DgEjT+Ai2ygdnm+yROfE0A, Rafael J. Wysocki,
khilman-QSEj5FYQhm4dnm+yROfE0A
Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Grygorii Strashko, Ulf Hansson,
Geert Uytterhoeven
Add TI Keystone 2 Generic PM Controller node and attach
the Davinci MDIO device to it.
CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
CC: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
CC: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
---
arch/arm/boot/dts/keystone.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index 5d3e83f..c669d0d 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -85,6 +85,11 @@
/include/ "keystone-clocks.dtsi"
+ pm_controller: pm-controller {
+ compatible = "ti,keystone-powerdomain";
+ #power-domain-cells = <0>;
+ };
+
uart0: serial@02530c00 {
compatible = "ns16550a";
current-speed = <115200>;
@@ -275,6 +280,7 @@
status = "disabled";
clocks = <&clkpa>;
clock-names = "fck";
+ power-domains = <&pm_controller>;
bus_freq = <2500000>;
};
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 0/5] ARM: keystone: pm: switch to use generic pm domains
2014-10-23 14:22 [PATCH v3 0/5] ARM: keystone: pm: switch to use generic pm domains Grygorii Strashko
` (4 preceding siblings ...)
[not found] ` <1414074182-25980-1-git-send-email-grygorii.strashko-l0cyMroinI0@public.gmane.org>
@ 2014-10-24 18:25 ` Santosh Shilimkar
[not found] ` <544A99B2.8080507-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
5 siblings, 1 reply; 12+ messages in thread
From: Santosh Shilimkar @ 2014-10-24 18:25 UTC (permalink / raw)
To: Grygorii Strashko, ssantosh, Rafael J. Wysocki, khilman
Cc: linux-pm, Rob Herring, grant.likely, linux-arm-kernel,
linux-kernel, devicetree, Ulf Hansson, Geert Uytterhoeven,
Dmitry Torokhov
Grygorii,
On 10/23/2014 07:22 AM, Grygorii Strashko wrote:
> Hi Santosh, Kevin,
>
> This series switches Keystone 2 PM code to use Generic PM domains
> instead of PM clock domains because of the lack of DT support
> for the last.
> It will finally allow to enable Runtime PM for Keystone 2.
>
> Patch 1 was reused from [1].
>
> RFC version of patches can be found at [2].
>
> Changes in v3:
> - handling of the case when !CONFIG_PM_RUNTIME has been
> moved in PM clock framework.
>
> Changes in v2:
> - minor comments applied and rebased on top of Linux 3.18-rc1.
>
> Links on previous versions:
> v2: https://lkml.org/lkml/2014/10/20/248
> v1: https://lkml.org/lkml/2014/9/29/382
>
> [1] "[PATCH/RFC 0/4] of: Register clocks for Runtime PM with PM core"
> https://lkml.org/lkml/2014/4/24/1118
>
> [2] "[RFC PATCH 0/4] ARM: keystone: pm: switch to use generic pm domains"
> https://lkml.org/lkml/2014/9/25/364
>
> CC: Santosh Shilimkar <ssantosh@kernel.org>
> CC: Kevin Hilman <khilman@linaro.org>
> CC: Ulf Hansson <ulf.hansson@linaro.org>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Geert Uytterhoeven (1):
> PM / clock_ops: Add pm_clk_add_clk()
>
> Grygorii Strashko (4):
> PM / clock_ops: make __pm_clk_enable more generic
> PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME
> ARM: keystone: pm: switch to use generic pm domains
> ARM: dts: keystone: add generic pm controller node
>
You can split this series. Lets get first 3 patches sorted out as
there have been active discussion on those. Once they get direction
and merged then you can post the keystone ones which I will be
happy to merge.
Regards,
Santosh
^ permalink raw reply [flat|nested] 12+ messages in thread