mbox series

[v1,00/17] NVIDIA Tegra20 CPUFreq driver major update

Message ID 20191015211618.20758-1-digetx@gmail.com
Headers show
Series NVIDIA Tegra20 CPUFreq driver major update | expand

Message

Dmitry Osipenko Oct. 15, 2019, 9:16 p.m. UTC
Hello,

This series moves intermediate-clk handling from tegra20-cpufreq into
tegra-clk driver, this allows us to switch to generic cpufreq-dt driver
which brings voltage scaling, per-hardware OPPs and Tegra30 support out
of the box. All boards need to adopt CPU OPPs in their device-trees in
order to get cpufreq support. This series adds OPPs only to selective
boards because there is assumption in a current device-trees that CPU
voltage is set for 1GHz freq and this won't work for those CPUs that
can go over 1GHz and thus require voltage regulators to be set up for
voltage scaling support (CC'ed Marcel for Toradex boards). We could
probably add delete-node for OPPs over 1GHz if there are not actively
maintained boards.

NOTE: the voltage scaling functionality depends on a reviewed and yet
unapplied series [0].

[0] https://lkml.org/lkml/2019/7/25/892

Dmitry Osipenko (17):
  clk: tegra: Add custom CCLK implementation
  clk: tegra: pll: Add pre/post rate-change hooks
  clk: tegra: cclk: Add helpers for handling PLLX rate changes
  clk: tegra20: Support custom CCLK implementation
  clk: tegra30: Support custom CCLK implementation
  dt-bindings: cpufreq: Add binding for NVIDIA Tegra20/30
  cpufreq: tegra20: Use generic cpufreq-dt driver (Tegra30 supported
    now)
  ARM: tegra: Remove tegra20-cpufreq platform device creation
  ARM: dts: tegra20: Add CPU clock
  ARM: dts: tegra30: Add CPU clock
  ARM: dts: tegra20: Add CPU Operating Performance Points
  ARM: dts: tegra30: Add CPU Operating Performance Points
  ARM: dts: tegra20: paz00: Set up voltage regulators for DVFS
  ARM: dts: tegra20: paz00: Add CPU Operating Performance Points
  ARM: dts: tegra20: trimslice: Add CPU Operating Performance Points
  ARM: dts: tegra30: beaver: Set up voltage regulators for DVFS
  ARM: dts: tegra30: beaver: Add CPU Operating Performance Points

 .../cpufreq/nvidia,tegra20-cpufreq.txt        |   56 +
 .../boot/dts/tegra20-cpu-opp-microvolt.dtsi   |  201 +++
 arch/arm/boot/dts/tegra20-cpu-opp.dtsi        |  302 +++++
 arch/arm/boot/dts/tegra20-paz00.dts           |   41 +-
 arch/arm/boot/dts/tegra20-trimslice.dts       |   11 +
 arch/arm/boot/dts/tegra20.dtsi                |    2 +
 arch/arm/boot/dts/tegra30-beaver.dts          |   40 +-
 .../boot/dts/tegra30-cpu-opp-microvolt.dtsi   |  801 +++++++++++
 arch/arm/boot/dts/tegra30-cpu-opp.dtsi        | 1202 +++++++++++++++++
 arch/arm/boot/dts/tegra30.dtsi                |    4 +
 arch/arm/mach-tegra/tegra.c                   |    4 -
 drivers/clk/tegra/Makefile                    |    1 +
 drivers/clk/tegra/clk-pll.c                   |   12 +-
 drivers/clk/tegra/clk-tegra-super-cclk.c      |  165 +++
 drivers/clk/tegra/clk-tegra20.c               |    6 +-
 drivers/clk/tegra/clk-tegra30.c               |    6 +-
 drivers/clk/tegra/clk.h                       |   12 +
 drivers/cpufreq/Kconfig.arm                   |    4 +-
 drivers/cpufreq/cpufreq-dt-platdev.c          |    2 +
 drivers/cpufreq/tegra20-cpufreq.c             |  236 +---
 20 files changed, 2902 insertions(+), 206 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
 create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
 create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp.dtsi
 create mode 100644 arch/arm/boot/dts/tegra30-cpu-opp-microvolt.dtsi
 create mode 100644 arch/arm/boot/dts/tegra30-cpu-opp.dtsi
 create mode 100644 drivers/clk/tegra/clk-tegra-super-cclk.c

Comments

Viresh Kumar Oct. 16, 2019, 5:18 a.m. UTC | #1
On 16-10-19, 00:16, Dmitry Osipenko wrote:
> Re-parenting to intermediate clock is supported now by the clock driver
> and thus there is no need in a customized CPUFreq driver, all that code
> is common for both Tegra20 and Tegra30. The available CPU freqs are now
> specified in device-tree in a form of OPPs, all users should update their
> device-trees.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/Kconfig.arm          |   4 +-
>  drivers/cpufreq/cpufreq-dt-platdev.c |   2 +
>  drivers/cpufreq/tegra20-cpufreq.c    | 236 ++++++---------------------
>  3 files changed, 55 insertions(+), 187 deletions(-)
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index a905796f7f85..2118c45d0acd 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -301,8 +301,8 @@ config ARM_TANGO_CPUFREQ
>  	default y
>  
>  config ARM_TEGRA20_CPUFREQ
> -	tristate "Tegra20 CPUFreq support"
> -	depends on ARCH_TEGRA
> +	bool "Tegra20 CPUFreq support"

Google is currently working on the GKI (generic kernel image) project where they
want to use a single kernel image with modules for all kind of android devices.
And for that they need all such drivers to be built as module. Since this is
already an module, I would ask you to keep it as is instead of moving it to bool
here. Else some google guy will switch it back as module later on.

LGTM otherwise. Nice work. Thanks.
Viresh Kumar Oct. 16, 2019, 5:23 a.m. UTC | #2
On 16-10-19, 00:16, Dmitry Osipenko wrote:
> Operating Point are specified per HW version. The OPP voltages are kept
> in a separate DTSI file because some boards may not define CPU regulator
> in their device-tree if voltage scaling isn't necessary, like for example
> in a case of tegra20-trimslice which is outlet-powered device.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../boot/dts/tegra20-cpu-opp-microvolt.dtsi   | 201 ++++++++++++
>  arch/arm/boot/dts/tegra20-cpu-opp.dtsi        | 302 ++++++++++++++++++
>  2 files changed, 503 insertions(+)
>  create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
>  create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp.dtsi
> 
> diff --git a/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi b/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
> new file mode 100644
> index 000000000000..e85ffdbef876
> --- /dev/null
> +++ b/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/ {
> +	cpu0_opp_table: cpu_opp_table0 {
> +		opp@216000000_750 {

Maybe just drop the _750 (i.e. voltage) from the names as we don't generally
follow it :)

> +			opp-microvolt = <750000 750000 1125000>;
> +		};
Viresh Kumar Oct. 16, 2019, 5:27 a.m. UTC | #3
On 16-10-19, 00:16, Dmitry Osipenko wrote:
> Hello,
> 
> This series moves intermediate-clk handling from tegra20-cpufreq into
> tegra-clk driver, this allows us to switch to generic cpufreq-dt driver
> which brings voltage scaling, per-hardware OPPs and Tegra30 support out
> of the box. All boards need to adopt CPU OPPs in their device-trees in
> order to get cpufreq support. This series adds OPPs only to selective
> boards because there is assumption in a current device-trees that CPU
> voltage is set for 1GHz freq and this won't work for those CPUs that
> can go over 1GHz and thus require voltage regulators to be set up for
> voltage scaling support (CC'ed Marcel for Toradex boards). We could
> probably add delete-node for OPPs over 1GHz if there are not actively
> maintained boards.

How do you want to get these patches merged ? Can I just pick the cpufreq bits
alone ?
Dmitry Osipenko Oct. 16, 2019, 1:16 p.m. UTC | #4
16.10.2019 08:27, Viresh Kumar пишет:
> On 16-10-19, 00:16, Dmitry Osipenko wrote:
>> Hello,
>>
>> This series moves intermediate-clk handling from tegra20-cpufreq into
>> tegra-clk driver, this allows us to switch to generic cpufreq-dt driver
>> which brings voltage scaling, per-hardware OPPs and Tegra30 support out
>> of the box. All boards need to adopt CPU OPPs in their device-trees in
>> order to get cpufreq support. This series adds OPPs only to selective
>> boards because there is assumption in a current device-trees that CPU
>> voltage is set for 1GHz freq and this won't work for those CPUs that
>> can go over 1GHz and thus require voltage regulators to be set up for
>> voltage scaling support (CC'ed Marcel for Toradex boards). We could
>> probably add delete-node for OPPs over 1GHz if there are not actively
>> maintained boards.
> 
> How do you want to get these patches merged ? Can I just pick the cpufreq bits
> alone ?
> 

The cpufreq bits strictly depend on the clk patches and the regulators
coupler/balancer series. Hence all patches in this series should collect
acks from relevant maintainers and then Thierry will pick up the
patchsets in a correct order via tegra tree, at least that's my vision.

Thierry, are you okay with that approach?
Dmitry Osipenko Oct. 16, 2019, 1:21 p.m. UTC | #5
16.10.2019 08:23, Viresh Kumar пишет:
> On 16-10-19, 00:16, Dmitry Osipenko wrote:
>> Operating Point are specified per HW version. The OPP voltages are kept
>> in a separate DTSI file because some boards may not define CPU regulator
>> in their device-tree if voltage scaling isn't necessary, like for example
>> in a case of tegra20-trimslice which is outlet-powered device.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  .../boot/dts/tegra20-cpu-opp-microvolt.dtsi   | 201 ++++++++++++
>>  arch/arm/boot/dts/tegra20-cpu-opp.dtsi        | 302 ++++++++++++++++++
>>  2 files changed, 503 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
>>  create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp.dtsi
>>
>> diff --git a/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi b/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
>> new file mode 100644
>> index 000000000000..e85ffdbef876
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/ {
>> +	cpu0_opp_table: cpu_opp_table0 {
>> +		opp@216000000_750 {
> 
> Maybe just drop the _750 (i.e. voltage) from the names as we don't generally
> follow it :)

The reason for the _750 postfix is that there are multiple OPPs for
216MHz and they have different voltages for different versions of
hardware, thus those are separate OPPs and they can't be squashed into a
single OPP node.
Dmitry Osipenko Oct. 16, 2019, 1:29 p.m. UTC | #6
16.10.2019 08:18, Viresh Kumar пишет:
> On 16-10-19, 00:16, Dmitry Osipenko wrote:
>> Re-parenting to intermediate clock is supported now by the clock driver
>> and thus there is no need in a customized CPUFreq driver, all that code
>> is common for both Tegra20 and Tegra30. The available CPU freqs are now
>> specified in device-tree in a form of OPPs, all users should update their
>> device-trees.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/cpufreq/Kconfig.arm          |   4 +-
>>  drivers/cpufreq/cpufreq-dt-platdev.c |   2 +
>>  drivers/cpufreq/tegra20-cpufreq.c    | 236 ++++++---------------------
>>  3 files changed, 55 insertions(+), 187 deletions(-)
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index a905796f7f85..2118c45d0acd 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -301,8 +301,8 @@ config ARM_TANGO_CPUFREQ
>>  	default y
>>  
>>  config ARM_TEGRA20_CPUFREQ
>> -	tristate "Tegra20 CPUFreq support"
>> -	depends on ARCH_TEGRA
>> +	bool "Tegra20 CPUFreq support"
> 
> Google is currently working on the GKI (generic kernel image) project where they
> want to use a single kernel image with modules for all kind of android devices.
> And for that they need all such drivers to be built as module. Since this is
> already an module, I would ask you to keep it as is instead of moving it to bool
> here. Else some google guy will switch it back as module later on.
> 
> LGTM otherwise. Nice work. Thanks.
> 

Okay, I'll keep the modularity in v2.

Although, tegra20-cpufreq isn't a driver anymore because now it merely
prepares OPP table for the cpufreq-dt driver, which is really a one-shot
action that is enough to do during boot and thus modularity is a bit
redundant here.
Thierry Reding Oct. 16, 2019, 2:01 p.m. UTC | #7
On Wed, Oct 16, 2019 at 04:16:27PM +0300, Dmitry Osipenko wrote:
> 16.10.2019 08:27, Viresh Kumar пишет:
> > On 16-10-19, 00:16, Dmitry Osipenko wrote:
> >> Hello,
> >>
> >> This series moves intermediate-clk handling from tegra20-cpufreq into
> >> tegra-clk driver, this allows us to switch to generic cpufreq-dt driver
> >> which brings voltage scaling, per-hardware OPPs and Tegra30 support out
> >> of the box. All boards need to adopt CPU OPPs in their device-trees in
> >> order to get cpufreq support. This series adds OPPs only to selective
> >> boards because there is assumption in a current device-trees that CPU
> >> voltage is set for 1GHz freq and this won't work for those CPUs that
> >> can go over 1GHz and thus require voltage regulators to be set up for
> >> voltage scaling support (CC'ed Marcel for Toradex boards). We could
> >> probably add delete-node for OPPs over 1GHz if there are not actively
> >> maintained boards.
> > 
> > How do you want to get these patches merged ? Can I just pick the cpufreq bits
> > alone ?
> > 
> 
> The cpufreq bits strictly depend on the clk patches and the regulators
> coupler/balancer series. Hence all patches in this series should collect
> acks from relevant maintainers and then Thierry will pick up the
> patchsets in a correct order via tegra tree, at least that's my vision.
> 
> Thierry, are you okay with that approach?

Works for me. I already have a set of clock patches that I'd like to
merge via the Tegra tree because of a runtime dependency, so it'd be
easy to apply these on top of that.

Thierry
Dmitry Osipenko Oct. 16, 2019, 2:20 p.m. UTC | #8
16.10.2019 17:01, Thierry Reding пишет:
> On Wed, Oct 16, 2019 at 04:16:27PM +0300, Dmitry Osipenko wrote:
>> 16.10.2019 08:27, Viresh Kumar пишет:
>>> On 16-10-19, 00:16, Dmitry Osipenko wrote:
>>>> Hello,
>>>>
>>>> This series moves intermediate-clk handling from tegra20-cpufreq into
>>>> tegra-clk driver, this allows us to switch to generic cpufreq-dt driver
>>>> which brings voltage scaling, per-hardware OPPs and Tegra30 support out
>>>> of the box. All boards need to adopt CPU OPPs in their device-trees in
>>>> order to get cpufreq support. This series adds OPPs only to selective
>>>> boards because there is assumption in a current device-trees that CPU
>>>> voltage is set for 1GHz freq and this won't work for those CPUs that
>>>> can go over 1GHz and thus require voltage regulators to be set up for
>>>> voltage scaling support (CC'ed Marcel for Toradex boards). We could
>>>> probably add delete-node for OPPs over 1GHz if there are not actively
>>>> maintained boards.
>>>
>>> How do you want to get these patches merged ? Can I just pick the cpufreq bits
>>> alone ?
>>>
>>
>> The cpufreq bits strictly depend on the clk patches and the regulators
>> coupler/balancer series. Hence all patches in this series should collect
>> acks from relevant maintainers and then Thierry will pick up the
>> patchsets in a correct order via tegra tree, at least that's my vision.
>>
>> Thierry, are you okay with that approach?
> 
> Works for me. I already have a set of clock patches that I'd like to
> merge via the Tegra tree because of a runtime dependency, so it'd be
> easy to apply these on top of that.

Awesome, thank you very much!

Viresh, then only acks to the patches related to cpufreq driver are
needed from you for this series.
Peter Geis Oct. 16, 2019, 2:58 p.m. UTC | #9
On Wed, Oct 16, 2019 at 9:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 16.10.2019 08:18, Viresh Kumar пишет:
> > On 16-10-19, 00:16, Dmitry Osipenko wrote:
> >> Re-parenting to intermediate clock is supported now by the clock driver
> >> and thus there is no need in a customized CPUFreq driver, all that code
> >> is common for both Tegra20 and Tegra30. The available CPU freqs are now
> >> specified in device-tree in a form of OPPs, all users should update their
> >> device-trees.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/cpufreq/Kconfig.arm          |   4 +-
> >>  drivers/cpufreq/cpufreq-dt-platdev.c |   2 +
> >>  drivers/cpufreq/tegra20-cpufreq.c    | 236 ++++++---------------------
> >>  3 files changed, 55 insertions(+), 187 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> >> index a905796f7f85..2118c45d0acd 100644
> >> --- a/drivers/cpufreq/Kconfig.arm
> >> +++ b/drivers/cpufreq/Kconfig.arm
> >> @@ -301,8 +301,8 @@ config ARM_TANGO_CPUFREQ
> >>      default y
> >>
> >>  config ARM_TEGRA20_CPUFREQ
> >> -    tristate "Tegra20 CPUFreq support"
> >> -    depends on ARCH_TEGRA
> >> +    bool "Tegra20 CPUFreq support"
> >
> > Google is currently working on the GKI (generic kernel image) project where they
> > want to use a single kernel image with modules for all kind of android devices.
> > And for that they need all such drivers to be built as module. Since this is
> > already an module, I would ask you to keep it as is instead of moving it to bool
> > here. Else some google guy will switch it back as module later on.
> >
> > LGTM otherwise. Nice work. Thanks.
> >
>
> Okay, I'll keep the modularity in v2.
>
> Although, tegra20-cpufreq isn't a driver anymore because now it merely
> prepares OPP table for the cpufreq-dt driver, which is really a one-shot
> action that is enough to do during boot and thus modularity is a bit
> redundant here.

I doubt Google will care much, since Android has moved on to aarch64.
Do they even support arm32 any more?
Dmitry Osipenko Oct. 16, 2019, 6:19 p.m. UTC | #10
16.10.2019 17:58, Peter Geis пишет:
> On Wed, Oct 16, 2019 at 9:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 16.10.2019 08:18, Viresh Kumar пишет:
>>> On 16-10-19, 00:16, Dmitry Osipenko wrote:
>>>> Re-parenting to intermediate clock is supported now by the clock driver
>>>> and thus there is no need in a customized CPUFreq driver, all that code
>>>> is common for both Tegra20 and Tegra30. The available CPU freqs are now
>>>> specified in device-tree in a form of OPPs, all users should update their
>>>> device-trees.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/cpufreq/Kconfig.arm          |   4 +-
>>>>  drivers/cpufreq/cpufreq-dt-platdev.c |   2 +
>>>>  drivers/cpufreq/tegra20-cpufreq.c    | 236 ++++++---------------------
>>>>  3 files changed, 55 insertions(+), 187 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>>>> index a905796f7f85..2118c45d0acd 100644
>>>> --- a/drivers/cpufreq/Kconfig.arm
>>>> +++ b/drivers/cpufreq/Kconfig.arm
>>>> @@ -301,8 +301,8 @@ config ARM_TANGO_CPUFREQ
>>>>      default y
>>>>
>>>>  config ARM_TEGRA20_CPUFREQ
>>>> -    tristate "Tegra20 CPUFreq support"
>>>> -    depends on ARCH_TEGRA
>>>> +    bool "Tegra20 CPUFreq support"
>>>
>>> Google is currently working on the GKI (generic kernel image) project where they
>>> want to use a single kernel image with modules for all kind of android devices.
>>> And for that they need all such drivers to be built as module. Since this is
>>> already an module, I would ask you to keep it as is instead of moving it to bool
>>> here. Else some google guy will switch it back as module later on.
>>>
>>> LGTM otherwise. Nice work. Thanks.
>>>
>>
>> Okay, I'll keep the modularity in v2.
>>
>> Although, tegra20-cpufreq isn't a driver anymore because now it merely
>> prepares OPP table for the cpufreq-dt driver, which is really a one-shot
>> action that is enough to do during boot and thus modularity is a bit
>> redundant here.
> 
> I doubt Google will care much, since Android has moved on to aarch64.
> Do they even support arm32 any more?

Yes, I don't think there is a real need to care about Google. They won't
use pure upstream and won't care about older hardware any ways.
Viresh Kumar Oct. 17, 2019, 2:28 a.m. UTC | #11
On 16-10-19, 16:21, Dmitry Osipenko wrote:
> 16.10.2019 08:23, Viresh Kumar пишет:
> > On 16-10-19, 00:16, Dmitry Osipenko wrote:
> >> Operating Point are specified per HW version. The OPP voltages are kept
> >> in a separate DTSI file because some boards may not define CPU regulator
> >> in their device-tree if voltage scaling isn't necessary, like for example
> >> in a case of tegra20-trimslice which is outlet-powered device.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  .../boot/dts/tegra20-cpu-opp-microvolt.dtsi   | 201 ++++++++++++
> >>  arch/arm/boot/dts/tegra20-cpu-opp.dtsi        | 302 ++++++++++++++++++
> >>  2 files changed, 503 insertions(+)
> >>  create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
> >>  create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp.dtsi
> >>
> >> diff --git a/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi b/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
> >> new file mode 100644
> >> index 000000000000..e85ffdbef876
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
> >> @@ -0,0 +1,201 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +/ {
> >> +	cpu0_opp_table: cpu_opp_table0 {
> >> +		opp@216000000_750 {
> > 
> > Maybe just drop the _750 (i.e. voltage) from the names as we don't generally
> > follow it :)
> 
> The reason for the _750 postfix is that there are multiple OPPs for
> 216MHz and they have different voltages for different versions of
> hardware, thus those are separate OPPs and they can't be squashed into a
> single OPP node.

Ah, okay. I missed that you are using supported-hw bindings.
Viresh Kumar Oct. 17, 2019, 2:32 a.m. UTC | #12
On 16-10-19, 21:19, Dmitry Osipenko wrote:
> 16.10.2019 17:58, Peter Geis пишет:
> > On Wed, Oct 16, 2019 at 9:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> 16.10.2019 08:18, Viresh Kumar пишет:
> >>> On 16-10-19, 00:16, Dmitry Osipenko wrote:
> >>>> Re-parenting to intermediate clock is supported now by the clock driver
> >>>> and thus there is no need in a customized CPUFreq driver, all that code
> >>>> is common for both Tegra20 and Tegra30. The available CPU freqs are now
> >>>> specified in device-tree in a form of OPPs, all users should update their
> >>>> device-trees.
> >>>>
> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>> ---
> >>>>  drivers/cpufreq/Kconfig.arm          |   4 +-
> >>>>  drivers/cpufreq/cpufreq-dt-platdev.c |   2 +
> >>>>  drivers/cpufreq/tegra20-cpufreq.c    | 236 ++++++---------------------
> >>>>  3 files changed, 55 insertions(+), 187 deletions(-)
> >>>>
> >>>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> >>>> index a905796f7f85..2118c45d0acd 100644
> >>>> --- a/drivers/cpufreq/Kconfig.arm
> >>>> +++ b/drivers/cpufreq/Kconfig.arm
> >>>> @@ -301,8 +301,8 @@ config ARM_TANGO_CPUFREQ
> >>>>      default y
> >>>>
> >>>>  config ARM_TEGRA20_CPUFREQ
> >>>> -    tristate "Tegra20 CPUFreq support"
> >>>> -    depends on ARCH_TEGRA
> >>>> +    bool "Tegra20 CPUFreq support"
> >>>
> >>> Google is currently working on the GKI (generic kernel image) project where they
> >>> want to use a single kernel image with modules for all kind of android devices.
> >>> And for that they need all such drivers to be built as module. Since this is
> >>> already an module, I would ask you to keep it as is instead of moving it to bool
> >>> here. Else some google guy will switch it back as module later on.
> >>>
> >>> LGTM otherwise. Nice work. Thanks.
> >>>
> >>
> >> Okay, I'll keep the modularity in v2.
> >>
> >> Although, tegra20-cpufreq isn't a driver anymore because now it merely
> >> prepares OPP table for the cpufreq-dt driver, which is really a one-shot
> >> action that is enough to do during boot and thus modularity is a bit
> >> redundant here.
> > 
> > I doubt Google will care much, since Android has moved on to aarch64.
> > Do they even support arm32 any more?
> 
> Yes, I don't think there is a real need to care about Google. They won't
> use pure upstream and won't care about older hardware any ways.

Well, using (almost) pure upstream is the idea I believe. And the thing is they
want to use a single multi-platform image which should be as small as possible
in size. So it won't have any drivers or platform stuff (if possible) and
everything is module.

I am not sure about arm32/64 thing though. And it is okay if you don't want to
care about Google right now. That was just some side knowledge I had :)
Viresh Kumar Oct. 17, 2019, 2:32 a.m. UTC | #13
On 16-10-19, 00:16, Dmitry Osipenko wrote:
> Operating Point are specified per HW version. The OPP voltages are kept
> in a separate DTSI file because some boards may not define CPU regulator
> in their device-tree if voltage scaling isn't necessary, like for example
> in a case of tegra20-trimslice which is outlet-powered device.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../boot/dts/tegra20-cpu-opp-microvolt.dtsi   | 201 ++++++++++++
>  arch/arm/boot/dts/tegra20-cpu-opp.dtsi        | 302 ++++++++++++++++++
>  2 files changed, 503 insertions(+)
>  create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
>  create mode 100644 arch/arm/boot/dts/tegra20-cpu-opp.dtsi
> 
> diff --git a/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi b/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
> new file mode 100644
> index 000000000000..e85ffdbef876
> --- /dev/null
> +++ b/arch/arm/boot/dts/tegra20-cpu-opp-microvolt.dtsi
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/ {
> +	cpu0_opp_table: cpu_opp_table0 {
> +		opp@216000000_750 {
> +			opp-microvolt = <750000 750000 1125000>;
> +		};
> +
> +		opp@216000000_800 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@312000000_750 {
> +			opp-microvolt = <750000 750000 1125000>;
> +		};
> +
> +		opp@312000000_800 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@456000000_750 {
> +			opp-microvolt = <750000 750000 1125000>;
> +		};
> +
> +		opp@456000000_800 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@456000000_800_2_2 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@456000000_800_3_2 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@456000000_825 {
> +			opp-microvolt = <825000 825000 1125000>;
> +		};
> +
> +		opp@608000000_750 {
> +			opp-microvolt = <750000 750000 1125000>;
> +		};
> +
> +		opp@608000000_800 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@608000000_800_3_2 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@608000000_825 {
> +			opp-microvolt = <825000 825000 1125000>;
> +		};
> +
> +		opp@608000000_850 {
> +			opp-microvolt = <850000 850000 1125000>;
> +		};
> +
> +		opp@608000000_900 {
> +			opp-microvolt = <900000 900000 1125000>;
> +		};
> +
> +		opp@760000000_775 {
> +			opp-microvolt = <775000 775000 1125000>;
> +		};
> +
> +		opp@760000000_800 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@760000000_850 {
> +			opp-microvolt = <850000 850000 1125000>;
> +		};
> +
> +		opp@760000000_875 {
> +			opp-microvolt = <875000 875000 1125000>;
> +		};
> +
> +		opp@760000000_875_1_1 {
> +			opp-microvolt = <875000 875000 1125000>;
> +		};
> +
> +		opp@760000000_875_0_2 {
> +			opp-microvolt = <875000 875000 1125000>;
> +		};
> +
> +		opp@760000000_875_1_2 {
> +			opp-microvolt = <875000 875000 1125000>;
> +		};
> +
> +		opp@760000000_900 {
> +			opp-microvolt = <900000 900000 1125000>;
> +		};
> +
> +		opp@760000000_975 {
> +			opp-microvolt = <975000 975000 1125000>;
> +		};
> +
> +		opp@816000000_800 {
> +			opp-microvolt = <800000 800000 1125000>;
> +		};
> +
> +		opp@816000000_850 {
> +			opp-microvolt = <850000 850000 1125000>;
> +		};
> +
> +		opp@816000000_875 {
> +			opp-microvolt = <875000 875000 1125000>;
> +		};
> +
> +		opp@816000000_950 {
> +			opp-microvolt = <950000 950000 1125000>;
> +		};
> +
> +		opp@816000000_1000 {
> +			opp-microvolt = <1000000 1000000 1125000>;
> +		};
> +
> +		opp@912000000_850 {
> +			opp-microvolt = <850000 850000 1125000>;
> +		};
> +
> +		opp@912000000_900 {
> +			opp-microvolt = <900000 900000 1125000>;
> +		};
> +
> +		opp@912000000_925 {
> +			opp-microvolt = <925000 925000 1125000>;
> +		};
> +
> +		opp@912000000_950 {
> +			opp-microvolt = <950000 950000 1125000>;
> +		};
> +
> +		opp@912000000_950_0_2 {
> +			opp-microvolt = <950000 950000 1125000>;
> +		};
> +
> +		opp@912000000_950_2_2 {
> +			opp-microvolt = <950000 950000 1125000>;
> +		};
> +
> +		opp@912000000_1000 {
> +			opp-microvolt = <1000000 1000000 1125000>;
> +		};
> +
> +		opp@912000000_1050 {
> +			opp-microvolt = <1050000 1050000 1125000>;
> +		};
> +
> +		opp@1000000000_875 {
> +			opp-microvolt = <875000 875000 1125000>;
> +		};
> +
> +		opp@1000000000_900 {
> +			opp-microvolt = <900000 900000 1125000>;
> +		};
> +
> +		opp@1000000000_950 {
> +			opp-microvolt = <950000 950000 1125000>;
> +		};
> +
> +		opp@1000000000_975 {
> +			opp-microvolt = <975000 975000 1125000>;
> +		};
> +
> +		opp@1000000000_1000 {
> +			opp-microvolt = <1000000 1000000 1125000>;
> +		};
> +
> +		opp@1000000000_1000_0_2 {
> +			opp-microvolt = <1000000 1000000 1125000>;
> +		};
> +
> +		opp@1000000000_1025 {
> +			opp-microvolt = <1025000 1025000 1125000>;
> +		};
> +
> +		opp@1000000000_1100 {
> +			opp-microvolt = <1100000 1100000 1125000>;
> +		};
> +
> +		opp@1200000000_1000 {
> +			opp-microvolt = <1000000 1000000 1125000>;
> +		};
> +
> +		opp@1200000000_1050 {
> +			opp-microvolt = <1050000 1050000 1125000>;
> +		};
> +
> +		opp@1200000000_1100 {
> +			opp-microvolt = <1100000 1100000 1125000>;
> +		};
> +
> +		opp@1200000000_1125 {
> +			opp-microvolt = <1125000 1125000 1125000>;
> +		};
> +	};
> +};
> diff --git a/arch/arm/boot/dts/tegra20-cpu-opp.dtsi b/arch/arm/boot/dts/tegra20-cpu-opp.dtsi
> new file mode 100644
> index 000000000000..c878f4231791
> --- /dev/null
> +++ b/arch/arm/boot/dts/tegra20-cpu-opp.dtsi
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/ {
> +	cpu0_opp_table: cpu_opp_table0 {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		opp@216000000_750 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x0F 0x0003>;
> +			opp-hz = /bits/ 64 <216000000>;
> +		};
> +
> +		opp@216000000_800 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x0F 0x0004>;
> +			opp-hz = /bits/ 64 <216000000>;
> +		};
> +
> +		opp@312000000_750 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x0F 0x0003>;
> +			opp-hz = /bits/ 64 <312000000>;
> +		};
> +
> +		opp@312000000_800 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x0F 0x0004>;
> +			opp-hz = /bits/ 64 <312000000>;
> +		};
> +
> +		opp@456000000_750 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x0C 0x0003>;
> +			opp-hz = /bits/ 64 <456000000>;
> +		};
> +
> +		opp@456000000_800 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0006>;
> +			opp-hz = /bits/ 64 <456000000>;
> +		};
> +
> +		opp@456000000_800_2_2 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0004>;
> +			opp-hz = /bits/ 64 <456000000>;
> +		};
> +
> +		opp@456000000_800_3_2 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0004>;
> +			opp-hz = /bits/ 64 <456000000>;
> +		};
> +
> +		opp@456000000_825 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0001>;
> +			opp-hz = /bits/ 64 <456000000>;
> +		};
> +
> +		opp@608000000_750 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0003>;
> +			opp-hz = /bits/ 64 <608000000>;
> +		};
> +
> +		opp@608000000_800 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0006>;
> +			opp-hz = /bits/ 64 <608000000>;
> +		};
> +
> +		opp@608000000_800_3_2 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0004>;
> +			opp-hz = /bits/ 64 <608000000>;
> +		};
> +
> +		opp@608000000_825 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0001>;
> +			opp-hz = /bits/ 64 <608000000>;
> +		};
> +
> +		opp@608000000_850 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0006>;
> +			opp-hz = /bits/ 64 <608000000>;
> +		};
> +
> +		opp@608000000_900 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0001>;
> +			opp-hz = /bits/ 64 <608000000>;
> +		};
> +
> +		opp@760000000_775 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0003>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@760000000_800 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0004>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@760000000_850 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0006>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@760000000_875 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0001>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@760000000_875_1_1 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x02 0x0002>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@760000000_875_0_2 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x01 0x0004>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@760000000_875_1_2 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x02 0x0004>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@760000000_900 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x01 0x0002>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@760000000_975 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0001>;
> +			opp-hz = /bits/ 64 <760000000>;
> +		};
> +
> +		opp@816000000_800 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0007>;
> +			opp-hz = /bits/ 64 <816000000>;
> +		};
> +
> +		opp@816000000_850 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0002>;
> +			opp-hz = /bits/ 64 <816000000>;
> +		};
> +
> +		opp@816000000_875 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0005>;
> +			opp-hz = /bits/ 64 <816000000>;
> +		};
> +
> +		opp@816000000_950 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0006>;
> +			opp-hz = /bits/ 64 <816000000>;
> +		};
> +
> +		opp@816000000_1000 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0001>;
> +			opp-hz = /bits/ 64 <816000000>;
> +		};
> +
> +		opp@912000000_850 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0007>;
> +			opp-hz = /bits/ 64 <912000000>;
> +		};
> +
> +		opp@912000000_900 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0002>;
> +			opp-hz = /bits/ 64 <912000000>;
> +		};
> +
> +		opp@912000000_925 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0001>;
> +			opp-hz = /bits/ 64 <912000000>;
> +		};
> +
> +		opp@912000000_950 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x02 0x0006>;
> +			opp-hz = /bits/ 64 <912000000>;
> +		};
> +
> +		opp@912000000_950_0_2 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x01 0x0004>;
> +			opp-hz = /bits/ 64 <912000000>;
> +		};
> +
> +		opp@912000000_950_2_2 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0004>;
> +			opp-hz = /bits/ 64 <912000000>;
> +		};
> +
> +		opp@912000000_1000 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x01 0x0002>;
> +			opp-hz = /bits/ 64 <912000000>;
> +		};
> +
> +		opp@912000000_1050 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0001>;
> +			opp-hz = /bits/ 64 <912000000>;
> +		};
> +
> +		opp@1000000000_875 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0007>;
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp@1000000000_900 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0002>;
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp@1000000000_950 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0004>;
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp@1000000000_975 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0001>;
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp@1000000000_1000 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x02 0x0006>;
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp@1000000000_1000_0_2 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x01 0x0004>;
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp@1000000000_1025 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x01 0x0002>;
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp@1000000000_1100 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x03 0x0001>;
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp@1200000000_1000 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x08 0x0004>;
> +			opp-hz = /bits/ 64 <1200000000>;
> +		};
> +
> +		opp@1200000000_1050 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x04 0x0004>;
> +			opp-hz = /bits/ 64 <1200000000>;
> +		};
> +
> +		opp@1200000000_1100 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x02 0x0004>;
> +			opp-hz = /bits/ 64 <1200000000>;
> +		};
> +
> +		opp@1200000000_1125 {
> +			clock-latency-ns = <400000>;
> +			opp-supported-hw = <0x01 0x0004>;
> +			opp-hz = /bits/ 64 <1200000000>;
> +		};
> +	};
> +};

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Viresh Kumar Oct. 17, 2019, 2:33 a.m. UTC | #14
On 16-10-19, 00:16, Dmitry Osipenko wrote:
> Operating Point are specified per HW version. The OPP voltages are kept
> in a separate DTSI file because some boards may not define CPU regulator
> in their device-tree if voltage scaling isn't necessary for them.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../boot/dts/tegra30-cpu-opp-microvolt.dtsi   |  801 +++++++++++
>  arch/arm/boot/dts/tegra30-cpu-opp.dtsi        | 1202 +++++++++++++++++
>  2 files changed, 2003 insertions(+)
>  create mode 100644 arch/arm/boot/dts/tegra30-cpu-opp-microvolt.dtsi
>  create mode 100644 arch/arm/boot/dts/tegra30-cpu-opp.dtsi

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Viresh Kumar Oct. 17, 2019, 2:33 a.m. UTC | #15
On 16-10-19, 00:16, Dmitry Osipenko wrote:
> Re-parenting to intermediate clock is supported now by the clock driver
> and thus there is no need in a customized CPUFreq driver, all that code
> is common for both Tegra20 and Tegra30. The available CPU freqs are now
> specified in device-tree in a form of OPPs, all users should update their
> device-trees.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/cpufreq/Kconfig.arm          |   4 +-
>  drivers/cpufreq/cpufreq-dt-platdev.c |   2 +
>  drivers/cpufreq/tegra20-cpufreq.c    | 236 ++++++---------------------
>  3 files changed, 55 insertions(+), 187 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Dmitry Osipenko Oct. 17, 2019, 9:09 p.m. UTC | #16
17.10.2019 05:32, Viresh Kumar пишет:
> On 16-10-19, 21:19, Dmitry Osipenko wrote:
>> 16.10.2019 17:58, Peter Geis пишет:
>>> On Wed, Oct 16, 2019 at 9:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> 16.10.2019 08:18, Viresh Kumar пишет:
>>>>> On 16-10-19, 00:16, Dmitry Osipenko wrote:
>>>>>> Re-parenting to intermediate clock is supported now by the clock driver
>>>>>> and thus there is no need in a customized CPUFreq driver, all that code
>>>>>> is common for both Tegra20 and Tegra30. The available CPU freqs are now
>>>>>> specified in device-tree in a form of OPPs, all users should update their
>>>>>> device-trees.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>>  drivers/cpufreq/Kconfig.arm          |   4 +-
>>>>>>  drivers/cpufreq/cpufreq-dt-platdev.c |   2 +
>>>>>>  drivers/cpufreq/tegra20-cpufreq.c    | 236 ++++++---------------------
>>>>>>  3 files changed, 55 insertions(+), 187 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>>>>>> index a905796f7f85..2118c45d0acd 100644
>>>>>> --- a/drivers/cpufreq/Kconfig.arm
>>>>>> +++ b/drivers/cpufreq/Kconfig.arm
>>>>>> @@ -301,8 +301,8 @@ config ARM_TANGO_CPUFREQ
>>>>>>      default y
>>>>>>
>>>>>>  config ARM_TEGRA20_CPUFREQ
>>>>>> -    tristate "Tegra20 CPUFreq support"
>>>>>> -    depends on ARCH_TEGRA
>>>>>> +    bool "Tegra20 CPUFreq support"
>>>>>
>>>>> Google is currently working on the GKI (generic kernel image) project where they
>>>>> want to use a single kernel image with modules for all kind of android devices.
>>>>> And for that they need all such drivers to be built as module. Since this is
>>>>> already an module, I would ask you to keep it as is instead of moving it to bool
>>>>> here. Else some google guy will switch it back as module later on.
>>>>>
>>>>> LGTM otherwise. Nice work. Thanks.
>>>>>
>>>>
>>>> Okay, I'll keep the modularity in v2.
>>>>
>>>> Although, tegra20-cpufreq isn't a driver anymore because now it merely
>>>> prepares OPP table for the cpufreq-dt driver, which is really a one-shot
>>>> action that is enough to do during boot and thus modularity is a bit
>>>> redundant here.
>>>
>>> I doubt Google will care much, since Android has moved on to aarch64.
>>> Do they even support arm32 any more?
>>
>> Yes, I don't think there is a real need to care about Google. They won't
>> use pure upstream and won't care about older hardware any ways.
> 
> Well, using (almost) pure upstream is the idea I believe. And the thing is they
> want to use a single multi-platform image which should be as small as possible
> in size. So it won't have any drivers or platform stuff (if possible) and
> everything is module.
> 
> I am not sure about arm32/64 thing though. And it is okay if you don't want to
> care about Google right now. That was just some side knowledge I had :)
> 

