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

[Ninad Palsule]

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