[PATCH 1/3] msm: iommu: Create a Kconfig item for the IOMMU driver

[PATCH 1/3] msm: iommu: Create a Kconfig item for the IOMMU driver

[Stepan Moskovchenko]

Break the IOMMU driver out as a Kconfig item. Initially it
was decided to always build this in for 8x60, but this
driver is not strictly necessary and should be optionally
selectable.

Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
---
 arch/arm/mach-msm/Kconfig  |   13 ++++++++++++-
 arch/arm/mach-msm/Makefile |    3 ++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-msm/Kconfig b/arch/arm/mach-msm/Kconfig
index df9d74e..32b9d1f 100644
--- a/arch/arm/mach-msm/Kconfig
+++ b/arch/arm/mach-msm/Kconfig
@@ -45,7 +45,6 @@ config ARCH_MSM8X60
 	select CPU_V7
 	select MSM_V2_TLMM
 	select MSM_GPIOMUX
-	select IOMMU_API
 	select MSM_SCM if SMP

 config ARCH_MSM8960
@@ -149,6 +148,18 @@ config MACH_MSM8960_RUMI3

 endmenu

+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.
+
 config IOMMU_PGTABLES_L2
 	def_bool y
 	depends on ARCH_MSM8X60 && MMU && SMP && CPU_DCACHE_DISABLE=n
diff --git a/arch/arm/mach-msm/Makefile b/arch/arm/mach-msm/Makefile
index ea8c74f..81f4811 100644
--- a/arch/arm/mach-msm/Makefile
+++ b/arch/arm/mach-msm/Makefile
@@ -4,11 +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_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 iommu.o iommu_dev.o devices-msm8x60-iommu.o
+obj-$(CONFIG_ARCH_MSM8X60) += clock-dummy.o devices-msm8x60-iommu.o
 obj-$(CONFIG_ARCH_MSM8960) += clock-dummy.o

 obj-$(CONFIG_MSM_PROC_COMM) += proc_comm.o clock-pcom.o vreg.o
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[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 <stepanm@codeaurora.org>
---
 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%)

