Re: [PATCH v1 1/1] soc/aspeed: Add host side BMC device driver

From: Ninad Palsule
Date: Wed Aug 23 2023 - 13:33:02 EST


Hello Andrew,

On 8/22/23 11:14 AM, Ninad Palsule wrote:
Hello Andrew,

Thanks for the review.

On 8/21/23 2:29 PM, Andrew Lunn wrote:
Testing:
   - This is tested on IBM rainier system with BMC. It requires BMC side
     BMC device driver which is available in the ASPEED's 5.15 SDK
     kernel.
How relevant is that? To the host side, it just appears to be an
16550A. Is the SDK emulating an 16550A? If you where to use a
different kernel, is it still guaranteed to be an 16550A? I also
notice there is a mainline
drivers/tty/serial/8250/8250_aspeed_vuart.c. Could that be used on the
BMC? That would be a better testing target than the vendor kernel.

This is just to indicate how I tested my code.

Yes, aspeed chip (in this case ast2600) is compatible with 16550 UART.

I am guessing it should work with different kernel too as 16550 standard is used.

The 8250_aspeed_vuart.c is a BMC side driver for accessing VUART over LPC bus and

this is a host side driver to access VUART over PCIe bus.

+config ASPEED_HOST_BMC_DEV
+    bool "ASPEED SoC Host BMC device driver"
+    default ARCH_ASPEED
+    select SOC_BUS
+    default ARCH_ASPEED
same default twice?
Removed.

+static int __init aspeed_host_bmc_device_init(void)
+{
+    int ret;
+
+    /* register pci driver */
+    ret = pci_register_driver(&aspeed_host_bmc_dev_driver);
+    if (ret < 0) {
+        pr_err("pci-driver: can't register pci driver\n");
+        return ret;
+    }
+
+    return 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);
It looks like you can use module_pci_driver() ?
yes, It should work unless the late initcall is important. I will test it and see.

I will not be able to use module_pci_driver() as it doesn't support late initcall which is required otherwise

8250 registration fails. So I am not making this change.


Thanks & Regards,

Ninad Palsule