[PATCH] Handle reg-shift property for of_serial ports

[PATCH] Handle reg-shift property for of_serial ports

[David Woodhouse]

The MV64660 has reg-shift==2 for its otherwise 16550-compatible uarts.
While the bootwrapper copes with this, of_serial.c doesn't. (The udbg
code doesn't either, but I'll fix that later).

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
index 7ffdaea..0761ff9 100644
--- a/drivers/serial/of_serial.c
+++ b/drivers/serial/of_serial.c
@@ -25,12 +25,13 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
 {
 	struct resource resource;
 	struct device_node *np = ofdev->node;
-	const unsigned int *clk, *spd;
+	const unsigned int *clk, *spd, *rs;
 	int ret;
 
 	memset(port, 0, sizeof *port);
 	spd = of_get_property(np, "current-speed", NULL);
 	clk = of_get_property(np, "clock-frequency", NULL);
+	rs = of_get_property(np, "reg-shift", NULL);
 	if (!clk) {
 		dev_warn(&ofdev->dev, "no clock-frequency property set\n");
 		return -ENODEV;
@@ -48,6 +49,8 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
 	port->iotype = UPIO_MEM;
 	port->type = type;
 	port->uartclk = *clk;
+	if (rs)
+		port->regshift = *rs;
 	port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP
 		| UPF_FIXED_PORT;
 	port->dev = &ofdev->dev;


-- 
dwmw2

Re: [PATCH] Handle reg-shift property for of_serial ports

[Arnd Bergmann]

On Saturday 07 July 2007, David Woodhouse wrote:
> 
> The MV64660 has reg-shift==2 for its otherwise 16550-compatible uarts.
> While the bootwrapper copes with this, of_serial.c doesn't. (The udbg
> code doesn't either, but I'll fix that later).
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Given the existence of the boards, it looks correct to do this.
However, I wonder if it was correct for the MV64660 to claim
compatibily witn ns16550 if the programming model is not exactly
the same. The official OF serial port bindings don't mention the
reg-shift property, so it maybe would have been better to have
a different value for the "compatible" property, in order not
to confuse existing operating systems that implement the standard.

Also, what about etherboot and yaboot? Do they handle this
correctly?

	Arnd <><

Re: [PATCH] Handle reg-shift property for of_serial ports

[David Woodhouse]

On Sat, 2007-07-07 at 14:10 +0200, Arnd Bergmann wrote:
> On Saturday 07 July 2007, David Woodhouse wrote:
> > 
> > The MV64660 has reg-shift==2 for its otherwise 16550-compatible uarts.
> > While the bootwrapper copes with this, of_serial.c doesn't. (The udbg
> > code doesn't either, but I'll fix that later).
> > 
> > Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> Given the existence of the boards, it looks correct to do this.
> However, I wonder if it was correct for the MV64660 to claim
> compatibily witn ns16550 if the programming model is not exactly
> the same. The official OF serial port bindings don't mention the
> reg-shift property, so it maybe would have been better to have
> a different value for the "compatible" property, in order not
> to confuse existing operating systems that implement the standard.

Well, so far this only exists in the hacked-up device tree for my
bootwrapper; not in any real firmware. If a reg-shift property is the
wrong way to do it, we can change it. I got the reg-shift thing from
Mark's code in arch/powerpc/boot/ns16550.c, which handles it that way
for the bootwrapper (although I don't see any device-tree blobs which
use it, apart from mine).

-- 
dwmw2

Re: [PATCH] Handle reg-shift property for of_serial ports

[David Woodhouse]

On Sat, 2007-07-07 at 14:10 +0200, Arnd Bergmann wrote:
> Given the existence of the boards, it looks correct to do this.
> However, I wonder if it was correct for the MV64660 to claim
> compatibily witn ns16550 if the programming model is not exactly
> the same. The official OF serial port bindings don't mention the
> reg-shift property, so it maybe would have been better to have
> a different value for the "compatible" property, in order not
> to confuse existing operating systems that implement the standard. 

Ok, how about 'sparse16550'? Otherwise identical to ns16550, but with
the reg-shift property. I'll send a patch shortly, and I'll reorder the
match table -- if something claims compatibility with both 8250 and
16550, shouldn't we drive it as the latter?

Can we add properties to indicate the common high-speed modes too? The
Natsemi baud-base thing could be autodetected by 8250.c if you'd let it,
but the SMSC trick just has to be set as a UPF_MAGIC_MULTIPLIER flag.

-- 
dwmw2

Re: [PATCH] Handle reg-shift property for of_serial ports

[Sergei Shtylyov]

Hello.

David Woodhouse wrote:

>>Given the existence of the boards, it looks correct to do this.
>>However, I wonder if it was correct for the MV64660 to claim
>>compatibily witn ns16550 if the programming model is not exactly
>>the same. The official OF serial port bindings don't mention the
>>reg-shift property, so it maybe would have been better to have

   I'd preferred "reg-stride" or "reg-size" but see below...

>>a different value for the "compatible" property, in order not
>>to confuse existing operating systems that implement the standard. 

