Re: [PATCH v6 3/5] iommu: Add verisilicon IOMMU driver

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 14/07/2025 à 14:08, Will Deacon a écrit :
> Hi,
>
> On Thu, Jul 10, 2025 at 10:24:44AM +0200, Benjamin Gaignard wrote:
>> diff --git a/drivers/iommu/vsi-iommu.c b/drivers/iommu/vsi-iommu.c
>> new file mode 100644
>> index 000000000000..15322b9929af
>> --- /dev/null
>> +++ b/drivers/iommu/vsi-iommu.c
>> @@ -0,0 +1,781 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright (C) 2025 Collabora Ltd.
>> + *
>> + * IOMMU API for Verisilicon
>> + *
>> + * Module Authors:	Yandong Lin <yandong.lin@rock-chips.com>
>> + *			Simon Xue <xxm@rock-chips.com>
>> + *			Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/compiler.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iommu.h>
>> +#include <linux/list.h>
>> +#include <linux/mm.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_iommu.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +
>> +#include "iommu-pages.h"
>> +
>> +struct vsi_iommu {
>> +	struct device *dev;
>> +	void __iomem *regs;
>> +	struct clk_bulk_data *clocks;
>> +	int num_clocks;
>> +	struct iommu_device iommu;
>> +	struct list_head node; /* entry in vsi_iommu_domain.iommus */
>> +	struct iommu_domain *domain; /* domain to which iommu is attached */
>> +	spinlock_t lock;
>> +	int irq;
>> +};
>> +
>> +struct vsi_iommu_domain {
>> +	struct list_head iommus;
>> +	struct device *dev;
>> +	u32 *dt;
>> +	dma_addr_t dt_dma;
>> +	struct iommu_domain domain;
>> +	u64 *pta;
>> +	dma_addr_t pta_dma;
>> +	spinlock_t lock;
>> +};
>> +
>> +static struct iommu_domain vsi_identity_domain;
>> +
>> +#define NUM_DT_ENTRIES	1024
>> +#define NUM_PT_ENTRIES	1024
>> +#define PT_SIZE		(NUM_PT_ENTRIES * sizeof(u32))
>> +
>> +#define SPAGE_SIZE	BIT(12)
>> +
>> +/* vsi iommu regs address */
>> +#define VSI_MMU_CONFIG1_BASE			0x1ac
>> +#define VSI_MMU_AHB_EXCEPTION_BASE		0x380
>> +#define VSI_MMU_AHB_CONTROL_BASE		0x388
>> +#define VSI_MMU_AHB_TLB_ARRAY_BASE_L_BASE	0x38C
>> +
>> +/* MMU register offsets */
>> +#define VSI_MMU_FLUSH_BASE		0x184
>> +#define VSI_MMU_BIT_FLUSH		BIT(4)
>> +
>> +#define VSI_MMU_PAGE_FAULT_ADDR		0x380
>> +#define VSI_MMU_STATUS_BASE		0x384	/* IRQ status */
>> +
>> +#define VSI_MMU_BIT_ENABLE		BIT(0)
>> +
>> +#define VSI_MMU_OUT_OF_BOUND		BIT(28)
>> +/* Irq mask */
>> +#define VSI_MMU_IRQ_MASK		0x7
>> +
>> +#define VSI_DTE_PT_ADDRESS_MASK		0xffffffc0
>> +#define VSI_DTE_PT_VALID		BIT(0)
>> +
>> +#define VSI_PAGE_DESC_LO_MASK		0xfffff000
>> +#define VSI_PAGE_DESC_HI_MASK		GENMASK_ULL(39, 32)
>> +#define VSI_PAGE_DESC_HI_SHIFT		(32 - 4)
> How does this page-table format relate to the one supported already by
> rockchip-iommu.c? From a quick glance, I suspect this is a derivative
> and so ideally we'd be able to have a common implementation of the
> page-table code which can be used by both of the drivers.
>
> Similarly:

No they comes from different IP providers, this one is from Verisilicon.
I agree they looks very similar and my first attempt was to add it into
rockchip-iommu code but when doing it I realize that registers addresses
where all different so I had to code all the functions twice.

Regards,
Benjamin

>
>> +static void vsi_iommu_domain_free(struct iommu_domain *domain)
>> +{
>> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
>> +	unsigned long flags;
>> +	int i;
>> +
>> +	spin_lock_irqsave(&vsi_domain->lock, flags);
>> +
>> +	WARN_ON(!list_empty(&vsi_domain->iommus));
>> +
>> +	for (i = 0; i < NUM_DT_ENTRIES; i++) {
>> +		u32 dte = vsi_domain->dt[i];
>> +
>> +		if (vsi_dte_is_pt_valid(dte)) {
>> +			phys_addr_t pt_phys = vsi_dte_pt_address(dte);
>> +			u32 *page_table = phys_to_virt(pt_phys);
>> +
>> +			dma_unmap_single(vsi_domain->dev, pt_phys,
>> +					 SPAGE_SIZE, DMA_TO_DEVICE);
>> +			iommu_free_pages(page_table);
>> +		}
>> +	}
>> +
>> +	dma_unmap_single(vsi_domain->dev, vsi_domain->dt_dma,
>> +			 SPAGE_SIZE, DMA_TO_DEVICE);
>> +	iommu_free_pages(vsi_domain->dt);
>> +
>> +	dma_unmap_single(vsi_domain->dev, vsi_domain->pta_dma,
>> +			 SPAGE_SIZE, DMA_TO_DEVICE);
>> +	iommu_free_pages(vsi_domain->pta);
>> +
>> +	spin_unlock_irqrestore(&vsi_domain->lock, flags);
>> +
>> +	kfree(vsi_domain);
>> +}
> is almost a carbon copy of rk_iommu_domain_free(), so it seems that
> there's room for code re-use even beyond the page-table support.
>
> I think that also means we'll want Heiko's Ack before we merge anything.
>
> Will