[PATCH] thermal: ti-soc-thermal: Enable addition power management

[PATCH] thermal: ti-soc-thermal: Enable addition power management

[Adam Ford]

The bandgap sensor can be idled when the processor is, but it
isn't currently being done. The power consumption of OMAP3
boards can elevated if the bangap sensor is enabled, because
the bandgap clock blocks deeper idle states the SoC.

We should idle bandgap with cpu_notifier instead of
runtime PM to avoid tagging it with pm_runtime_irq_safe()
that we want to stop using for drivers in general.

This patch uses additional power management to idle the clock
of the bandgap when it is not needed.

Suggested-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
index 2fa78f738568..d203ec041c39 100644
--- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
+++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
@@ -26,10 +26,18 @@
 #include <linux/of_irq.h>
 #include <linux/of_gpio.h>
 #include <linux/io.h>
+#include <linux/cpu_pm.h>
+#include <linux/device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include "ti-bandgap.h"
 
 static int ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id);
+static int bandgap_omap_cpu_notifier(struct notifier_block *nb,
+				  unsigned long cmd, void *v);
 
 /***   Helper functions to access registers and their bitfields   ***/
 
@@ -1025,6 +1033,9 @@ int ti_bandgap_probe(struct platform_device *pdev)
 		}
 	}
 
+	bgp->nb.notifier_call = bandgap_omap_cpu_notifier;
+	cpu_pm_register_notifier(&bgp->nb);
+
 	return 0;
 
 remove_last_cooling:
@@ -1174,6 +1185,38 @@ static int ti_bandgap_suspend(struct device *dev)
 	return err;
 }
 
+static int bandgap_omap_cpu_notifier(struct notifier_block *nb,
+				  unsigned long cmd, void *v)
+{
+	struct ti_bandgap *bgp;
+
+	bgp = container_of(nb, struct ti_bandgap, nb);
+
+	spin_lock(&bgp->lock);
+	switch (cmd) {
+	case CPU_CLUSTER_PM_ENTER:
+		if (bgp->is_suspended)
+			break;
+		ti_bandgap_save_ctxt(bgp);
+		ti_bandgap_power(bgp, false);
+		if (TI_BANDGAP_HAS(bgp, CLK_CTRL))
+			clk_disable(bgp->fclock);
+		break;
+	case CPU_CLUSTER_PM_ENTER_FAILED:
+	case CPU_CLUSTER_PM_EXIT:
+		if (bgp->is_suspended)
+			break;
+		if (TI_BANDGAP_HAS(bgp, CLK_CTRL))
+			clk_enable(bgp->fclock);
+		ti_bandgap_power(bgp, true);
+		ti_bandgap_restore_ctxt(bgp);
+		break;
+	}
+	spin_unlock(&bgp->lock);
+
+	return NOTIFY_OK;
+}
+
 static int ti_bandgap_resume(struct device *dev)
 {
 	struct ti_bandgap *bgp = dev_get_drvdata(dev);
diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.h b/drivers/thermal/ti-soc-thermal/ti-bandgap.h
index bb9b0f7faf99..a21d07a1a23a 100644
--- a/drivers/thermal/ti-soc-thermal/ti-bandgap.h
+++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.h
@@ -12,6 +12,10 @@
 #include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/err.h>
+#include <linux/cpu_pm.h>
+#include <linux/device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm.h>
 
 /**
  * DOC: bandgap driver data structure
@@ -201,6 +205,8 @@ struct ti_bandgap {
 	int				irq;
 	int				tshut_gpio;
 	u32				clk_rate;
+	struct notifier_block		nb;
+	unsigned int is_suspended:1;
 };
 
 /**
-- 
2.20.1

Re: [PATCH] thermal: ti-soc-thermal: Enable addition power management

[Tony Lindgren]

* Adam Ford <aford173@gmail.com> [191108 21:00]:
> +static int bandgap_omap_cpu_notifier(struct notifier_block *nb,
> +				  unsigned long cmd, void *v);
>  
>  /***   Helper functions to access registers and their bitfields   ***/
>  
> @@ -1025,6 +1033,9 @@ int ti_bandgap_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	bgp->nb.notifier_call = bandgap_omap_cpu_notifier;
> +	cpu_pm_register_notifier(&bgp->nb);
> +

Hmm looks like you're missing the related call to
cpu_pm_unregister_notifier(), right?

Other than that, it also works on droid4, so please
feel free to add:

Reviewed-by: Tony Lindgren <tony@atomide.com>
Tested-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH] thermal: ti-soc-thermal: Enable addition power management

[Adam Ford]

