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

["Jules Maselbas" <jmaselbas@zdiv.net>]

Hi,
On Tue Jan 14, 2025 at 9:38 AM CET, Ahmad Fatoum wrote:
> 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?

Yes it is barebox specific.
The linux drivers (clk/sunxi-ng) use SUNXI_CCU_* macros to define clks,
such macros could be reimplemented using the one-size fits all driver.
I think i'll try to do this for the H6 SoC.

>> 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.
thanks

>> ---
>
>> +#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.
ack

>
>> +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?
Not really worth the effort i think. This file is already too much different from
its Linux source. This is barebox specific.

>> +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.
ok

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