RE: [PATCH4/4] [POWERPC] Fix cpm_uart driver
> 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, 0x1000, _PAGE_IO);
209
210 /* Map I/O region to a 256MB BAT */
211 io_block_mapping(IO_VIRT_ADDR, IO_PHYS_ADDR, 0x1000,
_PAGE_IO);
212 }
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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, 0x1000, _PAGE_IO);
209
210 /* Map I/O region to a 256MB BAT */
211 io_block_mapping(IO_VIRT_ADDR, IO_PHYS_ADDR, 0x1000,
_PAGE_IO);
212 }
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH4/4] [POWERPC] Fix cpm_uart driver
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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 <[EMAIL PROTECTED]> --- drivers/serial/cpm_uart/cpm_uart_core.c | 12 1 files changed, 8 insertions(+), 4 deletions(-) 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...
[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 [EMAIL PROTECTED] --- drivers/serial/cpm_uart/cpm_uart_core.c | 12 1 files changed, 8 insertions(+), 4 deletions(-) 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...