I'll leave the module part as-is for now.
Peter De Schrijver Oct. 28, 2019, 2:57 p.m. UTC | #17
On Wed, Oct 16, 2019 at 12:16:02AM +0300, Dmitry Osipenko wrote:
> CCLK stands for "CPU Clock", CPU core is running off CCLK. CCLK supports
> multiple parents and it has internal clock divider which uses clock
> skipping technique, meaning that CPU's voltage should correspond to the
> parent clock rate and not CCLK. PLLX is the main CCLK parent that provides
> clock rates above 1GHz and it has special property such that the CCLK's
> internal divider is set into bypass mode when PLLX is set as a parent for
> CCLK.
> 
> This patch forks generic Super Clock into CCLK implementation which takes
> into account all CCLK specifics. The proper CCLK implementation is needed
> by the upcoming Tegra20 CPUFreq driver update that will allow to utilize
> the generic cpufreq-dt driver by moving intermediate clock handling into
> the clock driver. Note that technically this all could be squashed into
> clk-super, but result will be messier.
> 
> Note that currently all CCLKLP bits are left in the clk-super.c and only
> CCLKG is supported by clk-tegra-super-cclk. It shouldn't be difficult
> to move the CCLKLP bits, but CCLKLP is not used by anything in kernel
> and thus better not to touch it for now.

