[PATCH v4 1/2] iommu/shmobile: Add iommu driver for Renesas IPMMU modules

Hi Eiraku-san,

On Tuesday 11 December 2012 19:10:42 Hideki EIRAKU wrote:

On Mon, 10 Dec 2012 16:55:58 +0100 Laurent Pinchart wrote:

quoted

On Monday 15 October 2012 17:34:52 Hideki EIRAKU wrote:

quoted

This is the Renesas IPMMU driver and IOMMU API implementation.

The IPMMU module supports the MMU function and the PMB function.

That sentence make me believe that both MMU and PMB were supported by the
driver, as "module" often refers to Linux kernel modules in this context.
Maybe you could replace "module" by "hardware module".

OK,

quoted

quoted

The MMU function provides address translation by pagetable compatible
with ARMv6. The PMB function provides address translation including
tile-linear translation. This patch implements the MMU function.

The iommu driver does not register a platform driver directly because:
- the register space of the MMU function and the PMB function
  have a common register (used for settings flush), so they should
  ideally have a way to appropriately share this register.
- the MMU function uses the IOMMU API while the PMB function does not.
- the two functions may be used independently.

Signed-off-by: Hideki EIRAKU <redacted>
---

 arch/arm/mach-shmobile/Kconfig              |    6 +
 arch/arm/mach-shmobile/Makefile             |    3 +
 arch/arm/mach-shmobile/include/mach/ipmmu.h |   16 ++
 arch/arm/mach-shmobile/ipmmu.c              |  150 ++++++++++++
 drivers/iommu/Kconfig                       |   56 +++++
 drivers/iommu/Makefile                      |    1 +
 drivers/iommu/shmobile-iommu.c              |  352 +++++++++++++++++++++
 7 files changed, 584 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-shmobile/include/mach/ipmmu.h
 create mode 100644 arch/arm/mach-shmobile/ipmmu.c
 create mode 100644 drivers/iommu/shmobile-iommu.c

What is the reason for splitting the driver in two files ? Can't you put
all the code in drivers/iommu/shmobile-iommu.c ? Storing driver code in
arch/* is discouraged.

The reason is that I described in the above text. The PMB function is
completely different from the MMU function but both functions are on
the same IPMMU hardware module and sharing the register space. I think
that a driver using the PMB part which is not yet released should not
depend on the Linux's iommu interface, so I split the driver in two
files: the IPMMU platform driver part (in arch/arm/mach-shmobile/) and
Linux's iommu part (in drivers/iommu/). For the IPMMU platform driver part,
do you have any suggestions other than arch/* where this should go? It is a
generic platform device.

I think both parts should go to drivers/iommu/. You can keep the code split 
across two files, but I think you should register a single platform driver. 
The IPMMU is a single hardware module, so it should be handled by a single 
driver. That driver can expose two different APIs (IOMMU and whatever API will 
be used for PMB), and you can make those APIs selectable as Kconfig options, 
but they should in my opinion be implemented in a single driver.

quoted

quoted

+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301
USA

You can remove this last paragraph, we don't want to patch every file in
the kernel if the FSF moves to a new building :-)

OK, I will remove the paragraph.

quoted

quoted

+	for (dev = ipmmu_devices; dev; dev = dev->archdata.iommu) {
+		if (arm_iommu_attach_device(dev, iommu_mapping))
+			pr_err("arm_iommu_attach_device failed\n");
+	}
+err:
+	spin_unlock(&lock_add);
+	return 0;
+}
+
+void ipmmu_add_device(struct device *dev)
+{
+	spin_lock(&lock_add);
+	dev->archdata.iommu = ipmmu_devices;
+	ipmmu_devices = dev;

That looks a bit hackish to me. I'd like to suggest a different approach,
that would be compatible with supporting multiple IPMMU instances.

dev->archdata.iommu should point to a new sh_ipmmu_arch_data structure
that would contain an IPMMU name (const char *) and a pointer to a struct
shmobile_iommu_priv.

ipmmu_add_device() would take a new IPMMU name argument, allocate an
sh_ipmmu_arch_data instance dynamically and initialize its name field to
the name passed to the function. The shmobile_iommu_priv pointer would be
set to NULL. No other operation would be performed (you will likely get
rid of the global ipmmu_devices and iommu_mapping variables).

Then, the attach_dev operation handler would retrieve the
dev->archdata.iommu pointer, cast that to an sh_ipmmu_arch_data, and
retrieve the IPMMU associated with the name (either by walking a
driver-global list of IPMMUs, or by using driver_find_device()).

This mechanism would get rid of several global variables in the driver
(several of them would move to the shmobile_ipmmu_priv structure - which I
would have named shmobile_ipmmu or even sh_ipmmu, but that's up to you)
and add support for several IPMMU instances (there's 3 of them in the
sh7372, even if we only need to support one right now it's still a good
practice to design the driver in a way that multiple instances can be
supported).

Could you try to rework the driiver in that direction ? You can have a
look at the OMAP IOMMU driver if you need sample code, and obviously feel
free to contact me if you have any question.

I agree about this is hackish. I don't mean to make an excuse,

And I'm not blaming you :-)

but I could not find good sample code because no other drivers in the
upstream kernel use the arm_iommu_attach_device() API.

This is all pretty new code, so reference implementations are missing, that's 
true.

But I will try to modify the driver to support for several IPMMU
instances.

Thank you. I can also give it a try if you want, just drop me an e-mail.

-- 
Regards,

Laurent Pinchart