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

[Paul Cercueil]

Hi Aidan,

Le sam., févr. 12 2022 at 15:09:28 +0000, Aidan MacDonald 
<aidanmacdonald.0x0@xxxxxxxxx> a écrit :
> The X1000 does have a TCU clock gate, so pass it to the driver.
> Without this the TCU can be gated automatically, which prevents
> timers from running.
>
> Fixes: dc6a81c3382f74fe ("clk: Ingenic: Add support for TCU of 
> X1000.")
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@xxxxxxxxx>
> ---
> I've just realized, maybe this is an ABI break. Now that the TCU 
> clock is
> required, the driver probe will fail if given an old device tree 
> which is
> missing that clock. Is it necessary to add a hack of some sort to 
> support
> the old device tree?

Yes, that's a valid concern. The driver should then support the TCU 
clock being missing (but only for the x1000), with a comment that 
explain why the workaround exists.

You can use of_clk_get_by_name(), and if you get -EINVAL and the 
workaround flag is set, allow the driver to continue. Also change the 
checks for (tcu->soc_info->has_tcu_clk) in the function's cleanup to 
checks for (tcu->clk) so that the clk_disable_unprepare/clk_put are 
only done on a valid pointer.

Note that the x1830 also has a TCU clock that's not specified in the 
device tree; so you could add a patch similar to your current [1/2] 
that adds it to x1830.dtsi as well. It uses the "ingenic,x1000-tcu" 
string as fallback, so the driver wouldn't have to be modified further.

Cheers,
-Paul

>  drivers/clk/ingenic/tcu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
> index 77acfbeb4830..9c86043f673a 100644
> --- a/drivers/clk/ingenic/tcu.c
> +++ b/drivers/clk/ingenic/tcu.c
> @@ -320,7 +320,7 @@ 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,
>  };
>
>  static const struct of_device_id __maybe_unused 
> ingenic_tcu_of_match[] __initconst = {
> --
> 2.34.1