mbox series

[v3,00/11] Tegra210 DFLL implementation

Message ID 1517934852-23255-1-git-send-email-pdeschrijver@nvidia.com
Headers show
Series Tegra210 DFLL implementation | expand

Message

Peter De Schrijver Feb. 6, 2018, 4:34 p.m. UTC
This series introduces support for the DFLL as a CPU clock source
on Tegra210. As Jetson TX2 uses a PWM controlled regulator IC which
is driven directly by the DFLLs PWM output, we also introduce support
for PWM regulators next to I2C controlled regulators. The DFLL output
frequency is directly controlled by the regulator voltage. The registers                                                                                       for controlling the PWM are part of the DFLL IP block, so there's no
separate linux regulator object involved because the regulator IC only                                                                                         supplies the rail powering the DFLL and the CPUs. It doesn't have any
other controls.

Changes since v2:
* added DT updates for Tegra210
* updated dfll DT binding documentation
* split changes to i2c support into its own patch
* retrieve regulator parameters from framework rather than from CVB table
* bug fixes

Changes since v1:
* improved commit messages
* some style cleanups                                                                                                                                           
Laxman Dewangan (1):
  regulator: core: add API to get voltage constraints

Peter De Schrijver (10):
  clk: tegra: retrieve regulator info from framework
  clk: tegra: dfll registration for multiple SoCs
  clk: tegra: add CVB tables for Tegra210 CPU DFLL
  clk: tegra: prepare dfll driver for PWM regulator
  clk: tegra: dfll: support PWM regulator control
  dt-bindings: tegra: Update DFLL binding for PWM regulator
  clk: tegra: build clk-dfll.c for Tegra124 and Tegra210
  cpufreq: tegra124-cpufreq: extend to support Tegra210
  arm64: dts: tegra: Add Tegra210 DFLL definition
  arm64: dts: nvidia: Tegra210 CPU clock definition

 .../bindings/clock/nvidia,tegra124-dfll.txt        |  76 ++-
 arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi     |  18 +
 arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi     |  12 +
 arch/arm64/boot/dts/nvidia/tegra210.dtsi           |  26 +
 drivers/clk/tegra/Kconfig                          |   5 +
 drivers/clk/tegra/Makefile                         |   2 +-
 drivers/clk/tegra/clk-dfll.c                       | 462 +++++++++++++++---
 drivers/clk/tegra/clk-dfll.h                       |   2 +
 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c         | 526 ++++++++++++++++++++-
 drivers/clk/tegra/cvb.c                            |  16 +-
 drivers/clk/tegra/cvb.h                            |   7 +-
 drivers/cpufreq/tegra124-cpufreq.c                 |  15 +-
 drivers/regulator/core.c                           |  31 ++
 include/linux/regulator/consumer.h                 |   2 +
 14 files changed, 1106 insertions(+), 94 deletions(-)

Comments

Mark Brown Feb. 6, 2018, 4:35 p.m. UTC | #1
On Tue, Feb 06, 2018 at 06:34:02PM +0200, Peter De Schrijver wrote:
> From: Laxman Dewangan <ldewangan@nvidia.com>
> 
> Add API to get min/max rail voltage configured from platform for
> given rails.

because...
Peter De Schrijver Feb. 7, 2018, 8:47 a.m. UTC | #2
On Tue, Feb 06, 2018 at 04:35:44PM +0000, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Feb 06, 2018 at 06:34:02PM +0200, Peter De Schrijver wrote:
> > From: Laxman Dewangan <ldewangan@nvidia.com>
> > 
> > Add API to get min/max rail voltage configured from platform for
> > given rails.
> 
> because...

of the next patch where this is used to retrieve the minimum rail voltage.

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 7, 2018, 10:43 a.m. UTC | #3
On Wed, Feb 07, 2018 at 10:47:44AM +0200, Peter De Schrijver wrote:
> On Tue, Feb 06, 2018 at 04:35:44PM +0000, Mark Brown wrote:
> > On Tue, Feb 06, 2018 at 06:34:02PM +0200, Peter De Schrijver wrote:

> > > Add API to get min/max rail voltage configured from platform for
> > > given rails.

> > because...

> of the next patch where this is used to retrieve the minimum rail voltage.

And that in turn is needed for...?
Peter De Schrijver Feb. 7, 2018, 12:37 p.m. UTC | #4
On Wed, Feb 07, 2018 at 10:43:51AM +0000, Mark Brown wrote:
> On Wed, Feb 07, 2018 at 10:47:44AM +0200, Peter De Schrijver wrote:
> > On Tue, Feb 06, 2018 at 04:35:44PM +0000, Mark Brown wrote:
> > > On Tue, Feb 06, 2018 at 06:34:02PM +0200, Peter De Schrijver wrote:
> 
> > > > Add API to get min/max rail voltage configured from platform for
> > > > given rails.
> 
> > > because...
> 
> > of the next patch where this is used to retrieve the minimum rail voltage.
> 
> And that in turn is needed for...?

