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

[Hideki EIRAKU <hdk@igel.co.jp>]

Hi Laurent,

Thank you for your comments.

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH v4 1/2] iommu/shmobile: Add iommu driver for Renesas IPMMU modules
Date: Mon, 10 Dec 2012 16:55:58 +0100

> On Monday 15 October 2012 17:34:52 Hideki EIRAKU wrote:
>> 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,

>> 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 <hdk@igel.co.jp>
>> ---
>>  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.

>> + * 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.

>> +	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, but I
could not find good sample code because no other drivers in the
upstream kernel use the arm_iommu_attach_device() API.

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

-- 
Hideki EIRAKU <hdk@igel.co.jp>