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

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

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

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 */
 	}

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

Li Yang wrote:

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

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

I'll await resend pending resolution of comments

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

quoted

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

Nack. iopa() isn't that generic, shouldn't we really be using the dma
mapping API here?

Do you mean the dma_map_single()?  dma_map_single can map memory space
correctly, but I don't think it can handle ioremap-ed space.

- Leo

Kumar Gala wrote:

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

quoted

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

Nack. iopa() isn't that generic, shouldn't we really be using the dma 
mapping API here?

I'm having a hard time understanding what's wrong with iopa().  Is it because 
it's a 32-bit only function?  The memory has already been mapped with ioremap(), 
so why would we want to map it again?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

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

quoted

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

quoted

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

Nack. iopa() isn't that generic, shouldn't we really be using the dma
mapping API here?

Do you mean the dma_map_single()?  dma_map_single can map memory space
correctly, but I don't think it can handle ioremap-ed space.

What are you mapping?  It looks like buffer descriptor rings.  Are  
they being kept in QE sram?  If so that I think you can treat it as  
memory.  (I believe the SRAM behaves like memory, is cache-coherent,  
etc).

- k

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

Kumar Gala wrote:

quoted

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

quoted

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

Nack. iopa() isn't that generic, shouldn't we really be using the  
dma mapping API here?

I'm having a hard time understanding what's wrong with iopa().  Is  
it because it's a 32-bit only function?  The memory has already  
been mapped with ioremap(), so why would we want to map it again?

If its been mapped with ioremap() you know the physical address  
already so why do you need iopa().  The problem I have is iopa() is  
ppc specific and I can envision a day when someone wants to run Linux  
on a StarCore + QE, so having the drivers using ppc specific APIs is  
bad.

- k

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

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

quoted

If its been mapped with ioremap() you know the physical address

already

quoted

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

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

quoted

-----Original Message-----
From: Timur Tabi [mailto:timur@freescale.com]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private  
immrbar_virt_to_phys

to generic

quoted

iopa

Kumar Gala wrote:

quoted

If its been mapped with ioremap() you know the physical address

already

quoted

quoted

so why do you need iopa().

That's what the original function immrbar_virt_to_phys() does.  We're

trying to

quoted

get rid of it, because we thought is redundant with iopa().

static inline unsigned long immrbar_virt_to_phys(volatile void *

address)

quoted

{
	if ( ((u32)address >= (u32)qe_immr) &&
			((u32)address < ((u32)qe_immr + QE_IMMAP_SIZE))

)

quoted

		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

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

quoted

quoted

-----Original Message-----
From: Timur Tabi [mailto:timur@freescale.com]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private
immrbar_virt_to_phys

to generic

quoted

iopa

Kumar Gala wrote:

quoted

If its been mapped with ioremap() you know the physical address

already

quoted

quoted

so why do you need iopa().

That's what the original function immrbar_virt_to_phys() does.

We're

quoted

trying to

quoted

get rid of it, because we thought is redundant with iopa().

static inline unsigned long immrbar_virt_to_phys(volatile void *

address)

quoted

{
	if ( ((u32)address >= (u32)qe_immr) &&
			((u32)address < ((u32)qe_immr + QE_IMMAP_SIZE))

)

quoted

		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

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

quoted

-----Original Message-----
From: Kumar Gala [mailto:galak@kernel.crashing.org]
Sent: Thursday, February 08, 2007 1:58 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; netdev@vger.kernel.org; linuxppc- 
dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private  
immrbar_virt_to_phys

to generic

quoted

iopa


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

quoted

quoted

-----Original Message-----
From: Timur Tabi [mailto:timur@freescale.com]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private
immrbar_virt_to_phys

to generic

quoted

iopa

Kumar Gala wrote:

quoted

If its been mapped with ioremap() you know the physical address

already

quoted

quoted

so why do you need iopa().

That's what the original function immrbar_virt_to_phys() does.

We're

quoted

quoted

trying to

quoted

get rid of it, because we thought is redundant with iopa().

static inline unsigned long immrbar_virt_to_phys(volatile void *

address)

quoted

{
	if ( ((u32)address >= (u32)qe_immr) &&
			((u32)address < ((u32)qe_immr + QE_IMMAP_SIZE))

)

quoted

		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

quoted

quoted

quoted

quoted

quoted

If its been mapped with ioremap() you know the physical address

already

quoted

quoted

so why do you need iopa().

That's what the original function immrbar_virt_to_phys() does.

We're

quoted

quoted

trying to

quoted

get rid of it, because we thought is redundant with iopa().

static inline unsigned long immrbar_virt_to_phys(volatile void *

address)

quoted

{
	if ( ((u32)address >= (u32)qe_immr) &&
			((u32)address < ((u32)qe_immr + QE_IMMAP_SIZE))

)

quoted

		return (unsigned long)(address - (u32)qe_immr +
				(u32)get_qe_base());
	return (unsigned long)virt_to_phys(address);
}

get_qe_base() does a search of the OF tree the first time it's
called.

Here's the code that calls immrbar_virt_to_phys():

	out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
		 (u32) immrbar_virt_to_phys(ugeth->
					    p_tx_bd_ring[i]));


Would it be better to replace this code with something like this:

out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
	get_qe_base() + ((void *) ugeth->p_tx_bd_ring[i] - (void *)

qe_immr));

No, we don't know if the BD ring is in MURAM or main memory as it

is

quoted

quoted

quoted

configurable.  iopa() is best choice to handle both case, IMHO.

Does MURAM behave differently than normal memory?

MURAM is a mmio region so it don't share the characteristic of main
memory that phy_addr = 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

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

quoted

quoted

MURAM is a mmio region so it don't share the characteristic of main
memory that phy_addr = virt_addr - PAGE_OFFSET.  While they can
both be
mapped through page table using iopa().

Right, so when do you know if you'll be using MURAM or normal
memory?  Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal memory.

Yes, that can be a way.  But as the virt to phy mapping is only used
once, it's nothing bad to do it this way.

The problem as I stated before with using iopa() is that its not  
supported across platforms.

- k

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

quoted

quoted

quoted

MURAM is a mmio region so it don't share the characteristic of

main

quoted

quoted

quoted

memory that phy_addr = virt_addr - PAGE_OFFSET.  While they can
both be
mapped through page table using iopa().

Right, so when do you know if you'll be using MURAM or normal
memory?  Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal

memory.

quoted

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

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

quoted

-----Original Message-----
From: Kumar Gala [mailto:galak@kernel.crashing.org]
Sent: Thursday, February 08, 2007 3:16 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; netdev@vger.kernel.org; linuxppc- 
dev@ozlabs.org
Subject: Re: [PATCH 1/4] ucc_geth: Change private  
immrbar_virt_to_phys

to generic

quoted

iopa


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

quoted

quoted

quoted

MURAM is a mmio region so it don't share the characteristic of

main

quoted

quoted

quoted

quoted

memory that phy_addr = virt_addr - PAGE_OFFSET.  While they can
both be
mapped through page table using iopa().

Right, so when do you know if you'll be using MURAM or normal
memory?  Why not just keep around a token that is the physical
address at the point you make the decision of MURAM vs normal

memory.

quoted

quoted

Yes, that can be a way.  But as the virt to phy mapping is only used
once, it's nothing bad to do it this way.

The problem as I stated before with using iopa() is that its not
supported across platforms.

Yes, it is only for PPC32.  But we don't have another API to do  
it.  How
about make it more generic to add PPC64 version?

Why do you need another API to do this, you already have the  
information you want, its just a matter of you keeping track of it.

- k

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.

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.

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

Dan Malek wrote:

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

quoted

That's what the original code did, kinda.  It called virt_to_phys() if
it is main memory, and it called immrbar_virt_to_phys() if it is MURAM.
  immrbar_virt_to_phys() did pointer math to extract the physical 
address.

You've got to be kidding.  You created yet another function
to determine the physical address from a virtual one! LOL!
Plus, you need logic in the code to first determine which
one to call?  Everyone is OK with this?

The whole point behind the patch that we posted a couple days ago is to get rid 
of this function.  The patch replaced it with a call to iopa(), but Kumar nack'd 
that.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale