Re: [PATCH v4 2/3] clock: eswin: Add eic7700 clock driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 15/08/2025 11:37, dongxuyang@eswincomputing.com wrote:
> From: Xuyang Dong <dongxuyang@eswincomputing.com>
> 
> This driver depends on the CCF framework implementation.
>   Based on this driver, other modules in the SoC can use the APIs
>   provided by CCF to perform clock-related operations.

Useless description. Instead describe the hardware and architecture of
your driver. We all know what is the purpose of CCF and how it works.

>   The driver supports eic7700 series chips.

Messed indentation.

> 
> Signed-off-by: Yifeng Huang <huangyifeng@eswincomputing.com>
> Signed-off-by: Xuyang Dong <dongxuyang@eswincomputing.com>
> ---
>  drivers/clk/Kconfig             |   1 +
>  drivers/clk/Makefile            |   1 +
>  drivers/clk/eswin/Kconfig       |  10 +
>  drivers/clk/eswin/Makefile      |   8 +
>  drivers/clk/eswin/clk-eic7700.c |  44 ++
>  drivers/clk/eswin/clk.c         | 734 ++++++++++++++++++++++++++++++++
>  drivers/clk/eswin/clk.h         |  69 +++
>  7 files changed, 867 insertions(+)
>  create mode 100644 drivers/clk/eswin/Kconfig
>  create mode 100644 drivers/clk/eswin/Makefile
>  create mode 100644 drivers/clk/eswin/clk-eic7700.c
>  create mode 100644 drivers/clk/eswin/clk.c
>  create mode 100644 drivers/clk/eswin/clk.h
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 4d56475f94fc..184b76a406d7 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -505,6 +505,7 @@ source "drivers/clk/actions/Kconfig"
>  source "drivers/clk/analogbits/Kconfig"
>  source "drivers/clk/baikal-t1/Kconfig"
>  source "drivers/clk/bcm/Kconfig"
> +source "drivers/clk/eswin/Kconfig"
>  source "drivers/clk/hisilicon/Kconfig"
>  source "drivers/clk/imgtec/Kconfig"
>  source "drivers/clk/imx/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 18ed29cfdc11..42c61e216511 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -120,6 +120,7 @@ obj-$(CONFIG_CLK_BAIKAL_T1)		+= baikal-t1/
>  obj-y					+= bcm/
>  obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
>  obj-$(CONFIG_ARCH_DAVINCI)		+= davinci/
> +obj-$(CONFIG_ARCH_ESWIN)		+= eswin/
>  obj-$(CONFIG_ARCH_HISI)			+= hisilicon/
>  obj-y					+= imgtec/
>  obj-y					+= imx/
> diff --git a/drivers/clk/eswin/Kconfig b/drivers/clk/eswin/Kconfig
> new file mode 100644
> index 000000000000..f2284c2d790d
> --- /dev/null
> +++ b/drivers/clk/eswin/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config COMMON_CLK_EIC7700
> +	bool "EIC7700 Clock Driver"
> +	depends on ARCH_ESWIN
> +	help
> +	  Build the Eswin EIC7700 SoC clock driver based on the
> +	  common clock framework. This driver provides support
> +	  for the clock control on the Eswin EIC7700 SoC,
> +	  which is essential for managing clock rates and power management.
> diff --git a/drivers/clk/eswin/Makefile b/drivers/clk/eswin/Makefile
> new file mode 100644
> index 000000000000..a3139e34ee22
> --- /dev/null
> +++ b/drivers/clk/eswin/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Eswin Clock specific Makefile
> +#
> +
> +obj-y	+= clk.o
> +
> +obj-$(CONFIG_COMMON_CLK_EIC7700)	+= clk-eic7700.o
> diff --git a/drivers/clk/eswin/clk-eic7700.c b/drivers/clk/eswin/clk-eic7700.c
> new file mode 100644
> index 000000000000..278b256b4c52
> --- /dev/null
> +++ b/drivers/clk/eswin/clk-eic7700.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd..
> + * All rights reserved.
> + *
> + * ESWIN EIC7700 CLK Provider Driver
> + *
> + * Authors:
> + *	Yifeng Huang <huangyifeng@eswincomputing.com>
> + *	Xuyang Dong <dongxuyang@eswincomputing.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>

None of these three are used.

> +#include "clk.h"
> +
> +static void __init eic7700_clk_pll_init(struct device_node *np)
> +{
> +	eswin_clk_pll_register(np);
> +}
> +
> +static void __init eic7700_clk_mux_init(struct device_node *np)
> +{
> +	eswin_clk_mux_register(np);
> +}
> +
> +static void __init eic7700_clk_div_init(struct device_node *np)
> +{
> +	eswin_clk_div_register(np);
> +}
> +
> +static void __init eic7700_clk_gate_init(struct device_node *np)
> +{
> +	eswin_clk_gate_register(np);
> +}
> +
> +CLK_OF_DECLARE(eic7700_clk_pll, "eswin,pll-clock", eic7700_clk_pll_init);
> +CLK_OF_DECLARE(eic7700_clk_mux, "eswin,mux-clock", eic7700_clk_mux_init);
> +CLK_OF_DECLARE(eic7700_clk_div, "eswin,divider-clock", eic7700_clk_div_init);
> +CLK_OF_DECLARE(eic7700_clk_gate, "eswin,gate-clock", eic7700_clk_gate_init);

That's empty wrapper. You created just one more layer of indirection,
with no use at all, instead of calling these directly.

> diff --git a/drivers/clk/eswin/clk.c b/drivers/clk/eswin/clk.c
> new file mode 100644
> index 000000000000..e227cc4664ca
> --- /dev/null
> +++ b/drivers/clk/eswin/clk.c
> @@ -0,0 +1,734 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd..
> + * All rights reserved.
> + *
> + * Authors:
> + *	Yifeng Huang <huangyifeng@eswincomputing.com>
> + *	Xuyang Dong <dongxuyang@eswincomputing.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/math.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <linux/util_macros.h>
> +#include "clk.h"
> +
> +enum pll_clk {
> +	CLK_APLL_FOUT1 = 1,
> +	CLK_PLL_CPU
> +};
> +
> +static enum pll_clk str_to_pll_clk(const char *str)
> +{
> +	if (!strcmp(str, "clk_apll_fout1"))
> +		return CLK_APLL_FOUT1;
> +	else if (!strcmp(str, "clk_pll_cpu"))
> +		return CLK_PLL_CPU;
> +	else
> +		return 0;
> +}
> +
> +static void __iomem *parent_base;

Don't write singletons.

> +
> +static void __init get_parent_base(struct device_node *parent_np)

This is just poor code. Drop.


> +{
> +	if (!parent_base) {
> +		parent_base = of_iomap(parent_np, 0);
> +		if (IS_ERR(parent_base)) {
> +			pr_err("%s: Failed to map registers\n", __func__);
> +			parent_base = NULL;
> +		}
> +	}
> +}
> +


...

> +
> +void __init eswin_clk_gate_register(struct device_node *np)
> +{
> +	struct clk_hw *clk_hw;
> +	struct device_node *parent_np;
> +	const char *clk_name;
> +	const char *parent_name;
> +	u32 idx_bit;
> +	u32 reg;
> +	int ret;
> +
> +	parent_np = of_get_parent(np);
> +	if (!parent_np) {
> +		pr_err("%s: Failed to get parent node\n", __func__);
> +		return;
> +	}
> +
> +	if (of_device_is_compatible(parent_np, "eswin,eic7700-clock"))
> +		get_parent_base(parent_np);
> +	else
> +		return;

What? Don't write drivers like that. All this is completely unnecessary
and confusing code. You don't get a singleton, you don't reference it
from some other init code. It's not needed even! Design this properly so
 other clocks will be instantiated from parent clock driver. Just like
every other clock controller is doing.

> +
> +	if (IS_ERR(parent_base)) {
> +		pr_err("%s: Failed to map registers\n", __func__);

Even more spaghetti code. If you need to check for the value, you check
right after obtaining it.

> +		goto put_node;
> +	}
> +
> +	ret = of_property_read_string(np, "clock-output-names", &clk_name);
> +	if (ret) {
> +		pr_err("%s: Missing clock-output-names\n", __func__);
> +		goto put_node;
> +	}

...

> +
> +#define CLK_FREQ_1800M 1800000000
> +#define CLK_FREQ_1700M 1700000000
> +#define CLK_FREQ_1600M 1600000000
> +#define CLK_FREQ_1500M 1500000000
> +#define CLK_FREQ_1400M 1400000000
> +#define CLK_FREQ_1300M 1300000000
> +#define CLK_FREQ_1200M 1200000000
> +#define CLK_FREQ_1000M 1000000000
> +#define CLK_FREQ_900M 900000000
> +#define CLK_FREQ_800M 800000000
> +#define CLK_FREQ_700M 700000000
> +#define CLK_FREQ_600M 600000000
> +#define CLK_FREQ_500M 500000000
> +#define CLK_FREQ_400M 400000000
> +#define CLK_FREQ_200M 200000000
> +#define CLK_FREQ_100M 100000000
> +#define CLK_FREQ_24M 24000000

Useless defines, just like: #define true 1

> +
> +#define APLL_HIGH_FREQ 983040000
> +#define APLL_LOW_FREQ 225792000
> +
> +struct eswin_clk_pll {
> +	struct clk_hw hw;
> +	void __iomem *ctrl_reg0;
> +	u8 pllen_shift;
> +	u8 pllen_width;
> +	u8 refdiv_shift;
> +	u8 refdiv_width;
> +	u8 fbdiv_shift;
Best regards,
Krzysztof