[RFC/RFT 0/6] qcom: Add runtime PM support

[RFC/RFT 0/6] qcom: Add runtime PM support

[Rajendra Nayak]

The patches add runtime PM support (using PM clocks) for 
devices in qcom SoCs. Also converts serial/sdhci/i2c and
spi drivers to cleanup clock handling and use runtime PM
apis instead.

There are a couple [1][2] of other patch series that this one is
based off, so I have pushed a branch [3] with all the dependecies
if anyone wants to play with this series.
I have been able to test the serial and sdhci driver changes to a
fair extent, i2c for a sucessfull probe and could not test anything
on spi. So any testing on these drivers is highly appreciated.

[1] https://www.mail-archive.com/linux-arm-msm@vger.kernel.org/msg14157.html
[2] https://www.mail-archive.com/linux-arm-msm@vger.kernel.org/msg14268.html
[3] https://github.com/rrnayak/linux/tree/v4.0/runtime-rfc

Rajendra Nayak (6):
  PM / clock_ops: Make pm_clk_notify() do nothing in case DT passes
    power-domains
  clk: qcom: Add runtime support to handle clocks using PM clocks
  serial: msm: convert driver to use runtime PM apis
  mmc: sdhci-msm: convert driver to use runtime PM apis
  i2c: qup: Get rid of clock handling as its done using runtime
    callbacks
  spi: qup: Get rid of clock handling as its done using runtime
    callbacks

 drivers/base/power/clock_ops.c  |  8 ++++-
 drivers/clk/qcom/gdsc.c         | 20 +++++++++++
 drivers/i2c/busses/i2c-qup.c    | 74 +++++++++--------------------------------
 drivers/mmc/host/sdhci-msm.c    | 46 ++++++-------------------
 drivers/spi/spi-qup.c           | 54 ++++++------------------------
 drivers/tty/serial/msm_serial.c | 33 +++++++++---------
 6 files changed, 80 insertions(+), 155 deletions(-)

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

[RFC/RFT 0/6] qcom: Add runtime PM support

[Rajendra Nayak]

The patches add runtime PM support (using PM clocks) for 
devices in qcom SoCs. Also converts serial/sdhci/i2c and
spi drivers to cleanup clock handling and use runtime PM
apis instead.

There are a couple [1][2] of other patch series that this one is
based off, so I have pushed a branch [3] with all the dependecies
if anyone wants to play with this series.
I have been able to test the serial and sdhci driver changes to a
fair extent, i2c for a sucessfull probe and could not test anything
on spi. So any testing on these drivers is highly appreciated.

[1] https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg14157.html
[2] https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg14268.html
[3] https://github.com/rrnayak/linux/tree/v4.0/runtime-rfc

Rajendra Nayak (6):
  PM / clock_ops: Make pm_clk_notify() do nothing in case DT passes
    power-domains
  clk: qcom: Add runtime support to handle clocks using PM clocks
  serial: msm: convert driver to use runtime PM apis
  mmc: sdhci-msm: convert driver to use runtime PM apis
  i2c: qup: Get rid of clock handling as its done using runtime
    callbacks
  spi: qup: Get rid of clock handling as its done using runtime
    callbacks

 drivers/base/power/clock_ops.c  |  8 ++++-
 drivers/clk/qcom/gdsc.c         | 20 +++++++++++
 drivers/i2c/busses/i2c-qup.c    | 74 +++++++++--------------------------------
 drivers/mmc/host/sdhci-msm.c    | 46 ++++++-------------------
 drivers/spi/spi-qup.c           | 54 ++++++------------------------
 drivers/tty/serial/msm_serial.c | 33 +++++++++---------
 6 files changed, 80 insertions(+), 155 deletions(-)

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

[RFC/RFT 1/6] PM / clock_ops: Make pm_clk_notify() do nothing in case DT passes power-domains

[Rajendra Nayak]

pm_clk_notify() intends to do nothing if the device is associated
with a pm_domain other than whats stored in pm_clk_notifier_block.
In case of DT though, just checking for an existing dev->pm_domain
is not very useful as dev->pm_domain actually gets populated much later.

Add a check for a 'power-domains' DT property to identify if the
device would be assocaiated with a different power domain at
a later point in time.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/base/power/clock_ops.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 8abea66..6618818 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/pm_runtime.h>
+#include <linux/of.h>
 
 #ifdef CONFIG_PM
 
@@ -323,6 +324,7 @@ int pm_clk_resume(struct device *dev)
  * of PM clocks, depending on @action.
  *
  * If the device's pm_domain field is already populated with a value different
+ * or is expected to be populated with a different value later (in case of DT)
  * from the one stored in the struct pm_clk_notifier_block object, the function
  * does nothing.
  */
@@ -332,7 +334,7 @@ static int pm_clk_notify(struct notifier_block *nb,
 	struct pm_clk_notifier_block *clknb;
 	struct device *dev = data;
 	char **con_id;
-	int error;
+	int error, sz;
 
 	dev_dbg(dev, "%s() %ld\n", __func__, action);
 
@@ -343,6 +345,10 @@ static int pm_clk_notify(struct notifier_block *nb,
 		if (dev->pm_domain)
 			break;
 
+		/* With DT dev->pm_domain hookup happens later */
+		if (of_find_property(dev->of_node, "power-domains", &sz))
+			break;
+
 		error = pm_clk_create(dev);
 		if (error)
 			break;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC/RFT 1/6] PM / clock_ops: Make pm_clk_notify() do nothing in case DT passes power-domains

[Rajendra Nayak]

pm_clk_notify() intends to do nothing if the device is associated
with a pm_domain other than whats stored in pm_clk_notifier_block.
In case of DT though, just checking for an existing dev->pm_domain
is not very useful as dev->pm_domain actually gets populated much later.

Add a check for a 'power-domains' DT property to identify if the
device would be assocaiated with a different power domain at
a later point in time.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/base/power/clock_ops.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 8abea66..6618818 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/pm_runtime.h>
+#include <linux/of.h>
 
 #ifdef CONFIG_PM
 
@@ -323,6 +324,7 @@ int pm_clk_resume(struct device *dev)
  * of PM clocks, depending on @action.
  *
  * If the device's pm_domain field is already populated with a value different
+ * or is expected to be populated with a different value later (in case of DT)
  * from the one stored in the struct pm_clk_notifier_block object, the function
  * does nothing.
  */
@@ -332,7 +334,7 @@ static int pm_clk_notify(struct notifier_block *nb,
 	struct pm_clk_notifier_block *clknb;
 	struct device *dev = data;
 	char **con_id;
-	int error;
+	int error, sz;
 
 	dev_dbg(dev, "%s() %ld\n", __func__, action);
 
@@ -343,6 +345,10 @@ static int pm_clk_notify(struct notifier_block *nb,
 		if (dev->pm_domain)
 			break;
 
+		/* With DT dev->pm_domain hookup happens later */
+		if (of_find_property(dev->of_node, "power-domains", &sz))
+			break;
+
 		error = pm_clk_create(dev);
 		if (error)
 			break;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Rajendra Nayak]

Add runtime PM support to handle (core and iface) clocks for devices
without a controllable power domain. Once the drivers for these devices
are converted to use runtime PM apis, all clock handling (for core and
iface) from these drivers can then be removed.

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

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 480ebf6..92b0f6d 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -15,6 +15,7 @@
 #include <linux/err.h>
 #include <linux/jiffies.h>
 #include <linux/pm_clock.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include "gdsc.h"
 
@@ -226,3 +227,22 @@ void gdsc_unregister(struct device *dev)
 {
 	of_genpd_del_provider(dev->of_node);
 }
+
+static struct dev_pm_domain default_qcom_pm_domain = {
+	.ops = {
+		USE_PM_CLK_RUNTIME_OPS
+		USE_PLATFORM_PM_SLEEP_OPS
+	},
+};
+
+static struct pm_clk_notifier_block qcom_pm_notifier = {
+	.pm_domain	= &default_qcom_pm_domain,
+	.con_ids	= { "core", "iface" },
+};
+
+static int __init qcom_pm_runtime_init(void)
+{
+	pm_clk_add_notifier(&platform_bus_type, &qcom_pm_notifier);
+	return 0;
+}
+core_initcall(qcom_pm_runtime_init);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Rajendra Nayak]

Add runtime PM support to handle (core and iface) clocks for devices
without a controllable power domain. Once the drivers for these devices
are converted to use runtime PM apis, all clock handling (for core and
iface) from these drivers can then be removed.

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

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 480ebf6..92b0f6d 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -15,6 +15,7 @@
 #include <linux/err.h>
 #include <linux/jiffies.h>
 #include <linux/pm_clock.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include "gdsc.h"
 
@@ -226,3 +227,22 @@ void gdsc_unregister(struct device *dev)
 {
 	of_genpd_del_provider(dev->of_node);
 }
+
+static struct dev_pm_domain default_qcom_pm_domain = {
+	.ops = {
+		USE_PM_CLK_RUNTIME_OPS
+		USE_PLATFORM_PM_SLEEP_OPS
+	},
+};
+
+static struct pm_clk_notifier_block qcom_pm_notifier = {
+	.pm_domain	= &default_qcom_pm_domain,
+	.con_ids	= { "core", "iface" },
+};
+
+static int __init qcom_pm_runtime_init(void)
+{
+	pm_clk_add_notifier(&platform_bus_type, &qcom_pm_notifier);
+	return 0;
+}
+core_initcall(qcom_pm_runtime_init);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> Add runtime PM support to handle (core and iface) clocks for devices
> without a controllable power domain. Once the drivers for these devices
> are converted to use runtime PM apis, all clock handling (for core and
> iface) from these drivers can then be removed.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/gdsc.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 480ebf6..92b0f6d 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -15,6 +15,7 @@
>  #include <linux/err.h>
>  #include <linux/jiffies.h>
>  #include <linux/pm_clock.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include "gdsc.h"
>
> @@ -226,3 +227,22 @@ void gdsc_unregister(struct device *dev)
>  {
>         of_genpd_del_provider(dev->of_node);
>  }
> +
> +static struct dev_pm_domain default_qcom_pm_domain = {
> +       .ops = {
> +               USE_PM_CLK_RUNTIME_OPS
> +               USE_PLATFORM_PM_SLEEP_OPS
> +       },
> +};
> +
> +static struct pm_clk_notifier_block qcom_pm_notifier = {
> +       .pm_domain      = &default_qcom_pm_domain,
> +       .con_ids        = { "core", "iface" },
> +};
> +
> +static int __init qcom_pm_runtime_init(void)
> +{
> +       pm_clk_add_notifier(&platform_bus_type, &qcom_pm_notifier);
> +       return 0;
> +}
> +core_initcall(qcom_pm_runtime_init);

First, I don't follow how this code is related to GDSC.

Second, I want to see less users of pm_clk_add_notifier() since it's
not the proper/long-term way of how to assign PM domain pointers to a
device. Instead that shall be done at device registration point. In
your case while parsing the DT nodes and adding devices onto to their
buses.

Yes, I understand that it will requires quite some work to adopt to
this behaviour - but that how it shall be done.

Finally, an important note, you don't need to use PM domains for these
devices at all and thus you don't need to fix what I propose. Instead
you only have to implement the runtime PM callbacks for each driver
and manage the clocks from there. That is probably also a easier
solution.

Kind regards
Uffe

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> Add runtime PM support to handle (core and iface) clocks for devices
> without a controllable power domain. Once the drivers for these devices
> are converted to use runtime PM apis, all clock handling (for core and
> iface) from these drivers can then be removed.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/clk/qcom/gdsc.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 480ebf6..92b0f6d 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -15,6 +15,7 @@
>  #include <linux/err.h>
>  #include <linux/jiffies.h>
>  #include <linux/pm_clock.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include "gdsc.h"
>
> @@ -226,3 +227,22 @@ void gdsc_unregister(struct device *dev)
>  {
>         of_genpd_del_provider(dev->of_node);
>  }
> +
> +static struct dev_pm_domain default_qcom_pm_domain = {
> +       .ops = {
> +               USE_PM_CLK_RUNTIME_OPS
> +               USE_PLATFORM_PM_SLEEP_OPS
> +       },
> +};
> +
> +static struct pm_clk_notifier_block qcom_pm_notifier = {
> +       .pm_domain      = &default_qcom_pm_domain,
> +       .con_ids        = { "core", "iface" },
> +};
> +
> +static int __init qcom_pm_runtime_init(void)
> +{
> +       pm_clk_add_notifier(&platform_bus_type, &qcom_pm_notifier);
> +       return 0;
> +}
> +core_initcall(qcom_pm_runtime_init);

First, I don't follow how this code is related to GDSC.

Second, I want to see less users of pm_clk_add_notifier() since it's
not the proper/long-term way of how to assign PM domain pointers to a
device. Instead that shall be done at device registration point. In
your case while parsing the DT nodes and adding devices onto to their
buses.

Yes, I understand that it will requires quite some work to adopt to
this behaviour - but that how it shall be done.

Finally, an important note, you don't need to use PM domains for these
devices at all and thus you don't need to fix what I propose. Instead
you only have to implement the runtime PM callbacks for each driver
and manage the clocks from there. That is probably also a easier
solution.

Kind regards
Uffe

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Rajendra Nayak]

On 04/24/2015 03:33 PM, Ulf Hansson wrote:
> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> Add runtime PM support to handle (core and iface) clocks for devices
>> without a controllable power domain. Once the drivers for these devices
>> are converted to use runtime PM apis, all clock handling (for core and
>> iface) from these drivers can then be removed.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>   drivers/clk/qcom/gdsc.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index 480ebf6..92b0f6d 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/err.h>
>>   #include <linux/jiffies.h>
>>   #include <linux/pm_clock.h>
>> +#include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   #include "gdsc.h"
>>
>> @@ -226,3 +227,22 @@ void gdsc_unregister(struct device *dev)
>>   {
>>          of_genpd_del_provider(dev->of_node);
>>   }
>> +
>> +static struct dev_pm_domain default_qcom_pm_domain = {
>> +       .ops = {
>> +               USE_PM_CLK_RUNTIME_OPS
>> +               USE_PLATFORM_PM_SLEEP_OPS
>> +       },
>> +};
>> +
>> +static struct pm_clk_notifier_block qcom_pm_notifier = {
>> +       .pm_domain      = &default_qcom_pm_domain,
>> +       .con_ids        = { "core", "iface" },
>> +};
>> +
>> +static int __init qcom_pm_runtime_init(void)
>> +{
>> +       pm_clk_add_notifier(&platform_bus_type, &qcom_pm_notifier);
>> +       return 0;
>> +}
>> +core_initcall(qcom_pm_runtime_init);
>
> First, I don't follow how this code is related to GDSC.

Actually its not. I should probably move it to a pm_runtime.c someplace
in drivers/soc/qcom maybe.

>
> Second, I want to see less users of pm_clk_add_notifier() since it's
> not the proper/long-term way of how to assign PM domain pointers to a
> device. Instead that shall be done at device registration point. In
> your case while parsing the DT nodes and adding devices onto to their
> buses.

but these are devices which do not really have a controllable power
domain, they just have controllable clocks.

>
> Yes, I understand that it will requires quite some work to adopt to
> this behaviour - but that how it shall be done.
>
> Finally, an important note, you don't need to use PM domains for these
> devices at all and thus you don't need to fix what I propose. Instead
> you only have to implement the runtime PM callbacks for each driver
> and manage the clocks from there. That is probably also a easier
> solution.

But that would mean I repeat the same code in all drivers to do a
clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
clocks. I thought we have clock_ops.c just to avoid that (atleast up
until we have a better way of doing it)
And there are atleast a few architecture which have used it to avoid the
duplication across all drivers (omap1/davinci/sh/keystone)

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Rajendra Nayak]

On 04/24/2015 03:33 PM, Ulf Hansson wrote:
> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> Add runtime PM support to handle (core and iface) clocks for devices
>> without a controllable power domain. Once the drivers for these devices
>> are converted to use runtime PM apis, all clock handling (for core and
>> iface) from these drivers can then be removed.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>   drivers/clk/qcom/gdsc.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index 480ebf6..92b0f6d 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/err.h>
>>   #include <linux/jiffies.h>
>>   #include <linux/pm_clock.h>
>> +#include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   #include "gdsc.h"
>>
>> @@ -226,3 +227,22 @@ void gdsc_unregister(struct device *dev)
>>   {
>>          of_genpd_del_provider(dev->of_node);
>>   }
>> +
>> +static struct dev_pm_domain default_qcom_pm_domain = {
>> +       .ops = {
>> +               USE_PM_CLK_RUNTIME_OPS
>> +               USE_PLATFORM_PM_SLEEP_OPS
>> +       },
>> +};
>> +
>> +static struct pm_clk_notifier_block qcom_pm_notifier = {
>> +       .pm_domain      = &default_qcom_pm_domain,
>> +       .con_ids        = { "core", "iface" },
>> +};
>> +
>> +static int __init qcom_pm_runtime_init(void)
>> +{
>> +       pm_clk_add_notifier(&platform_bus_type, &qcom_pm_notifier);
>> +       return 0;
>> +}
>> +core_initcall(qcom_pm_runtime_init);
>
> First, I don't follow how this code is related to GDSC.

Actually its not. I should probably move it to a pm_runtime.c someplace
in drivers/soc/qcom maybe.

>
> Second, I want to see less users of pm_clk_add_notifier() since it's
> not the proper/long-term way of how to assign PM domain pointers to a
> device. Instead that shall be done at device registration point. In
> your case while parsing the DT nodes and adding devices onto to their
> buses.

but these are devices which do not really have a controllable power
domain, they just have controllable clocks.

>
> Yes, I understand that it will requires quite some work to adopt to
> this behaviour - but that how it shall be done.
>
> Finally, an important note, you don't need to use PM domains for these
> devices at all and thus you don't need to fix what I propose. Instead
> you only have to implement the runtime PM callbacks for each driver
> and manage the clocks from there. That is probably also a easier
> solution.

But that would mean I repeat the same code in all drivers to do a
clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
clocks. I thought we have clock_ops.c just to avoid that (atleast up
until we have a better way of doing it)
And there are atleast a few architecture which have used it to avoid the
duplication across all drivers (omap1/davinci/sh/keystone)

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Geert Uytterhoeven]

On Fri, Apr 24, 2015 at 12:58 PM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> Second, I want to see less users of pm_clk_add_notifier() since it's
>> not the proper/long-term way of how to assign PM domain pointers to a
>> device. Instead that shall be done at device registration point. In
>> your case while parsing the DT nodes and adding devices onto to their
>> buses.
>
> but these are devices which do not really have a controllable power
> domain, they just have controllable clocks.
>
>> Yes, I understand that it will requires quite some work to adopt to
>> this behaviour - but that how it shall be done.
>>
>> Finally, an important note, you don't need to use PM domains for these
>> devices at all and thus you don't need to fix what I propose. Instead
>> you only have to implement the runtime PM callbacks for each driver
>> and manage the clocks from there. That is probably also a easier
>> solution.
>
> But that would mean I repeat the same code in all drivers to do a
> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
> clocks. I thought we have clock_ops.c just to avoid that (atleast up
> until we have a better way of doing it)
> And there are atleast a few architecture which have used it to avoid the
> duplication across all drivers (omap1/davinci/sh/keystone)

At least for arm/shmobile, we're planning to move away from this, cfr.
http://www.spinics.net/lists/linux-sh/msg41114.html

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Geert Uytterhoeven]

On Fri, Apr 24, 2015 at 12:58 PM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> Second, I want to see less users of pm_clk_add_notifier() since it's
>> not the proper/long-term way of how to assign PM domain pointers to a
>> device. Instead that shall be done at device registration point. In
>> your case while parsing the DT nodes and adding devices onto to their
>> buses.
>
> but these are devices which do not really have a controllable power
> domain, they just have controllable clocks.
>
>> Yes, I understand that it will requires quite some work to adopt to
>> this behaviour - but that how it shall be done.
>>
>> Finally, an important note, you don't need to use PM domains for these
>> devices at all and thus you don't need to fix what I propose. Instead
>> you only have to implement the runtime PM callbacks for each driver
>> and manage the clocks from there. That is probably also a easier
>> solution.
>
> But that would mean I repeat the same code in all drivers to do a
> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
> clocks. I thought we have clock_ops.c just to avoid that (atleast up
> until we have a better way of doing it)
> And there are atleast a few architecture which have used it to avoid the
> duplication across all drivers (omap1/davinci/sh/keystone)

At least for arm/shmobile, we're planning to move away from this, cfr.
http://www.spinics.net/lists/linux-sh/msg41114.html

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Kevin Hilman]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Fri, Apr 24, 2015 at 12:58 PM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>> Second, I want to see less users of pm_clk_add_notifier() since it's
>>> not the proper/long-term way of how to assign PM domain pointers to a
>>> device. Instead that shall be done at device registration point. In
>>> your case while parsing the DT nodes and adding devices onto to their
>>> buses.
>>
>> but these are devices which do not really have a controllable power
>> domain, they just have controllable clocks.
>>
>>> Yes, I understand that it will requires quite some work to adopt to
>>> this behaviour - but that how it shall be done.
>>>
>>> Finally, an important note, you don't need to use PM domains for these
>>> devices at all and thus you don't need to fix what I propose. Instead
>>> you only have to implement the runtime PM callbacks for each driver
>>> and manage the clocks from there. That is probably also a easier
>>> solution.
>>
>> But that would mean I repeat the same code in all drivers to do a
>> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
>> clocks. I thought we have clock_ops.c just to avoid that (atleast up
>> until we have a better way of doing it)
>> And there are atleast a few architecture which have used it to avoid the
>> duplication across all drivers (omap1/davinci/sh/keystone)
>
> At least for arm/shmobile, we're planning to move away from this, cfr.
> http://www.spinics.net/lists/linux-sh/msg41114.html

Just to clarify for Rajendra's sake... 

SH is moving away from the pm_clk_add_notifier(), but not duplicating
the clk_get/prepare/enable/disable/unprepare across all the drivers.

Rather, they're using a genpd to model the clock domain, and taking
advantage of the pm_domain speciic attach callback to attach the PM
clocks.

This should work for qcom also assuming that these device nodes don't
also need to belong to a different PM domain.

Kevin

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Kevin Hilman]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Fri, Apr 24, 2015 at 12:58 PM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>> Second, I want to see less users of pm_clk_add_notifier() since it's
>>> not the proper/long-term way of how to assign PM domain pointers to a
>>> device. Instead that shall be done at device registration point. In
>>> your case while parsing the DT nodes and adding devices onto to their
>>> buses.
>>
>> but these are devices which do not really have a controllable power
>> domain, they just have controllable clocks.
>>
>>> Yes, I understand that it will requires quite some work to adopt to
>>> this behaviour - but that how it shall be done.
>>>
>>> Finally, an important note, you don't need to use PM domains for these
>>> devices at all and thus you don't need to fix what I propose. Instead
>>> you only have to implement the runtime PM callbacks for each driver
>>> and manage the clocks from there. That is probably also a easier
>>> solution.
>>
>> But that would mean I repeat the same code in all drivers to do a
>> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
>> clocks. I thought we have clock_ops.c just to avoid that (atleast up
>> until we have a better way of doing it)
>> And there are atleast a few architecture which have used it to avoid the
>> duplication across all drivers (omap1/davinci/sh/keystone)
>
> At least for arm/shmobile, we're planning to move away from this, cfr.
> http://www.spinics.net/lists/linux-sh/msg41114.html

Just to clarify for Rajendra's sake... 

SH is moving away from the pm_clk_add_notifier(), but not duplicating
the clk_get/prepare/enable/disable/unprepare across all the drivers.

Rather, they're using a genpd to model the clock domain, and taking
advantage of the pm_domain speciic attach callback to attach the PM
clocks.

This should work for qcom also assuming that these device nodes don't
also need to belong to a different PM domain.

Kevin

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Rajendra Nayak]

On 04/28/2015 01:32 AM, Kevin Hilman wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>
>> On Fri, Apr 24, 2015 at 12:58 PM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>> Second, I want to see less users of pm_clk_add_notifier() since it's
>>>> not the proper/long-term way of how to assign PM domain pointers to a
>>>> device. Instead that shall be done at device registration point. In
>>>> your case while parsing the DT nodes and adding devices onto to their
>>>> buses.
>>>
>>> but these are devices which do not really have a controllable power
>>> domain, they just have controllable clocks.
>>>
>>>> Yes, I understand that it will requires quite some work to adopt to
>>>> this behaviour - but that how it shall be done.
>>>>
>>>> Finally, an important note, you don't need to use PM domains for these
>>>> devices at all and thus you don't need to fix what I propose. Instead
>>>> you only have to implement the runtime PM callbacks for each driver
>>>> and manage the clocks from there. That is probably also a easier
>>>> solution.
>>>
>>> But that would mean I repeat the same code in all drivers to do a
>>> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
>>> clocks. I thought we have clock_ops.c just to avoid that (atleast up
>>> until we have a better way of doing it)
>>> And there are atleast a few architecture which have used it to avoid the
>>> duplication across all drivers (omap1/davinci/sh/keystone)
>>
>> At least for arm/shmobile, we're planning to move away from this, cfr.
>> http://www.spinics.net/lists/linux-sh/msg41114.html
>
> Just to clarify for Rajendra's sake...
>
> SH is moving away from the pm_clk_add_notifier(), but not duplicating
> the clk_get/prepare/enable/disable/unprepare across all the drivers.
>
> Rather, they're using a genpd to model the clock domain, and taking
> advantage of the pm_domain speciic attach callback to attach the PM
> clocks.
>
> This should work for qcom also assuming that these device nodes don't
> also need to belong to a different PM domain.

Thanks Kevin, I did look up the patches that Geert pointed me to, and
figured I can do something similar for qcom as well like you said.

There are 2 types of devices that I will need to handle, one which have
clocks and also a power switch to turn the power domain on/off (camera,
graphics, display), and others which only have clocks and no power
switch to control the power domain (serial, sdhc, i2c, spi).

I was already using genpd attach/detach to handle clocks for the
devices with a power switch and genpd on/off to turn the PD on and off.
I guess I can also control the rest of the devices the same way, just
that the genpd on/off for them would do nothing.
That way I don't have to use pm_clk_add_notifier() and can also
associate the power domain (with no on/off control) to devices
through DT (and there isn;t any duplication of code in the drivers)

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Rajendra Nayak]

On 04/28/2015 01:32 AM, Kevin Hilman wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>
>> On Fri, Apr 24, 2015 at 12:58 PM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>> Second, I want to see less users of pm_clk_add_notifier() since it's
>>>> not the proper/long-term way of how to assign PM domain pointers to a
>>>> device. Instead that shall be done at device registration point. In
>>>> your case while parsing the DT nodes and adding devices onto to their
>>>> buses.
>>>
>>> but these are devices which do not really have a controllable power
>>> domain, they just have controllable clocks.
>>>
>>>> Yes, I understand that it will requires quite some work to adopt to
>>>> this behaviour - but that how it shall be done.
>>>>
>>>> Finally, an important note, you don't need to use PM domains for these
>>>> devices at all and thus you don't need to fix what I propose. Instead
>>>> you only have to implement the runtime PM callbacks for each driver
>>>> and manage the clocks from there. That is probably also a easier
>>>> solution.
>>>
>>> But that would mean I repeat the same code in all drivers to do a
>>> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
>>> clocks. I thought we have clock_ops.c just to avoid that (atleast up
>>> until we have a better way of doing it)
>>> And there are atleast a few architecture which have used it to avoid the
>>> duplication across all drivers (omap1/davinci/sh/keystone)
>>
>> At least for arm/shmobile, we're planning to move away from this, cfr.
>> http://www.spinics.net/lists/linux-sh/msg41114.html
>
> Just to clarify for Rajendra's sake...
>
> SH is moving away from the pm_clk_add_notifier(), but not duplicating
> the clk_get/prepare/enable/disable/unprepare across all the drivers.
>
> Rather, they're using a genpd to model the clock domain, and taking
> advantage of the pm_domain speciic attach callback to attach the PM
> clocks.
>
> This should work for qcom also assuming that these device nodes don't
> also need to belong to a different PM domain.

Thanks Kevin, I did look up the patches that Geert pointed me to, and
figured I can do something similar for qcom as well like you said.

There are 2 types of devices that I will need to handle, one which have
clocks and also a power switch to turn the power domain on/off (camera,
graphics, display), and others which only have clocks and no power
switch to control the power domain (serial, sdhc, i2c, spi).

I was already using genpd attach/detach to handle clocks for the
devices with a power switch and genpd on/off to turn the PD on and off.
I guess I can also control the rest of the devices the same way, just
that the genpd on/off for them would do nothing.
That way I don't have to use pm_clk_add_notifier() and can also
associate the power domain (with no on/off control) to devices
through DT (and there isn;t any duplication of code in the drivers)

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Geert Uytterhoeven]

Hi Rajendra,

On Tue, Apr 28, 2015 at 4:52 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>> But that would mean I repeat the same code in all drivers to do a
>>>> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
>>>> clocks. I thought we have clock_ops.c just to avoid that (atleast up
>>>> until we have a better way of doing it)
>>>> And there are atleast a few architecture which have used it to avoid the
>>>> duplication across all drivers (omap1/davinci/sh/keystone)
>>>
>>>
>>> At least for arm/shmobile, we're planning to move away from this, cfr.
>>> http://www.spinics.net/lists/linux-sh/msg41114.html

The above are for the pure-clock domain case (no power areas), on R-Car
Gen2.

>> Just to clarify for Rajendra's sake...
>>
>> SH is moving away from the pm_clk_add_notifier(), but not duplicating
>> the clk_get/prepare/enable/disable/unprepare across all the drivers.
>>
>> Rather, they're using a genpd to model the clock domain, and taking
>> advantage of the pm_domain speciic attach callback to attach the PM
>> clocks.
>>
>> This should work for qcom also assuming that these device nodes don't
>> also need to belong to a different PM domain.
>
> Thanks Kevin, I did look up the patches that Geert pointed me to, and
> figured I can do something similar for qcom as well like you said.
>
> There are 2 types of devices that I will need to handle, one which have
> clocks and also a power switch to turn the power domain on/off (camera,
> graphics, display), and others which only have clocks and no power
> switch to control the power domain (serial, sdhc, i2c, spi).
>
> I was already using genpd attach/detach to handle clocks for the
> devices with a power switch and genpd on/off to turn the PD on and off.

Good.

> I guess I can also control the rest of the devices the same way, just
> that the genpd on/off for them would do nothing.
> That way I don't have to use pm_clk_add_notifier() and can also
> associate the power domain (with no on/off control) to devices
> through DT (and there isn;t any duplication of code in the drivers)

That looks similar to what we have on R-Mobile: some devices are in
controllable power areas, other are in an "always on" power area. All (most)
devices have controllable clocks, which we also control through the PM
domain. "git grep sysc-rmobile" will point you to the related code and DTS.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Geert Uytterhoeven]

Hi Rajendra,

On Tue, Apr 28, 2015 at 4:52 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>> But that would mean I repeat the same code in all drivers to do a
>>>> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
>>>> clocks. I thought we have clock_ops.c just to avoid that (atleast up
>>>> until we have a better way of doing it)
>>>> And there are atleast a few architecture which have used it to avoid the
>>>> duplication across all drivers (omap1/davinci/sh/keystone)
>>>
>>>
>>> At least for arm/shmobile, we're planning to move away from this, cfr.
>>> http://www.spinics.net/lists/linux-sh/msg41114.html

The above are for the pure-clock domain case (no power areas), on R-Car
Gen2.

>> Just to clarify for Rajendra's sake...
>>
>> SH is moving away from the pm_clk_add_notifier(), but not duplicating
>> the clk_get/prepare/enable/disable/unprepare across all the drivers.
>>
>> Rather, they're using a genpd to model the clock domain, and taking
>> advantage of the pm_domain speciic attach callback to attach the PM
>> clocks.
>>
>> This should work for qcom also assuming that these device nodes don't
>> also need to belong to a different PM domain.
>
> Thanks Kevin, I did look up the patches that Geert pointed me to, and
> figured I can do something similar for qcom as well like you said.
>
> There are 2 types of devices that I will need to handle, one which have
> clocks and also a power switch to turn the power domain on/off (camera,
> graphics, display), and others which only have clocks and no power
> switch to control the power domain (serial, sdhc, i2c, spi).
>
> I was already using genpd attach/detach to handle clocks for the
> devices with a power switch and genpd on/off to turn the PD on and off.

Good.

> I guess I can also control the rest of the devices the same way, just
> that the genpd on/off for them would do nothing.
> That way I don't have to use pm_clk_add_notifier() and can also
> associate the power domain (with no on/off control) to devices
> through DT (and there isn;t any duplication of code in the drivers)

That looks similar to what we have on R-Mobile: some devices are in
controllable power areas, other are in an "always on" power area. All (most)
devices have controllable clocks, which we also control through the PM
domain. "git grep sysc-rmobile" will point you to the related code and DTS.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Rajendra Nayak]

[]..

>> I guess I can also control the rest of the devices the same way, just
>> that the genpd on/off for them would do nothing.
>> That way I don't have to use pm_clk_add_notifier() and can also
>> associate the power domain (with no on/off control) to devices
>> through DT (and there isn;t any duplication of code in the drivers)
>
> That looks similar to what we have on R-Mobile: some devices are in
> controllable power areas, other are in an "always on" power area. All (most)
> devices have controllable clocks, which we also control through the PM
> domain. "git grep sysc-rmobile" will point you to the related code and DTS.

Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
rmobile. I mean who turns the clocks on for the devices when you build
with CONFIG_PM disabled?

>

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Rajendra Nayak]

[]..

>> I guess I can also control the rest of the devices the same way, just
>> that the genpd on/off for them would do nothing.
>> That way I don't have to use pm_clk_add_notifier() and can also
>> associate the power domain (with no on/off control) to devices
>> through DT (and there isn;t any duplication of code in the drivers)
>
> That looks similar to what we have on R-Mobile: some devices are in
> controllable power areas, other are in an "always on" power area. All (most)
> devices have controllable clocks, which we also control through the PM
> domain. "git grep sysc-rmobile" will point you to the related code and DTS.

Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
rmobile. I mean who turns the clocks on for the devices when you build
with CONFIG_PM disabled?

>

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Geert Uytterhoeven]

Hi Rajendra,

On Wed, Apr 29, 2015 at 11:49 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>> I guess I can also control the rest of the devices the same way, just
>>> that the genpd on/off for them would do nothing.
>>> That way I don't have to use pm_clk_add_notifier() and can also
>>> associate the power domain (with no on/off control) to devices
>>> through DT (and there isn;t any duplication of code in the drivers)
>>
>>
>> That looks similar to what we have on R-Mobile: some devices are in
>> controllable power areas, other are in an "always on" power area. All
>> (most)
>> devices have controllable clocks, which we also control through the PM
>> domain. "git grep sysc-rmobile" will point you to the related code and
>> DTS.
>
> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
> rmobile. I mean who turns the clocks on for the devices when you build
> with CONFIG_PM disabled?

We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
pm_clk_notify() will enable the clocks at driver binding time.
Hardware power domains are assumed enabled by reset state/the boot
loader.

Given the amount of PM infrastructure involved when hardware power and
clock domains are involved, I think at one point we'll have to start restricting
our builds to CONFIG_PM=y.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Geert Uytterhoeven]

Hi Rajendra,

On Wed, Apr 29, 2015 at 11:49 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>> I guess I can also control the rest of the devices the same way, just
>>> that the genpd on/off for them would do nothing.
>>> That way I don't have to use pm_clk_add_notifier() and can also
>>> associate the power domain (with no on/off control) to devices
>>> through DT (and there isn;t any duplication of code in the drivers)
>>
>>
>> That looks similar to what we have on R-Mobile: some devices are in
>> controllable power areas, other are in an "always on" power area. All
>> (most)
>> devices have controllable clocks, which we also control through the PM
>> domain. "git grep sysc-rmobile" will point you to the related code and
>> DTS.
>
> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
> rmobile. I mean who turns the clocks on for the devices when you build
> with CONFIG_PM disabled?

We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
pm_clk_notify() will enable the clocks at driver binding time.
Hardware power domains are assumed enabled by reset state/the boot
loader.

Given the amount of PM infrastructure involved when hardware power and
clock domains are involved, I think at one point we'll have to start restricting
our builds to CONFIG_PM=y.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 29 April 2015 at 13:30, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Rajendra,
>
> On Wed, Apr 29, 2015 at 11:49 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>> I guess I can also control the rest of the devices the same way, just
>>>> that the genpd on/off for them would do nothing.
>>>> That way I don't have to use pm_clk_add_notifier() and can also
>>>> associate the power domain (with no on/off control) to devices
>>>> through DT (and there isn;t any duplication of code in the drivers)
>>>
>>>
>>> That looks similar to what we have on R-Mobile: some devices are in
>>> controllable power areas, other are in an "always on" power area. All
>>> (most)
>>> devices have controllable clocks, which we also control through the PM
>>> domain. "git grep sysc-rmobile" will point you to the related code and
>>> DTS.
>>
>> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
>> rmobile. I mean who turns the clocks on for the devices when you build
>> with CONFIG_PM disabled?
>
> We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
> CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
> pm_clk_notify() will enable the clocks at driver binding time.
> Hardware power domains are assumed enabled by reset state/the boot
> loader.

Yes, it a bit tricky - but I guess that's the current only viable
solution if we have when using the pm_clk API.

>
> Given the amount of PM infrastructure involved when hardware power and
> clock domains are involved, I think at one point we'll have to start restricting
> our builds to CONFIG_PM=y.

I don't think that would solve the problem, since you may still have
cross SoC drivers which may be used without CONFIG_PM.

So all SoC that uses a driver which expects clocks to be managed using
PM clocks from a PM domain, will need the above "trick". Right?

Kind regards
Uffe

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 29 April 2015 at 13:30, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Rajendra,
>
> On Wed, Apr 29, 2015 at 11:49 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>> I guess I can also control the rest of the devices the same way, just
>>>> that the genpd on/off for them would do nothing.
>>>> That way I don't have to use pm_clk_add_notifier() and can also
>>>> associate the power domain (with no on/off control) to devices
>>>> through DT (and there isn;t any duplication of code in the drivers)
>>>
>>>
>>> That looks similar to what we have on R-Mobile: some devices are in
>>> controllable power areas, other are in an "always on" power area. All
>>> (most)
>>> devices have controllable clocks, which we also control through the PM
>>> domain. "git grep sysc-rmobile" will point you to the related code and
>>> DTS.
>>
>> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
>> rmobile. I mean who turns the clocks on for the devices when you build
>> with CONFIG_PM disabled?
>
> We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
> CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
> pm_clk_notify() will enable the clocks at driver binding time.
> Hardware power domains are assumed enabled by reset state/the boot
> loader.

Yes, it a bit tricky - but I guess that's the current only viable
solution if we have when using the pm_clk API.

>
> Given the amount of PM infrastructure involved when hardware power and
> clock domains are involved, I think at one point we'll have to start restricting
> our builds to CONFIG_PM=y.

I don't think that would solve the problem, since you may still have
cross SoC drivers which may be used without CONFIG_PM.

So all SoC that uses a driver which expects clocks to be managed using
PM clocks from a PM domain, will need the above "trick". Right?

Kind regards
Uffe

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Apr 29, 2015 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
>>> rmobile. I mean who turns the clocks on for the devices when you build
>>> with CONFIG_PM disabled?
>>
>> We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
>> CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
>> pm_clk_notify() will enable the clocks at driver binding time.
>> Hardware power domains are assumed enabled by reset state/the boot
>> loader.
>
> Yes, it a bit tricky - but I guess that's the current only viable
> solution if we have when using the pm_clk API.
>
>> Given the amount of PM infrastructure involved when hardware power and
>> clock domains are involved, I think at one point we'll have to start restricting
>> our builds to CONFIG_PM=y.
>
> I don't think that would solve the problem, since you may still have
> cross SoC drivers which may be used without CONFIG_PM.

That's not as much of a problem as it sounds:
  - If the driver cares (needs to know) about the clock, it should already
    manage it itself.
  - If it doesn't care about the clock, it just needs Runtime PM support.
    That will do the right thing on platforms with (needs PM=y) and without
    (doesn't care about PM=x) clock domains.
So the bigger "problem" is making sure all drivers have at least minimal
Runtime PM support, else they can't be reused as-is on systems with PM
domains.

> So all SoC that uses a driver which expects clocks to be managed using
> PM clocks from a PM domain, will need the above "trick". Right?

One remaining issue with pm_clk_add_notifier() is that it applies to all
platform devices in the system, not just the on-SoC devices. Hence it may
inadvertently manage clocks for off-SoC devices it's not supposed to touch.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Apr 29, 2015 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
>>> rmobile. I mean who turns the clocks on for the devices when you build
>>> with CONFIG_PM disabled?
>>
>> We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
>> CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
>> pm_clk_notify() will enable the clocks at driver binding time.
>> Hardware power domains are assumed enabled by reset state/the boot
>> loader.
>
> Yes, it a bit tricky - but I guess that's the current only viable
> solution if we have when using the pm_clk API.
>
>> Given the amount of PM infrastructure involved when hardware power and
>> clock domains are involved, I think at one point we'll have to start restricting
>> our builds to CONFIG_PM=y.
>
> I don't think that would solve the problem, since you may still have
> cross SoC drivers which may be used without CONFIG_PM.

That's not as much of a problem as it sounds:
  - If the driver cares (needs to know) about the clock, it should already
    manage it itself.
  - If it doesn't care about the clock, it just needs Runtime PM support.
    That will do the right thing on platforms with (needs PM=y) and without
    (doesn't care about PM=x) clock domains.
So the bigger "problem" is making sure all drivers have at least minimal
Runtime PM support, else they can't be reused as-is on systems with PM
domains.

> So all SoC that uses a driver which expects clocks to be managed using
> PM clocks from a PM domain, will need the above "trick". Right?

One remaining issue with pm_clk_add_notifier() is that it applies to all
platform devices in the system, not just the on-SoC devices. Hence it may
inadvertently manage clocks for off-SoC devices it's not supposed to touch.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 29 April 2015 at 15:08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Wed, Apr 29, 2015 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
>>>> rmobile. I mean who turns the clocks on for the devices when you build
>>>> with CONFIG_PM disabled?
>>>
>>> We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
>>> CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
>>> pm_clk_notify() will enable the clocks at driver binding time.
>>> Hardware power domains are assumed enabled by reset state/the boot
>>> loader.
>>
>> Yes, it a bit tricky - but I guess that's the current only viable
>> solution if we have when using the pm_clk API.
>>
>>> Given the amount of PM infrastructure involved when hardware power and
>>> clock domains are involved, I think at one point we'll have to start restricting
>>> our builds to CONFIG_PM=y.
>>
>> I don't think that would solve the problem, since you may still have
>> cross SoC drivers which may be used without CONFIG_PM.
>
> That's not as much of a problem as it sounds:
>   - If the driver cares (needs to know) about the clock, it should already
>     manage it itself.

Agree!

>   - If it doesn't care about the clock, it just needs Runtime PM support.
>     That will do the right thing on platforms with (needs PM=y) and without
>     (doesn't care about PM=x) clock domains.

How about those drivers that cares about clocks and thus manages them,
but also cares about runtime PM?

I believe we will get a clock reference count issue in these cases,
since both the PM domain and the driver will manage the clocks.

I assume that's why we have had a few attempts to have "runtime PM
clocks" specially marked, one was via DT, to have clear distinguish
between who will be responsible to manage them.

Those attempts did get nacked, so the problem is still there I assume.

> So the bigger "problem" is making sure all drivers have at least minimal
> Runtime PM support, else they can't be reused as-is on systems with PM
> domains.
>
>> So all SoC that uses a driver which expects clocks to be managed using
>> PM clocks from a PM domain, will need the above "trick". Right?
>
> One remaining issue with pm_clk_add_notifier() is that it applies to all
> platform devices in the system, not just the on-SoC devices. Hence it may
> inadvertently manage clocks for off-SoC devices it's not supposed to touch.

Yes. That's really bad. :-)

Additionally, it means devices that isn't part of the platform bus
isn't able to use PM clk domains at all.

Within this context, I find it hard to advise people to use PM clk
domains (via pm_clk_add_notifier()), since there just so many open
issues. What works a _little_ better is to use PM clks via genpd.

Kind regards
Uffe

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 29 April 2015 at 15:08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Wed, Apr 29, 2015 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
>>>> rmobile. I mean who turns the clocks on for the devices when you build
>>>> with CONFIG_PM disabled?
>>>
>>> We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
>>> CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
>>> pm_clk_notify() will enable the clocks at driver binding time.
>>> Hardware power domains are assumed enabled by reset state/the boot
>>> loader.
>>
>> Yes, it a bit tricky - but I guess that's the current only viable
>> solution if we have when using the pm_clk API.
>>
>>> Given the amount of PM infrastructure involved when hardware power and
>>> clock domains are involved, I think at one point we'll have to start restricting
>>> our builds to CONFIG_PM=y.
>>
>> I don't think that would solve the problem, since you may still have
>> cross SoC drivers which may be used without CONFIG_PM.
>
> That's not as much of a problem as it sounds:
>   - If the driver cares (needs to know) about the clock, it should already
>     manage it itself.

Agree!

>   - If it doesn't care about the clock, it just needs Runtime PM support.
>     That will do the right thing on platforms with (needs PM=y) and without
>     (doesn't care about PM=x) clock domains.

How about those drivers that cares about clocks and thus manages them,
but also cares about runtime PM?

I believe we will get a clock reference count issue in these cases,
since both the PM domain and the driver will manage the clocks.

I assume that's why we have had a few attempts to have "runtime PM
clocks" specially marked, one was via DT, to have clear distinguish
between who will be responsible to manage them.

Those attempts did get nacked, so the problem is still there I assume.

> So the bigger "problem" is making sure all drivers have at least minimal
> Runtime PM support, else they can't be reused as-is on systems with PM
> domains.
>
>> So all SoC that uses a driver which expects clocks to be managed using
>> PM clocks from a PM domain, will need the above "trick". Right?
>
> One remaining issue with pm_clk_add_notifier() is that it applies to all
> platform devices in the system, not just the on-SoC devices. Hence it may
> inadvertently manage clocks for off-SoC devices it's not supposed to touch.

Yes. That's really bad. :-)

Additionally, it means devices that isn't part of the platform bus
isn't able to use PM clk domains at all.

Within this context, I find it hard to advise people to use PM clk
domains (via pm_clk_add_notifier()), since there just so many open
issues. What works a _little_ better is to use PM clks via genpd.

Kind regards
Uffe

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 30 April 2015 at 08:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 29 April 2015 at 15:08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Hi Ulf,
>>
>> On Wed, Apr 29, 2015 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
>>>>> rmobile. I mean who turns the clocks on for the devices when you build
>>>>> with CONFIG_PM disabled?
>>>>
>>>> We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
>>>> CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
>>>> pm_clk_notify() will enable the clocks at driver binding time.
>>>> Hardware power domains are assumed enabled by reset state/the boot
>>>> loader.
>>>
>>> Yes, it a bit tricky - but I guess that's the current only viable
>>> solution if we have when using the pm_clk API.
>>>
>>>> Given the amount of PM infrastructure involved when hardware power and
>>>> clock domains are involved, I think at one point we'll have to start restricting
>>>> our builds to CONFIG_PM=y.
>>>
>>> I don't think that would solve the problem, since you may still have
>>> cross SoC drivers which may be used without CONFIG_PM.
>>
>> That's not as much of a problem as it sounds:
>>   - If the driver cares (needs to know) about the clock, it should already
>>     manage it itself.
>
> Agree!
>
>>   - If it doesn't care about the clock, it just needs Runtime PM support.
>>     That will do the right thing on platforms with (needs PM=y) and without
>>     (doesn't care about PM=x) clock domains.
>
> How about those drivers that cares about clocks and thus manages them,
> but also cares about runtime PM?
>
> I believe we will get a clock reference count issue in these cases,
> since both the PM domain and the driver will manage the clocks.
>
> I assume that's why we have had a few attempts to have "runtime PM
> clocks" specially marked, one was via DT, to have clear distinguish
> between who will be responsible to manage them.
>
> Those attempts did get nacked, so the problem is still there I assume.
>
>> So the bigger "problem" is making sure all drivers have at least minimal
>> Runtime PM support, else they can't be reused as-is on systems with PM
>> domains.
>>
>>> So all SoC that uses a driver which expects clocks to be managed using
>>> PM clocks from a PM domain, will need the above "trick". Right?
>>
>> One remaining issue with pm_clk_add_notifier() is that it applies to all
>> platform devices in the system, not just the on-SoC devices. Hence it may
>> inadvertently manage clocks for off-SoC devices it's not supposed to touch.
>
> Yes. That's really bad. :-)
>
> Additionally, it means devices that isn't part of the platform bus
> isn't able to use PM clk domains at all.

Correction: Of course they can register one PM clk notifier per bus
type. The API currently also provides this option.

>
> Within this context, I find it hard to advise people to use PM clk
> domains (via pm_clk_add_notifier()), since there just so many open
> issues. What works a _little_ better is to use PM clks via genpd.
>
> Kind regards
> Uffe

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 30 April 2015 at 08:21, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 29 April 2015 at 15:08, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Hi Ulf,
>>
>> On Wed, Apr 29, 2015 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> Geert, thanks, I was wondering how you handle the !CONFIG_PM case for
>>>>> rmobile. I mean who turns the clocks on for the devices when you build
>>>>> with CONFIG_PM disabled?
>>>>
>>>> We still use pm_clk_add_notifier() in drivers/sh/pm_runtime.c if
>>>> CONFIG_PM_GENERIC_DOMAINS_OF=n. Hence the second instance of
>>>> pm_clk_notify() will enable the clocks at driver binding time.
>>>> Hardware power domains are assumed enabled by reset state/the boot
>>>> loader.
>>>
>>> Yes, it a bit tricky - but I guess that's the current only viable
>>> solution if we have when using the pm_clk API.
>>>
>>>> Given the amount of PM infrastructure involved when hardware power and
>>>> clock domains are involved, I think at one point we'll have to start restricting
>>>> our builds to CONFIG_PM=y.
>>>
>>> I don't think that would solve the problem, since you may still have
>>> cross SoC drivers which may be used without CONFIG_PM.
>>
>> That's not as much of a problem as it sounds:
>>   - If the driver cares (needs to know) about the clock, it should already
>>     manage it itself.
>
> Agree!
>
>>   - If it doesn't care about the clock, it just needs Runtime PM support.
>>     That will do the right thing on platforms with (needs PM=y) and without
>>     (doesn't care about PM=x) clock domains.
>
> How about those drivers that cares about clocks and thus manages them,
> but also cares about runtime PM?
>
> I believe we will get a clock reference count issue in these cases,
> since both the PM domain and the driver will manage the clocks.
>
> I assume that's why we have had a few attempts to have "runtime PM
> clocks" specially marked, one was via DT, to have clear distinguish
> between who will be responsible to manage them.
>
> Those attempts did get nacked, so the problem is still there I assume.
>
>> So the bigger "problem" is making sure all drivers have at least minimal
>> Runtime PM support, else they can't be reused as-is on systems with PM
>> domains.
>>
>>> So all SoC that uses a driver which expects clocks to be managed using
>>> PM clocks from a PM domain, will need the above "trick". Right?
>>
>> One remaining issue with pm_clk_add_notifier() is that it applies to all
>> platform devices in the system, not just the on-SoC devices. Hence it may
>> inadvertently manage clocks for off-SoC devices it's not supposed to touch.
>
> Yes. That's really bad. :-)
>
> Additionally, it means devices that isn't part of the platform bus
> isn't able to use PM clk domains at all.

Correction: Of course they can register one PM clk notifier per bus
type. The API currently also provides this option.

>
> Within this context, I find it hard to advise people to use PM clk
> domains (via pm_clk_add_notifier()), since there just so many open
> issues. What works a _little_ better is to use PM clks via genpd.
>
> Kind regards
> Uffe

Re: [RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 24 April 2015 at 12:58, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
> On 04/24/2015 03:33 PM, Ulf Hansson wrote:
>>
>> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>
>>> Add runtime PM support to handle (core and iface) clocks for devices
>>> without a controllable power domain. Once the drivers for these devices
>>> are converted to use runtime PM apis, all clock handling (for core and
>>> iface) from these drivers can then be removed.
>>>
>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>> ---
>>>   drivers/clk/qcom/gdsc.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>> index 480ebf6..92b0f6d 100644
>>> --- a/drivers/clk/qcom/gdsc.c
>>> +++ b/drivers/clk/qcom/gdsc.c
>>> @@ -15,6 +15,7 @@
>>>   #include <linux/err.h>
>>>   #include <linux/jiffies.h>
>>>   #include <linux/pm_clock.h>
>>> +#include <linux/platform_device.h>
>>>   #include <linux/slab.h>
>>>   #include "gdsc.h"
>>>
>>> @@ -226,3 +227,22 @@ void gdsc_unregister(struct device *dev)
>>>   {
>>>          of_genpd_del_provider(dev->of_node);
>>>   }
>>> +
>>> +static struct dev_pm_domain default_qcom_pm_domain = {
>>> +       .ops = {
>>> +               USE_PM_CLK_RUNTIME_OPS
>>> +               USE_PLATFORM_PM_SLEEP_OPS
>>> +       },
>>> +};
>>> +
>>> +static struct pm_clk_notifier_block qcom_pm_notifier = {
>>> +       .pm_domain      = &default_qcom_pm_domain,
>>> +       .con_ids        = { "core", "iface" },
>>> +};
>>> +
>>> +static int __init qcom_pm_runtime_init(void)
>>> +{
>>> +       pm_clk_add_notifier(&platform_bus_type, &qcom_pm_notifier);
>>> +       return 0;
>>> +}
>>> +core_initcall(qcom_pm_runtime_init);
>>
>>
>> First, I don't follow how this code is related to GDSC.
>
>
> Actually its not. I should probably move it to a pm_runtime.c someplace
> in drivers/soc/qcom maybe.
>
>>
>> Second, I want to see less users of pm_clk_add_notifier() since it's
>> not the proper/long-term way of how to assign PM domain pointers to a
>> device. Instead that shall be done at device registration point. In
>> your case while parsing the DT nodes and adding devices onto to their
>> buses.
>
>
> but these are devices which do not really have a controllable power
> domain, they just have controllable clocks.

That's true. But I don't see an issue why we shouldn't allow to model
this in DT through the existing bindings.

If the DT guys think it's a bad idea, I will anyway think the proper
way to assign PM domains pointers is at device registration point.

>
>>
>> Yes, I understand that it will requires quite some work to adopt to
>> this behaviour - but that how it shall be done.
>>
>> Finally, an important note, you don't need to use PM domains for these
>> devices at all and thus you don't need to fix what I propose. Instead
>> you only have to implement the runtime PM callbacks for each driver
>> and manage the clocks from there. That is probably also a easier
>> solution.
>
>
> But that would mean I repeat the same code in all drivers to do a
> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
> clocks. I thought we have clock_ops.c just to avoid that (atleast up
> until we have a better way of doing it)
> And there are atleast a few architecture which have used it to avoid the
> duplication across all drivers (omap1/davinci/sh/keystone)

Yes, the "duplications" are unavoidable if you decide to follow my suggestion.

On the other hand your drivers will be more "standalone" and not
depending on that you have a PM domain attached to the device to
actually work. In some cases that might even be useful, when you have
a cross SOC driver used in various configurations.

Kind regards
Uffe

[RFC/RFT 2/6] clk: qcom: Add runtime support to handle clocks using PM clocks

[Ulf Hansson]

On 24 April 2015 at 12:58, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
> On 04/24/2015 03:33 PM, Ulf Hansson wrote:
>>
>> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>
>>> Add runtime PM support to handle (core and iface) clocks for devices
>>> without a controllable power domain. Once the drivers for these devices
>>> are converted to use runtime PM apis, all clock handling (for core and
>>> iface) from these drivers can then be removed.
>>>
>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>> ---
>>>   drivers/clk/qcom/gdsc.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>> index 480ebf6..92b0f6d 100644
>>> --- a/drivers/clk/qcom/gdsc.c
>>> +++ b/drivers/clk/qcom/gdsc.c
>>> @@ -15,6 +15,7 @@
>>>   #include <linux/err.h>
>>>   #include <linux/jiffies.h>
>>>   #include <linux/pm_clock.h>
>>> +#include <linux/platform_device.h>
>>>   #include <linux/slab.h>
>>>   #include "gdsc.h"
>>>
>>> @@ -226,3 +227,22 @@ void gdsc_unregister(struct device *dev)
>>>   {
>>>          of_genpd_del_provider(dev->of_node);
>>>   }
>>> +
>>> +static struct dev_pm_domain default_qcom_pm_domain = {
>>> +       .ops = {
>>> +               USE_PM_CLK_RUNTIME_OPS
>>> +               USE_PLATFORM_PM_SLEEP_OPS
>>> +       },
>>> +};
>>> +
>>> +static struct pm_clk_notifier_block qcom_pm_notifier = {
>>> +       .pm_domain      = &default_qcom_pm_domain,
>>> +       .con_ids        = { "core", "iface" },
>>> +};
>>> +
>>> +static int __init qcom_pm_runtime_init(void)
>>> +{
>>> +       pm_clk_add_notifier(&platform_bus_type, &qcom_pm_notifier);
>>> +       return 0;
>>> +}
>>> +core_initcall(qcom_pm_runtime_init);
>>
>>
>> First, I don't follow how this code is related to GDSC.
>
>
> Actually its not. I should probably move it to a pm_runtime.c someplace
> in drivers/soc/qcom maybe.
>
>>
>> Second, I want to see less users of pm_clk_add_notifier() since it's
>> not the proper/long-term way of how to assign PM domain pointers to a
>> device. Instead that shall be done at device registration point. In
>> your case while parsing the DT nodes and adding devices onto to their
>> buses.
>
>
> but these are devices which do not really have a controllable power
> domain, they just have controllable clocks.

That's true. But I don't see an issue why we shouldn't allow to model
this in DT through the existing bindings.

If the DT guys think it's a bad idea, I will anyway think the proper
way to assign PM domains pointers is at device registration point.

>
>>
>> Yes, I understand that it will requires quite some work to adopt to
>> this behaviour - but that how it shall be done.
>>
>> Finally, an important note, you don't need to use PM domains for these
>> devices at all and thus you don't need to fix what I propose. Instead
>> you only have to implement the runtime PM callbacks for each driver
>> and manage the clocks from there. That is probably also a easier
>> solution.
>
>
> But that would mean I repeat the same code in all drivers to do a
> clk_get/prepare/enable/disable/unprepare of the same "core" and "iface"
> clocks. I thought we have clock_ops.c just to avoid that (atleast up
> until we have a better way of doing it)
> And there are atleast a few architecture which have used it to avoid the
> duplication across all drivers (omap1/davinci/sh/keystone)

Yes, the "duplications" are unavoidable if you decide to follow my suggestion.

On the other hand your drivers will be more "standalone" and not
depending on that you have a PM domain attached to the device to
actually work. In some cases that might even be useful, when you have
a cross SOC driver used in various configurations.

Kind regards
Uffe

[RFC/RFT 3/6] serial: msm: convert driver to use runtime PM apis

[Rajendra Nayak]

With platform support now in place to manage clocks from within runtime PM
callbacks, get rid of all clock handling from the driver and convert the
driver to use runtime PM apis.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Cc: <linux-serial@vger.kernel.org>
---
 drivers/tty/serial/msm_serial.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index b73889c..eb66502 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -33,6 +33,7 @@
 #include <linux/serial.h>
 #include <linux/clk.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/delay.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -50,11 +51,11 @@ struct msm_port {
 	struct uart_port	uart;
 	char			name[16];
 	struct clk		*clk;
-	struct clk		*pclk;
 	unsigned int		imr;
 	int			is_uartdm;
 	unsigned int		old_snap_state;
 	bool			break_detected;
+	struct device		*dev;
 };
 
 static inline void wait_for_xmitr(struct uart_port *port)
@@ -464,12 +465,11 @@ static int msm_set_baud_rate(struct uart_port *port, unsigned int baud)
 	return baud;
 }
 
-static void msm_init_clock(struct uart_port *port)
+static void msm_init(struct uart_port *port)
 {
 	struct msm_port *msm_port = UART_TO_MSM(port);
 
-	clk_prepare_enable(msm_port->clk);
-	clk_prepare_enable(msm_port->pclk);
+	pm_runtime_get_sync(msm_port->dev);
 	msm_serial_set_mnd_regs(port);
 }
 
@@ -487,7 +487,7 @@ static int msm_startup(struct uart_port *port)
 	if (unlikely(ret))
 		return ret;
 
-	msm_init_clock(port);
+	msm_init(port);
 
 	if (likely(port->fifosize > 12))
 		rfr_level = port->fifosize - 12;
@@ -511,7 +511,7 @@ static void msm_shutdown(struct uart_port *port)
 	msm_port->imr = 0;
 	msm_write(port, 0, UART_IMR); /* disable interrupts */
 
-	clk_disable_unprepare(msm_port->clk);
+	pm_runtime_put_sync(msm_port->dev);
 
 	free_irq(port->irq, port);
 }
@@ -669,12 +669,10 @@ static void msm_power(struct uart_port *port, unsigned int state,
 
 	switch (state) {
 	case 0:
-		clk_prepare_enable(msm_port->clk);
-		clk_prepare_enable(msm_port->pclk);
+		pm_runtime_get_sync(msm_port->dev);
 		break;
 	case 3:
-		clk_disable_unprepare(msm_port->clk);
-		clk_disable_unprepare(msm_port->pclk);
+		pm_runtime_put_sync(msm_port->dev);
 		break;
 	default:
 		pr_err("msm_serial: Unknown PM state %d\n", state);
@@ -933,7 +931,7 @@ static int __init msm_console_setup(struct console *co, char *options)
 	if (unlikely(!port->membase))
 		return -ENXIO;
 
-	msm_init_clock(port);
+	msm_init(port);
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
@@ -1057,13 +1055,10 @@ static int msm_serial_probe(struct platform_device *pdev)
 	if (IS_ERR(msm_port->clk))
 		return PTR_ERR(msm_port->clk);
 
-	if (msm_port->is_uartdm) {
-		msm_port->pclk = devm_clk_get(&pdev->dev, "iface");
-		if (IS_ERR(msm_port->pclk))
-			return PTR_ERR(msm_port->pclk);
-
+	if (msm_port->is_uartdm)
 		clk_set_rate(msm_port->clk, 1843200);
-	}
+
+	msm_port->dev = &pdev->dev;
 
 	port->uartclk = clk_get_rate(msm_port->clk);
 	dev_info(&pdev->dev, "uartclk = %d\n", port->uartclk);
@@ -1080,13 +1075,17 @@ static int msm_serial_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, port);
 
+	pm_runtime_enable(&pdev->dev);
+
 	return uart_add_one_port(&msm_uart_driver, port);
 }
 
 static int msm_serial_remove(struct platform_device *pdev)
 {
 	struct uart_port *port = platform_get_drvdata(pdev);
+	struct msm_port *msm_port = UART_TO_MSM(port);
 
+	pm_runtime_disable(msm_port->dev);
 	uart_remove_one_port(&msm_uart_driver, port);
 
 	return 0;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC/RFT 3/6] serial: msm: convert driver to use runtime PM apis

[Rajendra Nayak]

With platform support now in place to manage clocks from within runtime PM
callbacks, get rid of all clock handling from the driver and convert the
driver to use runtime PM apis.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Cc: <linux-serial@vger.kernel.org>
---
 drivers/tty/serial/msm_serial.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index b73889c..eb66502 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -33,6 +33,7 @@
 #include <linux/serial.h>
 #include <linux/clk.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/delay.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -50,11 +51,11 @@ struct msm_port {
 	struct uart_port	uart;
 	char			name[16];
 	struct clk		*clk;
-	struct clk		*pclk;
 	unsigned int		imr;
 	int			is_uartdm;
 	unsigned int		old_snap_state;
 	bool			break_detected;
+	struct device		*dev;
 };
 
 static inline void wait_for_xmitr(struct uart_port *port)
@@ -464,12 +465,11 @@ static int msm_set_baud_rate(struct uart_port *port, unsigned int baud)
 	return baud;
 }
 
-static void msm_init_clock(struct uart_port *port)
+static void msm_init(struct uart_port *port)
 {
 	struct msm_port *msm_port = UART_TO_MSM(port);
 
-	clk_prepare_enable(msm_port->clk);
-	clk_prepare_enable(msm_port->pclk);
+	pm_runtime_get_sync(msm_port->dev);
 	msm_serial_set_mnd_regs(port);
 }
 
@@ -487,7 +487,7 @@ static int msm_startup(struct uart_port *port)
 	if (unlikely(ret))
 		return ret;
 
-	msm_init_clock(port);
+	msm_init(port);
 
 	if (likely(port->fifosize > 12))
 		rfr_level = port->fifosize - 12;
@@ -511,7 +511,7 @@ static void msm_shutdown(struct uart_port *port)
 	msm_port->imr = 0;
 	msm_write(port, 0, UART_IMR); /* disable interrupts */
 
-	clk_disable_unprepare(msm_port->clk);
+	pm_runtime_put_sync(msm_port->dev);
 
 	free_irq(port->irq, port);
 }
@@ -669,12 +669,10 @@ static void msm_power(struct uart_port *port, unsigned int state,
 
 	switch (state) {
 	case 0:
-		clk_prepare_enable(msm_port->clk);
-		clk_prepare_enable(msm_port->pclk);
+		pm_runtime_get_sync(msm_port->dev);
 		break;
 	case 3:
-		clk_disable_unprepare(msm_port->clk);
-		clk_disable_unprepare(msm_port->pclk);
+		pm_runtime_put_sync(msm_port->dev);
 		break;
 	default:
 		pr_err("msm_serial: Unknown PM state %d\n", state);
@@ -933,7 +931,7 @@ static int __init msm_console_setup(struct console *co, char *options)
 	if (unlikely(!port->membase))
 		return -ENXIO;
 
-	msm_init_clock(port);
+	msm_init(port);
 
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
@@ -1057,13 +1055,10 @@ static int msm_serial_probe(struct platform_device *pdev)
 	if (IS_ERR(msm_port->clk))
 		return PTR_ERR(msm_port->clk);
 
-	if (msm_port->is_uartdm) {
-		msm_port->pclk = devm_clk_get(&pdev->dev, "iface");
-		if (IS_ERR(msm_port->pclk))
-			return PTR_ERR(msm_port->pclk);
-
+	if (msm_port->is_uartdm)
 		clk_set_rate(msm_port->clk, 1843200);
-	}
+
+	msm_port->dev = &pdev->dev;
 
 	port->uartclk = clk_get_rate(msm_port->clk);
 	dev_info(&pdev->dev, "uartclk = %d\n", port->uartclk);
@@ -1080,13 +1075,17 @@ static int msm_serial_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, port);
 
+	pm_runtime_enable(&pdev->dev);
+
 	return uart_add_one_port(&msm_uart_driver, port);
 }
 
 static int msm_serial_remove(struct platform_device *pdev)
 {
 	struct uart_port *port = platform_get_drvdata(pdev);
+	struct msm_port *msm_port = UART_TO_MSM(port);
 
+	pm_runtime_disable(msm_port->dev);
 	uart_remove_one_port(&msm_uart_driver, port);
 
 	return 0;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC/RFT 3/6] serial: msm: convert driver to use runtime PM apis

[Stephen Boyd]

On 04/23/15 01:45, Rajendra Nayak wrote:
> With platform support now in place to manage clocks from within runtime PM
> callbacks, get rid of all clock handling from the driver and convert the
> driver to use runtime PM apis.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Cc: <linux-serial@vger.kernel.org>

Pramod is changing the same file in the same area. Please work together
if necessary.

-Stephen

> ---
>  drivers/tty/serial/msm_serial.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index b73889c..eb66502 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -33,6 +33,7 @@
>  #include <linux/serial.h>
>  #include <linux/clk.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -50,11 +51,11 @@ struct msm_port {
>  	struct uart_port	uart;
>  	char			name[16];
>  	struct clk		*clk;
> -	struct clk		*pclk;
>  	unsigned int		imr;
>  	int			is_uartdm;
>  	unsigned int		old_snap_state;
>  	bool			break_detected;
> +	struct device		*dev;
>  };
>  
>  static inline void wait_for_xmitr(struct uart_port *port)
> @@ -464,12 +465,11 @@ static int msm_set_baud_rate(struct uart_port *port, unsigned int baud)
>  	return baud;
>  }
>  
> -static void msm_init_clock(struct uart_port *port)
> +static void msm_init(struct uart_port *port)
>  {
>  	struct msm_port *msm_port = UART_TO_MSM(port);
>  
> -	clk_prepare_enable(msm_port->clk);
> -	clk_prepare_enable(msm_port->pclk);
> +	pm_runtime_get_sync(msm_port->dev);
>  	msm_serial_set_mnd_regs(port);
>  }
>  
> @@ -487,7 +487,7 @@ static int msm_startup(struct uart_port *port)
>  	if (unlikely(ret))
>  		return ret;
>  
> -	msm_init_clock(port);
> +	msm_init(port);
>  
>  	if (likely(port->fifosize > 12))
>  		rfr_level = port->fifosize - 12;
> @@ -511,7 +511,7 @@ static void msm_shutdown(struct uart_port *port)
>  	msm_port->imr = 0;
>  	msm_write(port, 0, UART_IMR); /* disable interrupts */
>  
> -	clk_disable_unprepare(msm_port->clk);
> +	pm_runtime_put_sync(msm_port->dev);
>  
>  	free_irq(port->irq, port);
>  }
> @@ -669,12 +669,10 @@ static void msm_power(struct uart_port *port, unsigned int state,
>  
>  	switch (state) {
>  	case 0:
> -		clk_prepare_enable(msm_port->clk);
> -		clk_prepare_enable(msm_port->pclk);
> +		pm_runtime_get_sync(msm_port->dev);
>  		break;
>  	case 3:
> -		clk_disable_unprepare(msm_port->clk);
> -		clk_disable_unprepare(msm_port->pclk);
> +		pm_runtime_put_sync(msm_port->dev);
>  		break;
>  	default:
>  		pr_err("msm_serial: Unknown PM state %d\n", state);
> @@ -933,7 +931,7 @@ static int __init msm_console_setup(struct console *co, char *options)
>  	if (unlikely(!port->membase))
>  		return -ENXIO;
>  
> -	msm_init_clock(port);
> +	msm_init(port);
>  
>  	if (options)
>  		uart_parse_options(options, &baud, &parity, &bits, &flow);
> @@ -1057,13 +1055,10 @@ static int msm_serial_probe(struct platform_device *pdev)
>  	if (IS_ERR(msm_port->clk))
>  		return PTR_ERR(msm_port->clk);
>  
> -	if (msm_port->is_uartdm) {
> -		msm_port->pclk = devm_clk_get(&pdev->dev, "iface");
> -		if (IS_ERR(msm_port->pclk))
> -			return PTR_ERR(msm_port->pclk);
> -
> +	if (msm_port->is_uartdm)
>  		clk_set_rate(msm_port->clk, 1843200);
> -	}
> +
> +	msm_port->dev = &pdev->dev;
>  
>  	port->uartclk = clk_get_rate(msm_port->clk);
>  	dev_info(&pdev->dev, "uartclk = %d\n", port->uartclk);
> @@ -1080,13 +1075,17 @@ static int msm_serial_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, port);
>  
> +	pm_runtime_enable(&pdev->dev);
> +
>  	return uart_add_one_port(&msm_uart_driver, port);
>  }
>  
>  static int msm_serial_remove(struct platform_device *pdev)
>  {
>  	struct uart_port *port = platform_get_drvdata(pdev);
> +	struct msm_port *msm_port = UART_TO_MSM(port);
>  
> +	pm_runtime_disable(msm_port->dev);
>  	uart_remove_one_port(&msm_uart_driver, port);
>  
>  	return 0;


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

[RFC/RFT 3/6] serial: msm: convert driver to use runtime PM apis

[Stephen Boyd]

On 04/23/15 01:45, Rajendra Nayak wrote:
> With platform support now in place to manage clocks from within runtime PM
> callbacks, get rid of all clock handling from the driver and convert the
> driver to use runtime PM apis.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Cc: <linux-serial@vger.kernel.org>

Pramod is changing the same file in the same area. Please work together
if necessary.

-Stephen

> ---
>  drivers/tty/serial/msm_serial.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index b73889c..eb66502 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -33,6 +33,7 @@
>  #include <linux/serial.h>
>  #include <linux/clk.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -50,11 +51,11 @@ struct msm_port {
>  	struct uart_port	uart;
>  	char			name[16];
>  	struct clk		*clk;
> -	struct clk		*pclk;
>  	unsigned int		imr;
>  	int			is_uartdm;
>  	unsigned int		old_snap_state;
>  	bool			break_detected;
> +	struct device		*dev;
>  };
>  
>  static inline void wait_for_xmitr(struct uart_port *port)
> @@ -464,12 +465,11 @@ static int msm_set_baud_rate(struct uart_port *port, unsigned int baud)
>  	return baud;
>  }
>  
> -static void msm_init_clock(struct uart_port *port)
> +static void msm_init(struct uart_port *port)
>  {
>  	struct msm_port *msm_port = UART_TO_MSM(port);
>  
> -	clk_prepare_enable(msm_port->clk);
> -	clk_prepare_enable(msm_port->pclk);
> +	pm_runtime_get_sync(msm_port->dev);
>  	msm_serial_set_mnd_regs(port);
>  }
>  
> @@ -487,7 +487,7 @@ static int msm_startup(struct uart_port *port)
>  	if (unlikely(ret))
>  		return ret;
>  
> -	msm_init_clock(port);
> +	msm_init(port);
>  
>  	if (likely(port->fifosize > 12))
>  		rfr_level = port->fifosize - 12;
> @@ -511,7 +511,7 @@ static void msm_shutdown(struct uart_port *port)
>  	msm_port->imr = 0;
>  	msm_write(port, 0, UART_IMR); /* disable interrupts */
>  
> -	clk_disable_unprepare(msm_port->clk);
> +	pm_runtime_put_sync(msm_port->dev);
>  
>  	free_irq(port->irq, port);
>  }
> @@ -669,12 +669,10 @@ static void msm_power(struct uart_port *port, unsigned int state,
>  
>  	switch (state) {
>  	case 0:
> -		clk_prepare_enable(msm_port->clk);
> -		clk_prepare_enable(msm_port->pclk);
> +		pm_runtime_get_sync(msm_port->dev);
>  		break;
>  	case 3:
> -		clk_disable_unprepare(msm_port->clk);
> -		clk_disable_unprepare(msm_port->pclk);
> +		pm_runtime_put_sync(msm_port->dev);
>  		break;
>  	default:
>  		pr_err("msm_serial: Unknown PM state %d\n", state);
> @@ -933,7 +931,7 @@ static int __init msm_console_setup(struct console *co, char *options)
>  	if (unlikely(!port->membase))
>  		return -ENXIO;
>  
> -	msm_init_clock(port);
> +	msm_init(port);
>  
>  	if (options)
>  		uart_parse_options(options, &baud, &parity, &bits, &flow);
> @@ -1057,13 +1055,10 @@ static int msm_serial_probe(struct platform_device *pdev)
>  	if (IS_ERR(msm_port->clk))
>  		return PTR_ERR(msm_port->clk);
>  
> -	if (msm_port->is_uartdm) {
> -		msm_port->pclk = devm_clk_get(&pdev->dev, "iface");
> -		if (IS_ERR(msm_port->pclk))
> -			return PTR_ERR(msm_port->pclk);
> -
> +	if (msm_port->is_uartdm)
>  		clk_set_rate(msm_port->clk, 1843200);
> -	}
> +
> +	msm_port->dev = &pdev->dev;
>  
>  	port->uartclk = clk_get_rate(msm_port->clk);
>  	dev_info(&pdev->dev, "uartclk = %d\n", port->uartclk);
> @@ -1080,13 +1075,17 @@ static int msm_serial_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, port);
>  
> +	pm_runtime_enable(&pdev->dev);
> +
>  	return uart_add_one_port(&msm_uart_driver, port);
>  }
>  
>  static int msm_serial_remove(struct platform_device *pdev)
>  {
>  	struct uart_port *port = platform_get_drvdata(pdev);
> +	struct msm_port *msm_port = UART_TO_MSM(port);
>  
> +	pm_runtime_disable(msm_port->dev);
>  	uart_remove_one_port(&msm_uart_driver, port);
>  
>  	return 0;


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

Re: [RFC/RFT 3/6] serial: msm: convert driver to use runtime PM apis

[Rajendra Nayak]

On 04/29/2015 05:46 AM, Stephen Boyd wrote:
> On 04/23/15 01:45, Rajendra Nayak wrote:
>> With platform support now in place to manage clocks from within runtime PM
>> callbacks, get rid of all clock handling from the driver and convert the
>> driver to use runtime PM apis.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Cc: <linux-serial@vger.kernel.org>
>
> Pramod is changing the same file in the same area. Please work together
> if necessary.

Sure, will do. For some reason I thought those changes already made it
in 4.1, but maybe not.

>
> -Stephen
>
>> ---
>>   drivers/tty/serial/msm_serial.c | 33 ++++++++++++++++-----------------
>>   1 file changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>> index b73889c..eb66502 100644
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -33,6 +33,7 @@
>>   #include <linux/serial.h>
>>   #include <linux/clk.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>   #include <linux/delay.h>
>>   #include <linux/of.h>
>>   #include <linux/of_device.h>
>> @@ -50,11 +51,11 @@ struct msm_port {
>>   	struct uart_port	uart;
>>   	char			name[16];
>>   	struct clk		*clk;
>> -	struct clk		*pclk;
>>   	unsigned int		imr;
>>   	int			is_uartdm;
>>   	unsigned int		old_snap_state;
>>   	bool			break_detected;
>> +	struct device		*dev;
>>   };
>>
>>   static inline void wait_for_xmitr(struct uart_port *port)
>> @@ -464,12 +465,11 @@ static int msm_set_baud_rate(struct uart_port *port, unsigned int baud)
>>   	return baud;
>>   }
>>
>> -static void msm_init_clock(struct uart_port *port)
>> +static void msm_init(struct uart_port *port)
>>   {
>>   	struct msm_port *msm_port = UART_TO_MSM(port);
>>
>> -	clk_prepare_enable(msm_port->clk);
>> -	clk_prepare_enable(msm_port->pclk);
>> +	pm_runtime_get_sync(msm_port->dev);
>>   	msm_serial_set_mnd_regs(port);
>>   }
>>
>> @@ -487,7 +487,7 @@ static int msm_startup(struct uart_port *port)
>>   	if (unlikely(ret))
>>   		return ret;
>>
>> -	msm_init_clock(port);
>> +	msm_init(port);
>>
>>   	if (likely(port->fifosize > 12))
>>   		rfr_level = port->fifosize - 12;
>> @@ -511,7 +511,7 @@ static void msm_shutdown(struct uart_port *port)
>>   	msm_port->imr = 0;
>>   	msm_write(port, 0, UART_IMR); /* disable interrupts */
>>
>> -	clk_disable_unprepare(msm_port->clk);
>> +	pm_runtime_put_sync(msm_port->dev);
>>
>>   	free_irq(port->irq, port);
>>   }
>> @@ -669,12 +669,10 @@ static void msm_power(struct uart_port *port, unsigned int state,
>>
>>   	switch (state) {
>>   	case 0:
>> -		clk_prepare_enable(msm_port->clk);
>> -		clk_prepare_enable(msm_port->pclk);
>> +		pm_runtime_get_sync(msm_port->dev);
>>   		break;
>>   	case 3:
>> -		clk_disable_unprepare(msm_port->clk);
>> -		clk_disable_unprepare(msm_port->pclk);
>> +		pm_runtime_put_sync(msm_port->dev);
>>   		break;
>>   	default:
>>   		pr_err("msm_serial: Unknown PM state %d\n", state);
>> @@ -933,7 +931,7 @@ static int __init msm_console_setup(struct console *co, char *options)
>>   	if (unlikely(!port->membase))
>>   		return -ENXIO;
>>
>> -	msm_init_clock(port);
>> +	msm_init(port);
>>
>>   	if (options)
>>   		uart_parse_options(options, &baud, &parity, &bits, &flow);
>> @@ -1057,13 +1055,10 @@ static int msm_serial_probe(struct platform_device *pdev)
>>   	if (IS_ERR(msm_port->clk))
>>   		return PTR_ERR(msm_port->clk);
>>
>> -	if (msm_port->is_uartdm) {
>> -		msm_port->pclk = devm_clk_get(&pdev->dev, "iface");
>> -		if (IS_ERR(msm_port->pclk))
>> -			return PTR_ERR(msm_port->pclk);
>> -
>> +	if (msm_port->is_uartdm)
>>   		clk_set_rate(msm_port->clk, 1843200);
>> -	}
>> +
>> +	msm_port->dev = &pdev->dev;
>>
>>   	port->uartclk = clk_get_rate(msm_port->clk);
>>   	dev_info(&pdev->dev, "uartclk = %d\n", port->uartclk);
>> @@ -1080,13 +1075,17 @@ static int msm_serial_probe(struct platform_device *pdev)
>>
>>   	platform_set_drvdata(pdev, port);
>>
>> +	pm_runtime_enable(&pdev->dev);
>> +
>>   	return uart_add_one_port(&msm_uart_driver, port);
>>   }
>>
>>   static int msm_serial_remove(struct platform_device *pdev)
>>   {
>>   	struct uart_port *port = platform_get_drvdata(pdev);
>> +	struct msm_port *msm_port = UART_TO_MSM(port);
>>
>> +	pm_runtime_disable(msm_port->dev);
>>   	uart_remove_one_port(&msm_uart_driver, port);
>>
>>   	return 0;
>
>

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

[RFC/RFT 3/6] serial: msm: convert driver to use runtime PM apis

[Rajendra Nayak]

On 04/29/2015 05:46 AM, Stephen Boyd wrote:
> On 04/23/15 01:45, Rajendra Nayak wrote:
>> With platform support now in place to manage clocks from within runtime PM
>> callbacks, get rid of all clock handling from the driver and convert the
>> driver to use runtime PM apis.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Cc: <linux-serial@vger.kernel.org>
>
> Pramod is changing the same file in the same area. Please work together
> if necessary.

Sure, will do. For some reason I thought those changes already made it
in 4.1, but maybe not.

>
> -Stephen
>
>> ---
>>   drivers/tty/serial/msm_serial.c | 33 ++++++++++++++++-----------------
>>   1 file changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>> index b73889c..eb66502 100644
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -33,6 +33,7 @@
>>   #include <linux/serial.h>
>>   #include <linux/clk.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>   #include <linux/delay.h>
>>   #include <linux/of.h>
>>   #include <linux/of_device.h>
>> @@ -50,11 +51,11 @@ struct msm_port {
>>   	struct uart_port	uart;
>>   	char			name[16];
>>   	struct clk		*clk;
>> -	struct clk		*pclk;
>>   	unsigned int		imr;
>>   	int			is_uartdm;
>>   	unsigned int		old_snap_state;
>>   	bool			break_detected;
>> +	struct device		*dev;
>>   };
>>
>>   static inline void wait_for_xmitr(struct uart_port *port)
>> @@ -464,12 +465,11 @@ static int msm_set_baud_rate(struct uart_port *port, unsigned int baud)
>>   	return baud;
>>   }
>>
>> -static void msm_init_clock(struct uart_port *port)
>> +static void msm_init(struct uart_port *port)
>>   {
>>   	struct msm_port *msm_port = UART_TO_MSM(port);
>>
>> -	clk_prepare_enable(msm_port->clk);
>> -	clk_prepare_enable(msm_port->pclk);
>> +	pm_runtime_get_sync(msm_port->dev);
>>   	msm_serial_set_mnd_regs(port);
>>   }
>>
>> @@ -487,7 +487,7 @@ static int msm_startup(struct uart_port *port)
>>   	if (unlikely(ret))
>>   		return ret;
>>
>> -	msm_init_clock(port);
>> +	msm_init(port);
>>
>>   	if (likely(port->fifosize > 12))
>>   		rfr_level = port->fifosize - 12;
>> @@ -511,7 +511,7 @@ static void msm_shutdown(struct uart_port *port)
>>   	msm_port->imr = 0;
>>   	msm_write(port, 0, UART_IMR); /* disable interrupts */
>>
>> -	clk_disable_unprepare(msm_port->clk);
>> +	pm_runtime_put_sync(msm_port->dev);
>>
>>   	free_irq(port->irq, port);
>>   }
>> @@ -669,12 +669,10 @@ static void msm_power(struct uart_port *port, unsigned int state,
>>
>>   	switch (state) {
>>   	case 0:
>> -		clk_prepare_enable(msm_port->clk);
>> -		clk_prepare_enable(msm_port->pclk);
>> +		pm_runtime_get_sync(msm_port->dev);
>>   		break;
>>   	case 3:
>> -		clk_disable_unprepare(msm_port->clk);
>> -		clk_disable_unprepare(msm_port->pclk);
>> +		pm_runtime_put_sync(msm_port->dev);
>>   		break;
>>   	default:
>>   		pr_err("msm_serial: Unknown PM state %d\n", state);
>> @@ -933,7 +931,7 @@ static int __init msm_console_setup(struct console *co, char *options)
>>   	if (unlikely(!port->membase))
>>   		return -ENXIO;
>>
>> -	msm_init_clock(port);
>> +	msm_init(port);
>>
>>   	if (options)
>>   		uart_parse_options(options, &baud, &parity, &bits, &flow);
>> @@ -1057,13 +1055,10 @@ static int msm_serial_probe(struct platform_device *pdev)
>>   	if (IS_ERR(msm_port->clk))
>>   		return PTR_ERR(msm_port->clk);
>>
>> -	if (msm_port->is_uartdm) {
>> -		msm_port->pclk = devm_clk_get(&pdev->dev, "iface");
>> -		if (IS_ERR(msm_port->pclk))
>> -			return PTR_ERR(msm_port->pclk);
>> -
>> +	if (msm_port->is_uartdm)
>>   		clk_set_rate(msm_port->clk, 1843200);
>> -	}
>> +
>> +	msm_port->dev = &pdev->dev;
>>
>>   	port->uartclk = clk_get_rate(msm_port->clk);
>>   	dev_info(&pdev->dev, "uartclk = %d\n", port->uartclk);
>> @@ -1080,13 +1075,17 @@ static int msm_serial_probe(struct platform_device *pdev)
>>
>>   	platform_set_drvdata(pdev, port);
>>
>> +	pm_runtime_enable(&pdev->dev);
>> +
>>   	return uart_add_one_port(&msm_uart_driver, port);
>>   }
>>
>>   static int msm_serial_remove(struct platform_device *pdev)
>>   {
>>   	struct uart_port *port = platform_get_drvdata(pdev);
>> +	struct msm_port *msm_port = UART_TO_MSM(port);
>>
>> +	pm_runtime_disable(msm_port->dev);
>>   	uart_remove_one_port(&msm_uart_driver, port);
>>
>>   	return 0;
>
>

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

[RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Rajendra Nayak]

With platform support now in place to manage clocks from within runtime PM
callbacks, get rid of all clock handling from the driver and convert the
driver to use runtime PM apis.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Cc: <linux-mmc@vger.kernel.org>
---
 drivers/mmc/host/sdhci-msm.c | 46 +++++++++++---------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 4a09f76..3c62a77 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -19,6 +19,7 @@
 #include <linux/delay.h>
 #include <linux/mmc/mmc.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 
 #include "sdhci-pltfm.h"
 
@@ -56,8 +57,6 @@
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
-	struct clk *clk;	/* main SD/MMC bus clock */
-	struct clk *pclk;	/* SDHC peripheral bus clock */
 	struct clk *bus_clk;	/* SDHC bus voter clock */
 	struct mmc_host *mmc;
 	struct sdhci_pltfm_data sdhci_msm_pdata;
@@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 			goto pltfm_free;
 	}
 
-	/* Setup main peripheral bus clock */
-	msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
-	if (IS_ERR(msm_host->pclk)) {
-		ret = PTR_ERR(msm_host->pclk);
-		dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n", ret);
-		goto bus_clk_disable;
-	}
-
-	ret = clk_prepare_enable(msm_host->pclk);
-	if (ret)
-		goto bus_clk_disable;
-
-	/* Setup SDC MMC clock */
-	msm_host->clk = devm_clk_get(&pdev->dev, "core");
-	if (IS_ERR(msm_host->clk)) {
-		ret = PTR_ERR(msm_host->clk);
-		dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n", ret);
-		goto pclk_disable;
-	}
-
-	ret = clk_prepare_enable(msm_host->clk);
-	if (ret)
-		goto pclk_disable;
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
 
 	core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
@@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	if (IS_ERR(msm_host->core_mem)) {
 		dev_err(&pdev->dev, "Failed to remap registers\n");
 		ret = PTR_ERR(msm_host->core_mem);
-		goto clk_disable;
+		goto err;
 	}
 
 	/* Reset the core and Enable SDHC mode */
@@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
 		dev_err(&pdev->dev, "Stuck in reset\n");
 		ret = -ETIMEDOUT;
-		goto clk_disable;
+		goto err;
 	}
 
 	/* Set HC_MODE_EN bit in HC_MODE register */
