[PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang]

Get rid of private immrbar_virt_to_phys() routine and
use generic iopa().

Signed-off-by: Li Yang <leoli@freescale.com>
---
 drivers/net/ucc_geth.c |   29 ++++++-----------------------
 1 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 7e4b23c..722a89f 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -3065,21 +3065,11 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 		endOfRing =
 		    ugeth->p_tx_bd_ring[i] + (ug_info->bdRingLenTx[i] -
 					      1) * sizeof(struct qe_bd);
-		if (ugeth->ug_info->uf_info.bd_mem_part == MEM_PART_SYSTEM) {
-			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
-				 (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
-			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
+		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
+				iopa((unsigned long)ugeth->p_tx_bd_ring[i]));
+		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
 				 last_bd_completed_address,
-				 (u32) virt_to_phys(endOfRing));
-		} else if (ugeth->ug_info->uf_info.bd_mem_part ==
-			   MEM_PART_MURAM) {
-			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
-				 (u32) immrbar_virt_to_phys(ugeth->
-							    p_tx_bd_ring[i]));
-			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
-				 last_bd_completed_address,
-				 (u32) immrbar_virt_to_phys(endOfRing));
-		}
+				iopa((unsigned long)endOfRing));
 	}
 
 	/* schedulerbasepointer */
@@ -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 */
 	}

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 6, 2007, at 5:31 AM, Li Yang wrote:

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

- k

>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> drivers/net/ucc_geth.c |   29 ++++++-----------------------
> 1 files changed, 6 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 7e4b23c..722a89f 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -3065,21 +3065,11 @@ static int ucc_geth_startup(struct  
> ucc_geth_private *ugeth)
> 		endOfRing =
> 		    ugeth->p_tx_bd_ring[i] + (ug_info->bdRingLenTx[i] -
> 					      1) * sizeof(struct qe_bd);
> -		if (ugeth->ug_info->uf_info.bd_mem_part == MEM_PART_SYSTEM) {
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> -				 (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> +		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> +				iopa((unsigned long)ugeth->p_tx_bd_ring[i]));
> +		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> 				 last_bd_completed_address,
> -				 (u32) virt_to_phys(endOfRing));
> -		} else if (ugeth->ug_info->uf_info.bd_mem_part ==
> -			   MEM_PART_MURAM) {
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> -				 (u32) immrbar_virt_to_phys(ugeth->
> -							    p_tx_bd_ring[i]));
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> -				 last_bd_completed_address,
> -				 (u32) immrbar_virt_to_phys(endOfRing));
> -		}
> +				iopa((unsigned long)endOfRing));
> 	}
> 	/* schedulerbasepointer */
> @@ -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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 6, 2007, at 5:31 AM, Li Yang wrote:

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

- k

>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> drivers/net/ucc_geth.c |   29 ++++++-----------------------
> 1 files changed, 6 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 7e4b23c..722a89f 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -3065,21 +3065,11 @@ static int ucc_geth_startup(struct  
> ucc_geth_private *ugeth)
> 		endOfRing =
> 		    ugeth->p_tx_bd_ring[i] + (ug_info->bdRingLenTx[i] -
> 					      1) * sizeof(struct qe_bd);
> -		if (ugeth->ug_info->uf_info.bd_mem_part == MEM_PART_SYSTEM) {
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> -				 (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> +		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> +				iopa((unsigned long)ugeth->p_tx_bd_ring[i]));
> +		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> 				 last_bd_completed_address,
> -				 (u32) virt_to_phys(endOfRing));
> -		} else if (ugeth->ug_info->uf_info.bd_mem_part ==
> -			   MEM_PART_MURAM) {
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> -				 (u32) immrbar_virt_to_phys(ugeth->
> -							    p_tx_bd_ring[i]));
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> -				 last_bd_completed_address,
> -				 (u32) immrbar_virt_to_phys(endOfRing));
> -		}
> +				iopa((unsigned long)endOfRing));
> 	}
> 	/* schedulerbasepointer */
> @@ -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

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

> On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
> 
> > 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.

- Leo

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

> On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
>=20
> > Get rid of private immrbar_virt_to_phys() routine and
> > use generic iopa().
>=20
> 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.

- Leo

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 7, 2007, at 3:34 AM, Li Yang-r58472 wrote:

>> On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
>>
>>> 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 7, 2007, at 3:34 AM, Li Yang-r58472 wrote:

>> On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
>>
>>> 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

Kumar Gala wrote:
> 
> On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
> 
>> 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

Kumar Gala wrote:
> 
> On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
> 
>> 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 7, 2007, at 10:43 AM, Timur Tabi wrote:

> Kumar Gala wrote:
>> On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
>>> 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 7, 2007, at 10:43 AM, Timur Tabi wrote:

> Kumar Gala wrote:
>> On Feb 6, 2007, at 5:31 AM, Li Yang wrote:
>>> 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

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

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

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

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

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

