RE: [PATCH v3 06/22] clk: starfive: Add JHB100 System-0 clock generator driver

[Hal Feng]

> On 26.08.07 19:29, Changhuang Liang wrote:
> Add support for JHB100 System-0 clock generator (SYS0CRG).
> 
> The StarFive JHB100 SoC has multiple CRGs with similar probe flows, so a
> generic starfive_crg_probe() function is introduced to facilitate the
> registration process of other CRGs in the future.
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                                   |   7 +
>  drivers/clk/starfive/Kconfig                  |  11 ++
>  drivers/clk/starfive/Makefile                 |   2 +
>  drivers/clk/starfive/clk-starfive-common.c    | 142 ++++++++++++++++-
>  drivers/clk/starfive/clk-starfive-common.h    |  12 ++
>  .../clk/starfive/clk-starfive-jhb100-sys0.c   | 150 ++++++++++++++++++
>  6 files changed, 321 insertions(+), 3 deletions(-)  create mode 100644
> drivers/clk/starfive/clk-starfive-jhb100-sys0.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ce97de46e610..c48b56d0ab94 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26069,6 +26069,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:	Maintained
> +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 DEVICETREES
>  M:	Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
>  L:	linux-riscv@xxxxxxxxxxxxxxxxxxx
> diff --git a/drivers/clk/starfive/Kconfig b/drivers/clk/starfive/Kconfig index
> ff8eace36e64..7926e02ccd7d 100644
> --- a/drivers/clk/starfive/Kconfig
> +++ b/drivers/clk/starfive/Kconfig
> @@ -72,3 +72,14 @@ 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 AUXILIARY_BUS
> +	select CLK_STARFIVE_COMMON
> +	select RESET_STARFIVE_JHB100 if RESET_CONTROLLER
> +	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
> 012f7ee83f8e..2c5e66d1d44e 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-common.c
> b/drivers/clk/starfive/clk-starfive-common.c
> index 9c0eb7a50d1e..1b5e56af4138 100644
> --- a/drivers/clk/starfive/clk-starfive-common.c
> +++ b/drivers/clk/starfive/clk-starfive-common.c
> @@ -9,6 +9,8 @@
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <soc/starfive/reset-starfive-common.h>
> 
>  #include "clk-starfive-common.h"
> 
> @@ -331,9 +333,143 @@ struct clk_hw *starfive_clk_get(struct
> of_phandle_args *clkspec, void *data)
>  	struct starfive_clk_priv *priv = data;
>  	unsigned int idx = clkspec->args[0];
> 
> -	if (idx < priv->num_reg)
> -		return &priv->reg[idx].hw;
> +	if (idx >= priv->num_reg)
> +		return ERR_PTR(-EINVAL);
> 
> -	return ERR_PTR(-EINVAL);
> +	/* Index space is sparse: reject holes that were never registered. */
> +	if (priv->info && !priv->info->clk_data[idx].name)
> +		return ERR_PTR(-ENOENT);
> +
> +	return &priv->reg[idx].hw;
>  }
>  EXPORT_SYMBOL_GPL(starfive_clk_get);
> +
> +static void starfive_reset_unregister_adev(void *_adev) {
> +	struct auxiliary_device *adev = _adev;
> +
> +	auxiliary_device_delete(adev);
> +	auxiliary_device_uninit(adev);
> +}
> +
> +static void starfive_reset_adev_release(struct device *dev) {
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +	struct starfive_reset_adev *rdev = to_starfive_reset_adev(adev);
> +
> +	kfree(rdev);
> +}
> +
> +static int starfive_reset_controller_register(struct starfive_clk_priv *priv,
> +					      const char *adev_name,
> +					      u32 adev_id)
> +{
> +	struct starfive_reset_adev *rdev;
> +	struct auxiliary_device *adev;
> +	int ret;
> +
> +	rdev = kzalloc_obj(*rdev);
> +	if (!rdev)
> +		return -ENOMEM;
> +
> +	rdev->base = priv->base;
> +
> +	adev = &rdev->adev;
> +	adev->name = adev_name;
> +	adev->dev.parent = priv->dev;
> +	adev->dev.release = starfive_reset_adev_release;
> +	adev->id = adev_id;
> +
> +	ret = auxiliary_device_init(adev);
> +	if (ret) {
> +		kfree(rdev);
> +		return ret;
> +	}
> +
> +	ret = auxiliary_device_add(adev);
> +	if (ret) {
> +		auxiliary_device_uninit(adev);
> +		return ret;
> +	}
> +
> +	return devm_add_action_or_reset(priv->dev,
> +					starfive_reset_unregister_adev, adev); }

You add starfive_reset_controller_register() but a most similiar jh7110_reset_controller_register()
remains in drivers/clk/starfive/clk-starfive-jh7110-sys.c. Can we merge them?

> +
> +int starfive_crg_probe(struct platform_device *pdev) {
 
This function name is too generic and we know not all starfive SoCs use it to probe.

> +	const struct starfive_crg_domain_info *info;
> +	struct starfive_clk_priv *priv;
> +	unsigned int idx;
> +	int ret;
> +
> +	info = of_device_get_match_data(&pdev->dev);
> +	if (!info)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(&pdev->dev, struct_size(priv, reg, info->num_clk),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&priv->rmw_lock);
> +	priv->info = info;
> +	priv->num_reg = info->num_clk;
> +	priv->dev = &pdev->dev;
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	if (info->power_domain) {
> +		ret = devm_pm_runtime_enable(priv->dev);
> +		if (ret)
> +			return dev_err_probe(priv->dev, ret,
> +					     "failed to enable runtime PM\n");
> +	}
> +
> +	for (idx = 0; idx < info->num_clk; idx++) {
> +		u32 max = info->clk_data[idx].max;
> +		struct clk_parent_data parents[4] = {};
> +		struct clk_init_data init = {
> +			.name = info->clk_data[idx].name,
> +			.ops = starfive_clk_ops(max),
> +			.parent_data = parents,
> +			.num_parents =
> +				((max & STARFIVE_CLK_MUX_MASK) >>
> STARFIVE_CLK_MUX_SHIFT) + 1,
> +			.flags = info->clk_data[idx].flags,
> +		};
> +		struct starfive_clk *clk = &priv->reg[idx];
> +		unsigned int i;
> +
> +		if (!init.name)
> +			continue;
> +
> +		for (i = 0; i < init.num_parents; i++) {
> +			unsigned int pidx = info->clk_data[idx].parents[i];
> +
> +			if (pidx < info->num_clk) {
> +				parents[i].hw = &priv->reg[pidx].hw;
> +			} else {
> +				if (pidx - info->num_clk >= info->num_ext_clk)
> +					return -EINVAL;
> +
> +				parents[i].fw_name = info->ext_clk[pidx - info->num_clk];
> +			}
> +		}
> +
> +		clk->hw.init = &init;
> +		clk->idx = idx;
> +		clk->max_div = max & STARFIVE_CLK_DIV_MASK;
> +
> +		ret = devm_clk_hw_register(&pdev->dev, &clk->hw);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(&pdev->dev, starfive_clk_get, priv);
> +	if (ret)
> +		return ret;
> +
> +	return starfive_reset_controller_register(priv, info->rst_name, 0); }
> +EXPORT_SYMBOL_GPL(starfive_crg_probe);
> diff --git a/drivers/clk/starfive/clk-starfive-common.h
> b/drivers/clk/starfive/clk-starfive-common.h
> index a03824e9e75f..4cd4357f8366 100644
> --- a/drivers/clk/starfive/clk-starfive-common.h
> +++ b/drivers/clk/starfive/clk-starfive-common.h
> @@ -5,6 +5,7 @@
>  #include <linux/bits.h>
>  #include <linux/clk-provider.h>
>  #include <linux/device.h>
> +#include <linux/platform_device.h>
>  #include <linux/spinlock.h>
> 
>  /* register fields */
> @@ -109,6 +110,15 @@ struct starfive_clk {
>  	unsigned int max_div;
>  };
> 
> +struct starfive_crg_domain_info {
> +	const struct starfive_clk_data *clk_data;
> +	unsigned int num_clk;
> +	const char * const *ext_clk;
> +	unsigned int num_ext_clk;
> +	const char *rst_name;
> +	bool power_domain;
> +};

Is the starfive_crg_domain_info struct introduced only for JHB100? Could we also use it for JH7110?

Best regards,
Hal

> +
>  struct starfive_clk_priv {
>  	/* protect clk enable and set rate/parent from happening at the same
> time */
>  	spinlock_t rmw_lock;
> @@ -116,6 +126,7 @@ struct starfive_clk_priv {
>  	void __iomem *base;
>  	struct clk *original_clk;
>  	struct notifier_block pll_clk_nb;
> +	const struct starfive_crg_domain_info *info;
>  	struct clk_hw *pll[3];
>  	unsigned int num_reg;
>  	struct starfive_clk reg[] __counted_by(num_reg); @@ -123,5 +134,6
> @@ struct starfive_clk_priv {
> 
>  const struct clk_ops *starfive_clk_ops(u32 max);  struct clk_hw
> *starfive_clk_get(struct of_phandle_args *clkspec, void *data);
> +int starfive_crg_probe(struct platform_device *pdev);
> 
>  #endif
> 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..eb3cd33e2309
> --- /dev/null
> +++ b/drivers/clk/starfive/clk-starfive-jhb100-sys0.c
> @@ -0,0 +1,150 @@
> +// 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-common.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)
> +
> +static const char * const jhb100_sys0_ext_clk[] = {
> +	"osc",
> +	"pll0",
> +	"pll1",
> +	"pll2",
> +};
> +
> +static const struct starfive_clk_data jhb100_sys0crg_clk_data[] = {
> +	/* bmcpcierp */
> +	STARFIVE__DIV(JHB100_SYS0CLK_BMCPCIERP_NCNOC_MAIN,
> "bmcpcierp_ncnoc_main", 6,
> +		      JHB100_SYS0CLK_PLL0),
> +	STARFIVE__DIV(JHB100_SYS0CLK_BMCPCIERP_NCNOC_CFG,
> "bmcpcierp_ncnoc_cfg", 12,
> +		      JHB100_SYS0CLK_PLL1),
> +	STARFIVE__DIV(JHB100_SYS0CLK_PCIE_REF_CML, "pcie_ref_cml", 24,
...
> +	/* gpu1 */
> +	STARFIVE__DIV(JHB100_SYS0CLK_GPU1_NCNOC_INIT,
> "gpu1_ncnoc_init", 10,
> +		      JHB100_SYS0CLK_PLL0),
> +};
> +
> +static const struct starfive_crg_domain_info jhb100_sys0crg_info = {
> +	.clk_data	= jhb100_sys0crg_clk_data,
> +	.num_clk	= ARRAY_SIZE(jhb100_sys0crg_clk_data),
> +	.ext_clk	= jhb100_sys0_ext_clk,
> +	.num_ext_clk	= ARRAY_SIZE(jhb100_sys0_ext_clk),
> +	.rst_name	= "jhb100-r-sys0",
> +	.power_domain	= false,
> +};
> +
> +static const struct of_device_id jhb100_sys0crg_match[] = {
> +	{
> +		.compatible = "starfive,jhb100-sys0crg",
> +		.data = &jhb100_sys0crg_info,
> +	},
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver jhb100_sys0crg_driver = {
> +	.probe = starfive_crg_probe,
> +	.driver = {
> +		.name = "clk-starfive-jhb100-sys0",
> +		.of_match_table = jhb100_sys0crg_match,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +builtin_platform_driver(jhb100_sys0crg_driver);
> --
> 2.25.1