[PATCH4/4] [POWERPC] Fix cpm_uart driver

In cpm_uart_core, functions cpm_uart_init_bd and cpm_uart_init_scc
an offset into DP RAM is calculated by substracting a physical
memory constant from an virtual address. This patch fixes the
problem by converting the virtual address into a physical
first.

Signed-off-by: Jochen Friedrich <jochen@scram.de>
---
 drivers/serial/cpm_uart/cpm_uart_core.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

Jochen Friedrich wrote:

In cpm_uart_core, functions cpm_uart_init_bd and cpm_uart_init_scc
an offset into DP RAM is calculated by substracting a physical
memory constant from an virtual address. This patch fixes the
problem by converting the virtual address into a physical
first.

Huh?  DPRAM_BASE is a virtual address.  With this patch, you'd be 
subtracting a virtual address from a physical address.

-Scott

Scott Wood schrieb:

Jochen Friedrich wrote:

quoted

In cpm_uart_core, functions cpm_uart_init_bd and cpm_uart_init_scc
an offset into DP RAM is calculated by substracting a physical
memory constant from an virtual address. This patch fixes the
problem by converting the virtual address into a physical
first.

Huh?  DPRAM_BASE is a virtual address.  With this patch, you'd be 
subtracting a virtual address from a physical address.

Thanks for pointing me to it. So the bug is in cpm_uart_cpm1.h assigning 
a physical memory to DPRAM_BASE (at least on ARC=ppc). cpm_uart_cpm2.h 
seems to be correct though. I'll submit a new patch for this.

Thanks,
Jochen

Jochen Friedrich wrote:

Scott Wood schrieb:

quoted

Jochen Friedrich wrote:

quoted

In cpm_uart_core, functions cpm_uart_init_bd and cpm_uart_init_scc
an offset into DP RAM is calculated by substracting a physical
memory constant from an virtual address. This patch fixes the
problem by converting the virtual address into a physical
first.

Huh?  DPRAM_BASE is a virtual address.  With this patch, you'd be 
subtracting a virtual address from a physical address.

Thanks for pointing me to it. So the bug is in cpm_uart_cpm1.h assigning 
a physical memory to DPRAM_BASE (at least on ARC=ppc). cpm_uart_cpm2.h 
seems to be correct though. I'll submit a new patch for this.

cpmp is a physical address on arch/ppc?
/me looks at arch/ppc/8xx_io/commproc.c

Yikes.  Please don't change cpm_uart_cpm1.h, as it's correct for 
arch/powerpc, and there are numerous other places that assume cpmp is 
virtual (including in the very same function that assigns it a physical 
address).

You could fix arch/ppc if you want, though it may be easier to wait for 
it to die, and insist on identity maps in the meantime. :-)

-Scott

On Sep 24, 2007, at 11:22 AM, Scott Wood wrote:

cpmp is a physical address on arch/ppc?

No, it's a well known ioremaped() address
into the IMMR space.  The only physical
addresses in any of the CPM/CPM2 are
those required to by the buffer descriptors.
There are DPRAM offsets, but they should
be just that, offsets from either a virtual
or physical base address as required.
Too many people screw around in this
CPM support code without fully understanding
the original implementation or its intended
use with the peripheral drivers.  A "better
idea" often breaks all drivers except the one
that is being changed.

	-- Dan

Dan Malek wrote:

On Sep 24, 2007, at 11:22 AM, Scott Wood wrote:

quoted

cpmp is a physical address on arch/ppc?

No, it's a well known ioremaped() address into the IMMR space.

Maybe that's how it was, but the current code initializes it (more or
less) directly with IMAP_ADDR, which also gets fed into ioremap.

One of the two has got to be wrong.

-Scott

Hi Scott,

Yikes.  Please don't change cpm_uart_cpm1.h, as it's correct for 
arch/powerpc, and there are numerous other places that assume cpmp is 
virtual (including in the very same function that assigns it a 
physical address).

