From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-m15571.qiye.163.com (mail-m15571.qiye.163.com [101.71.155.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 66D13364EB6 for ; Wed, 1 Apr 2026 09:31:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=101.71.155.71 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775035911; cv=none; b=aTCDS1JD0N4ctKq9jgJECvIkONsnD2Oh9iAoREp+TJX0smx/RuDQuJ0YcBZqFKf5TyzGIU0Z3KuMZ3MenrjGcDfwwc7efdDDadT+AQ13JuEdRVxuFiipmai6ZLF+0l0k+oGNZ8QL9Y3CFaRovYz7BNZS8pfae6C6Fxro+ePauFg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775035911; c=relaxed/simple; bh=RZKEjqBL4q7GCNAYxQ1VOF5lnvHFEl0fuCdwAzQ7NYA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=nJLBgkZ5NZnSk5RHAk977hA7TSEZSuTF+59DAW++dpi3JWi5wjqjQ/MBl73g7m7MccvCbBTYnWaBSznhrMHFsMjyYLGIwK/l6/Lq15uDvBlIrd/tmCIuypb+u52pVEVSeaNGOQnIQzJx++oa9XjlROnhUGrgg0o26JvBmr+uS9s= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=rock-chips.com; spf=pass smtp.mailfrom=rock-chips.com; dkim=pass (1024-bit key) header.d=rock-chips.com header.i=@rock-chips.com header.b=Jt+2kfwe; arc=none smtp.client-ip=101.71.155.71 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=rock-chips.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=rock-chips.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=rock-chips.com header.i=@rock-chips.com header.b="Jt+2kfwe" Received: from [192.168.31.31] (gy-adaptive-ssl-proxy-3-entmail-virt135.gy.ntes [58.22.7.114]) by smtp.qiye.163.com (Hmail) with ESMTP id 3924261a7; Wed, 1 Apr 2026 17:26:28 +0800 (GMT+08:00) Message-ID: <571079ed-a1e3-4fa0-a42a-28cd39864c22@rock-chips.com> Date: Wed, 1 Apr 2026 17:26:28 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] iommu/rockchip: Drop global rk_ops in favor of per-device ops To: Jonas Karlman , Shawn Lin Cc: iommu@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, Joerg Roedel , Will Deacon , Robin Murphy , Heiko Stuebner References: <20260310084812.126082-1-xxm@rock-chips.com> <20260310105303.128859-1-xxm@rock-chips.com> <8ca0a8d3-aed9-4c2a-89dd-e9029afdd279@kwiboo.se> From: Simon Xue In-Reply-To: <8ca0a8d3-aed9-4c2a-89dd-e9029afdd279@kwiboo.se> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-HM-Tid: 0a9d485d9f4f03ackunmcabf1e6a2ac0554 X-HM-MType: 1 X-HM-Spam-Status: e1kfGhgUHx5ZQUpXWQgPGg8OCBgUHx5ZQUlOS1dZFg8aDwILHllBWSg2Ly tZV1koWUFDSUNOT01LS0k3V1ktWUFJV1kPCRoVCBIfWUFZGU8ZSVYdQ05DHkhITk0fGEtWFRQJFh oXVRMBExYaEhckFA4PWVdZGBILWUFZTkNVSUlVTFVKSk9ZV1kWGg8SFR0UWUFZT0tIVUJCSU5LVU pLS1VKQktCWQY+ DKIM-Signature: a=rsa-sha256; b=Jt+2kfweWEwxes0sBj7HSXd9JCL3FN2m69A+IQyV71t3yOGMQE3utUNp0av7cbf9WefbFWcfCXueu2/uLBDiLczXJSTaORfaFlaNkhU+WuFMORIMw4Emd+VC8Qj/QFRzcQDfW+buGb3JhrUK98UDI9iB2MM487U/NHJOwcaDcys=; c=relaxed/relaxed; s=default; d=rock-chips.com; v=1; bh=b9sUdNfaZ3PRLrdJkysMaTf4aAlb6n1SWYaSIoqHMio=; h=date:mime-version:subject:message-id:from; Hi Jonas， Thanks for the  review. 在 2026/4/1 16:25, Jonas Karlman 写道: > Hi, > > On 4/1/2026 9:59 AM, Simon Xue wrote: >> Hi all, >> >> A gentle ping on this patch. >> >> 在 2026/3/13 17:32, Shawn Lin 写道: >>> 在 2026/03/10 星期二 18:53, Simon Xue 写道: >>>> The driver currently uses a global rk_ops pointer, forcing all IOMMU >>>> instances to share the same operations. This restricts the driver from >>>> supporting SoCs that might integrate different versions of IOMMU >>>> hardware. >>>> >>>> Since the IOMMU framework passes the master device information to >>>> iommu_paging_domain_alloc(), the global variable is no longer needed. >>>> >>>> Fix this by moving rk_ops into struct rk_iommu and struct >>>> rk_iommu_domain. >>>> Initialize it per-device during probe via of_device_get_match_data(), >>>> and replace all global references with the instance-specific pointers. >>>> >>> Thanks for the patch, Simon. I've tested it on the RK3576 EVB1 with >>> PCIe1 + IOMMU. NVMe works fine on it, and I also verified the IOVA >>> allocated in the NVMe driver, they look correct as I manually limited >>> the memblock to under 2GB, so here it is: >>> >>> nvme 0001:21:00.0: cq_dma_addr: 0x00000000f7fc7000 >>> >>> Tested-by: Shawn Lin >>> Reviewed-by: Shawn Lin >>> >>>> Signed-off-by: Simon Xue >>>> --- >>>> v2: >>>>   - Remove the one-time-used 'ops' variable in rk_iommu_probe() >>>> >>>>   drivers/iommu/rockchip-iommu.c | 71 ++++++++++++++++------------------ >>>>   1 file changed, 33 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/drivers/iommu/rockchip-iommu.c >>>> b/drivers/iommu/rockchip-iommu.c >>>> index 0013cf196c57..4da80136933c 100644 >>>> --- a/drivers/iommu/rockchip-iommu.c >>>> +++ b/drivers/iommu/rockchip-iommu.c >>>> @@ -82,6 +82,14 @@ >>>>     */ >>>>   #define RK_IOMMU_PGSIZE_BITMAP 0x007ff000 >>>>   +struct rk_iommu_ops { >>>> +    phys_addr_t (*pt_address)(u32 dte); >>>> +    u32 (*mk_dtentries)(dma_addr_t pt_dma); >>>> +    u32 (*mk_ptentries)(phys_addr_t page, int prot); >>>> +    u64 dma_bit_mask; >>>> +    gfp_t gfp_flags; >>>> +}; >>>> + >>>>   struct rk_iommu_domain { >>>>       struct list_head iommus; >>>>       u32 *dt; /* page directory table */ >>>> @@ -89,6 +97,7 @@ struct rk_iommu_domain { >>>>       spinlock_t iommus_lock; /* lock for iommus list */ >>>>       spinlock_t dt_lock; /* lock for modifying page directory table */ >>>>       struct device *dma_dev; >>>> +    const struct rk_iommu_ops *rk_ops; >>>>         struct iommu_domain domain; >>>>   }; >>>> @@ -98,14 +107,6 @@ static const char * const rk_iommu_clocks[] = { >>>>       "aclk", "iface", >>>>   }; >>>>   -struct rk_iommu_ops { >>>> -    phys_addr_t (*pt_address)(u32 dte); >>>> -    u32 (*mk_dtentries)(dma_addr_t pt_dma); >>>> -    u32 (*mk_ptentries)(phys_addr_t page, int prot); >>>> -    u64 dma_bit_mask; >>>> -    gfp_t gfp_flags; >>>> -}; >>>> - >>>>   struct rk_iommu { >>>>       struct device *dev; >>>>       void __iomem **bases; >>>> @@ -117,6 +118,7 @@ struct rk_iommu { >>>>       struct iommu_device iommu; >>>>       struct list_head node; /* entry in rk_iommu_domain.iommus */ >>>>       struct iommu_domain *domain; /* domain to which iommu is >>>> attached */ >>>> +    const struct rk_iommu_ops *rk_ops; > Do we really need the rk_ops on both the rk_iommu and rk_iommu_domain? > >>>>   }; >>>>     struct rk_iommudata { >>>> @@ -124,7 +126,6 @@ struct rk_iommudata { >>>>       struct rk_iommu *iommu; >>>>   }; >>>>   -static const struct rk_iommu_ops *rk_ops; >>>>   static struct iommu_domain rk_identity_domain; >>>>     static inline void rk_table_flush(struct rk_iommu_domain *dom, >>>> dma_addr_t dma, >>>> @@ -510,7 +511,7 @@ static int rk_iommu_force_reset(struct rk_iommu >>>> *iommu) >>>>        * and verifying that upper 5 (v1) or 7 (v2) nybbles are read >>>> back. >>>>        */ >>>>       for (i = 0; i < iommu->num_mmu; i++) { >>>> -        dte_addr = rk_ops->pt_address(DTE_ADDR_DUMMY); >>>> +        dte_addr = iommu->rk_ops->pt_address(DTE_ADDR_DUMMY); > Maybe this patch it trying to do too much at once that makes it harder > to review? To simplify review maybe one patch just drops the global > rk_ops and a assigns a local version based on domain/iommu, and a second > patch changes to use the iommu->rk_ops directly? Just a thought. 1. I think it's hard to split this patch safely. Once the global rk_ops is removed, domain-only paths (map/unmap/iova_to_phys/domain_free) still need SoC-specific helpers, while they only receive struct iommu_domain *. 2. Looking up ops from an attached IOMMU at call time may not reliable. So keeping rk_ops in both rk_domain and rk_iommu is reasonable. > >>>>           rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, dte_addr); >>>>             if (dte_addr != rk_iommu_read(iommu->bases[i], >>>> RK_MMU_DTE_ADDR)) { >>>> @@ -551,7 +552,7 @@ static void log_iova(struct rk_iommu *iommu, int >>>> index, dma_addr_t iova) >>>>       page_offset = rk_iova_page_offset(iova); >>>>         mmu_dte_addr = rk_iommu_read(base, RK_MMU_DTE_ADDR); >>>> -    mmu_dte_addr_phys = rk_ops->pt_address(mmu_dte_addr); >>>> +    mmu_dte_addr_phys = iommu->rk_ops->pt_address(mmu_dte_addr); >>>>         dte_addr_phys = mmu_dte_addr_phys + (4 * dte_index); >>>>       dte_addr = phys_to_virt(dte_addr_phys); >>>> @@ -560,14 +561,14 @@ static void log_iova(struct rk_iommu *iommu, >>>> int index, dma_addr_t iova) >>>>       if (!rk_dte_is_pt_valid(dte)) >>>>           goto print_it; >>>>   -    pte_addr_phys = rk_ops->pt_address(dte) + (pte_index * 4); >>>> +    pte_addr_phys = iommu->rk_ops->pt_address(dte) + (pte_index * 4); >>>>       pte_addr = phys_to_virt(pte_addr_phys); >>>>       pte = *pte_addr; >>>>         if (!rk_pte_is_page_valid(pte)) >>>>           goto print_it; >>>>   -    page_addr_phys = rk_ops->pt_address(pte) + page_offset; >>>> +    page_addr_phys = iommu->rk_ops->pt_address(pte) + page_offset; >>>>       page_flags = pte & RK_PTE_PAGE_FLAGS_MASK; >>>>     print_it: >>>> @@ -663,13 +664,13 @@ static phys_addr_t rk_iommu_iova_to_phys(struct >>>> iommu_domain *domain, >>>>       if (!rk_dte_is_pt_valid(dte)) >>>>           goto out; >>>>   -    pt_phys = rk_ops->pt_address(dte); >>>> +    pt_phys = rk_domain->rk_ops->pt_address(dte); >>>>       page_table = (u32 *)phys_to_virt(pt_phys); >>>>       pte = page_table[rk_iova_pte_index(iova)]; >>>>       if (!rk_pte_is_page_valid(pte)) >>>>           goto out; >>>>   -    phys = rk_ops->pt_address(pte) + rk_iova_page_offset(iova); >>>> +    phys = rk_domain->rk_ops->pt_address(pte) + >>>> rk_iova_page_offset(iova); >>>>   out: >>>>       spin_unlock_irqrestore(&rk_domain->dt_lock, flags); >>>>   @@ -730,7 +731,7 @@ static u32 *rk_dte_get_page_table(struct >>>> rk_iommu_domain *rk_domain, >>>>       if (rk_dte_is_pt_valid(dte)) >>>>           goto done; >>>>   -    page_table = iommu_alloc_pages_sz(GFP_ATOMIC | rk_ops->gfp_flags, >>>> +    page_table = iommu_alloc_pages_sz(GFP_ATOMIC | >>>> rk_domain->rk_ops->gfp_flags, >>>>                         SPAGE_SIZE); >>>>       if (!page_table) >>>>           return ERR_PTR(-ENOMEM); >>>> @@ -742,13 +743,13 @@ static u32 *rk_dte_get_page_table(struct >>>> rk_iommu_domain *rk_domain, >>>>           return ERR_PTR(-ENOMEM); >>>>       } >>>>   -    dte = rk_ops->mk_dtentries(pt_dma); >>>> +    dte = rk_domain->rk_ops->mk_dtentries(pt_dma); >>>>       *dte_addr = dte; >>>>         rk_table_flush(rk_domain, >>>>                  rk_domain->dt_dma + dte_index * sizeof(u32), 1); >>>>   done: >>>> -    pt_phys = rk_ops->pt_address(dte); >>>> +    pt_phys = rk_domain->rk_ops->pt_address(dte); >>>>       return (u32 *)phys_to_virt(pt_phys); >>>>   } >>>>   @@ -790,7 +791,7 @@ static int rk_iommu_map_iova(struct >>>> rk_iommu_domain *rk_domain, u32 *pte_addr, >>>>           if (rk_pte_is_page_valid(pte)) >>>>               goto unwind; >>>>   -        pte_addr[pte_count] = rk_ops->mk_ptentries(paddr, prot); >>>> +        pte_addr[pte_count] = rk_domain->rk_ops->mk_ptentries(paddr, >>>> prot); >>>>             paddr += SPAGE_SIZE; >>>>       } >>>> @@ -812,7 +813,7 @@ static int rk_iommu_map_iova(struct >>>> rk_iommu_domain *rk_domain, u32 *pte_addr, >>>>                   pte_count * SPAGE_SIZE); >>>>         iova += pte_count * SPAGE_SIZE; >>>> -    page_phys = rk_ops->pt_address(pte_addr[pte_count]); >>>> +    page_phys = rk_domain->rk_ops->pt_address(pte_addr[pte_count]); >>>>       pr_err("iova: %pad already mapped to %pa cannot remap to phys: >>>> %pa prot: %#x\n", >>>>              &iova, &page_phys, &paddr, prot); >>>>   @@ -849,7 +850,7 @@ static int rk_iommu_map(struct iommu_domain >>>> *domain, unsigned long _iova, >>>>       pte_index = rk_iova_pte_index(iova); >>>>       pte_addr = &page_table[pte_index]; >>>>   -    pte_dma = rk_ops->pt_address(dte_index) + pte_index * >>>> sizeof(u32); >>>> +    pte_dma = rk_domain->rk_ops->pt_address(dte_index) + pte_index * >>>> sizeof(u32); >>>>       ret = rk_iommu_map_iova(rk_domain, pte_addr, pte_dma, iova, >>>>                   paddr, size, prot); >>>>   @@ -887,7 +888,7 @@ static size_t rk_iommu_unmap(struct >>>> iommu_domain *domain, unsigned long _iova, >>>>           return 0; >>>>       } >>>>   -    pt_phys = rk_ops->pt_address(dte); >>>> +    pt_phys = rk_domain->rk_ops->pt_address(dte); >>>>       pte_addr = (u32 *)phys_to_virt(pt_phys) + rk_iova_pte_index(iova); >>>>       pte_dma = pt_phys + rk_iova_pte_index(iova) * sizeof(u32); >>>>       unmap_size = rk_iommu_unmap_iova(rk_domain, pte_addr, pte_dma, >>>> size); >>>> @@ -945,7 +946,7 @@ static int rk_iommu_enable(struct rk_iommu *iommu) >>>>         for (i = 0; i < iommu->num_mmu; i++) { >>>>           rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, >>>> - rk_ops->mk_dtentries(rk_domain->dt_dma)); >>>> + iommu->rk_ops->mk_dtentries(rk_domain->dt_dma)); >>>>           rk_iommu_base_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE); >>>>           rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, >>>> RK_MMU_IRQ_MASK); >>>>       } >>>> @@ -1068,17 +1069,19 @@ static struct iommu_domain >>>> *rk_iommu_domain_alloc_paging(struct device *dev) >>>>       if (!rk_domain) >>>>           return NULL; >>>>   +    iommu = rk_iommu_from_dev(dev); >>>> +    rk_domain->rk_ops = iommu->rk_ops; > As mentioned above, this seem strange and is possible just a shortcut > due to current call-paths? Why do we assign "iommu ops" to the domain? > > Regards, > Jonas > >>>> + >>>>       /* >>>>        * rk32xx iommus use a 2 level pagetable. >>>>        * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries. >>>>        * Allocate one 4 KiB page for each table. >>>>        */ >>>> -    rk_domain->dt = iommu_alloc_pages_sz(GFP_KERNEL | >>>> rk_ops->gfp_flags, >>>> +    rk_domain->dt = iommu_alloc_pages_sz(GFP_KERNEL | >>>> rk_domain->rk_ops->gfp_flags, >>>>                            SPAGE_SIZE); >>>>       if (!rk_domain->dt) >>>>           goto err_free_domain; >>>>   -    iommu = rk_iommu_from_dev(dev); >>>>       rk_domain->dma_dev = iommu->dev; >>>>       rk_domain->dt_dma = dma_map_single(rk_domain->dma_dev, >>>> rk_domain->dt, >>>>                          SPAGE_SIZE, DMA_TO_DEVICE); >>>> @@ -1117,7 +1120,7 @@ static void rk_iommu_domain_free(struct >>>> iommu_domain *domain) >>>>       for (i = 0; i < NUM_DT_ENTRIES; i++) { >>>>           u32 dte = rk_domain->dt[i]; >>>>           if (rk_dte_is_pt_valid(dte)) { >>>> -            phys_addr_t pt_phys = rk_ops->pt_address(dte); >>>> +            phys_addr_t pt_phys = rk_domain->rk_ops->pt_address(dte); >>>>               u32 *page_table = phys_to_virt(pt_phys); >>>>               dma_unmap_single(rk_domain->dma_dev, pt_phys, >>>>                        SPAGE_SIZE, DMA_TO_DEVICE); >>>> @@ -1197,7 +1200,6 @@ static int rk_iommu_probe(struct >>>> platform_device *pdev) >>>>       struct device *dev = &pdev->dev; >>>>       struct rk_iommu *iommu; >>>>       struct resource *res; >>>> -    const struct rk_iommu_ops *ops; >>>>       int num_res = pdev->num_resources; >>>>       int err, i; >>>>   @@ -1211,16 +1213,9 @@ static int rk_iommu_probe(struct >>>> platform_device *pdev) >>>>       iommu->dev = dev; >>>>       iommu->num_mmu = 0; >>>>   -    ops = of_device_get_match_data(dev); >>>> -    if (!rk_ops) >>>> -        rk_ops = ops; >>>> - >>>> -    /* >>>> -     * That should not happen unless different versions of the >>>> -     * hardware block are embedded the same SoC >>>> -     */ >>>> -    if (WARN_ON(rk_ops != ops)) >>>> -        return -EINVAL; >>>> +    iommu->rk_ops = of_device_get_match_data(dev); >>>> +    if (!iommu->rk_ops) >>>> +        return -ENOENT; >>>>         iommu->bases = devm_kcalloc(dev, num_res, sizeof(*iommu->bases), >>>>                       GFP_KERNEL); >>>> @@ -1286,7 +1281,7 @@ static int rk_iommu_probe(struct >>>> platform_device *pdev) >>>>               goto err_pm_disable; >>>>       } >>>>   -    dma_set_mask_and_coherent(dev, rk_ops->dma_bit_mask); >>>> +    dma_set_mask_and_coherent(dev, iommu->rk_ops->dma_bit_mask); >>>>         err = iommu_device_sysfs_add(&iommu->iommu, dev, NULL, >>>> dev_name(dev)); >>>>       if (err) >>>> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip >