soc: aspeed: Add BMC and host driver for PCIe BMC device

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +9 -0

+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 */

How virtual is this? Is this directly accessing the hardware via shared memory? Or is there software on the BMC which traps these reads/writes and responds?

But first we need to decide if this is the correct architecture. The alternative is rpmsg or virtio.

Either of these seem like a better way to expose resources of the BMC to the host.

Quoted 3 lines

How virtual is this? Is this directly accessing the hardware via shared memory? Or is there software on the BMC which traps these reads/writes and responds?

The VUART is virtual because there is no physical UART link between the host and the BMC.
Instead, the AST2600 exposes a 16550-compatible register set on both sides (BMC APB and PCIe host MMIO).
The data flows using an internal 16 byte FIFO shared between the two register views.
So it's hardware emulated and there is no software in the data path.

The AST2600 has four VUARTs, two of which are accessible via PCIe MMIO. This is based on the following section of the AST2600 datasheet:
III.48 VUART and III.64 PCI2VUART.

Because the silicon presents a standard 16550A interface in hardware, the existing 8250 driver works without modification.

Quoted 16 lines · Grégoire Layet

Quoted 3 lines

How virtual is this? Is this directly accessing the hardware via shared memory? Or is there software on the BMC which traps these reads/writes and responds?

The VUART is virtual because there is no physical UART link between the host and the BMC.
Instead, the AST2600 exposes a 16550-compatible register set on both sides (BMC APB and PCIe host MMIO).
The data flows using an internal 16 byte FIFO shared between the two register views.
So it's hardware emulated and there is no software in the data path.

The AST2600 has four VUARTs, two of which are accessible via PCIe MMIO. This is based on the following section of the AST2600 datasheet:
III.48 VUART and III.64 PCI2VUART.

Quoted 2 lines

Because the silicon presents a standard 16550A interface in hardware, the existing 8250 driver works without modification.

So tell us about security.

Is only this UART exposed in the shared memory?

So the memory window
is 8 bytes wide? Or are there other peripherals also exposed? How do we know the aspeed is not using the UART itself?

If two drivers are
using it, are we going to crash one or the other system?

https://en.wikipedia.org/wiki/Core_War

The advantage of rpmsg is that the aspeed would advertise what services it is willing to expose. The security issues are different, implementation bugs vs exposing bits of hardware to an attacker.

For the rest of the driver (shared memory, doorbell and mailbox), you are right, it makes more sense to implement rpmsg or virtio than just raw shared memory binding.
These are software-defined communication channels and not hardware-emulated, so they would fit better as rpmsg or virtio drivers.
I took the SDK driver as a starting point without questioning its structure. I have verified the VUART was working correctly with the shared memory, doorbell and mailbox setup removed.
I can split this into VUART only and defer the rest for a separate rpmsg/virtio work.

So I propose for v2:
- Remove the shared memory device, the sysfs doorbell and the mailbox from this series.
- Retain only the required configuration and initialisation on the BMC side driver.
  This should mainly be SCU and PCIe device configuration but other initialisation will be reviewed to determine what is required.
- The shared memory, doorbell and mailbox features could then be addressed in a separate future series, likely as virtio based driver.

Would that be acceptable?

Taken from ASPEED 6.18 Kernel SDK

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

 3 files changed, 195 insertions(+)

 create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c

drivers/soc/aspeed/Kconfig +0 -0

index f579ee0b5afa..3e1fcf3c3268 100644
@@ -4,6 +4,13 @@ if ARCH_ASPEED || COMPILE_TEST

 menu "ASPEED SoC drivers"

drivers/soc/aspeed/Kconfig +7 -0

+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

drivers/soc/aspeed/Makefile +1 -0

index b35d74592964..fab0d247df66 100644
@@ -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

drivers/soc/aspeed/aspeed-bmc-dev.c new file +12 -0

new file mode 100644
index 000000000000..7a204b543c97
@@ -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>

drivers/soc/aspeed/aspeed-bmc-dev.c new file +35 -0

+#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;
+};
+

drivers/soc/aspeed/aspeed-bmc-dev.c new file +6 -0

+	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)

drivers/soc/aspeed/aspeed-bmc-dev.c new file +4 -0

+#endif
+
+
+	return 0;

drivers/soc/aspeed/aspeed-bmc-dev.c new file +12 -0

+	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;
+

drivers/soc/aspeed/aspeed-bmc-dev.c new file +10 -0

+	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:

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.

We should avoid adding more drivers in drivers/soc/aspeed where we can. Is this really necessary?

Hello Grégoire,

Quoted 1 line · Grégoire Layet

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.

Quoted 4 lines

