Get rid of private immrbar_virt_to_phys() routine and
use generic iopa().
Signed-off-by: Li Yang <redacted>
---
drivers/net/ucc_geth.c | 29 ++++++-----------------------
1 files changed, 6 insertions(+), 23 deletions(-)
@@ -3331,15 +3321,8 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)/* Setup the table *//* Assume BD rings are already established */for(i=0;i<ug_info->numQueuesRx;i++){-if(ugeth->ug_info->uf_info.bd_mem_part==MEM_PART_SYSTEM){-out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,-(u32)virt_to_phys(ugeth->p_rx_bd_ring[i]));-}elseif(ugeth->ug_info->uf_info.bd_mem_part==-MEM_PART_MURAM){-out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,-(u32)immrbar_virt_to_phys(ugeth->-p_rx_bd_ring[i]));-}+out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,+iopa((unsignedlong)ugeth->p_rx_bd_ring[i]));/* rest of fields handled by QE */}
@@ -3331,15 +3321,8 @@ static int ucc_geth_startup(struct
ucc_geth_private *ugeth)
/* Setup the table */
/* Assume BD rings are already established */
for (i = 0; i < ug_info->numQueuesRx; i++) {
- if (ugeth->ug_info->uf_info.bd_mem_part == MEM_PART_SYSTEM) {
- out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
- (u32) virt_to_phys(ugeth->p_rx_bd_ring[i]));
- } else if (ugeth->ug_info->uf_info.bd_mem_part ==
- MEM_PART_MURAM) {
- out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
- (u32) immrbar_virt_to_phys(ugeth->
- p_rx_bd_ring[i]));
- }
+ out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
+ iopa((unsigned long)ugeth->p_rx_bd_ring[i]));
/* rest of fields handled by QE */
}
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jeff Garzik <hidden> Date: 2007-02-07 00:19:06
Li Yang wrote:
Get rid of private immrbar_virt_to_phys() routine and
use generic iopa().
Signed-off-by: Li Yang <redacted>
---
drivers/net/ucc_geth.c | 29 ++++++-----------------------
1 files changed, 6 insertions(+), 23 deletions(-)
From: Timur Tabi <hidden> Date: 2007-02-07 16:43:57
Kumar Gala wrote:
On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
quoted
Get rid of private immrbar_virt_to_phys() routine and
use generic iopa().
Nack. iopa() isn't that generic, shouldn't we really be using the dma
mapping API here?
I'm having a hard time understanding what's wrong with iopa(). Is it because
it's a 32-bit only function? The memory has already been mapped with ioremap(),
so why would we want to map it again?
--
Timur Tabi
Linux Kernel Developer @ Freescale
From: Kumar Gala <hidden> Date: 2007-02-07 16:50:03
On Feb 7, 2007, at 3:34 AM, Li Yang-r58472 wrote:
quoted
On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
quoted
Get rid of private immrbar_virt_to_phys() routine and
use generic iopa().
Nack. iopa() isn't that generic, shouldn't we really be using the dma
mapping API here?
Do you mean the dma_map_single()? dma_map_single can map memory space
correctly, but I don't think it can handle ioremap-ed space.
What are you mapping? It looks like buffer descriptor rings. Are
they being kept in QE sram? If so that I think you can treat it as
memory. (I believe the SRAM behaves like memory, is cache-coherent,
etc).
- k
From: Kumar Gala <hidden> Date: 2007-02-07 16:50:57
On Feb 7, 2007, at 10:43 AM, Timur Tabi wrote:
Kumar Gala wrote:
quoted
On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
quoted
Get rid of private immrbar_virt_to_phys() routine and
use generic iopa().
Nack. iopa() isn't that generic, shouldn't we really be using the
dma mapping API here?
I'm having a hard time understanding what's wrong with iopa(). Is
it because it's a 32-bit only function? The memory has already
been mapped with ioremap(), so why would we want to map it again?
If its been mapped with ioremap() you know the physical address
already so why do you need iopa(). The problem I have is iopa() is
ppc specific and I can envision a day when someone wants to run Linux
on a StarCore + QE, so having the drivers using ppc specific APIs is
bad.
- k
From: Timur Tabi <hidden> Date: 2007-02-07 17:03:06
Kumar Gala wrote:
If its been mapped with ioremap() you know the physical address already
so why do you need iopa().
That's what the original function immrbar_virt_to_phys() does. We're trying to
get rid of it, because we thought is redundant with iopa().
static inline unsigned long immrbar_virt_to_phys(volatile void * address)
{
if ( ((u32)address >= (u32)qe_immr) &&
((u32)address < ((u32)qe_immr + QE_IMMAP_SIZE)) )
return (unsigned long)(address - (u32)qe_immr +
(u32)get_qe_base());
return (unsigned long)virt_to_phys(address);
}
get_qe_base() does a search of the OF tree the first time it's called.
Here's the code that calls immrbar_virt_to_phys():
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
(u32) immrbar_virt_to_phys(ugeth->
p_tx_bd_ring[i]));
Would it be better to replace this code with something like this:
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
get_qe_base() + ((void *) ugeth->p_tx_bd_ring[i] - (void *) qe_immr));
--
Timur Tabi
Linux Kernel Developer @ Freescale
From: Li Yang-r58472 <hidden> Date: 2007-02-08 05:51:23
-----Original Message-----
From: Timur Tabi [mailto:timur@freescale.com]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys
to generic
iopa
=20
Kumar Gala wrote:
=20
quoted
If its been mapped with ioremap() you know the physical address
already
quoted
so why do you need iopa().
=20
That's what the original function immrbar_virt_to_phys() does. We're
trying to
get rid of it, because we thought is redundant with iopa().
=20
static inline unsigned long immrbar_virt_to_phys(volatile void *
return (unsigned long)(address - (u32)qe_immr +
(u32)get_qe_base());
return (unsigned long)virt_to_phys(address);
}
=20
get_qe_base() does a search of the OF tree the first time it's called.
=20
Here's the code that calls immrbar_virt_to_phys():
=20
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
(u32) immrbar_virt_to_phys(ugeth->
p_tx_bd_ring[i]));
=20
=20
Would it be better to replace this code with something like this:
=20
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
get_qe_base() + ((void *) ugeth->p_tx_bd_ring[i] - (void *)
qe_immr));
No, we don't know if the BD ring is in MURAM or main memory as it is
configurable. iopa() is best choice to handle both case, IMHO.
- Leo
From: Kumar Gala <hidden> Date: 2007-02-08 06:14:30
On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:
quoted
-----Original Message-----
From: Timur Tabi [mailto:timur@freescale.com]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private
immrbar_virt_to_phys
to generic
quoted
iopa
Kumar Gala wrote:
quoted
If its been mapped with ioremap() you know the physical address
already
quoted
quoted
so why do you need iopa().
That's what the original function immrbar_virt_to_phys() does. We're
trying to
quoted
get rid of it, because we thought is redundant with iopa().
static inline unsigned long immrbar_virt_to_phys(volatile void *
return (unsigned long)(address - (u32)qe_immr +
(u32)get_qe_base());
return (unsigned long)virt_to_phys(address);
}
get_qe_base() does a search of the OF tree the first time it's
called.
Here's the code that calls immrbar_virt_to_phys():
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
(u32) immrbar_virt_to_phys(ugeth->
p_tx_bd_ring[i]));
Would it be better to replace this code with something like this:
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
get_qe_base() + ((void *) ugeth->p_tx_bd_ring[i] - (void *)
qe_immr));
No, we don't know if the BD ring is in MURAM or main memory as it is
configurable. iopa() is best choice to handle both case, IMHO.
Does MURAM behave differently than normal memory?
- k
From: Li Yang-r58472 <hidden> Date: 2007-02-08 06:46:54
-----Original Message-----
From: Kumar Gala [mailto:galak@kernel.crashing.org]
Sent: Thursday, February 08, 2007 1:58 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys
to generic
iopa
=20
=20
On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:
=20
quoted
quoted
-----Original Message-----
From: Timur Tabi [mailto:timur@freescale.com]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private
immrbar_virt_to_phys
to generic
quoted
iopa
Kumar Gala wrote:
quoted
If its been mapped with ioremap() you know the physical address
already
quoted
quoted
so why do you need iopa().
That's what the original function immrbar_virt_to_phys() does.
We're
quoted
trying to
quoted
get rid of it, because we thought is redundant with iopa().
static inline unsigned long immrbar_virt_to_phys(volatile void *
return (unsigned long)(address - (u32)qe_immr +
(u32)get_qe_base());
return (unsigned long)virt_to_phys(address);
}
get_qe_base() does a search of the OF tree the first time it's
called.
Here's the code that calls immrbar_virt_to_phys():
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
(u32) immrbar_virt_to_phys(ugeth->
p_tx_bd_ring[i]));
Would it be better to replace this code with something like this:
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
get_qe_base() + ((void *) ugeth->p_tx_bd_ring[i] - (void *)
qe_immr));
No, we don't know if the BD ring is in MURAM or main memory as it is
configurable. iopa() is best choice to handle both case, IMHO.
=20
Does MURAM behave differently than normal memory?
MURAM is a mmio region so it don't share the characteristic of main
memory that phy_addr =3D virt_addr - PAGE_OFFSET. While they can both =
be
mapped through page table using iopa().
- Leo
From: Kumar Gala <hidden> Date: 2007-02-08 06:54:21
On Feb 8, 2007, at 12:48 AM, Li Yang-r58472 wrote:
quoted
-----Original Message-----
From: Kumar Gala [mailto:galak@kernel.crashing.org]
Sent: Thursday, February 08, 2007 1:58 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; netdev@vger.kernel.org; linuxppc-
dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private
immrbar_virt_to_phys
to generic
quoted
iopa
On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:
quoted
quoted
-----Original Message-----
From: Timur Tabi [mailto:timur@freescale.com]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private
immrbar_virt_to_phys
to generic
quoted
iopa
Kumar Gala wrote:
quoted
If its been mapped with ioremap() you know the physical address
already
quoted
quoted
so why do you need iopa().
That's what the original function immrbar_virt_to_phys() does.
We're
quoted
quoted
trying to
quoted
get rid of it, because we thought is redundant with iopa().
static inline unsigned long immrbar_virt_to_phys(volatile void *
return (unsigned long)(address - (u32)qe_immr +
(u32)get_qe_base());
return (unsigned long)virt_to_phys(address);
}
get_qe_base() does a search of the OF tree the first time it's
called.
Here's the code that calls immrbar_virt_to_phys():
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
(u32) immrbar_virt_to_phys(ugeth->
p_tx_bd_ring[i]));
Would it be better to replace this code with something like this:
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
get_qe_base() + ((void *) ugeth->p_tx_bd_ring[i] - (void *)
qe_immr));
No, we don't know if the BD ring is in MURAM or main memory as it is
configurable. iopa() is best choice to handle both case, IMHO.
Does MURAM behave differently than normal memory?
MURAM is a mmio region so it don't share the characteristic of main
memory that phy_addr = virt_addr - PAGE_OFFSET. While they can
both be
mapped through page table using iopa().
Right, so when do you know if you'll be using MURAM or normal
memory? Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal memory.
- k
return (unsigned long)(address - (u32)qe_immr +
(u32)get_qe_base());
return (unsigned long)virt_to_phys(address);
}
get_qe_base() does a search of the OF tree the first time it's
called.
Here's the code that calls immrbar_virt_to_phys():
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
(u32) immrbar_virt_to_phys(ugeth->
p_tx_bd_ring[i]));
Would it be better to replace this code with something like this:
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
get_qe_base() + ((void *) ugeth->p_tx_bd_ring[i] - (void *)
qe_immr));
No, we don't know if the BD ring is in MURAM or main memory as it
is
quoted
quoted
quoted
configurable. iopa() is best choice to handle both case, IMHO.
Does MURAM behave differently than normal memory?
MURAM is a mmio region so it don't share the characteristic of main
memory that phy_addr =3D virt_addr - PAGE_OFFSET. While they can
both be
mapped through page table using iopa().
=20
Right, so when do you know if you'll be using MURAM or normal
memory? Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal memory.
Yes, that can be a way. But as the virt to phy mapping is only used
once, it's nothing bad to do it this way.
- Leo
From: Kumar Gala <hidden> Date: 2007-02-08 07:16:21
On Feb 8, 2007, at 1:06 AM, Li Yang-r58472 wrote:
quoted
quoted
MURAM is a mmio region so it don't share the characteristic of main
memory that phy_addr = virt_addr - PAGE_OFFSET. While they can
both be
mapped through page table using iopa().
Right, so when do you know if you'll be using MURAM or normal
memory? Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal memory.
Yes, that can be a way. But as the virt to phy mapping is only used
once, it's nothing bad to do it this way.
The problem as I stated before with using iopa() is that its not
supported across platforms.
- k
From: Li Yang-r58472 <hidden> Date: 2007-02-08 07:35:11
-----Original Message-----
From: Kumar Gala [mailto:galak@kernel.crashing.org]
Sent: Thursday, February 08, 2007 3:16 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys
to generic
iopa
=20
=20
On Feb 8, 2007, at 1:06 AM, Li Yang-r58472 wrote:
=20
quoted
quoted
quoted
MURAM is a mmio region so it don't share the characteristic of
main
quoted
quoted
quoted
memory that phy_addr =3D virt_addr - PAGE_OFFSET. While they can
both be
mapped through page table using iopa().
Right, so when do you know if you'll be using MURAM or normal
memory? Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal
memory.
quoted
Yes, that can be a way. But as the virt to phy mapping is only used
once, it's nothing bad to do it this way.
=20
The problem as I stated before with using iopa() is that its not
supported across platforms.
Yes, it is only for PPC32. But we don't have another API to do it. How
about make it more generic to add PPC64 version?
- Leo
From: Kumar Gala <hidden> Date: 2007-02-08 07:43:50
On Feb 8, 2007, at 1:36 AM, Li Yang-r58472 wrote:
quoted
-----Original Message-----
From: Kumar Gala [mailto:galak@kernel.crashing.org]
Sent: Thursday, February 08, 2007 3:16 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; netdev@vger.kernel.org; linuxppc-
dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private
immrbar_virt_to_phys
to generic
quoted
iopa
On Feb 8, 2007, at 1:06 AM, Li Yang-r58472 wrote:
quoted
quoted
quoted
MURAM is a mmio region so it don't share the characteristic of
main
quoted
quoted
quoted
quoted
memory that phy_addr = virt_addr - PAGE_OFFSET. While they can
both be
mapped through page table using iopa().
Right, so when do you know if you'll be using MURAM or normal
memory? Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal
memory.
quoted
quoted
Yes, that can be a way. But as the virt to phy mapping is only used
once, it's nothing bad to do it this way.
The problem as I stated before with using iopa() is that its not
supported across platforms.
Yes, it is only for PPC32. But we don't have another API to do
it. How
about make it more generic to add PPC64 version?
Why do you need another API to do this, you already have the
information you want, its just a matter of you keeping track of it.
- k
From: Li Yang-r58472 <hidden> Date: 2007-02-08 09:21:04
quoted
quoted
quoted
quoted
quoted
MURAM is a mmio region so it don't share the characteristic of
main
quoted
quoted
quoted
quoted
memory that phy_addr =3D virt_addr - PAGE_OFFSET. While they =
can
quoted
quoted
quoted
quoted
quoted
both be
mapped through page table using iopa().
Right, so when do you know if you'll be using MURAM or normal
memory? Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal
memory.
quoted
quoted
Yes, that can be a way. But as the virt to phy mapping is only
used
quoted
quoted
quoted
once, it's nothing bad to do it this way.
The problem as I stated before with using iopa() is that its not
supported across platforms.
Yes, it is only for PPC32. But we don't have another API to do
it. How
about make it more generic to add PPC64 version?
=20
Why do you need another API to do this, you already have the
information you want, its just a matter of you keeping track of it.
Do you think we should remove iopa?
The information is often not so obvious. The most case of using iopa is
that we allocate memory from MURAM using rheap, and then get the
physical address of it. We can calculate phy_addr by using: virt_addr
- virt_muram_base + phy_muram_base, but it is not as straightforward as
just using the page table, let alone we need to keep track of
virt_muram_base and phy_muram_base.
- Leo
From: Timur Tabi <hidden> Date: 2007-02-08 13:28:00
Li Yang-r58472 wrote:
No, we don't know if the BD ring is in MURAM or main memory as it is
configurable. iopa() is best choice to handle both case, IMHO.
The above code would only be used if the BD is in MURAM. The "if
bd_mem_part == MEM_PART_MURAM" would stay.
If the BD ring can be in main memory, then I don't think we should be
using a function called "iopa" to get its physical address. It's
conceivable that one day, iopa() will only work on memory that's been
ioremap'ed.
From: Timur Tabi <hidden> Date: 2007-02-08 13:35:13
Kumar Gala wrote:
Right, so when do you know if you'll be using MURAM or normal memory?
Why not just keep around a token that is the physical address at the
point you make the decision of MURAM vs normal memory.
That's what the original code did, kinda. It called virt_to_phys() if
it is main memory, and it called immrbar_virt_to_phys() if it is MURAM.
immrbar_virt_to_phys() did pointer math to extract the physical address.
I knew that Leo's patch was calling iopa() on main memory, but since it
worked, I didn't think much about it. But it does make some sense that
since we obtain the physical address when we allocate the BD ring, we
should store that address and use it.
That's what most drivers do, anyway, and so we should do the same thing.
So instead of p_tx_bd_ring[i], we would have p_tx_bd_ring[i].virtual
and p_tx_bd_ring[i].physical.
Besides, iopa() is slower, because it walks page tables.
From: Timur Tabi <hidden> Date: 2007-02-08 13:42:08
Li Yang-r58472 wrote:
Do you think we should remove iopa?
It's not used very much any more. If you do a grep of the code, I think
only some legacy drivers use it.
As long as ioremap'd memory is both physically and virtually contiguous,
I think iopa() is unnecessary.
The information is often not so obvious. The most case of using iopa is
that we allocate memory from MURAM using rheap, and then get the
physical address of it. We can calculate phy_addr by using: virt_addr
- virt_muram_base + phy_muram_base, but it is not as straightforward as
just using the page table, let alone we need to keep track of
virt_muram_base and phy_muram_base.
Walking the page table is slower than using pointer math, so you want to
avoid iopa() if you can.
I think in general, the driver that calls ioremap() should store the
physical address along with the virtual address of the base pointer.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-02-08 21:26:12
Do you think we should remove iopa?
I'm really not fan of it. We might keep it for now to not break
everything that uses it, but I'd rather avoid new things using it
directly.
The information is often not so obvious. The most case of using iopa is
that we allocate memory from MURAM using rheap, and then get the
physical address of it.
Then provide specific MURAM functions that return a dma_addr_t in
addition to the virtual address and have your driver keep track of that.
It's more efficient than walking the page tables all the time anyway.
Remember that if you have CONFIG_HIGHMEM, you can potentially have to
create TLB entries for accessing the PTEs used by iopa to resolve the
address. It's really not efficient.
We can calculate phy_addr by using: virt_addr
- virt_muram_base + phy_muram_base, but it is not as straightforward as
just using the page table, let alone we need to keep track of
virt_muram_base and phy_muram_base.
Using the page table is much less efficient. As I said, I'd rather see
you having a set of alloc/free functions for your MURAM that return the
dma_addr_t as well as the virtual address (like dma_alloc_coherent does)
and have your drivers using these keep track of the dma_addr_t.
Ben.
From: Dan Malek <hidden> Date: 2007-02-09 17:13:37
On Feb 8, 2007, at 4:22 AM, Li Yang-r58472 wrote:
Do you think we should remove iopa?
I'm surprised it's still around :-) I had to fight hard many
years ago to implement it, since I found it very useful
for complex mappings we needed to manage uncached
spaces for the MPC8xx.
The information is often not so obvious. ...
I agree with you. Linux has the weirdest set
(or lack of) VM APIs I've ever seen, to the point
of preventing useful mapping or tracking
attributes. Good luck with your argument to
use useful functions, you have at least
one person on your side :-)
-- Dan
From: Dan Malek <hidden> Date: 2007-02-09 17:20:11
On Feb 8, 2007, at 4:26 PM, Benjamin Herrenschmidt wrote:
I'm really not fan of it.
That's because I think it's a terribly useful function
and the first step toward some orthogonal VM APIs. :-)
No one liked my idea of virtual interrupt mapping
years ago when the 8xx showed up with the
CPM and I needed a way to handle some
non-PC interrupt controller.......
Thanks.
-- Dan
From: Dan Malek <hidden> Date: 2007-02-09 17:23:05
On Feb 8, 2007, at 8:35 AM, Timur Tabi wrote:
That's what the original code did, kinda. It called virt_to_phys() if
it is main memory, and it called immrbar_virt_to_phys() if it is
MURAM.
immrbar_virt_to_phys() did pointer math to extract the physical
address.
You've got to be kidding. You created yet another function
to determine the physical address from a virtual one! LOL!
Plus, you need logic in the code to first determine which
one to call? Everyone is OK with this?
-- Dan
From: Timur Tabi <hidden> Date: 2007-02-09 17:26:41
Dan Malek wrote:
On Feb 8, 2007, at 8:35 AM, Timur Tabi wrote:
quoted
That's what the original code did, kinda. It called virt_to_phys() if
it is main memory, and it called immrbar_virt_to_phys() if it is MURAM.
immrbar_virt_to_phys() did pointer math to extract the physical
address.
You've got to be kidding. You created yet another function
to determine the physical address from a virtual one! LOL!
Plus, you need logic in the code to first determine which
one to call? Everyone is OK with this?
The whole point behind the patch that we posted a couple days ago is to get rid
of this function. The patch replaced it with a call to iopa(), but Kumar nack'd
that.
--
Timur Tabi
Linux Kernel Developer @ Freescale
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-02-09 22:06:32
On Fri, 2007-02-09 at 12:13 -0500, Dan Malek wrote:
On Feb 8, 2007, at 4:26 PM, Benjamin Herrenschmidt wrote:
quoted
I'm really not fan of it.
That's because I think it's a terribly useful function
and the first step toward some orthogonal VM APIs. :-)
No, I think it's very inefficient to walk the page tables, even worse if
you have PTEs in HIGHMEM. I think it's better to keep track the physical
address from alloc time, something similar than dma_alloc_coherent but
for use with that MURAM thingy.
Cheers,
Ben.
From: Kumar Gala <hidden> Date: 2007-02-09 22:47:51
On Feb 9, 2007, at 4:06 PM, Benjamin Herrenschmidt wrote:
On Fri, 2007-02-09 at 12:13 -0500, Dan Malek wrote:
quoted
On Feb 8, 2007, at 4:26 PM, Benjamin Herrenschmidt wrote:
quoted
I'm really not fan of it.
That's because I think it's a terribly useful function
and the first step toward some orthogonal VM APIs. :-)
No, I think it's very inefficient to walk the page tables, even
worse if
you have PTEs in HIGHMEM. I think it's better to keep track the
physical
address from alloc time, something similar than dma_alloc_coherent but
for use with that MURAM thingy.
My feeling is to leave the code alone until we have the dma mapping
api setup to handle MURAM. If we can't do alloc/map/free with the
same mechanism I dont see the value in just doing 'map'. It just
makes the code more confusing w/o any big gain.
Truthfully the normal system memory size should be cleaned up to use
the dma mapping API instead of straight allocations and mapping like
its using now.
- k
From: Timur Tabi <hidden> Date: 2007-02-09 22:51:09
Kumar Gala wrote:
My feeling is to leave the code alone until we have the dma mapping api
setup to handle MURAM. If we can't do alloc/map/free with the same
mechanism I dont see the value in just doing 'map'. It just makes the
code more confusing w/o any big gain.
So you don't want to see a patch where we store the physical address in our own
data structures when the memory is mapped (or allocated if it's regular RAM)?
--
Timur Tabi
Linux Kernel Developer @ Freescale
From: Kumar Gala <hidden> Date: 2007-02-09 23:07:40
On Feb 9, 2007, at 4:51 PM, Timur Tabi wrote:
Kumar Gala wrote:
quoted
My feeling is to leave the code alone until we have the dma
mapping api setup to handle MURAM. If we can't do alloc/map/free
with the same mechanism I dont see the value in just doing 'map'.
It just makes the code more confusing w/o any big gain.
So you don't want to see a patch where we store the physical
address in our own data structures when the memory is mapped (or
allocated if it's regular RAM)?
It doesn't seem like it's that big of a change. I'd rather the code
clearly do different things for MURAM vs system memory for ALL accesses.
- k
From: Kumar Gala <hidden> Date: 2007-02-09 23:23:29
On Feb 9, 2007, at 5:10 PM, Timur Tabi wrote:
Kumar Gala wrote:
quoted
It doesn't seem like it's that big of a change. I'd rather the
code clearly do different things for MURAM vs system memory for
ALL accesses.
Why? MURAM acts like RAM, it just needs to be mapped first.
It does from a coherence point of view, but not from an allocation
mechanism.
We should fix it so the driver just has to do dma_alloc_coherent,
dma_free_coherent, and dma_map_*/dma_unmap_*
- k
From: Timur Tabi <hidden> Date: 2007-02-09 23:33:14
Kumar Gala wrote:
We should fix it so the driver just has to do dma_alloc_coherent,
dma_free_coherent, and dma_map_*/dma_unmap_*
Ok, I'm getting confused now. The driver currently uses ioremap(). You're
saying that it should use the dma_xxx functions instead, but not until those
functions support MURAM? When that happens, won't that make ioremap() obsolete?
--
Timur Tabi
Linux Kernel Developer @ Freescale
From: Kumar Gala <hidden> Date: 2007-02-09 23:44:43
On Feb 9, 2007, at 5:33 PM, Timur Tabi wrote:
Kumar Gala wrote:
quoted
We should fix it so the driver just has to do dma_alloc_coherent,
dma_free_coherent, and dma_map_*/dma_unmap_*
Ok, I'm getting confused now. The driver currently uses ioremap
(). You're saying that it should use the dma_xxx functions
instead, but not until those functions support MURAM? When that
happens, won't that make ioremap() obsolete?
ioremap() is for IO space not normal memory or things that behave
like normal memory (MURAM, on chip SRAM, etc). ioremap will always
be needed to ensure proper non-cachable/guarded access to registers
on devices, etc... The problem is that we don't have a mechanism
between kmalloc/vmalloc (for system memory) and ioremap (for devices)
to handle other memories in the system. So we end up with a
specialized allocator (rheap + muram wrappers) to deal with it.
So, I'd rather mapping also be specialized for MURAM. I don't see
any benefit in making mapping behave the same while not addressing
the other functionality (allocation, deallocation, etc.)
- k
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-02-10 02:37:14
On Fri, 2007-02-09 at 12:08 -0500, Dan Malek wrote:
On Feb 8, 2007, at 8:41 AM, Timur Tabi wrote:
quoted
Walking the page table is slower than using pointer math, so you
want to
avoid iopa() if you can.
Big deal. With all of the other kernel bloat
going on you want to argue about a couple
of memory accesses? :-)
We are fairly careful about not bloating fast path in general. It's more
than a couple of memory accesses, especially with PTEs in highmem where
it involves kmap.
Ben.
From: Dan Malek <hidden> Date: 2007-02-10 18:04:05
On Feb 9, 2007, at 9:37 PM, Benjamin Herrenschmidt wrote:
We are fairly careful about not bloating fast path in general.
This isn't any fast path code, and the way the
exception handlers are growing it doesn't
seem to be a concern anyway.
.... It's more
than a couple of memory accesses, especially with PTEs in highmem
where
it involves kmap.
It is only a couple of memory accesses, even
less code than the TLB exception handlers.
Using highmem has a price any time it's
configured into a system, it's not unique in
this case. In fact, in this case highmem
shouldn't be a concern any different than
the TLB exceptions.
I just don't understand how such a trivial
and useful function that does exactly what
we need in a very clean way generates so
much polarized discussion. I'm beginning
to think it's just personal, since the only
argument against it is "I don't like it" when
the alternatives are just hacks at best that
still need to be "fixed up someday." :-)
The Linux VM implementation just sucks.
The majority of systems running this software
aren't servers and desktop PCs, it's embedded
SOCs with application specific peripherals.
They have attributes and are mapped in ways
that don't fit the "memory at 0" or "IO" model.
We have to find solutions to this, together.
Thanks.
-- Dan
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-02-10 21:41:10
On Sat, 2007-02-10 at 13:04 -0500, Dan Malek wrote:
On Feb 9, 2007, at 9:37 PM, Benjamin Herrenschmidt wrote:
quoted
We are fairly careful about not bloating fast path in general.
This isn't any fast path code, and the way the
exception handlers are growing it doesn't
seem to be a concern anyway.
The 64 bits exception handlers are growing a bit due to some optional
process time accounting stuff though I'm not too happy with the growth.
Appart from that, they aren't growing much and we are working hard to
keep them in check. Any specific example of that "growth" you are
talking about ?
It is only a couple of memory accesses, even
less code than the TLB exception handlers.
It's more specifically two loads on 2 level page tables we have on 32
bits though on 64 bits, page tables are 3 or 4 levels (64k or 4k page
size) and thus it's 3 or 4 loads, which can be very significant if those
are cache misses. So yes, while it's quite cheap on embedded 32 bits
CPUs that don't use HIGHMEM, it's not that good on other things, and
thus might not be the best approach.
I still think that it's preferable to simply obtain the physical address
along with the virtual one when allocating/creating an object (and thus
have the allocator for those object types, like rheap for MURAM, return
it, the same way the coherent dma allocator does).
There's also another issue with iopa that isn't obvious at first look:
It's racy vs. page tables being disposed on SMP machines (and possibly
with preempt). We handle the race against hash misses on hash-based CPUs
using the hash lock in pte_free but there is nothing in iopa to deal
with that. I don't think this is a problem with kernel mappings though,
but one should be careful.
Using highmem has a price any time it's
configured into a system, it's not unique in
this case. In fact, in this case highmem
shouldn't be a concern any different than
the TLB exceptions.
True, but it's more expensive than keeping track of the physical address
from allocation.
I just don't understand how such a trivial
and useful function that does exactly what
we need in a very clean way generates so
much polarized discussion. I'm beginning
to think it's just personal, since the only
argument against it is "I don't like it" when
the alternatives are just hacks at best that
still need to be "fixed up someday." :-)
The alternatives aren't just hack. The alternative that we recommend and
which is the way to do things in linux is to keep track of the physical
address or the struct page's at allocation time.
The Linux VM implementation just sucks.
This has very little to do with linux VM. Most if not all the uses of
iopa are purely for kernel mappings which are not handled by the core VM
in most areas. There are design choices in the linux kernel memory
management that you might not agree with, though just saying "just
sucks" is neither useful nor constructive. If you think that some
aspects of linux kernel memory handling should be done differently, you
are much welcome to propose alternatives (with patches) though keep in
mind that the way things are done now is actually very efficient from a
performance standpoint and well adapted to the need of most
architectures.
The majority of systems running this software
aren't servers and desktop PCs, it's embedded
SOCs with application specific peripherals.
They have attributes and are mapped in ways
that don't fit the "memory at 0" or "IO" model.
We have to find solutions to this, together.
Yes, and finding solutions involves more than just saying "sucks" :-)
Now, I don't completely agree with you that there are "fundamental"
limitations in the way memory is managed. First let's get off the
subject of "VM" as this is commonly used to represent the memory
management of user processes which isn't what we are talking about (and
doesnt' suffer from any of the "limitations" you mention anyway.
What we are talking about here is the management of the kernel memory
address space.
Some of the limitations you mention above like "memory at 0" are more
limitations of some architecture ports like x86 or powerpc and mostly
because on those CPUs, it make little sense to do differently due to the
way exceptions work, and even then, they aren't very hard to lift. (See
for example kdump which runs the ppc64 kernel in a reserved area of
memory at 32MB or so).
Some of those "limitations" result from design choices that provide best
performances for 99% of the usage scenario and though they might not be
suitable for the most exotic hardware, that doesn't make them bad
choices in the first place.
I do agree however that in some areas we can do better. For example, the
vmalloc/ioremap allocator should definitely be modified to be able to
allocate in different areas/pools. That's something we could use on
ppc64 as well to replace our imalloc crap and possibly on embedded to
replace rheap.
I'm not sure what you mean by "IO model" here.
Ben.
From: Dan Malek <hidden> Date: 2007-02-10 22:27:31
On Feb 10, 2007, at 4:40 PM, Benjamin Herrenschmidt wrote:
I still think that it's preferable to simply obtain the physical
address
along with the virtual one when allocating/creating an object (and
thus
have the allocator for those object types, like rheap for MURAM,
return
it, the same way the coherent dma allocator does).
I'd agree, but we don't have any functions to deal with this.
It's been an issue ever since I ported the first 8xx many
years ago. The arguments range from "it's too specialized"
or "fit it under something else" that isn't appropriate, or
it's yet another resource allocator with it's own set of
management APIs (which I think is silly, but seems to be
the way of Linux). Worse, we just hack something to
"fix another day", which never happens :-)
There's also another issue with iopa that isn't obvious at first look:
It's racy vs. page tables being disposed on SMP machines
Considering it was done before any SMP support,
and was ignored when support was added, that's
not really an argument to not use it but rather to fix it.
.... though just saying "just
sucks" is neither useful nor constructive.
About in line with "I don't like it" :-)
..... If you think that some
aspects of linux kernel memory handling should be done differently,
you
are much welcome to propose alternatives
They have always been ignored, and never
accepted as small changes over time. It seems
to be an all or nothing approach that I just
don't have the time to invest.
Now, I don't completely agree with you that there are "fundamental"
limitations in the way memory is managed.
Sure there are, but it's not for discussion here.
... First let's get off the
subject of "VM"
It's all about VM and the implicit connection Linux
makes between physical memory and virtual
addresses that makes this a problem. There are
special allocators for the different types of "memory",
different ways of setting/finding any attributes (if
you can at all), and the pre-knowledge you need
about the address spaces so you can call proper
support functions. There is no separation of
VM objects and what backs them, orthogonal
operations (to do something as simple as get
a physical address behind a virtual one regardless
of the backing store), the ridiculous need for something
like highmem and the yet another way to manage
that, the inability to have a separate kernel and
user VM space if I choose, the minimal support and
horrible hacks for 32-bit systems with greater than
32-bit physical addresses, the list goes on and on.....
.... as this is commonly used to represent the memory
management of user processes which isn't what we are talking about
(and
doesnt' suffer from any of the "limitations" you mention anyway.
What we are talking about here is the management of the kernel memory
address space.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-02-10 22:43:21
I'd agree, but we don't have any functions to deal with this.
It's been an issue ever since I ported the first 8xx many
years ago. The arguments range from "it's too specialized"
or "fit it under something else" that isn't appropriate, or
it's yet another resource allocator with it's own set of
management APIs (which I think is silly, but seems to be
the way of Linux). Worse, we just hack something to
"fix another day", which never happens :-)
Heh. Well, I do agree that the vmalloc/ioremap allocator should indeed
be improved to handle multiple constrained areas.
In fact, that's even something I intend to look into to remove ppc64's
imalloc :-)
Considering it was done before any SMP support,
and was ignored when support was added, that's
not really an argument to not use it but rather to fix it.
Sure.
quoted
.... though just saying "just
sucks" is neither useful nor constructive.
About in line with "I don't like it" :-)
Yeah well, that's why I'm trying to explain the reasons why I dislike
the approach :-)
quoted
..... If you think that some
aspects of linux kernel memory handling should be done differently,
you
are much welcome to propose alternatives
They have always been ignored, and never
accepted as small changes over time. It seems
to be an all or nothing approach that I just
don't have the time to invest.
I suppose I must have missed those attempts then. As I said, I do agree
that some aspects of the kernel memory address space handling can be
improved to handle more of those cases and I'd be happy to discuss
ideas/proposals/patches in that direction.
quoted
Now, I don't completely agree with you that there are "fundamental"
limitations in the way memory is managed.
Sure there are, but it's not for discussion here.
quoted
... First let's get off the
subject of "VM"
It's all about VM and the implicit connection Linux
makes between physical memory and virtual
addresses that makes this a problem. There are
special allocators for the different types of "memory",
different ways of setting/finding any attributes (if
you can at all), and the pre-knowledge you need
about the address spaces so you can call proper
support functions. There is no separation of
VM objects and what backs them, orthogonal
operations (to do something as simple as get
a physical address behind a virtual one regardless
of the backing store), the ridiculous need for something
like highmem and the yet another way to manage
that, the inability to have a separate kernel and
user VM space if I choose, the minimal support and
horrible hacks for 32-bit systems with greater than
32-bit physical addresses, the list goes on and on.....
Highmem and >32 bits physical addresses are two different things.
Highmem is a consequence of a design choice to improve performances on
most 32 bits CPUs which was done at a time where we didn't have
routinely gigabytes of RAM on those machines. The 3G/1G split allows
direct access to user memory from the kernel at no cost, and thus while
causing limitations like the need for highmem if you want more than 1G
(or rather 768Mb on ppc32) it does improve performances overall by a
significant factor.
However, it's not a fundamental limitation of the kernel. A fully
separate kernel address space is what the 4G/4G does on x86 and it can
be implemented (if not already) for pretty much free on some freescale
book-e processors with their load/store insns that take the address
space as an argument without the overhead of constantly mapping/umapping
bits of process space in kernel space.
It would be possible to implement 4G/4G on other CPUs using something
akin to highmem's kmap or a specialized TLB entry or BAT (depending on
the processor) that is used as a "window" to user space on other 32 bits
CPUs. The reason it's not in the kernel yet is that nobody actually did
it. I don't think we would reject patches implementing that (well, at
least not on the principle of the approach, possibly if they are coded
like crap :-)
Now, I'm not sure I see your problem with >32 bits address space. The
main question is wether this is used for memory or IO though.
When used for memory, the approach so far is what is done with PAE on
x86 via a highmem-type mecanism iirc, though I'm not too familiar with
it (heh, there's no other choice really here). When used for IO, then
it's very simple nowadays with 64 bits resources and 64 bits capable
ioremap.
Basically, everything out of the linear mapping is mapped via those
"objects" you are talking about managed by the vmalloc/ioremap core. As
I said, it could/should be improved to better handle different
areas/pools (especially on 64 bits implentation) but it provides a
pretty good base implementation for having virtual memory "objects" and
doesn't care about the physical mapping of those, you can pass
attributes (though we call them "protection bits" in linux, they are the
same).
If you tell us more precisely what you think could be improved in and in
what direction, I'd be happy to discuss the details.
Cheers,
Ben.