Re: [PATCH v4 5/7] soc: aspeed: add host-side PCIe BMC device driver

[Tan Siewert]

> Add support for VUART over PCIe between BMC and host.
> Add the host side driver.
> Support only the AST2600.
> 
> Taken from ASPEED 6.18 Kernel SDK and trimmed down.


> 
> The host can't detect the VUART addresses, so force them to
> 0x3f8 and 0x2f8, as in the initial ASPEED driver.
> 
> Change the MSI vector index of VUART2 from 15 to 17.
> The index 15 used in the initial driver was not working.
> 
> Data path in both direction is tested on both VUART.
> 
> This module is added in soc/aspeed as it's very soc specific.
> This is not added as a PCI 8250 UART device as this host module 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.

While I understand that this driver could be expanded with more features, it is
still not fitting `soc/aspeed` as it is not for the ASPEED SoC, but for systems
that communicate with an ASPEED SoC.

Maybe `drivers/misc` is more fitting for this?

> 
> This host module should be the entry point for setting up all features
> related to an AST2600 present on the PCI bus.
> 
> Signed-off-by: Jacky Chou <jacky_chou@xxxxxxxxxxxxxx>
> Signed-off-by: aspeedyh <yh_chung@xxxxxxxxxxxxxx>
> Signed-off-by: Grégoire Layet <gregoire.layet@xxxxxxxxxxxxx>
>
> diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> index f579ee0b5afa..147a9033bdc4 100644
> --- a/drivers/soc/aspeed/Kconfig
> +++ b/drivers/soc/aspeed/Kconfig
> @@ -55,3 +55,18 @@ config ASPEED_SOCINFO
>  endmenu
>  
>  endif
> +
> +menu "ASPEED host-side drivers"
> +	depends on PCI
> +
> +config ASPEED_HOST_BMC_DEV
> +	tristate "ASPEED host-side BMC PCIe device"

"device driver" would be more fitting instead of "host-side driver" I'd say.

> +	depends on SERIAL_8250
> +	help
> +	  Host-side driver for the ASPEED AST2600 BMC PCIe device found on
> +	  BMC expansion cards. Exposes two 8250-compatible VUART
> +	  ports.

In the commit message you're stating that the device driver can be expanded for
e.g. IPMI over KCS. Should the user be able to disable features selectively, or
do they have to always have 8250 serial support active?

> +
> +	  If unsure, say N. Choose M to build aspeed-host-bmc-dev.
> +
> +endmenu
> diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> index b35d74592964..c515e163eab7 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_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
> diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> new file mode 100644
> index 000000000000..e586d0505577
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c

*snip*

> +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;
> +
> +	pci_bmc_dev->message_bar_base = pci_resource_start(pdev, 1);
> +
> +	if (pdev->revision == 0x27) {
> +		pr_err("AST2700 detected but not supported");
> +		return -ENODEV;
> +	}
> +
> +	rc = aspeed_pci_bmc_device_setup_vuart(pdev, 0);
> +	if (rc)
> +		return rc;
> +
> +	rc = aspeed_pci_bmc_device_setup_vuart(pdev, 1);
> +	if (rc)
> +		goto out_free_VUART0;

This goto seems unnecessary. There's no specific cleanup that should be needed
before.

	Tan

-- 
Tan Siewert <tan.siewert@xxxxxxxxxxxxx>