Re: [PATCH v2 2/6] clk: Add clock driver for sun50i-a64

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 07.01.25 15:37, Jules Maselbas wrote:
> Adds a "one-size fit all" clock driver to be used for complex
> "composite" clock, composed of: a gate, a mux and severals dividers.
> This clk driver could be used to make SoC specific drivers easier
> to port from linux sources, but this is not the case yet.

Just curious: Here you say tha this driver can make porting kernel
drivers easier, but at the end you say this one-size fits all is
barebox-specific?

> The pll-cpux is set to 816MHz and pll-periph0-2x is set to 1.2GHz.
> 
> Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

As I read through the code, some nitpicks below, but nothing critical.

> ---

> +#define MHZ (1000UL * 1000UL)

There is linux/units.h with HZ_PER_MHZ.

> +static struct clk_div_table div_apb1[] = {
> +	{ .val = 0, .div = 2 },
> +	{ .val = 1, .div = 2 },
> +	{ .val = 2, .div = 4 },
> +	{ .val = 3, .div = 8 },
> +	{ /* Sentinel */ },

If comma is removed, it's less likely to add a member after it by mistake.

> +static void sun50i_a64_resets_init(void __iomem *regs)
> +{
> +	u32 rst;
> +
> +	rst = 0 |
> +		/* RST_USB_PHY0 */ BIT(0) |
> +		/* RST_USB_PHY1 */ BIT(1) |
> +		/* RST_USB_HSIC */ BIT(2);

Why not name this ${REG}_RST_USB_PHY0 and so on instead of the comments?

> +static inline void nop_delay(u32 cnt)
> +{
> +	while (cnt--)
> +		barrier();

While we use GCC by default, first clang support was recently added and
clang is known to do optimizations that would reorder cnt over a memory
clobber if cnt is held in a register. Better make cnt volatile here.

> +}
> +
> +/* a "one size fit all" clk for barebox */

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |