Re: FOR COMMENT: void __iomem * and similar casts are Bad News

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Wed, Sep 03, 2008 at 01:37:21PM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 12:49]:
> > Yes, that will be virtual.  But what does it mean to call:
> > 
> > 	omap_set_dma_dest_params()
> > 
> > specifying a virtual address?  Can the DMA controller cope with DMAing
> > to virtual addresses?  My hunch is that the DMA controller can't cope
> > with that, so giving it a virtual address is a bug.
> > 
> > Let me change the question: does omap_set_dma_dest_params()'s 4th
> > argument take a virtual or a physical address?  If the former, it's
> > prototype is wrong, and its 4th argument needs to be typed as
> > 'void __iomem *' rather than 'unsigned long'.  If the latter, the code
> > above is wrong.
> 
> The dma src and dest functions take physical addresses so the prototype
> should be void __iomem *.

Grr.  No.  Let me repeat the rule: 

- virtual addresses are pointer like.
- physical addresses are integer like.

So, if it's a physical address, it should be stored in an integer type
large enough to contain it, and that means something like u32, or
unsigned long.

If it's a virtual MMIO address, then it should be something like
'void __iomem *', or if you want to play roulette with compiler padding,
'struct foo __iomem *'.

Okay, so lets accept that the 4th argument to omap_set_dma_dest_params()
is a physical address.  It should be typed as 'unsigned long' (it is)
and it then means that:

diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index d084405..d9b1a42 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -652,6 +652,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
 	omap_set_dma_dest_params(mcbsp[id].dma_tx_lch,
 				 src_port,
 				 OMAP_DMA_AMODE_CONSTANT,
+				 /* FIXME: this is a virtual address */
 				 mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
 				 0, 0);
 
@@ -713,6 +714,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
 	omap_set_dma_src_params(mcbsp[id].dma_rx_lch,
 				src_port,
 				OMAP_DMA_AMODE_CONSTANT,
+				/* FIXME: this is a virtual address */
 				mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
 				0, 0);
 

are broken because they're passing a virtual address into a function
requiring a physical address.

Now, to fix this in the right way isn't going to be easy, because
arch/arm/plat-omap/mcbsp.c doesn't know what the physical address of
the mcbsp actually is - it's only passed the virtual address via
platform data (eww, yuck yuck yuck)...

If this was a properly reviewed platform driver, and on *any* *other*
ARM platform, it would take the resources containing the physical
addresses and ioremap them... and this would be a trivial bug fix.

I'll cook a patch up.