RK3576 NPU (RKNN) support + enable on Flipper One - #23
Draft
Notnurb wants to merge 258 commits into
Draft
Conversation
…_state() The drm_atomic_helper_bridge_reset() helper is deprecated in favour of the new atomic_create_state callback. Add drm_atomic_helper_bridge_create_state() as the counterpart helper for this new callback, and make drm_atomic_helper_bridge_reset() call this new helper. Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com> # imx8mp + sn65dsi84 + bridge hotplug Link: https://patch.msgid.link/20260619-drm-no-more-bridge-reset-v3-7-ff399263111b@kernel.org Signed-off-by: Maxime Ripard <mripard@kernel.org>
…drm_for_each_bridge_in_chain() drm_for_each_bridge_in_chain_scoped() was added in commit e46efc6 ("drm/bridge: add drm_for_each_bridge_in_chain_scoped()") to provide a safer alternative to drm_for_each_bridge_in_chain(). Following commits converted all users to the _scoped variant. Finally commit 2f08387 ("drm/bridge: remove drm_for_each_bridge_in_chain()") removed the old drm_for_each_bridge_in_chain() macro. It's time to rename drm_for_each_bridge_in_chain_scoped() back to the original name. Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com> Link: https://patch.msgid.link/20260630-drm-bridge-alloc-getput-for_each_bridge-2-v2-1-e0a1094cd1eb@bootlin.com Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> [Adapted for v7.2-rc5] Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
The drm_bridge_funcs.atomic_reset callback and its drm_atomic_helper_bridge_reset() helper are deprecated. Switch to the atomic_create_state callback and its drm_atomic_helper_bridge_create_state() counterpart. Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com> # imx8mp + sn65dsi84 + bridge hotplug Link: https://patch.msgid.link/20260619-drm-no-more-bridge-reset-v3-33-ff399263111b@kernel.org Signed-off-by: Maxime Ripard <mripard@kernel.org>
Since commit 55b48e2 ("genirq/devres: Add error handling in devm_request_*_irq()"), devm_request_threaded_irq() automatically logs detailed error messages on failure. Remove the now-redundant driver-specific dev_err_probe() call. Signed-off-by: Pan Chuang <panchuang@vivo.com> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com> Link: https://patch.msgid.link/20260723131649.134127-6-panchuang@vivo.com Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Enable multimedia related drivers used by the Radxa ROCK 5B (used as an example, the config options are relevant for most Rockchip RK3588 and RK3576 boards), so that all hardware is supported by the default config. * Synopsys MIPI CSI2RX - RK3588 CSI2 Controller * RKVDEC - RK3588/RK3576 video decoder for H.264 and H.265 * Sony IMX415 - Sensor used by the Radxa Cam 4K module * Rocket - RK3588 NPU driver * Verisilicon IOMMU - IOMMU used by RK3588 AV1 video decoder * Innosilicon CSI D-PHY - RK3588 CSI PHY (one of them) Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Also log to normal dmesg to assist debugging hard reset issues. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Also log to normal dmesg to assist debugging hard reset issues. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Add a new DIV_ROUND_UP helper, which cannot overflow when big numbers are being used. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
The clock framework handles clock rates as "unsigned long", so u32 on 32-bit architectures and u64 on 64-bit architectures. The current code casts the dividend to u64 on 32-bit to avoid a potential overflow. For example DIV_ROUND_UP(3000000000, 1500000000) = (3.0G + 1.5G - 1) / 1.5G = = OVERFLOW / 1.5G, which has been introduced in commit 9556f9d ("clk: divider: handle integer overflow when dividing large clock rates"). On 64 bit platforms this masks the divisor, so that only the lower 32 bit are used. Thus requesting a frequency >= 4.3GHz results in incorrect values. For example requesting 4300000000 (4.3 GHz) will effectively request ca. 5 MHz. Requesting clk_round_rate(clk, ULONG_MAX) is a bit of a special case, since that still returns correct values as long as the parent clock is below 8.5 GHz. Fix this by switching to DIV_ROUND_UP_NO_OVERFLOW, which cannot overflow. This avoids any requirements on the arguments (except that divisor should not be 0 obviously). Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Replace the open coded abs_diff() with the existing helper function. Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Add rfkill support for bluetooth. Bluetooth support itself is still missing, but this ensures bluetooth can be powered off properly. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Also describe wake signal PCIe pinmux for the onboard LAN card. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
This drops to hs200 mode and 150Mhz as this is actually stable across eMMC modules. There exist some that are incompatible at higher rates with the rk3588 and to avoid your filesystem corrupting due to IO errors, be more conservative and reduce the max. speed. Signed-off-by: Carsten Haitzler <raster@rasterman.com> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
When I converted rk808 to device managed resources I converted the rk808 specific pm_power_off handler to devm_register_sys_off_handler() using SYS_OFF_MODE_POWER_OFF_PREPARE, which is allowed to sleep. I did this because the driver's poweroff function makes use of regmap and the backend of that might sleep. But the PMIC poweroff function will kill off the board power and the kernel does some extra steps after the prepare handler. Thus the prepare handler should not be used for the PMIC's poweroff routine. Instead the normal SYS_OFF_MODE_POWER_OFF phase should be used. The old pm_power_off method is also being called from there, so this would have been a cleaner conversion anyways. But it still makes sense to investigate the sleep handling and check if there are any issues. Apparently the Rockchip and Meson I2C drivers (the only platforms using the PMICs handled by this driver) both have support for atomic transfers and thus may be called from the atomic poweroff context. Things are different on the SPI side. That is so far only used by rk806 and that one is only used by Rockchip RK3588. Unfortunately the Rockchip SPI driver does not support atomic transfers. That means this change will introduce an error splash directly before doing the final power off on all upstream supported RK3588 boards: [ 13.761353] ------------[ cut here ]------------ [ 13.761764] Voluntary context switch within RCU read-side critical section! [ 13.761776] WARNING: CPU: 0 PID: 1 at kernel/rcu/tree_plugin.h:330 rcu_note_context_switch+0x3ac/0x404 [ 13.763219] Modules linked in: [ 13.763498] CPU: 0 UID: 0 PID: 1 Comm: systemd-shutdow Not tainted 6.10.0-12284-g2818a9a19514 #1499 [ 13.764297] Hardware name: Rockchip RK3588 EVB1 V10 Board (DT) [ 13.764812] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 13.765427] pc : rcu_note_context_switch+0x3ac/0x404 [ 13.765871] lr : rcu_note_context_switch+0x3ac/0x404 [ 13.766314] sp : ffff800084f4b5b0 [ 13.766609] x29: ffff800084f4b5b0 x28: ffff00040139b800 x27: 00007dfb4439ae80 [ 13.767245] x26: ffff00040139bc80 x25: 0000000000000000 x24: ffff800082118470 [ 13.767880] x23: 0000000000000000 x22: ffff000400300000 x21: ffff000400300000 [ 13.768515] x20: ffff800083a9d600 x19: ffff0004fee48600 x18: fffffffffffed448 [ 13.769151] x17: 000000040044ffff x16: 005000f2b5503510 x15: 0000000000000048 [ 13.769787] x14: fffffffffffed490 x13: ffff80008473b3c0 x12: 0000000000000900 [ 13.770421] x11: 0000000000000300 x10: ffff800084797bc0 x9 : ffff80008473b3c0 [ 13.771057] x8 : 00000000ffffefff x7 : ffff8000847933c0 x6 : 0000000000000300 [ 13.771692] x5 : 0000000000000301 x4 : 40000000fffff300 x3 : 0000000000000000 [ 13.772328] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000400300000 [ 13.772964] Call trace: [ 13.773184] rcu_note_context_switch+0x3ac/0x404 [ 13.773598] __schedule+0x94/0xb0c [ 13.773907] schedule+0x34/0x104 [ 13.774198] schedule_timeout+0x84/0xfc [ 13.774544] wait_for_completion_timeout+0x78/0x14c [ 13.774980] spi_transfer_one_message+0x588/0x690 [ 13.775403] __spi_pump_transfer_message+0x19c/0x4ec [ 13.775846] __spi_sync+0x2a8/0x3c4 [ 13.776161] spi_write_then_read+0x120/0x208 [ 13.776543] rk806_spi_bus_read+0x54/0x88 [ 13.776905] _regmap_raw_read+0xec/0x16c [ 13.777257] _regmap_bus_read+0x44/0x7c [ 13.777601] _regmap_read+0x60/0xd8 [ 13.777915] _regmap_update_bits+0xf4/0x13c [ 13.778289] regmap_update_bits_base+0x64/0x98 [ 13.778686] rk808_power_off+0x70/0xfc [ 13.779024] sys_off_notify+0x40/0x6c [ 13.779356] atomic_notifier_call_chain+0x60/0x90 [ 13.779776] do_kernel_power_off+0x54/0x6c [ 13.780146] machine_power_off+0x18/0x24 [ 13.780499] kernel_power_off+0x70/0x7c [ 13.780845] __do_sys_reboot+0x210/0x270 [ 13.781198] __arm64_sys_reboot+0x24/0x30 [ 13.781558] invoke_syscall+0x48/0x10c [ 13.781897] el0_svc_common+0x3c/0xe8 [ 13.782228] do_el0_svc+0x20/0x2c [ 13.782528] el0_svc+0x34/0xd8 [ 13.782806] el0t_64_sync_handler+0x120/0x12c [ 13.783197] el0t_64_sync+0x190/0x194 [ 13.783527] ---[ end trace 0000000000000000 ]--- The board will shutdown nevertheless, since this also re-enables interrupts. A proper fix for this requires changes to the core SPI subsystem and will be done as a follow-up series. Note, that this patch also fixes a problem for the Asus C201. Without the function being registered as a proper shutdown handler the syscall for poweroff exits early and does not even call the shutdown prepare handler. This in turn means the system can no longer poweroff properly since my original change. Fixes: 4fec8a5 ("mfd: rk808: Convert to device managed resources") Cc: stable@vger.kernel.org Reported-by: Urja <urja@urja.dev> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Add the documentation for VOP2 video ports reset clocks. One reset can be set per video port. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
At the end of initialization, each VP clock needs to be reset before they can be used. Failing to do so can put the VOP in an undefined state where the generated HDMI signal is either lost or not matching the selected mode. This issue can be reproduced by switching modes multiple times. Depending on the setup, after about 10 mode switches, the signal will be lost and the value in register 0x890 (VSYNCWIDTH + VFRONT) will take the value `0x0000018c`. That makes VSYNCWIDTH=0, which is wrong. Adding the clock resets after the VOP configuration fixes the issue. Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
This adds the needed clock resets for all rk3588(s) based SOCs. Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
The RK3588 EVB1 comes with a W552793DBA-V10 Touchscreen/Display combination. It contains a Wanchanglong W552793BAA panel and a Goodix GT1158 touchscreen. This adds the DT description of it. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
A previous from Detlev Casanova adds reset handling for the video ports. This also resets the AHB and AXI interface when the system binds the VOP2 controller. This fixes issues when the bootloader (or a previously running kernel when using kexec) left the VOP2 initialized to some degree. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Fix the USB-C connector description, so that it follows the binding: port@0 is the high-speed lanes port@1 is the super-speed lanes port@2 is the SBU lanes Right now the high-speed and super-speed links are swapped and for the high-speed lanes the link points to the controller instead of the PHY. I'm still investigating if this should be changed. This also updates the port naming, so that it describes the hardware instead of how the drivers are using the information. These are effectively the same, but the DT should describe hardware and not software. Fixes: b37146b ("arm64: dts: rockchip: add USB3 to rk3588-evb1") Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Any camera related IP of the RK3588 is not yet supported and the cameras must be handled via overlays anyways, but it is sensible to expose the related I2C interfaces by default. This allows using i2cdetect to investigate anything connected to the CSI connectors right now. Since the Rockchip I2C driver implements proper power management there are no disadvantages, if nothing is connected to the port. Note, that the second CSI port's I2C in the Rock 5B+ and Rock 5T reuse I2C4, which is already used by fusb302 and thus already enabled. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
With USB type C connectors, the vbus detect pin of the OTG controller attached to it is pulled high by a USB Type C controller chip such as the fusb302. This means USB enumeration on Type-C ports never works, as the vbus is always seen as high. Rockchip added some GRF register flags to deal with this situation. The RK3576 TRM calls these "soft_vbusvalid_bvalid" (con0 bit index 15) and "soft_vbusvalid_bvalid_sel" (con0 bit index 14). Downstream introduces a new vendor property which tells the USB 2 PHY that it's connected to a type C port, but we can do better. Since in such an arrangement, we'll have an OF graph connection from the USB controller to the USB connector anyway, we can walk said OF graph and check the connector's compatible to determine this without adding any further vendor properties. Do keep in mind that the usbdp PHY driver seemingly fiddles with these register fields as well, but what it does doesn't appear to be enough for us to get working USB enumeration, presumably because the whole vbus_attach logic needs to be adjusted as well either way. Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> Link: https://lore.kernel.org/r/20250610-rk3576-sige5-usb-v4-1-7e7f779619c1@collabora.com Signed-off-by: Sebastian Reichel <sre@kernel.org>
adc-keys, unlike gpio-keys, does not allow linux,input-type as a valid property. This makes it impossible to model devices that have ADC inputs that should generate switch events. Add the property to the binding with the same default as gpio-keys. Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> Reviewed-by: Heiko Stuebner <heiko@sntech.de> Link: https://lore.kernel.org/r/20250630-rock4d-audio-v1-1-0b3c8e8fda9c@collabora.com Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Instead of doing something like what gpio-keys is doing, adc-keys hardcodes that all keycodes must be of type EV_KEY. This limits the usefulness of adc-keys, and overcomplicates the code with manual bit-setting logic. Instead, refactor the code to read the linux,input-type fwnode property, and get rid of the custom bit setting logic, replacing it with input_set_capability instead. input_report_key is replaced with input_event, which allows us to explicitly pass the type. Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> Reviewed-by: Heiko Stuebner <heiko@sntech.de> Link: https://lore.kernel.org/r/20250630-rock4d-audio-v1-2-0b3c8e8fda9c@collabora.com Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
The RADXA ROCK 4D, like many other Rockchip-based boards, uses an ES8388 analog audio codec. On the production version of the board, the codec's LOUT1 and ROUT1 pins are tied to the headphone jack, whereas pins LOUT2 and ROUT2 lead to a non-populated speaker amplifier that itself leads to a non-populated speaker jack. The schematic is still haunted by the ghosts of those symbols, but it clearly marks them as "NC". The 3.5mm TRRS jack has its microphone ring (and ground ring) wired to the codec's LINPUT1 and RINPUT1 pins for differential signalling. Furthermore, it uses the SoCs ADC to detect whether the inserted cable is of headphones (i.e., no microphone), or a headset (i.e., with microphone). The way this is done is that the ADC input taps the output of a 100K/100K resistor divider that divides the microphone ring pin that's pulled up to 3.3V. There is no ADC level difference between a completely empty jack and one with a set of headphones (i.e., ones that don't have a microphone) connected. Consequently headphone insertion detection isn't something that can be done. Add the necessary codec and audio card nodes. The non-populated parts, i.e. LOUT2 and ROUT2, are not modeled at all, as they are not present on the hardware. Also, add an adc-keys node for the headset detection, which uses an input type of EV_SW with the SW_MICROPHONE_INSERT keycode. Below the 220mV pressed voltage level of our SW_MICROPHONE_INSERT switch, we also define a button that emits a KEY_RESERVED code, which is there to model this part of the voltage range as not just being extra legroom for the button above it, but actually a state that is encountered in the real world, and should be recognised as a valid state for the ADC range to be in so that no "closer" ADC button is chosen. Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> Link: https://lore.kernel.org/r/20250630-rock4d-audio-v1-3-0b3c8e8fda9c@collabora.com Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
On Radxa ROCK 4D boards we are seeing some issues with PHY detection and stability (e.g. link loss or not capable of transceiving packages) after new board revisions switched from a dedicated crystal to providing the 25 MHz PHY input clock from the SoC instead. This board is using a RTL8211F PHY, which is connected to an always-on regulator. Unfortunately the datasheet does not explicitly mention the power-up sequence regarding the clock, but it seems to assume that the clock is always-on (i.e. dedicated crystal). By doing an explicit reset after enabling the clock, the issue on the boards could no longer be observed. Note, that the RK3576 SoC used by the ROCK 4D board does not yet support system level PM, so the resume path has not been tested. Cc: stable@vger.kernel.org Fixes: 7300c9b ("net: phy: realtek: Add optional external PHY clock") Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
According to the Ethernet controller device tree binding "rgmii-id"
means, that the PCB does not have extra long lines to add the required
delays. This is indeed the case for the ROCK 4D.
The problem is, that the Rockchip MAC Linux driver interprets the
interface type differently and abuses the information to configure
RX and TX delays in the MAC using (vendor) properties 'rx_delay' and
'tx_delay'.
When Detlev Casanova upstreamed the ROCK 4D device tree, he used the
correct description for the board ("rgmii-id"). This results in no delays
being configured in the MAC. At the same time the PHY will provide
some delays.
This works to some degree, but is not a stable configuration. All five
ROCK 4D production boards, which have recently been added to the Collabora
LAVA lab for CI purposes have trouble with data not getting through
after a connection has been established.
Using the same delay setup as the vendor device tree fixes the
functionality (at the cost of not properly following the DT binding).
As we cannot fix the driver behavior for RK3576 (some other boards
already depend on this), let's update the ROCK 4D DT instead.
Cc: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Before the Rockchip PCIe driver has been switched over to the FIELD_PREP_WM16 macro, PCIE_CLIENT_ENABLE_LTSSM and PCIE_CLIENT_DISABLE_LTSSM were setting bits with a mask of 0xc = 0b1100, which means BIT 2 and BIT 3. After the conversion it only sets bit 2, with bit 3 being handled by a separate define named PCIE_CLIENT_LD_RQ_RST_GRT. Apparently the conversion missed to make use of this new macros resulting in the third bit not being set. Fixes: 30e9195 ("PCI: dw-rockchip: Switch to FIELD_PREP_WM16 macro") Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
This reverts c930b10 ("PCI: dw-rockchip: Simplify regulator setup with devm_regulator_get_enable_optional()"), which nicely cleaned up the code. The vpcie3v3 regulator handle is needed to disable the regulator during system suspend (to be added in its own patch). Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
By moving devm_phy_get() to the probe routine, rockchip_pcie_phy_init() can be used to re-initialize the PCIe PHY, which is for example needed after a system suspend/resume cycle. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
HD3SS3220 requires two supply regulators to operate. It also strictly requires that 5V is present before 3.3V, otherwise it gets backpowered in a non-functional state through the 3.3V rail and kills the I2C bus. Add both supply regulators to enable their explicit description in board device trees. Signed-off-by: Alexey Charkov <alchark@flipper.net>
HD3SS3220 requires VDD5 input to be present 2ms before VCC33 is applied, or else it gets backpowered via the 3.3V rail in a non-functional state and wedges the I2C bus, bringing down all devices on it. Enable both regulators in the datasheet prescribed sequence if provided. Signed-off-by: Alexey Charkov <alchark@flipper.net>
Signed-off-by: Alexey Charkov <alchark@flipper.net>
Signed-off-by: Alexey Charkov <alchark@flipper.net>
Signed-off-by: Alexey Charkov <alchark@flipper.net>
rocket_ioctl_submit_job() releases rjob through rocket_job_put() on allocation error paths. rocket_job_cleanup() unconditionally calls rocket_iommu_domain_put(job->domain), but job->domain is assigned only after task copying and BO lookups. A failure before that assignment can therefore clean up a job with a NULL domain pointer. Take the per-file domain reference before the first error path can release rjob. Also clear rjob->tasks after freeing it in rocket_copy_tasks(), so the common cleanup path cannot free the task array again after a task-copy error. Fixes: 0810d5a ("accel/rocket: Add job submission IOCTL") Cc: stable@vger.kernel.org Signed-off-by: Shuvam Pandey <shuvampandey1@gmail.com> Link: https://lore.kernel.org/r/6a454b48.6a8fa39a.27019b.984b@mx.google.com Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
…_push()
rocket_job_push() allocates a temporary array to hold all input and
output GEM object pointers:
bos = kvmalloc_array(job->in_bo_count + job->out_bo_count,
sizeof(void *), GFP_KERNEL);
memcpy(bos, job->in_bos, job->in_bo_count * sizeof(void *));
memcpy(&bos[job->in_bo_count], job->out_bos, ...);
Two bugs exist:
1. Missing NULL check: if kvmalloc_array() fails, bos is NULL and
the subsequent memcpy() dereferences it, causing a kernel NULL
pointer dereference.
2. Integer overflow: in_bo_count and out_bo_count are both u32, set
directly from userspace-supplied in_bo_handle_count and
out_bo_handle_count with no prior validation. Their sum is computed
in u32 arithmetic and can wrap to a smaller value, causing the
allocation count passed to kvmalloc_array() to be smaller than
intended. Subsequent uses still operate on the original counts when
copying and locking objects, which may lead to out-of-bounds accesses
on the temporary array.
Fix by using check_add_overflow() to detect count overflow before the
allocation, and adding a NULL check on the allocation result.
Fixes: 0810d5a ("accel/rocket: Add job submission IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
Link: https://lore.kernel.org/r/20260524155716.90955-1-meatuni001@gmail.com
Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
In rocket_job_run(), after taking an extra fence reference for
job->done_fence via dma_fence_get(), the error paths have three bugs:
- The dma_fence reference held by job->done_fence is never released,
causing a reference leak.
- pm_runtime_get_sync() increments the usage counter even on failure,
but the error path does not decrement it, leaking the runtime PM
reference and preventing the NPU from suspending.
- A valid but unsignaled fence is returned to the DRM scheduler,
which triggers WARN("Fence ... released with pending signals!")
when the scheduler drops its reference.
Fix by replacing pm_runtime_get_sync() with pm_runtime_resume_and_get()
which auto-balances the usage counter on failure, releasing both fence
references on error, and returning ERR_PTR(ret) instead of the
unsignaled fence.
Cc: stable@vger.kernel.org
Fixes: 0810d5a ("accel/rocket: Add job submission IOCTL")
Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
Link: https://lore.kernel.org/r/20260610071045.3414828-1-zhaojinming@uniontech.com
[tomeu: Refactored error paths to use consolidated goto labels]
Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
The RK3568 NPU rail (vdd_npu) needs to be enabled before the domain is powered on and disabled after it is powered off. Give DOMAIN_RK3568 a regulator parameter (like DOMAIN_RK3588 already has) so the NPU domain can set need_regulator, letting genpd manage the rail wired up as the domain's domain-supply instead of marking it always-on in DT. Suggested-by: Chaoyi Chen <chaoyi.chen@rock-chips.com> Signed-off-by: Midgy BALON <midgy971@gmail.com> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com> Reviewed-by: Heiko Stuebner <heiko@sntech.de> Signed-off-by: Ulf Hansson <ulfh@kernel.org>
rocket_core_init() hands core->clks to devm_clk_bulk_get() without ever
setting the .id members. The rocket_core array is allocated with
devm_kcalloc() in rocket_device_init(), and rocket_probe() only fills in
.rdev, .dev and .index, so all four clk_bulk_data entries are requested
with a NULL con_id (unlike core->resets, whose ids are set a few lines
above).
clk_get(dev, NULL) ends up in of_clk_get_hw(np, 0, NULL), and
of_parse_clkspec() only consults "clock-names" when a name was passed,
so the index stays 0 for all four entries. Every entry therefore ends up
holding a handle to the *first* clock of the DT "clocks" property, i.e.
ACLK_NPUn. Nothing fails: probe succeeds and the driver believes it owns
four different clocks.
The consequence is that rocket_device_runtime_resume() prepares and
enables the AXI clock four times, while hclk, pclk and - most
importantly - the NPU compute clock ("npu", SCMI_CLK_NPU on RK3588) are
never prepared or enabled by this driver at all. The NPU still works
only because the Rockchip power-domain driver sets GENPD_FLAG_PM_CLK and
its attach_dev() callback walks the device node with of_clk_get() and
adds every clock to the pm_clk list, so genpd happens to keep the
remaining clocks running. The bug is therefore latent today, but it
means the driver holds no reference to the clock that actually feeds the
NPU, which stands in the way of any future frequency scaling
(OPP/devfreq) work.
Found on an Orange Pi 5 Plus (RK3588) by reading the live clock tree:
/sys/kernel/debug/clk/clk_summary shows four "fdab0000.npu" consumer
handles on aclk_npu0 (and likewise on aclk_npu1/aclk_npu2 for the other
two cores), while hclk_npu0, pclk_npu_root and scmi_clk_npu have no
"fdab0000.npu" consumer at all - their only consumers are the
"npu@fdab0000" handles created by the power-domain driver via
of_clk_get().
Set the ids explicitly, in the order mandated by the binding
(Documentation/devicetree/bindings/npu/rockchip,rk3588-rknn-core.yaml):
aclk, hclk, npu, pclk. After the change the driver holds one handle per
distinct clock and clk_bulk_prepare_enable() covers all four.
Note that this is a user-visible tightening for out-of-tree DTs: the
old NULL-id requests resolved by index and succeeded no matter what
"clock-names" contained, while the named requests fail probe with
-ENOENT when one of the four names is missing. That is the right
outcome for in-tree users - the binding requires exactly these four
clock-names and rk3588-base.dtsi carries them on all three cores - but
a DT that relied on the permissive lookup goes from silently running on
the wrong clock handles to not probing at all, so record the change
here where git log will find it.
Fixes: ed98261 ("accel/rocket: Add a new driver for Rockchip's NPU")
Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
Reviewed-by: Jiaxing Hu <gahing@gahingwoo.com>
rocket_job_handle_irq() writes OPERATION_ENABLE and INTERRUPT_CLEAR before taking job_lock, while rocket_job_hw_submit() writes OPERATION_ENABLE from inside it. The two can therefore race: a completion being handled on one core can write its zero after a submit on the same core has written its one, and stop a task that has only just started. Nothing in tree hits this often, because the interrupt is the only completion path and it does not overlap its own submit, but the ordering is wrong on its own terms. Move both writes inside the existing scoped_guard() rather than adding a second critical section, so stopping the block and deciding what to start next are one atomic step. Fixes: 0810d5a ("accel/rocket: Add job submission IOCTL") Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com> Tested-by: Igor Paunovic <royalnet026@gmail.com> # RK3588, three cores
rocket_reset() calls drm_sched_stop(), which stops the scheduler and returns. It does not wait for a threaded handler that is already running, so the comment that follows, "Remaining interrupts have been handled", states an assumption rather than something the code arranges. Call synchronize_irq(core->irq) after drm_sched_stop() and reword the comment to say what holds afterwards. It has to go before the scoped_guard(mutex, &core->job_lock) rather than inside it. rocket_job_handle_irq() takes job_lock, so waiting for the handler while holding that lock would be waiting for a handler that is waiting for us. Nothing is held at that point, and both callers, rocket_job_timedout() and rocket_reset_work(), run in process context, so sleeping there is allowed. This does not stop a handler that has already read in_flight_job from finishing its work on the job the reset is about to drop. That window needs the check and the register writes to be one step under the lock, which is what the previous patch does; the two are complementary. Mask the block before the sync as well. INTERRUPT_MASK is armed by hw_submit() on every submit and cleared only by the hardirq, so on an ordinary timeout it is still live and a completion can arrive after synchronize_irq() returns. Nothing is lost by clearing it, since the next submit arms it again. That write is the first register access this function has ever made, and it is guarded, because the function holds no runtime PM reference of its own. The only reference in the window belongs to in_flight_job, and the completion path can have put it and cleared the pointer before the timeout worker arrives: drm_sched_stop() sits in between and can block on cancel_work_sync() and on a dma_fence_wait(), and it subtracts every pending job's credits, so rocket_job_is_idle() is true and rocket_device_runtime_suspend() will not refuse. With the autosuspend delay elapsed the clocks are off and both NPU domains are down. A register access in that state takes an async SError on this hardware, which is the failure two later patches in this series describe from the power-on side. pm_runtime_get_if_active() resumes nothing and allocates nothing; if the core is already down there is no live interrupt to mask and the following synchronize_irq() is all that is needed. Igor Paunovic asked the general form of this on v8 -- whether rocket_reset() should hold a reference -- and it was deferred then because nothing in the path touched a register. This patch is what makes it matter. The deadlock this placement avoids would not have been reported. The wait is on desc->wait_for_threads rather than on a lock, so lockdep does not model it and it would have hung silently. Suggested-by: Igor Paunovic <royalnet026@gmail.com> Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
rocket_reset() drops the in-flight job's runtime PM reference with pm_runtime_put_noidle(), a bare decrement that requests nothing. The core is left at usage_count 0 but still runtime-active with no idle request pending, so it does not suspend until something else asks, and on a platform whose power domain does work on power-on that work never happens. On RK3576 that work is a bus interface reset the domain cycles when it comes up. Without it the NPU's IOMMU stops answering, and the job after a timeout returns a surface of the output zero point with rk_iommu reporting that MMU_DTE_ADDR is not functioning. Measured on a ROCK 4D in one boot, three runs, one variable between them. With the bare put the core reads runtime-active with its rail still up after the reset, the IOMMU reports the failure on the next attach and the inference returns 0 of 128 channels. With the reference put back through pm_runtime_put_autosuspend() the core reads suspended with the rail down, there is no IOMMU message, and the same inference returns 128 of 128. A third run repeating the first failed the same way. It also matches the put in the completion path a few lines away, so the reset path no longer leaves the device in a state the rest of the driver never produces. The remaining put, on the error path in rocket_job_run(), is a plain pm_runtime_put() and is left alone here: it unwinds a get_sync() that never reached the hardware, and changing it belongs in its own patch. Igor Paunovic ran the differential on RK3588: 45 induced resets across three cores, with and without the two preceding patches, and the domain dropped every single time with no MMU message on either kernel. So this is not rocket-wide. His conditions cross a healthy block with a lowered timeout rather than a hung one, which he was careful to say his protocol cannot settle, but it is what scopes the change to RK3576. Link: https://lore.kernel.org/all/20260819073530.6087-1-royalnet026@gmail.com/ Fixes: 0810d5a ("accel/rocket: Add job submission IOCTL") Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
rocket_job_handle_irq() stops the block and then either starts the job's next task or retires the job. The second half is a step of its own and reads better with a name, now that taking the register writes under job_lock has moved it a level deeper inside the scoped guard. Move it to rocket_job_next_locked(). The early return that used to leave the handler now leaves the helper, which is the same thing here: the scoped guard drops job_lock either way and nothing follows it. Doing it as its own patch keeps the locking fix at the head of the series minimal, so a bisect that stops before this one gets that fix and nothing else. There is one caller, and no functional change. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com> Reviewed-by: Igor Paunovic <royalnet026@gmail.com>
The RK3576 NPU has two cores of the same RKNN block the RK3588 binding already describes, but it wires them up differently: two extra CBUF clocks, two power domains per core, and a single reset instead of two. It also has no NPU SRAM supply. Widen the property ranges to cover both, then pin each SoC back to its own shape in allOf so nothing loosens for RK3588, and keep sram-supply required for rockchip,rk3588-rknn-core only. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Some domains do not come up in a usable state on their own and need their resets cycled once power is on. The RK3576 NPU domains are one case: without it the first access after power-on takes an async SError. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
The RK3576 NPU MMUs are rk3568-iommu compatible but take five clocks where every other Rockchip MMU takes two, the extra three being the compute clock and the two convolution buffer clocks. Give them a compatible of their own and pin both sides with an allOf, so that an rk3568-iommu cannot carry five clocks and an NPU MMU cannot carry two. Describing the extra clocks as belonging to one SoC without saying so in the schema, which is what a comment on a description does, leaves both of those spellings valid. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
The RK3576 NPU domains need a short settle time after the idle request is released before the registers behind the domain answer. Without it the QoS writes that rockchip_pmu_restore_qos() issues land while the domain is still coming up, and the NPU throws an async SError on the first cold power-on. Give rockchip_domain_info an optional delay_us and wait for it between releasing idle and restoring QoS. Rename DOMAIN_M_O_R_G to DOMAIN_M_O_R_G_W, since the suffixes name the fields the macro sets and this one now also carries a wakeup delay; RK3576 is its only user, so the old spelling is not kept around. While the macro is being rewritten, give it the regulator argument that DOMAIN_M_O_R and DOMAIN_M_R already take. Without .need_regulator set, rockchip_pd_regulator_enable() returns early for every RK3576 domain, so a domain-supply in the device tree is never looked up and never enabled. Add a DOMAIN_RK3576_R spelling that passes true and use it for RK3576_PD_NPU, which is the one RK3576 domain with a rail of its own; every other domain passes false and is unchanged. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Some Rockchip domains come out of power-on with their bus interface in an undefined state. On the RK3576 NPU this shows up as a hang on the first register access after the domain is switched on, and pulsing the domain's resets at this point clears it. Take the domain node's resets if it has any, and pulse them between releasing idle and restoring QoS. The resets are optional, so domains that do not list any are unaffected. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
The RK3576 carries the same RKNN block with a different set of clocks and resets, so the counts cannot stay compile-time constants. Add a soc_data struct to the of_device_id match data and take the bulk counts from it. RK3588 keeps four clocks and two resets, so nothing changes for it, and the arrays keep their present sizes: the SoC that needs a longer one grows it in the patch that adds the names. rocket_core_reset() is switched over as well. It is the same array, and leaving it on ARRAY_SIZE() would walk entries that were never acquired once a SoC asks for fewer. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
The RK3576 has two cores of the same RKNN block and a few platform differences: - the CBUF (convolution buffer) has its own clock domain, so the core needs six clocks rather than four; - there is no per-core hclk reset. The CRU has SRST_A_RKNN0 and SRST_A_RKNN1 but no SRST_H_RKNN0 or SRST_H_RKNN1, so a core takes one reset where RK3588 takes two; - the NPU spans two power domains, and a device with more than one is skipped by the driver-core single-domain auto-attach, so the list has to be attached explicitly; - PC_TASK_CON packs the task number with sixteen bits rather than twelve, moving the three controls above it up by four. That last one is the reason this series has been reporting, since v3, that the block accepts exactly one task per reset. rocket_registers.h is generated from the RK3588 description, so writing it unchanged to an RK3576 asks for task_number 0x7001, which is 28673 tasks, and puts TASK_COUNT_CLEAR on a bit that does nothing. The counter is then only ever cleared by a reset. The layout was confirmed by Chaoyi Chen of Rockchip, including a fourth control at BIT(18), task_last_layer_clear, which belongs on every submit alongside the count clear: https://lore.kernel.org/all/4f300b78-d96d-4d98-8819-dc292b0c9b97@rock-chips.com/ With that written correctly a job of several tasks runs to completion, the completion interrupt arrives, and /proc/interrupts counts up. A convolution submitted three times with three different inputs is byte exact against the CPU reference each time, with no reset in between and with nothing retiring the job but the interrupt. Counting the cores now walks the driver's own match table instead of a second, hand-kept list of compatibles. The array sized from that count is indexed by every core that goes on to probe, so the two lists cannot be allowed to disagree. All of it hangs off the soc_data added earlier, so the RK3588 path keeps its existing counts and behaviour. The match table moves to rocket_drv.h so rocket_device.c can walk it with for_each_matching_node() rather than repeating a for_each_compatible_node() loop per SoC, which also keeps num_cores in step with the table that sizes the array it counts into. The declaration needs struct of_device_id, taken from <linux/device-id/of.h> rather than <linux/mod_devicetable.h>, which carries every subsystem's tables with it. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Add the two RKNN cores and their IOMMUs. Both cores are disabled by default; boards enable what they wire up. PD_NPU0 and PD_NPU1 are siblings under PD_NPUTOP and hold one core each, but the convolution buffer and the DSU sit above them: ACLK_RKNN_CBUF, HCLK_RKNN_CBUF and CLK_RKNN_DSU0 belong to the block rather than to either core, and PD_NPUTOP already lists all three. Add them to both core domains as well, so a core domain switching state has the clocks of the path it shares running, and give each core domain the BIU reset that the pmdomain driver now cycles once power is on. Each core lists both core domains, its own first, so that a core in use has the whole block powered. Whether a single core can reach the shared path with the sibling domain off is not something this series establishes; listing both is the description that has been tested here. The IOMMU in front of each core lists that core's domain only. Label the outer PD_NPU node so a board can attach the NPU rail to the domain that gates the block. Signed-off-by: Jiaxing Hu <gahing@gahingwoo.com>
Wire vdd_npu_s0 (PMIC dcdc-reg2) into the NPU power domain and enable the first RKNN core with its MMU. Without domain-supply on pd_npu the domain cannot be powered, and the core's npu-supply is what keeps the regulator from being switched off as unused during late init. Only core 0 is enabled. The RK3576 has two, but single-core is the supported bring-up configuration and core 1 needs its own validation before it is turned on. Both board revisions inherit this from the shared dtsi.
Collaborator
|
I’m very skeptical of untested contributions. Asking an LLM to patch stuff is not the hard part: understanding what it does and what it missed is. And statements like “I don’t have a cross-compiler so I didn’t even try building what the LLM produced” are outright disrespectful of maintainers’ time |
alchark
force-pushed
the
flipper-devel
branch
from
September 3, 2026 15:52
e8750f6 to
ae5dbb0
Compare
alchark
force-pushed
the
flipper-devel
branch
2 times, most recently
from
September 11, 2026 08:13
d1b95bc to
e1d9cab
Compare
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
20 participants
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.
Draft. Not built, not booted. No cross toolchain or container was available on the machine this was prepared on, so nothing here has been compiled or run on hardware. Opening it so the shape of the change is reviewable, not because it is ready to merge.
What this is
18 commits enabling the RKNN NPU on RK3576. Only the last one is ours; the other 17 are other people's work, applied with
git amso authorship and Signed-off-by are intact.gahingwoo/linux-rk3576-npu)The risk to weigh: the 12-commit v9 series is unmerged and may change before it lands. If we take this, we are carrying an in-review series in a product kernel and will need to rebase as it churns. The alternative is waiting for it to merge upstream. That is a call for whoever owns the kernel branch, which is why this is a draft.
v9-0013from the original series is deliberately dropped. It enables the NPU on ROCK 4D, which is not our board. Our commit replaces it.Our commit
arm64: dts: rockchip: flipper-one: Enable the NPUdomain-supply = <&vdd_npu_s0>on&pd_npu, without which the domain cannot be powerednpu-supply = <&vdd_npu_s0>plusstatus = "okay"on&rknn_core_0, and&rknn_mmu_0enabledrk3576-flipper-one.dtsi, so bothrev-f0b0c1andrev-f0b1c2inherit it.What was actually verified
Static only, no compiler involved:
cpp+dtc): f0b0c1 95014 bytes, f0b1c2 95972 bytesnpu@27700000isrockchip,rk3576-rknn-core,status = "okay", and itsnpu-supplyphandle resolves to thedcdc-reg2node namedvdd_npu_s0pd_npu'sdomain-supplyresolves to that same phandlenpu@27708000andiommu@2770a000arestatus = "disabled", as intendednum_clks = 6against 6clock-names,num_resets = 1againstsrst_arocket_device.cusesfor_each_matching_node(rocket_dt_match)filtered onof_device_is_available(), so the disabled core 1 is not countedWhat was NOT verified
/dev/accel/accel0, no inferencedtbs_checkagainst dt-schemavdd_npu_s0survives past the late_initcall that drops unused regulators, which is the failure this whole series exists to avoidrocketGallium driver, but it has no RK3576 path, so even with this merged nothing will run models yetCompanion PR
flipperdevices/flipperone-linux-build-scripts#159 makes
CONFIG_DRM_ACCEL_ROCKETbuilt-in, which is required for any of this to work: as a module it probes after the regulator cleanup and the device never appears.