From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:05
These patches are needed for remoteproc to work on OMAP4.
Introduced iommu hwmod support for OMAP3 (iva, isp) and
OMAP4 (ipu, dsp), along with the corresponding runtime PM
and routines to deassert reset lines, enable/disable clocks
and configure sysc registers.
Although IOMMU hwmod patches were already submitted in the past,
this series adds few more changes, like:
- New reset handling.
- Save and restore context code rework.
- Device tree bindings for OMAP3 and OMAP4.
For this series I just dropped the patches already included in
mainline.
Previous work can be found at:
[v2]
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg75701.html
[v1]
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg70447.html
[old iteration without reset, save/restore and device tree]
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg60133.html
Omar Ramirez Luna (6):
ARM: OMAP3/4: iommu: migrate to hwmod framework
ARM: OMAP3/4: iommu: adapt to runtime pm
ARM: OMAP: iommu: pm runtime save and restore context
ARM: OMAP: iommu: optimize save and restore routines
ARM: OMAP: iommu: add device tree support
arm/dts: OMAP3/4: Add iommu nodes
.../devicetree/bindings/arm/omap/iommu.txt | 10 ++
arch/arm/boot/dts/omap3.dtsi | 12 +-
arch/arm/boot/dts/omap4.dtsi | 17 +-
arch/arm/mach-omap2/devices.c | 2 +-
arch/arm/mach-omap2/iommu2.c | 74 ++------
arch/arm/mach-omap2/omap-iommu.c | 176 +++++---------------
arch/arm/plat-omap/include/plat/iommu.h | 20 ++-
arch/arm/plat-omap/include/plat/iommu2.h | 4 -
drivers/iommu/omap-iommu.c | 163 ++++++++++++++----
9 files changed, 245 insertions(+), 233 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/omap/iommu.txt
--
1.7.9.5
From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:06
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 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.
- Use hwmod API for reset handling.
Signed-off-by: Omar Ramirez Luna <redacted>
---
arch/arm/mach-omap2/devices.c | 2 +-
arch/arm/mach-omap2/iommu2.c | 19 ----
arch/arm/mach-omap2/omap-iommu.c | 165 +++++++------------------------
arch/arm/plat-omap/include/plat/iommu.h | 8 +-
drivers/iommu/omap-iommu.c | 23 ++++-
5 files changed, 64 insertions(+), 153 deletions(-)
@@ -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);
From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:07
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 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.
- Use hwmod API for reset handling.
Signed-off-by: Omar Ramirez Luna <redacted>
---
arch/arm/mach-omap2/devices.c | 2 +-
arch/arm/mach-omap2/iommu2.c | 19 ----
arch/arm/mach-omap2/omap-iommu.c | 165 +++++++------------------------
arch/arm/plat-omap/include/plat/iommu.h | 8 +-
drivers/iommu/omap-iommu.c | 23 ++++-
5 files changed, 64 insertions(+), 153 deletions(-)
@@ -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);
From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:08
Use runtime PM functionality interfaced with hwmod enable/idle
functions, to replace direct clock operations and sysconfig
handling.
Dues to reset sequence, pm_runtime_put_sync must be used, to avoid
possible operations with the module under reset.
Signed-off-by: Omar Ramirez Luna <redacted>
---
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 | 45 +++++++++++++-----------------
5 files changed, 19 insertions(+), 48 deletions(-)
From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:09
Use runtime PM functionality interfaced with hwmod enable/idle
functions, to replace direct clock operations and sysconfig
handling.
Dues to reset sequence, pm_runtime_put_sync must be used, to avoid
possible operations with the module under reset.
Signed-off-by: Omar Ramirez Luna <redacted>
---
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 | 45 +++++++++++++-----------------
5 files changed, 19 insertions(+), 48 deletions(-)
From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:10
Save and restore context during pm runtime transitions.
For now, the previous API for this purpose will trigger
pm runtime functions, and will be left as exported symbol
for compatibility with it's only user.
Signed-off-by: Omar Ramirez Luna <redacted>
---
drivers/iommu/omap-iommu.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:11
Save and restore context during pm runtime transitions.
For now, the previous API for this purpose will trigger
pm runtime functions, and will be left as exported symbol
for compatibility with it's only user.
Signed-off-by: Omar Ramirez Luna <redacted>
---
drivers/iommu/omap-iommu.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:12
These functions save and restore registers irrespectively of their
read or write permissions, this ends up in registers being saved
that can't be restored because of read only attributes. OTOH, so
far only 3 of them need to be saved.
In future GP_REG (which is present only on OMAP4 ipu) needs to be
saved but right now there is no API that can alter its value. Also,
protected TLB entries must be saved but this can be in a separate
patch as the original code didn't implement the loop to traverse
protected TLB entries.
Signed-off-by: Omar Ramirez Luna <redacted>
---
arch/arm/mach-omap2/iommu2.c | 38 ++++++++++++++----------------
arch/arm/plat-omap/include/plat/iommu.h | 10 +++++++-
arch/arm/plat-omap/include/plat/iommu2.h | 2 --
drivers/iommu/omap-iommu.c | 3 +--
4 files changed, 28 insertions(+), 25 deletions(-)
From: Omar Ramirez Luna <hidden> Date: 2012-10-12 01:06:13
These functions save and restore registers irrespectively of their
read or write permissions, this ends up in registers being saved
that can't be restored because of read only attributes. OTOH, so
far only 3 of them need to be saved.
In future GP_REG (which is present only on OMAP4 ipu) needs to be
saved but right now there is no API that can alter its value. Also,
protected TLB entries must be saved but this can be in a separate
patch as the original code didn't implement the loop to traverse
protected TLB entries.
Signed-off-by: Omar Ramirez Luna <redacted>
---
arch/arm/mach-omap2/iommu2.c | 38 ++++++++++++++----------------
arch/arm/plat-omap/include/plat/iommu.h | 10 +++++++-
arch/arm/plat-omap/include/plat/iommu2.h | 2 --
drivers/iommu/omap-iommu.c | 3 +--
4 files changed, 28 insertions(+), 25 deletions(-)
@@ -0,0 +1,10 @@+* MMU (Memory Management Unit)++MMU present in OMAP subsystems.++Required properties:+ compatible : should be "ti,omap3-iommu" for OMAP3 MMUs+ compatible : should be "ti,omap4-iommu" for OMAP4 MMUs++Optional properties:+ None
From: Felipe Contreras <hidden> Date: 2012-10-12 07:48:33
On Fri, Oct 12, 2012 at 3:06 AM, Omar Ramirez Luna [off-list ref] wrote:
Use runtime PM functionality interfaced with hwmod enable/idle
functions, to replace direct clock operations and sysconfig
handling.
Dues to reset sequence, pm_runtime_put_sync must be used, to avoid
possible operations with the module under reset.
I already made most of these comments, but here they go again.
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.
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.
quoted
@@ -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) {
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.
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:
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?
quoted
quoted
@@ -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) {
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.
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.
quoted
Also, I still think that something like this is needed:
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.
quoted
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.
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.
quoted
quoted
Also, I still think that something like this is needed:
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
From: Tony Lindgren <tony@atomide.com> Date: 2012-10-16 17:22:59
* Omar Ramirez Luna [off-list ref] [121011 18:07]:
These patches are needed for remoteproc to work on OMAP4.
Introduced iommu hwmod support for OMAP3 (iva, isp) and
OMAP4 (ipu, dsp), along with the corresponding runtime PM
and routines to deassert reset lines, enable/disable clocks
and configure sysc registers.
Although IOMMU hwmod patches were already submitted in the past,
this series adds few more changes, like:
- New reset handling.
- Save and restore context code rework.
- Device tree bindings for OMAP3 and OMAP4.
For this series I just dropped the patches already included in
mainline.
These will need to be rebased on omap-for-v3.8/cleanup-headers-iommu
when I have that pushed out as that removes plat/*iommu*.h files.
Regards,
Tony
Previous work can be found at:
[v2]
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg75701.html
[v1]
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg70447.html
[old iteration without reset, save/restore and device tree]
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg60133.html
Omar Ramirez Luna (6):
ARM: OMAP3/4: iommu: migrate to hwmod framework
ARM: OMAP3/4: iommu: adapt to runtime pm
ARM: OMAP: iommu: pm runtime save and restore context
ARM: OMAP: iommu: optimize save and restore routines
ARM: OMAP: iommu: add device tree support
arm/dts: OMAP3/4: Add iommu nodes
.../devicetree/bindings/arm/omap/iommu.txt | 10 ++
arch/arm/boot/dts/omap3.dtsi | 12 +-
arch/arm/boot/dts/omap4.dtsi | 17 +-
arch/arm/mach-omap2/devices.c | 2 +-
arch/arm/mach-omap2/iommu2.c | 74 ++------
arch/arm/mach-omap2/omap-iommu.c | 176 +++++---------------
arch/arm/plat-omap/include/plat/iommu.h | 20 ++-
arch/arm/plat-omap/include/plat/iommu2.h | 4 -
drivers/iommu/omap-iommu.c | 163 ++++++++++++++----
9 files changed, 245 insertions(+), 233 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/omap/iommu.txt
--
1.7.9.5
From: Omar Ramirez Luna <hidden> Date: 2012-10-17 23:52:37
On 16 October 2012 12:22, Tony Lindgren [off-list ref] wrote:
* Omar Ramirez Luna [off-list ref] [121011 18:07]:
quoted
These patches are needed for remoteproc to work on OMAP4.
Introduced iommu hwmod support for OMAP3 (iva, isp) and
OMAP4 (ipu, dsp), along with the corresponding runtime PM
and routines to deassert reset lines, enable/disable clocks
and configure sysc registers.
Although IOMMU hwmod patches were already submitted in the past,
this series adds few more changes, like:
- New reset handling.
- Save and restore context code rework.
- Device tree bindings for OMAP3 and OMAP4.
For this series I just dropped the patches already included in
mainline.
These will need to be rebased on omap-for-v3.8/cleanup-headers-iommu
when I have that pushed out as that removes plat/*iommu*.h files.
Ok, will wait and rebase on top of it.
Thanks,
Omar
From: Tony Lindgren <tony@atomide.com> Date: 2012-10-18 23:52:46
Omar,
* Omar Ramirez Luna [off-list ref] [121017 16:54]:
On 16 October 2012 12:22, Tony Lindgren [off-list ref] wrote:
quoted
These will need to be rebased on omap-for-v3.8/cleanup-headers-iommu
when I have that pushed out as that removes plat/*iommu*.h files.
Ok, will wait and rebase on top of it.
Thanks, the related patches are now posted in thread
"[PATCH v3 0/6] omap iommu changes to remove plat includes".
Also, can you please take a look at the "Updated status of the removal
of plat headers" thread?
I've tagged you to remove the omap plat/mailbox.h :)
Regards,
Tony