Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The NPU node simultaneously defines `power-domains` and `rockchip,pmu`, causing the generic power domain framework and the NPU driver to concurrently manipulate the same PMU power and idle registers. - [Low] Device tree nodes `npu@fde40000` and `iommu@fde4b000` are not sorted by unit address. Pre-existing issues: - [Critical] The driver calls `devm_clk_bulk_get` without initializing the `.id` fields of the clock bulk data array, causing all clock requests to incorrectly default to the first clock in the device tree node. -- commit dff3ba648a490ef1a1c6644d672ca0b9e1d68360 Author: Midgy BALON <[email protected]> arm64: dts: rockchip: rk356x: Add the NPU and its IOMMU This commit adds the NPU and its associated IOMMU device tree nodes to the RK3568 base devicetree. It also configures the necessary clocks, resets, and power domain for the NPU. > diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi > b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi > index 64bdd8b7754b5..50ce5a5e4fc24 100644 > --- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi [ ... ] > @@ -948,6 +955,37 @@ qos_rga_wr: qos@fe158300 { > reg = <0x0 0xfe158300 0x0 0x20>; > }; > > + rknn_core_0: npu@fde40000 { [Severity: Low] This isn't a bug, but should the npu@fde40000 and iommu@fde4b000 nodes be ordered alphanumerically by unit address? They appear to be inserted after qos@fe158300 rather than being placed right before gpu@fde60000. > + compatible = "rockchip,rk3568-rknn-core"; > + reg = <0x0 0xfde40000 0x0 0x1000>, > + <0x0 0xfde41000 0x0 0x1000>, > + <0x0 0xfde43000 0x0 0x1000>; > + reg-names = "pc", "cna", "core"; > + interrupts = <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cru ACLK_NPU>, <&cru HCLK_NPU>, > + <&scmi_clk SCMI_CLK_NPU>, <&cru PCLK_NPU_PRE>; [Severity: Critical] This is a pre-existing issue, but enabling this node exposes a bug where devm_clk_bulk_get() is called without initializing the .id fields in rocket_core_init(). Since the connection ID is NULL, does clk_get() implicitly fall back to of_clk_get(np, 0) for each loop iteration, mapping all four clock array entries to ACLK_NPU? Later, when rk3568_noc_init() dynamically scales the SCMI clock rate to 1 GHz: clk_set_rate(core->clks[ROCKET_RK3568_SCMI_CLK].clk, 1000000000UL); Does this mistakenly configure the rate of ACLK_NPU instead, which could overclock the AXI bus and lead to immediate hardware instability? > + clock-names = "aclk", "hclk", "npu", "pclk"; > + assigned-clocks = <&scmi_clk SCMI_CLK_NPU>; > + assigned-clock-rates = <200000000>; > + resets = <&cru SRST_A_NPU>, <&cru SRST_H_NPU>; > + reset-names = "srst_a", "srst_h"; > + power-domains = <&power RK3568_PD_NPU>; > + rockchip,pmu = <&pmu>; [Severity: High] Could setting both power-domains and rockchip,pmu cause a race condition with TF-A and corrupt hardware state? The generic power domain framework inherently manages power gating and NoC bus idle requests for this domain, and explicitly protects these PMU transitions with pmu->mutex. Concurrently, the rocket driver's rk3568_noc_init() issues direct, un-polled regmap_write() operations to the exact same PMU offsets: regmap_write(pmu, 0xa0, BIT(1 + 16)); ... regmap_write(pmu, 0x50, BIT(2 + 16)); Does bypassing GenPD's tracked state machine and the pmu->mutex lock create a layering violation here? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
