update TI clocks

[oh]

Summary

Use syscon to get access to other nodes reg space
remove bus_new_pass dance
add clkctrl clocks and information about parent relations not described in the DTS.
Sometimes the parent child relation is described but the order in which they are parsed means their need multiple pass through the devicetree

[imp]

This is a bit too much for me to fully review...

sys/arm/ti/clk/am33xx.h

1233

Where do all these tables come from?

[rick_sloservers.com]

I've tested this stack on hardware and wanted to report the results. This is long, so in order: the test results; an answer to the tables question; a proposed block comment for am33xx.h; an offer to split this revision; one bug found, now up separately as D59120; and a note that four of the published diffs no longer apply to main. Nothing here needs action from you except where you want it.

The wiki page shows the stack booting a PocketBeagle on main, and notes that the BeagleBone Black does not boot because of the ethernet DTS change. This is that BBB case, with D46742, D46743, D46744 and D46746 applied, and on stable/15 rather than main.

Results

Setup. D46703, D46712, D46713, D46714, D41888, D46722, D46723, D46742, D46743, D46744 and D46746 rebased from stable/14 onto stable/15 (6b25004d1d18), KERNCONF=AM335X, on a BeagleBone Black Rev B3 (AM3358, 512 MB).

Result. The board boots 15.1-STABLE to multi-user with zero device attach failures. The failure documented at https://wiki.freebsd.org/arm/BeagleBoneBlackMain does not occur:

ti_sysc0: <TI SYSC Interconnect> on ofwbus0
device_attach: ti_sysc0 attach returned 6
...
panic: No usable event timer found!

ti_sysc attaches and the simple-pm-bus hierarchy enumerates.

Verified functional: serial console, microSD (UFS root rw), eMMC detected, Ethernet (DHCP lease, 100baseTX full duplex, bidirectional ping 0% loss, MAC read from board EEPROM), USB OTG (both root hubs at 480 Mbps), I2C (TPS65217 at 0x24 and board EEPROM at 0x50), GPIO (all four banks, direction and value verified). With device tree overlays additionally ADC (12-bit conversions), SPI (spigen attaches) and PWM (period/duty set and read back).

Six unattended reboot cycles, all reaching login with no attach failures and no panics.

Full boot logs, per-peripheral evidence and the rebase notes: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=297800

Rebase deltas from stable/14 to stable/15, in case they're useful:

• dev/extres/<sub>/ moved to dev/<sub>/ (clk, hwreset, regulator, syscon, phy)
• bus_generic_probe to bus_identify_children; bus_generic_attach(dev) to bus_attach_children(dev); return (0);
• OMAP4/OMAP5/DRA7 are gone from stable/15, so SOC_OMAP4 and CHIP_OMAP_4 hunks are dropped rather than ported
• sys/cdefs.h includes removed

Each hand-resolved file was diffed against its counterpart in this revision to confirm nothing was lost in translation.

Where the tables come from

I think the answer is already in the file and just easy to miss. am33xx.h annotates nearly every entry with both the Linux clock index name and the TRM section that documents it. Counting in the current revision: 1233 lines, 96 TRM citations, 101 references to am3.h, 177 parent lists. For example:

PLIST(l4ls_clkctrl_38_4) = { "mmc_clk" };  /* am3.h MMC1 clkctrl, TRM 18.2.2 */
PLIST(l4ls_clkctrl_38_14) = { "dpll_per_m2_div4_ck" }; /* am3.h SPI0 clkctrl, TRM 24.2.2 */

So each clkctrl register offset is cross-referenced to Linux's include/dt-bindings/clock/am3.h macro for the same module, and to the TRM section describing that module's clock.

The reason a table is needed at all: the clkctrl nodes in the device tree do not express which clock feeds each module. Linux recovers that from its am3.h indices plus driver knowledge; FreeBSD's clk framework wants explicit parent relationships, so they have to be stated somewhere.

There is already a comment above the ti_clkctrl.c tables covering the xxx_clkctrl_[baseoffset]_[offset] naming scheme; what isn't written down anywhere is the provenance above. Rather than just offering to document it, here is text for a block comment near the top of am33xx.h. It restates the paragraph above in a form that lives in the file, so the next person to ask this question finds the answer there rather than in a review. Use it, cut it down, or ignore it:

