[PATCH v2 2/2] soc: aspeed: add host-side PCIe BMC device driver
Andrew Jeffery
andrew at codeconstruct.com.au
Wed Jun 10 05:51:42 PDT 2026
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