2022-04-12 20:27:44

by Aidan MacDonald

[permalink] [raw]
Subject: [PATCH v5 0/2] Fix missing TCU clock for X1000/X1830 SoCs

v5: fix use of uninitialized variable reported by kernel test robot & Dan Carpenter
v4: resend of v3 with some minor changes
v3: https://lore.kernel.org/linux-mips/[email protected]/

Aidan MacDonald (2):
mips: dts: ingenic: Add TCU clock to x1000/x1830 tcu device node
clk: ingenic-tcu: Fix missing TCU clock for X1000 SoCs

arch/mips/boot/dts/ingenic/x1000.dtsi | 5 ++--
arch/mips/boot/dts/ingenic/x1830.dtsi | 5 ++--
drivers/clk/ingenic/tcu.c | 35 +++++++++++++++++++--------
3 files changed, 31 insertions(+), 14 deletions(-)

--
2.35.1


2022-04-12 22:50:30

by Aidan MacDonald

[permalink] [raw]
Subject: [PATCH v5 2/2] clk: ingenic-tcu: Fix missing TCU clock for X1000 SoCs

The TCU clock gate on X1000 wasn't requested by the driver and could
be gated automatically later on in boot, which prevents timers from
running and breaks PWM.

Add a workaround to support old device trees that don't specify the
"tcu" clock gate. In this case the kernel will print a warning and
attempt to continue without the clock, which is wrong, but it could
work if "clk_ignore_unused" is in the kernel arguments.

Signed-off-by: Aidan MacDonald <[email protected]>
---
drivers/clk/ingenic/tcu.c | 35 +++++++++++++++++++++++++----------
1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
index 77acfbeb4830..201bf6e6b6e0 100644
--- a/drivers/clk/ingenic/tcu.c
+++ b/drivers/clk/ingenic/tcu.c
@@ -31,6 +31,7 @@ struct ingenic_soc_info {
unsigned int num_channels;
bool has_ost;
bool has_tcu_clk;
+ bool allow_missing_tcu_clk;
};

