[PATCH/RFC 0/8] UART driver support for BMIPS multiplatform kernels

Use the pxa serial driver to support BCM7xxx's UARTs; add code to allow
native-endian operation on both LE and BE systems.  Enable OF_EARLYCON
in the pxa driver.

After applying these changes I am able to build a multiplatform kernel
that boots to the prompt on BCM6328 (bcm63xx_uart) and BCM7346 (pxa).


Kevin Cernekee (7):
  serial: core: Add big_endian flag
  of: Add helper function to check MMIO register endianness
  serial: pxa: Add fifo-size and {big,native}-endian properties
  serial: pxa: Make the driver buildable for BCM7xxx set-top platforms
  serial: pxa: Update DT binding documentation
  serial: earlycon: Set uart_port->big_endian based on DT properties
  serial: pxa: Add OF_EARLYCON support

Tushar Behera (1):
  tty: Fallback to use dynamic major number

 .../devicetree/bindings/serial/mrvl-serial.txt     | 34 +++++++++++-
 drivers/of/base.c                                  | 23 ++++++++
 drivers/of/fdt.c                                   |  9 +++-
 drivers/tty/serial/Kconfig                         |  2 +-
 drivers/tty/serial/earlycon.c                      |  3 +-
 drivers/tty/serial/pxa.c                           | 62 +++++++++++++++++++++-
 drivers/tty/tty_io.c                               | 19 +++++--
 include/linux/of.h                                 |  6 +++
 include/linux/serial_core.h                        |  5 +-
 9 files changed, 152 insertions(+), 11 deletions(-)

-- 
2.1.1

From: Tushar Behera <redacted>

In a multi-platform scenario, the hard-coded major/minor numbers in
serial drivers may conflict with each other. A typical scenario is
observed with amba-pl011 and samsung-uart drivers, both of these
drivers use same set of major/minor numbers. If both of these drivers
are enabled, probe of samsung-uart driver fails because the desired
node is busy.

The issue is fixed by adding a fallback in driver core, so that we can
use dynamic major number in case device node allocation fails for
hard-coded major/minor number.

Signed-off-by: Tushar Behera <redacted>
[cernekee: fix checkpatch warnings]
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/tty/tty_io.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 0508a1d..a6d4d9a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c

@@ -3365,6 +3365,22 @@ int tty_register_driver(struct tty_driver *driver)
 	dev_t dev;
 	struct device *d;
 
+	if (driver->major) {
+		dev = MKDEV(driver->major, driver->minor_start);
+		error = register_chrdev_region(dev, driver->num, driver->name);
+		/* In case of error, fall back to dynamic allocation */
+		if (error < 0) {
+			pr_warn("Default device node (%d:%d) for %s is busy, using dynamic major number\n",
+				driver->major, driver->minor_start,
+				driver->name);
+			driver->major = 0;
+		}
+	}
+
+	/*
+	 * Don't replace the following check with an else to above if statement,
+	 * as it may also be called as a fallback.
+	 */
 	if (!driver->major) {
 		error = alloc_chrdev_region(&dev, driver->minor_start,
 						driver->num, driver->name);

@@ -3372,9 +3388,6 @@ int tty_register_driver(struct tty_driver *driver)
 			driver->major = MAJOR(dev);
 			driver->minor_start = MINOR(dev);
 		}
-	} else {
-		dev = MKDEV(driver->major, driver->minor_start);
-		error = register_chrdev_region(dev, driver->num, driver->name);
 	}
 	if (error < 0)
 		goto err;

-- 
2.1.1

Add a big_endian flag alongside membase, regshift, and iotype.  Most
drivers currently use readl/writel, but if it is necessary to support
a BE SoC, the driver can check this field to see which MMIO accessor
functions to use.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 include/linux/serial_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 21c2e05..ae372f4 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h