/*
 * The tables below state, for each clock this SoC exposes, which clock
 * feeds it. That relationship is not in the device tree: a clkctrl node
 * describes a register window and the modules within it, but not the
 * source clock of each module. Linux recovers it from the per-module
 * macros in dt-bindings/clock/am3.h combined with knowledge held in its
 * drivers. FreeBSD's clk framework wants parents stated explicitly, so
 * they are stated here.
 *
 * Each entry is derived from two documents and cites both:
 *
 *	am3.h	the macro naming the module that owns this clkctrl
 *		register, which fixes which module an offset belongs to
 *	TRM	that module's clocking section, which names the source clock
 *
 * So "am3.h MMC1 clkctrl, TRM 18.2.2" on l4ls_clkctrl_38_4 reads: offset
 * 0x4 in the l4ls window based at 0x38 is MMC1 per am3.h, and TRM 18.2.2
 * gives its functional clock as mmc_clk.
 *
 * To add or check an entry: look the offset up in am3.h to get the
 * module, then that module's clock section in the TRM to get the parent.
 * The naming scheme itself is described above the ti_clkctrl.c tables.
 */

If you'd rather it said something different, or lived somewhere else in the file, say so and I'll redo it.

On the size of this review

If splitting D46723 would make it reviewable, I'm willing to do that work: separate pieces for the syscon conversion, the clkctrl tables, and the per-clock-type changes, each small enough to read in one sitting.

On the mechanics, so there's no ambiguity: Phabricator has no way to file a revision under someone else's name, so if I filed the split pieces they would show as authored by me, which I don't think either of us wants. What I'd suggest instead is that I prepare the three diffs and post them as pastes here, and you file them as your own revisions if you like what you see. That gets the split done and keeps everything under your name.

If you'd rather I filed them directly, I can, with the code credited to you in each summary and --author set to you on commit. And if you'd rather I left the whole thing alone, that's a perfectly good answer too. I don't want to take the work over, only to unblock it.

One bug found on hardware

Now up as D59120, as a child of this revision and D41888, rather than folded in here.

Under the ti,clksel binding the parent holds the register offset and a child's reg is an index matching its unit address. ti_mux_clock.c, ti_divider_clock.c and ti_gate_clock.c test the child's reg first, so the offset becomes 1, the syscon read lands on an odd address and takes a fatal alignment fault while registering mux clocks:

ti_clksel0: <TI CLKSEL> on ofw_clkbus0
ti_mux12: <TI Mux Clock> on ti_clksel0
Fatal kernel mode data abort: 'Alignment Fault' on read
FSR=00000001, FAR=ffd00001

The existing comment /* assume parent is clksel... */ suggests the expectation was that clksel children carry no reg; the shipped device tree gives them one. All three drivers share it, so fixing only the mux hits the same fault twice more.

The published diffs no longer apply

Applying them in dependency order onto current main (ee81cd1d8f55) today:

• apply cleanly: D46703, D46712, D46713, D41888, D46722, D46742, and D46746 (that last one needs -p0, since it uses the older Index: format without a/ b/ prefixes, which is a format quirk rather than drift)
• fail: D46714, D46723, D46743, D46744

On stable/14, seven of the eleven fail. So someone trying to reproduce this from the reviews today cannot get as far as booting, and the two central pieces are among the ones that have drifted.

I have the whole stack rebased and building on stable/15 already, since that is what the results above were produced on. If a refreshed set of diffs would be useful, either updated against main or the stable/15 versions, tell me how you'd like them and I'll do it that way: as pastes linked from here, as attachments on the PR above, or by updating these revisions directly if you'd rather I did that. I'd rather not touch your revisions uninvited.

Either way the rebase is mechanical and the work is yours, so whatever lands should be committed with --author set to you.

The reviews and the branch have diverged

This may matter to others reproducing it. D46713's branch commit 6d93b69bcf99 touches six files, while the D46713 diff here touches four, and D46712 differs the other way (3 files here, 1 on the branch). Neither source is a superset.

Concretely, the branch version of D46713 fixes am335x_musb.c to request usbotg_fck instead of usbotg_fck@47c; this revision set does not. I rebased from the Phabricator diffs, USB then failed to attach, and I diagnosed it myself before finding it had already been fixed on the branch, so credit for that one is the author's, not mine.

Anyone reproducing from the reviews alone will get a tree where USB does not attach, with no indication anything is missing. Might be worth syncing the two.