Uartlite driver

Uartlite driver

[Michal Simek]

Hi All,

I have started to use uartlite driver which is in Linus tree.
I look at history and differences between my ancient version and new one.
I found one "insensible" commit from Grant with ACK from Josh and John and one
revert of this.

This commit a15da8eff3627b8368db7f5dd260e5643213d918 (description below) is
trying to fix problem 32bit access to ulite registers. You change 8bit access to
32bit (and commit 077b50c29a7e8be5812d1156934ea837b712ca6) reverts changes back.

"Magic offset" is not magic it is normal - because there is normal big endian
mess where readb and writeb read first byte from big endian but uartlite
registers have are on the last byte -> this mean that magical offset +3.

goto next;

[POWERPC] Uartlite: Fix reg io to access documented register size
The Uartlite data sheet defines the registers as 32 bit wide.  This
patch changes the register access to use 32 bit transfers and eliminates
the magic +3 offset which is currently required to make the device
work.

[POWERPC] Uartlite: Revert register io access changes
Reverts commit a15da8eff3627b8368db7f5dd260e5643213d918
This driver is used by devices other than the xilinx opb-uartlite which
depend on bytewise access to the registers.  The change to 32 bit access
does not work on these devices.

next:

This changes has impact to registration of device (OF in driver and simple in
virtex_devices.c)
 599         return ulite_assign(&op->dev, id ? *id : -1, res.start+3, irq);

