[PATCH v1 0/2] soc: aspeed: Add BMC and host driver for PCIe BMC device

[PATCH v1 0/2] soc: aspeed: Add BMC and host driver for PCIe BMC device

[Grégoire Layet]

This series upstreams the PCIe BMC device driver from the ASPEED kernel SDK (branch master-v6.18)  [1].
There are two drivers: a BMC-side driver and a host-side driver.
Together they enable host<->BMC communication via PCIe. 
This includes : shared memory, doorbell interrupt communication and two VUARTs.

This driver can be used on AST2600/AST2700 based PCIe extension cards, such as Asus IPMI Kommando Card.
The virtual UART support can be used for Serial over LAN.
The shared memory can be claimed by userspace programs.

Tested on :
BMC :
- Asus IPMI Kommando Card R1.01, AST2600 A3.
- OpenBMC
Host:
- Linux kernel v7.0.0

I tested VUART communication with interrupt.
No driver modifications were required.

Not tested on AST2700.

Provenance:

I did zero modifications to the ASPEED code, this is only an upstream patch.
The driver files match the ASPEED SDK tree at :
  drivers/soc/aspeed/aspeed-bmc-dev.c
    1815546a54f5f89bc9d3bd8f5658f0a573927509 [2]
    "soc: aspeed: remove MMBI implement in host bmc dev"

  drivers/soc/aspeed/aspeed-host-bmc-dev.c
    7217e3c872166d56389a97e1b81996f73a3e76d5 [3]
    "soc: aspeed: remove iounmap"

The driver code build cleanly against Linux 7.0 without any modification.
No fix were required. 
I built both the BMC side and Host side with KCFLAGS=-W and no warning apperead.
Original authors are credited via Signed-off-by in the individual patches.

checkpatch.pl --strict reports 1 warning for BMC side driver
and 2 warnings for host side driver. 
These are unchanged to keep zero diff against the ASPEED SDK tree.
I can clean them in a v2 if needed.

The host side driver had a submission from Ninad Palsule : 
"soc/aspeed: Add host side BMC device driver" in August 2023 [4].
This submission stalled and did not make it into the mainline kernel.
Since then, the host drivers have changed a lot on the ASPEED SDK. 
I decided not to base this patch on Ninad's one 
but rather to simply upstream the new host driver from ASPEED.

[1]: https://github.com/AspeedTech-BMC/linux/tree/aspeed-master-v6.18/drivers/soc/aspeed
[2]: https://github.com/AspeedTech-BMC/linux/commit/1815546a54f5f89bc9d3bd8f5658f0a573927509
[3]: https://github.com/AspeedTech-BMC/linux/commit/7217e3c872166d56389a97e1b81996f73a3e76d5
[4]: https://lore.kernel.org/linux-aspeed/20230821183525.3427144-1-ninad@linux.ibm.com/

Grégoire Layet (2):
  soc: aspeed: add BMC-side PCIe BMC device driver
  soc: aspeed: add host-side PCIe BMC device driver

 drivers/soc/aspeed/Kconfig               |  22 +
 drivers/soc/aspeed/Makefile              |   2 +
 drivers/soc/aspeed/aspeed-bmc-dev.c      | 701 +++++++++++++++++++++++
 drivers/soc/aspeed/aspeed-host-bmc-dev.c | 664 +++++++++++++++++++++
 4 files changed, 1389 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c
 create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c

-- 
2.51.2

[PATCH v1 1/2] soc: aspeed: add BMC-side PCIe BMC device driver

[Grégoire Layet]

Taken from ASPEED 6.18 Kernel SDK

Add support for PCIe communication BMC<->host.
This add BMC side driver.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/soc/aspeed/Kconfig          |   9 +
 drivers/soc/aspeed/Makefile         |   1 +
 drivers/soc/aspeed/aspeed-bmc-dev.c | 701 ++++++++++++++++++++++++++++
 3 files changed, 711 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c

diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index f579ee0b5afa..341728df07b3 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -4,6 +4,15 @@ if ARCH_ASPEED || COMPILE_TEST
 
 menu "ASPEED SoC drivers"
 
+config ASPEED_BMC_DEV
+	tristate "ASPEED BMC Device"
+	default n
+	help
+	  Enable support for the ASPEED AST2600/AST2700 BMC Device.
+	  This exposes the BMC to the host over PCIe,
+	  providing a shared-memory BAR, host-to-BMC and BMC-to-host
+	  message queues with doorbell interrupts and PCIe-to-LPC bridge.
+
 config ASPEED_LPC_CTRL
 	tristate "ASPEED LPC firmware cycle control"
 	select REGMAP
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..fab0d247df66 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_ASPEED_BMC_DEV)		+= aspeed-bmc-dev.o
 obj-$(CONFIG_ASPEED_LPC_CTRL)		+= aspeed-lpc-ctrl.o
 obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
diff --git a/drivers/soc/aspeed/aspeed-bmc-dev.c b/drivers/soc/aspeed/aspeed-bmc-dev.c
new file mode 100644
index 000000000000..06005fa41d2a
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-bmc-dev.c
@@ -0,0 +1,701 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) ASPEED Technology Inc.
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+#include <linux/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/dma-mapping.h>
+#include <linux/miscdevice.h>
+
+#define SCU_TRIGGER_MSI
+
+/* AST2600 SCU */
+#define ASPEED_SCU04			0x04
+#define AST2600A3_SCU04				0x05030303
+#define ASPEED_SCUC20			0xC20
+#define ASPEED_SCUC24			0xC24
+#define MSI_ROUTING_MASK			GENMASK(11, 10)
+#define PCIDEV1_INTX_MSI_HOST2BMC_EN		BIT(18)
+#define MSI_ROUTING_PCIe2LPC_PCIDEV0		(0x1 << 10)
+#define MSI_ROUTING_PCIe2LPC_PCIDEV1		(0x2 << 10)
+/* AST2700 SCU */
+#define SCU0_REVISION_ID		0x0
+#define REVISION_ID				GENMASK(23, 16)
+#define SCU0_PCIE_CONF_CTRL		0x970
+/* Host2BMC */
+#define ASPEED_BMC_MEM_BAR			0xF10
+#define  PCIE2PCI_MEM_BAR_ENABLE		BIT(1)
+#define  HOST2BMC_MEM_BAR_ENABLE		BIT(0)
+#define ASPEED_BMC_MEM_BAR_REMAP	0xF18
+
+#define ASPEED_BMC_SHADOW_CTRL		0xF50
+#define  READ_ONLY_MASK				BIT(31)
+#define  MASK_BAR1					BIT(2)
+#define  MASK_BAR0					BIT(1)
+#define  SHADOW_CFG					BIT(0)
+
+#define ASPEED_BMC_HOST2BMC_Q1		0xA000
+#define ASPEED_BMC_HOST2BMC_Q2		0xA010
+#define ASPEED_BMC_BMC2HOST_Q1		0xA020
+#define ASPEED_BMC_BMC2HOST_Q2		0xA030
+#define ASPEED_BMC_BMC2HOST_STS		0xA040
+#define	 BMC2HOST_INT_STS_DOORBELL		BIT(31)
+#define	 BMC2HOST_ENABLE_INTB			BIT(30)
+#define	 BMC2HOST_Q1_FULL				BIT(27)
+#define	 BMC2HOST_Q1_EMPTY				BIT(26)
+#define	 BMC2HOST_Q2_FULL				BIT(25)
+#define	 BMC2HOST_Q2_EMPTY				BIT(24)
+#define	 BMC2HOST_Q1_FULL_UNMASK		BIT(23)
+#define	 BMC2HOST_Q1_EMPTY_UNMASK		BIT(22)
+#define	 BMC2HOST_Q2_FULL_UNMASK		BIT(21)
+#define	 BMC2HOST_Q2_EMPTY_UNMASK		BIT(20)
+
+#define ASPEED_BMC_HOST2BMC_STS		0xA044
+#define	 HOST2BMC_INT_STS_DOORBELL		BIT(31)
+#define	 HOST2BMC_ENABLE_INTB			BIT(30)
+#define	 HOST2BMC_Q1_FULL				BIT(27)
+#define	 HOST2BMC_Q1_EMPTY				BIT(26)
+#define	 HOST2BMC_Q2_FULL				BIT(25)
+#define	 HOST2BMC_Q2_EMPTY				BIT(24)
+#define	 HOST2BMC_Q1_FULL_UNMASK		BIT(23)
+#define	 HOST2BMC_Q1_EMPTY_UNMASK		BIT(22)
+#define	 HOST2BMC_Q2_FULL_UNMASK		BIT(21)
+#define	 HOST2BMC_Q2_EMPTY_UNMASK		BIT(20)
+
+#define ASPEED_SCU_PCIE_CONF_CTRL	0xC20
+#define  SCU_PCIE_CONF_BMC_DEV_EN			 BIT(8)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_MMIO		 BIT(9)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_MSI		 BIT(11)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_IRQ		 BIT(13)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_DMA		 BIT(14)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_E2L		 BIT(15)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE BIT(21)
+
+#define ASPEED_SCU_BMC_DEV_CLASS	0xC68
+
+#define ASPEED_QUEUE_NUM 2
+enum queue_index {
+	QUEUE1 = 0,
+	QUEUE2,
+};
+
+struct aspeed_platform {
+	int (*init)(struct platform_device *pdev);
+	ssize_t (*queue_rx)(struct file *filp, struct kobject *kobj,
+			    const struct bin_attribute *attr, char *buf, loff_t off, size_t count);
+	ssize_t (*queue_tx)(struct file *filp, struct kobject *kobj,
+			    const struct bin_attribute *attr, char *buf, loff_t off, size_t count);
+};
+
+struct aspeed_queue_message {
+	/* Queue waiters for idle engine */
+	wait_queue_head_t tx_wait;
+	wait_queue_head_t rx_wait;
+	struct kernfs_node *kn;
+	struct bin_attribute bin;
+	int index;
+	struct aspeed_bmc_device *bmc_device;
+};
+
+struct aspeed_bmc_device {
+	unsigned char *host2bmc_base_virt;
+	struct device *dev;
+	struct miscdevice miscdev;
+	int id;
+	void __iomem *reg_base;
+	dma_addr_t bmc_mem_phy;
+	phys_addr_t bmc_mem_size;
+	void *bmc_mem_cpu;
+
+	int pcie2lpc;
+	int irq;
+
+	struct aspeed_queue_message queue[ASPEED_QUEUE_NUM];
+
+	const struct aspeed_platform *platform;
+
+	/* AST2700 */
+	struct regmap *device;
+	struct regmap *e2m;
+
+	struct regmap *scu;
+	int pcie_irq;
+};
+
+static struct aspeed_bmc_device *file_aspeed_bmc_device(struct file *file)
+{
+	return container_of(file->private_data, struct aspeed_bmc_device,
+			miscdev);
+}
+
+static int aspeed_bmc_device_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct aspeed_bmc_device *bmc_device = file_aspeed_bmc_device(file);
+	unsigned long vsize = vma->vm_end - vma->vm_start;
+
+	if (vsize > bmc_device->bmc_mem_size)
+		return -EINVAL;
+
+	return dma_mmap_coherent(bmc_device->dev, vma,
+				 bmc_device->bmc_mem_cpu,
+				 bmc_device->bmc_mem_phy,
+				 bmc_device->bmc_mem_size);
+
+}
+
+static const struct file_operations aspeed_bmc_device_fops = {
+	.owner		= THIS_MODULE,
+	.mmap		= aspeed_bmc_device_mmap,
+};
+
+static ssize_t aspeed_ast2600_queue_rx(struct file *filp, struct kobject *kobj,
+				       const struct bin_attribute *attr, char *buf, loff_t off,
+				       size_t count)
+{
+	struct aspeed_queue_message *queue = attr->private;
+	struct aspeed_bmc_device *bmc_device = queue->bmc_device;
+	int index = queue->index;
+	u32 *data = (u32 *)buf;
+	u32 scu_id;
+	int ret;
+
+	ret = wait_event_interruptible(queue->rx_wait,
+				       !(readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS) &
+				       ((index == QUEUE1) ? HOST2BMC_Q1_EMPTY : HOST2BMC_Q2_EMPTY)));
+	if (ret)
+		return -EINTR;
+
+	data[0] = readl(bmc_device->reg_base +
+			((index == QUEUE1) ? ASPEED_BMC_HOST2BMC_Q1 : ASPEED_BMC_HOST2BMC_Q2));
+
+	regmap_read(bmc_device->scu, ASPEED_SCU04, &scu_id);
+	if (scu_id == AST2600A3_SCU04) {
+		writel(BMC2HOST_INT_STS_DOORBELL | BMC2HOST_ENABLE_INTB,
+		       bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS);
+	} else {
+		//A0 : BIT(12) A1 : BIT(15)
+		regmap_update_bits(bmc_device->scu, 0x560, BIT(15), BIT(15));
+		regmap_update_bits(bmc_device->scu, 0x560, BIT(15), 0);
+	}
+
+	return sizeof(u32);
+}
+
+static ssize_t aspeed_ast2600_queue_tx(struct file *filp, struct kobject *kobj,
+				       const struct bin_attribute *attr, char *buf, loff_t off,
+				       size_t count)
+{
+	struct aspeed_queue_message *queue = attr->private;
+	struct aspeed_bmc_device *bmc_device = queue->bmc_device;
+	int index = queue->index;
+	u32 tx_buff;
+	u32 scu_id;
+	int ret;
+
+	if (count != sizeof(u32))
+		return -EINVAL;
+
+	ret = wait_event_interruptible(queue->tx_wait,
+				       !(readl(bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS) &
+				       ((index == QUEUE1) ? BMC2HOST_Q1_FULL : BMC2HOST_Q2_FULL)));
+	if (ret)
+		return -EINTR;
+
+	memcpy(&tx_buff, buf, 4);
+	writel(tx_buff, bmc_device->reg_base + ((index == QUEUE1) ? ASPEED_BMC_BMC2HOST_Q1 :
+								    ASPEED_BMC_BMC2HOST_Q2));
+
+	/* trigger to host
+	 * Only After AST2600A3 support DoorBell MSI
+	 */
+	regmap_read(bmc_device->scu, ASPEED_SCU04, &scu_id);
+	if (scu_id == AST2600A3_SCU04) {
+		writel(BMC2HOST_INT_STS_DOORBELL | BMC2HOST_ENABLE_INTB,
+		       bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS);
+	} else {
+		//A0 : BIT(12) A1 : BIT(15)
+		regmap_update_bits(bmc_device->scu, 0x560, BIT(15), BIT(15));
+		regmap_update_bits(bmc_device->scu, 0x560, BIT(15), 0);
+	}
+
+	return sizeof(u32);
+}
+
+static ssize_t aspeed_ast2700_queue_rx(struct file *filp, struct kobject *kobj,
+				       const struct bin_attribute *attr, char *buf, loff_t off,
+				       size_t count)
+{
+	struct aspeed_queue_message *queue = attr->private;
+	struct aspeed_bmc_device *bmc_device = queue->bmc_device;
+	int index = queue->index;
+	u32 *data = (u32 *)buf;
+	int ret;
+
+	ret = wait_event_interruptible(queue->rx_wait,
+				       !(readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS) &
+				       ((index == QUEUE1) ? HOST2BMC_Q1_EMPTY : HOST2BMC_Q2_EMPTY)));
+	if (ret)
+		return -EINTR;
+
+	data[0] = readl(bmc_device->reg_base +
+			((index == QUEUE1) ? ASPEED_BMC_HOST2BMC_Q1 : ASPEED_BMC_HOST2BMC_Q2));
+
+	writel(BMC2HOST_INT_STS_DOORBELL | BMC2HOST_ENABLE_INTB,
+	       bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS);
+
+	return sizeof(u32);
+}
+
+static ssize_t aspeed_ast2700_queue_tx(struct file *filp, struct kobject *kobj,
+				       const struct bin_attribute *attr, char *buf, loff_t off,
+				       size_t count)
+{
+	struct aspeed_queue_message *queue = attr->private;
+	struct aspeed_bmc_device *bmc_device = queue->bmc_device;
+	int index = queue->index;
+	u32 tx_buff;
+	int ret;
+
+	if (count != sizeof(u32))
+		return -EINVAL;
+
+	ret = wait_event_interruptible(queue->tx_wait,
+				       !(readl(bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS) &
+				       ((index == QUEUE1) ? BMC2HOST_Q1_FULL : BMC2HOST_Q2_FULL)));
+	if (ret)
+		return -EINTR;
+
+	memcpy(&tx_buff, buf, 4);
+	writel(tx_buff, bmc_device->reg_base + ((index == QUEUE1) ? ASPEED_BMC_BMC2HOST_Q1 :
+								    ASPEED_BMC_BMC2HOST_Q2));
+
+	writel(BMC2HOST_INT_STS_DOORBELL | BMC2HOST_ENABLE_INTB,
+	       bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS);
+
+	return sizeof(u32);
+}
+
+/* AST2600 */
+static irqreturn_t aspeed_bmc_dev_pcie_isr(int irq, void *dev_id)
+{
+	struct aspeed_bmc_device *bmc_device = dev_id;
+
+	while (!(readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS) & HOST2BMC_Q1_EMPTY))
+		readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_Q1);
+
+	while (!(readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS) & HOST2BMC_Q2_EMPTY))
+		readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_Q2);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t aspeed_bmc_dev_isr(int irq, void *dev_id)
+{
+	struct aspeed_bmc_device *bmc_device = dev_id;
+	u32 host2bmc_q_sts = readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS);
+
+	if (host2bmc_q_sts & HOST2BMC_INT_STS_DOORBELL)
+		writel(HOST2BMC_INT_STS_DOORBELL, bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS);
+
+	if (host2bmc_q_sts & HOST2BMC_ENABLE_INTB)
+		writel(HOST2BMC_ENABLE_INTB, bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS);
+
+	if (host2bmc_q_sts & HOST2BMC_Q1_FULL)
+		dev_info(bmc_device->dev, "Q1 Full\n");
+
+	if (host2bmc_q_sts & HOST2BMC_Q2_FULL)
+		dev_info(bmc_device->dev, "Q2 Full\n");
+
+	if (!(readl(bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS) & BMC2HOST_Q1_FULL))
+		wake_up_interruptible(&bmc_device->queue[QUEUE1].tx_wait);
+
+	if (!(readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS) & HOST2BMC_Q1_EMPTY))
+		wake_up_interruptible(&bmc_device->queue[QUEUE1].rx_wait);
+
+	if (!(readl(bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS) & BMC2HOST_Q2_FULL))
+		wake_up_interruptible(&bmc_device->queue[QUEUE2].tx_wait);
+
+	if (!(readl(bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS) & HOST2BMC_Q2_EMPTY))
+		wake_up_interruptible(&bmc_device->queue[QUEUE2].rx_wait);
+
+	return IRQ_HANDLED;
+}
+
+static int aspeed_ast2600_init(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
+			      SCU_PCIE_CONF_BMC_DEV_EN_MMIO | SCU_PCIE_CONF_BMC_DEV_EN;
+	u32 scu_id;
+
+	bmc_device->scu = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,scu");
+	if (IS_ERR(bmc_device->scu)) {
+		dev_err(&pdev->dev, "failed to find SCU regmap\n");
+		return PTR_ERR(bmc_device->scu);
+	}
+
+	if (bmc_device->pcie2lpc)
+		pcie_config_ctl |= SCU_PCIE_CONF_BMC_DEV_EN_E2L |
+				   SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE;
+
+	regmap_update_bits(bmc_device->scu, ASPEED_SCU_PCIE_CONF_CTRL,
+			   pcie_config_ctl, pcie_config_ctl);
+
+	/* update class code to others as it is a MFD device */
+	regmap_write(bmc_device->scu, ASPEED_SCU_BMC_DEV_CLASS, 0xff000000);
+
+#ifdef SCU_TRIGGER_MSI
+	//SCUC24[17]: Enable PCI device 1 INTx/MSI from SCU560[15]. Will be added in next version
+	regmap_update_bits(bmc_device->scu, ASPEED_SCUC20, BIT(11) | BIT(14), BIT(11) | BIT(14));
+
+	regmap_read(bmc_device->scu, ASPEED_SCU04, &scu_id);
+	if (scu_id == AST2600A3_SCU04)
+		regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
+				   PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_MASK,
+				   PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_PCIe2LPC_PCIDEV1);
+	else
+		regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
+				   BIT(17) | BIT(14) | BIT(11), BIT(17) | BIT(14) | BIT(11));
+#else
+	//SCUC24[18]: Enable PCI device 1 INTx/MSI from Host-to-BMC controller.
+	regmap_update_bits(bmc_device->scu, 0xc24, BIT(18) | BIT(14), BIT(18) | BIT(14));
+#endif
+
+	writel((~(bmc_device->bmc_mem_size - 1) & 0xFFFFFFFF) | HOST2BMC_MEM_BAR_ENABLE,
+	       bmc_device->reg_base + ASPEED_BMC_MEM_BAR);
+	writel(bmc_device->bmc_mem_phy, bmc_device->reg_base + ASPEED_BMC_MEM_BAR_REMAP);
+
+	//Setting BMC to Host Q register
+	writel(BMC2HOST_Q2_FULL_UNMASK | BMC2HOST_Q1_FULL_UNMASK | BMC2HOST_ENABLE_INTB,
+	       bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS);
+	writel(HOST2BMC_Q2_FULL_UNMASK |  HOST2BMC_Q1_FULL_UNMASK | HOST2BMC_ENABLE_INTB,
+	       bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS);
+
+	return 0;
+}
+
+static int aspeed_ast2700_init(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	u32 pcie_config_ctl;
+	u32 scu_id;
+	int i;
+
+	bmc_device->device = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,device");
+	if (IS_ERR(bmc_device->device)) {
+		dev_err(&pdev->dev, "failed to find device regmap\n");
+		return PTR_ERR(bmc_device->device);
+	}
+
+	bmc_device->e2m = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,e2m");
+	if (IS_ERR(bmc_device->e2m)) {
+		dev_err(&pdev->dev, "failed to find e2m regmap\n");
+		return PTR_ERR(bmc_device->e2m);
+	}
+
+	bmc_device->scu = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,scu");
+	if (IS_ERR(bmc_device->scu)) {
+		dev_err(&pdev->dev, "failed to find SCU regmap\n");
+		return PTR_ERR(bmc_device->scu);
+	}
+
+	if (bmc_device->pcie2lpc) {
+		pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_E2L |
+				  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE;
+		regmap_update_bits(bmc_device->scu, SCU0_PCIE_CONF_CTRL,
+				   pcie_config_ctl, pcie_config_ctl);
+	}
+
+	/* update class code to others as it is a MFD device */
+	regmap_write(bmc_device->device, 0x18, 0xff000027);
+
+	/* MSI */
+	regmap_update_bits(bmc_device->device, 0x74, GENMASK(7, 4), BIT(7) | (5 << 4));
+	/* EnPCIaMSI:BIT(25), EnPCIaIntA:BIT(17), EnPCIaMst:BIT(9), EnPCIaDev:BIT(1) */
+	regmap_read(bmc_device->scu, SCU0_REVISION_ID, &scu_id);
+	if (scu_id & REVISION_ID)
+		regmap_update_bits(bmc_device->device, 0x70,
+				   BIT(25) | BIT(17) | BIT(9) | BIT(1),
+				   BIT(25) | BIT(17) | BIT(9) | BIT(1));
+	else
+		/* Disable MSI[bit25] in ast2700A0 int only */
+		regmap_update_bits(bmc_device->device, 0x70,
+				   BIT(17) | BIT(9) | BIT(1),
+				   BIT(25) | BIT(17) | BIT(9) | BIT(1));
+
+	/* bar size check for 4k align */
+	for (i = 1; i < 16; i++) {
+		if ((bmc_device->bmc_mem_size / 4096) == (1 << (i - 1)))
+			break;
+	}
+	if (i == 16) {
+		i = 0;
+		dev_warn(bmc_device->dev,
+			 "Bar size not align for 4K : %dK\n", (u32)bmc_device->bmc_mem_size / 1024);
+	}
+
+	/*
+	 * BAR assign in scu
+	 * ((bar_mem / 4k) << 8) | per_size
+	 */
+	regmap_write(bmc_device->device, 0x1c, ((bmc_device->bmc_mem_phy) >> 4) | i);
+
+	if (bmc_device->id == 0)
+		/* Node 0 Bar 0 */
+		regmap_write(bmc_device->e2m, 0x108, ((bmc_device->bmc_mem_phy) >> 4) | i);
+	else
+		/* Node 1 Bar 0 */
+		regmap_write(bmc_device->e2m, 0x128, ((bmc_device->bmc_mem_phy) >> 4) | i);
+
+	/* Setting BMC to Host Q register */
+	writel(BMC2HOST_Q2_FULL_UNMASK | BMC2HOST_Q1_FULL_UNMASK | BMC2HOST_ENABLE_INTB,
+	       bmc_device->reg_base + ASPEED_BMC_BMC2HOST_STS);
+	writel(HOST2BMC_Q2_FULL_UNMASK | HOST2BMC_Q1_FULL_UNMASK | HOST2BMC_ENABLE_INTB,
+	       bmc_device->reg_base + ASPEED_BMC_HOST2BMC_STS);
+
+	return 0;
+}
+
+static int aspeed_bmc_device_setup_queue(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int ret, i;
+
+	for (i = 0; i < ASPEED_QUEUE_NUM; i++) {
+		struct aspeed_queue_message *queue = &bmc_device->queue[i];
+
+		init_waitqueue_head(&queue->tx_wait);
+		init_waitqueue_head(&queue->rx_wait);
+
+		sysfs_bin_attr_init(&queue->bin);
+
+		/* Queue name index starts from 1 */
+		queue->bin.attr.name =
+			devm_kasprintf(dev, GFP_KERNEL, "bmc-dev-queue%d", (i + 1));
+		queue->bin.attr.mode = 0600;
+		queue->bin.read = bmc_device->platform->queue_rx;
+		queue->bin.write = bmc_device->platform->queue_tx;
+		queue->bin.size = 4;
+		queue->bin.private = queue;
+
+		ret = sysfs_create_bin_file(&pdev->dev.kobj, &queue->bin);
+		if (ret) {
+			dev_err(dev, "error for bin%d file\n", i);
+			return ret;
+		}
+
+		queue->kn = kernfs_find_and_get(dev->kobj.sd, queue->bin.attr.name);
+		if (!queue->kn) {
+			sysfs_remove_bin_file(&dev->kobj, &queue->bin);
+			return ret;
+		}
+
+		queue->index = i;
+		queue->bmc_device = bmc_device;
+	}
+
+	return 0;
+}
+
+static int aspeed_bmc_device_setup_memory_mapping(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	bmc_device->miscdev.minor = MISC_DYNAMIC_MINOR;
+	bmc_device->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "bmc-device%d", bmc_device->id);
+	bmc_device->miscdev.fops = &aspeed_bmc_device_fops;
+	bmc_device->miscdev.parent = dev;
+	ret = misc_register(&bmc_device->miscdev);
+	if (ret) {
+		dev_err(dev, "Unable to register device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct aspeed_platform ast2600_plaform = {
+	.init = aspeed_ast2600_init,
+	.queue_rx = aspeed_ast2600_queue_rx,
+	.queue_tx = aspeed_ast2600_queue_tx
+};
+
+static struct aspeed_platform ast2700_plaform = {
+	.init = aspeed_ast2700_init,
+	.queue_rx = aspeed_ast2700_queue_rx,
+	.queue_tx = aspeed_ast2700_queue_tx
+};
+
+static const struct of_device_id aspeed_bmc_device_of_matches[] = {
+	{ .compatible = "aspeed,ast2600-bmc-device", .data = &ast2600_plaform },
+	{ .compatible = "aspeed,ast2700-bmc-device", .data = &ast2700_plaform },
+	{},
+};
+MODULE_DEVICE_TABLE(of, aspeed_bmc_device_of_matches);
+
+static int aspeed_bmc_device_probe(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device;
+	struct device *dev = &pdev->dev;
+	struct resource res;
+	const void *md = of_device_get_match_data(dev);
+	struct device_node *np;
+	int ret = 0, i;
+
+	if (!md)
+		return -ENODEV;
+
+	bmc_device = devm_kzalloc(&pdev->dev, sizeof(struct aspeed_bmc_device), GFP_KERNEL);
+	if (!bmc_device)
+		return -ENOMEM;
+	dev_set_drvdata(dev, bmc_device);
+
+	bmc_device->platform = md;
+
+	bmc_device->id = of_alias_get_id(dev->of_node, "bmcdev");
+	if (bmc_device->id < 0)
+		bmc_device->id = 0;
+
+	bmc_device->dev = dev;
+	bmc_device->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(bmc_device->reg_base))
+		return PTR_ERR(bmc_device->reg_base);
+
+	ret = of_reserved_mem_device_init(dev);
+	if (ret) {
+		dev_err(dev, "of_reserved_mem_device_init failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+	if (ret) {
+		dev_err(dev, "cannot set 64-bits DMA mask\n");
+		return ret;
+	}
+
+	np = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (!np || of_address_to_resource(np, 0, &res)) {
+		dev_err(dev, "Failed to find memory-region.\n");
+		return -ENOMEM;
+	}
+
+	of_node_put(np);
+
+	bmc_device->bmc_mem_size = resource_size(&res);
+	bmc_device->bmc_mem_cpu = dmam_alloc_coherent(dev, bmc_device->bmc_mem_size,
+						      &bmc_device->bmc_mem_phy, GFP_KERNEL);
+	if (!bmc_device->bmc_mem_cpu) {
+		dev_err(dev, "Failed to allocate BMC memory.\n");
+		return -ENOMEM;
+	}
+
+	bmc_device->irq = platform_get_irq(pdev, 0);
+	if (bmc_device->irq < 0) {
+		dev_err(&pdev->dev, "platform get of irq[=%d] failed!\n", bmc_device->irq);
+		return bmc_device->irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, bmc_device->irq, aspeed_bmc_dev_isr, 0,
+			       dev_name(&pdev->dev), bmc_device);
+	if (ret) {
+		dev_err(dev, "aspeed bmc device Unable to get IRQ");
+		return ret;
+	}
+
+	ret = aspeed_bmc_device_setup_queue(pdev);
+	if (ret) {
+		dev_err(dev, "Cannot setup queue message");
+		goto out;
+	}
+
+	ret = aspeed_bmc_device_setup_memory_mapping(pdev);
+	if (ret) {
+		dev_err(dev, "Cannot setup memory mapping misc");
+		goto out_free_queue;
+	}
+
+	if (of_property_read_bool(dev->of_node, "pcie2lpc"))
+		bmc_device->pcie2lpc = 1;
+
+	ret = bmc_device->platform->init(pdev);
+	if (ret) {
+		dev_err(dev, "Initialize bmc device failed\n");
+		goto out_free_misc;
+	}
+
+	bmc_device->pcie_irq =  platform_get_irq(pdev, 1);
+	if (bmc_device->pcie_irq < 0) {
+		dev_warn(&pdev->dev,
+			 "platform get of pcie irq[=%d] failed!\n", bmc_device->pcie_irq);
+	} else {
+		ret = devm_request_irq(&pdev->dev, bmc_device->pcie_irq,
+				       aspeed_bmc_dev_pcie_isr, IRQF_SHARED,
+				       dev_name(&pdev->dev), bmc_device);
+		if (ret < 0) {
+			dev_warn(dev, "Failed to request PCI-E IRQ %d.\n", ret);
+			bmc_device->pcie_irq = -1;
+		}
+	}
+
+	dev_info(dev, "aspeed bmc device: driver successfully loaded.\n");
+
+	return 0;
+
+out_free_misc:
+	misc_deregister(&bmc_device->miscdev);
+out_free_queue:
+	for (i = 0; i < ASPEED_QUEUE_NUM; i++)
+		sysfs_remove_bin_file(&pdev->dev.kobj, &bmc_device->queue[i].bin);
+out:
+	dev_warn(dev, "aspeed bmc device: driver init failed (ret=%d)!\n", ret);
+	return ret;
+}
+
+static void aspeed_bmc_device_remove(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < ASPEED_QUEUE_NUM; i++)
+		sysfs_remove_bin_file(&pdev->dev.kobj, &bmc_device->queue[i].bin);
+	misc_deregister(&bmc_device->miscdev);
+	devm_free_irq(&pdev->dev, bmc_device->irq, bmc_device);
+	devm_free_irq(&pdev->dev, bmc_device->pcie_irq, bmc_device);
+
+	devm_kfree(&pdev->dev, bmc_device);
+}
+
+static struct platform_driver aspeed_bmc_device_driver = {
+	.probe		= aspeed_bmc_device_probe,
+	.remove		= aspeed_bmc_device_remove,
+	.driver		= {
+		.name	= KBUILD_MODNAME,
+		.of_match_table = aspeed_bmc_device_of_matches,
+	},
+};
+
+module_platform_driver(aspeed_bmc_device_driver);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED BMC DEVICE Driver");
+MODULE_LICENSE("GPL");
\ No newline at end of file
-- 
2.51.2

