2009-10-26 11:12:03

by Shmulik Ladkani

Subject: [PATCH] serial: copy UART properties of UPF_FIXED_TYPE ports provisioned using early_serial_setup

Augment the UPF_FIXED_TYPE logic, which currently applies to UART ports
provisioned using platform_device_register.

The suggested patch applies same logic into 'serial8250_register_ports',
making UART ports provisioned using early_serial_setup inherit their
properties from the uart_config entry.

The patch applies to linux-2.6.32-rc5

Signed-off-by: Shmulik Ladkani <[email protected]>
---
diff -upr linux-2.6.32-rc5.clean/drivers/serial/8250.c
linux-2.6.32-rc5/drivers/serial/8250.c
--- linux-2.6.32-rc5.clean/drivers/serial/8250.c 2009-10-16
02:41:50.000000000 +0200
+++ linux-2.6.32-rc5/drivers/serial/8250.c 2009-10-26 11:36:09.000000000
+0200
@@ -2704,6 +2704,14 @@ serial8250_register_ports(struct uart_dr
struct uart_8250_port *up = &serial8250_ports[i];

up->port.dev = dev;
+
+ if (up->port.flags & UPF_FIXED_TYPE) {
+ up->port.fifosize =
+ uart_config[up->port.type].fifo_size;
+ up->capabilities = uart_config[up->port.type].flags;
+ up->tx_loadsz = uart_config[up->port.type].tx_loadsz;
+ }
+
uart_add_one_port(drv, &up->port);
}
}
--
Shmulik Ladkani Jungo Ltd.

2009-10-29 05:47:52

by Andrew Morton

Subject: Re: [PATCH] serial: copy UART properties of UPF_FIXED_TYPE ports provisioned using early_serial_setup

On Mon, 26 Oct 2009 12:50:07 +0200 Shmulik Ladkani <[email protected]> wrote:

> Augment the UPF_FIXED_TYPE logic, which currently applies to UART ports
> provisioned using platform_device_register.
>
> The suggested patch applies same logic into 'serial8250_register_ports',
> making UART ports provisioned using early_serial_setup inherit their
> properties from the uart_config entry.
>
> The patch applies to linux-2.6.32-rc5
>
> Signed-off-by: Shmulik Ladkani <[email protected]>
> ---
> diff -upr linux-2.6.32-rc5.clean/drivers/serial/8250.c
> linux-2.6.32-rc5/drivers/serial/8250.c
> --- linux-2.6.32-rc5.clean/drivers/serial/8250.c 2009-10-16
> 02:41:50.000000000 +0200
> +++ linux-2.6.32-rc5/drivers/serial/8250.c 2009-10-26 11:36:09.000000000
> +0200

Your email client wordwraps the patches.

> @@ -2704,6 +2704,14 @@ serial8250_register_ports(struct uart_dr
> struct uart_8250_port *up = &serial8250_ports[i];
>
> up->port.dev = dev;
> +
> + if (up->port.flags & UPF_FIXED_TYPE) {
> + up->port.fifosize =
> + uart_config[up->port.type].fifo_size;
> + up->capabilities = uart_config[up->port.type].flags;
> + up->tx_loadsz = uart_config[up->port.type].tx_loadsz;
> + }
> +
> uart_add_one_port(drv, &up->port);
> }
> }

Why don't we also initialise up->port.type here?

Perhaps it would be neater to write a little function to do this rather
than copy-n-paste?

2009-10-29 17:07:40

by Shmulik Ladkani

Subject: Re: [PATCH] serial: copy UART properties of UPF_FIXED_TYPE ports provisioned using early_serial_setup

On Wed, 28 Oct 2009 22:47:37 -0700 Andrew Morton <[email protected]> wrote:
> On Mon, 26 Oct 2009 12:50:07 +0200 Shmulik Ladkani <[email protected]> wrote:
>
> > @@ -2704,6 +2704,14 @@ serial8250_register_ports(struct uart_dr
> > struct uart_8250_port *up = &serial8250_ports[i];
> >
> > up->port.dev = dev;
> > +
> > + if (up->port.flags & UPF_FIXED_TYPE) {
> > + up->port.fifosize =
> > + uart_config[up->port.type].fifo_size;
> > + up->capabilities = uart_config[up->port.type].flags;
> > + up->tx_loadsz = uart_config[up->port.type].tx_loadsz;
> > + }
> > +
> > uart_add_one_port(drv, &up->port);
> > }
> > }
>
> Why don't we also initialise up->port.type here?
In the original place (serial8250_register_port) there's a new uart port
instance to be registered (the function's argument).
This new instance *replaces* one of the matched/unused ports in the
serial8250_ports array - hence, the port's type must be properly assigned.

However, in 'serial8250_register_ports' there's no new port to replace an
existing port; but still, the uart port instance has uninitialized
capabilities/tx_loadsz fields that must be set in the UPF_FIXED_TYPE case.
The up->port.type was already initialized by early_serial_setup, and the patch
uses this type as the index to the uart_config array.

An alternative apprach is to move the UPF_FIXED_TYPE initialization code
into early_serial_setup. Do you think it is better?

> Perhaps it would be neater to write a little function to do this rather
> than copy-n-paste?
Yep, I guess it would be. Re-submitting a neater version of the patch.

Signed-off-by: Shmulik Ladkani <[email protected]>
---
diff -upr linux-2.6.32-rc5.clean/drivers/serial/8250.c linux-2.6.32-rc5/drivers/serial/8250.c
--- linux-2.6.32-rc5.clean/drivers/serial/8250.c 2009-10-16 02:41:50.000000000 +0200
+++ linux-2.6.32-rc5/drivers/serial/8250.c 2009-10-29 18:33:50.000000000 +0200
@@ -2688,6 +2688,15 @@ static void __init serial8250_isa_init_p
}
}

+static void
+serial8250_init_fixed_type_port(struct uart_8250_port *up, unsigned int type)
+{
+ up->port.type = type;
+ up->port.fifosize = uart_config[type].fifo_size;
+ up->capabilities = uart_config[type].flags;
+ up->tx_loadsz = uart_config[type].tx_loadsz;
+}
+
static void __init
serial8250_register_ports(struct uart_driver *drv, struct device *dev)
{
@@ -2704,6 +2713,10 @@ serial8250_register_ports(struct uart_dr
struct uart_8250_port *up = &serial8250_ports[i];

up->port.dev = dev;
+
+ if (up->port.flags & UPF_FIXED_TYPE)
+ serial8250_init_fixed_type_port(up, up->port.type);
+
uart_add_one_port(drv, &up->port);
}
}
@@ -3114,12 +3127,8 @@ int serial8250_register_port(struct uart
if (port->dev)
uart->port.dev = port->dev;

- if (port->flags & UPF_FIXED_TYPE) {
- uart->port.type = port->type;
- uart->port.fifosize = uart_config[port->type].fifo_size;
- uart->capabilities = uart_config[port->type].flags;
- uart->tx_loadsz = uart_config[port->type].tx_loadsz;
- }
+ if (port->flags & UPF_FIXED_TYPE)
+ serial8250_init_fixed_type_port(uart, port->type);

set_io_from_upio(&uart->port);
/* Possibly override default I/O functions. */

--
Shmulik Ladkani Jungo Ltd.