[PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[John Linn]

The Xilinx 16550 uart core is not a standard 16550 because it uses
word-based addressing rather than byte-based addressing. With
additional properties it is compatible with the open firmware
'ns16550' compatible binding.

This code updates the of_serial driver to handle the reg-offset
and reg-shift properties to enable this core to be used.

Signed-off-by: John Linn <john.linn@xilinx.com>
---

V3 has fixes suggested by Grant. They were incorporated and tested.

 Documentation/powerpc/booting-without-of.txt |   10 ++++++++++
 drivers/serial/of_serial.c                   |   14 +++++++++++++-
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 87f4d84..4066ec8 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -2539,6 +2539,16 @@ platforms are moved over to use the flattened-device-tree model.
                       differ between different families.  May be
                       'virtex2p', 'virtex4', or 'virtex5'.
 
+      iv) Xilinx Uart 16550
+
+      Xilinx UART 16550 devices are very similar to the NS16550 but with
+      different register spacing and an offset from the base address.
+
+      Requred properties:
+       - clock-frequency : Frequency of the clock input
+       - reg-offset : A value of 3 is required
+       - reg-shift : A value of 2 is required
+
    More devices will be defined as this spec matures.
 
 VII - Specifying interrupt information for devices
diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
index 2efb892..73c47a5 100644
--- a/drivers/serial/of_serial.c
+++ b/drivers/serial/of_serial.c
@@ -31,7 +31,8 @@ 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;
-	int ret;
+	const u32 *prop;
+	int ret, prop_size;
 
 	memset(port, 0, sizeof *port);
 	spd = of_get_property(np, "current-speed", NULL);
@@ -49,6 +50,17 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
 
 	spin_lock_init(&port->lock);
 	port->mapbase = resource.start;
+
+	/* Check for shifted address mapping */
+	prop = of_get_property(np, "reg-offset", &prop_size);
+	if (prop && (prop_size == sizeof(u32)))
+		port->mapbase += *prop;
+
+	/* Check for registers offset within the devices address range */
+	prop = of_get_property(np, "reg-shift", &prop_size);
+	if (prop && (prop_size == sizeof(u32)))
+		port->regshift = *prop;
+
 	port->irq = irq_of_parse_and_map(np, 0);
 	port->iotype = UPIO_MEM;
 	port->type = type;
-- 
1.5.2.1

Re: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[Arnd Bergmann]

On Thursday 03 April 2008, John Linn wrote:
> The Xilinx 16550 uart core is not a standard 16550 because it uses
> word-based addressing rather than byte-based addressing. With
> additional properties it is compatible with the open firmware
> 'ns16550' compatible binding.
> 
> This code updates the of_serial driver to handle the reg-offset
> and reg-shift properties to enable this core to be used.
> 
> Signed-off-by: John Linn <john.linn@xilinx.com>

I may not be the best driver maintainer, but please keep the illusion
alive for me and Cc me on patches to drivers I wrote ;-)

> diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
> index 87f4d84..4066ec8 100644
> --- a/Documentation/powerpc/booting-without-of.txt
> +++ b/Documentation/powerpc/booting-without-of.txt
> @@ -2539,6 +2539,16 @@ platforms are moved over to use the flattened-device-tree model.
>                        differ between different families.  May be
>                        'virtex2p', 'virtex4', or 'virtex5'.
>  
> +      iv) Xilinx Uart 16550
> +
> +      Xilinx UART 16550 devices are very similar to the NS16550 but with
> +      different register spacing and an offset from the base address.
> +
> +      Requred properties:
> +       - clock-frequency : Frequency of the clock input
> +       - reg-offset : A value of 3 is required
> +       - reg-shift : A value of 2 is required
> +
>     More devices will be defined as this spec matures.

Since it is not really compatible with ns16550, shouldn't you at least specify
a different "compatible" property? That way, the driver won't do incorrect
accesses when you try to use an old driver with a device tree that specifies
one of these.

