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

[Paul Cercueil]

Hi Aidan,

Le lun., avril 11 2022 at 17:08:41 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >  > ---
> >  >  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
> >  >