@@ -138,7 +138,7 @@ struct uart_port {
 	unsigned char		x_char;			/* xon/xoff char */
 	unsigned char		regshift;		/* reg offset shift */
 	unsigned char		iotype;			/* io access style */
-	unsigned char		unused1;
+	unsigned char		big_endian;		/* BE registers */
 
 #define UPIO_PORT		(0)
 #define UPIO_HUB6		(1)

-- 
2.1.1

SoC peripherals can come in several different flavors:

 - little-endian: registers always need to be accessed in LE mode (so the
   kernel should perform a swap if the CPU is running BE)

 - big-endian: registers always need to be accessed in BE mode (so the
   kernel should perform a swap if the CPU is running LE)

 - native-endian: the bus will automatically swap accesses, so the kernel
   should never swap

Introduce a function that checks an OF device node to see whether it
contains a "big-endian" or "native-endian" property.  For the former case,
always return 1.  For the latter case, return 1 iff the kernel was built
for BE (implying that the BE MMIO accessors do not perform a swap).
Otherwise return 0, assuming LE registers.

LE registers are assumed by default because most existing drivers (libahci,
serial8250, usb) always use readl/writel in the absence of instructions
to the contrary, so that will be our fallback.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/of/base.c  | 23 +++++++++++++++++++++++
 include/linux/of.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3823edf..9dd494a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c

@@ -552,6 +552,29 @@ int of_device_is_available(const struct device_node *device)
 EXPORT_SYMBOL(of_device_is_available);
 
 /**
+ *  of_device_is_big_endian - check if a device has BE registers
+ *
+ *  @device: Node to check for availability
+ *
+ *  Returns 1 if the device has a "big-endian" property, or if the kernel
+ *  was compiled for BE *and* the device has a "native-endian" property.
+ *  Returns 0 otherwise.
+ *
+ *  Callers would nominally use ioread32be/iowrite32be if
+ *  of_device_is_big_endian() == 1, or readl/writel otherwise.
+ */
+int of_device_is_big_endian(const struct device_node *device)
+{
+	if (of_property_read_bool(device, "big-endian"))
+		return 1;
+	if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) &&
+	    of_property_read_bool(device, "native-endian"))
+		return 1;
+	return 0;
+}
+EXPORT_SYMBOL(of_device_is_big_endian);
+
+/**
  *	of_get_parent - Get a node's parent if any
  *	@node:	Node to get parent
  *

diff --git a/include/linux/of.h b/include/linux/of.h
index 29f0adc..d24ccf3 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h

@@ -276,6 +276,7 @@ extern int of_property_read_string_helper(struct device_node *np,
 extern int of_device_is_compatible(const struct device_node *device,
 				   const char *);
 extern int of_device_is_available(const struct device_node *device);
+extern int of_device_is_big_endian(const struct device_node *device);
 extern const void *of_get_property(const struct device_node *node,
 				const char *name,
 				int *lenp);

@@ -431,6 +432,11 @@ static inline int of_device_is_available(const struct device_node *device)
 	return 0;
 }
 
+static inline int of_device_is_big_endian(const struct device_node *device)
+{
+	return 0;
+}
+
 static inline struct property *of_find_property(const struct device_node *np,
 						const char *name,
 						int *lenp)

-- 
2.1.1

With a few tweaks, the PXA serial driver can handle other 16550A clones.
Add a fifo-size DT property to override the FIFO depth (BCM7xxx uses 32),
and {native,big}-endian properties similar to regmap to support SoCs that
have BE or "automagic endian" registers.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/tty/serial/pxa.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 21b7d8b..78ed7ee 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c

@@ -60,13 +60,19 @@ struct uart_pxa_port {
 static inline unsigned int serial_in(struct uart_pxa_port *up, int offset)
 {
 	offset <<= 2;
-	return readl(up->port.membase + offset);
+	if (!up->port.big_endian)
+		return readl(up->port.membase + offset);
+	else
+		return ioread32be(up->port.membase + offset);
 }
 
 static inline void serial_out(struct uart_pxa_port *up, int offset, int value)
 {
 	offset <<= 2;
-	writel(value, up->port.membase + offset);
+	if (!up->port.big_endian)
+		writel(value, up->port.membase + offset);
+	else
+		iowrite32be(value, up->port.membase + offset);
 }
 
 static void serial_pxa_enable_ms(struct uart_port *port)

@@ -833,6 +839,7 @@ static int serial_pxa_probe_dt(struct platform_device *pdev,
 {
 	struct device_node *np = pdev->dev.of_node;
 	int ret;
+	u32 val;
 
 	if (!np)
 		return 1;

@@ -843,6 +850,11 @@ static int serial_pxa_probe_dt(struct platform_device *pdev,
 		return ret;
 	}
 	sport->port.line = ret;
+
+	if (of_property_read_u32(np, "fifo-size", &val) == 0)
+		sport->port.fifosize = val;
+	sport->port.big_endian = of_device_is_big_endian(np);
+
 	return 0;
 }
 

-- 
2.1.1

Remove the platform dependency in Kconfig and add an appropriate
compatible string.  Note that BCM7401 has one 16550A-compatible UART
in the UPG uart_clk domain, and two proprietary UARTs in the 27 MHz
clock domain.  This driver handles the former one.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/tty/serial/Kconfig | 2 +-
 drivers/tty/serial/pxa.c   | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index fdd851e..2015057 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig

@@ -436,7 +436,7 @@ config SERIAL_MPSC_CONSOLE
 
 config SERIAL_PXA
 	bool "PXA serial port support"
-	depends on ARCH_PXA || ARCH_MMP
+	depends on ARM || MIPS
 	select SERIAL_CORE
 	help
 	  If you have a machine based on an Intel XScale PXA2xx CPU you

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 78ed7ee..f6cc773 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c

@@ -830,6 +830,7 @@ static const struct dev_pm_ops serial_pxa_pm_ops = {
 static struct of_device_id serial_pxa_dt_ids[] = {
 	{ .compatible = "mrvl,pxa-uart", },
 	{ .compatible = "mrvl,mmp-uart", },
+	{ .compatible = "brcm,bcm7401-upg-uart", },
 	{}
 };
 MODULE_DEVICE_TABLE(of, serial_pxa_dt_ids);

-- 
2.1.1

Add a couple of missing required properties; add the new optional
properties and an example.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 .../devicetree/bindings/serial/mrvl-serial.txt     | 34 +++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/serial/mrvl-serial.txt b/Documentation/devicetree/bindings/serial/mrvl-serial.txt
index d744340..5bab455 100644
--- a/Documentation/devicetree/bindings/serial/mrvl-serial.txt
+++ b/Documentation/devicetree/bindings/serial/mrvl-serial.txt

@@ -1,4 +1,36 @@
 PXA UART controller
 
 Required properties:
-- compatible : should be "mrvl,mmp-uart" or "mrvl,pxa-uart".
+- compatible : should be "mrvl,mmp-uart", "mrvl,pxa-uart", or
+  "brcm,bcm7401-uart".
+- interrupts : a single interrupt specifier.
+- clocks : phandle to a clock; used to compute the baud divisor.
+
+Optional properties:
+- fifo-size : defaults to 64 bytes.
+- big-endian : always use BE register accesses.
+- native-endian : use BE register accesses if the kernel was built for BE,
+  otherwise use LE register accesses.
+
+Example:
+
+	clocks {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		uart_clk: uart_clk@0 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <81000000>;
+		};
+	};
+
+	uart0: serial@10406900 {
+		compatible = "brcm,bcm7401-upg-uart";
+		reg = <0x10406900 0x20>;
+		native-endian;
+		fifo-size = <32>;
+		interrupt-parent = <&periph_intc>;
+		interrupts = <64>;
+		clocks = <&uart_clk>;
+	};

-- 
2.1.1

If an earlycon (stdout-path) node is being used, check for "big-endian"
and "native-endian" properties and pass that information to the driver.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/of/fdt.c              | 9 ++++++++-
 drivers/tty/serial/earlycon.c | 3 ++-
 include/linux/serial_core.h   | 3 ++-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 30e97bc..2668097 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c

@@ -10,6 +10,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/kconfig.h>
 #include <linux/initrd.h>
 #include <linux/memblock.h>
 #include <linux/of.h>

@@ -784,7 +785,13 @@ int __init early_init_dt_scan_chosen_serial(void)
 		if (!addr)
 			return -ENXIO;
 
-		of_setup_earlycon(addr, match->data);
+		if (fdt_getprop(fdt, offset, "big-endian", NULL) ||
+		    (fdt_getprop(fdt, offset, "native-endian", NULL) &&
+		     IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))) {
+			of_setup_earlycon(addr, true, match->data);
+		} else {
+			of_setup_earlycon(addr, false, match->data);
+		}
 		return 0;
 	}
 	return -ENODEV;

diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a514ee6..7556280 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c

@@ -148,13 +148,14 @@ int __init setup_earlycon(char *buf, const char *match,
 	return 0;
 }
 
-int __init of_setup_earlycon(unsigned long addr,
+int __init of_setup_earlycon(unsigned long addr, bool big_endian,
 			     int (*setup)(struct earlycon_device *, const char *))
 {
 	int err;
 	struct uart_port *port = &early_console_dev.port;
 
 	port->iotype = UPIO_MEM;
+	port->big_endian = big_endian;
 	port->mapbase = addr;
 	port->uartclk = BASE_BAUD * 16;
 	port->membase = earlycon_map(addr, SZ_4K);

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index ae372f4..1937fca 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h

@@ -29,6 +29,7 @@
 #include <linux/tty.h>
 #include <linux/mutex.h>
 #include <linux/sysrq.h>
+#include <linux/types.h>
 #include <uapi/linux/serial_core.h>
 
 #ifdef CONFIG_SERIAL_CORE_CONSOLE

@@ -309,7 +310,7 @@ struct earlycon_device {
 int setup_earlycon(char *buf, const char *match,
 		   int (*setup)(struct earlycon_device *, const char *));
 
-extern int of_setup_earlycon(unsigned long addr,
+extern int of_setup_earlycon(unsigned long addr, bool big_endian,
 			     int (*setup)(struct earlycon_device *, const char *));
 
 #define EARLYCON_DECLARE(name, func) \

-- 
2.1.1

Implement a bare bones earlycon; this assumes that the bootloader sets
up the tty parameters.  Matches all three compatible strings.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/tty/serial/pxa.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index f6cc773..0764cf5 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c

@@ -761,6 +761,51 @@ static struct console serial_pxa_console = {
 	.data		= &serial_pxa_reg,
 };
 
+static struct uart_pxa_port serial_pxa_early_port;
+
+static void __init early_wait_for_xmitr(struct uart_pxa_port *up)
+{
+	/* it's unsafe to call udelay() in the "early" variant */
+	while ((serial_in(up, UART_LSR) & BOTH_EMPTY) != BOTH_EMPTY)
+		;
+}
+
+static void __init serial_pxa_early_putchar(struct uart_port *port, int ch)
+{
+	struct uart_pxa_port *up = (struct uart_pxa_port *)port;
+
+	early_wait_for_xmitr(up);
+	serial_out(up, UART_TX, ch);
+}
+
+static void __init serial_pxa_early_write(struct console *con, const char *s,
+					  unsigned n)
+{
+	uart_console_write(&serial_pxa_early_port.port, s, n,
+			   serial_pxa_early_putchar);
+	early_wait_for_xmitr(&serial_pxa_early_port);
+}
+
+static int __init serial_pxa_early_console_setup(struct earlycon_device *device,
+						 const char *opt)
+{
+	if (!device->port.membase)
+		return -ENODEV;
+
+	serial_pxa_early_port.port.membase = device->port.membase;
+	serial_pxa_early_port.port.big_endian = device->port.big_endian;
+
+	device->con->write = serial_pxa_early_write;
+	return 0;
+}
+
+OF_EARLYCON_DECLARE(pxa_uart, "mrvl,pxa-uart",
+		    serial_pxa_early_console_setup);
+OF_EARLYCON_DECLARE(mmp_uart, "mrvl,mmp-uart",
+		    serial_pxa_early_console_setup);
+OF_EARLYCON_DECLARE(bcm7401_upg_uart, "brcm,bcm7401-upg-uart",
+		    serial_pxa_early_console_setup);
+
 #define PXA_CONSOLE	&serial_pxa_console
 #else
 #define PXA_CONSOLE	NULL

-- 
2.1.1

On 11/12/2014, 09:46 AM, Kevin Cernekee wrote:

This should actually return bool and use true/false.

-- 
js
suse labs

On Wednesday 12 November 2014 00:46:29 Kevin Cernekee wrote:

How about making this a different UPIO type and using UPIO_MEM32/UPIO_MEM32BE
to tell the difference rather than a separate flag?

	Arnd

On 11/12/2014, 09:46 AM, Kevin Cernekee wrote:

This needn't fly IMO, unless you map the space using iomap (not ioremap).

-- 
js
suse labs

On Wed, Nov 12, 2014 at 12:50 AM, Jiri Slaby [off-list ref] wrote:

quoted

 /**
+ *  of_device_is_big_endian - check if a device has BE registers
+ *
+ *  @device: Node to check for availability

Oops, just noticed a copy/paste error here.

quoted

+ *
+ *  Returns 1 if the device has a "big-endian" property, or if the kernel
+ *  was compiled for BE *and* the device has a "native-endian" property.
+ *  Returns 0 otherwise.
+ *
+ *  Callers would nominally use ioread32be/iowrite32be if
+ *  of_device_is_big_endian() == 1, or readl/writel otherwise.
+ */
+int of_device_is_big_endian(const struct device_node *device)
+{
+     if (of_property_read_bool(device, "big-endian"))
+             return 1;
+     if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) &&
+         of_property_read_bool(device, "native-endian"))
+             return 1;
+     return 0;
+}