Add support for VUART over PCIe between BMC and host. This add BMC side driver.

Quoted 8 lines

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?

Quoted 49 lines

 3 files changed, 195 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-bmc-dev.c

drivers/soc/aspeed/Kconfig +7 -0

index f579ee0b5afa..3e1fcf3c3268 100644
@@ -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

drivers/soc/aspeed/Makefile +1 -0

index b35d74592964..fab0d247df66 100644
@@ -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

drivers/soc/aspeed/aspeed-bmc-dev.c new file +12 -0

new file mode 100644
index 000000000000..7a204b543c97
@@ -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>

drivers/soc/aspeed/aspeed-bmc-dev.c line 22

Quoted 10 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +10 -0

+#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.

Quoted 35 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +35 -0

+#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;
+};
+

drivers/soc/aspeed/aspeed-bmc-dev.c line 67

Quoted 10 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +10 -0

+
+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.

Quoted 6 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +6 -0

+     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)

drivers/soc/aspeed/aspeed-bmc-dev.c line 83

Quoted 10 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +10 -0

+             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?

drivers/soc/aspeed/aspeed-bmc-dev.c line 85

Quoted 2 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +2 -0

+     //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.

drivers/soc/aspeed/aspeed-bmc-dev.c line 94

Quoted 9 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +9 -0

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

drivers/soc/aspeed/aspeed-bmc-dev.c line 97

Quoted 3 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +3 -0

+#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.

Quoted 4 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +4 -0

+#endif
+
+
+     return 0;

drivers/soc/aspeed/aspeed-bmc-dev.c line 111

Quoted 10 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +10 -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.

drivers/soc/aspeed/aspeed-bmc-dev.c line 119

Quoted 8 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +8 -0

+     {},
+};
+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.

drivers/soc/aspeed/aspeed-bmc-dev.c line 120

Quoted 1 line

drivers/soc/aspeed/aspeed-bmc-dev.c new file +1 -0

+     const void *md = of_device_get_match_data(dev);

I think we can do without this, see below.

Quoted 12 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +12 -0

+     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;
+

drivers/soc/aspeed/aspeed-bmc-dev.c line 142

Quoted 10 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +10 -0

+     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.

drivers/soc/aspeed/aspeed-bmc-dev.c line 148

Quoted 6 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +6 -0

+     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.

drivers/soc/aspeed/aspeed-bmc-dev.c line 151

Quoted 3 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +3 -0

+             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.

Quoted 10 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +10 -0

+     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:

drivers/soc/aspeed/aspeed-bmc-dev.c line 171

Quoted 10 lines

drivers/soc/aspeed/aspeed-bmc-dev.c new file +10 -0

+     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

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.

Hello Andrew,

Anirudh Srinivasan and I have found that IPMI over KCS using the PCI worked with the stock ASPEED bmc driver (the bmc driver in the V1) but not with the trimmed-down version in the V2. I have apparently removed a bit too much from the V2 , but that's not what I want to focus on.

This brings back the question of where we should put the registers configuration,
considering that two different functionalities depend on it.

Quoted 4 lines

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.

Similarly to this idea, we could include have the necessary configuration in the 'kcs_bmc_aspeed' driver. This could be activated using a similar flag ,such as 'pci2lpc'
or 'pci2kcs' directly. However, this would result in a lot of code duplication for most
of the configuration.

The issue for me is that, two drivers configuring the same registers is not a good idea.

The concept sounds reasonable to me. There's probably some bikeshedding to do on the devicetree property though.

Can you outline the duplication you're concerned about? I think it's a matter of resolving the SCU syscon to its regmap, then performing the necessary accesses?

I think it's not as bad as you make it out to be. The SCU's regmap protects updates to individual registers under a lock, so concurrent modification isn't a concern. The hardware design choices make all of this slightly awkward for any related software design. As an alternative you could implement a mini subsystem that relevant drivers could call through to set the bits, but I currently think that's unnecessary work.

Hello Andrew,

Quoted 2 lines

The concept sounds reasonable to me. There's probably some bikeshedding to do on the devicetree property though.

Yes, having looked at how it's done, I would say :
'aspeed,vuart-over-pci' and 'aspeed,kcs-over-pci' flags would be better.

Quoted 3 lines

Can you outline the duplication you're concerned about? I think it's a matter of resolving the SCU syscon to its regmap, then performing the necessary accesses?

Both drivers will need to set :
- Enable PCI BMC Device MMIO
- Enable PCI BMC Device IRQ
- Enable PCI BMC Device MSI rooting over PCI Device 1 (BAR1)
- Enable Host 2 BMC MSI interrupts
- PCI device class to 0xff000000 to be identified as a MFD device. The reset default is 0x0C070100 which is an IPMI KCS device, but that causes issues as it is detected by ipmi_si but can't be loaded because of non default KCS address.

