diff mbox series

[V2,03/12] clk: tegra: save and restore PLLs state for system

Message ID 1559084936-4610-4-git-send-email-skomatineni@nvidia.com (mailing list archive)
State Changes Requested, archived
Headers show
Series LP0 entry and exit support for Tegra210 | expand

Commit Message

Sowjanya Komatineni May 28, 2019, 11:08 p.m. UTC
This patch has implementation of saving and restoring PLL's state to
support system suspend and resume operations.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 drivers/clk/tegra/clk-divider.c | 19 ++++++++
 drivers/clk/tegra/clk-pll-out.c | 25 +++++++++++
 drivers/clk/tegra/clk-pll.c     | 99 ++++++++++++++++++++++++++++++++---------
 drivers/clk/tegra/clk.h         |  9 ++++
 4 files changed, 132 insertions(+), 20 deletions(-)

Comments

Stephen Boyd May 29, 2019, 11:28 p.m. UTC | #1
Quoting Sowjanya Komatineni (2019-05-28 16:08:47)
> This patch has implementation of saving and restoring PLL's state to
> support system suspend and resume operations.

Can you provide some more background on _why_ this patch should exist?
That's typically what gets written in the commit text.

> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/clk/tegra/clk-divider.c | 19 ++++++++
>  drivers/clk/tegra/clk-pll-out.c | 25 +++++++++++
>  drivers/clk/tegra/clk-pll.c     | 99 ++++++++++++++++++++++++++++++++---------
>  drivers/clk/tegra/clk.h         |  9 ++++
>  4 files changed, 132 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
> index 2a1822a22740..718694727042 100644
> --- a/drivers/clk/tegra/clk-divider.c
> +++ b/drivers/clk/tegra/clk-divider.c
> @@ -14,6 +14,7 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/kernel.h>
>  #include <linux/io.h>
>  #include <linux/err.h>
> @@ -179,3 +180,21 @@ struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
>                                           reg, 16, 1, CLK_DIVIDER_READ_ONLY,
>                                           mc_div_table, lock);
>  }
> +
> +#if defined(CONFIG_PM_SLEEP)
> +void tegra_clk_divider_resume(struct clk_hw *hw, unsigned long rate)
> +{
> +       struct clk_hw *parent = clk_hw_get_parent(hw);
> +       unsigned long parent_rate;
> +
> +       if (IS_ERR(parent)) {

Will this ever happen? Collapse the WARN_ON into the if please:

	if (WARN_ON(IS_ERR(parent)))

> +               WARN_ON(1);
> +               return;
> +       }
> +
> +       parent_rate = clk_hw_get_rate(parent);
> +
> +       if (clk_frac_div_set_rate(hw, rate, parent_rate) < 0)
> +               WARN_ON(1);
> +}
> +#endif
> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> index 09bccbb9640c..e4d124cc5657 100644
> --- a/drivers/clk/tegra/clk.h
> +++ b/drivers/clk/tegra/clk.h
> @@ -841,6 +841,15 @@ int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
>  int div_frac_get(unsigned long rate, unsigned parent_rate, u8 width,
>                  u8 frac_width, u8 flags);
>  
> +#ifdef CONFIG_PM_SLEEP

Can you remove this ifdef? It just complicates compilation testing.

> +void tegra_clk_pll_resume(struct clk *c, unsigned long rate);
> +void tegra_clk_divider_resume(struct clk_hw *hw, unsigned long rate);
> +void tegra_clk_pll_out_resume(struct clk *clk, unsigned long rate);
> +void tegra_clk_plle_tegra210_resume(struct clk *c);
> +void tegra_clk_sync_state_pll(struct clk *c);
> +void tegra_clk_sync_state_pll_out(struct clk *clk);

Do these APIs need to operate on struct clk? Why can't they operate on
clk_hw or why can't we drive the suspend/resume sequence from the clk
provider driver itself?
Sowjanya Komatineni May 31, 2019, 7:52 p.m. UTC | #2
On 5/29/19 4:28 PM, Stephen Boyd wrote:
> Quoting Sowjanya Komatineni (2019-05-28 16:08:47)
>> This patch has implementation of saving and restoring PLL's state to
>> support system suspend and resume operations.
> Can you provide some more background on _why_ this patch should exist?
> That's typically what gets written in the commit text.
Will add more in next version of this series.
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>   drivers/clk/tegra/clk-divider.c | 19 ++++++++
>>   drivers/clk/tegra/clk-pll-out.c | 25 +++++++++++
>>   drivers/clk/tegra/clk-pll.c     | 99 ++++++++++++++++++++++++++++++++---------
>>   drivers/clk/tegra/clk.h         |  9 ++++
>>   4 files changed, 132 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
>> index 2a1822a22740..718694727042 100644
>> --- a/drivers/clk/tegra/clk-divider.c
>> +++ b/drivers/clk/tegra/clk-divider.c
>> @@ -14,6 +14,7 @@
>>    * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>    */
>>   
>> +#include <linux/clk.h>
>>   #include <linux/kernel.h>
>>   #include <linux/io.h>
>>   #include <linux/err.h>
>> @@ -179,3 +180,21 @@ struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
>>                                            reg, 16, 1, CLK_DIVIDER_READ_ONLY,
>>                                            mc_div_table, lock);
>>   }
>> +
>> +#if defined(CONFIG_PM_SLEEP)
>> +void tegra_clk_divider_resume(struct clk_hw *hw, unsigned long rate)
>> +{
>> +       struct clk_hw *parent = clk_hw_get_parent(hw);
>> +       unsigned long parent_rate;
>> +
>> +       if (IS_ERR(parent)) {
> Will this ever happen? Collapse the WARN_ON into the if please:
>
> 	if (WARN_ON(IS_ERR(parent)))
>
Will fix in next version of this series.
>> +               WARN_ON(1);
>> +               return;
>> +       }
>> +
>> +       parent_rate = clk_hw_get_rate(parent);
>> +
>> +       if (clk_frac_div_set_rate(hw, rate, parent_rate) < 0)
>> +               WARN_ON(1);
>> +}
>> +#endif
>> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
>> index 09bccbb9640c..e4d124cc5657 100644
>> --- a/drivers/clk/tegra/clk.h
>> +++ b/drivers/clk/tegra/clk.h
>> @@ -841,6 +841,15 @@ int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
>>   int div_frac_get(unsigned long rate, unsigned parent_rate, u8 width,
>>                   u8 frac_width, u8 flags);
>>   
>> +#ifdef CONFIG_PM_SLEEP
> Can you remove this ifdef? It just complicates compilation testing.
OK, Will fix in next version
>> +void tegra_clk_pll_resume(struct clk *c, unsigned long rate);
>> +void tegra_clk_divider_resume(struct clk_hw *hw, unsigned long rate);
>> +void tegra_clk_pll_out_resume(struct clk *clk, unsigned long rate);
>> +void tegra_clk_plle_tegra210_resume(struct clk *c);
>> +void tegra_clk_sync_state_pll(struct clk *c);
>> +void tegra_clk_sync_state_pll_out(struct clk *clk);
> Do these APIs need to operate on struct clk? Why can't they operate on
> clk_hw or why can't we drive the suspend/resume sequence from the clk
> provider driver itself?
>
Yes can change to use clk_hw.

By clk provider driver, are you referring to clk-tegra210?

clk-terga210 driver has suspend/resume implementation. These API's are 
for corresponding clock specific implementations (clk-pll, clk-pll-out, 
clk-divider) for enabling and restoring to proper rate and are invoked 
during clk-tegra210 driver resume.

thanks