This should actually return bool and use true/false.

Well, the other APIs currently return an int:

extern int of_device_is_compatible(const struct device_node *device,
                                   const char *);
extern int of_device_is_available(const struct device_node *device);
[...]
extern int of_machine_is_compatible(const char *compat);

Do you think it is best to change all of them at once, or just the
newly introduced function?

On Wednesday 12 November 2014 00:46:30 Kevin Cernekee wrote:

Remove the platform dependency in Kconfig and add an appropriate
compatible string.  Note that BCM7401 has one 16550A-compatible UART
in the UPG uart_clk domain, and two proprietary UARTs in the 27 MHz
clock domain.  This driver handles the former one.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>

Can you explain why you are using the PXA serial driver instead of the
8250 driver, if this is 16550A compatible? I don't know the history
why PXA is using a separate driver.

	Arnd

On Wednesday 12 November 2014 10:03:58 Jiri Slaby wrote:

On 11/12/2014, 09:46 AM, Kevin Cernekee wrote:

quoted

With a few tweaks, the PXA serial driver can handle other 16550A clones.
Add a fifo-size DT property to override the FIFO depth (BCM7xxx uses 32),
and {native,big}-endian properties similar to regmap to support SoCs that
have BE or "automagic endian" registers.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/tty/serial/pxa.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 21b7d8b..78ed7ee 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c

@@ -60,13 +60,19 @@ struct uart_pxa_port {
 static inline unsigned int serial_in(struct uart_pxa_port *up, int offset)
 {
      offset <<= 2;
-     return readl(up->port.membase + offset);
+     if (!up->port.big_endian)
+             return readl(up->port.membase + offset);
+     else
+             return ioread32be(up->port.membase + offset);

This needn't fly IMO, unless you map the space using iomap (not ioremap).

For all I know, the ioread family is required to work with tokens returned
from ioremap on all architectures. The difference to readl is that it
also works on tokens returned from ioport_map or one of the wrappers
around it.

	Arnd

On Wed, Nov 12, 2014 at 1:04 AM, Arnd Bergmann [off-list ref] wrote:

On Wednesday 12 November 2014 00:46:30 Kevin Cernekee wrote:

quoted

Remove the platform dependency in Kconfig and add an appropriate
compatible string.  Note that BCM7401 has one 16550A-compatible UART
in the UPG uart_clk domain, and two proprietary UARTs in the 27 MHz
clock domain.  This driver handles the former one.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>

Can you explain why you are using the PXA serial driver instead of the
8250 driver, if this is 16550A compatible? I don't know the history
why PXA is using a separate driver.

I wasn't able to get serial8250 to work in any situation where another
driver tried to claim "ttyS"/4/64.

serial8250 calls uart_add_one_port() in its module_init function, even
if the system doesn't have any ports.  Setting nr_uarts
(CONFIG_SERIAL_8250_RUNTIME_UARTS) to 0 doesn't help because
serial8250_find_match_or_unused() will just return NULL.

I guess I could try to rework that logic but several cases would need
to be retested, going back to PCs with ISA buses and PCI add-in cards.
And the differences may be visible to userspace.

The PXA driver seemed like a much cleaner starting point, even if it
was intended for a different SoC.

On 11/12/2014, 10:04 AM, Kevin Cernekee wrote:

quoted

This should actually return bool and use true/false.

Well, the other APIs currently return an int:

extern int of_device_is_compatible(const struct device_node *device,
                                   const char *);
extern int of_device_is_available(const struct device_node *device);
[...]
extern int of_machine_is_compatible(const char *compat);

Do you think it is best to change all of them at once, or just the
newly introduced function?

Possibly fix all these in a separate patch and then add the new one
fixed :).

-- 
js
suse labs

On Wednesday 12 November 2014 01:19:24 Kevin Cernekee wrote:

On Wed, Nov 12, 2014 at 1:04 AM, Arnd Bergmann [off-list ref] wrote:

quoted

On Wednesday 12 November 2014 00:46:30 Kevin Cernekee wrote:

quoted

Remove the platform dependency in Kconfig and add an appropriate
compatible string.  Note that BCM7401 has one 16550A-compatible UART
in the UPG uart_clk domain, and two proprietary UARTs in the 27 MHz
clock domain.  This driver handles the former one.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>

Can you explain why you are using the PXA serial driver instead of the
8250 driver, if this is 16550A compatible? I don't know the history
why PXA is using a separate driver.

I wasn't able to get serial8250 to work in any situation where another
driver tried to claim "ttyS"/4/64.

serial8250 calls uart_add_one_port() in its module_init function, even
if the system doesn't have any ports.  Setting nr_uarts
(CONFIG_SERIAL_8250_RUNTIME_UARTS) to 0 doesn't help because
serial8250_find_match_or_unused() will just return NULL.

I guess I could try to rework that logic but several cases would need
to be retested, going back to PCs with ISA buses and PCI add-in cards.
And the differences may be visible to userspace.

The PXA driver seemed like a much cleaner starting point, even if it
was intended for a different SoC.

Hmm, I've seen you already posted v2 of the series, but I'm not sure
that this is what Greg had in mind when he suggested using /dev/ttyS*
for the other driver.

TTY naming is a mess today, and you seem to be caught in the middle
of it trying to work around the inherent problems. Extending the PXA
driver is an interesting approach since as you say it's a very nice
clean subset of the 8250 driver, but that doesn't mean that it's
a good long-term strategy, as we will likely have more chips with
8250 variants.

Some of the ways forward that I can see are:

- (your approach) use and extend the pxa serial driver for new SoCs,
  possibly migrate some of the existing users of 8250 to use that
  and leave 8250 alone.

- fix the problem you see in a different way, and get the 8250 driver
  to solve your problem. Possibly integrate the pxa driver back into
  8250 in eventually, as we did with the omap driver.

- Do a fresh start for a general-purpose soc-type 8250 driver, using
  tty_port instead of uart_port as the abstraction layer. Use that for
  all new socs instead of extending the 8250 driver more, possibly
  migrating some of the existing 8250 users.

- split out the /dev/ttyS number allocation from the 8250 driver and
  make it usable by arbitrary drivers.

Greg, Jiri, do you have some guidance, or possibly other ideas?

	Arnd

On Thu, Nov 13, 2014 at 1:42 AM, Arnd Bergmann [off-list ref] wrote:

TTY naming is a mess today, and you seem to be caught in the middle
of it trying to work around the inherent problems. Extending the PXA
driver is an interesting approach since as you say it's a very nice
clean subset of the 8250 driver, but that doesn't mean that it's
a good long-term strategy, as we will likely have more chips with
8250 variants.

Some of the ways forward that I can see are:

- (your approach) use and extend the pxa serial driver for new SoCs,
  possibly migrate some of the existing users of 8250 to use that
  and leave 8250 alone.

- fix the problem you see in a different way, and get the 8250 driver
  to solve your problem. Possibly integrate the pxa driver back into
  8250 in eventually, as we did with the omap driver.

Do you think it might make sense to come up with a set of guidelines
that ensure that SoCs using a non-serial8250 driver (like pxa) on
16550-compatible hardware can be easily moved back to serial8250
someday?

e.g. maybe I should be adding a reg-shift property to my pxa DT entry.
It isn't necessary for pxa.c, but if we ever move to serial8250 it
will be necessary.

- Do a fresh start for a general-purpose soc-type 8250 driver, using
  tty_port instead of uart_port as the abstraction layer.

Hmm, does that mean we can't use the serial_core.c helpers?

  Use that for
  all new socs instead of extending the 8250 driver more, possibly
  migrating some of the existing 8250 users.

One nice thing about a brand new driver is that we can use dynamic
major/minor numbers unconditionally without breaking existing users.
If either pxa.c or bcm63xx_uart.c had used dynamic numbers, I could
drop Tushar's original workaround.

Another advantage is that we can assume all users have DT, simplifying
the probe function.

Would it be helpful to split parts of pxa.c and/or serial8250 into a
"lib8250", similar to libahci, that can be called by many different
implementations (some of which have special features like DMA
support)?

On Thursday 13 November 2014 11:08:08 Kevin Cernekee wrote:

On Thu, Nov 13, 2014 at 1:42 AM, Arnd Bergmann [off-list ref] wrote:

quoted

TTY naming is a mess today, and you seem to be caught in the middle
of it trying to work around the inherent problems. Extending the PXA
driver is an interesting approach since as you say it's a very nice
clean subset of the 8250 driver, but that doesn't mean that it's
a good long-term strategy, as we will likely have more chips with
8250 variants.

Some of the ways forward that I can see are:

- (your approach) use and extend the pxa serial driver for new SoCs,
  possibly migrate some of the existing users of 8250 to use that
  and leave 8250 alone.

- fix the problem you see in a different way, and get the 8250 driver
  to solve your problem. Possibly integrate the pxa driver back into
  8250 in eventually, as we did with the omap driver.

Do you think it might make sense to come up with a set of guidelines
that ensure that SoCs using a non-serial8250 driver (like pxa) on
16550-compatible hardware can be easily moved back to serial8250
someday?

e.g. maybe I should be adding a reg-shift property to my pxa DT entry.
It isn't necessary for pxa.c, but if we ever move to serial8250 it
will be necessary.

I'm not sure how many others exist that are 8250-like with different
drivers. I think it would be a good idea to have the properties in
there, but I'm not sure if I can come up with an exhaustive list
of requirements.

quoted

- Do a fresh start for a general-purpose soc-type 8250 driver, using
  tty_port instead of uart_port as the abstraction layer.

Hmm, does that mean we can't use the serial_core.c helpers?

Correct. IIRC the consensus among the tty maintainers has been
for some time that serial_core.c and uart_port doesn't actually do
that much good compared to the complexity it adds in other places.
I may misremember the exact argument.

quoted

  Use that for
  all new socs instead of extending the 8250 driver more, possibly
  migrating some of the existing 8250 users.

One nice thing about a brand new driver is that we can use dynamic
major/minor numbers unconditionally without breaking existing users.
If either pxa.c or bcm63xx_uart.c had used dynamic numbers, I could
drop Tushar's original workaround.

Another advantage is that we can assume all users have DT, simplifying
the probe function.

Would it be helpful to split parts of pxa.c and/or serial8250 into a
"lib8250", similar to libahci, that can be called by many different
implementations (some of which have special features like DMA
support)?

That sounds like a very good idea, yes. I had actually worked on something
in this area years ago, but haven't followed up in some time. In particular,
I think it makes sense to split the specific I/O register access method into
a separate library from the code that knows about the 8250 register set,
and from the code that performs the probing.

	Arnd