| Message ID | CAMP44s2LTGPnXQuetVLV1tA8w90gUtoJ5coucBUZsbqb3vFnng@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12 October 2012 02:48, Felipe Contreras <felipe.contreras@gmail.com> wrote: > I already made most of these comments, but here they go again. I replied to all, but here it goes again: >> @@ -142,11 +142,10 @@ static int iommu_enable(struct omap_iommu *obj) >> } >> } >> >> - clk_enable(obj->clk); >> + pm_runtime_get_sync(obj->dev); >> >> err = arch_iommu->enable(obj); >> >> - clk_disable(obj->clk); > > The device will never go to sleep, until iommu_disable is called. > clk_enable -> pm_runtime_get_sync, clk_disable pm_runtime_put. Which is what you want... why would you want your iommu to be disabled if the client of that iommu could request a translation? Remember that these iommus, sit along of other processors not on the main processor side. So, this code should enable it for the other processor to use, and there is no point where the processor can say "I'm not using it, shut it down" or "I'm using it, turn it on" in the middle of execution, other than suspend/resume and if supported, autosuspend. >> @@ -288,7 +285,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) { >> @@ -318,7 +315,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_sync(obj->dev); >> return PTR_ERR(cr); >> } > > If I'm correct, the above pm_runtime_get/put are redundant, because > the count can't possibly reach 0 because of the reason I explained > before. > > The same for all the cases below. You can access this paths through debugfs, some of them doesn't work if the module is not enabled first, but in future if you just want to idle the iommu withouth freeing, these are needed to debug. BTW, the next patch in the series: ARM: OMAP: iommu: pm runtime save and restore context, let's you do a pm_runtime_[enable|put] through save/restore ctx functions, which is just for compatibility on how isp code uses the save and restore code. >> @@ -1009,7 +1001,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); > > This will turn on the device unnecessarily, wasting power, and there's > no need for that, kfree will take care of that without resuming. Left aside the aesthetics of having balanced calls, the device will be enabled if there was a pending resume to be executed, otherwise it won't, kfree won't increment the disable_depth counter and I don't think that freeing the pointer is enough reason to ignore pm_runtime_disable. > Also, I still think that something like this is needed: ... > +static struct clk cam_fck = { > + .name = "cam_fck", > + .ops = &clkops_omap2_iclk_dflt, > + .parent = &l3_ick, > + .enable_reg = OMAP_CM_REGADDR(OMAP3430_CAM_MOD, CM_ICLKEN), a cam_fck name to enable the ick? Cheers, Omar
On Fri, Oct 12, 2012 at 7:46 PM, Omar Ramirez Luna <omar.luna@linaro.org> wrote: > On 12 October 2012 02:48, Felipe Contreras <felipe.contreras@gmail.com> wrote: >> I already made most of these comments, but here they go again. > > I replied to all, but here it goes again: Mostly, but not all :) >>> @@ -142,11 +142,10 @@ static int iommu_enable(struct omap_iommu *obj) >>> } >>> } >>> >>> - clk_enable(obj->clk); >>> + pm_runtime_get_sync(obj->dev); >>> >>> err = arch_iommu->enable(obj); >>> >>> - clk_disable(obj->clk); >> >> The device will never go to sleep, until iommu_disable is called. >> clk_enable -> pm_runtime_get_sync, clk_disable pm_runtime_put. > > Which is what you want... why would you want your iommu to be disabled > if the client of that iommu could request a translation? That's the whole point of *dynamic* pm; _when_ the client wants to request a translation, _then_ the device is waken up, which is what I believe the code currently does. After your patch, even if I don't use the camera, or the DSP, the iommu devices will be enabled, and will be consuming energy *all the time*. Which I don't think is what we want. I'm not saying I have a solution, I'm simply saying that's what's going to happen if I'm correct. > Remember that these iommus, sit along of other processors not on the > main processor side. So, this code should enable it for the other > processor to use, and there is no point where the processor can say > "I'm not using it, shut it down" or "I'm using it, turn it on" in the > middle of execution, other than suspend/resume and if supported, > autosuspend. I understand, but perhaps there should be? >>> @@ -288,7 +285,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) { >>> @@ -318,7 +315,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_sync(obj->dev); >>> return PTR_ERR(cr); >>> } >> >> If I'm correct, the above pm_runtime_get/put are redundant, because >> the count can't possibly reach 0 because of the reason I explained >> before. >> >> The same for all the cases below. > > You can access this paths through debugfs, some of them doesn't work > if the module is not enabled first, but in future if you just want to > idle the iommu withouth freeing, these are needed to debug. > > BTW, the next patch in the series: ARM: OMAP: iommu: pm runtime save > and restore context, let's you do a pm_runtime_[enable|put] through > save/restore ctx functions, which is just for compatibility on how isp > code uses the save and restore code. All right, it has been some time since I've touched pm code, so if you say so. >>> @@ -1009,7 +1001,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); >> >> This will turn on the device unnecessarily, wasting power, and there's >> no need for that, kfree will take care of that without resuming. > > Left aside the aesthetics of having balanced calls, the device will be > enabled if there was a pending resume to be executed, otherwise it > won't, kfree won't increment the disable_depth counter and I don't > think that freeing the pointer is enough reason to ignore > pm_runtime_disable. You are doing __pm_runtime_disable(dev, true), kfree will do __pm_runtime_disable(dev, false), which is what we want. Both will decrement the disable_depth. But at least you agree that there's a chance that the device will be waken up. >> Also, I still think that something like this is needed: > ... >> +static struct clk cam_fck = { >> + .name = "cam_fck", >> + .ops = &clkops_omap2_iclk_dflt, >> + .parent = &l3_ick, >> + .enable_reg = OMAP_CM_REGADDR(OMAP3430_CAM_MOD, CM_ICLKEN), > > a cam_fck name to enable the ick? Yeap, according to the TRM. Take a look at 12.3 Camera ISP Integration Fig 12-50. Cheers.
Hi Felipe, On 12 October 2012 16:25, Felipe Contreras <felipe.contreras@gmail.com> wrote: >>>> @@ -142,11 +142,10 @@ static int iommu_enable(struct omap_iommu *obj) >>>> } >>>> } >>>> >>>> - clk_enable(obj->clk); >>>> + pm_runtime_get_sync(obj->dev); >>>> >>>> err = arch_iommu->enable(obj); >>>> >>>> - clk_disable(obj->clk); >>> >>> The device will never go to sleep, until iommu_disable is called. >>> clk_enable -> pm_runtime_get_sync, clk_disable pm_runtime_put. >> >> Which is what you want... why would you want your iommu to be disabled >> if the client of that iommu could request a translation? > > That's the whole point of *dynamic* pm; _when_ the client wants to > request a translation, _then_ the device is waken up, which is what I > believe the code currently does. No it doesn't, current code is working because the processor and the iommu share the same clock, so enabling the processor is implicitly guaranteeing that the iommu will be enabled. IMHO, there shouldn't be such assumption that you can control both with the same clock. So, once the remote processor is enabled, any "dynamic pm" from iommu with current code has no effect because the clock was already enabled for the processor. > After your patch, even if I don't use the camera, or the DSP, the > iommu devices will be enabled, and will be consuming energy *all the > time*. Which I don't think is what we want. Wrong, the iommu device will be enabled by pm_runtime_get_sync once you decide to attach with iommu_attach_device, if you do not use camera or the dsp, you won't turn ON the iommus. On probe this patch does pm_runtime_enable, however this doesn't mean the device is turned ON or resumed or kept ON all the time. > I'm not saying I have a solution, I'm simply saying that's what's > going to happen if I'm correct. Ok, but that is not what happens here. >> Remember that these iommus, sit along of other processors not on the >> main processor side. So, this code should enable it for the other >> processor to use, and there is no point where the processor can say >> "I'm not using it, shut it down" or "I'm using it, turn it on" in the >> middle of execution, other than suspend/resume and if supported, >> autosuspend. > > I understand, but perhaps there should be? Autosuspend is a feature missing and should handle the scenario where the remote processor can sleep dynamically, this scenario should turn off the iommu and the remote processor itself when there is no workload but it depends on the remote processor activity not the iommu activity. >>>> @@ -1009,7 +1001,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); >>> >>> This will turn on the device unnecessarily, wasting power, and there's >>> no need for that, kfree will take care of that without resuming. >> >> Left aside the aesthetics of having balanced calls, the device will be >> enabled if there was a pending resume to be executed, otherwise it >> won't, kfree won't increment the disable_depth counter and I don't >> think that freeing the pointer is enough reason to ignore >> pm_runtime_disable. > > You are doing __pm_runtime_disable(dev, true), kfree will do > __pm_runtime_disable(dev, false), which is what we want. Both will > decrement the disable_depth. I'm quite confused here, could you please point me to the kfree snip that does __pm_runtime_disable(dev, false)? > But at least you agree that there's a chance that the device will be waken up. Of course, if there is a pending resume to be executed, it must honor that resume request and then turn off the device before removing the iommu, IMHO. >>> Also, I still think that something like this is needed: >> ... >>> +static struct clk cam_fck = { >>> + .name = "cam_fck", >>> + .ops = &clkops_omap2_iclk_dflt, >>> + .parent = &l3_ick, >>> + .enable_reg = OMAP_CM_REGADDR(OMAP3430_CAM_MOD, CM_ICLKEN), >> >> a cam_fck name to enable the ick? > > Yeap, according to the TRM. Take a look at 12.3 Camera ISP Integration > Fig 12-50. What I meant is that, you are using the CM_ICLKEN to enable a clock named "cam_fck" which has l3_ick as a parent. And that is not consistent with what that register is meant to do, which is: 4.14.1.10 CAM_CM Registers CM_ICKLEN_CAM 0x0: CAM_L3_ICK and CAM_L4_ICLK are disabled 0x1: CAM_L3_ICK and CAM_L4_ICLK are enabled So, I'm complaining about the name "cam_fck", for an interface clock with parent l3_ick. However I don't know why on section 12.3 they refer to CAM_FCK to a l3_ick child clock. Cheers, Omar
Hi, On Tue, Oct 16, 2012 at 3:29 AM, Omar Ramirez Luna <omar.luna@linaro.org> wrote: >> After your patch, even if I don't use the camera, or the DSP, the >> iommu devices will be enabled, and will be consuming energy *all the >> time*. Which I don't think is what we want. > > Wrong, the iommu device will be enabled by pm_runtime_get_sync once > you decide to attach with iommu_attach_device, if you do not use > camera or the dsp, you won't turn ON the iommus. I see, somehow I conflated the two functions. > On probe this patch does pm_runtime_enable, however this doesn't mean > the device is turned ON or resumed or kept ON all the time. In fact it's the other way around; pm_runtime_enable turns off the power (if it's ON). >>>>> @@ -1009,7 +1001,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); >>>> >>>> This will turn on the device unnecessarily, wasting power, and there's >>>> no need for that, kfree will take care of that without resuming. >>> >>> Left aside the aesthetics of having balanced calls, the device will be >>> enabled if there was a pending resume to be executed, otherwise it >>> won't, kfree won't increment the disable_depth counter and I don't >>> think that freeing the pointer is enough reason to ignore >>> pm_runtime_disable. >> >> You are doing __pm_runtime_disable(dev, true), kfree will do >> __pm_runtime_disable(dev, false), which is what we want. Both will >> decrement the disable_depth. > > I'm quite confused here, could you please point me to the kfree snip > that does __pm_runtime_disable(dev, false)? Sorry, not kfree, but the device removal process: device_del device_pm_remove pm_runtime_remove __pm_runtime_disable <- HERE bus_remove_device device_release_driver __device_release_driver .remove => platform_drv_remove .remove => omap_iommu_remove Actually, it seems the pm is disabled _before_ omap_iommu_remove is even called, so it's a no-op. >> But at least you agree that there's a chance that the device will be waken up. > > Of course, if there is a pending resume to be executed, it must honor > that resume request and then turn off the device before removing the > iommu, IMHO. Who will turn off the device afterwards? >>>> Also, I still think that something like this is needed: >>> ... >>>> +static struct clk cam_fck = { >>>> + .name = "cam_fck", >>>> + .ops = &clkops_omap2_iclk_dflt, >>>> + .parent = &l3_ick, >>>> + .enable_reg = OMAP_CM_REGADDR(OMAP3430_CAM_MOD, CM_ICLKEN), >>> >>> a cam_fck name to enable the ick? >> >> Yeap, according to the TRM. Take a look at 12.3 Camera ISP Integration >> Fig 12-50. > > What I meant is that, you are using the CM_ICLKEN to enable a clock > named "cam_fck" which has l3_ick as a parent. And that is not > consistent with what that register is meant to do, which is: > > 4.14.1.10 CAM_CM Registers > > CM_ICKLEN_CAM > 0x0: CAM_L3_ICK and CAM_L4_ICLK are disabled > 0x1: CAM_L3_ICK and CAM_L4_ICLK are enabled > > So, I'm complaining about the name "cam_fck", for an interface clock > with parent l3_ick. However I don't know why on section 12.3 they > refer to CAM_FCK to a l3_ick child clock. Because it's also used as a functional clock. Anyway, I don't care much about the name of the clock, what is clear is that there's a link missing to the l3_ick. Cheers.
On 15 October 2012 22:23, Felipe Contreras <felipe.contreras@gmail.com> wrote: >> On probe this patch does pm_runtime_enable, however this doesn't mean >> the device is turned ON or resumed or kept ON all the time. > > In fact it's the other way around; pm_runtime_enable turns off the > power (if it's ON). pm_runtime_enable just decrements the disable_depth counter. Doesn't turn off anything by itself. >>>>>> @@ -1009,7 +1001,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); >>>>> >>>>> This will turn on the device unnecessarily, wasting power, and there's >>>>> no need for that, kfree will take care of that without resuming. >>>> >>>> Left aside the aesthetics of having balanced calls, the device will be >>>> enabled if there was a pending resume to be executed, otherwise it >>>> won't, kfree won't increment the disable_depth counter and I don't >>>> think that freeing the pointer is enough reason to ignore >>>> pm_runtime_disable. >>> >>> You are doing __pm_runtime_disable(dev, true), kfree will do >>> __pm_runtime_disable(dev, false), which is what we want. Both will >>> decrement the disable_depth. >> >> I'm quite confused here, could you please point me to the kfree snip >> that does __pm_runtime_disable(dev, false)? > > Sorry, not kfree, but the device removal process: > > device_del > device_pm_remove > pm_runtime_remove > __pm_runtime_disable <- HERE I'm not entirely convinced _iommu_ follows this path, it doesn't create any devices nor register them... whereas mailbox does create devices and mailbox does follow this path for the devices created which are child devices. So this path won't take care of the omap-iommu device pm_runtime_disable. >>> But at least you agree that there's a chance that the device will be waken up. >> >> Of course, if there is a pending resume to be executed, it must honor >> that resume request and then turn off the device before removing the >> iommu, IMHO. > > Who will turn off the device afterwards? What I meant is that omap_iommu_detach should turn off the device before removing the iommu. Cheers, Omar
On Thu, Oct 18, 2012 at 1:51 AM, Omar Ramirez Luna <omar.luna@linaro.org> wrote: > On 15 October 2012 22:23, Felipe Contreras <felipe.contreras@gmail.com> wrote: >>> On probe this patch does pm_runtime_enable, however this doesn't mean >>> the device is turned ON or resumed or kept ON all the time. >> >> In fact it's the other way around; pm_runtime_enable turns off the >> power (if it's ON). > > pm_runtime_enable just decrements the disable_depth counter. Doesn't > turn off anything by itself. pm_runtime_enable turns on power management, without it the device remains in whatever state it booted. >> device_del >> device_pm_remove >> pm_runtime_remove >> __pm_runtime_disable <- HERE > > I'm not entirely convinced _iommu_ follows this path, it doesn't > create any devices nor register them... whereas mailbox does create > devices and mailbox does follow this path for the devices created > which are child devices. > > So this path won't take care of the omap-iommu device pm_runtime_disable. Are you sure? What code-path calls omap_iommu_remove then? Anyway, it was just an observation. I've seen other code do this.
--- a/arch/arm/mach-omap2/clock3xxx_data.c +++ b/arch/arm/mach-omap2/clock3xxx_data.c @@ -2222,8 +2222,17 @@ static struct clk cam_mclk = { .recalc = &followparent_recalc, }; +static struct clk cam_fck = { + .name = "cam_fck", + .ops = &clkops_omap2_iclk_dflt, + .parent = &l3_ick, + .enable_reg = OMAP_CM_REGADDR(OMAP3430_CAM_MOD, CM_ICLKEN), + .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, @@ -3394,6 +3403,7 @@ static struct omap_clk omap3xxx_clks[] = { CLK("omapdss_dss", "ick", &dss_ick_3430es2, CK_3430ES2PLUS | CK_AM35XX | CK_36XX), CLK(NULL, "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),