[RFC PATCH 0/2] serial: Hack up 8520.c for evil device tree hookin.

I wanted to get some feedback on my attempts to control my serial
device registration with device tree data.

It was suggested by Grant Likely that I could use of_serial.c for my
purposes.  However I think it is simpler to do my own registration
code, because I need to set my own device type, required port flags,
and I/O functions, instead of using general purpose ones.  Adding this
to of_serial.c would be quite ugly, so I would like to keep it with my
chip/board code.

This leads to a problem: You cannot call serial8250_register_port()
until the 8250.c driver is registered.  of_serial.c is lucky that it
comes after 8250.c in the make file so that the device initcall order
is correct.  When I put the code in my board's serial.c file, I am not
so lucky.  To get the initialization order correct, I add a notifier
chain to 8250.c

What say you all to this approach?

David Daney (2):
  serial: 8250: Add a notifier chain for driver registration.
  MIPS: Octeon: Use device tree to register serial ports.

 arch/mips/cavium-octeon/serial.c |  140 ++++++++++++++++----------------------
 drivers/tty/serial/8250.c        |   20 ++++++
 include/linux/serial_8250.h      |   21 ++++++
 3 files changed, 101 insertions(+), 80 deletions(-)

-- 
1.7.2.3

The 8250 driver is a bit weird in that in addition to supporting
platform devices, extra devices can be added by calling
serial8250_register_port().

The problem is that if we call serial8250_register_port() before the
driver is initialized Bad Things happen (we dereference NULL
pointers).

There doesn't seem to be a general way to know if a driver has been
initialized, so we add a notifier chain just for this driver.  When the
SERIAL8250_DRIVER_ADD notifier is called, we know it is safe to call
serial8250_register_port(), and we can add our devices.

Signed-off-by: David Daney <redacted>
---
 drivers/tty/serial/8250.c   |   20 ++++++++++++++++++++
 include/linux/serial_8250.h |   21 +++++++++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 3975df6..229a1d2 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c

@@ -3281,6 +3281,20 @@ void serial8250_unregister_port(int line)
 }
 EXPORT_SYMBOL(serial8250_unregister_port);
 
+static BLOCKING_NOTIFIER_HEAD(serial8250_notifier);
+
+int serial8250_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&serial8250_notifier, nb);
+}
+EXPORT_SYMBOL(serial8250_register_notifier);
+
+int serial8250_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&serial8250_notifier, nb);
+}
+EXPORT_SYMBOL(serial8250_unregister_notifier);
+
 static int __init serial8250_init(void)
 {
 	int ret;

@@ -3328,6 +3342,10 @@ unreg_uart_drv:
 	uart_unregister_driver(&serial8250_reg);
 #endif
 out:
+	if (ret == 0)
+		blocking_notifier_call_chain(&serial8250_notifier,
+					     SERIAL8250_DRIVER_ADD, NULL);
+
 	return ret;
 }
 

