| Message ID | 20220411154241.50834-3-aidanmacdonald.0x0@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix missing TCU clock for X1000/X1830 SoCs | expand |
Hi Aidan, Le lun., avril 11 2022 at 16:42:41 +0100, Aidan MacDonald <aidanmacdonald.0x0@gmail.com> 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 <aidanmacdonald.0x0@gmail.com> > --- > drivers/clk/ingenic/tcu.c | 38 ++++++++++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 12 deletions(-) > > diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c > index 77acfbeb4830..ce8c768db997 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 = { > @@ -354,15 +356,27 @@ static int __init ingenic_tcu_probe(struct > device_node *np) > if (tcu->soc_info->has_tcu_clk) { > 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". > + */ Why not unconditionally enable it instead? Then it would boot without clk_ignore_unused. Cheers, -Paul > + if (tcu->soc_info->allow_missing_tcu_clk && > + PTR_ERR(tcu->clk) == -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 +446,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 >
On Mon, Apr 11, 2022 at 04:48:15PM +0100, Paul Cercueil wrote: > Hi Aidan, > > Le lun., avril 11 2022 at 16:42:41 +0100, Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> 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 <aidanmacdonald.0x0@gmail.com> > > --- > > drivers/clk/ingenic/tcu.c | 38 ++++++++++++++++++++++++++------------ > > 1 file changed, 26 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c > > index 77acfbeb4830..ce8c768db997 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 = { > > @@ -354,15 +356,27 @@ static int __init ingenic_tcu_probe(struct > > device_node *np) > > if (tcu->soc_info->has_tcu_clk) { > > 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". > > + */ > > Why not unconditionally enable it instead? Then it would boot without > clk_ignore_unused. > > Cheers, > -Paul I could, but why add essentially dead code to the kernel? Maintaining the old behavior has the "advantage" that it remains broken in the same way as before, so any workarounds anyone was using will continue to work the same. And if they were not using workarounds and got a broken kernel, this patch will not make anything *more* broken, in fact it will not cause any change in behavior in that case (aside from the warning message). But if you think it's best to just enable the clock anyway, let me know and I'll send a new patch. Regards, Aidan > > > + if (tcu->soc_info->allow_missing_tcu_clk && > > + PTR_ERR(tcu->clk) == -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 +446,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 > > > >
Hi Aidan, Le lun., avril 11 2022 at 17:08:41 +0100, Aidan MacDonald <aidanmacdonald.0x0@gmail.com> a écrit : > On Mon, Apr 11, 2022 at 04:48:15PM +0100, Paul Cercueil wrote: >> Hi Aidan, >> >> Le lun., avril 11 2022 at 16:42:41 +0100, Aidan MacDonald >> <aidanmacdonald.0x0@gmail.com> 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 <aidanmacdonald.0x0@gmail.com> >> > --- >> > drivers/clk/ingenic/tcu.c | 38 >> ++++++++++++++++++++++++++------------ >> > 1 file changed, 26 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c >> > index 77acfbeb4830..ce8c768db997 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 = { >> > @@ -354,15 +356,27 @@ static int __init ingenic_tcu_probe(struct >> > device_node *np) >> > if (tcu->soc_info->has_tcu_clk) { >> > 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". >> > + */ >> >> Why not unconditionally enable it instead? Then it would boot >> without >> clk_ignore_unused. >> >> Cheers, >> -Paul > > I could, but why add essentially dead code to the kernel? Maintaining > the > old behavior has the "advantage" that it remains broken in the same > way as > before, so any workarounds anyone was using will continue to work the > same. > And if they were not using workarounds and got a broken kernel, this > patch > will not make anything *more* broken, in fact it will not cause any > change > in behavior in that case (aside from the warning message). OK. -Paul > But if you think it's best to just enable the clock anyway, let me > know > and I'll send a new patch. > > Regards, > Aidan > >> >> > + if (tcu->soc_info->allow_missing_tcu_clk && >> > + PTR_ERR(tcu->clk) == -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 +446,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 >> > >> >>
Le lun., avril 11 2022 at 16:42:41 +0100, Aidan MacDonald <aidanmacdonald.0x0@gmail.com> 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 <aidanmacdonald.0x0@gmail.com> Reviewed-by: Paul Cercueil <paul@crapouillou.net> Cheers, -Paul > --- > drivers/clk/ingenic/tcu.c | 38 ++++++++++++++++++++++++++------------ > 1 file changed, 26 insertions(+), 12 deletions(-) > > diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c > index 77acfbeb4830..ce8c768db997 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 = { > @@ -354,15 +356,27 @@ static int __init ingenic_tcu_probe(struct > device_node *np) > if (tcu->soc_info->has_tcu_clk) { > 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 && > + PTR_ERR(tcu->clk) == -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 +446,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 >
Hi Aidan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on clk/clk-next linus/master linux/master v5.18-rc2 next-20220411]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Aidan-MacDonald/Fix-missing-TCU-clock-for-X1000-X1830-SoCs/20220411-234531
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: mips-randconfig-c004-20220411 (https://download.01.org/0day-ci/archive/20220412/202204120958.uBiGandq-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c6e83f560f06cdfe8aa47b248d8bdc58f947274b)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/8c04eee82a9d67a768bb6c787d66724782fce89b
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Aidan-MacDonald/Fix-missing-TCU-clock-for-X1000-X1830-SoCs/20220411-234531
git checkout 8c04eee82a9d67a768bb6c787d66724782fce89b
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/clk/ingenic/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/clk/ingenic/tcu.c:366:8: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (tcu->soc_info->allow_missing_tcu_clk &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/clk/ingenic/tcu.c:456:9: note: uninitialized use occurs here
return ret;
^~~
drivers/clk/ingenic/tcu.c:366:4: note: remove the 'if' if its condition is always true
if (tcu->soc_info->allow_missing_tcu_clk &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/clk/ingenic/tcu.c:366:8: warning: variable 'ret' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
if (tcu->soc_info->allow_missing_tcu_clk &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/clk/ingenic/tcu.c:456:9: note: uninitialized use occurs here
return ret;
^~~
drivers/clk/ingenic/tcu.c:366:8: note: remove the '&&' if its condition is always true
if (tcu->soc_info->allow_missing_tcu_clk &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/clk/ingenic/tcu.c:343:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
2 warnings generated.
vim +366 drivers/clk/ingenic/tcu.c
336
337 static int __init ingenic_tcu_probe(struct device_node *np)
338 {
339 const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, np);
340 struct ingenic_tcu *tcu;
341 struct regmap *map;
342 unsigned int i;
343 int ret;
344
345 map = device_node_to_regmap(np);
346 if (IS_ERR(map))
347 return PTR_ERR(map);
348
349 tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
350 if (!tcu)
351 return -ENOMEM;
352
353 tcu->map = map;
354 tcu->soc_info = id->data;
355
356 if (tcu->soc_info->has_tcu_clk) {
357 tcu->clk = of_clk_get_by_name(np, "tcu");
358 if (IS_ERR(tcu->clk)) {
359 /*
360 * Old device trees for some SoCs did not include the
361 * TCU clock because this driver (incorrectly) didn't
362 * use it. In this case we complain loudly and attempt
363 * to continue without the clock, which might work if
364 * booting with workarounds like "clk_ignore_unused".
365 */
> 366 if (tcu->soc_info->allow_missing_tcu_clk &&
367 PTR_ERR(tcu->clk) == -EINVAL) {
368 pr_warn("TCU clock missing from device tree, please update your device tree\n");
369 tcu->clk = NULL;
370 } else {
371 pr_crit("Cannot get TCU clock from device tree\n");
372 goto err_free_tcu;
373 }
374 } else {
375 ret = clk_prepare_enable(tcu->clk);
376 if (ret) {
377 pr_crit("Unable to enable TCU clock\n");
378 goto err_put_clk;
379 }
380 }
381 }
382
383 tcu->clocks = kzalloc(struct_size(tcu->clocks, hws, TCU_CLK_COUNT),
384 GFP_KERNEL);
385 if (!tcu->clocks) {
386 ret = -ENOMEM;
387 goto err_clk_disable;
388 }
389
390 tcu->clocks->num = TCU_CLK_COUNT;
391
392 for (i = 0; i < tcu->soc_info->num_channels; i++) {
393 ret = ingenic_tcu_register_clock(tcu, i, TCU_PARENT_EXT,
394 &ingenic_tcu_clk_info[i],
395 tcu->clocks);
396 if (ret) {
397 pr_crit("cannot register clock %d\n", i);
398 goto err_unregister_timer_clocks;
399 }
400 }
401
402 /*
403 * We set EXT as the default parent clock for all the TCU clocks
404 * except for the watchdog one, where we set the RTC clock as the
405 * parent. Since the EXT and PCLK are much faster than the RTC clock,
406 * the watchdog would kick after a maximum time of 5s, and we might
407 * want a slower kicking time.
408 */
409 ret = ingenic_tcu_register_clock(tcu, TCU_CLK_WDT, TCU_PARENT_RTC,
410 &ingenic_tcu_watchdog_clk_info,
411 tcu->clocks);
412 if (ret) {
413 pr_crit("cannot register watchdog clock\n");
414 goto err_unregister_timer_clocks;
415 }
416
417 if (tcu->soc_info->has_ost) {
418 ret = ingenic_tcu_register_clock(tcu, TCU_CLK_OST,
419 TCU_PARENT_EXT,
420 &ingenic_tcu_ost_clk_info,
421 tcu->clocks);
422 if (ret) {
423 pr_crit("cannot register ost clock\n");
424 goto err_unregister_watchdog_clock;
425 }
426 }
427
428 ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, tcu->clocks);
429 if (ret) {
430 pr_crit("cannot add OF clock provider\n");
431 goto err_unregister_ost_clock;
432 }
433
434 ingenic_tcu = tcu;
435
436 return 0;
437
438 err_unregister_ost_clock:
439 if (tcu->soc_info->has_ost)
440 clk_hw_unregister(tcu->clocks->hws[i + 1]);
441 err_unregister_watchdog_clock:
442 clk_hw_unregister(tcu->clocks->hws[i]);
443 err_unregister_timer_clocks:
444 for (i = 0; i < tcu->clocks->num; i++)
445 if (tcu->clocks->hws[i])
446 clk_hw_unregister(tcu->clocks->hws[i]);
447 kfree(tcu->clocks);
448 err_clk_disable:
449 if (tcu->clk)
450 clk_disable_unprepare(tcu->clk);
451 err_put_clk:
452 if (tcu->clk)
453 clk_put(tcu->clk);
454 err_free_tcu:
455 kfree(tcu);
456 return ret;
457 }
458
Hi Aidan,
url: https://github.com/intel-lab-lkp/linux/commits/Aidan-MacDonald/Fix-missing-TCU-clock-for-X1000-X1830-SoCs/20220411-234531
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-randconfig-m031-20220411 (https://download.01.org/0day-ci/archive/20220412/202204121856.LxK9kEyg-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/clk/ingenic/tcu.c:456 ingenic_tcu_probe() error: uninitialized symbol 'ret'.
vim +/ret +456 drivers/clk/ingenic/tcu.c
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 337 static int __init ingenic_tcu_probe(struct device_node *np)
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 338 {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 339 const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, np);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 340 struct ingenic_tcu *tcu;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 341 struct regmap *map;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 342 unsigned int i;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 343 int ret;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 344
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 345 map = device_node_to_regmap(np);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 346 if (IS_ERR(map))
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 347 return PTR_ERR(map);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 348
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 349 tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 350 if (!tcu)
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 351 return -ENOMEM;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 352
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 353 tcu->map = map;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 354 tcu->soc_info = id->data;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 355
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 356 if (tcu->soc_info->has_tcu_clk) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 357 tcu->clk = of_clk_get_by_name(np, "tcu");
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 358 if (IS_ERR(tcu->clk)) {
8c04eee82a9d67a Aidan MacDonald 2022-04-11 359 /*
8c04eee82a9d67a Aidan MacDonald 2022-04-11 360 * Old device trees for some SoCs did not include the
8c04eee82a9d67a Aidan MacDonald 2022-04-11 361 * TCU clock because this driver (incorrectly) didn't
8c04eee82a9d67a Aidan MacDonald 2022-04-11 362 * use it. In this case we complain loudly and attempt
8c04eee82a9d67a Aidan MacDonald 2022-04-11 363 * to continue without the clock, which might work if
8c04eee82a9d67a Aidan MacDonald 2022-04-11 364 * booting with workarounds like "clk_ignore_unused".
8c04eee82a9d67a Aidan MacDonald 2022-04-11 365 */
8c04eee82a9d67a Aidan MacDonald 2022-04-11 366 if (tcu->soc_info->allow_missing_tcu_clk &&
8c04eee82a9d67a Aidan MacDonald 2022-04-11 367 PTR_ERR(tcu->clk) == -EINVAL) {
8c04eee82a9d67a Aidan MacDonald 2022-04-11 368 pr_warn("TCU clock missing from device tree, please update your device tree\n");
8c04eee82a9d67a Aidan MacDonald 2022-04-11 369 tcu->clk = NULL;
8c04eee82a9d67a Aidan MacDonald 2022-04-11 370 } else {
8c04eee82a9d67a Aidan MacDonald 2022-04-11 371 pr_crit("Cannot get TCU clock from device tree\n");
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 372 goto err_free_tcu;
no error code.
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 373 }
8c04eee82a9d67a Aidan MacDonald 2022-04-11 374 } else {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 375 ret = clk_prepare_enable(tcu->clk);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 376 if (ret) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 377 pr_crit("Unable to enable TCU clock\n");
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 378 goto err_put_clk;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 379 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 380 }
8c04eee82a9d67a Aidan MacDonald 2022-04-11 381 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 382
e620a1e061c4738 Stephen Kitt 2019-09-27 383 tcu->clocks = kzalloc(struct_size(tcu->clocks, hws, TCU_CLK_COUNT),
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 384 GFP_KERNEL);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 385 if (!tcu->clocks) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 386 ret = -ENOMEM;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 387 goto err_clk_disable;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 388 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 389
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 390 tcu->clocks->num = TCU_CLK_COUNT;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 391
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 392 for (i = 0; i < tcu->soc_info->num_channels; i++) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 393 ret = ingenic_tcu_register_clock(tcu, i, TCU_PARENT_EXT,
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 394 &ingenic_tcu_clk_info[i],
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 395 tcu->clocks);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 396 if (ret) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 397 pr_crit("cannot register clock %d\n", i);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 398 goto err_unregister_timer_clocks;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 399 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 400 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 401
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 402 /*
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 403 * We set EXT as the default parent clock for all the TCU clocks
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 404 * except for the watchdog one, where we set the RTC clock as the
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 405 * parent. Since the EXT and PCLK are much faster than the RTC clock,
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 406 * the watchdog would kick after a maximum time of 5s, and we might
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 407 * want a slower kicking time.
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 408 */
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 409 ret = ingenic_tcu_register_clock(tcu, TCU_CLK_WDT, TCU_PARENT_RTC,
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 410 &ingenic_tcu_watchdog_clk_info,
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 411 tcu->clocks);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 412 if (ret) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 413 pr_crit("cannot register watchdog clock\n");
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 414 goto err_unregister_timer_clocks;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 415 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 416
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 417 if (tcu->soc_info->has_ost) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 418 ret = ingenic_tcu_register_clock(tcu, TCU_CLK_OST,
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 419 TCU_PARENT_EXT,
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 420 &ingenic_tcu_ost_clk_info,
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 421 tcu->clocks);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 422 if (ret) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 423 pr_crit("cannot register ost clock\n");
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 424 goto err_unregister_watchdog_clock;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 425 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 426 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 427
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 428 ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, tcu->clocks);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 429 if (ret) {
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 430 pr_crit("cannot add OF clock provider\n");
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 431 goto err_unregister_ost_clock;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 432 }
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 433
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 434 ingenic_tcu = tcu;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 435
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 436 return 0;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 437
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 438 err_unregister_ost_clock:
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 439 if (tcu->soc_info->has_ost)
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 440 clk_hw_unregister(tcu->clocks->hws[i + 1]);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 441 err_unregister_watchdog_clock:
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 442 clk_hw_unregister(tcu->clocks->hws[i]);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 443 err_unregister_timer_clocks:
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 444 for (i = 0; i < tcu->clocks->num; i++)
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 445 if (tcu->clocks->hws[i])
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 446 clk_hw_unregister(tcu->clocks->hws[i]);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 447 kfree(tcu->clocks);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 448 err_clk_disable:
8c04eee82a9d67a Aidan MacDonald 2022-04-11 449 if (tcu->clk)
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 450 clk_disable_unprepare(tcu->clk);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 451 err_put_clk:
8c04eee82a9d67a Aidan MacDonald 2022-04-11 452 if (tcu->clk)
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 453 clk_put(tcu->clk);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 454 err_free_tcu:
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 455 kfree(tcu);
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 @456 return ret;
4f89e4b8f1215c1 Paul Cercueil 2019-07-24 457 }
diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c index 77acfbeb4830..ce8c768db997 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 = { @@ -354,15 +356,27 @@ static int __init ingenic_tcu_probe(struct device_node *np) if (tcu->soc_info->has_tcu_clk) { 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 && + PTR_ERR(tcu->clk) == -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 +446,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);
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 <aidanmacdonald.0x0@gmail.com> --- drivers/clk/ingenic/tcu.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)