| Message ID | 20240327110021.59793-5-tony@atomide.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for DEVNAME:0.0 style hardware based addressing | expand |
Hi, On Mar 27, 2024 at 12:59:38 +0200, Tony Lindgren wrote: > We can now add hardware based addressing for serial ports. Starting with > commit 84a9582fd203 ("serial: core: Start managing serial controllers to > enable runtime PM"), and all the related fixes to this commit, the serial > core now knows to which serial port controller the ports are connected. > > The serial ports can be addressed with DEVNAME:0.0 style naming. The names > are something like 00:04:0.0 for a serial port on qemu, and something like > 2800000.serial:0.0 on platform device using systems like ARM64 for example. > > The DEVNAME is the unique serial port hardware controller device name, AKA > the name for port->dev. The 0.0 are the serial core controller id and port > id. > > Typically 0.0 are used for each controller and port instance unless the > serial port hardware controller has multiple controllers or ports. > > Using DEVNAME:0.0 style naming actually solves two long term issues for > addressing the serial ports: > > 1. According to Andy Shevchenko, using DEVNAME:0.0 style naming fixes an > issue where depending on the BIOS settings, the kernel serial port ttyS > instance number may change if HSUART is enabled > > 2. Device tree using architectures no longer necessarily need to specify > aliases to find a specific serial port, and we can just allocate the This is GOOD! > ttyS instance numbers dynamically in whatever probe order > > To do this, let's match the hardware addressing style console name to > the character device name used, and add a preferred console using the > character device name. > > Note that when using console=DEVNAME:0.0 style kernel command line, the > 8250 serial console gets enabled later compared to using console=ttyS > naming for ISA ports. This is because the serial port DEVNAME to character > device mapping is not known until the serial driver probe time. If used > together with earlycon, this issue is avoided. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/tty/serial/serial_base.h | 16 +++++++ > drivers/tty/serial/serial_base_bus.c | 66 ++++++++++++++++++++++++++++ > drivers/tty/serial/serial_core.c | 4 ++ > 3 files changed, 86 insertions(+) > > diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h > --- a/drivers/tty/serial/serial_base.h > +++ b/drivers/tty/serial/serial_base.h > @@ -45,3 +45,19 @@ void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port > > int serial_core_register_port(struct uart_driver *drv, struct uart_port *port); > void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port); > + > +#ifdef CONFIG_SERIAL_CORE_CONSOLE > + > +int serial_base_add_preferred_console(struct uart_driver *drv, > + struct uart_port *port); > + > +#else > + > +static inline > +int serial_base_add_preferred_console(struct uart_driver *drv, > + struct uart_port *port) > +{ > + return 0; > +} > + > +#endif > diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c > --- a/drivers/tty/serial/serial_base_bus.c > +++ b/drivers/tty/serial/serial_base_bus.c > @@ -8,6 +8,7 @@ > * The serial core bus manages the serial core controller instances. > */ > > +#include <linux/cleanup.h> > #include <linux/container_of.h> > #include <linux/device.h> > #include <linux/idr.h> > @@ -204,6 +205,71 @@ void serial_base_port_device_remove(struct serial_port_device *port_dev) > put_device(&port_dev->dev); > } > > +#ifdef CONFIG_SERIAL_CORE_CONSOLE > + > +static int serial_base_add_one_prefcon(const char *match, const char *dev_name, > + int port_id) > +{ > + int ret; > + > + ret = add_preferred_console_match(match, dev_name, port_id); > + if (ret == -ENOENT) > + return 0; > + > + return ret; Can we do this instead? return (ret == -ENOENT ? 0 : ret); > +} > + > +static int serial_base_add_prefcon(const char *name, int idx) > +{ > + const char *char_match __free(kfree) = NULL; > + > + /* Handle the traditional character device name style console=ttyS0 */ > + char_match = kasprintf(GFP_KERNEL, "%s%i", name, idx); > + if (!char_match) > + return -ENOMEM; > + > + return serial_base_add_one_prefcon(char_match, name, idx); > +} > + > +/** > + * serial_base_add_preferred_console - Adds a preferred console > + * @drv: Serial port device driver > + * @port: Serial port instance > + * > + * Tries to add a preferred console for a serial port if specified in the > + * kernel command line. Supports both the traditional character device such > + * as console=ttyS0, and a hardware addressing based console=DEVNAME:0.0 > + * style name. > + * > + * Translates the kernel command line option using a hardware based addressing > + * console=DEVNAME:0.0 to the serial port character device such as ttyS0. > + * Cannot be called early for ISA ports, depends on struct device. > + * > + * Note that duplicates are ignored by add_preferred_console(). > + * > + * Return: 0 on success, negative error code on failure. > + */ > +int serial_base_add_preferred_console(struct uart_driver *drv, > + struct uart_port *port) > +{ > + const char *port_match __free(kfree) = NULL; > + int ret; > + > + ret = serial_base_add_prefcon(drv->dev_name, port->line); > + if (ret) > + return ret; > + > + port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev), > + port->ctrl_id, port->port_id); > + if (!port_match) > + return -ENOMEM; > + > + /* Translate a hardware addressing style console=DEVNAME:0.0 */ > + return serial_base_add_one_prefcon(port_match, drv->dev_name, port->line); > +} > + > +#endif > + > static int serial_base_init(void) > { > int ret; > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -3407,6 +3407,10 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port) > if (ret) > goto err_unregister_ctrl_dev; > > + ret = serial_base_add_preferred_console(drv, port); > + if (ret) > + goto err_unregister_port_dev; > + Looks okay otherwise, Reviewed-by: Dhruva Gole <d-gole@ti.com>
On Thu, Mar 28, 2024 at 12:01:52PM +0530, Dhruva Gole wrote: > Hi, > > On Mar 27, 2024 at 12:59:38 +0200, Tony Lindgren wrote: > > We can now add hardware based addressing for serial ports. Starting with > > commit 84a9582fd203 ("serial: core: Start managing serial controllers to > > enable runtime PM"), and all the related fixes to this commit, the serial > > core now knows to which serial port controller the ports are connected. > > > > The serial ports can be addressed with DEVNAME:0.0 style naming. The names > > are something like 00:04:0.0 for a serial port on qemu, and something like > > 2800000.serial:0.0 on platform device using systems like ARM64 for example. > > > > The DEVNAME is the unique serial port hardware controller device name, AKA > > the name for port->dev. The 0.0 are the serial core controller id and port > > id. > > > > Typically 0.0 are used for each controller and port instance unless the > > serial port hardware controller has multiple controllers or ports. > > > > Using DEVNAME:0.0 style naming actually solves two long term issues for > > addressing the serial ports: > > > > 1. According to Andy Shevchenko, using DEVNAME:0.0 style naming fixes an > > issue where depending on the BIOS settings, the kernel serial port ttyS > > instance number may change if HSUART is enabled > > > > 2. Device tree using architectures no longer necessarily need to specify > > aliases to find a specific serial port, and we can just allocate the > > This is GOOD! > > > ttyS instance numbers dynamically in whatever probe order > > > > To do this, let's match the hardware addressing style console name to > > the character device name used, and add a preferred console using the > > character device name. > > > > Note that when using console=DEVNAME:0.0 style kernel command line, the > > 8250 serial console gets enabled later compared to using console=ttyS > > naming for ISA ports. This is because the serial port DEVNAME to character > > device mapping is not known until the serial driver probe time. If used > > together with earlycon, this issue is avoided. > > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > --- > > drivers/tty/serial/serial_base.h | 16 +++++++ > > drivers/tty/serial/serial_base_bus.c | 66 ++++++++++++++++++++++++++++ > > drivers/tty/serial/serial_core.c | 4 ++ > > 3 files changed, 86 insertions(+) > > > > diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h > > --- a/drivers/tty/serial/serial_base.h > > +++ b/drivers/tty/serial/serial_base.h > > @@ -45,3 +45,19 @@ void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port > > > > int serial_core_register_port(struct uart_driver *drv, struct uart_port *port); > > void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port); > > + > > +#ifdef CONFIG_SERIAL_CORE_CONSOLE > > + > > +int serial_base_add_preferred_console(struct uart_driver *drv, > > + struct uart_port *port); > > + > > +#else > > + > > +static inline > > +int serial_base_add_preferred_console(struct uart_driver *drv, > > + struct uart_port *port) > > +{ > > + return 0; > > +} > > + > > +#endif > > diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c > > --- a/drivers/tty/serial/serial_base_bus.c > > +++ b/drivers/tty/serial/serial_base_bus.c > > @@ -8,6 +8,7 @@ > > * The serial core bus manages the serial core controller instances. > > */ > > > > +#include <linux/cleanup.h> > > #include <linux/container_of.h> > > #include <linux/device.h> > > #include <linux/idr.h> > > @@ -204,6 +205,71 @@ void serial_base_port_device_remove(struct serial_port_device *port_dev) > > put_device(&port_dev->dev); > > } > > > > +#ifdef CONFIG_SERIAL_CORE_CONSOLE > > + > > +static int serial_base_add_one_prefcon(const char *match, const char *dev_name, > > + int port_id) > > +{ > > + int ret; > > + > > + ret = add_preferred_console_match(match, dev_name, port_id); > > + if (ret == -ENOENT) > > + return 0; > > + > > + return ret; > > Can we do this instead? > return (ret == -ENOENT ? 0 : ret); No, please no. Just spell it out, like was done here, dealing with ? : is a pain to read and follow and the generated code should be identical. Only use ? : in places where it's the only way to do it (i.e. as function parameters or in printk-like lines.) Write for people first, compilers second. thanks, greg k-h
diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h --- a/drivers/tty/serial/serial_base.h +++ b/drivers/tty/serial/serial_base.h @@ -45,3 +45,19 @@ void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port int serial_core_register_port(struct uart_driver *drv, struct uart_port *port); void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port); + +#ifdef CONFIG_SERIAL_CORE_CONSOLE + +int serial_base_add_preferred_console(struct uart_driver *drv, + struct uart_port *port); + +#else + +static inline +int serial_base_add_preferred_console(struct uart_driver *drv, + struct uart_port *port) +{ + return 0; +} + +#endif diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c --- a/drivers/tty/serial/serial_base_bus.c +++ b/drivers/tty/serial/serial_base_bus.c @@ -8,6 +8,7 @@ * The serial core bus manages the serial core controller instances. */ +#include <linux/cleanup.h> #include <linux/container_of.h> #include <linux/device.h> #include <linux/idr.h> @@ -204,6 +205,71 @@ void serial_base_port_device_remove(struct serial_port_device *port_dev) put_device(&port_dev->dev); } +#ifdef CONFIG_SERIAL_CORE_CONSOLE + +static int serial_base_add_one_prefcon(const char *match, const char *dev_name, + int port_id) +{ + int ret; + + ret = add_preferred_console_match(match, dev_name, port_id); + if (ret == -ENOENT) + return 0; + + return ret; +} + +static int serial_base_add_prefcon(const char *name, int idx) +{ + const char *char_match __free(kfree) = NULL; + + /* Handle the traditional character device name style console=ttyS0 */ + char_match = kasprintf(GFP_KERNEL, "%s%i", name, idx); + if (!char_match) + return -ENOMEM; + + return serial_base_add_one_prefcon(char_match, name, idx); +} + +/** + * serial_base_add_preferred_console - Adds a preferred console + * @drv: Serial port device driver + * @port: Serial port instance + * + * Tries to add a preferred console for a serial port if specified in the + * kernel command line. Supports both the traditional character device such + * as console=ttyS0, and a hardware addressing based console=DEVNAME:0.0 + * style name. + * + * Translates the kernel command line option using a hardware based addressing + * console=DEVNAME:0.0 to the serial port character device such as ttyS0. + * Cannot be called early for ISA ports, depends on struct device. + * + * Note that duplicates are ignored by add_preferred_console(). + * + * Return: 0 on success, negative error code on failure. + */ +int serial_base_add_preferred_console(struct uart_driver *drv, + struct uart_port *port) +{ + const char *port_match __free(kfree) = NULL; + int ret; + + ret = serial_base_add_prefcon(drv->dev_name, port->line); + if (ret) + return ret; + + port_match = kasprintf(GFP_KERNEL, "%s:%i.%i", dev_name(port->dev), + port->ctrl_id, port->port_id); + if (!port_match) + return -ENOMEM; + + /* Translate a hardware addressing style console=DEVNAME:0.0 */ + return serial_base_add_one_prefcon(port_match, drv->dev_name, port->line); +} + +#endif + static int serial_base_init(void) { int ret; diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -3407,6 +3407,10 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port) if (ret) goto err_unregister_ctrl_dev; + ret = serial_base_add_preferred_console(drv, port); + if (ret) + goto err_unregister_port_dev; + ret = serial_core_add_one_port(drv, port); if (ret) goto err_unregister_port_dev;
We can now add hardware based addressing for serial ports. Starting with commit 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM"), and all the related fixes to this commit, the serial core now knows to which serial port controller the ports are connected. The serial ports can be addressed with DEVNAME:0.0 style naming. The names are something like 00:04:0.0 for a serial port on qemu, and something like 2800000.serial:0.0 on platform device using systems like ARM64 for example. The DEVNAME is the unique serial port hardware controller device name, AKA the name for port->dev. The 0.0 are the serial core controller id and port id. Typically 0.0 are used for each controller and port instance unless the serial port hardware controller has multiple controllers or ports. Using DEVNAME:0.0 style naming actually solves two long term issues for addressing the serial ports: 1. According to Andy Shevchenko, using DEVNAME:0.0 style naming fixes an issue where depending on the BIOS settings, the kernel serial port ttyS instance number may change if HSUART is enabled 2. Device tree using architectures no longer necessarily need to specify aliases to find a specific serial port, and we can just allocate the ttyS instance numbers dynamically in whatever probe order To do this, let's match the hardware addressing style console name to the character device name used, and add a preferred console using the character device name. Note that when using console=DEVNAME:0.0 style kernel command line, the 8250 serial console gets enabled later compared to using console=ttyS naming for ISA ports. This is because the serial port DEVNAME to character device mapping is not known until the serial driver probe time. If used together with earlycon, this issue is avoided. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/tty/serial/serial_base.h | 16 +++++++ drivers/tty/serial/serial_base_bus.c | 66 ++++++++++++++++++++++++++++ drivers/tty/serial/serial_core.c | 4 ++ 3 files changed, 86 insertions(+)