Re: [PATCH 11/11] arm64: dts: apple: Initial T6030 (M3 Pro) device trees

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `gpio-ranges` for `pinctrl_nub` and `pinctrl_aop` incorrectly reference `&pinctrl_ap`
- [High] Missing `power-domains` property in `pinctrl_nub`
- [Low] Incorrect unit address for `serial0` node (extra trailing zero)
--

--- Patch [11]: [PATCH 11/11] arm64: dts: apple: Initial T6030 (M3 Pro) device trees ---
commit fa2e19405991d7d2172c7f5e2dd0812674d702eb
Author: Janne Grunau <j@jannau.net>

arm64: dts: apple: Initial T6030 (M3 Pro) device trees

Minimal device trees for the M3 Pro based 14-inch and 16-inch MacBook
Pro released in November 2023.

> diff --git a/arch/arm64/boot/dts/apple/t6030.dtsi b/arch/arm64/boot/dts/apple/t6030.dtsi
> new file mode 100644
> index 0000000000000..f79fbadaa92cf
> --- /dev/null
> +++ b/arch/arm64/boot/dts/apple/t6030.dtsi

[ ... ]

> +		serial0: serial@2892000000 {
> +			compatible = "apple,s5l-uart";
> +			reg = <0x2 0x89200000 0x0 0x1000>;

[Severity: Low]
Does this node have an incorrect unit address? The unit address
@2892000000 has an extra trailing zero, as the reg property resolves to
0x289200000. This might trigger a simple_bus_reg device tree compiler
warning.

[ ... ]

> +		pinctrl_nub: pinctrl@3641f0000 {
> +			compatible = "apple,t6030-pinctrl", "apple,t8103-pinctrl";
> +			reg = <0x3 0x641f0000 0x0 0x4000>;

[Severity: High]
Is a power-domains property needed here, similar to other Apple SoCs?
Without it, the OS might not track and enable the NUB GPIO power state,
which could lead to an SError if the hardware is accessed while powered down.

> +
> +			interrupt-parent = <&aic>;
> +			interrupts = <AIC_IRQ 472 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 473 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 474 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 475 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 476 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 477 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 478 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-ranges = <&pinctrl_ap 0 0 37>;

[Severity: High]
Does this incorrectly reference &pinctrl_ap instead of a local pinctrl
node? This would map the NUB GPIOs onto the Application Processor (AP)
pin controller's domain, which could break GPIO operations for the NUB
controller and clobber the first 37 pins of the AP pin controller.

[ ... ]

> +		pinctrl_aop: pinctrl@374824000 {
> +			compatible = "apple,t6030-pinctrl", "apple,t8103-pinctrl";
> +			reg = <0x3 0x74824000 0x0 0x4000>;
> +
> +			interrupt-parent = <&aic>;
> +			interrupts = <AIC_IRQ 394 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 395 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 396 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 397 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 398 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 399 IRQ_TYPE_LEVEL_HIGH>,
> +				     <AIC_IRQ 400 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			clocks = <&clkref>;
> +
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-ranges = <&pinctrl_ap 0 0 54>;

[Severity: High]
Is it possible this node also incorrectly references &pinctrl_ap instead
of its local node? Similar to the pinctrl_nub node, this would break AOP
GPIO operations and clobber the first 54 pins of the AP pin controller.

> +			apple,npins = <54>;
> +
> +			interrupt-controller;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-apple-t603x-initial-devices-v1-0-55b305833123@jannau.net?part=11