diff --git a/arch/arm/mach-msm/Makefile b/arch/arm/mach-msm/Makefile
index 81f4811..2099c97 100644
--- a/arch/arm/mach-msm/Makefile
+++ b/arch/arm/mach-msm/Makefile
@@ -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
diff --git a/arch/arm/mach-msm/devices-msm8x60-iommu.c b/arch/arm/mach-msm/devices-iommu.c
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
--- a/arch/arm/mach-msm/devices-msm8x60-iommu.c
+++ b/arch/arm/mach-msm/devices-iommu.c
@@ -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>
-#include <mach/irqs-8x60.h>
+#include <mach/irqs.h>
 #include <mach/iommu.h>

 static struct resource msm_iommu_jpegd_resources[] = {
 	{
-		.start = MSM_IOMMU_JPEGD_PHYS,
-		.end   = MSM_IOMMU_JPEGD_PHYS + MSM_IOMMU_JPEGD_SIZE - 1,
+		.start = 0x07300000,
+		.end   = 0x07300000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -46,8 +44,8 @@ static struct resource msm_iommu_jpegd_resources[] = {

 static struct resource msm_iommu_vpe_resources[] = {
 	{
-		.start = MSM_IOMMU_VPE_PHYS,
-		.end   = MSM_IOMMU_VPE_PHYS + MSM_IOMMU_VPE_SIZE - 1,
+		.start = 0x07400000,
+		.end   = 0x07400000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -67,8 +65,8 @@ static struct resource msm_iommu_vpe_resources[] = {

 static struct resource msm_iommu_mdp0_resources[] = {
 	{
-		.start = MSM_IOMMU_MDP0_PHYS,
-		.end   = MSM_IOMMU_MDP0_PHYS + MSM_IOMMU_MDP0_SIZE - 1,
+		.start = 0x07500000,
+		.end   = 0x07500000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -88,8 +86,8 @@ static struct resource msm_iommu_mdp0_resources[] = {

 static struct resource msm_iommu_mdp1_resources[] = {
 	{
-		.start = MSM_IOMMU_MDP1_PHYS,
-		.end   = MSM_IOMMU_MDP1_PHYS + MSM_IOMMU_MDP1_SIZE - 1,
+		.start = 0x07600000,
+		.end   = 0x07600000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -109,8 +107,8 @@ static struct resource msm_iommu_mdp1_resources[] = {

 static struct resource msm_iommu_rot_resources[] = {
 	{
-		.start = MSM_IOMMU_ROT_PHYS,
-		.end   = MSM_IOMMU_ROT_PHYS + MSM_IOMMU_ROT_SIZE - 1,
+		.start = 0x07700000,
+		.end   = 0x07700000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -130,8 +128,8 @@ static struct resource msm_iommu_rot_resources[] = {

 static struct resource msm_iommu_ijpeg_resources[] = {
 	{
-		.start = MSM_IOMMU_IJPEG_PHYS,
-		.end   = MSM_IOMMU_IJPEG_PHYS + MSM_IOMMU_IJPEG_SIZE - 1,
+		.start = 0x07800000,
+		.end   = 0x07800000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -151,8 +149,8 @@ static struct resource msm_iommu_ijpeg_resources[] = {

 static struct resource msm_iommu_vfe_resources[] = {
 	{
-		.start = MSM_IOMMU_VFE_PHYS,
-		.end   = MSM_IOMMU_VFE_PHYS + MSM_IOMMU_VFE_SIZE - 1,
+		.start = 0x07900000,
+		.end   = 0x07900000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -172,8 +170,8 @@ static struct resource msm_iommu_vfe_resources[] = {

 static struct resource msm_iommu_vcodec_a_resources[] = {
 	{
-		.start = MSM_IOMMU_VCODEC_A_PHYS,
-		.end   = MSM_IOMMU_VCODEC_A_PHYS + MSM_IOMMU_VCODEC_A_SIZE - 1,
+		.start = 0x07A00000,
+		.end   = 0x07A00000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -193,8 +191,8 @@ static struct resource msm_iommu_vcodec_a_resources[] = {

 static struct resource msm_iommu_vcodec_b_resources[] = {
 	{
-		.start = MSM_IOMMU_VCODEC_B_PHYS,
-		.end   = MSM_IOMMU_VCODEC_B_PHYS + MSM_IOMMU_VCODEC_B_SIZE - 1,
+		.start = 0x07B00000,
+		.end   = 0x07B00000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -214,8 +212,8 @@ static struct resource msm_iommu_vcodec_b_resources[] = {

 static struct resource msm_iommu_gfx3d_resources[] = {
 	{
-		.start = MSM_IOMMU_GFX3D_PHYS,
-		.end   = MSM_IOMMU_GFX3D_PHYS + MSM_IOMMU_GFX3D_SIZE - 1,
+		.start = 0x07C00000,
+		.end   = 0x07C00000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -235,8 +233,8 @@ static struct resource msm_iommu_gfx3d_resources[] = {

 static struct resource msm_iommu_gfx2d0_resources[] = {
 	{
-		.start = MSM_IOMMU_GFX2D0_PHYS,
-		.end   = MSM_IOMMU_GFX2D0_PHYS + MSM_IOMMU_GFX2D0_SIZE - 1,
+		.start = 0x07D00000,
+		.end   = 0x07D00000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
@@ -256,8 +254,8 @@ static struct resource msm_iommu_gfx2d0_resources[] = {

 static struct resource msm_iommu_gfx2d1_resources[] = {
 	{
-		.start = MSM_IOMMU_GFX2D1_PHYS,
-		.end   = MSM_IOMMU_GFX2D1_PHYS + MSM_IOMMU_GFX2D1_SIZE - 1,
+		.start = 0x07E00000,
+		.end   = 0x07E00000 + SZ_1M - 1,
 		.name  = "physbase",
 		.flags = IORESOURCE_MEM,
 	},
diff --git a/arch/arm/mach-msm/include/mach/msm_iomap-8x60.h b/arch/arm/mach-msm/include/mach/msm_iomap-8x60.h
index 5bd18db..3b19b8f 100644
--- a/arch/arm/mach-msm/include/mach/msm_iomap-8x60.h
+++ b/arch/arm/mach-msm/include/mach/msm_iomap-8x60.h
@@ -62,40 +62,4 @@
 #define MSM8X60_TMR0_PHYS	0x02040000
 #define MSM8X60_TMR0_SIZE	SZ_4K

-#define MSM_IOMMU_JPEGD_PHYS	0x07300000
-#define MSM_IOMMU_JPEGD_SIZE	SZ_1M
-
-#define MSM_IOMMU_VPE_PHYS	0x07400000
-#define MSM_IOMMU_VPE_SIZE	SZ_1M
-
-#define MSM_IOMMU_MDP0_PHYS	0x07500000
-#define MSM_IOMMU_MDP0_SIZE	SZ_1M
-
-#define MSM_IOMMU_MDP1_PHYS	0x07600000
-#define MSM_IOMMU_MDP1_SIZE	SZ_1M
-
-#define MSM_IOMMU_ROT_PHYS	0x07700000
-#define MSM_IOMMU_ROT_SIZE	SZ_1M
-
-#define MSM_IOMMU_IJPEG_PHYS	0x07800000
-#define MSM_IOMMU_IJPEG_SIZE	SZ_1M
-
-#define MSM_IOMMU_VFE_PHYS	0x07900000
-#define MSM_IOMMU_VFE_SIZE	SZ_1M
-
-#define MSM_IOMMU_VCODEC_A_PHYS	0x07A00000
-#define MSM_IOMMU_VCODEC_A_SIZE	SZ_1M
-
-#define MSM_IOMMU_VCODEC_B_PHYS	0x07B00000
-#define MSM_IOMMU_VCODEC_B_SIZE	SZ_1M
-
-#define MSM_IOMMU_GFX3D_PHYS	0x07C00000
-#define MSM_IOMMU_GFX3D_SIZE	SZ_1M
-
-#define MSM_IOMMU_GFX2D0_PHYS	0x07D00000
-#define MSM_IOMMU_GFX2D0_SIZE	SZ_1M
-
-#define MSM_IOMMU_GFX2D1_PHYS	0x07E00000
-#define MSM_IOMMU_GFX2D1_SIZE	SZ_1M
-
 #endif
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Daniel Walker]

On Fri, 2011-02-11 at 12:28 -0800, Stepan Moskovchenko wrote:
> 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 <stepanm@codeaurora.org>
> ---
>  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%)
> 
> diff --git a/arch/arm/mach-msm/Makefile b/arch/arm/mach-msm/Makefile
> index 81f4811..2099c97 100644
> --- a/arch/arm/mach-msm/Makefile
> +++ b/arch/arm/mach-msm/Makefile
> @@ -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
> diff --git a/arch/arm/mach-msm/devices-msm8x60-iommu.c b/arch/arm/mach-msm/devices-iommu.c
> 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
> --- a/arch/arm/mach-msm/devices-msm8x60-iommu.c
> +++ b/arch/arm/mach-msm/devices-iommu.c
> @@ -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>
> -#include <mach/irqs-8x60.h>
> +#include <mach/irqs.h>
>  #include <mach/iommu.h>
> 
>  static struct resource msm_iommu_jpegd_resources[] = {
>  	{
> -		.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.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Steve Muckle]

On 02/11/11 12:42, Daniel Walker wrote:
>>  static struct resource msm_iommu_jpegd_resources[] = {
>>  	{
>> -		.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.

thanks,
Steve

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Daniel Walker]

On Fri, 2011-02-11 at 12:51 -0800, Steve Muckle wrote:
> On 02/11/11 12:42, Daniel Walker wrote:
> >>  static struct resource msm_iommu_jpegd_resources[] = {
> >>  	{
> >> -		.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.

So your saying if you look at the number 0x07300000 you instantly know
that this JPEGD? What if I pick a random other kernel developer do you
think they would instantly know that? I have no idea what 0x07300000 is.

Also if it's in a header you could ifdef them with out touching the C
file, which is just forward looking.

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Steve Muckle]

On 02/11/11 12:58, Daniel Walker wrote:
> On Fri, 2011-02-11 at 12:51 -0800, Steve Muckle wrote:
>> On 02/11/11 12:42, Daniel Walker wrote:
>>>>  static struct resource msm_iommu_jpegd_resources[] = {
>>>>  	{
>>>> -		.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.
> 
> 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.

thanks,
Steve

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Daniel Walker]

On Fri, 2011-02-11 at 13:00 -0800, Steve Muckle wrote:
> On 02/11/11 12:58, Daniel Walker wrote:
> > On Fri, 2011-02-11 at 12:51 -0800, Steve Muckle wrote:
> >> On 02/11/11 12:42, Daniel Walker wrote:
> >>>>  static struct resource msm_iommu_jpegd_resources[] = {
> >>>>  	{
> >>>> -		.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.
> > 
> > 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.

Yeah I guess that's true .. I still think it's better design not to do
it this way.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[David Brown]

On Fri, Feb 11 2011, Steve Muckle wrote:

> On 02/11/11 12:42, Daniel Walker wrote:
>>>  static struct resource msm_iommu_jpegd_resources[] = {
>>>  	{
>>> -		.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.

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.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Daniel Walker]

On Fri, 2011-02-11 at 13:03 -0800, David Brown wrote:
> On Fri, Feb 11 2011, Steve Muckle wrote:
> 
> > On 02/11/11 12:42, Daniel Walker wrote:
> >>>  static struct resource msm_iommu_jpegd_resources[] = {
> >>>  	{
> >>> -		.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.
> 
> 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.

It only obscures the constant, which no one really looks at anyway. in
general it's better design to hide constant like this, because people
don't work naturally with numbers like this.

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.

Daniel


-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[David Brown]

On Fri, Feb 11 2011, Daniel Walker wrote:

> On Fri, 2011-02-11 at 13:03 -0800, David Brown wrote:
>> 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.

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.

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

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

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Daniel Walker]

On Fri, 2011-02-11 at 13:53 -0800, David Brown wrote:
> On Fri, Feb 11 2011, Daniel Walker wrote:
> 
> > On Fri, 2011-02-11 at 13:03 -0800, David Brown wrote:
> >> 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.
> 
> 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.

Like I said in my example, people looking at the code won't always be
debugging.

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

I think your thinking of stuff that's not properly grouped.

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

In my example I showed you there is something to be gained by doing
this. As you said already there isn't must lost in doing it this way.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Daniel Walker]

On Fri, 2011-02-11 at 12:28 -0800, Stepan Moskovchenko wrote:
> 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 <stepanm@codeaurora.org>
> ---
>  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 ..

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[David Brown]

On Fri, Feb 11 2011, Daniel Walker wrote:

> On Fri, 2011-02-11 at 12:28 -0800, Stepan Moskovchenko wrote:
>> 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 <stepanm@codeaurora.org>
>> ---
>>  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 ..

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.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Daniel Walker]

On Fri, 2011-02-11 at 14:37 -0800, David Brown wrote:
> On Fri, Feb 11 2011, Daniel Walker wrote:
> 
> > On Fri, 2011-02-11 at 12:28 -0800, Stepan Moskovchenko wrote:
> >> 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 <stepanm@codeaurora.org>
> >> ---
> >>  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 ..
> 
> 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 ?

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[David Brown]

On Fri, Feb 11 2011, Daniel Walker wrote:

> On Fri, 2011-02-11 at 14:37 -0800, David Brown wrote:

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

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.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Generalize platform data for multiple targets

[Daniel Walker]

On Fri, 2011-02-11 at 15:16 -0800, David Brown wrote:
> On Fri, Feb 11 2011, Daniel Walker wrote:
> 
> > On Fri, 2011-02-11 at 14:37 -0800, David Brown wrote:
> 
> >> 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 ?
> 
> 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.

I'm talking about the whole deal here, this whole patch series. It
doesn't seem like this has been thought out too well. 

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 3/3] msm: iommu: Enable IOMMU support for MSM8960

[Stepan Moskovchenko]

Allow IOMMU to be selected for MSM8960 now that the
platform data has been generalized.

Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
---
 arch/arm/mach-msm/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-msm/Kconfig b/arch/arm/mach-msm/Kconfig
index 32b9d1f..997c5bd 100644
--- a/arch/arm/mach-msm/Kconfig
+++ b/arch/arm/mach-msm/Kconfig
@@ -150,7 +150,7 @@ endmenu

 config MSM_IOMMU
 	bool "MSM IOMMU Support"
-	depends on ARCH_MSM8X60
+	depends on ARCH_MSM8X60 || ARCH_MSM8960
 	select IOMMU_API
 	default n
 	help
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/3] msm: iommu: Create a Kconfig item for the IOMMU driver

[Daniel Walker]

On Fri, 2011-02-11 at 12:28 -0800, Stepan Moskovchenko wrote:
> +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.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 1/3] msm: iommu: Create a Kconfig item for the IOMMU driver

[David Brown]

On Fri, Feb 11 2011, Daniel Walker wrote:

> On Fri, 2011-02-11 at 12:28 -0800, Stepan Moskovchenko wrote:
>> +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.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/3] msm: iommu: Create a Kconfig item for the IOMMU driver

[Daniel Walker]

On Tue, 2011-02-15 at 11:35 -0800, David Brown wrote:
> On Fri, Feb 11 2011, Daniel Walker wrote:
> 
> > On Fri, 2011-02-11 at 12:28 -0800, Stepan Moskovchenko wrote:
> >> +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.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/3] msm: iommu: Create a Kconfig item for the IOMMU driver

[David Brown]

On Tue, Feb 15 2011, Daniel Walker wrote:

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

I want to try to resolve this particular series.  I had originally
pulled the series in because I thought everything had been addressed,
you followed this with a generic NAK.

You raised a couple of issues:

  - Should this be configurable?  I responded that our iommu is
    optional.  Drivers will work whether the iommu is enabled or not.
    Other architectures have configurable iommu drivers (and some are
    selected).  For example: arm/plat-s5p, powerpc/pasemi, as well x86.

    One reason to disable may be for building a slimmed kernel for
    kexec.

  - Should the iommu driver be under arch/arm/mach-msm.  As discussed in
    <https://lkml.org/lkml/2011/2/22/722>, iommu drivers fall into kind
    of a grey area.  Currently, most platform iommu drivers are in arch
    specific directories.

  - The renaming of the device file.  At this point, the two MSM chips
    that have an IOMMU have an identical IOMMU configuration.  The
    patches reflect this by removing the soc name from the file.  Future
    chips will probably have iommu hardware that differs, but it isn't
    possible to predict what the differences will be, so that will have
    to be addressed then.

I can certainly back out the changes if necessary.  Please let me know
if you have any specific concerns.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/3] msm: iommu: Create a Kconfig item for the IOMMU driver

[Daniel Walker]

On Tue, 2011-02-15 at 11:35 -0800, David Brown wrote:
> On Fri, Feb 11 2011, Daniel Walker wrote:
> 
> > On Fri, 2011-02-11 at 12:28 -0800, Stepan Moskovchenko wrote:
> >> +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.

This driver doesn't belong under mach-msm.

Daniel

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.