Re: [PATCH v3 2/3] arm64: dts: allwinner: add A733 SoC description

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] arm64: dts: allwinner: missing A733 CCU dt-bindings headers and
  schemas
- [High] arm64: dts: allwinner: pio node lists 10 interrupts instead of
  11 on A733
- [Low] arm64: dts: allwinner: undocumented allwinner,sun60i-a733-uart
  compatible
--

commit 7fc84ade8f1b2531c3b62b4ae64bebc46f692d4e
Author: Enzo Adriano <enzo.adriano.code@gmail.com>

arm64: dts: allwinner: add A733 SoC description

This commit adds the base SoC device tree description for the Allwinner
A733 (sun60i-a733.dtsi), covering the CPUs, PSCI, timers, GIC, RTC,
clock/reset and pin controllers, UARTs, I2C controllers, and MMC0-2.

Link: https://gist.githubusercontent.com/apritzel/60fd7379b9560a15b8b352986b69d39f/raw/efd9cdf8f26ba5b0c7e138f093af44a1cf322469/sun60i-a733.dts

> diff --git a/arch/arm64/boot/dts/allwinner/sun60i-a733.dtsi b/arch/arm64/boot/dts/allwinner/sun60i-a733.dtsi
> new file mode 100644
> index 0000000000000..d0b9f432e3ab3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun60i-a733.dtsi
> @@ -0,0 +1,601 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> +
> +#include <dt-bindings/clock/sun6i-rtc.h>
> +#include <dt-bindings/clock/allwinner,sun60i-a733-ccu.h>
> +#include <dt-bindings/clock/allwinner,sun60i-a733-r-ccu.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/reset/allwinner,sun60i-a733-ccu.h>
> +#include <dt-bindings/reset/allwinner,sun60i-a733-r-ccu.h>

[Severity: High]
Are the A733 CCU and R-CCU dt-binding headers and schemas missing from the
tree?

None of dt-bindings/clock/allwinner,sun60i-a733-ccu.h,
dt-bindings/clock/allwinner,sun60i-a733-r-ccu.h,
dt-bindings/reset/allwinner,sun60i-a733-ccu.h, or
dt-bindings/reset/allwinner,sun60i-a733-r-ccu.h exist under
include/dt-bindings/, so preprocessing any DTB that includes
sun60i-a733.dtsi fails immediately and leaves all CCU and R-CCU clock and
reset macros undefined.  In addition, the allwinner,sun60i-a733-ccu and
allwinner,sun60i-a733-r-ccu compatibles used by ccu and r_ccu below have no
binding documentation or driver support.

[ ... ]

> +		pio: pinctrl@2000000 {
> +			compatible = "allwinner,sun60i-a733-pinctrl";
> +			reg = <0x02000000 0x800>;
> +			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;

[Severity: High]
Does omitting Port A's interrupt (GIC_SPI 67) shift all GPIO bank interrupts
on pio off by one bank?

Documentation/devicetree/bindings/pinctrl/allwinner,sun55i-a523-pinctrl.yaml
requires 11 interrupts for allwinner,sun60i-a733-pinctrl to cover Port A
through Port K.  In drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c,
prepare_function_table() assigns irq_bank = 1 to Port B through irq_bank = 10
to Port K (since Port A has 0 pins at bank 0), and sunxi_irq_base_reg()
indexes IRQ status/control registers by 0-based bank number:

drivers/pinctrl/sunxi/pinctrl-sunxi.h:sunxi_irq_base_reg() {
	if (pctl->flags & SUNXI_PINCTRL_NCAT3_REG_LAYOUT)
		return pctl->bank_offset + bank * pctl->bank_mem_size +
		       A733_IRQ_REGS_OFFSET;
    ...
}

Listing only 10 interrupts starting at Port B (GIC_SPI 69) maps GIC_SPI 69
to bank 0 (Port A) instead of bank 1 (Port B), so when any GPIO interrupt on
Port B through Port K fires, sunxi_pinctrl_irq_handler() reads the preceding
bank's status register and fails to clear or dispatch the interrupt.

[ ... ]

> +		uart0: serial@2500000 {
> +			compatible = "allwinner,sun60i-a733-uart",
> +				     "snps,dw-apb-uart";

[Severity: Low]
Is "allwinner,sun60i-a733-uart" documented in
Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml?

The binding schema does not list allwinner,sun60i-a733-uart, which causes
dtbs_check validation failures on uart0..uart6 and r_uart0..r_uart1 (other
Allwinner arm64 SoCs use "snps,dw-apb-uart" directly).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794227241.git.enzo.adriano.code@gmail.com?part=2