@@ -3335,6 +3353,8 @@ static void __exit serial8250_exit(void)
 {
 	struct platform_device *isa_dev = serial8250_isa_devs;
 
+	blocking_notifier_call_chain(&serial8250_notifier,
+				     SERIAL8250_DRIVER_REMOVE, NULL);
 	/*
 	 * This tells serial8250_unregister_port() not to re-register
 	 * the ports (thereby making serial8250_isa_driver permanently

diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 97f5b45..2155c28 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h

@@ -11,6 +11,7 @@
 #ifndef _LINUX_SERIAL_8250_H
 #define _LINUX_SERIAL_8250_H
 
+#include <linux/notifier.h>
 #include <linux/serial_core.h>
 #include <linux/platform_device.h>
 

@@ -85,4 +86,24 @@ extern void serial8250_set_isa_configurator(void (*v)
 					(int port, struct uart_port *up,
 						unsigned short *capabilities));
 
+extern int serial8250_register_notifier(struct notifier_block *nb);
+extern int serial8250_unregister_notifier(struct notifier_block *nb);
+/*
+ * Notifiers get called to allow serial8250_register_port() after the
+ * driver is registered, and serial8250_unregister_port() to be called
+ * before it is unregistered.
+ */
+#define SERIAL8250_DRIVER_ADD 1
+#define SERIAL8250_DRIVER_REMOVE 2
+
+#define serial8250_notifier(fn, pri)				\
+({								\
+	static struct notifier_block fn##_nb = {		\
+		.notifier_call = fn,				\
+		.priority = pri					\
+	};							\
+								\
+	serial8250_register_notifier(&fn##_nb);			\
+})
+
 #endif

-- 
1.7.2.3

Switch to using the device tree to register serial ports.

After the serial8250 driver indicates that it is initialized, add all
the ports with compatible = "cavium,octeon-3860-uart".  Octeon serial
ports have their own device type, required port flags, and I/O
functions, so using of_serial.c is not indicated.

Signed-off-by: David Daney <redacted>
---
 arch/mips/cavium-octeon/serial.c |  140 ++++++++++++++++----------------------
 1 files changed, 60 insertions(+), 80 deletions(-)

diff --git a/arch/mips/cavium-octeon/serial.c b/arch/mips/cavium-octeon/serial.c
index 057f0ae..28cacac 100644
--- a/arch/mips/cavium-octeon/serial.c
+++ b/arch/mips/cavium-octeon/serial.c

@@ -43,95 +43,75 @@ void octeon_serial_out(struct uart_port *up, int offset, int value)
 	cvmx_write_csr((uint64_t)(up->membase + (offset << 3)), (u8)value);
 }
 
-/*
- * Allocated in .bss, so it is all zeroed.
- */
-#define OCTEON_MAX_UARTS 3
-static struct plat_serial8250_port octeon_uart8250_data[OCTEON_MAX_UARTS + 1];
-static struct platform_device octeon_uart8250_device = {
-	.name			= "serial8250",
-	.id			= PLAT8250_DEV_PLATFORM,
-	.dev			= {
-		.platform_data	= octeon_uart8250_data,
-	},
-};
-
-static void __init octeon_uart_set_common(struct plat_serial8250_port *p)
+static int __devinit octeon_serial_probe(struct platform_device *pdev)
 {
-	p->flags = ASYNC_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
-	p->type = PORT_OCTEON;
-	p->iotype = UPIO_MEM;
-	p->regshift = 3;	/* I/O addresses are every 8 bytes */
+	int irq, res;
+	struct resource *res_mem;
+	struct uart_port port;
+
+	/* All adaptors have an irq.  */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	memset(&port, 0, sizeof(port));
+
+	port.flags = ASYNC_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
+	port.type = PORT_OCTEON;
+	port.iotype = UPIO_MEM;
+	port.regshift = 3;
+	port.dev = &pdev->dev;
+
 	if (octeon_is_simulation())
 		/* Make simulator output fast*/
-		p->uartclk = 115200 * 16;
+		port.uartclk = 115200 * 16;
 	else
-		p->uartclk = octeon_get_io_clock_rate();
-	p->serial_in = octeon_serial_in;
-	p->serial_out = octeon_serial_out;
-}
+		port.uartclk = octeon_get_io_clock_rate();
 
