* Eduardo Valentin <edubezval@xxxxxxxxx> [080903 14:27]: > 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 *'. Oops sorry, that's correct. > > 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. > > Heh, I don't see how we still have working audio on at least 24xx :) Maybe the DMA src and dest addresses are really only 28-bit and it's ignoring the rest. Tony -- 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