@@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	ret = sdhci_add_host(host);
 	if (ret)
-		goto clk_disable;
+		goto err;
 
 	return 0;
 
-clk_disable:
-	clk_disable_unprepare(msm_host->clk);
-pclk_disable:
-	clk_disable_unprepare(msm_host->pclk);
-bus_clk_disable:
+err:
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	if (!IS_ERR(msm_host->bus_clk))
 		clk_disable_unprepare(msm_host->bus_clk);
 pltfm_free:
@@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device *pdev)
 
 	sdhci_remove_host(host, dead);
 	sdhci_pltfm_free(pdev);
-	clk_disable_unprepare(msm_host->clk);
-	clk_disable_unprepare(msm_host->pclk);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	if (!IS_ERR(msm_host->bus_clk))
 		clk_disable_unprepare(msm_host->bus_clk);
 	return 0;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Rajendra Nayak]

With platform support now in place to manage clocks from within runtime PM
callbacks, get rid of all clock handling from the driver and convert the
driver to use runtime PM apis.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Cc: <linux-mmc@vger.kernel.org>
---
 drivers/mmc/host/sdhci-msm.c | 46 +++++++++++---------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 4a09f76..3c62a77 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -19,6 +19,7 @@
 #include <linux/delay.h>
 #include <linux/mmc/mmc.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 
 #include "sdhci-pltfm.h"
 
