[PATCH v2 2/2] soc: aspeed: add host-side PCIe BMC device driver
Grégoire Layet
gregoire.layet at 9elements.com
Thu Jun 11 01:41:02 PDT 2026
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 at 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 at aspeedtech.com>
> > Signed-off-by: aspeedyh <yh_chung at aspeedtech.com>
> > Signed-off-by: Grégoire Layet <gregoire.layet at 9elements.com>
> > Tested-by: Grégoire Layet <gregoire.layet at 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 at aspeedtech.com>");
> > +MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
> > +MODULE_LICENSE("GPL");
More information about the linux-arm-kernel
mailing list