> @@ -49,6 +50,17 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
>  
>  	spin_lock_init(&port->lock);
>  	port->mapbase = resource.start;
> +
> +	/* Check for shifted address mapping */
> +	prop = of_get_property(np, "reg-offset", &prop_size);
> +	if (prop && (prop_size == sizeof(u32)))
> +		port->mapbase += *prop;
> +
> +	/* Check for registers offset within the devices address range */
> +	prop = of_get_property(np, "reg-shift", &prop_size);
> +	if (prop && (prop_size == sizeof(u32)))
> +		port->regshift = *prop;
> +
>  	port->irq = irq_of_parse_and_map(np, 0);
>  	port->iotype = UPIO_MEM;
>  	port->type = type;

This part looks good to me.

	Arnd <><

Re: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[Grant Likely]

On Wed, Apr 2, 2008 at 5:34 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 03 April 2008, John Linn wrote:
>  > The Xilinx 16550 uart core is not a standard 16550 because it uses
>  > word-based addressing rather than byte-based addressing. With
>  > additional properties it is compatible with the open firmware
>  > 'ns16550' compatible binding.
>  >
>  > This code updates the of_serial driver to handle the reg-offset
>  > and reg-shift properties to enable this core to be used.
>  >
>  > Signed-off-by: John Linn <john.linn@xilinx.com>
>
>  I may not be the best driver maintainer, but please keep the illusion
>  alive for me and Cc me on patches to drivers I wrote ;-)
>
>
>  > diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
>  > index 87f4d84..4066ec8 100644
>  > --- a/Documentation/powerpc/booting-without-of.txt
>  > +++ b/Documentation/powerpc/booting-without-of.txt
>  > @@ -2539,6 +2539,16 @@ platforms are moved over to use the flattened-device-tree model.
>  >                        differ between different families.  May be
>  >                        'virtex2p', 'virtex4', or 'virtex5'.
>  >
>  > +      iv) Xilinx Uart 16550
>  > +
>  > +      Xilinx UART 16550 devices are very similar to the NS16550 but with
>  > +      different register spacing and an offset from the base address.
>  > +
>  > +      Requred properties:
>  > +       - clock-frequency : Frequency of the clock input
>  > +       - reg-offset : A value of 3 is required
>  > +       - reg-shift : A value of 2 is required
>  > +
>  >     More devices will be defined as this spec matures.
>
>  Since it is not really compatible with ns16550, shouldn't you at least specify
>  a different "compatible" property? That way, the driver won't do incorrect
>  accesses when you try to use an old driver with a device tree that specifies
>  one of these.

Heh; we've gone back and forth on this issue.  The problem is that we
have a common case of ns16550 like devices that require a little bit
of register address tweaking that spans a whole range of vendors (so
adding a compatible match with each of those vendor's prefixes is
probably non-scalable).  So, if "ns16550" is not a good idea, then
what should be used?  "sparse16550" has been suggested more than once.

On the other side of the coin; the draft ePAPR spec already redefines
the meaning of "ns16550" to add an optional "reg-shift" property.
Also, in this particular case the problem is most likely more
theoretical than actual.  The likely hood of a platform needing these
new properties being handed a kernel which does not support the
"reg-*" properties is very slim.

Anyway, all that just to say that I prefer "ns16550", but I'll put my
vote and support behind "sparse16550" (or any other string) if other
people express consensus with it.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[Arnd Bergmann]

On Thursday 03 April 2008, Grant Likely wrote:
> >
> > =A0Since it is not really compatible with ns16550, shouldn't you at lea=
st specify
> > =A0a different "compatible" property? That way, the driver won't do inc=
orrect
> > =A0accesses when you try to use an old driver with a device tree that s=
pecifies
> > =A0one of these.
>=20
> Heh; we've gone back and forth on this issue. =A0The problem is that we
> have a common case of ns16550 like devices that require a little bit
> of register address tweaking that spans a whole range of vendors (so
> adding a compatible match with each of those vendor's prefixes is
> probably non-scalable). =A0So, if "ns16550" is not a good idea, then
> what should be used? =A0"sparse16550" has been suggested more than once.

After another IRC discussion between Grant, Segher and myself, we concluded
that we don't need to invent a new "compatible" value, as only new device
trees with old kernels will have a problem with this, and they don't work
in the first place.

The devices will still have their specific "compatible" value, e.g.
"xlnx,plb-uart16550-1.00.c", followed by "ns16550", and possibly
"ns16450" and "i8250", although the last two do not have an effect
on Linux.

Josh, can you please forward all three patches in their latest version?

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

RE: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[John Linn]

Thanks Arnd, I apologize for not keeping you in the loop on this.

