clk/broadcom: Add CPRMAN clkdev backend and peripheral clock node

[3293789706_qq.com]

Summary

Add the shared CPRMAN clkdev backend (bcm_cprman) and the peripheral
clock node (bcm_clk_periph) for the Broadcom clock manager.

bcm_cprman provides the clkdev interface (read/write/modify/lock) on
top of a memory-mapped register window, and a generic attach routine
that iterates over a chip-supplied bcm_cprman_clk_data table to
register clocks. Chip-specific drivers inherit from bcm_cprman_driver
via DEFINE_CLASS_1 and supply only probe/attach.

bcm_clk_periph implements the fractional-N peripheral clock node:

• init reads the SRC field to select the active parent
• set_gate enables/disables the clock output
• set_freq disables the clock, waits for BUSY, programs the 12.12 fixed-point divider, re-enables, and sets MASH mode
• recalc_freq extracts the combined integer+fractional divider field with a div==0 guard to avoid division by zero

The passwd field is removed from bcm_clk_periph_def; the PASSWD token
is now applied uniformly by CPR_WRITE4 in the backend.

Signed-off-by: perdixky <[e-mail]>

[mhorne]

This looks straightforward to me. I did not review the recalc / set_freq methods in detail.

Presumably the actual clock definitions are coming soon? It would not make sense to commit this without them.

Also, what hardware are you targeting?

sys/dev/clk/broadcom/bcm_clk_periph.c

29–31

__FBSDID is a historical stub, and not to be added in new code.

sys/dev/clk/broadcom/bcm_cprman.c

55

153

I don't think it needs to be a spin mutex; other clkdev implementations do not seem to use this. Am I wrong?

180–184

Please align to a common tab-indentation.

[mhorne]

sys/dev/clk/broadcom/bcm_clk_periph.c

257

This field is assigned but not used, unlike div_int_mask.

260

Here too.

[3293789706_qq.com]

sys/dev/clk/broadcom/bcm_clk_periph.c

29–31

__FBSDID is a historical stub, and not to be added in new code.

Thanks, I'll remember that.

sys/dev/clk/broadcom/bcm_cprman.c

153

Based on my understanding, read and write operations on registers are tiny and can be completed very quickly, so a spin mutex may reduce the cost of waiting.

[3293789706_qq.com]

Updating D55907: clk/broadcom: Add CPRMAN clkdev backend and peripheral clock node

[mmel]

sys/dev/clk/broadcom/bcm_clk_periph.c

139

There are some issues:

1. The meaning of the rounding flags is as follows:

Both clear -> exact frequency
Both set -> nearest lower or upper
Only one set-> nearest, but not upper or lower than requested.

2. You should actively modify the divider to satisfy these rounding requirements and only return ERANGE if the divider goes out of the HW range.
3. You must not change *freq when ERANGE is returned.

141

There are some issues:

1. The meaning of the rounding flags is as follows:

Both clear ->exact frequency
Both set -> nearest lower or upper
Only one set -> nearest, but not upper or lower than requested.

2. You should actively modify the divider to satisfy these rounding requirements and only return ERANGE if the divider goes out of the HW range.

3. You should set the *freq argument to the exact computed (thus aslo rounded) output frequency in both cases (DRY_RUN or real setting), but you must not modify it if ERANGE is returned.

sys/dev/clk/broadcom/bcm_cprman.c

30

__FBSDID();

55

not done

153

I agree with @mhorne. If nothing else, access to secondary or tertiary interconnects can be very slow.
In this case, however, the mutex is also held over by bcm_clk_wait_while_busy(), so the spin mutex is not an option.

180–184

not done

[3293789706_qq.com]

sys/dev/clk/broadcom/bcm_cprman.c

153

I agree with @mhorne. If nothing else, access to secondary or tertiary interconnects can be very slow.
In this case, however, the mutex is also held over by bcm_clk_wait_while_busy(), so the spin mutex is not an option.

That makes sense. I missed the fact that bcm_clk_wait_while_busy() holds the lock while potentially sleeping. Thanks for pointing that out, I'll update the mutex type to MTX_DEF and post an updated diff soon.

[3293789706_qq.com]

Updating D55907: clk/broadcom: Add CPRMAN clkdev backend and peripheral clock node

[mmel]

LGTM

[mhorne]

Hi, thanks for your patience. I have not forgotten about this series.

I am in the process of testing the changes, and my intent is to give a complete review this week.

So far it looks straightforward on my RPI 4B. From the verbose boot log:

ofwbus0: <Open Firmware Device Tree>
simplebus0: <Flattened device tree simple bus> on ofwbus0
ofw_clkbus0: <OFW clocks bus> on ofwbus0
clk_fixed0: <Fixed clock> on ofw_clkbus0
Clock: osc, parent: none, freq: 54000000
clk_fixed1: <Fixed clock> on ofw_clkbus0
Clock: otg, parent: none, freq: 480000000
clk_fixed2: <Fixed clock> on ofwbus0
Clock: 27MHz-clock, parent: none, freq: 27000000
clk_fixed3: <Fixed clock> on ofwbus0
Clock: 108MHz-clock, parent: none, freq: 108000000
simplebus1: <Flattened device tree simple bus> on ofwbus0
simplebus2: <Flattened device tree simple bus> on ofwbus0
regfix0: <Fixed Regulator> on ofwbus0
clk_fixed4: clock-fixed has no clock-frequency
clk_fixed4: clock-fixed has no clock-frequency
regfix1: <Fixed Regulator> on ofwbus0
regfix2: <Fixed Regulator> on ofwbus0
regfix3: <Fixed Regulator> on ofwbus0
regfix4: <Fixed Regulator> on ofwbus0
simplebus3: <Flattened device tree simple bus> on ofwbus0
bcm2835_cprman0: <BCM2835 CPRMAN Clock Controller> mem 0x7e101000-0x7e102fff on simplebus0
Clock: gnd, parent: none, freq: 0
Clock: testdebug0, parent: none, freq: 0
Clock: testdebug1, parent: none, freq: 0
Clock: pllh_aux, parent: none, freq: 0
Clock: plla, parent: osc(0), freq: 2999999988
Clock: pllc, parent: osc(0), freq: 2592000000
Clock: plld, parent: osc(0), freq: 3000000091
Clock: plla_dsi0, parent: plla(0), freq: 11718749
Clock: plla_ccp2, parent: plla(0), freq: 11718749
Clock: plla_core, parent: plla(0), freq: 499999998
Clock: plla_per, parent: plla(0), freq: 11718749
Clock: pllc_core0, parent: pllc(0), freq: 10125000
Clock: pllc_core1, parent: pllc(0), freq: 10125000
Clock: pllc_core2, parent: pllc(0), freq: 10125000
Clock: pllc_per, parent: pllc(0), freq: 648000000
Clock: plld_dsi0, parent: plld(0), freq: 11718750
Clock: plld_dsi1, parent: plld(0), freq: 11718750
Clock: plld_core, parent: plld(0), freq: 600000018
Clock: plld_per, parent: plld(0), freq: 750000022
Clock: gp0, parent: gnd(0), freq: 0
Clock: gp1, parent: gnd(0), freq: 0
Clock: gp2, parent: gnd(0), freq: 0
Clock: pwm, parent: gnd(0), freq: 0
Clock: pcm, parent: gnd(0), freq: 0

Do these frequency values seem right to you? I am not sure if the imprecision in some of these figures is acceptable or expected, e.g. 2999999988 or 3000000091.