> -----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
> > If its been mapped with ioremap() you know the physical address
already
> > 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 *
address)
> {
> 	if ( ((u32)address >=3D (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);
> }
>=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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:

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

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

> -----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
> 
> 
> On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:
> 
> >> -----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
> >>
> >> 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));
> >
> > 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().

- Leo

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

> -----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
> >> -----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
> >>
> >> 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 >=3D (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));
> >
> > 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 8, 2007, at 12:48 AM, Li Yang-r58472 wrote:

>> -----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
>>
>>
>> On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:
>>
>>>> -----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
>>>>
>>>> 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));
>>>
>>> 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 8, 2007, at 12:48 AM, Li Yang-r58472 wrote:

>> -----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
>>
>>
>> On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:
>>
>>>> -----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
>>>>
>>>> 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));
>>>
>>> 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

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

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

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

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

> >>>>> 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 >=3D (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));
> >>>
> >>> 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 =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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 8, 2007, at 1:06 AM, Li Yang-r58472 wrote:

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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 8, 2007, at 1:06 AM, Li Yang-r58472 wrote:

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

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

> -----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
> 
> 
> On Feb 8, 2007, at 1:06 AM, Li Yang-r58472 wrote:
> 
> >>> 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.

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

RE: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Li Yang-r58472]

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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 8, 2007, at 1:36 AM, Li Yang-r58472 wrote:

>> -----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
>>
>>
>> On Feb 8, 2007, at 1:06 AM, Li Yang-r58472 wrote:
>>
>>>>> 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.
>
> 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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Kumar Gala]

On Feb 8, 2007, at 1:36 AM, Li Yang-r58472 wrote:

>> -----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
>>
>>
>> On Feb 8, 2007, at 1:06 AM, Li Yang-r58472 wrote:
>>
>>>>> 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.
>
> 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

Discussion about iopa()

[Li Yang-r58472]

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

Re: Discussion about iopa()

[Timur Tabi]

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.

Re: Discussion about iopa()

[Dan Malek]

On Feb 8, 2007, at 8:41 AM, Timur Tabi wrote:

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

	-- Dan

Re: Discussion about iopa()

[Benjamin Herrenschmidt]

On Fri, 2007-02-09 at 12:08 -0500, Dan Malek wrote:
> On Feb 8, 2007, at 8:41 AM, Timur Tabi wrote:
> 
> > 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.

Re: Discussion about iopa()

[Dan Malek]

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

Re: Discussion about iopa()

[Benjamin Herrenschmidt]

On Sat, 2007-02-10 at 13:04 -0500, Dan Malek wrote:
> 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.

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.

Re: Discussion about iopa()

[Dan Malek]

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.

I see.....  I now understand.....

Thanks.

	-- Dan

Re: Discussion about iopa()

[Benjamin Herrenschmidt]

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

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

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

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

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.

Re: Discussion about iopa()

[Benjamin Herrenschmidt]

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

Re: Discussion about iopa()

[Dan Malek]

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

Re: Discussion about iopa()

[Benjamin Herrenschmidt]

On Fri, 2007-02-09 at 12:13 -0500, Dan Malek wrote:
> 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, 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.
 

Re: Discussion about iopa()

[Kumar Gala]

On Feb 9, 2007, at 4:06 PM, Benjamin Herrenschmidt wrote:

> On Fri, 2007-02-09 at 12:13 -0500, Dan Malek wrote:
>> 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, 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

Re: Discussion about iopa()

[Timur Tabi]

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

Re: Discussion about iopa()

[Kumar Gala]

On Feb 9, 2007, at 4:51 PM, Timur Tabi wrote:

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

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

Re: Discussion about iopa()

[Timur Tabi]

Kumar Gala wrote:

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

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: Discussion about iopa()

[Benjamin Herrenschmidt]

On Fri, 2007-02-09 at 17:10 -0600, Timur Tabi wrote:
> Kumar Gala wrote:
> 
> > 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.

Specific allocator, no struct pages, etc.... no, it's not just like RAM.

Ben.

Re: Discussion about iopa()

[Kumar Gala]

On Feb 9, 2007, at 5:10 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> 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

Re: Discussion about iopa()

[Timur Tabi]

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

Re: Discussion about iopa()

[Kumar Gala]

On Feb 9, 2007, at 5:33 PM, Timur Tabi wrote:

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

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

Re: Discussion about iopa()

[Dan Malek]

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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

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.

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

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.

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Dan Malek]

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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

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

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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

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

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

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Timur Tabi]

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.

Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys to generic iopa

[Jeff Garzik]

Li Yang wrote:
> Get rid of private immrbar_virt_to_phys() routine and
> use generic iopa().
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> drivers/net/ucc_geth.c |   29 ++++++-----------------------
> 1 files changed, 6 insertions(+), 23 deletions(-)

I'll await resend pending resolution of comments