[PATCH v2 1/2] soc: aspeed: add BMC-side PCIe BMC device driver
Grégoire Layet
gregoire.layet at 9elements.com
Thu Jun 11 01:40:35 PDT 2026
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 at 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 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 | 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
More information about the linux-arm-kernel
mailing list