[PATCH] Revert "firmware: arm_scmi: Add clock management to the SCMI power domain"

[PATCH] Revert "firmware: arm_scmi: Add clock management to the SCMI power domain"

[Ulf Hansson]

This reverts commit a3b884cef873 ("firmware: arm_scmi: Add clock management
to the SCMI power domain").

Using the GENPD_FLAG_PM_CLK tells genpd to gate/ungate the consumer
device's clock(s) during runtime suspend/resume through the PM clock API.
More precisely, in genpd_runtime_resume() the clock(s) for the consumer
device would become ungated prior to the driver-level ->runtime_resume()
callbacks gets invoked.

This behaviour isn't a good fit for all platforms/drivers. For example, a
driver may need to make some preparations of its device in its
->runtime_resume() callback, like calling clk_set_rate() before the
clock(s) should be ungated. In these cases, it's easier to let the clock(s)
to be managed solely by the driver, rather than at the PM domain level.

For these reasons, let's drop the use GENPD_FLAG_PM_CLK for the SCMI PM
domain, as to enable it to be more easily adopted across ARM platforms.

Fixes: a3b884cef873 ("firmware: arm_scmi: Add clock management to the SCMI power domain")
Cc: Nicolas Pitre <npitre@baylibre.com>
Cc: stable@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

To get some more background to $subject patch, please have a look at the
lore-link below.

https://lore.kernel.org/all/DU0PR04MB94173B45A2CFEE3BF1BD313A88409@DU0PR04MB9417.eurprd04.prod.outlook.com/

Kind regards
Ulf Hansson

---
 drivers/firmware/arm_scmi/scmi_pm_domain.c | 26 ----------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/firmware/arm_scmi/scmi_pm_domain.c b/drivers/firmware/arm_scmi/scmi_pm_domain.c
index 581d34c95769..d5dee625de78 100644
--- a/drivers/firmware/arm_scmi/scmi_pm_domain.c
+++ b/drivers/firmware/arm_scmi/scmi_pm_domain.c
@@ -8,7 +8,6 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/pm_clock.h>
 #include <linux/pm_domain.h>
 #include <linux/scmi_protocol.h>
 
@@ -53,27 +52,6 @@ static int scmi_pd_power_off(struct generic_pm_domain *domain)
 	return scmi_pd_power(domain, false);
 }
 
-static int scmi_pd_attach_dev(struct generic_pm_domain *pd, struct device *dev)
-{
-	int ret;
-
-	ret = pm_clk_create(dev);
-	if (ret)
-		return ret;
-
-	ret = of_pm_clk_add_clks(dev);
-	if (ret >= 0)
-		return 0;
-
-	pm_clk_destroy(dev);
-	return ret;
-}
-
-static void scmi_pd_detach_dev(struct generic_pm_domain *pd, struct device *dev)
-{
-	pm_clk_destroy(dev);
-}
-
 static int scmi_pm_domain_probe(struct scmi_device *sdev)
 {
 	int num_domains, i;
@@ -124,10 +102,6 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
 		scmi_pd->genpd.name = scmi_pd->name;
 		scmi_pd->genpd.power_off = scmi_pd_power_off;
 		scmi_pd->genpd.power_on = scmi_pd_power_on;
-		scmi_pd->genpd.attach_dev = scmi_pd_attach_dev;
-		scmi_pd->genpd.detach_dev = scmi_pd_detach_dev;
-		scmi_pd->genpd.flags = GENPD_FLAG_PM_CLK |
-				       GENPD_FLAG_ACTIVE_WAKEUP;
 
 		pm_genpd_init(&scmi_pd->genpd, NULL,
 			      state == SCMI_POWER_STATE_GENERIC_OFF);
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] Revert "firmware: arm_scmi: Add clock management to the SCMI power domain"

[Peng Fan]

