Re: [PATCH v3 3/3] clk: ultrarisc: Add DP1000 clock driver

[Jia Wang]

On 2026-07-16 14:17 -0400, Brian Masney wrote:
> Hi Jia,
> 
> I have one minor question below.
> 
> On Tue, Jul 14, 2026 at 09:35:09AM +0800, Jia Wang via B4 Relay wrote:
> > From: Jia Wang <wangjia@xxxxxxxxxxxxx>
> > 
> > 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.
> > 
> > Signed-off-by: Jia Wang <wangjia@xxxxxxxxxxxxx>
> > ---
> >  MAINTAINERS                           |   1 +
> >  drivers/clk/Kconfig                   |   1 +
> >  drivers/clk/Makefile                  |   1 +
> >  drivers/clk/ultrarisc/Kconfig         |  18 ++
> >  drivers/clk/ultrarisc/Makefile        |   4 +
> >  drivers/clk/ultrarisc/clk-dp1000.c    | 154 +++++++++++++
> >  drivers/clk/ultrarisc/clk-ultrarisc.c | 407 ++++++++++++++++++++++++++++++++++
> >  drivers/clk/ultrarisc/clk-ultrarisc.h |  71 ++++++
> >  8 files changed, 657 insertions(+)
> 
> [snip]
> 
> > 
> > +static int ultrarisc_clk_register_dividers(struct platform_device *pdev,
> > +					   struct clk_hw_onecell_data *clk_data,
> > +					   const struct ultrarisc_clk_soc_data *soc_data,
> > +					   void __iomem *base,
> > +					   spinlock_t *lock)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	u32 i;
> > +
> > +	for (i = 0; i < soc_data->num_dividers; i++) {
> > +		const struct ultrarisc_divider_desc *desc;
> > +		struct clk_hw *parent_hw;
> > +		struct clk_hw *hw;
> > +
> > +		desc = &soc_data->dividers[i];
> > +		if (desc->id >= clk_data->num || desc->parent_id >= clk_data->num)
> > +			return -EINVAL;
> > +
> > +		parent_hw = clk_data->hws[desc->parent_id];
> > +		if (!parent_hw)
> > +			return -EINVAL;
> > +
> > +		hw = ultrarisc_clk_register_divider(dev, desc, parent_hw, base,
> > +						    lock);
> > +		if (IS_ERR(hw))
> > +			return PTR_ERR(hw);
> > +
> > +		if (desc->max_rate) {
> > +			unsigned long rate;
> > +
> > +			clk_hw_set_rate_range(hw, 0, desc->max_rate);
> > +
> > +			rate = clk_hw_get_rate(hw);
> > +			if (rate > desc->max_rate)
> > +				dev_warn(dev, "%s rate %lu exceeds max %lu\n",
> > +					 desc->name, rate, desc->max_rate);
> 
> At this point in the probe, the clock tree may not be fully propagated
> yet. Is this check needed?
> 

Agreed, I will drop this diagnostic check in the next version and keep
only the rate range constraint.

> Everything else in the driver looks fine to me.
> 
> Brian
> 
> 

Best regards,
Jia Wang