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

[Samuel Holland]

On 8/19/21 11:34 PM, 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.

Another reason is so the driver can be built as a module. Each of these
CCU drivers has 30k-70k of data in it (lots of pointers, plus lots of
relocations). So it saves some RAM to only load the ones you need,
especially if that is none of them.

Regards,
Samuel