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

---
drivers/net/ucc_geth.c | 29 ++++++-----------------------

1 files changed, 6 insertions(+), 23 deletions(-)

drivers/net/ucc_geth.c +6 -24

index 7e4b23c..722a89f 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

=20

Quoted 2 lines

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?

Quoted 7 lines · Li Yang

Quoted 3 lines

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.

Quoted 2 lines · Li Yang

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?

Quoted 4 lines · Kumar Gala

Quoted 2 lines · Li Yang

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

-----Original Message-----
From: Timur Tabi [mailto:[email protected]]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; [email protected]; [email protected]
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys

iopa
=20
Kumar Gala wrote:
=20

Quoted 1 line

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

Quoted 1 line

so why do you need iopa().

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

get rid of it, because we thought is redundant with iopa(). =20
static inline unsigned long immrbar_virt_to_phys(volatile void *

{
	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 *)

Quoted 7 lines

-----Original Message-----
From: Timur Tabi [mailto:[email protected]]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; [email protected]; [email protected]
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys

to generic

Quoted 5 lines

iopa

Quoted 1 line · Kumar Gala

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

already

Quoted 3 lines

Quoted 1 line

so why do you need iopa().

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

trying to

Quoted 3 lines

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

static inline unsigned long immrbar_virt_to_phys(volatile void *

address)

Quoted 3 lines

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

)

Quoted 15 lines

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

Quoted 4 lines

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.

-----Original Message-----
From: Kumar Gala [mailto:[email protected]]
Sent: Thursday, February 08, 2007 1:58 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; [email protected]; [email protected]
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys

iopa

Quoted 17 lines · On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:

Quoted 7 lines

-----Original Message-----
From: Timur Tabi [mailto:[email protected]]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; [email protected]; [email protected]
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys

to generic

Quoted 5 lines

iopa

Quoted 1 line · Kumar Gala

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

already

Quoted 3 lines

Quoted 1 line

so why do you need iopa().

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

Quoted 26 lines

trying to

Quoted 3 lines

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

static inline unsigned long immrbar_virt_to_phys(volatile void *

address)

Quoted 3 lines

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

)

Quoted 17 lines

		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:

Quoted 6 lines

Quoted 2 lines

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?

Quoted 6 lines

-----Original Message-----
From: Kumar Gala [mailto:[email protected]]
Sent: Thursday, February 08, 2007 1:58 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; [email protected]; [email protected]
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys

to generic

Quoted 22 lines

iopa

Quoted 17 lines · On Feb 7, 2007, at 11:52 PM, Li Yang-r58472 wrote:

Quoted 7 lines

-----Original Message-----
From: Timur Tabi [mailto:[email protected]]
Sent: Thursday, February 08, 2007 1:03 AM
To: Kumar Gala
Cc: Li Yang-r58472; [email protected]; [email protected]
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys

to generic

Quoted 5 lines

iopa

Quoted 1 line · Kumar Gala

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

already

Quoted 3 lines

Quoted 1 line

so why do you need iopa().

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

We're

Quoted 30 lines

Quoted 30 lines

trying to

Quoted 3 lines

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

static inline unsigned long immrbar_virt_to_phys(volatile void *

address)

Quoted 3 lines

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

)

Quoted 15 lines

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

Quoted 4 lines

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

Quoted 4 lines

Quoted 2 lines

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

Quoted 30 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

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

already

Quoted 3 lines

Quoted 1 line

so why do you need iopa().

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

We're

Quoted 24 lines

Quoted 24 lines

trying to

Quoted 3 lines

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

static inline unsigned long immrbar_virt_to_phys(volatile void *

address)

Quoted 3 lines

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

)

Quoted 15 lines

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

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

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

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

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

Does MURAM behave differently than normal memory?

Quoted 4 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

-----Original Message-----
From: Kumar Gala [mailto:[email protected]]
Sent: Thursday, February 08, 2007 3:16 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; [email protected]; [email protected]
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys

iopa

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

Quoted 1 line

Quoted 1 line

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

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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

Quoted 3 lines

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.

Quoted 6 lines

-----Original Message-----
From: Kumar Gala [mailto:[email protected]]
Sent: Thursday, February 08, 2007 3:16 PM
To: Li Yang-r58472
Cc: Tabi Timur-B04825; [email protected]; [email protected]
Subject: Re: [PATCH 1/4] ucc_geth: Change private immrbar_virt_to_phys

to generic

Quoted 6 lines

iopa

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

Quoted 1 line

Quoted 1 line

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

main

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

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?

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

main

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

memory that phy_addr =3D virt_addr - PAGE_OFFSET. While they =

Quoted 9 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

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

Quoted 2 lines

Yes, that can be a way. But as the virt to phy mapping is only

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Walking the page table is slower than using pointer math, so you want to avoid iopa() if you can.

Quoted 3 lines · Timur Tabi

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.

Quoted 1 line · Benjamin Herrenschmidt

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

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

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

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.

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

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.

Quoted 2 lines

.... though just saying "just
sucks" is neither useful nor constructive.

About in line with "I don't like it" :-)

Quoted 4 lines

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

Quoted 2 lines

Now, I don't completely agree with you that there are "fundamental" limitations in the way memory is managed.

Sure there are, but it's not for discussion here.

Quoted 2 lines

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

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.

I'm really not fan of it.

Quoted 1 line · Benjamin Herrenschmidt

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

Quoted 4 lines · Dan Malek

Quoted 1 line · Benjamin Herrenschmidt

I'm really not fan of it.

Quoted 2 lines

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.

Quoted 1 line · Kumar Gala

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.

Quoted 2 lines · Kumar Gala

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.

Quoted 1 line · Kumar Gala

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.

We should fix it so the driver just has to do dma_alloc_coherent, dma_free_coherent, and dma_map_*/dma_unmap_*

Quoted 1 line · Kumar Gala

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?

Do you think we should remove iopa?

The information is often not so obvious. ...

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.

Quoted 4 lines · Timur Tabi

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?

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.

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

Signed-off-by Li Yang <[email protected]>

---
drivers/net/ucc_geth.c | 29 ++++++-----------------------

1 files changed, 6 insertions(+), 23 deletions(-)