Re: [PATCH 10/17] clk: sunxi=ng: add support for R329 R-CCU

[Jernej Škrabec]

Dne sreda, 25. avgust 2021 ob 16:50:27 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Fri, Aug 20, 2021 at 06:34:38AM +0200, Jernej Škrabec wrote:
> > > > +static void __init sun50i_r329_r_ccu_setup(struct device_node *node)
> > > > +{
> > > > +	void __iomem *reg;
> > > > +	u32 val;
> > > > +	int i;
> > > > +
> > > > +	reg = of_io_request_and_map(node, 0, of_node_full_name(node));
> > > > +	if (IS_ERR(reg)) {
> > > > +		pr_err("%pOF: Could not map clock registers\n", node);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	/* Enable the lock bits and the output enable bits on all PLLs */
> > > > +	for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
> > > > +		val = readl(reg + pll_regs[i]);
> > > > +		val |= BIT(29) | BIT(27);
> > > > +		writel(val, reg + pll_regs[i]);
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Force the I/O dividers of PLL-AUDIO1 to reset default value
> > > > +	 *
> > > > +	 * See the comment before pll-audio1 definition for the reason.
> > > > +	 */
> > > > +
> > > > +	val = readl(reg + SUN50I_R329_PLL_AUDIO1_REG);
> > > > +	val &= ~BIT(1);
> > > > +	val |= BIT(0);
> > > > +	writel(val, reg + SUN50I_R329_PLL_AUDIO1_REG);
> > > > +
> > > > +	i = sunxi_ccu_probe(node, reg, &sun50i_r329_r_ccu_desc);
> > > > +	if (i)
> > > > +		pr_err("%pOF: probing clocks fails: %d\n", node, i);
> > > > +}
> > > > +
> > > > +CLK_OF_DECLARE(sun50i_r329_r_ccu, "allwinner,sun50i-r329-r-ccu",
> > > > +	       sun50i_r329_r_ccu_setup);
> > > 
> > > Please make this a platform driver. There is no particular reason why it
> > > needs to be an early OF clock provider.
> > 
> > Why? It's good to have it as early clock provider. It has no dependencies
> > and other drivers that depends on it, like IR, can be deferred, if this
> > is loaded later.
> 
> No, Samuel is right, we should make them regular drivers as much as we
> can.
> 
> The reason we had CLK_OF_DECLARE in the first place is that timers
> usually have a parent clock, and you need the timers before the device
> model is set up.
> 
> Fortunately for us, since the A20, the architected timers don't require
> a parent clock from us, and we can thus boot up fine.

There are other timers. A lot of SoCs, newer than A20 (like H6), have High 
Speed Timer, which requires parent clock to be enabled. We just choose not to 
add node for it to DT, even if it's there and driver already exists.

Best regards,
Jernej

> 
> Since the dependencies are minimal, it should probe fairly early and
> with the on-demand probing from the device links you might not even tell
> the difference for most consumers.
> 
> Maxime