I'm still not convinced cpm_uart_cpm1.h is correct:

pinfo->rx_bd_base and pinfo->tx_bd_base are both initialized with an 
address obtained by cpm_dpram_addr(). In both ppc and powerpc, this is 
an address relative to dpram_vbase in commproc.c

In cpm_uart_core.c, the operation "pinfo->rx_bd_base - DPRAM_BASE" is 
used to calculate the DPRAM offset. So DPRAM_BASE must be relative to 
dpram_vbase in commproc.c as well. However, cpm_uart_cpm1.h uses cpmp in 
commproc.c to initialize DPRAM_BASE.

On ARCH=ppc, cpmp is a physical address with 1:1 virtual mapping ("well 
known address"). On ARC=powerpc, this is an address obtained by 
ioremap(), however it's a different ioremap() call than dpram_vbase is 
obtained from, so noone can guarantee
cpmp is always the same as dpram_vbase even on ARCH=powerpc.

To me, it looks like setting DPRAM_BASE to cpm_dpram_addr(0) is the fix 
as it makes the DPRAM offset a defined result.

Thanks,
Jochen

On Tue, Sep 25, 2007 at 02:09:03PM +0200, Jochen Friedrich wrote:

In cpm_uart_core.c, the operation "pinfo->rx_bd_base - DPRAM_BASE" is 
used to calculate the DPRAM offset. So DPRAM_BASE must be relative to 
dpram_vbase in commproc.c as well. However, cpm_uart_cpm1.h uses cpmp in 
commproc.c to initialize DPRAM_BASE.

On ARCH=ppc, cpmp is a physical address with 1:1 virtual mapping ("well 
known address"). On ARC=powerpc, this is an address obtained by 
ioremap(), however it's a different ioremap() call than dpram_vbase is 
obtained from, so noone can guarantee
cpmp is always the same as dpram_vbase even on ARCH=powerpc.

I have patches submitted in which they're from the same ioremap, but
I agree that using cpm_dpram_addr(0) is a more robust way.

-Scott

On Wed, Sep 26, 2007 at 03:32:29PM -0500, Rune Torgersen wrote:

quoted

From: Scott Wood
Maybe that's how it was, but the current code initializes it (more or
less) directly with IMAP_ADDR, which also gets fed into ioremap.

One of the two has got to be wrong.

arch/ppc maps the immr area 1:1 into kernel memory, so ioremap and
physical are the same.
See arch/ppc/syslib/m8260_setup.c, line 208 (function m8260_map_io)

We were talking about 8xx, not 82xx -- is it always identity mapped there?

If so, then why bother with the ioremap in immr_map_size() in
arch/ppc/8xx_io/commproc.c?  And why compare the result from ioremap() with
a raw identity-mapped address?

-Scott

From: Scott Wood
Maybe that's how it was, but the current code initializes it (more or
less) directly with IMAP_ADDR, which also gets fed into ioremap.
=20
One of the two has got to be wrong.

arch/ppc maps the immr area 1:1 into kernel memory, so ioremap and
physical are the same.
See arch/ppc/syslib/m8260_setup.c, line 208 (function m8260_map_io)

Here quoted:
arch/ppc/syslib/m8260_setup.c
196 /* Map the IMMR, plus anything else we can cover
197  * in that upper space according to the memory controller
198  * chip select mapping.  Grab another bunch of space
199  * below that for stuff we can't cover in the upper.
200  */
201 static void __init
202 m8260_map_io(void)
203 {
204         uint addr;
205
206         /* Map IMMR region to a 256MB BAT */
207         addr =3D (cpm2_immr !=3D NULL) ? (uint)cpm2_immr : =
CPM_MAP_ADDR;
208         io_block_mapping(addr, addr, 0x10000000, _PAGE_IO);
209
210         /* Map I/O region to a 256MB BAT */
211         io_block_mapping(IO_VIRT_ADDR, IO_PHYS_ADDR, 0x10000000,
_PAGE_IO);
212 }