load_goke: opt-in sensor_dvp / sensor_mclk gate for DVP-wired boards - #2276
Conversation
PR Summary by Qodoload_goke: opt-in sensor_dvp/sensor_mclk gates for DVP-wired boards
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Thanks — both findings on this PR were fair and are now addressed in 1. 2. Silent MCLK write. Also agreed, and this is precisely the failure mode For context on why the MCLK hook exists at all: |
widgetii
left a comment
widgetii
left a comment
There was a problem hiding this comment.
Sorry for the delay on this one — you did the hard part and then waited, which is the wrong way round.
I checked this against the pinned SDK source (openhisilicon @ 2d637e35, kernel/sys_config/sys_config.c) rather than just reading the diff. The approach is right and your diagnosis is exactly correct. Two changes I'd like before merge, then this goes in.
What checks out
The DVP lever is the only one that exists. pinmux() dispatches on chip=, and per kernel/compat/compat.h in the goke build C_HI3518EV300 is "gk7202v300". That branch is MIPI-only and ignores cmos_yuv_flag entirely — it unconditionally calls i2c0_for_mipi_sensor_pin_mux_gk7205v200(), sensor_cfg_for_mipi_sensor_mux(), vi_mipi_rx_mux(). There is no DVP path reachable for chip=gk7202v300 at any g_cmos_yuv_flag. Only the gk7205v200 branch honours cmos_yuv_flag == 1 → i2c0_for_vi_raw_sensor_pin_mux() + vi_raw_mode_mux_gk7205v200(). Your i2c-NACK symptom falls straight out of that. This isn't a hack, it's the only lever.
The SYSCFG_CHIP separation is provably as contained as you say. chip_list feeds only pinmux() and clkcfg() — and clkcfg()'s C_HI3518EV300 and C_HI3516EV200 branches write byte-identical values (0x00A0=0x00240400, 0x00F4=0x00110000, 0x00FC=0x8). So the swap moves pinmux and nothing else. Splitting it out of CHIP_TYPE was the right call.
The MCLK story is confirmed. parse_sensor_clock() returns 0x6 for gc2053, and the file's own table says 0x6: 27MHz. MCLK is sourced only from that sensor-name table, so the .ini key genuinely is never consulted on this path. Your note that sweeping it gives identical results at every value — indistinguishable from having ruled it out — is accurate and worth having in the history. Thanks for writing it down.
1. The devmem write is a blind store to a read-modify-write register
void sensor_clock_config(int index, unsigned int clock)
{
reg_write32(clock << 2, 0xF << 2, reg_crg_base + 0x00F0); /* RMW bits [5:2] */
}reg_crg_base = ioremap(0x12010000), so 0x120100F0 is the right address, and the sns0 select field is bits [5:2] only.
devmem 0x120100F0 32 0x0000000D stores the whole word. Decomposed: 0xD = 0b1101 → bits[5:2] = 0b0011 = 0x3, and the table reads 0x3: 24MHz — so you are selecting the clock you intend, which is why it works on your three boards. But the same store also sets bit 0 and forces bits [31:6] to zero, and nothing in the driver ever writes that register wholesale.
Could you mask to the field instead? It also removes the magic constant and makes the var extensible to the other seven values:
# CRG PERI_CRG60 @ 0x120100F0, sns0 clock select = bits [5:2].
# 0x0:74.25 0x1:72 0x2:54 0x3:24 0x4:37.125 0x5:36 0x6:27 0x7:12 (MHz)
# open_sys_config's sensor_clock_config() read-modify-writes just this
# field -- match that rather than storing the whole word.
cur=$(devmem 0x120100F0)
devmem 0x120100F0 32 $(( (cur & ~0x3C) | (3 << 2) ))While you're there — add a *) arm that logs a non-empty unsupported value. Right now sensor_mclk=37 does nothing silently, which is the same failure class the rest of this PR is guarding against.
2. sensor_dvp=1 shouldn't unconditionally force gk7205v200
gk7205v300 has its own DVP path (vi_raw_mode_mux_gk7205v300() under cmos_yuv_flag == 1). A DVP-wired v300 that sets sensor_dvp=1 would get pushed onto the v200 VI pad table — the same bug this PR fixes, pointing the other way. Only gk7202v300 lacks a DVP branch and needs to borrow:
if [ "$(fw_printenv -n sensor_dvp 2>/dev/null)" = "1" ]; then
YUV_TYPE0=1
# gk7202v300's pinmux() branch is MIPI-only -- it ignores g_cmos_yuv_flag.
# Borrow gk7205v200's tables (same die family, and they do honour it).
# gk7205v200/v300 already have their own DVP paths.
if [ "$CHIP_TYPE" = "gk7202v300" ]; then
SYSCFG_CHIP=gk7205v200
fi
fiMinor
logger -p daemon.err -t load_goke— the file's convention islogger -s -p ... -t goke(lines 356, 360, 366, 377). The-smatters here: a failed MCLK write is bring-up-fatal and should reach the console the way "SENSOR is not detected" does.- The
command -v devmemguard is harmless, butCONFIG_DEVMEM=yis set ingeneral/package/busybox/busybox.config, so it is always present. Keep it if you prefer belt and braces. - Asymmetry worth a comment rather than a change:
sensor_dvpapplies to bothinsert_detectandinsert_ko, butsensor_mclkonly toinsert_ko, so first-boot autodetect probes at the default clock. Probably fine since i2c is master-clocked, but it will surprise someone eventually.
Before this can merge
- CI has never run on this branch — fork PRs need workflow approval, so the umbrella gate has nothing to go on. I'll approve the run once you push.
- The branch is behind master; please rebase.
The firmware/builder split is right — generic mechanism here, board profile in OpenIPC/builder#119. I'll pick that one up next.
|
Thanks for going to the SDK source rather than the diff — that turned a "looks 1. The MCLK writeYou're right, and the way it was wrong is worth stating: Now masked to the field the driver actually touches: mclk_cur="$(devmem 0x120100F0)"
devmem 0x120100F0 32 $(( (mclk_cur & ~0x3C) | (mclk_sel << 2) ))I also took your suggestion to make it extensible and filled in the whole table 2.
|
open_sys_config picks MIPI or DVP pad routing from its chip= and g_cmos_yuv_flag= arguments. On a board that wires the sensor to the DVP pads, the MIPI arguments mux the i2c controller to pads the sensor is not connected to, so every address NACKs and no sensor is ever detected. Gated on an env var rather than $CHIP_TYPE deliberately: both wirings exist on gk7202v300, so keying this off the SoC name would fix DVP boards by breaking every MIPI one. Profiles opt in with 'fw_setenv sensor_dvp 1'; unset means the current behaviour is unchanged. sensor_mclk is gated the same way. open_sys_config reads MCLK only from its module parameters and defaults to 27 MHz at this chip=, so a sensor init table tuned for 24 MHz runs against the wrong clock -- and the ini's MCLK key is never consulted on this path, which makes sweeping it look like a ruled-out cause. Refs: OpenIPC#2074
…rite MCLK blind Two findings from review: 1. CHIP_TYPE role conflation. The opt-in was overwriting CHIP_TYPE, which is the DETECTED SoC, with the value wanted for open_sys_config's chip= pad-routing selector. Those are two different things that happen to coincide by default: a DVP board needs the gk7205v200 routing tables while still BEING a gk7202v300. Conflating them makes every existing and future CHIP_TYPE branch harder to reason about. Now a dedicated SYSCFG_CHIP carries the selector and CHIP_TYPE keeps meaning exactly one thing. 2. Silent MCLK write. devmem was called without checking that it exists or that the write succeeded. A wrong MCLK does not fail loudly — it yields a corrupted or absent image while every log line still looks healthy — so this is exactly the case that must not pass quietly. Both failures now log to daemon.err. Still opt-in and still keyed off env vars: with sensor_dvp unset the script is behaviourally identical to before. Refs: OpenIPC#2074
Addresses @widgetii's review, which checked this against the pinned SDK source rather than the diff. Both findings were correct. 1. The devmem write was a blind whole-word store to a register the driver only ever read-modify-writes. sensor_clock_config() is reg_write32(clock << 2, 0xF << 2, base+0xF0) — bits [5:2] and nothing else. 0x0000000D happens to put 0x3 (24MHz) in that field, so it worked, but it also cleared bits [31:6] and set bit 0. Now masks: (cur & ~0x3C) | (sel << 2). Also extended to the full documented table (74.25/72/54/24/37.125/36/27/12) instead of a lone magic constant, and an unsupported value now logs instead of doing nothing silently — that was the same failure class the rest of this PR guards against. 2. sensor_dvp=1 no longer forces gk7205v200 unconditionally. Only gk7202v300 lacks a DVP branch: its pinmux() case is MIPI-only and ignores g_cmos_yuv_flag entirely. gk7205v200 and gk7205v300 have their own DVP paths, and pushing a DVP-wired v300 onto the v200 VI pad table would be this very bug pointing the other way. Minor, also from review: logger now follows the file's convention (logger -s -p ... -t goke). The -s matters — a failed MCLK write is bring-up fatal and should reach the console the way 'SENSOR is not detected' does. The command -v devmem guard is kept as belt and braces even though CONFIG_DEVMEM=y, since the read is now load-bearing too. The insert_detect/insert_ko asymmetry is documented rather than changed: first-boot autodetect probes at the sensor table's default clock, which is fine because i2c is master-clocked, but it is the kind of thing that surprises people. Refs: OpenIPC#2074
1084143 to
aa6c983
Compare
|
Heads up: I rebased this branch onto current master and force-pushed it — Nothing in your change was altered: the three commits replayed cleanly (master hadn't touched I also approved the CI runs — that was the reason this PR had never had a check on it, and it was my hold-up, not yours. On the masked writeI checked the arithmetic rather than eyeballing it, and it holds. In both Two ways that expression could have silently broken, both of which happen to be fine, but worth recording since they are invisible failure modes of exactly the kind this PR is about:
Your Agreed on keeping the Where this standsThe review is still marked changes-requested, and I am leaving it that way until you confirm on hardware — for the reason you gave yourself, which was the right instinct: the failure mode here is a silently corrupted image, so "it compiles" should not stand in for "it works". Report back either way and I will clear it and merge. Thanks for the turnaround, and sorry again for the nine days of silence before it. |
|
Rebased head is green across all six workflows, and the matrix picked exactly the right set — all seven Goke boards that use this Since a build cannot catch a wrong MCLK, I verified the two things a build can't, so you aren't the only evidence for this. 1. The register layout, confirmed on hardwareI read
I also ran your expression in that board's own busybox ash against the live register value, and exercised the write path with a value equal to what was already there — a true no-op, readback identical, Worth knowing: 2. The no-op path, proven by execution traceThe gate's safety claim is that a board with neither var set behaves exactly as before. That is a claim about the sequence of commands the script runs, so I asserted it that way: stub every external onto Byte-identical across 4 chips × 3 sensors, plus the first-boot autodetect path ( The important part: I ran the same harness against I would like to add this as Where that leaves thingsBoth review findings are addressed and independently verified. The one thing neither CI nor I can reach is the DVP pinmux actually bringing up the GC2053 on a DVP-wired gk7202v300 — the lab board is MIPI-wired with an imx335, so it is the twin of a gk7205v300, not of your W7. Given the above I am clearing the changes-requested. If your flash comes back clean, say so and I will merge; if you would rather I merge now on the strength of the equivalence above, that is reasonable too, since the opt-in path is inert for every board that does not set the vars. |
Both findings addressed in 1084143 and independently verified: register layout measured on lab hardware, no-op path proven by execution-trace equivalence with a working negative control. Holding merge only on the author's DVP bring-up test.
|
Hardware verification, as promised. Flashed to a GK7202V300 (OpenIPC nightly The interesting part is that the boards had more to say than "it works". The reset value. Read from the U-Boot prompt, before Linux touches anything: (I dumped What the write actually sees. I then instrumented Decoded, that is:
Two things fall out of that. First, it independently confirms the table in the patch: the hardware sits at Second, and more to your original point: the register is already What I did not verify. My board is a gk7202v300, so it still takes the |
Two review items from @widgetii and @flyrouter. 1. The profile never set sensor_mclk. This was a real hole, not a doc mismatch: the PR description claimed sensor_dvp=1 AND sensor_mclk=24, but only the first was in customizer.sh, so a board flashed from this profile would have come up at 27 MHz against 24 MHz init tables — the silent-corruption failure the firmware PR exists to prevent. My bench boards had sensor_mclk set by hand, which is exactly why the gap did not show. Confirmed against hardware rather than re-reading the code: PERI_CRG60 reads 0x00000019 at the moment load_goke writes it, i.e. bits [5:2] = 0x6 = 27 MHz, matching parse_sensor_clock()'s gc2053 -> 0x6 mapping. The ini cannot fix this — MCLK is a CRG clock programmed before majestic reads any ini. 2. Renamed gk7202v300_lite_w7_8m -> gk7202v300_lite_generic-w7, per the documented <soc>_<flavor>_<vendor>-<model> shape. "gk-w7" is dropped: GK is Goke, the SoC vendor, already carried by gk7202v300 — flyrouter's doubt that it names the board vendor was right, it does not. "generic" is the honest token for a board with no identifiable vendor, and is already established here (gk7205v200_otg_generic, ssc30kq_rubyfpv_generic). _8m dropped since flash size is BR2_OPENIPC_FLASH_SIZE. The excludes list stays gk7202v300_lite.list — that name is keyed to <soc>_<flavor>, not to the profile, matching every other profile in the tree. The inert ini Data_seq field is left as-is per review. Refs: OpenIPC/firmware#2276
|
Your Set beside what I read on the lab board:
Two different SoCs, two different sensors, two different table entries, both matching — with bit 0 set and bits [31:6] clear in both cases. Neither of us had that alone. Your U-Boot One correction to the recordYou posted a correction on OpenIPC/builder#119 sixteen minutes after this comment: your boards run a forked The timing agrees with your correction rather than with the original: this merged at 17:11 and no nightly built from it existed by 17:35, and a board on stock pre-merge Flagging it because this is already merged and I'd rather the thread not read as end-to-end validation it isn't. It doesn't change the merge — that went in on the lab register measurement, the trace-equivalence harness with its negative control, and CI green across all seven Goke boards, none of which route through your bench. And it doesn't touch the register measurements above, which are reads of hardware and don't care which script logged them. What's still open is the same gap you identified yourself: merged The case neither of us can reachAgreed on the DVP-wired gk7205v200/v300 path. Your board is a gk7202v300, so it exercises the borrow branch; the case where a chip keeps its own It is covered by the trace harness at the level of "the right arguments reach Thanks for the instrumented reboot, and for the correction. Both made the record more accurate than it would have been. |
|
Correcting my "Hardware verification" comment above. @widgetii is right, and the Concretely, our fork hardcodes the DVP branch behind That makes one line of my comment worse than merely unsupported. I wrote that the What does not survive: What does survive, because it is a register read and does not care which script
Your lab board reading The outstanding test is unchanged and now has a defined shape: stock upstream Thank you for joining the two threads up. I had posted the correction on #119 and |
…6006C Wi-Fi) (#119) GK-W7 board (product string ipc533331a-W7-gc2053dvp-f8): GK7202V300, 8MB NOR, GC2053 wired in DVP/parallel mode with the SID strap high, iComm/SSV SSV6006C USB WiFi (8065:6000), no Ethernet PHY. Differs from the generic gk7202v300_lite target in two ways it cannot express without a profile. The sensor is on the DVP pads, so open_sys_config's MIPI arguments mux i2c to pads nothing is connected to and no sensor is ever detected; customizer.sh opts into the sensor_dvp / sensor_mclk gate added in OpenIPC/firmware#2276. And the Lite target ships only mt7601u, which will never bind an SSV part, so the defconfig selects BR2_PACKAGE_SSV635X_OPENIPC and the overlay adds the ssv6x5x-generic wlandev branch. Named generic-w7 rather than after a vendor: the board carries no identifiable vendor marking, and "gk" is Goke -- the SoC vendor, already encoded in the soc field -- not the board's. `generic` is the established token here (gk7205v200_otg_generic, ssc30kq_rubyfpv_generic). Verified: builds green, rootfs 5017600B with 220KB of headroom in the 5120KB partition; the baked load_goke is master's verbatim (md5 confirmed after normalising the build's comment stripping); customizer.sh carries both env vars and is sh/dash/ash clean; S30customizer runs it on first boot ahead of S70vendor, so the vars are set before load_goke reads them; and on hardware, master's load_goke driven only by those two vars brings up the DVP GC2053 at 1920x1080 @ 25 fps with i2c init failures 0 and ISP errors 0. Not verified: the conjunction of the above on a single unattended first boot from this image with no hand-set environment. Each link is independently evidenced; the composition is not. Follow-up expected on the PR. Headroom note for later: 220KB of rootfs headroom is about 4% of the partition, and this family has a history of profiles going over as majestic grows. gk7202v300_lite.list is the lever. Refs: OpenIPC/firmware#2074, OpenIPC/firmware#2276
2 participants
Adds an opt-in env-var gate so DVP-wired boards can select the DVP pad
routing, without changing behaviour for anything that does not ask for it.
open_sys_configpicks MIPI or DVP pad routing from itschip=andg_cmos_yuv_flag=arguments. On a board that routes the sensor's parallel data,sync, PCLK and i2c to the SoC's DVP pads, the MIPI arguments mux the i2c
controller to pads the sensor is not connected to — every address NACKs, no chip
ID is ever read, and no video is possible.
Per @widgetii's review note on #2074, this is keyed off an env
var rather than
$CHIP_TYPE:So a profile opts in with
fw_setenv sensor_dvp 1. Unset — every existing board —takes exactly the path it takes today.
sensor_mclkis gated the same way.open_sys_configreads MCLK only from itsmodule parameters and defaults to 27 MHz at this
chip=, so a sensor init tabletuned for 24 MHz (the GC2053 ForCar tables are) runs against the wrong clock.
Worth noting for anyone who hits this: the
.iniMCLK key is not consulted onthis path, so sweeping it produces identical results at every value — which is
indistinguishable from having ruled the cause out. That cost me a while.
Companion to the
gk7202v300_lite_w7_8mdevice profile in OpenIPC/builder,which is where the board-specific bring-up lives.
Verified on three GK-W7 boards (GK7202V300, 8 MB NOR, GC2053 in DVP mode with
SID strapped high): with
sensor_dvp=1andsensor_mclk=24the i2c bus comesup, the sensor answers at 7-bit
0x3fwith chip ID0x2053, and the pipelinedelivers 1920x1080 H.264 over RTSP at 25 fps with
FrmErrCnt 0. With the varsunset the script is byte-for-byte equivalent in behaviour to before.
sh -nclean.Refs: #2074