Message-ID: <ZjzUfznxVkyWGPAx@surfacebook.localdomain>
Date: Thu, 9 May 2024 16:49:51 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Sunil V L <sunilvl@...tanamicro.com>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Tony Lindgren <tony@...mide.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [PATCH v1 2/2] serial: 8250: Extract platform driver

Thu, May 09, 2024 at 10:51:11AM +0530, Sunil V L kirjoitti:

> Many thanks for helping with this!. Just few questions below for my
> understanding.

You are welcome! Meanwhile, can you test these changes?

> On Mon, May 06, 2024 at 05:00:59PM +0300, Andy Shevchenko wrote:

..

> > +void __init serial8250_isa_init_ports(void)
> > +{
> > +	static int first = 1;
> > +	int i, irqflag = 0;
> > +
> > +	if (!first)
> > +		return;
> > +	first = 0;
> > +
> > +	if (nr_uarts > UART_NR)
> > +		nr_uarts = UART_NR;
> > +
> > +	/*
> > +	 * Set up initial isa ports based on nr_uart module param, or else
> > +	 * default to CONFIG_SERIAL_8250_RUNTIME_UARTS. Note that we do not
> > +	 * need to increase nr_uarts when setting up the initial isa ports.
> > +	 */
> > +	for (i = 0; i < nr_uarts; i++)
> > +		serial8250_setup_port(i);
> > +
> > +	/* chain base port ops to support Remote Supervisor Adapter */
> > +	univ8250_port_ops = *univ8250_port_base_ops;
> > +	univ8250_rsa_support(&univ8250_port_ops);
> > +
> > +	if (share_irqs)
> > +		irqflag = IRQF_SHARED;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(old_serial_port) && i < nr_uarts; i++) {
> > +		struct uart_8250_port *up = serial8250_get_port(i);
> > +		struct uart_port *port = &up->port;
> > +
> > +		port->iobase   = old_serial_port[i].port;
> > +		port->irq      = irq_canonicalize(old_serial_port[i].irq);
> > +		port->irqflags = 0;
> > +		port->uartclk  = old_serial_port[i].baud_base * 16;
> > +		port->flags    = old_serial_port[i].flags;
> > +		port->hub6     = 0;
> > +		port->membase  = old_serial_port[i].iomem_base;
> > +		port->iotype   = old_serial_port[i].io_type;
> > +		port->regshift = old_serial_port[i].iomem_reg_shift;
> > +
> > +		port->irqflags |= irqflag;
> > +		if (serial8250_isa_config != NULL)
> > +			serial8250_isa_config(i, &up->port, &up->capabilities);
> > +
> > +		serial_base_add_isa_preferred_console(serial8250_reg.dev_name, i);
> > +	}
> > +}
> > +
> > +/*
> > + * Register a set of serial devices attached to a platform device.  The
> > + * list is terminated with a zero flags entry, which means we expect
> > + * all entries to have at least UPF_BOOT_AUTOCONF set.
> > + */
> > +static int serial8250_probe(struct platform_device *dev)
> > +{
> > +	struct plat_serial8250_port *p = dev_get_platdata(&dev->dev);

> Not related to the change. But..
> How is this supposed to work? When serial8250_init creates this ISA
> platform device,
> it doesn't add any platdata. In that case, entire loop
> below appears dead code to me. Am I missing something?

Yes, you are. Here is the answer:
https://elixir.bootlin.com/linux/latest/C/ident/plat_serial8250_port

I.o.w. it depends on the platform and actual drivers that instantiate the
serial8250 platform device. You found only one case for which it's a dead
code, but not for many others.

> > +	struct uart_8250_port uart;
> > +	int ret, i, irqflag = 0;
> > +
> > +	memset(&uart, 0, sizeof(uart));
> > +
> > +	if (share_irqs)
> > +		irqflag = IRQF_SHARED;
> > +
> > +	for (i = 0; p && p->flags != 0; p++, i++) {
> > +		uart.port.iobase	= p->iobase;
> > +		uart.port.membase	= p->membase;
> > +		uart.port.irq		= p->irq;
> > +		uart.port.irqflags	= p->irqflags;
> > +		uart.port.uartclk	= p->uartclk;
> > +		uart.port.regshift	= p->regshift;
> > +		uart.port.iotype	= p->iotype;
> > +		uart.port.flags		= p->flags;
> > +		uart.port.mapbase	= p->mapbase;
> > +		uart.port.mapsize	= p->mapsize;
> > +		uart.port.hub6		= p->hub6;
> > +		uart.port.has_sysrq	= p->has_sysrq;
> > +		uart.port.private_data	= p->private_data;
> > +		uart.port.type		= p->type;
> > +		uart.bugs		= p->bugs;
> > +		uart.port.serial_in	= p->serial_in;
> > +		uart.port.serial_out	= p->serial_out;
> > +		uart.dl_read		= p->dl_read;
> > +		uart.dl_write		= p->dl_write;
> > +		uart.port.handle_irq	= p->handle_irq;
> > +		uart.port.handle_break	= p->handle_break;
> > +		uart.port.set_termios	= p->set_termios;
> > +		uart.port.set_ldisc	= p->set_ldisc;
> > +		uart.port.get_mctrl	= p->get_mctrl;
> > +		uart.port.pm		= p->pm;
> > +		uart.port.dev		= &dev->dev;
> > +		uart.port.irqflags	|= irqflag;
> > +		ret = serial8250_register_8250_port(&uart);
> > +		if (ret < 0) {
> > +			dev_err(&dev->dev, "unable to register port at index %d "
> > +				"(IO%lx MEM%llx IRQ%d): %d\n", i,
> > +				p->iobase, (unsigned long long)p->mapbase,
> > +				p->irq, ret);
> > +		}
> > +	}
> > +	return 0;
> > +}

> > +static struct platform_driver serial8250_isa_driver = {
> > +	.probe		= serial8250_probe,
> > +	.remove_new	= serial8250_remove,
> > +	.suspend	= serial8250_suspend,
> > +	.resume		= serial8250_resume,
> > +	.driver		= {
> > +		.name	= "serial8250",
> > +	},
> > +};
> > +
> > +/*
> > + * This "device" covers _all_ ISA 8250-compatible serial devices listed
> > + * in the table in include/asm/serial.h
> > + */
> > +struct platform_device *serial8250_isa_devs;
> > +
> > +static int __init serial8250_init(void)
> > +{
> > +	int ret;
> > +
> > +	if (nr_uarts == 0)
> > +		return -ENODEV;
> > +
> > +	serial8250_isa_init_ports();
> > +
> > +	pr_info("Serial: 8250/16550 driver, %d ports, IRQ sharing %s\n",
> > +		nr_uarts, str_enabled_disabled(share_irqs));
> > +
> > +#ifdef CONFIG_SPARC
> > +	ret = sunserial_register_minors(&serial8250_reg, UART_NR);
> > +#else
> > +	serial8250_reg.nr = UART_NR;
> > +	ret = uart_register_driver(&serial8250_reg);
> > +#endif
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = serial8250_pnp_init();
> > +	if (ret)
> > +		goto unreg_uart_drv;
> > +
> > +	serial8250_isa_devs = platform_device_alloc("serial8250",
> > +						    PLAT8250_DEV_LEGACY);
> > +	if (!serial8250_isa_devs) {
> > +		ret = -ENOMEM;
> > +		goto unreg_pnp;
> > +	}
> > +
> > +	ret = platform_device_add(serial8250_isa_devs);
> > +	if (ret)
> > +		goto put_dev;
> > +
> > +	serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
> > +
> > +	ret = platform_driver_register(&serial8250_isa_driver);
> > +	if (ret == 0)
> > +		goto out;
> > +
> > +	platform_device_del(serial8250_isa_devs);
> > +put_dev:
> > +	platform_device_put(serial8250_isa_devs);
> > +unreg_pnp:
> > +	serial8250_pnp_exit();
> > +unreg_uart_drv:
> > +#ifdef CONFIG_SPARC
> > +	sunserial_unregister_minors(&serial8250_reg, UART_NR);
> > +#else
> > +	uart_unregister_driver(&serial8250_reg);
> > +#endif
> > +out:
> > +	return ret;
> > +}
> > +module_init(serial8250_init);

..

> > +#ifdef CONFIG_SERIAL_8250_DEPRECATED_OPTIONS
> > +#ifndef MODULE
> > +/* This module was renamed to 8250_core in 3.7.  Keep the old "8250" name
> > + * working as well for the module options so we don't break people.  We
> > + * need to keep the names identical and the convenient macros will happily
> > + * refuse to let us do that by failing the build with redefinition errors
> > + * of global variables.  So we stick them inside a dummy function to avoid
> > + * those conflicts.  The options still get parsed, and the redefined
> > + * MODULE_PARAM_PREFIX lets us keep the "8250." syntax alive.
> > + *
> > + * This is hacky.  I'm sorry.
> 
> Should this comment be updated?

Why? There is no change in this approach. It stays as it was.

> > + */
> > +static void __used s8250_options(void)
> > +{
> > +#undef MODULE_PARAM_PREFIX
> > +#define MODULE_PARAM_PREFIX "8250_core."
> > +
> > +	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
> > +	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
> > +	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
> > +}
> > +#else
> > +MODULE_ALIAS("8250_core");
> 
> Should this be 8250_platform or keeping it same for compatibility reason?

No. The module name is still 8250_core if you carefully check the Makefile.

> > +#endif
> > +#endif

..

> > -obj-$(CONFIG_SERIAL_8250)		+= 8250.o 8250_base.o
> > +obj-$(CONFIG_SERIAL_8250)		+= 8250.o
> >  8250-y					:= 8250_core.o
> > +8250-y					+= 8250_platform.o
> >  8250-$(CONFIG_SERIAL_8250_PNP)		+= 8250_pnp.o
> >  8250-$(CONFIG_SERIAL_8250_RSA)		+= 8250_rsa.o
> > +
> > +obj-$(CONFIG_SERIAL_8250)		+= 8250_base.o
> 
> Curious to know why this is split?

The line

obj-$(X) += foo.o bar.o

means that if X=m, the foo and bar will be separate modules. Since we have a
few object files per each of them it's better to split them in Makefile in
order to have better readability and maintenance. It's pure done for these
purposes.

> Acked-by: Sunil V L <sunilvl@...tanamicro.com> 

Thank you, but I would prefer Tested-by as it is kinda intrusive change.
I tested it on Intel hardware but it might have (hidden) issues on other
setups.

> >  8250_base-y				:= 8250_port.o
> >  8250_base-$(CONFIG_SERIAL_8250_DMA)	+= 8250_dma.o
> >  8250_base-$(CONFIG_SERIAL_8250_DWLIB)	+= 8250_dwlib.o

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists