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 <[email protected]>
---
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:
>>
>> 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:
>> 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
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:
>
>> 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:
> > 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.
>
> 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 }