[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

[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

[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

[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

[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

[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

[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

[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

[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

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

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