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

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 <[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          |   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