> Subject: [PATCH] Revert "firmware: arm_scmi: Add clock management to
> the SCMI power domain"
> 
> This reverts commit a3b884cef873 ("firmware: arm_scmi: Add clock
> management to the SCMI power domain").
> 
> Using the GENPD_FLAG_PM_CLK tells genpd to gate/ungate the consumer
> device's clock(s) during runtime suspend/resume through the PM clock API.
> More precisely, in genpd_runtime_resume() the clock(s) for the consumer
> device would become ungated prior to the driver-level ->runtime_resume()
> callbacks gets invoked.
> 
> This behaviour isn't a good fit for all platforms/drivers. For example, a
> driver may need to make some preparations of its device in its
> ->runtime_resume() callback, like calling clk_set_rate() before the
> clock(s) should be ungated. In these cases, it's easier to let the clock(s) to be
> managed solely by the driver, rather than at the PM domain level.
> 
> For these reasons, let's drop the use GENPD_FLAG_PM_CLK for the SCMI PM
> domain, as to enable it to be more easily adopted across ARM platforms.
> 
> Fixes: a3b884cef873 ("firmware: arm_scmi: Add clock management to the
> SCMI power domain")
> Cc: Nicolas Pitre <npitre@baylibre.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Tested-by: Peng Fan <peng.fan@nxp.com>

Thanks,
Peng.

> ---
> 
> To get some more background to $subject patch, please have a look at the
> lore-link below.
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Fall%2FDU0PR04MB94173B45A2CFEE3BF1BD313A88409%40D
> U0PR04MB9417.eurprd04.prod.outlook.com%2F&amp;data=05%7C01%7Cp
> eng.fan%40nxp.com%7C05437bc19cb14760450a08da9a3968c0%7C686ea1d
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637991868616277658%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik
> 1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=QpQfK8lTApY
> qhYj80lUudQJMCujYyN9j1RNB4Q00wrM%3D&amp;reserved=0
> 
> Kind regards
> Ulf Hansson
> 
> ---
>  drivers/firmware/arm_scmi/scmi_pm_domain.c | 26 ----------------------
>  1 file changed, 26 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/scmi_pm_domain.c
> b/drivers/firmware/arm_scmi/scmi_pm_domain.c
> index 581d34c95769..d5dee625de78 100644
> --- a/drivers/firmware/arm_scmi/scmi_pm_domain.c
> +++ b/drivers/firmware/arm_scmi/scmi_pm_domain.c
> @@ -8,7 +8,6 @@
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> -#include <linux/pm_clock.h>
>  #include <linux/pm_domain.h>
>  #include <linux/scmi_protocol.h>
> 
> @@ -53,27 +52,6 @@ static int scmi_pd_power_off(struct
> generic_pm_domain *domain)
>  	return scmi_pd_power(domain, false);
>  }
> 
> -static int scmi_pd_attach_dev(struct generic_pm_domain *pd, struct device
> *dev) -{
> -	int ret;
> -
> -	ret = pm_clk_create(dev);
> -	if (ret)
> -		return ret;
> -
> -	ret = of_pm_clk_add_clks(dev);
> -	if (ret >= 0)
> -		return 0;
> -
> -	pm_clk_destroy(dev);
> -	return ret;
> -}
> -
> -static void scmi_pd_detach_dev(struct generic_pm_domain *pd, struct
> device *dev) -{
> -	pm_clk_destroy(dev);
> -}
> -
>  static int scmi_pm_domain_probe(struct scmi_device *sdev)  {
>  	int num_domains, i;
> @@ -124,10 +102,6 @@ static int scmi_pm_domain_probe(struct
> scmi_device *sdev)
>  		scmi_pd->genpd.name = scmi_pd->name;
>  		scmi_pd->genpd.power_off = scmi_pd_power_off;
>  		scmi_pd->genpd.power_on = scmi_pd_power_on;
> -		scmi_pd->genpd.attach_dev = scmi_pd_attach_dev;
> -		scmi_pd->genpd.detach_dev = scmi_pd_detach_dev;
> -		scmi_pd->genpd.flags = GENPD_FLAG_PM_CLK |
> -				       GENPD_FLAG_ACTIVE_WAKEUP;
> 
>  		pm_genpd_init(&scmi_pd->genpd, NULL,
>  			      state == SCMI_POWER_STATE_GENERIC_OFF);
> --
> 2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "firmware: arm_scmi: Add clock management to the SCMI power domain"

[Sudeep Holla]

Hi Dien, Gaku,

On Mon, Sep 19, 2022 at 02:20:33PM +0200, Ulf Hansson wrote:
> This reverts commit a3b884cef873 ("firmware: arm_scmi: Add clock management
> to the SCMI power domain").
> 
> Using the GENPD_FLAG_PM_CLK tells genpd to gate/ungate the consumer
> device's clock(s) during runtime suspend/resume through the PM clock API.
> More precisely, in genpd_runtime_resume() the clock(s) for the consumer
> device would become ungated prior to the driver-level ->runtime_resume()
> callbacks gets invoked.
> 
> This behaviour isn't a good fit for all platforms/drivers. For example, a
> driver may need to make some preparations of its device in its
> ->runtime_resume() callback, like calling clk_set_rate() before the
> clock(s) should be ungated. In these cases, it's easier to let the clock(s)
> to be managed solely by the driver, rather than at the PM domain level.
> 
> For these reasons, let's drop the use GENPD_FLAG_PM_CLK for the SCMI PM
> domain, as to enable it to be more easily adopted across ARM platforms.
> 
> Fixes: a3b884cef873 ("firmware: arm_scmi: Add clock management to the SCMI power domain")
> Cc: Nicolas Pitre <npitre@baylibre.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> To get some more background to $subject patch, please have a look at the
> lore-link below.
> 
> https://lore.kernel.org/all/DU0PR04MB94173B45A2CFEE3BF1BD313A88409@DU0PR04MB9417.eurprd04.prod.outlook.com/
>

If you have any objections, this is your last chance to speak up before
the original change gets reverted in the mainline with this patch.

Hi Ulf,

I don't have any other SCMI changes for v6.0 fixes or v6.1
I am fine if you are happy to take this via your tree or I can send it
to SoC team. Let me know. I will give final one or 2 days for Renesas
to get back if they really care much.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "firmware: arm_scmi: Add clock management to the SCMI power domain"

[Ulf Hansson]

On Wed, 21 Sept 2022 at 17:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Hi Dien, Gaku,
>
> On Mon, Sep 19, 2022 at 02:20:33PM +0200, Ulf Hansson wrote:
> > This reverts commit a3b884cef873 ("firmware: arm_scmi: Add clock management
> > to the SCMI power domain").
> >
> > Using the GENPD_FLAG_PM_CLK tells genpd to gate/ungate the consumer
> > device's clock(s) during runtime suspend/resume through the PM clock API.
> > More precisely, in genpd_runtime_resume() the clock(s) for the consumer
> > device would become ungated prior to the driver-level ->runtime_resume()
> > callbacks gets invoked.
> >
> > This behaviour isn't a good fit for all platforms/drivers. For example, a
> > driver may need to make some preparations of its device in its
> > ->runtime_resume() callback, like calling clk_set_rate() before the
> > clock(s) should be ungated. In these cases, it's easier to let the clock(s)
> > to be managed solely by the driver, rather than at the PM domain level.
> >
> > For these reasons, let's drop the use GENPD_FLAG_PM_CLK for the SCMI PM
> > domain, as to enable it to be more easily adopted across ARM platforms.
> >
> > Fixes: a3b884cef873 ("firmware: arm_scmi: Add clock management to the SCMI power domain")
> > Cc: Nicolas Pitre <npitre@baylibre.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > To get some more background to $subject patch, please have a look at the
> > lore-link below.
> >
> > https://lore.kernel.org/all/DU0PR04MB94173B45A2CFEE3BF1BD313A88409@DU0PR04MB9417.eurprd04.prod.outlook.com/
> >
>
> If you have any objections, this is your last chance to speak up before
> the original change gets reverted in the mainline with this patch.
>
> Hi Ulf,
>
> I don't have any other SCMI changes for v6.0 fixes or v6.1
> I am fine if you are happy to take this via your tree or I can send it
> to SoC team. Let me know. I will give final one or 2 days for Renesas
> to get back if they really care much.

I have a slew of fixes for mmc that I intend to send next week, I can
funnel them through that pull request.

Assuming, Renesas folkz are okay, I consider that as an ack from you, right?

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "firmware: arm_scmi: Add clock management to the SCMI power domain"

[Sudeep Holla]

On Wed, Sep 21, 2022 at 08:46:21PM +0200, Ulf Hansson wrote:
> On Wed, 21 Sept 2022 at 17:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > Hi Dien, Gaku,
> >
> > On Mon, Sep 19, 2022 at 02:20:33PM +0200, Ulf Hansson wrote:
> > > This reverts commit a3b884cef873 ("firmware: arm_scmi: Add clock management
> > > to the SCMI power domain").
> > >
> > > Using the GENPD_FLAG_PM_CLK tells genpd to gate/ungate the consumer
> > > device's clock(s) during runtime suspend/resume through the PM clock API.
> > > More precisely, in genpd_runtime_resume() the clock(s) for the consumer
> > > device would become ungated prior to the driver-level ->runtime_resume()
> > > callbacks gets invoked.
> > >
> > > This behaviour isn't a good fit for all platforms/drivers. For example, a
> > > driver may need to make some preparations of its device in its
> > > ->runtime_resume() callback, like calling clk_set_rate() before the
> > > clock(s) should be ungated. In these cases, it's easier to let the clock(s)
> > > to be managed solely by the driver, rather than at the PM domain level.
> > >
> > > For these reasons, let's drop the use GENPD_FLAG_PM_CLK for the SCMI PM
> > > domain, as to enable it to be more easily adopted across ARM platforms.
> > >
> > > Fixes: a3b884cef873 ("firmware: arm_scmi: Add clock management to the SCMI power domain")
> > > Cc: Nicolas Pitre <npitre@baylibre.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >
> > > To get some more background to $subject patch, please have a look at the
> > > lore-link below.
> > >
> > > https://lore.kernel.org/all/DU0PR04MB94173B45A2CFEE3BF1BD313A88409@DU0PR04MB9417.eurprd04.prod.outlook.com/
> > >
> >
> > If you have any objections, this is your last chance to speak up before
> > the original change gets reverted in the mainline with this patch.
> >
> > Hi Ulf,
> >
> > I don't have any other SCMI changes for v6.0 fixes or v6.1
> > I am fine if you are happy to take this via your tree or I can send it
> > to SoC team. Let me know. I will give final one or 2 days for Renesas
> > to get back if they really care much.
> 
> I have a slew of fixes for mmc that I intend to send next week, I can
> funnel them through that pull request.
> 
> Assuming, Renesas folkz are okay, I consider that as an ack from you, right?
>

Yes for official reasons, here is the formal :)

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

in case you manage to get this in via your tree.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Revert "firmware: arm_scmi: Add clock management to the SCMI power domain"

[Ulf Hansson]

On Thu, 22 Sept 2022 at 13:04, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Sep 21, 2022 at 08:46:21PM +0200, Ulf Hansson wrote:
> > On Wed, 21 Sept 2022 at 17:56, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > Hi Dien, Gaku,
> > >
> > > On Mon, Sep 19, 2022 at 02:20:33PM +0200, Ulf Hansson wrote:
> > > > This reverts commit a3b884cef873 ("firmware: arm_scmi: Add clock management
> > > > to the SCMI power domain").
> > > >
> > > > Using the GENPD_FLAG_PM_CLK tells genpd to gate/ungate the consumer
> > > > device's clock(s) during runtime suspend/resume through the PM clock API.
> > > > More precisely, in genpd_runtime_resume() the clock(s) for the consumer
> > > > device would become ungated prior to the driver-level ->runtime_resume()
> > > > callbacks gets invoked.
> > > >
> > > > This behaviour isn't a good fit for all platforms/drivers. For example, a
> > > > driver may need to make some preparations of its device in its
> > > > ->runtime_resume() callback, like calling clk_set_rate() before the
> > > > clock(s) should be ungated. In these cases, it's easier to let the clock(s)
> > > > to be managed solely by the driver, rather than at the PM domain level.
> > > >
> > > > For these reasons, let's drop the use GENPD_FLAG_PM_CLK for the SCMI PM
> > > > domain, as to enable it to be more easily adopted across ARM platforms.
> > > >
> > > > Fixes: a3b884cef873 ("firmware: arm_scmi: Add clock management to the SCMI power domain")
> > > > Cc: Nicolas Pitre <npitre@baylibre.com>
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >
> > > > To get some more background to $subject patch, please have a look at the
> > > > lore-link below.
> > > >
> > > > https://lore.kernel.org/all/DU0PR04MB94173B45A2CFEE3BF1BD313A88409@DU0PR04MB9417.eurprd04.prod.outlook.com/
> > > >
> > >
> > > If you have any objections, this is your last chance to speak up before
> > > the original change gets reverted in the mainline with this patch.
> > >
> > > Hi Ulf,
> > >
> > > I don't have any other SCMI changes for v6.0 fixes or v6.1
> > > I am fine if you are happy to take this via your tree or I can send it
> > > to SoC team. Let me know. I will give final one or 2 days for Renesas
> > > to get back if they really care much.
> >
> > I have a slew of fixes for mmc that I intend to send next week, I can
> > funnel them through that pull request.
> >
> > Assuming, Renesas folkz are okay, I consider that as an ack from you, right?
> >
>
> Yes for official reasons, here is the formal :)
>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>

Thanks!

> in case you manage to get this in via your tree.

I have now queued up the patch through
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git fixes

Let's see how it goes when it gets tested in linux-next.

>
> --
> Regards,
> Sudeep

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel