Add support for Photonicat2 board with USB hub watchdog#9765
Conversation
- Introduced a USB hub watchdog service to fix USB quirk for photonicat2 - Updated U-Boot version to v2026.04 for photonicat2 - Add maintainer information for photonicat2
HackingGate
requested review from
a team,
ColorfulRhino,
HeyMeco,
NicoD-SBC,
SuperKali,
Tonymac32,
amazingfate,
catalinii,
efectn,
igorpecovnik,
paolosabatino,
pyavitz,
rpardini and
schwar3kat
as code owners
github-actions
Bot
added
size/medium
📝 WalkthroughWalkthroughThis PR adds Photonicat2 board support with a USB hub watchdog implementation, updates the U-Boot version from v2026.01 to v2026.04, adds a board maintainer designation, and supplies the necessary U-Boot device-tree patch and build configuration for the RK3576-based board. ChangesPhotonicat2 Board Support & USB Hub Watchdog
Sequence DiagramsequenceDiagram
participant Boot as System Boot
participant Timer as systemd Timer
participant Service as Watchdog Service
participant Script as Watchdog Script
participant Sysfs as /sys/bus/platform/drivers/
Boot->>Timer: 20s elapsed (OnBootSec)
Timer->>Service: Trigger service
Service->>Script: ExecStart watchdog-run
Script->>Sysfs: Check unbind/bind paths exist
Script->>Sysfs: Enumerate bound USB devices
Script->>Sysfs: Validate USB link count per device
alt Device needs recovery
Script->>Sysfs: Unbind device
Script->>Sysfs: Rebind device
Script->>Sysfs: Poll recovery status (2min timeout)
Script->>Service: Return success/failure
else No recovery needed
Script->>Service: Exit early
end
Service->>Timer: Service complete
Timer->>Timer: Reschedule (OnUnitActiveSec=2min)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
Needs review
igorpecovnik
left a comment
igorpecovnik
left a comment
There was a problem hiding this comment.
Please add yourself here: https://armbian.com/update-data
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (10)
packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.timer (1)
7-7: 💤 Low value
AccuracySec=1sis tighter than typical and may inhibit timer coalescing.systemd's default accuracy is 1min, which lets the kernel coalesce wakeups for power saving. Since this watchdog only needs to react over minutes,
AccuracySec=30s(or even the default) would still meet recovery goals while reducing wake-ups. Not blocking — Photonicat2 is mains-powered with battery backup — just noting the tradeoff.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.timer` at line 7, The timer unit photonicat-usb-hub-watchdog.timer currently sets AccuracySec=1s which is unnecessarily tight and prevents systemd from coalescing wakeups; update the unit to use a coarser accuracy (e.g., AccuracySec=30s or remove the setting to use the default) so the watchdog still meets minute-scale recovery goals while reducing wake-ups — modify the AccuracySec directive in photonicat-usb-hub-watchdog.timer accordingly.packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.service (2)
6-11: 💤 Low valueOptional: minor sandboxing for a small defense-in-depth.
The unit runs as root (required for sysfs
unbind/bind), but you can still narrow its privileges withProtectHome=yes,ProtectSystem=strict,ReadWritePaths=/sys/bus/platform/drivers/onboard-usb-dev,NoNewPrivileges=yes,PrivateTmp=yes, etc. Not blocking — the unit is short-lived and triggered locally — but recommended hygiene for a timer-driven root service.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.service` around lines 6 - 11, Add lightweight sandboxing directives to the photonicat-usb-hub-watchdog.service [Service] stanza while preserving Type=oneshot and the existing ExecStart; specifically add ProtectHome=yes, ProtectSystem=strict, ReadWritePaths=/sys/bus/platform/drivers/onboard-usb-dev (so sysfs bind/unbind still works), NoNewPrivileges=yes and PrivateTmp=yes to narrow privileges for this root-run unit.
1-11: ⚡ Quick winNaming inconsistency between unit name and executable name.
The unit file is named
photonicat-usb-hub-watchdog.service(no2), but it executes/usr/lib/armbian/photonicat2-usb-hub-watchdog-run(with2). The same dual-naming appears inconfig/boards/photonicat2.cscand the source filenames inpackages/bsp/photonicat2/usb-hub-watchdog/. It works, but the inconsistency is confusing for anyone debugging viasystemctl status/journalctl -uvs. inspecting the binary path. Consider settling on one naming scheme (e.g.,photonicat2-usb-hub-watchdog.{service,timer,run}) across all artifacts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.service` around lines 1 - 11, The unit name and executable use inconsistent names: the service file photonicat-usb-hub-watchdog.service references ExecStart=/usr/lib/armbian/photonicat2-usb-hub-watchdog-run while config/boards/photonicat2.csc and package filenames also use the "photonicat2" prefix; pick one naming scheme (recommended: photonicat2) and make them consistent by renaming the unit to photonicat2-usb-hub-watchdog.service (or alternatively rename the binary to photonicat-usb-hub-watchdog-run), update ExecStart to match, and update all matching artifacts and references (service/timer unit names, config/boards entries, and package source filenames like photonicat2/usb-hub-watchdog/*) so systemctl status/journalctl -u and the filesystem path use the same identifier.patch/u-boot/v2026.04/board_photonicat2/add-photonicat2-board.patch (3)
879-892: ⚡ Quick winMisleading pinctrl group name
pwm2_ch1_m3.The pinctrl group at lines 884-886 is named
pwm2_ch1_m3and lives underusb { ... }, but it actually configuresgpio3 RK_PB0as a GPIO with pull-up — there is no PWM here. It is used both as&usb_drd0_dwc3.pinctrl-0(line 957) and shares the same physical line as the hubs'reset-gpios(lines 966, 974). Please rename the group to something likeusb_hub_rstn(or similar) so it accurately reflects the GPIO/reset purpose, and consider moving it outside theusb { }group only if you create a hub-specific pinctrl group. The current name will confuse anyone porting/maintaining this DTS.♻️ Suggested rename
usb { gpio0_c2: gpio0-c2 { rockchip,pins = <0 RK_PC2 RK_FUNC_GPIO &pcfg_pull_none>; }; - pwm2_ch1_m3: pwm2-ch1-m3 { + usb_hub_rstn: usb-hub-rstn { rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_up>; }; usb_otg0_pwren_h: usb-otg0-pwren-h { rockchip,pins = <4 RK_PC4 RK_FUNC_GPIO &pcfg_pull_none>; }; }; ... &usb_drd0_dwc3 { dr_mode = "host"; pinctrl-names = "default"; - pinctrl-0 = <&pwm2_ch1_m3>; + pinctrl-0 = <&usb_hub_rstn>;Also applies to: 954-977
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/u-boot/v2026.04/board_photonicat2/add-photonicat2-board.patch` around lines 879 - 892, The pinctrl group name pwm2_ch1_m3 under the usb { } node is misleading because it configures RK_PB0 as a GPIO used for hub reset; rename that node to a descriptive name such as usb_hub_rstn (or similar) and update all references (for example &usb_drd0_dwc3.pinctrl-0 and any reset-gpios that reference the old pwm2_ch1_m3) so they point to the new group name; keep the pinctrl properties unchanged (rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_up>), and if you decide to create a hub-specific group move the renamed node accordingly, otherwise keep it at the same scope but with the new name.
40-42: 💤 Low valueCopyright header looks copy-pasted from a Rockchip reference DTS.
This is a new Photonicat2 board file authored externally. The "Copyright (c) 2024 Rockchip Electronics Co., Ltd." line likely doesn't reflect the actual authorship/year. Please update to reflect the actual contributor and current year.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/u-boot/v2026.04/board_photonicat2/add-photonicat2-board.patch` around lines 40 - 42, Replace the incorrect Rockchip copyright header line ("Copyright (c) 2024 Rockchip Electronics Co., Ltd.") at the top of add-photonicat2-board.patch with an accurate header that reflects the actual contributor(s) and current year (e.g., "Copyright (c) 2026 <Your Name or Organization>") and ensure any required SPDX or license tag is present; update the comment block at the start of the patch (the two-line /* ... */ block) accordingly so authorship and year are correct.
833-841: 💤 Low valueUse the
RK_FUNC_*macro for SPI pin function values.Pin function
10is used as a raw integer here while every other entry in the file uses macros (e.g.RK_FUNC_GPIO). For consistency and readability, prefer a macro (or at minimum a brief comment explaining the SPI function-mux value of 10).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/u-boot/v2026.04/board_photonicat2/add-photonicat2-board.patch` around lines 833 - 841, Replace the raw integer pin function value 10 with the appropriate RK_FUNC_* macro for SPI in the spi1m2_1_pins node: update the entries for spi1_clk_m2 and spi1_mosi_m2 to use RK_FUNC_SPI (or the correct RK_FUNC_<something> SPI macro used elsewhere, e.g. RK_FUNC_SPI0/SPI1) instead of the literal "10", preserving the bank index (<3 ...>) and &pcfg_pull_none; change both occurrences in the spi1m2_1_pins block.packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog-run (3)
74-75: 💤 Low valueEmpty-driver case silently exits 0.
If no platform devices are bound to
onboard-usb-devat all,mapfileproduces an empty array and the script exits 0 without emitting anything. This is likely the intended fast-path, but in pathological cases (driver loaded but binding completely failed) the watchdog will not surface the problem. Considerlogging "no onboard USB platform devices bound" before the early exit so the journal makes the state explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog-run` around lines 74 - 75, The script currently does an early exit when mapfile -t onboard_devices < <(list_bound_devices) yields an empty onboard_devices array, which silently returns 0; modify the logic around mapfile and the subsequent check of onboard_devices (symbols: mapfile, onboard_devices, list_bound_devices) to emit a clear log message like "no onboard USB platform devices bound" via the existing logging facility before performing the exit so the journal records this fast-path; ensure the log is only emitted when the array is empty and preserve the original exit behavior.
18-31: 💤 Low valueConsider enabling
nullglobfor safer glob iteration.Both
for path in "$DRIVER_DIR"/*(line 18) andfor link in "$dir"/usb_dev.*(line 38) rely on per-iteration[[ -e ... ]]checks to silently skip the literal pattern when no files match. Settingshopt -s nullglob(locally or at script start) makes the intent explicit and avoids iterating once over a non-existent literal path. Current behavior is correct, but slightly fragile if checks are ever modified.Also applies to: 38-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog-run` around lines 18 - 31, Enable bash nullglob to avoid iterating a literal pattern when no matches exist by setting shopt -s nullglob before the directory globs used in the script (specifically the loops using for path in "$DRIVER_DIR"/* and for link in "$dir"/usb_dev.*) and restore the previous setting afterward (or confine it to a subshell) so behavior is localized; this replaces the fragile per-iteration [[ -e ... ]] checks and keeps the rest of the logic (name filtering, readlink -f target checks) unchanged.
86-97: 💤 Low valueConsider continuing rebind on per-device unbind/bind failure.
If the first device's
unbindorbindwrite fails (e.g. transient EBUSY), the scriptexit 1s immediately and skips any remaining rebinds. Since this is a recovery watchdog, attempting recovery for the remaining devices and then reporting overall failure at the end may yield better outcomes after warm-reboot drops.♻️ Sketch
+failed=0 for device in "${devices_to_rebind[@]}"; do log "rebinding onboard USB platform device $device" if ! printf '%s' "$device" > "$DRIVER_DIR/unbind"; then log "failed to unbind onboard USB platform device $device" - exit 1 + failed=1 + continue fi sleep "$UNBIND_DELAY_SECS" if ! printf '%s' "$device" > "$DRIVER_DIR/bind"; then log "failed to bind onboard USB platform device $device" - exit 1 + failed=1 fi doneThen check
failedat the end alongsidestill_pending.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog-run` around lines 86 - 97, The current loop over devices_to_rebind exits immediately on the first failed printf to "$DRIVER_DIR/unbind" or "$DRIVER_DIR/bind"; change it to record per-device failures instead of exiting so remaining devices are still attempted: inside the for loop (the block that calls log and printf to DRIVER_DIR/unbind and DRIVER_DIR/bind with sleep UNBIND_DELAY_SECS), on any failed printf set a boolean flag (e.g. failed_rebind=true) and log the error but continue to the next device rather than exit; after the loop, check that flag (and still_pending if present) and exit 1 only if any device failed, otherwise continue normally.patch/u-boot/v2026.04/defconfig/photonicat2-rk3576_defconfig (1)
67-68: 💤 Low valueRemove unused EHCI configuration options from RK3576 defconfig.
The RK3576 hardware only includes DWC3 USB controllers, not EHCI. The device tree for photonicat2 defines
&usb_drd0_dwc3but contains no EHCI nodes. EnablingCONFIG_USB_EHCI_HCDandCONFIG_USB_EHCI_GENERICcompiles unused code that provides no functional value. These options can be safely removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/u-boot/v2026.04/defconfig/photonicat2-rk3576_defconfig` around lines 67 - 68, Remove the two EHCI Kconfig options (CONFIG_USB_EHCI_HCD and CONFIG_USB_EHCI_GENERIC) from the defconfig because RK3576 uses only DWC3; edit the defconfig to delete the lines setting CONFIG_USB_EHCI_HCD=y and CONFIG_USB_EHCI_GENERIC=y so EHCI code is not compiled into the kernel, leaving only DWC3-related USB configs (e.g., those referencing usb_drd0_dwc3).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog-run`:
- Around line 74-75: The script currently does an early exit when mapfile -t
onboard_devices < <(list_bound_devices) yields an empty onboard_devices array,
which silently returns 0; modify the logic around mapfile and the subsequent
check of onboard_devices (symbols: mapfile, onboard_devices, list_bound_devices)
to emit a clear log message like "no onboard USB platform devices bound" via the
existing logging facility before performing the exit so the journal records this
fast-path; ensure the log is only emitted when the array is empty and preserve
the original exit behavior.
- Around line 18-31: Enable bash nullglob to avoid iterating a literal pattern
when no matches exist by setting shopt -s nullglob before the directory globs
used in the script (specifically the loops using for path in "$DRIVER_DIR"/* and
for link in "$dir"/usb_dev.*) and restore the previous setting afterward (or
confine it to a subshell) so behavior is localized; this replaces the fragile
per-iteration [[ -e ... ]] checks and keeps the rest of the logic (name
filtering, readlink -f target checks) unchanged.
- Around line 86-97: The current loop over devices_to_rebind exits immediately
on the first failed printf to "$DRIVER_DIR/unbind" or "$DRIVER_DIR/bind"; change
it to record per-device failures instead of exiting so remaining devices are
still attempted: inside the for loop (the block that calls log and printf to
DRIVER_DIR/unbind and DRIVER_DIR/bind with sleep UNBIND_DELAY_SECS), on any
failed printf set a boolean flag (e.g. failed_rebind=true) and log the error but
continue to the next device rather than exit; after the loop, check that flag
(and still_pending if present) and exit 1 only if any device failed, otherwise
continue normally.
In
`@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.service`:
- Around line 6-11: Add lightweight sandboxing directives to the
photonicat-usb-hub-watchdog.service [Service] stanza while preserving
Type=oneshot and the existing ExecStart; specifically add ProtectHome=yes,
ProtectSystem=strict, ReadWritePaths=/sys/bus/platform/drivers/onboard-usb-dev
(so sysfs bind/unbind still works), NoNewPrivileges=yes and PrivateTmp=yes to
narrow privileges for this root-run unit.
- Around line 1-11: The unit name and executable use inconsistent names: the
service file photonicat-usb-hub-watchdog.service references
ExecStart=/usr/lib/armbian/photonicat2-usb-hub-watchdog-run while
config/boards/photonicat2.csc and package filenames also use the "photonicat2"
prefix; pick one naming scheme (recommended: photonicat2) and make them
consistent by renaming the unit to photonicat2-usb-hub-watchdog.service (or
alternatively rename the binary to photonicat-usb-hub-watchdog-run), update
ExecStart to match, and update all matching artifacts and references
(service/timer unit names, config/boards entries, and package source filenames
like photonicat2/usb-hub-watchdog/*) so systemctl status/journalctl -u and the
filesystem path use the same identifier.
In `@packages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.timer`:
- Line 7: The timer unit photonicat-usb-hub-watchdog.timer currently sets
AccuracySec=1s which is unnecessarily tight and prevents systemd from coalescing
wakeups; update the unit to use a coarser accuracy (e.g., AccuracySec=30s or
remove the setting to use the default) so the watchdog still meets minute-scale
recovery goals while reducing wake-ups — modify the AccuracySec directive in
photonicat-usb-hub-watchdog.timer accordingly.
In `@patch/u-boot/v2026.04/board_photonicat2/add-photonicat2-board.patch`:
- Around line 879-892: The pinctrl group name pwm2_ch1_m3 under the usb { } node
is misleading because it configures RK_PB0 as a GPIO used for hub reset; rename
that node to a descriptive name such as usb_hub_rstn (or similar) and update all
references (for example &usb_drd0_dwc3.pinctrl-0 and any reset-gpios that
reference the old pwm2_ch1_m3) so they point to the new group name; keep the
pinctrl properties unchanged (rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO
&pcfg_pull_up>), and if you decide to create a hub-specific group move the
renamed node accordingly, otherwise keep it at the same scope but with the new
name.
- Around line 40-42: Replace the incorrect Rockchip copyright header line
("Copyright (c) 2024 Rockchip Electronics Co., Ltd.") at the top of
add-photonicat2-board.patch with an accurate header that reflects the actual
contributor(s) and current year (e.g., "Copyright (c) 2026 <Your Name or
Organization>") and ensure any required SPDX or license tag is present; update
the comment block at the start of the patch (the two-line /* ... */ block)
accordingly so authorship and year are correct.
- Around line 833-841: Replace the raw integer pin function value 10 with the
appropriate RK_FUNC_* macro for SPI in the spi1m2_1_pins node: update the
entries for spi1_clk_m2 and spi1_mosi_m2 to use RK_FUNC_SPI (or the correct
RK_FUNC_<something> SPI macro used elsewhere, e.g. RK_FUNC_SPI0/SPI1) instead of
the literal "10", preserving the bank index (<3 ...>) and &pcfg_pull_none;
change both occurrences in the spi1m2_1_pins block.
In `@patch/u-boot/v2026.04/defconfig/photonicat2-rk3576_defconfig`:
- Around line 67-68: Remove the two EHCI Kconfig options (CONFIG_USB_EHCI_HCD
and CONFIG_USB_EHCI_GENERIC) from the defconfig because RK3576 uses only DWC3;
edit the defconfig to delete the lines setting CONFIG_USB_EHCI_HCD=y and
CONFIG_USB_EHCI_GENERIC=y so EHCI code is not compiled into the kernel, leaving
only DWC3-related USB configs (e.g., those referencing usb_drd0_dwc3).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 470acae1-3266-4b11-860e-77cd3808a421
📒 Files selected for processing (6)
config/boards/photonicat2.cscpackages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog-runpackages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.servicepackages/bsp/photonicat2/usb-hub-watchdog/photonicat-usb-hub-watchdog.timerpatch/u-boot/v2026.04/board_photonicat2/add-photonicat2-board.patchpatch/u-boot/v2026.04/defconfig/photonicat2-rk3576_defconfig
|
✅ This PR has been reviewed and approved — all set for merge! |
|
@igorpecovnik done |
2 participants
Summary
Description
Photonicat2 can lose the onboard Genesys USB hub in a high chance after a warm reboot. The underlying issue appears to be in the Linux kernel Rockchip USBDP PHY probe-defer path, specifically
drivers/phy/rockchip/phy-rockchip-usbdp.c: whendevm_clk_bulk_get_all()returns-EPROBE_DEFER, older code can turn that into-ENODEV, so USB initialization may fail on RK3576 when theutmiclock fromu2phy0is not ready yet.References:
vdd-supply: immortalwrt/immortalwrt@e61a1d0CONFIG_USB_ONBOARD_DEVfor the GL852G hub: immortalwrt/immortalwrt@5e1650fThis PR intentionally does not copy the ImmortalWrt's DTS workaround. Instead, this PR uses a small BSP-packaged systemd watchdog (same idea with photonicat vendor but without touching kernel) as a runtime mitigation until the kernel-side fix is available in the target kernel.
Testing
0/SUCCESSwithout rebind logs when the onboard Genesys hubs are already enumerated.lsusb.lsusbshowed the Genesys USB2/USB3 hubs and downstream devices again.Summary by CodeRabbit
New Features
Updates
Device Support