Re: [PATCH 2/2] clk: ultrarisc: add DP1000 clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing clocks in `clk_data->hws` evaluate to `NULL` instead of an error pointer, creating dummy clocks.
- [High] Read-modify-write operations on clock control registers fail to mask the self-clearing load bit, risking spurious loads and preventing divider updates.
- [Low] Missing `<linux/module.h>` include.
--

commit 8cba5aa205eda7abb58c5b279348bd81f69fdc45
Author: Jia Wang <wangjia@ultrarisc.com>

clk: ultrarisc: add DP1000 clock driver
    
Add a clock driver for the UltraRISC DP1000 SoC.
    
The clock tree is driven by a SYSPLL and provides fixed-factor clocks for
the subsystem and PCIe, divider-based root clocks for GMAC and the UART,
I2C, and SPI blocks, and per-instance gate clocks for UART0-3, I2C0-3,
and SPI0-1.

> diff --git a/drivers/clk/ultrarisc/clk-ultrarisc.c b/drivers/clk/ultrarisc/clk-ultrarisc.c
> index 0000000000000..2b970565a4be8
> --- /dev/null
> +++ b/drivers/clk/ultrarisc/clk-ultrarisc.c
> @@ -0,0 +1,417 @@
[ ... ]
> +#include <linux/bitfield.h>
> +#include <linux/clk-provider.h>

[Severity: Low]
Should <linux/module.h> be explicitly included here?

The file uses module macros like MODULE_DESCRIPTION and EXPORT_SYMBOL_GPL(),
and relying on implicit inclusion can sometimes cause build failures.

[ ... ]
> +static int ultrarisc_divider_set_rate(struct clk_hw *hw, unsigned long rate,
> +				      unsigned long parent_rate)
> +{
[ ... ]
> +	scoped_guard(spinlock_irqsave, divider->lock) {
> +		val = readl_relaxed(divider->reg);
> +		val &= ~(clk_div_mask(divider->width) << divider->shift);
> +		val |= value << divider->shift;
> +		writel_relaxed(val, divider->reg);

[Severity: High]
Does this read-modify-write sequence need to mask out divider_clk->load_mask
from the read value before the first write?

If the hardware hasn't cleared the bit from a previous operation, the first
write could write it as 1. Then the second write (below) would also write 1,
failing to provide the required 0->1 transition to trigger the hardware.

> +
> +		if (divider_clk->load_mask) {
> +			/*
> +			 * The divider update is triggered by writing 1 to the
> +			 * self-clearing load bit after programming the new
> +			 * divider field.
> +			 */
> +			writel_relaxed(val | divider_clk->load_mask,
> +				       divider->reg);
> +		}
> +	}
> +
> +	return 0;
> +}

[ ... ]
> +static struct clk_hw *
> +ultrarisc_clk_register_divider(struct device *dev,
> +			       const struct ultrarisc_divider_desc *desc,
> +			       struct clk_hw *parent_hw, void __iomem *base,
> +			       spinlock_t *lock)
> +{
[ ... ]
> +	if (desc->gate_bit != ULTRARISC_CLK_NO_GATE) {
> +		divider->gate.reg = reg;
> +		divider->gate.bit_idx = desc->gate_bit;
> +		divider->gate.flags = desc->gate_flags;
> +		divider->gate.lock = lock;
> +		gate_hw = &divider->gate.hw;
> +		gate_ops = &clk_gate_ops;

[Severity: High]
Can using the standard clk_gate_ops here cause spurious loads?

The standard gate ops perform a generic read-modify-write. If a gate
operation executes while the load_mask bit is still 1, it will blindly
write 1 back to the load bit, potentially triggering a spurious hardware
reload or corrupting the update state machine.

> +	}
> +
> +	return devm_clk_hw_register_composite_pdata(dev, desc->name, &pdata, 1,
> +						    NULL, NULL,
> +						    &divider->divider.hw,
> +						    &ultrarisc_divider_ops,
> +						    gate_hw, gate_ops,
> +						    CLK_GET_RATE_NOCACHE);
> +}

[ ... ]
> +static int ultrarisc_clk_register_fixed_factors(struct device *dev,
> +						struct clk_hw_onecell_data *clk_data,
> +						const struct ultrarisc_clk_soc_data *soc_data)
> +{
[ ... ]
> +		parent_hw = clk_data->hws[desc->parent_id];
> +		if (!parent_hw)
> +			return -EINVAL;

[Severity: High]
If the clk_data initialization (below) is fixed to use error pointers
instead of NULL, should this validation check use IS_ERR_OR_NULL() or
IS_ERR() to avoid dereferencing an ERR_PTR?

> +
> +		hw = devm_clk_hw_register_fixed_factor_parent_hw(dev, desc->name,
> +								 parent_hw,

[ ... ]
> +int ultrarisc_clk_probe(struct platform_device *pdev,
> +			const struct ultrarisc_clk_soc_data *soc_data)
> +{
[ ... ]
> +	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws,
> +						 soc_data->num_clks),
> +				GFP_KERNEL);

[Severity: High]
Since this array is zero-initialized, any unpopulated clock IDs will remain
NULL. Does this need to be explicitly filled with ERR_PTR(-ENOENT)?

When a consumer requests an invalid clock index, of_clk_hw_onecell_get()
returns this NULL pointer. The clock framework considers NULL a valid
optional dummy clock, allowing consumer drivers to silently succeed at
enabling the clock instead of failing. If they then access peripheral
registers without the clock actually being enabled, it could trigger a bus
fault.

> +	if (!clk_data)
> +		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-ultrarisc-clock-v1-0-2d93ebb4cc13@ultrarisc.com?part=2