-static int __init octeon_serial_init(void)
-{
-	int enable_uart0;
-	int enable_uart1;
-	int enable_uart2;
-	struct plat_serial8250_port *p;
+	port.serial_in = octeon_serial_in;
+	port.serial_out = octeon_serial_out;
+	port.irq = irq;
 
-#ifdef CONFIG_CAVIUM_OCTEON_2ND_KERNEL
-	/*
-	 * If we are configured to run as the second of two kernels,
-	 * disable uart0 and enable uart1. Uart0 is owned by the first
-	 * kernel
-	 */
-	enable_uart0 = 0;
-	enable_uart1 = 1;
-#else
-	/*
-	 * We are configured for the first kernel. We'll enable uart0
-	 * if the bootloader told us to use 0, otherwise will enable
-	 * uart 1.
-	 */
-	enable_uart0 = (octeon_get_boot_uart() == 0);
-	enable_uart1 = (octeon_get_boot_uart() == 1);
-#ifdef CONFIG_KGDB
-	enable_uart1 = 1;
-#endif
-#endif
-
-	/* Right now CN52XX is the only chip with a third uart */
-	enable_uart2 = OCTEON_IS_MODEL(OCTEON_CN52XX);
-
-	p = octeon_uart8250_data;
-	if (enable_uart0) {
-		/* Add a ttyS device for hardware uart 0 */
-		octeon_uart_set_common(p);
-		p->membase = (void *) CVMX_MIO_UARTX_RBR(0);
-		p->mapbase = CVMX_MIO_UARTX_RBR(0) & ((1ull << 49) - 1);
-		p->irq = OCTEON_IRQ_UART0;
-		p++;
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res_mem == NULL) {
+		dev_err(&pdev->dev, "found no memory resource\n");
+		return -ENXIO;
 	}
+	port.mapbase = res_mem->start;
+	port.membase = ioremap(res_mem->start, resource_size(res_mem));
 
-	if (enable_uart1) {
-		/* Add a ttyS device for hardware uart 1 */
-		octeon_uart_set_common(p);
-		p->membase = (void *) CVMX_MIO_UARTX_RBR(1);
-		p->mapbase = CVMX_MIO_UARTX_RBR(1) & ((1ull << 49) - 1);
-		p->irq = OCTEON_IRQ_UART1;
-		p++;
-	}
-	if (enable_uart2) {
-		/* Add a ttyS device for hardware uart 2 */
-		octeon_uart_set_common(p);
-		p->membase = (void *) CVMX_MIO_UART2_RBR;
-		p->mapbase = CVMX_MIO_UART2_RBR & ((1ull << 49) - 1);
-		p->irq = OCTEON_IRQ_UART2;
-		p++;
-	}
+	res = serial8250_register_port(&port);
+
+	return res >= 0 ? 0 : res;
+}
+
+static struct of_device_id octeon_serial_match[] = {
+	{
+		.compatible = "cavium,octeon-3860-uart",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, octeon_serial_match);
 
-	BUG_ON(p > &octeon_uart8250_data[OCTEON_MAX_UARTS]);
+static struct platform_driver octeon_serial_driver = {
+	.probe		= octeon_serial_probe,
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "octeon_serial",
+		.of_match_table = octeon_serial_match,
+	},
+};
 
-	return platform_device_register(&octeon_uart8250_device);
+static int octeon_serial_register_driver(struct notifier_block *nb,
+					 unsigned long code, void *arg)
+{
+	if (code == SERIAL8250_DRIVER_ADD)
+		platform_driver_register(&octeon_serial_driver);
+	return NOTIFY_OK;
 }
 
-device_initcall(octeon_serial_init);
+static int __init octeon_serial_init(void)
+{
+	return serial8250_notifier(octeon_serial_register_driver, 0);
+}
+arch_initcall(octeon_serial_init);

-- 
1.7.2.3

is correct.  When I put the code in my board's serial.c file, I am not
so lucky.  To get the initialization order correct, I add a notifier
chain to 8250.c

Can't you use a late_initcall ? or indeed we could make the 8250 core
functionality only init first ?

Alan

On Wed, 16 Mar 2011 18:26:06 -0700
David Daney [off-list ref] wrote:

The 8250 driver is a bit weird in that in addition to supporting
platform devices, extra devices can be added by calling
serial8250_register_port().

The problem is that if we call serial8250_register_port() before the
driver is initialized Bad Things happen (we dereference NULL
pointers).

There doesn't seem to be a general way to know if a driver has been
initialized

I've had a bigger dig into this. I think the correct answer is probably
"always go via platform devices or similar". That *is* the notifier in
the kernel of today. serial8250_register_port ultimately should I think
ultimatly become an internal helper.

Alan

On 03/17/2011 05:18 AM, Alan Cox wrote:

On Wed, 16 Mar 2011 18:26:06 -0700
David Daney[off-list ref]  wrote:

quoted

The 8250 driver is a bit weird in that in addition to supporting
platform devices, extra devices can be added by calling
serial8250_register_port().

The problem is that if we call serial8250_register_port() before the
driver is initialized Bad Things happen (we dereference NULL
pointers).

There doesn't seem to be a general way to know if a driver has been
initialized

I've had a bigger dig into this. I think the correct answer is probably
"always go via platform devices or similar". That *is* the notifier in
the kernel of today. serial8250_register_port ultimately should I think
ultimatly become an internal helper.

That was kind of my thought too.  However we have all sorts of things 
calling serial8250_register_port(). Things like:

8250_pci.c
of_serial.c
8250_acorn.c
8250_gsc.c
.
.
.

The resulting view of the drivers in sysfs is that the little stub code 
that calls serial8250_register_port() is shown as the driver rather than 
serial8250.  But I suppose that is a matter of aesthetics more than 
function.

All those 'stub drivers' are relying on the ordering of module_init 
calls caused indirectly by the Makefile layout.  The path of least 
resistance is your suggestion that I use late_initcall() in my driver 
stub.  I actually tried that before hacking up this patch, but didn't 
like the idea of relying on *_initcall() ordering being necessary for 
correct initialization.

David Daney

On Thu, Mar 17, 2011 at 12:18:49PM +0000, Alan Cox wrote:

On Wed, 16 Mar 2011 18:26:06 -0700
David Daney [off-list ref] wrote:

quoted

The 8250 driver is a bit weird in that in addition to supporting
platform devices, extra devices can be added by calling
serial8250_register_port().

The problem is that if we call serial8250_register_port() before the
driver is initialized Bad Things happen (we dereference NULL
pointers).

There doesn't seem to be a general way to know if a driver has been
initialized

I've had a bigger dig into this. I think the correct answer is probably
"always go via platform devices or similar". That *is* the notifier in
the kernel of today. serial8250_register_port ultimately should I think
ultimatly become an internal helper.

+1

Depending on serial8250_register_port() definitely the wrong thing to
do for platform support code.  It would be better to figure out how to
get the dt bits you need into 8250.c or of_serial.c.

g.

On Wed, Mar 16, 2011 at 06:26:07PM -0700, David Daney wrote:

Switch to using the device tree to register serial ports.

After the serial8250 driver indicates that it is initialized, add all
the ports with compatible = "cavium,octeon-3860-uart".  Octeon serial
ports have their own device type, required port flags, and I/O
functions, so using of_serial.c is not indicated.

What specifically is the list extra settings needed to make of_serial
work?  From what I can see in this driver, nothing looks particularly
troublesome to handle.

g.

On 03/17/2011 11:28 AM, Grant Likely wrote:

On Wed, Mar 16, 2011 at 06:26:07PM -0700, David Daney wrote:

quoted

Switch to using the device tree to register serial ports.

After the serial8250 driver indicates that it is initialized, add all
the ports with compatible = "cavium,octeon-3860-uart".  Octeon serial
ports have their own device type, required port flags, and I/O
functions, so using of_serial.c is not indicated.

What specifically is the list extra settings needed to make of_serial
work?  From what I can see in this driver, nothing looks particularly
troublesome to handle.

Specifically:

+	port.flags = ASYNC_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
+	port.type = PORT_OCTEON;
+	port.serial_in = octeon_serial_in;
+	port.serial_out = octeon_serial_out;


This one is iffy:
+	port.uartclk = octeon_get_io_clock_rate();



Those octeon_serial_in() and octeon_serial_out() are chip specific code 
that does not belong in a generic driver file like of_serial.c

David Daney

g.

quoted

Signed-off-by: David Daney<redacted>
---
  arch/mips/cavium-octeon/serial.c |  140 ++++++++++++++++----------------------
  1 files changed, 60 insertions(+), 80 deletions(-)

diff --git a/arch/mips/cavium-octeon/serial.c b/arch/mips/cavium-octeon/serial.c
index 057f0ae..28cacac 100644
--- a/arch/mips/cavium-octeon/serial.c
+++ b/arch/mips/cavium-octeon/serial.c

@@ -43,95 +43,75 @@ void octeon_serial_out(struct uart_port *up, int offset, int value)
  	cvmx_write_csr((uint64_t)(up->membase + (offset<<  3)), (u8)value);
  }

-/*
- * Allocated in .bss, so it is all zeroed.
- */
-#define OCTEON_MAX_UARTS 3
-static struct plat_serial8250_port octeon_uart8250_data[OCTEON_MAX_UARTS + 1];
-static struct platform_device octeon_uart8250_device = {
-	.name			= "serial8250",
-	.id			= PLAT8250_DEV_PLATFORM,
-	.dev			= {
-		.platform_data	= octeon_uart8250_data,
-	},
-};
-
-static void __init octeon_uart_set_common(struct plat_serial8250_port *p)
+static int __devinit octeon_serial_probe(struct platform_device *pdev)
  {
-	p->flags = ASYNC_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
-	p->type = PORT_OCTEON;
-	p->iotype = UPIO_MEM;
-	p->regshift = 3;	/* I/O addresses are every 8 bytes */
+	int irq, res;
+	struct resource *res_mem;
+	struct uart_port port;
+
+	/* All adaptors have an irq.  */
+	irq = platform_get_irq(pdev, 0);
+	if (irq<  0)
+		return irq;
+
+	memset(&port, 0, sizeof(port));
+
+	port.flags = ASYNC_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
+	port.type = PORT_OCTEON;
+	port.iotype = UPIO_MEM;
+	port.regshift = 3;
+	port.dev =&pdev->dev;
+
  	if (octeon_is_simulation())
  		/* Make simulator output fast*/
-		p->uartclk = 115200 * 16;
+		port.uartclk = 115200 * 16;
  	else
-		p->uartclk = octeon_get_io_clock_rate();
-	p->serial_in = octeon_serial_in;
-	p->serial_out = octeon_serial_out;
-}
+		port.uartclk = octeon_get_io_clock_rate();

