Resending this series since it appears none of the patches were picked up
the first time around.
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.
Original submission:
https://lore.kernel.org/linux-mips/[email protected]/
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 | 2 ++
drivers/clk/ingenic/jz4740-cgu.c | 2 ++
drivers/clk/ingenic/jz4760-cgu.c | 2 ++
drivers/clk/ingenic/jz4770-cgu.c | 1 +
drivers/clk/ingenic/jz4780-cgu.c | 3 +++
drivers/clk/ingenic/x1000-cgu.c | 3 +++
drivers/clk/ingenic/x1830-cgu.c | 3 +++
10 files changed, 20 insertions(+), 27 deletions(-)
--
2.35.1
Consider the CPU, L2 cache, and memory as critical to ensure they
are not disabled.
Signed-off-by: Aidan MacDonald <[email protected]>
Reviewed-by: Paul Cercueil <[email protected]>
---
drivers/clk/ingenic/jz4725b-cgu.c | 2 ++
drivers/clk/ingenic/jz4740-cgu.c | 2 ++
drivers/clk/ingenic/jz4760-cgu.c | 2 ++
drivers/clk/ingenic/jz4770-cgu.c | 1 +
drivers/clk/ingenic/jz4780-cgu.c | 3 +++
drivers/clk/ingenic/x1000-cgu.c | 3 +++
drivers/clk/ingenic/x1830-cgu.c | 3 +++
7 files changed, 16 insertions(+)
diff --git a/drivers/clk/ingenic/jz4725b-cgu.c b/drivers/clk/ingenic/jz4725b-cgu.c
index 15d61793f53b..c209a170be5d 100644
--- a/drivers/clk/ingenic/jz4725b-cgu.c
+++ b/drivers/clk/ingenic/jz4725b-cgu.c
@@ -87,6 +87,7 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = {
[JZ4725B_CLK_CCLK] = {
"cclk", CGU_CLK_DIV,
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4725B_CLK_PLL, -1, -1, -1 },
.div = {
CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1, 0,
@@ -114,6 +115,7 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = {
[JZ4725B_CLK_MCLK] = {
"mclk", CGU_CLK_DIV,
+ .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..2b6f7a9fcea8 100644
--- a/drivers/clk/ingenic/jz4740-cgu.c
+++ b/drivers/clk/ingenic/jz4740-cgu.c
@@ -102,6 +102,7 @@ static const struct ingenic_cgu_clk_info jz4740_cgu_clocks[] = {
[JZ4740_CLK_CCLK] = {
"cclk", CGU_CLK_DIV,
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4740_CLK_PLL, -1, -1, -1 },
.div = {
CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1, 0,
@@ -129,6 +130,7 @@ static const struct ingenic_cgu_clk_info jz4740_cgu_clocks[] = {
[JZ4740_CLK_MCLK] = {
"mclk", CGU_CLK_DIV,
+ .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..7920df2cee1a 100644
--- a/drivers/clk/ingenic/jz4760-cgu.c
+++ b/drivers/clk/ingenic/jz4760-cgu.c
@@ -143,6 +143,7 @@ static const struct ingenic_cgu_clk_info jz4760_cgu_clocks[] = {
[JZ4760_CLK_CCLK] = {
"cclk", CGU_CLK_DIV,
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4760_CLK_PLL0, },
.div = {
CGU_REG_CPCCR, 0, 1, 4, 22, -1, -1, 0,
@@ -175,6 +176,7 @@ static const struct ingenic_cgu_clk_info jz4760_cgu_clocks[] = {
},
[JZ4760_CLK_MCLK] = {
"mclk", CGU_CLK_DIV,
+ .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..02b266c2a537 100644
--- a/drivers/clk/ingenic/jz4770-cgu.c
+++ b/drivers/clk/ingenic/jz4770-cgu.c
@@ -149,6 +149,7 @@ static const struct ingenic_cgu_clk_info jz4770_cgu_clocks[] = {
[JZ4770_CLK_CCLK] = {
"cclk", CGU_CLK_DIV,
+ .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..014674486038 100644
--- a/drivers/clk/ingenic/jz4780-cgu.c
+++ b/drivers/clk/ingenic/jz4780-cgu.c
@@ -341,12 +341,14 @@ static const struct ingenic_cgu_clk_info jz4780_cgu_clocks[] = {
[JZ4780_CLK_CPU] = {
"cpu", CGU_CLK_DIV,
+ .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,
+ .flags = CLK_IS_CRITICAL,
.parents = { JZ4780_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CLOCKCONTROL, 4, 1, 4, -1, -1, -1 },
},
@@ -380,6 +382,7 @@ static const struct ingenic_cgu_clk_info jz4780_cgu_clocks[] = {
[JZ4780_CLK_DDR] = {
"ddr", CGU_CLK_MUX | CGU_CLK_DIV,
+ .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..1bd421cc2ab3 100644
--- a/drivers/clk/ingenic/x1000-cgu.c
+++ b/drivers/clk/ingenic/x1000-cgu.c
@@ -251,6 +251,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
[X1000_CLK_CPU] = {
"cpu", CGU_CLK_DIV | CGU_CLK_GATE,
+ .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 +259,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
[X1000_CLK_L2CACHE] = {
"l2cache", CGU_CLK_DIV,
+ .flags = CLK_IS_CRITICAL,
.parents = { X1000_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CPCCR, 4, 1, 4, 22, -1, -1 },
},
@@ -290,6 +292,7 @@ static const struct ingenic_cgu_clk_info x1000_cgu_clocks[] = {
[X1000_CLK_DDR] = {
"ddr", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
+ .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..b08e318aa095 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,7 @@ static const struct ingenic_cgu_clk_info x1830_cgu_clocks[] = {
[X1830_CLK_L2CACHE] = {
"l2cache", CGU_CLK_DIV,
+ .flags = CLK_IS_CRITICAL,
.parents = { X1830_CLK_CPUMUX, -1, -1, -1 },
.div = { CGU_REG_CPCCR, 4, 1, 4, 22, -1, -1 },
},
@@ -264,6 +266,7 @@ static const struct ingenic_cgu_clk_info x1830_cgu_clocks[] = {
[X1830_CLK_DDR] = {
"ddr", CGU_CLK_MUX | CGU_CLK_DIV | CGU_CLK_GATE,
+ .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
Quoting Aidan MacDonald (2022-04-11 03:14:40)
> Consider the CPU, L2 cache, and memory as critical to ensure they
> are not disabled.
>
> Signed-off-by: Aidan MacDonald <[email protected]>
> Reviewed-by: Paul Cercueil <[email protected]>
> ---
General comment, please add a comment around CLK_IS_CRITICAL usage if it
isn't very clear why such a clk shouldn't be turned off. Second, is
there any point in describing these clks in the kernel and using memory
to do that if they're just going to always be on? Wouldn't a dummy clk
returned from clk_get() work just as well if anything is grabbing a
reference with clk_get()?
On Thu, Apr 21, 2022 at 07:33:57PM -0700, Stephen Boyd wrote:
> Quoting Aidan MacDonald (2022-04-11 03:14:40)
> > Consider the CPU, L2 cache, and memory as critical to ensure they
> > are not disabled.
> >
> > Signed-off-by: Aidan MacDonald <[email protected]>
> > Reviewed-by: Paul Cercueil <[email protected]>
> > ---
>
> General comment, please add a comment around CLK_IS_CRITICAL usage if it
> isn't very clear why such a clk shouldn't be turned off. Second, is
> there any point in describing these clks in the kernel and using memory
> to do that if they're just going to always be on? Wouldn't a dummy clk
> returned from clk_get() work just as well if anything is grabbing a
> reference with clk_get()?
I'd guess they're there to keep track of which PLLs are in use, at least
for SoCs that have more than one PLL. Using a dummy clock sounds like a
bad idea since it won't represent that, and besides the clock configuration
is something that can change at runtime so hardcoding it would be foolish.
I'll send a v2 with explanatory comments around CLK_IS_CRITICAL.
Regards,
Aidan