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

I sent a similar patch to the one below against mainline to Tony today.

I've marked some changes with certain tags:

- FIXME: where I think the code is just plain wrong -
  - such as putting a physical address through a macro which converts virtual to physical addresses (1510 and 1610 mcbsp).
  - or passing virtual addresses to DMA functions (mcbsp).
  - or passing physical addresses when what's required is a virtual address (omap_udc).

- CHECKME: where I've changed something to make it build but I don't know if this is right.

- WBNI: more a preference to see something changed than a bug.

Comments on the FIXMEs are what I'm really interested in, and preferably having them actually fixed would be a good idea.

Quoted 1 line · Russell King

I sent a similar patch to the one below against mainline to Tony today.

And I'm glad to see those updates. I used to constantly trip over code doing strange stuff in those areas, and it would be nice to see "sparse" approve a lot more driver code...

Quoted 9 lines

I've marked some changes with certain tags:

- FIXME: where I think the code is just plain wrong -
  - such as putting a physical address through a macro which converts virtual to physical addresses (1510 and 1610 mcbsp).
  - or passing virtual addresses to DMA functions (mcbsp).
  - or passing physical addresses when what's required is a virtual address (omap_udc).

Yeah, the omap_udc one looks like the __REG() conversion patch just did the wrong thing. Fix looks trivial, and once I verify it then I'll send it through mainline.

Quoted 7 lines

- CHECKME: where I've changed something to make it build but I don't know if this is right.

- WBNI: more a preference to see something changed than a bug.

Comments on the FIXMEs are what I'm really interested in, and preferably having them actually fixed would be a good idea.

I verified that the OMAP tree boots on my OMAP5912 OSK, at least once things like cpufreq, PM, and MTD are disabled. The MTD thing is a known/solved issue in mainline. (Block queue changes broke MTD...) The oopses in cpufreq and pm_idle are more cryptic.

So I can try my patch for the omap_udc thing ... even though for OSK the mainline kernel ** DOES NOT BOOT ** and dies even before the kernel "hello, I'm RC5!" banner prints. Someone with a JTAG to throw at this might be able to fix that regression quickly.

Back online now. These changes look ggood to me in general, except I suggest that we use the following standard:

- Keep OMAP1_IO_ADDRESS() and OMAP2_IO_ADDRESS() as I have some experimental patches to compile in both omap1 and omap2 into the same binary. Booting the kernel currently still requires some Makefile.boot patching, but at least compiling everything in makes things easier to maintain in the long run.

- Use io_p2v() for initializing dynamic stuff as it can be a function for non-optimized multiboot binaries.

Quoted 8 lines · Tony Lindgren

Back online now. These changes look ggood to me in general, except I suggest that we use the following standard:

- Keep OMAP1_IO_ADDRESS() and OMAP2_IO_ADDRESS() as I have some experimental patches to compile in both omap1 and omap2 into the same binary. Booting the kernel currently still requires some Makefile.boot patching, but at least compiling everything in makes things easier to maintain in the long run.

Already decided to do that.

Quoted 2 lines

- Use io_p2v() for initializing dynamic stuff as it can be a function for non-optimized multiboot binaries.

It can't become a function - it's used in structure initialization so must be constant.

There's a few places where it's used where resources are passed into drivers - in which case if the device is at a different physical address it's the resources which should be changed, not the translation macro.

Anyway, I've put a modified version in my git tree.

I still want to hear on the other build fix in the patch, and there's also a missing function for mmc stuff which I've not looked into yet.

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines

- Use io_p2v() for initializing dynamic stuff as it can be a function for non-optimized multiboot binaries.

It can't become a function - it's used in structure initialization so must be constant.

Outside the drivers we can use XXX_IO_ADDRESS() or set it during resource init with io_p2v().

Quoted 2 lines

I still want to hear on the other build fix in the patch, and there's also a missing function for mmc stuff which I've not looked into yet.

The mpuio build fix? That is a correct fix.

Quoted 7 lines · Tony Lindgren

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines

- Use io_p2v() for initializing dynamic stuff as it can be a function for non-optimized multiboot binaries.

It can't become a function - it's used in structure initialization so must be constant.

Quoted 2 lines

Outside the drivers we can use XXX_IO_ADDRESS() or set it during resource init with io_p2v().

The question is why do we need it? If the correct physical address is passed, then things should work out just fine anyway, especially if drivers start to use ioremap rather than relying on all these fixed translations.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

I still want to hear on the other build fix in the patch, and there's also a missing function for mmc stuff which I've not looked into yet.

The mpuio build fix? That is a correct fix.

Review

Quoted 5 lines

No, see drivers/usb/host/ohci-omap.c:

(quoted diff) +1 -1

-#ifdef CONFIG_ARCH_OMAP_OTG
+#ifdef CONFIG_USB_OTG /* CHECKME */

That's a Dave question.

Quoted 5 lines · Tony Lindgren

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

I still want to hear on the other build fix in the patch, and there's also a missing function for mmc stuff which I've not looked into yet.

The mpuio build fix? That is a correct fix.

Quoted 7 lines

Review

Quoted 5 lines

No, see drivers/usb/host/ohci-omap.c:

(quoted diff) +1 -1

-#ifdef CONFIG_ARCH_OMAP_OTG
+#ifdef CONFIG_USB_OTG /* CHECKME */

That's a Dave question.

That stuff's been whacked around a bunch, so I'm not sure what its latest incarnation is expected to do. Looks like that change (to USB_OTG) couldn't hurt ... go for it.

From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Russell King - ARM Linux
Sent: Wednesday, September 03, 2008 2:34 PM

The question is why do we need it? If the correct physical address is passed, then things should work out just fine anyway, especially if drivers start to use ioremap rather than relying on all these fixed translations.

Fixed translations do have some benefits. You can ensure that you are using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.

Each chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-boots with in the same architecture family seems really good but across seems less so.

From: Russell King - ARM Linux [mailto:[email protected]]
Sent: Wednesday, September 03, 2008 3:30 PM

Quoted 3 lines

Fixed translations do have some benefits. You can ensure that you are using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries

you

Quoted 2 lines

also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.

These are valid points, and you can arrange for ioremap to give you the same effect. That way, you end up using _standard_ Linux interfaces which everyone understands and still having your cake.

Quoted 4 lines

Each chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-boots with in the same architecture family seems really good but across seems less so.

This thread isn't about multi-boots across differnet architecture; please take that discussion elsewhere - it's off topic.

Quoted 4 lines

From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Russell King - ARM Linux
Sent: Wednesday, September 03, 2008 2:34 PM

<snip>

Quoted 4 lines

The question is why do we need it? If the correct physical address is passed, then things should work out just fine anyway, especially if drivers start to use ioremap rather than relying on all these fixed translations.

Fixed translations do have some benefits. You can ensure that you are using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.

With in a family of chips like 2420/22/23 or 3410/20/30/40 the internal space is mapped the same.

Frankly I've never been convinced that a multi OMAP1/2/3 image makes much sense apart forcing better code structure and being kind of cool. Each chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-boots with in the same architecture family seems really good but across seems less so.

From: Tony Lindgren [mailto:[email protected]]
Sent: Wednesday, September 03, 2008 3:33 PM

Quoted 1 line

Fixed translations do have some benefits. You can ensure that you are

using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.

Quoted 1 line

Quoted 1 line

With in a family of chips like 2420/22/23 or 3410/20/30/40 the

internal space is mapped the same.

I guess there's no advantage of using ioremap if the area is already mapped. For drivers that are shared across multiple archs or buses it makes sense.

Quoted 1 line

Frankly I've never been convinced that a multi OMAP1/2/3 image makes

much sense apart forcing better code structure and being kind of cool. Each chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi- boots with in the same architecture family seems really good but across seems less so.

It definitely makes sense from distro and maintenance point of view. If we did not work towards multi-omap we would already have the same code duplicated many times over.

Quoted 3 lines

From: Tony Lindgren [mailto:[email protected]]
Sent: Wednesday, September 03, 2008 3:33 PM

<snip>

Quoted 6 lines

Quoted 1 line

Fixed translations do have some benefits. You can ensure that you are

using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.

Quoted 1 line

Quoted 6 lines

Quoted 1 line

With in a family of chips like 2420/22/23 or 3410/20/30/40 the

internal space is mapped the same.

I guess there's no advantage of using ioremap if the area is already mapped. For drivers that are shared across multiple archs or buses it makes sense.

That has been my general feeling.

Many drivers have some ISA'ish way to get address or they have dynamic ways. While many probably hate the old fixed systems in embedded it doesn't always seem so offensive.

You still can abstract resources into platform specific data areas.

Quoted 7 lines

Quoted 1 line

Frankly I've never been convinced that a multi OMAP1/2/3 image makes

much sense apart forcing better code structure and being kind of cool. Each chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi- boots with in the same architecture family seems really good but across seems less so.

Quoted 3 lines

It definitely makes sense from distro and maintenance point of view. If we did not work towards multi-omap we would already have the same code duplicated many times over.

Yes. But this also adds a high level of coupling. Sometimes this great like an internal MUSB driver other times it complicates development like for power aspects.

For example domain OFF mode is not compelling given how good retention is at the process node used for all OMAP2s. But it is compelling on an OMAP3. Pushing back OMAP3 needed code on to OMAP2 in this area ends up being not so beneficial for OMAP2. It causes regressions and slows down OMAP3 code adoption. If its down nicely like Paul has been working at it may turn out well. But the time to do this is extended.

Quoted 3 lines

From: Tony Lindgren [mailto:[email protected]]
Sent: Wednesday, September 03, 2008 3:33 PM

<snip>

Quoted 6 lines

Quoted 1 line

Fixed translations do have some benefits. You can ensure that you are

using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.

Quoted 1 line

Quoted 6 lines

Quoted 1 line

With in a family of chips like 2420/22/23 or 3410/20/30/40 the

internal space is mapped the same.

I guess there's no advantage of using ioremap if the area is already mapped. For drivers that are shared across multiple archs or buses it makes sense.

That has been my general feeling.

Many drivers have some ISA'ish way to get address or they have dynamic ways. While many probably hate the old fixed systems in embedded it doesn't always seem so offensive.

You still can abstract resources into platform specific data areas.

Quoted 7 lines

Quoted 1 line