> Ok, how about 'sparse16550'? Otherwise identical to ns16550, but with

    Erm, wouldn't it be *too* generic approach?  I'd suggest to name the 
device with its own name and make of_serial.c recognize it and register with 
8250.c as needed.

> the reg-shift property. I'll send a patch shortly, and I'll reorder the
> match table -- if something claims compatibility with both 8250 and
> 16550, shouldn't we drive it as the latter?

    Certainly. BTW, was there really "ns8250" -- 8250 is Intel's chip?

WBR, Sergei

Re: [PATCH] Handle reg-shift property for of_serial ports

[Arnd Bergmann]

On Saturday 07 July 2007, Sergei Shtylyov wrote:
> > the reg-shift property. I'll send a patch shortly, and I'll reorder the
> > match table -- if something claims compatibility with both 8250 and
> > 16550, shouldn't we drive it as the latter?
>=20
> =A0 =A0 Certainly. BTW, was there really "ns8250" -- 8250 is Intel's chip?
>=20

No, you're right. http://playground.sun.com/1275/proposals/Closed/Accepted/=
384-it.txt
actually mentions that the the name should be "i8250". It also mentions
that the compatible property should contain "pnpPNP,501" for ns16450 and=20
higher, and "pnpPNP,500" for all serial ports starting from i8250.

	Arnd <><

Re: [PATCH] Handle reg-shift property for of_serial ports

[Segher Boessenkool]

> No, you're right. http://playground.sun.com/1275/proposals/Closed/ 
> Accepted/384-it.txt
> actually mentions that the the name should be "i8250". It also  
> mentions
> that the compatible property should contain "pnpPNP,501" for  
> ns16450 and
> higher,

16550 and higher

> and "pnpPNP,500" for all serial ports starting from i8250.

Yup.


Segher

Re: [PATCH] Handle reg-shift property for of_serial ports

[Segher Boessenkool]

>>> Given the existence of the boards, it looks correct to do this.
>>> However, I wonder if it was correct for the MV64660 to claim
>>> compatibily witn ns16550 if the programming model is not exactly
>>> the same. The official OF serial port bindings don't mention the
>>> reg-shift property, so it maybe would have been better to have
>
>    I'd preferred "reg-stride" or "reg-size" but see below...

It is not the register size.  "Register spacing" is the most
common name I believe, but "register shift" is nicer for
computer programs.

>>> a different value for the "compatible" property, in order not
>>> to confuse existing operating systems that implement the standard.
>
>> Ok, how about 'sparse16550'? Otherwise identical to ns16550, but with
>
>     Erm, wouldn't it be *too* generic approach?  I'd suggest to  
> name the
> device with its own name and make of_serial.c recognize it and  
> register with
> 8250.c as needed.

Yes, name the device by its real name, *please*.

>> the reg-shift property. I'll send a patch shortly, and I'll  
>> reorder the
>> match table -- if something claims compatibility with both 8250 and
>> 16550, shouldn't we drive it as the latter?
>
>     Certainly. BTW, was there really "ns8250" -- 8250 is Intel's chip?

It should be "i8250" yes.  Not that you'd ever find any anymore
of course.  And in many cases, it should be "pnpPNP,xxx" anyway.


Segher

Re: [PATCH] Handle reg-shift property for of_serial ports

[Arnd Bergmann]

On Saturday 07 July 2007, David Woodhouse wrote:
> Can we add properties to indicate the common high-speed modes too? The
> Natsemi baud-base thing could be autodetected by 8250.c if you'd let it,
> but the SMSC trick just has to be set as a UPF_MAGIC_MULTIPLIER flag.

Yes, that sounds good. I do not have enough understanding about the
extra feature to know which ones should get detected with their own
property, so I left that out for the next person that needs it...

	Arnd <><

Re: [PATCH] Handle reg-shift property for of_serial ports

[Segher Boessenkool]

>> Can we add properties to indicate the common high-speed modes too?  
>> The
>> Natsemi baud-base thing could be autodetected by 8250.c if you'd  
>> let it,
>> but the SMSC trick just has to be set as a UPF_MAGIC_MULTIPLIER flag.
>
> Yes, that sounds good. I do not have enough understanding about the
> extra feature to know which ones should get detected with their own
> property, so I left that out for the next person that needs it...

Since these devices will work (just not with the extra-high
baudrates) when driven as a "standard" 16550, you can indeed
just add a property to declare the device supports this stuff.

But please also declare the device to be such a special device
in its "compatible" property.  So you end up with something like

	serial@i3f8
	{
		reg = <1 3f8>;
		device_type = "serial";
		compatible = "smsc,12345", "ns16550a", "pnpPNP,501";
		clock-frequency = 1843200;
		smsc-multipliers; # <--- there's the new thing
	}


Segher

Re: [PATCH] Handle reg-shift property for of_serial ports

[Segher Boessenkool]

