msm: iommu: Create a Kconfig item for the IOMMU driver

Make the IOMMU platform data target-independent in preparation for adding MSM8960 IOMMU support. The IOMMU configuration on MSM8x60 and MSM8960 is identical and the same platform data can be used for both.

Signed-off-by Stepan Moskovchenko <[email protected]>

---
 arch/arm/mach-msm/Makefile                         |    4 +-
 .../{devices-msm8x60-iommu.c => devices-iommu.c}   |   54 +++++++++----------
 arch/arm/mach-msm/include/mach/msm_iomap-8x60.h    |   36 -------------
 3 files changed, 28 insertions(+), 66 deletions(-)
 rename arch/arm/mach-msm/{devices-msm8x60-iommu.c => devices-iommu.c} (93%)

arch/arm/mach-msm/Makefile +2 -2

index 81f4811..2099c97 100644
@@ -4,12 +4,12 @@ obj-$(CONFIG_DEBUG_FS) += clock-debug.o
 endif
 
 obj-$(CONFIG_MSM_VIC) += irq-vic.o
-obj-$(CONFIG_MSM_IOMMU) += iommu.o iommu_dev.o
+obj-$(CONFIG_MSM_IOMMU) += iommu.o iommu_dev.o devices-iommu.o
 
 obj-$(CONFIG_ARCH_MSM7X00A) += dma.o irq.o acpuclock-arm11.o
 obj-$(CONFIG_ARCH_MSM7X30) += dma.o
 obj-$(CONFIG_ARCH_QSD8X50) += dma.o sirc.o
-obj-$(CONFIG_ARCH_MSM8X60) += clock-dummy.o devices-msm8x60-iommu.o
+obj-$(CONFIG_ARCH_MSM8X60) += clock-dummy.o
 obj-$(CONFIG_ARCH_MSM8960) += clock-dummy.o
 
 obj-$(CONFIG_MSM_PROC_COMM) += proc_comm.o clock-pcom.o vreg.o

arch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +1 -3

similarity index 93%
rename from arch/arm/mach-msm/devices-msm8x60-iommu.c
rename to arch/arm/mach-msm/devices-iommu.c
index f9e7bd3..c0206b7 100644
@@ -1,4 +1,4 @@
-/* Copyright (c) 2010, Code Aurora Forum. All rights reserved.
+/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and

@@ -18,15 +18,13 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/bootmem.h>
-
-#include <mach/msm_iomap-8x60.h>

arch/arm/mach-msm/devices-iommu.c line 27

Quoted 6 lines

static struct resource msm_iommu_jpegd_resources[] = {

arch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +2 -2

 	{
-		.start = MSM_IOMMU_JPEGD_PHYS,
-		.end   = MSM_IOMMU_JPEGD_PHYS + MSM_IOMMU_JPEGD_SIZE - 1,
+		.start = 0x07300000,
+		.end   = 0x07300000 + SZ_1M - 1,

Looks worse .. Just put the macros into a static header file for both.

Quoted 8 lines · Daniel Walker

arch/arm/mach-msm/devices-iommu.c line 27

Quoted 6 lines

static struct resource msm_iommu_jpegd_resources[] = {

arch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +2 -2

 	{
-		.start = MSM_IOMMU_JPEGD_PHYS,
-		.end   = MSM_IOMMU_JPEGD_PHYS + MSM_IOMMU_JPEGD_SIZE - 1,
+		.start = 0x07300000,
+		.end   = 0x07300000 + SZ_1M - 1,

Looks worse .. Just put the macros into a static header file for both.

Why bother defining macros for these if they only appear here? I don't think that adds any value or readability - these addresses are clearly the physical area for the msm_iommu_jpegd. It just makes it more annoying to have to look up the values in a separate file if you are wondering what they are.

Quoted 10 lines · Steve Muckle

Quoted 8 lines · Daniel Walker

arch/arm/mach-msm/devices-iommu.c line 27

Quoted 6 lines

static struct resource msm_iommu_jpegd_resources[] = {

arch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +2 -2

 	{
-		.start = MSM_IOMMU_JPEGD_PHYS,
-		.end   = MSM_IOMMU_JPEGD_PHYS + MSM_IOMMU_JPEGD_SIZE - 1,
+		.start = 0x07300000,
+		.end   = 0x07300000 + SZ_1M - 1,

Looks worse .. Just put the macros into a static header file for both.

Quoted 5 lines

Why bother defining macros for these if they only appear here? I don't think that adds any value or readability - these addresses are clearly the physical area for the msm_iommu_jpegd. It just makes it more annoying to have to look up the values in a separate file if you are wondering what they are.

So your saying if you look at the number 0x07300000 you instantly know that this JPEGD?

Quoted 17 lines · Daniel Walker

Quoted 15 lines · Steve Muckle

Quoted 8 lines · Daniel Walker

arch/arm/mach-msm/devices-iommu.c line 27

Quoted 6 lines

static struct resource msm_iommu_jpegd_resources[] = {

arch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +2 -2

 	{
-		.start = MSM_IOMMU_JPEGD_PHYS,
-		.end   = MSM_IOMMU_JPEGD_PHYS + MSM_IOMMU_JPEGD_SIZE - 1,
+		.start = 0x07300000,
+		.end   = 0x07300000 + SZ_1M - 1,

Looks worse .. Just put the macros into a static header file for both.

Why bother defining macros for these if they only appear here? I don't think that adds any value or readability - these addresses are clearly the physical area for the msm_iommu_jpegd. It just makes it more annoying to have to look up the values in a separate file if you are wondering what they are.

Quoted 2 lines

So your saying if you look at the number 0x07300000 you instantly know that this JPEGD?

Yes, because it's the start address for the msm_iommu_jpegd resource.

Quoted 8 lines · Daniel Walker

arch/arm/mach-msm/devices-iommu.c line 27

Quoted 6 lines

static struct resource msm_iommu_jpegd_resources[] = {

arch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +2 -2

 	{
-		.start = MSM_IOMMU_JPEGD_PHYS,
-		.end   = MSM_IOMMU_JPEGD_PHYS + MSM_IOMMU_JPEGD_SIZE - 1,
+		.start = 0x07300000,
+		.end   = 0x07300000 + SZ_1M - 1,

Looks worse .. Just put the macros into a static header file for both.

Why bother defining macros for these if they only appear here? I don't think that adds any value or readability - these addresses are clearly the physical area for the msm_iommu_jpegd. It just makes it more annoying to have to look up the values in a separate file if you are wondering what they are.

Quoted 10 lines · Steve Muckle

Quoted 8 lines · Daniel Walker

arch/arm/mach-msm/devices-iommu.c line 27

Quoted 6 lines

static struct resource msm_iommu_jpegd_resources[] = {

arch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +2 -2

 	{
-		.start = MSM_IOMMU_JPEGD_PHYS,
-		.end   = MSM_IOMMU_JPEGD_PHYS + MSM_IOMMU_JPEGD_SIZE - 1,
+		.start = 0x07300000,
+		.end   = 0x07300000 + SZ_1M - 1,

Looks worse .. Just put the macros into a static header file for both.

Quoted 5 lines

Why bother defining macros for these if they only appear here? I don't think that adds any value or readability - these addresses are clearly the physical area for the msm_iommu_jpegd. It just makes it more annoying to have to look up the values in a separate file if you are wondering what they are.

I want to chime in with a second on this. Defining names for constants serves several purposes:

- It gives meaning to the constants.

- It allows the definition to be centralized if the value is used in one place.

If the constants are initializers in a table, it satisfies both of these reasons. Adding #defines for these constants does nothing other than cause an extra indirection that the reader of the code has to make.

If they were used in more than one place, we could justify the definition, but in this case, the definition just obscures the code slightly.

Quoted 5 lines · David Brown

On Fri, Feb 11 2011, Steve Muckle wrote:

If they were used in more than one place, we could justify the definition, but in this case, the definition just obscures the code slightly.

A good example might be if all these constants are enumerated in a header file, but aren't all used. In that case it would be fairly easy to add a new resource without even know what the constant is just by following the pattern.

I think in general this series just makes this iommu code very much 8660/8960 only code, but what about the potential next iteration of SoC that uses very similar code to this with all new constants. So this doesn't seem forward thinking to me.

Quoted 2 lines · Daniel Walker

Quoted 1 line · David Brown

On Fri, Feb 11 2011, Steve Muckle wrote:

Quoted 3 lines

Quoted 3 lines

If they were used in more than one place, we could justify the definition, but in this case, the definition just obscures the code slightly.

Someone debugging it will look at the constant. In fact, in general, the only person looking at this structure will want to know the value in the table. Indirecting it through a pointer only serves to hide it from the person who wants to know the value.

Quoted 4 lines

A good example might be if all these constants are enumerated in a header file, but aren't all used. In that case it would be fairly easy to add a new resource without even know what the constant is just by following the pattern.

This I definitely want to avoid. I have seen header files with hundreds of thousands of register definitions, where only a few were used.

Quoted 4 lines

I think in general this series just makes this iommu code very much 8660/8960 only code, but what about the potential next iteration of SoC that uses very similar code to this with all new constants. So this doesn't seem forward thinking to me.

The table would have the different addresses in it. My point is that the resource table _is_ the definition of the addres. Nothing is gained by inventing yet another name and putting that somewhere else.

Make the IOMMU platform data target-independent in preparation for adding MSM8960 IOMMU support. The IOMMU configuration on MSM8x60 and MSM8960 is identical and the same platform data can be used for both.

Signed-off-by Stepan Moskovchenko <[email protected]>

---
 arch/arm/mach-msm/Makefile                         |    4 +-
 .../{devices-msm8x60-iommu.c => devices-iommu.c}   |   54 +++++++++----------
 arch/arm/mach-msm/include/mach/msm_iomap-8x60.h    |   36 -------------
 3 files changed, 28 insertions(+), 66 deletions(-)
 rename arch/arm/mach-msm/{devices-msm8x60-iommu.c => devices-iommu.c} (93%)

Quoted 5 lines · Stepan Moskovchenko

Make the IOMMU platform data target-independent in preparation for adding MSM8960 IOMMU support. The IOMMU configuration on MSM8x60 and MSM8960 is identical and the same platform data can be used for both.

Quoted 7 lines

Signed-off-by Stepan Moskovchenko <[email protected]>

---
 arch/arm/mach-msm/Makefile                         |    4 +-
 .../{devices-msm8x60-iommu.c => devices-iommu.c}   |   54 +++++++++----------
 arch/arm/mach-msm/include/mach/msm_iomap-8x60.h    |   36 -------------
 3 files changed, 28 insertions(+), 66 deletions(-)
 rename arch/arm/mach-msm/{devices-msm8x60-iommu.c => devices-iommu.c} (93%)

If it's like what you and David are suggesting I think you would need a SoC designation in the filename ..

Quoted 14 lines · Daniel Walker

Quoted 12 lines · Stepan Moskovchenko

Make the IOMMU platform data target-independent in preparation for adding MSM8960 IOMMU support. The IOMMU configuration on MSM8x60 and MSM8960 is identical and the same platform data can be used for both.

Signed-off-by Stepan Moskovchenko <[email protected]>

---
 arch/arm/mach-msm/Makefile                         |    4 +-
 .../{devices-msm8x60-iommu.c => devices-iommu.c}   |   54 +++++++++----------
 arch/arm/mach-msm/include/mach/msm_iomap-8x60.h    |   36 -------------
 3 files changed, 28 insertions(+), 66 deletions(-)
 rename arch/arm/mach-msm/{devices-msm8x60-iommu.c => devices-iommu.c} (93%)

Quoted 2 lines

If it's like what you and David are suggesting I think you would need a SoC designation in the filename ..

It is functionality that will be shared across multiple socs. Putting the name of a specific soc would just be misleading. Currently, it's our only iommu. Support for another family that uses a different iommu could perhaps call it iommu2.

Quoted 4 lines · David Brown

It is functionality that will be shared across multiple socs. Putting the name of a specific soc would just be misleading. Currently, it's our only iommu. Support for another family that uses a different iommu could perhaps call it iommu2.

Your missing my point. I'm saying it doesn't look flexible enough to allow support for multiple SoCs .. Is everything going to be identical across all the supported socs ?

Quoted 7 lines · Daniel Walker

Quoted 4 lines · David Brown

It is functionality that will be shared across multiple socs. Putting the name of a specific soc would just be misleading. Currently, it's our only iommu. Support for another family that uses a different iommu could perhaps call it iommu2.

Quoted 3 lines

Your missing my point. I'm saying it doesn't look flexible enough to allow support for multiple SoCs .. Is everything going to be identical across all the supported socs ?

It wouldn't help, though. If the addresses differ across targets, we don't want defines that are conditionally defined, so we would need multiple tables, giving the address for specific targets. Still no reason to have an indirection on the names.

arch/arm/mach-msm/Kconfig config MACH_MSM8960_RUMI3 · line 161

Quoted 11 lines · Stepan Moskovchenko

arch/arm/mach-msm/Kconfig +11 -0

+config MSM_IOMMU
+       bool "MSM IOMMU Support"
+       depends on ARCH_MSM8X60
+       select IOMMU_API
+       default n
+       help
+         Support for the IOMMUs found on certain Qualcomm SOCs.
+         These IOMMUs allow virtualization of the address space used by most
+         cores within the multimedia subsystem.
+
+         If unsure, say N here. 

I think you should just make this a hidden option, unless there is a good reason why any given users might want to turn this off.

Quoted 15 lines · Daniel Walker

arch/arm/mach-msm/Kconfig config MACH_MSM8960_RUMI3 · line 161

Quoted 11 lines · Stepan Moskovchenko

arch/arm/mach-msm/Kconfig +11 -0

+config MSM_IOMMU
+       bool "MSM IOMMU Support"
+       depends on ARCH_MSM8X60
+       select IOMMU_API
+       default n
+       help
+         Support for the IOMMUs found on certain Qualcomm SOCs.
+         These IOMMUs allow virtualization of the address space used by most
+         cores within the multimedia subsystem.
+
+         If unsure, say N here. 

I think you should just make this a hidden option, unless there is a good reason why any given users might want to turn this off.

In this particular case, the driver is optional, even devices that use it will be able to work both with and without it. It makes sense to be able to decide whether to have it or not.

Typically we include everything the SoC has regardless of if drivers use the hardware or not . For instance there could be modules that use the hardware ..

Regardless of this point, I've nacked the whole series. It looks like there was very little thought put into this.

Quoted 15 lines · Daniel Walker

arch/arm/mach-msm/Kconfig config MACH_MSM8960_RUMI3 · line 161

Quoted 11 lines · Stepan Moskovchenko

arch/arm/mach-msm/Kconfig +11 -0

+config MSM_IOMMU
+       bool "MSM IOMMU Support"
+       depends on ARCH_MSM8X60
+       select IOMMU_API
+       default n
+       help
+         Support for the IOMMUs found on certain Qualcomm SOCs.
+         These IOMMUs allow virtualization of the address space used by most
+         cores within the multimedia subsystem.
+
+         If unsure, say N here. 

I think you should just make this a hidden option, unless there is a good reason why any given users might want to turn this off.

In this particular case, the driver is optional, even devices that use it will be able to work both with and without it. It makes sense to be able to decide whether to have it or not.