Frankly I've never been convinced that a multi OMAP1/2/3 image makes

much sense apart forcing better code structure and being kind of cool. Each chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi- boots with in the same architecture family seems really good but across seems less so.

Quoted 3 lines

It definitely makes sense from distro and maintenance point of view. If we did not work towards multi-omap we would already have the same code duplicated many times over.

Yes. But this also adds a high level of coupling. Sometimes this great like an internal MUSB driver other times it complicates development like for power aspects.

For example domain OFF mode is not compelling given how good retention is at the process node used for all OMAP2s. But it is compelling on an OMAP3. Pushing back OMAP3 needed code on to OMAP2 in this area ends up being not so beneficial for OMAP2. It causes regressions and slows down OMAP3 code adoption. If its down nicely like Paul has been working at it may turn out well. But the time to do this is extended.

From: Russell King - ARM Linux [mailto:[email protected]]

Can we _PLEASE_ take this discussion to a different thread. OFF TOPIC. Thank you.

Fixed translations do have some benefits.  You can ensure that you are using section or super section descriptors to cover large areas. This does result in better TLB usage.  Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.

Frankly I've never been convinced that a multi OMAP1/2/3 image makes much sense apart forcing better code structure

and being kind of cool.  Each
chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7).  Doing multi-boots with in the same architecture family seems really good but across seems less so.

From: David Brownell [mailto:[email protected]]

Quoted 5 lines

Fixed translations do have some benefits. You can ensure that you are using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.

... which is exactly why some linux/arch/... code makes sure ioremap() can return fixed mappings instead of always requiring dynamic ones.

True enough, but that decision can be in the hands of whoever builds a kernel. If you want to deploy an A8-optimized system, the kernel will be only one part of the equation.

Quoted 4 lines

Quoted 1 line

... [ big/inexpensive fixed mappings can be virtuous ... ]

... which is exactly why some linux/arch/... code makes sure ioremap() can return fixed mappings instead of always requiring dynamic ones.

That does sound like a nice consistent way to do it.  Which implementation of this would you recommend looking at?  Do any ARMs you know of do it today?

According to Mr. Grep, there are at least 10 ARMs that work like that in mainline. Maybe Russell can recommend one of them as a preferred model.

Quoted 3 lines · David Brownell

According to Mr. Grep, there are at least 10 ARMs that work like that in mainline. Maybe Russell can recommend one of them as a preferred model.

As I've been trying to say, I see this as a separate issue for the near future. At the moment, I'm trying to concentrate on one aspect only.

That is, getting OMAP to the point that we're using the compiler to warn us when we do something silly, like passing a virtual address to a function which takes a physical address, and fixing the places which are currently wrong.

Anything else like changing the ioremap behaviour is actually completely orthogonal to that, and *is* a distraction. Rather than actually fixing the mcbsp.c issue, I've spent the last hour or so composing various replies to Richard's emails, and then postponing them, reading more of this thread, creating more replies, postponing those as well, etc.

Anyway, what I've done now is committed the minimal set of fixes so far to go into mainline for the current -rc so we can at least improve the situation there. That's not to say that the other patch won't be going in - it will in some form or other.

It can also be applied to the omap tree by saving this message as "whateverpatchfile", running:

sed -i 's,\[id\]\.,->,' whateverpatchfile

and then applying "whateverpatchfile". Expect some offsets.

Arun - can you please test this patch on your 5912 OSK board to see if it resolves your problem please?

arch/arm/mach-omap1/mcbsp.c +4 -4

index 826010d..b6ffbab 100644
@@ -184,7 +184,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	= io_p2v(OMAP1510_MCBSP1_BASE),
 		.dma_rx_sync	= OMAP_DMA_MCBSP1_RX,
 		.dma_tx_sync	= OMAP_DMA_MCBSP1_TX,
 		.rx_irq		= INT_McBSP1RX,

