* [RFC] uartlite driver MicroBlaze compatability
@ 2007-05-01 4:55 John Williams
2007-05-01 5:55 ` Grant Likely
2007-05-02 13:45 ` Peter Korsgaard
0 siblings, 2 replies; 11+ messages in thread
From: John Williams @ 2007-05-01 4:55 UTC (permalink / raw)
To: linuxppc-embedded, jacmet
[-- Attachment #1: Type: text/plain, Size: 488 bytes --]
Hi Peter,
The attached patch gets your uartlite driver going on MicroBlaze.
All readb/writeb ops are converted to ioread32/iowrite32.
On MicroBlaze readb/writeb are picking up the MSB, instead of LSB, and
thus reading all zeros instead of the 8-bit control/status/FIFO
registers that you intended.
Can you please confirm if this works on PPC?
I note that Grant's recent bootloader driver uses in_be32/out_be32 -
would you prefer that instead of ioread32/iowrite32?
Thanks,
John
[-- Attachment #2: 00001-uartlite-32bit-ops.patch --]
[-- Type: text/plain, Size: 3731 bytes --]
Convert readb/writeb ops into ioread32/iowrite32.
This gets the driver working with MicroBlaze (2.6.20).
signed-off-by: John Williams jwilliams@itee.uq.edu.au
Index: linux-2.6.x/drivers/serial/uartlite.c
===================================================================
--- linux-2.6.x/drivers/serial/uartlite.c (revision 2561)
+++ linux-2.6.x/drivers/serial/uartlite.c (working copy)
@@ -61,7 +61,7 @@
/* stats */
if (stat & ULITE_STATUS_RXVALID) {
port->icount.rx++;
- ch = readb(port->membase + ULITE_RX);
+ ch = ioread32(port->membase + ULITE_RX);
if (stat & ULITE_STATUS_PARITY)
port->icount.parity++;
@@ -106,7 +106,7 @@
return 0;
if (port->x_char) {
- writeb(port->x_char, port->membase + ULITE_TX);
+ iowrite32(port->x_char, port->membase + ULITE_TX);
port->x_char = 0;
port->icount.tx++;
return 1;
@@ -115,7 +115,7 @@
if (uart_circ_empty(xmit) || uart_tx_stopped(port))
return 0;
- writeb(xmit->buf[xmit->tail], port->membase + ULITE_TX);
+ iowrite32(xmit->buf[xmit->tail], port->membase + ULITE_TX);
xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE-1);
port->icount.tx++;
@@ -132,7 +132,7 @@
int busy;
do {
- int stat = readb(port->membase + ULITE_STATUS);
+ int stat = ioread32(port->membase + ULITE_STATUS);
busy = ulite_receive(port, stat);
busy |= ulite_transmit(port, stat);
} while (busy);
@@ -148,7 +148,7 @@
unsigned int ret;
spin_lock_irqsave(&port->lock, flags);
- ret = readb(port->membase + ULITE_STATUS);
+ ret = ioread32(port->membase + ULITE_STATUS);
spin_unlock_irqrestore(&port->lock, flags);
return ret & ULITE_STATUS_TXEMPTY ? TIOCSER_TEMT : 0;
@@ -171,7 +171,7 @@
static void ulite_start_tx(struct uart_port *port)
{
- ulite_transmit(port, readb(port->membase + ULITE_STATUS));
+ ulite_transmit(port, ioread32(port->membase + ULITE_STATUS));
}
static void ulite_stop_rx(struct uart_port *port)
@@ -200,17 +200,17 @@
if (ret)
return ret;
- writeb(ULITE_CONTROL_RST_RX | ULITE_CONTROL_RST_TX,
+ iowrite32(ULITE_CONTROL_RST_RX | ULITE_CONTROL_RST_TX,
port->membase + ULITE_CONTROL);
- writeb(ULITE_CONTROL_IE, port->membase + ULITE_CONTROL);
+ iowrite32(ULITE_CONTROL_IE, port->membase + ULITE_CONTROL);
return 0;
}
static void ulite_shutdown(struct uart_port *port)
{
- writeb(0, port->membase + ULITE_CONTROL);
- readb(port->membase + ULITE_CONTROL); /* dummy */
+ iowrite32(0, port->membase + ULITE_CONTROL);
+ ioread32(port->membase + ULITE_CONTROL); /* dummy */
free_irq(port->irq, port);
}
@@ -314,7 +314,7 @@
/* wait up to 10ms for the character(s) to be sent */
for (i = 0; i < 10000; i++) {
- if (readb(port->membase + ULITE_STATUS) & ULITE_STATUS_TXEMPTY)
+ if (ioread32(port->membase + ULITE_STATUS) & ULITE_STATUS_TXEMPTY)
break;
udelay(1);
}
@@ -323,7 +323,7 @@
static void ulite_console_putchar(struct uart_port *port, int ch)
{
ulite_console_wait_tx(port);
- writeb(ch, port->membase + ULITE_TX);
+ iowrite32(ch, port->membase + ULITE_TX);
}
static void ulite_console_write(struct console *co, const char *s,
@@ -340,8 +340,8 @@
spin_lock_irqsave(&port->lock, flags);
/* save and disable interrupt */
- ier = readb(port->membase + ULITE_STATUS) & ULITE_STATUS_IE;
- writeb(0, port->membase + ULITE_CONTROL);
+ ier = ioread32(port->membase + ULITE_STATUS) & ULITE_STATUS_IE;
+ iowrite32(0, port->membase + ULITE_CONTROL);
uart_console_write(port, s, count, ulite_console_putchar);
@@ -349,7 +349,7 @@
/* restore interrupt state */
if (ier)
- writeb(ULITE_CONTROL_IE, port->membase + ULITE_CONTROL);
+ iowrite32(ULITE_CONTROL_IE, port->membase + ULITE_CONTROL);
if (locked)
spin_unlock_irqrestore(&port->lock, flags);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-01 4:55 [RFC] uartlite driver MicroBlaze compatability John Williams
@ 2007-05-01 5:55 ` Grant Likely
2007-05-01 6:42 ` John Williams
2007-05-02 13:59 ` Peter Korsgaard
2007-05-02 13:45 ` Peter Korsgaard
1 sibling, 2 replies; 11+ messages in thread
From: Grant Likely @ 2007-05-01 5:55 UTC (permalink / raw)
To: John Williams; +Cc: jacmet, linuxppc-embedded
[-- Attachment #1: Type: text/plain, Size: 2943 bytes --]
On 4/30/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
> Hi Peter,
>
> The attached patch gets your uartlite driver going on MicroBlaze.
>
> All readb/writeb ops are converted to ioread32/iowrite32.
>
> On MicroBlaze readb/writeb are picking up the MSB, instead of LSB, and
> thus reading all zeros instead of the 8-bit control/status/FIFO
> registers that you intended.
>
> Can you please confirm if this works on PPC?
Yes, I've confirmed this does work on PPC; but I don't think it's
quite the correct fix.
ioread/write32 is mapped to in/out_le32, yet the bootloader driver
must use in/out_be32. This is because the uartlite driver follows the
lead of 8250 and requires an offset of 3 from the base address in
order to find the relevant byte wise address. In fact, I believe the
driver should work as-is on microblaze if the offset-by-3 is not used
when registering it to the platform bus.
However, the uartlite is *not* an 8250. The 8250 turns up all over
the place and it's registers are defined as 8 bit wide. The
offset-by-3 stuff is part of the plat_serial8250_port structure which
is also used to specify .regshift (increment between registers).
Whereas the UARTLITE is defined as a 32 bit device and it doesn't show
up in anywhere near as many designs. Registers are always 4 bytes
wide and are always located at multiples of 4 bytes off the base
address.
The biggest problem with keeping the 3 byte offset and using
ioread/write32 on it makes every register access straddle a 32-bit
boundary. This means 2 bus transactions for every register access.
Absolutely not what we want.
The problem with keeping the byte-wise access as it is now is that it
means the platform bus binding needs to explicitly know what the host
access width is and add the 3 byte offset accordingly (rather than
using the base address as specified in xparameters unmodified and
using the in/out_be32 macro take care of reading it correctly &
efficiently).
(There are also annoyances that will come up when we move to
arch/powerpc and hook it up to the of_platform_bus)
>
> I note that Grant's recent bootloader driver uses in_be32/out_be32 -
> would you prefer that instead of ioread32/iowrite32?
I certainly think so. The device is documented as using 32 bit BE
registers; so the driver should access them as 32bit BE registers
IMHO. Or at least, if there is a good reason to continue the bytewise
access, then the driver should contain the smarts to translate from
documented base address to the appropriate offset.
So; starting with your patch and modifying it, I've attached I think
the change should be. It should work for microblaze, but I've only
tested w/ ppc. Unfortunately the (void*) casts are ugly; there might
be a way around that, but it's due to the type used for the (struct
uart_port)->membase variable.
Cheers,
g.
--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
[-- Attachment #2: 0001-PPC-Fix-UARTLITE-register-access-for-little-endian.patch --]
[-- Type: application/x-patch, Size: 5228 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-01 5:55 ` Grant Likely
@ 2007-05-01 6:42 ` John Williams
2007-05-02 5:47 ` Grant Likely
2007-05-02 13:59 ` Peter Korsgaard
1 sibling, 1 reply; 11+ messages in thread
From: John Williams @ 2007-05-01 6:42 UTC (permalink / raw)
To: Grant Likely; +Cc: jacmet, linuxppc-embedded
Grant Likely wrote:
> On 4/30/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
>
>> All readb/writeb ops are converted to ioread32/iowrite32.
>>
>> On MicroBlaze readb/writeb are picking up the MSB, instead of LSB, and
>> thus reading all zeros instead of the 8-bit control/status/FIFO
>> registers that you intended.
>>
>> Can you please confirm if this works on PPC?
>
> Yes, I've confirmed this does work on PPC; but I don't think it's
> quite the correct fix.
>
> ioread/write32 is mapped to in/out_le32, yet the bootloader driver
> must use in/out_be32. This is because the uartlite driver follows the
> lead of 8250 and requires an offset of 3 from the base address in
> order to find the relevant byte wise address. In fact, I believe the
> driver should work as-is on microblaze if the offset-by-3 is not used
> when registering it to the platform bus.
ugh. I missed the off-by-3 stuff in the PPC platform setup. Smells bad!
> However, the uartlite is *not* an 8250. The 8250 turns up all over
> the place and it's registers are defined as 8 bit wide. The
> offset-by-3 stuff is part of the plat_serial8250_port structure which
> is also used to specify .regshift (increment between registers).
> Whereas the UARTLITE is defined as a 32 bit device and it doesn't show
> up in anywhere near as many designs. Registers are always 4 bytes
> wide and are always located at multiples of 4 bytes off the base
> address.
Agreed.
> So; starting with your patch and modifying it, I've attached I think
> the change should be. It should work for microblaze, but I've only
> tested w/ ppc. Unfortunately the (void*) casts are ugly; there might
> be a way around that, but it's due to the type used for the (struct
> uart_port)->membase variable.
Looks good - boot tested on MicroBlaze 2.6.20
acked-by: John Williams <jwilliams@itee.uq.edu.au>
John
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-01 6:42 ` John Williams
@ 2007-05-02 5:47 ` Grant Likely
2007-05-02 6:18 ` John Williams
2007-05-02 14:09 ` Peter Korsgaard
0 siblings, 2 replies; 11+ messages in thread
From: Grant Likely @ 2007-05-02 5:47 UTC (permalink / raw)
To: John Williams, David H. Lynch Jr., Peter Korsgaard; +Cc: linuxppc-embedded
[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]
On 5/1/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
> Grant Likely wrote:
> > However, the uartlite is *not* an 8250. The 8250 turns up all over
> > the place and it's registers are defined as 8 bit wide. The
> > offset-by-3 stuff is part of the plat_serial8250_port structure which
> > is also used to specify .regshift (increment between registers).
> > Whereas the UARTLITE is defined as a 32 bit device and it doesn't show
> > up in anywhere near as many designs. Registers are always 4 bytes
> > wide and are always located at multiples of 4 bytes off the base
Hmm, I think I was smoking something last night. Address used for 8
bit access should not be affected by CPU endianess. After David's
comments, I reread the uartlite documentation. The current design is
definately for 32bit OPB bus connections, but it looks like there is a
posibility for xilinx to add a 16 or 8 bit attachment. Since the
uartlite design explicitly supports 8, 16 and 32 bit access, sticking
with 8 bit io may be the safest. However, I still think the
application of the 3 byte offset should be done in the driver, and not
in the platform bus registration.
I've reworked the patch with the following changes
- remove 3 byte offset from platform bus registration.
- added ulite_in/ulite_out macros to make changing bus attachment
details simpler if xilinx changes the uartlite design.
- stick with 8 bit IO.
Tested on PPC. John, can you please test on microblaze?
Cheers,
g.
--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
[-- Attachment #2: 0001-POWERPC-Fix-UARTLITE-driver-to-add-a-3-byte-offset.patch --]
[-- Type: application/x-patch, Size: 5412 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-02 5:47 ` Grant Likely
@ 2007-05-02 6:18 ` John Williams
2007-05-02 14:09 ` Peter Korsgaard
1 sibling, 0 replies; 11+ messages in thread
From: John Williams @ 2007-05-02 6:18 UTC (permalink / raw)
To: Grant Likely; +Cc: Peter Korsgaard, linuxppc-embedded
Grant Likely wrote:
> On 5/1/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
>
>> Grant Likely wrote:
>> > However, the uartlite is *not* an 8250. The 8250 turns up all over
>> > the place and it's registers are defined as 8 bit wide. The
>> > offset-by-3 stuff is part of the plat_serial8250_port structure which
>> > is also used to specify .regshift (increment between registers).
>> > Whereas the UARTLITE is defined as a 32 bit device and it doesn't show
>> > up in anywhere near as many designs. Registers are always 4 bytes
>> > wide and are always located at multiples of 4 bytes off the base
>
> Hmm, I think I was smoking something last night. Address used for 8
> bit access should not be affected by CPU endianess. After David's
> comments, I reread the uartlite documentation. The current design is
> definately for 32bit OPB bus connections, but it looks like there is a
> posibility for xilinx to add a 16 or 8 bit attachment. Since the
> uartlite design explicitly supports 8, 16 and 32 bit access, sticking
> with 8 bit io may be the safest.
To be honest I don't think that will ever happen - just because the OPB
bus data width is parameterisable, doesn't mean that it actually *works*
or has been tested on anything other than 32-bits wide. I've certainly
never heard of anyone doing so, on either MicroBlaze or PPC.
but, I won't fight over it :)
Either way, it will still require a code change if/when someone does a
16/8 bit wide OPB bus. Whether they change the IO access operation, or
a hardcoded constant, it's still not perfect.
Of course the real solution here is to create an OPB bus driver, with a
'width' field that you can pull out of XPAR, and so on... Use that
instead of platform bus, and all this rubbish can be dealt with cleanly.
> However, I still think the
> application of the 3 byte offset should be done in the driver, and not
> in the platform bus registration.
If it has to be done, I agree the driver is the place to put it.
> I've reworked the patch with the following changes
> - remove 3 byte offset from platform bus registration.
> - added ulite_in/ulite_out macros to make changing bus attachment
> details simpler if xilinx changes the uartlite design.
> - stick with 8 bit IO.
It works fine, however perhaps a comment explaining the +3 offset might
be appreciated by those who follow.
> Tested on PPC. John, can you please test on microblaze?
Acked-by: John Williams <jwilliams@itee.uq.edu.au>
John
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-01 4:55 [RFC] uartlite driver MicroBlaze compatability John Williams
2007-05-01 5:55 ` Grant Likely
@ 2007-05-02 13:45 ` Peter Korsgaard
2007-05-03 1:08 ` John Williams
1 sibling, 1 reply; 11+ messages in thread
From: Peter Korsgaard @ 2007-05-02 13:45 UTC (permalink / raw)
To: linuxppc-embedded
>>>>> "JW" == John Williams <jwilliams@itee.uq.edu.au> writes:
Hi,
JW> The attached patch gets your uartlite driver going on MicroBlaze.
Nice!
JW> All readb/writeb ops are converted to ioread32/iowrite32.
JW> On MicroBlaze readb/writeb are picking up the MSB, instead of LSB,
JW> and thus reading all zeros instead of the 8-bit
JW> control/status/FIFO registers that you intended.
I take it that the microblaze is big endian? Then you just need to add
3 to the base address and everything should work without your patch.
JW> Can you please confirm if this works on PPC?
It won't as ioread/write does big/little endian byte swapping. Isn't
that done on microblaze?
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-01 5:55 ` Grant Likely
2007-05-01 6:42 ` John Williams
@ 2007-05-02 13:59 ` Peter Korsgaard
1 sibling, 0 replies; 11+ messages in thread
From: Peter Korsgaard @ 2007-05-02 13:59 UTC (permalink / raw)
To: linuxppc-embedded
>>>>> "GL" == Grant Likely <grant.likely@secretlab.ca> writes:
Hi,
>> Can you please confirm if this works on PPC?
GL> Yes, I've confirmed this does work on PPC; but I don't think it's
GL> quite the correct fix.
GL> ioread/write32 is mapped to in/out_le32, yet the bootloader driver
GL> must use in/out_be32. This is because the uartlite driver follows
GL> the lead of 8250 and requires an offset of 3 from the base address
GL> in order to find the relevant byte wise address. In fact, I
GL> believe the driver should work as-is on microblaze if the
GL> offset-by-3 is not used when registering it to the platform bus.
Not used? Isn't the microblaze big endian as well?
GL> However, the uartlite is *not* an 8250. The 8250 turns up all
GL> over the place and it's registers are defined as 8 bit wide. The
GL> offset-by-3 stuff is part of the plat_serial8250_port structure
GL> which is also used to specify .regshift (increment between
GL> registers). Whereas the UARTLITE is defined as a 32 bit device
GL> and it doesn't show up in anywhere near as many designs.
GL> Registers are always 4 bytes wide and are always located at
GL> multiples of 4 bytes off the base address.
Well, yes and no - The registers physically contains 8 bit of
information, but are commonly located on the 32bit opb bus.
GL> The biggest problem with keeping the 3 byte offset and using
GL> ioread/write32 on it makes every register access straddle a 32-bit
GL> boundary. This means 2 bus transactions for every register
GL> access. Absolutely not what we want.
Exactly.
GL> The problem with keeping the byte-wise access as it is now is that
GL> it means the platform bus binding needs to explicitly know what
GL> the host access width is and add the 3 byte offset accordingly
GL> (rather than using the base address as specified in xparameters
GL> unmodified and using the in/out_be32 macro take care of reading it
GL> correctly & efficiently).
I don't think that's a big problem. Other reasons for using 8bit I/O
are:
- No endianness problems (besides setting the proper base
address). "There's no read/write register in native endianness"
interface in the kernel. readl is always little endian, and _be32
would be wrong/not available on all archs. The uartlite interface is
nice and simple - Who knows if someone would add a FPGA with a bunch
of uartlite's to an ARM/MIPS/whatever design?
- No bus width problems. We have designs which needed extra uarts late
in the design, and have implemented uartlite compatible firmware in
a SP3e FPGA connected over a 16bit bus (EMC). The uartlite driver
works nicely with that as it is. With 32bit access you would double
the bus transactions.
- It matches 8250.c
GL> (There are also annoyances that will come up when we move to
GL> arch/powerpc and hook it up to the of_platform_bus)
Ohh, like what?
>> I note that Grant's recent bootloader driver uses in_be32/out_be32
>> - would you prefer that instead of ioread32/iowrite32?
GL> I certainly think so. The device is documented as using 32 bit BE
GL> registers; so the driver should access them as 32bit BE registers
GL> IMHO. Or at least, if there is a good reason to continue the
GL> bytewise access, then the driver should contain the smarts to
GL> translate from documented base address to the appropriate offset.
How should it be able to do that? Using some magic #ifdef to know if
it's compiled for a big endian arch and do a +3? That seems ugly to
me.
In other words - I disagree.
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-02 5:47 ` Grant Likely
2007-05-02 6:18 ` John Williams
@ 2007-05-02 14:09 ` Peter Korsgaard
2007-05-02 15:59 ` Grant Likely
1 sibling, 1 reply; 11+ messages in thread
From: Peter Korsgaard @ 2007-05-02 14:09 UTC (permalink / raw)
To: linuxppc-embedded
>>>>> "GL" == Grant Likely <grant.likely@secretlab.ca> writes:
Hi,
GL> Hmm, I think I was smoking something last night.
;)
GL> Address used for 8 bit access should not be affected by CPU
GL> endianess. After David's comments, I reread the uartlite
GL> documentation. The current design is definately for 32bit OPB bus
GL> connections, but it looks like there is a posibility for xilinx to
GL> add a 16 or 8 bit attachment. Since the uartlite design
GL> explicitly supports 8, 16 and 32 bit access, sticking with 8 bit
GL> io may be the safest. However, I still think the application of
GL> the 3 byte offset should be done in the driver, and not in the
GL> platform bus registration.
That would effectively make the driver big endian only. What if Xilinx
would come out with a FPGA with a ARM core in it?
GL> I've reworked the patch with the following changes - remove 3 byte
GL> offset from platform bus registration. - added ulite_in/ulite_out
GL> macros to make changing bus attachment details simpler if xilinx
GL> changes the uartlite design. - stick with 8 bit IO.
Russell didn't like those accessor macros back when it was submitted
last year:
http://thread.gmane.org/gmane.linux.serial/1237/focus=1251
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-02 14:09 ` Peter Korsgaard
@ 2007-05-02 15:59 ` Grant Likely
0 siblings, 0 replies; 11+ messages in thread
From: Grant Likely @ 2007-05-02 15:59 UTC (permalink / raw)
To: Peter Korsgaard; +Cc: linuxppc-embedded
On 5/2/07, Peter Korsgaard <peter.korsgaard@barco.com> wrote:
> >>>>> "GL" == Grant Likely <grant.likely@secretlab.ca> writes:
>
> Hi,
>
> GL> Hmm, I think I was smoking something last night.
>
> ;)
>
> GL> Address used for 8 bit access should not be affected by CPU
> GL> endianess. After David's comments, I reread the uartlite
> GL> documentation. The current design is definately for 32bit OPB bus
> GL> connections, but it looks like there is a posibility for xilinx to
> GL> add a 16 or 8 bit attachment. Since the uartlite design
> GL> explicitly supports 8, 16 and 32 bit access, sticking with 8 bit
> GL> io may be the safest. However, I still think the application of
> GL> the 3 byte offset should be done in the driver, and not in the
> GL> platform bus registration.
>
> That would effectively make the driver big endian only. What if Xilinx
> would come out with a FPGA with a ARM core in it?
It shouldn't. When doing byte-wise access, byte 3 is *always* byte 3;
regardless of the endianess of the processor. It's the endianess of
the device that determines where the individual bytes show up. The
opb_uartlite documentation defines the registers as big endian,
therefore if you do a bytewise access to address 0x3, you'll always
get the least significant byte of the first register.
Since the 3 byte offset does not change between little endian and big
endian processors then I think the knowledge of the offset should be
kept in the driver, not the device registration.
>
> GL> I've reworked the patch with the following changes - remove 3 byte
> GL> offset from platform bus registration. - added ulite_in/ulite_out
> GL> macros to make changing bus attachment details simpler if xilinx
> GL> changes the uartlite design. - stick with 8 bit IO.
>
> Russell didn't like those accessor macros back when it was submitted
> last year:
>> +static inline void serial_out(struct uart_port *port, int offset, int
Russell> Since there's no additional complication here, do you need separate
Russell> serial_in/serial_out inline functions?
As I think the driver should know about the 3 byte offset; the
accessor macro makes sense again. Plus if Xilinx ever adds a uartlite
varient w/ bytewise or halfword wise registers it becomes much easier
to taylor in the future.
Cheers,
g.
--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-02 13:45 ` Peter Korsgaard
@ 2007-05-03 1:08 ` John Williams
2007-05-03 10:22 ` David H. Lynch Jr.
0 siblings, 1 reply; 11+ messages in thread
From: John Williams @ 2007-05-03 1:08 UTC (permalink / raw)
To: Peter Korsgaard; +Cc: linuxppc-embedded
Hi Peter,
Peter Korsgaard wrote:
> JW> The attached patch gets your uartlite driver going on MicroBlaze.
>
> Nice!
>
> JW> All readb/writeb ops are converted to ioread32/iowrite32.
>
> JW> On MicroBlaze readb/writeb are picking up the MSB, instead of LSB,
> JW> and thus reading all zeros instead of the 8-bit
> JW> control/status/FIFO registers that you intended.
>
> I take it that the microblaze is big endian? Then you just need to add
> 3 to the base address and everything should work without your patch.
I struggle to see adding 3 to the base address in the platform driver as
a clean solution. The base address of the peripheral is 0x10240000, or
whatever, not 0x10240003.
I understand the reasoning for it, but from the platform's perspective
it seems wrong.
If you read the opb_uartlite datasheet, it says that bits 0-26 of the
FIFO, CTRL and STATUS regs are "reserved". It doesn't say, this is an
8-bit peripheral that is mapped onto a 32bit bus with a stride of 4.
If you also read page 6 , under address map, it says
BASE_ADDRESS+0 : read from receive FIFO
BASE_ADDRESS+4 : write to transmit FIFO
and so on.
It is a 32-bit peripheral, it just so happens the 24 of those bits are
currently "reserved".
Grant's recanting may have been triggered by the figure on page 4 of the
datasheet, which is generic Xilnx IP Core datasheet material explaining
the endian interpretation for different data widths.
> JW> Can you please confirm if this works on PPC?
>
> It won't as ioread/write does big/little endian byte swapping. Isn't
> that done on microblaze?
Not presently, but I will fix that.
I think that's Grant's approach of using in/out_be32, and the real base
address (ie not +3) is the only logically correct solution.
Regards,
John
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] uartlite driver MicroBlaze compatability
2007-05-03 1:08 ` John Williams
@ 2007-05-03 10:22 ` David H. Lynch Jr.
0 siblings, 0 replies; 11+ messages in thread
From: David H. Lynch Jr. @ 2007-05-03 10:22 UTC (permalink / raw)
To: linuxppc-embedded
John Williams wrote:
>
> I think that's Grant's approach of using in/out_be32, and the real base
> address (ie not +3) is the only logically correct solution.
>
Trying to track how every permutation maps in every possible
scenario is more than my brain can handle.
What I do know is that throwing an arbitrary +3 offset in is
incredibly confusing.
While I pointed out that the Xilinx docs sugguest implimentations
with regshifts of 0,1 &2 - and I beleive
I have seen xilinx example code that sugest they may have somehow
implimented UartLite on occasion
with less than 32bit registers. The stock GreenHills Integrity
default UartLite driver presumes a regshift of 0.
But I have never seen an 8 or 16 bit Uartlite.
However before either my driver or Peters showed up there was one
posted to this list that used dcr.
My suggestion would be to handle "port IO" inside
serial_in()serial_out() functions like the 8250 driver (and about 1/3 of
the
other serial drivers) do.
While in a perfect world we should be able to compile the UartLite
driver without change for an x86
if somehow that can be created, atleast with all the io moved to two
functions, adjusting for different regshifts,
endians, .... requires a very limited number of localized changes.
I would be happy to submit a patch to do that
but I can not test it because I can not get Peter's driver to work
with my hardware.
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2007-05-03 10:24 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-05-01 4:55 [RFC] uartlite driver MicroBlaze compatability John Williams
2007-05-01 5:55 ` Grant Likely
2007-05-01 6:42 ` John Williams
2007-05-02 5:47 ` Grant Likely
2007-05-02 6:18 ` John Williams
2007-05-02 14:09 ` Peter Korsgaard
2007-05-02 15:59 ` Grant Likely
2007-05-02 13:59 ` Peter Korsgaard
2007-05-02 13:45 ` Peter Korsgaard
2007-05-03 1:08 ` John Williams
2007-05-03 10:22 ` David H. Lynch Jr.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).