davinci: eliminate use of IO_ADDRESS() on sysmod

PATCH Superseded Manjunath Hadli 4 participants 5 messages Mar 14, 2011 – Mar 15, 2011

Subsystems (4)

Arm (32-bit) Media (V4L/DVB) Arm sub-architectures TI DaVinci machine support

lore

Share & tools

lore

Manjunath Hadli

author

2011-03-14 13:55 UTC #0 lore raw

PATCH 2/7 davinci: eliminate use of IO_ADDRESS() on sysmod

Current devices.c file has a number of instances where IO_ADDRESS() is used for system module register access. Eliminate this in favor of a ioremap() based access.

Consequent to this, a new global pointer davinci_sysmodbase has been introduced which gets initialized during the initialization of each relevant SoC

Signed-off-by Manjunath Hadli <[email protected]>

---
 arch/arm/mach-davinci/devices.c               |   23 ++++++++++++++---------
 arch/arm/mach-davinci/dm355.c                 |    1 +
 arch/arm/mach-davinci/dm365.c                 |    1 +
 arch/arm/mach-davinci/dm644x.c                |    1 +
 arch/arm/mach-davinci/dm646x.c                |    1 +
 arch/arm/mach-davinci/include/mach/hardware.h |    6 ++++++
 6 files changed, 24 insertions(+), 9 deletions(-)

arch/arm/mach-davinci/devices.c +14 -9

index d3b2040..66a948d 100644
@@ -33,6 +33,14 @@
 #define DM365_MMCSD0_BASE	     0x01D11000
 #define DM365_MMCSD1_BASE	     0x01D00000
 
