Re: [PATCH v6 05/15] clk: starfive: Add JHB100 System-0 clock generator driver

[Changhuang Liang]

Hi, Jerome

> > Add support for JHB100 System-0 clock generator (SYS0CRG).
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > ---
> >  MAINTAINERS                                   |   7 +
> >  drivers/clk/starfive/Kconfig                  |  10 ++
> >  drivers/clk/starfive/Makefile                 |   2 +
> >  .../clk/starfive/clk-starfive-jhb100-sys0.c   | 156 ++++++++++++++++++
> >  4 files changed, 175 insertions(+)
> >  create mode 100644 drivers/clk/starfive/clk-starfive-jhb100-sys0.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 66d6c15f1dd2..dff8b819f83e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -26217,6 +26217,13 @@ F:
> 	Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
> >  F:	drivers/phy/starfive/phy-jh7110-pcie.c
> >  F:	drivers/phy/starfive/phy-jh7110-usb.c
> >
> > +STARFIVE JHB100 CLOCK DRIVERS
> > +M:	Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > +S:	Supported
> > +F:	Documentation/devicetree/bindings/clock/starfive,jhb1*.yaml
> > +F:	drivers/clk/starfive/clk-starfive-jhb1*
> > +F:	include/dt-bindings/clock/starfive,jhb1*.h
> > +
> >  STARFIVE JHB100 EXTERNAL INTERRUPT CONTROLLER DRIVER
> >  M:	Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> >  S:	Supported
> > diff --git a/drivers/clk/starfive/Kconfig
> > b/drivers/clk/starfive/Kconfig index 51716655554f..cd5b7754403e 100644
> > --- a/drivers/clk/starfive/Kconfig
> > +++ b/drivers/clk/starfive/Kconfig
> > @@ -72,3 +72,13 @@ config CLK_STARFIVE_JH7110_VOUT
> >  	help
> >  	  Say yes here to support the Video-Output clock controller
> >  	  on the StarFive JH7110 SoC.
> > +
> > +config CLK_STARFIVE_JHB100_SYS0
> > +	bool "StarFive JHB100 system-0 clock support"
> > +	depends on ARCH_STARFIVE || COMPILE_TEST
> > +	select CLK_STARFIVE_JH71X0
> > +	select RESET_STARFIVE_JHB100 if RESET_CONTROLLER
> 
> just imply RESET_STARFIVE_JHB100 ?
> 
> It can be disabled if RESET_CONTROLLER is not set, so it is kind of weak
> already.
> 
> > +	default ARCH_STARFIVE
> > +	help
> > +	  Say yes here to support the system-0 clock controller on the
> > +	  StarFive JHB100 SoC.
> > diff --git a/drivers/clk/starfive/Makefile
> > b/drivers/clk/starfive/Makefile index 199ac0f37a2f..d2ba83d7d02c
> > 100644
> > --- a/drivers/clk/starfive/Makefile
> > +++ b/drivers/clk/starfive/Makefile
> > @@ -10,3 +10,5 @@ obj-$(CONFIG_CLK_STARFIVE_JH7110_AON)	+=
> clk-starfive-jh7110-aon.o
> >  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_SYS0)		+=
> clk-starfive-jhb100-sys0.o
> > diff --git a/drivers/clk/starfive/clk-starfive-jhb100-sys0.c
> > b/drivers/clk/starfive/clk-starfive-jhb100-sys0.c
> > new file mode 100644
> > index 000000000000..edab54f228ad
> > --- /dev/null
> > +++ b/drivers/clk/starfive/clk-starfive-jhb100-sys0.c
> > @@ -0,0 +1,156 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * StarFive JHB100 System-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/init.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "clk-starfive-jh71x0.h"
> > +
> > +#define JHB100_SYS0CLK_NUM_CLKS
> 	(JHB100_SYS0CLK_GPU1_NCNOC_INIT + 1)
> > +
> > +/* external clocks */
> > +#define JHB100_SYS0CLK_OSC
> 	(JHB100_SYS0CLK_NUM_CLKS + 0)
> > +#define JHB100_SYS0CLK_PLL0
> 	(JHB100_SYS0CLK_NUM_CLKS + 1)
> > +#define JHB100_SYS0CLK_PLL1
> 	(JHB100_SYS0CLK_NUM_CLKS + 2)
> > +#define JHB100_SYS0CLK_PLL2
> 	(JHB100_SYS0CLK_NUM_CLKS + 3)
> 
> Does this mean that if the you ever find out you've forgot an internal clock
> (which does happen fairly often) the whole external block will shift ?
> 

Later, I will introduce the following macros to define the external clock, which can avoid this problem.

#define JH71X0_EXT_CLK_FLAG	BIT(15)
#define JH71X0_EXT_CLK(_n) 	(JH71X0_EXT_CLK_FLAG | (_n))

> > +
> > +static const char * const jhb100_sys0_ext_clk[] = {
> > +	"osc",
> > +	"pll0",
> > +	"pll1",
> > +	"pll2",
> > +};
> > +
> > +/*
> > + * Regarding the CLK_IS_CRITICAL flag:
> > + *   Clocks flagged CLK_IS_CRITICAL feed logic that has to keep running
> whether
> > + *   or not a consumer holds them, such as the bus interconnect and
> always-on
> > + *   blocks. Gating them hangs the system, so they must never be
> disabled.
> 
> PCIE and USB are critical below and do not appear to meant this criteria, yet
> they are critical too.

Best Regards,
Changhuang