load_goke: opt-in sensor_dvp / sensor_mclk gate for DVP-wired boards - #2276
load_goke: opt-in sensor_dvp / sensor_mclk gate for DVP-wired boards#2276bneigher wants to merge 3 commits into
Conversation
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
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}
…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
|
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.
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
|
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.
|
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