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.

---
drivers/serial/cpm_uart/cpm_uart_core.c | 12 ++++++++----

1 files changed, 8 insertions(+), 4 deletions(-)

index cefde58..7f5db7c 100644
@@ -752,8 +752,10 @@ static void cpm_uart_init_scc(struct uart_cpm_port *pinfo)
 	sup = pinfo->sccup;
 
 	/* Store address */
-	pinfo->sccup->scc_genscc.scc_rbase = (unsigned char *)pinfo->rx_bd_base - DPRAM_BASE;
-	pinfo->sccup->scc_genscc.scc_tbase = (unsigned char *)pinfo->tx_bd_base - DPRAM_BASE;
+	pinfo->sccup->scc_genscc.scc_rbase = 
+		(u_char *)cpm_dpram_phys((u8 *)pinfo->rx_bd_base) - DPRAM_BASE;
+	pinfo->sccup->scc_genscc.scc_tbase = 
+		(u_char *)cpm_dpram_phys((u8 *)pinfo->tx_bd_base) - DPRAM_BASE;
 
 	/* Set up the uart parameters in the
 	 * parameter ram.

@@ -813,8 +815,10 @@ static void cpm_uart_init_smc(struct uart_cpm_port *pinfo)
 	up = pinfo->smcup;
 
 	/* Store address */
-	pinfo->smcup->smc_rbase = (u_char *)pinfo->rx_bd_base - DPRAM_BASE;
-	pinfo->smcup->smc_tbase = (u_char *)pinfo->tx_bd_base - DPRAM_BASE;
+	pinfo->smcup->smc_rbase = 
+		(u_char *)cpm_dpram_phys((u8 *)pinfo->rx_bd_base) - DPRAM_BASE;
+	pinfo->smcup->smc_tbase = 
+		(u_char *)cpm_dpram_phys((u8 *)pinfo->tx_bd_base) - DPRAM_BASE;
 
 /*
  *  In case SMC1 is being relocated...

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

-Scott

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

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

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

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

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 }

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

Hi Scott,

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

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