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

[John Linn]

Sounds good, those are easy changes and make sense.

Since I'm a newbie, I don't know any better sometimes when I copy other
code that may not be as safe.  

The same thing, of_get_property(np, "current-speed", NULL);, is done
right above my code I added.  

Should the other code in the driver using the same method be fixed, or
just my patch?

Thanks for your patience,
John

-----Original Message-----
From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On Behalf Of
Grant Likely
Sent: Wednesday, April 02, 2008 12:00 PM
To: John Linn
Cc: linuxppc-dev at ozlabs.org
Subject: Re: [PATCH 2/3][POWERPC][V2] Xilinx: of_serial support for
Xilinx uart 16550.

On Wed, Apr 2, 2008 at 10:52 AM, John Linn <john.linn at xilinx.com> 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 at xilinx.com>

Comments below...

>  ---
>   Documentation/powerpc/booting-without-of.txt |   11 +++++++++++
>   drivers/serial/of_serial.c                   |   15 +++++++++++++--
>   2 files changed, 24 insertions(+), 2 deletions(-)
>
>  diff --git a/Documentation/powerpc/booting-without-of.txt
b/Documentation/powerpc/booting-without-of.txt
>  index 87f4d84..af112d9 100644
>  --- a/Documentation/powerpc/booting-without-of.txt
>  +++ b/Documentation/powerpc/booting-without-of.txt
>  @@ -2539,6 +2539,17 @@ 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 such
that they
>  +      use the ns16550 binding with properties to specify 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..af9ed48 100644
>  --- a/drivers/serial/of_serial.c
>  +++ b/drivers/serial/of_serial.c
>  @@ -30,7 +30,7 @@ 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, *reg_offset, *reg_shift;

These should really be u32's I believe; on 64 bit architectures this
will misbehave (not an immediate practical problem, but it's best to
be explicit about these things).

>         int ret;
>
>         memset(port, 0, sizeof *port);
>  @@ -48,7 +48,18 @@ static int __devinit
of_platform_serial_setup(struct of_device *ofdev,
>         }
>
>         spin_lock_init(&port->lock);
>  -       port->mapbase = resource.start;
>  +
>  +       reg_offset = of_get_property(np, "reg-offset", NULL);
>  +       reg_shift = of_get_property(np, "reg-shift", NULL);
>  +
>  +       if (!reg_offset)
>  +               port->mapbase = resource.start;
>  +       else
>  +               port->mapbase = resource.start + *reg_offset;
>  +
>  +       if (reg_shift)
>  +               port->regshift = *reg_shift;
>  +

This is a little unsafe since it doesn't check the property size, I'd
do the following instead:

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;

Cheers,
g.


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