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

[Tony Lindgren <tony@atomide.com>]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 12:49]:
> On Wed, Sep 03, 2008 at 03:33:55PM -0400, Eduardo Valentin wrote:
> > Hi,
> > 
> > On Wed, Sep 3, 2008 at 2:48 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> > > On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
> > >> > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> > >> >  #ifdef CONFIG_ARCH_OMAP15XX
> > >> >  static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> > >> >        {
> > >> > -               .virt_base      = OMAP1510_MCBSP1_BASE,
> > >> > +               .virt_base      = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
> > >> AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
> > >> +               .virt_base      = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
> > >>
> > >> Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
> > >
> > > Ok, I'll fix these which you've confirmed in my version for mainline.
> > >
> > >> > @@ -701,6 +702,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> > >> >        omap_set_dma_dest_params(mcbsp->dma_tx_lch,
> > >> >                                 src_port,
> > >> >                                 OMAP_DMA_AMODE_CONSTANT,
> > >> > +                                /* FIXME: this is a virtual address */
> > >> >                                 mcbsp->io_base + OMAP_MCBSP_REG_DXR1,
> > >>
> > >> yes, that's true. This is expected to be virtual (mcbsp->io_base).
> > >
> > > Don't you mean that it is expected to be physical?
> > >
> > 
> > Well, correct me if I'm wrong, but I meant that the result of
> >  mcbsp->io_base + OMAP_MCBSP_REG_DXR1
> > 
> > is supposed to be virtual.
> 
> 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 *.

Tony

> Do you see what I'm getting at now?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html