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. 

Best regards,
Xuyang Dong

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