-static int __init octeon_serial_init(void)
-{
-	int enable_uart0;
-	int enable_uart1;
-	int enable_uart2;
-	struct plat_serial8250_port *p;
+	port.serial_in = octeon_serial_in;
+	port.serial_out = octeon_serial_out;
+	port.irq = irq;

-#ifdef CONFIG_CAVIUM_OCTEON_2ND_KERNEL
-	/*
-	 * If we are configured to run as the second of two kernels,
-	 * disable uart0 and enable uart1. Uart0 is owned by the first
-	 * kernel
-	 */
-	enable_uart0 = 0;
-	enable_uart1 = 1;
-#else
-	/*
-	 * We are configured for the first kernel. We'll enable uart0
-	 * if the bootloader told us to use 0, otherwise will enable
-	 * uart 1.
-	 */
-	enable_uart0 = (octeon_get_boot_uart() == 0);
-	enable_uart1 = (octeon_get_boot_uart() == 1);
-#ifdef CONFIG_KGDB
-	enable_uart1 = 1;
-#endif
-#endif
-
-	/* Right now CN52XX is the only chip with a third uart */
-	enable_uart2 = OCTEON_IS_MODEL(OCTEON_CN52XX);
-
-	p = octeon_uart8250_data;
-	if (enable_uart0) {
-		/* Add a ttyS device for hardware uart 0 */
-		octeon_uart_set_common(p);
-		p->membase = (void *) CVMX_MIO_UARTX_RBR(0);
-		p->mapbase = CVMX_MIO_UARTX_RBR(0)&  ((1ull<<  49) - 1);
-		p->irq = OCTEON_IRQ_UART0;
-		p++;
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res_mem == NULL) {
+		dev_err(&pdev->dev, "found no memory resource\n");
+		return -ENXIO;
  	}
+	port.mapbase = res_mem->start;
+	port.membase = ioremap(res_mem->start, resource_size(res_mem));

-	if (enable_uart1) {
-		/* Add a ttyS device for hardware uart 1 */
-		octeon_uart_set_common(p);
-		p->membase = (void *) CVMX_MIO_UARTX_RBR(1);
-		p->mapbase = CVMX_MIO_UARTX_RBR(1)&  ((1ull<<  49) - 1);
-		p->irq = OCTEON_IRQ_UART1;
-		p++;
-	}
-	if (enable_uart2) {
-		/* Add a ttyS device for hardware uart 2 */
-		octeon_uart_set_common(p);
-		p->membase = (void *) CVMX_MIO_UART2_RBR;
-		p->mapbase = CVMX_MIO_UART2_RBR&  ((1ull<<  49) - 1);
-		p->irq = OCTEON_IRQ_UART2;
-		p++;
-	}
+	res = serial8250_register_port(&port);
+
+	return res>= 0 ? 0 : res;
+}
+
+static struct of_device_id octeon_serial_match[] = {
+	{
+		.compatible = "cavium,octeon-3860-uart",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, octeon_serial_match);

-	BUG_ON(p>  &octeon_uart8250_data[OCTEON_MAX_UARTS]);
+static struct platform_driver octeon_serial_driver = {
+	.probe		= octeon_serial_probe,
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "octeon_serial",
+		.of_match_table = octeon_serial_match,
+	},
+};

-	return platform_device_register(&octeon_uart8250_device);
+static int octeon_serial_register_driver(struct notifier_block *nb,
+					 unsigned long code, void *arg)
+{
+	if (code == SERIAL8250_DRIVER_ADD)
+		platform_driver_register(&octeon_serial_driver);
+	return NOTIFY_OK;
  }

-device_initcall(octeon_serial_init);
+static int __init octeon_serial_init(void)
+{
+	return serial8250_notifier(octeon_serial_register_driver, 0);
+}
+arch_initcall(octeon_serial_init);
--

1.7.2.3

On 03/17/2011 11:25 AM, Grant Likely wrote:

On Thu, Mar 17, 2011 at 12:18:49PM +0000, Alan Cox wrote:

quoted

On Wed, 16 Mar 2011 18:26:06 -0700
David Daney[off-list ref]  wrote:

quoted

The 8250 driver is a bit weird in that in addition to supporting
platform devices, extra devices can be added by calling
serial8250_register_port().

The problem is that if we call serial8250_register_port() before the
driver is initialized Bad Things happen (we dereference NULL
pointers).

There doesn't seem to be a general way to know if a driver has been
initialized

I've had a bigger dig into this. I think the correct answer is probably
"always go via platform devices or similar". That *is* the notifier in
the kernel of today. serial8250_register_port ultimately should I think
ultimatly become an internal helper.

+1

Depending on serial8250_register_port() definitely the wrong thing to
do for platform support code.  It would be better to figure out how to
get the dt bits you need into 8250.c or of_serial.c.

IMHO, of_serial.c is no better than my board/chip specific code that 
calls serial8250_register_port().

Really what would be ideal would be a hook to add a dev.platform_data 
pointer to the appropriate struct plat_serial8250_port when the platform 
device is created.  It is possible that the platform bus notifiers could 
be used for this.  We would also want to have a way to add an 
of_device_id to those recognized by 8250.c

If we did that, serial8250_probe() would automatically do the right thing.

David Daney

On Thu, Mar 17, 2011 at 11:42:19AM -0700, David Daney wrote:

On 03/17/2011 11:25 AM, Grant Likely wrote:

quoted