To calculate the required voltage for each frequency.

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 7, 2018, 2:18 p.m. UTC | #5
On Wed, Feb 07, 2018 at 02:37:51PM +0200, Peter De Schrijver wrote:
> On Wed, Feb 07, 2018 at 10:43:51AM +0000, Mark Brown wrote:
> > On Wed, Feb 07, 2018 at 10:47:44AM +0200, Peter De Schrijver wrote:
> > > On Tue, Feb 06, 2018 at 04:35:44PM +0000, Mark Brown wrote:
> > > > On Tue, Feb 06, 2018 at 06:34:02PM +0200, Peter De Schrijver wrote:

> > > > > Add API to get min/max rail voltage configured from platform for
> > > > > given rails.

> > > > because...

> > > of the next patch where this is used to retrieve the minimum rail voltage.

> > And that in turn is needed for...?

> To calculate the required voltage for each frequency.

You're going to have to provide a much better explanation of what this
is doing - right now it seems like an abuse of constraints.  Client
drivers can already determine if a particular voltage they want to set
is available via regulator_list_voltage() and so on, that's what
constraints are there to set.  It sounds like you're trying to use them
for something else but you're really not explaining your use case
clearly.
Peter De Schrijver Feb. 7, 2018, 2:32 p.m. UTC | #6
On Wed, Feb 07, 2018 at 02:18:46PM +0000, Mark Brown wrote:
> On Wed, Feb 07, 2018 at 02:37:51PM +0200, Peter De Schrijver wrote:
> > On Wed, Feb 07, 2018 at 10:43:51AM +0000, Mark Brown wrote:
> > > On Wed, Feb 07, 2018 at 10:47:44AM +0200, Peter De Schrijver wrote:
> > > > On Tue, Feb 06, 2018 at 04:35:44PM +0000, Mark Brown wrote:
> > > > > On Tue, Feb 06, 2018 at 06:34:02PM +0200, Peter De Schrijver wrote:
> 
> > > > > > Add API to get min/max rail voltage configured from platform for
> > > > > > given rails.
> 
> > > > > because...
> 
> > > > of the next patch where this is used to retrieve the minimum rail voltage.
> 
> > > And that in turn is needed for...?
> 
> > To calculate the required voltage for each frequency.
> 
> You're going to have to provide a much better explanation of what this
> is doing - right now it seems like an abuse of constraints.  Client
> drivers can already determine if a particular voltage they want to set
> is available via regulator_list_voltage() and so on, that's what
> constraints are there to set.  It sounds like you're trying to use them
> for something else but you're really not explaining your use case
> clearly.

There is no way to query what voltage I will actually get for a given input
voltage. If you read drivers/clk/tegra/cvb. (you did do that right?), you
will see that there is a minimum and maximum voltage defined by
charaterization which needs to be capped to the regulator generated voltages
for those points.

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 7, 2018, 3:01 p.m. UTC | #7
On Wed, Feb 07, 2018 at 04:32:13PM +0200, Peter De Schrijver wrote:
> On Wed, Feb 07, 2018 at 02:18:46PM +0000, Mark Brown wrote:

> > You're going to have to provide a much better explanation of what this
> > is doing - right now it seems like an abuse of constraints.  Client
> > drivers can already determine if a particular voltage they want to set
> > is available via regulator_list_voltage() and so on, that's what
> > constraints are there to set.  It sounds like you're trying to use them
> > for something else but you're really not explaining your use case
> > clearly.

> There is no way to query what voltage I will actually get for a given input

I looked at patch 2.  It looked like an abuse of what constraints do,
and had zero explanation of why it was doing what it was doing.  In any
case we need the regulator code and changelog to be clear about what the
interface is for and why it should be used, that's not happening here.

> voltage. If you read drivers/clk/tegra/cvb. (you did do that right?), you
> will see that there is a minimum and maximum voltage defined by
> charaterization which needs to be capped to the regulator generated voltages
> for those points.

I can't really tell what you're saying here.  If the driver needs to
know if it can set the a given voltage there's already an API for doing
that as I said.  If you're trying to convey this minimum and maximum
voltage via the constraints that sounds like an abuse of the constraints.
Peter De Schrijver Feb. 7, 2018, 3:20 p.m. UTC | #8
On Wed, Feb 07, 2018 at 03:01:55PM +0000, Mark Brown wrote:
> On Wed, Feb 07, 2018 at 04:32:13PM +0200, Peter De Schrijver wrote:
> > On Wed, Feb 07, 2018 at 02:18:46PM +0000, Mark Brown wrote:
> 
> > > You're going to have to provide a much better explanation of what this
> > > is doing - right now it seems like an abuse of constraints.  Client
> > > drivers can already determine if a particular voltage they want to set
> > > is available via regulator_list_voltage() and so on, that's what
> > > constraints are there to set.  It sounds like you're trying to use them
> > > for something else but you're really not explaining your use case
> > > clearly.
> 
> > There is no way to query what voltage I will actually get for a given input
> 
> I looked at patch 2.  It looked like an abuse of what constraints do,
> and had zero explanation of why it was doing what it was doing.  In any
> case we need the regulator code and changelog to be clear about what the
> interface is for and why it should be used, that's not happening here.
> 
> > voltage. If you read drivers/clk/tegra/cvb. (you did do that right?), you
> > will see that there is a minimum and maximum voltage defined by
> > charaterization which needs to be capped to the regulator generated voltages
> > for those points.
> 
> I can't really tell what you're saying here.  If the driver needs to
> know if it can set the a given voltage there's already an API for doing
> that as I said.  If you're trying to convey this minimum and maximum
> voltage via the constraints that sounds like an abuse of the constraints.


No, what I want is the voltage which the regulator will output for a given
regulator_set_voltage request taking constraints, regulator step etc into
account.

Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 7, 2018, 3:37 p.m. UTC | #9
On Wed, Feb 07, 2018 at 05:20:45PM +0200, Peter De Schrijver wrote:
> On Wed, Feb 07, 2018 at 03:01:55PM +0000, Mark Brown wrote:

> > I can't really tell what you're saying here.  If the driver needs to
> > know if it can set the a given voltage there's already an API for doing
> > that as I said.  If you're trying to convey this minimum and maximum
> > voltage via the constraints that sounds like an abuse of the constraints.

> No, what I want is the voltage which the regulator will output for a given
> regulator_set_voltage request taking constraints, regulator step etc into
> account.

Knowing the range of the constaints is going to tell you nothing useful
about that, it has zero information on steps or anything.  The way to
find out what voltages can be set is to enumerate the voltages that can
be set through the existing API and then if you want to set a specific
voltage that you've confirmed is available you can set exactly that
voltage via the normal voltage setting interface, no need to provide a
range.
Laxman Dewangan Feb. 8, 2018, 10:04 a.m. UTC | #10
On Wednesday 07 February 2018 09:07 PM, Mark Brown wrote:
> On Wed, Feb 07, 2018 at 05:20:45PM +0200, Peter De Schrijver wrote:
>> On Wed, Feb 07, 2018 at 03:01:55PM +0000, Mark Brown wrote:
>>> I can't really tell what you're saying here.  If the driver needs to
>>> know if it can set the a given voltage there's already an API for doing
>>> that as I said.  If you're trying to convey this minimum and maximum
>>> voltage via the constraints that sounds like an abuse of the constraints.
>> No, what I want is the voltage which the regulator will output for a given
>> regulator_set_voltage request taking constraints, regulator step etc into
>> account.
> Knowing the range of the constaints is going to tell you nothing useful
> about that, it has zero information on steps or anything.  The way to
> find out what voltages can be set is to enumerate the voltages that can
> be set through the existing API and then if you want to set a specific
> voltage that you've confirmed is available you can set exactly that
> voltage via the normal voltage setting interface, no need to provide a
> range.

Hi,
I added this API in downstream for the purpose of finding whether rail 
output can be changed or not(fixed) based on constraints.
If min and max is same then it can not change. I used this information 
to set the pad voltage of SOC automatically.

I dont know other usage when I added this API. May be Peter is needed here.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 8, 2018, 2:58 p.m. UTC | #11
On Thu, Feb 08, 2018 at 03:34:56PM +0530, Laxman Dewangan wrote:

> I added this API in downstream for the purpose of finding whether rail
> output can be changed or not(fixed) based on constraints.
> If min and max is same then it can not change. I used this information to
> set the pad voltage of SOC automatically.

That's a reasonable thing to want to know.  The way the API wants you to
ask that question at the minute is to check if you can set the voltages
you might want to set and then if there's only one possible voltage you
can conclude that change is not possible.  I appreciate that that's a
bit of a long way round to do it, the thinking was to try to keep the
client drivers more robust in cases you can change the voltage but not
to all the values the driver wants.  I'd consider relaxing that and
having a direct API to ask about variability (just as a boolean) since
it does occur to me that this can be a useful speedup during init but I
need to think.

> I dont know other usage when I added this API. May be Peter is needed here.