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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Upstream IRQs will not be triggered for the new pcie_kcs4 channel because PCIE_LPC_ODR4 is omitted from the ODR check in aspeed_kcs_outb().
--

--- Patch [2]: [PATCH v2 2/2] ARM: dts: aspeed-g6: add pcie-kcs4 ---
commit ad67c996a0b89123187ac23dc1115c5dc8b37472
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
device tree. It enables a KCS-compatible interface exposed over PCIe for
host IPMI communication.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 56bb3b0444f7e..a93fa4cf5d8ff 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -617,6 +617,13 @@ kcs4: kcs@114 {
>  					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>;
> +					status = "disabled";
> +				};

[Severity: High]
Will the upstream IRQs be triggered for this new pcie_kcs4 channel?

Looking at aspeed_kcs_outb() in drivers/char/ipmi/kcs_bmc_aspeed.c, it
verifies the write targets an ODR register using a switch statement, but
PCIE_LPC_ODR4 is omitted:

drivers/char/ipmi/kcs_bmc_aspeed.c:aspeed_kcs_outb() {
        ...
        switch (reg) {
        case LPC_ODR1:
        case LPC_ODR2:
        case LPC_ODR3:
        case LPC_ODR4:
                break;
        default:
                return;
        }
        ...
}

Since PCIE_LPC_ODR4 falls into the default case, it returns early and skips
the subsequent IRQ generation logic entirely when the BMC responds to an
IPMI request over this new channel. 

Does the driver need to be updated to handle PCIE_LPC_ODR4 before this
channel can function correctly?

> +
>  				lpc_ctrl: lpc-ctrl@80 {
>  					compatible = "aspeed,ast2600-lpc-ctrl";
>  					reg = <0x80 0x80>;

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