Re: [PATCH v2 2/3] clk: eswin: Add eic7700 HSP clock driver

Brian Masney
2026-04-21T15:21:15Z

Hi Xuyang,

On Mon, Apr 20, 2026 at 05:47:34PM +0800, dongxuyang@eswincomputing.com wrote:
> From: Xuyang Dong <dongxuyang@eswincomputing.com>
>
> Add driver for the ESWIN EIC7700 high-speed peripherals system
> clock controller and register an auxiliary device for system
> reset controller which is named as "hsp-reset".
>
> Signed-off-by: Xuyang Dong <dongxuyang@eswincomputing.com>
> ---
> drivers/clk/eswin/Kconfig | 12 +
> drivers/clk/eswin/Makefile | 1 +
> drivers/clk/eswin/clk-eic7700-hsp.c | 385 ++++++++++++++++++++++++++++
> 3 files changed, 398 insertions(+)
> create mode 100644 drivers/clk/eswin/clk-eic7700-hsp.c
>
> diff --git a/drivers/clk/eswin/Kconfig b/drivers/clk/eswin/Kconfig
> index 0406ec499ec9..e6cc2a407bac 100644
> --- a/drivers/clk/eswin/Kconfig
> +++ b/drivers/clk/eswin/Kconfig
> @@ -13,3 +13,15 @@ config COMMON_CLK_EIC7700
> SoC. The clock controller generates and supplies clocks to various
> peripherals within the SoC.
> Say yes here to support the clock controller on the EIC7700 SoC.
> +
> +config COMMON_CLK_EIC7700_HSP
> + tristate "EIC7700 HSP Clock Driver"
> + depends on ARCH_ESWIN || COMPILE_TEST
> + select AUXILIARY_BUS
> + select COMMON_CLK_EIC7700
> + select RESET_EIC7700_HSP if RESET_CONTROLLER
> + help
> + This driver provides support for clock controller on ESWIN EIC7700
> + HSP. The clock controller generates and supplies clocks to high
> + speed peripherals within the SoC.
> + Say yes here to support the clock controller on the EIC7700 HSP.
> diff --git a/drivers/clk/eswin/Makefile b/drivers/clk/eswin/Makefile
> index 4a7c2af82164..21a09a3396df 100644
> --- a/drivers/clk/eswin/Makefile
> +++ b/drivers/clk/eswin/Makefile
> @@ -6,3 +6,4 @@
> obj-$(CONFIG_COMMON_CLK_ESWIN) += clk.o
>
> obj-$(CONFIG_COMMON_CLK_EIC7700) += clk-eic7700.o
> +obj-$(CONFIG_COMMON_CLK_EIC7700_HSP) += clk-eic7700-hsp.o
> diff --git a/drivers/clk/eswin/clk-eic7700-hsp.c b/drivers/clk/eswin/clk-eic7700-hsp.c
> new file mode 100644
> index 000000000000..d8f5493b45e7
> --- /dev/null
> +++ b/drivers/clk/eswin/clk-eic7700-hsp.c
> @@ -0,0 +1,385 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2026, Beijing ESWIN Computing Technology Co., Ltd..
> + * All rights reserved.
> + *
> + * ESWIN EIC7700 HSP Clock Driver
> + *
> + * Authors: Xuyang Dong <dongxuyang@eswincomputing.com>
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/eswin,eic7700-hspcrg.h>
> +
> +#include "common.h"
> +
> +#define EIC7700_HSP_SATA_REG 0x300
> +#define EIC7700_HSP_MSHC0_REG 0x510
> +#define EIC7700_HSP_MSHC1_REG 0x610
> +#define EIC7700_HSP_MSHC2_REG 0x710
> +#define EIC7700_HSP_USB0_REG 0x800
> +#define EIC7700_HSP_USB0_REF_REG 0x83c
> +#define EIC7700_HSP_USB1_REG 0x900
> +#define EIC7700_HSP_USB1_REF_REG 0x93c
> +
> +#define USB_REF_XTAL24M 0x2a
> +#define EIC7700_HSP_NR_CLKS (EIC7700_HSP_CLK_GATE_SATA + 1)
> +
> +struct eic7700_hsp_clk_gate {
> + struct clk_hw hw;
> + unsigned int id;
> + void __iomem *reg;
> + void __iomem *ref_reg;
> + const char *name;
> + const struct clk_parent_data *parent_data;
> + unsigned long flags;
> + unsigned long offset;
> + unsigned long ref_offset;
> + u8 bit_idx;
> + u8 gate_flags;

Is this used anywhere?

> + spinlock_t *lock; /* protect register read-modify-write cycle */
> +};
> +
> +/*
> + * The USB clock gate (hsp_clk_gate_endisable) and the reset driver both
> + * perform read-modify-write cycles on registers 0x800 and 0x900. Use
> + * custom regmap lock callbacks so that regmap operations hold data->lock
> + * with IRQs disabled, the same lock the clock gate path uses, preventing
> + * concurrent RMW races on those shared registers.
> + */
> +struct eic7700_hsp_regmap_lock {
> + spinlock_t *lock; /* protect register read-modify-write cycle */
> + unsigned long flags;
> +};
> +
> +static void eic7700_hsp_regmap_lock_fn(void *arg)

