diff mbox series

[v4,2/2] clk: ingenic-tcu: Fix missing TCU clock for X1000 SoCs

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

Commit Message

Aidan MacDonald April 11, 2022, 3:42 p.m. UTC
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(-)

Comments

Paul Cercueil April 11, 2022, 3:48 p.m. UTC | #1
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
>
Aidan MacDonald April 11, 2022, 4:08 p.m. UTC | #2
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
> > 
> 
>
Paul Cercueil April 11, 2022, 4:36 p.m. UTC | #3
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
>>  >
>> 
>>
Paul Cercueil April 11, 2022, 7:07 p.m. UTC | #4
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
>
kernel test robot April 12, 2022, 2:03 a.m. UTC | #5
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
Dan Carpenter April 12, 2022, 10:57 a.m. UTC | #6
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 mbox series

Patch

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);