struct ingenic_tcu_clk_info {
@@ -320,7 +321,8 @@ static const struct ingenic_soc_info jz4770_soc_info = {
static const struct ingenic_soc_info x1000_soc_info = {
.num_channels = 8,
.has_ost = false, /* X1000 has OST, but it not belong TCU */
- .has_tcu_clk = false,
+ .has_tcu_clk = true,
+ .allow_missing_tcu_clk = true,
};

static const struct of_device_id __maybe_unused ingenic_tcu_of_match[] __initconst = {
@@ -355,14 +357,27 @@ static int __init ingenic_tcu_probe(struct device_node *np)
tcu->clk = of_clk_get_by_name(np, "tcu");
if (IS_ERR(tcu->clk)) {
ret = PTR_ERR(tcu->clk);
- pr_crit("Cannot get TCU clock\n");
- goto err_free_tcu;
- }

- ret = clk_prepare_enable(tcu->clk);
- if (ret) {
- pr_crit("Unable to enable TCU clock\n");
- goto err_put_clk;
+ /*
+ * Old device trees for some SoCs did not include the
+ * TCU clock because this driver (incorrectly) didn't
+ * use it. In this case we complain loudly and attempt
+ * to continue without the clock, which might work if
+ * booting with workarounds like "clk_ignore_unused".
+ */
+ if (tcu->soc_info->allow_missing_tcu_clk && ret == -EINVAL) {
+ pr_warn("TCU clock missing from device tree, please update your device tree\n");
+ tcu->clk = NULL;
+ } else {
+ pr_crit("Cannot get TCU clock from device tree\n");
+ goto err_free_tcu;
+ }
+ } else {
+ ret = clk_prepare_enable(tcu->clk);
+ if (ret) {
+ pr_crit("Unable to enable TCU clock\n");
+ goto err_put_clk;
+ }
}
}

@@ -432,10 +447,10 @@ static int __init ingenic_tcu_probe(struct device_node *np)
clk_hw_unregister(tcu->clocks->hws[i]);
kfree(tcu->clocks);
err_clk_disable:
- if (tcu->soc_info->has_tcu_clk)
+ if (tcu->clk)
clk_disable_unprepare(tcu->clk);
err_put_clk:
- if (tcu->soc_info->has_tcu_clk)
+ if (tcu->clk)
clk_put(tcu->clk);
err_free_tcu:
kfree(tcu);
--
2.35.1

2022-04-12 23:24:52

by Aidan MacDonald

[permalink] [raw]
Subject: [PATCH v5 1/2] mips: dts: ingenic: Add TCU clock to x1000/x1830 tcu device node

This clock is a gate for the TCU hardware block on these SoCs, but
it wasn't included in the device tree since the ingenic-tcu driver
erroneously did not request it.

Signed-off-by: Aidan MacDonald <[email protected]>
---
arch/mips/boot/dts/ingenic/x1000.dtsi | 5 +++--
arch/mips/boot/dts/ingenic/x1830.dtsi | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/mips/boot/dts/ingenic/x1000.dtsi b/arch/mips/boot/dts/ingenic/x1000.dtsi
index 8bd27edef216..c69df8eb158e 100644
--- a/arch/mips/boot/dts/ingenic/x1000.dtsi
+++ b/arch/mips/boot/dts/ingenic/x1000.dtsi
@@ -111,8 +111,9 @@ tcu: timer@10002000 {

clocks = <&cgu X1000_CLK_RTCLK>,
<&cgu X1000_CLK_EXCLK>,
- <&cgu X1000_CLK_PCLK>;
- clock-names = "rtc", "ext", "pclk";
+ <&cgu X1000_CLK_PCLK>,
+ <&cgu X1000_CLK_TCU>;
+ clock-names = "rtc", "ext", "pclk", "tcu";

interrupt-controller;
#interrupt-cells = <1>;
diff --git a/arch/mips/boot/dts/ingenic/x1830.dtsi b/arch/mips/boot/dts/ingenic/x1830.dtsi
index 2595df8671c7..4408df24ca98 100644
--- a/arch/mips/boot/dts/ingenic/x1830.dtsi
+++ b/arch/mips/boot/dts/ingenic/x1830.dtsi
@@ -104,8 +104,9 @@ tcu: timer@10002000 {

clocks = <&cgu X1830_CLK_RTCLK>,
<&cgu X1830_CLK_EXCLK>,
- <&cgu X1830_CLK_PCLK>;
- clock-names = "rtc", "ext", "pclk";
+ <&cgu X1830_CLK_PCLK>,
+ <&cgu X1830_CLK_TCU>;
+ clock-names = "rtc", "ext", "pclk", "tcu";

interrupt-controller;
#interrupt-cells = <1>;
--
2.35.1

2022-04-12 23:51:42

by Paul Cercueil

[permalink] [raw]
Subject: Re: [PATCH v5 1/2] mips: dts: ingenic: Add TCU clock to x1000/x1830 tcu device node

Hi,

Le mar., avril 12 2022 at 13:27:49 +0100, Aidan MacDonald
<[email protected]> a ?crit :
> This clock is a gate for the TCU hardware block on these SoCs, but
> it wasn't included in the device tree since the ingenic-tcu driver
> erroneously did not request it.
>
> Signed-off-by: Aidan MacDonald <[email protected]>

Reviewed-by: Paul Cercueil <[email protected]>

Cheers,
-Paul

> ---
> arch/mips/boot/dts/ingenic/x1000.dtsi | 5 +++--
> arch/mips/boot/dts/ingenic/x1830.dtsi | 5 +++--
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/mips/boot/dts/ingenic/x1000.dtsi
> b/arch/mips/boot/dts/ingenic/x1000.dtsi
> index 8bd27edef216..c69df8eb158e 100644
> --- a/arch/mips/boot/dts/ingenic/x1000.dtsi
> +++ b/arch/mips/boot/dts/ingenic/x1000.dtsi
> @@ -111,8 +111,9 @@ tcu: timer@10002000 {
>
> clocks = <&cgu X1000_CLK_RTCLK>,
> <&cgu X1000_CLK_EXCLK>,
> - <&cgu X1000_CLK_PCLK>;
> - clock-names = "rtc", "ext", "pclk";
> + <&cgu X1000_CLK_PCLK>,
> + <&cgu X1000_CLK_TCU>;
> + clock-names = "rtc", "ext", "pclk", "tcu";
>
> interrupt-controller;
> #interrupt-cells = <1>;
> diff --git a/arch/mips/boot/dts/ingenic/x1830.dtsi
> b/arch/mips/boot/dts/ingenic/x1830.dtsi
> index 2595df8671c7..4408df24ca98 100644
> --- a/arch/mips/boot/dts/ingenic/x1830.dtsi
> +++ b/arch/mips/boot/dts/ingenic/x1830.dtsi
> @@ -104,8 +104,9 @@ tcu: timer@10002000 {
>
> clocks = <&cgu X1830_CLK_RTCLK>,
> <&cgu X1830_CLK_EXCLK>,
> - <&cgu X1830_CLK_PCLK>;
> - clock-names = "rtc", "ext", "pclk";
> + <&cgu X1830_CLK_PCLK>,
> + <&cgu X1830_CLK_TCU>;
> + clock-names = "rtc", "ext", "pclk", "tcu";
>
> interrupt-controller;
> #interrupt-cells = <1>;
> --
> 2.35.1
>


2022-04-13 01:25:51

by Paul Cercueil

[permalink] [raw]
Subject: Re: [PATCH v5 2/2] clk: ingenic-tcu: Fix missing TCU clock for X1000 SoCs

Hi,

Le mar., avril 12 2022 at 13:27:50 +0100, Aidan MacDonald
<[email protected]> a ?crit :
> The TCU clock gate on X1000 wasn't requested by the driver and could
> be gated automatically later on in boot, which prevents timers from
> running and breaks PWM.
>
> Add a workaround to support old device trees that don't specify the
> "tcu" clock gate. In this case the kernel will print a warning and
> attempt to continue without the clock, which is wrong, but it could
> work if "clk_ignore_unused" is in the kernel arguments.
>
> Signed-off-by: Aidan MacDonald <[email protected]>

Reviewed-by: Paul Cercueil <[email protected]>

Cheers,
-Paul

> ---
> drivers/clk/ingenic/tcu.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
> index 77acfbeb4830..201bf6e6b6e0 100644
> --- a/drivers/clk/ingenic/tcu.c
> +++ b/drivers/clk/ingenic/tcu.c
> @@ -31,6 +31,7 @@ struct ingenic_soc_info {
> unsigned int num_channels;
> bool has_ost;
> bool has_tcu_clk;
> + bool allow_missing_tcu_clk;
> };
>
> struct ingenic_tcu_clk_info {
> @@ -320,7 +321,8 @@ static const struct ingenic_soc_info
> jz4770_soc_info = {
> static const struct ingenic_soc_info x1000_soc_info = {
> .num_channels = 8,
> .has_ost = false, /* X1000 has OST, but it not belong TCU */
> - .has_tcu_clk = false,
> + .has_tcu_clk = true,
> + .allow_missing_tcu_clk = true,
> };
>
> static const struct of_device_id __maybe_unused
> ingenic_tcu_of_match[] __initconst = {
> @@ -355,14 +357,27 @@ static int __init ingenic_tcu_probe(struct
> device_node *np)
> tcu->clk = of_clk_get_by_name(np, "tcu");
> if (IS_ERR(tcu->clk)) {
> ret = PTR_ERR(tcu->clk);
> - pr_crit("Cannot get TCU clock\n");
> - goto err_free_tcu;
> - }
>
> - ret = clk_prepare_enable(tcu->clk);
> - if (ret) {
> - pr_crit("Unable to enable TCU clock\n");
> - goto err_put_clk;
> + /*
> + * Old device trees for some SoCs did not include the
> + * TCU clock because this driver (incorrectly) didn't
> + * use it. In this case we complain loudly and attempt
> + * to continue without the clock, which might work if
> + * booting with workarounds like "clk_ignore_unused".
> + */
> + if (tcu->soc_info->allow_missing_tcu_clk && ret == -EINVAL) {
> + pr_warn("TCU clock missing from device tree, please update your
> device tree\n");
> + tcu->clk = NULL;
> + } else {
> + pr_crit("Cannot get TCU clock from device tree\n");
> + goto err_free_tcu;
> + }
> + } else {
> + ret = clk_prepare_enable(tcu->clk);
> + if (ret) {
> + pr_crit("Unable to enable TCU clock\n");
> + goto err_put_clk;
> + }
> }
> }
>
> @@ -432,10 +447,10 @@ static int __init ingenic_tcu_probe(struct
> device_node *np)
> clk_hw_unregister(tcu->clocks->hws[i]);
> kfree(tcu->clocks);
> err_clk_disable:
> - if (tcu->soc_info->has_tcu_clk)
> + if (tcu->clk)
> clk_disable_unprepare(tcu->clk);
> err_put_clk:
> - if (tcu->soc_info->has_tcu_clk)
> + if (tcu->clk)
> clk_put(tcu->clk);
> err_free_tcu:
> kfree(tcu);
> --
> 2.35.1
>


2022-05-18 20:50:21

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 2/2] clk: ingenic-tcu: Fix missing TCU clock for X1000 SoCs

Quoting Aidan MacDonald (2022-04-12 05:27:50)
> The TCU clock gate on X1000 wasn't requested by the driver and could
> be gated automatically later on in boot, which prevents timers from
> running and breaks PWM.
>
> Add a workaround to support old device trees that don't specify the
> "tcu" clock gate. In this case the kernel will print a warning and
> attempt to continue without the clock, which is wrong, but it could
> work if "clk_ignore_unused" is in the kernel arguments.
>
> Signed-off-by: Aidan MacDonald <[email protected]>
> ---

Can I take just this one patch through clk tree without the dts part?
dts snippets go through soc trees.

2022-05-19 01:05:55

by Aidan MacDonald

[permalink] [raw]
Subject: Re: [PATCH v5 2/2] clk: ingenic-tcu: Fix missing TCU clock for X1000 SoCs

On Wed, May 18, 2022 at 01:47:32PM -0700, Stephen Boyd wrote:
> Quoting Aidan MacDonald (2022-04-12 05:27:50)
> > The TCU clock gate on X1000 wasn't requested by the driver and could
> > be gated automatically later on in boot, which prevents timers from
> > running and breaks PWM.
> >
> > Add a workaround to support old device trees that don't specify the
> > "tcu" clock gate. In this case the kernel will print a warning and
> > attempt to continue without the clock, which is wrong, but it could
> > work if "clk_ignore_unused" is in the kernel arguments.
> >
> > Signed-off-by: Aidan MacDonald <[email protected]>
> > ---
>
> Can I take just this one patch through clk tree without the dts part?
> dts snippets go through soc trees.

Yeah. The patches can go through in any order but they both need to be
merged to fix the bug -- each on its own should not change any behavior.

Regards,
Aidan

2022-05-19 06:10:13

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v5 2/2] clk: ingenic-tcu: Fix missing TCU clock for X1000 SoCs

Quoting Aidan MacDonald (2022-04-12 05:27:50)
> The TCU clock gate on X1000 wasn't requested by the driver and could
> be gated automatically later on in boot, which prevents timers from
> running and breaks PWM.
>
> Add a workaround to support old device trees that don't specify the
> "tcu" clock gate. In this case the kernel will print a warning and
> attempt to continue without the clock, which is wrong, but it could
> work if "clk_ignore_unused" is in the kernel arguments.
>
> Signed-off-by: Aidan MacDonald <[email protected]>
> ---

Applied to clk-next