Sorry for my errors, there is not that much. But both drivers will do almost the same initialisation. That was my code duplication concern.

Quoted 8 lines

I think it's not as bad as you make it out to be. The SCU's regmap protects updates to individual registers under a lock, so concurrent modification isn't a concern. The hardware design choices make all of this slightly awkward for any related software design. As an alternative you could implement a mini subsystem that relevant drivers could call through to set the bits, but I currently think that's unnecessary work.

You are right it's not as bad as I thought.
For now, I will focus on the VUART until the solution has been validated. Then I will easily do the same for the KCS over PCI.

So I'll do for the V3 of the BMC side driver:
- modify the device tree binding to have 'aspeed,ast2600-vuart' and add the 'aspeed,vuart-over-pci' boolean flag, only for the ast2600.

- modify the '8250_aspeed_vuart' driver to add 'aspeed,ast2600-vuart' support.
- add vuart over pci enable and disable code to the '8250_aspeed_vuart' driver.

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

 3 files changed, 258 insertions(+)

 create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c

drivers/soc/aspeed/Kconfig +0 -0

index 3e1fcf3c3268..5deefb64e8c7 100644
@@ -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.

drivers/soc/aspeed/Kconfig +8 -0

+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

drivers/soc/aspeed/Makefile +0 -0

index fab0d247df66..3fd3f6d8d36e 100644
@@ -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

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +5 -0

new file mode 100644
index 000000000000..7cb52a770fb6
@@ -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>

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +101 -0

+#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;

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +18 -0

+		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[] = {

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +7 -0

+	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;

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +5 -0

+	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;

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +4 -0

+		aspeed_pci_host_bmc_device_release_vuart(pdev);
+
+	ida_free(&bmc_device_ida, pci_bmc_dev->id);
+

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +21 -0

+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);

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +4 -0

+
+MODULE_AUTHOR("Ryan Chen <[email protected]>");
+MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
+MODULE_LICENSE("GPL");

Again, I'd rather we avoid drivers/soc/aspeed.

Quoted 5 lines · Grégoire Layet

Taken from ASPEED 6.18 Kernel SDK

Add support for VUART over PCIe between BMC and host. This add host side driver.

Quoted 8 lines

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.

Quoted 44 lines

 3 files changed, 258 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-host-bmc-dev.c

drivers/soc/aspeed/Kconfig +8 -0

index 3e1fcf3c3268..5deefb64e8c7 100644
@@ -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

drivers/soc/aspeed/Makefile +1 -0

index fab0d247df66..3fd3f6d8d36e 100644
@@ -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

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +5 -0

new file mode 100644
index 000000000000..7cb52a770fb6
@@ -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>

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 15

Quoted 10 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +10 -0

+#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.

Quoted 101 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +101 -0

+#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;

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 126

Quoted 10 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +10 -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.

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 130

Quoted 4 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +4 -0

+             (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?

Quoted 18 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +18 -0

+             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[] = {

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 158

Quoted 10 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +10 -0

+     { .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.

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 168

Quoted 10 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +10 -0

+
+     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.

Quoted 7 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +7 -0

+     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;

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 185

Quoted 10 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +10 -0

+     }
+
+     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.

Quoted 5 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +5 -0

+     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;

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 200

Quoted 10 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +10 -0

+     }
+
+     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.

Quoted 4 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +4 -0

+             aspeed_pci_host_bmc_device_release_vuart(pdev);
+
+     ida_free(&bmc_device_ida, pci_bmc_dev->id);
+

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 214

Quoted 10 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +10 -0

+     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.

Quoted 21 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +21 -0

+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);

drivers/soc/aspeed/aspeed-host-bmc-dev.c line 245

Quoted 10 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +10 -0

+}
+
+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.

Quoted 4 lines

drivers/soc/aspeed/aspeed-host-bmc-dev.c new file +4 -0

+
+MODULE_AUTHOR("Ryan Chen <[email protected]>");
+MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
+MODULE_LICENSE("GPL");

This is a v2 for upstreaming the VUART over PCIe BMC device driver from the ASPEED kernel SDK (branch master-v6.18) [1]. There are two drivers: a BMC-side driver and a host-side driver.
Together they enable host<->BMC VUART communication via PCIe.

From the replies i got the host can access anything on the LPC bus of the aspeed.

TPMs are often there, but does a BMC have a TPM?

The ast2600 was using the ast2500 vuart compatible string. This change makes it possible to have ast2600-specific properties.

Signed-off-by Grégoire Layet <[email protected]>

---
 .../devicetree/bindings/serial/8250.yaml      | 20 +++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Documentation/devicetree/bindings/serial/8250.yaml +0 -0

index bb7b9c87a807..3cbd0f532e15 100644

Documentation/devicetree/bindings/serial/8250.yaml +9 -5

+            - const: aspeed,ast2500-vuart
+            - const: aspeed,ast2600-vuart
   - if:
       properties:
         compatible:

@@ -287,17 +289,19 @@ properties:
   aspeed,sirq-polarity-sense:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     description: |
-      Phandle to aspeed,ast2500-scu compatible syscon alongside register
-      offset and bit number to identify how the SIRQ polarity should be
-      configured. One possible data source is the LPC/eSPI mode bit. Only
-      applicable to aspeed,ast2500-vuart.
+      Phandle to aspeed,ast2500-scu or aspeed,ast2600-scu compatible syscon
+      alongside register offset and bit number to identify how the SIRQ
+      polarity should be configured. One possible data source is the LPC/eSPI
+      mode bit. Only applicable to aspeed,ast2500-vuart and
+      aspeed,ast2600-vuart.
     deprecated: true
 
   aspeed,lpc-io-reg:
     $ref: /schemas/types.yaml#/definitions/uint32-array
     maxItems: 1
     description: |
-      The VUART LPC address.  Only applicable to aspeed,ast2500-vuart.
+      The VUART LPC address. Only applicable to aspeed,ast2500-vuart and
+      aspeed,ast2600-vuart.
 
   aspeed,lpc-interrupts:
     $ref: /schemas/types.yaml#/definitions/uint32-array

Do not attach (thread) your patchsets to some other threads (unrelated or older versions). This buries them deep in the mailbox and might interfere with applying entire sets. See also:
https://elixir.bootlin.com/linux/v6.16-rc2/source/Documentation/process/submitting-patches.rst#L830

This should be oneOf (by convention and actually more accurate meaning).

More important, where is documenting of the actual compatible?

The ASPEED AST2600 has 2 VUART accessible over PCI.

This boolean can be set to specify if the VUART is used over PCI.

Signed-off-by Grégoire Layet <[email protected]>

---
 .../devicetree/bindings/serial/8250.yaml          | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Documentation/devicetree/bindings/serial/8250.yaml +8 -0

index 3cbd0f532e15..b03797f4674d 100644
@@ -26,6 +26,14 @@ allOf:
           anyOf:
             - const: aspeed,ast2500-vuart
             - const: aspeed,ast2600-vuart
+  - if:
+      anyOf:
+        - required:
+            - aspeed,vuart-over-pci
+    then:
+      properties:
+        compatible:
+          const: aspeed,ast2600-vuart
   - if:
       properties:
         compatible:

What does that mean? How UART can be accessible over PCI bus?

Documentation/devicetree/bindings/serial/8250.yaml properties: · line 325

Quoted 3 lines

Documentation/devicetree/bindings/serial/8250.yaml +3 -0

+  aspeed,vuart-over-pci:
+    type: boolean
+    default: false

There is no such syntax. Please do not introduce own style. Instead, look at other files how this is done.

Quoted 1 line

+ description: |

Do not need '|' unless you need to preserve formatting.

Hi Krzysztof,

Quoted 1 line

What does that mean? How UART can be accessible over PCI bus?

It's a Virtual UART. Internally, it's two FIFOs accessible via 8250-compatible register sets on both ends.

There is 4 Virtuals UARTs on the LPC bus of the AST2600 and 2 of them are bridged over the PCI bus.
So, from the host, you can access the 8250 register set on the PCI bus.

Quoted 2 lines

It's a Virtual UART. Internally, it's two FIFOs accessible via 8250-compatible register sets on both ends.

I do not know what is Virtual UART...

Quoted 3 lines

There is 4 Virtuals UARTs on the LPC bus of the AST2600 and 2 of them are bridged over the PCI bus.
So, from the host, you can access the 8250 register set on the PCI bus.

You mean these appear (or are) as PCI devices?

Use the ast2600 compatible string.
This makes it more precise and enables specific ast2600 properties. Still use the ast2500 compatible string as a fallback.

Signed-off-by Grégoire Layet <[email protected]>

---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/arm/boot/dts/aspeed/aspeed-g6.dtsi +0 -0

index 56bb3b0444f7..7c02633f2bd6 100644

Please start testing your patches. This for sure fails tests.

It does not look like you tested the DTS against bindings. Please run 'make dtbs_check W=1' (see Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on distro packages for dtschema and be sure you are using the latest released dtschema.