On Thu, Mar 17, 2011 at 12:18:49PM +0000, Alan Cox wrote:

quoted

On Wed, 16 Mar 2011 18:26:06 -0700
David Daney[off-list ref]  wrote:

quoted

The 8250 driver is a bit weird in that in addition to supporting
platform devices, extra devices can be added by calling
serial8250_register_port().

The problem is that if we call serial8250_register_port() before the
driver is initialized Bad Things happen (we dereference NULL
pointers).

There doesn't seem to be a general way to know if a driver has been
initialized

I've had a bigger dig into this. I think the correct answer is probably
"always go via platform devices or similar". That *is* the notifier in
the kernel of today. serial8250_register_port ultimately should I think
ultimatly become an internal helper.

+1

Depending on serial8250_register_port() definitely the wrong thing to
do for platform support code.  It would be better to figure out how to
get the dt bits you need into 8250.c or of_serial.c.

IMHO, of_serial.c is no better than my board/chip specific code that
calls serial8250_register_port().

Really what would be ideal would be a hook to add a
dev.platform_data pointer to the appropriate struct
plat_serial8250_port when the platform device is created.  It is
possible that the platform bus notifiers could be used for this.  We
would also want to have a way to add an of_device_id to those
recognized by 8250.c

If we did that, serial8250_probe() would automatically do the right thing.

Take a look at the way arch/powerpc/platforms/512x/pdm360ng.c uses a
notifier for amending a platform_device with additional data..

g.

quoted

If we did that, serial8250_probe() would automatically do the right thing.

Take a look at the way arch/powerpc/platforms/512x/pdm360ng.c uses a
notifier for amending a platform_device with additional data..

I tend to view arch specific embedded code as rather like very dubious
parties. What goes on in other peoples' house out of sight is none of my
business.

The 8250 however is core code so it should keep its clothers on and behave
in a manner befitting its status.

What part of the problem can't be solved by doing it properly using the
device registration interfaces we have today ?

Alan

On Thu, Mar 17, 2011 at 07:24:41PM +0000, Alan Cox wrote:

quoted

quoted

If we did that, serial8250_probe() would automatically do the right thing.

Take a look at the way arch/powerpc/platforms/512x/pdm360ng.c uses a
notifier for amending a platform_device with additional data..

I tend to view arch specific embedded code as rather like very dubious
parties. What goes on in other peoples' house out of sight is none of my
business.

The 8250 however is core code so it should keep its clothers on and behave
in a manner befitting its status.

What part of the problem can't be solved by doing it properly using the
device registration interfaces we have today ?

Device registration isn't the problem.  The problem is supplying
machine-specific callbacks from the board support code to the
drivers.  When devices are sourced from a device tree, it is easy to
get data about the device out of the tree, but it is really hard to
get callback pointers.  To make it all work without this fiddling
about, the octeon serial_{in,out} implementation would need to be
rolled into of_serial.c (which FWIW, I have absolutely no problem
with).

g.

On 03/17/2011 12:31 PM, Grant Likely wrote:

On Thu, Mar 17, 2011 at 07:24:41PM +0000, Alan Cox wrote:

quoted

quoted

quoted

If we did that, serial8250_probe() would automatically do the right thing.

Take a look at the way arch/powerpc/platforms/512x/pdm360ng.c uses a
notifier for amending a platform_device with additional data..

I tend to view arch specific embedded code as rather like very dubious
parties. What goes on in other peoples' house out of sight is none of my
business.

The 8250 however is core code so it should keep its clothers on and behave
in a manner befitting its status.

What part of the problem can't be solved by doing it properly using the
device registration interfaces we have today ?

Device registration isn't the problem.  The problem is supplying
machine-specific callbacks from the board support code to the
drivers.  When devices are sourced from a device tree, it is easy to
get data about the device out of the tree, but it is really hard to
get callback pointers.  To make it all work without this fiddling
about, the octeon serial_{in,out} implementation would need to be
rolled into of_serial.c (which FWIW, I have absolutely no problem
with).

