Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: tc9564: duplicate clock names across chips due to using dev->id
- [High] clk: tc9564: uninitialized clk_data->num in tc9564_clk_probe()
--

commit d0d52063b982041aca91451199195cd79995784b
Author: Alex Elder <elder@riscstar.com>

clk: toshiba: introduce a TC9564 SoC clock driver

This commit adds a platform clock driver for the Toshiba TC9564 SoC,
registering its PLL, fixed-factor, and gate clocks via a syscon regmap.

> diff --git a/drivers/clk/clk-tc9564.c b/drivers/clk/clk-tc9564.c
> new file mode 100644
> index 0000000000000..8eeba85919703
> --- /dev/null
> +++ b/drivers/clk/clk-tc9564.c
> @@ -0,0 +1,288 @@

[ ... ]

> +/*
> + * The full name of every clock has a prefix "tc9564" followed by a single
> + * character ('a', 'b', etc.) that differs for each chip/device number.
> + * Each clock's "name" defines the unique portion of the clock's full name.
> + */
> +#define FULL_NAME_SIZE	24	/* Enough for formatted full name */
> +#define NAME_SIZE	12	/* Unique portion of the clock name */
> +#define FORMAT_FULL_NAME(_full_name, _id, _name)		\
> +	snprintf(_full_name, FULL_NAME_SIZE, "tc9564%c_%.*s",	\
> +		'a' + (_id), NAME_SIZE, _name)

[ ... ]

> +/* Parent is the input oscillator running at 25 MHz; PLL runs at 1250 MHz */
> +static struct clk_hw *pll_register(struct device *dev,
> +				   const struct clock_init *init)
> +{
> +	char full_name[FULL_NAME_SIZE];
> +
> +	FORMAT_FULL_NAME(full_name, dev->id, init->name);
> +
> +	return devm_clk_hw_register_fixed_factor_index(dev, full_name, 0, 0,
> +						       init->mult, 1);
> +}
> +
> +/* The PLL is the parent of all fixed clocks */
> +static struct clk_hw *fixed_register(struct device *dev,
> +				     const struct clock_init *init)
> +{
> +	struct tc9564_clock *tc9564_clock = dev_get_drvdata(dev);
> +	char full_name[FULL_NAME_SIZE];
> +	struct clk_hw *pll_hw;
> +
> +	FORMAT_FULL_NAME(full_name, dev->id, init->name);

[ ... ]

> +static struct clk_hw *gate_register(struct device *dev,
> +				    const struct clock_init *init)
> +{
> +	struct tc9564_clock *tc9564_clock = dev_get_drvdata(dev);
> +	struct clk_init_data clk_init = { };
> +	const struct clk_hw *parent_hw;
> +	char full_name[FULL_NAME_SIZE];
> +	struct gate_clock *clock;
> +	int ret;
> +
> +	clock = devm_kzalloc_obj(dev, *clock);
> +	if (!clock)
> +		return ERR_PTR(-ENOMEM);
> +
> +	FORMAT_FULL_NAME(full_name, dev->id, init->name);

[Severity: High]
Will passing dev->id to FORMAT_FULL_NAME() in pll_register(),
fixed_register(), and gate_register() cause clock registration to fail with
-EEXIST when more than one TC9564 device is present?

The clock MFD cell is registered as a platform device in
tc9564_config_syscon_probe():

drivers/soc/toshiba/tc9564-syscon.c:tc9564_config_syscon_probe() {
	...
	return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
				    tc9564_config_syscon_cells,
				    ARRAY_SIZE(tc9564_config_syscon_cells),
				    NULL, 0, NULL);
}

For platform devices, the platform bus stores the instance ID in pdev->id
(struct platform_device::id), while dev->id (struct device::id) is never set
and remains 0 for every instance. As a result, every TC9564 device formats
its clocks with the "tc9564a_" prefix (such as "tc9564a_PLL"), so probing a
second TC9564 device collides in __clk_core_init() when registering its
clocks in the global clock namespace.

Note that pdev->id cannot be passed directly to 'a' + (_id) either, because
PLATFORM_DEVID_AUTO allocates IDs from the system-wide platform_devid_ida
shared across all auto-numbered platform devices rather than a 0-based
per-chip index.

> +
> +	parent_hw = tc9564_clock->clk_data->hws[init->parent];
> +
> +	clk_init.name = full_name;
> +	clk_init.ops = &gate_clk_ops;
> +	clk_init.parent_hws = &parent_hw;
> +	clk_init.num_parents = 1;
> +
> +	clock->hw.init = &clk_init;
> +	clock->init = init;
> +
> +	ret = devm_clk_hw_register(dev, &clock->hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return &clock->hw;
> +}
> +
> +static int tc9564_clk_probe(struct platform_device *pdev)
> +{
> +	struct clk_hw_onecell_data *clk_data;
> +	struct tc9564_clock *tc9564_clock;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *parent_np;
> +	int ret;
> +
> +	parent_np = dev_of_node(dev->parent);
> +	if (!parent_np)
> +		return dev_err_probe(dev, -EINVAL, "no devicetree node\n");
> +
> +	tc9564_clock = devm_kzalloc_obj(dev, *tc9564_clock);
> +	if (!tc9564_clock)
> +		return -ENOMEM;
> +
> +	/* Note: MMIO regmap (no error checking needed when used) */
> +	tc9564_clock->regmap = device_node_to_regmap(parent_np);
> +	if (IS_ERR(tc9564_clock->regmap))
> +		return dev_err_probe(dev, PTR_ERR(tc9564_clock->regmap),
> +				     "failed to get config regmap\n");
> +
> +	clk_data = devm_kzalloc_flex(dev, *tc9564_clock->clk_data, hws,
> +				     CLOCK_COUNT);
> +	if (!clk_data)
> +		return -ENOMEM;
> +	tc9564_clock->clk_data = clk_data;
> +
> +	dev_set_drvdata(dev, tc9564_clock);
> +
> +	/* Initialize all the clocks */
> +	for (u32 i = 0; i < clk_data->num; i++) {

[Severity: High]
Does clk_data->num remain 0 in tc9564_clk_probe() when building with
compilers that do not support __builtin_counted_by_ref?

devm_kzalloc_flex() populates the __counted_by counter via
__set_flex_counter():

include/linux/compiler_types.h:
#if __has_builtin(__builtin_counted_by_ref) && \
    !defined(CONFIG_CC_HAS_BROKEN_COUNTED_BY_REF)
#define __flex_counter(FAM)	__builtin_counted_by_ref(FAM)
#else
#define __flex_counter(FAM)	((void *)NULL)
#endif

include/linux/overflow.h:
#define __set_flex_counter(FAM, COUNT)				\
({								\
	*_Generic(__flex_counter(FAM),				\
		  void *:  &(size_t){ 0 },			\
		  default: __flex_counter(FAM)) = (COUNT);	\
})

On compilers without a working __builtin_counted_by_ref (such as GCC < 15.1
or Clang < 22.1), __set_flex_counter() writes COUNT into a temporary compound
literal and leaves clk_data->num as 0. Without an explicit
clk_data->num = CLOCK_COUNT assignment, the loop in tc9564_clk_probe()
registers zero clocks and of_clk_hw_onecell_get() rejects all clock lookups
with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104004332.2141282-1-elder@riscstar.com?part=3