msm: iommu: Create a Kconfig item for the IOMMU driver
Subsystems (3)
PATCH 1/3 msm: iommu: Create a Kconfig item for the IOMMU driver
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.
--- arch/arm/mach-msm/Kconfig | 13 ++++++++++++- arch/arm/mach-msm/Makefile | 3 ++- 2 files changed, 14 insertions(+), 2 deletions(-)
arch/arm/mach-msm/Kconfig +12 -1
@@ -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
arch/arm/mach-msm/Makefile +2 -1
@@ -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
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.
---
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
@@ -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 +26 -28
133 lines not on this page · raw message
arch/arm/mach-msm/include/mach/msm_iomap-8x60.h +0 -36
@@ -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.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 51 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%)arch/arm/mach-msm/Makefile +2
-2@@ -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.oarch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +1
-3@@ -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 10 lines
arch/arm/mach-msm/devices-iommu.c renamed from arch/arm/mach-msm/devices-msm8x60-iommu.c +3
-3-#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
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
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.
thanks,
Steve
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
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? 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
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 11 lines · Daniel Walker
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 8 lines
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
0x07300000you instantly know that this JPEGD?
Yes, because it's the start address for the msm_iommu_jpegd resource.
thanks,
Steve
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 18 lines · Steve Muckle
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 4 lines
Quoted 2 lines
So your saying if you look at the number
0x07300000you instantly know that this JPEGD?Yes, because it's the start address for the
msm_iommu_jpegdresource.
Yeah I guess that's true .. I still think it's better design not to do it this way.
Daniel
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
In reply to Steve MuckleQuoted 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.
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 26 lines · David Brown
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.
Quoted 7 lines
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
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 6 lines · Daniel Walker
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.
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.
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 5 lines · David Brown
Quoted 2 lines · Daniel Walker
Quoted 1 line · David Brown
On Fri, Feb 11 2011, Steve Muckle wrote:
Quoted 8 lines
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.
Like I said in my example, people looking at the code won't always be debugging.
Quoted 7 lines
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.
I think your thinking of stuff that's not properly grouped.
Quoted 8 lines
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.
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
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
In reply to Stepan MoskovchenkoQuoted 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 ..
Daniel
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 14 lines · Daniel Walker
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%)
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.
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 16 lines · David Brown
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 7 lines
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.
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
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 10 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.
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
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH 2/3] msm: iommu: Generalize platform data for multiple targets
Quoted 9 lines · David Brown
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 8 lines
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.
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
Signature · 3 lines
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
In reply to Stepan MoskovchenkoAllow IOMMU to be selected for MSM8960 now that the platform data has been generalized.
--- arch/arm/mach-msm/Kconfig | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-msm/Kconfig +1 -1
@@ -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.
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.
Daniel
Signature · 3 lines
Sent by an consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
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.
David
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 18 lines · David Brown
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.
Quoted 3 lines
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
Signature · 2 lines
Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 6 lines · Daniel Walker
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
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 18 lines · David Brown
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.
Quoted 3 lines
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
Signature · 2 lines
Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.