Repository navigation
ambarella-osdrv-s3l: release the sensor before looking for it - #2768
Merged
Merged
Conversation
openipc-ai
requested review from
cronyx,
flyrouter and
viktorxda
as code owners
November 6, 2026 08:23
PR Summary by QodoRelease S3L sensor reset before probing
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. A driver reset gap is patched in a shell script
|
Contributor
|
Tested on an Amcrest IP4M-1026E (OV4689, reset on GPIO 107), using the PR's release_sensor verbatim. What works:
On the PR's open question, whether the sensor answers before the VIN clock is running:
Caveats:
Overall it works as intended on this hardware. I'd ask for the U-Boot dependency to be stated, and for sensor= to stay the recommended path. |
On an S3L the sensor sits in reset until its driver loads, which drives the pin, so load_ambarella found no sensor on the IP4M-1026E with no sensor value set. Which pin it is differs by board: GPIO 107 there, GPIO 39 on a DS-2CD2423G0-I. The pin is in the device tree at /iav/vinc0/vinrst-gpios, so it is read from there. It is released through sysfs and handed back, since iav.ko claims it when it loads, which is why the probe now comes first. A pin on an I2C expander, as on the vendor's reference board, is left alone. Releasing the pin is not enough on its own. The sensor also needs a clock, which only its driver starts, so detection depends on U-Boot having left the sensor PLL running. sensor= stays the path to rely on.
openipc-ai
force-pushed
the
ambarella-s3l-sensor-reset
branch
from
November 6, 2026 16:11
67ddf96 to
be057d7
Compare
Collaborator
Author
|
@DeathCamel58 thanks, that was exactly what this PR needed. Your U-Boot dependency is now stated in the script header and the PR description, and One more run would let this land. The review rules need pasted output from a whole image, and so far the release has only been tested on its own with the clock state recreated by hand. Could you do a real cold boot of On the camera, clear fw_printenv sensor # note it if it's set, so you can restore it afterwards
fw_setenv sensorAfter it comes back, please paste the raw output of: cat /usr/lib/os-release
fw_printenv sensor
ipcinfo -s
grep -E 'iav|ov4689' /proc/modules
logread | grep -iE 'ambarella|sensor'
dmesg | grep -iE 'iav|ov4689|vin|sensor|i2c'A full serial boot log would be even better if you have it to hand. If detection fails, please post that too, since that result is just as useful. Your earlier Only do this on a camera where you still have serial recovery. |
Contributor
|
Ran it: a real cold boot (power unplugged, ~10 s) of How it was built and booted, so nothing is hidden:
Notes on the output:
Full serial boot log (U-Boot + kernel): attached. Output |
Collaborator
Author
|
@DeathCamel58 thank you, this is the run the PR needed. The ordering in your log matches the script (probe, then Two small things:
|
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
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.
Problem
On an S3L the sensor sits in reset until its driver loads, which drives the pin from
vinrst-gpios, as measured on the IP4M-1026E (OpenIPC/qemu-bench#233). Soload_ambarellacan't probe to find out which driver to load: with nosensorvalue it reports no sensor. A board profile settingsensor=avoids that and stays the path to rely on. This PR covers images with no profile.The pin is board data: GPIO 107 on the IP4M-1026E, GPIO 39 on a DS-2CD2423G0-I. So it can't be hardcoded.
Fix
load_ambarellareads/iav/vinc0/vinrst-gpiosfrom the device tree, drives the pin out of reset through sysfs, probes, and frees the pin.iav.koclaims that pin when it loads, so the probe now comes beforeiav. Asensorvalue in the environment skips all of this.iav.koresets with the flag as the level, so flag0means held low. Both boards use 0, as does the vendor's reference tree.directionwrite still unexports the pin, soiav.kocan claim it.What detection depends on
Releasing the pin is not enough on its own. The sensor needs a clock to answer I2C, and of everything this script loads, only
ov4689_mipistarts one. Detection therefore works only if U-Boot left the sensor PLL running:sensor=.Accepted for merge: on the IP4M-1026E the sensor gets about 74 MHz at this point. The OV4689 datasheet (v1.3, table 7-4) specifies EXTCLK as 6 to 64 MHz, 24 typical, so that is about 16% over the maximum. The clock isn't new, but the sensor used to sit in reset while it ran; now it is awake on that clock from the release until its driver sets the clock up. It's a timing specification, not listed under absolute maximum ratings, and the sensor answered fine, but every boot without
sensor=would do this. The maintainer accepted that for this merge; a board that wants to avoid it can setsensor=.A failed probe still costs about 72 s at boot (3 buses × 8 addresses × 3 s timeouts). That is not new in this PR.
Hardware tested on
Amcrest IP4M-1026E (S3L, OV4689, reset on GPIO 107), tested by DeathCamel58.
After, on this commit: a real cold boot with power removed and
sensorunset, full output here. The sensor is found,ov4689_mipiloads, and the tester reports video streaming from that boot.release_sensorlogged no failure.The order is the script's: the probe, then
iav, then the driver.Before: the tester's earlier report has
ipcinfo -sfinding nothing before the release and ov4689 (chip ID 0x4688) after it, on the previous revision ofrelease_sensor. It also covers the other path: withsensor=set nothing probes andov4689_mipireleases the pin, across repeated boots. That report is a summary, not pasted output.What this run does not cover:
s3l_liteimage. It'sbe057d787plus the tester's bench copy of thes3l_lite_amcrest-ip4m1026edevice profile, which isn't upstream yet: an initramfs, dev tools, and the unit's own microcode added locally, so os-release reads-gbe057d78-dirty. The tester statesload_ambarellaand the kernel modules are this branch's.s3l_lite, so the pin comes from whatever tree the boot supplies. Here that appears to be the profile's. A tree withoutvinrst-gpioslogs the failed release and detects nothing.ipcinfo -sreturns nothing once the driver is up, which costs about 60 s of probing if anyone runs it then. Detection at boot is not affected, since it runs before the driver loads.Tested without hardware
On this commit:
-M s3lfrom OpenIPC/qemu-bench,qemu-boot/test-s3l-boot.sh, with the real script and shipped modules: 0 failures in both cases. With nosensorvalue the script releases the pin from the device tree andov4689_mipiloads. Withsensor=ov4689nothing probes. This used an ipctool built from current master.release_sensorandfind_sensorunder busybox ash against a fake device tree and sysfs, plain and comment-stripped: 22 of 22 checks pass, covering each failed sysfs write, an expander phandle, and a short or missing property. These were scratch checks and are not in the tree.test_shell_parse.shandtest_strip_shell_comments.shwithSTRICT=1: pass.CI gives no coverage here:
s3lis inUNBUILT_FAMILIES, so no job buildss3l_lite.The property values above were read from a real DS-2CD2423G0-I.
Scope
general/package/all-patches/linux/LD_PRELOAD, and no binaries that cannot be rebuilt from source