Re: [PATCH v6 10/15] clk: starfive: Add StarFive JHB100 Peripheral-0 clock driver

[Jerome Brunet]

> Add driver for the StarFive JHB100 Peripheral-0 clock controller.
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> ---
>  drivers/clk/starfive/Kconfig                  |  10 +
>  drivers/clk/starfive/Makefile                 |   1 +
>  .../clk/starfive/clk-starfive-jhb100-per0.c   | 617 ++++++++++++++++++
>  3 files changed, 628 insertions(+)
>  create mode 100644 drivers/clk/starfive/clk-starfive-jhb100-per0.c
>
> diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig
> index cfaac934a8e0..b2a05bbb450f 100644
> --- a/drivers/clk/starfive/Kconfig
> +++ b/drivers/clk/starfive/Kconfig
> @@ -73,6 +73,16 @@ config CLK_STARFIVE_JH7110_VOUT
>  	  Say yes here to support the Video-Output clock controller
>  	  on the StarFive JH7110 SoC.
>  
> +config CLK_STARFIVE_JHB100_PER0
> +	bool "StarFive JHB100 peripheral-0 clock support"
> +	depends on ARCH_STARFIVE || COMPILE_TEST
> +	select CLK_STARFIVE_JH71X0
> +	select RESET_STARFIVE_JHB100 if RESET_CONTROLLER
> +	default ARCH_STARFIVE
> +	help
> +	  Say yes here to support the peripheral-0 clock controller
> +	  on the StarFive JHB100 SoC.
> +
>  config CLK_STARFIVE_JHB100_SYS0
>  	bool "StarFive JHB100 system-0 clock support"
>  	depends on ARCH_STARFIVE || COMPILE_TEST
> diff --git a/drivers/clk/starfive/Makefile b/drivers/clk/starfive/Makefile
> index 29cdef20575e..352717886295 100644
> --- a/drivers/clk/starfive/Makefile
> +++ b/drivers/clk/starfive/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_CLK_STARFIVE_JH7110_STG)	+= clk-starfive-jh7110-stg.o
>  obj-$(CONFIG_CLK_STARFIVE_JH7110_ISP)	+= clk-starfive-jh7110-isp.o
>  obj-$(CONFIG_CLK_STARFIVE_JH7110_VOUT)	+= clk-starfive-jh7110-vout.o
>  
> +obj-$(CONFIG_CLK_STARFIVE_JHB100_PER0)		+= clk-starfive-jhb100-per0.o
>  obj-$(CONFIG_CLK_STARFIVE_JHB100_SYS0)		+= clk-starfive-jhb100-sys0.o
>  obj-$(CONFIG_CLK_STARFIVE_JHB100_SYS1)		+= clk-starfive-jhb100-sys1.o
>  obj-$(CONFIG_CLK_STARFIVE_JHB100_SYS2)		+= clk-starfive-jhb100-sys2.o
> diff --git a/drivers/clk/starfive/clk-starfive-jhb100-per0.c b/drivers/clk/starfive/clk-starfive-jhb100-per0.c
> new file mode 100644
> index 000000000000..ef6e6906cd2f
> --- /dev/null
> +++ b/drivers/clk/starfive/clk-starfive-jhb100-per0.c
> @@ -0,0 +1,617 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * StarFive JHB100 Peripheral-0 Clock Driver
> + *
> + * Copyright (C) 2024 StarFive Technology Co., Ltd.
> + *
> + * Author: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> + *
> + */
> +
> +#include <dt-bindings/clock/starfive,jhb100-crg.h>
> +#include <linux/platform_device.h>
> +
> +#include "clk-starfive-jh71x0.h"
> +
> +#define JHB100_PER0CLK_NUM_CLKS			(JHB100_PER0CLK_MAIN_ICG_EN_TRNG + 1)
> +
> +/* external clocks */
> +#define JHB100_PER0CLK_OSC			(JHB100_PER0CLK_NUM_CLKS + 0)
> +#define JHB100_PER0CLK_PLL6			(JHB100_PER0CLK_NUM_CLKS + 1)
> +#define JHB100_PER0CLK_CFG_400			(JHB100_PER0CLK_NUM_CLKS + 2)
> +#define JHB100_PER0CLK_CFG_800			(JHB100_PER0CLK_NUM_CLKS + 3)
> +#define JHB100_PER0CLK_NCNOC_INIT		(JHB100_PER0CLK_NUM_CLKS + 4)
> +#define JHB100_PER0CLK_NCNOC_TARG		(JHB100_PER0CLK_NUM_CLKS + 5)
> +
> +static const char * const jhb100_per0_ext_clk[] = {
> +	"osc",
> +	"pll6",
> +	"cfg_400",
> +	"cfg_800",
> +	"ncnoc_init",
> +	"ncnoc_targ",
> +};
> +
> +/*
> + * Regarding the CLK_IS_CRITICAL flag:
> + *   What is special here is the UART*-related clocks. On the JHB100, the secure
> + *   core may consume one UART, while the AP core consumes another. Different
> + *   core boards may select different UARTs, so we usually want to keep all UART*
> + *   clocks always on to prevent the AP core from turning off the clock for the
> + *   secure core's UART.
> + *   UART0-3 are usually used by the HOST side, and there will be no driver on
> + *   the BMC side to consume these clocks. Therefore, these clocks need to be
> + *   enabled for the host to consume.

So you are forcefully enabling 14 clocks for 1 to 4 clocks that may or may not be used.
This is not how this should work. You just enable clock randomly hoping it will cover
your use case. If you are managing the clock for these secure core, it clearly shows you
need a driver for it.

> + *
> + * Regarding the CLK_IGNORE_UNUSED flag:
> + *   Clocks flagged CLK_IGNORE_UNUSED are not consumed by any driver. The flag
> + *   keeps them in their hardware default state (enabled) instead of letting
> + *   the clock framework gate them once it disables unused clocks.  If this
> + *   flag needs to be removed later, it must be ensured that these clocks will
> + *   not be disabled even if they are not consumed.

Why ? you are ignoring all sorts of thing in there like i3c. If the i3c bus is unused,
the clock should be disabled. If it is actively used, why don't you have a driver for it ?

Throwing CLK_IGNORE_UNUSED and CLK_IS_CRITICAL at the majority of your clocks is not OK.

-- 
Jerome