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

["Xuyang Dong" <dongxuyang@eswincomputing.com>]

> 
> On Tue, May 12, 2026 at 10:07:47AM +0800, Xuyang Dong wrote:
> > 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 | 338 ++++++++++++++++++++++++++++
> >  3 files changed, 351 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..0d5bd5b705dc
> > --- /dev/null
> > +++ b/drivers/clk/eswin/clk-eic7700-hsp.c
> > @@ -0,0 +1,338 @@
> > +// 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/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;
> > +	struct regmap *regmap;
> > +	unsigned int reg;
> > +	unsigned int ref_reg;
> > +	const char *name;
> > +	const struct clk_parent_data *parent_data;
> > +	unsigned long flags;
> > +	unsigned int offset;
> > +	unsigned int ref_offset;
> > +	u8 bit_idx;
> > +};
> > +
> > +static const struct regmap_config eic7700_hsp_regmap_config = {
> > +	.reg_bits = 32,
> > +	.val_bits = 32,
> > +	.max_register = 0x1ffc,
> > +	.reg_stride = 4,
> > +	.fast_io = true,
> > +	.use_raw_spinlock = true,
> > +};
> > +
> > +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, bool enable)
> > +{
> > +	struct eic7700_hsp_clk_gate *gate = to_gate_clk(hw);
> > +
> > +	if (enable) {
> > +		/*
> > +		 * Hardware bug: The USB reference clock must be 24MHz.
> > +		 * The default register value after reset is invalid.
> > +		 * Workaround: Rewrite the correct value before enabling
> > +		 * the USB gate clock.
> > +		 */
> > +		regmap_update_bits(gate->regmap, gate->ref_reg, 0x3f,
> > +				   USB_REF_XTAL24M);
> > +	}
> > +	regmap_assign_bits(gate->regmap, gate->reg, BIT(gate->bit_idx), enable);
> > +}
> > +
> > +static int hsp_clk_gate_enable(struct clk_hw *hw)
> > +{
> > +	hsp_clk_gate_endisable(hw, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static void hsp_clk_gate_disable(struct clk_hw *hw)
> > +{
> > +	hsp_clk_gate_endisable(hw, false);
> > +}
> > +
> > +static int hsp_clk_gate_is_enabled(struct clk_hw *hw)
> > +{
> > +	struct eic7700_hsp_clk_gate *gate = to_gate_clk(hw);
> > +	unsigned int val;
> > +
> > +	regmap_read(gate->regmap, gate->reg, &val);
> > +
> > +	return !!(val & BIT(gate->bit_idx));
> 
> If the regmap_read() fails, then val will be uninitialized.

Hi Brian and Sashiko,

I will update the following code to address this in next version.

+	int ret;
+
+	ret = regmap_read(gate->regmap, gate->reg, &val);
+	if (ret != 0)
+		return ret;

Does this change look acceptable to you?

Best regards,
Xuyang Dong

> 
> With that fixed:
> 
> Reviewed-by: Brian Masney <bmasney@redhat.com>