Tegra132 has almost the same clock structure as Tegra124, except for the CPU
related clocks.
Patch 1 is a small change to not stop initializing clocks when an error is
encountered
Patch 2 makes tegra_clocks_apply_init_table an arch_initcall to make it work
on ARM64
Patch 3 updates the Tegra124 binding doc with the details about the clock
changes for Tegra132
Patch 4 adds a new binding for the CPU clocks on Tegra132
Patch 5 Implements the changes to the CAR clocks for Tegra132
Patch 6 Implements the new Tegar132 ccplex clocks
Peter De Schrijver (6):
clk: tegra: don't abort clk init on error
clk: tegra: make tegra_clocks_apply_init_table arch_initcall
clk: tegra: Update binding doc Tegra132
clk: tegra: add nvidia,tegra132-ccplex-clk binding
clk: tegra: Add support for Tegra132 CAR clocks
clk: tegra: Add Tegra132 ccplex clocks
.../bindings/clock/nvidia,tegra124-car.txt | 8 +++--
.../bindings/clock/nvidia,tegra132-ccplex-clk.txt | 27 ++++++++++++++++
arch/arm/mach-tegra/tegra.c | 3 --
drivers/clk/tegra/Makefile | 2 +
drivers/clk/tegra/clk-tegra124.c | 32 ++++++++++++++++++++
drivers/clk/tegra/clk.c | 9 ++++--
include/dt-bindings/clock/tegra132-ccplex.h | 12 +++++++
include/linux/clk/tegra.h | 2 -
8 files changed, 84 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt
create mode 100644 include/dt-bindings/clock/tegra132-ccplex.h
--
1.7.7.rc0.72.g4b5ea.dirty
tegra_clocks_apply_init_table needs to be called after the udelay loop has
been calibrated (see 441f199a37cfd66c5dd8dd45490bd3ea6971117d why that is).
On existing Tegra SoCs this was done by calling tegra_clocks_apply_init_table
from tegra_dt_init. To make this also work on ARM64, we need to
change this into an initcall. tegra_dt_init is called from customize_machine
which is an arch_initcall. Therefore this should also work on existing 32bit
Tegra SoCs.
Tested on Tegra20 (ventana), Tegra30 (beaverboard), Tegra124 (jetson TK1) and
Tegra132.
Signed-off-by: Peter De Schrijver <[email protected]>
---
arch/arm/mach-tegra/tegra.c | 3 ---
drivers/clk/tegra/clk.c | 7 +++++--
include/linux/clk/tegra.h | 2 --
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index 15ac9fc..6c5cad5 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -32,7 +32,6 @@
#include <linux/slab.h>
#include <linux/sys_soc.h>
#include <linux/usb/tegra_usb_phy.h>
-#include <linux/clk/tegra.h>
#include <linux/irqchip.h>
#include <asm/hardware/cache-l2x0.h>
@@ -96,8 +95,6 @@ static void __init tegra_dt_init(void)
tegra_pmc_init();
- tegra_clocks_apply_init_table();
-
soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
if (!soc_dev_attr)
goto out;
diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
index d081732..65cde4e 100644
--- a/drivers/clk/tegra/clk.c
+++ b/drivers/clk/tegra/clk.c
@@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id,
tegra_clk_apply_init_table_func tegra_clk_apply_init_table;
-void __init tegra_clocks_apply_init_table(void)
+static int __init tegra_clocks_apply_init_table(void)
{
if (!tegra_clk_apply_init_table)
- return;
+ return 0;
tegra_clk_apply_init_table();
+
+ return 0;
}
+arch_initcall(tegra_clocks_apply_init_table);
diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h
index 3ca9fca..19c4208 100644
--- a/include/linux/clk/tegra.h
+++ b/include/linux/clk/tegra.h
@@ -120,6 +120,4 @@ static inline void tegra_cpu_clock_resume(void)
}
#endif
-void tegra_clocks_apply_init_table(void);
-
#endif /* __LINUX_CLK_TEGRA_H_ */
--
1.7.7.rc0.72.g4b5ea.dirty
Just continue initializing clocks if there's an error on one of them. This
is useful if there's a mistake in the inittable, because the system could
hang if clk_disable_unused() disables some of the critical clocks in this
table.
Signed-off-by: Peter De Schrijver <[email protected]>
---
drivers/clk/tegra/clk.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
index c0a7d77..d081732 100644
--- a/drivers/clk/tegra/clk.c
+++ b/drivers/clk/tegra/clk.c
@@ -207,7 +207,7 @@ void __init tegra_init_from_table(struct tegra_clk_init_table *tbl,
for (; tbl->clk_id < clk_max; tbl++) {
clk = clks[tbl->clk_id];
if (IS_ERR_OR_NULL(clk))
- return;
+ continue;
if (tbl->parent_id < clk_max) {
struct clk *parent = clks[tbl->parent_id];
--
1.7.7.rc0.72.g4b5ea.dirty
Tegra132 has a few new clocks for the CPU complex (ccplex).
Signed-off-by: Peter De Schrijver <[email protected]>
---
.../bindings/clock/nvidia,tegra132-ccplex-clk.txt | 27 ++++++++++++++++++++
include/dt-bindings/clock/tegra132-ccplex.h | 12 +++++++++
2 files changed, 39 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt
create mode 100644 include/dt-bindings/clock/tegra132-ccplex.h
diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt
new file mode 100644
index 0000000..1441e36
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt
@@ -0,0 +1,27 @@
+NVIDIA Tegra132 ccplex clocks
+
+This binding uses the common clock binding:
+Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+The Tegra132 ccplex clock module on Tegra132 is the HW module responsible
+for the ccplex related clocks.
+
+Required properties :
+- compatible : Should be "nvidia,tegra132-ccplex-clk"
+- reg : Should contain ccplex clock registers location and length
+- #clock-cells : Should be 1.
+ In clock consumers, this cell represents the clock ID exposed by the
+ ccplex clock module. The assignments may be found in header file
+ <dt-bindings/clock/tegra132-ccplex.h>.
+
+Example SoC include file:
+
+/ {
+ ccplex-clock@0,70040000 {
+ compatible = "nvidia,tegra132-ccplex-clk";
+ reg = <0x0 0x70040000 0x0 0x1000>;
+ status = "okay";
+ #clock-cells = <1>;
+ };
+
+};
diff --git a/include/dt-bindings/clock/tegra132-ccplex.h b/include/dt-bindings/clock/tegra132-ccplex.h
new file mode 100644
index 0000000..58ea190
--- /dev/null
+++ b/include/dt-bindings/clock/tegra132-ccplex.h
@@ -0,0 +1,12 @@
+/*
+ * This header provides constants for the binding nvidia,tegra132-ccplex-clk
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_TEGRA132_CCPLEX_H
+#define __DT_BINDINGS_CLOCK_TEGRA132_CCPLEX_H
+
+#define TEGRA132_CCPLEX_CCLK_G 1
+#define TEGRA132_PLL_X 2
+#define TEGRA132_CCPLEX_CLK_MAX 3
+
+#endif
--
1.7.7.rc0.72.g4b5ea.dirty
Tegra132 CAR supports almost the same clocks as Tegra124 CAR. This patch
deals with the small differences.
--
I'm not entirely sure why the soc_therm clock needs to be enabled on Tegra132,
but turning it off results in a system hang. I presume this might be because
of fastboot initializing soc_therm.
Signed-off-by: Peter De Schrijver <[email protected]>
---
drivers/clk/tegra/clk-tegra124.c | 32 ++++++++++++++++++++++++++++++++
1 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
index 80efe51..b857aab 100644
--- a/drivers/clk/tegra/clk-tegra124.c
+++ b/drivers/clk/tegra/clk-tegra124.c
@@ -1369,6 +1369,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
{TEGRA124_CLK_XUSB_HS_SRC, TEGRA124_CLK_PLL_U_60M, 60000000, 0},
{TEGRA124_CLK_XUSB_FALCON_SRC, TEGRA124_CLK_PLL_RE_OUT, 224000000, 0},
{TEGRA124_CLK_XUSB_HOST_SRC, TEGRA124_CLK_PLL_RE_OUT, 112000000, 0},
+ {TEGRA124_CLK_SOC_THERM, TEGRA124_CLK_PLL_P, 51000000, 0},
/* This MUST be the last entry. */
{TEGRA124_CLK_CLK_MAX, TEGRA124_CLK_CLK_MAX, 0, 0},
};
@@ -1378,9 +1379,25 @@ static void __init tegra124_clock_apply_init_table(void)
tegra_init_from_table(init_table, clks, TEGRA124_CLK_CLK_MAX);
}
+enum {
+ TEGRA124_CLK,
+ TEGRA132_CLK,
+};
+
+static const struct of_device_id tegra_clock_of_match[] = {
+ { .compatible = "nvidia,tegra124-car", .data = (void *)TEGRA124_CLK },
+ { .compatible = "nvidia,tegra132-car", .data = (void *)TEGRA132_CLK },
+ {},
+};
+
static void __init tegra124_clock_init(struct device_node *np)
{
struct device_node *node;
+ const struct of_device_id *match;
+ uintptr_t id;
+
+ match = of_match_node(tegra_clock_of_match, np);
+ id = (uintptr_t)match->data;
clk_base = of_iomap(np, 0);
if (!clk_base) {
@@ -1416,6 +1433,20 @@ static void __init tegra124_clock_init(struct device_node *np)
tegra_audio_clk_init(clk_base, pmc_base, tegra124_clks, &pll_a_params);
tegra_pmc_clk_init(pmc_base, tegra124_clks);
+ if (id == TEGRA132_CLK) {
+ int i;
+
+ tegra124_clks[tegra_clk_cclk_g].present = false;
+ tegra124_clks[tegra_clk_cclk_lp].present = false;
+ tegra124_clks[tegra_clk_pll_x].present = false;
+ tegra124_clks[tegra_clk_pll_x_out0].present = false;
+
+ /* Tegra132 requires the soc_therm clock to be always on */
+ for (i = 0; i < ARRAY_SIZE(init_table); i++) {
+ if (init_table[i].clk_id == TEGRA124_CLK_SOC_THERM)
+ init_table[i].state = 1;
+ }
+ }
tegra_super_clk_gen4_init(clk_base, pmc_base, tegra124_clks,
&pll_x_params);
tegra_add_of_provider(np);
@@ -1426,3 +1457,4 @@ static void __init tegra124_clock_init(struct device_node *np)
tegra_cpu_car_ops = &tegra124_cpu_car_ops;
}
CLK_OF_DECLARE(tegra124, "nvidia,tegra124-car", tegra124_clock_init);
+CLK_OF_DECLARE(tegra132, "nvidia,tegra132-car", tegra124_clock_init);
--
1.7.7.rc0.72.g4b5ea.dirty
Add support for the ccplex clocks in Tegra132.
Signed-off-by: Peter De Schrijver <[email protected]>
---
drivers/clk/tegra/Makefile | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
index f7dfb72..4231865 100644
--- a/drivers/clk/tegra/Makefile
+++ b/drivers/clk/tegra/Makefile
@@ -15,3 +15,5 @@ obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o
obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o
obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o
obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o
+obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.c
+obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra132-ccplex.c
--
1.7.7.rc0.72.g4b5ea.dirty
Tegra132 has almost the same clock structure than Tegra124. This patch
documents the missing clock IDs.
Signed-off-by: Peter De Schrijver <[email protected]>
---
.../bindings/clock/nvidia,tegra124-car.txt | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
index ded5d62..28129a9 100644
--- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
+++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
@@ -1,4 +1,4 @@
-NVIDIA Tegra124 Clock And Reset Controller
+NVIDIA Tegra124 and Tegra132 Clock And Reset Controller
This binding uses the common clock binding:
Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -7,14 +7,16 @@ The CAR (Clock And Reset) Controller on Tegra is the HW module responsible
for muxing and gating Tegra's clocks, and setting their rates.
Required properties :
-- compatible : Should be "nvidia,tegra124-car"
+- compatible : Should be "nvidia,tegra124-car" or "nvidia,tegra132-car"
- reg : Should contain CAR registers location and length
- clocks : Should contain phandle and clock specifiers for two clocks:
the 32 KHz "32k_in", and the board-specific oscillator "osc".
- #clock-cells : Should be 1.
In clock consumers, this cell represents the clock ID exposed by the
CAR. The assignments may be found in header file
- <dt-bindings/clock/tegra124-car.h>.
+ <dt-bindings/clock/tegra124-car.h>. The following clocks do not exist
+ in the nvidia,tegra132-car binding: TEGRA124_CLK_CCLK_G,
+ TEGRA124_CLK_CCLK_LP, TEGRA124_CLK_PLL_X and TEGRA124_CLK_PLL_X_OUT0.
- #reset-cells : Should be 1.
In clock consumers, this cell represents the bit number in the CAR's
array of CLK_RST_CONTROLLER_RST_DEVICES_* registers.
--
1.7.7.rc0.72.g4b5ea.dirty
On 7/15/2014 11:24 AM, Peter De Schrijver wrote:
> Add support for the ccplex clocks in Tegra132.
>
> Signed-off-by: Peter De Schrijver <[email protected]>
> ---
> drivers/clk/tegra/Makefile | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> index f7dfb72..4231865 100644
> --- a/drivers/clk/tegra/Makefile
> +++ b/drivers/clk/tegra/Makefile
> @@ -15,3 +15,5 @@ obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o
> obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o
> obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o
> obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o
> +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.c
> +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra132-ccplex.c
>
This doesn't seem to actually add the clk-tegra132-ccplex.c file.
-rhyland
--
nvpublic
On 7/15/2014 4:35 PM, Rhyland Klein wrote:
> On 7/15/2014 11:24 AM, Peter De Schrijver wrote:
>> Add support for the ccplex clocks in Tegra132.
>>
>> Signed-off-by: Peter De Schrijver <[email protected]>
>> ---
>> drivers/clk/tegra/Makefile | 2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
>> index f7dfb72..4231865 100644
>> --- a/drivers/clk/tegra/Makefile
>> +++ b/drivers/clk/tegra/Makefile
>> @@ -15,3 +15,5 @@ obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o
>> obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o
>> obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o
>> obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o
>> +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.c
>> +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra132-ccplex.c
Also, shouldn't both these be .o not .c ?
-rhyland
>>
>
> This doesn't seem to actually add the clk-tegra132-ccplex.c file.
>
> -rhyland
>
--
nvpublic
On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote:
[...]
> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> index d081732..65cde4e 100644
> --- a/drivers/clk/tegra/clk.c
> +++ b/drivers/clk/tegra/clk.c
> @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id,
>
> tegra_clk_apply_init_table_func tegra_clk_apply_init_table;
>
> -void __init tegra_clocks_apply_init_table(void)
> +static int __init tegra_clocks_apply_init_table(void)
> {
> if (!tegra_clk_apply_init_table)
> - return;
> + return 0;
Shouldn't this be an error? Or perhaps WARN()? To make sure this gets
noticed since it's obviously a mistake. I'm wondering if perhaps we
could simply remove this check and let the kernel crash if it isn't a
valid function pointer. Is there a case where this not being set at
this point is even possible (or valid?). If not, perhaps it would be
better to just call the SoC generation versions of this function from
here directly rather than going through a function pointer?
Thierry
On Tue, Jul 15, 2014 at 06:24:33PM +0300, Peter De Schrijver wrote:
> Tegra132 has almost the same clock structure than Tegra124. This patch
> documents the missing clock IDs.
>
> Signed-off-by: Peter De Schrijver <[email protected]>
> ---
> .../bindings/clock/nvidia,tegra124-car.txt | 8 +++++---
> 1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> index ded5d62..28129a9 100644
> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> @@ -1,4 +1,4 @@
> -NVIDIA Tegra124 Clock And Reset Controller
> +NVIDIA Tegra124 and Tegra132 Clock And Reset Controller
>
> This binding uses the common clock binding:
> Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -7,14 +7,16 @@ The CAR (Clock And Reset) Controller on Tegra is the HW module responsible
> for muxing and gating Tegra's clocks, and setting their rates.
>
> Required properties :
> -- compatible : Should be "nvidia,tegra124-car"
> +- compatible : Should be "nvidia,tegra124-car" or "nvidia,tegra132-car"
> - reg : Should contain CAR registers location and length
> - clocks : Should contain phandle and clock specifiers for two clocks:
> the 32 KHz "32k_in", and the board-specific oscillator "osc".
> - #clock-cells : Should be 1.
> In clock consumers, this cell represents the clock ID exposed by the
> CAR. The assignments may be found in header file
> - <dt-bindings/clock/tegra124-car.h>.
> + <dt-bindings/clock/tegra124-car.h>. The following clocks do not exist
> + in the nvidia,tegra132-car binding: TEGRA124_CLK_CCLK_G,
> + TEGRA124_CLK_CCLK_LP, TEGRA124_CLK_PLL_X and TEGRA124_CLK_PLL_X_OUT0.
Perhaps it would be useful to split up the common clocks and the
Tegra124-only clocks into separate header files and then provide
tegra132-car.h which includes only the ones in common with Tegra124.
In other words: tegra124-car-common.h would have all except the ones
above, then tegra124-car.h includes tegra124-car-common.h and defines
those that are Tegra124-only (the ones you listed above) and
tegra132-car.h can include tegra124-car-common.h.
That's somewhat extreme, but it has the benefit of giving us compile-
time checks as to whether the correct clocks are being used.
Thierry
On Tue, Jul 15, 2014 at 06:24:31PM +0300, Peter De Schrijver wrote:
> Just continue initializing clocks if there's an error on one of them. This
> is useful if there's a mistake in the inittable, because the system could
> hang if clk_disable_unused() disables some of the critical clocks in this
> table.
>
> Signed-off-by: Peter De Schrijver <[email protected]>
> ---
> drivers/clk/tegra/clk.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> index c0a7d77..d081732 100644
> --- a/drivers/clk/tegra/clk.c
> +++ b/drivers/clk/tegra/clk.c
> @@ -207,7 +207,7 @@ void __init tegra_init_from_table(struct tegra_clk_init_table *tbl,
> for (; tbl->clk_id < clk_max; tbl++) {
> clk = clks[tbl->clk_id];
> if (IS_ERR_OR_NULL(clk))
> - return;
> + continue;
Perhaps rather than silently ignoring, should this at least print out an
error? I'd even go as far as make it a full-blown WARN to make sure
people notice and this gets fixed early.
Thierry
On Tue, Jul 15, 2014 at 06:24:34PM +0300, Peter De Schrijver wrote:
> Tegra132 has a few new clocks for the CPU complex (ccplex).
>
> Signed-off-by: Peter De Schrijver <[email protected]>
> ---
> .../bindings/clock/nvidia,tegra132-ccplex-clk.txt | 27 ++++++++++++++++++++
> include/dt-bindings/clock/tegra132-ccplex.h | 12 +++++++++
> 2 files changed, 39 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt
> create mode 100644 include/dt-bindings/clock/tegra132-ccplex.h
>
> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt
> new file mode 100644
> index 0000000..1441e36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt
> @@ -0,0 +1,27 @@
> +NVIDIA Tegra132 ccplex clocks
Perhaps: "NVIDIA Tegra132 CPU complex (ccplex) clocks". Otherwise this
file doesn't expand the ccplex abbreviation at all and it may not be
obvious to everyone.
> +This binding uses the common clock binding:
> +Documentation/devicetree/bindings/clock/clock-bindings.txt
clock-bindings.txt is in the same directory as this file, so a slightly
better wording would be:
This binding uses the common clock binding as described in the
clock-bindings.txt file in this directory.
Note that these absolute paths may not remain the same when the device
tree bindings are moved out of the kernel tree.
> +The Tegra132 ccplex clock module on Tegra132 is the HW module responsible
> +for the ccplex related clocks.
The end repeats the beginning of this sentence, so its information
content is rather low. Maybe the last part of this sentence could be:
"... responsible for the CPU and related clocks".
> diff --git a/include/dt-bindings/clock/tegra132-ccplex.h b/include/dt-bindings/clock/tegra132-ccplex.h
[...]
> +#define TEGRA132_CCPLEX_CCLK_G 1
> +#define TEGRA132_PLL_X 2
> +#define TEGRA132_CCPLEX_CLK_MAX 3
Maybe these should use TEGRA132_CLK_ as prefix for consistency with the
CAR binding?
Thierry
On Tue, Jul 15, 2014 at 06:24:35PM +0300, Peter De Schrijver wrote:
> Tegra132 CAR supports almost the same clocks as Tegra124 CAR. This patch
> deals with the small differences.
>
> --
> I'm not entirely sure why the soc_therm clock needs to be enabled on Tegra132,
> but turning it off results in a system hang. I presume this might be because
> of fastboot initializing soc_therm.
>
> Signed-off-by: Peter De Schrijver <[email protected]>
> ---
> drivers/clk/tegra/clk-tegra124.c | 32 ++++++++++++++++++++++++++++++++
> 1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
> index 80efe51..b857aab 100644
> --- a/drivers/clk/tegra/clk-tegra124.c
> +++ b/drivers/clk/tegra/clk-tegra124.c
> @@ -1369,6 +1369,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
> {TEGRA124_CLK_XUSB_HS_SRC, TEGRA124_CLK_PLL_U_60M, 60000000, 0},
> {TEGRA124_CLK_XUSB_FALCON_SRC, TEGRA124_CLK_PLL_RE_OUT, 224000000, 0},
> {TEGRA124_CLK_XUSB_HOST_SRC, TEGRA124_CLK_PLL_RE_OUT, 112000000, 0},
> + {TEGRA124_CLK_SOC_THERM, TEGRA124_CLK_PLL_P, 51000000, 0},
> /* This MUST be the last entry. */
> {TEGRA124_CLK_CLK_MAX, TEGRA124_CLK_CLK_MAX, 0, 0},
> };
> @@ -1378,9 +1379,25 @@ static void __init tegra124_clock_apply_init_table(void)
> tegra_init_from_table(init_table, clks, TEGRA124_CLK_CLK_MAX);
> }
>
> +enum {
> + TEGRA124_CLK,
> + TEGRA132_CLK,
> +};
I'd prefer this to be something like:
struct tegra_car_soc {
bool has_ccplex_clk;
};
static const struct tegra_car_soc tegra124_car_soc = {
.has_ccplex_clk = false,
};
static const struct tegra_car_soc tegra132_car_soc = {
.has_ccplex_clk = true,
};
> +static const struct of_device_id tegra_clock_of_match[] = {
> + { .compatible = "nvidia,tegra124-car", .data = (void *)TEGRA124_CLK },
.data = &tegra124_car_soc,
> + { .compatible = "nvidia,tegra132-car", .data = (void *)TEGRA132_CLK },
.data = &tegra132_car_soc,
> static void __init tegra124_clock_init(struct device_node *np)
> {
> struct device_node *node;
> + const struct of_device_id *match;
const struct tegra_car_soc *soc;
> + uintptr_t id;
> + match = of_match_node(tegra_clock_of_match, np);
> + id = (uintptr_t)match->data;
soc = match->data;
>
> clk_base = of_iomap(np, 0);
> if (!clk_base) {
> @@ -1416,6 +1433,20 @@ static void __init tegra124_clock_init(struct device_node *np)
> tegra_audio_clk_init(clk_base, pmc_base, tegra124_clks, &pll_a_params);
> tegra_pmc_clk_init(pmc_base, tegra124_clks);
>
> + if (id == TEGRA132_CLK) {
if (soc->has_ccplex_clk) {
That's somewhat more explicit and avoids a lot of ugly casting.
> + int i;
> +
> + tegra124_clks[tegra_clk_cclk_g].present = false;
> + tegra124_clks[tegra_clk_cclk_lp].present = false;
> + tegra124_clks[tegra_clk_pll_x].present = false;
> + tegra124_clks[tegra_clk_pll_x_out0].present = false;
> +
> + /* Tegra132 requires the soc_therm clock to be always on */
> + for (i = 0; i < ARRAY_SIZE(init_table); i++) {
> + if (init_table[i].clk_id == TEGRA124_CLK_SOC_THERM)
> + init_table[i].state = 1;
I wonder if we could do this someplace else. If we could, then we'd have
the opportunity to make the init_table const.
> + }
> + }
> tegra_super_clk_gen4_init(clk_base, pmc_base, tegra124_clks,
Could use a blank line after the closing } above.
Thierry
On Wed, Jul 16, 2014 at 09:19:33AM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote:
> [...]
> > diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> > index d081732..65cde4e 100644
> > --- a/drivers/clk/tegra/clk.c
> > +++ b/drivers/clk/tegra/clk.c
> > @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id,
> >
> > tegra_clk_apply_init_table_func tegra_clk_apply_init_table;
> >
> > -void __init tegra_clocks_apply_init_table(void)
> > +static int __init tegra_clocks_apply_init_table(void)
> > {
> > if (!tegra_clk_apply_init_table)
> > - return;
> > + return 0;
>
> Shouldn't this be an error? Or perhaps WARN()? To make sure this gets
An arch_initcall will be called for every ARM platform I think? In case
this gets called on a non-Tegra platform, tegra_clk_apply_init_table will not
be set and therefore a silent return 0; seems the most appropriate thing to do
to me?
Cheers,
Peter.
On Tue, Jul 15, 2014 at 10:35:24PM +0200, Rhyland Klein wrote:
> On 7/15/2014 11:24 AM, Peter De Schrijver wrote:
> > Add support for the ccplex clocks in Tegra132.
> >
> > Signed-off-by: Peter De Schrijver <[email protected]>
> > ---
> > drivers/clk/tegra/Makefile | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> > index f7dfb72..4231865 100644
> > --- a/drivers/clk/tegra/Makefile
> > +++ b/drivers/clk/tegra/Makefile
> > @@ -15,3 +15,5 @@ obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o
> > obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o
> > obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o
> > obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o
> > +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.c
> > +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra132-ccplex.c
> >
>
> This doesn't seem to actually add the clk-tegra132-ccplex.c file.
>
Indeed... how did that happen...
On Tue, Jul 15, 2014 at 10:40:17PM +0200, Rhyland Klein wrote:
> On 7/15/2014 4:35 PM, Rhyland Klein wrote:
> > On 7/15/2014 11:24 AM, Peter De Schrijver wrote:
> >> Add support for the ccplex clocks in Tegra132.
> >>
> >> Signed-off-by: Peter De Schrijver <[email protected]>
> >> ---
> >> drivers/clk/tegra/Makefile | 2 ++
> >> 1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> >> index f7dfb72..4231865 100644
> >> --- a/drivers/clk/tegra/Makefile
> >> +++ b/drivers/clk/tegra/Makefile
> >> @@ -15,3 +15,5 @@ obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o
> >> obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o
> >> obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o
> >> obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o
> >> +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra124.c
> >> +obj-$(CONFIG_ARCH_TEGRA_132_SOC) += clk-tegra132-ccplex.c
>
> Also, shouldn't both these be .o not .c ?
>
> -rhyland
Yes, obviously :)
Cheers,
Peter.
On Wed, Jul 16, 2014 at 09:44:10AM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 15, 2014 at 06:24:35PM +0300, Peter De Schrijver wrote:
> > Tegra132 CAR supports almost the same clocks as Tegra124 CAR. This patch
> > deals with the small differences.
> >
> > --
> > I'm not entirely sure why the soc_therm clock needs to be enabled on Tegra132,
> > but turning it off results in a system hang. I presume this might be because
> > of fastboot initializing soc_therm.
> >
> > Signed-off-by: Peter De Schrijver <[email protected]>
> > ---
> > drivers/clk/tegra/clk-tegra124.c | 32 ++++++++++++++++++++++++++++++++
> > 1 files changed, 32 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
> > index 80efe51..b857aab 100644
> > --- a/drivers/clk/tegra/clk-tegra124.c
> > +++ b/drivers/clk/tegra/clk-tegra124.c
> > @@ -1369,6 +1369,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
> > {TEGRA124_CLK_XUSB_HS_SRC, TEGRA124_CLK_PLL_U_60M, 60000000, 0},
> > {TEGRA124_CLK_XUSB_FALCON_SRC, TEGRA124_CLK_PLL_RE_OUT, 224000000, 0},
> > {TEGRA124_CLK_XUSB_HOST_SRC, TEGRA124_CLK_PLL_RE_OUT, 112000000, 0},
> > + {TEGRA124_CLK_SOC_THERM, TEGRA124_CLK_PLL_P, 51000000, 0},
> > /* This MUST be the last entry. */
> > {TEGRA124_CLK_CLK_MAX, TEGRA124_CLK_CLK_MAX, 0, 0},
> > };
> > @@ -1378,9 +1379,25 @@ static void __init tegra124_clock_apply_init_table(void)
> > tegra_init_from_table(init_table, clks, TEGRA124_CLK_CLK_MAX);
> > }
> >
> > +enum {
> > + TEGRA124_CLK,
> > + TEGRA132_CLK,
> > +};
>
> I'd prefer this to be something like:
>
> struct tegra_car_soc {
> bool has_ccplex_clk;
> };
>
> static const struct tegra_car_soc tegra124_car_soc = {
> .has_ccplex_clk = false,
> };
>
> static const struct tegra_car_soc tegra132_car_soc = {
> .has_ccplex_clk = true,
> };
>
> > +static const struct of_device_id tegra_clock_of_match[] = {
> > + { .compatible = "nvidia,tegra124-car", .data = (void *)TEGRA124_CLK },
>
> .data = &tegra124_car_soc,
>
> > + { .compatible = "nvidia,tegra132-car", .data = (void *)TEGRA132_CLK },
>
> .data = &tegra132_car_soc,
>
> > static void __init tegra124_clock_init(struct device_node *np)
> > {
> > struct device_node *node;
> > + const struct of_device_id *match;
>
> const struct tegra_car_soc *soc;
>
> > + uintptr_t id;
>
> > + match = of_match_node(tegra_clock_of_match, np);
> > + id = (uintptr_t)match->data;
>
> soc = match->data;
>
> >
> > clk_base = of_iomap(np, 0);
> > if (!clk_base) {
> > @@ -1416,6 +1433,20 @@ static void __init tegra124_clock_init(struct device_node *np)
> > tegra_audio_clk_init(clk_base, pmc_base, tegra124_clks, &pll_a_params);
> > tegra_pmc_clk_init(pmc_base, tegra124_clks);
> >
> > + if (id == TEGRA132_CLK) {
>
> if (soc->has_ccplex_clk) {
>
> That's somewhat more explicit and avoids a lot of ugly casting.
>
It also adds another struct + pointers to essentially store 1 bit. Which is why
I decided to go this route.
> > + int i;
> > +
> > + tegra124_clks[tegra_clk_cclk_g].present = false;
> > + tegra124_clks[tegra_clk_cclk_lp].present = false;
> > + tegra124_clks[tegra_clk_pll_x].present = false;
> > + tegra124_clks[tegra_clk_pll_x_out0].present = false;
> > +
> > + /* Tegra132 requires the soc_therm clock to be always on */
> > + for (i = 0; i < ARRAY_SIZE(init_table); i++) {
> > + if (init_table[i].clk_id == TEGRA124_CLK_SOC_THERM)
> > + init_table[i].state = 1;
>
> I wonder if we could do this someplace else. If we could, then we'd have
> the opportunity to make the init_table const.
>
The easiest solution would be to turn on soc_therm for Tegra124 and Tegra132.
I don't think this would cause a measureable increase in power consumption.
If you're ok with this, this logic could just be removed. Another solution
would be to do an explicit clk_enable. PLL_P is already enabled so enabling
this clock, does not require PLL locking and could be done before udelay()
is available.
Cheers,
Peter.
On Wed, Jul 16, 2014 at 09:25:40AM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 15, 2014 at 06:24:33PM +0300, Peter De Schrijver wrote:
> > Tegra132 has almost the same clock structure than Tegra124. This patch
> > documents the missing clock IDs.
> >
> > Signed-off-by: Peter De Schrijver <[email protected]>
> > ---
> > .../bindings/clock/nvidia,tegra124-car.txt | 8 +++++---
> > 1 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> > index ded5d62..28129a9 100644
> > --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> > +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-car.txt
> > @@ -1,4 +1,4 @@
> > -NVIDIA Tegra124 Clock And Reset Controller
> > +NVIDIA Tegra124 and Tegra132 Clock And Reset Controller
> >
> > This binding uses the common clock binding:
> > Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -7,14 +7,16 @@ The CAR (Clock And Reset) Controller on Tegra is the HW module responsible
> > for muxing and gating Tegra's clocks, and setting their rates.
> >
> > Required properties :
> > -- compatible : Should be "nvidia,tegra124-car"
> > +- compatible : Should be "nvidia,tegra124-car" or "nvidia,tegra132-car"
> > - reg : Should contain CAR registers location and length
> > - clocks : Should contain phandle and clock specifiers for two clocks:
> > the 32 KHz "32k_in", and the board-specific oscillator "osc".
> > - #clock-cells : Should be 1.
> > In clock consumers, this cell represents the clock ID exposed by the
> > CAR. The assignments may be found in header file
> > - <dt-bindings/clock/tegra124-car.h>.
> > + <dt-bindings/clock/tegra124-car.h>. The following clocks do not exist
> > + in the nvidia,tegra132-car binding: TEGRA124_CLK_CCLK_G,
> > + TEGRA124_CLK_CCLK_LP, TEGRA124_CLK_PLL_X and TEGRA124_CLK_PLL_X_OUT0.
>
> Perhaps it would be useful to split up the common clocks and the
> Tegra124-only clocks into separate header files and then provide
> tegra132-car.h which includes only the ones in common with Tegra124.
>
> In other words: tegra124-car-common.h would have all except the ones
> above, then tegra124-car.h includes tegra124-car-common.h and defines
> those that are Tegra124-only (the ones you listed above) and
> tegra132-car.h can include tegra124-car-common.h.
>
> That's somewhat extreme, but it has the benefit of giving us compile-
> time checks as to whether the correct clocks are being used.
>
Yes. That seems like a good plan.
Cheers,
Peter.
On 07/16/2014 02:27 AM, Peter De Schrijver wrote:
> On Wed, Jul 16, 2014 at 09:19:33AM +0200, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote:
>> [...]
>>> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
>>> index d081732..65cde4e 100644
>>> --- a/drivers/clk/tegra/clk.c
>>> +++ b/drivers/clk/tegra/clk.c
>>> @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id,
>>>
>>> tegra_clk_apply_init_table_func tegra_clk_apply_init_table;
>>>
>>> -void __init tegra_clocks_apply_init_table(void)
>>> +static int __init tegra_clocks_apply_init_table(void)
>>> {
>>> if (!tegra_clk_apply_init_table)
>>> - return;
>>> + return 0;
>>
>> Shouldn't this be an error? Or perhaps WARN()? To make sure this gets
>
> An arch_initcall will be called for every ARM platform I think? In case
> this gets called on a non-Tegra platform, tegra_clk_apply_init_table will not
> be set and therefore a silent return 0; seems the most appropriate thing to do
> to me?
This is one reason that doing all the initialization from separate
initcalls sucks. Much better to have a single top-level initialization
function that calls exactly what is needed, only what is needed, and
only runs on the correct SoCs.
But failing that, I guess you need to say something like
of_is_compatible(root node, "nvidia Tegra"), but of course the
definition of "nvidia Tegra" is an ever-growing list of possible values
that needs to be used from each separate initcall...
On Mon, Jul 21, 2014 at 03:43:08PM -0600, Stephen Warren wrote:
> On 07/16/2014 02:27 AM, Peter De Schrijver wrote:
> > On Wed, Jul 16, 2014 at 09:19:33AM +0200, Thierry Reding wrote:
> >> * PGP Signed by an unknown key
> >>
> >> On Tue, Jul 15, 2014 at 06:24:32PM +0300, Peter De Schrijver wrote:
> >> [...]
> >>> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> >>> index d081732..65cde4e 100644
> >>> --- a/drivers/clk/tegra/clk.c
> >>> +++ b/drivers/clk/tegra/clk.c
> >>> @@ -290,10 +290,13 @@ struct clk ** __init tegra_lookup_dt_id(int clk_id,
> >>>
> >>> tegra_clk_apply_init_table_func tegra_clk_apply_init_table;
> >>>
> >>> -void __init tegra_clocks_apply_init_table(void)
> >>> +static int __init tegra_clocks_apply_init_table(void)
> >>> {
> >>> if (!tegra_clk_apply_init_table)
> >>> - return;
> >>> + return 0;
> >>
> >> Shouldn't this be an error? Or perhaps WARN()? To make sure this gets
> >
> > An arch_initcall will be called for every ARM platform I think? In case
> > this gets called on a non-Tegra platform, tegra_clk_apply_init_table will not
> > be set and therefore a silent return 0; seems the most appropriate thing to do
> > to me?
>
> This is one reason that doing all the initialization from separate
> initcalls sucks. Much better to have a single top-level initialization
> function that calls exactly what is needed, only what is needed, and
> only runs on the correct SoCs.
>
> But failing that, I guess you need to say something like
> of_is_compatible(root node, "nvidia Tegra"), but of course the
> definition of "nvidia Tegra" is an ever-growing list of possible values
> that needs to be used from each separate initcall...
FWIW, we have soc_is_tegra() now in include/soc/tegra/common.h which is
meant to be used for exactly this purpose. I agree that it isn't optimal
but it's pretty good. It should be easy to refactor this to make it
callable from a top-level initialization function when a decision
regarding that has been made.
Thierry
On 07/15/2014 09:24 AM, Peter De Schrijver wrote:
> tegra_clocks_apply_init_table needs to be called after the udelay loop has
> been calibrated (see 441f199a37cfd66c5dd8dd45490bd3ea6971117d why that is).
Instead of just the commit ID, can you please mention the commit subject
too:
441f199a37cf "clk: tegra: defer application of init table"
> On existing Tegra SoCs this was done by calling tegra_clocks_apply_init_table
> from tegra_dt_init. To make this also work on ARM64, we need to
> change this into an initcall. tegra_dt_init is called from customize_machine
> which is an arch_initcall. Therefore this should also work on existing 32bit
> Tegra SoCs.
I still strongly dislike performing this basic initialization from
random separate initcalls. I think we should create a single initcall
for all the Tegra initialization, even if it isn't able to be a machine
descriptor hook function any more.
That said, discussions re: that are ongoing in other threads, so it's no
worth reworking this patch yet.
On 07/15/2014 09:24 AM, Peter De Schrijver wrote:
> Just continue initializing clocks if there's an error on one of them. This
> is useful if there's a mistake in the inittable, because the system could
> hang if clk_disable_unused() disables some of the critical clocks in this
> table.
If there's a problem in the init table, we should simply fix it instead
of working around it.
At the very least, we need to WARN on this rather than just ignoring
problems.
On 07/15/2014 09:24 AM, Peter De Schrijver wrote:
> Tegra132 has a few new clocks for the CPU complex (ccplex).
> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra132-ccplex-clk.txt
> +Example SoC include file:
> +
> +/ {
> + ccplex-clock@0,70040000 {
> + compatible = "nvidia,tegra132-ccplex-clk";
> + reg = <0x0 0x70040000 0x0 0x1000>;
> + status = "okay";
That's the default, so it's not worth including that property.