@@ -201,7 +201,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
 		.ops		= &omap1_mcbsp_ops,
 	},
 	{
-		.virt_base	= OMAP1510_MCBSP3_BASE,
+		.virt_base	= io_p2v(OMAP1510_MCBSP3_BASE),
 		.dma_rx_sync	= OMAP_DMA_MCBSP3_RX,
 		.dma_tx_sync	= OMAP_DMA_MCBSP3_TX,
 		.rx_irq		= INT_McBSP3RX,

@@ -219,7 +219,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
 #ifdef CONFIG_ARCH_OMAP16XX
 static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
 	{
-		.virt_base	= OMAP1610_MCBSP1_BASE,
+		.virt_base	= io_p2v(OMAP1610_MCBSP1_BASE),
 		.dma_rx_sync	= OMAP_DMA_MCBSP1_RX,
 		.dma_tx_sync	= OMAP_DMA_MCBSP1_TX,
 		.rx_irq		= INT_McBSP1RX,

@@ -236,7 +236,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
 		.ops		= &omap1_mcbsp_ops,
 	},
 	{
-		.virt_base	= OMAP1610_MCBSP3_BASE,
+		.virt_base	= io_p2v(OMAP1610_MCBSP3_BASE),
 		.dma_rx_sync	= OMAP_DMA_MCBSP3_RX,
 		.dma_tx_sync	= OMAP_DMA_MCBSP3_TX,
 		.rx_irq		= INT_McBSP3RX,

arch/arm/plat-omap/mcbsp.c +2 -2

index d084405..5245a2a 100644
@@ -651,7 +651,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,
-				 mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
+				 io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1),
 				 0, 0);
 
 	omap_set_dma_src_params(mcbsp[id].dma_tx_lch,

@@ -712,7 +712,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,
-				mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
+				io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1),
 				0, 0);
 
 	omap_set_dma_dest_params(mcbsp[id].dma_rx_lch,

Quoted 3 lines · David Brownell

According to Mr. Grep, there are at least 10 ARMs that work like that [__arch_ioremap] in mainline. Maybe Russell can recommend one of them as a preferred model.

As I've been trying to say, I see this as a separate issue for the near future. At the moment, I'm trying to concentrate on one aspect only.

That is, getting OMAP to the point that we're using the compiler to warn us when we do something silly, like passing a virtual address to a function which takes a physical address, and fixing the places which are currently wrong.

Quoted 3 lines · David Brownell

According to Mr. Grep, there are at least 10 ARMs that work like that in mainline. Maybe Russell can recommend one of them as a preferred model.

As I've been trying to say, I see this as a separate issue for the near future. At the moment, I'm trying to concentrate on one aspect only.

That is, getting OMAP to the point that we're using the compiler to warn us when we do something silly, like passing a virtual address to a function which takes a physical address, and fixing the places which are currently wrong.

Anything else like changing the ioremap behaviour is actually completely orthogonal to that, and *is* a distraction. Rather than actually fixing the mcbsp.c issue, I've spent the last hour or so composing various replies to Richard's emails, and then postponing them, reading more of this thread, creating more replies, postponing those as well, etc.

Anyway, what I've done now is committed the minimal set of fixes so far to go into mainline for the current -rc so we can at least improve the situation there. That's not to say that the other patch won't be going in - it will in some form or other.

It can also be applied to the omap tree by saving this message as "whateverpatchfile", running:

sed -i 's,\[id\]\.,->,' whateverpatchfile

and then applying "whateverpatchfile". Expect some offsets.

Arun - can you please test this patch on your 5912 OSK board to see if it resolves your problem please?

arch/arm/mach-omap1/mcbsp.c +4 -4

index 826010d..b6ffbab 100644
@@ -184,7 +184,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      = io_p2v(OMAP1510_MCBSP1_BASE),
               .dma_rx_sync    = OMAP_DMA_MCBSP1_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP1_TX,
               .rx_irq         = INT_McBSP1RX,

@@ -201,7 +201,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
               .ops            = &omap1_mcbsp_ops,
       },
       {
-               .virt_base      = OMAP1510_MCBSP3_BASE,
+               .virt_base      = io_p2v(OMAP1510_MCBSP3_BASE),
               .dma_rx_sync    = OMAP_DMA_MCBSP3_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP3_TX,
               .rx_irq         = INT_McBSP3RX,

@@ -219,7 +219,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
 #ifdef CONFIG_ARCH_OMAP16XX
 static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
       {
-               .virt_base      = OMAP1610_MCBSP1_BASE,
+               .virt_base      = io_p2v(OMAP1610_MCBSP1_BASE),
               .dma_rx_sync    = OMAP_DMA_MCBSP1_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP1_TX,
               .rx_irq         = INT_McBSP1RX,

@@ -236,7 +236,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
               .ops            = &omap1_mcbsp_ops,
       },
       {
-               .virt_base      = OMAP1610_MCBSP3_BASE,
+               .virt_base      = io_p2v(OMAP1610_MCBSP3_BASE),
               .dma_rx_sync    = OMAP_DMA_MCBSP3_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP3_TX,
               .rx_irq         = INT_McBSP3RX,

arch/arm/plat-omap/mcbsp.c +2 -2

index d084405..5245a2a 100644
@@ -651,7 +651,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,
-                                mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
+                                io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1),
                                0, 0);
 
       omap_set_dma_src_params(mcbsp[id].dma_tx_lch,

@@ -712,7 +712,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,
-                               mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
+                               io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1),
                               0, 0);
 
       omap_set_dma_dest_params(mcbsp[id].dma_rx_lch,

Hi,

Quoted 6 lines · David Brownell

Quoted 1 line · Russell King

I sent a similar patch to the one below against mainline to Tony today.

And I'm glad to see those updates. I used to constantly trip over code doing strange stuff in those areas, and it would be nice to see "sparse" approve a lot more driver code...

Back online now. These changes look ggood to me in general, except I suggest that we use the following standard:

- Keep OMAP1_IO_ADDRESS() and OMAP2_IO_ADDRESS() as I have some experimental patches to compile in both omap1 and omap2 into the same binary. Booting the kernel currently still requires some Makefile.boot patching, but at least compiling everything in makes things easier to maintain in the long run.

- Use io_p2v() for initializing dynamic stuff as it can be a function for non-optimized multiboot binaries.

Quoted 28 lines

Quoted 9 lines

I've marked some changes with certain tags:

- FIXME: where I think the code is just plain wrong -
- such as putting a physical address through a macro which converts virtual to physical addresses (1510 and 1610 mcbsp).
- or passing virtual addresses to DMA functions (mcbsp).
- or passing physical addresses when what's required is a virtual address

    (omap_udc).

Yeah, the omap_udc one looks like the __REG() conversion patch just did the wrong thing. Fix looks trivial, and once I verify it then I'll send it through mainline.

Quoted 7 lines

- CHECKME: where I've changed something to make it build but I don't know if this is right.

- WBNI: more a preference to see something changed than a bug.

Comments on the FIXMEs are what I'm really interested in, and preferably having them actually fixed would be a good idea.

I verified that the OMAP tree boots on my OMAP5912 OSK, at least once things like cpufreq, PM, and MTD are disabled. The MTD thing is a known/solved issue in mainline. (Block queue changes broke MTD...) The oopses in cpufreq and pm_idle are more cryptic.

Quoted 4 lines

So I can try my patch for the omap_udc thing ... even though for OSK the mainline kernel ** DOES NOT BOOT ** and dies even before the kernel "hello, I'm RC5!" banner prints. Someone with a JTAG to throw at this might be able to fix that regression quickly.

I'll take a look at the OSK boot problem unless somebody else is already working on it.

Looks like the FIXME mcbsp phys vs virt addresses are wrong for some board-*.c files. Eduardo, can you please take a look at them?

Tony

Hi,

Quoted 1 line · David Brownell

On Wednesday 27 August 2008, Russell King wrote:

Quoted 5 lines

So I can try my patch for the omap_udc thing ... even though for OSK the mainline kernel ** DOES NOT BOOT ** and dies even before the kernel "hello, I'm RC5!" banner prints. Someone with a JTAG to throw at this might be able to fix that regression quickly.

I'll take a look at the OSK boot problem unless somebody else is already working on it.

My OSK is booting just fine here with current mainline and the omap_osk_5912_defconfig. This is the current head at 2.6.27-rc5-00100-gec0c15a. Have you checked if yours boots with the mainline omap_osk_5912_defconfig?

... are bad when overused. Unexpected bugs can creap in. That one cast which someone added to shut up a perfectly valid compiler warning now hides a potential problem.

It is possible to eliminate _lots_ of these casts (and the potential for bad casts) by making things return the right types. Like, making IO_ADDRESS return "void __iomem *" rather than an integer.

I sent a similar patch to the one below against mainline to Tony today. I've marked some changes with certain tags:

- FIXME: where I think the code is just plain wrong -
- such as putting a physical address through a macro which converts virtual to physical addresses (1510 and 1610 mcbsp).
- or passing virtual addresses to DMA functions (mcbsp).
- or passing physical addresses when what's required is a virtual address (omap_udc).

- CHECKME: where I've changed something to make it build but I don't know if this is right.

- WBNI: more a preference to see something changed than a bug.

Comments on the FIXMEs are what I'm really interested in, and preferably having them actually fixed would be a good idea.

I've compile tested this on a few omap defconfigs and it at least builds.

arch/arm/mach-omap1/mcbsp.c +2 -2

index 265cfc2..e5ac038 100644
@@ -134,7 +134,7 @@ static struct omap_mcbsp_ops omap1_mcbsp_ops = {
 #ifdef CONFIG_ARCH_OMAP730
 static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
       {
-               .virt_base      = io_p2v(OMAP730_MCBSP1_BASE),
+               .virt_base      = IO_ADDRESS(OMAP730_MCBSP1_BASE),
               .dma_rx_sync    = OMAP_DMA_MCBSP1_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP1_TX,
               .rx_irq         = INT_730_McBSP1RX,

@@ -142,7 +142,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
               .ops            = &omap1_mcbsp_ops,
       },
       {
-               .virt_base      = io_p2v(OMAP730_MCBSP2_BASE),
+               .virt_base      = IO_ADDRESS(OMAP730_MCBSP2_BASE),
               .dma_rx_sync    = OMAP_DMA_MCBSP3_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP3_TX,
               .rx_irq         = INT_730_McBSP2RX,

arch/arm/mach-omap1/mcbsp.c +1 -1

               .dma_rx_sync    = OMAP_DMA_MCBSP1_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP1_TX,
               .rx_irq         = INT_McBSP1RX,

@@ -168,7 +168,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
               .clk_name       = "mcbsp_clk",
               },
       {
-               .virt_base      = io_p2v(OMAP1510_MCBSP2_BASE),
+               .virt_base      = IO_ADDRESS(OMAP1510_MCBSP2_BASE),
               .dma_rx_sync    = OMAP_DMA_MCBSP2_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP2_TX,
               .rx_irq         = INT_1510_SPI_RX,

arch/arm/mach-omap1/mcbsp.c +1 -1

               .dma_rx_sync    = OMAP_DMA_MCBSP1_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP1_TX,
               .rx_irq         = INT_McBSP1RX,

@@ -203,7 +203,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
               .clk_name       = "mcbsp_clk",
       },
       {
-               .virt_base      = io_p2v(OMAP1610_MCBSP2_BASE),
+               .virt_base      = IO_ADDRESS(OMAP1610_MCBSP2_BASE),
               .dma_rx_sync    = OMAP_DMA_MCBSP2_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP2_TX,
               .rx_irq         = INT_1610_McBSP2_RX,

arch/arm/mach-omap1/mcbsp.c +0 -0

               .dma_rx_sync    = OMAP_DMA_MCBSP3_RX,
               .dma_tx_sync    = OMAP_DMA_MCBSP3_TX,
               .rx_irq         = INT_McBSP3RX,

arch/arm/mach-omap1/serial.c +6 -6

index 0e25a99..4965986 100644
@@ -67,8 +67,8 @@ static void __init omap_serial_reset(struct plat_serial8250_port *p)
 
 static struct plat_serial8250_port serial_platform_data[] = {
       {
-               .membase        = (char*)IO_ADDRESS(OMAP_UART1_BASE),
-               .mapbase        = (unsigned long)OMAP_UART1_BASE,
+               .membase        = IO_ADDRESS(OMAP_UART1_BASE),
+               .mapbase        = OMAP_UART1_BASE,
               .irq            = INT_UART1,
               .flags          = UPF_BOOT_AUTOCONF,
               .iotype         = UPIO_MEM,

@@ -76,8 +76,8 @@ static struct plat_serial8250_port serial_platform_data[] = {
               .uartclk        = OMAP16XX_BASE_BAUD * 16,
       },
       {
-               .membase        = (char*)IO_ADDRESS(OMAP_UART2_BASE),
-               .mapbase        = (unsigned long)OMAP_UART2_BASE,
+               .membase        = IO_ADDRESS(OMAP_UART2_BASE),
+               .mapbase        = OMAP_UART2_BASE,
               .irq            = INT_UART2,
               .flags          = UPF_BOOT_AUTOCONF,
               .iotype         = UPIO_MEM,

@@ -85,8 +85,8 @@ static struct plat_serial8250_port serial_platform_data[] = {
               .uartclk        = OMAP16XX_BASE_BAUD * 16,
       },
       {
-               .membase        = (char*)IO_ADDRESS(OMAP_UART3_BASE),
-               .mapbase        = (unsigned long)OMAP_UART3_BASE,
+               .membase        = IO_ADDRESS(OMAP_UART3_BASE),
+               .mapbase        = OMAP_UART3_BASE,
               .irq            = INT_UART3,
               .flags          = UPF_BOOT_AUTOCONF,
               .iotype         = UPIO_MEM,

arch/arm/mach-omap2/clock.c +3 -2

index be5c616..0c6a1b4 100644
@@ -223,10 +223,11 @@ int omap2_wait_clock_ready(void __iomem *reg, u32 mask, const char *name)
 static void omap2_clk_wait_ready(struct clk *clk)
 {
       u32 other_bit, idlest_bit;
-       unsigned long reg, other_reg, idlest_reg, prcm_mod, prcm_regid;
+       unsigned long reg, other_reg, idlest_reg, prcm_regid;
+       void __iomem *prcm_mod;
 
       reg = (unsigned long)clk->enable_reg;
-       prcm_mod = reg & ~0xff;
+       prcm_mod = (void __iomem *)(reg & ~0xff);
       prcm_regid = reg & 0xff;
 
       if (prcm_regid >= CM_FCLKEN1 && prcm_regid <= OMAP24XX_CM_FCLKEN2)

arch/arm/mach-omap2/gpmc.c +10 -10

index 9d6c916..7e3de6e 100644
@@ -59,34 +59,34 @@ static struct resource      gpmc_cs_mem[GPMC_CS_NUM];
 static DEFINE_SPINLOCK(gpmc_mem_lock);
 static unsigned                gpmc_cs_map;
 
-static u32 gpmc_base;
+static void __iomem *gpmc_base;
 
 static struct clk *gpmc_l3_clk;
 
 static void gpmc_write_reg(int idx, u32 val)
 {
-       __raw_writel(val, (__force void __iomem *)(gpmc_base + idx));
+       __raw_writel(val, gpmc_base + idx);
 }
 
 static u32 gpmc_read_reg(int idx)
 {
-       return __raw_readl((__force void __iomem *)(gpmc_base + idx));
+       return __raw_readl(gpmc_base + idx);
 }
 
 void gpmc_cs_write_reg(int cs, int idx, u32 val)
 {
-       u32 reg_addr;
+       void __iomem *reg_addr;
 
       reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
-       __raw_writel(val, (__force void __iomem *)reg_addr);
+       __raw_writel(val, reg_addr);
 }
 
 u32 gpmc_cs_read_reg(int cs, int idx)
 {
-       u32 reg_addr;
+       void __iomem *reg_addr;
 
       reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
-       return __raw_readl((__force void __iomem *)reg_addr);
+       return __raw_readl(reg_addr);
 }
 
 /* TODO: Add support for gpmc_fck to clock framework and use it */

@@ -417,12 +417,12 @@ void __init gpmc_init(void)
       if (cpu_is_omap24xx()) {
               gpmc_l3_clk = clk_get(NULL, "core_l3_ck");
               if (cpu_is_omap2420())
-                       gpmc_base = io_p2v(OMAP2420_GPMC_BASE);
+                       gpmc_base = IO_ADDRESS(OMAP2420_GPMC_BASE);
               else if (cpu_is_omap2430())
-                       gpmc_base = io_p2v(OMAP243X_GPMC_BASE);
+                       gpmc_base = IO_ADDRESS(OMAP243X_GPMC_BASE);
       } else if (cpu_is_omap34xx()) {
               gpmc_l3_clk = clk_get(NULL, "gpmc_fck");
-               gpmc_base = io_p2v(OMAP34XX_GPMC_BASE);
+               gpmc_base = IO_ADDRESS(OMAP34XX_GPMC_BASE);
       }
 
       BUG_ON(IS_ERR(gpmc_l3_clk));

arch/arm/mach-omap2/irq.c +6 -6

index 82e954a..7014505 100644
@@ -38,7 +38,7 @@
 * for each bank.. when in doubt, consult the TRM.
 */
 static struct omap_irq_bank {
-       unsigned long base_reg;
+       void __iomem *base_reg;
       unsigned int nr_irqs;
 } __attribute__ ((aligned(4))) irq_banks[] = {
       {

@@ -52,12 +52,12 @@ static struct omap_irq_bank {
 
 static void intc_bank_write_reg(u32 val, struct omap_irq_bank *bank, u16 reg)
 {
-       __raw_writel(val, (__force void __iomem *)(bank->base_reg + reg));
+       __raw_writel(val, bank->base_reg + reg);
 }
 
 static u32 intc_bank_read_reg(struct omap_irq_bank *bank, u16 reg)
 {
-       return __raw_readl((__force void __iomem *)(bank->base_reg + reg));
+       return __raw_readl(bank->base_reg + reg);
 }
 
 /* XXX: FIQ and additional INTC support (only MPU at the moment) */

@@ -102,7 +102,7 @@ static void __init omap_irq_bank_init_one(struct omap_irq_bank *bank)
       unsigned long tmp;
 
       tmp = intc_bank_read_reg(bank, INTC_REVISION) & 0xff;
-       printk(KERN_INFO "IRQ: Found an INTC at 0x%08lx "
+       printk(KERN_INFO "IRQ: Found an INTC at 0x%p "
                        "(revision %ld.%ld) with %d interrupts\n",
                        bank->base_reg, tmp >> 4, tmp & 0xf, bank->nr_irqs);
 

@@ -147,9 +147,9 @@ void __init omap_init_irq(void)
               struct omap_irq_bank *bank = irq_banks + i;
 
               if (cpu_is_omap24xx())
-                       bank->base_reg = io_p2v(OMAP24XX_IC_BASE);
+                       bank->base_reg = IO_ADDRESS(OMAP24XX_IC_BASE);
               else if (cpu_is_omap34xx())
-                       bank->base_reg = io_p2v(OMAP34XX_IC_BASE);
+                       bank->base_reg = IO_ADDRESS(OMAP34XX_IC_BASE);
 
               omap_irq_bank_init_one(bank);
 

arch/arm/mach-omap2/mcbsp.c +7 -7

index cedb9a6..bf6f413 100644
@@ -148,7 +148,7 @@ static struct omap_mcbsp_ops omap2_mcbsp_ops = {
 #ifdef CONFIG_ARCH_OMAP24XX
 static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
       {
-               .virt_base      = OMAP2_IO_ADDRESS(OMAP24XX_MCBSP1_BASE),
+               .virt_base      = IO_ADDRESS(OMAP24XX_MCBSP1_BASE),
               .dma_rx_sync    = OMAP24XX_DMA_MCBSP1_RX,
               .dma_tx_sync    = OMAP24XX_DMA_MCBSP1_TX,
               .rx_irq         = INT_24XX_MCBSP1_IRQ_RX,

@@ -157,7 +157,7 @@ static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
               .clk_name       = "mcbsp_clk",
       },
       {
-               .virt_base      = OMAP2_IO_ADDRESS(OMAP24XX_MCBSP2_BASE),
+               .virt_base      = IO_ADDRESS(OMAP24XX_MCBSP2_BASE),
               .dma_rx_sync    = OMAP24XX_DMA_MCBSP2_RX,
               .dma_tx_sync    = OMAP24XX_DMA_MCBSP2_TX,
               .rx_irq         = INT_24XX_MCBSP2_IRQ_RX,

@@ -175,7 +175,7 @@ static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
 #ifdef CONFIG_ARCH_OMAP34XX
 static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
       {
-               .virt_base      = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP1_BASE),
+               .virt_base      = IO_ADDRESS(OMAP34XX_MCBSP1_BASE),
               .dma_rx_sync    = OMAP24XX_DMA_MCBSP1_RX,
               .dma_tx_sync    = OMAP24XX_DMA_MCBSP1_TX,
               .rx_irq         = INT_24XX_MCBSP1_IRQ_RX,

@@ -184,7 +184,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
               .clk_name       = "mcbsp_clk",
       },
       {
-               .virt_base      = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP2_BASE),
+               .virt_base      = IO_ADDRESS(OMAP34XX_MCBSP2_BASE),
               .dma_rx_sync    = OMAP24XX_DMA_MCBSP2_RX,
               .dma_tx_sync    = OMAP24XX_DMA_MCBSP2_TX,
               .rx_irq         = INT_24XX_MCBSP2_IRQ_RX,

@@ -193,21 +193,21 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
               .clk_name       = "mcbsp_clk",
       },
       {
-               .virt_base      = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP3_BASE),
+               .virt_base      = IO_ADDRESS(OMAP34XX_MCBSP3_BASE),
               .dma_rx_sync    = OMAP24XX_DMA_MCBSP3_RX,
               .dma_tx_sync    = OMAP24XX_DMA_MCBSP3_TX,
               .ops            = &omap2_mcbsp_ops,
               .clk_name       = "mcbsp_clk",
       },
       {
-               .virt_base      = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP4_BASE),
+               .virt_base      = IO_ADDRESS(OMAP34XX_MCBSP4_BASE),
               .dma_rx_sync    = OMAP24XX_DMA_MCBSP4_RX,
               .dma_tx_sync    = OMAP24XX_DMA_MCBSP4_TX,
               .ops            = &omap2_mcbsp_ops,
               .clk_name       = "mcbsp_clk",
       },
       {
-               .virt_base      = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP5_BASE),
+               .virt_base      = IO_ADDRESS(OMAP34XX_MCBSP5_BASE),
               .dma_rx_sync    = OMAP24XX_DMA_MCBSP5_RX,
               .dma_tx_sync    = OMAP24XX_DMA_MCBSP5_TX,
               .ops            = &omap2_mcbsp_ops,

arch/arm/mach-omap2/serial.c +6 -6

index bd6f8c9..dbbef90 100644
@@ -28,24 +28,24 @@ static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
 
 static struct plat_serial8250_port serial_platform_data[] = {
       {
-               .membase        = (void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),
-               .mapbase        = (unsigned long)OMAP_UART1_BASE,
+               .membase        = IO_ADDRESS(OMAP_UART1_BASE),
+               .mapbase        = OMAP_UART1_BASE,
               .irq            = 72,
               .flags          = UPF_BOOT_AUTOCONF,
               .iotype         = UPIO_MEM,
               .regshift       = 2,
               .uartclk        = OMAP24XX_BASE_BAUD * 16,
       }, {
-               .membase        = (void __iomem *)IO_ADDRESS(OMAP_UART2_BASE),
-               .mapbase        = (unsigned long)OMAP_UART2_BASE,
+               .membase        = IO_ADDRESS(OMAP_UART2_BASE),
+               .mapbase        = OMAP_UART2_BASE,
               .irq            = 73,
               .flags          = UPF_BOOT_AUTOCONF,
               .iotype         = UPIO_MEM,
               .regshift       = 2,
               .uartclk        = OMAP24XX_BASE_BAUD * 16,
       }, {
-               .membase        = (void __iomem *)IO_ADDRESS(OMAP_UART3_BASE),
-               .mapbase        = (unsigned long)OMAP_UART3_BASE,
+               .membase        = IO_ADDRESS(OMAP_UART3_BASE),
+               .mapbase        = OMAP_UART3_BASE,
               .irq            = 74,
               .flags          = UPF_BOOT_AUTOCONF,
               .iotype         = UPIO_MEM,

arch/arm/plat-omap/common.c +18 -18

index 2914af0..3cf726b 100644
@@ -281,12 +281,12 @@ static void __init __omap2_set_globals(void)
 
 static struct omap_globals omap242x_globals = {
       .class  = OMAP242X_CLASS,
-       .tap    = (__force void __iomem *)OMAP2_IO_ADDRESS(0x48014000),
-       .sdrc   = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_SDRC_BASE),
-       .sms    = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_SMS_BASE),
-       .ctrl   = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_CTRL_BASE),
-       .prm    = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_PRM_BASE),
-       .cm     = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_CM_BASE),
+       .tap    = IO_ADDRESS(0x48014000),
+       .sdrc   = IO_ADDRESS(OMAP2420_SDRC_BASE),
+       .sms    = IO_ADDRESS(OMAP2420_SMS_BASE),
+       .ctrl   = IO_ADDRESS(OMAP2420_CTRL_BASE),
+       .prm    = IO_ADDRESS(OMAP2420_PRM_BASE),
+       .cm     = IO_ADDRESS(OMAP2420_CM_BASE),
 };
 
 void __init omap2_set_globals_242x(void)

@@ -300,12 +300,12 @@ void __init omap2_set_globals_242x(void)
 
 static struct omap_globals omap243x_globals = {
       .class  = OMAP243X_CLASS,
-       .tap    = (__force void __iomem *)OMAP2_IO_ADDRESS(0x4900a000),
-       .sdrc   = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_SDRC_BASE),
-       .sms    = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_SMS_BASE),
-       .ctrl   = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_CTRL_BASE),
-       .prm    = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2430_PRM_BASE),
-       .cm     = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2430_CM_BASE),
+       .tap    = IO_ADDRESS(0x4900a000),
+       .sdrc   = IO_ADDRESS(OMAP243X_SDRC_BASE),
+       .sms    = IO_ADDRESS(OMAP243X_SMS_BASE),
+       .ctrl   = IO_ADDRESS(OMAP243X_CTRL_BASE),
+       .prm    = IO_ADDRESS(OMAP2430_PRM_BASE),
+       .cm     = IO_ADDRESS(OMAP2430_CM_BASE),
 };
 
 void __init omap2_set_globals_243x(void)

@@ -319,12 +319,12 @@ void __init omap2_set_globals_243x(void)
 
 static struct omap_globals omap343x_globals = {
       .class  = OMAP343X_CLASS,
-       .tap    = (__force void __iomem *)OMAP2_IO_ADDRESS(0x4830A000),
-       .sdrc   = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_SDRC_BASE),
-       .sms    = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_SMS_BASE),
-       .ctrl   = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_CTRL_BASE),
-       .prm    = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP3430_PRM_BASE),
-       .cm     = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP3430_CM_BASE),
+       .tap    = IO_ADDRESS(0x4830A000),
+       .sdrc   = IO_ADDRESS(OMAP343X_SDRC_BASE),
+       .sms    = IO_ADDRESS(OMAP343X_SMS_BASE),
+       .ctrl   = IO_ADDRESS(OMAP343X_CTRL_BASE),
+       .prm    = IO_ADDRESS(OMAP3430_PRM_BASE),
+       .cm     = IO_ADDRESS(OMAP3430_CM_BASE),
 };
 
 void __init omap2_set_globals_343x(void)

