OMAP: iommu: hwmod support and runtime PM
Subsystems (6)
v4 cover OMAP: iommu: hwmod support and runtime PM
Introduced hwmod support for OMAP3 (iva, isp) and OMAP4 (ipu, dsp), along with the corresponding runtime PM routines to deassert reset lines, enable/disable clocks and configure sysc registers.
v4:
- Rebased to 3.2-rc5 LO.
- Added missing clkdm field for OMAP4 hwmod data.
- Moved pm runtime enable/disable to probe/remove functions as suggested by MyungJoo Ham.
- Removed HWMOD_INIT_NO_RESET as it is not needed and because of Kevin Hilman's comments, the failure I was seeing without HWMOD_INIT_NO_RESET might have been related to local changes as it couldn't be reproduced anymore.
- Removed iommu latency as it is the same as the default omap latency.
- Tested on OMAP3 (zoom3 + tidspbridge) and OMAP4 (Panda + rpmsg).
v3:
- Rebased to 3.1-rc10 lo rebuilt, added structure terminators, and removed .omap_chip field.
v2:
- Added oh reset info to assert/deassert mmu reset lines.
- Addressed previous comments on v1
http://www.spinics.net/lists/arm-kernel/msg103271.html
Due to compatibility an ifdef needs to be propagated (previously on iommu resource info) to hwmod data in OMAP3, so users of iommu and tidspbridge can avoid issues of two modules managing mmu data/irqs/resets; this until tidspbridge can be safely migrated to iommu framework.
Omar Ramirez Luna (4):
OMAP3: hwmod data: add mmu data for iva and isp
OMAP4: hwmod data: add mmu hwmod for ipu and dsp OMAP3/4: iommu: migrate to hwmod framework OMAP3/4: iommu: adapt to runtime pm
arch/arm/mach-omap2/iommu2.c | 36 ------- arch/arm/mach-omap2/omap-iommu.c | 156 +++++----------------------- arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 135 ++++++++++++++++++++++++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 154 +++++++++++++++++++++++++-- arch/arm/plat-omap/include/plat/iommu.h | 17 +++- arch/arm/plat-omap/include/plat/iommu2.h | 2 - drivers/iommu/omap-iommu.c | 47 +++----- 7 files changed, 334 insertions(+), 213 deletions(-)
Signature · 1 line
1.7.4.1
PATCH v4 1/4 OMAP3: hwmod data: add mmu data for iva and isp
Add mmu hwmod data for iva and isp.
Due to compatibility an ifdef CONFIG_OMAP_IOMMU_IVA2 needs to be propagated (previously on iommu resource info) to hwmod data in OMAP3, so users of iommu and tidspbridge can avoid issues of two modules managing mmu data/irqs/resets; this until tidspbridge can be migrated to iommu framework.
--- arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 135 ++++++++++++++++++++++++++++ arch/arm/plat-omap/include/plat/iommu.h | 13 +++ 2 files changed, 148 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +135 -0
@@ -26,6 +26,7 @@#include <plat/mcbsp.h> #include <plat/mcspi.h> #include <plat/dmtimer.h> +#include <plat/iommu.h> #include "omap_hwmod_common_data.h"
@@ -2947,6 +2948,132 @@ static struct omap_hwmod omap34xx_mcspi4 = { };/* +* 'mmu' class +* The memory management unit performs virtual to physical address translation +* for its requestors. +*/ + +static struct omap_hwmod_class_sysconfig mmu_sysc = { + .rev_offs =0x000 , + .sysc_offs =0x010 , + .syss_offs =0x014 , + .sysc_flags = (SYSC_HAS_CLOCKACTIVITY |SYSC_HAS_SIDLEMODE | +SYSC_HAS_SOFTRESET |SYSC_HAS_AUTOIDLE ), + .idlemodes = (SIDLE_FORCE |SIDLE_NO |SIDLE_SMART ), + .sysc_fields = &omap_hwmod_sysc_type1, +}; + +static struct omap_hwmod_class omap3xxx_mmu_hwmod_class = { + .name ="mmu" , + .sysc = &mmu_sysc, +}; + +/* isp mmu */ + +static struct omap_mmu_dev_attr isp_mmu_dev_attr = { + .da_start =0x0 , + .da_end =0xfffff000 , + .nr_tlb_entries =8 , +}; + +static struct omap_hwmod omap3xxx_isp_mmu_hwmod; +static struct omap_hwmod_irq_info omap3xxx_isp_mmu_irqs[] = { + { .irq =24 }, + { .irq =-1 } +}; + +static struct omap_hwmod_addr_space omap3xxx_isp_mmu_addrs[] = { + { + .pa_start =0x480bd400 , + .pa_end =0x480bd47f , + .flags =ADDR_TYPE_RT , + }, + { } +}; + +/* l4_core -> isp mmu */ +static struct omap_hwmod_ocp_if omap3xxx_l4_core__isp_mmu = { + .master = &omap3xxx_l4_core_hwmod, + .slave = &omap3xxx_isp_mmu_hwmod, + .addr = omap3xxx_isp_mmu_addrs, + .user =OCP_USER_MPU |OCP_USER_SDMA , +}; + +/* isp mmu slave ports */ +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = { + &omap3xxx_l4_core__isp_mmu, +}; + +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = { + .name ="isp_mmu" , + .class = &omap3xxx_mmu_hwmod_class, + .mpu_irqs = omap3xxx_isp_mmu_irqs, + .main_clk ="cam_ick" , + .dev_attr = &isp_mmu_dev_attr, + .slaves = omap3xxx_isp_mmu_slaves, + .slaves_cnt =ARRAY_SIZE (omap3xxx_isp_mmu_slaves), + .flags =HWMOD_NO_IDLEST , +}; + +/* iva mmu */ + +static struct omap_mmu_dev_attr iva_mmu_dev_attr = { + .da_start =0x11000000 , + .da_end =0xfffff000 , + .nr_tlb_entries =32 , +}; + +static struct omap_hwmod omap3xxx_iva_mmu_hwmod; +static struct omap_hwmod_irq_info omap3xxx_iva_mmu_irqs[] = { + { .irq =28 }, + { .irq =-1 } +}; + +static struct omap_hwmod_rst_info omap3xxx_iva_mmu_resets[] = { + { .name ="mmu" , .rst_shift =1 , .st_shift =9 }, +}; + +static struct omap_hwmod_addr_space omap3xxx_iva_mmu_addrs[] = { + { + .pa_start =0x5d000000 , + .pa_end =0x5d00007f , + .flags =ADDR_TYPE_RT , + }, + { } +}; + +/* l3_main -> iva mmu */ +static struct omap_hwmod_ocp_if omap3xxx_l3_main__iva_mmu = { + .master = &omap3xxx_l3_main_hwmod, + .slave = &omap3xxx_iva_mmu_hwmod, + .addr = omap3xxx_iva_mmu_addrs, + .user =OCP_USER_MPU |OCP_USER_SDMA , +}; + +/* iva mmu slave ports */ +static struct omap_hwmod_ocp_if *omap3xxx_iva_mmu_slaves[] = { + &omap3xxx_l3_main__iva_mmu, +}; + +static struct omap_hwmod omap3xxx_iva_mmu_hwmod = { + .name ="iva_mmu" , + .class = &omap3xxx_mmu_hwmod_class, + .mpu_irqs = omap3xxx_iva_mmu_irqs, + .rst_lines = omap3xxx_iva_mmu_resets, + .rst_lines_cnt =ARRAY_SIZE (omap3xxx_iva_mmu_resets), + .main_clk ="iva2_ck" , + .prcm = { + .omap2 = { + .module_offs =OMAP3430_IVA2_MOD , + }, + }, + .dev_attr = &iva_mmu_dev_attr, + .slaves = omap3xxx_iva_mmu_slaves, + .slaves_cnt =ARRAY_SIZE (omap3xxx_iva_mmu_slaves), + .flags =HWMOD_NO_IDLEST , +}; + +/* * usbhsotg */ static struct omap_hwmod_class_sysconfig omap3xxx_usbhsotg_sysc = {
@@ -3264,7 +3391,11 @@ static __initdata struct omap_hwmod *omap3430es2plus_hwmods[] = {/* 34xx-only hwmods (all ES revisions) */ static __initdata struct omap_hwmod *omap34xx_hwmods[] = { + &omap3xxx_isp_mmu_hwmod, &omap3xxx_iva_hwmod, +#ifdef CONFIG_OMAP_IOMMU_IVA2 + &omap3xxx_iva_mmu_hwmod, +#endif &omap34xx_sr1_hwmod, &omap34xx_sr2_hwmod, &omap3xxx_mailbox_hwmod,
@@ -3273,7 +3404,11 @@ static __initdata struct omap_hwmod *omap34xx_hwmods[] = {/* 36xx-only hwmods (all ES revisions) */ static __initdata struct omap_hwmod *omap36xx_hwmods[] = { + &omap3xxx_isp_mmu_hwmod, &omap3xxx_iva_hwmod, +#ifdef CONFIG_OMAP_IOMMU_IVA2 + &omap3xxx_iva_mmu_hwmod, +#endif &omap3xxx_uart4_hwmod, &omap3xxx_dss_core_hwmod, &omap36xx_sr1_hwmod,
arch/arm/plat-omap/include/plat/iommu.h +13 -0
@@ -103,6 +103,19 @@ struct iommu_functions {ssize_t (*dump_ctx)(struct omap_iommu *obj,char *buf,ssize_t len); }; +/** +* omap_mmu_dev_attr - OMAP mmu device attributes for omap_hwmod +* @da_start: device address where the va space starts. +* @da_end: device address where the va space ends. +* @nr_tlb_entries: number of entries supported by the translation +* look-aside buffer (TLB). +*/ +struct omap_mmu_dev_attr { +u32 da_start; +u32 da_end; +int nr_tlb_entries; +}; +struct iommu_platform_data {const char *name;const char *clk_name;
Signature · 1 line
1.7.4.1
PATCH v4 1/4 OMAP3: hwmod data: add mmu data for iva and isp
Review
Quoted 7 lines · Omar Ramirez Luna
(quoted diff) +7
-0+/* l4_core -> isp mmu */ +static struct omap_hwmod_ocp_if omap3xxx_l4_core__isp_mmu = { + ? ? ? .master ? ? ? ? = &omap3xxx_l4_core_hwmod, + ? ? ? .slave ? ? ? ? ?= &omap3xxx_isp_mmu_hwmod, + ? ? ? .addr ? ? ? ? ? = omap3xxx_isp_mmu_addrs, + ? ? ? .user ? ? ? ? ? = OCP_USER_MPU | OCP_USER_SDMA, +};
Are you sure you are not missing something like:
.clk = "cam_ick",
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c static struct omap_hwmod omap34xx_mcspi4 = {
Quoted 10 lines
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +10
-0+/* isp mmu slave ports */ +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = { + ? ? ? &omap3xxx_l4_core__isp_mmu, +}; + +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = { + ? ? ? .name ? ? ? ? ? = "isp_mmu", + ? ? ? .class ? ? ? ? ?= &omap3xxx_mmu_hwmod_class, + ? ? ? .mpu_irqs ? ? ? = omap3xxx_isp_mmu_irqs, + ? ? ? .main_clk ? ? ? = "cam_ick",
It's not "cam_fck"?
Quoted 5 lines
+ ? ? ? .dev_attr ? ? ? =
&isp_mmu_dev_attr,
+ ? ? ? .slaves ? ? ? ? =omap3xxx_isp_mmu_slaves,
+ ? ? ? .slaves_cnt ? ? =ARRAY_SIZE(omap3xxx_isp_mmu_slaves),
+ ? ? ? .flags ? ? ? ? ?=HWMOD_NO_IDLEST,
+};
Most of the stuff I see the hwmods is .main_lock = "foo_fck", slave .clk = "foo_ick". Maybe that explains the irq issues you get.
Cheers.
Signature · 1 line
Felipe Contreras
PATCH v4 1/4 OMAP3: hwmod data: add mmu data for iva and isp
Quoted 11 lines · Felipe Contreras
Review
Quoted 7 lines · Omar Ramirez Luna
(quoted diff) +7
-0+/* l4_core -> isp mmu */ +static struct omap_hwmod_ocp_if omap3xxx_l4_core__isp_mmu = { + ? ? ? .master ? ? ? ? = &omap3xxx_l4_core_hwmod, + ? ? ? .slave ? ? ? ? ?= &omap3xxx_isp_mmu_hwmod, + ? ? ? .addr ? ? ? ? ? = omap3xxx_isp_mmu_addrs, + ? ? ? .user ? ? ? ? ? = OCP_USER_MPU | OCP_USER_SDMA, +};Are you sure you are not missing something like:
Quoted 1 line
?.clk = "
cam_ick",
I believe in this case cam_ick is used as the main clock as it supplies both functional and interface.
Quoted 13 lines
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c static struct omap_hwmod omap34xx_mcspi4 = {
Quoted 11 lines
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +10
-0+/* isp mmu slave ports */ +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = { + ? ? ? &omap3xxx_l4_core__isp_mmu, +}; + +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = { + ? ? ? .name ? ? ? ? ? = "isp_mmu", + ? ? ? .class ? ? ? ? ?= &omap3xxx_mmu_hwmod_class, + ? ? ? .mpu_irqs ? ? ? = omap3xxx_isp_mmu_irqs, + ? ? ? .main_clk ? ? ? = "cam_ick",It's not "
cam_fck"?
AFAIK cam_fck doesn't exist in the code, and CAM_L3_ICK is used as both ick/fck according to TRM.
Quoted 9 lines
Quoted 6 lines
+ ? ? ? .dev_attr ? ? ? =
&isp_mmu_dev_attr,
+ ? ? ? .slaves ? ? ? ? =omap3xxx_isp_mmu_slaves,
+ ? ? ? .slaves_cnt ? ? =ARRAY_SIZE(omap3xxx_isp_mmu_slaves),
+ ? ? ? .flags ? ? ? ? ?=HWMOD_NO_IDLEST,
+};Most of the stuff I see the hwmods is .main_lock = "
foo_fck", slave .clk = "foo_ick". Maybe that explains the irq issues you get.
I see irq issues with iva hwmod because tidspbridge doesn't use iommu API yet, so if you enable both the mmu hwmod and tidspbridge own mmu implementation there will be some conflicts.
I didn't see isp issues though, but I didn't went more than booting/enabling with isp mmu.
Regards,
Omar
PATCH v4 1/4 OMAP3: hwmod data: add mmu data for iva and isp
Quoted 8 lines · Omar
Quoted 3 lines · Felipe Contreras
Are you sure you are not missing something like:
?.clk = "
cam_ick",I believe in this case
cam_ickis used as the main clock as it supplies both functional and interface.
Are you sure?
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+/* isp mmu slave ports */ +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = { + ? ? ? &omap3xxx_l4_core__isp_mmu, +};Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = { + ? ? ? .name ? ? ? ? ? = "isp_mmu", + ? ? ? .class ? ? ? ? ?= &omap3xxx_mmu_hwmod_class, + ? ? ? .mpu_irqs ? ? ? = omap3xxx_isp_mmu_irqs, + ? ? ? .main_clk ? ? ? = "cam_ick",It's not "
cam_fck"?
Quoted 2 lines
AFAIK
cam_fckdoesn't exist in the code, andCAM_L3_ICKis used as both ick/fck according to TRM.
Then maybe the code is wrong.
Look at the OMAP34xx documentation, it says that:
CAM_L3_ICLK -> CAM_FCLKCAM_L4_ICLK -> CAM_ICLKCAM_MCLK -> CAM_MCLKCSI2_96M_FCLK -> CSI2_96M_FCLK
CAM_FCLK
Functional clock (L3 interconnect clock domain)
Functional clock domain.
CAM_ICLK
Interface clock (L4 interconnect clock domain)
Interface clock domain.
Maybe something like this:
arch/arm/mach-omap2/clock3xxx_data.c +11 -1
@@ -2225,8 +2225,17 @@ static struct clk cam_mclk = { .recalc = &followparent_recalc, }; +static struct clk cam_fck = { + .name ="cam_fck" , + .ops = &clkops_omap2_dflt, + .parent = &l3_ick, + .enable_reg =OMAP_CM_REGADDR (OMAP3430_CAM_MOD ,CM_FCLKEN ), + .enable_bit =OMAP3430_EN_CAM_SHIFT , + .clkdm_name ="cam_clkdm" , + .recalc = &followparent_recalc, +}; +static struct clk cam_ick = { -/* Handles both L3 and L4 clocks */ .name ="cam_ick" , .ops = &clkops_omap2_iclk_dflt, .parent = &l4_ick,
@@ -3364,6 +3373,7 @@ static struct omap_clk omap3xxx_clks[] = {CLK ("omapdss_dss" ,"ick" , &dss_ick_3430es1,CK_3430ES1 ),CLK ("omapdss_dss" ,"ick" , &dss_ick_3430es2,CK_3430ES2PLUS |CK_AM35XX |CK_36XX ),CLK (NULL ,"cam_mclk" , &cam_mclk,CK_34XX |CK_36XX ), +CLK (NULL ,"cam_fck" , &cam_fck,CK_34XX |CK_36XX ), CLK(NULL, "cam_ick", &cam_ick, CK_34XX | CK_36XX),
CLK(NULL, "csi2_96m_fck", &csi2_96m_fck, CK_34XX | CK_36XX),
CLK(NULL, "usbhost_120m_fck", &usbhost_120m_fck,CK_3430ES2PLUS | CK_AM35XX | CK_36XX),
Also, I find this chunk peculiar in drivers/media/video/omap3isp/isp.c:
static const char *isp_clocks[] = {
"cam_ick",
"cam_mclk",
"dpll4_m5_ck",
"csi2_96m_fck",
"l3_ick",
};Looks like the driver is manually calling clk_get() and clk_put() for the "i3_ick", where I guess the clock framework is supposed to do that... It's almost as if somebody forgot a dependency somewhere.
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
+ ? ? ? .dev_attr ? ? ? =
&isp_mmu_dev_attr,
+ ? ? ? .slaves ? ? ? ? =omap3xxx_isp_mmu_slaves,
+ ? ? ? .slaves_cnt ? ? =ARRAY_SIZE(omap3xxx_isp_mmu_slaves),
+ ? ? ? .flags ? ? ? ? ?=HWMOD_NO_IDLEST,
+};Most of the stuff I see the hwmods is .main_lock = "
foo_fck", slave .clk = "foo_ick". Maybe that explains the irq issues you get.
Quoted 6 lines
I see irq issues with iva hwmod because tidspbridge doesn't use iommu API yet, so if you enable both the mmu hwmod and tidspbridge own mmu implementation there will be some conflicts.
I didn't see isp issues though, but I didn't went more than booting/enabling with isp mmu.
This is what you said:
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance.
I'm not sure how this clock stuff works, but I'm guessing the device is supposed to go to sleep at some points in time, and with your patch "OMAP3/4: iommu: adapt to runtime pm" it won't, as long as the module is loaded, unless I'm missing something.
Cheers.
Signature · 1 line
Felipe Contreras
PATCH v4 1/4 OMAP3: hwmod data: add mmu data for iva and isp
Quoted 11 lines · Felipe Contreras
Quoted 8 lines · Omar
Quoted 3 lines · Felipe Contreras
Are you sure you are not missing something like:
?.clk = "
cam_ick",I believe in this case
cam_ickis used as the main clock as it supplies both functional and interface.Are you sure?
As sure as 4.7.4.1.7 CAM Power Domain ;), if someone else could clarify would be great to avoid the "are you sure" discussion.
Quoted 25 lines
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+/* isp mmu slave ports */ +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = { + ? ? ? &omap3xxx_l4_core__isp_mmu, +};Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = { + ? ? ? .name ? ? ? ? ? = "isp_mmu", + ? ? ? .class ? ? ? ? ?= &omap3xxx_mmu_hwmod_class, + ? ? ? .mpu_irqs ? ? ? = omap3xxx_isp_mmu_irqs, + ? ? ? .main_clk ? ? ? = "cam_ick",It's not "
cam_fck"?Quoted 2 lines
AFAIK
cam_fckdoesn't exist in the code, andCAM_L3_ICKis used as both ick/fck according to TRM.Then maybe the code is wrong.
Look at the OMAP34xx documentation, it says that:
CAM_L3_ICLK->CAM_FCLKCAM_L4_ICLK->CAM_ICLKCAM_MCLK->CAM_MCLKCSI2_96M_FCLK->CSI2_96M_FCLK
Quoted 7 lines
CAM_FCLK
Functional clock (L3 interconnect clock domain)
Functional clock domain.
CAM_ICLK
Interface clock (L4 interconnect clock domain)
Interface clock domain.
"The camera subsystem interface is clocked with the L3 and L4 clocks (CAM_L3_ICLK and CAM_L4_ICLK, respectively). CAM_L3_ICLK is also used as the main functional clock. The functional clock (CAM_MCLK) is provided by DPLL4 to supply the external sensor."
Either CAM subsystem section or PRCM - CAM PWRDM is ambiguous or wrong.
...
Quoted 4 lines
Looks like the driver is manually calling
clk_get()andclk_put()for the "i3_ick", where I guess the clock framework is supposed to do that... It's almost as if somebody forgot a dependency somewhere.
Would be good to clarify the intentions to keep the code as it is.
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines
Quoted 5 lines
+ ? ? ? .dev_attr ? ? ? =
&isp_mmu_dev_attr,
+ ? ? ? .slaves ? ? ? ? =omap3xxx_isp_mmu_slaves,
+ ? ? ? .slaves_cnt ? ? =ARRAY_SIZE(omap3xxx_isp_mmu_slaves),
+ ? ? ? .flags ? ? ? ? ?=HWMOD_NO_IDLEST,
+};Most of the stuff I see the hwmods is .main_lock = "
foo_fck", slave .clk = "foo_ick". Maybe that explains the irq issues you get.I see irq issues with iva hwmod because tidspbridge doesn't use iommu API yet, so if you enable both the mmu hwmod and tidspbridge own mmu implementation there will be some conflicts.
Quoted 6 lines
Quoted 2 lines
I didn't see isp issues though, but I didn't went more than booting/enabling with isp mmu.
This is what you said:
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance.
Yes, you should receive an interrupt if the clock is enabled and the iommu is being used, hence the part inside in the ISR to enable the clocks was removed.
Quoted 4 lines
I'm not sure how this clock stuff works, but I'm guessing the device is supposed to go to sleep at some points in time, and with your patch "OMAP3/4: iommu: adapt to runtime pm" it won't, as long as the module is loaded, unless I'm missing something.
The device should be able to be put to sleep at anytime when it is NOT being used. AFAIK there is no mechanism for the main processor (the one running the kernel) to know when the other iommus are not being used, given that they are in independent processors/subsystems, at least for the ones in the DSP or M3 processors. Once the user releases its iommu resource it means it is no longer using it, at that point the device can be put to sleep.
Regards,
Omar
PATCH v4 1/4 OMAP3: hwmod data: add mmu data for iva and isp
Hi Omar,
Quoted 7 lines · Omar
Quoted 6 lines · Felipe Contreras
Quoted 5 lines · Omar
Quoted 3 lines · Felipe Contreras
Are you sure you are not missing something like:
.clk = "
cam_ick",
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I believe in this case
cam_ickis used as the main clock as it supplies both functional and interface.Are you sure?
As sure as 4.7.4.1.7 CAM Power Domain ;), if someone else could clarify would be great to avoid the "are you sure" discussion.
I don't know where the ISP IOMMU fonctional and interface clocks come from, sorry. Shouldn't it be possible to find someone inside TI who can provide that information ? :-)
Quoted 33 lines
Quoted 25 lines
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+/* isp mmu slave ports */ +static struct omap_hwmod_ocp_if *omap3xxx_isp_mmu_slaves[] = { + &omap3xxx_l4_core__isp_mmu, +};Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ +static struct omap_hwmod omap3xxx_isp_mmu_hwmod = { + .name = "isp_mmu", + .class = &omap3xxx_mmu_hwmod_class, + .mpu_irqs = omap3xxx_isp_mmu_irqs, + .main_clk = "cam_ick",It's not "
cam_fck"?Quoted 2 lines
AFAIK
cam_fckdoesn't exist in the code, andCAM_L3_ICKis used as both ick/fck according to TRM.Then maybe the code is wrong.
Look at the OMAP34xx documentation, it says that:
CAM_L3_ICLK->CAM_FCLKCAM_L4_ICLK->CAM_ICLKCAM_MCLK->CAM_MCLKCSI2_96M_FCLK->CSI2_96M_FCLKQuoted 7 lines
CAM_FCLK
Functional clock (L3 interconnect clock domain)
Functional clock domain.
CAM_ICLK
Interface clock (L4 interconnect clock domain)
Interface clock domain.
Quoted 6 lines
"The camera subsystem interface is clocked with the L3 and L4 clocks (
CAM_L3_ICLKandCAM_L4_ICLK, respectively).CAM_L3_ICLKis also used as the main functional clock. The functional clock (CAM_MCLK) is provided by DPLL4 to supply the external sensor."Either CAM subsystem section or PRCM - CAM PWRDM is ambiguous or wrong.
I haven't checked how the clocks are used in details (I started working on the OMAP3 ISP driver after the clock handling code was in place), but I can at least confirm that CAM_L3_ICLK is the main functional clock. I would be a bit surprised if CAM_L3_ICLK was used as the interface clock as well.
Once again, you should be able to find clarification on this subject inside TI.
Quoted 7 lines
...
Quoted 3 lines
Looks like the driver is manually calling
clk_get()andclk_put()for the "i3_ick", where I guess the clock framework is supposed to do that... It's almost as if somebody forgot a dependency somewhere.Would be good to clarify the intentions to keep the code as it is.
Once proper clock dependencies will be in place I'll be glad to remove the direct l3_ick if needed.
Quoted 29 lines
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ .dev_attr = &isp_mmu_dev_attr, + .slaves = omap3xxx_isp_mmu_slaves, + .slaves_cnt = ARRAY_SIZE(omap3xxx_isp_mmu_slaves), + .flags = HWMOD_NO_IDLEST, +};Most of the stuff I see the hwmods is .main_lock = "
foo_fck", slave .clk = "foo_ick". Maybe that explains the irq issues you get.I see irq issues with iva hwmod because tidspbridge doesn't use iommu API yet, so if you enable both the mmu hwmod and tidspbridge own mmu implementation there will be some conflicts.
Quoted 6 lines
Quoted 2 lines
I didn't see isp issues though, but I didn't went more than booting/enabling with isp mmu.
This is what you said:
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance.Yes, you should receive an interrupt if the clock is enabled and the iommu is being used, hence the part inside in the ISR to enable the clocks was removed.
Quoted 4 lines
I'm not sure how this clock stuff works, but I'm guessing the device is supposed to go to sleep at some points in time, and with your patch "OMAP3/4: iommu: adapt to runtime pm" it won't, as long as the module is loaded, unless I'm missing something.
Quoted 7 lines
The device should be able to be put to sleep at anytime when it is NOT being used. AFAIK there is no mechanism for the main processor (the one running the kernel) to know when the other iommus are not being used, given that they are in independent processors/subsystems, at least for the ones in the DSP or M3 processors. Once the user releases its iommu resource it means it is no longer using it, at that point the device can be put to sleep.
How should the OMAP3 ISP driver proceed to make sure that its IOMMU is clocked off when it doesn't need it ?
Signature · 3 lines
Regards, Laurent Pinchart
PATCH v4 1/4 OMAP3: hwmod data: add mmu data for iva and isp
Hi Laurent
Quoted 13 lines · Laurent Pinchart
Quoted 5 lines
Quoted 4 lines
I'm not sure how this clock stuff works, but I'm guessing the device is supposed to go to sleep at some points in time, and with your patch "OMAP3/4: iommu: adapt to runtime pm" it won't, as long as the module is loaded, unless I'm missing something.
Quoted 7 lines
The device should be able to be put to sleep at anytime when it is NOT being used. AFAIK there is no mechanism for the main processor (the one running the kernel) to know when the other iommus are not being used, given that they are in independent processors/subsystems, at least for the ones in the DSP or M3 processors. Once the user releases its iommu resource it means it is no longer using it, at that point the device can be put to sleep.
Quoted 2 lines
How should the OMAP3 ISP driver proceed to make sure that its IOMMU is clocked off when it doesn't need it ?
If there is an specific scenario where the iommu should be disabled, iommu_detach_device can be called to release the iommu resource. On suspend/resume scenarios runtime pm callbacks should still be able to put the device in idle.
Regards,
Omar
PATCH v4 1/4 OMAP3: hwmod data: add mmu data for iva and isp
Hi Omar,
Quoted 14 lines · Omar
Quoted 13 lines · Laurent Pinchart
Quoted 12 lines
Quoted 4 lines
I'm not sure how this clock stuff works, but I'm guessing the device is supposed to go to sleep at some points in time, and with your patch "OMAP3/4: iommu: adapt to runtime pm" it won't, as long as the module is loaded, unless I'm missing something.
The device should be able to be put to sleep at anytime when it is NOT being used. AFAIK there is no mechanism for the main processor (the one running the kernel) to know when the other iommus are not being used, given that they are in independent processors/subsystems, at least for the ones in the DSP or M3 processors. Once the user releases its iommu resource it means it is no longer using it, at that point the device can be put to sleep.
Quoted 7 lines
Quoted 2 lines
How should the OMAP3 ISP driver proceed to make sure that its IOMMU is clocked off when it doesn't need it ?
If there is an specific scenario where the iommu should be disabled,
iommu_detach_devicecan be called to release the iommu resource. On suspend/resume scenarios runtime pm callbacks should still be able to put the device in idle.
Runtime PM might indeed be a better option. The OMAP3 ISP doesn't need to IOMMU until video streams are started, so we should keep it powered down in that case.
Signature · 3 lines
Regards, Laurent Pinchart
PATCH v4 2/4 OMAP4: hwmod data: add mmu hwmod for ipu and dsp
In reply to Omar Ramirez LunaAdd mmu hwmod data for ipu and dsp.
--- arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 154 +++++++++++++++++++++++++-- 1 files changed, 142 insertions(+), 12 deletions(-)
arch/arm/mach-omap2/omap_hwmod_44xx_data.c +142 -12
@@ -31,6 +31,7 @@#include <plat/i2c.h> #include <plat/dmtimer.h> #include <plat/common.h> +#include <plat/iommu.h> #include "omap_hwmod_common_data.h"
@@ -1102,10 +1103,6 @@ static struct omap_hwmod_irq_info omap44xx_dsp_irqs[] = { { .irq = -1 } }; -static struct omap_hwmod_rst_info omap44xx_dsp_resets[] = { - { .name ="mmu_cache" , .rst_shift =1 }, -}; -static struct omap_hwmod_rst_info omap44xx_dsp_c0_resets[] = { { .name ="dsp" , .rst_shift =0 }, };
@@ -1157,8 +1154,6 @@ static struct omap_hwmod omap44xx_dsp_hwmod = { .class = &omap44xx_dsp_hwmod_class, .clkdm_name = "tesla_clkdm", .mpu_irqs = omap44xx_dsp_irqs, - .rst_lines = omap44xx_dsp_resets, - .rst_lines_cnt = ARRAY_SIZE(omap44xx_dsp_resets), .main_clk = "dsp_fck", .prcm = { .omap4 = {
@@ -2507,10 +2502,6 @@ static struct omap_hwmod_rst_info omap44xx_ipu_c1_resets[] = { { .name = "cpu1", .rst_shift = 1 }, }; -static struct omap_hwmod_rst_info omap44xx_ipu_resets[] = { - { .name ="mmu_cache" , .rst_shift =2 }, -}; -/* ipu master ports */ static struct omap_hwmod_ocp_if *omap44xx_ipu_masters[] = { &omap44xx_ipu__l3_main_2,
@@ -2564,8 +2555,6 @@ static struct omap_hwmod omap44xx_ipu_hwmod = { .class = &omap44xx_ipu_hwmod_class, .clkdm_name = "ducati_clkdm", .mpu_irqs = omap44xx_ipu_irqs, - .rst_lines = omap44xx_ipu_resets, - .rst_lines_cnt = ARRAY_SIZE(omap44xx_ipu_resets), .main_clk = "ipu_fck", .prcm = { .omap4 = {
@@ -3932,6 +3921,143 @@ static struct omap_hwmod omap44xx_mpu_hwmod = { };/* +* 'mmu' class +* The memory management unit performs virtual to physical address translation +* for its requestors. +*/ + +static struct omap_hwmod_class_sysconfig mmu_sysc = { + .rev_offs =0x000 , + .sysc_offs =0x010 , + .syss_offs =0x014 , + .sysc_flags = (SYSC_HAS_CLOCKACTIVITY |SYSC_HAS_SIDLEMODE | +SYSC_HAS_SOFTRESET |SYSC_HAS_AUTOIDLE ), + .idlemodes = (SIDLE_FORCE |SIDLE_NO |SIDLE_SMART ), + .sysc_fields = &omap_hwmod_sysc_type1, +}; + +static struct omap_hwmod_class omap44xx_mmu_hwmod_class = { + .name ="mmu" , + .sysc = &mmu_sysc, +}; + +/* ipu mmu */ + +static struct omap_mmu_dev_attr ipu_mmu_dev_attr = { + .da_start =0x0 , + .da_end =0xfffff000 , + .nr_tlb_entries =32 , +}; + +static struct omap_hwmod omap44xx_ipu_mmu_hwmod; +static struct omap_hwmod_irq_info omap44xx_ipu_mmu_irqs[] = { + { .irq =100 +OMAP44XX_IRQ_GIC_START , }, + { .irq =-1 } +}; + +static struct omap_hwmod_rst_info omap44xx_ipu_mmu_resets[] = { + { .name ="mmu_cache" , .rst_shift =2 }, +}; + +static struct omap_hwmod_addr_space omap44xx_ipu_mmu_addrs[] = { + { + .pa_start =0x55082000 , + .pa_end =0x550820ff , + .flags =ADDR_TYPE_RT , + }, + { } +}; + +/* l3_main_1 -> ipu mmu */ +static struct omap_hwmod_ocp_if omap44xx_l3_main_1__ipu_mmu = { + .master = &omap44xx_l3_main_1_hwmod, + .slave = &omap44xx_ipu_mmu_hwmod, + .addr = omap44xx_ipu_mmu_addrs, + .user =OCP_USER_MPU |OCP_USER_SDMA , +}; + +/* ipu mmu slave ports */ +static struct omap_hwmod_ocp_if *omap44xx_ipu_mmu_slaves[] = { + &omap44xx_l3_main_1__ipu_mmu, +}; + +static struct omap_hwmod omap44xx_ipu_mmu_hwmod = { + .name ="ipu_mmu" , + .class = &omap44xx_mmu_hwmod_class, + .clkdm_name ="ducati_clkdm" , + .mpu_irqs = omap44xx_ipu_mmu_irqs, + .rst_lines = omap44xx_ipu_mmu_resets, + .rst_lines_cnt =ARRAY_SIZE (omap44xx_ipu_mmu_resets), + .main_clk ="ipu_fck" , + .prcm = { + .omap4 = { + .rstctrl_offs =OMAP4_RM_DUCATI_RSTCTRL_OFFSET , + }, + }, + .dev_attr = &ipu_mmu_dev_attr, + .slaves = omap44xx_ipu_mmu_slaves, + .slaves_cnt =ARRAY_SIZE (omap44xx_ipu_mmu_slaves), +}; + +/* dsp mmu */ + +static struct omap_mmu_dev_attr dsp_mmu_dev_attr = { + .da_start =0x0 , + .da_end =0xfffff000 , + .nr_tlb_entries =32 , +}; + +static struct omap_hwmod omap44xx_dsp_mmu_hwmod; +static struct omap_hwmod_irq_info omap44xx_dsp_mmu_irqs[] = { + { .irq =28 +OMAP44XX_IRQ_GIC_START }, + { .irq =-1 } +}; + +static struct omap_hwmod_rst_info omap44xx_dsp_mmu_resets[] = { + { .name ="mmu_cache" , .rst_shift =1 }, +}; + +static struct omap_hwmod_addr_space omap44xx_dsp_mmu_addrs[] = { + { + .pa_start =0x4a066000 , + .pa_end =0x4a0660ff , + .flags =ADDR_TYPE_RT , + }, + { } +}; + +/* l3_main_1 -> dsp mmu */ +static struct omap_hwmod_ocp_if omap44xx_l3_main_1__dsp_mmu = { + .master = &omap44xx_l3_main_1_hwmod, + .slave = &omap44xx_dsp_mmu_hwmod, + .addr = omap44xx_dsp_mmu_addrs, + .user =OCP_USER_MPU |OCP_USER_SDMA , +}; + +/* dsp mmu slave ports */ +static struct omap_hwmod_ocp_if *omap44xx_dsp_mmu_slaves[] = { + &omap44xx_l3_main_1__dsp_mmu, +}; + +static struct omap_hwmod omap44xx_dsp_mmu_hwmod = { + .name ="dsp_mmu" , + .class = &omap44xx_mmu_hwmod_class, + .clkdm_name ="tesla_clkdm" , + .mpu_irqs = omap44xx_dsp_mmu_irqs, + .rst_lines = omap44xx_dsp_mmu_resets, + .rst_lines_cnt =ARRAY_SIZE (omap44xx_dsp_mmu_resets), + .main_clk ="dsp_fck" , + .prcm = { + .omap4 = { + .rstctrl_offs =OMAP4_RM_TESLA_RSTCTRL_OFFSET , + }, + }, + .dev_attr = &dsp_mmu_dev_attr, + .slaves = omap44xx_dsp_mmu_slaves, + .slaves_cnt =ARRAY_SIZE (omap44xx_dsp_mmu_slaves), +}; + +/* * 'smartreflex' class * smartreflex module (monitor silicon performance and outputs a measure of * performance error)
@@ -5388,6 +5514,10 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {/* mpu class */ &omap44xx_mpu_hwmod, +/* mmu class */ + &omap44xx_ipu_mmu_hwmod, + &omap44xx_dsp_mmu_hwmod, +/* smartreflex class */ &omap44xx_smartreflex_core_hwmod, &omap44xx_smartreflex_iva_hwmod,
Signature · 1 line
1.7.4.1
PATCH v4 3/4 OMAP3/4: iommu: migrate to hwmod framework
In reply to Omar Ramirez LunaUse hwmod data and device attributes to build and register an omap device for iommu driver.
- Update the naming convention in isp module.
- Remove unneeded check for number of resources, as this is now handled by omap_device and prevents driver from loading.
- Now unused, remove platform device and resource data, handling of sysconfig register for softreset purposes, use default latency structure.
--- arch/arm/mach-omap2/iommu2.c | 19 ---- arch/arm/mach-omap2/omap-iommu.c | 157 +++++-------------------------- arch/arm/plat-omap/include/plat/iommu.h | 2 +- drivers/iommu/omap-iommu.c | 3 - 4 files changed, 27 insertions(+), 154 deletions(-)
arch/arm/mach-omap2/iommu2.c +0 -19
@@ -32,12 +32,8 @@#define MMU_SYS_IDLE_SMART (2 << MMU_SYS_IDLE_SHIFT)#define MMU_SYS_IDLE_MASK (3 << MMU_SYS_IDLE_SHIFT) -#define MMU_SYS_SOFTRESET (1 << 1)#define MMU_SYS_AUTOIDLE 1 -/* SYSSTATUS */ -#define MMU_SYS_RESETDONE 1 -/* IRQSTATUS & IRQENABLE */ #define MMU_IRQ_MULTIHITFAULT (1 << 4)#define MMU_IRQ_TABLEWALKFAULT (1 << 3)
@@ -88,7 +84,6 @@ static void __iommu_set_twl(struct omap_iommu *obj, bool on)static int omap2_iommu_enable (struct omap_iommu *obj) {u32 l, pa; -unsigned long timeout;if (!obj->iopgd || !IS_ALIGNED ((u32 )obj->iopgd,SZ_16K ))return -EINVAL ;
@@ -97,20 +92,6 @@ static int omap2_iommu_enable(struct omap_iommu *obj)if (!IS_ALIGNED (pa,SZ_16K ))return -EINVAL ; -iommu_write_reg (obj,MMU_SYS_SOFTRESET ,MMU_SYSCONFIG ); - - timeout = jiffies +msecs_to_jiffies (20 ); -do { - l =iommu_read_reg (obj,MMU_SYSSTATUS ); -if (l &MMU_SYS_RESETDONE ) -break ; - }while (!time_after (jiffies, timeout)); - -if (!(l &MMU_SYS_RESETDONE )) { -dev_err (obj->dev,"can't take mmu out of reset\n" ); -return -ENODEV ; - } - l =iommu_read_reg (obj,MMU_REVISION );dev_info (obj->dev,"%s: version %d.%d\n" , obj->name, (l >>4 ) &0xf , l &0xf );
arch/arm/mach-omap2/omap-iommu.c +26 -131
178 lines not on this page · raw message
arch/arm/plat-omap/include/plat/iommu.h +1 -1
@@ -119,7 +119,7 @@ struct omap_mmu_dev_attr {struct iommu_platform_data {const char *name;const char *clk_name; -const int nr_tlb_entries; +int nr_tlb_entries;u32 da_start;u32 da_end; };
drivers/iommu/omap-iommu.c +0 -3
@@ -916,9 +916,6 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)struct resource *res;struct iommu_platform_data *pdata = pdev->dev.platform_data; -if (pdev->num_resources !=2 ) -return -EINVAL ; - obj =kzalloc (sizeof (*obj) +MMU_REG_SIZE ,GFP_KERNEL );if (!obj)return -ENOMEM ;
Signature · 1 line
1.7.4.1
PATCH v4 3/4 OMAP3/4: iommu: migrate to hwmod framework
Quoted 9 lines · Omar Ramirez Luna
Use hwmod data and device attributes to build and register an omap device for iommu driver.
?- Update the naming convention in isp module.
?- Remove unneeded check for number of resources, as this is now ? handled byomap_deviceand prevents driver from loading. ?- Now unused, remove platform device and resource data, handling ? of sysconfig register for softreset purposes, use default ? latency structure.
Looks good to me.
Signature · 1 line
Felipe Contreras
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
In reply to Omar Ramirez LunaUse runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling.
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance. Now doing pm_runtime_get/put on iommu_enable/disable so it doesn't rely on others to keep the clocks on.
--- arch/arm/mach-omap2/iommu2.c | 17 ----------- arch/arm/mach-omap2/omap-iommu.c | 1 - arch/arm/plat-omap/include/plat/iommu.h | 2 - arch/arm/plat-omap/include/plat/iommu2.h | 2 - drivers/iommu/omap-iommu.c | 44 ++++++++++++----------------- 5 files changed, 18 insertions(+), 48 deletions(-)
arch/arm/mach-omap2/iommu2.c +0 -17
@@ -25,15 +25,6 @@*/ #define IOMMU_ARCH_VERSION 0x00000011 -/* SYSCONF */ -#define MMU_SYS_IDLE_SHIFT 3 -#define MMU_SYS_IDLE_FORCE (0 << MMU_SYS_IDLE_SHIFT) -#define MMU_SYS_IDLE_NONE (1 << MMU_SYS_IDLE_SHIFT) -#define MMU_SYS_IDLE_SMART (2 << MMU_SYS_IDLE_SHIFT) -#define MMU_SYS_IDLE_MASK (3 << MMU_SYS_IDLE_SHIFT) - -#define MMU_SYS_AUTOIDLE 1 -/* IRQSTATUS & IRQENABLE */ #define MMU_IRQ_MULTIHITFAULT (1 << 4)#define MMU_IRQ_TABLEWALKFAULT (1 << 3)
@@ -96,11 +87,6 @@ static int omap2_iommu_enable(struct omap_iommu *obj)dev_info (obj->dev,"%s: version %d.%d\n" , obj->name, (l >>4 ) &0xf , l &0xf ); - l =iommu_read_reg (obj,MMU_SYSCONFIG ); - l &= ~MMU_SYS_IDLE_MASK ; - l |= (MMU_SYS_IDLE_SMART |MMU_SYS_AUTOIDLE ); -iommu_write_reg (obj, l,MMU_SYSCONFIG ); -iommu_write_reg (obj, pa,MMU_TTB );__iommu_set_twl (obj, true);
@@ -114,7 +100,6 @@ static void omap2_iommu_disable(struct omap_iommu *obj) l &= ~MMU_CNTL_MASK ;iommu_write_reg (obj, l,MMU_CNTL ); -iommu_write_reg (obj,MMU_SYS_IDLE_FORCE ,MMU_SYSCONFIG );dev_dbg (obj->dev,"%s is shutting down\n" , obj->name); }
@@ -243,8 +228,6 @@ omap2_iommu_dump_ctx(struct omap_iommu *obj, char *buf, ssize_t len)char *p = buf;pr_reg (REVISION ); -pr_reg (SYSCONFIG ); -pr_reg (SYSSTATUS );pr_reg (IRQSTATUS );pr_reg (IRQENABLE );pr_reg (WALKING_ST );
arch/arm/mach-omap2/omap-iommu.c +0 -1
@@ -27,7 +27,6 @@ static int __init omap_iommu_dev_init(struct omap_hwmod *oh, void *unused)static int i; pdata.name = oh->name; - pdata.clk_name = oh->main_clk; pdata.nr_tlb_entries = a->nr_tlb_entries; pdata.da_start = a->da_start; pdata.da_end = a->da_end;
arch/arm/plat-omap/include/plat/iommu.h +0 -2
@@ -28,7 +28,6 @@ struct iotlb_entry {struct omap_iommu {const char *name;struct module *owner; -struct clk *clk;void __iomem *regbase;struct device *dev;void *isr_priv;
@@ -118,7 +117,6 @@ struct omap_mmu_dev_attr {struct iommu_platform_data {const char *name; -const char *clk_name;int nr_tlb_entries;u32 da_start;u32 da_end;
arch/arm/plat-omap/include/plat/iommu2.h +0 -2
@@ -19,8 +19,6 @@* MMU Register offsets */ #define MMU_REVISION 0x00 -#define MMU_SYSCONFIG 0x10 -#define MMU_SYSSTATUS 0x14#define MMU_IRQSTATUS 0x18#define MMU_IRQENABLE 0x1c#define MMU_WALKING_ST 0x40
drivers/iommu/omap-iommu.c +18 -26
165 lines not on this page · raw message
Signature · 1 line
1.7.4.1
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Review static int iommu_enable(struct omap_iommu *obj) · line 131
Quoted 12 lines · Omar Ramirez Luna
(quoted diff) +1
-2@@ -123,11 +123,10 @@ static int iommu_enable(struct omap_iommu *obj) ? ? ? ?if (!arch_iommu) ? ? ? ? ? ? ? ?return -ENODEV; - ? ? ? clk_enable(obj->clk); + ? ? ? pm_runtime_get_sync(obj->dev); ? ? ? ?err = arch_iommu->enable(obj); - ? ? ? clk_disable(obj->clk); ? ? ? ?return err; ?}
Hmm, this is called on omap_iommu_attach, and iommu_disable is not called until omap_iommu_detach, so this means the device will never sleep. Why don't you call pm_runtime_put() instead of clk_disable()?
All the rest of the calls to pm_runtime_get/put after this are basically no-ops, because the count will never go below 1.
You mention some irq issues, but could it be due to some bad clocks in the hwmod data?
Quoted 109 lines
(quoted diff) +1
-3@@ -136,11 +135,9 @@ static void iommu_disable(struct omap_iommu *obj) ? ? ? ?if (!obj) ? ? ? ? ? ? ? ?return; - ? ? ? clk_enable(obj->clk); - ? ? ? ?arch_iommu->disable(obj); - ? ? ? clk_disable(obj->clk); + ? ? ? pm_runtime_put(obj->dev); ?}?/*
(quoted diff) +2
-2@@ -263,7 +260,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e) ? ? ? ?if (!obj || !obj->nr_tlb_entries || !e) ? ? ? ? ? ? ? ?return -EINVAL; - ? ? ? clk_enable(obj->clk); + ? ? ? pm_runtime_get_sync(obj->dev); ? ? ? ?iotlb_lock_get(obj, &l); ? ? ? ?if (l.base == obj->nr_tlb_entries) {@@ -293,7 +290,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e) ? ? ? ?cr = iotlb_alloc_cr(obj, e); ? ? ? ?if (IS_ERR(cr)) { - ? ? ? ? ? ? ? clk_disable(obj->clk); + ? ? ? ? ? ? ? pm_runtime_put(obj->dev); ? ? ? ? ? ? ? ?return PTR_ERR(cr); ? ? ? ?}@@ -307,7 +304,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e) ? ? ? ? ? ? ? ?l.vict = l.base; ? ? ? ?iotlb_lock_set(obj, &l);?out:
- ? ? ?clk_disable(obj->clk);
+ ? ? ?pm_runtime_put(obj->dev);
? ? ? ?return err;
?}(quoted diff) +8
-8@@ -337,7 +334,7 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da) ? ? ? ?int i; ? ? ? ?struct cr_regs cr; - ? ? ? clk_enable(obj->clk); + ? ? ? pm_runtime_get_sync(obj->dev); ? ? ? ?for_each_iotlb_cr(obj, obj->nr_tlb_entries, i, cr) { ? ? ? ? ? ? ? ?u32 start;@@ -356,7 +353,7 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da) ? ? ? ? ? ? ? ? ? ? ? ?iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY); ? ? ? ? ? ? ? ?} ? ? ? ?} - ? ? ? clk_disable(obj->clk); + ? ? ? pm_runtime_put(obj->dev); ? ? ? ?if (i == obj->nr_tlb_entries) ? ? ? ? ? ? ? ?dev_dbg(obj->dev, "%s: no page for %08x\n", __func__, da);@@ -370,7 +367,7 @@ static void flush_iotlb_all(struct omap_iommu *obj) ?{ ? ? ? ?struct iotlb_lock l; - ? ? ? clk_enable(obj->clk); + ? ? ? pm_runtime_get_sync(obj->dev); ? ? ? ?l.base = 0; ? ? ? ?l.vict = 0;@@ -378,7 +375,7 @@ static void flush_iotlb_all(struct omap_iommu *obj) ? ? ? ?iommu_write_reg(obj, 1, MMU_GFLUSH); - ? ? ? clk_disable(obj->clk); + ? ? ? pm_runtime_put(obj->dev); ?} ?#if defined(CONFIG_OMAP_IOMMU_DEBUG) || defined(CONFIG_OMAP_IOMMU_DEBUG_MODULE)@@ -388,11 +385,11 @@ ssize_t omap_iommu_dump_ctx(struct omap_iommu *obj, char *buf, ssize_t bytes) ? ? ? ?if (!obj || !buf) ? ? ? ? ? ? ? ?return -EINVAL; - ? ? ? clk_enable(obj->clk); + ? ? ? pm_runtime_get_sync(obj->dev); ? ? ? ?bytes = arch_iommu->dump_ctx(obj, buf, bytes); - ? ? ? clk_disable(obj->clk); + ? ? ? pm_runtime_put(obj->dev); ? ? ? ?return bytes; ?}@@ -406,7 +403,7 @@ __dump_tlb_entries(struct omap_iommu *obj, struct cr_regs *crs, int num) ? ? ? ?struct cr_regs tmp; ? ? ? ?struct cr_regs *p = crs; - ? ? ? clk_enable(obj->clk); + ? ? ? pm_runtime_get_sync(obj->dev); ? ? ? ?iotlb_lock_get(obj, &saved); ? ? ? ?for_each_iotlb_cr(obj, num, i, tmp) {@@ -416,7 +413,7 @@ __dump_tlb_entries(struct omap_iommu *obj, struct cr_regs *crs, int num) ? ? ? ?} ? ? ? ?iotlb_lock_set(obj, &saved); - ? ? ? clk_disable(obj->clk); + ? ? ? pm_runtime_put(obj->dev); ? ? ? ?return ?p - crs; ?}
Review static irqreturn_t iommu_fault_handler(int irq, void *data) · line 785
Quoted 6 lines
(quoted diff) +0
-2@@ -780,9 +777,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) ? ? ? ?if (!obj->refcount) ? ? ? ? ? ? ? ?return IRQ_NONE; - ? ? ? clk_enable(obj->clk); ? ? ? ?errs = iommu_report_fault(obj, &da); - ? ? ? clk_disable(obj->clk);
Why?
Quoted 33 lines
(quoted diff) +2
-4? ? ? ?if (errs == 0) ? ? ? ? ? ? ? ?return IRQ_HANDLED;@@ -920,10 +915,6 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev) ? ? ? ?if (!obj) ? ? ? ? ? ? ? ?return -ENOMEM; - ? ? ? obj->clk = clk_get(&pdev->dev, pdata->clk_name); - ? ? ? if (IS_ERR(obj->clk)) - ? ? ? ? ? ? ? goto err_clk; - ? ? ? ?obj->nr_tlb_entries = pdata->nr_tlb_entries; ? ? ? ?obj->name = pdata->name; ? ? ? ?obj->dev = &pdev->dev;@@ -966,6 +957,8 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev) ? ? ? ? ? ? ? ?goto err_irq; ? ? ? ?platform_set_drvdata(pdev, obj); + ? ? ? pm_runtime_enable(obj->dev); + ? ? ? ?dev_info(&pdev->dev, "%s registered\n", obj->name); ? ? ? ?return 0;@@ -974,8 +967,6 @@ err_irq:?err_ioremap:
? ? ? ?release_mem_region(res->start,resource_size(res)); ?err_mem:
- ? ? ?clk_put(obj->clk);
-err_clk:
? ? ? ?kfree(obj);
? ? ? ?return err;
?}
Review static int __devexit omap_iommu_remove(struct platform_device *pdev) · line 990
Quoted 5 lines
(quoted diff) +1
-1@@ -996,7 +987,8 @@ static int __devexit omap_iommu_remove(struct platform_device *pdev) ? ? ? ?release_mem_region(res->start, resource_size(res)); ? ? ? ?iounmap(obj->regbase); - ? ? ? clk_put(obj->clk); + ? ? ? pm_runtime_disable(obj->dev);
I'm not sure if this is needed; you want to resume the driver? AFAICS kfree will take care of that _without_ resuming.
Review static int __devexit omap_iommu_remove(struct platform_device *pdev) · line 993
Quoted 3 lines
(quoted diff) +0
-0? ? ? ?dev_info(&pdev->dev, "%s removed\n", obj->name); ? ? ? ?kfree(obj); ? ? ? ?return 0;
Cheers.
Signature · 1 line
Felipe Contreras
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Quoted 10 lines · Felipe Contreras
Quoted 9 lines · Omar Ramirez Luna
(quoted diff) +1
-1@@ -123,11 +123,10 @@ static int iommu_enable(struct omap_iommu *obj) ? ? ? ?if (!arch_iommu) ? ? ? ? ? ? ? ?return -ENODEV; - ? ? ? clk_enable(obj->clk); + ? ? ? pm_runtime_get_sync(obj->dev); ? ? ? ?err = arch_iommu->enable(obj);
Quoted 7 lines
Review static int iommu_enable(struct omap_iommu *obj) · line 131
Quoted 3 lines
(quoted diff) +0
-1- ? ? ? clk_disable(obj->clk); ? ? ? ?return err; ?}Hmm, this is called on
omap_iommu_attach, andiommu_disableis not called untilomap_iommu_detach, so this means the device will never sleep. Why don't you callpm_runtime_put()instead ofclk_disable()?
Here once you call pm_runtime_put, the hwmod should turn off iva2_ck leaving device unpowered, not usable if it is part of an independent module and that module is still awake, but since the module clock feeds both the dsp and the mmu this doesn't occur. However might be possible if there is a specific clock to feed the mmu and another for the mmu user.
In theory iommu should be independent on handling its clock resources, take tidspbridge, it powers iva2_clk which indirectly powers iva_mmu, so as long as the dsp is powered and using the mmu (omap_iommu_attach) the iommu should be ON, once you omap_iommu_detach means that the device is no longer using the iommu and can be put to sleep.
This could be helpful if your main processor can go to sleep independently of other processors, say you attach the iommu and leave the dsp doing calculations and memory accesses through mmu while the main processor decides to enter sleep.
Quoted 2 lines
All the rest of the calls to pm_runtime_get/put after this are basically no-ops, because the count will never go below 1.
I didn't try but without calling iommu_attach the other functions that use pm_runtime_get/put can still be called as they are exported symbols. I believe omap-iommu-debug.c does something like this to dump stuff. This should be cleaned up/audited, IMHO, but it doesn't seem to belong to this conversion series.
Quoted 2 lines
You mention some irq issues, but could it be due to some bad clocks in the hwmod data?
There are no "issues" with irqs, just a weird case, see below...
...
Quoted 9 lines
Review static irqreturn_t iommu_fault_handler(int irq, void *data) · line 785
Quoted 7 lines
(quoted diff) +0
-2@@ -780,9 +777,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) ? ? ? ?if (!obj->refcount) ? ? ? ? ? ? ? ?return IRQ_NONE; - ? ? ? clk_enable(obj->clk); ? ? ? ?errs = iommu_report_fault(obj, &da); - ? ? ? clk_disable(obj->clk);Why?
In order to get an irq from the mmu, the mmu should be functional and have a clock, but iommu_enable used to enable and disable the clock.
In the end you are able to get an irq because someone else (tidspbridge) has the mmu indirectly powered (since they share the same clock), I felt this shouldn't be and the iommu should handle its own clocks even if they are shared with other modules.
Hence iommu_enable does pm_runtime_get for the life time of the user of the mmu.
...
Quoted 9 lines
Review static int __devexit omap_iommu_remove(struct platform_device *pdev) · line 990
Quoted 6 lines
(quoted diff) +1
-1@@ -996,7 +987,8 @@ static int __devexit omap_iommu_remove(struct platform_device *pdev) ? ? ? ?release_mem_region(res->start, resource_size(res)); ? ? ? ?iounmap(obj->regbase); - ? ? ? clk_put(obj->clk); + ? ? ? pm_runtime_disable(obj->dev);I'm not sure if this is needed; you want to resume the driver? AFAICS kfree will take care of that _without_ resuming.
No, the driver should resume only if there are outstanding wake up requests, right? I don't seem to get this question.
Thanks,
Omar
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
In reply to Omar Ramirez LunaQuoted 8 lines · Omar Ramirez Luna
Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling.
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance. Now doing pm_runtime_get/put on iommu_enable/disable so it doesn't rely on others to keep the clocks on.
Quoted 8 lines
Signed-off-by Omar Ramirez Luna <[email protected]>---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ? | ? 17 ----------- ?arch/arm/mach-omap2/omap-iommu.c ? ? ? ? | ? ?1 - ?arch/arm/plat-omap/include/plat/iommu.h ?| ? ?2 - ?arch/arm/plat-omap/include/plat/iommu2.h | ? ?2 - ?drivers/iommu/omap-iommu.c ? ? ? ? ? ? ? | ? 44 ++++++++++++-----------------?5 files changed, 18 insertions(+), 48 deletions(-)
Shouldn't pm_runtime_enable() be called in omap_iommu_init(), and omap_iommu_probe() call pm_runtime_get_sync()/put() on the sections where the device should be active?
Cheers.
Signature · 1 line
Felipe Contreras
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Quoted 18 lines · Felipe Contreras
Quoted 8 lines · Omar Ramirez Luna
Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling.
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance. Now doing pm_runtime_get/put on iommu_enable/disable so it doesn't rely on others to keep the clocks on.
Quoted 8 lines
Signed-off-by Omar Ramirez Luna <[email protected]>---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ? | ? 17 ----------- ?arch/arm/mach-omap2/omap-iommu.c ? ? ? ? | ? ?1 - ?arch/arm/plat-omap/include/plat/iommu.h ?| ? ?2 - ?arch/arm/plat-omap/include/plat/iommu2.h | ? ?2 - ?drivers/iommu/omap-iommu.c ? ? ? ? ? ? ? | ? 44 ++++++++++++-----------------?5 files changed, 18 insertions(+), 48 deletions(-)
Quoted 3 lines
Shouldn't
pm_runtime_enable()be called inomap_iommu_init(), andomap_iommu_probe()call pm_runtime_get_sync()/put() on the sections where the device should be active?
omap_iommu_init is called on module init however omap_iommu_probe is called by driver instance (isp, iva), on probe pm_runtime_enable activates runtime for both isp and iva devices, one at a time.
Regards,
Omar
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Quoted 20 lines · Omar
Quoted 18 lines · Felipe Contreras
Quoted 16 lines · Omar Ramirez Luna
Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling.
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance. Now doing pm_runtime_get/put on iommu_enable/disable so it doesn't rely on others to keep the clocks on.
Signed-off-by Omar Ramirez Luna <[email protected]>---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ? | ? 17 ----------- ?arch/arm/mach-omap2/omap-iommu.c ? ? ? ? | ? ?1 - ?arch/arm/plat-omap/include/plat/iommu.h ?| ? ?2 - ?arch/arm/plat-omap/include/plat/iommu2.h | ? ?2 - ?drivers/iommu/omap-iommu.c ? ? ? ? ? ? ? | ? 44 ++++++++++++-----------------?5 files changed, 18 insertions(+), 48 deletions(-)
Quoted 7 lines
Quoted 3 lines
Shouldn't
pm_runtime_enable()be called inomap_iommu_init(), andomap_iommu_probe()call pm_runtime_get_sync()/put() on the sections where the device should be active?
omap_iommu_initis called on module init howeveromap_iommu_probeis called by driver instance (isp, iva), on probepm_runtime_enableactivates runtime for both isp and iva devices, one at a time.
Yes, but omap_iommu_init() will *always* be called at boot time and will register the data for all the devices. If the 'iommu' module is never loaded, then the devices will remain active the whole time.
Maybe Paul or somebody with better knowledge of the pm runtime framework can clarify this.
Cheers.
Signature · 1 line
Felipe Contreras
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Quoted 25 lines · Felipe Contreras
Quoted 24 lines · Omar
Quoted 21 lines · Felipe Contreras
Quoted 8 lines · Omar Ramirez Luna
Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling.
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance. Now doing pm_runtime_get/put on iommu_enable/disable so it doesn't rely on others to keep the clocks on.
Quoted 8 lines
Signed-off-by Omar Ramirez Luna <[email protected]>---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ? | ? 17 ----------- ?arch/arm/mach-omap2/omap-iommu.c ? ? ? ? | ? ?1 - ?arch/arm/plat-omap/include/plat/iommu.h ?| ? ?2 - ?arch/arm/plat-omap/include/plat/iommu2.h | ? ?2 - ?drivers/iommu/omap-iommu.c ? ? ? ? ? ? ? | ? 44 ++++++++++++-----------------?5 files changed, 18 insertions(+), 48 deletions(-)
Shouldn't
pm_runtime_enable()be called inomap_iommu_init(), andomap_iommu_probe()call pm_runtime_get_sync()/put() on the sections where the device should be active?
Quoted 6 lines
Quoted 3 lines
omap_iommu_initis called on module init howeveromap_iommu_probeis called by driver instance (isp, iva), on probepm_runtime_enableactivates runtime for both isp and iva devices, one at a time.Yes, but
omap_iommu_init()will *always* be called at boot time and will register the data for all the devices.
There are 2 omap_iommu_init :/ Thought you were talking about the one in drivers/iommu/omap-iommu.c.
Quoted 2 lines
If the 'iommu' module is
never loaded, then the devices will remain active the whole time.
oma-iommu is meant to be built-in as part of the kernel, there is no option for module anymore.
Regards,
Omar
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Quoted 34 lines · Omar
Quoted 25 lines · Felipe Contreras
Quoted 24 lines · Omar
Quoted 21 lines · Felipe Contreras
Quoted 8 lines · Omar Ramirez Luna
Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling.
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance. Now doing pm_runtime_get/put on iommu_enable/disable so it doesn't rely on others to keep the clocks on.
Quoted 8 lines
Signed-off-by Omar Ramirez Luna <[email protected]>---
?arch/arm/mach-omap2/iommu2.c ? ? ? ? ? ? | ? 17 ----------- ?arch/arm/mach-omap2/omap-iommu.c ? ? ? ? | ? ?1 - ?arch/arm/plat-omap/include/plat/iommu.h ?| ? ?2 - ?arch/arm/plat-omap/include/plat/iommu2.h | ? ?2 - ?drivers/iommu/omap-iommu.c ? ? ? ? ? ? ? | ? 44 ++++++++++++-----------------?5 files changed, 18 insertions(+), 48 deletions(-)
Shouldn't
pm_runtime_enable()be called inomap_iommu_init(), andomap_iommu_probe()call pm_runtime_get_sync()/put() on the sections where the device should be active?Quoted 6 lines
Quoted 3 lines
omap_iommu_initis called on module init howeveromap_iommu_probeis called by driver instance (isp, iva), on probepm_runtime_enableactivates runtime for both isp and iva devices, one at a time.Yes, but
omap_iommu_init()will *always* be called at boot time and will register the data for all the devices.
Quoted 8 lines
There are 2
omap_iommu_init:/ Thought you were talking about the one indrivers/iommu/omap-iommu.c.Quoted 2 lines
If the 'iommu' module is
never loaded, then the devices will remain active the whole time.oma-iommu is meant to be built-in as part of the kernel, there is no option for module anymore.
Which omap-iommu? The platform driver, or the device stuff? The device stuff is always built-in, but not the platform driver (drivers/iommu/omap-iommu.c), that can be a module.
Signature · 1 line
Felipe Contreras
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Quoted 3 lines · Felipe Contreras
Which omap-iommu? The platform driver, or the device stuff? The device stuff is always built-in, but not the platform driver (
drivers/iommu/omap-iommu.c), that can be a module.
Both, I can't recall exactly when it changed (prior to being moved to drivers/iommu it could be built as a module), but now CONFIG_OMAP_IOMMU is boolean type.
Regards,
Omar
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Quoted 9 lines · Omar
Quoted 3 lines · Felipe Contreras
Which omap-iommu? The platform driver, or the device stuff? The device stuff is always built-in, but not the platform driver (
drivers/iommu/omap-iommu.c), that can be a module.Both, I can't recall exactly when it changed (prior to being moved to
drivers/iommuit could be built as a module), but nowCONFIG_OMAP_IOMMUis boolean type.
I see. Still, it looks like the proper way to use the API is to call _enable() on the platform driver.
Signature · 1 line
Felipe Contreras
PATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
Quoted 7 lines · Felipe Contreras
Quoted 6 lines · Omar
Quoted 3 lines · Felipe Contreras
Which omap-iommu? The platform driver, or the device stuff? The device stuff is always built-in, but not the platform driver (
drivers/iommu/omap-iommu.c), that can be a module.
Quoted 6 lines
Quoted 3 lines
Both, I can't recall exactly when it changed (prior to being moved to
drivers/iommuit could be built as a module), but nowCONFIG_OMAP_IOMMUis boolean type.I see. Still, it looks like the proper way to use the API is to call
_enable()on the platform driver.
I'm sorry, I don't seem to follow... you want _enable in platform driver, and you call drivers/iommu/omap-iommu.c the platform driver...
And that is precisely where pm_runtime_enable() is, no?
Regards,
Omar
Tony Lindgren
maintainerPATCH v4 4/4 OMAP3/4: iommu: adapt to runtime pm
In reply to Omar Ramirez LunaQuoted 9 lines · Omar Ramirez Luna
Use runtime PM functionality interfaced with hwmod enable/idle functions, to replace direct clock operations, reset and sysconfig handling.
Removed clk handling during interrupt, given that in order to receive one, the device should be powered on in advance. Now doing pm_runtime_get/put on iommu_enable/disable so it doesn't rely on others to keep the clocks on.
Signed-off-by Omar Ramirez Luna <[email protected]>
The omap parts look OK to me for Ohad to queue: