[PATCH] cpufreq: tegra: add regulator dependency for T124

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Arnd Bergmann]

This driver is the only one that calls regulator_sync_voltage(), but it
can currently be built with CONFIG_REGULATOR disabled, producing
this build error:

drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
  regulator_sync_voltage(priv->vdd_cpu_reg);

My first attempt was to implement a helper for this function
for regulator_sync_voltage, but Mark Brown explained:

   We don't do this for *all* regulator API functions - there's some where
   using them strongly suggests that there is actually a dependency on
   the regulator API.  This does seem like it might be falling into the
   specialist category [...]
   Looking at the code I'm pretty unclear on what the authors think the
   use of _sync_voltage() is doing in the first place so it may be even
   better to just remove the call.  It seems to have been included in the
   first commit so there's not changelog explaining things and there's
   no comment either.  I'd *expect* it to be a noop as far as I can see.

This adds the dependency to make the driver always build successfully
or not be enabled at all. Alternatively, we could investigate if the
driver should stop calling regulator_sync_voltage instead.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 235a1ba73d92..b1f8a73e5a94 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -226,7 +226,7 @@ config ARM_TEGRA20_CPUFREQ
 
 config ARM_TEGRA124_CPUFREQ
 	tristate "Tegra124 CPUFreq support"
-	depends on ARCH_TEGRA && CPUFREQ_DT
+	depends on ARCH_TEGRA && CPUFREQ_DT && REGULATOR
 	default y
 	help
 	  This adds the CPUFreq driver support for Tegra124 SOCs.

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Arnd Bergmann]

This driver is the only one that calls regulator_sync_voltage(), but it
can currently be built with CONFIG_REGULATOR disabled, producing
this build error:

drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
  regulator_sync_voltage(priv->vdd_cpu_reg);

My first attempt was to implement a helper for this function
for regulator_sync_voltage, but Mark Brown explained:

   We don't do this for *all* regulator API functions - there's some where
   using them strongly suggests that there is actually a dependency on
   the regulator API.  This does seem like it might be falling into the
   specialist category [...]
   Looking at the code I'm pretty unclear on what the authors think the
   use of _sync_voltage() is doing in the first place so it may be even
   better to just remove the call.  It seems to have been included in the
   first commit so there's not changelog explaining things and there's
   no comment either.  I'd *expect* it to be a noop as far as I can see.

This adds the dependency to make the driver always build successfully
or not be enabled at all. Alternatively, we could investigate if the
driver should stop calling regulator_sync_voltage instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 235a1ba73d92..b1f8a73e5a94 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -226,7 +226,7 @@ config ARM_TEGRA20_CPUFREQ
 
 config ARM_TEGRA124_CPUFREQ
 	tristate "Tegra124 CPUFreq support"
-	depends on ARCH_TEGRA && CPUFREQ_DT
+	depends on ARCH_TEGRA && CPUFREQ_DT && REGULATOR
 	default y
 	help
 	  This adds the CPUFreq driver support for Tegra124 SOCs.

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Arnd Bergmann]

This driver is the only one that calls regulator_sync_voltage(), but it
can currently be built with CONFIG_REGULATOR disabled, producing
this build error:

drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
  regulator_sync_voltage(priv->vdd_cpu_reg);

My first attempt was to implement a helper for this function
for regulator_sync_voltage, but Mark Brown explained:

   We don't do this for *all* regulator API functions - there's some where
   using them strongly suggests that there is actually a dependency on
   the regulator API.  This does seem like it might be falling into the
   specialist category [...]
   Looking at the code I'm pretty unclear on what the authors think the
   use of _sync_voltage() is doing in the first place so it may be even
   better to just remove the call.  It seems to have been included in the
   first commit so there's not changelog explaining things and there's
   no comment either.  I'd *expect* it to be a noop as far as I can see.

This adds the dependency to make the driver always build successfully
or not be enabled at all. Alternatively, we could investigate if the
driver should stop calling regulator_sync_voltage instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 235a1ba73d92..b1f8a73e5a94 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -226,7 +226,7 @@ config ARM_TEGRA20_CPUFREQ
 
 config ARM_TEGRA124_CPUFREQ
 	tristate "Tegra124 CPUFreq support"
-	depends on ARCH_TEGRA && CPUFREQ_DT
+	depends on ARCH_TEGRA && CPUFREQ_DT && REGULATOR
 	default y
 	help
 	  This adds the CPUFreq driver support for Tegra124 SOCs.

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Viresh Kumar]

On 08-12-15, 22:52, Arnd Bergmann wrote:
> This driver is the only one that calls regulator_sync_voltage(), but it
> can currently be built with CONFIG_REGULATOR disabled, producing
> this build error:
> 
> drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
> drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
>   regulator_sync_voltage(priv->vdd_cpu_reg);
> 
> My first attempt was to implement a helper for this function
> for regulator_sync_voltage, but Mark Brown explained:
> 
>    We don't do this for *all* regulator API functions - there's some where
>    using them strongly suggests that there is actually a dependency on
>    the regulator API.  This does seem like it might be falling into the
>    specialist category [...]
>    Looking at the code I'm pretty unclear on what the authors think the
>    use of _sync_voltage() is doing in the first place so it may be even
>    better to just remove the call.  It seems to have been included in the
>    first commit so there's not changelog explaining things and there's
>    no comment either.  I'd *expect* it to be a noop as far as I can see.
> 
> This adds the dependency to make the driver always build successfully
> or not be enabled at all. Alternatively, we could investigate if the
> driver should stop calling regulator_sync_voltage instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 235a1ba73d92..b1f8a73e5a94 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -226,7 +226,7 @@ config ARM_TEGRA20_CPUFREQ
>  
>  config ARM_TEGRA124_CPUFREQ
>  	tristate "Tegra124 CPUFreq support"
> -	depends on ARCH_TEGRA && CPUFREQ_DT
> +	depends on ARCH_TEGRA && CPUFREQ_DT && REGULATOR
>  	default y
>  	help
>  	  This adds the CPUFreq driver support for Tegra124 SOCs.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Viresh Kumar]

On 08-12-15, 22:52, Arnd Bergmann wrote:
> This driver is the only one that calls regulator_sync_voltage(), but it
> can currently be built with CONFIG_REGULATOR disabled, producing
> this build error:
> 
> drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
> drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
>   regulator_sync_voltage(priv->vdd_cpu_reg);
> 
> My first attempt was to implement a helper for this function
> for regulator_sync_voltage, but Mark Brown explained:
> 
>    We don't do this for *all* regulator API functions - there's some where
>    using them strongly suggests that there is actually a dependency on
>    the regulator API.  This does seem like it might be falling into the
>    specialist category [...]
>    Looking at the code I'm pretty unclear on what the authors think the
>    use of _sync_voltage() is doing in the first place so it may be even
>    better to just remove the call.  It seems to have been included in the
>    first commit so there's not changelog explaining things and there's
>    no comment either.  I'd *expect* it to be a noop as far as I can see.
> 
> This adds the dependency to make the driver always build successfully
> or not be enabled at all. Alternatively, we could investigate if the
> driver should stop calling regulator_sync_voltage instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 235a1ba73d92..b1f8a73e5a94 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -226,7 +226,7 @@ config ARM_TEGRA20_CPUFREQ
>  
>  config ARM_TEGRA124_CPUFREQ
>  	tristate "Tegra124 CPUFreq support"
> -	depends on ARCH_TEGRA && CPUFREQ_DT
> +	depends on ARCH_TEGRA && CPUFREQ_DT && REGULATOR
>  	default y
>  	help
>  	  This adds the CPUFreq driver support for Tegra124 SOCs.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

Hi Arnd,

On 08/12/15 21:52, Arnd Bergmann wrote:
> This driver is the only one that calls regulator_sync_voltage(), but it
> can currently be built with CONFIG_REGULATOR disabled, producing
> this build error:
> 
> drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
> drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
>   regulator_sync_voltage(priv->vdd_cpu_reg);
> 
> My first attempt was to implement a helper for this function
> for regulator_sync_voltage, but Mark Brown explained:
> 
>    We don't do this for *all* regulator API functions - there's some where
>    using them strongly suggests that there is actually a dependency on
>    the regulator API.  This does seem like it might be falling into the
>    specialist category [...]
>    Looking at the code I'm pretty unclear on what the authors think the
>    use of _sync_voltage() is doing in the first place so it may be even
>    better to just remove the call.  It seems to have been included in the
>    first commit so there's not changelog explaining things and there's
>    no comment either.  I'd *expect* it to be a noop as far as I can see.

In this sequence we are switching from the DFLL clock source (which
directly controls the voltage) back to a PLL (which does not control the
voltage directly). What we want to do is to restore the voltage back to
the voltage it was operating at before we switched to the DFLL clock
(which could have changed it).

I am not familiar with regulator_sync_voltage() but from the comment it
does say that it will re-apply the last voltage that was configured for
the regulator. So I can see what they were doing. The question I have
is, if the consumer has not explicitly called regulator_set_voltage()
then what does regulator_sync_voltage() do? I am wondering if we should
have been doing a regulator_get_voltage() during the probe and a
regulator_set_voltage() when switching back?

> This adds the dependency to make the driver always build successfully
> or not be enabled at all. Alternatively, we could investigate if the
> driver should stop calling regulator_sync_voltage instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 235a1ba73d92..b1f8a73e5a94 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -226,7 +226,7 @@ config ARM_TEGRA20_CPUFREQ
>  
>  config ARM_TEGRA124_CPUFREQ
>  	tristate "Tegra124 CPUFreq support"
> -	depends on ARCH_TEGRA && CPUFREQ_DT
> +	depends on ARCH_TEGRA && CPUFREQ_DT && REGULATOR
>  	default y
>  	help
>  	  This adds the CPUFreq driver support for Tegra124 SOCs.

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

Hi Arnd,

On 08/12/15 21:52, Arnd Bergmann wrote:
> This driver is the only one that calls regulator_sync_voltage(), but it
> can currently be built with CONFIG_REGULATOR disabled, producing
> this build error:
> 
> drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
> drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
>   regulator_sync_voltage(priv->vdd_cpu_reg);
> 
> My first attempt was to implement a helper for this function
> for regulator_sync_voltage, but Mark Brown explained:
> 
>    We don't do this for *all* regulator API functions - there's some where
>    using them strongly suggests that there is actually a dependency on
>    the regulator API.  This does seem like it might be falling into the
>    specialist category [...]
>    Looking at the code I'm pretty unclear on what the authors think the
>    use of _sync_voltage() is doing in the first place so it may be even
>    better to just remove the call.  It seems to have been included in the
>    first commit so there's not changelog explaining things and there's
>    no comment either.  I'd *expect* it to be a noop as far as I can see.

In this sequence we are switching from the DFLL clock source (which
directly controls the voltage) back to a PLL (which does not control the
voltage directly). What we want to do is to restore the voltage back to
the voltage it was operating at before we switched to the DFLL clock
(which could have changed it).

I am not familiar with regulator_sync_voltage() but from the comment it
does say that it will re-apply the last voltage that was configured for
the regulator. So I can see what they were doing. The question I have
is, if the consumer has not explicitly called regulator_set_voltage()
then what does regulator_sync_voltage() do? I am wondering if we should
have been doing a regulator_get_voltage() during the probe and a
regulator_set_voltage() when switching back?

> This adds the dependency to make the driver always build successfully
> or not be enabled at all. Alternatively, we could investigate if the
> driver should stop calling regulator_sync_voltage instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 235a1ba73d92..b1f8a73e5a94 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -226,7 +226,7 @@ config ARM_TEGRA20_CPUFREQ
>  
>  config ARM_TEGRA124_CPUFREQ
>  	tristate "Tegra124 CPUFreq support"
> -	depends on ARCH_TEGRA && CPUFREQ_DT
> +	depends on ARCH_TEGRA && CPUFREQ_DT && REGULATOR
>  	default y
>  	help
>  	  This adds the CPUFreq driver support for Tegra124 SOCs.

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

Hi Arnd,

On 08/12/15 21:52, Arnd Bergmann wrote:
> This driver is the only one that calls regulator_sync_voltage(), but it
> can currently be built with CONFIG_REGULATOR disabled, producing
> this build error:
> 
> drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
> drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
>   regulator_sync_voltage(priv->vdd_cpu_reg);
> 
> My first attempt was to implement a helper for this function
> for regulator_sync_voltage, but Mark Brown explained:
> 
>    We don't do this for *all* regulator API functions - there's some where
>    using them strongly suggests that there is actually a dependency on
>    the regulator API.  This does seem like it might be falling into the
>    specialist category [...]
>    Looking at the code I'm pretty unclear on what the authors think the
>    use of _sync_voltage() is doing in the first place so it may be even
>    better to just remove the call.  It seems to have been included in the
>    first commit so there's not changelog explaining things and there's
>    no comment either.  I'd *expect* it to be a noop as far as I can see.

In this sequence we are switching from the DFLL clock source (which
directly controls the voltage) back to a PLL (which does not control the
voltage directly). What we want to do is to restore the voltage back to
the voltage it was operating at before we switched to the DFLL clock
(which could have changed it).

I am not familiar with regulator_sync_voltage() but from the comment it
does say that it will re-apply the last voltage that was configured for
the regulator. So I can see what they were doing. The question I have
is, if the consumer has not explicitly called regulator_set_voltage()
then what does regulator_sync_voltage() do? I am wondering if we should
have been doing a regulator_get_voltage() during the probe and a
regulator_set_voltage() when switching back?

> This adds the dependency to make the driver always build successfully
> or not be enabled at all. Alternatively, we could investigate if the
> driver should stop calling regulator_sync_voltage instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 235a1ba73d92..b1f8a73e5a94 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -226,7 +226,7 @@ config ARM_TEGRA20_CPUFREQ
>  
>  config ARM_TEGRA124_CPUFREQ
>  	tristate "Tegra124 CPUFreq support"
> -	depends on ARCH_TEGRA && CPUFREQ_DT
> +	depends on ARCH_TEGRA && CPUFREQ_DT && REGULATOR
>  	default y
>  	help
>  	  This adds the CPUFreq driver support for Tegra124 SOCs.

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On Wed, Dec 09, 2015 at 12:03:27PM +0000, Jon Hunter wrote:
> On 08/12/15 21:52, Arnd Bergmann wrote:

> > My first attempt was to implement a helper for this function
> > for regulator_sync_voltage, but Mark Brown explained:

> >    We don't do this for *all* regulator API functions - there's some where
> >    using them strongly suggests that there is actually a dependency on
> >    the regulator API.  This does seem like it might be falling into the
> >    specialist category [...]
> >    Looking at the code I'm pretty unclear on what the authors think the
> >    use of _sync_voltage() is doing in the first place so it may be even
> >    better to just remove the call.  It seems to have been included in the
> >    first commit so there's not changelog explaining things and there's
> >    no comment either.  I'd *expect* it to be a noop as far as I can see.

> In this sequence we are switching from the DFLL clock source (which
> directly controls the voltage) back to a PLL (which does not control the
> voltage directly). What we want to do is to restore the voltage back to
> the voltage it was operating at before we switched to the DFLL clock
> (which could have changed it).

If changes implemented by the clock driver are trashing the regulator
settings I would expect the clock driver to be responsible for fixing
things up rather than another driver that happens to use the clock.  I'd
also expect some kind of internal documentation explaining what's going
on, and possibly 

> I am not familiar with regulator_sync_voltage() but from the comment it
> does say that it will re-apply the last voltage that was configured for
> the regulator. So I can see what they were doing. The question I have
> is, if the consumer has not explicitly called regulator_set_voltage()
> then what does regulator_sync_voltage() do? I am wondering if we should
> have been doing a regulator_get_voltage() during the probe and a
> regulator_set_voltage() when switching back?

This *is* the sort of thing _sync() is intended for, though it's mainly
expected to be used in cases like suspend where things have been powered
off.  As you can see from the code it's based on the settings that
software made, but then if nothing in software has any need to configure
anything then why do we even care that the hardware changed anything?

Setting the voltage you've read back sounds broken, if the hardware
might randomly change things how do you know the settings we read were
sane?  Shouldn't we know what voltage range the device requires in a
given mode and set that - that's much more normal?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Mark Brown]

On Wed, Dec 09, 2015 at 12:03:27PM +0000, Jon Hunter wrote:
> On 08/12/15 21:52, Arnd Bergmann wrote:

> > My first attempt was to implement a helper for this function
> > for regulator_sync_voltage, but Mark Brown explained:

> >    We don't do this for *all* regulator API functions - there's some where
> >    using them strongly suggests that there is actually a dependency on
> >    the regulator API.  This does seem like it might be falling into the
> >    specialist category [...]
> >    Looking at the code I'm pretty unclear on what the authors think the
> >    use of _sync_voltage() is doing in the first place so it may be even
> >    better to just remove the call.  It seems to have been included in the
> >    first commit so there's not changelog explaining things and there's
> >    no comment either.  I'd *expect* it to be a noop as far as I can see.

> In this sequence we are switching from the DFLL clock source (which
> directly controls the voltage) back to a PLL (which does not control the
> voltage directly). What we want to do is to restore the voltage back to
> the voltage it was operating at before we switched to the DFLL clock
> (which could have changed it).

If changes implemented by the clock driver are trashing the regulator
settings I would expect the clock driver to be responsible for fixing
things up rather than another driver that happens to use the clock.  I'd
also expect some kind of internal documentation explaining what's going
on, and possibly 

> I am not familiar with regulator_sync_voltage() but from the comment it
> does say that it will re-apply the last voltage that was configured for
> the regulator. So I can see what they were doing. The question I have
> is, if the consumer has not explicitly called regulator_set_voltage()
> then what does regulator_sync_voltage() do? I am wondering if we should
> have been doing a regulator_get_voltage() during the probe and a
> regulator_set_voltage() when switching back?

This *is* the sort of thing _sync() is intended for, though it's mainly
expected to be used in cases like suspend where things have been powered
off.  As you can see from the code it's based on the settings that
software made, but then if nothing in software has any need to configure
anything then why do we even care that the hardware changed anything?

Setting the voltage you've read back sounds broken, if the hardware
might randomly change things how do you know the settings we read were
sane?  Shouldn't we know what voltage range the device requires in a
given mode and set that - that's much more normal?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151209/29aea2c8/attachment.sig>

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On Wed, Dec 09, 2015 at 12:03:27PM +0000, Jon Hunter wrote:
> On 08/12/15 21:52, Arnd Bergmann wrote:

> > My first attempt was to implement a helper for this function
> > for regulator_sync_voltage, but Mark Brown explained:

> >    We don't do this for *all* regulator API functions - there's some where
> >    using them strongly suggests that there is actually a dependency on
> >    the regulator API.  This does seem like it might be falling into the
> >    specialist category [...]
> >    Looking at the code I'm pretty unclear on what the authors think the
> >    use of _sync_voltage() is doing in the first place so it may be even
> >    better to just remove the call.  It seems to have been included in the
> >    first commit so there's not changelog explaining things and there's
> >    no comment either.  I'd *expect* it to be a noop as far as I can see.

> In this sequence we are switching from the DFLL clock source (which
> directly controls the voltage) back to a PLL (which does not control the
> voltage directly). What we want to do is to restore the voltage back to
> the voltage it was operating at before we switched to the DFLL clock
> (which could have changed it).

If changes implemented by the clock driver are trashing the regulator
settings I would expect the clock driver to be responsible for fixing
things up rather than another driver that happens to use the clock.  I'd
also expect some kind of internal documentation explaining what's going
on, and possibly 

> I am not familiar with regulator_sync_voltage() but from the comment it
> does say that it will re-apply the last voltage that was configured for
> the regulator. So I can see what they were doing. The question I have
> is, if the consumer has not explicitly called regulator_set_voltage()
> then what does regulator_sync_voltage() do? I am wondering if we should
> have been doing a regulator_get_voltage() during the probe and a
> regulator_set_voltage() when switching back?

This *is* the sort of thing _sync() is intended for, though it's mainly
expected to be used in cases like suspend where things have been powered
off.  As you can see from the code it's based on the settings that
software made, but then if nothing in software has any need to configure
anything then why do we even care that the hardware changed anything?

Setting the voltage you've read back sounds broken, if the hardware
might randomly change things how do you know the settings we read were
sane?  Shouldn't we know what voltage range the device requires in a
given mode and set that - that's much more normal?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 09/12/15 14:47, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Dec 09, 2015 at 12:03:27PM +0000, Jon Hunter wrote:
>> On 08/12/15 21:52, Arnd Bergmann wrote:
> 
>>> My first attempt was to implement a helper for this function
>>> for regulator_sync_voltage, but Mark Brown explained:
> 
>>>    We don't do this for *all* regulator API functions - there's some where
>>>    using them strongly suggests that there is actually a dependency on
>>>    the regulator API.  This does seem like it might be falling into the
>>>    specialist category [...]
>>>    Looking at the code I'm pretty unclear on what the authors think the
>>>    use of _sync_voltage() is doing in the first place so it may be even
>>>    better to just remove the call.  It seems to have been included in the
>>>    first commit so there's not changelog explaining things and there's
>>>    no comment either.  I'd *expect* it to be a noop as far as I can see.
> 
>> In this sequence we are switching from the DFLL clock source (which
>> directly controls the voltage) back to a PLL (which does not control the
>> voltage directly). What we want to do is to restore the voltage back to
>> the voltage it was operating at before we switched to the DFLL clock
>> (which could have changed it).
> 
> If changes implemented by the clock driver are trashing the regulator
> settings I would expect the clock driver to be responsible for fixing
> things up rather than another driver that happens to use the clock.  I'd
> also expect some kind of internal documentation explaining what's going
> on, and possibly 

Yes, the DFLL clock driver could restore the voltage, however, that
does not guarantee that the voltage is still sufficient for the other
clock source.

>> I am not familiar with regulator_sync_voltage() but from the comment it
>> does say that it will re-apply the last voltage that was configured for
>> the regulator. So I can see what they were doing. The question I have
>> is, if the consumer has not explicitly called regulator_set_voltage()
>> then what does regulator_sync_voltage() do? I am wondering if we should
>> have been doing a regulator_get_voltage() during the probe and a
>> regulator_set_voltage() when switching back?
> 
> This *is* the sort of thing _sync() is intended for, though it's mainly
> expected to be used in cases like suspend where things have been powered
> off.  As you can see from the code it's based on the settings that
> software made, but then if nothing in software has any need to configure
> anything then why do we even care that the hardware changed anything?

Yes, that's my point. When switching between these clock sources, I
think that software does need to set something and so we should care.

> Setting the voltage you've read back sounds broken, if the hardware
> might randomly change things how do you know the settings we read were
> sane?  Shouldn't we know what voltage range the device requires in a
> given mode and set that - that's much more normal?

The hardware will not randomly change the voltage until the DFLL is
enabled and so you would have to do this before.

Yes, setting the frequency and voltage as defined by a given operating
mode would make sense. However, I am not sure we have those defined in
the kernel for this PLL and would have to be added.

I was thinking that during boot we could read the default voltage and
frequency set by the bootloader and use this as it should not be
changing dynamically at this point because the cpufreq driver has not
been activated yet.

Jon

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 09/12/15 14:47, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Dec 09, 2015 at 12:03:27PM +0000, Jon Hunter wrote:
>> On 08/12/15 21:52, Arnd Bergmann wrote:
> 
>>> My first attempt was to implement a helper for this function
>>> for regulator_sync_voltage, but Mark Brown explained:
> 
>>>    We don't do this for *all* regulator API functions - there's some where
>>>    using them strongly suggests that there is actually a dependency on
>>>    the regulator API.  This does seem like it might be falling into the
>>>    specialist category [...]
>>>    Looking at the code I'm pretty unclear on what the authors think the
>>>    use of _sync_voltage() is doing in the first place so it may be even
>>>    better to just remove the call.  It seems to have been included in the
>>>    first commit so there's not changelog explaining things and there's
>>>    no comment either.  I'd *expect* it to be a noop as far as I can see.
> 
>> In this sequence we are switching from the DFLL clock source (which
>> directly controls the voltage) back to a PLL (which does not control the
>> voltage directly). What we want to do is to restore the voltage back to
>> the voltage it was operating at before we switched to the DFLL clock
>> (which could have changed it).
> 
> If changes implemented by the clock driver are trashing the regulator
> settings I would expect the clock driver to be responsible for fixing
> things up rather than another driver that happens to use the clock.  I'd
> also expect some kind of internal documentation explaining what's going
> on, and possibly 

Yes, the DFLL clock driver could restore the voltage, however, that
does not guarantee that the voltage is still sufficient for the other
clock source.

>> I am not familiar with regulator_sync_voltage() but from the comment it
>> does say that it will re-apply the last voltage that was configured for
>> the regulator. So I can see what they were doing. The question I have
>> is, if the consumer has not explicitly called regulator_set_voltage()
>> then what does regulator_sync_voltage() do? I am wondering if we should
>> have been doing a regulator_get_voltage() during the probe and a
>> regulator_set_voltage() when switching back?
> 
> This *is* the sort of thing _sync() is intended for, though it's mainly
> expected to be used in cases like suspend where things have been powered
> off.  As you can see from the code it's based on the settings that
> software made, but then if nothing in software has any need to configure
> anything then why do we even care that the hardware changed anything?

Yes, that's my point. When switching between these clock sources, I
think that software does need to set something and so we should care.

> Setting the voltage you've read back sounds broken, if the hardware
> might randomly change things how do you know the settings we read were
> sane?  Shouldn't we know what voltage range the device requires in a
> given mode and set that - that's much more normal?

The hardware will not randomly change the voltage until the DFLL is
enabled and so you would have to do this before.

Yes, setting the frequency and voltage as defined by a given operating
mode would make sense. However, I am not sure we have those defined in
the kernel for this PLL and would have to be added.

I was thinking that during boot we could read the default voltage and
frequency set by the bootloader and use this as it should not be
changing dynamically at this point because the cpufreq driver has not
been activated yet.

Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 09/12/15 14:47, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Dec 09, 2015 at 12:03:27PM +0000, Jon Hunter wrote:
>> On 08/12/15 21:52, Arnd Bergmann wrote:
> 
>>> My first attempt was to implement a helper for this function
>>> for regulator_sync_voltage, but Mark Brown explained:
> 
>>>    We don't do this for *all* regulator API functions - there's some where
>>>    using them strongly suggests that there is actually a dependency on
>>>    the regulator API.  This does seem like it might be falling into the
>>>    specialist category [...]
>>>    Looking at the code I'm pretty unclear on what the authors think the
>>>    use of _sync_voltage() is doing in the first place so it may be even
>>>    better to just remove the call.  It seems to have been included in the
>>>    first commit so there's not changelog explaining things and there's
>>>    no comment either.  I'd *expect* it to be a noop as far as I can see.
> 
>> In this sequence we are switching from the DFLL clock source (which
>> directly controls the voltage) back to a PLL (which does not control the
>> voltage directly). What we want to do is to restore the voltage back to
>> the voltage it was operating at before we switched to the DFLL clock
>> (which could have changed it).
> 
> If changes implemented by the clock driver are trashing the regulator
> settings I would expect the clock driver to be responsible for fixing
> things up rather than another driver that happens to use the clock.  I'd
> also expect some kind of internal documentation explaining what's going
> on, and possibly 

Yes, the DFLL clock driver could restore the voltage, however, that
does not guarantee that the voltage is still sufficient for the other
clock source.

>> I am not familiar with regulator_sync_voltage() but from the comment it
>> does say that it will re-apply the last voltage that was configured for
>> the regulator. So I can see what they were doing. The question I have
>> is, if the consumer has not explicitly called regulator_set_voltage()
>> then what does regulator_sync_voltage() do? I am wondering if we should
>> have been doing a regulator_get_voltage() during the probe and a
>> regulator_set_voltage() when switching back?
> 
> This *is* the sort of thing _sync() is intended for, though it's mainly
> expected to be used in cases like suspend where things have been powered
> off.  As you can see from the code it's based on the settings that
> software made, but then if nothing in software has any need to configure
> anything then why do we even care that the hardware changed anything?

Yes, that's my point. When switching between these clock sources, I
think that software does need to set something and so we should care.

> Setting the voltage you've read back sounds broken, if the hardware
> might randomly change things how do you know the settings we read were
> sane?  Shouldn't we know what voltage range the device requires in a
> given mode and set that - that's much more normal?

The hardware will not randomly change the voltage until the DFLL is
enabled and so you would have to do this before.

