[PATCH v3 0/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe

[PATCH v3 0/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe

[Grégoire Layet]

The ASPEED AST2600 has a PCIe to LPC controller. It includes a KCS
interface on channel 4. This is a fully KCS-compatible interface
that is exposed over PCIe.

Add pcie_kcs4 to existing LPC bus in aspeed-g6.dtsi.
Modify kcs_bmc_aspeed driver to detect the PCIe over LPC channel.

In the driver, this PCIe over LPC channel will be detected as Channel 5.
In the datasheet this is defined as Channel 4 over PCIe.

The Channel 4 name is still referenced in the offsets macros.

The host need a driver to activate the MSI interrupts with the PCIe BMC
Device.

Tests:
With the patch [1] adding PCIe BMC device for Virtual UART, the MSI
interrupts are correctly setups. The Host driver is build and side
loaded on the host. The ipmi_si driver on the host is loaded manually
with the KCS address given by hand. 'ipmitool mc info' is run to confirm
it's working.

Tested on : Asus IPMI Expansion Card, AST2600.
```
$ insmod aspeed-host-bmc-dev.ko
$ lspci -vvv -d 1a03:2402
02:01.0 Unassigned class [ff00]: ASPEED Technology, Inc. Device 2402
	Subsystem: ASPEED Technology, Inc. Device 2402
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 154
	IOMMU group: 12
	Region 0: Memory at 82080000 (32-bit, non-prefetchable) [size=4K]
	Region 1: Memory at 82040000 (32-bit, non-prefetchable) [size=256K]
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [50] MSI: Enable+ Count=32/32 Maskable- 64bit+
		Address: 00000000fee00818  Data: 0000
	Kernel driver in use: aspeed-host-bmc-dev

$ modprobe ipmi_si type=kcs addrs=0x82040E88 regspacings=4 regsizes=1 regshifts=0

$ ipmitool mc info
Device ID                 : 32
Device Revision           : 1
Firmware Revision         : 26.04
IPMI Version              : 2.0
Manufacturer ID           : 2623
Manufacturer Name         : ASUSTek Computer Inc.
...
```

Changes since v2 [2]:
 - Put pcie_kcs4 node in ascending address order.
 - Add missing "case PCIE_LPC_ODR4" in aspeed_kcs_outb.

An RFC [3] is in discussion for implementing multiple LPC controller support
in kcs_bmc_aspeed. This would change the implementation of the pcie_lpc.
This series adds the pcie_kcs4 to have devices like the Asus IPMI Expansion Card
use it, before waiting to have full PCIe LPC support.

[1] https://lore.kernel.org/linux-aspeed/51e455417bdcfe608270c6cb2806b1fd971fb998.1783524645.git.gregoire.layet@9elements.com/
[2] https://lore.kernel.org/linux-aspeed/cover.1784732161.git.gregoire.layet@9elements.com/
[3] https://lkml.org/lkml/2026/7/23/429

Grégoire Layet (2):
  ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe
  ARM: dts: aspeed-g6: add pcie-kcs4

 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi |  8 +++++
 drivers/char/ipmi/kcs_bmc_aspeed.c      | 41 +++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 2 deletions(-)


base-commit: 03f906d8f5541e8bb741035981304feceed5993d
--
2.54.0

[PATCH v3 1/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe

[Grégoire Layet]

Created a 5th KCS channel to match against the KCS 4 over PCIe addresses.

The ASPEED AST2600 has a PCIe to LPC controller. It includes a KCS
interface on channel 4. This is a fully KCS-compatible interface
that is exposed over PCIe.

The 5th channel created is only valid on the AST2600.
This cannot be used for AST2400 and AST2500 chips, as they don't have LPC
over PCIE.

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/char/ipmi/kcs_bmc_aspeed.c | 41 ++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index a13a3470c17a..97daa2b3d6a0 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -26,7 +26,7 @@
 
 #define DEVICE_NAME     "ast-kcs-bmc"
 
-#define KCS_CHANNEL_MAX     4
+#define KCS_CHANNEL_MAX     5
 
 /*
  * Field class descriptions
@@ -102,6 +102,12 @@
 #define     LPC_LSADR12_LSADR2_SHIFT 16
 #define     LPC_LSADR12_LSADR1_MASK  GENMASK(15, 0)
 #define     LPC_LSADR12_LSADR1_SHIFT 0
+#define PCIE_LPC_HICRB       0x900
+#define PCIE_LPC_HICRC       0x904
+#define PCIE_LPC_LADR4       0x910
+#define PCIE_LPC_IDR4        0x914
+#define PCIE_LPC_ODR4        0x918
+#define PCIE_LPC_STR4        0x91C
 
 #define OBE_POLL_PERIOD	     (HZ / 2)
 
@@ -159,6 +165,7 @@ static void aspeed_kcs_outb(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 data)
 	case LPC_ODR2:
 	case LPC_ODR3:
 	case LPC_ODR4:
+	case PCIE_LPC_ODR4:
 		break;
 	default:
 		return;
@@ -191,6 +198,9 @@ static void aspeed_kcs_outb(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 data)
 	case 4:
 		regmap_update_bits(priv->map, LPC_HICRC, LPC_HICRC_IRQXE4, LPC_HICRC_IRQXE4);
 		break;
+	case 5:
+		regmap_update_bits(priv->map, PCIE_LPC_HICRC, LPC_HICRC_IRQXE4, LPC_HICRC_IRQXE4);
+		break;
 	default:
 		break;
 	}
@@ -278,6 +288,14 @@ static int aspeed_kcs_set_address(struct kcs_bmc_device *kcs_bmc, u32 addrs[2],
 
 		break;
 
+	case 5:
+		if (nr_addrs == 1)
+			regmap_write(priv->map, PCIE_LPC_LADR4, ((addrs[0] + 1) << 16) | addrs[0]);
+		else
+			regmap_write(priv->map, PCIE_LPC_LADR4, (addrs[1] << 16) | addrs[0]);
+
+		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -344,6 +362,11 @@ static int aspeed_kcs_config_upstream_irq(struct aspeed_kcs_bmc *priv, u32 id, u
 		val = (id << LPC_HICRC_ID4IRQX_SHIFT) | (hw_type << LPC_HICRC_TY4IRQX_SHIFT);
 		regmap_update_bits(priv->map, LPC_HICRC, mask, val);
 		break;
+	case 5:
+		mask = LPC_HICRC_ID4IRQX_MASK | LPC_HICRC_TY4IRQX_MASK | LPC_HICRC_OBF4_AUTO_CLR;
+		val = (id << LPC_HICRC_ID4IRQX_SHIFT) | (hw_type << LPC_HICRC_TY4IRQX_SHIFT);
+		regmap_update_bits(priv->map, PCIE_LPC_HICRC, mask, val);
+		break;
 	default:
 		dev_warn(priv->kcs_bmc.dev,
 			 "SerIRQ configuration not supported on KCS channel %d\n",
@@ -373,6 +396,10 @@ static void aspeed_kcs_enable_channel(struct kcs_bmc_device *kcs_bmc, bool enabl
 	case 4:
 		regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_LPC4E, enable * LPC_HICRB_LPC4E);
 		return;
+	case 5:
+		regmap_update_bits(priv->map, PCIE_LPC_HICRB, LPC_HICRB_LPC4E,
+				   enable * LPC_HICRB_LPC4E);
+		return;
 	default:
 		pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel);
 		return;
@@ -452,6 +479,10 @@ static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask,
 			regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_IBFIE4,
 					   enable * LPC_HICRB_IBFIE4);
 			return;
+		case 5:
+			regmap_update_bits(priv->map, PCIE_LPC_HICRB, LPC_HICRB_IBFIE4,
+					   enable * LPC_HICRB_IBFIE4);
+			return;
 		default:
 			pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel);
 			return;
@@ -492,6 +523,7 @@ static const struct kcs_ioreg ast_kcs_bmc_ioregs[KCS_CHANNEL_MAX] = {
 	{ .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
 	{ .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
 	{ .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
+	{ .idr = PCIE_LPC_IDR4, .odr = PCIE_LPC_ODR4, .str = PCIE_LPC_STR4 },
 };
 
 static int aspeed_kcs_of_get_channel(struct platform_device *pdev)
@@ -575,11 +607,16 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
 	if (channel < 0)
 		return channel;
 
+	np = pdev->dev.of_node;
+	if (channel == 5 && !of_device_is_compatible(np, "aspeed,ast2600-kcs-bmc")) {
+		dev_err(&pdev->dev, "LPC over PCIE KCS4 only supported on ast2600\n");
+		return -ENODEV;
+	}
+
 	nr_addrs = aspeed_kcs_of_get_io_address(pdev, addrs);
 	if (nr_addrs < 0)
 		return nr_addrs;
 
-	np = pdev->dev.of_node;
 	rc = of_property_read_u32_array(np, "aspeed,lpc-interrupts", upstream_irq, 2);
 	if (rc && rc != -EINVAL)
 		return -EINVAL;
-- 
2.54.0

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

[Grégoire Layet]

Add pcie_kcs4 node to the LPC bus in the ast2600 g6 common dtsi.

The ASPEED AST2600 has a PCIe to LPC controller. It includes a KCS
interface on channel 4. This is a fully KCS-compatible interface
that is exposed over PCIe.

This can be used by the host for IPMI when the PCIe BMC Device is
activated.

While the PCIe to LPC Controller provides KCS channels 1, 2,
3 and 4 over PCI, this patch only adds channel 4.
This needs little adjustment to the driver and provides PCIe BMC Devices,
like the Asus IPMI Expansion Card, the IPMI over PCIe feature.

The pcie_kcs4 uses existing "aspeed,ast2600-kcs-bmc" compatible as it's a
standard KCS interface.

Node placed according to the ascending address ordering.
'dt-check-style --mode strict' reports no issues on added lines, except
'line-length' issues.

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 56bb3b0444f7..c9d94c25d1ad 100644
--- 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>;
+
+					status = "disabled";
+				};
 			};

 			sdc: sdc@1e740000 {
--
2.54.0

Re: [PATCH v3 1/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe

[sashiko-bot]

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

Pre-existing issues:
- [High] Uninitialized spinlock access in interrupt handler.
- [High] Hardware channel remains enabled when `kcs_bmc_add_device()` fails.
- [Medium] OBE timer race condition allows infinite background polling.
- [Medium] Unchecked dereference of `pdev->dev.of_node`.
--

--- Patch [1]: [PATCH v3 1/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe ---
commit 99fe5c8c1ecb27a7e6072d1058a4963ff797b165
Author: Grégoire Layet <gregoire.layet@9elements.com>
ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe

This commit adds a 5th KCS channel for the ASPEED AST2600 to support
KCS 4 over PCIe. It configures the appropriate registers and restricts
usage of this 5th channel to compatible AST2600 controllers.

> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
> index a13a3470c17a4..97daa2b3d6a04 100644
> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c

[ ... ]

> @@ -452,6 +479,10 @@ static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask,
>  			regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_IBFIE4,
>  					   enable * LPC_HICRB_IBFIE4);
>  			return;
> +		case 5:
> +			regmap_update_bits(priv->map, PCIE_LPC_HICRB, LPC_HICRB_IBFIE4,
> +					   enable * LPC_HICRB_IBFIE4);
> +			return;
>  		default:
>  			pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel);
>  			return;

[Severity: Medium]
This is a pre-existing issue, but since we are updating channel 5 here, I
wanted to ask about the OBE timer logic earlier in this function.

When disabling the OBE event, aspeed_kcs_irq_mask_update() calls
timer_delete(&priv->obe.timer). Because timer_delete() is asynchronous, if a
concurrent timer handler (aspeed_kcs_check_obe) is executing on another CPU,
it might observe the hardware OBF status is still set and call mod_timer().

Could this result in the timer endlessly re-arming itself and polling the
hardware indefinitely in the background? Might timer_delete_sync() or a shared
shutdown flag be needed here to prevent that?

[ ... ]

> @@ -575,11 +607,16 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
>  	if (channel < 0)
>  		return channel;
>  
> +	np = pdev->dev.of_node;
> +	if (channel == 5 && !of_device_is_compatible(np, "aspeed,ast2600-kcs-bmc")) {
> +		dev_err(&pdev->dev, "LPC over PCIE KCS4 only supported on ast2600\n");
> +		return -ENODEV;
> +	}
> +
>  	nr_addrs = aspeed_kcs_of_get_io_address(pdev, addrs);
>  	if (nr_addrs < 0)
>  		return nr_addrs;
>  
> -	np = pdev->dev.of_node;

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed pdev->dev.of_node is
used here in aspeed_kcs_probe().

Earlier in aspeed_kcs_probe(), we have this assignment:

aspeed_kcs_probe() {
    ...
    np = pdev->dev.of_node->parent;
    ...

Is there a scenario where this driver could be manually bound via sysfs by a
root user without a valid Device Tree node? If that happens, would
dereferencing pdev->dev.of_node->parent cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but I have a question about the probe error
path further down in aspeed_kcs_probe().

During probe, the driver enables the hardware channel and then registers the
device:

aspeed_kcs_probe() {
    ...
    aspeed_kcs_enable_channel(kcs_bmc, true);

    rc = kcs_bmc_add_device(&priv->kcs_bmc);
    if (rc) {
        dev_warn(&pdev->dev, "Failed to register channel ...\n");
        return rc;
    }
    ...
}

If kcs_bmc_add_device() fails, the hardware remains active on the host bus.
If the device asserts an interrupt in this state, could it result in an
unhandled interrupt storm since the channel is never explicitly disabled on
this error path?

[Severity: High]
This isn't a bug introduced by this patch, but looking at the sequence above
in aspeed_kcs_probe(), there appears to be a window where the hardware is
fully enabled to assert IRQs before kcs_bmc_add_device() is called.

If an interrupt fires immediately after aspeed_kcs_enable_channel(), the
registered handler aspeed_kcs_irq() will trigger and call
kcs_bmc_handle_event(), which attempts to take kcs_bmc->lock.

However, kcs_bmc->lock is initialized inside kcs_bmc_add_device():

kcs_bmc_add_device() {
    ...
    spin_lock_init(&kcs_bmc->lock);
    ...
}

Does this mean the interrupt handler could operate on an uninitialized
spinlock if the hardware asserts an IRQ before kcs_bmc_add_device() finishes?

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

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

[sashiko-bot]

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