[PATCH4/4] [POWERPC] Fix cpm_uart driver

[PATCH4/4] [POWERPC] Fix cpm_uart driver

[Jochen Friedrich]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]


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(-)

[-- Attachment #2: 2c5cf6868e9aa6eadea285e524d88859cc5e54fb.diff --]
[-- Type: text/x-patch, Size: 1298 bytes --]

diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index cefde58..7f5db7c 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -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...

Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Scott Wood]

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

Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Jochen Friedrich]

Scott Wood schrieb:
> 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.

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

Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Scott Wood]

Jochen Friedrich wrote:
> Scott Wood schrieb:
>> 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.
> 
> 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

Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Dan Malek]

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

Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Scott Wood]

Dan Malek wrote:
> 
> 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.

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

Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Jochen Friedrich]

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

Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Scott Wood]

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

RE: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Rune Torgersen]

> 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)

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 = (cpm2_immr != 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 }

Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver

[Scott Wood]

On Wed, Sep 26, 2007 at 03:32:29PM -0500, Rune Torgersen wrote:
> > 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