+void __iomem  *davinci_sysmodbase;
+
+void davinci_map_sysmod(void)
+{
+	davinci_sysmodbase = ioremap_nocache(DAVINCI_SYSTEM_MODULE_BASE, 0x800);
+	WARN_ON(!davinci_sysmodbase);
+}
+
 static struct resource i2c_resources[] = {
 	{
 		.start		= DAVINCI_I2C_BASE,

@@ -210,12 +218,12 @@ void __init davinci_setup_mmc(int module, struct davinci_mmc_config *config)
 			davinci_cfg_reg(DM355_SD1_DATA2);
 			davinci_cfg_reg(DM355_SD1_DATA3);
 		} else if (cpu_is_davinci_dm365()) {
-			void __iomem *pupdctl1 =
-				IO_ADDRESS(DAVINCI_SYSTEM_MODULE_BASE + 0x7c);
-
 			/* Configure pull down control */
-			__raw_writel((__raw_readl(pupdctl1) & ~0xfc0),
-					pupdctl1);
+			void __iomem *pupdctl1 = DAVINCI_SYSMODULE_VIRT(0x7c);
+			unsigned v;
+
+			v = readl(pupdctl1);
+			writel(v & ~0xfc0, pupdctl1);
 
 			mmcsd1_resources[0].start = DM365_MMCSD1_BASE;
 			mmcsd1_resources[0].end = DM365_MMCSD1_BASE +

@@ -244,11 +252,8 @@ void __init davinci_setup_mmc(int module, struct davinci_mmc_config *config)
 			mmcsd0_resources[2].start = IRQ_DM365_SDIOINT0;
 		} else if (cpu_is_davinci_dm644x()) {
 			/* REVISIT: should this be in board-init code? */
-			void __iomem *base =
-				IO_ADDRESS(DAVINCI_SYSTEM_MODULE_BASE);
-
 			/* Power-on 3.3V IO cells */
-			__raw_writel(0, base + DM64XX_VDD3P3V_PWDN);
+			writel(0, DAVINCI_SYSMODULE_VIRT(DM64XX_VDD3P3V_PWDN));
 			/*Set up the pull regiter for MMC */
 			davinci_cfg_reg(DM644X_MSTK);
 		}

arch/arm/mach-davinci/dm355.c +1 -0

index a5f8a80..1baab94 100644
@@ -874,6 +874,7 @@ void __init dm355_init_asp1(u32 evt_enable, struct snd_platform_data *pdata)
 void __init dm355_init(void)
 {
 	davinci_common_init(&davinci_soc_info_dm355);
+	davinci_map_sysmod();
 }
 
 static int __init dm355_init_devices(void)

arch/arm/mach-davinci/dm365.c +1 -0

index 02d2cc3..a788980 100644
@@ -1127,6 +1127,7 @@ void __init dm365_init_rtc(void)
 void __init dm365_init(void)
 {
 	davinci_common_init(&davinci_soc_info_dm365);
+	davinci_map_sysmod();
 }
 
 static struct resource dm365_vpss_resources[] = {

arch/arm/mach-davinci/dm644x.c +1 -0

index 9a2376b..77dea11 100644
@@ -779,6 +779,7 @@ void __init dm644x_init_asp(struct snd_platform_data *pdata)
 void __init dm644x_init(void)
 {
 	davinci_common_init(&davinci_soc_info_dm644x);
+	davinci_map_sysmod();
 }
 
 static int __init dm644x_init_devices(void)

arch/arm/mach-davinci/dm646x.c +1 -0

index 1e0f809..ce93b83 100644
@@ -903,6 +903,7 @@ void __init dm646x_init(void)
 {
 	dm646x_board_setup_refclk(&ref_clk);
 	davinci_common_init(&davinci_soc_info_dm646x);
+	davinci_map_sysmod();
 }
 
 static int __init dm646x_init_devices(void)

arch/arm/mach-davinci/include/mach/hardware.h +6 -0

index 414e0b9..5296025 100644
@@ -21,6 +21,12 @@
  */
 #define DAVINCI_SYSTEM_MODULE_BASE        0x01C40000
 
+#ifndef __ASSEMBLER__
+extern void __iomem  *davinci_sysmodbase;
+#define DAVINCI_SYSMODULE_VIRT(x)	(davinci_sysmodbase + (x))
+void davinci_map_sysmod(void);
+#endif
+
 /*
  * I/O mapping
  */

Signature · 1 line

1.6.2.4

Sergei Shtylyov

2011-03-14 14:11 UTC #1 lore raw

PATCH 2/7 davinci: eliminate use of IO_ADDRESS() on sysmod

Hello.

Quoted 10 lines · Manjunath Hadli

Current devices.c file has a number of instances where IO_ADDRESS() is used for system module register access. Eliminate this in favor of a ioremap() based access.
Consequent to this, a new global pointer davinci_sysmodbase has been introduced which gets initialized during the initialization of each relevant SoC
Signed-off-by Manjunath Hadli <[email protected]>

[...]

Quoted 4 lines

arch/arm/mach-davinci/devices.c +0 -0
index d3b2040..66a948d 100644

[...]

Quoted 5 lines

arch/arm/mach-davinci/devices.c +0 -1

@@ -210,12 +218,12 @@ void __init davinci_setup_mmc(int module, struct davinci_mmc_config *config)
 			davinci_cfg_reg(DM355_SD1_DATA2);
 			davinci_cfg_reg(DM355_SD1_DATA3);
 		} else if (cpu_is_davinci_dm365()) {
-			void __iomem *pupdctl1 =

arch/arm/mach-davinci/devices.c void __init davinci_setup_mmc(int module, struct davinci_mmc_config *config) · line 226

Quoted 10 lines

arch/arm/mach-davinci/devices.c +5 -4

-				IO_ADDRESS(DAVINCI_SYSTEM_MODULE_BASE + 0x7c);
-
 			/* Configure pull down control */
-			__raw_writel((__raw_readl(pupdctl1) & ~0xfc0),
-					pupdctl1);
+			void __iomem *pupdctl1 = DAVINCI_SYSMODULE_VIRT(0x7c);
+			unsigned v;
+
+			v = readl(pupdctl1);
+			writel(v & ~0xfc0, pupdctl1);

Why are you changing from __raw_{readl|writel}() to {readl|writel}()? You
don't mention it in the change log...

WBR, Sergei

Arnd Bergmann

2011-03-14 16:21 UTC #2 lore raw

PATCH 2/7 davinci: eliminate use of IO_ADDRESS() on sysmod

In reply to Manjunath Hadli

Quoted 10 lines · Manjunath Hadli

Current devices.c file has a number of instances where IO_ADDRESS() is used for system module register access. Eliminate this in favor of a ioremap() based access.
Consequent to this, a new global pointer davinci_sysmodbase has been introduced which gets initialized during the initialization of each relevant SoC
Signed-off-by Manjunath Hadli <[email protected]>

The change looks good, it's definitely a step in the right direction.

Acked-by Arnd Bergmann <[email protected]>

I think you can go even further:

* A straightforward change would be to move davinci_sysmodbase into a local variable of the davinci_setup_mmc function, which I believe is the only user. Then you can ioremap and iounmap it directly there.

* If you need to access sysmod in multiple places, a nicer way would be to make the virtual address pointer static, and export the accessor functions for it, rather than having a global pointer.

Arnd

Nori, Sekhar

2011-03-15 06:00 UTC #3 lore raw

PATCH 2/7 davinci: eliminate use of IO_ADDRESS() on sysmod

Hi Arnd,

Quoted 18 lines · Arnd Bergmann

Quoted 10 lines · Manjunath Hadli
Current devices.c file has a number of instances where IO_ADDRESS() is used for system module register access. Eliminate this in favor of a ioremap() based access.
Consequent to this, a new global pointer davinci_sysmodbase has been introduced which gets initialized during the initialization of each relevant SoC
Signed-off-by Manjunath Hadli <[email protected]>
The change looks good, it's definitely a step in the right direction.
Acked-by Arnd Bergmann <[email protected]>

Quoted 6 lines

I think you can go even further:
* A straightforward change would be to move davinci_sysmodbase into a local variable of the davinci_setup_mmc function, which I believe is the only user. Then you can ioremap and iounmap it directly there.

This patch accesses sysmodule only in davinci_setup_mmc, but follow-on patches use it in other places. So, this patch sort of lays the foundation for that. This is not really evident in this patch so the patch description should have captured that.

Quoted 4 lines

* If you need to access sysmod in multiple places, a nicer way would be to make the virtual address pointer static, and export the accessor functions for it, rather than having a global pointer.

Seems like opinion is divided on this. A while back I submitted a patch with such an accessor function and was asked to do the opposite of what you are asking here.

https://patchwork.kernel.org/patch/366501/

It can be changed to the way you are asking, but would like to know what is more universally acceptable (if at all there is such a thing).

Thanks,
Sekhar

Arnd Bergmann

2011-03-15 09:00 UTC #4 lore raw

PATCH 2/7 davinci: eliminate use of IO_ADDRESS() on sysmod

Quoted 9 lines · Sekhar

Quoted 4 lines
* If you need to access sysmod in multiple places, a nicer way would be to make the virtual address pointer static, and export the accessor functions for it, rather than having a global pointer.
Seems like opinion is divided on this. A while back I submitted a patch with such an accessor function and was asked to do the opposite of what you are asking here.

Quoted 5 lines

https://patchwork.kernel.org/patch/366501/
It can be changed to the way you are asking, but would like to know what is more universally acceptable (if at all there is such a thing).

One difference is that the base address pointer here can be treated as read-only by using an accessor function, which was not possible for the case you cited. Doing an inline function would also let you make the access more type-safe, e.g forcing the right kind of readl/writel variant and possibly locking if necessary.

I would also argue against Sergei's point for the other patch -- the current solution is not better than the originally suggested one IMHO. I believe a better way would have been to pass the maximum frequency as an argument to da850_register_cpufreq() in that case.

However, neither of these discussion is really important, and we don't have a strict rule for doing it one way or the other. Just use common sense and decide case-by-case, as I said in the previous comment, you got the important parts right.

Arnd