[PATCH v1 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Grégoire Layet]

Taken from ASPEED 6.18 Kernel SDK

Add support for PCIe communication BMC<->host.
This add Host side driver.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/soc/aspeed/Kconfig               |  13 +
 drivers/soc/aspeed/Makefile              |   1 +
 drivers/soc/aspeed/aspeed-host-bmc-dev.c | 664 +++++++++++++++++++++++
 3 files changed, 678 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c

diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index 341728df07b3..e8670dfef1e6 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -13,6 +13,19 @@ config ASPEED_BMC_DEV
 	  providing a shared-memory BAR, host-to-BMC and BMC-to-host
 	  message queues with doorbell interrupts and PCIe-to-LPC bridge.
 
+config ASPEED_HOST_BMC_DEV
+	tristate "ASPEED Host BMC Device"
+	depends on PCI
+	depends on SERIAL_8250
+	help
+	  Enable support for the ASPEED AST2600/AST2700 BMC Device on the Host.
+	  This configure the PCIe and setup:
+	  - Two 8250 compatible VUART ports.
+	  - A character device exposing the BMC's shared memory
+	    region for host<->BMC data exchange.
+	  - A mailbox interrupt path and BMC message queue handler for
+		doorbell-style host<->BMC signaling.
+
 config ASPEED_LPC_CTRL
 	tristate "ASPEED LPC firmware cycle control"
 	select REGMAP
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index fab0d247df66..3fd3f6d8d36e 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_ASPEED_BMC_DEV)		+= aspeed-bmc-dev.o
+obj-$(CONFIG_ASPEED_HOST_BMC_DEV)	+= aspeed-host-bmc-dev.o
 obj-$(CONFIG_ASPEED_LPC_CTRL)		+= aspeed-lpc-ctrl.o
 obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
new file mode 100644
index 000000000000..9e6f1d39f18a
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
@@ -0,0 +1,664 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) ASPEED Technology Inc.
+
+#include <linux/init.h>
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/pci.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/interrupt.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/serial_core.h>
+#include <linux/serial_8250.h>
+#include <linux/poll.h>
+#include <linux/bitfield.h>
+
+#include <linux/if_arp.h>
+#include <linux/skbuff.h>
+#include <linux/mctp.h>
+#include <net/mctp.h>
+#include <net/pkt_sched.h>
+
+#define PCI_BMC_HOST2BMC_Q1 0x30000
+#define PCI_BMC_HOST2BMC_Q2 0x30010
+#define PCI_BMC_BMC2HOST_Q1 0x30020
+#define PCI_BMC_BMC2HOST_Q2 0x30030
+#define PCI_BMC_BMC2HOST_STS 0x30040
+#define BMC2HOST_INT_STS_DOORBELL BIT(31)
+#define BMC2HOST_ENABLE_INTB BIT(30)
+
+#define BMC2HOST_Q1_FULL BIT(27)
+#define BMC2HOST_Q1_EMPTY BIT(26)
+#define BMC2HOST_Q2_FULL BIT(25)
+#define BMC2HOST_Q2_EMPTY BIT(24)
+#define BMC2HOST_Q1_FULL_UNMASK BIT(23)
+#define BMC2HOST_Q1_EMPTY_UNMASK BIT(22)
+#define BMC2HOST_Q2_FULL_UNMASK BIT(21)
+#define BMC2HOST_Q2_EMPTY_UNMASK BIT(20)
+
+#define PCI_BMC_HOST2BMC_STS 0x30044
+#define HOST2BMC_INT_STS_DOORBELL BIT(31)
+#define HOST2BMC_ENABLE_INTB BIT(30)
+
+#define HOST2BMC_Q1_FULL BIT(27)
+#define HOST2BMC_Q1_EMPTY BIT(26)
+#define HOST2BMC_Q2_FULL BIT(25)
+#define HOST2BMC_Q2_EMPTY BIT(24)
+#define HOST2BMC_Q1_FULL_UNMASK BIT(23)
+#define HOST2BMC_Q1_EMPTY_UNMASK BIT(22)
+#define HOST2BMC_Q2_FULL_UNMASK BIT(21)
+#define HOST2BMC_Q2_EMPTY_UNMASK BIT(20)
+
+static DEFINE_IDA(bmc_device_ida);
+
+#define VUART_MAX_PARMS 2
+#define ASPEED_QUEUE_NUM 2
+#define MAX_MSI_NUM 8
+
+enum aspeed_platform_id {
+	ASPEED,
+};
+
+enum queue_index {
+	QUEUE1 = 0,
+	QUEUE2,
+};
+
+enum msi_index {
+	BMC_MSI,
+	MBX_MSI,
+	VUART0_MSI,
+	VUART1_MSI,
+};
+
+/* Match msi_index */
+static int ast2600_msi_idx_table[MAX_MSI_NUM] = { 4, 21, 16, 15 };
+static int ast2700_soc0_msi_idx_table[MAX_MSI_NUM] = { 0, 11, 6, 5 };
+
+struct aspeed_platform {
+	int (*setup)(struct pci_dev *pdev);
+};
+
+struct aspeed_queue_message {
+	/* Queue waiters for idle engine */
+	wait_queue_head_t tx_wait;
+	wait_queue_head_t rx_wait;
+	struct kernfs_node *kn;
+	struct bin_attribute bin;
+	int index;
+	struct aspeed_pci_bmc_dev *pci_bmc_device;
+};
+
+struct aspeed_pci_bmc_dev {
+	struct device *dev;
+	struct miscdevice miscdev;
+	struct aspeed_platform *platform;
+	kernel_ulong_t driver_data;
+	int id;
+
+	unsigned long mem_bar_base;
+	unsigned long mem_bar_size;
+	void __iomem *mem_bar_reg;
+
+	unsigned long message_bar_base;
+	unsigned long message_bar_size;
+	void __iomem *msg_bar_reg;
+
+	void __iomem *pcie_sio_decode_addr;
+
+	struct aspeed_queue_message queue[ASPEED_QUEUE_NUM];
+
+	void __iomem *sio_mbox_reg;
+	struct uart_8250_port uart[VUART_MAX_PARMS];
+	int uart_line[VUART_MAX_PARMS];
+
+	/* Interrupt
+	 * The index of array is using to enum msi_index
+	 */
+	int *msi_idx_table;
+};
+
+#define PCIE_DEVICE_SIO_ADDR (0x2E * 4)
+#define BMC_MULTI_MSI 32
+
+#define DRIVER_NAME "aspeed-host-bmc-dev"
+
+static struct aspeed_pci_bmc_dev *file_aspeed_bmc_device(struct file *file)
+{
+	return container_of(file->private_data, struct aspeed_pci_bmc_dev,
+			    miscdev);
+}
+
+static int aspeed_pci_bmc_dev_mmap(struct file *file,
+				   struct vm_area_struct *vma)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = file_aspeed_bmc_device(file);
+	unsigned long vsize = vma->vm_end - vma->vm_start;
+	pgprot_t prot = vma->vm_page_prot;
+
+	if (vma->vm_pgoff + vsize > pci_bmc_dev->mem_bar_base + 0x100000)
+		return -EINVAL;
+
+	prot = pgprot_noncached(prot);
+
+	if (remap_pfn_range(vma, vma->vm_start,
+			    (pci_bmc_dev->mem_bar_base >> PAGE_SHIFT) +
+				    vma->vm_pgoff,
+			    vsize, prot))
+		return -EAGAIN;
+
+	return 0;
+}
+
+static const struct file_operations aspeed_pci_bmc_dev_fops = {
+	.owner = THIS_MODULE,
+	.mmap = aspeed_pci_bmc_dev_mmap,
+};
+
+static ssize_t aspeed_queue_rx(struct file *filp, struct kobject *kobj,
+			       const struct bin_attribute *attr, char *buf,
+			       loff_t off, size_t count)
+{
+	struct aspeed_queue_message *queue = attr->private;
+	struct aspeed_pci_bmc_dev *pci_bmc_device = queue->pci_bmc_device;
+	int index = queue->index;
+	u32 *data = (u32 *)buf;
+	int ret;
+
+	ret = wait_event_interruptible(
+		queue->rx_wait,
+		!(readl(pci_bmc_device->msg_bar_reg + PCI_BMC_BMC2HOST_STS) &
+		  ((index == QUEUE1) ? BMC2HOST_Q1_EMPTY : BMC2HOST_Q2_EMPTY)));
+	if (ret)
+		return -EINTR;
+
+	data[0] = readl(pci_bmc_device->msg_bar_reg +
+			((index == QUEUE1) ? PCI_BMC_BMC2HOST_Q1 :
+					     PCI_BMC_BMC2HOST_Q2));
+
+	writel(HOST2BMC_INT_STS_DOORBELL | HOST2BMC_ENABLE_INTB,
+	       pci_bmc_device->msg_bar_reg + PCI_BMC_HOST2BMC_STS);
+
+	return sizeof(u32);
+}
+
+static ssize_t aspeed_queue_tx(struct file *filp, struct kobject *kobj,
+			       const struct bin_attribute *attr, char *buf,
+			       loff_t off, size_t count)
+{
+	struct aspeed_queue_message *queue = attr->private;
+	struct aspeed_pci_bmc_dev *pci_bmc_device = queue->pci_bmc_device;
+	int index = queue->index;
+	u32 tx_buff;
+	int ret;
+
+	if (count != sizeof(u32))
+		return -EINVAL;
+
+	ret = wait_event_interruptible(
+		queue->tx_wait,
+		!(readl(pci_bmc_device->msg_bar_reg + PCI_BMC_HOST2BMC_STS) &
+		  ((index == QUEUE1) ? HOST2BMC_Q1_FULL : HOST2BMC_Q2_FULL)));
+	if (ret)
+		return -EINTR;
+
+	memcpy(&tx_buff, buf, 4);
+	writel(tx_buff, pci_bmc_device->msg_bar_reg +
+				((index == QUEUE1) ? PCI_BMC_HOST2BMC_Q1 :
+						     PCI_BMC_HOST2BMC_Q2));
+	//trigger to host
+	writel(HOST2BMC_INT_STS_DOORBELL | HOST2BMC_ENABLE_INTB,
+	       pci_bmc_device->msg_bar_reg + PCI_BMC_HOST2BMC_STS);
+
+	return sizeof(u32);
+}
+
+static irqreturn_t aspeed_pci_host_bmc_device_interrupt(int irq, void *dev_id)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_device = dev_id;
+	u32 bmc2host_q_sts =
+		readl(pci_bmc_device->msg_bar_reg + PCI_BMC_BMC2HOST_STS);
+
+	if (bmc2host_q_sts & BMC2HOST_INT_STS_DOORBELL)
+		writel(BMC2HOST_INT_STS_DOORBELL,
+		       pci_bmc_device->msg_bar_reg + PCI_BMC_BMC2HOST_STS);
+
+	if (bmc2host_q_sts & BMC2HOST_ENABLE_INTB)
+		writel(BMC2HOST_ENABLE_INTB,
+		       pci_bmc_device->msg_bar_reg + PCI_BMC_BMC2HOST_STS);
+
+	if (bmc2host_q_sts & BMC2HOST_Q1_FULL)
+		dev_info(pci_bmc_device->dev, "Q1 Full\n");
+
+	if (bmc2host_q_sts & BMC2HOST_Q2_FULL)
+		dev_info(pci_bmc_device->dev, "Q2 Full\n");
+
+	//check q1
+	if (!(readl(pci_bmc_device->msg_bar_reg + PCI_BMC_HOST2BMC_STS) &
+	      HOST2BMC_Q1_FULL))
+		wake_up_interruptible(&pci_bmc_device->queue[QUEUE1].tx_wait);
+
+	if (!(readl(pci_bmc_device->msg_bar_reg + PCI_BMC_BMC2HOST_STS) &
+	      BMC2HOST_Q1_EMPTY))
+		wake_up_interruptible(&pci_bmc_device->queue[QUEUE1].rx_wait);
+	//chech q2
+	if (!(readl(pci_bmc_device->msg_bar_reg + PCI_BMC_HOST2BMC_STS) &
+	      HOST2BMC_Q2_FULL))
+		wake_up_interruptible(&pci_bmc_device->queue[QUEUE2].tx_wait);
+
+	if (!(readl(pci_bmc_device->msg_bar_reg + PCI_BMC_BMC2HOST_STS) &
+	      BMC2HOST_Q2_EMPTY))
+		wake_up_interruptible(&pci_bmc_device->queue[QUEUE2].rx_wait);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t aspeed_pci_host_mbox_interrupt(int irq, void *dev_id)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_device = dev_id;
+	u32 isr = readl(pci_bmc_device->sio_mbox_reg + 0x94);
+
+	if (isr & BIT(7))
+		writel(BIT(7), pci_bmc_device->sio_mbox_reg + 0x94);
+
+	return IRQ_HANDLED;
+}
+
+static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+
+	/* Assign static msi index table by platform */
+	if (pdev->revision == 0x27) {
+		pci_bmc_dev->msi_idx_table = ast2700_soc0_msi_idx_table;
+	} else {
+		pci_bmc_dev->msi_idx_table = ast2600_msi_idx_table;
+	}
+
+	if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI,
+				  PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)
+		/* Set all msi index to the first vector */
+		memset(pci_bmc_dev->msi_idx_table, 0,
+		       sizeof(int) * MAX_MSI_NUM);
+}
+
+static int aspeed_pci_bmc_device_setup_queue(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_device = pci_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int ret, i;
+
+	for (i = 0; i < ASPEED_QUEUE_NUM; i++) {
+		struct aspeed_queue_message *queue = &pci_bmc_device->queue[i];
+
+		init_waitqueue_head(&queue->tx_wait);
+		init_waitqueue_head(&queue->rx_wait);
+
+		sysfs_bin_attr_init(&queue->bin);
+
+		/* Queue name index starts from 1 */
+		queue->bin.attr.name = devm_kasprintf(
+			dev, GFP_KERNEL, "pci-bmc-dev-queue%d", (i + 1));
+		queue->bin.attr.mode = 0600;
+		queue->bin.read = aspeed_queue_rx;
+		queue->bin.write = aspeed_queue_tx;
+		queue->bin.size = 4;
+		queue->bin.private = queue;
+
+		ret = sysfs_create_bin_file(&pdev->dev.kobj, &queue->bin);
+		if (ret) {
+			dev_err(dev, "error for bin%d file\n", i);
+			return ret;
+		}
+
+		queue->kn =
+			kernfs_find_and_get(dev->kobj.sd, queue->bin.attr.name);
+		if (!queue->kn) {
+			sysfs_remove_bin_file(&dev->kobj, &queue->bin);
+			return ret;
+		}
+
+		queue->index = i;
+		queue->pci_bmc_device = pci_bmc_device;
+	}
+
+	return 0;
+}
+
+static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	u16 vuart_ioport;
+	int ret, i;
+
+	for (i = 0; i < VUART_MAX_PARMS; i++) {
+		/* Assign the line to non-exist device */
+		pci_bmc_dev->uart_line[i] = -ENOENT;
+		vuart_ioport = 0x3F8 - (i * 0x100);
+		pci_bmc_dev->uart[i].port.flags =
+			UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
+		pci_bmc_dev->uart[i].port.uartclk = 115200 * 16;
+		pci_bmc_dev->uart[i].port.irq = pci_irq_vector(
+			pdev, pci_bmc_dev->msi_idx_table[VUART0_MSI + i]);
+		pci_bmc_dev->uart[i].port.dev = dev;
+		pci_bmc_dev->uart[i].port.iotype = UPIO_MEM32;
+		pci_bmc_dev->uart[i].port.iobase = 0;
+		pci_bmc_dev->uart[i].port.mapbase =
+			pci_bmc_dev->message_bar_base + (vuart_ioport << 2);
+		pci_bmc_dev->uart[i].port.membase = 0;
+		pci_bmc_dev->uart[i].port.type = PORT_16550A;
+		pci_bmc_dev->uart[i].port.flags |=
+			(UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
+		pci_bmc_dev->uart[i].port.regshift = 2;
+		ret = serial8250_register_8250_port(&pci_bmc_dev->uart[i]);
+		if (ret < 0) {
+			dev_err_probe(dev, ret, "Can't setup PCIe VUART\n");
+			return ret;
+		}
+		pci_bmc_dev->uart_line[i] = ret;
+	}
+	return 0;
+}
+
+static int aspeed_pci_bmc_device_setup_memory_mapping(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pci_bmc_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
+	pci_bmc_dev->miscdev.name = devm_kasprintf(
+		dev, GFP_KERNEL, "%s%d", DRIVER_NAME, pci_bmc_dev->id);
+	pci_bmc_dev->miscdev.fops = &aspeed_pci_bmc_dev_fops;
+	pci_bmc_dev->miscdev.parent = dev;
+
+	ret = misc_register(&pci_bmc_dev->miscdev);
+	if (ret) {
+		pr_err("host bmc register fail %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int aspeed_pci_bmc_device_setup_mbox(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	/* setup mbox */
+	pci_bmc_dev->pcie_sio_decode_addr =
+		pci_bmc_dev->msg_bar_reg + PCIE_DEVICE_SIO_ADDR;
+	writel(0xaa, pci_bmc_dev->pcie_sio_decode_addr);
+	writel(0xa5, pci_bmc_dev->pcie_sio_decode_addr);
+	writel(0xa5, pci_bmc_dev->pcie_sio_decode_addr);
+	writel(0x07, pci_bmc_dev->pcie_sio_decode_addr);
+	writel(0x0e, pci_bmc_dev->pcie_sio_decode_addr + 0x04);
+	/* disable */
+	writel(0x30, pci_bmc_dev->pcie_sio_decode_addr);
+	writel(0x00, pci_bmc_dev->pcie_sio_decode_addr + 0x04);
+	/* set decode address 0x100 */
+	writel(0x60, pci_bmc_dev->pcie_sio_decode_addr);
+	writel(0x01, pci_bmc_dev->pcie_sio_decode_addr + 0x04);
+	writel(0x61, pci_bmc_dev->pcie_sio_decode_addr);
+	writel(0x00, pci_bmc_dev->pcie_sio_decode_addr + 0x04);
+	/* enable */
+	writel(0x30, pci_bmc_dev->pcie_sio_decode_addr);
+	writel(0x01, pci_bmc_dev->pcie_sio_decode_addr + 0x04);
+	pci_bmc_dev->sio_mbox_reg = pci_bmc_dev->msg_bar_reg + 0x400;
+
+	ret = devm_request_irq(
+		dev, pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[MBX_MSI]),
+		aspeed_pci_host_mbox_interrupt, IRQF_SHARED,
+		devm_kasprintf(dev, GFP_KERNEL, "aspeed-sio-mbox%d",
+			       pci_bmc_dev->id),
+		pci_bmc_dev);
+	if (ret) {
+		pr_err("host bmc device Unable to get IRQ %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void aspeed_pci_host_bmc_device_release_queue(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < ASPEED_QUEUE_NUM; i++)
+		sysfs_remove_bin_file(&pdev->dev.kobj,
+				      &pci_bmc_dev->queue[i].bin);
+}
+
+static void aspeed_pci_host_bmc_device_release_vuart(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < VUART_MAX_PARMS; i++) {
+		if (pci_bmc_dev->uart_line[i] >= 0)
+			serial8250_unregister_port(pci_bmc_dev->uart_line[i]);
+	}
+}
+
+static void
+aspeed_pci_host_bmc_device_release_memory_mapping(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+
+	if (!list_empty(&pci_bmc_dev->miscdev.list))
+		misc_deregister(&pci_bmc_dev->miscdev);
+}
+
+static int aspeed_pci_host_setup(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	int rc = 0;
+
+	/* Get share memory BAR */
+	pci_bmc_dev->mem_bar_base = pci_resource_start(pdev, 0);
+	pci_bmc_dev->mem_bar_size = pci_resource_len(pdev, 0);
+	pci_bmc_dev->mem_bar_reg = pci_ioremap_bar(pdev, 0);
+	if (!pci_bmc_dev->mem_bar_reg)
+		return -ENOMEM;
+
+	/* Get Message BAR */
+	pci_bmc_dev->message_bar_base = pci_resource_start(pdev, 1);
+	pci_bmc_dev->message_bar_size = pci_resource_len(pdev, 1);
+	pci_bmc_dev->msg_bar_reg = pci_ioremap_bar(pdev, 1);
+	if (!pci_bmc_dev->msg_bar_reg) {
+		rc = -ENOMEM;
+		goto out_free0;
+	}
+
+	/* AST2600 ERRTA40: dummy read */
+	if (pdev->revision < 0x27)
+		(void)__raw_readl((void __iomem *)pci_bmc_dev->msg_bar_reg);
+
+	rc = aspeed_pci_bmc_device_setup_queue(pdev);
+	if (rc) {
+		pr_err("Cannot setup Queue Message");
+		goto out_free1;
+	}
+
+	rc = aspeed_pci_bmc_device_setup_memory_mapping(pdev);
+	if (rc) {
+		pr_err("Cannot setup Memory Mapping");
+		goto out_free_queue;
+	}
+
+	rc = aspeed_pci_bmc_device_setup_mbox(pdev);
+	if (rc) {
+		pr_err("Cannot setup Mailnbox");
+		goto out_free_mmapping;
+	}
+
+	rc = aspeed_pci_bmc_device_setup_vuart(pdev);
+	if (rc) {
+		pr_err("Cannot setup Virtual UART");
+		goto out_free_mbox;
+	}
+
+	rc = devm_request_irq(
+		&pdev->dev,
+		pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[BMC_MSI]),
+		aspeed_pci_host_bmc_device_interrupt, IRQF_SHARED,
+		pci_bmc_dev->miscdev.name, pci_bmc_dev);
+	if (rc) {
+		pr_err("Get BMC DEVICE IRQ failed. (err=%d)\n", rc);
+		goto out_free_uart;
+	}
+
+	return 0;
+
+out_free_uart:
+	aspeed_pci_host_bmc_device_release_vuart(pdev);
+out_free_mbox:
+	devm_free_irq(&pdev->dev,
+		      pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[MBX_MSI]),
+		      pci_bmc_dev);
+out_free_mmapping:
+	aspeed_pci_host_bmc_device_release_memory_mapping(pdev);
+out_free_queue:
+	aspeed_pci_host_bmc_device_release_queue(pdev);
+out_free1:
+	pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
+out_free0:
+	pci_iounmap(pdev, pci_bmc_dev->mem_bar_reg);
+
+	pci_release_regions(pdev);
+	return rc;
+}
+
+static struct aspeed_platform aspeed_pcie_host[] = {
+	{ .setup = aspeed_pci_host_setup },
+	{ 0 }
+};
+
+static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev,
+					    const struct pci_device_id *ent)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev;
+	int rc = 0;
+
+	pr_info("ASPEED BMC PCI ID %04x:%04x, IRQ=%u\n", pdev->vendor,
+		pdev->device, pdev->irq);
+
+	pci_bmc_dev =
+		devm_kzalloc(&pdev->dev, sizeof(*pci_bmc_dev), GFP_KERNEL);
+	if (!pci_bmc_dev)
+		return -ENOMEM;
+
+	/* Get platform id */
+	pci_bmc_dev->driver_data = ent->driver_data;
+	pci_bmc_dev->platform = &aspeed_pcie_host[ent->driver_data];
+
+	pci_bmc_dev->id = ida_alloc(&bmc_device_ida, GFP_KERNEL);
+	if (pci_bmc_dev->id < 0)
+		return pci_bmc_dev->id;
+
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "pci_enable_device() returned error %d\n",
+			rc);
+		return rc;
+	}
+
+	pci_set_master(pdev);
+	pci_set_drvdata(pdev, pci_bmc_dev);
+
+	/* Prepare IRQ resource */
+	aspeed_pci_setup_irq_resource(pdev);
+
+	/* Setup BMC PCI device */
+	rc = pci_bmc_dev->platform->setup(pdev);
+	if (rc) {
+		dev_err(&pdev->dev,
+			"ASPEED PCIe Host device returned error %d\n", rc);
+		pci_free_irq_vectors(pdev);
+		pci_disable_device(pdev);
+		return rc;
+	}
+
+	return 0;
+}
+
+static void aspeed_pci_host_bmc_device_remove(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+
+	if (pci_bmc_dev->driver_data == ASPEED) {
+		aspeed_pci_host_bmc_device_release_queue(pdev);
+		aspeed_pci_host_bmc_device_release_memory_mapping(pdev);
+		aspeed_pci_host_bmc_device_release_vuart(pdev);
+
+		devm_free_irq(
+			&pdev->dev,
+			pci_irq_vector(pdev,
+				       pci_bmc_dev->msi_idx_table[BMC_MSI]),
+			pci_bmc_dev);
+		devm_free_irq(
+			&pdev->dev,
+			pci_irq_vector(pdev,
+				       pci_bmc_dev->msi_idx_table[MBX_MSI]),
+			pci_bmc_dev);
+	}
+
+	ida_free(&bmc_device_ida, pci_bmc_dev->id);
+
+	pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
+	pci_iounmap(pdev, pci_bmc_dev->mem_bar_reg);
+
+	pci_free_irq_vectors(pdev);
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+}
+
+/**
+ * This table holds the list of (VendorID,DeviceID) supported by this driver
+ *
+ */
+static struct pci_device_id aspeed_host_bmc_dev_pci_ids[] = {
+	/* ASPEED BMC Device */
+	{ PCI_DEVICE(0x1A03, 0x2402), .class = 0xFF0000, .class_mask = 0xFFFF00,
+	  .driver_data = ASPEED },
+	{
+		0,
+	}
+};
+
+MODULE_DEVICE_TABLE(pci, aspeed_host_bmc_dev_pci_ids);
+
+static struct pci_driver aspeed_host_bmc_dev_driver = {
+	.name = DRIVER_NAME,
+	.id_table = aspeed_host_bmc_dev_pci_ids,
+	.probe = aspeed_pci_host_bmc_device_probe,
+	.remove = aspeed_pci_host_bmc_device_remove,
+};
+
+static int __init aspeed_host_bmc_device_init(void)
+{
+	return pci_register_driver(&aspeed_host_bmc_dev_driver);
+}
+
+static void aspeed_host_bmc_device_exit(void)
+{
+	/* unregister pci driver */
+	pci_unregister_driver(&aspeed_host_bmc_dev_driver);
+}
+
+late_initcall(aspeed_host_bmc_device_init);
+module_exit(aspeed_host_bmc_device_exit);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
+MODULE_LICENSE("GPL");
\ No newline at end of file
-- 
2.51.2

Re: [PATCH v1 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Andrew Lunn]

> +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	u16 vuart_ioport;
> +	int ret, i;
> +
> +	for (i = 0; i < VUART_MAX_PARMS; i++) {
> +		/* Assign the line to non-exist device */
> +		pci_bmc_dev->uart_line[i] = -ENOENT;
> +		vuart_ioport = 0x3F8 - (i * 0x100);
> +		pci_bmc_dev->uart[i].port.flags =
> +			UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
> +		pci_bmc_dev->uart[i].port.uartclk = 115200 * 16;
> +		pci_bmc_dev->uart[i].port.irq = pci_irq_vector(
> +			pdev, pci_bmc_dev->msi_idx_table[VUART0_MSI + i]);
> +		pci_bmc_dev->uart[i].port.dev = dev;
> +		pci_bmc_dev->uart[i].port.iotype = UPIO_MEM32;
> +		pci_bmc_dev->uart[i].port.iobase = 0;

How virtual is this? Is this directly accessing the hardware via
shared memory? Or is there software on the BMC which traps these
reads/writes and responds?

But first we need to decide if this is the correct architecture. The
alternative is rpmsg or virtio. There is already tty/rpmsg_tty.c. A
gpio driver over rpmsg is on the way. I2C has been discussed. There
are plenty of virtio drivers, console, i2c, spi, gpio. virtio has
higher performance, but also requires more memory. rpmsg is more
suited to slower devices where memory is tight.

Either of these seem like a better way to expose resources of the BMC
to the host.

       Andrew

Re: [PATCH v1 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Grégoire Layet]

> How virtual is this? Is this directly accessing the hardware via
> shared memory? Or is there software on the BMC which traps these
> reads/writes and responds?

The VUART is virtual because there is no physical UART link between
the host and the BMC.
Instead, the AST2600 exposes a 16550-compatible register set on both
sides (BMC APB and PCIe host MMIO).
The data flows using an internal 16 byte FIFO shared between the two
register views.
So it's hardware emulated and there is no software in the data path.

The AST2600 has four VUARTs, two of which are accessible via PCIe MMIO.
This is based on the following section of the AST2600 datasheet:
III.48 VUART and III.64 PCI2VUART.

Because the silicon presents a standard 16550A interface in hardware,
the existing 8250 driver works without modification.

> But first we need to decide if this is the correct architecture. The
> alternative is rpmsg or virtio.

> Either of these seem like a better way to expose resources of the BMC
> to the host.

For the rest of the driver (shared memory, doorbell and mailbox), you are right,
it makes more sense to implement rpmsg or virtio than just raw shared
memory binding.
These are software-defined communication channels and not hardware-emulated,
so they would fit better as rpmsg or virtio drivers.
I took the SDK driver as a starting point without questioning its structure.
I have verified the VUART was working correctly with the shared
memory, doorbell and mailbox setup removed.
I can split this into VUART only and defer the rest for a separate
rpmsg/virtio work.

So I propose for v2:
- Remove the shared memory device, the sysfs doorbell and the mailbox
from this series.
- Retain only the required configuration and initialisation on the BMC
side driver.
  This should mainly be SCU and PCIe device configuration but other
initialisation will be reviewed to determine what is required.
- The shared memory, doorbell and mailbox features could then be
addressed in a separate future series, likely as virtio based driver.

Would that be acceptable?

Thanks,
Grégoire

Re: [PATCH v1 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Andrew Lunn]

On Wed, Jun 03, 2026 at 03:43:36PM +0200, Grégoire Layet wrote:
> > How virtual is this? Is this directly accessing the hardware via
> > shared memory? Or is there software on the BMC which traps these
> > reads/writes and responds?
> 
> The VUART is virtual because there is no physical UART link between
> the host and the BMC.
> Instead, the AST2600 exposes a 16550-compatible register set on both
> sides (BMC APB and PCIe host MMIO).
> The data flows using an internal 16 byte FIFO shared between the two
> register views.
> So it's hardware emulated and there is no software in the data path.
> 
> The AST2600 has four VUARTs, two of which are accessible via PCIe MMIO.
> This is based on the following section of the AST2600 datasheet:
> III.48 VUART and III.64 PCI2VUART.
> 
> Because the silicon presents a standard 16550A interface in hardware,
> the existing 8250 driver works without modification.

So tell us about security.

Is only this UART exposed in the shared memory? So the memory window
is 8 bytes wide? Or are there other peripherals also exposed? How do
we know the aspeed is not using the UART itself? If two drivers are
using it, are we going to crash one or the other system?

https://en.wikipedia.org/wiki/Core_War

The advantage of rpmsg is that the aspeed would advertise what
services it is willing to expose. The security issues are different,
implementation bugs vs exposing bits of hardware to an attacker.

    Andrew

Re: [PATCH v1 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Andrew Jeffery]

On Wed, 2026-06-03 at 16:30 +0200, Andrew Lunn wrote:
> On Wed, Jun 03, 2026 at 03:43:36PM +0200, Grégoire Layet wrote:
> > > How virtual is this? Is this directly accessing the hardware via
> > > shared memory? Or is there software on the BMC which traps these
> > > reads/writes and responds?
> > 
> > The VUART is virtual because there is no physical UART link between
> > the host and the BMC.
> > Instead, the AST2600 exposes a 16550-compatible register set on both
> > sides (BMC APB and PCIe host MMIO).
> > The data flows using an internal 16 byte FIFO shared between the two
> > register views.
> > So it's hardware emulated and there is no software in the data path.
> > 
> > The AST2600 has four VUARTs, two of which are accessible via PCIe MMIO.
> > This is based on the following section of the AST2600 datasheet:
> > III.48 VUART and III.64 PCI2VUART.
> > 
> > Because the silicon presents a standard 16550A interface in hardware,
> > the existing 8250 driver works without modification.
> 
> So tell us about security.
> 
> Is only this UART exposed in the shared memory?
> 

No, however the BMC PCI interface provides a PCI-to-LPC bridge, so PCI
accesses can drive cycles in to e.g LPC IO devices exposed by the BMC.

>  So the memory window
> is 8 bytes wide? Or are there other peripherals also exposed? How do
> we know the aspeed is not using the UART itself?
> 

For the "regular" SuperIO-controlled UARTs this is a concern, but it's
not a concern for the VUARTs. Each VUART has a pair of 8250 register
sets, one accessible from the BMC, the other accessible to the host,
where both interfaces share the FIFOs to propagate data.

>  If two drivers are
> using it, are we going to crash one or the other system?

By the above, not for the VUARTs.

> 
> https://en.wikipedia.org/wiki/Core_War
> 
> The advantage of rpmsg is that the aspeed would advertise what
> services it is willing to expose. The security issues are different,
> implementation bugs vs exposing bits of hardware to an attacker.

From my understanding rpmsg seems like a reasonable fit for the mailbox
functionality.

Andrew

Re: [PATCH v1 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Andrew Jeffery]

On Wed, 2026-06-03 at 15:43 +0200, Grégoire Layet wrote:
> 
> For the rest of the driver (shared memory, doorbell and mailbox), you are right,
> it makes more sense to implement rpmsg or virtio than just raw shared
> memory binding.
> These are software-defined communication channels and not hardware-emulated,
> so they would fit better as rpmsg or virtio drivers.
> I took the SDK driver as a starting point without questioning its structure.
> I have verified the VUART was working correctly with the shared
> memory, doorbell and mailbox setup removed.
> I can split this into VUART only and defer the rest for a separate
> rpmsg/virtio work.
> 
> So I propose for v2:
> - Remove the shared memory device, the sysfs doorbell and the mailbox
> from this series.
> - Retain only the required configuration and initialisation on the BMC
> side driver.
>   This should mainly be SCU and PCIe device configuration but other
> initialisation will be reviewed to determine what is required.
> - The shared memory, doorbell and mailbox features could then be
> addressed in a separate future series, likely as virtio based driver.
> 
> Would that be acceptable?

That sounds good to me.

From a brief inspection the driver also had support for both the 2600
and 2700. Something to consider is just supporting one of those for
now, and adding support for the other in later patches.

Andrew

[PATCH v2 0/2] soc: aspeed: Add BMC and host driver for PCIe BMC device

[Grégoire Layet]

This is a v2 for upstreaming the VUART over PCIe BMC device driver from the ASPEED kernel SDK (branch master-v6.18) [1].
There are two drivers: a BMC-side driver and a host-side driver.
Together they enable host<->BMC VUART communication via PCIe. 

This v2 narrows down the scope to VUART support only, to address review feedback on v1 [2] 
that the additional subsystems (shared memory, doorbell and mailbox) were software-defined
IPC channels better used with rpmsg or virtio.
Those subsystems are deferred to a separate future series.

VUART data flow and MSI interrupts have been verified working on the test hardware.

Tested on:
BMC:
- Asus IPMI Kommando Card R1.01, AST2600 A3.
- OpenBMC
Host:
- Linux kernel v7.0.0

This v2 only supports AST2600; the AST2700 is untested and not supported by this patch.

Changes since v1 [2]:
 - BMC driver: trimmed down to only SCU and PCIe initialization
 - Host driver: removed shared memory misc device, sysfs doorbell, mailbox setup and message queue handler.
    Driver now only supports VUART registration.
 - Host driver: Fixed cleanup path: removed pci_release_regions() call as there was no matching pci_request_regions call

[1]: https://github.com/AspeedTech-BMC/linux/tree/aspeed-master-v6.18/drivers/soc/aspeed
[2]: https://lore.kernel.org/linux-aspeed/cover.1780409151.git.gregoire.layet@9elements.com/

Grégoire Layet (2):
  soc: aspeed: add BMC-side PCIe BMC device driver
  soc: aspeed: add host-side PCIe BMC device driver

 drivers/soc/aspeed/Kconfig               |  15 ++
 drivers/soc/aspeed/Makefile              |   2 +
 drivers/soc/aspeed/aspeed-bmc-dev.c      | 187 +++++++++++++++++
 drivers/soc/aspeed/aspeed-host-bmc-dev.c | 249 +++++++++++++++++++++++
 4 files changed, 453 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c
 create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c

-- 
2.51.2

[PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver

[Grégoire Layet]

Taken from ASPEED 6.18 Kernel SDK

Add support for VUART over PCIe between BMC and host.
This add BMC side driver.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/soc/aspeed/Kconfig          |   7 ++
 drivers/soc/aspeed/Makefile         |   1 +
 drivers/soc/aspeed/aspeed-bmc-dev.c | 187 ++++++++++++++++++++++++++++
 3 files changed, 195 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c

diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index f579ee0b5afa..3e1fcf3c3268 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -4,6 +4,13 @@ if ARCH_ASPEED || COMPILE_TEST
 
 menu "ASPEED SoC drivers"
 
+config ASPEED_BMC_DEV
+	tristate "ASPEED BMC Device"
+	default n
+	help
+	  Enable support for the ASPEED AST2600 BMC Device.
+	  This exposes the PCIe-to-LPC bridge of the BMC to the host over PCIe.
+
 config ASPEED_LPC_CTRL
 	tristate "ASPEED LPC firmware cycle control"
 	select REGMAP
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..fab0d247df66 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_ASPEED_BMC_DEV)		+= aspeed-bmc-dev.o
 obj-$(CONFIG_ASPEED_LPC_CTRL)		+= aspeed-lpc-ctrl.o
 obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
diff --git a/drivers/soc/aspeed/aspeed-bmc-dev.c b/drivers/soc/aspeed/aspeed-bmc-dev.c
new file mode 100644
index 000000000000..7a204b543c97
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-bmc-dev.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) ASPEED Technology Inc.
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include <linux/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+
+#define SCU_TRIGGER_MSI
+
+/* AST2600 SCU */
+#define ASPEED_SCU04			0x04
+#define AST2600A3_SCU04				0x05030303
+#define ASPEED_SCUC20			0xC20
+#define ASPEED_SCUC24			0xC24
+#define MSI_ROUTING_MASK			GENMASK(11, 10)
+#define PCIDEV1_INTX_MSI_HOST2BMC_EN		BIT(18)
+#define MSI_ROUTING_PCIe2LPC_PCIDEV0		(0x1 << 10)
+#define MSI_ROUTING_PCIe2LPC_PCIDEV1		(0x2 << 10)
+
+#define ASPEED_SCU_PCIE_CONF_CTRL	0xC20
+#define  SCU_PCIE_CONF_BMC_DEV_EN			 BIT(8)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_MMIO		 BIT(9)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_MSI		 BIT(11)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_IRQ		 BIT(13)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_DMA		 BIT(14)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_E2L		 BIT(15)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE BIT(21)
+
+#define ASPEED_SCU_BMC_DEV_CLASS	0xC68
+
+
+struct aspeed_platform {
+	int (*init)(struct platform_device *pdev);
+};
+
+struct aspeed_bmc_device {
+	struct device *dev;
+	int id;
+	void __iomem *reg_base;
+
+	int pcie2lpc;
+	int irq;
+
+	const struct aspeed_platform *platform;
+
+	struct regmap *scu;
+	int pcie_irq;
+};
+
+
+static int aspeed_ast2600_init(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
+			      SCU_PCIE_CONF_BMC_DEV_EN_MMIO | SCU_PCIE_CONF_BMC_DEV_EN;
+	u32 scu_id;
+
+	bmc_device->scu = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,scu");
+	if (IS_ERR(bmc_device->scu)) {
+		dev_err(&pdev->dev, "failed to find SCU regmap\n");
+		return PTR_ERR(bmc_device->scu);
+	}
+
+	if (bmc_device->pcie2lpc)
+		pcie_config_ctl |= SCU_PCIE_CONF_BMC_DEV_EN_E2L |
+				   SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE;
+
+	regmap_update_bits(bmc_device->scu, ASPEED_SCU_PCIE_CONF_CTRL,
+			   pcie_config_ctl, pcie_config_ctl);
+
+	/* update class code to others as it is a MFD device */
+	regmap_write(bmc_device->scu, ASPEED_SCU_BMC_DEV_CLASS, 0xff000000);
+
+#ifdef SCU_TRIGGER_MSI
+	//SCUC24[17]: Enable PCI device 1 INTx/MSI from SCU560[15]. Will be added in next version
+	regmap_update_bits(bmc_device->scu, ASPEED_SCUC20, BIT(11) | BIT(14), BIT(11) | BIT(14));
+
+	regmap_read(bmc_device->scu, ASPEED_SCU04, &scu_id);
+	if (scu_id == AST2600A3_SCU04)
+		regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
+				   PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_MASK,
+				   PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_PCIe2LPC_PCIDEV1);
+	else
+		regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
+				   BIT(17) | BIT(14) | BIT(11), BIT(17) | BIT(14) | BIT(11));
+#else
+	//SCUC24[18]: Enable PCI device 1 INTx/MSI from Host-to-BMC controller.
+	regmap_update_bits(bmc_device->scu, 0xc24, BIT(18) | BIT(14), BIT(18) | BIT(14));
+#endif
+
+
+	return 0;
+}
+
+
+static struct aspeed_platform ast2600_plaform = {
+	.init = aspeed_ast2600_init
+};
+
+
+static const struct of_device_id aspeed_bmc_device_of_matches[] = {
+	{ .compatible = "aspeed,ast2600-bmc-device", .data = &ast2600_plaform },
+	{},
+};
+MODULE_DEVICE_TABLE(of, aspeed_bmc_device_of_matches);
+
+static int aspeed_bmc_device_probe(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device;
+	struct device *dev = &pdev->dev;
+	const void *md = of_device_get_match_data(dev);
+	int ret = 0;
+
+	if (!md)
+		return -ENODEV;
+
+	bmc_device = devm_kzalloc(&pdev->dev, sizeof(struct aspeed_bmc_device), GFP_KERNEL);
+	if (!bmc_device)
+		return -ENOMEM;
+	dev_set_drvdata(dev, bmc_device);
+
+	bmc_device->platform = md;
+
+	bmc_device->id = of_alias_get_id(dev->of_node, "bmcdev");
+	if (bmc_device->id < 0)
+		bmc_device->id = 0;
+
+	bmc_device->dev = dev;
+	bmc_device->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(bmc_device->reg_base))
+		return PTR_ERR(bmc_device->reg_base);
+
+	bmc_device->irq = platform_get_irq(pdev, 0);
+	if (bmc_device->irq < 0) {
+		dev_err(&pdev->dev, "platform get of irq[=%d] failed!\n", bmc_device->irq);
+		return bmc_device->irq;
+	}
+
+	if (of_property_read_bool(dev->of_node, "pcie2lpc"))
+		bmc_device->pcie2lpc = 1;
+
+	ret = bmc_device->platform->init(pdev);
+	if (ret) {
+		dev_err(dev, "Initialize bmc device failed\n");
+		goto out;
+	}
+
+	dev_info(dev, "aspeed bmc device: driver successfully loaded.\n");
+
+	return 0;
+
+out:
+	dev_warn(dev, "aspeed bmc device: driver init failed (ret=%d)!\n", ret);
+	return ret;
+}
+
+static void aspeed_bmc_device_remove(struct platform_device *pdev)
+{
+	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
+
+	devm_free_irq(&pdev->dev, bmc_device->irq, bmc_device);
+	devm_kfree(&pdev->dev, bmc_device);
+}
+
+static struct platform_driver aspeed_bmc_device_driver = {
+	.probe		= aspeed_bmc_device_probe,
+	.remove		= aspeed_bmc_device_remove,
+	.driver		= {
+		.name	= KBUILD_MODNAME,
+		.of_match_table = aspeed_bmc_device_of_matches,
+	},
+};
+
+module_platform_driver(aspeed_bmc_device_driver);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED BMC DEVICE Driver");
+MODULE_LICENSE("GPL");
\ No newline at end of file
-- 
2.51.2

[PATCH v2 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Grégoire Layet]

Taken from ASPEED 6.18 Kernel SDK

Add support for VUART over PCIe between BMC and host.
This add host side driver.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/soc/aspeed/Kconfig               |   8 +
 drivers/soc/aspeed/Makefile              |   1 +
 drivers/soc/aspeed/aspeed-host-bmc-dev.c | 249 +++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c

diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index 3e1fcf3c3268..5deefb64e8c7 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -11,6 +11,14 @@ config ASPEED_BMC_DEV
 	  Enable support for the ASPEED AST2600 BMC Device.
 	  This exposes the PCIe-to-LPC bridge of the BMC to the host over PCIe.
 
+config ASPEED_HOST_BMC_DEV
+	tristate "ASPEED Host BMC Device"
+	depends on PCI
+	depends on SERIAL_8250
+	help
+	  Enable support for the ASPEED AST2600 BMC Device on the Host.
+	  This configure the PCIe and setup two 8250 compatible VUART ports.
+
 config ASPEED_LPC_CTRL
 	tristate "ASPEED LPC firmware cycle control"
 	select REGMAP
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index fab0d247df66..3fd3f6d8d36e 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_ASPEED_BMC_DEV)		+= aspeed-bmc-dev.o
+obj-$(CONFIG_ASPEED_HOST_BMC_DEV)	+= aspeed-host-bmc-dev.o
 obj-$(CONFIG_ASPEED_LPC_CTRL)		+= aspeed-lpc-ctrl.o
 obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
new file mode 100644
index 000000000000..7cb52a770fb6
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) ASPEED Technology Inc.
+
+#include <linux/init.h>
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/pci.h>
+#include <linux/serial_core.h>
+#include <linux/serial_8250.h>
+
+static DEFINE_IDA(bmc_device_ida);
+
+#define VUART_MAX_PARMS	2
+#define MAX_MSI_NUM		8
+#define BMC_MULTI_MSI	32
+
+#define DRIVER_NAME "aspeed-host-bmc-dev"
+
+enum aspeed_platform_id {
+	ASPEED,
+};
+
+enum msi_index {
+	VUART0_MSI,
+	VUART1_MSI,
+};
+
+/* Match msi_index */
+static int ast2600_msi_idx_table[MAX_MSI_NUM] = { 16, 15 };
+
+struct aspeed_platform {
+	int (*setup)(struct pci_dev *pdev);
+};
+
+struct aspeed_pci_bmc_dev {
+	struct device *dev;
+	struct aspeed_platform *platform;
+	kernel_ulong_t driver_data;
+	int id;
+
+	unsigned long message_bar_base;
+	unsigned long message_bar_size;
+	void __iomem *msg_bar_reg;
+
+	struct uart_8250_port uart[VUART_MAX_PARMS];
+	int uart_line[VUART_MAX_PARMS];
+
+	/* Interrupt
+	 * The index of array is using to enum msi_index
+	 */
+	int *msi_idx_table;
+};
+
+static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+
+	/* Assign static msi index table by platform */
+	pci_bmc_dev->msi_idx_table = ast2600_msi_idx_table;
+
+	if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)
+		/* Set all msi index to the first vector */
+		memset(pci_bmc_dev->msi_idx_table, 0, sizeof(int) * MAX_MSI_NUM);
+}
+
+static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	u16 vuart_ioport;
+	int ret, i;
+
+	for (i = 0; i < VUART_MAX_PARMS; i++) {
+		/* Assign the line to non-exist device */
+		pci_bmc_dev->uart_line[i] = -ENOENT;
+		vuart_ioport = 0x3F8 - (i * 0x100);
+		pci_bmc_dev->uart[i].port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
+		pci_bmc_dev->uart[i].port.uartclk = 115200 * 16;
+		pci_bmc_dev->uart[i].port.irq =
+			pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[VUART0_MSI + i]);
+		pci_bmc_dev->uart[i].port.dev = dev;
+		pci_bmc_dev->uart[i].port.iotype = UPIO_MEM32;
+		pci_bmc_dev->uart[i].port.iobase = 0;
+		pci_bmc_dev->uart[i].port.mapbase =
+			pci_bmc_dev->message_bar_base + (vuart_ioport << 2);
+		pci_bmc_dev->uart[i].port.membase = 0;
+		pci_bmc_dev->uart[i].port.type = PORT_16550A;
+		pci_bmc_dev->uart[i].port.flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
+		pci_bmc_dev->uart[i].port.regshift = 2;
+		ret = serial8250_register_8250_port(&pci_bmc_dev->uart[i]);
+		if (ret < 0) {
+			dev_err_probe(dev, ret, "Can't setup PCIe VUART\n");
+			return ret;
+		}
+		pci_bmc_dev->uart_line[i] = ret;
+	}
+	return 0;
+}
+
+static void aspeed_pci_host_bmc_device_release_vuart(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < VUART_MAX_PARMS; i++) {
+		if (pci_bmc_dev->uart_line[i] >= 0)
+			serial8250_unregister_port(pci_bmc_dev->uart_line[i]);
+	}
+}
+
+static int aspeed_pci_host_setup(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	int rc = 0;
+
+	/* Get Message BAR */
+	pci_bmc_dev->message_bar_base = pci_resource_start(pdev, 1);
+	pci_bmc_dev->message_bar_size = pci_resource_len(pdev, 1);
+	pci_bmc_dev->msg_bar_reg = pci_ioremap_bar(pdev, 1);
+	if (!pci_bmc_dev->msg_bar_reg)
+		return -ENOMEM;
+
+	if (pdev->revision < 0x27) {
+		/* AST2600 ERRTA40: dummy read */
+		(void)__raw_readl((void __iomem *)pci_bmc_dev->msg_bar_reg);
+	} else {
+		/* AST2700 not supported */
+		pr_err("AST2700 detected but not supported");
+		goto out_free0;
+	}
+
+	rc = aspeed_pci_bmc_device_setup_vuart(pdev);
+	if (rc) {
+		pr_err("Cannot setup Virtual UART");
+		goto out_free0;
+	}
+
+	return 0;
+
+out_free0:
+	pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
+
+	return rc;
+}
+
+static struct aspeed_platform aspeed_pcie_host[] = {
+	{ .setup = aspeed_pci_host_setup },
+	{ 0 }
+};
+
+static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev;
+	int rc = 0;
+
+	pr_info("ASPEED BMC PCI ID %04x:%04x, IRQ=%u\n", pdev->vendor, pdev->device, pdev->irq);
+
+	pci_bmc_dev = devm_kzalloc(&pdev->dev, sizeof(*pci_bmc_dev), GFP_KERNEL);
+	if (!pci_bmc_dev)
+		return -ENOMEM;
+
+	/* Get platform id */
+	pci_bmc_dev->driver_data = ent->driver_data;
+	pci_bmc_dev->platform = &aspeed_pcie_host[ent->driver_data];
+
+	pci_bmc_dev->id = ida_alloc(&bmc_device_ida, GFP_KERNEL);
+	if (pci_bmc_dev->id < 0)
+		return pci_bmc_dev->id;
+
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "pci_enable_device() returned error %d\n", rc);
+		return rc;
+	}
+
+	pci_set_master(pdev);
+	pci_set_drvdata(pdev, pci_bmc_dev);
+
+	/* Prepare IRQ resource */
+	aspeed_pci_setup_irq_resource(pdev);
+
+	/* Setup BMC PCI device */
+	rc = pci_bmc_dev->platform->setup(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "ASPEED PCIe Host device returned error %d\n", rc);
+		pci_free_irq_vectors(pdev);
+		pci_disable_device(pdev);
+		return rc;
+	}
+
+	return 0;
+}
+
+static void aspeed_pci_host_bmc_device_remove(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+
+	if (pci_bmc_dev->driver_data == ASPEED)
+		aspeed_pci_host_bmc_device_release_vuart(pdev);
+
+	ida_free(&bmc_device_ida, pci_bmc_dev->id);
+
+	pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
+
+	pci_free_irq_vectors(pdev);
+	pci_disable_device(pdev);
+}
+
+/**
+ * This table holds the list of (VendorID,DeviceID) supported by this driver
+ *
+ */
+static struct pci_device_id aspeed_host_bmc_dev_pci_ids[] = {
+	/* ASPEED BMC Device */
+	{ PCI_DEVICE(0x1A03, 0x2402), .class = 0xFF0000, .class_mask = 0xFFFF00,
+	  .driver_data = ASPEED },
+	{
+		0,
+	}
+};
+
+MODULE_DEVICE_TABLE(pci, aspeed_host_bmc_dev_pci_ids);
+
+static struct pci_driver aspeed_host_bmc_dev_driver = {
+	.name		= DRIVER_NAME,
+	.id_table	= aspeed_host_bmc_dev_pci_ids,
+	.probe		= aspeed_pci_host_bmc_device_probe,
+	.remove		= aspeed_pci_host_bmc_device_remove,
+};
+
+static int __init aspeed_host_bmc_device_init(void)
+{
+	return pci_register_driver(&aspeed_host_bmc_dev_driver);
+}
+
+static void aspeed_host_bmc_device_exit(void)
+{
+	/* unregister pci driver */
+	pci_unregister_driver(&aspeed_host_bmc_dev_driver);
+}
+
+late_initcall(aspeed_host_bmc_device_init);
+module_exit(aspeed_host_bmc_device_exit);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
+MODULE_LICENSE("GPL");
-- 
2.51.2

Re: [PATCH v2 0/2] soc: aspeed: Add BMC and host driver for PCIe BMC device

[Andrew Lunn]

On Mon, Jun 08, 2026 at 02:51:21PM +0000, Grégoire Layet wrote:
> This is a v2 for upstreaming the VUART over PCIe BMC device driver from the ASPEED kernel SDK (branch master-v6.18) [1].
> There are two drivers: a BMC-side driver and a host-side driver.
> Together they enable host<->BMC VUART communication via PCIe. 

I would like to see somewhere a comment about security. From the
replies i got the host can access anything on the LPC bus of the
aspeed. What are the implications of that? What are typically on such
a bus? TPMs are often there, but does a BMC have a TPM?

	Andrew

Re: [PATCH v2 0/2] soc: aspeed: Add BMC and host driver for PCIe BMC device

[Grégoire Layet]

> From the replies i got the host can access anything on the LPC bus of the
> aspeed.

The PCI-to-LPC bridge only decodes some addresses.
In fact, only the following are accessible over PCIe :
- LPC Channel 4, which only supports the KCS protocol
- The 2 VUARTs
- SuperIO-style Mailbox

Regarding the KCS interface over the LPC channel 4,
the BMC driver in this series does not set this up.
It just needs to be activated in the DTS.
Then the host can just start the ipmi_si driver with the correct address.
This is the intended usage for IPMI management and is part of the normal
trust model of a BMC-managed system.

Here is a summary dump of the PCI BAR1 from the host.
The BAR0 is only used to expose a BMC DRAM region for shared memory access.
This v2 removed the memory mapping so BAR0 is not exposed to userspace.
The BAR1 is used for the PCI-to-LPC bridge.

Offset          Size        Contents
--------        --------    ------------------------------------
0x0000-0x00B7   184 B       Blank (0xFF)
0x00B8-0x00BF   8 B         SuperIO Index/Data port, mailbox config only
0x00C0-0x0E87   3528 B      Blank (some no decode 0xFF and some reading 0x00)
0x0E88-0x0E8F   8 B         LPC KCS4
0x0E90-0x0FDF   336 B       Blank (0xFF)
0x0FE0-0x0FFF   32 B        VUART1
0x1000-0x2FFFF  188 KiB     Blank (some no decode 0xFF and some reading 0x00)
0x30000-0x3FFFF 64 KiB      Inert region: 256 times repeating reset-default
                            values, not affected by mailbox activation,
                            not accessed by this driver
                            The datasheet doesn't explain what this is.

So, the host does not have arbitrary LPC bus access,
it can only access a small subset of it.
The relevant peripherals must be explicitly enabled the PCI bridge
to be activated on the BMC to be accessed over PCIe.

Only the specific interfaces listed above are exposed,
and each one must be enabled by the BMC.

The security implications are limited.
The host side attack surface is bounded by what the BMC chooses to expose.

> TPMs are often there, but does a BMC have a TPM?
The AST2600 chip doesn't have a TPM.
The Asus IPMI card doesn't have a TPM module either.
If a similar PCIe add-in IPMI card has a TPM module,
it will be accessible to the BMC via the LPC bus.

But this will not be bridged to BAR1 by default
as only the above-listed interfaces are exposed.

However, the host could access the TPM with some specific BMC side
configuration of the PCIe BMC Device (PBMC).
That would be a deliberate BMC side firmware decision or bug.
This driver doesn't expose a BMC TPM.

Re: [PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver

[Andrew Jeffery]

Hello Grégoire,

On Mon, 2026-06-08 at 14:51 +0000, Grégoire Layet wrote:
> Taken from ASPEED 6.18 Kernel SDK

It's probably best to use ASPEED's SDK as a source of inspiration for
fixing obscure bugs, but not send drivers directly extracted from it.

> 
> Add support for VUART over PCIe between BMC and host.
> This add BMC side driver.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
> Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
> Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
> ---
>  drivers/soc/aspeed/Kconfig          |   7 ++
>  drivers/soc/aspeed/Makefile         |   1 +
>  drivers/soc/aspeed/aspeed-bmc-dev.c | 187 ++++++++++++++++++++++++++++

We should avoid adding more drivers in drivers/soc/aspeed where we can.

Is this really necessary?

>  3 files changed, 195 insertions(+)
>  create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c
> 
> diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> index f579ee0b5afa..3e1fcf3c3268 100644
> --- a/drivers/soc/aspeed/Kconfig
> +++ b/drivers/soc/aspeed/Kconfig
> @@ -4,6 +4,13 @@ if ARCH_ASPEED || COMPILE_TEST
>  
>  menu "ASPEED SoC drivers"
>  
> +config ASPEED_BMC_DEV
> +	tristate "ASPEED BMC Device"
> +	default n
> +	help
> +	  Enable support for the ASPEED AST2600 BMC Device.
> +	  This exposes the PCIe-to-LPC bridge of the BMC to the host over PCIe.
> +
>  config ASPEED_LPC_CTRL
>  	tristate "ASPEED LPC firmware cycle control"
>  	select REGMAP
> diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> index b35d74592964..fab0d247df66 100644
> --- a/drivers/soc/aspeed/Makefile
> +++ b/drivers/soc/aspeed/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_ASPEED_BMC_DEV)		+= aspeed-bmc-dev.o
>  obj-$(CONFIG_ASPEED_LPC_CTRL)		+= aspeed-lpc-ctrl.o
>  obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
>  obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
> diff --git a/drivers/soc/aspeed/aspeed-bmc-dev.c b/drivers/soc/aspeed/aspeed-bmc-dev.c
> new file mode 100644
> index 000000000000..7a204b543c97
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-bmc-dev.c
> @@ -0,0 +1,187 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (C) ASPEED Technology Inc.
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define SCU_TRIGGER_MSI
> +
> +/* AST2600 SCU */
> +#define ASPEED_SCU04			0x04
> +#define AST2600A3_SCU04				0x05030303
> +#define ASPEED_SCUC20			0xC20
> +#define ASPEED_SCUC24			0xC24

These could all use properly descriptive names.

Pinctrl is an exception because of how the documentation is structured.

> +#define MSI_ROUTING_MASK			GENMASK(11, 10)
> +#define PCIDEV1_INTX_MSI_HOST2BMC_EN		BIT(18)
> +#define MSI_ROUTING_PCIe2LPC_PCIDEV0		(0x1 << 10)
> +#define MSI_ROUTING_PCIe2LPC_PCIDEV1		(0x2 << 10)
> +
> +#define ASPEED_SCU_PCIE_CONF_CTRL	0xC20
> +#define  SCU_PCIE_CONF_BMC_DEV_EN			 BIT(8)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_MMIO		 BIT(9)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_MSI		 BIT(11)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_IRQ		 BIT(13)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_DMA		 BIT(14)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_E2L		 BIT(15)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE BIT(21)
> +
> +#define ASPEED_SCU_BMC_DEV_CLASS	0xC68
> +
> +
> +struct aspeed_platform {
> +	int (*init)(struct platform_device *pdev);
> +};
> +
> +struct aspeed_bmc_device {
> +	struct device *dev;
> +	int id;
> +	void __iomem *reg_base;
> +
> +	int pcie2lpc;
> +	int irq;
> +
> +	const struct aspeed_platform *platform;
> +
> +	struct regmap *scu;
> +	int pcie_irq;
> +};
> +
> +
> +static int aspeed_ast2600_init(struct platform_device *pdev)
> +{
> +	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
> +			      SCU_PCIE_CONF_BMC_DEV_EN_MMIO | SCU_PCIE_CONF_BMC_DEV_EN;
> +	u32 scu_id;
> +
> +	bmc_device->scu = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,scu");

We should rather look at auxbus for the SCU.

> +	if (IS_ERR(bmc_device->scu)) {
> +		dev_err(&pdev->dev, "failed to find SCU regmap\n");
> +		return PTR_ERR(bmc_device->scu);
> +	}
> +
> +	if (bmc_device->pcie2lpc)
> +		pcie_config_ctl |= SCU_PCIE_CONF_BMC_DEV_EN_E2L |
> +				   SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE;
> +
> +	regmap_update_bits(bmc_device->scu, ASPEED_SCU_PCIE_CONF_CTRL,
> +			   pcie_config_ctl, pcie_config_ctl);
> +
> +	/* update class code to others as it is a MFD device */
> +	regmap_write(bmc_device->scu, ASPEED_SCU_BMC_DEV_CLASS, 0xff000000);
> +
> +#ifdef SCU_TRIGGER_MSI

I don't see that this needs to be a CPP test. This could be a C test.
The construct would be optimised because of the constant and we'd get
compile time coverage of both sides without additional configuration.

Have you tested both sides?

> +	//SCUC24[17]: Enable PCI device 1 INTx/MSI from SCU560[15]. Will be added in next version
> +	regmap_update_bits(bmc_device->scu, ASPEED_SCUC20, BIT(11) | BIT(14), BIT(11) | BIT(14));

These bits need descriptive macros.

> +
> +	regmap_read(bmc_device->scu, ASPEED_SCU04, &scu_id);
> +	if (scu_id == AST2600A3_SCU04)
> +		regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
> +				   PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_MASK,
> +				   PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_PCIe2LPC_PCIDEV1);
> +	else
> +		regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
> +				   BIT(17) | BIT(14) | BIT(11), BIT(17) | BIT(14) | BIT(11));

As do these

> +#else
> +	//SCUC24[18]: Enable PCI device 1 INTx/MSI from Host-to-BMC controller.
> +	regmap_update_bits(bmc_device->scu, 0xc24, BIT(18) | BIT(14), BIT(18) | BIT(14));

And these.

> +#endif
> +
> +
> +	return 0;
> +}
> +
> +
> +static struct aspeed_platform ast2600_plaform = {
> +	.init = aspeed_ast2600_init
> +};
> +
> +
> +static const struct of_device_id aspeed_bmc_device_of_matches[] = {
> +	{ .compatible = "aspeed,ast2600-bmc-device", .data = &ast2600_plaform },

This compatible isn't documented in this series and isn't present in
linux-next at a87737435cfa ("Add linux-next specific files for
20260608"). You'll need to address that if it's reasonable to continue
down this path. I expect you'll want to avoid it, and define any
necessary properties on the SCU node rather than add further children.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_bmc_device_of_matches);
> +
> +static int aspeed_bmc_device_probe(struct platform_device *pdev)
> +{
> +	struct aspeed_bmc_device *bmc_device;
> +	struct device *dev = &pdev->dev;

This shortcut is defined but inconsistently used.

> +	const void *md = of_device_get_match_data(dev);

I think we can do without this, see below.

> +	int ret = 0;
> +
> +	if (!md)
> +		return -ENODEV;
> +
> +	bmc_device = devm_kzalloc(&pdev->dev, sizeof(struct aspeed_bmc_device), GFP_KERNEL);
> +	if (!bmc_device)
> +		return -ENOMEM;
> +	dev_set_drvdata(dev, bmc_device);
> +
> +	bmc_device->platform = md;
> +
> +	bmc_device->id = of_alias_get_id(dev->of_node, "bmcdev");
> +	if (bmc_device->id < 0)
> +		bmc_device->id = 0;
> +
> +	bmc_device->dev = dev;
> +	bmc_device->reg_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(bmc_device->reg_base))
> +		return PTR_ERR(bmc_device->reg_base);
> +
> +	bmc_device->irq = platform_get_irq(pdev, 0);

This seems unnecessary.

> +	if (bmc_device->irq < 0) {
> +		dev_err(&pdev->dev, "platform get of irq[=%d] failed!\n", bmc_device->irq);
> +		return bmc_device->irq;
> +	}
> +
> +	if (of_property_read_bool(dev->of_node, "pcie2lpc"))

This property isn't documented.

> +		bmc_device->pcie2lpc = 1;
> +
> +	ret = bmc_device->platform->init(pdev);

The driver only supports one SoC, this indirection seems unnecessary
right now. We can add that later when there's a need to differentiate.
I'd rather you call the setup function directly for now.

> +	if (ret) {
> +		dev_err(dev, "Initialize bmc device failed\n");
> +		goto out;
> +	}
> +
> +	dev_info(dev, "aspeed bmc device: driver successfully loaded.\n");
> +
> +	return 0;
> +
> +out:
> +	dev_warn(dev, "aspeed bmc device: driver init failed (ret=%d)!\n", ret);
> +	return ret;
> +}
> +
> +static void aspeed_bmc_device_remove(struct platform_device *pdev)
> +{
> +	struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
> +
> +	devm_free_irq(&pdev->dev, bmc_device->irq, bmc_device);
> +	devm_kfree(&pdev->dev, bmc_device);

These are unnecessary due to cleanup of devres on release.

Andrew

Re: [PATCH v2 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Andrew Jeffery]

On Mon, 2026-06-08 at 14:51 +0000, Grégoire Layet wrote:
> Taken from ASPEED 6.18 Kernel SDK
> 
> Add support for VUART over PCIe between BMC and host.
> This add host side driver.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
> Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
> Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
> ---
>  drivers/soc/aspeed/Kconfig               |   8 +
>  drivers/soc/aspeed/Makefile              |   1 +
>  drivers/soc/aspeed/aspeed-host-bmc-dev.c | 249 +++++++++++++++++++++++

Again, I'd rather we avoid drivers/soc/aspeed.

>  3 files changed, 258 insertions(+)
>  create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c
> 
> diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> index 3e1fcf3c3268..5deefb64e8c7 100644
> --- a/drivers/soc/aspeed/Kconfig
> +++ b/drivers/soc/aspeed/Kconfig
> @@ -11,6 +11,14 @@ config ASPEED_BMC_DEV
>  	  Enable support for the ASPEED AST2600 BMC Device.
>  	  This exposes the PCIe-to-LPC bridge of the BMC to the host over PCIe.
>  
> +config ASPEED_HOST_BMC_DEV
> +	tristate "ASPEED Host BMC Device"
> +	depends on PCI
> +	depends on SERIAL_8250
> +	help
> +	  Enable support for the ASPEED AST2600 BMC Device on the Host.
> +	  This configure the PCIe and setup two 8250 compatible VUART ports.
> +
>  config ASPEED_LPC_CTRL
>  	tristate "ASPEED LPC firmware cycle control"
>  	select REGMAP
> diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> index fab0d247df66..3fd3f6d8d36e 100644
> --- a/drivers/soc/aspeed/Makefile
> +++ b/drivers/soc/aspeed/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_ASPEED_BMC_DEV)		+= aspeed-bmc-dev.o
> +obj-$(CONFIG_ASPEED_HOST_BMC_DEV)	+= aspeed-host-bmc-dev.o
>  obj-$(CONFIG_ASPEED_LPC_CTRL)		+= aspeed-lpc-ctrl.o
>  obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
>  obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
> diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> new file mode 100644
> index 000000000000..7cb52a770fb6
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright (C) ASPEED Technology Inc.
> +
> +#include <linux/init.h>
> +#include <linux/version.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/pci.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_8250.h>
> +
> +static DEFINE_IDA(bmc_device_ida);
> +
> +#define VUART_MAX_PARMS	2

Given the one supported piece of hardware we could avoid the associated
loops and rather extract loop bodies to functions and call the function
twice.

> +#define MAX_MSI_NUM		8
> +#define BMC_MULTI_MSI	32
> +
> +#define DRIVER_NAME "aspeed-host-bmc-dev"
> +
> +enum aspeed_platform_id {
> +	ASPEED,
> +};
> +
> +enum msi_index {
> +	VUART0_MSI,
> +	VUART1_MSI,
> +};
> +
> +/* Match msi_index */
> +static int ast2600_msi_idx_table[MAX_MSI_NUM] = { 16, 15 };
> +
> +struct aspeed_platform {
> +	int (*setup)(struct pci_dev *pdev);
> +};
> +
> +struct aspeed_pci_bmc_dev {
> +	struct device *dev;
> +	struct aspeed_platform *platform;
> +	kernel_ulong_t driver_data;
> +	int id;
> +
> +	unsigned long message_bar_base;
> +	unsigned long message_bar_size;
> +	void __iomem *msg_bar_reg;
> +
> +	struct uart_8250_port uart[VUART_MAX_PARMS];
> +	int uart_line[VUART_MAX_PARMS];
> +
> +	/* Interrupt
> +	 * The index of array is using to enum msi_index
> +	 */
> +	int *msi_idx_table;
> +};
> +
> +static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +
> +	/* Assign static msi index table by platform */
> +	pci_bmc_dev->msi_idx_table = ast2600_msi_idx_table;
> +
> +	if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)
> +		/* Set all msi index to the first vector */
> +		memset(pci_bmc_dev->msi_idx_table, 0, sizeof(int) * MAX_MSI_NUM);
> +}
> +
> +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	u16 vuart_ioport;
> +	int ret, i;
> +
> +	for (i = 0; i < VUART_MAX_PARMS; i++) {
> +		/* Assign the line to non-exist device */
> +		pci_bmc_dev->uart_line[i] = -ENOENT;
> +		vuart_ioport = 0x3F8 - (i * 0x100);
> +		pci_bmc_dev->uart[i].port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
> +		pci_bmc_dev->uart[i].port.uartclk = 115200 * 16;
> +		pci_bmc_dev->uart[i].port.irq =
> +			pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[VUART0_MSI + i]);
> +		pci_bmc_dev->uart[i].port.dev = dev;
> +		pci_bmc_dev->uart[i].port.iotype = UPIO_MEM32;
> +		pci_bmc_dev->uart[i].port.iobase = 0;
> +		pci_bmc_dev->uart[i].port.mapbase =
> +			pci_bmc_dev->message_bar_base + (vuart_ioport << 2);
> +		pci_bmc_dev->uart[i].port.membase = 0;
> +		pci_bmc_dev->uart[i].port.type = PORT_16550A;
> +		pci_bmc_dev->uart[i].port.flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
> +		pci_bmc_dev->uart[i].port.regshift = 2;
> +		ret = serial8250_register_8250_port(&pci_bmc_dev->uart[i]);
> +		if (ret < 0) {
> +			dev_err_probe(dev, ret, "Can't setup PCIe VUART\n");
> +			return ret;
> +		}
> +		pci_bmc_dev->uart_line[i] = ret;
> +	}
> +	return 0;
> +}
> +
> +static void aspeed_pci_host_bmc_device_release_vuart(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < VUART_MAX_PARMS; i++) {
> +		if (pci_bmc_dev->uart_line[i] >= 0)
> +			serial8250_unregister_port(pci_bmc_dev->uart_line[i]);
> +	}
> +}
> +
> +static int aspeed_pci_host_setup(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +	int rc = 0;
> +
> +	/* Get Message BAR */
> +	pci_bmc_dev->message_bar_base = pci_resource_start(pdev, 1);
> +	pci_bmc_dev->message_bar_size = pci_resource_len(pdev, 1);
> +	pci_bmc_dev->msg_bar_reg = pci_ioremap_bar(pdev, 1);
> +	if (!pci_bmc_dev->msg_bar_reg)
> +		return -ENOMEM;
> +
> +	if (pdev->revision < 0x27) {
> +		/* AST2600 ERRTA40: dummy read */

Can you please rather document what problem this is actually solving.

> +		(void)__raw_readl((void __iomem *)pci_bmc_dev->msg_bar_reg);
> +	} else {
> +		/* AST2700 not supported */
> +		pr_err("AST2700 detected but not supported");

This logs an error but rc = 0 on return. Perhaps drop the log message
and return an appropriate error code?

> +		goto out_free0;
> +	}
> +
> +	rc = aspeed_pci_bmc_device_setup_vuart(pdev);
> +	if (rc) {
> +		pr_err("Cannot setup Virtual UART");
> +		goto out_free0;
> +	}
> +
> +	return 0;
> +
> +out_free0:
> +	pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
> +
> +	return rc;
> +}
> +
> +static struct aspeed_platform aspeed_pcie_host[] = {
> +	{ .setup = aspeed_pci_host_setup },
> +	{ 0 }
> +};
> +
> +static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev;
> +	int rc = 0;
> +
> +	pr_info("ASPEED BMC PCI ID %04x:%04x, IRQ=%u\n", pdev->vendor, pdev->device, pdev->irq);

I think we could do without this.

> +
> +	pci_bmc_dev = devm_kzalloc(&pdev->dev, sizeof(*pci_bmc_dev), GFP_KERNEL);
> +	if (!pci_bmc_dev)
> +		return -ENOMEM;
> +
> +	/* Get platform id */
> +	pci_bmc_dev->driver_data = ent->driver_data;
> +	pci_bmc_dev->platform = &aspeed_pcie_host[ent->driver_data];
> +
> +	pci_bmc_dev->id = ida_alloc(&bmc_device_ida, GFP_KERNEL);

This seems unnecessary.

> +	if (pci_bmc_dev->id < 0)
> +		return pci_bmc_dev->id;
> +
> +	rc = pci_enable_device(pdev);
> +	if (rc) {
> +		dev_err(&pdev->dev, "pci_enable_device() returned error %d\n", rc);
> +		return rc;
> +	}
> +
> +	pci_set_master(pdev);
> +	pci_set_drvdata(pdev, pci_bmc_dev);
> +
> +	/* Prepare IRQ resource */
> +	aspeed_pci_setup_irq_resource(pdev);
> +
> +	/* Setup BMC PCI device */
> +	rc = pci_bmc_dev->platform->setup(pdev);

As with patch 1 this indirection seems unnecessary.

> +	if (rc) {
> +		dev_err(&pdev->dev, "ASPEED PCIe Host device returned error %d\n", rc);
> +		pci_free_irq_vectors(pdev);
> +		pci_disable_device(pdev);
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static void aspeed_pci_host_bmc_device_remove(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +
> +	if (pci_bmc_dev->driver_data == ASPEED)

This condition seems unnecessary as the value shouldn't be anything
else.

> +		aspeed_pci_host_bmc_device_release_vuart(pdev);
> +
> +	ida_free(&bmc_device_ida, pci_bmc_dev->id);
> +
> +	pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
> +
> +	pci_free_irq_vectors(pdev);
> +	pci_disable_device(pdev);
> +}
> +
> +/**
> + * This table holds the list of (VendorID,DeviceID) supported by this driver
> + *
> + */

I think that's self-evident and prefer the comment be removed.

> +static struct pci_device_id aspeed_host_bmc_dev_pci_ids[] = {
> +	/* ASPEED BMC Device */
> +	{ PCI_DEVICE(0x1A03, 0x2402), .class = 0xFF0000, .class_mask = 0xFFFF00,
> +	  .driver_data = ASPEED },
> +	{
> +		0,
> +	}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, aspeed_host_bmc_dev_pci_ids);
> +
> +static struct pci_driver aspeed_host_bmc_dev_driver = {
> +	.name		= DRIVER_NAME,
> +	.id_table	= aspeed_host_bmc_dev_pci_ids,
> +	.probe		= aspeed_pci_host_bmc_device_probe,
> +	.remove		= aspeed_pci_host_bmc_device_remove,
> +};
> +
> +static int __init aspeed_host_bmc_device_init(void)
> +{
> +	return pci_register_driver(&aspeed_host_bmc_dev_driver);
> +}
> +
> +static void aspeed_host_bmc_device_exit(void)
> +{
> +	/* unregister pci driver */
> +	pci_unregister_driver(&aspeed_host_bmc_dev_driver);
> +}
> +
> +late_initcall(aspeed_host_bmc_device_init);
> +module_exit(aspeed_host_bmc_device_exit);

module_driver() could be used here.

> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver

[Grégoire Layet]

Hello Andrew,

> It's probably best to use ASPEED's SDK as a source of inspiration for
> fixing obscure bugs, but not send drivers directly extracted from it.

I would like to discuss the different options then.

If we decide to continue on the current path, I'll modify the code according
to your remarks.

> We should avoid adding more drivers in drivers/soc/aspeed where we can.
> Is this really necessary?

This driver (for the BMC side) only enables some configuration on the SCU to
make MSI interrupts work. It is a very specific configuration, which is why I
thought it was OK as a separate driver.

Fundamentally, the BMC side driver is not necessary. During my testing,
I successfully made data flow in both directions without the BMC side driver,
but this was using polling mode (IRQ = 0), it's not ideal.


It is also possible to put the SCU initialisation on the
8250_aspeed_vuart driver
directly. This could be activated with a specific flag added to VUART nodes
('pcie2vuart' for example) on the DeviceTree.

Putting this configuration in the VUART driver directly would make the setup
on the devicetree easier and more understandable. Because in this series, the
bmc side driver needs a bmc_device node to be loaded. But the bmc_dev node
doesn't hold any meaningful information.
If we decide to take this approach, I think we should also add the
differentiation for the ast2600 compatibility. Currently, the aspeed-g6.dtsi
uses the "aspeed,ast2500-vuart" compatibility entry for the four VUARTs.

What do you think about this solution ?


Thanks,
Grégoire


On Wed, 10 Jun 2026 at 14:33, Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> Hello Grégoire,
>
> On Mon, 2026-06-08 at 14:51 +0000, Grégoire Layet wrote:
> > Taken from ASPEED 6.18 Kernel SDK
>
> It's probably best to use ASPEED's SDK as a source of inspiration for
> fixing obscure bugs, but not send drivers directly extracted from it.
>
> >
> > Add support for VUART over PCIe between BMC and host.
> > This add BMC side driver.
> >
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> > Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
> > Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
> > Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
> > ---
> >  drivers/soc/aspeed/Kconfig          |   7 ++
> >  drivers/soc/aspeed/Makefile         |   1 +
> >  drivers/soc/aspeed/aspeed-bmc-dev.c | 187 ++++++++++++++++++++++++++++
>
> We should avoid adding more drivers in drivers/soc/aspeed where we can.
>
> Is this really necessary?
>
> >  3 files changed, 195 insertions(+)
> >  create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c
> >
> > diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> > index f579ee0b5afa..3e1fcf3c3268 100644
> > --- a/drivers/soc/aspeed/Kconfig
> > +++ b/drivers/soc/aspeed/Kconfig
> > @@ -4,6 +4,13 @@ if ARCH_ASPEED || COMPILE_TEST
> >
> >  menu "ASPEED SoC drivers"
> >
> > +config ASPEED_BMC_DEV
> > +     tristate "ASPEED BMC Device"
> > +     default n
> > +     help
> > +       Enable support for the ASPEED AST2600 BMC Device.
> > +       This exposes the PCIe-to-LPC bridge of the BMC to the host over PCIe.
> > +
> >  config ASPEED_LPC_CTRL
> >       tristate "ASPEED LPC firmware cycle control"
> >       select REGMAP
> > diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> > index b35d74592964..fab0d247df66 100644
> > --- a/drivers/soc/aspeed/Makefile
> > +++ b/drivers/soc/aspeed/Makefile
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_ASPEED_BMC_DEV)         += aspeed-bmc-dev.o
> >  obj-$(CONFIG_ASPEED_LPC_CTRL)                += aspeed-lpc-ctrl.o
> >  obj-$(CONFIG_ASPEED_LPC_SNOOP)               += aspeed-lpc-snoop.o
> >  obj-$(CONFIG_ASPEED_UART_ROUTING)    += aspeed-uart-routing.o
> > diff --git a/drivers/soc/aspeed/aspeed-bmc-dev.c b/drivers/soc/aspeed/aspeed-bmc-dev.c
> > new file mode 100644
> > index 000000000000..7a204b543c97
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/aspeed-bmc-dev.c
> > @@ -0,0 +1,187 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +// Copyright (C) ASPEED Technology Inc.
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/regmap.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#define SCU_TRIGGER_MSI
> > +
> > +/* AST2600 SCU */
> > +#define ASPEED_SCU04                 0x04
> > +#define AST2600A3_SCU04                              0x05030303
> > +#define ASPEED_SCUC20                        0xC20
> > +#define ASPEED_SCUC24                        0xC24
>
> These could all use properly descriptive names.
>
> Pinctrl is an exception because of how the documentation is structured.
>
> > +#define MSI_ROUTING_MASK                     GENMASK(11, 10)
> > +#define PCIDEV1_INTX_MSI_HOST2BMC_EN         BIT(18)
> > +#define MSI_ROUTING_PCIe2LPC_PCIDEV0         (0x1 << 10)
> > +#define MSI_ROUTING_PCIe2LPC_PCIDEV1         (0x2 << 10)
> > +
> > +#define ASPEED_SCU_PCIE_CONF_CTRL    0xC20
> > +#define  SCU_PCIE_CONF_BMC_DEV_EN                     BIT(8)
> > +#define  SCU_PCIE_CONF_BMC_DEV_EN_MMIO                BIT(9)
> > +#define  SCU_PCIE_CONF_BMC_DEV_EN_MSI                 BIT(11)
> > +#define  SCU_PCIE_CONF_BMC_DEV_EN_IRQ                 BIT(13)
> > +#define  SCU_PCIE_CONF_BMC_DEV_EN_DMA                 BIT(14)
> > +#define  SCU_PCIE_CONF_BMC_DEV_EN_E2L                 BIT(15)
> > +#define  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE BIT(21)
> > +
> > +#define ASPEED_SCU_BMC_DEV_CLASS     0xC68
> > +
> > +
> > +struct aspeed_platform {
> > +     int (*init)(struct platform_device *pdev);
> > +};
> > +
> > +struct aspeed_bmc_device {
> > +     struct device *dev;
> > +     int id;
> > +     void __iomem *reg_base;
> > +
> > +     int pcie2lpc;
> > +     int irq;
> > +
> > +     const struct aspeed_platform *platform;
> > +
> > +     struct regmap *scu;
> > +     int pcie_irq;
> > +};
> > +
> > +
> > +static int aspeed_ast2600_init(struct platform_device *pdev)
> > +{
> > +     struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
> > +     struct device *dev = &pdev->dev;
> > +     u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
> > +                           SCU_PCIE_CONF_BMC_DEV_EN_MMIO | SCU_PCIE_CONF_BMC_DEV_EN;
> > +     u32 scu_id;
> > +
> > +     bmc_device->scu = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,scu");
>
> We should rather look at auxbus for the SCU.
>
> > +     if (IS_ERR(bmc_device->scu)) {
> > +             dev_err(&pdev->dev, "failed to find SCU regmap\n");
> > +             return PTR_ERR(bmc_device->scu);
> > +     }
> > +
> > +     if (bmc_device->pcie2lpc)
> > +             pcie_config_ctl |= SCU_PCIE_CONF_BMC_DEV_EN_E2L |
> > +                                SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE;
> > +
> > +     regmap_update_bits(bmc_device->scu, ASPEED_SCU_PCIE_CONF_CTRL,
> > +                        pcie_config_ctl, pcie_config_ctl);
> > +
> > +     /* update class code to others as it is a MFD device */
> > +     regmap_write(bmc_device->scu, ASPEED_SCU_BMC_DEV_CLASS, 0xff000000);
> > +
> > +#ifdef SCU_TRIGGER_MSI
>
> I don't see that this needs to be a CPP test. This could be a C test.
> The construct would be optimised because of the constant and we'd get
> compile time coverage of both sides without additional configuration.
>
> Have you tested both sides?
>
> > +     //SCUC24[17]: Enable PCI device 1 INTx/MSI from SCU560[15]. Will be added in next version
> > +     regmap_update_bits(bmc_device->scu, ASPEED_SCUC20, BIT(11) | BIT(14), BIT(11) | BIT(14));
>
> These bits need descriptive macros.
>
> > +
> > +     regmap_read(bmc_device->scu, ASPEED_SCU04, &scu_id);
> > +     if (scu_id == AST2600A3_SCU04)
> > +             regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
> > +                                PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_MASK,
> > +                                PCIDEV1_INTX_MSI_HOST2BMC_EN | MSI_ROUTING_PCIe2LPC_PCIDEV1);
> > +     else
> > +             regmap_update_bits(bmc_device->scu, ASPEED_SCUC24,
> > +                                BIT(17) | BIT(14) | BIT(11), BIT(17) | BIT(14) | BIT(11));
>
> As do these
>
> > +#else
> > +     //SCUC24[18]: Enable PCI device 1 INTx/MSI from Host-to-BMC controller.
> > +     regmap_update_bits(bmc_device->scu, 0xc24, BIT(18) | BIT(14), BIT(18) | BIT(14));
>
> And these.
>
> > +#endif
> > +
> > +
> > +     return 0;
> > +}
> > +
> > +
> > +static struct aspeed_platform ast2600_plaform = {
> > +     .init = aspeed_ast2600_init
> > +};
> > +
> > +
> > +static const struct of_device_id aspeed_bmc_device_of_matches[] = {
> > +     { .compatible = "aspeed,ast2600-bmc-device", .data = &ast2600_plaform },
>
> This compatible isn't documented in this series and isn't present in
> linux-next at a87737435cfa ("Add linux-next specific files for
> 20260608"). You'll need to address that if it's reasonable to continue
> down this path. I expect you'll want to avoid it, and define any
> necessary properties on the SCU node rather than add further children.
>
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_bmc_device_of_matches);
> > +
> > +static int aspeed_bmc_device_probe(struct platform_device *pdev)
> > +{
> > +     struct aspeed_bmc_device *bmc_device;
> > +     struct device *dev = &pdev->dev;
>
> This shortcut is defined but inconsistently used.
>
> > +     const void *md = of_device_get_match_data(dev);
>
> I think we can do without this, see below.
>
> > +     int ret = 0;
> > +
> > +     if (!md)
> > +             return -ENODEV;
> > +
> > +     bmc_device = devm_kzalloc(&pdev->dev, sizeof(struct aspeed_bmc_device), GFP_KERNEL);
> > +     if (!bmc_device)
> > +             return -ENOMEM;
> > +     dev_set_drvdata(dev, bmc_device);
> > +
> > +     bmc_device->platform = md;
> > +
> > +     bmc_device->id = of_alias_get_id(dev->of_node, "bmcdev");
> > +     if (bmc_device->id < 0)
> > +             bmc_device->id = 0;
> > +
> > +     bmc_device->dev = dev;
> > +     bmc_device->reg_base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(bmc_device->reg_base))
> > +             return PTR_ERR(bmc_device->reg_base);
> > +
> > +     bmc_device->irq = platform_get_irq(pdev, 0);
>
> This seems unnecessary.
>
> > +     if (bmc_device->irq < 0) {
> > +             dev_err(&pdev->dev, "platform get of irq[=%d] failed!\n", bmc_device->irq);
> > +             return bmc_device->irq;
> > +     }
> > +
> > +     if (of_property_read_bool(dev->of_node, "pcie2lpc"))
>
> This property isn't documented.
>
> > +             bmc_device->pcie2lpc = 1;
> > +
> > +     ret = bmc_device->platform->init(pdev);
>
> The driver only supports one SoC, this indirection seems unnecessary
> right now. We can add that later when there's a need to differentiate.
> I'd rather you call the setup function directly for now.
>
> > +     if (ret) {
> > +             dev_err(dev, "Initialize bmc device failed\n");
> > +             goto out;
> > +     }
> > +
> > +     dev_info(dev, "aspeed bmc device: driver successfully loaded.\n");
> > +
> > +     return 0;
> > +
> > +out:
> > +     dev_warn(dev, "aspeed bmc device: driver init failed (ret=%d)!\n", ret);
> > +     return ret;
> > +}
> > +
> > +static void aspeed_bmc_device_remove(struct platform_device *pdev)
> > +{
> > +     struct aspeed_bmc_device *bmc_device = platform_get_drvdata(pdev);
> > +
> > +     devm_free_irq(&pdev->dev, bmc_device->irq, bmc_device);
> > +     devm_kfree(&pdev->dev, bmc_device);
>
> These are unnecessary due to cleanup of devres on release.
>
> Andrew

Re: [PATCH v2 2/2] soc: aspeed: add host-side PCIe BMC device driver

[Grégoire Layet]

Hello Andrew,

> Again, I'd rather we avoid drivers/soc/aspeed.

If creating a new specific driver is the right move, where should it go if
not in drivers/soc/aspeed ?
But again, considering all you remarks, maybe this driver is unnecessary.

As initially the driver was doing all the PCI BMC device functionality
provided by ASPEED (shared memory, VUART, message queue and doorbell),
it made sense to have everything in a driver for this specific case.

However for a TTY-only driver, that might be excessive.
When I trimmed down the driver, I didn't consider whether the driver itself
was still necessary.
I think adding a new driver is not the right solution for this.

From my research, the ASPEED PCIe device could be added to the 8250_pci driver.
The ASPEED PCIe device has a specific device ID and Vendor ID.

What do you think about this?


Thanks,
Grégoire

On Wed, 10 Jun 2026 at 14:51, Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> On Mon, 2026-06-08 at 14:51 +0000, Grégoire Layet wrote:
> > Taken from ASPEED 6.18 Kernel SDK
> >
> > Add support for VUART over PCIe between BMC and host.
> > This add host side driver.
> >
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> > Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
> > Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
> > Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
> > ---
> >  drivers/soc/aspeed/Kconfig               |   8 +
> >  drivers/soc/aspeed/Makefile              |   1 +
> >  drivers/soc/aspeed/aspeed-host-bmc-dev.c | 249 +++++++++++++++++++++++
>
> Again, I'd rather we avoid drivers/soc/aspeed.
>
> >  3 files changed, 258 insertions(+)
> >  create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c
> >
> > diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> > index 3e1fcf3c3268..5deefb64e8c7 100644
> > --- a/drivers/soc/aspeed/Kconfig
> > +++ b/drivers/soc/aspeed/Kconfig
> > @@ -11,6 +11,14 @@ config ASPEED_BMC_DEV
> >         Enable support for the ASPEED AST2600 BMC Device.
> >         This exposes the PCIe-to-LPC bridge of the BMC to the host over PCIe.
> >
> > +config ASPEED_HOST_BMC_DEV
> > +     tristate "ASPEED Host BMC Device"
> > +     depends on PCI
> > +     depends on SERIAL_8250
> > +     help
> > +       Enable support for the ASPEED AST2600 BMC Device on the Host.
> > +       This configure the PCIe and setup two 8250 compatible VUART ports.
> > +
> >  config ASPEED_LPC_CTRL
> >       tristate "ASPEED LPC firmware cycle control"
> >       select REGMAP
> > diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> > index fab0d247df66..3fd3f6d8d36e 100644
> > --- a/drivers/soc/aspeed/Makefile
> > +++ b/drivers/soc/aspeed/Makefile
> > @@ -1,5 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  obj-$(CONFIG_ASPEED_BMC_DEV)         += aspeed-bmc-dev.o
> > +obj-$(CONFIG_ASPEED_HOST_BMC_DEV)    += aspeed-host-bmc-dev.o
> >  obj-$(CONFIG_ASPEED_LPC_CTRL)                += aspeed-lpc-ctrl.o
> >  obj-$(CONFIG_ASPEED_LPC_SNOOP)               += aspeed-lpc-snoop.o
> >  obj-$(CONFIG_ASPEED_UART_ROUTING)    += aspeed-uart-routing.o
> > diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> > new file mode 100644
> > index 000000000000..7cb52a770fb6
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +// Copyright (C) ASPEED Technology Inc.
> > +
> > +#include <linux/init.h>
> > +#include <linux/version.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/pci.h>
> > +#include <linux/serial_core.h>
> > +#include <linux/serial_8250.h>
> > +
> > +static DEFINE_IDA(bmc_device_ida);
> > +
> > +#define VUART_MAX_PARMS      2
>
> Given the one supported piece of hardware we could avoid the associated
> loops and rather extract loop bodies to functions and call the function
> twice.
>
> > +#define MAX_MSI_NUM          8
> > +#define BMC_MULTI_MSI        32
> > +
> > +#define DRIVER_NAME "aspeed-host-bmc-dev"
> > +
> > +enum aspeed_platform_id {
> > +     ASPEED,
> > +};
> > +
> > +enum msi_index {
> > +     VUART0_MSI,
> > +     VUART1_MSI,
> > +};
> > +
> > +/* Match msi_index */
> > +static int ast2600_msi_idx_table[MAX_MSI_NUM] = { 16, 15 };
> > +
> > +struct aspeed_platform {
> > +     int (*setup)(struct pci_dev *pdev);
> > +};
> > +
> > +struct aspeed_pci_bmc_dev {
> > +     struct device *dev;
> > +     struct aspeed_platform *platform;
> > +     kernel_ulong_t driver_data;
> > +     int id;
> > +
> > +     unsigned long message_bar_base;
> > +     unsigned long message_bar_size;
> > +     void __iomem *msg_bar_reg;
> > +
> > +     struct uart_8250_port uart[VUART_MAX_PARMS];
> > +     int uart_line[VUART_MAX_PARMS];
> > +
> > +     /* Interrupt
> > +      * The index of array is using to enum msi_index
> > +      */
> > +     int *msi_idx_table;
> > +};
> > +
> > +static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
> > +{
> > +     struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > +
> > +     /* Assign static msi index table by platform */
> > +     pci_bmc_dev->msi_idx_table = ast2600_msi_idx_table;
> > +
> > +     if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)
> > +             /* Set all msi index to the first vector */
> > +             memset(pci_bmc_dev->msi_idx_table, 0, sizeof(int) * MAX_MSI_NUM);
> > +}
> > +
> > +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev)
> > +{
> > +     struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > +     struct device *dev = &pdev->dev;
> > +     u16 vuart_ioport;
> > +     int ret, i;
> > +
> > +     for (i = 0; i < VUART_MAX_PARMS; i++) {
> > +             /* Assign the line to non-exist device */
> > +             pci_bmc_dev->uart_line[i] = -ENOENT;
> > +             vuart_ioport = 0x3F8 - (i * 0x100);
> > +             pci_bmc_dev->uart[i].port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
> > +             pci_bmc_dev->uart[i].port.uartclk = 115200 * 16;
> > +             pci_bmc_dev->uart[i].port.irq =
> > +                     pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[VUART0_MSI + i]);
> > +             pci_bmc_dev->uart[i].port.dev = dev;
> > +             pci_bmc_dev->uart[i].port.iotype = UPIO_MEM32;
> > +             pci_bmc_dev->uart[i].port.iobase = 0;
> > +             pci_bmc_dev->uart[i].port.mapbase =
> > +                     pci_bmc_dev->message_bar_base + (vuart_ioport << 2);
> > +             pci_bmc_dev->uart[i].port.membase = 0;
> > +             pci_bmc_dev->uart[i].port.type = PORT_16550A;
> > +             pci_bmc_dev->uart[i].port.flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
> > +             pci_bmc_dev->uart[i].port.regshift = 2;
> > +             ret = serial8250_register_8250_port(&pci_bmc_dev->uart[i]);
> > +             if (ret < 0) {
> > +                     dev_err_probe(dev, ret, "Can't setup PCIe VUART\n");
> > +                     return ret;
> > +             }
> > +             pci_bmc_dev->uart_line[i] = ret;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static void aspeed_pci_host_bmc_device_release_vuart(struct pci_dev *pdev)
> > +{
> > +     struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > +     int i;
> > +
> > +     for (i = 0; i < VUART_MAX_PARMS; i++) {
> > +             if (pci_bmc_dev->uart_line[i] >= 0)
> > +                     serial8250_unregister_port(pci_bmc_dev->uart_line[i]);
> > +     }
> > +}
> > +
> > +static int aspeed_pci_host_setup(struct pci_dev *pdev)
> > +{
> > +     struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > +     int rc = 0;
> > +
> > +     /* Get Message BAR */
> > +     pci_bmc_dev->message_bar_base = pci_resource_start(pdev, 1);
> > +     pci_bmc_dev->message_bar_size = pci_resource_len(pdev, 1);
> > +     pci_bmc_dev->msg_bar_reg = pci_ioremap_bar(pdev, 1);
> > +     if (!pci_bmc_dev->msg_bar_reg)
> > +             return -ENOMEM;
> > +
> > +     if (pdev->revision < 0x27) {
> > +             /* AST2600 ERRTA40: dummy read */
>
> Can you please rather document what problem this is actually solving.
>
> > +             (void)__raw_readl((void __iomem *)pci_bmc_dev->msg_bar_reg);
> > +     } else {
> > +             /* AST2700 not supported */
> > +             pr_err("AST2700 detected but not supported");
>
> This logs an error but rc = 0 on return. Perhaps drop the log message
> and return an appropriate error code?
>
> > +             goto out_free0;
> > +     }
> > +
> > +     rc = aspeed_pci_bmc_device_setup_vuart(pdev);
> > +     if (rc) {
> > +             pr_err("Cannot setup Virtual UART");
> > +             goto out_free0;
> > +     }
> > +
> > +     return 0;
> > +
> > +out_free0:
> > +     pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
> > +
> > +     return rc;
> > +}
> > +
> > +static struct aspeed_platform aspeed_pcie_host[] = {
> > +     { .setup = aspeed_pci_host_setup },
> > +     { 0 }
> > +};
> > +
> > +static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > +{
> > +     struct aspeed_pci_bmc_dev *pci_bmc_dev;
> > +     int rc = 0;
> > +
> > +     pr_info("ASPEED BMC PCI ID %04x:%04x, IRQ=%u\n", pdev->vendor, pdev->device, pdev->irq);
>
> I think we could do without this.
>
> > +
> > +     pci_bmc_dev = devm_kzalloc(&pdev->dev, sizeof(*pci_bmc_dev), GFP_KERNEL);
> > +     if (!pci_bmc_dev)
> > +             return -ENOMEM;
> > +
> > +     /* Get platform id */
> > +     pci_bmc_dev->driver_data = ent->driver_data;
> > +     pci_bmc_dev->platform = &aspeed_pcie_host[ent->driver_data];
> > +
> > +     pci_bmc_dev->id = ida_alloc(&bmc_device_ida, GFP_KERNEL);
>
> This seems unnecessary.
>
> > +     if (pci_bmc_dev->id < 0)
> > +             return pci_bmc_dev->id;
> > +
> > +     rc = pci_enable_device(pdev);
> > +     if (rc) {
> > +             dev_err(&pdev->dev, "pci_enable_device() returned error %d\n", rc);
> > +             return rc;
> > +     }
> > +
> > +     pci_set_master(pdev);
> > +     pci_set_drvdata(pdev, pci_bmc_dev);
> > +
> > +     /* Prepare IRQ resource */
> > +     aspeed_pci_setup_irq_resource(pdev);
> > +
> > +     /* Setup BMC PCI device */
> > +     rc = pci_bmc_dev->platform->setup(pdev);
>
> As with patch 1 this indirection seems unnecessary.
>
> > +     if (rc) {
> > +             dev_err(&pdev->dev, "ASPEED PCIe Host device returned error %d\n", rc);
> > +             pci_free_irq_vectors(pdev);
> > +             pci_disable_device(pdev);
> > +             return rc;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void aspeed_pci_host_bmc_device_remove(struct pci_dev *pdev)
> > +{
> > +     struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> > +
> > +     if (pci_bmc_dev->driver_data == ASPEED)
>
> This condition seems unnecessary as the value shouldn't be anything
> else.
>
> > +             aspeed_pci_host_bmc_device_release_vuart(pdev);
> > +
> > +     ida_free(&bmc_device_ida, pci_bmc_dev->id);
> > +
> > +     pci_iounmap(pdev, pci_bmc_dev->msg_bar_reg);
> > +
> > +     pci_free_irq_vectors(pdev);
> > +     pci_disable_device(pdev);
> > +}
> > +
> > +/**
> > + * This table holds the list of (VendorID,DeviceID) supported by this driver
> > + *
> > + */
>
> I think that's self-evident and prefer the comment be removed.
>
> > +static struct pci_device_id aspeed_host_bmc_dev_pci_ids[] = {
> > +     /* ASPEED BMC Device */
> > +     { PCI_DEVICE(0x1A03, 0x2402), .class = 0xFF0000, .class_mask = 0xFFFF00,
> > +       .driver_data = ASPEED },
> > +     {
> > +             0,
> > +     }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(pci, aspeed_host_bmc_dev_pci_ids);
> > +
> > +static struct pci_driver aspeed_host_bmc_dev_driver = {
> > +     .name           = DRIVER_NAME,
> > +     .id_table       = aspeed_host_bmc_dev_pci_ids,
> > +     .probe          = aspeed_pci_host_bmc_device_probe,
> > +     .remove         = aspeed_pci_host_bmc_device_remove,
> > +};
> > +
> > +static int __init aspeed_host_bmc_device_init(void)
> > +{
> > +     return pci_register_driver(&aspeed_host_bmc_dev_driver);
> > +}
> > +
> > +static void aspeed_host_bmc_device_exit(void)
> > +{
> > +     /* unregister pci driver */
> > +     pci_unregister_driver(&aspeed_host_bmc_dev_driver);
> > +}
> > +
> > +late_initcall(aspeed_host_bmc_device_init);
> > +module_exit(aspeed_host_bmc_device_exit);
>
> module_driver() could be used here.
>
> > +
> > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> > +MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
> > +MODULE_LICENSE("GPL");

Re: [PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver

[Grégoire Layet]

Hello Andrew,

Anirudh Srinivasan and I have found that IPMI over KCS using the
PCI worked with the stock ASPEED bmc driver (the bmc driver in the V1)
but not with the trimmed-down version in the V2. I have apparently removed
a bit too much from the V2 , but that's not what I want to focus on.

This brings back the question of where we should put the registers
configuration,
considering that two different functionalities depend on it.

> It is also possible to put the SCU initialisation on the
> 8250_aspeed_vuart driver
> directly. This could be activated with a specific flag added to VUART nodes
> ('pcie2vuart' for example) on the DeviceTree.

Similarly to this idea, we could include have the necessary configuration in the
'kcs_bmc_aspeed' driver. This could be activated using a similar flag
,such as 'pci2lpc'
or 'pci2kcs' directly. However, this would result in a lot of code
duplication for most
of the configuration.

The issue for me is that, two drivers configuring the same registers
is not a good idea.
I don't know how to do this the proper way.

I wanted to add this information because it could influence the move to a
bmc_device driver that configures both VUART and KCS, over PCI.
This would be similar to the V2 bmc driver, and I fix the KCS bug.

Thanks,
Grégoire

Re: [PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver

[Andrew Jeffery]

On Fri, 2026-06-12 at 11:21 +0200, Grégoire Layet wrote:
> Hello Andrew,
> 
> Anirudh Srinivasan and I have found that IPMI over KCS using the
> PCI worked with the stock ASPEED bmc driver (the bmc driver in the V1)
> but not with the trimmed-down version in the V2. I have apparently removed
> a bit too much from the V2 , but that's not what I want to focus on.

Sure.

> 
> This brings back the question of where we should put the registers
> configuration,
> considering that two different functionalities depend on it.
> 
> > It is also possible to put the SCU initialisation on the
> > 8250_aspeed_vuart driver
> > directly. This could be activated with a specific flag added to VUART nodes
> > ('pcie2vuart' for example) on the DeviceTree.

The concept sounds reasonable to me. There's probably some bikeshedding
to do on the devicetree property though.

> 
> Similarly to this idea, we could include have the necessary configuration in the
> 'kcs_bmc_aspeed' driver. This could be activated using a similar flag
> ,such as 'pci2lpc'
> or 'pci2kcs' directly. However, this would result in a lot of code
> duplication for most
> of the configuration.

Can you outline the duplication you're concerned about? I think it's a
matter of resolving the SCU syscon to its regmap, then performing the
necessary accesses?

> 
> The issue for me is that, two drivers configuring the same registers
> is not a good idea.

I think it's not as bad as you make it out to be. The SCU's regmap
protects updates to individual registers under a lock, so concurrent
modification isn't a concern. The hardware design choices make all of
this slightly awkward for any related software design. As an
alternative you could implement a mini subsystem that relevant drivers
could call through to set the bits, but I currently think that's
unnecessary work.

Andrew

Re: [PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver

[Grégoire Layet]

Hello Andrew,

> The concept sounds reasonable to me. There's probably some bikeshedding
> to do on the devicetree property though.

Yes, having looked at how it's done, I would say :
'aspeed,vuart-over-pci' and 'aspeed,kcs-over-pci' flags would be
better.

> Can you outline the duplication you're concerned about? I think it's a
> matter of resolving the SCU syscon to its regmap, then performing the
> necessary accesses?

Both drivers will need to set :
- Enable PCI BMC Device MMIO
- Enable PCI BMC Device IRQ
- Enable PCI BMC Device MSI rooting over PCI Device 1 (BAR1)
- Enable Host 2 BMC MSI interrupts
- PCI device class to 0xff000000 to be identified as a MFD device. The
reset default is 0x0C070100 which is an IPMI KCS device, but that
causes issues as it is detected by ipmi_si but can't be loaded because
of non default KCS address.

Sorry for my errors, there is not that much. But both drivers will do
almost the same initialisation. That was my code duplication concern.

> I think it's not as bad as you make it out to be. The SCU's regmap
> protects updates to individual registers under a lock, so concurrent
> modification isn't a concern. The hardware design choices make all of
> this slightly awkward for any related software design. As an
> alternative you could implement a mini subsystem that relevant drivers
> could call through to set the bits, but I currently think that's
> unnecessary work.

You are right it's not as bad as I thought.
For now, I will focus on the VUART until the solution has been
validated. Then I will easily do the same for the KCS over PCI.

So I'll do for the V3 of the BMC side driver:
- modify the device tree binding to have 'aspeed,ast2600-vuart' and
add the 'aspeed,vuart-over-pci' boolean flag, only for the ast2600.
- modify the '8250_aspeed_vuart' driver to add 'aspeed,ast2600-vuart' support.
- add vuart over pci enable and disable code to the '8250_aspeed_vuart' driver.

Regards,
Grégoire

Re: [PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver

[Andrew Jeffery]

Hi Grégoire,

On Wed, 2026-06-17 at 08:40 +0200, Grégoire Layet wrote:
> Hello Andrew,
> 
> > The concept sounds reasonable to me. There's probably some bikeshedding
> > to do on the devicetree property though.
> 
> Yes, having looked at how it's done, I would say :
> 'aspeed,vuart-over-pci' and 'aspeed,kcs-over-pci' flags would be
> better.
> 
> > Can you outline the duplication you're concerned about? I think it's a
> > matter of resolving the SCU syscon to its regmap, then performing the
> > necessary accesses?
> 
> Both drivers will need to set :
> - Enable PCI BMC Device MMIO
> - Enable PCI BMC Device IRQ
> - Enable PCI BMC Device MSI rooting over PCI Device 1 (BAR1)
> - Enable Host 2 BMC MSI interrupts
> - PCI device class to 0xff000000 to be identified as a MFD device. The
> reset default is 0x0C070100 which is an IPMI KCS device, but that
> causes issues as it is detected by ipmi_si but can't be loaded because
> of non default KCS address.
> 
> Sorry for my errors, there is not that much. But both drivers will do
> almost the same initialisation. That was my code duplication concern.

I think it's valid to be concerned, but perhaps not for the reason of
code duplication. If there are multiple consumers then we need to
ensure consistency of configuration and correctness wrt to enabling /
disabling the capability based on the number of consumers.

> 
> > I think it's not as bad as you make it out to be. The SCU's regmap
> > protects updates to individual registers under a lock, so concurrent
> > modification isn't a concern. The hardware design choices make all of
> > this slightly awkward for any related software design. As an
> > alternative you could implement a mini subsystem that relevant drivers
> > could call through to set the bits, but I currently think that's
> > unnecessary work.
> 
> You are right it's not as bad as I thought.
> For now, I will focus on the VUART until the solution has been
> validated. Then I will easily do the same for the KCS over PCI.

I think it's a good step to at least solve one thing at a time, so long
as we're not precluding making those future steps.

> 
> So I'll do for the V3 of the BMC side driver:
> - modify the device tree binding to have 'aspeed,ast2600-vuart' and
> add the 'aspeed,vuart-over-pci' boolean flag, only for the ast2600.

Just to confirm, you're proposing modifying the 8250 binding?

> - modify the '8250_aspeed_vuart' driver to add 'aspeed,ast2600-vuart' support.
> - add vuart over pci enable and disable code to the '8250_aspeed_vuart' driver.
> 

Sounds like a reasonable start to me.

Andrew

[PATCH v3 0/7] soc: aspeed: Add BMC and host driver for PCIe BMC device

[Grégoire Layet]

This is a v3 for upstreaming the VUART over PCIe BMC device driver.
The initial driver is from the ASPEED kernel SDK (master-v6.18) [1].

There are two drivers: a BMC-side driver and a host-side driver.
Together they enable host<->BMC VUART communication via PCIe.

The host cannot access the BMC's memory. Only the enabled features are
accessible. These are the KCS4 channel and 2 VUARTs. There is also some
mailbox register functionality also exist for a communication between
the host and the BMC. More information can be found here [2].

This v3 mainly modifies the BMC driver and focuses on VUART.
The BMC driver is now incorporated into the '8250_aspeed_vuart' driver.
A specific flag can be set to indicate that the VUART should be used
over PCI. Several changes have been made to the 8250 device tree binding
and the 'aspeed-g6.dtsi'.

Changes since v2 [3]:
- Add the aspeed,ast2600-vuart compatible entry to the '8250' DT binding
- Add the aspeed,ast2600-vuart compatible property in 'aspeed-g6.dtsi'
- Add the aspeed,vuart-over-pci boolean property to the '8250' DT binding,
  only for the aspeed,ast2600-vuart
- Add the aspeed,vuart-over-pci flag to the vuart3 and vuart4 
- Add the aspeed,ast2600-vuart compatible property to the 
  '8250_aspeed_vuart' driver
- Add the VUART over PCI code to the '8250_aspeed_vuart' driver
- The v2 review of the host-side BMC driver has been applied.

The host-side driver is still in /soc/aspeed/, as it is very specific to
this SoC for me. I didn't receive any feedback on where to put this
driver. I can, of course, change this to the relevant location.

It's important to consider that the host driver will do multiple
functions. The AST2600 also supports LPC over PCI, with a specific KCS
channel (KCS4). This driver should also be used to enable the IPMI
automatically via this KCS channel. The UART and the IPMI will depend on
the same PCI resource (BAR1), so this must be configured in one driver.

As with v2, VUART data flow and MSI interrupts have been verified 
working on the test hardware.

Tested on:
BMC:
- Asus IPMI Kommando Card R1.01, AST2600 A3.
- OpenBMC
Host:
- Linux kernel v7.0.0

This v3 only supports AST2600; the AST2700 is not supported by this series.

I would like to know whether I should add the 'lpc-io-reg' and
'lpc-interrupt' values to the vuart3 and vuart4 nodes directly in the
'aspeed-g6.dtsi'. The host driver is not capable of finding the vuart
address on his own, so they are hardcoded to 0x3f8 and 0x2f8. It will
not work with other adresses, so perhaps they should be in the .dtsi to
ensure the correct configuration for the 2 vuart over PCI.

For the interrupt number, my test is working with interrupt = 0
for vuart3 and interrupt = 1 for vuart4. I don't fully understand how
the silicon routes MSI numbers to the VUART but the following
combination is working :
       | host MSI idx | BMC lpc-interrupts |
VUART3 |      16      |       0            |
VUART4 |      17      |       1            |

The original ASPEED driver used MSI index 15 for the VUART4.
I tested every lpc-interrupts on the BMC from 0 to 15, but none of them
worked with the host MSI index set to 15.

For me, the silicon only routes the MSI index 16 to VUART3 and 17 to 
VUART4, and the lpc-interrupt needs to match the 4 least significant bits.
I might be wrong on this explanation but the data path is working with 
those numbers. There is no explanation for any of this in the datasheet.

[1]: https://github.com/AspeedTech-BMC/linux/tree/aspeed-master-v6.18/drivers/soc/aspeed
[2]: https://lore.kernel.org/linux-aspeed/CAFi2wKYOAotiezepDqaR5PZDqDaPKKDfAEnpx5EHC0mL39hy6w@mail.gmail.com/
[3]: https://lore.kernel.org/linux-aspeed/cover.1780929570.git.gregoire.layet@9elements.com/

Grégoire Layet (7):
  dt-bindings: serial: 8250: aspeed: add compatible string for ast2600
  dt-bindings: serial: 8250: aspeed: add aspeed,vuart-over-pci bool prop
  serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string
  serial: 8250_aspeed_vuart: add VUART over PCI
  soc: aspeed: add host-side PCIe BMC device driver
  ARM: dts: aspeed: g6: Change vuart compatible string for ast2600
  ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4

 .../devicetree/bindings/serial/8250.yaml      |  35 +++-
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi       |  10 +-
 drivers/soc/aspeed/Kconfig                    |   8 +
 drivers/soc/aspeed/Makefile                   |   1 +
 drivers/soc/aspeed/aspeed-host-bmc-dev.c      | 183 ++++++++++++++++++
 drivers/tty/serial/8250/8250_aspeed_vuart.c   |  87 +++++++++
 6 files changed, 312 insertions(+), 12 deletions(-)
 create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c

-- 
2.54.0

[PATCH v3 1/7] dt-bindings: serial: 8250: aspeed: add compatible string for ast2600

[Grégoire Layet]

The ast2600 was using the ast2500 vuart compatible string.
This change makes it possible to have ast2600-specific properties.

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 .../devicetree/bindings/serial/8250.yaml      | 20 +++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index bb7b9c87a807..3cbd0f532e15 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -23,7 +23,9 @@ allOf:
     then:
       properties:
         compatible:
-          const: aspeed,ast2500-vuart
+          anyOf:
+            - const: aspeed,ast2500-vuart
+            - const: aspeed,ast2600-vuart
   - if:
       properties:
         compatible:
@@ -287,17 +289,19 @@ properties:
   aspeed,sirq-polarity-sense:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     description: |
-      Phandle to aspeed,ast2500-scu compatible syscon alongside register
-      offset and bit number to identify how the SIRQ polarity should be
-      configured. One possible data source is the LPC/eSPI mode bit. Only
-      applicable to aspeed,ast2500-vuart.
+      Phandle to aspeed,ast2500-scu or aspeed,ast2600-scu compatible syscon
+      alongside register offset and bit number to identify how the SIRQ
+      polarity should be configured. One possible data source is the LPC/eSPI
+      mode bit. Only applicable to aspeed,ast2500-vuart and
+      aspeed,ast2600-vuart.
     deprecated: true
 
   aspeed,lpc-io-reg:
     $ref: /schemas/types.yaml#/definitions/uint32-array
     maxItems: 1
     description: |
-      The VUART LPC address.  Only applicable to aspeed,ast2500-vuart.
+      The VUART LPC address. Only applicable to aspeed,ast2500-vuart and
+      aspeed,ast2600-vuart.
 
   aspeed,lpc-interrupts:
     $ref: /schemas/types.yaml#/definitions/uint32-array
@@ -305,8 +309,8 @@ properties:
     maxItems: 2
     description: |
       A 2-cell property describing the VUART SIRQ number and SIRQ
-      polarity (IRQ_TYPE_LEVEL_LOW or IRQ_TYPE_LEVEL_HIGH).  Only
-      applicable to aspeed,ast2500-vuart.
+      polarity (IRQ_TYPE_LEVEL_LOW or IRQ_TYPE_LEVEL_HIGH). Only
+      applicable to aspeed,ast2500-vuart and aspeed,ast2600-vuart.
 
 required:
   - reg
-- 
2.54.0

[PATCH v3 2/7] dt-bindings: serial: 8250: aspeed: add aspeed,vuart-over-pci bool prop

[Grégoire Layet]

The ASPEED AST2600 has 2 VUART accessible over PCI.
This boolean can be set to specify if the VUART is used over PCI.

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 .../devicetree/bindings/serial/8250.yaml          | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index 3cbd0f532e15..b03797f4674d 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -26,6 +26,14 @@ allOf:
           anyOf:
             - const: aspeed,ast2500-vuart
             - const: aspeed,ast2600-vuart
+  - if:
+      anyOf:
+        - required:
+            - aspeed,vuart-over-pci
+    then:
+      properties:
+        compatible:
+          const: aspeed,ast2600-vuart
   - if:
       properties:
         compatible:
@@ -312,6 +320,13 @@ properties:
       polarity (IRQ_TYPE_LEVEL_LOW or IRQ_TYPE_LEVEL_HIGH). Only
       applicable to aspeed,ast2500-vuart and aspeed,ast2600-vuart.
 
+  aspeed,vuart-over-pci:
+    type: boolean
+    default: false
+    description: |
+      Enable the VUART over the BMC PCI device. Only applicable to
+      aspeed,ast2600-vuart.
+
 required:
   - reg
   - interrupts
-- 
2.54.0

[PATCH v3 3/7] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

[Grégoire Layet]

Makes the driver compatible with the ast2600-vuart.
This enables specific configuration for the AST2600.

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 26fc0464f1cc..6afa2f4057e1 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -560,6 +560,7 @@ static void aspeed_vuart_remove(struct platform_device *pdev)
 static const struct of_device_id aspeed_vuart_table[] = {
 	{ .compatible = "aspeed,ast2400-vuart" },
 	{ .compatible = "aspeed,ast2500-vuart" },
+	{ .compatible = "aspeed,ast2600-vuart" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, aspeed_vuart_table);
-- 
2.54.0

[PATCH v3 4/7] serial: 8250_aspeed_vuart: add VUART over PCI

[Grégoire Layet]

This patch enables the VUART over PCI possible for the AST2600. This is
only activated if the 'aspeed,vuart-over-pci' property flag is set on an
'ast2600-vuart' compatible node.

The AST2600 has 2 VUART that are usable over PCI. These are the VUART3
and VUART4 in the 'apseed-g6.dtsi'.

This code sets the BMC PCI device enables
bits, sets the PCI class code to MFD device and configures MSI interrupts.

There is no disable function. Removing this driver should not disable
the BMC PCI device, as other drivers could use it.
However, if all the drivers using it are removed, the
BMC PCI device will still be activated, which is not ideal. Realistically though, this is not a
use case for a BMC, the drivers will never be removed.

This is useful on PCIe BMC expansion cards that use the AST2600, such as the
ASUS Kommando IPMI Expansion Card.

Registers initialisation taken from ASPEED 6.18 Kernel SDK.
Return code checks were added to each register write.
The code has been simplified and macros have been added.

The ASPEED_SCUC24 regmap update is missing a macro for 'BIT(14)'. I was
unable to  determine the purpose of this bit. In the AST2600 A3
datasheet it is marked as 'reserved'. It is only used on the other
revision. As I only have the AST2600A3, I was unable to try this code
path.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 86 +++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 6afa2f4057e1..e204e26fa173 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -32,6 +32,27 @@
 #define ASPEED_VUART_DEFAULT_SIRQ	4
 #define ASPEED_VUART_DEFAULT_SIRQ_POLARITY	IRQ_TYPE_LEVEL_LOW
 
+#define ASPEED_SCU_SILICON_REVISION_ID			0x04
+#define AST2600A3_REVISION_ID				0x05030303
+
+#define ASPEED_SCUC24			0xC24
+#define  ASPEED_SCUC24_MSI_ROUTING_MASK			GENMASK(11, 10)
+#define  ASPEED_SCUC24_MSI_ROUTING_PCIe2LPC_PCIDEV1		(0x2 << 10)
+#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN		BIT(18)
+#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN			BIT(17)
+
+
+#define ASPEED_SCU_PCIE_CONF_CTRL	0xC20
+#define  SCU_PCIE_CONF_BMC_DEV_EN					BIT(8)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_MMIO				BIT(9)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_MSI				BIT(11)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_IRQ				BIT(13)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER	BIT(14)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_E2L				BIT(15)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE		BIT(21)
+
+#define ASPEED_SCU_BMC_DEV_CLASS	0xC68
+
 struct aspeed_vuart {
 	struct device		*dev;
 	int			line;
@@ -412,6 +433,62 @@ static int aspeed_vuart_map_irq_polarity(u32 dt)
 	}
 }
 
+static int aspeed_ast2600_vuart_over_pci_set_enabled(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	u32 silicon_revision_id;
+	struct regmap *scu;
+	int rc;
+
+	u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
+						SCU_PCIE_CONF_BMC_DEV_EN_MMIO |
+						SCU_PCIE_CONF_BMC_DEV_EN_MSI |
+						SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER |
+						SCU_PCIE_CONF_BMC_DEV_EN_E2L |
+						SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE |
+						SCU_PCIE_CONF_BMC_DEV_EN;
+
+	scu = syscon_regmap_lookup_by_phandle(dev->of_node, "clocks");
+	if (IS_ERR(scu)) {
+		dev_err(&pdev->dev, "failed to find SCU regmap\n");
+		return PTR_ERR(scu);
+	}
+
+	/* update class code to be an MFD device */
+	if (regmap_write(scu, ASPEED_SCU_BMC_DEV_CLASS, 0xff000000)) {
+		dev_err(dev, "could not set PCI class code\n");
+		return -EIO;
+	}
+
+	if (regmap_update_bits(scu, ASPEED_SCU_PCIE_CONF_CTRL,
+			   pcie_config_ctl, pcie_config_ctl)) {
+		dev_err(dev, "could not set PCIe configuration\n");
+		return -EIO;
+	}
+
+	if (regmap_read(scu, ASPEED_SCU_SILICON_REVISION_ID, &silicon_revision_id)) {
+		dev_err(dev, "could not read silicon revision\n");
+		return -EIO;
+	}
+
+	if (silicon_revision_id == AST2600A3_REVISION_ID)
+		rc = regmap_update_bits(scu, ASPEED_SCUC24,
+				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN | ASPEED_SCUC24_MSI_ROUTING_MASK,
+				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN | ASPEED_SCUC24_MSI_ROUTING_PCIe2LPC_PCIDEV1);
+	else
+		rc = regmap_update_bits(scu, ASPEED_SCUC24,
+					/* The bit 14 is reserved in the Datasheet, so we can't say what it does. This revision has not been tested */
+				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN | BIT(14) | ASPEED_SCUC24_MSI_ROUTING_MASK,
+				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN | BIT(14) | ASPEED_SCUC24_MSI_ROUTING_PCIe2LPC_PCIDEV1);
+	if (rc) {
+		dev_err(dev, "could not set PCI device 1 MSI interrupt routing\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
 	struct of_phandle_args sirq_polarity_sense_args;
@@ -540,6 +617,15 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 	aspeed_vuart_set_host_tx_discard(vuart, true);
 	platform_set_drvdata(pdev, vuart);
 
+	if (of_device_is_compatible(dev->of_node, "aspeed,ast2600-vuart") &&
+		of_property_read_bool(dev->of_node, "aspeed,vuart-over-pci")) {
+		rc = aspeed_ast2600_vuart_over_pci_set_enabled(pdev);
+		if (rc) {
+			dev_err(dev, "could not enable VUART over PCI\n");
+			return rc;
+		}
+	}
+
 	return 0;
 
 err_sysfs_remove:
-- 
2.54.0

[PATCH v3 5/7] soc: aspeed: add host-side PCIe BMC device driver

[Grégoire Layet]

Add support for VUART over PCIe between BMC and host.
This add host side driver.
This only support the AST2600.

Taken from ASPEED 6.18 Kernel SDK and trimmed down.

The host can't detect the VUART adresses, they are forced
at 0x3f8 and 0x2f8, similar from the initial ASPEED driver.

The MSI vector index has been changed for the VUART2 from 15 to 17.
The index 15 used in the initial driver was not working.

Data path in both direction is tested on both VUART.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
Tested-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/soc/aspeed/Kconfig               |   8 +
 drivers/soc/aspeed/Makefile              |   1 +
 drivers/soc/aspeed/aspeed-host-bmc-dev.c | 183 +++++++++++++++++++++++
 3 files changed, 192 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c

diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index 63a656449a1a..ebd023b10701 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -4,6 +4,14 @@ if ARCH_ASPEED || COMPILE_TEST
 
 menu "ASPEED SoC drivers"
 
+config ASPEED_HOST_BMC_DEV
+	tristate "ASPEED Host BMC Device"
+	depends on PCI
+	depends on SERIAL_8250
+	help
+	  Enable support for the ASPEED AST2600 BMC Device on the Host.
+	  This configure the PCIe and setup two 8250 compatible VUART ports.
+
 config ASPEED_LPC_CTRL
 	tristate "ASPEED LPC firmware cycle control"
 	select REGMAP
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..c515e163eab7 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_ASPEED_HOST_BMC_DEV)	+= aspeed-host-bmc-dev.o
 obj-$(CONFIG_ASPEED_LPC_CTRL)		+= aspeed-lpc-ctrl.o
 obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
new file mode 100644
index 000000000000..3160b6aedb5b
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) ASPEED Technology Inc.
+
+#include <linux/init.h>
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/pci.h>
+#include <linux/serial_core.h>
+#include <linux/serial_8250.h>
+
+#define BMC_MULTI_MSI	32
+#define PCI_BMC_DEVICE_ID 0x2402
+
+#define DRIVER_NAME "aspeed-host-bmc-dev"
+
+enum aspeed_platform_id {
+	ASPEED,
+};
+
+static int vuart_msi_index[2] = { 16, 17 };
+static int vuart_port_addr[2] = {0x3f8, 0x2f8};
+
+struct aspeed_pci_bmc_dev {
+	struct device *dev;
+	kernel_ulong_t driver_data;
+	int id;
+
+	unsigned long message_bar_base;
+
+	struct uart_8250_port uart[2];
+	int uart_line[2];
+
+	int *msi_idx_table;
+};
+
+static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+
+	pci_bmc_dev->msi_idx_table = vuart_msi_index;
+
+	if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)
+		/* If pci_alloc fail, set all msi index to the first vector */
+		memset(pci_bmc_dev->msi_idx_table, 0, sizeof(vuart_msi_index));
+}
+
+static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev, int idx)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	struct uart_8250_port *uart = &pci_bmc_dev->uart[idx];
+	u16 vuart_ioport;
+	int ret;
+
+	/* Assign the line to non-exist device before everything is setup */
+	pci_bmc_dev->uart_line[idx] = -ENOENT;
+
+	vuart_ioport = vuart_port_addr[idx];
+	/* ASPEED BMC device shift adresses by 2 to the left */
+	vuart_ioport = vuart_ioport << 2;
+
+	uart->port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
+	uart->port.uartclk = 115200 * 16;
+	uart->port.irq = pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[idx]);
+	uart->port.dev = dev;
+	uart->port.iotype = UPIO_MEM32;
+	uart->port.iobase = 0;
+	uart->port.mapbase = pci_bmc_dev->message_bar_base + vuart_ioport;
+	uart->port.membase = 0;
+	uart->port.type = PORT_16550A;
+	uart->port.flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
+	uart->port.regshift = 2;
+
+	ret = serial8250_register_8250_port(&pci_bmc_dev->uart[idx]);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Can't setup PCIe VUART%d\n", idx);
+		return ret;
+	}
+
+	pci_bmc_dev->uart_line[idx] = ret;
+
+	return 0;
+}
+
+static void aspeed_pci_host_bmc_device_release_vuart(struct pci_dev *pdev, int idx)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+
+	if (pci_bmc_dev->uart_line[idx] >= 0)
+		serial8250_unregister_port(pci_bmc_dev->uart_line[idx]);
+}
+
+static int aspeed_pci_host_setup(struct pci_dev *pdev)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
+	int rc = 0;
+
+	pci_bmc_dev->message_bar_base = pci_resource_start(pdev, 1);
+
+	if (pdev->revision == 0x27) {
+		pr_err("AST2700 detected but not supported");
+		return -ENODEV;
+	}
+
+	rc = aspeed_pci_bmc_device_setup_vuart(pdev, 0);
+	if (rc)
+		return rc;
+
+	rc = aspeed_pci_bmc_device_setup_vuart(pdev, 1);
+	if (rc)
+		goto out_freeVUART1;
+
+	return 0;
+
+out_freeVUART1:
+	aspeed_pci_host_bmc_device_release_vuart(pdev, 0);
+
+	return rc;
+}
+
+static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+{
+	struct aspeed_pci_bmc_dev *pci_bmc_dev;
+	int rc = 0;
+
+	pci_bmc_dev = devm_kzalloc(&pdev->dev, sizeof(*pci_bmc_dev), GFP_KERNEL);
+	if (!pci_bmc_dev)
+		return -ENOMEM;
+
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "pci_enable_device() returned error %d\n", rc);
+		return rc;
+	}
+
+	pci_set_master(pdev);
+	pci_set_drvdata(pdev, pci_bmc_dev);
+
+	aspeed_pci_setup_irq_resource(pdev);
+
+	/* Setup BMC PCI device */
+	rc = aspeed_pci_host_setup(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "ASPEED PCIe Host device returned error %d\n", rc);
+		pci_free_irq_vectors(pdev);
+		pci_disable_device(pdev);
+		return rc;
+	}
+
+	return 0;
+}
+
+static void aspeed_pci_host_bmc_device_remove(struct pci_dev *pdev)
+{
+	aspeed_pci_host_bmc_device_release_vuart(pdev, 0);
+	aspeed_pci_host_bmc_device_release_vuart(pdev, 1);
+
+	pci_free_irq_vectors(pdev);
+	pci_disable_device(pdev);
+}
+
+static struct pci_device_id aspeed_host_bmc_dev_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_ASPEED, PCI_BMC_DEVICE_ID), .class = 0xFF0000, .class_mask = 0xFFFF00,
+	  .driver_data = ASPEED },
+	{ 0 }
+};
+
+MODULE_DEVICE_TABLE(pci, aspeed_host_bmc_dev_pci_ids);
+
+static struct pci_driver aspeed_host_bmc_dev_driver = {
+	.name		= DRIVER_NAME,
+	.id_table	= aspeed_host_bmc_dev_pci_ids,
+	.probe		= aspeed_pci_host_bmc_device_probe,
+	.remove		= aspeed_pci_host_bmc_device_remove,
+};
+
+module_driver(aspeed_host_bmc_dev_driver, pci_register_driver, pci_unregister_driver);
+
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
+MODULE_LICENSE("GPL");
-- 
2.54.0

[PATCH v3 6/7] ARM: dts: aspeed: g6: Change vuart compatible string for ast2600

[Grégoire Layet]

Use the ast2600 compatible string.
This makes it more precise and enables specific ast2600 properties.
Still use the ast2500 compatible string as a fallback.

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

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 56bb3b0444f7..7c02633f2bd6 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -707,7 +707,7 @@ emmc: sdhci@1e750100 {
 			};
 
 			vuart1: serial@1e787000 {
-				compatible = "aspeed,ast2500-vuart";
+				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";
 				reg = <0x1e787000 0x40>;
 				reg-shift = <2>;
 				interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>;
@@ -717,7 +717,7 @@ vuart1: serial@1e787000 {
 			};
 
 			vuart3: serial@1e787800 {
-				compatible = "aspeed,ast2500-vuart";
+				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";
 				reg = <0x1e787800 0x40>;
 				reg-shift = <2>;
 				interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
@@ -727,7 +727,7 @@ vuart3: serial@1e787800 {
 			};
 
 			vuart2: serial@1e788000 {
-				compatible = "aspeed,ast2500-vuart";
+				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";
 				reg = <0x1e788000 0x40>;
 				reg-shift = <2>;
 				interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
@@ -737,7 +737,7 @@ vuart2: serial@1e788000 {
 			};
 
 			vuart4: serial@1e788800 {
-				compatible = "aspeed,ast2500-vuart";
+				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";
 				reg = <0x1e788800 0x40>;
 				reg-shift = <2>;
 				interrupts = <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>;
-- 
2.54.0

[PATCH v3 7/7] ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4

[Grégoire Layet]

The VUART 3 and 4 are VUART over PCI.
This flag indicates this information.

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

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 7c02633f2bd6..2a19463b4c21 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -723,6 +723,7 @@ vuart3: serial@1e787800 {
 				interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&syscon ASPEED_CLK_APB2>;
 				no-loopback-test;
+				aspeed,vuart-over-pci;
 				status = "disabled";
 			};
 
@@ -743,6 +744,7 @@ vuart4: serial@1e788800 {
 				interrupts = <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&syscon ASPEED_CLK_APB2>;
 				no-loopback-test;
+				aspeed,vuart-over-pci;
 				status = "disabled";
 			};
 
-- 
2.54.0

Re: [PATCH v3 1/7] dt-bindings: serial: 8250: aspeed: add compatible string for ast2600

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly introduced compatible string `aspeed,ast2600-vuart` is missing from the top-level `properties: compatible: oneOf:` allowed list. While it was correctly added to the `allOf` conditional blocks, its absence from the main enumeration will cause device tree schema validation (`dt_binding_check` and `dtbs_check`) to fail for any device tree node utilizing this compatible string.
--

commit 6f7f111bc5bb5cca5a6591d60c1457c4bf00cb3e
Author: Grégoire Layet <gregoire.layet@9elements.com>

dt-bindings: serial: 8250: aspeed: add compatible string for ast2600

The ast2600 was using the ast2500 vuart compatible string.
This change makes it possible to have ast2600-specific properties.

> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index bb7b9c87a8072..3cbd0f532e151 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -23,7 +23,9 @@ allOf:
>      then:
>        properties:
>          compatible:
> -          const: aspeed,ast2500-vuart
> +          anyOf:
> +            - const: aspeed,ast2500-vuart
> +            - const: aspeed,ast2600-vuart

[Severity: Low]
Does this compatible string also need to be added to the top-level properties
enumeration?

While it is added here in the allOf conditional block, it appears to be missing
from the main allowed list, which currently looks like this:

properties:
  compatible:
    oneOf:
      - const: ns8250
      - const: ns16450
      - const: ns16550
      - const: ns16550a
      - const: ns16850
      - const: aspeed,ast2400-vuart
      - const: aspeed,ast2500-vuart

Will this omission cause a regression during device tree schema validation
for device trees using the new aspeed,ast2600-vuart compatible string?

[ ... ]

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

Re: [PATCH v3 2/7] dt-bindings: serial: 8250: aspeed: add aspeed,vuart-over-pci bool prop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing `aspeed,ast2600-vuart` in the top-level `compatible` property schema definition, causing `dt_binding_check` failures.
--

commit 33747bb385812ccb653177817f0ed8cba28dbdec
Author: Grégoire Layet <gregoire.layet@9elements.com>

dt-bindings: serial: 8250: aspeed: add aspeed,vuart-over-pci bool prop
    
The ASPEED AST2600 has 2 VUART accessible over PCI.
This boolean can be set to specify if the VUART is used over PCI.

> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index 3cbd0f532e151..b03797f4674d4 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -26,6 +26,14 @@ allOf:
>            anyOf:
>              - const: aspeed,ast2500-vuart
>              - const: aspeed,ast2600-vuart
> +  - if:
> +      anyOf:
> +        - required:
> +            - aspeed,vuart-over-pci
> +    then:
> +      properties:
> +        compatible:
> +          const: aspeed,ast2600-vuart

[Severity: Low]
Should aspeed,ast2600-vuart also be explicitly added to the top-level
properties: compatible: oneOf: block?

Documentation/devicetree/bindings/serial/8250.yaml:properties.compatible.oneOf:
properties:
  compatible:
    oneOf:
      - const: ns8250
      - const: ns16450
      - const: ns16550
      - const: ns16550a
      - const: ns16850
      - const: aspeed,ast2400-vuart
      - const: aspeed,ast2500-vuart

Device tree nodes using compatible = "aspeed,ast2600-vuart" will be rejected by
make dt_binding_check because the string is missing from this top-level
definition.

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

Re: [PATCH v3 5/7] soc: aspeed: add host-side PCIe BMC device driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Modifying a shared static global array on IRQ allocation fallback permanently corrupts the driver state for all subsequent devices and creates a data race.
- [High] The driver mishandles `pci_alloc_irq_vectors()` return values, leading to silent probe successes with invalid IRQs or out-of-bounds vector accesses that trigger kernel warnings.
- [Medium] Truncation of a 64-bit PCI physical address due to storing it in an `unsigned long` on 32-bit architectures.
--

commit 64de7243e0e8e8cd9ac72d04be5ef937e8143347
Author: Grégoire Layet <gregoire.layet@9elements.com>

soc: aspeed: add host-side PCIe BMC device driver

Add support for VUART over PCIe between BMC and host.
This configures the PCIe and sets up two 8250 compatible VUART ports.

> diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> new file mode 100644
> index 0000000000000..3160b6aedb5b6
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
[ ... ]
> +static int vuart_msi_index[2] = { 16, 17 };
> +static int vuart_port_addr[2] = {0x3f8, 0x2f8};
> +
> +struct aspeed_pci_bmc_dev {
> +	struct device *dev;
> +	kernel_ulong_t driver_data;
> +	int id;
> +
> +	unsigned long message_bar_base;

[Severity: Medium]
Does storing the BAR address in an unsigned long truncate 64-bit physical
addresses on 32-bit architectures with PAE/LPAE?

pci_resource_start() returns a resource_size_t, which could be 64-bit,
potentially leading to incorrect I/O memory mapping here.

> +
> +	struct uart_8250_port uart[2];
> +	int uart_line[2];
> +
> +	int *msi_idx_table;
> +};
> +
> +static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +
> +	pci_bmc_dev->msi_idx_table = vuart_msi_index;
> +
> +	if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)

[Severity: High]
Can this condition mishandle partial allocations and negative error codes?

If pci_alloc_irq_vectors() partially succeeds and returns a value between
2 and 16, the <= 1 check evaluates to false. This leaves the indices at 16
and 17, but pci_irq_vector() will later trigger a WARN_ON_ONCE out-of-bounds
check in the PCI core when it tries to access index 16.

Additionally, if it fails entirely and returns a negative error, the check
passes, but the probe doesn't abort. Does this result in silently registering
the 8250 UART with an invalid IRQ?

> +		/* If pci_alloc fail, set all msi index to the first vector */
> +		memset(pci_bmc_dev->msi_idx_table, 0, sizeof(vuart_msi_index));

[Severity: High]
Does this memset corrupt the driver state for all subsequent instances?

Because pci_bmc_dev->msi_idx_table points directly to the static global array
vuart_msi_index, writing zeroes here permanently overwrites the shared array.

If multiple ASPEED PCI devices are probed, or if the device is probed again
later, will the indices be permanently stuck at 0?

> +}

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

Re: [PATCH v3 4/7] serial: 8250_aspeed_vuart: add VUART over PCI

[sashiko-bot]

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

New issues:
- [High] The newly added error path directly returns on failure, bypassing sysfs cleanup and leaving the hardware enabled and port registered.
- [Medium] Brittle lookup of the SCU syscon regmap by parsing the "clocks" property.
- [Medium] Setting untested and reserved bits in SCU registers.

Pre-existing issues:
- [High] Preexisting error paths fail to unregister the 8250 serial port and delete the timer on probe failure, leading to a Use-After-Free.
- [High] Unprotected Read-Modify-Write operations on shared hardware registers.
- [High] Sysfs files and TTY port are exposed to userspace before driver data is initialized.
--

commit 277ed031956064f236c5536027c1f37b4a76d71d
Author: Grégoire Layet <gregoire.layet@9elements.com>

    serial: 8250_aspeed_vuart: add VUART over PCI

This commit adds support for VUART over PCI on the AST2600 platform. It
configures the BMC PCI device enables, sets the PCI class code to MFD, and
configures MSI interrupts based on the silicon revision.

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 6afa2f4057e1b..e204e26fa173b 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
[ ... ]
> @@ -412,6 +433,62 @@ static int aspeed_vuart_map_irq_polarity(u32 dt)
> +static int aspeed_ast2600_vuart_over_pci_set_enabled(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	u32 silicon_revision_id;
> +	struct regmap *scu;
> +	int rc;
> +
> +	u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
> +						SCU_PCIE_CONF_BMC_DEV_EN_MMIO |
> +						SCU_PCIE_CONF_BMC_DEV_EN_MSI |
> +						SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER |
> +						SCU_PCIE_CONF_BMC_DEV_EN_E2L |
> +						SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE |
> +						SCU_PCIE_CONF_BMC_DEV_EN;
> +
> +	scu = syscon_regmap_lookup_by_phandle(dev->of_node, "clocks");
> +	if (IS_ERR(scu)) {

[Severity: Medium]
Will this break if any clock multiplexer or intermediate clock controller is
inserted between the SCU and the VUART?

This code uses syscon_regmap_lookup_by_phandle() to fetch the SCU regmap by
parsing the "clocks" property. While this may happen to work if the SCU is
the direct clock provider, does this conflate the clock provider bindings
with syscon mappings?

> +		dev_err(&pdev->dev, "failed to find SCU regmap\n");
> +		return PTR_ERR(scu);
> +	}
[ ... ]
> +	if (silicon_revision_id == AST2600A3_REVISION_ID)
> +		rc = regmap_update_bits(scu, ASPEED_SCUC24,
> +				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN | ASPEED_SCUC24_MSI_ROUTING_MASK,
> +				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN | ASPEED_SCUC24_MSI_ROUTING_PCIe2LPC_PCIDEV1);
> +	else
> +		rc = regmap_update_bits(scu, ASPEED_SCUC24,
> +					/* The bit 14 is reserved in the Datasheet, so we can't say what it does. This revision has not been tested */
> +				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN | BIT(14) | ASPEED_SCUC24_MSI_ROUTING_MASK,

[Severity: Medium]
Is it safe to write BIT(14) into ASPEED_SCUC24 when it is undocumented and
reserved?

The comment indicates the bit's purpose is unknown and this revision is
untested. Can setting this undocumented, reserved bit trigger unpredictable
hardware behavior on other silicon revisions?

> +				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN | BIT(14) | ASPEED_SCUC24_MSI_ROUTING_PCIe2LPC_PCIDEV1);
> +	if (rc) {
> +		dev_err(dev, "could not set PCI device 1 MSI interrupt routing\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> @@ -540,6 +617,15 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  	aspeed_vuart_set_host_tx_discard(vuart, true);

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent access
clobber register modifications?

Functions like aspeed_vuart_set_host_tx_discard() and
aspeed_vuart_set_sirq_polarity() perform read-modify-write operations on
ASPEED_VUART_GCRA. Does a concurrent sysfs write and TTY port open/close
cause a race condition here, since these are invoked from contexts with
different locks (or no locks)?

>  	platform_set_drvdata(pdev, vuart);

[Severity: High]
This isn't a bug introduced by this patch, but does the delay in calling
platform_set_drvdata() expose userspace to a null pointer dereference?

In aspeed_vuart_probe(), sysfs_create_group() is called very early.
If a concurrent userspace process reads the sysfs files, invoking
lpc_address_show() before platform_set_drvdata() is called here, it will
retrieve a null pointer and immediately dereference it.

>  
> +	if (of_device_is_compatible(dev->of_node, "aspeed,ast2600-vuart") &&
> +		of_property_read_bool(dev->of_node, "aspeed,vuart-over-pci")) {
> +		rc = aspeed_ast2600_vuart_over_pci_set_enabled(pdev);
> +		if (rc) {
> +			dev_err(dev, "could not enable VUART over PCI\n");
> +			return rc;

[Severity: High]
Does this direct return bypass necessary cleanup logic and leave the
hardware enabled?

By returning rc directly instead of jumping to a cleanup label, the sysfs
group aspeed_vuart_attr_group is leaked. Furthermore, does this bypass
unregistering the 8250 port and leave the VUART active despite probe
failure, since aspeed_vuart_set_enabled(vuart, true) was called earlier?

> +		}
> +	}
> +
>  	return 0;
>  
>  err_sysfs_remove:
>  	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
>  	return rc;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this error path lead
to a use-after-free?

The err_sysfs_remove block cleans up sysfs attributes but entirely omits
calling serial8250_unregister_port() and timer_delete_sync(). Because the
driver uses devres, the vuart structure will be freed upon probe failure.
Since the port remains actively registered, any subsequent access to the
port via TTY operations will dereference this freed memory.

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

Re: [PATCH v3 3/7] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

[sashiko-bot]

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

New issues:
- [High] Direct return in the newly added AST2600 PCI initialization logic bypasses all resource cleanup.

Pre-existing issues:
- [High] Missing serial port unregistration and timer cleanup on probe error paths causes resource leaks and a Use-After-Free.
- [High] Sysfs attributes are created prematurely in probe() and removed late in remove(), allowing NULL pointer dereference and unmapped memory access.
- [High] Incorrect teardown order in remove() deletes the timer before unregistering the port, allowing the timer to be re-armed.
--

commit 0b81ae92f1a0f52b8cec821070559651477025dd
Author: Grégoire Layet <gregoire.layet@9elements.com>

serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

Makes the driver compatible with the ast2600-vuart.
This enables specific configuration for the AST2600.

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 26fc0464f1cc2..6afa2f4057e1b 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -560,6 +560,7 @@ static void aspeed_vuart_remove(struct platform_device *pdev)
>  static const struct of_device_id aspeed_vuart_table[] = {
>  	{ .compatible = "aspeed,ast2400-vuart" },
>  	{ .compatible = "aspeed,ast2500-vuart" },
> +	{ .compatible = "aspeed,ast2600-vuart" },

[Severity: High]
Does the AST2600 PCI initialization logic bypass the error handling labels
in aspeed_vuart_probe()?

If aspeed_ast2600_vuart_over_pci_set_enabled() fails, the code returns
directly:

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe() {
    ...
    if (of_device_is_compatible(dev->of_node, "aspeed,ast2600-vuart") &&
        of_property_read_bool(dev->of_node, "aspeed,vuart-over-pci")) {
        rc = aspeed_ast2600_vuart_over_pci_set_enabled(pdev);
        if (rc) {
            dev_err(dev, "could not enable VUART over PCI\n");
            return rc;
        }
    }
    return 0;

err_sysfs_remove:
    sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
    return rc;
}

This direct return bypasses the err_sysfs_remove cleanup label, which would
leave the sysfs group leaked and the UART port and timer active with
pointers to the devm-freed vuart structure.

[Severity: High]
This isn't a bug introduced by this patch, but does aspeed_vuart_probe()
leak resources and cause a use-after-free on other error paths?

If the probe fails and jumps to err_sysfs_remove, it removes the sysfs
group but fails to call serial8250_unregister_port() or timer_delete_sync():

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe() {
    ...
    rc = serial8250_register_8250_port(&port);
    ...
err_sysfs_remove:
    sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
    return rc;
}

Because vuart is devm-allocated, it will be freed upon probe failure,
leaving the UART port registered with private_data pointing to freed memory.

[Severity: High]
This is a pre-existing issue, but are the sysfs attributes exposed too
early in probe() and removed too late in remove()?

In aspeed_vuart_probe(), sysfs_create_group() is called before the
driver data is set via platform_set_drvdata():

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe() {
    ...
    rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
    ...
    platform_set_drvdata(pdev, vuart);
    ...
}

If userspace reads attributes like lpc_address during this window,
dev_get_drvdata() returns NULL which is then dereferenced in the callback.

Additionally, in aspeed_vuart_remove(), sysfs_remove_group() is called
after serial8250_unregister_port():

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_remove() {
    ...
    serial8250_unregister_port(vuart->line);
    sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
}

Because unregistering the port calls iounmap(), any concurrent sysfs access
in this window could access unmapped I/O memory.

[Severity: High]
This is also a pre-existing issue, but does aspeed_vuart_remove() delete
the unthrottle timer before disabling the interrupt source that re-arms it?

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_remove() {
    ...
    timer_delete_sync(&vuart->unthrottle_timer);
    aspeed_vuart_set_enabled(vuart, false);
    serial8250_unregister_port(vuart->line);
    ...
}

At the point timer_delete_sync() is called, the UART port is still active.
If an RX interrupt triggers concurrently, aspeed_vuart_handle_irq() can
call mod_timer() and re-arm it. The re-armed timer could then fire after
the devm-allocated vuart structure is freed.

>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, aspeed_vuart_table);

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

Re: [PATCH v3 6/7] ARM: dts: aspeed: g6: Change vuart compatible string for ast2600

[sashiko-bot]

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

New issues:
- [High] Newly introduced error path completely bypasses all probe cleanup.
- [High] Modifying a static global array when MSI allocation fails causes state corruption.
- [High] Assuming index 16 and 17 are unconditionally valid for partially allocated MSI vectors.
- [Medium] Device Tree Binding Schema Validation Failure
- [Medium] Using the 'clocks' property as a syscon phandle violates DT semantics.

Pre-existing issues:
- [High] Probe error paths fail to unregister the 8250 serial port, leading to Use-After-Free.
--

commit cd4f0040d97e2d9918c8ecf27108ed2de48871b7
Author: Grégoire Layet <gregoire.layet@9elements.com>

ARM: dts: aspeed: g6: Change vuart compatible string for ast2600

Use the ast2600 compatible string.
This makes it more precise and enables specific ast2600 properties.
Still use the ast2500 compatible string as a fallback.

> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index bb7b9c87a8072..b03797f4674d4 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -23,7 +23,17 @@ allOf:
>      then:
>        properties:
>          compatible:
> -          const: aspeed,ast2500-vuart
> +          anyOf:
> +            - const: aspeed,ast2500-vuart
> +            - const: aspeed,ast2600-vuart

[ ... ]

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 189bc3bbb47c9..5ff52697e8bb4 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -698,7 +698,7 @@ emmc: sdhci@1e750100 {
>  			};
>  
>  			vuart1: serial@1e787000 {
> -				compatible = "aspeed,ast2500-vuart";
> +				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";

[Severity: Medium]
Will this pass device tree schema validation (dtbs_check)? The binding in
8250.yaml defines the compatible property as anyOf with single const strings,
meaning it expects exactly one of those strings. The device tree uses an array
of two strings. Should the schema be updated with an items array definition to
accept this combination?

[ ... ]

> diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> new file mode 100644
> index 0000000000000..3160b6aedb5b6
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
[ ... ]
> +static int vuart_msi_index[2] = { 16, 17 };
[ ... ]
> +static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +
> +	pci_bmc_dev->msi_idx_table = vuart_msi_index;
> +
> +	if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)
> +		/* If pci_alloc fail, set all msi index to the first vector */
> +		memset(pci_bmc_dev->msi_idx_table, 0, sizeof(vuart_msi_index));

[Severity: High]
Does this code inadvertently corrupt global state for all driver instances?
Since pci_bmc_dev->msi_idx_table points to vuart_msi_index (a static global
array), calling memset on it will permanently zero out the MSI indices
system-wide if any device fails to allocate multiple MSI vectors.

Also, does the threshold check protect against partial allocations falling
short of index 17?

If pci_alloc_irq_vectors() successfully allocates an intermediate number of
vectors (e.g., 4 or 8), the '<= 1' check evaluates to false. The driver
retains indices 16 and 17 and attempts to fetch them via pci_irq_vector()
below, which triggers an out-of-bounds warning and returns -EINVAL.

[ ... ]
> +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev, int idx)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
[ ... ]
> +	uart->port.irq = pci_irq_vector(pdev, pci_bmc_dev->msi_idx_table[idx]);

[Severity: High]
If a partial allocation leaves us with fewer vectors than the index stored in
msi_idx_table[idx], pci_irq_vector() returns a negative error code like
-EINVAL. Could this error code be incorrectly passed as the IRQ number here,
causing interrupt registration to fail later?

[ ... ]

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 26fc0464f1cc2..e204e26fa173b 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
[ ... ]
> +static int aspeed_ast2600_vuart_over_pci_set_enabled(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
[ ... ]
> +	scu = syscon_regmap_lookup_by_phandle(dev->of_node, "clocks");

[Severity: Medium]
Is it semantically correct to use the "clocks" property as a syscon phandle?
The "clocks" property specifies clock providers and specifiers, not raw syscon
phandles. Piggybacking on this to extract a syscon regmap leverages an
implementation detail of syscon_regmap_lookup_by_phandle() and violates
device tree semantics.

[ ... ]
> @@ -540,6 +617,15 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  	aspeed_vuart_set_host_tx_discard(vuart, true);
>  	platform_set_drvdata(pdev, vuart);
>  
> +	if (of_device_is_compatible(dev->of_node, "aspeed,ast2600-vuart") &&
> +		of_property_read_bool(dev->of_node, "aspeed,vuart-over-pci")) {
> +		rc = aspeed_ast2600_vuart_over_pci_set_enabled(pdev);
> +		if (rc) {
> +			dev_err(dev, "could not enable VUART over PCI\n");
> +			return rc;

[Severity: High]
Does returning directly here skip probe cleanup? Bypassing the err_sysfs_remove
label leaves sysfs groups, timers, and the 8250 serial port registered while
devres frees the underlying vuart structure. Does this leak the sysfs groups
and timers, and potentially cause a use-after-free if the 8250 core accesses
the unregistered port?

> +		}
> +	}
> +
>  	return 0;
>  
>  err_sysfs_remove:
>  	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
>  	return rc;

[Severity: High]
This isn't a bug introduced by this patch, but does the err_sysfs_remove path
fail to unregister the 8250 port? If probe fails after port registration,
serial8250_unregister_port() is missed. The 8250 core will later dereference
the freed vuart pointer via port.private_data, causing a use-after-free.

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

Re: [PATCH v3 7/7] ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4

[sashiko-bot]

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

New issues:
- [Low] Adding `aspeed,vuart-over-pci` to nodes with multiple compatible strings violates the `const` constraint in the YAML binding, causing a schema validation failure.

Pre-existing issues:
- [High] Probe error paths fail to unregister the 8250 port and remove the sysfs group, resulting in a use-after-free of the devm-allocated driver state and resource leaks.
--

commit 3ce60a66a65e6d307209da21c88e6e5c758ddf74
Author: Grégoire Layet <gregoire.layet@9elements.com>
Date:   Tue Jun 23 14:25:45 2026 +0000

    ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4
    
    The VUART 3 and 4 are VUART over PCI.
    This flag indicates this information.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 5ff52697e8bb4..b33468288d4a5 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -714,6 +714,7 @@ vuart3: serial@1e787800 {
>  				interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
>  				clocks = <&syscon ASPEED_CLK_APB2>;
>  				no-loopback-test;
> +				aspeed,vuart-over-pci;

[Severity: Low]
Does adding this property cause a device tree schema validation failure?

Looking at Documentation/devicetree/bindings/serial/8250.yaml, the schema
restricts the compatible property to a single string when
aspeed,vuart-over-pci is present:

        compatible:
          const: aspeed,ast2600-vuart

Since the vuart3 node uses multiple compatible strings
("aspeed,ast2600-vuart", "aspeed,ast2500-vuart"), does this violate the
constraint and cause dtbs_check to fail?

[Severity: High]
This isn't a bug introduced by this patch, but adding this property appears
to trigger a pre-existing resource leak and use-after-free in the driver's
error path.

In drivers/tty/serial/8250/8250_aspeed_vuart.c, if
aspeed_ast2600_vuart_over_pci_set_enabled() fails during
aspeed_vuart_probe(), the function returns directly:

        rc = aspeed_ast2600_vuart_over_pci_set_enabled(pdev);
        if (rc) {
                dev_err(dev, "could not enable VUART over PCI\n");
                return rc;
        }

Does this bypass calling serial8250_unregister_port() and
sysfs_remove_group() after a successful serial8250_register_8250_port()?
Since the devm-allocated driver state is freed on probe failure, does the
UART core retain a registered port pointing to freed memory?

>  				status = "disabled";
>  			};

[ ... ]

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