..

> +	super->reg = reg;
> +	super->lock = lock;
> +	super->width = 4;
> +	super->flags = clk_super_flags;
> +	super->frac_div.reg = reg + 4;
> +	super->frac_div.shift = 16;
> +	super->frac_div.width = 8;
> +	super->frac_div.frac_width = 1;
> +	super->frac_div.lock = lock;
> +	super->frac_div.flags = TEGRA_DIVIDER_SUPER;
> +	super->div_ops = &tegra_clk_frac_div_ops;
> +

This is not right. The super clock divider is not a divider, it's a
pulse skipper.

> +	/* Data in .init is copied by clk_register(), so stack variable OK */
> +	super->hw.init = &init;
> +
> +	clk = clk_register(NULL, &super->hw);
> +	if (IS_ERR(clk))
> +		kfree(super);
> +
> +	return clk;
> +}
> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> index f81c10654aa9..095595a5b8a8 100644
> --- a/drivers/clk/tegra/clk.h
> +++ b/drivers/clk/tegra/clk.h
> @@ -699,6 +699,10 @@ struct clk *tegra_clk_register_super_clk(const char *name,
>  		const char * const *parent_names, u8 num_parents,
>  		unsigned long flags, void __iomem *reg, u8 clk_super_flags,
>  		spinlock_t *lock);
> +struct clk *tegra_clk_register_super_cclk(const char *name,
> +		const char * const *parent_names, u8 num_parents,
> +		unsigned long flags, void __iomem *reg, u8 clk_super_flags,
> +		spinlock_t *lock);
>  
>  /**
>   * struct tegra_sdmmc_mux - switch divider with Low Jitter inputs for SDMMC
> -- 
> 2.23.0
>
Dmitry Osipenko Oct. 28, 2019, 11:48 p.m. UTC | #18
28.10.2019 17:57, Peter De Schrijver пишет:
> On Wed, Oct 16, 2019 at 12:16:02AM +0300, Dmitry Osipenko wrote:
>> CCLK stands for "CPU Clock", CPU core is running off CCLK. CCLK supports
>> multiple parents and it has internal clock divider which uses clock
>> skipping technique, meaning that CPU's voltage should correspond to the
>> parent clock rate and not CCLK. PLLX is the main CCLK parent that provides
>> clock rates above 1GHz and it has special property such that the CCLK's
>> internal divider is set into bypass mode when PLLX is set as a parent for
>> CCLK.
>>
>> This patch forks generic Super Clock into CCLK implementation which takes
>> into account all CCLK specifics. The proper CCLK implementation is needed
>> by the upcoming Tegra20 CPUFreq driver update that will allow to utilize
>> the generic cpufreq-dt driver by moving intermediate clock handling into
>> the clock driver. Note that technically this all could be squashed into
>> clk-super, but result will be messier.
>>
>> Note that currently all CCLKLP bits are left in the clk-super.c and only
>> CCLKG is supported by clk-tegra-super-cclk. It shouldn't be difficult
>> to move the CCLKLP bits, but CCLKLP is not used by anything in kernel
>> and thus better not to touch it for now.
> 
> ..
> 
>> +	super->reg = reg;
>> +	super->lock = lock;
>> +	super->width = 4;
>> +	super->flags = clk_super_flags;
>> +	super->frac_div.reg = reg + 4;
>> +	super->frac_div.shift = 16;
>> +	super->frac_div.width = 8;
>> +	super->frac_div.frac_width = 1;
>> +	super->frac_div.lock = lock;
>> +	super->frac_div.flags = TEGRA_DIVIDER_SUPER;
>> +	super->div_ops = &tegra_clk_frac_div_ops;
>> +
> 
> This is not right. The super clock divider is not a divider, it's a
> pulse skipper.

For the reference: on #tegra Peter explained to me in a more details
what was meant here. Turned out that T30+ has a real CCLK divider and we
won't use the pulse skipper for T20 nor for T30+, I'll update clk
patches accordingly in the next revision.