Re: [PATCH v3 2/2] ARM: dts: aspeed-g6: add pcie-kcs4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The `pcie_kcs4` device tree node is missing the `clocks` property.

Pre-existing issues:
- [High] The `kcs_bmc_aspeed` driver accesses hardware registers without requesting or enabling the required clock.
--

--- Patch [2]: [PATCH v3 2/2] ARM: dts: aspeed-g6: add pcie-kcs4 ---
commit 206a33dca85271dba16c0470168b764c592b52a0
Author: Grégoire Layet <gregoire.layet@9elements.com>

ARM: dts: aspeed-g6: add pcie-kcs4

This commit adds the pcie_kcs4 node to the LPC bus in the ast2600 g6
common dtsi. It enables a KCS-compatible interface exposed over PCIe
for IPMI communication with the host.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi

[ ... ]

> @@ -656,6 +656,14 @@ ibt: ibt@140 {
>  					clocks = <&syscon ASPEED_CLK_GATE_LCLK>;
>  					status = "disabled";
>  				};
> +
> +				pcie_kcs4: kcs@914 {
> +					compatible = "aspeed,ast2600-kcs-bmc";
> +					reg = <0x914 0x1>, <0x918 0x1>, <0x91c 0x1>;
> +					interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;

[Severity: Low]
Should this node include a clocks property? Neighboring KCS nodes include 
clocks = <&syscon ASPEED_CLK_GATE_LCLK>;. Leaving this out could result in 
an incomplete hardware description in the device tree.

[Severity: High]
This is a pre-existing issue, but does the kcs_bmc_aspeed driver need
to request and enable the required LCLK before accessing these registers?

When aspeed_kcs_probe() in drivers/char/ipmi/kcs_bmc_aspeed.c is called,
it issues regmap writes to the LPC controller registers via 
aspeed_kcs_set_address():

drivers/char/ipmi/kcs_bmc_aspeed.c:aspeed_kcs_probe() {
        ...
        rc = aspeed_kcs_set_address(kcs_bmc, addrs, nr_addrs);
        if (rc)
                return rc;

        /* Host to BMC IRQ */
        rc = aspeed_kcs_config_downstream_irq(kcs_bmc, pdev);
        ...
}

If the bootloader leaves the ASPEED_CLK_GATE_LCLK disabled, or if the clock
framework dynamically disables unused clocks, accessing these registers
without enabling the clock might cause a synchronous external abort or
system hang.

> +
> +					status = "disabled";
> +				};
>  			};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785135437.git.gregoire.layet@9elements.com?part=2