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

[Jiri Slaby <jirislaby@kernel.org>]

On 08. 07. 26, 17:35, Grégoire Layet wrote:
> 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.
...
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> @@ -0,0 +1,174 @@
> +// 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_core.h>
> +#include <linux/serial_8250.h>
> +
> +#define BMC_MULTI_MSI	32
> +#define PCI_BMC_DEVICE_ID 0x2402
> +
> +#define DRIVER_NAME "aspeed-host-bmc-dev"
> +
> +enum aspeed_platform_id {
> +	ASPEED,

What is this good for?

> +};
> +
> +static const int vuart_msi_index[2] = { 16, 17 };
> +static const int vuart_port_addr[2] = {0x3f8, 0x2f8};

Sort of inconsistent spaces. Both arrays should be unsigned anyway. And 
for the latter, u16 should be enough.

> +struct aspeed_pci_bmc_dev {
> +	unsigned long message_bar_base;
> +
> +	struct uart_8250_port uart[2];
> +	int uart_line[2];
> +};
> +
> +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev, int idx)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	struct uart_8250_port *uart = &pci_bmc_dev->uart[idx];
> +	u16 vuart_ioport;
> +	int ret;
> +
> +	/* Assign the line to non-exist device before everything is setup */
> +	pci_bmc_dev->uart_line[idx] = -ENOENT;
> +
> +	vuart_ioport = vuart_port_addr[idx];
> +	/* ASPEED BMC device shift addresses by 2 to the left */
> +	vuart_ioport = vuart_ioport << 2;

Simply:
vuart_ioport <<= 2;
? Or join the two lines?

> +	uart->port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
> +	uart->port.uartclk = 115200 * 16;
> +	uart->port.irq = pci_irq_vector(pdev, vuart_msi_index[idx]);
> +	uart->port.dev = dev;
> +	uart->port.iotype = UPIO_MEM32;
> +	uart->port.iobase = 0;
> +	uart->port.mapbase = pci_bmc_dev->message_bar_base + vuart_ioport;
> +	uart->port.membase = 0;
> +	uart->port.type = PORT_16550A;
> +	uart->port.flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
> +	uart->port.regshift = 2;
> +
> +	ret = serial8250_register_8250_port(&pci_bmc_dev->uart[idx]);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "Can't setup PCIe VUART%d\n", idx);
> +		return ret;
> +	}
> +
> +	pci_bmc_dev->uart_line[idx] = ret;
> +
> +	return 0;
> +}
...
> +static struct pci_device_id aspeed_host_bmc_dev_pci_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_ASPEED, PCI_BMC_DEVICE_ID),
> +		.class = 0xFF0000, .class_mask = 0xFFFF00,

PCI_CLASS_OTHERS << 16

> +		.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,
> +};
> +
> +module_driver(aspeed_host_bmc_dev_driver, pci_register_driver, pci_unregister_driver);
> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +MODULE_DESCRIPTION("ASPEED Host BMC DEVICE Driver");
> +MODULE_LICENSE("GPL");

thanks,
-- 
js
suse labs