Message ID | CAMP44s2H7v6dRPXO6eBBNrYAnym_ZLdmKcAQPmYyCC6Wo5qOmw@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Mon, Dec 19, 2011 at 10:11 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote: > On Fri, Dec 16, 2011 at 4:01 AM, Ramirez Luna, Omar <omar.ramirez@ti.com> wrote: >> On Thu, Dec 15, 2011 at 6:39 PM, Felipe Contreras >> <felipe.contreras@gmail.com> wrote: >>> Are you sure you are not missing something like: >>> >>> .clk = "cam_ick", >> >> I believe in this case cam_ick is 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. >>>> +/* 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. > > Then maybe the code is wrong. > > Look at the OMAP34xx documentation, it says that: > > CAM_L3_ICLK -> CAM_FCLK > CAM_L4_ICLK -> CAM_ICLK > CAM_MCLK -> CAM_MCLK > CSI2_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. "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. ... > > 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. Would be good to clarify the intentions to keep the code as it is. >>>> + .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. > > 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. > 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
Hi Omar, On Friday 23 December 2011 16:53:58 Ramirez Luna, Omar wrote: > On Mon, Dec 19, 2011 at 10:11 AM, Felipe Contreras wrote: > > On Fri, Dec 16, 2011 at 4:01 AM, Ramirez Luna, Omar wrote: > >> On Thu, Dec 15, 2011 at 6:39 PM, Felipe Contreras wrote: > >>> Are you sure you are not missing something like: > >>> > >>> .clk = "cam_ick", > >> > >> I believe in this case cam_ick is 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 ? :-) > >>>> +/* 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. > > > > Then maybe the code is wrong. > > > > Look at the OMAP34xx documentation, it says that: > > > > CAM_L3_ICLK -> CAM_FCLK > > CAM_L4_ICLK -> CAM_ICLK > > CAM_MCLK -> CAM_MCLK > > CSI2_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. > > "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. 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. > ... > > > 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. > > 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. > >>>> + .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. > > > > 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. > > > 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. How should the OMAP3 ISP driver proceed to make sure that its IOMMU is clocked off when it doesn't need it ?
Hi Laurent On Sun, Dec 25, 2011 at 3:08 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> > 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. > > 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
Hi Omar, On Thursday 05 January 2012 20:24:25 Ramirez Luna, Omar wrote: > On Sun, Dec 25, 2011 at 3:08 PM, Laurent Pinchart wrote: > >> > 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. > > > > 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. 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.
--- a/arch/arm/mach-omap2/clock3xxx_data.c +++ b/arch/arm/mach-omap2/clock3xxx_data.c @@ -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),