You can add an __acquires to the declaration here. See
dio48e_regmap_lock() in drivers/gpio/gpio-104-dio-48e.c for an example.

> +{
> + struct eic7700_hsp_regmap_lock *ctx = arg;
> +
> + spin_lock_irqsave(ctx->lock, ctx->flags);
> +}
> +
> +static void eic7700_hsp_regmap_unlock_fn(void *arg)
> +{
> + struct eic7700_hsp_regmap_lock *ctx = arg;
> +
> + spin_unlock_irqrestore(ctx->lock, ctx->flags);
> +}
> +
> +static inline struct eic7700_hsp_clk_gate *to_gate_clk(struct clk_hw *hw)
> +{
> + return container_of(hw, struct eic7700_hsp_clk_gate, hw);
> +}
> +
> +#define EIC7700_HSP_GATE(_id, _name, _pdata, _flags, _offset, _idx, \
> + _ref_offset) \
> + { \
> + .id = _id, \
> + .name = _name, \
> + .parent_data = _pdata, \
> + .flags = _flags, \
> + .offset = _offset, \
> + .ref_offset = _ref_offset, \
> + .bit_idx = _idx, \
> + }
> +
> +static void hsp_clk_gate_endisable(struct clk_hw *hw, int enable)
> +{
> + struct eic7700_hsp_clk_gate *gate = to_gate_clk(hw);
> + u32 reg;
> +
> + guard(spinlock_irqsave)(gate->lock);
> +
> + reg = readl(gate->reg);
> +
> + if (enable)
> + reg |= BIT(gate->bit_idx);
> + else
> + reg &= ~BIT(gate->bit_idx);
> +
> + /*
> + * Hardware bug: The reference clock is 24MHz, but the reference clock
> + * register reset to an incorrect default value.
> + * Workaround: Rewrite the correct value before enabling/disabling
> + * the gate clock.
> + */
> + writel(USB_REF_XTAL24M, gate->ref_reg);
> + writel(reg, gate->reg);

Does this only needed on the enable path? Or is this still needed on the
disable path if there are other clocks referencing the parent?

> +}
> +
> +static int hsp_clk_gate_enable(struct clk_hw *hw)
> +{
> + hsp_clk_gate_endisable(hw, 1);
> +
> + return 0;
> +}
> +
> +static void hsp_clk_gate_disable(struct clk_hw *hw)
> +{
> + hsp_clk_gate_endisable(hw, 0);
> +}
> +
> +static int hsp_clk_gate_is_enabled(struct clk_hw *hw)
> +{
> + struct eic7700_hsp_clk_gate *gate = to_gate_clk(hw);
> + u32 reg;
> +
> + reg = readl(gate->reg);
> + reg &= BIT(gate->bit_idx);
> +
> + return reg ? 1 : 0;

You can simplify this to:

return !!(readl(gate->reg) & BIT(gate->bit_idx));

> +}
> +
> +static const struct clk_ops hsp_clk_gate_ops = {
> + .enable = hsp_clk_gate_enable,
> + .disable = hsp_clk_gate_disable,
> + .is_enabled = hsp_clk_gate_is_enabled,
> +};
> +
> +static struct clk_hw *
> +hsp_clk_register_gate(struct device *dev, unsigned int id, const char *name,
> + const struct clk_parent_data *parent_data,
> + unsigned long flags, void __iomem *reg,
> + void __iomem *ref_reg, u8 bit_idx, u8 clk_gate_flags,
> + spinlock_t *lock)
> +{
> + struct eic7700_hsp_clk_gate *gate;
> + struct clk_init_data init = {};
> + struct clk_hw *hw;
> + int ret;
> +
> + gate = devm_kzalloc(dev, sizeof(*gate), GFP_KERNEL);
> + if (!gate)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = name;
> + init.ops = &hsp_clk_gate_ops;
> + init.flags = flags;
> + init.parent_data = parent_data;
> + init.num_parents = 1;
> +
> + gate->id = id;
> + gate->reg = reg;
> + gate->ref_reg = ref_reg;
> + gate->bit_idx = bit_idx;
> + gate->gate_flags = clk_gate_flags;
> + gate->lock = lock;
> + gate->hw.init = &init;
> +
> + hw = &gate->hw;
> + ret = devm_clk_hw_register(dev, hw);
> + if (ret)
> + hw = ERR_PTR(ret);
> +
> + return hw;
> +}
> +
> +static const struct clk_parent_data hsp_cfg[] = {
> + { .index = 0 }
> +};
> +
> +static const struct clk_parent_data hsp_mmc[] = {
> + { .index = 1 }
> +};
> +
> +static const struct clk_parent_data hsp_usb_sata[] = {
> + { .index = 2 }
> +};
> +
> +static struct eswin_fixed_factor_clock eic7700_hsp_factor_clks[] = {
> + ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_CFG_DIV2, "factor_hsp_cfg_div2",
> + hsp_cfg, 1, 2, 0),
> + ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_CFG_DIV4, "factor_hsp_cfg_div4",
> + hsp_cfg, 1, 4, 0),
> + ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_MMC_DIV10, "factor_hsp_mmc_div10",
> + hsp_mmc, 1, 10, 0),
> +};
> +
> +static struct eswin_gate_clock eic7700_hsp_gate_clks[] = {
> + ESWIN_GATE(EIC7700_HSP_CLK_GATE_SATA, "gate_clk_hsp_sata", hsp_usb_sata,
> + CLK_SET_RATE_PARENT, EIC7700_HSP_SATA_REG, 28, 0),
> + ESWIN_GATE(EIC7700_HSP_CLK_GATE_MSHC0_TMR, "gate_clk_hsp_mshc0_tmr",
> + hsp_mmc, CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC0_REG, 8, 0),
> + ESWIN_GATE(EIC7700_HSP_CLK_GATE_MSHC1_TMR, "gate_clk_hsp_mshc1_tmr",
> + hsp_mmc, CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC1_REG, 8, 0),
> + ESWIN_GATE(EIC7700_HSP_CLK_GATE_MSHC2_TMR, "gate_clk_hsp_mshc2_tmr",
> + hsp_mmc, CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC2_REG, 8, 0),
> +};
> +
> +static struct eic7700_hsp_clk_gate eic7700_hsp_spec_gate_clks[] = {
> + EIC7700_HSP_GATE(EIC7700_HSP_CLK_GATE_USB0, "gate_clk_hsp_usb0",
> + hsp_usb_sata, CLK_SET_RATE_PARENT,
> + EIC7700_HSP_USB0_REG, 28, EIC7700_HSP_USB0_REF_REG),
> + EIC7700_HSP_GATE(EIC7700_HSP_CLK_GATE_USB1, "gate_clk_hsp_usb1",
> + hsp_usb_sata, CLK_SET_RATE_PARENT,
> + EIC7700_HSP_USB1_REG, 28, EIC7700_HSP_USB1_REF_REG),
> +};
> +
> +static const struct clk_parent_data mux_mmc_3mux1_p[] = {
> + { .fw_name = "cfg" },
> + { .hw = &eic7700_hsp_factor_clks[0].hw },
> + { .hw = &eic7700_hsp_factor_clks[1].hw },
> +};
> +
> +static const struct clk_parent_data mux_mmc_2mux1_p[] = {
> + { .fw_name = "mmc" },
> + { .hw = &eic7700_hsp_factor_clks[2].hw },
> +};
> +
> +static u32 mux_mmc_3mux1_tbl[] = { 0x0, 0x1, 0x3 };
> +
> +static struct eswin_mux_clock eic7700_hsp_mux_clks[] = {
> + ESWIN_MUX_TBL(EIC7700_HSP_CLK_MUX_EMMC_3MUX1, "mux_hsp_emmc_3mux1",
> + mux_mmc_3mux1_p, ARRAY_SIZE(mux_mmc_3mux1_p),
> + CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC0_REG, 16, 2, 0,
> + mux_mmc_3mux1_tbl),
> + ESWIN_MUX_TBL(EIC7700_HSP_CLK_MUX_SD0_3MUX1, "mux_hsp_sd0_3mux1",
> + mux_mmc_3mux1_p, ARRAY_SIZE(mux_mmc_3mux1_p),
> + CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC1_REG, 16, 2, 0,
> + mux_mmc_3mux1_tbl),
> + ESWIN_MUX_TBL(EIC7700_HSP_CLK_MUX_SD1_3MUX1, "mux_hsp_sd1_3mux1",
> + mux_mmc_3mux1_p, ARRAY_SIZE(mux_mmc_3mux1_p),
> + CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC2_REG, 16, 2, 0,
> + mux_mmc_3mux1_tbl),
> + ESWIN_MUX(EIC7700_HSP_CLK_MUX_EMMC_CQE_2MUX1, "mux_hsp_emmc_cqe_2mux1",
> + mux_mmc_2mux1_p, ARRAY_SIZE(mux_mmc_2mux1_p),
> + CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC0_REG, 0, 1, 0),
> + ESWIN_MUX(EIC7700_HSP_CLK_MUX_SD0_CQE_2MUX1, "mux_hsp_sd0_cqe_2mux1",
> + mux_mmc_2mux1_p, ARRAY_SIZE(mux_mmc_2mux1_p),
> + CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC1_REG, 0, 1, 0),
> + ESWIN_MUX(EIC7700_HSP_CLK_MUX_SD1_CQE_2MUX1, "mux_hsp_sd1_cqe_2mux1",
> + mux_mmc_2mux1_p, ARRAY_SIZE(mux_mmc_2mux1_p),
> + CLK_SET_RATE_PARENT, EIC7700_HSP_MSHC2_REG, 0, 1, 0),
> +};
> +
> +static struct eswin_clk_info eic7700_hsp_clks[] = {
> + ESWIN_GATE_TYPE(EIC7700_HSP_CLK_GATE_EMMC, "gate_clk_hsp_emmc",
> + EIC7700_HSP_CLK_MUX_EMMC_3MUX1,
> + CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + EIC7700_HSP_MSHC0_REG, 24, 0),
> + ESWIN_GATE_TYPE(EIC7700_HSP_CLK_GATE_SD0, "gate_clk_hsp_sd0",
> + EIC7700_HSP_CLK_MUX_SD0_3MUX1,
> + CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + EIC7700_HSP_MSHC1_REG, 24, 0),
> + ESWIN_GATE_TYPE(EIC7700_HSP_CLK_GATE_SD1, "gate_clk_hsp_sd1",
> + EIC7700_HSP_CLK_MUX_SD1_3MUX1,
> + CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + EIC7700_HSP_MSHC2_REG, 24, 0),
> +};
> +
> +static int eic7700_hsp_clk_probe(struct platform_device *pdev)
> +{
> + struct eic7700_hsp_regmap_lock *lock_ctx;
> + struct device *dev = &pdev->dev;
> + struct auxiliary_device *adev;
> + struct eswin_clock_data *data;
> + struct regmap *regmap;
> + struct clk_hw *hw;
> + int i, ret;
> +
> + data = eswin_clk_init(pdev, EIC7700_HSP_NR_CLKS);
> + if (IS_ERR(data))
> + return dev_err_probe(dev, PTR_ERR(data),
> + "failed to get clk data!\n");
> +
> + lock_ctx = devm_kzalloc(dev, sizeof(*lock_ctx), GFP_KERNEL);
> + if (!lock_ctx)
> + return dev_err_probe(dev, -ENOMEM,
> + "failed to alloc regmap lock ctx\n");
> +
> + lock_ctx->lock = &data->lock;
> + const struct regmap_config eic7700_hsp_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .max_register = 0x1ffc,
> + .reg_stride = 4,
> + .lock = eic7700_hsp_regmap_lock_fn,
> + .unlock = eic7700_hsp_regmap_unlock_fn,
> + .lock_arg = lock_ctx,
> + };

So this is valid C99 with the const struct declared here. This can't be
moved outside the probe function because of the lock_ctx. I would drop
the const, declare eic7700_hsp_regmap_config as a regular variable at
the top. Similar to what's done in drivers/gpio/gpio-104-dio-48e.c with
dio48e_regmap_config.

Brian