I ran across a problem trying to get Linux running on an Ingenic X1000 SoC:
since the memory clock isn't referenced by any driver, it appears unused and
gets disabled automatically. After that, the system hangs on any RAM access.
There is a hack in board-ingenic.c to forcibly enable the CPU clock, but this
is insufficient for the X1000 since the memory clock has its own gate and mux
that isn't tied to the CPU.
This patch series fixes the bug by adding CLK_IS_CRITICAL flags to important
clocks, which seems to be the approach used in many other SoC clock drivers.
v2: Add comments to patch 02 to explain why we need CLK_IS_CRITICAL.
Aidan MacDonald (3):
clk: ingenic: Allow specifying common clock flags
clk: ingenic: Mark critical clocks in Ingenic SoCs
mips: ingenic: Do not manually reference the CPU clock
arch/mips/generic/board-ingenic.c | 26 --------------------------
drivers/clk/ingenic/cgu.c | 2 +-
drivers/clk/ingenic/cgu.h | 3 +++
drivers/clk/ingenic/jz4725b-cgu.c | 10 ++++++++++
drivers/clk/ingenic/jz4740-cgu.c | 10 ++++++++++
drivers/clk/ingenic/jz4760-cgu.c | 10 ++++++++++
drivers/clk/ingenic/jz4770-cgu.c | 5 +++++
drivers/clk/ingenic/jz4780-cgu.c | 15 +++++++++++++++
drivers/clk/ingenic/x1000-cgu.c | 15 +++++++++++++++
drivers/clk/ingenic/x1830-cgu.c | 11 +++++++++++
10 files changed, 80 insertions(+), 27 deletions(-)
--
2.35.1
Consider CPU, L2 cache, and memory clocks as critical to prevent
them -- and the parent clocks -- from being automatically gated,
since nothing calls clk_get() on these clocks.
Gating the CPU clock hangs the processor, and gating memory makes
external DRAM inaccessible. Normal kernel code can't hope to deal
with either situation so those clocks have to be critical.
The L2 cache is required only if caches are running, and could be
gated if the kernel takes care to flush and disable caches before
gating the clock. There's no mechanism to do this, and probably no
reason to do it, so it's simpler to mark the L2 cache as critical.
Signed-off-by: Aidan MacDonald <[email protected]>
Reviewed-by: Paul Cercueil <[email protected]>
---
drivers/clk/ingenic/jz4725b-cgu.c | 10 ++++++++++
drivers/clk/ingenic/jz4740-cgu.c | 10 ++++++++++
drivers/clk/ingenic/jz4760-cgu.c | 10 ++++++++++
drivers/clk/ingenic/jz4770-cgu.c | 5 +++++
drivers/clk/ingenic/jz4780-cgu.c | 15 +++++++++++++++
drivers/clk/ingenic/x1000-cgu.c | 15 +++++++++++++++
drivers/clk/ingenic/x1830-cgu.c | 11 +++++++++++
7 files changed, 76 insertions(+)
diff --git a/drivers/clk/ingenic/jz4725b-cgu.c b/drivers/clk/ingenic/jz4725b-cgu.c
index 15d61793f53b..590e9c85cb25 100644
--- a/drivers/clk/ingenic/jz4725b-cgu.c
+++ b/drivers/clk/ingenic/jz4725b-cgu.c
@@ -87,6 +87,11 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = {
[JZ4725B_CLK_CCLK] = {
"cclk", CGU_CLK_DIV,
+ /*
+ * Disabling the CPU clock or any parent clocks will hang the
+ * system; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4725B_CLK_PLL, -1, -1, -1 },
.div = {
CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1, 0,
@@ -114,6 +119,11 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = {
[JZ4725B_CLK_MCLK] = {
"mclk", CGU_CLK_DIV,
+ /*
+ * Disabling MCLK or its parents will render DRAM
+ * inaccessible; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4725B_CLK_PLL, -1, -1, -1 },
.div = {
CGU_REG_CPCCR, 12, 1, 4, 22, -1, -1, 0,
diff --git a/drivers/clk/ingenic/jz4740-cgu.c b/drivers/clk/ingenic/jz4740-cgu.c
index 43ffb62c42bb..3e0a30574ebb 100644
--- a/drivers/clk/ingenic/jz4740-cgu.c
+++ b/drivers/clk/ingenic/jz4740-cgu.c
@@ -102,6 +102,11 @@ static const struct ingenic_cgu_clk_info jz4740_cgu_clocks[] = {
[JZ4740_CLK_CCLK] = {
"cclk", CGU_CLK_DIV,
+ /*
+ * Disabling the CPU clock or any parent clocks will hang the
+ * system; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4740_CLK_PLL, -1, -1, -1 },
.div = {
CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1, 0,
@@ -129,6 +134,11 @@ static const struct ingenic_cgu_clk_info jz4740_cgu_clocks[] = {
[JZ4740_CLK_MCLK] = {
"mclk", CGU_CLK_DIV,
+ /*
+ * Disabling MCLK or its parents will render DRAM
+ * inaccessible; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4740_CLK_PLL, -1, -1, -1 },
.div = {
CGU_REG_CPCCR, 12, 1, 4, 22, -1, -1, 0,
diff --git a/drivers/clk/ingenic/jz4760-cgu.c b/drivers/clk/ingenic/jz4760-cgu.c
index 8fdd383560fb..ecd395ac8a28 100644
--- a/drivers/clk/ingenic/jz4760-cgu.c
+++ b/drivers/clk/ingenic/jz4760-cgu.c
@@ -143,6 +143,11 @@ static const struct ingenic_cgu_clk_info jz4760_cgu_clocks[] = {
[JZ4760_CLK_CCLK] = {
"cclk", CGU_CLK_DIV,
+ /*
+ * Disabling the CPU clock or any parent clocks will hang the
+ * system; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4760_CLK_PLL0, },
.div = {
CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1, 0,
@@ -175,6 +180,11 @@ static const struct ingenic_cgu_clk_info jz4760_cgu_clocks[] = {
},
[JZ4760_CLK_MCLK] = {
"mclk", CGU_CLK_DIV,
+ /*
+ * Disabling MCLK or its parents will render DRAM
+ * inaccessible; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4760_CLK_PLL0, },
.div = {
CGU_REG_CPCCR, 12, 1, 4, 22, -1, -1, 0,
diff --git a/drivers/clk/ingenic/jz4770-cgu.c b/drivers/clk/ingenic/jz4770-cgu.c
index 7ef91257630e..6ae1740367f9 100644
--- a/drivers/clk/ingenic/jz4770-cgu.c
+++ b/drivers/clk/ingenic/jz4770-cgu.c
@@ -149,6 +149,11 @@ static const struct ingenic_cgu_clk_info jz4770_cgu_clocks[] = {
[JZ4770_CLK_CCLK] = {
"cclk", CGU_CLK_DIV,
+ /*
+ * Disabling the CPU clock or any parent clocks will hang the
+ * system; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4770_CLK_PLL0, },
.div = {
CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1, 0,
diff --git a/drivers/clk/ingenic/jz4780-cgu.c b/drivers/clk/ingenic/jz4780-cgu.c
index e357c228e0f1..b1dadc0a5e75 100644
--- a/drivers/clk/ingenic/jz4780-cgu.c
+++ b/drivers/clk/ingenic/jz4780-cgu.c
@@ -341,12 +341,22 @@ static const struct ingenic_cgu_clk_info jz4780_cgu_clocks[] = {
[JZ4780_CLK_CPU] = {
"cpu", CGU_CLK_DIV,
+ /*
+ * Disabling the CPU clock or any parent clocks will hang the
+ * system; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4780_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CLOCKCONTROL, 0, 1, 4, 22, -1, -1 },
},
[JZ4780_CLK_L2CACHE] = {
"l2cache", CGU_CLK_DIV,
+ /*
+ * The L2 cache clock is critical if caches are enabled and
+ * disabling it or any parent clocks will hang the system.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4780_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CLOCKCONTROL, 4, 1, 4, -1, -1, -1 },
},
@@ -380,6 +390,11 @@ static const struct ingenic_cgu_clk_info jz4780_cgu_clocks[] = {
[JZ4780_CLK_DDR] = {
"ddr", CGU_CLK_MUX | CGU_CLK_DIV,
+ /*
+ * Disabling DDR clock or its parents will render DRAM
+ * inaccessible; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { -1, JZ4780_CLK_SCLKA, JZ4780_CLK_MPLL, -1 },
.mux = { CGU_REG_DDRCDR, 30, 2 },
.div = { CGU_REG_DDRCDR, 0, 1, 4, 29, 28, 27 },
diff --git a/drivers/clk/ingenic/x1000-cgu.c b/drivers/clk/ingenic/x1000-cgu.c
index 3c4d5a77ccbd..b2ce3fb83f54 100644
--- a/drivers/clk/ingenic/x1000-cgu.c
+++ b/drivers/clk/ingenic/x1000-cgu.c
@@ -251,6 +251,11 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
[X1000_CLK_CPU] = {
"cpu", CGU_CLK_DIV | CGU_CLK_GATE,
+ /*
+ * Disabling the CPU clock or any parent clocks will hang the
+ * system; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { X1000_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1 },
.gate = { CGU_REG_CLKGR, 30 },
@@ -258,6 +263,11 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
[X1000_CLK_L2CACHE] = {
"l2cache", CGU_CLK_DIV,
+ /*
+ * The L2 cache clock is critical if caches are enabled and
+ * disabling it or any parent clocks will hang the system.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { X1000_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CPCCR, 4, 1, 4, 22, -1, -1 },
},
@@ -290,6 +300,11 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
[X1000_CLK_DDR] = {
"ddr", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
+ /*
+ * Disabling DDR clock or its parents will render DRAM
+ * inaccessible; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { -1, X1000_CLK_SCLKA, X1000_CLK_MPLL, -1 },
.mux = { CGU_REG_DDRCDR, 30, 2 },
.div = { CGU_REG_DDRCDR, 0, 1, 4, 29, 28, 27 },
diff --git a/drivers/clk/ingenic/x1830-cgu.c b/drivers/clk/ingenic/x1830-cgu.c
index e01ec2dc7a1a..0fd46e50a513 100644
--- a/drivers/clk/ingenic/x1830-cgu.c
+++ b/drivers/clk/ingenic/x1830-cgu.c
@@ -225,6 +225,7 @@ static const struct ingenic_cgu_clk_info x1830_cgu_clocks[] = {
[X1830_CLK_CPU] = {
"cpu", CGU_CLK_DIV | CGU_CLK_GATE,
+ .flags = CLK_IS_CRITICAL,
.parents = { X1830_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1 },
.gate = { CGU_REG_CLKGR1, 15 },
@@ -232,6 +233,11 @@ static const struct ingenic_cgu_clk_info x1830_cgu_clocks[] = {
[X1830_CLK_L2CACHE] = {
"l2cache", CGU_CLK_DIV,
+ /*
+ * The L2 cache clock is critical if caches are enabled and
+ * disabling it or any parent clocks will hang the system.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { X1830_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CPCCR, 4, 1, 4, 22, -1, -1 },
},
@@ -264,6 +270,11 @@ static const struct ingenic_cgu_clk_info x1830_cgu_clocks[] = {
[X1830_CLK_DDR] = {
"ddr", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
+ /*
+ * Disabling DDR clock or its parents will render DRAM
+ * inaccessible; mark it critical.
+ */
+ .flags = CLK_IS_CRITICAL,
.parents = { -1, X1830_CLK_SCLKA, X1830_CLK_MPLL, -1 },
.mux = { CGU_REG_DDRCDR, 30, 2 },
.div = { CGU_REG_DDRCDR, 0, 1, 4, 29, 28, 27 },
--
2.35.1
It isn't necessary to manually walk the device tree and enable
the CPU clock anymore. The CPU and other necessary clocks are
now flagged as critical in the clock driver, which accomplishes
the same thing in a more declarative fashion.
Signed-off-by: Aidan MacDonald <[email protected]>
Reviewed-by: Paul Cercueil <[email protected]>
---
arch/mips/generic/board-ingenic.c | 26 --------------------------
1 file changed, 26 deletions(-)
diff --git a/arch/mips/generic/board-ingenic.c b/arch/mips/generic/board-ingenic.c
index 3f44f14bdb33..c422bbc890ed 100644
--- a/arch/mips/generic/board-ingenic.c
+++ b/arch/mips/generic/board-ingenic.c
@@ -131,36 +131,10 @@ static const struct platform_suspend_ops ingenic_pm_ops __maybe_unused = {
static int __init ingenic_pm_init(void)
{
- struct device_node *cpu_node;
- struct clk *cpu0_clk;
- int ret;
-
if (boot_cpu_type() == CPU_XBURST) {
if (IS_ENABLED(CONFIG_PM_SLEEP))
suspend_set_ops(&ingenic_pm_ops);
_machine_halt = ingenic_halt;
-
- /*
- * Unconditionally enable the clock for the first CPU.
- * This makes sure that the PLL that feeds the CPU won't be
- * stopped while the kernel is running.
- */
- cpu_node = of_get_cpu_node(0, NULL);
- if (!cpu_node) {
- pr_err("Unable to get CPU node\n");
- } else {
- cpu0_clk = of_clk_get(cpu_node, 0);
- if (IS_ERR(cpu0_clk)) {
- pr_err("Unable to get CPU0 clock\n");
- return PTR_ERR(cpu0_clk);
- }
-
- ret = clk_prepare_enable(cpu0_clk);
- if (ret) {
- pr_err("Unable to enable CPU0 clock\n");
- return ret;
- }
- }
}
return 0;
--
2.35.1
Provide a flags field for clocks under the ingenic-cgu driver,
which can be used to set generic common clock framework flags
on the created clocks. For example, the CLK_IS_CRITICAL flag
is needed for some clocks (such as CPU or memory) to stop them
being automatically disabled.
Signed-off-by: Aidan MacDonald <[email protected]>
Reviewed-by: Paul Cercueil <[email protected]>
---
drivers/clk/ingenic/cgu.c | 2 +-
drivers/clk/ingenic/cgu.h | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index af31633a8862..861c50d6cb24 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -660,7 +660,7 @@ static int ingenic_register_clock(struct ingenic_cgu *cgu, unsigned idx)
ingenic_clk->idx = idx;
clk_init.name = clk_info->name;
- clk_init.flags = 0;
+ clk_init.flags = clk_info->flags;
clk_init.parent_names = parent_names;
caps = clk_info->type;
diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
index bfc2b9c38a41..147b7df0d657 100644
--- a/drivers/clk/ingenic/cgu.h
+++ b/drivers/clk/ingenic/cgu.h
@@ -136,6 +136,7 @@ struct ingenic_cgu_custom_info {
* struct ingenic_cgu_clk_info - information about a clock
* @name: name of the clock
* @type: a bitmask formed from CGU_CLK_* values
+ * @flags: common clock flags to set on this clock
* @parents: an array of the indices of potential parents of this clock
* within the clock_info array of the CGU, or -1 in entries
* which correspond to no valid parent
@@ -161,6 +162,8 @@ struct ingenic_cgu_clk_info {
CGU_CLK_CUSTOM = BIT(7),
} type;
+ unsigned long flags;
+
int parents[4];
union {
--
2.35.1
Hi,
On 2022/4/29 上午12:44, Aidan MacDonald wrote:
> I ran across a problem trying to get Linux running on an Ingenic X1000 SoC:
> since the memory clock isn't referenced by any driver, it appears unused and
> gets disabled automatically. After that, the system hangs on any RAM access.
>
> There is a hack in board-ingenic.c to forcibly enable the CPU clock, but this
> is insufficient for the X1000 since the memory clock has its own gate and mux
> that isn't tied to the CPU.
>
> This patch series fixes the bug by adding CLK_IS_CRITICAL flags to important
> clocks, which seems to be the approach used in many other SoC clock drivers.
>
> v2: Add comments to patch 02 to explain why we need CLK_IS_CRITICAL.
>
> Aidan MacDonald (3):
> clk: ingenic: Allow specifying common clock flags
> clk: ingenic: Mark critical clocks in Ingenic SoCs
> mips: ingenic: Do not manually reference the CPU clock
>
> arch/mips/generic/board-ingenic.c | 26 --------------------------
> drivers/clk/ingenic/cgu.c | 2 +-
> drivers/clk/ingenic/cgu.h | 3 +++
> drivers/clk/ingenic/jz4725b-cgu.c | 10 ++++++++++
> drivers/clk/ingenic/jz4740-cgu.c | 10 ++++++++++
> drivers/clk/ingenic/jz4760-cgu.c | 10 ++++++++++
> drivers/clk/ingenic/jz4770-cgu.c | 5 +++++
> drivers/clk/ingenic/jz4780-cgu.c | 15 +++++++++++++++
> drivers/clk/ingenic/x1000-cgu.c | 15 +++++++++++++++
> drivers/clk/ingenic/x1830-cgu.c | 11 +++++++++++
> 10 files changed, 80 insertions(+), 27 deletions(-)
Tested-by: 周琰杰 (Zhou Yanjie) <[email protected]> # On X1000 and
X1830
Quoting Aidan MacDonald (2022-04-28 09:44:53)
> Consider CPU, L2 cache, and memory clocks as critical to prevent
> them -- and the parent clocks -- from being automatically gated,
> since nothing calls clk_get() on these clocks.
>
> Gating the CPU clock hangs the processor, and gating memory makes
> external DRAM inaccessible. Normal kernel code can't hope to deal
> with either situation so those clocks have to be critical.
>
> The L2 cache is required only if caches are running, and could be
> gated if the kernel takes care to flush and disable caches before
> gating the clock. There's no mechanism to do this, and probably no
> reason to do it, so it's simpler to mark the L2 cache as critical.
>
> Signed-off-by: Aidan MacDonald <[email protected]>
> Reviewed-by: Paul Cercueil <[email protected]>
> ---
Applied to clk-next
Quoting Aidan MacDonald (2022-04-28 09:44:52)
> Provide a flags field for clocks under the ingenic-cgu driver,
> which can be used to set generic common clock framework flags
> on the created clocks. For example, the CLK_IS_CRITICAL flag
> is needed for some clocks (such as CPU or memory) to stop them
> being automatically disabled.
>
> Signed-off-by: Aidan MacDonald <[email protected]>
> Reviewed-by: Paul Cercueil <[email protected]>
> ---
Applied to clk-next
Quoting Aidan MacDonald (2022-04-28 09:44:54)
> It isn't necessary to manually walk the device tree and enable
> the CPU clock anymore. The CPU and other necessary clocks are
> now flagged as critical in the clock driver, which accomplishes
> the same thing in a more declarative fashion.
>
> Signed-off-by: Aidan MacDonald <[email protected]>
> Reviewed-by: Paul Cercueil <[email protected]>
> ---
Applied to clk-next