Yes, setting the frequency and voltage as defined by a given operating
mode would make sense. However, I am not sure we have those defined in
the kernel for this PLL and would have to be added.

I was thinking that during boot we could read the default voltage and
frequency set by the bootloader and use this as it should not be
changing dynamically at this point because the cpufreq driver has not
been activated yet.

Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
> On 09/12/15 14:47, Mark Brown wrote:

> > If changes implemented by the clock driver are trashing the regulator
> > settings I would expect the clock driver to be responsible for fixing
> > things up rather than another driver that happens to use the clock.  I'd
> > also expect some kind of internal documentation explaining what's going
> > on, and possibly 

> Yes, the DFLL clock driver could restore the voltage, however, that
> does not guarantee that the voltage is still sufficient for the other
> clock source.

But the code we've got won't do that either - it'l just set the voltage
to whatever the last thing the regulator API had that might have been
within its constraints.

> > Setting the voltage you've read back sounds broken, if the hardware
> > might randomly change things how do you know the settings we read were
> > sane?  Shouldn't we know what voltage range the device requires in a
> > given mode and set that - that's much more normal?

> The hardware will not randomly change the voltage until the DFLL is
> enabled and so you would have to do this before.

I'm not clear that there's even a guarantee that the kernel will ever
have seen this configuration, consider for example what happens if
someone uses kexec?

> Yes, setting the frequency and voltage as defined by a given operating
> mode would make sense. However, I am not sure we have those defined in
> the kernel for this PLL and would have to be added.

I think given how you're describing the hardware that this will be
required in order to provide something robust (and also to get the best
power savings from the hardware).

> I was thinking that during boot we could read the default voltage and
> frequency set by the bootloader and use this as it should not be
> changing dynamically at this point because the cpufreq driver has not
> been activated yet.

I'm a bit confused here, we're talking about a change to the cpufreq
driver here aren't we?  Or alternatively why are we manipulating the
clock tree like this if we don't yet have support for the hardware?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Mark Brown]

On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
> On 09/12/15 14:47, Mark Brown wrote:

> > If changes implemented by the clock driver are trashing the regulator
> > settings I would expect the clock driver to be responsible for fixing
> > things up rather than another driver that happens to use the clock.  I'd
> > also expect some kind of internal documentation explaining what's going
> > on, and possibly 

> Yes, the DFLL clock driver could restore the voltage, however, that
> does not guarantee that the voltage is still sufficient for the other
> clock source.

But the code we've got won't do that either - it'l just set the voltage
to whatever the last thing the regulator API had that might have been
within its constraints.

> > Setting the voltage you've read back sounds broken, if the hardware
> > might randomly change things how do you know the settings we read were
> > sane?  Shouldn't we know what voltage range the device requires in a
> > given mode and set that - that's much more normal?

> The hardware will not randomly change the voltage until the DFLL is
> enabled and so you would have to do this before.

I'm not clear that there's even a guarantee that the kernel will ever
have seen this configuration, consider for example what happens if
someone uses kexec?

> Yes, setting the frequency and voltage as defined by a given operating
> mode would make sense. However, I am not sure we have those defined in
> the kernel for this PLL and would have to be added.

I think given how you're describing the hardware that this will be
required in order to provide something robust (and also to get the best
power savings from the hardware).

> I was thinking that during boot we could read the default voltage and
> frequency set by the bootloader and use this as it should not be
> changing dynamically at this point because the cpufreq driver has not
> been activated yet.

I'm a bit confused here, we're talking about a change to the cpufreq
driver here aren't we?  Or alternatively why are we manipulating the
clock tree like this if we don't yet have support for the hardware?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151209/fd5be391/attachment-0001.sig>

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 09/12/15 20:10, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
>> On 09/12/15 14:47, Mark Brown wrote:
> 
>>> If changes implemented by the clock driver are trashing the regulator
>>> settings I would expect the clock driver to be responsible for fixing
>>> things up rather than another driver that happens to use the clock.  I'd
>>> also expect some kind of internal documentation explaining what's going
>>> on, and possibly 
> 
>> Yes, the DFLL clock driver could restore the voltage, however, that
>> does not guarantee that the voltage is still sufficient for the other
>> clock source.
> 
> But the code we've got won't do that either - it'l just set the voltage
> to whatever the last thing the regulator API had that might have been
> within its constraints.

Today yes, however, I meant that we *could* change it to restore the
previous voltage before the DFLL was enabled.

>>> Setting the voltage you've read back sounds broken, if the hardware
>>> might randomly change things how do you know the settings we read were
>>> sane?  Shouldn't we know what voltage range the device requires in a
>>> given mode and set that - that's much more normal?
> 
>> The hardware will not randomly change the voltage until the DFLL is
>> enabled and so you would have to do this before.
> 
> I'm not clear that there's even a guarantee that the kernel will ever
> have seen this configuration, consider for example what happens if
> someone uses kexec?

True. I have not played around with kexec, but with something like the
DFLL, we would need to make sure that this get disabled. Looks like the
reboot notifier gets called by kexec and so I guess that could be used
to disable the DFLL and switch back to the PLL.

>> Yes, setting the frequency and voltage as defined by a given operating
>> mode would make sense. However, I am not sure we have those defined in
>> the kernel for this PLL and would have to be added.
> 
> I think given how you're describing the hardware that this will be
> required in order to provide something robust (and also to get the best
> power savings from the hardware).

Yes I agree it would be more robust. However, if you care about power
savings then you should be using the DFLL/cpufreq anyway.

>> I was thinking that during boot we could read the default voltage and
>> frequency set by the bootloader and use this as it should not be
>> changing dynamically at this point because the cpufreq driver has not
>> been activated yet.
> 
> I'm a bit confused here, we're talking about a change to the cpufreq
> driver here aren't we?  Or alternatively why are we manipulating the
> clock tree like this if we don't yet have support for the hardware?

Yes, a change to the cpu-freq driver. When probing the cpufreq driver,
we can query the clock-source, clock frequency and voltage and save
this, so that if we ever disable the DFLL/cpufreq, we can switch back to
the initial configuration.

From testing the t124 jetson and nyan-big, both of these boards have a
different configuration for the PLL at boot time, so although we could
pick a safe operating point for all t124 boards, I was thinking of just
restoring their initial configuration.

Jon

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 09/12/15 20:10, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
>> On 09/12/15 14:47, Mark Brown wrote:
> 
>>> If changes implemented by the clock driver are trashing the regulator
>>> settings I would expect the clock driver to be responsible for fixing
>>> things up rather than another driver that happens to use the clock.  I'd
>>> also expect some kind of internal documentation explaining what's going
>>> on, and possibly 
> 
>> Yes, the DFLL clock driver could restore the voltage, however, that
>> does not guarantee that the voltage is still sufficient for the other
>> clock source.
> 
> But the code we've got won't do that either - it'l just set the voltage
> to whatever the last thing the regulator API had that might have been
> within its constraints.

Today yes, however, I meant that we *could* change it to restore the
previous voltage before the DFLL was enabled.

>>> Setting the voltage you've read back sounds broken, if the hardware
>>> might randomly change things how do you know the settings we read were
>>> sane?  Shouldn't we know what voltage range the device requires in a
>>> given mode and set that - that's much more normal?
> 
>> The hardware will not randomly change the voltage until the DFLL is
>> enabled and so you would have to do this before.
> 
> I'm not clear that there's even a guarantee that the kernel will ever
> have seen this configuration, consider for example what happens if
> someone uses kexec?