arch/arm/plat-omap/dma.c +3 -3

index 1c6b905..bb3f187 100644
@@ -2298,13 +2298,13 @@ static int __init omap_init_dma(void)
       int ch, r;
 
       if (cpu_class_is_omap1()) {
-               omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP1_DMA_BASE);
+               omap_dma_base = IO_ADDRESS(OMAP1_DMA_BASE);
               dma_lch_count = OMAP1_LOGICAL_DMA_CH_COUNT;
       } else if (cpu_is_omap24xx()) {
-               omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP24XX_DMA4_BASE);
+               omap_dma_base = IO_ADDRESS(OMAP24XX_DMA4_BASE);
               dma_lch_count = OMAP_DMA4_LOGICAL_DMA_CH_COUNT;
       } else if (cpu_is_omap34xx()) {
-               omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP34XX_DMA4_BASE);
+               omap_dma_base = IO_ADDRESS(OMAP34XX_DMA4_BASE);
               dma_lch_count = OMAP_DMA4_LOGICAL_DMA_CH_COUNT;
       } else {
               pr_err("DMA init failed for unsupported omap\n");

arch/arm/plat-omap/dmtimer.c +1 -1

index 8b4b553..167ec2f 100644
@@ -693,7 +693,7 @@ int __init omap_dm_timer_init(void)
 
       for (i = 0; i < dm_timer_count; i++) {
               timer = &dm_timers[i];
-               timer->io_base = (void __iomem *)io_p2v(timer->phys_base);
+               timer->io_base = IO_ADDRESS(timer->phys_base);
 #if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
               if (cpu_class_is_omap2()) {
                       char clk_name[16];

arch/arm/plat-omap/gpio.c +1 -1

index 3e76ee2..dee380b 100644
@@ -1488,7 +1488,7 @@ static int __init _omap_gpio_init(void)
               bank->chip.set = gpio_set;
               if (bank_is_mpuio(bank)) {
                       bank->chip.label = "mpuio";
-#ifdef CONFIG_ARCH_OMAP1
+#ifdef CONFIG_ARCH_OMAP16XX /* CHECKME */
                       bank->chip.dev = &omap_mpuio_device.dev;
 #endif
                       bank->chip.base = OMAP_MPUIO(0);

arch/arm/plat-omap/include/mach/control.h +0 -9

index 1cd98fd..91d7480 100644
@@ -18,18 +18,9 @@
 
 #include <mach/io.h>
 
-#ifndef __ASSEMBLY__
-#define OMAP242X_CTRL_REGADDR(reg)                                     \
-       (void __iomem *)IO_ADDRESS(OMAP242X_CTRL_BASE + (reg))
-#define OMAP243X_CTRL_REGADDR(reg)                                     \
-       (void __iomem *)IO_ADDRESS(OMAP243X_CTRL_BASE + (reg))
-#define OMAP343X_CTRL_REGADDR(reg)                                     \
-       (void __iomem *)IO_ADDRESS(OMAP343X_CTRL_BASE + (reg))
-#else
 #define OMAP242X_CTRL_REGADDR(reg)     IO_ADDRESS(OMAP242X_CTRL_BASE + (reg))
 #define OMAP243X_CTRL_REGADDR(reg)     IO_ADDRESS(OMAP243X_CTRL_BASE + (reg))
 #define OMAP343X_CTRL_REGADDR(reg)     IO_ADDRESS(OMAP343X_CTRL_BASE + (reg))
-#endif /* __ASSEMBLY__ */
 
 /*
 * As elsewhere, the "OMAP2_" prefix indicates that the macro is valid for

arch/arm/plat-omap/include/mach/io.h +17 -17

index 7d3481b..5126a0a 100644
@@ -55,14 +55,12 @@
 
 #if defined(CONFIG_ARCH_OMAP1)
 
-#define IO_PHYS                0xFFFB0000
-#define IO_OFFSET      0x01000000      /* Virtual IO = 0xfefb0000 */
-#define IO_SIZE                0x40000
-#define IO_VIRT                (IO_PHYS - IO_OFFSET)
-#define IO_ADDRESS(pa) ((pa) - IO_OFFSET)
-#define OMAP1_IO_ADDRESS(pa)   ((pa) - IO_OFFSET)
-#define io_p2v(pa)     ((pa) - IO_OFFSET)
-#define io_v2p(va)     ((va) + IO_OFFSET)
+#define IO_PHYS                        0xFFFB0000
+#define IO_OFFSET              0x01000000      /* Virtual IO = 0xfefb0000 */
+#define IO_SIZE                        0x40000
+#define IO_VIRT                        (IO_PHYS - IO_OFFSET)
+#define __IO_ADDRESS(pa)       ((pa) - IO_OFFSET)
+#define io_v2p(va)             ((va) + IO_OFFSET)
 
 #elif defined(CONFIG_ARCH_OMAP2)
 

@@ -90,11 +88,9 @@
 
 #endif
 
-#define IO_OFFSET      0x90000000
-#define IO_ADDRESS(pa) ((pa) + IO_OFFSET)      /* Works for L3 and L4 */
-#define OMAP2_IO_ADDRESS(pa)   ((pa) + IO_OFFSET)      /* Works for L3 and L4 */
-#define io_p2v(pa)     ((pa) + IO_OFFSET)      /* Works for L3 and L4 */
-#define io_v2p(va)     ((va) - IO_OFFSET)      /* Works for L3 and L4 */
+#define IO_OFFSET              0x90000000
+#define __IO_ADDRESS(pa)       ((pa) + IO_OFFSET)      /* Works for L3 and L4 */
+#define io_v2p(va)             ((va) - IO_OFFSET)      /* Works for L3 and L4 */
 
 /* DSP */
 #define DSP_MEM_24XX_PHYS      OMAP2420_DSP_MEM_BASE   /* 0x58000000 */

@@ -149,9 +145,7 @@
 
 
 #define IO_OFFSET              0x90000000
-#define IO_ADDRESS(pa)         ((pa) + IO_OFFSET)/* Works for L3 and L4 */
-#define OMAP2_IO_ADDRESS(pa)   ((pa) + IO_OFFSET)/* Works for L3 and L4 */
-#define io_p2v(pa)             ((pa) + IO_OFFSET)/* Works for L3 and L4 */
+#define __IO_ADDRESS(pa)       ((pa) + IO_OFFSET)/* Works for L3 and L4 */
 #define io_v2p(va)             ((va) - IO_OFFSET)/* Works for L3 and L4 */
 
 /* DSP */

@@ -167,7 +161,13 @@
 
 #endif
 
-#ifndef __ASSEMBLER__
+#ifdef __ASSEMBLER__
+
+#define IO_ADDRESS(pa)         __IO_ADDRESS(pa)
+
+#else
+
+#define IO_ADDRESS(pa)         ((void __iomem *)__IO_ADDRESS(pa))
 
 /*
 * Functions to access the OMAP IO region

arch/arm/plat-omap/include/mach/mcbsp.h +2 -2

index c04bcd5..3c02bf6 100644
@@ -326,7 +326,7 @@ struct omap_mcbsp_ops {
 };
 
 struct omap_mcbsp_platform_data {
-       u32 virt_base;
+       void __iomem *virt_base;
       u8 dma_rx_sync, dma_tx_sync;
       u16 rx_irq, tx_irq;
       struct omap_mcbsp_ops *ops;

@@ -335,7 +335,7 @@ struct omap_mcbsp_platform_data {
 
 struct omap_mcbsp {
       struct device *dev;
-       u32 io_base;
+       void __iomem *io_base;
       u8 id;
       u8 free;
       omap_mcbsp_word_length rx_word_length;

arch/arm/plat-omap/include/mach/pm.h +2 -2

index 5a1f3db..e4cf1c5 100644
@@ -39,11 +39,11 @@
 * Register and offset definitions to be used in PM assembler code
 * ----------------------------------------------------------------------------
 */
-#define CLKGEN_REG_ASM_BASE            io_p2v(0xfffece00)
+#define CLKGEN_REG_ASM_BASE            IO_ADDRESS(0xfffece00)
 #define ARM_IDLECT1_ASM_OFFSET         0x04
 #define ARM_IDLECT2_ASM_OFFSET         0x08
 
-#define TCMIF_ASM_BASE                 io_p2v(0xfffecc00)
+#define TCMIF_ASM_BASE                 IO_ADDRESS(0xfffecc00)
 #define EMIFS_CONFIG_ASM_OFFSET                0x0c
 #define EMIFF_SDRAM_CONFIG_ASM_OFFSET  0x20
 

arch/arm/plat-omap/include/mach/sdrc.h +3 -6

index b7db862..c66c838 100644
@@ -75,12 +75,9 @@
 * SMS register access
 */
 
-#define OMAP242X_SMS_REGADDR(reg)                                      \
-                       (void __iomem *)IO_ADDRESS(OMAP2420_SMS_BASE + reg)
-#define OMAP243X_SMS_REGADDR(reg)                                      \
-                       (void __iomem *)IO_ADDRESS(OMAP243X_SMS_BASE + reg)
-#define OMAP343X_SMS_REGADDR(reg)                                      \
-                       (void __iomem *)IO_ADDRESS(OMAP343X_SMS_BASE + reg)
+#define OMAP242X_SMS_REGADDR(reg)      IO_ADDRESS(OMAP2420_SMS_BASE + reg)
+#define OMAP243X_SMS_REGADDR(reg)      IO_ADDRESS(OMAP243X_SMS_BASE + reg)
+#define OMAP343X_SMS_REGADDR(reg)      IO_ADDRESS(OMAP343X_SMS_BASE + reg)
 
 /* SMS register offsets - read/write with sms_{read,write}_reg() */
 

arch/arm/plat-omap/include/mach/serial.h +3 -3

index abea6ef..c6b7f59 100644
@@ -33,9 +33,9 @@
 #define OMAP24XX_BASE_BAUD     (48000000/16)
 
 #define is_omap_port(p)        ({int __ret = 0;                        \
-                       if (p == IO_ADDRESS(OMAP_UART1_BASE) || \
-                           p == IO_ADDRESS(OMAP_UART2_BASE) || \
-                           p == IO_ADDRESS(OMAP_UART3_BASE))   \
+                       if (p == OMAP_UART1_BASE ||             \
+                           p == OMAP_UART2_BASE ||             \
+                           p == OMAP_UART3_BASE)               \
                               __ret = 1;                      \
                       __ret;                                  \
                       })

arch/arm/plat-omap/mcbsp.c +13 -12

index 0f0e3f3..51d715f 100644
@@ -30,7 +30,7 @@
 struct omap_mcbsp **mcbsp_ptr;
 int omap_mcbsp_count;
 
-void omap_mcbsp_write(u32 io_base, u16 reg, u32 val)
+void omap_mcbsp_write(void __iomem *io_base, u16 reg, u32 val)
 {
       if (cpu_class_is_omap1() || cpu_is_omap2420())
               __raw_writew((u16)val, io_base + reg);

@@ -38,7 +38,7 @@ void omap_mcbsp_write(u32 io_base, u16 reg, u32 val)
               __raw_writel(val, io_base + reg);
 }
 
-int omap_mcbsp_read(u32 io_base, u16 reg)
+int omap_mcbsp_read(void __iomem *io_base, u16 reg)
 {
       if (cpu_class_is_omap1() || cpu_is_omap2420())
               return __raw_readw(io_base + reg);

@@ -149,7 +149,7 @@ static void omap_mcbsp_rx_dma_callback(int lch, u16 ch_status, void *data)
 void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config)
 {
       struct omap_mcbsp *mcbsp;
-       u32 io_base;
+       void __iomem *io_base;
 
       if (!omap_mcbsp_check_valid_id(id)) {
               printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);

@@ -158,7 +158,7 @@ void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config)
       mcbsp = id_to_mcbsp_ptr(id);
 
       io_base = mcbsp->io_base;
-       dev_dbg(mcbsp->dev, "Configuring McBSP%d  io_base: 0x%8x\n",
+       dev_dbg(mcbsp->dev, "Configuring McBSP%d  io_base: 0x%p\n",
                       mcbsp->id, io_base);
 
       /* We write the given config */

@@ -306,7 +306,7 @@ EXPORT_SYMBOL(omap_mcbsp_free);
 void omap_mcbsp_start(unsigned int id)
 {
       struct omap_mcbsp *mcbsp;
-       u32 io_base;
+       void __iomem *io_base;
       u16 w;
 
       if (!omap_mcbsp_check_valid_id(id)) {

@@ -344,7 +344,7 @@ EXPORT_SYMBOL(omap_mcbsp_start);
 void omap_mcbsp_stop(unsigned int id)
 {
       struct omap_mcbsp *mcbsp;
-       u32 io_base;
+       void __iomem *io_base;
       u16 w;
 
       if (!omap_mcbsp_check_valid_id(id)) {

@@ -373,7 +373,7 @@ EXPORT_SYMBOL(omap_mcbsp_stop);
 int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
 {
       struct omap_mcbsp *mcbsp;
-       u32 base;
+       void __iomem *base;
 
       if (!omap_mcbsp_check_valid_id(id)) {
               printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);

@@ -418,7 +418,7 @@ EXPORT_SYMBOL(omap_mcbsp_pollwrite);
 int omap_mcbsp_pollread(unsigned int id, u16 *buf)
 {
       struct omap_mcbsp *mcbsp;
-       u32 base;
+       void __iomem *base;
 
       if (!omap_mcbsp_check_valid_id(id)) {
               printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);

@@ -465,7 +465,7 @@ EXPORT_SYMBOL(omap_mcbsp_pollread);
 void omap_mcbsp_xmit_word(unsigned int id, u32 word)
 {
       struct omap_mcbsp *mcbsp;
-       u32 io_base;
+       void __iomem *io_base;
       omap_mcbsp_word_length word_length;
 
       if (!omap_mcbsp_check_valid_id(id)) {

@@ -488,7 +488,7 @@ EXPORT_SYMBOL(omap_mcbsp_xmit_word);
 u32 omap_mcbsp_recv_word(unsigned int id)
 {
       struct omap_mcbsp *mcbsp;
-       u32 io_base;
+       void __iomem *io_base;
       u16 word_lsb, word_msb = 0;
       omap_mcbsp_word_length word_length;
 

@@ -514,7 +514,7 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word);
 int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word)
 {
       struct omap_mcbsp *mcbsp;
-       u32 io_base;
+       void __iomem *io_base;
       omap_mcbsp_word_length tx_word_length;
       omap_mcbsp_word_length rx_word_length;
       u16 spcr2, spcr1, attempts = 0, word_lsb, word_msb = 0;

@@ -580,7 +580,8 @@ EXPORT_SYMBOL(omap_mcbsp_spi_master_xmit_word_poll);
 int omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 *word)
 {
       struct omap_mcbsp *mcbsp;
-       u32 io_base, clock_word = 0;
+       u32 clock_word = 0;
+       void __iomem *io_base;
       omap_mcbsp_word_length tx_word_length;
       omap_mcbsp_word_length rx_word_length;
       u16 spcr2, spcr1, attempts = 0, word_lsb, word_msb = 0;

arch/arm/plat-omap/mcbsp.c +0 -0

                               0, 0);
 

drivers/char/hw_random/omap-rng.c +1 -1

index 154a21f..668bf7d 100644
@@ -122,7 +122,7 @@ static int __init omap_rng_probe(struct platform_device *pdev)
               return -EBUSY;
 
       dev_set_drvdata(&pdev->dev, mem);
-       rng_base = (u32 __force __iomem *)io_p2v(res->start);
+       rng_base = IO_ADDRESS(res->start); /* WBNI: why not ioremap? */
 
       ret = hwrng_register(&omap_rng_ops);
       if (ret) {

drivers/i2c/busses/i2c-omap.c +1 -1

index 3c4e581..978c072 100644
@@ -757,7 +757,7 @@ omap_i2c_probe(struct platform_device *pdev)
       dev->speed = *speed;
       dev->dev = &pdev->dev;
       dev->irq = irq->start;
-       dev->base = (void __iomem *) IO_ADDRESS(mem->start);
+       dev->base = IO_ADDRESS(mem->start); /* WBNI: why not ioremap? */
       platform_set_drvdata(pdev, dev);
 
       if ((r = omap_i2c_get_clocks(dev)) != 0)

drivers/mmc/host/omap.c +1 -1

index c160288..802877c 100644
@@ -1455,7 +1455,7 @@ static int __init mmc_omap_probe(struct platform_device *pdev)
 
       host->irq = irq;
       host->phys_base = host->mem_res->start;
-       host->virt_base = (void __iomem *) IO_ADDRESS(host->phys_base);
+       host->virt_base = IO_ADDRESS(host->phys_base); /* WBNI: why not ioremap? */
 
       if (cpu_is_omap24xx()) {
               host->iclk = clk_get(&pdev->dev, "mmc_ick");

drivers/serial/8250.c +2 -2

index 5a5f95f..2ba5cbb 100644
@@ -2209,7 +2209,7 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 
 #ifdef CONFIG_ARCH_OMAP15XX
       /* Workaround to enable 115200 baud on OMAP1510 internal ports */
-       if (cpu_is_omap1510() && is_omap_port((unsigned int)up->port.membase)) {
+       if (cpu_is_omap1510() && is_omap_port(up->port.mapbase)) {
               if (baud == 115200) {
                       quot = 1;
                       serial_out(up, UART_OMAP_OSC_12M_SEL, 1);

@@ -2284,7 +2284,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
       int ret = 0;
 
 #ifdef CONFIG_ARCH_OMAP
-       if (is_omap_port((unsigned int)up->port.membase))
+       if (is_omap_port(up->port.mapbase))
               size = 0x16 << up->port.regshift;
 #endif
 

drivers/spi/omap2_mcspi.c +6 -2

index 9d2186f..8178665 100644
@@ -119,12 +119,14 @@ struct omap2_mcspi {
       struct clk              *fck;
       /* Virtual base address of the controller */
       void __iomem            *base;
+       unsigned long           phys;
       /* SPI1 has 4 channels, while SPI2 has 2 */
       struct omap2_mcspi_dma  *dma_channels;
 };
 
 struct omap2_mcspi_cs {
       void __iomem            *base;
+       unsigned long           phys;
       int                     word_len;
 };
 

@@ -233,7 +235,7 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
       c = count;
       word_len = cs->word_len;
 
-       base = (unsigned long) io_v2p(cs->base);
+       base = cs->phys;
       tx_reg = base + OMAP2_MCSPI_TX0;
       rx_reg = base + OMAP2_MCSPI_RX0;
       rx = xfer->rx_buf;

@@ -633,6 +635,7 @@ static int omap2_mcspi_setup(struct spi_device *spi)
               if (!cs)
                       return -ENOMEM;
               cs->base = mcspi->base + spi->chip_select * 0x14;
+               cs->phys = mcspi->phys + spi->chip_select * 0x14;
               spi->controller_state = cs;
       }
 

@@ -1005,7 +1008,8 @@ static int __init omap2_mcspi_probe(struct platform_device *pdev)
               goto err1;
       }
 
-       mcspi->base = (void __iomem *) io_p2v(r->start);
+       mcspi->phys = r->start;
+       mcspi->base = IO_ADDRESS(r->start); /* WBNI: why not ioremap? */
 
       INIT_WORK(&mcspi->work, omap2_mcspi_work);
 

drivers/spi/omap_uwire.c +1 -1

index 5515eb9..3166635 100644
@@ -59,7 +59,7 @@
 * and irqs should show there too...
 */
 #define UWIRE_BASE_PHYS                0xFFFB3000
-#define UWIRE_BASE             ((void *__iomem)IO_ADDRESS(UWIRE_BASE_PHYS))
+#define UWIRE_BASE             IO_ADDRESS(UWIRE_BASE_PHYS)
 
 /* uWire Registers: */
 #define UWIRE_IO_SIZE 0x20

drivers/usb/gadget/omap_udc.c +2 -0

index a2638ee..7032975 100644
@@ -786,6 +786,7 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
                       omap_set_dma_dest_params(ep->lch,
                               OMAP_DMA_PORT_TIPB,
                               OMAP_DMA_AMODE_CONSTANT,
+                               /* FIXME: aren't these already physical addresses */
                               (unsigned long) io_v2p(UDC_DATA_DMA),
                               0, 0);
               }

@@ -803,6 +804,7 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
                       omap_set_dma_src_params(ep->lch,
                               OMAP_DMA_PORT_TIPB,
                               OMAP_DMA_AMODE_CONSTANT,
+                               /* FIXME: aren't these already physical addresses */
                               (unsigned long) io_v2p(UDC_DATA_DMA),
                               0, 0);
                       /* EMIFF or SDRC */

drivers/usb/host/ohci-omap.c +2 -2

index 71fb9fd..c2fa621 100644
@@ -208,7 +208,7 @@ static int ohci_omap_init(struct usb_hcd *hcd)
       if (cpu_is_omap16xx())
               ocpi_enable();
 
-#ifdef CONFIG_ARCH_OMAP_OTG
+#ifdef CONFIG_USB_OTG /* CHECKME */
       if (need_transceiver) {
               ohci->transceiver = otg_get_transceiver();
               if (ohci->transceiver) {

@@ -343,7 +343,7 @@ static int usb_hcd_omap_probe (const struct hc_driver *driver,
               goto err1;
       }
 
-       hcd->regs = (void __iomem *) (int) IO_ADDRESS(hcd->rsrc_start);
+       hcd->regs = IO_ADDRESS(hcd->rsrc_start); /* WBNI: why not ioremap? */
 
       ohci = hcd_to_ohci(hcd);
       ohci_hcd_init(ohci);

drivers/video/omap/dispc.c +5 -5

index 00ad6b2..3e802b7 100644
@@ -156,7 +156,7 @@ struct resmap {
 };
 
 static struct {
-       u32             base;
+       void __iomem    *base;
 
       struct omapfb_mem_desc  mem_desc;
       struct resmap           *res_map[DISPC_MEMTYPE_NUM];

@@ -212,9 +212,9 @@ static void enable_rfbi_mode(int enable)
       dispc_write_reg(DISPC_CONTROL, l);
 
       /* Set bypass mode in RFBI module */
-       l = __raw_readl(io_p2v(RFBI_CONTROL));
+       l = __raw_readl(IO_ADDRESS(RFBI_CONTROL));
       l |= enable ? 0 : (1 << 1);
-       __raw_writel(l, io_p2v(RFBI_CONTROL));
+       __raw_writel(l, IO_ADDRESS(RFBI_CONTROL));
 }
 
 static void set_lcd_data_lines(int data_lines)

@@ -1353,7 +1353,7 @@ static int omap_dispc_init(struct omapfb_device *fbdev, int ext_mode,
 
       memset(&dispc, 0, sizeof(dispc));
 
-       dispc.base = io_p2v(DISPC_BASE);
+       dispc.base = IO_ADDRESS(DISPC_BASE);
       dispc.fbdev = fbdev;
       dispc.ext_mode = ext_mode;
 

@@ -1417,7 +1417,7 @@ static int omap_dispc_init(struct omapfb_device *fbdev, int ext_mode,
       }
 
       /* L3 firewall setting: enable access to OCM RAM */
-       __raw_writel(0x402000b0, io_p2v(0x680050a0));
+       __raw_writel(0x402000b0, IO_ADDRESS(0x680050a0));
 
       if ((r = alloc_palette_ram()) < 0)
               goto fail2;

drivers/video/omap/rfbi.c +2 -2

index 470920d..59f857e 100644
@@ -59,7 +59,7 @@
 #define DISPC_CONTROL          0x0040
 
 static struct {
-       u32             base;
+       void __iomem    *base;
       void            (*lcdc_callback)(void *data);
       void            *lcdc_callback_data;
       unsigned long   l4_khz;

@@ -518,7 +518,7 @@ static int rfbi_init(struct omapfb_device *fbdev)
       int r;
 
       rfbi.fbdev = fbdev;
-       rfbi.base = io_p2v(RFBI_BASE);
+       rfbi.base = IO_ADDRESS(RFBI_BASE);
 
       if ((r = rfbi_get_clocks()) < 0)
               return r;

drivers/video/omap/sossi.c +1 -2

index b4f67fc..9839d35 100644
@@ -574,7 +574,7 @@ static int sossi_init(struct omapfb_device *fbdev)
       struct clk *dpll1out_ck;
       int r;
 
-       sossi.base = (void __iomem *)IO_ADDRESS(OMAP_SOSSI_BASE);
+       sossi.base = IO_ADDRESS(OMAP_SOSSI_BASE);
       sossi.fbdev = fbdev;
       spin_lock_init(&sossi.lock);
 
 
 
 
--

Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

arch/arm/plat-omap/mcbsp.c int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer, · line 706

Quoted 6 lines

arch/arm/plat-omap/mcbsp.c +1 -0

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

Quoted 10 lines · Eduardo Valentin

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

Quoted 8 lines

arch/arm/plat-omap/mcbsp.c int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer, · line 706

Quoted 6 lines

arch/arm/plat-omap/mcbsp.c +1 -0

@@ -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?

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

Hi,

Quoted 23 lines · Russell King

Quoted 10 lines · Eduardo Valentin

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

Quoted 8 lines

arch/arm/plat-omap/mcbsp.c int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer, · line 706

Quoted 6 lines

arch/arm/plat-omap/mcbsp.c +1 -0

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

Quoted 2 lines

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.

Quoted 26 lines · Eduardo Valentin

Hi,

Quoted 23 lines · Russell King

Quoted 10 lines · Eduardo Valentin

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

Quoted 8 lines

arch/arm/plat-omap/mcbsp.c int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer, · line 706

Quoted 6 lines

arch/arm/plat-omap/mcbsp.c +1 -0

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

Quoted 7 lines

Quoted 2 lines

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.

Do you see what I'm getting at now?

Hi Russell,

Quoted 14 lines · Russell King - ARM Linux

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.

Quoted 1 line

Do you see what I'm getting at now?

Ok. I got your point. I took a look at it and it expects a virtual address on its 4th argument.

I uses it with dma_write() macro, which uses it with __raw_write() (arch/arm/plat-omap/dma.c).

Quoted 18 lines · Eduardo Valentin

Hi Russell,

Quoted 14 lines · Russell King - ARM Linux

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.

Quoted 4 lines

Quoted 1 line

Do you see what I'm getting at now?

Ok. I got your point. I took a look at it and it expects a virtual address on its 4th argument.

I'm sorry, but I don't think you have.

Quoted 2 lines

I uses it with dma_write() macro, which uses it with __raw_write() (arch/arm/plat-omap/dma.c).

That's true, but probably not in the way which would require it to be a virtual address that you're thinking.

void omap_set_dma_dest_params(int lch, int dest_port, int dest_amode,
unsigned long dest_start, int dst_ei, int dst_fi)

{
        if (cpu_class_is_omap1()) {
                dma_write(dest_start >> 16, CDSA_U(lch));
                dma_write(dest_start, CDSA_L(lch));
        }

        if (cpu_class_is_omap2())
                dma_write(dest_start, CDSA(lch));
}

'dest_start' is a value to be written to the hardware registers in the DMA controller. So, the question now is: do the CDSA registers expect a virtual or a physical address to be written to them?

I would look up this CDSA register in the OMAP manuals, if they were accessible... Are they not publically available? (Google isn't helping, neither is searching on ti.com...)

Quoted 45 lines · Russell King - ARM Linux

Quoted 18 lines · Eduardo Valentin

Hi Russell,

Quoted 14 lines · Russell King - ARM Linux

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.

Quoted 4 lines

Quoted 1 line

Do you see what I'm getting at now?

Ok. I got your point. I took a look at it and it expects a virtual address on its 4th argument.

I'm sorry, but I don't think you have.

Quoted 2 lines

I uses it with dma_write() macro, which uses it with __raw_write() (arch/arm/plat-omap/dma.c).

That's true, but probably not in the way which would require it to be a virtual address that you're thinking.

void omap_set_dma_dest_params(int lch, int dest_port, int dest_amode,
unsigned long dest_start, int dst_ei, int dst_fi)

{
        if (cpu_class_is_omap1()) {
                dma_write(dest_start >> 16, CDSA_U(lch));
                dma_write(dest_start, CDSA_L(lch));
        }

        if (cpu_class_is_omap2())
                dma_write(dest_start, CDSA(lch));
}

Quoted 3 lines

'dest_start' is a value to be written to the hardware registers in the DMA controller. So, the question now is: do the CDSA registers expect a virtual or a physical address to be written to them?

C[DS][DS]A register need physical addresses for the DMA. The DMA controller does not know anything about virtual addresses.

Quoted 3 lines

I would look up this CDSA register in the OMAP manuals, if they were accessible... Are they not publically available? (Google isn't helping, neither is searching on ti.com...)

There is a 3450 public TRM somewhere AFAIK, anybody got a link for that? At least 5912 TRM is available somewhere.

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines · Tony Lindgren

There is a 3450 public TRM somewhere AFAIK, anybody got a link for that? At least 5912 TRM is available somewhere.

Sorry I meant 3550 public TRM. Here's the link to the 5912 TRM, which covers 16xx.

http://focus.ti.com/dsp/docs/dspsupporttechdocsc.tsp?abstractName=spru742&sectionId=3&tabId=409

Quoted 8 lines · Tony Lindgren

Quoted 2 lines · Tony Lindgren

There is a 3450 public TRM somewhere AFAIK, anybody got a link for that? At least 5912 TRM is available somewhere.

Sorry I meant 3550 public TRM. Here's the link to the 5912 TRM, which covers 16xx.

http://focus.ti.com/dsp/docs/dspsupporttechdocsc.tsp?abstractName=spru742&sectionId=3&tabId=409

Hmm, claims to only be 106K, and xpdf 3 claims the file is unreadable.

I would look up this CDSA register in the OMAP manuals, if they were accessible... Are they not publically available? (Google isn't helping, neither is searching on ti.com...)

Quoted 26 lines · Eduardo Valentin

Hi,

Quoted 23 lines · Russell King

Quoted 10 lines · Eduardo Valentin

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

Quoted 8 lines

arch/arm/plat-omap/mcbsp.c int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer, · line 706

Quoted 6 lines

arch/arm/plat-omap/mcbsp.c +1 -0

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

Quoted 7 lines

Quoted 2 lines

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.

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

Quoted 8 lines · Russell King - ARM Linux

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.

Quoted 5 lines

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

Quoted 9 lines · Tony Lindgren

Quoted 8 lines · Russell King - ARM Linux

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.

Quoted 8 lines

Quoted 5 lines

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:

arch/arm/plat-omap/mcbsp.c +2 -0

index d084405..d9b1a42 100644
@@ -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.

Hi Russell,

Quoted 31 lines · Russell King - ARM Linux

Quoted 9 lines · Tony Lindgren

Quoted 8 lines · Russell King - ARM Linux

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.

Quoted 8 lines

Quoted 5 lines

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

Quoted 25 lines

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:

arch/arm/plat-omap/mcbsp.c +2 -0

index d084405..d9b1a42 100644
@@ -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);
 

Quoted 2 lines

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.

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 3 lines

I'll cook a patch up.

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

Quoted 6 lines · Russell King - ARM Linux

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

And to complete the puzzle: in the absense of IOMMU technologies what's the difference between a "physical address" and a dma_addr_t? (DMA addreses being integer-like.)

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

Quoted 10 lines · Eduardo Valentin

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

And today, the story is completely different, having looked through more of the code and some documentation.

- OMAPxxxx_MCBSPx_BASE are all physical addresses. Fine.
- physical addresses > 0xfffb0000 are subject to an offset (IO_OFFSET) but others for the DSP located and DSP shared peripherals aren't.

So, applying the IO_OFFSET via IO_ADDRESS() or io_p2v() to all addresses breaks. Meaning it's completely random whether something should be put through IO_ADDRESS() and similar.

This isn't obvious. It isn't readable. It isn't maintainable. It doesn't lend itself to compile time checking.

Quoted 19 lines · Russell King - ARM Linux

Quoted 10 lines · Eduardo Valentin

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

And today, the story is completely different, having looked through more of the code and some documentation.

- OMAPxxxx_MCBSPx_BASE are all physical addresses. Fine.
- physical addresses > 0xfffb0000 are subject to an offset (IO_OFFSET) but others for the DSP located and DSP shared peripherals aren't.

Quoted 6 lines

So, applying the IO_OFFSET via IO_ADDRESS() or io_p2v() to all addresses breaks. Meaning it's completely random whether something should be put through IO_ADDRESS() and similar.

This isn't obvious. It isn't readable. It isn't maintainable. It doesn't lend itself to compile time checking.

Ouch. Looking at the history of the mcbsp.c the MCBSPX_BASE defines have been a mix of virt and phys addresses to start with.

Looks like in the short term we need to define both virt_base and phys_base for omap_mcbsp_platform_data.

Then define __arch_ioremap that understands also the DSP mappings and get rid of the remaining hardcoded virtual DSP defines.

At that point we can also remove the virt_base from omap_mcbsp_platform_data.

Quoted 33 lines · Tony Lindgren

Quoted 19 lines · Russell King - ARM Linux

Quoted 10 lines · Eduardo Valentin

arch/arm/mach-omap1/mcbsp.c static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = { · line 162

Quoted 6 lines

arch/arm/mach-omap1/mcbsp.c +1 -1

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

And today, the story is completely different, having looked through more of the code and some documentation.

- OMAPxxxx_MCBSPx_BASE are all physical addresses. Fine.
- physical addresses > 0xfffb0000 are subject to an offset (IO_OFFSET) but others for the DSP located and DSP shared peripherals aren't.

Quoted 6 lines

So, applying the IO_OFFSET via IO_ADDRESS() or io_p2v() to all addresses breaks. Meaning it's completely random whether something should be put through IO_ADDRESS() and similar.

This isn't obvious. It isn't readable. It isn't maintainable. It doesn't lend itself to compile time checking.

Ouch. Looking at the history of the mcbsp.c the MCBSPX_BASE defines have been a mix of virt and phys addresses to start with.

Looks like in the short term we need to define both virt_base and phys_base for omap_mcbsp_platform_data.

Quoted 4 lines

Then define __arch_ioremap that understands also the DSP mappings and get rid of the remaining hardcoded virtual DSP defines.

At that point we can also remove the virt_base from omap_mcbsp_platform_data.

Already done. ;)

That was fast :) Sounds we'll have a patch to test soon.

Quoted 1 line · Tony Lindgren

That was fast :) Sounds we'll have a patch to test soon.

Of course. In my tree. The master branch contains the minimal fixes, and the devel branch contains everything. If you want the individual patches, either pull the tree or grab the mboxes.

Quoted 6 lines · Russell King - ARM Linux

Quoted 1 line · Tony Lindgren

That was fast :) Sounds we'll have a patch to test soon.

Of course. In my tree. The master branch contains the minimal fixes, and the devel branch contains everything. If you want the individual patches, either pull the tree or grab the mboxes.

Hey looks good! I'll cherry pick them to l-o tree, then let's make sure things work as expected.

Quoted 8 lines · Tony Lindgren

Quoted 6 lines · Russell King - ARM Linux

Quoted 1 line · Tony Lindgren

That was fast :) Sounds we'll have a patch to test soon.

Of course. In my tree. The master branch contains the minimal fixes, and the devel branch contains everything. If you want the individual patches, either pull the tree or grab the mboxes.

Quoted 2 lines

Hey looks good! I'll cherry pick them to l-o tree, then let's make sure things work as expected.

BTW, having an ack (or even an ok) for that last patch in the master branch would be useful:

[ARM] omap: fix virtual vs physical address space confusions

Planning to send them off tonight.

Quoted 9 lines · Russell King - ARM Linux

Quoted 8 lines · Tony Lindgren

Quoted 6 lines · Russell King - ARM Linux

Quoted 1 line · Tony Lindgren

That was fast :) Sounds we'll have a patch to test soon.

Of course. In my tree. The master branch contains the minimal fixes, and the devel branch contains everything. If you want the individual patches, either pull the tree or grab the mboxes.

Quoted 7 lines

Quoted 2 lines

Hey looks good! I'll cherry pick them to l-o tree, then let's make sure things work as expected.

BTW, having an ack (or even an ok) for that last patch in the master branch would be useful:

[ARM] omap: fix virtual vs physical address space confusions

Here you are:

Acked-by Tony Lindgren <[email protected]>

Quoted 1 line

Planning to send them off tonight.

Great.

Also, I'm about to push your devel patches to l-o tree once I get done transplanting them. So far things are working just fine.

Hello Russell, Tony,

in arch/arm/plat-omap/include/mach/io.h, shouldn't these casts be "__force void __iomem *" ?

#define IO_ADDRESS(pa) ((void __iomem *)__IO_ADDRESS(pa))
#define OMAP1_IO_ADDRESS(pa) ((void __iomem *)__OMAP1_IO_ADDRESS(pa))
#define OMAP2_IO_ADDRESS(pa) ((void __iomem *)__OMAP2_IO_ADDRESS(pa))

sparse is emitting warnings again - i.e.,

arch/arm/mach-omap2/irq.c:150:21: warning: cast adds address space to expression (<asn:2>)

The system hangs when there is a request for mcbsp in the platform driver of omap ie.. at
File:sound/soc/omap/omap-mcbsp.c
Function: omap_mcbsp_dai_startup omap_mcbsp_request(mcbsp_data->bus_id);
This is executed when we do an aplay.

In the ALSA driver(sound/arm/omap/omap-alsa.c:513) the omap_mcbsp_request() is during the boot time. There no issues.

So during my testing of the ASOC driver i comment out omap_mcbsp_request(mcbsp_data->bus_id); from
platform driver (sound/soc/omap/omap-mcbsp.c) and insted requested the omap_mcbsp in my
moudule_init of machine driver(ie sound/soc/omap/osk.c).

Quoted 7 lines · On Mon, 29 Sep 2008 10:46:33 +0530 "ext Arun KS" <[email protected]>…

The system hangs when there is a request for mcbsp in the platform driver of omap ie.. at
File:sound/soc/omap/omap-mcbsp.c
Function: omap_mcbsp_dai_startup omap_mcbsp_request(mcbsp_data->bus_id);
This is executed when we do an aplay.

Was this patch included?

commit f9d06c24f43b7620b14e4dd6dbf667fa68457766

Author: Jarkko Nikula <[email protected]>
Date:   Mon Sep 22 16:47:51 2008 +0300

ARM: OMAP: Fixes to omap_mcbsp_request function

Bootloader may let McBSP logic running so make sure that block is
idle before requesting IRQs. Also make sure that TX and RX waitqueues are initialized before request_irq.

Also you can get some debug messages from mcbsp by patch below and "echo 8 > /proc/sys/kernel/printk" in your target before executing aplay.

---

arch/arm/plat-omap/mcbsp.c +1 -1

index 798c91e..2bfb177 100644
@@ -12,6 +12,7 @@
 * Multichannel mode not supported.
 */
 
+#define DEBUG
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/device.h>
---

Quoted 11 lines

In the ALSA driver(sound/arm/omap/omap-alsa.c:513) the omap_mcbsp_request() is during the boot time. There no issues.

So during my testing of the ASOC driver i comment out omap_mcbsp_request(mcbsp_data->bus_id); from
platform driver (sound/soc/omap/omap-mcbsp.c) and insted requested the omap_mcbsp in my
moudule_init of machine driver(ie sound/soc/omap/osk.c).

Legacy driver is doing some clock setup in snd_omap_alsa_post_probe before omap_mcbsp_request.

       alsa_codec_config->codec_clock_setup();
       alsa_codec_config->codec_clock_on();

       omap_mcbsp_request(AUDIO_MCBSP);

If my patch wasn't included I can understand that it would explain why legacy driver is working. If it was, then there still might be some registers which must be set to reset default before requesting irqs in omap_mcbsp_request.

Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html