I'm still learning the process and appreciate your help and patience.

Thanks to all for the work to get consensus on this,
John

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de]=20
Sent: Wednesday, April 02, 2008 10:16 PM
To: linuxppc-dev@ozlabs.org
Cc: Grant Likely; John Linn; Segher Boessenkool; Josh Boyer
Subject: Re: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for =
Xilinx uart 16550.

On Thursday 03 April 2008, Grant Likely wrote:
> >
> > =A0Since it is not really compatible with ns16550, shouldn't you at =
least specify
> > =A0a different "compatible" property? That way, the driver won't do =
incorrect
> > =A0accesses when you try to use an old driver with a device tree =
that specifies
> > =A0one of these.
>=20
> Heh; we've gone back and forth on this issue. =A0The problem is that =
we
> have a common case of ns16550 like devices that require a little bit
> of register address tweaking that spans a whole range of vendors (so
> adding a compatible match with each of those vendor's prefixes is
> probably non-scalable). =A0So, if "ns16550" is not a good idea, then
> what should be used? =A0"sparse16550" has been suggested more than =
once.

After another IRC discussion between Grant, Segher and myself, we =
concluded
that we don't need to invent a new "compatible" value, as only new =
device
trees with old kernels will have a problem with this, and they don't =
work
in the first place.

The devices will still have their specific "compatible" value, e.g.
"xlnx,plb-uart16550-1.00.c", followed by "ns16550", and possibly
"ns16450" and "i8250", although the last two do not have an effect
on Linux.

Josh, can you please forward all three patches in their latest version?

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

RE: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinxuart 16550.

[Stephen Neuendorffer]

The device tree generator now reflects this.

Steve

> -----Original Message-----
> From: =
linuxppc-dev-bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org =
[mailto:linuxppc-dev-
> bounces+stephen.neuendorffer=3Dxilinx.com@ozlabs.org] On Behalf Of =
Arnd Bergmann
> Sent: Wednesday, April 02, 2008 9:16 PM
> To: linuxppc-dev@ozlabs.org
> Cc: John Linn
> Subject: Re: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for =
Xilinxuart 16550.
>=20
> On Thursday 03 April 2008, Grant Likely wrote:
> > >
> > > =A0Since it is not really compatible with ns16550, shouldn't you =
at least specify
> > > =A0a different "compatible" property? That way, the driver won't =
do incorrect
> > > =A0accesses when you try to use an old driver with a device tree =
that specifies
> > > =A0one of these.
> >
> > Heh; we've gone back and forth on this issue. =A0The problem is that =
we
> > have a common case of ns16550 like devices that require a little bit
> > of register address tweaking that spans a whole range of vendors (so
> > adding a compatible match with each of those vendor's prefixes is
> > probably non-scalable). =A0So, if "ns16550" is not a good idea, then
> > what should be used? =A0"sparse16550" has been suggested more than =
once.
>=20
> After another IRC discussion between Grant, Segher and myself, we =
concluded
> that we don't need to invent a new "compatible" value, as only new =
device
> trees with old kernels will have a problem with this, and they don't =
work
> in the first place.
>=20
> The devices will still have their specific "compatible" value, e.g.
> "xlnx,plb-uart16550-1.00.c", followed by "ns16550", and possibly
> "ns16450" and "i8250", although the last two do not have an effect
> on Linux.
>=20
> Josh, can you please forward all three patches in their latest =
version?
>=20
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[Josh Boyer]

On Thu, 2008-04-03 at 06:16 +0200, Arnd Bergmann wrote:
> On Thursday 03 April 2008, Grant Likely wrote:
> > >
> > >  Since it is not really compatible with ns16550, shouldn't you at least specify
> > >  a different "compatible" property? That way, the driver won't do incorrect
> > >  accesses when you try to use an old driver with a device tree that specifies
> > >  one of these.
> > 
> > Heh; we've gone back and forth on this issue.  The problem is that we
> > have a common case of ns16550 like devices that require a little bit
> > of register address tweaking that spans a whole range of vendors (so
> > adding a compatible match with each of those vendor's prefixes is
> > probably non-scalable).  So, if "ns16550" is not a good idea, then
> > what should be used?  "sparse16550" has been suggested more than once.
> 
> After another IRC discussion between Grant, Segher and myself, we concluded
> that we don't need to invent a new "compatible" value, as only new device
> trees with old kernels will have a problem with this, and they don't work
> in the first place.
> 
> The devices will still have their specific "compatible" value, e.g.
> "xlnx,plb-uart16550-1.00.c", followed by "ns16550", and possibly
> "ns16450" and "i8250", although the last two do not have an effect
> on Linux.
> 
> Josh, can you please forward all three patches in their latest version?
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>

So is the discussion on reg-offset settled then?  It seemed Paul and
David had some issues with that, and I'd like to make sure everyone is
agreed on that before I bring in patches 2 and 3.

josh

Re: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[David Gibson]

On Sat, Apr 05, 2008 at 09:45:56AM -0500, Josh Boyer wrote:
> On Thu, 2008-04-03 at 06:16 +0200, Arnd Bergmann wrote:
> > On Thursday 03 April 2008, Grant Likely wrote:
> > > >
> > > >  Since it is not really compatible with ns16550, shouldn't you at least specify
> > > >  a different "compatible" property? That way, the driver won't do incorrect
> > > >  accesses when you try to use an old driver with a device tree that specifies
> > > >  one of these.
> > > 
> > > Heh; we've gone back and forth on this issue.  The problem is that we
> > > have a common case of ns16550 like devices that require a little bit
> > > of register address tweaking that spans a whole range of vendors (so
> > > adding a compatible match with each of those vendor's prefixes is
> > > probably non-scalable).  So, if "ns16550" is not a good idea, then
> > > what should be used?  "sparse16550" has been suggested more than once.
> > 
> > After another IRC discussion between Grant, Segher and myself, we concluded
> > that we don't need to invent a new "compatible" value, as only new device
> > trees with old kernels will have a problem with this, and they don't work
> > in the first place.
> > 
> > The devices will still have their specific "compatible" value, e.g.
> > "xlnx,plb-uart16550-1.00.c", followed by "ns16550", and possibly
> > "ns16450" and "i8250", although the last two do not have an effect
> > on Linux.
> > 
> > Josh, can you please forward all three patches in their latest version?
> > 
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> So is the discussion on reg-offset settled then?  It seemed Paul and
> David had some issues with that, and I'd like to make sure everyone is
> agreed on that before I bring in patches 2 and 3.

I didn't like it very much, but I don't really care enough to argue
about it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 2/3] [POWERPC][V3] Xilinx: of_serial support for Xilinx uart 16550.

[Sergei Shtylyov]

Hello.

John Linn wrote:

> The Xilinx 16550 uart core is not a standard 16550 because it uses
> word-based addressing rather than byte-based addressing. With
> additional properties it is compatible with the open firmware
> 'ns16550' compatible binding.

> This code updates the of_serial driver to handle the reg-offset
> and reg-shift properties to enable this core to be used.

> Signed-off-by: John Linn <john.linn@xilinx.com>
> ---

> diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
> index 2efb892..73c47a5 100644
> --- a/drivers/serial/of_serial.c
> +++ b/drivers/serial/of_serial.c
> @@ -31,7 +31,8 @@ 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;
> -	int ret;
> +	const u32 *prop;
> +	int ret, prop_size;
>  
>  	memset(port, 0, sizeof *port);
>  	spd = of_get_property(np, "current-speed", NULL);
> @@ -49,6 +50,17 @@ static int __devinit of_platform_serial_setup(struct of_device *ofdev,
>  
>  	spin_lock_init(&port->lock);
>  	port->mapbase = resource.start;
> +
> +	/* Check for shifted address mapping */
> +	prop = of_get_property(np, "reg-offset", &prop_size);
> +	if (prop && (prop_size == sizeof(u32)))
> +		port->mapbase += *prop;

    I disagree. That trick leads to I/O resource misaccounting/misreporting. 
You should teach 8250.c about the offset trick instead, just as it's been 
taught the shift trick in its time.

> +
> +	/* Check for registers offset within the devices address range */
> +	prop = of_get_property(np, "reg-shift", &prop_size);
> +	if (prop && (prop_size == sizeof(u32)))
> +		port->regshift = *prop;
> +
>  	port->irq = irq_of_parse_and_map(np, 0);
>  	port->iotype = UPIO_MEM;
>  	port->type = type;

WBR, Sergei