Sowjanya
Stephen Boyd June 5, 2019, 11:31 p.m. UTC | #3
Quoting Sowjanya Komatineni (2019-05-31 12:52:44)
> 
> On 5/29/19 4:28 PM, Stephen Boyd wrote:
> > Quoting Sowjanya Komatineni (2019-05-28 16:08:47)
> >> +               WARN_ON(1);
> >> +               return;
> >> +       }
> >> +
> >> +       parent_rate = clk_hw_get_rate(parent);
> >> +
> >> +       if (clk_frac_div_set_rate(hw, rate, parent_rate) < 0)
> >> +               WARN_ON(1);
> >> +}
> >> +#endif
> >> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> >> index 09bccbb9640c..e4d124cc5657 100644
> >> --- a/drivers/clk/tegra/clk.h
> >> +++ b/drivers/clk/tegra/clk.h
> >> @@ -841,6 +841,15 @@ int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
> >>   int div_frac_get(unsigned long rate, unsigned parent_rate, u8 width,
> >>                   u8 frac_width, u8 flags);
> >>   
> >> +#ifdef CONFIG_PM_SLEEP
> > Can you remove this ifdef? It just complicates compilation testing.
> OK, Will fix in next version
> >> +void tegra_clk_pll_resume(struct clk *c, unsigned long rate);
> >> +void tegra_clk_divider_resume(struct clk_hw *hw, unsigned long rate);
> >> +void tegra_clk_pll_out_resume(struct clk *clk, unsigned long rate);
> >> +void tegra_clk_plle_tegra210_resume(struct clk *c);
> >> +void tegra_clk_sync_state_pll(struct clk *c);
> >> +void tegra_clk_sync_state_pll_out(struct clk *clk);
> > Do these APIs need to operate on struct clk? Why can't they operate on
> > clk_hw or why can't we drive the suspend/resume sequence from the clk
> > provider driver itself?
> >
> Yes can change to use clk_hw.
> 
> By clk provider driver, are you referring to clk-tegra210?

I guess so.

> 
> clk-terga210 driver has suspend/resume implementation. These API's are 
> for corresponding clock specific implementations (clk-pll, clk-pll-out, 
> clk-divider) for enabling and restoring to proper rate and are invoked 
> during clk-tegra210 driver resume.

Yes, so when the clk provider suspends it needs to do something? Our
handling of clk rates and other state like enable/disable over
suspend/resume isn't really well thought out or implemented so far. TI
has some code to do some stuff, but otherwise I haven't seen drivers
handling this. Ideally it would be something generic in the framework so
that drivers don't have to work around stuff.
diff mbox series

Patch

diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
index 2a1822a22740..718694727042 100644
--- a/drivers/clk/tegra/clk-divider.c
+++ b/drivers/clk/tegra/clk-divider.c
@@ -14,6 +14,7 @@ 
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/err.h>
@@ -179,3 +180,21 @@  struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
 					  reg, 16, 1, CLK_DIVIDER_READ_ONLY,
 					  mc_div_table, lock);
 }
+
+#if defined(CONFIG_PM_SLEEP)
+void tegra_clk_divider_resume(struct clk_hw *hw, unsigned long rate)
+{
+	struct clk_hw *parent = clk_hw_get_parent(hw);
+	unsigned long parent_rate;
+
+	if (IS_ERR(parent)) {
+		WARN_ON(1);
+		return;
+	}
+
+	parent_rate = clk_hw_get_rate(parent);
+
+	if (clk_frac_div_set_rate(hw, rate, parent_rate) < 0)
+		WARN_ON(1);
+}
+#endif
diff --git a/drivers/clk/tegra/clk-pll-out.c b/drivers/clk/tegra/clk-pll-out.c
index 257cae0c1488..8b8c3b77d243 100644
--- a/drivers/clk/tegra/clk-pll-out.c
+++ b/drivers/clk/tegra/clk-pll-out.c
@@ -14,6 +14,7 @@ 
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/clk.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/err.h>
@@ -120,3 +121,27 @@  struct clk *tegra_clk_register_pll_out(const char *name,
 
 	return clk;
 }
+
+#if defined(CONFIG_PM_SLEEP)
+void tegra_clk_pll_out_resume(struct clk *clk, unsigned long rate)
+{
+	struct clk_hw *hw = __clk_get_hw(clk);
+	struct clk_hw *parent = clk_hw_get_parent(hw);
+
+	if (IS_ERR(parent)) {
+		WARN_ON(1);
+		return;
+	}
+
+	tegra_clk_divider_resume(parent, rate);
+	clk_pll_out_enable(hw);
+}
+
+void tegra_clk_sync_state_pll_out(struct clk *clk)
+{
+	struct clk_hw *hw = __clk_get_hw(clk);
+
+	if (!__clk_get_enable_count(clk))
+		clk_pll_out_disable(hw);
+}
+#endif
diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 6b976b2514f7..b363b6c6f600 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -20,6 +20,7 @@ 
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/clk/tegra.h>
 
 #include "clk.h"
 
@@ -1813,6 +1814,28 @@  static int clk_pllu_tegra114_enable(struct clk_hw *hw)
 
 	return ret;
 }
+
+static void _clk_plle_tegra_init_parent(struct tegra_clk_pll *pll)
+{
+	u32 val, val_aux;
+
+	/* ensure parent is set to pll_ref */
+
+	val = pll_readl_base(pll);
+	val_aux = pll_readl(pll->params->aux_reg, pll);
+
+	if (val & PLL_BASE_ENABLE) {
+		if ((val_aux & PLLE_AUX_PLLRE_SEL) ||
+		    (val_aux & PLLE_AUX_PLLP_SEL))
+			WARN(1, "pll_e enabled with unsupported parent %s\n",
+			     (val_aux & PLLE_AUX_PLLP_SEL) ? "pllp_out0" :
+			     "pll_re_vco");
+	} else {
+		val_aux &= ~(PLLE_AUX_PLLRE_SEL | PLLE_AUX_PLLP_SEL);
+		pll_writel(val_aux, pll->params->aux_reg, pll);
+		fence_udelay(1, pll->clk_base);
+	}
+}
 #endif
 
 static struct tegra_clk_pll *_tegra_init_pll(void __iomem *clk_base,
@@ -2289,6 +2312,21 @@  static const struct clk_ops tegra_clk_pllss_ops = {
 	.set_rate = clk_pllxc_set_rate,
 };
 
+static void _pllss_set_defaults(struct tegra_clk_pll *pll)
+{
+	u32 val;
+
+	pll_writel_misc(PLLSS_MISC_DEFAULT, pll);
+	pll_writel(PLLSS_CFG_DEFAULT, pll->params->ext_misc_reg[0], pll);
+	pll_writel(PLLSS_CTRL1_DEFAULT, pll->params->ext_misc_reg[1], pll);
+	pll_writel(PLLSS_CTRL2_DEFAULT, pll->params->ext_misc_reg[2], pll);
+
+	val = pll_readl_base(pll);
+	val &= ~PLLSS_LOCK_OVERRIDE;
+	pll_writel_base(val, pll);
+}
+
 struct clk *tegra_clk_register_pllss(const char *name, const char *parent_name,
 				void __iomem *clk_base, unsigned long flags,
 				struct tegra_clk_pll_params *pll_params,
@@ -2339,10 +2377,7 @@  struct clk *tegra_clk_register_pllss(const char *name, const char *parent_name,
 
 	_update_pll_mnp(pll, &cfg);
 
-	pll_writel_misc(PLLSS_MISC_DEFAULT, pll);
-	pll_writel(PLLSS_CFG_DEFAULT, pll_params->ext_misc_reg[0], pll);
-	pll_writel(PLLSS_CTRL1_DEFAULT, pll_params->ext_misc_reg[1], pll);
-	pll_writel(PLLSS_CTRL1_DEFAULT, pll_params->ext_misc_reg[2], pll);
+	_pllss_set_defaults(pll);
 
 	val = pll_readl_base(pll);
 	val_iddq = readl_relaxed(clk_base + pll_params->iddq_reg);
@@ -2546,27 +2581,12 @@  struct clk *tegra_clk_register_plle_tegra210(const char *name,
 {
 	struct tegra_clk_pll *pll;
 	struct clk *clk;
-	u32 val, val_aux;
 
 	pll = _tegra_init_pll(clk_base, NULL, pll_params, lock);
 	if (IS_ERR(pll))
 		return ERR_CAST(pll);
 
-	/* ensure parent is set to pll_re_vco */
-
-	val = pll_readl_base(pll);
-	val_aux = pll_readl(pll_params->aux_reg, pll);
-
-	if (val & PLLE_BASE_ENABLE) {
-		if ((val_aux & PLLE_AUX_PLLRE_SEL) ||
-			(val_aux & PLLE_AUX_PLLP_SEL))
-			WARN(1, "pll_e enabled with unsupported parent %s\n",
-			  (val_aux & PLLE_AUX_PLLP_SEL) ? "pllp_out0" :
-					"pll_re_vco");
-	} else {
-		val_aux &= ~(PLLE_AUX_PLLRE_SEL | PLLE_AUX_PLLP_SEL);
-		pll_writel(val_aux, pll_params->aux_reg, pll);
-	}
+	_clk_plle_tegra_init_parent(pll);
 
 	clk = _tegra_clk_register_pll(pll, name, parent_name, flags,
 				      &tegra_clk_plle_tegra210_ops);
@@ -2710,3 +2730,42 @@  struct clk *tegra_clk_register_pllmb(const char *name, const char *parent_name,
 }
 
 #endif
+
+#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARCH_TEGRA_210_SOC)
+void tegra_clk_pll_resume(struct clk *c, unsigned long rate)
+{
+	struct clk_hw *hw = __clk_get_hw(c);
+	struct tegra_clk_pll *pll = to_clk_pll(hw);
+	struct clk_hw *parent = clk_hw_get_parent(hw);
+
+	if (clk_pll_is_enabled(hw))
+		return;
+
+	if (IS_ERR(parent)) {
+		WARN_ON(1);
+		return;
+	}
+
+	if (pll->params->set_defaults)
+		pll->params->set_defaults(pll);
+
+	clk_set_rate(c, rate);
+	clk_pll_enable(hw);
+}
+
+void tegra_clk_sync_state_pll(struct clk *c)
+{
+	struct clk_hw *hw = __clk_get_hw(c);
+
+	if (!__clk_get_enable_count(c))
+		clk_pll_disable(hw);
+}
+
+void tegra_clk_plle_tegra210_resume(struct clk *c)
+{
+	struct clk_hw *hw = __clk_get_hw(c);
+	struct tegra_clk_pll *pll = to_clk_pll(hw);
+
+	_clk_plle_tegra_init_parent(pll);
+}
+#endif
diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
index 09bccbb9640c..e4d124cc5657 100644
--- a/drivers/clk/tegra/clk.h
+++ b/drivers/clk/tegra/clk.h
@@ -841,6 +841,15 @@  int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
 int div_frac_get(unsigned long rate, unsigned parent_rate, u8 width,
 		 u8 frac_width, u8 flags);
 
+#ifdef CONFIG_PM_SLEEP
+void tegra_clk_pll_resume(struct clk *c, unsigned long rate);
+void tegra_clk_divider_resume(struct clk_hw *hw, unsigned long rate);
+void tegra_clk_pll_out_resume(struct clk *clk, unsigned long rate);
+void tegra_clk_plle_tegra210_resume(struct clk *c);
+void tegra_clk_sync_state_pll(struct clk *c);
+void tegra_clk_sync_state_pll_out(struct clk *clk);
+#endif
+
 
 /* Combined read fence with delay */
 #define fence_udelay(delay, reg)	\