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

Authored by 3293789706_qq.com on Wed, Mar 18, 7:42 AM.

Details

Reviewers

emaste
mhorne
manu
mmel

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 <3293789706@qq.com>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 71520
Build 68403: arc lint + arc unit

Event Timeline

3293789706_qq.com created this revision.Wed, Mar 18, 7:42 AM

Herald added a subscriber: imp. · View Herald TranscriptWed, Mar 18, 7:42 AM

3293789706_qq.com requested review of this revision.Wed, Mar 18, 7:42 AM

3293789706_qq.com added a child revision: D55908: clk/broadcom: Add PLL and PLL channel clock nodes.

Harbormaster completed remote builds in B71476: Diff 173837.Wed, Mar 18, 7:43 AM

mhorne mentioned this in D55910: clk/broadcom: Add CPRMAN clkdev backend and peripheral clock node.Wed, Mar 18, 4:10 PM

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 mentioned this in D55908: clk/broadcom: Add PLL and PLL channel clock nodes.Wed, Mar 18, 4:37 PM

mhorne added inline comments.

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 added inline comments.Thu, Mar 19, 2:51 PM

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.

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

Harbormaster completed remote builds in B71520: Diff 173909.Thu, Mar 19, 3:19 PM

3293789706_qq.com marked 5 inline comments as done.Thu, Mar 19, 3:26 PM

mmel requested changes to this revision.Fri, Mar 20, 8:17 AM

mmel added inline comments.

sys/dev/clk/broadcom/bcm_clk_periph.c
138	There are some issues: 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. You should actively modify the divider to satisfy these rounding requirements and only return ERANGE if the divider goes out of the HW range. You must not change *freq when ERANGE is returned.
140	There are some issues: 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. You should actively modify the divider to satisfy these rounding requirements and only return ERANGE if the divider goes out of the HW range. 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
29	__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

This revision now requires changes to proceed.Fri, Mar 20, 8:17 AM

3293789706_qq.com added inline comments.Sat, Mar 21, 2:01 AM

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.