On Fri, Nov 8, 2019 at 3:17 PM Tony Lindgren <tony@atomide.com> wrote:
>
> * Adam Ford <aford173@gmail.com> [191108 21:00]:
> > +static int bandgap_omap_cpu_notifier(struct notifier_block *nb,
> > +                               unsigned long cmd, void *v);
> >
> >  /***   Helper functions to access registers and their bitfields   ***/
> >
> > @@ -1025,6 +1033,9 @@ int ti_bandgap_probe(struct platform_device *pdev)
> >               }
> >       }
> >
> > +     bgp->nb.notifier_call = bandgap_omap_cpu_notifier;
> > +     cpu_pm_register_notifier(&bgp->nb);
> > +
>
> Hmm looks like you're missing the related call to
> cpu_pm_unregister_notifier(), right?

Good catch.  I'm new to this PM stuff.  :-)

>
> Other than that, it also works on droid4, so please
> feel free to add:

Awesome!

>
> Reviewed-by: Tony Lindgren <tony@atomide.com>
> Tested-by: Tony Lindgren <tony@atomide.com>

Thank you.  I have already resent V2

adam

>

[PATCH] thermal: ti-soc-thermal: Enable addition power management

[Adam Ford]

The bandgap sensor can be idled when the processor is too, but it
isn't currently being done, so the power consumption of OMAP3
boards can elevated if the bangap sensor is enabled.

This patch attempts to use some additional power management
to idle the clock to the bandgap when not needed.

Suggested-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.c b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
index 2fa78f738568..7a221f71bc9f 100644
--- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
+++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
@@ -26,10 +26,18 @@
 #include <linux/of_irq.h>
 #include <linux/of_gpio.h>
 #include <linux/io.h>
+#include <linux/cpu_pm.h>
+#include <linux/device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include "ti-bandgap.h"
 
 static int ti_bandgap_force_single_read(struct ti_bandgap *bgp, int id);
+static int bandgap_omap_cpu_notifier(struct notifier_block *nb,
+				  unsigned long cmd, void *v);
 
 /***   Helper functions to access registers and their bitfields   ***/
 
@@ -1025,6 +1033,9 @@ int ti_bandgap_probe(struct platform_device *pdev)
 		}
 	}
 
+	bgp->nb.notifier_call = bandgap_omap_cpu_notifier;
+	cpu_pm_register_notifier(&bgp->nb);
+
 	return 0;
 
 remove_last_cooling:
@@ -1174,6 +1185,38 @@ static int ti_bandgap_suspend(struct device *dev)
 	return err;
 }
 
+static int bandgap_omap_cpu_notifier(struct notifier_block *nb,
+				  unsigned long cmd, void *v)
+{
+	struct ti_bandgap *bgp;
+
+	bgp = container_of(nb, struct ti_bandgap, nb);
+
+	spin_lock(&bgp->lock);
+	switch (cmd) {
+	case CPU_CLUSTER_PM_ENTER:
+		if (bgp->is_suspended)
+			break;
+		ti_bandgap_save_ctxt(bgp);
+		ti_bandgap_power(bgp, false);
+		if (TI_BANDGAP_HAS(bgp, CLK_CTRL))
+			clk_disable_unprepare(bgp->fclock);
+		break;
+	case CPU_CLUSTER_PM_ENTER_FAILED:
+	case CPU_CLUSTER_PM_EXIT:
+		if (bgp->is_suspended)
+			break;
+		if (TI_BANDGAP_HAS(bgp, CLK_CTRL))
+			clk_prepare_enable(bgp->fclock);
+		ti_bandgap_power(bgp, true);
+		ti_bandgap_restore_ctxt(bgp);
+		break;
+	}
+	spin_unlock(&bgp->lock);
+
+	return NOTIFY_OK;
+}
+
 static int ti_bandgap_resume(struct device *dev)
 {
 	struct ti_bandgap *bgp = dev_get_drvdata(dev);
diff --git a/drivers/thermal/ti-soc-thermal/ti-bandgap.h b/drivers/thermal/ti-soc-thermal/ti-bandgap.h
index bb9b0f7faf99..a21d07a1a23a 100644
--- a/drivers/thermal/ti-soc-thermal/ti-bandgap.h
+++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.h
@@ -12,6 +12,10 @@
 #include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/err.h>
+#include <linux/cpu_pm.h>
+#include <linux/device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pm.h>
 
 /**
  * DOC: bandgap driver data structure
@@ -201,6 +205,8 @@ struct ti_bandgap {
 	int				irq;
 	int				tshut_gpio;
 	u32				clk_rate;
+	struct notifier_block		nb;
+	unsigned int is_suspended:1;
 };
 
 /**
-- 
2.20.1

Re: [PATCH] thermal: ti-soc-thermal: Enable addition power management

[Tony Lindgren]

* Adam Ford <aford173@gmail.com> [191108 20:05]:
> The bandgap sensor can be idled when the processor is too, but it
> isn't currently being done, so the power consumption of OMAP3
> boards can elevated if the bangap sensor is enabled.

Great, thanks for doing this!

> This patch attempts to use some additional power management
> to idle the clock to the bandgap when not needed.

Maybe add also something like this to the patch description:

As otherwise the bandgap clock blocks deeper idle states
the SoC. To must idle bandgap with cpu_notifier instead of
runtime PM to avoid tagging it with pm_runtime_irq_safe()
that we want to stop using for drivers in general.

> --- a/drivers/thermal/ti-soc-thermal/ti-bandgap.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-bandgap.c
...
> +static int bandgap_omap_cpu_notifier(struct notifier_block *nb,
> +				  unsigned long cmd, void *v)
> +{
> +	struct ti_bandgap *bgp;
> +
> +	bgp = container_of(nb, struct ti_bandgap, nb);
> +
> +	spin_lock(&bgp->lock);
> +	switch (cmd) {
> +	case CPU_CLUSTER_PM_ENTER:
> +		if (bgp->is_suspended)
> +			break;
> +		ti_bandgap_save_ctxt(bgp);
> +		ti_bandgap_power(bgp, false);
> +		if (TI_BANDGAP_HAS(bgp, CLK_CTRL))
> +			clk_disable_unprepare(bgp->fclock);
> +		break;
> +	case CPU_CLUSTER_PM_ENTER_FAILED:
> +	case CPU_CLUSTER_PM_EXIT:
> +		if (bgp->is_suspended)
> +			break;
> +		if (TI_BANDGAP_HAS(bgp, CLK_CTRL))
> +			clk_prepare_enable(bgp->fclock);
> +		ti_bandgap_power(bgp, true);
> +		ti_bandgap_restore_ctxt(bgp);
> +		break;
> +	}
> +	spin_unlock(&bgp->lock);
> +
> +	return NOTIFY_OK;
> +}

You need to use clk_disable() and clk_enable() instead
of the prepare and unprepare variants here because
the prepare and unprepare variants are helpers for
non-atomic context and won't necessarily work for the
cpu_notifier. See the comments in include/linux/clk.h
for clock_prepare_enable().

I tested this briefly with CONFIG_OMAP3_THERMAL=y and
after idling the UARTs on torpedo, and it still hits
off mode during idle consuming 5 - 7 mW for the
main_battery_som line as measured with the shunt :)
So this means we can finally then enable also
CONFIG_OMAP3_THERMAL=y in the defconfigs.

Also, you may want to check if the driver needs to
save and restore it's context in the notifier as that
might get lost during the off mode depending what
domain it's at.

Regards,

Tony

Re: [PATCH] thermal: ti-soc-thermal: Enable addition power management

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [191108 20:46]:
> Also, you may want to check if the driver needs to
> save and restore it's context in the notifier as that
> might get lost during the off mode depending what
> domain it's at.

Oh never mind, looks like you already took care of
saving and restoring the context in the notifier,
I just missed it.

Thanks,

Tony

Re: [PATCH] thermal: ti-soc-thermal: Enable addition power management

[Adam Ford]

On Fri, Nov 8, 2019 at 2:50 PM Tony Lindgren <tony@atomide.com> wrote:
>
> * Tony Lindgren <tony@atomide.com> [191108 20:46]:
> > Also, you may want to check if the driver needs to
> > save and restore it's context in the notifier as that
> > might get lost during the off mode depending what
> > domain it's at.
>
> Oh never mind, looks like you already took care of
> saving and restoring the context in the notifier,
> I just missed it.

Great!

Is there any testing you can do and think we need on the OMAP4/5+ or
am33xx?  I don't have any of that hardware.

I'm readying a patch without the RFC shortly.

adam
>
> Thanks,
>
> Tony

Re: [PATCH] thermal: ti-soc-thermal: Enable addition power management

[Tony Lindgren]

* Adam Ford <aford173@gmail.com> [191108 20:58]:
> On Fri, Nov 8, 2019 at 2:50 PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > * Tony Lindgren <tony@atomide.com> [191108 20:46]:
> > > Also, you may want to check if the driver needs to
> > > save and restore it's context in the notifier as that
> > > might get lost during the off mode depending what
> > > domain it's at.
> >
> > Oh never mind, looks like you already took care of
> > saving and restoring the context in the notifier,
> > I just missed it.
> 
> Great!
> 
> Is there any testing you can do and think we need on the OMAP4/5+ or
> am33xx?  I don't have any of that hardware.

I can test oswr idle (open switch retention) on droid4
which means save and restore of context is needed like
it is for off mode.

> I'm readying a patch without the RFC shortly.

OK will give it a try and reply when done.

Regards,

Tony