@@ -56,8 +57,6 @@
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
-	struct clk *clk;	/* main SD/MMC bus clock */
-	struct clk *pclk;	/* SDHC peripheral bus clock */
 	struct clk *bus_clk;	/* SDHC bus voter clock */
 	struct mmc_host *mmc;
 	struct sdhci_pltfm_data sdhci_msm_pdata;
@@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 			goto pltfm_free;
 	}
 
-	/* Setup main peripheral bus clock */
-	msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
-	if (IS_ERR(msm_host->pclk)) {
-		ret = PTR_ERR(msm_host->pclk);
-		dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n", ret);
-		goto bus_clk_disable;
-	}
-
-	ret = clk_prepare_enable(msm_host->pclk);
-	if (ret)
-		goto bus_clk_disable;
-
-	/* Setup SDC MMC clock */
-	msm_host->clk = devm_clk_get(&pdev->dev, "core");
-	if (IS_ERR(msm_host->clk)) {
-		ret = PTR_ERR(msm_host->clk);
-		dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n", ret);
-		goto pclk_disable;
-	}
-
-	ret = clk_prepare_enable(msm_host->clk);
-	if (ret)
-		goto pclk_disable;
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
 
 	core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
@@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	if (IS_ERR(msm_host->core_mem)) {
 		dev_err(&pdev->dev, "Failed to remap registers\n");
 		ret = PTR_ERR(msm_host->core_mem);
-		goto clk_disable;
+		goto err;
 	}
 
 	/* Reset the core and Enable SDHC mode */
@@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
 		dev_err(&pdev->dev, "Stuck in reset\n");
 		ret = -ETIMEDOUT;
-		goto clk_disable;
+		goto err;
 	}
 
 	/* Set HC_MODE_EN bit in HC_MODE register */
@@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	ret = sdhci_add_host(host);
 	if (ret)
-		goto clk_disable;
+		goto err;
 
 	return 0;
 
-clk_disable:
-	clk_disable_unprepare(msm_host->clk);
-pclk_disable:
-	clk_disable_unprepare(msm_host->pclk);
-bus_clk_disable:
+err:
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	if (!IS_ERR(msm_host->bus_clk))
 		clk_disable_unprepare(msm_host->bus_clk);
 pltfm_free:
@@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device *pdev)
 
 	sdhci_remove_host(host, dead);
 	sdhci_pltfm_free(pdev);
-	clk_disable_unprepare(msm_host->clk);
-	clk_disable_unprepare(msm_host->pclk);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	if (!IS_ERR(msm_host->bus_clk))
 		clk_disable_unprepare(msm_host->bus_clk);
 	return 0;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Ulf Hansson]

On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> With platform support now in place to manage clocks from within runtime PM
> callbacks, get rid of all clock handling from the driver and convert the
> driver to use runtime PM apis.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Cc: <linux-mmc@vger.kernel.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 46 +++++++++++---------------------------------
>  1 file changed, 11 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 4a09f76..3c62a77 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -19,6 +19,7 @@
>  #include <linux/delay.h>
>  #include <linux/mmc/mmc.h>
>  #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
>
>  #include "sdhci-pltfm.h"
>
> @@ -56,8 +57,6 @@
>  struct sdhci_msm_host {
>         struct platform_device *pdev;
>         void __iomem *core_mem; /* MSM SDCC mapped address */
> -       struct clk *clk;        /* main SD/MMC bus clock */
> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>         struct clk *bus_clk;    /* SDHC bus voter clock */
>         struct mmc_host *mmc;
>         struct sdhci_pltfm_data sdhci_msm_pdata;
> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>                         goto pltfm_free;
>         }
>
> -       /* Setup main peripheral bus clock */
> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
> -       if (IS_ERR(msm_host->pclk)) {
> -               ret = PTR_ERR(msm_host->pclk);
> -               dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n", ret);
> -               goto bus_clk_disable;
> -       }
> -
> -       ret = clk_prepare_enable(msm_host->pclk);
> -       if (ret)
> -               goto bus_clk_disable;
> -
> -       /* Setup SDC MMC clock */
> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
> -       if (IS_ERR(msm_host->clk)) {
> -               ret = PTR_ERR(msm_host->clk);
> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n", ret);
> -               goto pclk_disable;
> -       }
> -
> -       ret = clk_prepare_enable(msm_host->clk);
> -       if (ret)
> -               goto pclk_disable;
> +       pm_runtime_enable(&pdev->dev);
> +       pm_runtime_get_sync(&pdev->dev);
>
>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         if (IS_ERR(msm_host->core_mem)) {
>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>                 ret = PTR_ERR(msm_host->core_mem);
> -               goto clk_disable;
> +               goto err;
>         }
>
>         /* Reset the core and Enable SDHC mode */
> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>                 dev_err(&pdev->dev, "Stuck in reset\n");
>                 ret = -ETIMEDOUT;
> -               goto clk_disable;
> +               goto err;
>         }
>
>         /* Set HC_MODE_EN bit in HC_MODE register */
> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>
>         ret = sdhci_add_host(host);
>         if (ret)
> -               goto clk_disable;
> +               goto err;
>
>         return 0;
>
> -clk_disable:
> -       clk_disable_unprepare(msm_host->clk);
> -pclk_disable:
> -       clk_disable_unprepare(msm_host->pclk);
> -bus_clk_disable:
> +err:
> +       pm_runtime_put_sync(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
>         if (!IS_ERR(msm_host->bus_clk))
>                 clk_disable_unprepare(msm_host->bus_clk);
>  pltfm_free:
> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device *pdev)
>
>         sdhci_remove_host(host, dead);
>         sdhci_pltfm_free(pdev);
> -       clk_disable_unprepare(msm_host->clk);
> -       clk_disable_unprepare(msm_host->pclk);
> +       pm_runtime_put_sync(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
>         if (!IS_ERR(msm_host->bus_clk))
>                 clk_disable_unprepare(msm_host->bus_clk);
>         return 0;
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

This all looks wrong. The driver will no longer work unless CONFIG_PM is set.

Kind regards
Uffe

[RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Ulf Hansson]

On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> With platform support now in place to manage clocks from within runtime PM
> callbacks, get rid of all clock handling from the driver and convert the
> driver to use runtime PM apis.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Cc: <linux-mmc@vger.kernel.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 46 +++++++++++---------------------------------
>  1 file changed, 11 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 4a09f76..3c62a77 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -19,6 +19,7 @@
>  #include <linux/delay.h>
>  #include <linux/mmc/mmc.h>
>  #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
>
>  #include "sdhci-pltfm.h"
>
> @@ -56,8 +57,6 @@
>  struct sdhci_msm_host {
>         struct platform_device *pdev;
>         void __iomem *core_mem; /* MSM SDCC mapped address */
> -       struct clk *clk;        /* main SD/MMC bus clock */
> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>         struct clk *bus_clk;    /* SDHC bus voter clock */
>         struct mmc_host *mmc;
>         struct sdhci_pltfm_data sdhci_msm_pdata;
> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>                         goto pltfm_free;
>         }
>
> -       /* Setup main peripheral bus clock */
> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
> -       if (IS_ERR(msm_host->pclk)) {
> -               ret = PTR_ERR(msm_host->pclk);
> -               dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n", ret);
> -               goto bus_clk_disable;
> -       }
> -
> -       ret = clk_prepare_enable(msm_host->pclk);
> -       if (ret)
> -               goto bus_clk_disable;
> -
> -       /* Setup SDC MMC clock */
> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
> -       if (IS_ERR(msm_host->clk)) {
> -               ret = PTR_ERR(msm_host->clk);
> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n", ret);
> -               goto pclk_disable;
> -       }
> -
> -       ret = clk_prepare_enable(msm_host->clk);
> -       if (ret)
> -               goto pclk_disable;
> +       pm_runtime_enable(&pdev->dev);
> +       pm_runtime_get_sync(&pdev->dev);
>
>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev, core_memres);
> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         if (IS_ERR(msm_host->core_mem)) {
>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>                 ret = PTR_ERR(msm_host->core_mem);
> -               goto clk_disable;
> +               goto err;
>         }
>
>         /* Reset the core and Enable SDHC mode */
> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>                 dev_err(&pdev->dev, "Stuck in reset\n");
>                 ret = -ETIMEDOUT;
> -               goto clk_disable;
> +               goto err;
>         }
>
>         /* Set HC_MODE_EN bit in HC_MODE register */
> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>
>         ret = sdhci_add_host(host);
>         if (ret)
> -               goto clk_disable;
> +               goto err;
>
>         return 0;
>
> -clk_disable:
> -       clk_disable_unprepare(msm_host->clk);
> -pclk_disable:
> -       clk_disable_unprepare(msm_host->pclk);
> -bus_clk_disable:
> +err:
> +       pm_runtime_put_sync(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
>         if (!IS_ERR(msm_host->bus_clk))
>                 clk_disable_unprepare(msm_host->bus_clk);
>  pltfm_free:
> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device *pdev)
>
>         sdhci_remove_host(host, dead);
>         sdhci_pltfm_free(pdev);
> -       clk_disable_unprepare(msm_host->clk);
> -       clk_disable_unprepare(msm_host->pclk);
> +       pm_runtime_put_sync(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
>         if (!IS_ERR(msm_host->bus_clk))
>                 clk_disable_unprepare(msm_host->bus_clk);
>         return 0;
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

This all looks wrong. The driver will no longer work unless CONFIG_PM is set.

Kind regards
Uffe

Re: [RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Rajendra Nayak]

> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> With platform support now in place to manage clocks from within runtime
>> PM
>> callbacks, get rid of all clock handling from the driver and convert the
>> driver to use runtime PM apis.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Cc: <linux-mmc@vger.kernel.org>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 46
>> +++++++++++---------------------------------
>>  1 file changed, 11 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4a09f76..3c62a77 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/mmc/mmc.h>
>>  #include <linux/slab.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #include "sdhci-pltfm.h"
>>
>> @@ -56,8 +57,6 @@
>>  struct sdhci_msm_host {
>>         struct platform_device *pdev;
>>         void __iomem *core_mem; /* MSM SDCC mapped address */
>> -       struct clk *clk;        /* main SD/MMC bus clock */
>> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>>         struct clk *bus_clk;    /* SDHC bus voter clock */
>>         struct mmc_host *mmc;
>>         struct sdhci_pltfm_data sdhci_msm_pdata;
>> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>                         goto pltfm_free;
>>         }
>>
>> -       /* Setup main peripheral bus clock */
>> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>> -       if (IS_ERR(msm_host->pclk)) {
>> -               ret = PTR_ERR(msm_host->pclk);
>> -               dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n",
>> ret);
>> -               goto bus_clk_disable;
>> -       }
>> -
>> -       ret = clk_prepare_enable(msm_host->pclk);
>> -       if (ret)
>> -               goto bus_clk_disable;
>> -
>> -       /* Setup SDC MMC clock */
>> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
>> -       if (IS_ERR(msm_host->clk)) {
>> -               ret = PTR_ERR(msm_host->clk);
>> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n",
>> ret);
>> -               goto pclk_disable;
>> -       }
>> -
>> -       ret = clk_prepare_enable(msm_host->clk);
>> -       if (ret)
>> -               goto pclk_disable;
>> +       pm_runtime_enable(&pdev->dev);
>> +       pm_runtime_get_sync(&pdev->dev);
>>
>>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>> core_memres);
>> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>         if (IS_ERR(msm_host->core_mem)) {
>>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>>                 ret = PTR_ERR(msm_host->core_mem);
>> -               goto clk_disable;
>> +               goto err;
>>         }
>>
>>         /* Reset the core and Enable SDHC mode */
>> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>>                 dev_err(&pdev->dev, "Stuck in reset\n");
>>                 ret = -ETIMEDOUT;
>> -               goto clk_disable;
>> +               goto err;
>>         }
>>
>>         /* Set HC_MODE_EN bit in HC_MODE register */
>> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>
>>         ret = sdhci_add_host(host);
>>         if (ret)
>> -               goto clk_disable;
>> +               goto err;
>>
>>         return 0;
>>
>> -clk_disable:
>> -       clk_disable_unprepare(msm_host->clk);
>> -pclk_disable:
>> -       clk_disable_unprepare(msm_host->pclk);
>> -bus_clk_disable:
>> +err:
>> +       pm_runtime_put_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>>         if (!IS_ERR(msm_host->bus_clk))
>>                 clk_disable_unprepare(msm_host->bus_clk);
>>  pltfm_free:
>> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device
>> *pdev)
>>
>>         sdhci_remove_host(host, dead);
>>         sdhci_pltfm_free(pdev);
>> -       clk_disable_unprepare(msm_host->clk);
>> -       clk_disable_unprepare(msm_host->pclk);
>> +       pm_runtime_put_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>>         if (!IS_ERR(msm_host->bus_clk))
>>                 clk_disable_unprepare(msm_host->bus_clk);
>>         return 0;
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> This all looks wrong. The driver will no longer work unless CONFIG_PM is
> set.

Right, I seem to have completely ignored the !CONFIG_PM case.
Will look at how to handle that.

>
> Kind regards
> Uffe
>


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

[RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Rajendra Nayak]

> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> With platform support now in place to manage clocks from within runtime
>> PM
>> callbacks, get rid of all clock handling from the driver and convert the
>> driver to use runtime PM apis.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Cc: <linux-mmc@vger.kernel.org>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 46
>> +++++++++++---------------------------------
>>  1 file changed, 11 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4a09f76..3c62a77 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/mmc/mmc.h>
>>  #include <linux/slab.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #include "sdhci-pltfm.h"
>>
>> @@ -56,8 +57,6 @@
>>  struct sdhci_msm_host {
>>         struct platform_device *pdev;
>>         void __iomem *core_mem; /* MSM SDCC mapped address */
>> -       struct clk *clk;        /* main SD/MMC bus clock */
>> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>>         struct clk *bus_clk;    /* SDHC bus voter clock */
>>         struct mmc_host *mmc;
>>         struct sdhci_pltfm_data sdhci_msm_pdata;
>> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>                         goto pltfm_free;
>>         }
>>
>> -       /* Setup main peripheral bus clock */
>> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>> -       if (IS_ERR(msm_host->pclk)) {
>> -               ret = PTR_ERR(msm_host->pclk);
>> -               dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n",
>> ret);
>> -               goto bus_clk_disable;
>> -       }
>> -
>> -       ret = clk_prepare_enable(msm_host->pclk);
>> -       if (ret)
>> -               goto bus_clk_disable;
>> -
>> -       /* Setup SDC MMC clock */
>> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
>> -       if (IS_ERR(msm_host->clk)) {
>> -               ret = PTR_ERR(msm_host->clk);
>> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n",
>> ret);
>> -               goto pclk_disable;
>> -       }
>> -
>> -       ret = clk_prepare_enable(msm_host->clk);
>> -       if (ret)
>> -               goto pclk_disable;
>> +       pm_runtime_enable(&pdev->dev);
>> +       pm_runtime_get_sync(&pdev->dev);
>>
>>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>> core_memres);
>> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>         if (IS_ERR(msm_host->core_mem)) {
>>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>>                 ret = PTR_ERR(msm_host->core_mem);
>> -               goto clk_disable;
>> +               goto err;
>>         }
>>
>>         /* Reset the core and Enable SDHC mode */
>> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>>                 dev_err(&pdev->dev, "Stuck in reset\n");
>>                 ret = -ETIMEDOUT;
>> -               goto clk_disable;
>> +               goto err;
>>         }
>>
>>         /* Set HC_MODE_EN bit in HC_MODE register */
>> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>
>>         ret = sdhci_add_host(host);
>>         if (ret)
>> -               goto clk_disable;
>> +               goto err;
>>
>>         return 0;
>>
>> -clk_disable:
>> -       clk_disable_unprepare(msm_host->clk);
>> -pclk_disable:
>> -       clk_disable_unprepare(msm_host->pclk);
>> -bus_clk_disable:
>> +err:
>> +       pm_runtime_put_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>>         if (!IS_ERR(msm_host->bus_clk))
>>                 clk_disable_unprepare(msm_host->bus_clk);
>>  pltfm_free:
>> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device
>> *pdev)
>>
>>         sdhci_remove_host(host, dead);
>>         sdhci_pltfm_free(pdev);
>> -       clk_disable_unprepare(msm_host->clk);
>> -       clk_disable_unprepare(msm_host->pclk);
>> +       pm_runtime_put_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>>         if (!IS_ERR(msm_host->bus_clk))
>>                 clk_disable_unprepare(msm_host->bus_clk);
>>         return 0;
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> This all looks wrong. The driver will no longer work unless CONFIG_PM is
> set.

