Re: [PATCH v2 2/8] clk: airoha: Add econet EN751221 clock/reset support to en7523-scu

[Brian Masney <bmasney@redhat.com>]

Hi Caleb,

On Mon, Mar 09, 2026 at 01:18:12PM +0000, Caleb James DeLisle wrote:
> EcoNet EN751221 clock/reset driver is significantly similar to the
> EN7523 / EN7581, however the EN751221 does not have a neat batch of clock
> divider registers so there are fewer known clocks, and the frequency of
> each clock is derived differently. This clock driver will probably work
> correctly on EN751627, EN7528, and EN7580.
> 
> Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
> ---
>  drivers/clk/Kconfig      |   6 +-
>  drivers/clk/clk-en7523.c | 238 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 236 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 3d803b4cf5c1..47df6073a72b 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -218,13 +218,13 @@ config COMMON_CLK_CS2000_CP
>  	  If you say yes here you get support for the CS2000 clock multiplier.
>  
>  config COMMON_CLK_EN7523
> -	bool "Clock driver for Airoha EN7523 SoC system clocks"
> +	bool "Clock driver for Airoha/EcoNet SoC system clocks"
>  	depends on OF
> -	depends on ARCH_AIROHA || COMPILE_TEST
> +	depends on ARCH_AIROHA || ECONET || COMPILE_TEST
>  	default ARCH_AIROHA
>  	help
>  	  This driver provides the fixed clocks and gates present on Airoha
> -	  ARM silicon.
> +	  and EcoNet silicon.
>  
>  config COMMON_CLK_EP93XX
>  	tristate "Clock driver for Cirrus Logic ep93xx SoC"
> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 08cc8e5acf43..f7bd7034cf7f 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/clk-provider.h>
>  #include <linux/io.h>
> @@ -11,6 +12,8 @@
>  #include <dt-bindings/clock/en7523-clk.h>
>  #include <dt-bindings/reset/airoha,en7523-reset.h>
>  #include <dt-bindings/reset/airoha,en7581-reset.h>
> +#include <dt-bindings/clock/econet,en751221-scu.h>
> +#include <dt-bindings/reset/econet,en751221-scu.h>
>  
>  #define RST_NR_PER_BANK			32
>  
> @@ -33,15 +36,49 @@
>  #define   REG_RESET_CONTROL_PCIEHB	BIT(29)
>  #define   REG_RESET_CONTROL_PCIE1	BIT(27)
>  #define   REG_RESET_CONTROL_PCIE2	BIT(26)
> +#define REG_HIR				0x064
> +#define   REG_HIR_MASK			GENMASK(31, 16)
>  /* EN7581 */
>  #define REG_NP_SCU_PCIC			0x88
>  #define REG_NP_SCU_SSTR			0x9c
>  #define REG_PCIE_XSI0_SEL_MASK		GENMASK(14, 13)
>  #define REG_PCIE_XSI1_SEL_MASK		GENMASK(12, 11)
>  #define REG_CRYPTO_CLKSRC2		0x20c
> +/* EN751221 */
> +#define EN751221_REG_SPI_DIV		0x0cc
> +#define EN751221_REG_SPI_DIV_MASK	GENMASK(31, 8)
> +#define EN751221_SPI_BASE		500000000
> +#define EN751221_SPI_BASE_EN7526C	400000000
> +#define EN751221_REG_BUS		0x284
> +#define EN751221_REG_BUS_MASK		GENMASK(21, 12)
> +#define EN751221_REG_SSR3		0x094
> +#define EN751221_REG_SSR3_GSW_MASK	GENMASK(9, 8)
>  
>  #define REG_RST_CTRL2			0x830
>  #define REG_RST_CTRL1			0x834
> +#define EN751221_REG_RST_DMT		0x84
> +#define EN751221_REG_RST_USB		0xec
> +
> +#define EN751221_MAX_CLKS		6
> +
> +enum en_hir {
> +	HIR_UNKNOWN	= -1,
> +	HIR_TC3169	= 0,
> +	HIR_TC3182	= 1,
> +	HIR_RT65168	= 2,
> +	HIR_RT63165	= 3,
> +	HIR_RT63365	= 4,
> +	HIR_MT751020	= 5,
> +	HIR_MT7505	= 6,
> +	HIR_EN751221	= 7,
> +	HIR_EN7526C	= 8,
> +	HIR_EN751627	= 9,
> +	HIR_EN7580	= 10,
> +	HIR_EN7528	= 11,
> +	HIR_EN7523	= 12,
> +	HIR_EN7581	= 13,
> +	HIR_MAX		= 14,
> +};
>  
>  struct en_clk_desc {
>  	int id;
> @@ -93,6 +130,8 @@ static const u32 bus7581_base[] = { 600000000, 540000000 };
>  static const u32 npu7581_base[] = { 800000000, 750000000, 720000000, 600000000 };
>  static const u32 crypto_base[] = { 540000000, 480000000 };
>  static const u32 emmc7581_base[] = { 200000000, 150000000 };
> +/* EN751221 */
> +static const u32 gsw751221_base[] = { 500000000, 250000000, 400000000, 200000000 };
>  
>  static const struct en_clk_desc en7523_base_clks[] = {
>  	{
> @@ -300,6 +339,13 @@ static const u16 en7581_rst_ofs[] = {
>  	REG_RST_CTRL1,
>  };
>  
> +static const u16 en751221_rst_ofs[] = {
> +	REG_RST_CTRL2,
> +	REG_RST_CTRL1,
> +	EN751221_REG_RST_DMT,
> +	EN751221_REG_RST_USB,
> +};
> +
>  static const u16 en7523_rst_map[] = {
>  	/* RST_CTRL2 */
>  	[EN7523_XPON_PHY_RST]		= 0,
> @@ -405,8 +451,61 @@ static const u16 en7581_rst_map[] = {
>  	[EN7581_XPON_MAC_RST]		= RST_NR_PER_BANK + 31,
>  };
>  
> +static const u16 en751221_rst_map[] = {
> +	/* RST_CTRL2 */
> +	[EN751221_XPON_PHY_RST]		= 0,
> +	[EN751221_GFAST_RST]		= 1,
> +	[EN751221_CPU_TIMER2_RST]	= 2,
> +	[EN751221_UART3_RST]		= 3,
> +	[EN751221_UART4_RST]		= 4,
> +	[EN751221_UART5_RST]		= 5,
> +	[EN751221_I2C2_RST]		= 6,
> +	[EN751221_XSI_MAC_RST]		= 7,
> +	[EN751221_XSI_PHY_RST]		= 8,
> +
> +	/* RST_CTRL1 */
> +	[EN751221_PCM1_ZSI_ISI_RST]	= RST_NR_PER_BANK + 0,
> +	[EN751221_FE_QDMA1_RST]		= RST_NR_PER_BANK + 1,
> +	[EN751221_FE_QDMA2_RST]		= RST_NR_PER_BANK + 2,
> +	[EN751221_FE_UNZIP_RST]		= RST_NR_PER_BANK + 3,
> +	[EN751221_PCM2_RST]		= RST_NR_PER_BANK + 4,
> +	[EN751221_PTM_MAC_RST]		= RST_NR_PER_BANK + 5,
> +	[EN751221_CRYPTO_RST]		= RST_NR_PER_BANK + 6,
> +	[EN751221_SAR_RST]		= RST_NR_PER_BANK + 7,
> +	[EN751221_TIMER_RST]		= RST_NR_PER_BANK + 8,
> +	[EN751221_INTC_RST]		= RST_NR_PER_BANK + 9,
> +	[EN751221_BONDING_RST]		= RST_NR_PER_BANK + 10,
> +	[EN751221_PCM1_RST]		= RST_NR_PER_BANK + 11,
> +	[EN751221_UART_RST]		= RST_NR_PER_BANK + 12,
> +	[EN751221_GPIO_RST]		= RST_NR_PER_BANK + 13,
> +	[EN751221_GDMA_RST]		= RST_NR_PER_BANK + 14,
> +	[EN751221_I2C_MASTER_RST]	= RST_NR_PER_BANK + 16,
> +	[EN751221_PCM2_ZSI_ISI_RST]	= RST_NR_PER_BANK + 17,
> +	[EN751221_SFC_RST]		= RST_NR_PER_BANK + 18,
> +	[EN751221_UART2_RST]		= RST_NR_PER_BANK + 19,
> +	[EN751221_GDMP_RST]		= RST_NR_PER_BANK + 20,
> +	[EN751221_FE_RST]		= RST_NR_PER_BANK + 21,
> +	[EN751221_USB_HOST_P0_RST]	= RST_NR_PER_BANK + 22,
> +	[EN751221_GSW_RST]		= RST_NR_PER_BANK + 23,
> +	[EN751221_SFC2_PCM_RST]		= RST_NR_PER_BANK + 25,
> +	[EN751221_PCIE0_RST]		= RST_NR_PER_BANK + 26,
> +	[EN751221_PCIE1_RST]		= RST_NR_PER_BANK + 27,
> +	[EN751221_CPU_TIMER_RST]	= RST_NR_PER_BANK + 28,
> +	[EN751221_PCIE_HB_RST]		= RST_NR_PER_BANK + 29,
> +	[EN751221_SIMIF_RST]		= RST_NR_PER_BANK + 30,
> +	[EN751221_XPON_MAC_RST]		= RST_NR_PER_BANK + 31,
> +
> +	/* RST_DMT */
> +	[EN751221_DMT_RST]		= 2 * RST_NR_PER_BANK + 0,
> +
> +	/* RST_USB */
> +	[EN751221_USB_PHY_P0_RST]	= 3 * RST_NR_PER_BANK + 6,
> +	[EN751221_USB_PHY_P1_RST]	= 3 * RST_NR_PER_BANK + 7,
> +};
> +
>  static int en7581_reset_register(struct device *dev, void __iomem *base,
> -				 const u16 *rst_map, int nr_resets);
> +				 const u16 *rst_map, int nr_resets,
> +				 const u16 *rst_reg_ofs);
>  
>  static u32 en7523_get_base_rate(const struct en_clk_desc *desc, u32 val)
>  {
> @@ -604,7 +703,8 @@ static int en7523_clk_hw_init(struct platform_device *pdev,
>  	en7523_register_clocks(&pdev->dev, clk_data, base, np_base);
>  
>  	return en7581_reset_register(&pdev->dev, np_base, en7523_rst_map,
> -				     ARRAY_SIZE(en7523_rst_map));
> +				     ARRAY_SIZE(en7523_rst_map),
> +				     en7581_rst_ofs);

I assume the mix of en7523 and en7581 is ok here?

>  }
>  
>  static void en7581_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
> @@ -705,7 +805,8 @@ static const struct reset_control_ops en7581_reset_ops = {
>  };
>  
>  static int en7581_reset_register(struct device *dev, void __iomem *base,
> -				 const u16 *rst_map, int nr_resets)
> +				 const u16 *rst_map, int nr_resets,
> +				 const u16 *rst_reg_ofs)
>  {
>  	struct en_rst_data *rst_data;
>  
> @@ -713,7 +814,7 @@ static int en7581_reset_register(struct device *dev, void __iomem *base,
>  	if (!rst_data)
>  		return -ENOMEM;
>  
> -	rst_data->bank_ofs = en7581_rst_ofs;
> +	rst_data->bank_ofs = rst_reg_ofs;
>  	rst_data->idx_map = rst_map;
>  	rst_data->base = base;
>  
> @@ -752,7 +853,123 @@ static int en7581_clk_hw_init(struct platform_device *pdev,
>  	writel(val | 3, base + REG_NP_SCU_PCIC);
>  
>  	return en7581_reset_register(&pdev->dev, base, en7581_rst_map,
> -				     ARRAY_SIZE(en7581_rst_map));
> +				     ARRAY_SIZE(en7581_rst_map),
> +				     en7581_rst_ofs);
> +}
> +
> +static enum en_hir get_hw_id(void __iomem *np_base)
> +{
> +	u32 val = FIELD_GET(REG_HIR_MASK, readl(np_base + REG_HIR));
> +
> +	if (val < HIR_MAX)
> +		return (enum en_hir) val;

No space with the cast.

> +
> +	return HIR_UNKNOWN;
> +}
> +
> +static void en751221_try_register_clk(struct device *dev, int key,
> +				      struct clk_hw_onecell_data *clk_data,
> +				      const char *name, u32 rate)
> +{
> +	struct clk_hw *hw;
> +
> +	hw = clk_hw_register_fixed_rate(dev, name, NULL, 0, rate);
> +	if (IS_ERR(hw) || key >= EN751221_MAX_CLKS)
> +		pr_err("Failed to register clk %s: %pe\n", name, hw);

Is %pe correct in the case when key >= EN751221_MAX_CLKS?

> +	else
> +		clk_data->hws[key] = hw;

Should the error code be returned here? I know the function has try in
it's name, however if this fails, then it still registers it.

> +}
> +
> +static void en751221_register_clocks(struct device *dev,
> +				     struct clk_hw_onecell_data *clk_data,
> +				     struct regmap *map, void __iomem *np_base)
> +{
> +	enum en_hir hid = get_hw_id(np_base);
> +	struct clk_hw *hw;
> +	u32 rate;
> +	u32 div;
> +	int err;
> +
> +	/* PCI */
> +	hw = en7523_register_pcie_clk(dev, np_base);
> +	clk_data->hws[EN751221_CLK_PCIE] = hw;
> +
> +	/* SPI */
> +	rate = EN751221_SPI_BASE;
> +	if (hid == HIR_EN7526C)
> +		rate = EN751221_SPI_BASE_EN7526C;
> +
> +	err = regmap_read(map, EN751221_REG_SPI_DIV, &div);
> +	if (err) {
> +		pr_err("Failed reading fixed clk div %s: %d\n",
> +		       "spi", err);
> +	} else {
> +		div = FIELD_GET(EN751221_REG_SPI_DIV_MASK, div) * 2;
> +		if (!div)
> +			div = 40;

Should 40 be documented a little better with a #define?

> +
> +		en751221_try_register_clk(dev, EN751221_CLK_SPI, clk_data,
> +					  "spi", rate / div);
> +	}
> +
> +	/* BUS */
> +	rate = FIELD_GET(EN751221_REG_BUS_MASK,
> +			 readl(np_base + EN751221_REG_BUS));
> +	rate *= 1000000;
> +	en751221_try_register_clk(dev, EN751221_CLK_BUS, clk_data, "bus",
> +				  rate);
> +
> +	/* CPU */
> +	en751221_try_register_clk(dev, EN751221_CLK_CPU, clk_data, "cpu",
> +				  rate * 4);
> +
> +	/* HPT */
> +	switch (hid) {
> +	case HIR_EN751221:
> +	case HIR_EN751627:
> +	case HIR_EN7526C:
> +	case HIR_EN7580:
> +	case HIR_EN7528:
> +		rate = 200000000;
> +		break;
> +	case HIR_MT7505:
> +		rate = 100000000;
> +		break;
> +	case HIR_MT751020:
> +		rate = 800000000 / 3;
> +		break;
> +	default:
> +		rate = 250000000;

Should a warning be logged here or in get_hw_id() above? hid can be set
to HIR_UNKNOWN here.

> +	}
> +	en751221_try_register_clk(dev, EN751221_CLK_HPT, clk_data, "hpt",
> +				  rate);
> +
> +	/* GSW */
> +	rate = FIELD_GET(EN751221_REG_SSR3_GSW_MASK,
> +			 readl(np_base + EN751221_REG_SSR3));
> +	en751221_try_register_clk(dev, EN751221_CLK_GSW, clk_data, "gsw",
> +				  gsw751221_base[rate]);
> +}
> +
> +static int en751221_clk_hw_init(struct platform_device *pdev,
> +				struct clk_hw_onecell_data *clk_data)
> +{
> +	struct regmap *map;
> +	void __iomem *base;
> +
> +	map = syscon_regmap_lookup_by_compatible("econet,en751221-chip-scu");
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	en751221_register_clocks(&pdev->dev, clk_data, map, base);

Again, any reason why the error handling is missing here?

Brian


> +
> +	return en7581_reset_register(&pdev->dev, base, en751221_rst_map,
> +				     ARRAY_SIZE(en751221_rst_map),
> +				     en751221_rst_ofs);
>  }
>  
>  static int en7523_clk_probe(struct platform_device *pdev)
> @@ -799,9 +1016,20 @@ static const struct en_clk_soc_data en7581_data = {
>  	.hw_init = en7581_clk_hw_init,
>  };
>  
> +static const struct en_clk_soc_data en751221_data = {
> +	.num_clocks = EN751221_MAX_CLKS,
> +	.pcie_ops = {
> +		.is_enabled = en7523_pci_is_enabled,
> +		.prepare = en7523_pci_prepare,
> +		.unprepare = en7523_pci_unprepare,
> +	},
> +	.hw_init = en751221_clk_hw_init,
> +};
> +
>  static const struct of_device_id of_match_clk_en7523[] = {
>  	{ .compatible = "airoha,en7523-scu", .data = &en7523_data },
>  	{ .compatible = "airoha,en7581-scu", .data = &en7581_data },
> +	{ .compatible = "econet,en751221-scu", .data = &en751221_data },
>  	{ /* sentinel */ }
>  };
>  
> -- 
> 2.39.5
>