The only problem I have with that is that it ends up moving chip 
specific erratum workarounds into drivers/tty/serial instead of 
arch/mips/cavium-octeon.

I will think about this more.

David Daney

On Thu, Mar 17, 2011 at 01:13:06PM -0700, David Daney wrote:

On 03/17/2011 12:31 PM, Grant Likely wrote:

quoted

On Thu, Mar 17, 2011 at 07:24:41PM +0000, Alan Cox wrote:

quoted

quoted

quoted

If we did that, serial8250_probe() would automatically do the right thing.

Take a look at the way arch/powerpc/platforms/512x/pdm360ng.c uses a
notifier for amending a platform_device with additional data..

I tend to view arch specific embedded code as rather like very dubious
parties. What goes on in other peoples' house out of sight is none of my
business.

The 8250 however is core code so it should keep its clothers on and behave
in a manner befitting its status.

What part of the problem can't be solved by doing it properly using the
device registration interfaces we have today ?

Device registration isn't the problem.  The problem is supplying
machine-specific callbacks from the board support code to the
drivers.  When devices are sourced from a device tree, it is easy to
get data about the device out of the tree, but it is really hard to
get callback pointers.  To make it all work without this fiddling
about, the octeon serial_{in,out} implementation would need to be
rolled into of_serial.c (which FWIW, I have absolutely no problem
with).

The only problem I have with that is that it ends up moving chip
specific erratum workarounds into drivers/tty/serial instead of
arch/mips/cavium-octeon.

I will think about this more.

arch/mips/cavium-octeon/serial.c appears to be void of any
chip-specific errata at the moment.  :-)

g.

Device registration isn't the problem.  The problem is supplying
machine-specific callbacks from the board support code to the
drivers.  When devices are sourced from a device tree, it is easy to
get data about the device out of the tree, but it is really hard to
get callback pointers.  To make it all work without this fiddling
about, the octeon serial_{in,out} implementation would need to be
rolled into of_serial.c (which FWIW, I have absolutely no problem
with).

Disagree - the arch code needs to register I/O method descriptions with
the of_serial code they don't neccessarily need to be in it.

Ie you'd have something like

	of_serial8250_register_ops("dt-op-type-name-blah", &ops);

in the early boot code, and the ops can be in the arch, providing the ops
has a module owner field the rest can even work modular. Sure the stuff
should be able to describe standard forms directly without extra methods
being registered but for the special stuff I think that is the right
approach

Funnily enough I'm in the middle of trying to rip the rm9k, au and other
crap out of 8250.c by doing this for the UPIO_xxx ids and once you have
an ops struct you can also then go and boot out the resource claim crap
and package it all nicely.

On Thu, Mar 17, 2011 at 11:48:14PM +0000, Alan Cox wrote:

quoted

Device registration isn't the problem.  The problem is supplying
machine-specific callbacks from the board support code to the
drivers.  When devices are sourced from a device tree, it is easy to
get data about the device out of the tree, but it is really hard to
get callback pointers.  To make it all work without this fiddling
about, the octeon serial_{in,out} implementation would need to be
rolled into of_serial.c (which FWIW, I have absolutely no problem
with).

Disagree - the arch code needs to register I/O method descriptions with
the of_serial code they don't neccessarily need to be in it.

Ie you'd have something like

	of_serial8250_register_ops("dt-op-type-name-blah", &ops);

in the early boot code, and the ops can be in the arch, providing the ops
has a module owner field the rest can even work modular. Sure the stuff
should be able to describe standard forms directly without extra methods
being registered but for the special stuff I think that is the right
approach

Yeah, okay.  That's nice and clean.  I like it.

Funnily enough I'm in the middle of trying to rip the rm9k, au and other
crap out of 8250.c by doing this for the UPIO_xxx ids and once you have
an ops struct you can also then go and boot out the resource claim crap
and package it all nicely.

:-)

g.