True. I have not played around with kexec, but with something like the
DFLL, we would need to make sure that this get disabled. Looks like the
reboot notifier gets called by kexec and so I guess that could be used
to disable the DFLL and switch back to the PLL.

>> Yes, setting the frequency and voltage as defined by a given operating
>> mode would make sense. However, I am not sure we have those defined in
>> the kernel for this PLL and would have to be added.
> 
> I think given how you're describing the hardware that this will be
> required in order to provide something robust (and also to get the best
> power savings from the hardware).

Yes I agree it would be more robust. However, if you care about power
savings then you should be using the DFLL/cpufreq anyway.

>> I was thinking that during boot we could read the default voltage and
>> frequency set by the bootloader and use this as it should not be
>> changing dynamically at this point because the cpufreq driver has not
>> been activated yet.
> 
> I'm a bit confused here, we're talking about a change to the cpufreq
> driver here aren't we?  Or alternatively why are we manipulating the
> clock tree like this if we don't yet have support for the hardware?

Yes, a change to the cpu-freq driver. When probing the cpufreq driver,
we can query the clock-source, clock frequency and voltage and save
this, so that if we ever disable the DFLL/cpufreq, we can switch back to
the initial configuration.

>From testing the t124 jetson and nyan-big, both of these boards have a
different configuration for the PLL at boot time, so although we could
pick a safe operating point for all t124 boards, I was thinking of just
restoring their initial configuration.

Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 09/12/15 20:10, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
>> On 09/12/15 14:47, Mark Brown wrote:
> 
>>> If changes implemented by the clock driver are trashing the regulator
>>> settings I would expect the clock driver to be responsible for fixing
>>> things up rather than another driver that happens to use the clock.  I'd
>>> also expect some kind of internal documentation explaining what's going
>>> on, and possibly 
> 
>> Yes, the DFLL clock driver could restore the voltage, however, that
>> does not guarantee that the voltage is still sufficient for the other
>> clock source.
> 
> But the code we've got won't do that either - it'l just set the voltage
> to whatever the last thing the regulator API had that might have been
> within its constraints.

Today yes, however, I meant that we *could* change it to restore the
previous voltage before the DFLL was enabled.

>>> Setting the voltage you've read back sounds broken, if the hardware
>>> might randomly change things how do you know the settings we read were
>>> sane?  Shouldn't we know what voltage range the device requires in a
>>> given mode and set that - that's much more normal?
> 
>> The hardware will not randomly change the voltage until the DFLL is
>> enabled and so you would have to do this before.
> 
> I'm not clear that there's even a guarantee that the kernel will ever
> have seen this configuration, consider for example what happens if
> someone uses kexec?

True. I have not played around with kexec, but with something like the
DFLL, we would need to make sure that this get disabled. Looks like the
reboot notifier gets called by kexec and so I guess that could be used
to disable the DFLL and switch back to the PLL.

>> Yes, setting the frequency and voltage as defined by a given operating
>> mode would make sense. However, I am not sure we have those defined in
>> the kernel for this PLL and would have to be added.
> 
> I think given how you're describing the hardware that this will be
> required in order to provide something robust (and also to get the best
> power savings from the hardware).

Yes I agree it would be more robust. However, if you care about power
savings then you should be using the DFLL/cpufreq anyway.

>> I was thinking that during boot we could read the default voltage and
>> frequency set by the bootloader and use this as it should not be
>> changing dynamically at this point because the cpufreq driver has not
>> been activated yet.
> 
> I'm a bit confused here, we're talking about a change to the cpufreq
> driver here aren't we?  Or alternatively why are we manipulating the
> clock tree like this if we don't yet have support for the hardware?

Yes, a change to the cpu-freq driver. When probing the cpufreq driver,
we can query the clock-source, clock frequency and voltage and save
this, so that if we ever disable the DFLL/cpufreq, we can switch back to
the initial configuration.

>From testing the t124 jetson and nyan-big, both of these boards have a
different configuration for the PLL at boot time, so although we could
pick a safe operating point for all t124 boards, I was thinking of just
restoring their initial configuration.

Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 09/12/15 20:10, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
>> On 09/12/15 14:47, Mark Brown wrote:
> 
>>> If changes implemented by the clock driver are trashing the regulator
>>> settings I would expect the clock driver to be responsible for fixing
>>> things up rather than another driver that happens to use the clock.  I'd
>>> also expect some kind of internal documentation explaining what's going
>>> on, and possibly 
> 
>> Yes, the DFLL clock driver could restore the voltage, however, that
>> does not guarantee that the voltage is still sufficient for the other
>> clock source.
> 
> But the code we've got won't do that either - it'l just set the voltage
> to whatever the last thing the regulator API had that might have been
> within its constraints.

Today yes, however, I meant that we *could* change it to restore the
previous voltage before the DFLL was enabled.

>>> Setting the voltage you've read back sounds broken, if the hardware
>>> might randomly change things how do you know the settings we read were
>>> sane?  Shouldn't we know what voltage range the device requires in a
>>> given mode and set that - that's much more normal?
> 
>> The hardware will not randomly change the voltage until the DFLL is
>> enabled and so you would have to do this before.
> 
> I'm not clear that there's even a guarantee that the kernel will ever
> have seen this configuration, consider for example what happens if
> someone uses kexec?

True. I have not played around with kexec, but with something like the
DFLL, we would need to make sure that this get disabled. Looks like the
reboot notifier gets called by kexec and so I guess that could be used
to disable the DFLL and switch back to the PLL.

>> Yes, setting the frequency and voltage as defined by a given operating
>> mode would make sense. However, I am not sure we have those defined in
>> the kernel for this PLL and would have to be added.
> 
> I think given how you're describing the hardware that this will be
> required in order to provide something robust (and also to get the best
> power savings from the hardware).

Yes I agree it would be more robust. However, if you care about power
savings then you should be using the DFLL/cpufreq anyway.

>> I was thinking that during boot we could read the default voltage and
>> frequency set by the bootloader and use this as it should not be
>> changing dynamically at this point because the cpufreq driver has not
>> been activated yet.
> 
> I'm a bit confused here, we're talking about a change to the cpufreq
> driver here aren't we?  Or alternatively why are we manipulating the
> clock tree like this if we don't yet have support for the hardware?

Yes, a change to the cpu-freq driver. When probing the cpufreq driver,
we can query the clock-source, clock frequency and voltage and save
this, so that if we ever disable the DFLL/cpufreq, we can switch back to
the initial configuration.

>From testing the t124 jetson and nyan-big, both of these boards have a
different configuration for the PLL at boot time, so although we could
pick a safe operating point for all t124 boards, I was thinking of just
restoring their initial configuration.

Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On Thu, Dec 10, 2015 at 10:07:54AM +0000, Jon Hunter wrote:
> On 09/12/15 20:10, Mark Brown wrote:
> > On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:

> >> Yes, setting the frequency and voltage as defined by a given operating
> >> mode would make sense. However, I am not sure we have those defined in
> >> the kernel for this PLL and would have to be added.

> > I think given how you're describing the hardware that this will be
> > required in order to provide something robust (and also to get the best
> > power savings from the hardware).

> Yes I agree it would be more robust. However, if you care about power
> savings then you should be using the DFLL/cpufreq anyway.

Without knowing anything about the hardware this is all a bit confusing
I'm afraid.  What is "DFLL/cpufreq" as opposed to "the PLL"?

> From testing the t124 jetson and nyan-big, both of these boards have a
> different configuration for the PLL at boot time, so although we could
> pick a safe operating point for all t124 boards, I was thinking of just
> restoring their initial configuration.

This seems more complex, and also makes the idea of relying on the
initial configuration seem slightly concerning - other software seems to
be already changing the configuration before we get to the kernel so if
we don't fully understand the configuration we're doing we seem likely
to find some configuration where we miss things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Mark Brown]

On Thu, Dec 10, 2015 at 10:07:54AM +0000, Jon Hunter wrote:
> On 09/12/15 20:10, Mark Brown wrote:
> > On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:

> >> Yes, setting the frequency and voltage as defined by a given operating
> >> mode would make sense. However, I am not sure we have those defined in
> >> the kernel for this PLL and would have to be added.

> > I think given how you're describing the hardware that this will be
> > required in order to provide something robust (and also to get the best
> > power savings from the hardware).

> Yes I agree it would be more robust. However, if you care about power
> savings then you should be using the DFLL/cpufreq anyway.

Without knowing anything about the hardware this is all a bit confusing
I'm afraid.  What is "DFLL/cpufreq" as opposed to "the PLL"?

> From testing the t124 jetson and nyan-big, both of these boards have a
> different configuration for the PLL at boot time, so although we could
> pick a safe operating point for all t124 boards, I was thinking of just
> restoring their initial configuration.

This seems more complex, and also makes the idea of relying on the
initial configuration seem slightly concerning - other software seems to
be already changing the configuration before we get to the kernel so if
we don't fully understand the configuration we're doing we seem likely
to find some configuration where we miss things.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151210/11858660/attachment-0001.sig>

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

On Thu, Dec 10, 2015 at 10:07:54AM +0000, Jon Hunter wrote:
> On 09/12/15 20:10, Mark Brown wrote:
> > On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:

> >> Yes, setting the frequency and voltage as defined by a given operating
> >> mode would make sense. However, I am not sure we have those defined in
> >> the kernel for this PLL and would have to be added.

> > I think given how you're describing the hardware that this will be
> > required in order to provide something robust (and also to get the best
> > power savings from the hardware).

> Yes I agree it would be more robust. However, if you care about power
> savings then you should be using the DFLL/cpufreq anyway.

Without knowing anything about the hardware this is all a bit confusing
I'm afraid.  What is "DFLL/cpufreq" as opposed to "the PLL"?

> From testing the t124 jetson and nyan-big, both of these boards have a
> different configuration for the PLL at boot time, so although we could
> pick a safe operating point for all t124 boards, I was thinking of just
> restoring their initial configuration.

This seems more complex, and also makes the idea of relying on the
initial configuration seem slightly concerning - other software seems to
be already changing the configuration before we get to the kernel so if
we don't fully understand the configuration we're doing we seem likely
to find some configuration where we miss things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 10/12/15 11:35, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Dec 10, 2015 at 10:07:54AM +0000, Jon Hunter wrote:
>> On 09/12/15 20:10, Mark Brown wrote:
>>> On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
> 
>>>> Yes, setting the frequency and voltage as defined by a given operating
>>>> mode would make sense. However, I am not sure we have those defined in
>>>> the kernel for this PLL and would have to be added.
> 
>>> I think given how you're describing the hardware that this will be
>>> required in order to provide something robust (and also to get the best
>>> power savings from the hardware).
> 
>> Yes I agree it would be more robust. However, if you care about power
>> savings then you should be using the DFLL/cpufreq anyway.
> 
> Without knowing anything about the hardware this is all a bit confusing
> I'm afraid.  What is "DFLL/cpufreq" as opposed to "the PLL"?

No problem.

The two main clocks sources (there are others) used for the CPU complex
are the PLLX and the DFLL. The PLLX is a normal PLL and you can
configure to run at various frequencies. However, this PLL requires that
you/software set the voltage appropriately. The DFLL has the ability to
control the voltage itself per the frequency requested (no software
management of the voltage required).

There is no reason why you could not use the PLLX in conjunction with
cpufreq, but for T124 we use the DFLL.

Typically, T124 will boot using the PLLX and then if cpufreq is enabled,
the DFLL will be enabled and we will switch to this clock.

We would only ever switch back to the PLLX if cpufreq is removed and
hence the DFLL is disabled. And this is where this patch comes in and I
am wondering if this has ever been tested.

>> From testing the t124 jetson and nyan-big, both of these boards have a
>> different configuration for the PLL at boot time, so although we could
>> pick a safe operating point for all t124 boards, I was thinking of just
>> restoring their initial configuration.
> 
> This seems more complex, and also makes the idea of relying on the
> initial configuration seem slightly concerning - other software seems to
> be already changing the configuration before we get to the kernel so if
> we don't fully understand the configuration we're doing we seem likely
> to find some configuration where we miss things.

I don't think so. At probe time for cpufreq, we know the clock source
for the CPU complex and we can query the voltage and frequency. You may
say what happens if you are already running from the DFLL that happened
to be setup by the bootloader? It is probably not likely as there is
more software setup involved before you can enable the DFLL. However, it
could be handled.

By the way, I am not saying that we should do this, but if we don't we
need to define a safe/default V-F for all devices to operate at. I guess
that it could also be specified in the DTB and if not we use a default.

Jon

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 10/12/15 11:35, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Dec 10, 2015 at 10:07:54AM +0000, Jon Hunter wrote:
>> On 09/12/15 20:10, Mark Brown wrote:
>>> On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
> 
>>>> Yes, setting the frequency and voltage as defined by a given operating
>>>> mode would make sense. However, I am not sure we have those defined in
>>>> the kernel for this PLL and would have to be added.
> 
>>> I think given how you're describing the hardware that this will be
>>> required in order to provide something robust (and also to get the best
>>> power savings from the hardware).
> 
>> Yes I agree it would be more robust. However, if you care about power
>> savings then you should be using the DFLL/cpufreq anyway.
> 
> Without knowing anything about the hardware this is all a bit confusing
> I'm afraid.  What is "DFLL/cpufreq" as opposed to "the PLL"?

No problem.

The two main clocks sources (there are others) used for the CPU complex
are the PLLX and the DFLL. The PLLX is a normal PLL and you can
configure to run at various frequencies. However, this PLL requires that
you/software set the voltage appropriately. The DFLL has the ability to
control the voltage itself per the frequency requested (no software
management of the voltage required).

There is no reason why you could not use the PLLX in conjunction with
cpufreq, but for T124 we use the DFLL.

Typically, T124 will boot using the PLLX and then if cpufreq is enabled,
the DFLL will be enabled and we will switch to this clock.

We would only ever switch back to the PLLX if cpufreq is removed and
hence the DFLL is disabled. And this is where this patch comes in and I
am wondering if this has ever been tested.

