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

[Xuyang Dong]

> 
> > 
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > - [Low] Global static clock arrays are modified during probe without locking, breaking support for multiple instances.
> > - [Low] The Kconfig symbol `COMMON_CLK_EIC7700_HSP` lacks the `default ARCH_ESWIN` property, which is inconsistent with the base clock driver.
> > --
> > 
> > commit 5c87f865ca8ae746d80ff71fdbbce73f7be502ec
> > Author: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> > 
> > clk: eswin: Add eic7700 HSP clock driver
> > 
> > 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".
> > 
> > > diff --git a/drivers/clk/eswin/Kconfig b/drivers/clk/eswin/Kconfig
> > > index 0406ec499ec9c..37857bd773356 100644
> > > --- a/drivers/clk/eswin/Kconfig
> > > +++ b/drivers/clk/eswin/Kconfig
> > [ ... ]
> > > @@ -13,3 +13,16 @@ 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
> > 
> > This isn't a bug, but should this config have default ARCH_ESWIN to match
> > the base clock driver? Without it, the high-speed peripheral clocks might
> > be unexpectedly disabled by default on ESWIN platforms.
> > 
> 
> Hi Sashiko,
> 
> The HSP clock reset device can be built as a kernel module. 
> However, if 'default ARCH_ESWIN' is set, it will be compiled directly 
> into the kernel instead of as a module.
> 
> > > +	select AUXILIARY_BUS
> > > +	select COMMON_CLK_EIC7700
> > > +	select RESET_EIC7700_HSP if RESET_CONTROLLER
> > > +	select REGMAP_MMIO
> > 
> > [ ... ]
> > > diff --git a/drivers/clk/eswin/clk-eic7700-hsp.c b/drivers/clk/eswin/clk-eic7700-hsp.c
> > > new file mode 100644
> > > index 0000000000000..3383aa64948a6
> > > --- /dev/null
> > > +++ b/drivers/clk/eswin/clk-eic7700-hsp.c
> > [ ... ]
> > > +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 int eic7700_hsp_clk_probe(struct platform_device *pdev)
> > > +{
> > > +	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");
> > > +
> > > +	regmap = devm_regmap_init_mmio(dev, data->base,
> > > +				       &eic7700_hsp_regmap_config);
> > > +	if (IS_ERR(regmap))
> > > +		return dev_err_probe(dev, PTR_ERR(regmap),
> > > +				     "failed to get regmap!\n");
> > > +
> > > +	ret = eswin_clk_register_fixed_factor(dev, eic7700_hsp_factor_clks,
> > > +					      ARRAY_SIZE(eic7700_hsp_factor_clks),
> > > +					      data);
> > 
> > Is it safe to pass the global static array eic7700_hsp_factor_clks to this
> > function?
> > 
> > Looking at eswin_clk_register_fixed_factor() in drivers/clk/eswin/clk.c, it
> > modifies the array during probe:
> > 
> > int eswin_clk_register_fixed_factor(...)
> > {
> >     ...
> >         clks[i].hw = *clk_hw;
> >         data->clk_data.hws[clks[i].id] = clk_hw;
> >     ...
> > }
> > 
> > If multiple instances of this driver probe concurrently or defer, could this
> > data race corrupt the CCF parent-child linkages? The same pattern appears
> > to affect eic7700_hsp_gate_clks and eic7700_hsp_mux_clks.
> > 
> 
> No. There is only one compatible string in the DTS, so there will only be
> a single driver instance. Therefore, concurrent probing cannot occur, and 
> the CCF parent-child linkages will not be corrupted by a data race. 
> 

Hi Sashiko,

I have clarified your comments above. Please let me know if my replies 
are acceptable. 
If there are no further issues, I will send the next version of the patch.

Best regards,
Xuyang Dong

> 
> > -- 
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260519081431.1424-1-dongxuyang@xxxxxxxxxxxxxxxxxx?part=2