Right, I seem to have completely ignored the !CONFIG_PM case.
Will look at how to handle that.

>
> Kind regards
> Uffe
>


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

Re: [RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Kevin Hilman]

"Rajendra Nayak" <rnayak@codeaurora.org> writes:

>> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>> With platform support now in place to manage clocks from within runtime
>>> PM
>>> callbacks, get rid of all clock handling from the driver and convert the
>>> driver to use runtime PM apis.
>>>
>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>> Cc: <linux-mmc@vger.kernel.org>
>>> ---
>>>  drivers/mmc/host/sdhci-msm.c | 46
>>> +++++++++++---------------------------------
>>>  1 file changed, 11 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>> index 4a09f76..3c62a77 100644
>>> --- a/drivers/mmc/host/sdhci-msm.c
>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>> @@ -19,6 +19,7 @@
>>>  #include <linux/delay.h>
>>>  #include <linux/mmc/mmc.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/pm_runtime.h>
>>>
>>>  #include "sdhci-pltfm.h"
>>>
>>> @@ -56,8 +57,6 @@
>>>  struct sdhci_msm_host {
>>>         struct platform_device *pdev;
>>>         void __iomem *core_mem; /* MSM SDCC mapped address */
>>> -       struct clk *clk;        /* main SD/MMC bus clock */
>>> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>>>         struct clk *bus_clk;    /* SDHC bus voter clock */
>>>         struct mmc_host *mmc;
>>>         struct sdhci_pltfm_data sdhci_msm_pdata;
>>> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device
>>> *pdev)
>>>                         goto pltfm_free;
>>>         }
>>>
>>> -       /* Setup main peripheral bus clock */
>>> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>>> -       if (IS_ERR(msm_host->pclk)) {
>>> -               ret = PTR_ERR(msm_host->pclk);
>>> -               dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n",
>>> ret);
>>> -               goto bus_clk_disable;
>>> -       }
>>> -
>>> -       ret = clk_prepare_enable(msm_host->pclk);
>>> -       if (ret)
>>> -               goto bus_clk_disable;
>>> -
>>> -       /* Setup SDC MMC clock */
>>> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
>>> -       if (IS_ERR(msm_host->clk)) {
>>> -               ret = PTR_ERR(msm_host->clk);
>>> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n",
>>> ret);
>>> -               goto pclk_disable;
>>> -       }
>>> -
>>> -       ret = clk_prepare_enable(msm_host->clk);
>>> -       if (ret)
>>> -               goto pclk_disable;
>>> +       pm_runtime_enable(&pdev->dev);
>>> +       pm_runtime_get_sync(&pdev->dev);
>>>
>>>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>>> core_memres);
>>> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device
>>> *pdev)
>>>         if (IS_ERR(msm_host->core_mem)) {
>>>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>>>                 ret = PTR_ERR(msm_host->core_mem);
>>> -               goto clk_disable;
>>> +               goto err;
>>>         }
>>>
>>>         /* Reset the core and Enable SDHC mode */
>>> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device
>>> *pdev)
>>>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>>>                 dev_err(&pdev->dev, "Stuck in reset\n");
>>>                 ret = -ETIMEDOUT;
>>> -               goto clk_disable;
>>> +               goto err;
>>>         }
>>>
>>>         /* Set HC_MODE_EN bit in HC_MODE register */
>>> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device
>>> *pdev)
>>>
>>>         ret = sdhci_add_host(host);
>>>         if (ret)
>>> -               goto clk_disable;
>>> +               goto err;
>>>
>>>         return 0;
>>>
>>> -clk_disable:
>>> -       clk_disable_unprepare(msm_host->clk);
>>> -pclk_disable:
>>> -       clk_disable_unprepare(msm_host->pclk);
>>> -bus_clk_disable:
>>> +err:
>>> +       pm_runtime_put_sync(&pdev->dev);
>>> +       pm_runtime_disable(&pdev->dev);
>>>         if (!IS_ERR(msm_host->bus_clk))
>>>                 clk_disable_unprepare(msm_host->bus_clk);
>>>  pltfm_free:
>>> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device
>>> *pdev)
>>>
>>>         sdhci_remove_host(host, dead);
>>>         sdhci_pltfm_free(pdev);
>>> -       clk_disable_unprepare(msm_host->clk);
>>> -       clk_disable_unprepare(msm_host->pclk);
>>> +       pm_runtime_put_sync(&pdev->dev);
>>> +       pm_runtime_disable(&pdev->dev);
>>>         if (!IS_ERR(msm_host->bus_clk))
>>>                 clk_disable_unprepare(msm_host->bus_clk);
>>>         return 0;
>>> --
>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>>> member
>>> of Code Aurora Forum, hosted by The Linux Foundation
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> This all looks wrong. The driver will no longer work unless CONFIG_PM is
>> set.
>
> Right, I seem to have completely ignored the !CONFIG_PM case.
> Will look at how to handle that.

IMO, don't complicate the driver with that.  Just enable (or leave
enabled) all the clocks in the clock driver in the !CONFIG_PM case.

Kevin

[RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Kevin Hilman]

"Rajendra Nayak" <rnayak@codeaurora.org> writes:

>> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>> With platform support now in place to manage clocks from within runtime
>>> PM
>>> callbacks, get rid of all clock handling from the driver and convert the
>>> driver to use runtime PM apis.
>>>
>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>> Cc: <linux-mmc@vger.kernel.org>
>>> ---
>>>  drivers/mmc/host/sdhci-msm.c | 46
>>> +++++++++++---------------------------------
>>>  1 file changed, 11 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>>> index 4a09f76..3c62a77 100644
>>> --- a/drivers/mmc/host/sdhci-msm.c
>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>> @@ -19,6 +19,7 @@
>>>  #include <linux/delay.h>
>>>  #include <linux/mmc/mmc.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/pm_runtime.h>
>>>
>>>  #include "sdhci-pltfm.h"
>>>
>>> @@ -56,8 +57,6 @@
>>>  struct sdhci_msm_host {
>>>         struct platform_device *pdev;
>>>         void __iomem *core_mem; /* MSM SDCC mapped address */
>>> -       struct clk *clk;        /* main SD/MMC bus clock */
>>> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>>>         struct clk *bus_clk;    /* SDHC bus voter clock */
>>>         struct mmc_host *mmc;
>>>         struct sdhci_pltfm_data sdhci_msm_pdata;
>>> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device
>>> *pdev)
>>>                         goto pltfm_free;
>>>         }
>>>
>>> -       /* Setup main peripheral bus clock */
>>> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>>> -       if (IS_ERR(msm_host->pclk)) {
>>> -               ret = PTR_ERR(msm_host->pclk);
>>> -               dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n",
>>> ret);
>>> -               goto bus_clk_disable;
>>> -       }
>>> -
>>> -       ret = clk_prepare_enable(msm_host->pclk);
>>> -       if (ret)
>>> -               goto bus_clk_disable;
>>> -
>>> -       /* Setup SDC MMC clock */
>>> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
>>> -       if (IS_ERR(msm_host->clk)) {
>>> -               ret = PTR_ERR(msm_host->clk);
>>> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n",
>>> ret);
>>> -               goto pclk_disable;
>>> -       }
>>> -
>>> -       ret = clk_prepare_enable(msm_host->clk);
>>> -       if (ret)
>>> -               goto pclk_disable;
>>> +       pm_runtime_enable(&pdev->dev);
>>> +       pm_runtime_get_sync(&pdev->dev);
>>>
>>>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>>> core_memres);
>>> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device
>>> *pdev)
>>>         if (IS_ERR(msm_host->core_mem)) {
>>>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>>>                 ret = PTR_ERR(msm_host->core_mem);
>>> -               goto clk_disable;
>>> +               goto err;
>>>         }
>>>
>>>         /* Reset the core and Enable SDHC mode */
>>> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device
>>> *pdev)
>>>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>>>                 dev_err(&pdev->dev, "Stuck in reset\n");
>>>                 ret = -ETIMEDOUT;
>>> -               goto clk_disable;
>>> +               goto err;
>>>         }
>>>
>>>         /* Set HC_MODE_EN bit in HC_MODE register */
>>> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device
>>> *pdev)
>>>
>>>         ret = sdhci_add_host(host);
>>>         if (ret)
>>> -               goto clk_disable;
>>> +               goto err;
>>>
>>>         return 0;
>>>
>>> -clk_disable:
>>> -       clk_disable_unprepare(msm_host->clk);
>>> -pclk_disable:
>>> -       clk_disable_unprepare(msm_host->pclk);
>>> -bus_clk_disable:
>>> +err:
>>> +       pm_runtime_put_sync(&pdev->dev);
>>> +       pm_runtime_disable(&pdev->dev);
>>>         if (!IS_ERR(msm_host->bus_clk))
>>>                 clk_disable_unprepare(msm_host->bus_clk);
>>>  pltfm_free:
>>> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device
>>> *pdev)
>>>
>>>         sdhci_remove_host(host, dead);
>>>         sdhci_pltfm_free(pdev);
>>> -       clk_disable_unprepare(msm_host->clk);
>>> -       clk_disable_unprepare(msm_host->pclk);
>>> +       pm_runtime_put_sync(&pdev->dev);
>>> +       pm_runtime_disable(&pdev->dev);
>>>         if (!IS_ERR(msm_host->bus_clk))
>>>                 clk_disable_unprepare(msm_host->bus_clk);
>>>         return 0;
>>> --
>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>>> member
>>> of Code Aurora Forum, hosted by The Linux Foundation
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>>> the body of a message to majordomo at vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> This all looks wrong. The driver will no longer work unless CONFIG_PM is
>> set.
>
> Right, I seem to have completely ignored the !CONFIG_PM case.
> Will look at how to handle that.

IMO, don't complicate the driver with that.  Just enable (or leave
enabled) all the clocks in the clock driver in the !CONFIG_PM case.

Kevin

Re: [RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Rajendra Nayak]

> "Rajendra Nayak" <rnayak@codeaurora.org> writes:
>
>>> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org>
>>> wrote:
>>>> With platform support now in place to manage clocks from within
>>>> runtime
>>>> PM
>>>> callbacks, get rid of all clock handling from the driver and convert
>>>> the
>>>> driver to use runtime PM apis.
>>>>
>>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>>> Cc: <linux-mmc@vger.kernel.org>
>>>> ---
>>>>  drivers/mmc/host/sdhci-msm.c | 46
>>>> +++++++++++---------------------------------
>>>>  1 file changed, 11 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c
>>>> b/drivers/mmc/host/sdhci-msm.c
>>>> index 4a09f76..3c62a77 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -19,6 +19,7 @@
>>>>  #include <linux/delay.h>
>>>>  #include <linux/mmc/mmc.h>
>>>>  #include <linux/slab.h>
>>>> +#include <linux/pm_runtime.h>
>>>>
>>>>  #include "sdhci-pltfm.h"
>>>>
>>>> @@ -56,8 +57,6 @@
>>>>  struct sdhci_msm_host {
>>>>         struct platform_device *pdev;
>>>>         void __iomem *core_mem; /* MSM SDCC mapped address */
>>>> -       struct clk *clk;        /* main SD/MMC bus clock */
>>>> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>>>>         struct clk *bus_clk;    /* SDHC bus voter clock */
>>>>         struct mmc_host *mmc;
>>>>         struct sdhci_pltfm_data sdhci_msm_pdata;
>>>> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device
>>>> *pdev)
>>>>                         goto pltfm_free;
>>>>         }
>>>>
>>>> -       /* Setup main peripheral bus clock */
>>>> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>>>> -       if (IS_ERR(msm_host->pclk)) {
>>>> -               ret = PTR_ERR(msm_host->pclk);
>>>> -               dev_err(&pdev->dev, "Perpheral clk setup failed
>>>> (%d)\n",
>>>> ret);
>>>> -               goto bus_clk_disable;
>>>> -       }
>>>> -
>>>> -       ret = clk_prepare_enable(msm_host->pclk);
>>>> -       if (ret)
>>>> -               goto bus_clk_disable;
>>>> -
>>>> -       /* Setup SDC MMC clock */
>>>> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
>>>> -       if (IS_ERR(msm_host->clk)) {
>>>> -               ret = PTR_ERR(msm_host->clk);
>>>> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n",
>>>> ret);
>>>> -               goto pclk_disable;
>>>> -       }
>>>> -
>>>> -       ret = clk_prepare_enable(msm_host->clk);
>>>> -       if (ret)
>>>> -               goto pclk_disable;
>>>> +       pm_runtime_enable(&pdev->dev);
>>>> +       pm_runtime_get_sync(&pdev->dev);
>>>>
>>>>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>>>> core_memres);
>>>> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device
>>>> *pdev)
>>>>         if (IS_ERR(msm_host->core_mem)) {
>>>>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>>>>                 ret = PTR_ERR(msm_host->core_mem);
>>>> -               goto clk_disable;
>>>> +               goto err;
>>>>         }
>>>>
>>>>         /* Reset the core and Enable SDHC mode */
>>>> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device
>>>> *pdev)
>>>>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>>>>                 dev_err(&pdev->dev, "Stuck in reset\n");
>>>>                 ret = -ETIMEDOUT;
>>>> -               goto clk_disable;
>>>> +               goto err;
>>>>         }
>>>>
>>>>         /* Set HC_MODE_EN bit in HC_MODE register */
>>>> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct
>>>> platform_device
>>>> *pdev)
>>>>
>>>>         ret = sdhci_add_host(host);
>>>>         if (ret)
>>>> -               goto clk_disable;
>>>> +               goto err;
>>>>
>>>>         return 0;
>>>>
>>>> -clk_disable:
>>>> -       clk_disable_unprepare(msm_host->clk);
>>>> -pclk_disable:
>>>> -       clk_disable_unprepare(msm_host->pclk);
>>>> -bus_clk_disable:
>>>> +err:
>>>> +       pm_runtime_put_sync(&pdev->dev);
>>>> +       pm_runtime_disable(&pdev->dev);
>>>>         if (!IS_ERR(msm_host->bus_clk))
>>>>                 clk_disable_unprepare(msm_host->bus_clk);
>>>>  pltfm_free:
>>>> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device
>>>> *pdev)
>>>>
>>>>         sdhci_remove_host(host, dead);
>>>>         sdhci_pltfm_free(pdev);
>>>> -       clk_disable_unprepare(msm_host->clk);
>>>> -       clk_disable_unprepare(msm_host->pclk);
>>>> +       pm_runtime_put_sync(&pdev->dev);
>>>> +       pm_runtime_disable(&pdev->dev);
>>>>         if (!IS_ERR(msm_host->bus_clk))
>>>>                 clk_disable_unprepare(msm_host->bus_clk);
>>>>         return 0;
[]..
>>>
>>> This all looks wrong. The driver will no longer work unless CONFIG_PM
>>> is
>>> set.
>>
>> Right, I seem to have completely ignored the !CONFIG_PM case.
>> Will look at how to handle that.
>
> IMO, don't complicate the driver with that.  Just enable (or leave
> enabled) all the clocks in the clock driver in the !CONFIG_PM case.

so I just looked at what pm_clk_notify() does in !CONFIG_PM case and it
seems to do just that, leaves all the clocks enabled on
BUS_NOTIFY_BIND_DRIVER case and disables them for BUS_NOTIFY_UNBOUND_DRIVER
case. So looks like things are already in place to handle this :)

Ulf, does that address your concern or did I miss anything?

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

[RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Rajendra Nayak]

> "Rajendra Nayak" <rnayak@codeaurora.org> writes:
>
>>> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org>
>>> wrote:
>>>> With platform support now in place to manage clocks from within
>>>> runtime
>>>> PM
>>>> callbacks, get rid of all clock handling from the driver and convert
>>>> the
>>>> driver to use runtime PM apis.
>>>>
>>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>>> Cc: <linux-mmc@vger.kernel.org>
>>>> ---
>>>>  drivers/mmc/host/sdhci-msm.c | 46
>>>> +++++++++++---------------------------------
>>>>  1 file changed, 11 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-msm.c
>>>> b/drivers/mmc/host/sdhci-msm.c
>>>> index 4a09f76..3c62a77 100644
>>>> --- a/drivers/mmc/host/sdhci-msm.c
>>>> +++ b/drivers/mmc/host/sdhci-msm.c
>>>> @@ -19,6 +19,7 @@
>>>>  #include <linux/delay.h>
>>>>  #include <linux/mmc/mmc.h>
>>>>  #include <linux/slab.h>
>>>> +#include <linux/pm_runtime.h>
>>>>
>>>>  #include "sdhci-pltfm.h"
>>>>
>>>> @@ -56,8 +57,6 @@
>>>>  struct sdhci_msm_host {
>>>>         struct platform_device *pdev;
>>>>         void __iomem *core_mem; /* MSM SDCC mapped address */
>>>> -       struct clk *clk;        /* main SD/MMC bus clock */
>>>> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>>>>         struct clk *bus_clk;    /* SDHC bus voter clock */
>>>>         struct mmc_host *mmc;
>>>>         struct sdhci_pltfm_data sdhci_msm_pdata;
>>>> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device
>>>> *pdev)
>>>>                         goto pltfm_free;
>>>>         }
>>>>
>>>> -       /* Setup main peripheral bus clock */
>>>> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>>>> -       if (IS_ERR(msm_host->pclk)) {
>>>> -               ret = PTR_ERR(msm_host->pclk);
>>>> -               dev_err(&pdev->dev, "Perpheral clk setup failed
>>>> (%d)\n",
>>>> ret);
>>>> -               goto bus_clk_disable;
>>>> -       }
>>>> -
>>>> -       ret = clk_prepare_enable(msm_host->pclk);
>>>> -       if (ret)
>>>> -               goto bus_clk_disable;
>>>> -
>>>> -       /* Setup SDC MMC clock */
>>>> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
>>>> -       if (IS_ERR(msm_host->clk)) {
>>>> -               ret = PTR_ERR(msm_host->clk);
>>>> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n",
>>>> ret);
>>>> -               goto pclk_disable;
>>>> -       }
>>>> -
>>>> -       ret = clk_prepare_enable(msm_host->clk);
>>>> -       if (ret)
>>>> -               goto pclk_disable;
>>>> +       pm_runtime_enable(&pdev->dev);
>>>> +       pm_runtime_get_sync(&pdev->dev);
>>>>
>>>>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>>>> core_memres);
>>>> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device
>>>> *pdev)
>>>>         if (IS_ERR(msm_host->core_mem)) {
>>>>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>>>>                 ret = PTR_ERR(msm_host->core_mem);
>>>> -               goto clk_disable;
>>>> +               goto err;
>>>>         }
>>>>
>>>>         /* Reset the core and Enable SDHC mode */
>>>> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device
>>>> *pdev)
>>>>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>>>>                 dev_err(&pdev->dev, "Stuck in reset\n");
>>>>                 ret = -ETIMEDOUT;
>>>> -               goto clk_disable;
>>>> +               goto err;
>>>>         }
>>>>
>>>>         /* Set HC_MODE_EN bit in HC_MODE register */
>>>> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct
>>>> platform_device
>>>> *pdev)
>>>>
>>>>         ret = sdhci_add_host(host);
>>>>         if (ret)
>>>> -               goto clk_disable;
>>>> +               goto err;
>>>>
>>>>         return 0;
>>>>
>>>> -clk_disable:
>>>> -       clk_disable_unprepare(msm_host->clk);
>>>> -pclk_disable:
>>>> -       clk_disable_unprepare(msm_host->pclk);
>>>> -bus_clk_disable:
>>>> +err:
>>>> +       pm_runtime_put_sync(&pdev->dev);
>>>> +       pm_runtime_disable(&pdev->dev);
>>>>         if (!IS_ERR(msm_host->bus_clk))
>>>>                 clk_disable_unprepare(msm_host->bus_clk);
>>>>  pltfm_free:
>>>> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device
>>>> *pdev)
>>>>
>>>>         sdhci_remove_host(host, dead);
>>>>         sdhci_pltfm_free(pdev);
>>>> -       clk_disable_unprepare(msm_host->clk);
>>>> -       clk_disable_unprepare(msm_host->pclk);
>>>> +       pm_runtime_put_sync(&pdev->dev);
>>>> +       pm_runtime_disable(&pdev->dev);
>>>>         if (!IS_ERR(msm_host->bus_clk))
>>>>                 clk_disable_unprepare(msm_host->bus_clk);
>>>>         return 0;
[]..
>>>
>>> This all looks wrong. The driver will no longer work unless CONFIG_PM
>>> is
>>> set.
>>
>> Right, I seem to have completely ignored the !CONFIG_PM case.
>> Will look at how to handle that.
>
> IMO, don't complicate the driver with that.  Just enable (or leave
> enabled) all the clocks in the clock driver in the !CONFIG_PM case.