>> From testing the t124 jetson and nyan-big, both of these boards have a
>> different configuration for the PLL at boot time, so although we could
>> pick a safe operating point for all t124 boards, I was thinking of just
>> restoring their initial configuration.
> 
> This seems more complex, and also makes the idea of relying on the
> initial configuration seem slightly concerning - other software seems to
> be already changing the configuration before we get to the kernel so if
> we don't fully understand the configuration we're doing we seem likely
> to find some configuration where we miss things.

I don't think so. At probe time for cpufreq, we know the clock source
for the CPU complex and we can query the voltage and frequency. You may
say what happens if you are already running from the DFLL that happened
to be setup by the bootloader? It is probably not likely as there is
more software setup involved before you can enable the DFLL. However, it
could be handled.

By the way, I am not saying that we should do this, but if we don't we
need to define a safe/default V-F for all devices to operate at. I guess
that it could also be specified in the DTB and if not we use a default.

Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Jon Hunter]

On 10/12/15 11:35, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Dec 10, 2015 at 10:07:54AM +0000, Jon Hunter wrote:
>> On 09/12/15 20:10, Mark Brown wrote:
>>> On Wed, Dec 09, 2015 at 05:33:33PM +0000, Jon Hunter wrote:
> 
>>>> Yes, setting the frequency and voltage as defined by a given operating
>>>> mode would make sense. However, I am not sure we have those defined in
>>>> the kernel for this PLL and would have to be added.
> 
>>> I think given how you're describing the hardware that this will be
>>> required in order to provide something robust (and also to get the best
>>> power savings from the hardware).
> 
>> Yes I agree it would be more robust. However, if you care about power
>> savings then you should be using the DFLL/cpufreq anyway.
> 
> Without knowing anything about the hardware this is all a bit confusing
> I'm afraid.  What is "DFLL/cpufreq" as opposed to "the PLL"?

No problem.

The two main clocks sources (there are others) used for the CPU complex
are the PLLX and the DFLL. The PLLX is a normal PLL and you can
configure to run at various frequencies. However, this PLL requires that
you/software set the voltage appropriately. The DFLL has the ability to
control the voltage itself per the frequency requested (no software
management of the voltage required).

There is no reason why you could not use the PLLX in conjunction with
cpufreq, but for T124 we use the DFLL.

Typically, T124 will boot using the PLLX and then if cpufreq is enabled,
the DFLL will be enabled and we will switch to this clock.

We would only ever switch back to the PLLX if cpufreq is removed and
hence the DFLL is disabled. And this is where this patch comes in and I
am wondering if this has ever been tested.

>> From testing the t124 jetson and nyan-big, both of these boards have a
>> different configuration for the PLL at boot time, so although we could
>> pick a safe operating point for all t124 boards, I was thinking of just
>> restoring their initial configuration.
> 
> This seems more complex, and also makes the idea of relying on the
> initial configuration seem slightly concerning - other software seems to
> be already changing the configuration before we get to the kernel so if
> we don't fully understand the configuration we're doing we seem likely
> to find some configuration where we miss things.

I don't think so. At probe time for cpufreq, we know the clock source
for the CPU complex and we can query the voltage and frequency. You may
say what happens if you are already running from the DFLL that happened
to be setup by the bootloader? It is probably not likely as there is
more software setup involved before you can enable the DFLL. However, it
could be handled.

By the way, I am not saying that we should do this, but if we don't we
need to define a safe/default V-F for all devices to operate at. I guess
that it could also be specified in the DTB and if not we use a default.

Jon

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Rafael J. Wysocki]

On Tuesday, December 08, 2015 10:52:45 PM Arnd Bergmann wrote:
> This driver is the only one that calls regulator_sync_voltage(), but it
> can currently be built with CONFIG_REGULATOR disabled, producing
> this build error:
> 
> drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
> drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
>   regulator_sync_voltage(priv->vdd_cpu_reg);
> 
> My first attempt was to implement a helper for this function
> for regulator_sync_voltage, but Mark Brown explained:
> 
>    We don't do this for *all* regulator API functions - there's some where
>    using them strongly suggests that there is actually a dependency on
>    the regulator API.  This does seem like it might be falling into the
>    specialist category [...]
>    Looking at the code I'm pretty unclear on what the authors think the
>    use of _sync_voltage() is doing in the first place so it may be even
>    better to just remove the call.  It seems to have been included in the
>    first commit so there's not changelog explaining things and there's
>    no comment either.  I'd *expect* it to be a noop as far as I can see.
> 
> This adds the dependency to make the driver always build successfully
> or not be enabled at all. Alternatively, we could investigate if the
> driver should stop calling regulator_sync_voltage instead.
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

Applied, thanks!

Rafael

[PATCH] cpufreq: tegra: add regulator dependency for T124

[Rafael J. Wysocki]

On Tuesday, December 08, 2015 10:52:45 PM Arnd Bergmann wrote:
> This driver is the only one that calls regulator_sync_voltage(), but it
> can currently be built with CONFIG_REGULATOR disabled, producing
> this build error:
> 
> drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
> drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
>   regulator_sync_voltage(priv->vdd_cpu_reg);
> 
> My first attempt was to implement a helper for this function
> for regulator_sync_voltage, but Mark Brown explained:
> 
>    We don't do this for *all* regulator API functions - there's some where
>    using them strongly suggests that there is actually a dependency on
>    the regulator API.  This does seem like it might be falling into the
>    specialist category [...]
>    Looking at the code I'm pretty unclear on what the authors think the
>    use of _sync_voltage() is doing in the first place so it may be even
>    better to just remove the call.  It seems to have been included in the
>    first commit so there's not changelog explaining things and there's
>    no comment either.  I'd *expect* it to be a noop as far as I can see.
> 
> This adds the dependency to make the driver always build successfully
> or not be enabled at all. Alternatively, we could investigate if the
> driver should stop calling regulator_sync_voltage instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Applied, thanks!

Rafael

Re: [PATCH] cpufreq: tegra: add regulator dependency for T124

[Rafael J. Wysocki]

On Tuesday, December 08, 2015 10:52:45 PM Arnd Bergmann wrote:
> This driver is the only one that calls regulator_sync_voltage(), but it
> can currently be built with CONFIG_REGULATOR disabled, producing
> this build error:
> 
> drivers/cpufreq/tegra124-cpufreq.c: In function 'tegra124_cpu_switch_to_pllx':
> drivers/cpufreq/tegra124-cpufreq.c:68:2: error: implicit declaration of function 'regulator_sync_voltage' [-Werror=implicit-function-declaration]
>   regulator_sync_voltage(priv->vdd_cpu_reg);
> 
> My first attempt was to implement a helper for this function
> for regulator_sync_voltage, but Mark Brown explained:
> 
>    We don't do this for *all* regulator API functions - there's some where
>    using them strongly suggests that there is actually a dependency on
>    the regulator API.  This does seem like it might be falling into the
>    specialist category [...]
>    Looking at the code I'm pretty unclear on what the authors think the
>    use of _sync_voltage() is doing in the first place so it may be even
>    better to just remove the call.  It seems to have been included in the
>    first commit so there's not changelog explaining things and there's
>    no comment either.  I'd *expect* it to be a noop as far as I can see.
> 
> This adds the dependency to make the driver always build successfully
> or not be enabled at all. Alternatively, we could investigate if the
> driver should stop calling regulator_sync_voltage instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Applied, thanks!

Rafael