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

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

[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 <manjunath.hadli@ti.com>
---
 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(-)

diff --git a/arch/arm/mach-davinci/devices.c b/arch/arm/mach-davinci/devices.c
index d3b2040..66a948d 100644
--- a/arch/arm/mach-davinci/devices.c
+++ b/arch/arm/mach-davinci/devices.c
@@ -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);
 		}
diff --git a/arch/arm/mach-davinci/dm355.c b/arch/arm/mach-davinci/dm355.c
index a5f8a80..1baab94 100644
--- a/arch/arm/mach-davinci/dm355.c
+++ b/arch/arm/mach-davinci/dm355.c
@@ -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)
diff --git a/arch/arm/mach-davinci/dm365.c b/arch/arm/mach-davinci/dm365.c
index 02d2cc3..a788980 100644
--- a/arch/arm/mach-davinci/dm365.c
+++ b/arch/arm/mach-davinci/dm365.c
@@ -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[] = {
diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
index 9a2376b..77dea11 100644
--- a/arch/arm/mach-davinci/dm644x.c
+++ b/arch/arm/mach-davinci/dm644x.c
@@ -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)
diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c
index 1e0f809..ce93b83 100644
--- a/arch/arm/mach-davinci/dm646x.c
+++ b/arch/arm/mach-davinci/dm646x.c
@@ -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)
diff --git a/arch/arm/mach-davinci/include/mach/hardware.h b/arch/arm/mach-davinci/include/mach/hardware.h
index 414e0b9..5296025 100644
--- a/arch/arm/mach-davinci/include/mach/hardware.h
+++ b/arch/arm/mach-davinci/include/mach/hardware.h
@@ -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
  */
-- 
1.6.2.4

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

[Sergei Shtylyov]

Hello.

Manjunath Hadli wrote:

> 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 <manjunath.hadli@ti.com>
[...]

> diff --git a/arch/arm/mach-davinci/devices.c b/arch/arm/mach-davinci/devices.c
> index d3b2040..66a948d 100644
> --- a/arch/arm/mach-davinci/devices.c
> +++ b/arch/arm/mach-davinci/devices.c
[...]
> @@ -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);

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

WBR, Sergei

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

[Arnd Bergmann]

On Monday 14 March 2011, Manjunath Hadli wrote:
> 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 <manjunath.hadli@ti.com>

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

Acked-by: Arnd Bergmann <arnd@arndb.de>


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

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

[Nori, Sekhar]

Hi Arnd,

On Mon, Mar 14, 2011 at 21:51:51, Arnd Bergmann wrote:
> On Monday 14 March 2011, Manjunath Hadli wrote:
> > 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 <manjunath.hadli@ti.com>
> 
> The change looks good, it's definitely a step in the right
> direction.
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> 
> 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.

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

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

[Arnd Bergmann]

On Tuesday 15 March 2011 07:00:44 Nori, Sekhar wrote:

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

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