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

[Jerome Brunet <jbrunet@baylibre.com>]

On 2026-10-14 02:13 +0000, Changhuang Liang wrote:
> Hi, Jerome
> 
> > > Add driver for the StarFive JHB100 Peripheral-0 clock controller.
> > >
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > ---
> > >  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@starfivetech.com>
> > > + *
> > > + */
> > > +
> > > +#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.
> 
> Can I keep the clocks for uart0-3 as CLK_IS_CRITICAL here? On the JHB100 BMC SoC, 
> uart0-3 are provided for the host to read and write registers via eSPI. On our BMC 
> side, we need to ensure the clocks are always on to provide host access.
>

It would be less problematic, but still I do not understand why a uart end up being
critical.

If you are letting the host map the uart memory region and implement some
'userspace' driver, you already know what you are doing is bad. So it is
hardly a good reason.

-- 
Jerome