Re: [PATCH 8/9] arm64: dts: airoha: en7581: add crypto node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] arm64: dts: airoha: unbound UART compatibles break EN7581 serial console
- [Medium] arm64: dts: airoha: watchdog@1fbf0100 reg size 0x38 excludes WDT_RELOAD
- [Medium] arm64: dts: airoha: thermal@1efbd000 reg size 0xd5c excludes 0xf20..0xf28
- [Low] arm64: dts: airoha: undocumented airoha,en7581-chip-scu syscon compatible
--

commit 4273f644e034fd0b9c1514ce767c400e4a6f58a5
Author: Christian Marangi <ansuelsmth@gmail.com>

arm64: dts: airoha: en7581: add crypto node

This series reorders and extends the Airoha EN7581 device tree with CPU OPP,
Chip-SCU, thermal sensor, watchdog, PWM, UART/HSUART, and EIP93 crypto nodes.

> diff --git a/arch/arm64/boot/dts/airoha/en7581.dtsi b/arch/arm64/boot/dts/airoha/en7581.dtsi
> index ff6908a76e8eb..f214fa3a3e2bb 100644
> --- a/arch/arm64/boot/dts/airoha/en7581.dtsi
> +++ b/arch/arm64/boot/dts/airoha/en7581.dtsi

[ ... ]

> @@ -150,6 +357,15 @@ gic: interrupt-controller@9000000 {
>  			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
>  		};
>  
> +		thermal: thermal-sensor@1efbd000 {
> +			compatible = "airoha,en7581-thermal";
> +			reg = <0x0 0x1efbd000 0x0 0xd5c>;

[Severity: Medium]
Is the register region size of 0xd5c on thermal-sensor@1efbd000 (added in
commit 32580fdfac9a ("arm64: dts: airoha: en7581: add thermal sensor nodes"))
large enough for the registers accessed by the thermal driver?

During probe, en7581_thermal_probe() -> en7581_thermal_setup_adc_val() in
drivers/thermal/airoha_thermal.c reads calibration registers at offsets 0xf20
(EN7581_EFUSE_TEMP_OFFSET_REG) and 0xf28 (EN7581_EFUSE_TEMP_CPU_SENSOR_REG):

    regmap_read(priv->map, EN7581_EFUSE_TEMP_OFFSET_REG, &efuse_calib_info);
    if (efuse_calib_info) {
        priv->default_offset = FIELD_GET(EN7581_EFUSE_TEMP_OFFSET, efuse_calib_info);
        /* Different slope are applied if the sensor is used for CPU or for package */
        regmap_read(priv->map, EN7581_EFUSE_TEMP_CPU_SENSOR_REG, &cpu_sensor);

Since both offsets exceed 0xd5c, should the reg size be at least 0xf2c so
these reads stay within the declared resource bounds?

[ ... ]

> @@ -173,16 +389,9 @@ spi_nand: nand@0 {
>  			};
>  		};
>  
> -		scuclk: clock-controller@1fb00000 {
> -			compatible = "airoha,en7581-scu";
> -			reg = <0x0 0x1fb00000 0x0 0x970>;
> -			#clock-cells = <1>;
> -			#reset-cells = <1>;
> -		};
> -
> -		pbus_csr: syscon@1fbe3400 {
> -			compatible = "airoha,en7581-pbus-csr", "syscon";
> -			reg = <0x0 0x1fbe3400 0x0 0xff>;
> +		chip_scu: syscon@1fa20000 {
> +			compatible = "airoha,en7581-chip-scu", "syscon";
> +			reg = <0x0 0x1fa20000 0x0 0x388>;
>  		};

[Severity: Low]
Does chip_scu: syscon@1fa20000 (added in commit bb621c4a83bc ("arm64: dts:
airoha: en7581: add Chip-SCU accessor node")) pass dtbs_check?

In Documentation/devicetree/bindings/mfd/syscon.yaml, the compatible enum
includes "airoha,en7581-pbus-csr" and "econet,en751221-chip-scu", but
"airoha,en7581-chip-scu" is not listed in that schema or any other binding.
Should "airoha,en7581-chip-scu" be documented in the syscon binding schema?

[ ... ]

> +		serial@1fbe1000 {
> +			compatible = "airoha,an7581-hsuart";
> +			reg = <0x0 0x1fbe1000 0x0 0x40>;
> +			reg-io-width = <4>;
> +			reg-shift = <2>;
> +			interrupts = <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>;
> +			clock-frequency = <7372800>;
> +
> +			status = "disabled";
> +		};
> +
> +		pbus_csr: syscon@1fbe3400 {
> +			compatible = "airoha,en7581-pbus-csr", "syscon";
> +			reg = <0x0 0x1fbe3400 0x0 0xff>;
> +		};
> +
> +		uart1: serial@1fbf0000 {
> +			compatible = "airoha,en7523-uart";
> +			reg = <0x0 0x1fbf0000 0x0 0x30>;
> +			reg-io-width = <4>;
> +			reg-shift = <2>;
> +			interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> +			clock-frequency = <1843200>;
> +		};

[Severity: High]
Can the serial driver still bind to uart1 (serial@1fbf0000) or the newly
added serial@1fbe1000, serial@1fbf0300, serial@1fbf0600, and serial@1fbf0700
nodes after commit 6e4317a9ed57 ("arm64: dts: airoha: en7581: add nodes for
all supported uart and hsuart")?

Previously, uart1: serial@1fbf0000 used compatible = "ns16550", which matched
of_platform_serial_table in drivers/tty/serial/8250/8250_of.c:

    static const struct of_device_id of_platform_serial_table[] = {
        { .compatible = "ns8250",   .data = (void *)PORT_8250, },
        { .compatible = "ns16450",  .data = (void *)PORT_16450, },
        { .compatible = "ns16550a", .data = (void *)PORT_16550A, },
        { .compatible = "ns16550",  .data = (void *)PORT_16550, },

Replacing "ns16550" with "airoha,en7523-uart" (and using
"airoha,an7581-hsuart" on serial@1fbe1000) without a fallback compatible leaves
these nodes unmatched by any driver under drivers/tty/serial/ and undocumented
in Documentation/devicetree/bindings/serial/8250.yaml. On en7581-evb.dts,
where serial0 aliases uart1 for stdout-path, won't this regression break the
serial console on boot?

> +
> +		watchdog@1fbf0100 {
> +			compatible = "airoha,en7581-wdt";
> +			reg = <0x0 0x1fbf0100 0x0 0x38>;

[Severity: Medium]
Does the reg size of 0x38 on watchdog@1fbf0100 (added in commit 285873958d4b
("arm64: dts: airoha: en7581: add watchdog node")) exclude the WDT_RELOAD
register at offset 0x38?

In drivers/watchdog/airoha_wdt.c, WDT_RELOAD is defined at offset 0x38, and
airoha_wdt_ping() performs 32-bit accesses at 0x38..0x3b:

    val = readl(airoha_wdt->base + WDT_RELOAD);
    val |= WDT_RLD;
    writel(val, airoha_wdt->base + WDT_RELOAD);

The binding example in
Documentation/devicetree/bindings/watchdog/airoha,en7581-wdt.yaml also lists:

    watchdog@1fbf0100 {
        compatible = "airoha,en7581-wdt";
        reg = <0x1fbf0100 0x3c>;

Should the reg size in en7581.dtsi be 0x3c instead of 0x38?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026095727.31708-1-ansuelsmth@gmail.com?part=8