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

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 <[email protected]>
> Signed-off-by: aspeedyh <[email protected]>
> Signed-off-by: Grégoire Layet <[email protected]>
> Tested-by: Grégoire Layet <[email protected]>
> ---
>  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 <[email protected]>");
> +MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
> +MODULE_LICENSE("GPL");