Re: [PATCH v3 4/6] ARM: dts: rockchip: Add support for RV1106 and RV1103

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Deleting the `&gpio2` node causes a kernel panic when reading pinctrl debugfs because the statically registered pins reference a missing bank node.
- [High] Schema violation and hardware misconfiguration by injecting `rockchip,grf` into `gpio-bank` nodes and assigning a dummy global GRF.
- [Low] The unit-address of the `gic` node does not match the first address in its `reg` property.
--

--- Patch [4]: [PATCH v3 4/6] ARM: dts: rockchip: Add support for RV1106 and RV1103 ---
commit 2a704554f0ba16f51aa70ddcf7fa1a4a81337f55
Author: Simon Glass <sjg@chromium.org>

ARM: dts: rockchip: Add support for RV1106 and RV1103
    
Add the devicetree files for the Rockchip RV1106, a Cortex-A7 SoC
aimed at IP cameras, and its RV1103 package variant, which has fewer
pins and no GPIO2 bank. The pinctrl data comes from the vendor kernel in
the Luckfox Pico SDK.

[1] https://github.com/LuckfoxTECH/luckfox-pico

> diff --git a/arch/arm/boot/dts/rockchip/rv1103.dtsi b/arch/arm/boot/dts/rockchip/rv1103.dtsi
> new file mode 100644
> index 0000000000000..025fde5bf09cf
> --- /dev/null
> +++ b/arch/arm/boot/dts/rockchip/rv1103.dtsi
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2022 Rockchip Electronics Co., Ltd.
> + */
> +
> +#include "rv1106.dtsi"
> +
> +/ {
> +	compatible = "rockchip,rv1103";
> +
> +	aliases {
> +		/delete-property/ gpio2;
> +	};
> +};
> +
> +/delete-node/ &gpio2;

[Severity: High]
Does deleting the gpio2 node cause a kernel panic when reading pinctrl
debugfs?

The Rockchip pinctrl driver registers pins statically based on the SoC
variant. Because the node is deleted, the pinctrl driver cannot retrieve
the per-bank rockchip,grf regmap, leaving it NULL or ERR_PTR.

When /sys/kernel/debug/pinctrl/pinctrl-rockchip/pinconf-pins iterates over
all statically registered pins and calls rockchip_pinconf_get(), would
passing this uninitialized regmap to regmap_read() trigger a kernel panic?

Should this use status = "disabled" instead to safely disable the bank?

[ ... ]
> diff --git a/arch/arm/boot/dts/rockchip/rv1106.dtsi b/arch/arm/boot/dts/rockchip/rv1106.dtsi
> new file mode 100644
> index 0000000000000..70d3da6bd03ef
> --- /dev/null
> +++ b/arch/arm/boot/dts/rockchip/rv1106.dtsi
[ ... ]
> +	pinctrl: pinctrl {
> +		compatible = "rockchip,rv1106-pinctrl";
> +		rockchip,grf = <&gpio1_ioc>;

[Severity: High]
Does assigning gpio1_ioc as the global GRF cause the generic
pinctrl-rockchip driver to incorrectly route pull/drive configurations for
all banks (0, 2, 3, 4) to gpio1_ioc?

To satisfy the rockchip,pinctrl.yaml requirement for a global rockchip,grf
on the parent node, gpio1_ioc is assigned here. This instructs the generic
driver to use it as info->regmap_base for all banks, which could result in
silent failures and register corruption.

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		gpio0: gpio@ff380000 {
> +			compatible = "rockchip,gpio-bank";
> +			reg = <0xff380000 0x100>;
> +			interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cru PCLK_PMU_GPIO0>, <&cru DBCLK_PMU_GPIO0>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-ranges = <&pinctrl 0 0 32>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +			rockchip,grf = <&gpio0_ioc>;

[Severity: High]
Does injecting rockchip,grf into the gpio-bank nodes violate the
Devicetree schema?

The rockchip,gpio-bank.yaml schema has additionalProperties: false and does
not define rockchip,grf. Adding it here would trigger a dtbs_check error.

[ ... ]
> +	soc {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		grf: syscon@ff000000 {
> +			compatible = "rockchip,rv1106-grf", "syscon";
> +			reg = <0xff000000 0x68000>;
> +		};
> +
> +		gic: interrupt-controller@ff1f0000 {
> +			compatible = "arm,gic-400";
> +			reg = <0xff1f1000 0x1000>,

[Severity: Low]
Does the unit-address in the node name mismatch the first address in the
reg property?

The node is named interrupt-controller@ff1f0000, but the first address in
its reg block is 0xff1f1000. This violates the Devicetree specification
which requires the unit-address to strictly match the first reg address,
causing a dtc warning.

Could this be renamed to interrupt-controller@ff1f1000?

> +			      <0xff1f2000 0x2000>,
> +			      <0xff1f4000 0x2000>,
> +			      <0xff1f6000 0x2000>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729133609.3465563-1-sjg@chromium.org?part=4