> Ok, how about 'sparse16550'? Otherwise identical to ns16550, but with
> the reg-shift property. I'll send a patch shortly, and I'll reorder  
> the
> match table -- if something claims compatibility with both 8250 and
> 16550, shouldn't we drive it as the latter?
>
> Can we add properties to indicate the common high-speed modes too? The
> Natsemi baud-base thing could be autodetected by 8250.c if you'd  
> let it,
> but the SMSC trick just has to be set as a UPF_MAGIC_MULTIPLIER flag.

So just use "compatible" = "smsc,blablabla".  Your device
has extra features over a 16550, namely that extra multiplier
or whatever; so just match those new features from the device
name (i.e., "compatible" property), don't try to retrofit those
features onto an existing driver.

And don't make up names for devices (like "sparse16550") unless
you really really have to, it definitely isn't needed here.

For your new device name, you can either have the register
spacing an implicit property of the device, or you can put
it in a "reg-shift" property if you want.


Segher

[PATCH 1/2] Add 'sparse16550' to of_serial.c and handle 'reg-shift' property.

[David Woodhouse]

Some boards have UARTs which are mostly compatible with a 16550, but
with registers spaced more widely. This isn't strictly compatible with
ns16550, so introduce a new 'compatible' type for it: sparse16550. The
'reg-shift' property indicates how far the registers are separated.

Also, reorder the match table to favour better modes if a device claims
compatibility with both 8250 and 16550, for example.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
index 7ffdaea..6ae1b5e 100644
--- a/drivers/serial/of_serial.c
+++ b/drivers/serial/of_serial.c
@@ -25,12 +25,13 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
 {
 	struct resource resource;
 	struct device_node *np = ofdev->node;
-	const unsigned int *clk, *spd;
+	const unsigned int *clk, *spd, *rs;
 	int ret;
 
 	memset(port, 0, sizeof *port);
 	spd = of_get_property(np, "current-speed", NULL);
 	clk = of_get_property(np, "clock-frequency", NULL);
+	rs = of_get_property(np, "reg-shift", NULL);
 	if (!clk) {
 		dev_warn(&ofdev->dev, "no clock-frequency property set\n");
 		return -ENODEV;
@@ -48,6 +49,8 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
 	port->iotype = UPIO_MEM;
 	port->type = type;
 	port->uartclk = *clk;
+	if (rs)
+		port->regshift = *rs;
 	port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP
 		| UPF_FIXED_PORT;
 	port->dev = &ofdev->dev;
@@ -111,11 +114,12 @@ static int of_platform_serial_remove(struct of_device *ofdev)
  * A few common types, add more as needed.
  */
 static struct of_device_id __devinitdata of_platform_serial_table[] = {
-	{ .type = "serial", .compatible = "ns8250",   .data = (void *)PORT_8250, },
-	{ .type = "serial", .compatible = "ns16450",  .data = (void *)PORT_16450, },
-	{ .type = "serial", .compatible = "ns16550",  .data = (void *)PORT_16550, },
-	{ .type = "serial", .compatible = "ns16750",  .data = (void *)PORT_16750, },
-	{ .type = "serial",			      .data = (void *)PORT_UNKNOWN, },
+	{ .type = "serial", .compatible = "sparse16550", .data = (void *)PORT_16550, },
+	{ .type = "serial", .compatible = "ns16750",     .data = (void *)PORT_16750, },
+	{ .type = "serial", .compatible = "ns16550",     .data = (void *)PORT_16550, },
+	{ .type = "serial", .compatible = "ns16450",     .data = (void *)PORT_16450, },
+	{ .type = "serial", .compatible = "ns8250",      .data = (void *)PORT_8250, },
+	{ .type = "serial",			         .data = (void *)PORT_UNKNOWN, },
 	{ /* end of list */ },
 };
 

-- 
dwmw2

[PATCH 2/2] Add 'sparse16550' support to PowerPC bootwrapper

[David Woodhouse]

The bootwrapper already handles a 'reg-shift' property on serial ports
and does the right thing. However, these ports really shouldn't be
claiming to be compatible with 'ns16550'. Introduce a new 'sparse16550'
type for them instead.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/arch/powerpc/boot/serial.c b/arch/powerpc/boot/serial.c
index 7fd3233..6d9f3f8 100644
--- a/arch/powerpc/boot/serial.c
+++ b/arch/powerpc/boot/serial.c
@@ -123,7 +123,7 @@ int serial_console_init(void)
 	if (getprop(devp, "compatible", compat, sizeof(compat)) < 0)
 		goto err_out;
 
-	if (!strcmp(compat, "ns16550"))
+	if (!strcmp(compat, "ns16550") || !strcmp(compat, "sparse16550"))
 		rc = ns16550_console_init(devp, &serial_cd);
 	else if (!strcmp(compat, "marvell,mpsc"))
 		rc = mpsc_console_init(devp, &serial_cd);

-- 
dwmw2

Re: [PATCH] Handle reg-shift property for of_serial ports

[Segher Boessenkool]

>> The MV64660 has reg-shift==2 for its otherwise 16550-compatible  
>> uarts.
>> While the bootwrapper copes with this, of_serial.c doesn't. (The udbg
>> code doesn't either, but I'll fix that later).
>>
>> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

So let me just say NAK so this doesn't accidentally get
applied :-)


Segher