Re: [PATCH v5 4/6] soc: mfd: add ASPEED AST2600 PCIe BMC device driver

> Add support for UART over PCIe between ASPEED
> AST2600 BMC and host.
> This is the host-side driver.
> 
> Based on ASPEED 6.18 Kernel SDK and trimmed down
> and changed to MFD driver.

This doesn't seem to be aligned (72/75 chars per line).

> 
> The AST2600 has two 8250-compatible register set usable over PCIe.
> These act as two UARTs. As there is only a FIFO internally between both
> ends, they are called Virtual UARTs. This patch adds both VUARTs as
> plat_serial8250_port. This is the core driver in charge of setting up
> MSI interrupts. It doesn't bind the whole PCIe resource to allow each
> sub-driver to bind the needed addresses.
> 
> This is not added as a PCI 8250 UART device, but as a MFD driver, as this
> host driver can be expanded upon for IPMI over KCS. It can also be used
> in the future for custom BMC<->host communication with shared memory and
> doorbell.
> 
> This host module should be the entry point for setting up all features
> related to an AST2600 present on the PCI bus.
> 
> Notes on the implementation:
> The host can't detect the VUART addresses, so they are forced to
> 0x3f8 and 0x2f8, as in the initial ASPEED driver.
> 
> The MSI vector index of VUART2 has been changed from 15 to 17.
> The index 15 used in the initial driver was not working.
> 
> Tested:
> Data path in both direction is tested on both VUARTs.
> 
> Signed-off-by: Jacky Chou <jacky_chou-SAlXDmAnmOAqDJ6do+/[email protected]>
> Signed-off-by: aspeedyh <yh_chung-SAlXDmAnmOAqDJ6do+/[email protected]>
> Signed-off-by: Grégoire Layet <[email protected]>
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 15011f5752a9..33a0351819e6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4011,6 +4011,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/crypto/aspeed,*
>  F:	drivers/crypto/aspeed/
>  
> +ASPEED PCI BMC DEVICE DRIVER
> +M:	Grégoire Layet <git-EbdbDBxZ4ojRpl3pRkHMRQC/[email protected]>
> +M:	Tan Siewert <[email protected]>
> +S:	Maintained
> +F:	drivers/mfd/aspeed-pci-core.c
> +
>  ASPEED PECI CONTROLLER
>  M:	Iwona Winiarska <[email protected]>
>  L:	linux-aspeed-uLR06cmDAlY/[email protected] (moderated for non-subscribers)
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 763ce6a34782..dd780f72d169 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2573,5 +2573,17 @@ config MFD_MAX7360
>  	  additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +config MFD_ASPEED_PCI_BMC_DEVICE
> +	tristate "ASPEED BMC PCIe device"
> +	depends on PCI
> +	select MFD_CORE
> +	help
> +	  Host-side driver for the ASPEED AST2600 BMC PCIe device found on
> +	  BMC expansion cards. Exposes two 8250-compatible VUART
> +	  ports.
> +
> +	  If unsure, say N. Choose M here if this machine has an
> +	  AST2600-based BMC expansion card on its PCIe bus.
> +
>  endmenu
>  endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index dd4bb7e77c33..a364b1b5936b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -303,3 +303,5 @@ obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu_spi.o rsmu_core.o
>  obj-$(CONFIG_MFD_UPBOARD_FPGA)	+= upboard-fpga.o
>  
>  obj-$(CONFIG_MFD_LOONGSON_SE)	+= loongson-se.o
> +
> +obj-$(CONFIG_MFD_ASPEED_PCI_BMC_DEVICE)	+= aspeed-pci-core.o
> diff --git a/drivers/mfd/aspeed-pci-core.c b/drivers/mfd/aspeed-pci-core.c
> new file mode 100644
> index 000000000000..ad6732133ec5
> --- /dev/null
> +++ b/drivers/mfd/aspeed-pci-core.c
> @@ -0,0 +1,120 @@
> +// 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/pci.h>
> +#include <linux/serial_8250.h>
> +#include <linux/mfd/core.h>
> +
> +#define ASPEED_BMC_MULTI_MSI		32
> +#define ASPEED_BMC_PCI_DEVICE_ID	0x2402
> +#define ASPEED_BMC_REVISION_AST2700	0x27
> +#define ASPEED_BMC_VUART		2
> +
> +#define DRIVER_NAME "ast2600-pci-core"
> +
> +static const unsigned int vuart_msi_index[ASPEED_BMC_VUART] = { 16, 17 };
> +static const u16 vuart_port_addr[ASPEED_BMC_VUART] = { 0x3f8, 0x2f8 };
> +
> +struct aspeed_pci_bmc_dev {
> +	struct plat_serial8250_port uart[ASPEED_BMC_VUART + 1];
> +	struct mfd_cell cell;
> +};
> +
> +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev,
> +					     struct aspeed_pci_bmc_dev *pci_bmc_dev)
> +{
> +	resource_size_t bar = pci_resource_start(pdev, 1);
> +	struct plat_serial8250_port *port;
> +	u16 vuart_ioport;
> +	unsigned int i;
> +
> +	for (i = 0; i < ASPEED_BMC_VUART; i++) {
> +		port = &pci_bmc_dev->uart[i];
> +
> +		/* ASPEED BMC device shift addresses by 2 to the left */
> +		vuart_ioport = vuart_port_addr[i] << 2;
> +
> +		port->mapbase = bar + vuart_ioport;
> +		port->uartclk = 115200 * 16;
> +		port->irq = pci_irq_vector(pdev, vuart_msi_index[i]);
> +		port->iotype = UPIO_MEM32;
> +		port->type = PORT_16550A;
> +		port->flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
> +		port->regshift = 2;
> +	}
> +
> +	pci_bmc_dev->cell = (struct mfd_cell) {
> +		.name		= "serial8250",
> +		.platform_data	= pci_bmc_dev->uart,
> +		.pdata_size	= sizeof(pci_bmc_dev->uart),
> +	};

In the commit message you're describing that the MFD driver will be used for
setting up MSI interrupts:
> It doesn't bind the whole PCIe resource to allow each sub-driver to bind the
> needed addresses.

But you're adding the UART devices here which now means that the core driver is
doing more than MSI interrupt init.

Suggestion from my side would be to have an array of `mfd_cell` where you define
which devices are part of the MFD, and your VUART lives in another driver.
See `ls2k-bmc-core` for example.

Tan

-- 
Tan Siewert <[email protected]>