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 <[email protected]>
---
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.
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%)
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.
Allow IOMMU to be selected for MSM8960 now that the
platform data has been generalized.
Signed-off-by: Stepan Moskovchenko <[email protected]>
---
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.
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.
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 <[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%)
>
> 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.
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.
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.
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.
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.
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.
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.
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.
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.
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 <[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 ..
Daniel
--
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.
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 <[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 ..
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.
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 <[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 ..
>
> 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.
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.
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.
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.
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.
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.
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.