pinmux: publish the pad tables through libipchw - #190
Merged
Merged
Conversation
`ipctool gpio mux` read the pad register, then wrote back `val & 0xfff0 | new_func`. These are 32-bit registers: on every one of them that mask silently zeroed bits 16-31, so a mux change also threw away whatever drive strength, pull or slew the boot had configured above the half-word. The selector width is not a constant either. HiSilicon and Goke put the function in the low nibble and SigmaStar in the low half-word, and `dump_regs()` already knew that -- by matching vendor strings, in a branch `fill_enabled_gpios()` had separately open-coded as a bare `0xf`. Collect all three on one `padmux_func_mask()` and read-modify-write against it. The mask is keyed on `chip_generation` rather than on the vendor string it used to come from. Same answer on every SoC that has a table -- Goke parts report HISI_V4 -- and it means a caller that sets the generation directly gets the right width without a live chip underneath it.
`src/reginfo.c` was executable-only. Everything in it that is worth reusing -- 1333 hand-entered pad rows across sixteen HiSilicon tables plus SigmaStar and Ingenic, and the per-SoC dispatch that picks the right one -- was therefore reachable only by running `ipctool`, and a consumer wanting to ask "which register and which value select PWM0 on this chip?" had no way in but to shell out and parse the output. Three things stood between the file and `libipchw`, and this fixes all three without moving a single table row, so `git blame` still answers for every address in here: - `regs_by_chip()` ended in `exit(EXIT_FAILURE)`. Inside a long-lived daemon an unrecognised SoC would then be an outage rather than a refusal, so it returns NULL and the CLI keeps its old behaviour through a `regs_by_chip_or_die()` wrapper. `fill_enabled_gpios()` takes the softer path instead: `gpio_possible_ircut()` now drops the ircut hint from its report rather than killing the process that asked for it. - The command half calls `print_usage()`, which lives in main.c. One `#ifndef STANDALONE_LIBRARY` fence after `regs_by_chip()` -- the last function the tables need -- takes that link edge and every printf with it. `num2gpio_groupnum()` and `find_pinfunc()` move above the fence; they are pure and the lookups to come want them. - `src/reginfo.c` moves from IPCTOOL_SRC to COMMON_LIB_SRC_BASE. `get_function()` had no callers and is deleted. The library pays nothing for this yet: with no exported entry point the tables are still static and unreferenced, and reginfo.c compiles to an empty object in libipchw. The executable is byte-for-byte the same size and answers "Platform is not supported" to `reginfo` and `gpio mux` on an unsupported host exactly as before.
The tables know that PWM1 on an hi3516ev200 is selector 1 in the register at 0x100C0010, and that the pad is GPIO0_4 the rest of the time. Until now the only way to get that out of ipctool was to run `reginfo` and parse the output, so consumers hand-copied the rows they needed and the copies went stale in the usual way. Three lookups, on include/ipchw.h next to the identity strings: by function name, by name prefix, and by pad. Each fills a flat POD, so muxctrl_reg_t stays private, and each returns the number of MATCHES rather than the number written -- a caller whose array was too small learns the size it needed instead of quietly losing rows. Two things about this data surprise people, so both are in the contract and both have a test: - A function is not unique to a pad. hi3516ev300 offers PWM2 and PWM3 on three pads each and hi3516cv200 offers PWM0 on two, so "the pad for PWM3" is not a question with an answer. Callers enumerate. - The spelling is not portable. It is PWM_OUT0 on the V1 parts, PWM0 from V2 onward and PWM0_OUT1 on V5, which is what the prefix query is for -- but "PWM" also prefixes SVB_PWM and PMC_PWM, and those are different controllers on different pads. Matching anywhere in the string instead of at the front would drive the sensor bias supply. ev200 and ev300 share one SDK build and one register map but not one pad table, so the family cannot be resolved at compile time; the lookups read chip_generation and chip_name, which is also what lets the whole thing be tested on a host. src/reginfo_test.c sets those two globals by hand and asserts the rows a camera was actually measured against, so an edit that moves PWM1 off 0x100C0010 fails here rather than on a bench. It links the real libipchw, which is what proves the symbols are exported and not merely present. It also sweeps every compiled-in family for two data invariants worth having over 1333 hand-entered rows: no selector wider than its own field, and no GPIO name that fails to parse. Nothing here exits, prints, or touches /dev/mem.
The pad tables are 46 KB on arm32 and regs_by_chip() names every one of
them from a single switch, so --gc-sections drops none of it. That is fine
in a tool you copy to /tmp, and not fine at all in a daemon on an 8 MB NOR
board -- an OpenIPC nightly was recently lost because one image went 8 KB
over its cap.
IPCHW_PADMUX picks the families, in the same shape as IPCHW_VENDORS
directly above it: 'all' (the default, and what the ipctool executable
always gets), 'none', or a subset of
v1 v2 v2a v3 v3a v4 v4a v5 3536c 3536d. Each token brackets its tables and
its regs[] arrays together, and its arm of regs_by_chip() with them, so a
trimmed family reports IPCHW_PADMUX_NO_TABLE rather than silently reading
like a chip with no pads. The SigmaStar and Ingenic tables now follow their
existing IPCHW_VENDOR_* macros, which is 10.6 KB nobody building for
HiSilicon was using.
Measured, arm-linux-gnueabi-gcc -Os -DSTANDALONE_LIBRARY, reginfo.c alone:
all ten families 46207
v4 13613 ev200/ev300/gk7205/dv200, four tables
v4a 9811
v2 5893
v1 4667
v3 4330
none 353
v4 is the worst case in the matrix and it is structural: one SDK build runs
on four chips with four different pad tables, so all four have to be there.
IPCHW_PADMUX_* is PUBLIC on the ipchw target, so a consumer that trims the
wrong family can #error on the missing macro at compile time instead of
meeting NO_TABLE on a camera. reginfo_test.c compiles its per-family cases
behind the same macros and asserts that a trimmed-out family is
distinguishable from an SoC that simply has no such pad.
The ipctool executable is byte-for-byte the size it was.
cYAML_test has existed for years and no workflow ever ran it. Add a native job that builds and runs it alongside reginfo_test, then configures a trimmed build (-DIPCHW_VENDORS=none -DIPCHW_PADMUX=v1) and runs the tests again -- which is the only automatic check that the family gating actually compiles and that a trimmed-out family stays distinguishable from an SoC with no such pad. mem_reg() keeps its mapping, offset, size and descriptor in four function-local statics with no lock, and any address outside the cached window unmaps it and maps another. Two threads on different windows will unmap the mapping the other is about to dereference. That has always been true and was written down nowhere; it matters more now that a daemon can link this. Also note what a false return does and does not mean: it is "/dev/mem would not open or mmap", not "that address is bad" -- a bad address on live silicon raises SIGBUS.
`num2gpio_groupnum()` and `find_pinfunc()` were moved above the STANDALONE_LIBRARY fence on the expectation that the pad-mux lookups would want them. They did not: the lookups go the other way, from a table row to a pad number, and they enumerate rather than stopping at the first match. Both helpers exist solely for `gpio_mux_by()`, which parses argv, so they belong on its side of the fence. Left where they were they are two unused statics in every library build. That is silent today -- the release flags carry -Wextra but not -Wall, and -Wunused-function lives in -Wall -- which is exactly the kind of thing that turns into noise the day someone tightens the flags. Checked with -Wall -Wextra across v1, v4, every family and none: clean.
PR Summary by QodoPublish pad-mux tables through libipchw
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Making fill_enabled_gpios() return a bool, two commits ago, I added the `return false` for "no pad table" and never added the `return true` for the ordinary path. Both callers branch on the result, so on every supported SoC they were deciding whether the platform is usable from whatever happened to be in the return register. It passed CI and it passed a run on a camera, because the garbage was truthy. The default flags carry -Wextra but not -Wall, and -Wreturn-type lives in -Wall, so nothing said a word. Turning it on as an error in every configuration found four more, all of the same shape: - `backup_with_yaml()` in main.c computes do_backup()'s verdict into `ret` and then drops it, so `ipctool backup <file>` has been exiting on whatever was in the register. That one is user-visible. - `dummy_change_addr()` in sensors.c is an empty `static int` stub wired into the i2c_change_addr pointer; callers that check it were reading rubbish. - `cv100_sensor_clksel()` and the RAW_DATA name lookup in ptrace.c are switches with no default. The first is genuinely exhaustive over a 3-bit field and now says so; the second decodes a value that came off a traced ioctl and is not bound to the enum at all, so "unknown" is a real answer it needed. Verified on a lab hi3516cv100: `reginfo` and the full YAML report are both byte-identical to the stock binary's, and the executable is the same size.
Contributor
Author
|
Good catch, and it was worse than it looked. Fixed in 3ee8992.
What let it through is that the default flags carry
Verification, since the original also survived a smoke test:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
src/reginfo.cholds 1333 hand-entered pad rows across sixteen HiSilicontables plus SigmaStar and Ingenic: for each pad, the physical register, the
selector value that puts each named function on it, and the GPIO the pad
carries otherwise. All of it was executable-only, so the only way to ask
"which register and which value select PWM0 on this chip?" was to run
reginfoand parse the output — which is why consumers hand-copy the rowsthey need, and why those copies go stale.
This publishes the data through
libipchwwithout moving a single table row,so
git blamestill answers for every address.Commits
gpio muxwrites the selector field, not the low half-word. The writewas
val & 0xfff0 | new_funcagainst a 32-bit register, which zeroes bits16–31 along with the selector. The selector width is not a constant either
— low nibble on HiSilicon/Goke, low half-word on SigmaStar — and
dump_regs()knew that by matching vendor strings whilefill_enabled_gpios()had separately open-coded a bare0xf. All three nowshare one
padmux_func_mask().In fairness: I read every pad register on four lab boards (hi3516cv100,
hi3518ev200, hi3516cv300, hi3516av300) and none of them currently has a bit
above 15 set, so this is a correctness fix with no victim I can point at,
not a bug report. Keying the mask on
chip_generationinstead of the vendorstring is what makes the lookups below testable off-camera.
reginfo.cbecomes linkable.regs_by_chip()returnedexit(EXIT_FAILURE)for an unknown SoC — inside a long-lived daemon that isan outage rather than a refusal, so it returns NULL and the CLI keeps its
old behaviour through a
regs_by_chip_or_die()wrapper.gpio_possible_ircut()takes the softer path instead and drops the ircuthint from its report rather than killing the process that asked for it. One
#ifndef STANDALONE_LIBRARYfence afterregs_by_chip()takes theprint_usage()link edge and everyprintfwith it.get_function()hadno callers and is gone.
The lookups.
ipchw_padmux_by_func,_by_prefixand_by_padoninclude/ipchw.h, filling a flat POD somuxctrl_reg_tstays private.Each returns the number of matches rather than the number written, so a
caller whose array was too small learns the size it needed.
Two things about this data surprise people, so both are in the contract and
both have a test:
three pads each and hi3516cv200 offers PWM0 on two, so "the pad for PWM3"
is not a question with an answer. Callers enumerate.
PWM_OUT0on the V1 parts,PWM0fromV2 on,
PWM0_OUT1on V5 — which is what the prefix query is for. ButSVB_PWMandPMC_PWMare different controllers, so the prefix matchis anchored at offset 0, never a substring search.
ev200 and ev300 share one SDK build and one register map but not one pad
table, so the family cannot be resolved at compile time; the lookups read
chip_generationandchip_name.IPCHW_PADMUX, so a consumer pays for the SoCs it targets. The tablesare 46 KB on arm32 and
regs_by_chip()names every one of them from asingle switch, so
--gc-sectionsdrops nothing. Same shape asIPCHW_VENDORSdirectly above it. Measured,-Os -DSTANDALONE_LIBRARY,reginfo.calone:v4(ev200/ev300/gk7205/dv200 — four tables)v4av2v1v3noneThe macros are PUBLIC on the
ipchwtarget so a consumer that trims thewrong family gets a compile error rather than
IPCHW_PADMUX_NO_TABLEon acamera. The SigmaStar and Ingenic tables now follow their existing
IPCHW_VENDOR_*macros — 10.6 KB nobody building for HiSilicon was using.CI runs the unit tests.
cYAML_testhas existed for years and noworkflow ever ran it. The new native job builds and runs it alongside
reginfo_test, then configures a trimmed build and runs the tests again.Also writes down what
mem_reg()does not promise: it is not thread-safe(one cached window in four file-statics, no lock), and a
falsereturnmeans "/dev/mem would not open or mmap", not "that address is bad" — a bad
address on live silicon raises SIGBUS.
Verification
reginfo_testsetschip_generation/chip_nameby hand and asserts rows areal camera was measured against, so an edit that moves PWM1 off
0x100C0010fails there rather than on a bench. It links the reallibipchw, which is what proves the lookups are exported and not merelypresent. It also sweeps every compiled-in family for two invariants worth
having over 1333 hand-entered rows: no selector wider than its own field, and
no GPIO name that fails to parse. Both hold today across all sixteen tables.
hi3516cv100:
reginfooutput is byte-identical to the stock binary's, 87rows, and
gpio scanstill works. Same check on a lab hi3516ev300 (93 rows,PWM0–PWM3 decoded on the pads they belong to).
-Wall -Wextraclean on arm32 acrossv1,v4, every family andnone.ipctoolexecutable is byte-for-byte the size it was.Note for reviewers
CV500regsgenuinely lacks the HDMI and power-sequencer pads(
0x114F0000–0x114F000C,0x112F00B0,0x112F00B8) thatDV300regshas,and that is correct — hi3516cv500 has no HDMI. I checked, because a
downstream consumer carries one merged table for all three V4A chips and I
assumed the omission was a gap. It is the merged table that is wrong.