Hi Russell, On Wed, Sep 3, 2008 at 5:04 PM, Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> wrote: > On Wed, Sep 03, 2008 at 01:37:21PM -0700, Tony Lindgren wrote: >> * Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> [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. Yes, the code above is wrong. Now I understood that I was messing up with what you meant. > > 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. Yes, I agreed here. > > I'll cook a patch up. > -- Eduardo Bezerra Valentin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