--- a/arch/ppc/syslib/virtex_devices.c
+++ b/arch/ppc/syslib/virtex_devices.c
@@ -28,7 +28,7 @@
        .num_resources = 2, \
        .resource = (struct resource[]) { \
                { \
                        .start = XPAR_UARTLITE_##num##_BASEADDR + 3, \
                        .end = XPAR_UARTLITE_##num##_HIGHADDR, \
                        .flags = IORESOURCE_MEM, \
                }, \
556         return ulite_assign(&pdev->dev, pdev->id, res->start, res2->start);


The result of this mess is senseless output in kernel log
40100000.serial: ttyUL0 at MMIO 0x40100003 (irq = 3) is a uartlite

I think better solution will be remove our magic offset +3 and add it offset +3
to register with comment why this offset is. This changes will help us to clear
registration for non OF devices on Microblaze.

  49 #define ULITE_RX                (0x00 + 3)
  50 #define ULITE_TX                (0x04 + 3)
  51 #define ULITE_STATUS            (0x08 + 3)
  52 #define ULITE_CONTROL           (0x0c + 3)

Patch is below.

Regards,
Michal Simek



>From 7f151ef33344eff401e1e50fb85a77449768b62d Mon Sep 17 00:00:00 2001
From: Michal Simek <monstr@monstr.eu>
Date: Sun, 11 May 2008 15:26:16 +0200
Subject: [PATCH 1/1] Serial: Uartlite - moving register displacement to driver

Clearing registration driver with magic displacement +3
to register offset. For readb and writeb function is this better
way how to read/write proper position than registering whole
device drivers with magic offset +3.
Magic offset is only displacement from manual because uarlite
driver use last byte from 32bit register.
Byte reading/writing is neutral on little and big endian archs.

This patch clean senseless line in kernel log
40100000.serial: ttyUL0 at MMIO 0x40100003 (irq = 3) is a uartlite

Signed-off-by: Michal Simek <monstr@monstr.eu>
---
 arch/ppc/syslib/virtex_devices.c |    2 +-
 drivers/serial/uartlite.c        |   11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/ppc/syslib/virtex_devices.c b/arch/ppc/syslib/virtex_devices.c
index 7322781..3ff1cbc 100644
--- a/arch/ppc/syslib/virtex_devices.c
+++ b/arch/ppc/syslib/virtex_devices.c
@@ -28,7 +28,7 @@
 	.num_resources = 2, \
 	.resource = (struct resource[]) { \
 		{ \
-			.start = XPAR_UARTLITE_##num##_BASEADDR + 3, \
+			.start = XPAR_UARTLITE_##num##_BASEADDR, \
 			.end = XPAR_UARTLITE_##num##_HIGHADDR, \
 			.flags = IORESOURCE_MEM, \
 		}, \
diff --git a/drivers/serial/uartlite.c b/drivers/serial/uartlite.c
index b51c242..d5c96e5 100644
--- a/drivers/serial/uartlite.c
+++ b/drivers/serial/uartlite.c
@@ -46,10 +46,10 @@ MODULE_DEVICE_TABLE(of, ulite_of_match);
  * http://www.xilinx.com/bvdocs/ipcenter/data_sheet/opb_uartlite.pdf
  */

-#define ULITE_RX		0x00
-#define ULITE_TX		0x04
-#define ULITE_STATUS		0x08
-#define ULITE_CONTROL		0x0c
+#define ULITE_RX		(0x00 + 3)
+#define ULITE_TX		(0x04 + 3)
+#define ULITE_STATUS		(0x08 + 3)
+#define ULITE_CONTROL		(0x0c + 3)

 #define ULITE_REGION		16

@@ -66,7 +66,6 @@ MODULE_DEVICE_TABLE(of, ulite_of_match);
 #define ULITE_CONTROL_RST_RX	0x02
 #define ULITE_CONTROL_IE	0x10

-
 static struct uart_port ulite_ports[ULITE_NR_UARTS];

 /* ---------------------------------------------------------------------
@@ -596,7 +595,7 @@ ulite_of_probe(struct of_device *op, const struct
of_device_id *match)

 	id = of_get_property(op->node, "port-number", NULL);

-	return ulite_assign(&op->dev, id ? *id : -1, res.start+3, irq);
+	return ulite_assign(&op->dev, id ? *id : -1, res.start, irq);
 }

 static int __devexit ulite_of_remove(struct of_device *op)
-- 
1.5.4.GIT

Re: Uartlite driver

[John Williams]

Hi Michal,

On Sun, 2008-05-11 at 16:01 +0200, Michal Simek wrote:

> I have started to use uartlite driver which is in Linus tree.
> I look at history and differences between my ancient version and new one.
> I found one "insensible" commit from Grant with ACK from Josh and John and one
> revert of this.
> 
> This commit a15da8eff3627b8368db7f5dd260e5643213d918 (description below) is
> trying to fix problem 32bit access to ulite registers. You change 8bit access to
> 32bit (and commit 077b50c29a7e8be5812d1156934ea837b712ca6) reverts changes back.
> 
> "Magic offset" is not magic it is normal - because there is normal big endian
> mess where readb and writeb read first byte from big endian but uartlite
> registers have are on the last byte -> this mean that magical offset +3.
> 
> goto next;
> 
> [POWERPC] Uartlite: Fix reg io to access documented register size
> The Uartlite data sheet defines the registers as 32 bit wide.  This
> patch changes the register access to use 32 bit transfers and eliminates
> the magic +3 offset which is currently required to make the device
> work.
> 
> [POWERPC] Uartlite: Revert register io access changes
> Reverts commit a15da8eff3627b8368db7f5dd260e5643213d918
> This driver is used by devices other than the xilinx opb-uartlite which
> depend on bytewise access to the registers.  The change to 32 bit access
> does not work on these devices.

The uartlite register set is 32 bits wide - that's what the datasheet
says, and that's how we should interact with it.  I really don't
understand why this commit was reverted.

Who uses the uartlite driver for anything other than the uartlite?

Magic +3 offsets anywhere are a bad idea - I agree having addresses like
0x40100003 in the kernel log for base addresses is stupid, but I don't
like a +3 offset in the platform code either.

Why can't we use the device as documented, with 32 bit wide accesses?

Regards,

John

Re: Uartlite driver

[Grant Likely]

On Sun, May 11, 2008 at 4:39 PM, John Williams
<john.williams@petalogix.com> wrote:
> Hi Michal,
>
>  On Sun, 2008-05-11 at 16:01 +0200, Michal Simek wrote:
>
>  > This commit a15da8eff3627b8368db7f5dd260e5643213d918 (description below) is
>  > trying to fix problem 32bit access to ulite registers. You change 8bit access to
>  > 32bit (and commit 077b50c29a7e8be5812d1156934ea837b712ca6) reverts changes back.
>  >
>  > "Magic offset" is not magic it is normal - because there is normal big endian
>  > mess where readb and writeb read first byte from big endian but uartlite
>  > registers have are on the last byte -> this mean that magical offset +3.

I fully understand the reason for the +3.  What makes it 'magic' is
the fact that it is an undocumented value added to the base address
with no comment describing what it is for.

>  The uartlite register set is 32 bits wide - that's what the datasheet
>  says, and that's how we should interact with it.  I really don't
>  understand why this commit was reverted.
>
>  Who uses the uartlite driver for anything other than the uartlite?
>
>  Magic +3 offsets anywhere are a bad idea - I agree having addresses like
>  0x40100003 in the kernel log for base addresses is stupid, but I don't
>  like a +3 offset in the platform code either.
>
>  Why can't we use the device as documented, with 32 bit wide accesses?

Peter (who wrote the driver in the first place) has another device
which has almost the same register layout, but either does not support
32bit accesses, or is based at a different offset (I can't remember
which).  Regardless, my change broke support for his device.

As for the platform code (in virtex_devices.c), I'm no longer
concerned what ugliness lives there.  It will be gone in the next
kernel version regardless.  Nor am I particularly concerned about the
internal structure of the driver anymore as long as it works (it's
Peter's responsibility as long as he is maintaining that driver).
What I *do* care about is the device tree binding for the uartlite.
As long as the device tree binding shows the real device base address
and not any byte-addressable offset nonsense, then I'm happy.

Cheers,
g.

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

Re: Uartlite driver

[Michal Simek]

Hi Grant,


> On Sun, May 11, 2008 at 4:39 PM, John Williams
> <john.williams@petalogix.com> wrote:
>> Hi Michal,
>>
>>  On Sun, 2008-05-11 at 16:01 +0200, Michal Simek wrote:
>>
>>  > This commit a15da8eff3627b8368db7f5dd260e5643213d918 (description below) is
>>  > trying to fix problem 32bit access to ulite registers. You change 8bit access to
>>  > 32bit (and commit 077b50c29a7e8be5812d1156934ea837b712ca6) reverts changes back.
>>  >
>>  > "Magic offset" is not magic it is normal - because there is normal big endian
>>  > mess where readb and writeb read first byte from big endian but uartlite
>>  > registers have are on the last byte -> this mean that magical offset +3.
> 
> I fully understand the reason for the +3.  What makes it 'magic' is
> the fact that it is an undocumented value added to the base address
> with no comment describing what it is for.

This is documented offset - look at uartlite manual - useful bits are 24-31.
On big endian systems with 32bit access is everything ok because the least
significant byte is the last in memory. But when you changed from 32bits to 8
bits access than you have to move your base address to +3 offset.

This is not undocumented value.


>>  The uartlite register set is 32 bits wide - that's what the datasheet
>>  says, and that's how we should interact with it.  I really don't
>>  understand why this commit was reverted.
>>
>>  Who uses the uartlite driver for anything other than the uartlite?
>>
>>  Magic +3 offsets anywhere are a bad idea - I agree having addresses like
>>  0x40100003 in the kernel log for base addresses is stupid, but I don't
>>  like a +3 offset in the platform code either.
>>
>>  Why can't we use the device as documented, with 32 bit wide accesses?
> 
> Peter (who wrote the driver in the first place) has another device
> which has almost the same register layout, but either does not support
> 32bit accesses, or is based at a different offset (I can't remember
> which).  Regardless, my change broke support for his device.
> 
> As for the platform code (in virtex_devices.c), I'm no longer
> concerned what ugliness lives there.  It will be gone in the next
> kernel version regardless.  Nor am I particularly concerned about the
> internal structure of the driver anymore as long as it works (it's
> Peter's responsibility as long as he is maintaining that driver).
> What I *do* care about is the device tree binding for the uartlite.
> As long as the device tree binding shows the real device base address
> and not any byte-addressable offset nonsense, then I'm happy.

:-)

Michal

Re: Uartlite driver

[David H. Lynch Jr.]

Michal Simek wrote:
>>
>> I fully understand the reason for the +3.  What makes it 'magic' is
>> the fact that it is an undocumented value added to the base address
>> with no comment describing what it is for.
>>     
>
> This is documented offset - look at uartlite manual - useful bits are 24-31.
> On big endian systems with 32bit access is everything ok because the least
> significant byte is the last in memory. But when you changed from 32bits to 8
> bits access than you have to move your base address to +3 offset.
>
> This is not undocumented value.
    I do not buy that as any way documented.
    Are there substantial numbers of other drivers for other devices
    where  a +3  jumps in from nowhere ?
    I am also not happy with the register definitions hard coded as 0,4,8,12
   
    I have not read the Xilinx UartLite docs recently, but there was older
    Xilinx code  that implied the registers could be 8, 16 or 32 bits.
    There is also atleast one implimentation out there that accesses
    the UartLite via DCR.

     Can we just move all the in's and outs to something like:

static unsigned int
serial_in(struct uart_port *port, int offset) {
    unsigned int value;

    switch (port->iotype) {
        case UPIO_PORT:
            value = mfdcr(offset);
            break;
        default:
            offset <<= port->regshift;
            value = readb(port->membase + offset);
    }
    return value;
}

Then we can handle all the 8/16/32 BE/LE, DCR, .... in one place ?
I think that is what is done in other drivers.




   


-- 
Dave Lynch 					  	    DLA Systems
Software Development:  				         Embedded Linux
717.627.3770 	       dhlii@dlasys.net 	  http://www.dlasys.net
fax: 1.253.369.9244 			           Cell: 1.717.587.7774
Over 25 years' experience in platforms, languages, and technologies too numerous to list.

"Any intelligent fool can make things bigger and more complex... It takes a touch of genius - and a lot of courage to move in the opposite direction."
Albert Einstein

RE: Uartlite driver

[Stephen Neuendorffer]

Somewhat off topic here, but related to David's suggestion below, I've
been pondering a 'control register' abstraction, partly because it
appears that new cores are being built which have the option to replace
dcr control interfaces with PLB interfaces, or replace PLB interfaces
with dcr interfaces, hence the drivers need to handle dcr interfaces and
plb interfaces more or less interchangeably.

One possibility would be to hijack the dcr-specific code that's already
there, but I'm not sure if that really makes alot of sense.  Comments
appreciated...

Steve

> -----Original Message-----
> From: David H. Lynch Jr. [mailto:dhlii@dlasys.net]
> Sent: Thursday, May 15, 2008 7:32 AM
> To: monstr@seznam.cz
> Cc: Grant Likely; John Williams; Josh Boyer;
linux-serial@vger.kernel.org; jacmet@sunsite.dk; Stephen
> Neuendorffer
> Subject: Re: Uartlite driver
> 
> Michal Simek wrote:
> >>
> >> I fully understand the reason for the +3.  What makes it 'magic' is
> >> the fact that it is an undocumented value added to the base address
> >> with no comment describing what it is for.
> >>
> >
> > This is documented offset - look at uartlite manual - useful bits
are 24-31.
> > On big endian systems with 32bit access is everything ok because the
least
> > significant byte is the last in memory. But when you changed from
32bits to 8
> > bits access than you have to move your base address to +3 offset.
> >
> > This is not undocumented value.
>     I do not buy that as any way documented.
>     Are there substantial numbers of other drivers for other devices
>     where  a +3  jumps in from nowhere ?
>     I am also not happy with the register definitions hard coded as
0,4,8,12
> 
>     I have not read the Xilinx UartLite docs recently, but there was
older
>     Xilinx code  that implied the registers could be 8, 16 or 32 bits.
>     There is also atleast one implimentation out there that accesses
>     the UartLite via DCR.
> 
>      Can we just move all the in's and outs to something like:
> 
> static unsigned int
> serial_in(struct uart_port *port, int offset) {
>     unsigned int value;
> 
>     switch (port->iotype) {
>         case UPIO_PORT:
>             value = mfdcr(offset);
>             break;
>         default:
>             offset <<= port->regshift;
>             value = readb(port->membase + offset);
>     }
>     return value;
> }
> 
> Then we can handle all the 8/16/32 BE/LE, DCR, .... in one place ?
> I think that is what is done in other drivers.
> 
> 
> 
> 
> 
> 
> 
> --
> Dave Lynch 					  	    DLA Systems
> Software Development:
Embedded Linux
> 717.627.3770 	       dhlii@dlasys.net 	  http://www.dlasys.net
> fax: 1.253.369.9244 			           Cell: 1.717.587.7774
> Over 25 years' experience in platforms, languages, and technologies
too numerous to list.
> 
> "Any intelligent fool can make things bigger and more complex... It
takes a touch of genius - and a
> lot of courage to move in the opposite direction."
> Albert Einstein
> 


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: Uartlite driver

[Peter Korsgaard]

>>>>> "David" == David H Lynch <dhlii@dlasys.net> writes:

Hi,

 David> I am also not happy with the register definitions hard coded
 David> as 0,4,8,12 I have not read the Xilinx UartLite docs recently,
 David> but there was older Xilinx code that implied the registers
 David> could be 8, 16 or 32 bits.  There is also atleast one
 David> implimentation out there that accesses the UartLite via DCR.

Can the Xilinx core be configured for anything else than 32bit offset?
I don't think so.

 David>      Can we just move all the in's and outs to something like:

 David> static unsigned int
 David> serial_in(struct uart_port *port, int offset) {
 David>     unsigned int value;

 David>     switch (port->iotype) {
 David>         case UPIO_PORT:
 David>             value = mfdcr(offset);
 David>             break;
 David>         default:
 David>             offset <<= port->regshift;
 David>             value = readb(port->membase + offset);
 David>     }
 David>     return value;
 David> }

 David> Then we can handle all the 8/16/32 BE/LE, DCR, .... in one place ?
 David> I think that is what is done in other drivers.

The only one that afaik does this is 8250.c because of the mindblowing
number of more-or-less compatible 8250 implementations. That's surely
not the case for the uartlite.

But ok, if the DCR access mode is supported by Xilinx - Why not?
Patches are welcome.

-- 
Bye, Peter Korsgaard

Re: Uartlite driver & CONFIG_CONSOLE_POLL

[David H. Lynch Jr.]

    One of the key differences between my uartlite driver and Peter's
is that like many other serial drivers I have a timer to support irqless
use. Just like Peter has UartLite Like devices with 16bit datapaths,
I have UartLites with no interrupts.

   I have tried (unsuccessfully) several times to migrate my timer
code into Peter's driver. This is probably the most fundimental
remaining reason I can not use Peter's driver.

    More recently I noticed a CONFIG_CONSOLE_POLL option show
up. There are remarks that this is similar to NETPOLL - though
it seems to be implimented using a getc and putc rather than a
poll entry.

    Is this a general serial facility, rather than just for consoles ?
    If I create the polled getc/putc routines can I expect a serial driver
to be functional without having to implement  my own  timer based
polling ?
   
    Is this the proper way to do this ?





-- 
Dave Lynch 					  	    DLA Systems
Software Development:  				         Embedded Linux
717.627.3770 	       dhlii@dlasys.net 	  http://www.dlasys.net
fax: 1.253.369.9244 			           Cell: 1.717.587.7774
Over 25 years' experience in platforms, languages, and technologies too numerous to list.

"Any intelligent fool can make things bigger and more complex... It takes a touch of genius - and a lot of courage to move in the opposite direction."
Albert Einstein

Re: Uartlite driver & CONFIG_CONSOLE_POLL

[Peter Korsgaard]

>>>>> "David" == David H Lynch <dhlii@dlasys.net> writes:

Hi,

 David>     One of the key differences between my uartlite driver and
 David> Peter's is that like many other serial drivers I have a timer
 David> to support irqless use. Just like Peter has UartLite Like
 David> devices with 16bit datapaths, I have UartLites with no
 David> interrupts.

Many others? I think not.

ls -l *.c|wc -l                     ~/source/linux-2.6/drivers/serial
62
git grep -l request_irq `git grep -l init_timer`|wc -l
9

 David>    I have tried (unsuccessfully) several times to migrate my
 David> timer code into Peter's driver. This is probably the most
 David> fundimental remaining reason I can not use Peter's driver.

unsuccessfully in what way? I afaik haven't seen any patches.

-- 
Bye, Peter Korsgaard

Re: Uartlite driver & CONFIG_CONSOLE_POLL

[Peter Korsgaard]

>>>>> "David" == David H Lynch <dhlii@dlasys.net> writes:

Hi,

Please keep replies on the list.

 David> Though I was looking to point out that just as YOU sometimes
 David> have unique requirements, so do the rest of us. And I was
 David> offering my suggestion for a flexible solution, that might
 David> meet current as well as unanticipated needs.

Sure, patches are welcome. I'll gladly accept them if they are clean
enough.

 David> You are not obligated to give me what I want, but I am not
 David> obligated to use what you provide.

Exactly, it's a free world.

-- 
Bye, Peter Korsgaard

Re: Uartlite driver

[Peter Korsgaard]

>>>>> "John" == John Williams <john.williams@petalogix.com> writes:

Hi,

 John> The uartlite register set is 32 bits wide - that's what the datasheet
 John> says, and that's how we should interact with it.  I really don't
 John> understand why this commit was reverted.

 John> Who uses the uartlite driver for anything other than the uartlite?

Me (the author of uartlite.c). We have designs where we needed more
uarts, but didn't have any more room/ I/O pins on the virtex, so we
have implemented an uartlite compatible interface in a S3E connected
through a 16bit bus.

Not exactly brilliant hw design, but it works and it keeps the sw
support to a minimum.

-- 
Bye, Peter Korsgaard