so I just looked at what pm_clk_notify() does in !CONFIG_PM case and it
seems to do just that, leaves all the clocks enabled on
BUS_NOTIFY_BIND_DRIVER case and disables them for BUS_NOTIFY_UNBOUND_DRIVER
case. So looks like things are already in place to handle this :)

Ulf, does that address your concern or did I miss anything?

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

Re: [RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Rajendra Nayak]

> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> With platform support now in place to manage clocks from within runtime
>> PM
>> callbacks, get rid of all clock handling from the driver and convert the
>> driver to use runtime PM apis.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Cc: <linux-mmc@vger.kernel.org>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 46
>> +++++++++++---------------------------------
>>  1 file changed, 11 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4a09f76..3c62a77 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/mmc/mmc.h>
>>  #include <linux/slab.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #include "sdhci-pltfm.h"
>>
>> @@ -56,8 +57,6 @@
>>  struct sdhci_msm_host {
>>         struct platform_device *pdev;
>>         void __iomem *core_mem; /* MSM SDCC mapped address */
>> -       struct clk *clk;        /* main SD/MMC bus clock */
>> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>>         struct clk *bus_clk;    /* SDHC bus voter clock */
>>         struct mmc_host *mmc;
>>         struct sdhci_pltfm_data sdhci_msm_pdata;
>> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>                         goto pltfm_free;
>>         }
>>
>> -       /* Setup main peripheral bus clock */
>> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>> -       if (IS_ERR(msm_host->pclk)) {
>> -               ret = PTR_ERR(msm_host->pclk);
>> -               dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n",
>> ret);
>> -               goto bus_clk_disable;
>> -       }
>> -
>> -       ret = clk_prepare_enable(msm_host->pclk);
>> -       if (ret)
>> -               goto bus_clk_disable;
>> -
>> -       /* Setup SDC MMC clock */
>> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
>> -       if (IS_ERR(msm_host->clk)) {
>> -               ret = PTR_ERR(msm_host->clk);
>> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n",
>> ret);
>> -               goto pclk_disable;
>> -       }
>> -
>> -       ret = clk_prepare_enable(msm_host->clk);
>> -       if (ret)
>> -               goto pclk_disable;
>> +       pm_runtime_enable(&pdev->dev);
>> +       pm_runtime_get_sync(&pdev->dev);
>>
>>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>> core_memres);
>> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>         if (IS_ERR(msm_host->core_mem)) {
>>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>>                 ret = PTR_ERR(msm_host->core_mem);
>> -               goto clk_disable;
>> +               goto err;
>>         }
>>
>>         /* Reset the core and Enable SDHC mode */
>> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>>                 dev_err(&pdev->dev, "Stuck in reset\n");
>>                 ret = -ETIMEDOUT;
>> -               goto clk_disable;
>> +               goto err;
>>         }
>>
>>         /* Set HC_MODE_EN bit in HC_MODE register */
>> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>
>>         ret = sdhci_add_host(host);
>>         if (ret)
>> -               goto clk_disable;
>> +               goto err;
>>
>>         return 0;
>>
>> -clk_disable:
>> -       clk_disable_unprepare(msm_host->clk);
>> -pclk_disable:
>> -       clk_disable_unprepare(msm_host->pclk);
>> -bus_clk_disable:
>> +err:
>> +       pm_runtime_put_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>>         if (!IS_ERR(msm_host->bus_clk))
>>                 clk_disable_unprepare(msm_host->bus_clk);
>>  pltfm_free:
>> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device
>> *pdev)
>>
>>         sdhci_remove_host(host, dead);
>>         sdhci_pltfm_free(pdev);
>> -       clk_disable_unprepare(msm_host->clk);
>> -       clk_disable_unprepare(msm_host->pclk);
>> +       pm_runtime_put_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>>         if (!IS_ERR(msm_host->bus_clk))
>>                 clk_disable_unprepare(msm_host->bus_clk);
>>         return 0;
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> This all looks wrong. The driver will no longer work unless CONFIG_PM is
> set.

Right, I seem to have completely ignored the !CONFIG_PM case.
Will look at how to handle that.

>
> Kind regards
> Uffe
>


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

[RFC/RFT 4/6] mmc: sdhci-msm: convert driver to use runtime PM apis

[Rajendra Nayak]

> On 23 April 2015 at 10:45, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>> With platform support now in place to manage clocks from within runtime
>> PM
>> callbacks, get rid of all clock handling from the driver and convert the
>> driver to use runtime PM apis.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Cc: <linux-mmc@vger.kernel.org>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 46
>> +++++++++++---------------------------------
>>  1 file changed, 11 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 4a09f76..3c62a77 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/mmc/mmc.h>
>>  #include <linux/slab.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #include "sdhci-pltfm.h"
>>
>> @@ -56,8 +57,6 @@
>>  struct sdhci_msm_host {
>>         struct platform_device *pdev;
>>         void __iomem *core_mem; /* MSM SDCC mapped address */
>> -       struct clk *clk;        /* main SD/MMC bus clock */
>> -       struct clk *pclk;       /* SDHC peripheral bus clock */
>>         struct clk *bus_clk;    /* SDHC bus voter clock */
>>         struct mmc_host *mmc;
>>         struct sdhci_pltfm_data sdhci_msm_pdata;
>> @@ -469,29 +468,8 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>                         goto pltfm_free;
>>         }
>>
>> -       /* Setup main peripheral bus clock */
>> -       msm_host->pclk = devm_clk_get(&pdev->dev, "iface");
>> -       if (IS_ERR(msm_host->pclk)) {
>> -               ret = PTR_ERR(msm_host->pclk);
>> -               dev_err(&pdev->dev, "Perpheral clk setup failed (%d)\n",
>> ret);
>> -               goto bus_clk_disable;
>> -       }
>> -
>> -       ret = clk_prepare_enable(msm_host->pclk);
>> -       if (ret)
>> -               goto bus_clk_disable;
>> -
>> -       /* Setup SDC MMC clock */
>> -       msm_host->clk = devm_clk_get(&pdev->dev, "core");
>> -       if (IS_ERR(msm_host->clk)) {
>> -               ret = PTR_ERR(msm_host->clk);
>> -               dev_err(&pdev->dev, "SDC MMC clk setup failed (%d)\n",
>> ret);
>> -               goto pclk_disable;
>> -       }
>> -
>> -       ret = clk_prepare_enable(msm_host->clk);
>> -       if (ret)
>> -               goto pclk_disable;
>> +       pm_runtime_enable(&pdev->dev);
>> +       pm_runtime_get_sync(&pdev->dev);
>>
>>         core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>         msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>> core_memres);
>> @@ -499,7 +477,7 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>         if (IS_ERR(msm_host->core_mem)) {
>>                 dev_err(&pdev->dev, "Failed to remap registers\n");
>>                 ret = PTR_ERR(msm_host->core_mem);
>> -               goto clk_disable;
>> +               goto err;
>>         }
>>
>>         /* Reset the core and Enable SDHC mode */
>> @@ -511,7 +489,7 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>         if (readl(msm_host->core_mem + CORE_POWER) & CORE_SW_RST) {
>>                 dev_err(&pdev->dev, "Stuck in reset\n");
>>                 ret = -ETIMEDOUT;
>> -               goto clk_disable;
>> +               goto err;
>>         }
>>
>>         /* Set HC_MODE_EN bit in HC_MODE register */
>> @@ -545,15 +523,13 @@ static int sdhci_msm_probe(struct platform_device
>> *pdev)
>>
>>         ret = sdhci_add_host(host);
>>         if (ret)
>> -               goto clk_disable;
>> +               goto err;
>>
>>         return 0;
>>
>> -clk_disable:
>> -       clk_disable_unprepare(msm_host->clk);
>> -pclk_disable:
>> -       clk_disable_unprepare(msm_host->pclk);
>> -bus_clk_disable:
>> +err:
>> +       pm_runtime_put_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>>         if (!IS_ERR(msm_host->bus_clk))
>>                 clk_disable_unprepare(msm_host->bus_clk);
>>  pltfm_free:
>> @@ -571,8 +547,8 @@ static int sdhci_msm_remove(struct platform_device
>> *pdev)
>>
>>         sdhci_remove_host(host, dead);
>>         sdhci_pltfm_free(pdev);
>> -       clk_disable_unprepare(msm_host->clk);
>> -       clk_disable_unprepare(msm_host->pclk);
>> +       pm_runtime_put_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>>         if (!IS_ERR(msm_host->bus_clk))
>>                 clk_disable_unprepare(msm_host->bus_clk);
>>         return 0;
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> This all looks wrong. The driver will no longer work unless CONFIG_PM is
> set.

Right, I seem to have completely ignored the !CONFIG_PM case.
Will look at how to handle that.

>
> Kind regards
> Uffe
>


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

[RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Rajendra Nayak]

Remove all clock handling from the driver as this is not handled from
within platform runtime callbacks.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/i2c/busses/i2c-qup.c | 74 ++++++++++----------------------------------
 1 file changed, 16 insertions(+), 58 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index fdcbdab..465a2b2 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -104,7 +104,6 @@ struct qup_i2c_dev {
 	void __iomem		*base;
 	int			irq;
 	struct clk		*clk;
-	struct clk		*pclk;
 	struct i2c_adapter	adap;
 
 	int			clk_ctl;
@@ -532,24 +531,6 @@ static struct i2c_adapter_quirks qup_i2c_quirks = {
 	.max_read_len = QUP_READ_LIMIT,
 };
 
-static void qup_i2c_enable_clocks(struct qup_i2c_dev *qup)
-{
-	clk_prepare_enable(qup->clk);
-	clk_prepare_enable(qup->pclk);
-}
-
-static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
-{
-	u32 config;
-
-	qup_i2c_change_state(qup, QUP_RESET_STATE);
-	clk_disable_unprepare(qup->clk);
-	config = readl(qup->base + QUP_CONFIG);
-	config |= QUP_CLOCK_AUTO_GATE;
-	writel(config, qup->base + QUP_CONFIG);
-	clk_disable_unprepare(qup->pclk);
-}
-
 static int qup_i2c_probe(struct platform_device *pdev)
 {
 	static const int blk_sizes[] = {4, 16, 32};
@@ -596,13 +577,10 @@ static int qup_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(qup->clk);
 	}
 
-	qup->pclk = devm_clk_get(qup->dev, "iface");
-	if (IS_ERR(qup->pclk)) {
-		dev_err(qup->dev, "Could not get iface clock\n");
-		return PTR_ERR(qup->pclk);
-	}
-
-	qup_i2c_enable_clocks(qup);
+	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
+	pm_runtime_use_autosuspend(qup->dev);
+	pm_runtime_enable(qup->dev);
+	pm_runtime_get_sync(qup->dev);
 
 	/*
 	 * Bootloaders might leave a pending interrupt on certain QUP's,
@@ -673,22 +651,15 @@ static int qup_i2c_probe(struct platform_device *pdev)
 	qup->adap.dev.of_node = pdev->dev.of_node;
 	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
 
-	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
-	pm_runtime_use_autosuspend(qup->dev);
-	pm_runtime_set_active(qup->dev);
-	pm_runtime_enable(qup->dev);
-
 	ret = i2c_add_adapter(&qup->adap);
 	if (ret)
-		goto fail_runtime;
+		goto fail;
 
 	return 0;
 
-fail_runtime:
+fail:
 	pm_runtime_disable(qup->dev);
 	pm_runtime_set_suspended(qup->dev);
-fail:
-	qup_i2c_disable_clocks(qup);
 	return ret;
 }
 
@@ -697,7 +668,6 @@ static int qup_i2c_remove(struct platform_device *pdev)
 	struct qup_i2c_dev *qup = platform_get_drvdata(pdev);
 
 	disable_irq(qup->irq);
-	qup_i2c_disable_clocks(qup);
 	i2c_del_adapter(&qup->adap);
 	pm_runtime_disable(qup->dev);
 	pm_runtime_set_suspended(qup->dev);
@@ -707,43 +677,31 @@ static int qup_i2c_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM
 static int qup_i2c_pm_suspend_runtime(struct device *device)
 {
+	u32 config;
 	struct qup_i2c_dev *qup = dev_get_drvdata(device);
 
 	dev_dbg(device, "pm_runtime: suspending...\n");
-	qup_i2c_disable_clocks(qup);
+	qup_i2c_change_state(qup, QUP_RESET_STATE);
+	config = readl(qup->base + QUP_CONFIG);
+	config |= QUP_CLOCK_AUTO_GATE;
+	writel(config, qup->base + QUP_CONFIG);
 	return 0;
 }
 
 static int qup_i2c_pm_resume_runtime(struct device *device)
 {
+	u32 config;
 	struct qup_i2c_dev *qup = dev_get_drvdata(device);
 
-	dev_dbg(device, "pm_runtime: resuming...\n");
-	qup_i2c_enable_clocks(qup);
-	return 0;
-}
-#endif
-
-#ifdef CONFIG_PM_SLEEP
-static int qup_i2c_suspend(struct device *device)
-{
-	qup_i2c_pm_suspend_runtime(device);
-	return 0;
-}
-
-static int qup_i2c_resume(struct device *device)
-{
-	qup_i2c_pm_resume_runtime(device);
-	pm_runtime_mark_last_busy(device);
-	pm_request_autosuspend(device);
+	dev_dbg(device, "pm_runtime: suspending...\n");
+	config = readl(qup->base + QUP_CONFIG);
+	config &= ~QUP_CLOCK_AUTO_GATE;
+	writel(config, qup->base + QUP_CONFIG);
 	return 0;
 }
 #endif
 
 static const struct dev_pm_ops qup_i2c_qup_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(
-		qup_i2c_suspend,
-		qup_i2c_resume)
 	SET_RUNTIME_PM_OPS(
 		qup_i2c_pm_suspend_runtime,
 		qup_i2c_pm_resume_runtime,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Rajendra Nayak]

Remove all clock handling from the driver as this is not handled from
within platform runtime callbacks.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/i2c/busses/i2c-qup.c | 74 ++++++++++----------------------------------
 1 file changed, 16 insertions(+), 58 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index fdcbdab..465a2b2 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -104,7 +104,6 @@ struct qup_i2c_dev {
 	void __iomem		*base;
 	int			irq;
 	struct clk		*clk;
-	struct clk		*pclk;
 	struct i2c_adapter	adap;
 
 	int			clk_ctl;
@@ -532,24 +531,6 @@ static struct i2c_adapter_quirks qup_i2c_quirks = {
 	.max_read_len = QUP_READ_LIMIT,
 };
 
-static void qup_i2c_enable_clocks(struct qup_i2c_dev *qup)
-{
-	clk_prepare_enable(qup->clk);
-	clk_prepare_enable(qup->pclk);
-}
-
-static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
-{
-	u32 config;
-
-	qup_i2c_change_state(qup, QUP_RESET_STATE);
-	clk_disable_unprepare(qup->clk);
-	config = readl(qup->base + QUP_CONFIG);
-	config |= QUP_CLOCK_AUTO_GATE;
-	writel(config, qup->base + QUP_CONFIG);
-	clk_disable_unprepare(qup->pclk);
-}
-
 static int qup_i2c_probe(struct platform_device *pdev)
 {
 	static const int blk_sizes[] = {4, 16, 32};
@@ -596,13 +577,10 @@ static int qup_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(qup->clk);
 	}
 
-	qup->pclk = devm_clk_get(qup->dev, "iface");
-	if (IS_ERR(qup->pclk)) {
-		dev_err(qup->dev, "Could not get iface clock\n");
-		return PTR_ERR(qup->pclk);
-	}
-
-	qup_i2c_enable_clocks(qup);
+	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
+	pm_runtime_use_autosuspend(qup->dev);
+	pm_runtime_enable(qup->dev);
+	pm_runtime_get_sync(qup->dev);
 
 	/*
 	 * Bootloaders might leave a pending interrupt on certain QUP's,
@@ -673,22 +651,15 @@ static int qup_i2c_probe(struct platform_device *pdev)
 	qup->adap.dev.of_node = pdev->dev.of_node;
 	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
 
-	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
-	pm_runtime_use_autosuspend(qup->dev);
-	pm_runtime_set_active(qup->dev);
-	pm_runtime_enable(qup->dev);
-
 	ret = i2c_add_adapter(&qup->adap);
 	if (ret)
-		goto fail_runtime;
+		goto fail;
 
 	return 0;
 
-fail_runtime:
+fail:
 	pm_runtime_disable(qup->dev);
 	pm_runtime_set_suspended(qup->dev);
-fail:
-	qup_i2c_disable_clocks(qup);
 	return ret;
 }
 
@@ -697,7 +668,6 @@ static int qup_i2c_remove(struct platform_device *pdev)
 	struct qup_i2c_dev *qup = platform_get_drvdata(pdev);
 
 	disable_irq(qup->irq);
-	qup_i2c_disable_clocks(qup);
 	i2c_del_adapter(&qup->adap);
 	pm_runtime_disable(qup->dev);
 	pm_runtime_set_suspended(qup->dev);
@@ -707,43 +677,31 @@ static int qup_i2c_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM
 static int qup_i2c_pm_suspend_runtime(struct device *device)
 {
+	u32 config;
 	struct qup_i2c_dev *qup = dev_get_drvdata(device);
 
 	dev_dbg(device, "pm_runtime: suspending...\n");
-	qup_i2c_disable_clocks(qup);
+	qup_i2c_change_state(qup, QUP_RESET_STATE);
+	config = readl(qup->base + QUP_CONFIG);
+	config |= QUP_CLOCK_AUTO_GATE;
+	writel(config, qup->base + QUP_CONFIG);
 	return 0;
 }
 
 static int qup_i2c_pm_resume_runtime(struct device *device)
 {
+	u32 config;
 	struct qup_i2c_dev *qup = dev_get_drvdata(device);
 
-	dev_dbg(device, "pm_runtime: resuming...\n");
-	qup_i2c_enable_clocks(qup);
-	return 0;
-}
-#endif
-
-#ifdef CONFIG_PM_SLEEP
-static int qup_i2c_suspend(struct device *device)
-{
-	qup_i2c_pm_suspend_runtime(device);
-	return 0;
-}
-
-static int qup_i2c_resume(struct device *device)
-{
-	qup_i2c_pm_resume_runtime(device);
-	pm_runtime_mark_last_busy(device);
-	pm_request_autosuspend(device);
+	dev_dbg(device, "pm_runtime: suspending...\n");
+	config = readl(qup->base + QUP_CONFIG);
+	config &= ~QUP_CLOCK_AUTO_GATE;
+	writel(config, qup->base + QUP_CONFIG);
 	return 0;
 }
 #endif
 
 static const struct dev_pm_ops qup_i2c_qup_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(
-		qup_i2c_suspend,
-		qup_i2c_resume)
 	SET_RUNTIME_PM_OPS(
 		qup_i2c_pm_suspend_runtime,
 		qup_i2c_pm_resume_runtime,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Kevin Hilman]

Rajendra Nayak <rnayak@codeaurora.org> writes:

> Remove all clock handling from the driver as this is not handled from
> within platform runtime callbacks.

s/not/now/  ??

Kevin

[RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Kevin Hilman]

Rajendra Nayak <rnayak@codeaurora.org> writes:

> Remove all clock handling from the driver as this is not handled from
> within platform runtime callbacks.

s/not/now/  ??

Kevin

Re: [RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Rajendra Nayak]

On 04/24/2015 02:46 AM, Kevin Hilman wrote:
> Rajendra Nayak <rnayak@codeaurora.org> writes:
>
>> Remove all clock handling from the driver as this is not handled from
>> within platform runtime callbacks.
>
> s/not/now/  ??

yeah, it was a typo. I will fix it here and in the next (6/6) patch.
thanks for taking a look.

regards,
Rajendra

[RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Rajendra Nayak]

On 04/24/2015 02:46 AM, Kevin Hilman wrote:
> Rajendra Nayak <rnayak@codeaurora.org> writes:
>
>> Remove all clock handling from the driver as this is not handled from
>> within platform runtime callbacks.
>
> s/not/now/  ??

yeah, it was a typo. I will fix it here and in the next (6/6) patch.
thanks for taking a look.

regards,
Rajendra

Re: [RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Ivan T. Ivanov]

Hi Rajendra,

> On Apr 23, 2015, at 11:45 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> 
> Remove all clock handling from the driver as this is not handled from
> within platform runtime callbacks.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
> drivers/i2c/busses/i2c-qup.c | 74 ++++++++++----------------------------------
> 1 file changed, 16 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index fdcbdab..465a2b2 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -104,7 +104,6 @@ struct qup_i2c_dev {
> 	void __iomem		*base;
> 	int			irq;
> 	struct clk		*clk;
> -	struct clk		*pclk;
> 	struct i2c_adapter	adap;
> 
> 	int			clk_ctl;
> @@ -532,24 +531,6 @@ static struct i2c_adapter_quirks qup_i2c_quirks = {
> 	.max_read_len = QUP_READ_LIMIT,
> };
> 
> -static void qup_i2c_enable_clocks(struct qup_i2c_dev *qup)
> -{
> -	clk_prepare_enable(qup->clk);
> -	clk_prepare_enable(qup->pclk);
> -}
> -
> -static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
> -{
> -	u32 config;
> -
> -	qup_i2c_change_state(qup, QUP_RESET_STATE);
> -	clk_disable_unprepare(qup->clk);
> -	config = readl(qup->base + QUP_CONFIG);
> -	config |= QUP_CLOCK_AUTO_GATE;
> -	writel(config, qup->base + QUP_CONFIG);
> -	clk_disable_unprepare(qup->pclk);
> -}
> -
> static int qup_i2c_probe(struct platform_device *pdev)
> {
> 	static const int blk_sizes[] = {4, 16, 32};
> @@ -596,13 +577,10 @@ static int qup_i2c_probe(struct platform_device *pdev)
> 		return PTR_ERR(qup->clk);
> 	}
> 
> -	qup->pclk = devm_clk_get(qup->dev, "iface");
> -	if (IS_ERR(qup->pclk)) {
> -		dev_err(qup->dev, "Could not get iface clock\n");
> -		return PTR_ERR(qup->pclk);
> -	}
> -
> -	qup_i2c_enable_clocks(qup);
> +	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
> +	pm_runtime_use_autosuspend(qup->dev);
> +	pm_runtime_enable(qup->dev);
> +	pm_runtime_get_sync(qup->dev);
> 
> 	/*
> 	 * Bootloaders might leave a pending interrupt on certain QUP's,
> @@ -673,22 +651,15 @@ static int qup_i2c_probe(struct platform_device *pdev)
> 	qup->adap.dev.of_node = pdev->dev.of_node;
> 	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
> 
> -	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
> -	pm_runtime_use_autosuspend(qup->dev);
> -	pm_runtime_set_active(qup->dev);
> -	pm_runtime_enable(qup->dev);
> -
> 	ret = i2c_add_adapter(&qup->adap);
> 	if (ret)
> -		goto fail_runtime;
> +		goto fail;
> 
> 	return 0;
> 
> -fail_runtime:
> +fail:
> 	pm_runtime_disable(qup->dev);
> 	pm_runtime_set_suspended(qup->dev);
> -fail:
> -	qup_i2c_disable_clocks(qup);
> 	return ret;
> }


Same thing like in MMC driver. Clocks should be explicitly
enabled because CONFIG_PM could be off.

Thanks,
Ivan

[RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Ivan T. Ivanov]

Hi Rajendra,

> On Apr 23, 2015, at 11:45 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> 
> Remove all clock handling from the driver as this is not handled from
> within platform runtime callbacks.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
> drivers/i2c/busses/i2c-qup.c | 74 ++++++++++----------------------------------
> 1 file changed, 16 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index fdcbdab..465a2b2 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -104,7 +104,6 @@ struct qup_i2c_dev {
> 	void __iomem		*base;
> 	int			irq;
> 	struct clk		*clk;
> -	struct clk		*pclk;
> 	struct i2c_adapter	adap;
> 
> 	int			clk_ctl;
> @@ -532,24 +531,6 @@ static struct i2c_adapter_quirks qup_i2c_quirks = {
> 	.max_read_len = QUP_READ_LIMIT,
> };
> 
> -static void qup_i2c_enable_clocks(struct qup_i2c_dev *qup)
> -{
> -	clk_prepare_enable(qup->clk);
> -	clk_prepare_enable(qup->pclk);
> -}
> -
> -static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
> -{
> -	u32 config;
> -
> -	qup_i2c_change_state(qup, QUP_RESET_STATE);
> -	clk_disable_unprepare(qup->clk);
> -	config = readl(qup->base + QUP_CONFIG);
> -	config |= QUP_CLOCK_AUTO_GATE;
> -	writel(config, qup->base + QUP_CONFIG);
> -	clk_disable_unprepare(qup->pclk);
> -}
> -
> static int qup_i2c_probe(struct platform_device *pdev)
> {
> 	static const int blk_sizes[] = {4, 16, 32};
> @@ -596,13 +577,10 @@ static int qup_i2c_probe(struct platform_device *pdev)
> 		return PTR_ERR(qup->clk);
> 	}
> 
> -	qup->pclk = devm_clk_get(qup->dev, "iface");
> -	if (IS_ERR(qup->pclk)) {
> -		dev_err(qup->dev, "Could not get iface clock\n");
> -		return PTR_ERR(qup->pclk);
> -	}
> -
> -	qup_i2c_enable_clocks(qup);
> +	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
> +	pm_runtime_use_autosuspend(qup->dev);
> +	pm_runtime_enable(qup->dev);
> +	pm_runtime_get_sync(qup->dev);
> 
> 	/*
> 	 * Bootloaders might leave a pending interrupt on certain QUP's,
> @@ -673,22 +651,15 @@ static int qup_i2c_probe(struct platform_device *pdev)
> 	qup->adap.dev.of_node = pdev->dev.of_node;
> 	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
> 
> -	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
> -	pm_runtime_use_autosuspend(qup->dev);
> -	pm_runtime_set_active(qup->dev);
> -	pm_runtime_enable(qup->dev);
> -
> 	ret = i2c_add_adapter(&qup->adap);
> 	if (ret)
> -		goto fail_runtime;
> +		goto fail;
> 
> 	return 0;
> 
> -fail_runtime:
> +fail:
> 	pm_runtime_disable(qup->dev);
> 	pm_runtime_set_suspended(qup->dev);
> -fail:
> -	qup_i2c_disable_clocks(qup);
> 	return ret;
> }


Same thing like in MMC driver. Clocks should be explicitly
enabled because CONFIG_PM could be off.

Thanks,
Ivan

Re: [RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Rajendra Nayak]

Hi Ivan,

On 04/25/2015 12:31 PM, Ivan T. Ivanov wrote:
> Hi Rajendra,
>
>> On Apr 23, 2015, at 11:45 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>> Remove all clock handling from the driver as this is not handled from
>> within platform runtime callbacks.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>> drivers/i2c/busses/i2c-qup.c | 74 ++++++++++----------------------------------
>> 1 file changed, 16 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>> index fdcbdab..465a2b2 100644
>> --- a/drivers/i2c/busses/i2c-qup.c
>> +++ b/drivers/i2c/busses/i2c-qup.c
>> @@ -104,7 +104,6 @@ struct qup_i2c_dev {
>> 	void __iomem		*base;
>> 	int			irq;
>> 	struct clk		*clk;
>> -	struct clk		*pclk;
>> 	struct i2c_adapter	adap;
>>
>> 	int			clk_ctl;
>> @@ -532,24 +531,6 @@ static struct i2c_adapter_quirks qup_i2c_quirks = {
>> 	.max_read_len = QUP_READ_LIMIT,
>> };
>>
>> -static void qup_i2c_enable_clocks(struct qup_i2c_dev *qup)
>> -{
>> -	clk_prepare_enable(qup->clk);
>> -	clk_prepare_enable(qup->pclk);
>> -}
>> -
>> -static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
>> -{
>> -	u32 config;
>> -
>> -	qup_i2c_change_state(qup, QUP_RESET_STATE);
>> -	clk_disable_unprepare(qup->clk);
>> -	config = readl(qup->base + QUP_CONFIG);
>> -	config |= QUP_CLOCK_AUTO_GATE;
>> -	writel(config, qup->base + QUP_CONFIG);
>> -	clk_disable_unprepare(qup->pclk);
>> -}
>> -
>> static int qup_i2c_probe(struct platform_device *pdev)
>> {
>> 	static const int blk_sizes[] = {4, 16, 32};
>> @@ -596,13 +577,10 @@ static int qup_i2c_probe(struct platform_device *pdev)
>> 		return PTR_ERR(qup->clk);
>> 	}
>>
>> -	qup->pclk = devm_clk_get(qup->dev, "iface");
>> -	if (IS_ERR(qup->pclk)) {
>> -		dev_err(qup->dev, "Could not get iface clock\n");
>> -		return PTR_ERR(qup->pclk);
>> -	}
>> -
>> -	qup_i2c_enable_clocks(qup);
>> +	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
>> +	pm_runtime_use_autosuspend(qup->dev);
>> +	pm_runtime_enable(qup->dev);
>> +	pm_runtime_get_sync(qup->dev);
>>
>> 	/*
>> 	 * Bootloaders might leave a pending interrupt on certain QUP's,
>> @@ -673,22 +651,15 @@ static int qup_i2c_probe(struct platform_device *pdev)
>> 	qup->adap.dev.of_node = pdev->dev.of_node;
>> 	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
>>
>> -	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
>> -	pm_runtime_use_autosuspend(qup->dev);
>> -	pm_runtime_set_active(qup->dev);
>> -	pm_runtime_enable(qup->dev);
>> -
>> 	ret = i2c_add_adapter(&qup->adap);
>> 	if (ret)
>> -		goto fail_runtime;
>> +		goto fail;
>>
>> 	return 0;
>>
>> -fail_runtime:
>> +fail:
>> 	pm_runtime_disable(qup->dev);
>> 	pm_runtime_set_suspended(qup->dev);
>> -fail:
>> -	qup_i2c_disable_clocks(qup);
>> 	return ret;
>> }
>
>
> Same thing like in MMC driver. Clocks should be explicitly
> enabled because CONFIG_PM could be off.

Like I responded on the mmc driver patch thread [1], pm_clk_notify()
already takes care of enabling all clocks (during driver bind) in
case CONFIG_PM is disabled.

[1] http://www.spinics.net/lists/arm-kernel/msg413898.html

regards,
Rajendra

[RFC/RFT 5/6] i2c: qup: Get rid of clock handling as its done using runtime callbacks

[Rajendra Nayak]

Hi Ivan,

On 04/25/2015 12:31 PM, Ivan T. Ivanov wrote:
> Hi Rajendra,
>
>> On Apr 23, 2015, at 11:45 AM, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>> Remove all clock handling from the driver as this is not handled from
>> within platform runtime callbacks.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>> drivers/i2c/busses/i2c-qup.c | 74 ++++++++++----------------------------------
>> 1 file changed, 16 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>> index fdcbdab..465a2b2 100644
>> --- a/drivers/i2c/busses/i2c-qup.c
>> +++ b/drivers/i2c/busses/i2c-qup.c
>> @@ -104,7 +104,6 @@ struct qup_i2c_dev {
>> 	void __iomem		*base;
>> 	int			irq;
>> 	struct clk		*clk;
>> -	struct clk		*pclk;
>> 	struct i2c_adapter	adap;
>>
>> 	int			clk_ctl;
>> @@ -532,24 +531,6 @@ static struct i2c_adapter_quirks qup_i2c_quirks = {
>> 	.max_read_len = QUP_READ_LIMIT,
>> };
>>
>> -static void qup_i2c_enable_clocks(struct qup_i2c_dev *qup)
>> -{
>> -	clk_prepare_enable(qup->clk);
>> -	clk_prepare_enable(qup->pclk);
>> -}
>> -
>> -static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
>> -{
>> -	u32 config;
>> -
>> -	qup_i2c_change_state(qup, QUP_RESET_STATE);
>> -	clk_disable_unprepare(qup->clk);
>> -	config = readl(qup->base + QUP_CONFIG);
>> -	config |= QUP_CLOCK_AUTO_GATE;
>> -	writel(config, qup->base + QUP_CONFIG);
>> -	clk_disable_unprepare(qup->pclk);
>> -}
>> -
>> static int qup_i2c_probe(struct platform_device *pdev)
>> {
>> 	static const int blk_sizes[] = {4, 16, 32};
>> @@ -596,13 +577,10 @@ static int qup_i2c_probe(struct platform_device *pdev)
>> 		return PTR_ERR(qup->clk);
>> 	}
>>
>> -	qup->pclk = devm_clk_get(qup->dev, "iface");
>> -	if (IS_ERR(qup->pclk)) {
>> -		dev_err(qup->dev, "Could not get iface clock\n");
>> -		return PTR_ERR(qup->pclk);
>> -	}
>> -
>> -	qup_i2c_enable_clocks(qup);
>> +	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
>> +	pm_runtime_use_autosuspend(qup->dev);
>> +	pm_runtime_enable(qup->dev);
>> +	pm_runtime_get_sync(qup->dev);
>>
>> 	/*
>> 	 * Bootloaders might leave a pending interrupt on certain QUP's,
>> @@ -673,22 +651,15 @@ static int qup_i2c_probe(struct platform_device *pdev)
>> 	qup->adap.dev.of_node = pdev->dev.of_node;
>> 	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
>>
>> -	pm_runtime_set_autosuspend_delay(qup->dev, MSEC_PER_SEC);
>> -	pm_runtime_use_autosuspend(qup->dev);
>> -	pm_runtime_set_active(qup->dev);
>> -	pm_runtime_enable(qup->dev);
>> -
>> 	ret = i2c_add_adapter(&qup->adap);
>> 	if (ret)
>> -		goto fail_runtime;
>> +		goto fail;
>>
>> 	return 0;
>>
>> -fail_runtime:
>> +fail:
>> 	pm_runtime_disable(qup->dev);
>> 	pm_runtime_set_suspended(qup->dev);
>> -fail:
>> -	qup_i2c_disable_clocks(qup);
>> 	return ret;
>> }
>
>
> Same thing like in MMC driver. Clocks should be explicitly
> enabled because CONFIG_PM could be off.

Like I responded on the mmc driver patch thread [1], pm_clk_notify()
already takes care of enabling all clocks (during driver bind) in
case CONFIG_PM is disabled.

[1] http://www.spinics.net/lists/arm-kernel/msg413898.html

regards,
Rajendra

[RFC/RFT 6/6] spi: qup: Get rid of clock handling as its done using runtime callbacks

[Rajendra Nayak]

Remove all clock handling from the driver as this is not handled from
within platform runtime callbacks.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/spi/spi-qup.c | 54 ++++++++++-----------------------------------------
 1 file changed, 10 insertions(+), 44 deletions(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 810a7fa..a95993b 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -131,7 +131,6 @@ struct spi_qup {
 	void __iomem		*base;
 	struct device		*dev;
 	struct clk		*cclk;	/* core clock */
-	struct clk		*iclk;	/* interface clock */
 	int			irq;
 	spinlock_t		lock;
 
@@ -753,7 +752,7 @@ err_tx:
 static int spi_qup_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;
-	struct clk *iclk, *cclk;
+	struct clk *cclk;
 	struct spi_qup *controller;
 	struct resource *res;
 	struct device *dev;
@@ -775,10 +774,6 @@ static int spi_qup_probe(struct platform_device *pdev)
 	if (IS_ERR(cclk))
 		return PTR_ERR(cclk);
 
-	iclk = devm_clk_get(dev, "iface");
-	if (IS_ERR(iclk))
-		return PTR_ERR(iclk);
-
 	/* This is optional parameter */
 	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
 		max_freq = SPI_MAX_RATE;
@@ -788,23 +783,15 @@ static int spi_qup_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	ret = clk_prepare_enable(cclk);
-	if (ret) {
-		dev_err(dev, "cannot enable core clock\n");
-		return ret;
-	}
-
-	ret = clk_prepare_enable(iclk);
-	if (ret) {
-		clk_disable_unprepare(cclk);
-		dev_err(dev, "cannot enable iface clock\n");
-		return ret;
-	}
+	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 
 	master = spi_alloc_master(dev, sizeof(struct spi_qup));
 	if (!master) {
-		clk_disable_unprepare(cclk);
-		clk_disable_unprepare(iclk);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_disable(dev);
 		dev_err(dev, "cannot allocate master\n");
 		return -ENOMEM;
 	}
@@ -832,7 +819,6 @@ static int spi_qup_probe(struct platform_device *pdev)
 
 	controller->dev = dev;
 	controller->base = base;
-	controller->iclk = iclk;
 	controller->cclk = cclk;
 	controller->irq = irq;
 
@@ -904,24 +890,17 @@ static int spi_qup_probe(struct platform_device *pdev)
 	if (ret)
 		goto error_dma;
 
-	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
-	pm_runtime_use_autosuspend(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
 	ret = devm_spi_register_master(dev, master);
 	if (ret)
-		goto disable_pm;
+		goto error_dma;
 
 	return 0;
 
-disable_pm:
-	pm_runtime_disable(&pdev->dev);
 error_dma:
 	spi_qup_release_dma(master);
 error:
-	clk_disable_unprepare(cclk);
-	clk_disable_unprepare(iclk);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	spi_master_put(master);
 	return ret;
 }
@@ -969,8 +948,6 @@ static int spi_qup_suspend(struct device *device)
 	if (ret)
 		return ret;
 
-	clk_disable_unprepare(controller->cclk);
-	clk_disable_unprepare(controller->iclk);
 	return 0;
 }
 
@@ -980,14 +957,6 @@ static int spi_qup_resume(struct device *device)
 	struct spi_qup *controller = spi_master_get_devdata(master);
 	int ret;
 
-	ret = clk_prepare_enable(controller->iclk);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(controller->cclk);
-	if (ret)
-		return ret;
-
 	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
 	if (ret)
 		return ret;
@@ -1012,9 +981,6 @@ static int spi_qup_remove(struct platform_device *pdev)
 
 	spi_qup_release_dma(master);
 
-	clk_disable_unprepare(controller->cclk);
-	clk_disable_unprepare(controller->iclk);
-
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	return 0;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[RFC/RFT 6/6] spi: qup: Get rid of clock handling as its done using runtime callbacks

[Rajendra Nayak]

Remove all clock handling from the driver as this is not handled from
within platform runtime callbacks.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/spi/spi-qup.c | 54 ++++++++++-----------------------------------------
 1 file changed, 10 insertions(+), 44 deletions(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 810a7fa..a95993b 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -131,7 +131,6 @@ struct spi_qup {
 	void __iomem		*base;
 	struct device		*dev;
 	struct clk		*cclk;	/* core clock */
-	struct clk		*iclk;	/* interface clock */
 	int			irq;
 	spinlock_t		lock;
 
@@ -753,7 +752,7 @@ err_tx:
 static int spi_qup_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;
-	struct clk *iclk, *cclk;
+	struct clk *cclk;
 	struct spi_qup *controller;
 	struct resource *res;
 	struct device *dev;
@@ -775,10 +774,6 @@ static int spi_qup_probe(struct platform_device *pdev)
 	if (IS_ERR(cclk))
 		return PTR_ERR(cclk);
 
-	iclk = devm_clk_get(dev, "iface");
-	if (IS_ERR(iclk))
-		return PTR_ERR(iclk);
-
 	/* This is optional parameter */
 	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
 		max_freq = SPI_MAX_RATE;
@@ -788,23 +783,15 @@ static int spi_qup_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	ret = clk_prepare_enable(cclk);
-	if (ret) {
-		dev_err(dev, "cannot enable core clock\n");
-		return ret;
-	}
-
-	ret = clk_prepare_enable(iclk);
-	if (ret) {
-		clk_disable_unprepare(cclk);
-		dev_err(dev, "cannot enable iface clock\n");
-		return ret;
-	}
+	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
 
 	master = spi_alloc_master(dev, sizeof(struct spi_qup));
 	if (!master) {
-		clk_disable_unprepare(cclk);
-		clk_disable_unprepare(iclk);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_disable(dev);
 		dev_err(dev, "cannot allocate master\n");
 		return -ENOMEM;
 	}
@@ -832,7 +819,6 @@ static int spi_qup_probe(struct platform_device *pdev)
 
 	controller->dev = dev;
 	controller->base = base;
-	controller->iclk = iclk;
 	controller->cclk = cclk;
 	controller->irq = irq;
 
@@ -904,24 +890,17 @@ static int spi_qup_probe(struct platform_device *pdev)
 	if (ret)
 		goto error_dma;
 
-	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
-	pm_runtime_use_autosuspend(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
 	ret = devm_spi_register_master(dev, master);
 	if (ret)
-		goto disable_pm;
+		goto error_dma;
 
 	return 0;
 
-disable_pm:
-	pm_runtime_disable(&pdev->dev);
 error_dma:
 	spi_qup_release_dma(master);
 error:
-	clk_disable_unprepare(cclk);
-	clk_disable_unprepare(iclk);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	spi_master_put(master);
 	return ret;
 }
@@ -969,8 +948,6 @@ static int spi_qup_suspend(struct device *device)
 	if (ret)
 		return ret;
 
-	clk_disable_unprepare(controller->cclk);
-	clk_disable_unprepare(controller->iclk);
 	return 0;
 }
 
@@ -980,14 +957,6 @@ static int spi_qup_resume(struct device *device)
 	struct spi_qup *controller = spi_master_get_devdata(master);
 	int ret;
 
-	ret = clk_prepare_enable(controller->iclk);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(controller->cclk);
-	if (ret)
-		return ret;
-
 	ret = spi_qup_set_state(controller, QUP_STATE_RESET);
 	if (ret)
 		return ret;
@@ -1012,9 +981,6 @@ static int spi_qup_remove(struct platform_device *pdev)
 
 	spi_qup_release_dma(master);
 
-	clk_disable_unprepare(controller->cclk);
-	clk_disable_unprepare(controller->iclk);
-
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	return 0;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation