[PATCH v6 4/6] mfd: aspeed-pci-core: add AST2600 PCIe BMC device driver

[Phil Rosenthal]

Hi Grégoire,

A few notes on the host driver, from driving the VUARTs of the same BMC
device function (1a03:2402) on an ASUS Pro WS W680-ACE IPMI.

Some context for those who haven't seen the other thread [1]: boards
whose BMC runs the board maker's own firmware rather than OpenBMC (on
the ASUS board, ASUS's ASMB10-iKVM firmware) leave the function at the
KCS class code (0c0701) and put the VUART interrupts on other MSI
vectors, so this driver doesn't bind them. I've proposed a separate 8250
driver for those boards, and whether that support should go into
aspeed-pci-core instead is still open in that thread; input from the
serial and MFD maintainers there would be welcome. I said there that I'd
hold these notes until that's settled, but none of them depend on it, so
I'd rather not sit on them. If the support does go into this series,
I'll follow up with what it needs on top of these.

The points below are about the code itself and apply to OpenBMC boards
as well.

> +static const unsigned int vuart_msi_index[ASPEED_BMC_NR_VUART] = { 16, 17 };

These vectors only hold if the BMC's device tree puts the two VUARTs
on SIRQ 0 and 1. 8250_aspeed_vuart takes the SIRQ from
"aspeed,lpc-interrupts" and defaults to SIRQ 4 without it, and
aspeed-g6.dtsi, even with patch 6/6, doesn't set it for vuart3 or
vuart4. A board that enables VUART over PCI without also setting
aspeed,lpc-interrupts gets both VUARTs on SIRQ 4, and, as far as I
can tell, neither on vector 16 or 17. (That the vector is 16 + SIRQ is
my inference: 16 and 17 for SIRQ 0 and 1 on your boards, and vector 20
for the VUART on the ASUS board, whose stock firmware uses SIRQ 4.)
The change from 15 to 17 mentioned in the commit message also suggests
the table came from testing rather than documentation.

The simplest fix would be to make aspeed,lpc-interrupts required
together with aspeed,vuart-over-pci in the binding, or to set SIRQ 0
and 1 for vuart3 and vuart4 in patch 6/6, so the BMC side always
matches the host's table.

The more robust option is to find the vectors at probe, which Tan asked
about in that thread. That works whatever SIRQs the BMC side uses, so a
board DT that leaves them out or gets them wrong still gets interrupts,
and if no vector can be found, the ports fall back to polling instead of
waiting on a vector that never fires. It would also be the basis for
stock firmware support if that ends up in this series. It's the MSI
counterpart of what the 8250 core's autoconfig_irq() does for legacy
ports with UPF_AUTO_IRQ; I described the method in [1], and it's
aspeed_pci8250_find_irqs() in [2]. With the DT fix above your boards
don't strictly need it, but I'm happy to send it as a patch for
aspeed-pci-core if you'd like it.

> +static struct plat_serial8250_port aspeed_uart_port[ASPEED_BMC_NR_VUART + 1] = {
> +	{
> +		.uartclk = 115200 * 16,
> +		.iotype = UPIO_MEM32,
> +		.type = PORT_16550A,

Did you consider PORT_ASPEED_VUART here? The BMC side uses it because
of commit a603ca60cebf ("serial: 8250_aspeed_vuart: add
PORT_ASPEED_VUART port type"): host-side THRE misbehaves when the
BMC-side receive FIFO trigger is above one byte. That commit only
covers that direction, so I can't say the host side needs it too, but
using the same type on both ends would avoid the question.

On the array itself being static and written at probe, I agree with
Sashiko's comment.

> +	for (i = 0; i < ASPEED_BMC_NR_VUART; i++) {
> +		aspeed_uart_port[i].mapbase = bar + (vuart_port_addr[i] << 2);
> +		aspeed_uart_port[i].irq = pci_irq_vector(pdev, vuart_msi_index[i]);
> +	}

Both VUARTs are registered whether or not the BMC enables them. A VUART
the BMC hasn't enabled reads back as all ones through BAR1 (the one at
0x2f8 does on the ASUS board). The 8250 core's "LSR safety check" at
open would normally catch that, but it compares against 0xff, and with
UPIO_MEM32 the read returns 0xffffffff. So, as far as I can tell from
the code, the port opens normally and whatever is written to it is lost.
Reading MCR first and skipping a VUART that reads as all ones avoids
that: MCR bits 6-7 are reserved as zero, and reading it has no side
effects, unlike LSR.

> +static void aspeed_bmc_pci_free_irqs(void *pdev)
> +{
> +	pci_free_irq_vectors(pdev);
> +}

Since the device is enabled with pcim_enable_device(), the vectors are
already managed: pci_alloc_irq_vectors() registers pcim_msi_release(),
which frees them on unbind. The kerneldoc of pci_free_irq_vectors()
says not to call it in that case ("can lead to double-free issues").
It happens to be harmless today, because the second pci_disable_msi()
finds MSI already disabled, but this action can simply be dropped.

> +	rc = pci_alloc_irq_vectors(pdev, ASPEED_BMC_MULTI_MSI, ASPEED_BMC_MULTI_MSI, PCI_IRQ_MSI);
> +	if (rc < 0)
> +		return dev_err_probe(dev, rc, "failed to allocate %d MSI vectors\n",
> +				     ASPEED_BMC_MULTI_MSI);

If 32 vectors aren't available, probe fails and there is no serial
port at all. On x86 that happens whenever interrupt remapping is off,
since only the Intel and AMD remapping domains support multi-MSI. The
VUARTs work fine polled: the 8250 core polls a port with irq 0. On the
ASUS board polling manages about 2 KB/s against 14.6 KB/s with MSI,
which is slow but plenty for a console. Falling back to irq 0 instead
of failing would keep the port usable.

Related, for when KCS or mailbox children are added: with multiple MSI
vectors the function has a single MSI address/data, which is written
when vector 0 is activated and cleared when vector 0 is shut down.
Since the 8250 core frees a port's IRQ on close, a child that uses
vector 0 would cut off every other vector when it releases it. Nothing
uses vector 0 today, so this is just something to keep in mind (or a
comment in the core).

One more thing you may run into when testing unbind, though it isn't
specific to either driver: if a VUART is the console and nothing holds
it open when the port is removed, serial_core uses freed memory. That
panicked my test host when I unloaded my driver. I sent a fix for it
[3].

[1] https://lore.kernel.org/r/8CD30026-4C04-4E0F-B7A3-5C107D2A8410@phil.gs
[2] https://github.com/philrosenthal/aspeed-pci-vuart-dkms/blob/v2.1/dkms/8250_aspeed_pci.c
[3] https://lore.kernel.org/r/20261008-serial-console-remove-v1-1-288b9c315af7@phil.gs

Thanks,
Phil