Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 00/16] IOMMU driver for Kirin 960/970
To:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        devel@driverdev.osuosl.org, devicetree@vger.kernel.org,
        Joerg Roedel <jroedel@suse.de>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Chenfeng <puck.chen@hisilicon.com>, linuxarm@huawei.com,
        Wei Xu <xuwei5@hisilicon.com>, linux-kernel@vger.kernel.org,
        iommu@lists.linux-foundation.org, Rob Herring <robh+dt@kernel.org>,
        John Stultz <john.stultz@linaro.org>, mauro.chehab@huawei.com,
        Suzhuangluan <suzhuangluan@hisilicon.com>,
        linux-arm-kernel@lists.infradead.org
References: <cover.1597650455.git.mchehab+huawei@kernel.org>
 <5c7918b6-c506-680b-cb0f-9e5f6a7038d9@arm.com>
 <20200818172909.71f5243a@coco.lan>
From:   Robin Murphy <robin.murphy@arm.com>
Message-ID: <79f40595-7769-aa6a-fbba-53adcffca327@arm.com>
Date:   Tue, 18 Aug 2020 17:26:30 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200818172909.71f5243a@coco.lan>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-08-18 16:29, Mauro Carvalho Chehab wrote:
> Hi Robin,
> 
> Em Tue, 18 Aug 2020 15:47:55 +0100
> Robin Murphy <robin.murphy@arm.com> escreveu:
> 
>> On 2020-08-17 08:49, Mauro Carvalho Chehab wrote:
>>> Add a driver for the Kirin 960/970 iommu.
>>>
>>> As on the past series, this starts from the original 4.9 driver from
>>> the 96boards tree:
>>>
>>> 	https://github.com/96boards-hikey/linux/tree/hikey970-v4.9
>>>
>>> The remaining patches add SPDX headers and make it build and run with
>>> the upstream Kernel.
>>>
>>> Chenfeng (1):
>>>     iommu: add support for HiSilicon Kirin 960/970 iommu
>>>
>>> Mauro Carvalho Chehab (15):
>>>     iommu: hisilicon: remove default iommu_map_sg handler
>>>     iommu: hisilicon: map and unmap ops gained new arguments
>>>     iommu: hisi_smmu_lpae: rebase it to work with upstream
>>>     iommu: hisi_smmu: remove linux/hisi/hisi-iommu.h
>>>     iommu: hisilicon: cleanup its code style
>>>     iommu: hisi_smmu_lpae: get rid of IOMMU_SEC and IOMMU_DEVICE
>>>     iommu: get rid of map/unmap tile functions
>>>     iommu: hisi_smmu_lpae: use the right code to get domain-priv data
>>>     iommu: hisi_smmu_lpae: convert it to probe_device
>>>     iommu: add Hisilicon Kirin970 iommu at the building system
>>>     iommu: hisi_smmu_lpae: cleanup printk macros
>>>     iommu: hisi_smmu_lpae: make OF compatible more standard
>>
>> Echoing the other comments about none of the driver patches being CC'd
>> to the IOMMU list...
>>
>> Still, I dug the series up on lore and frankly I'm not sure what to make
>> of it - AFAICS the "driver" is just yet another implementation of Arm
>> LPAE pagetable code, with no obvious indication of how those pagetables
>> ever get handed off to IOMMU hardware (and indeed no indication of IOMMU
>> hardware at all). Can you explain how it's supposed to work?
>>
>> And as a pre-emptive strike, we really don't need any more LPAE
>> implementations - that's what the io-pgtable library is all about (which
>> incidentally has been around since 4.0...). I think that should make the
>> issue of preserving authorship largely moot since there's no need to
>> preserve most of the code anyway ;)
> 
> I didn't know about that, since I got a Hikey 970 board for the first time
> about one month ago, and that's the first time I looked into iommu code.
> 
> My end goal with this is to make the DRM/KMS driver to work with upstream
> Kernels.
> 
> The full patch series are at:
> 
> 	https://github.com/mchehab/linux/commits/hikey970/to_upstream-2.0-v1.1
> 
> (I need to put a new version there, after some changes due to recent
> upstream discussions at the regulator's part of the code)
> 
> Basically, the DT binding has this, for IOMMU:
> 
> 
> 	smmu_lpae {
> 		compatible = "hisilicon,smmu-lpae";
> 	};
> 
> ...
> 	dpe: dpe@e8600000 {
> 		compatible = "hisilicon,kirin970-dpe";
> 		memory-region = <&drm_dma_reserved>;
> ...
> 		iommu_info {
> 			start-addr = <0x8000>;
> 			size = <0xbfff8000>;
> 		};
> 	}
> 
> This is used by kirin9xx_drm_dss.c in order to enable and use
> the iommu:
> 
> 
> 	static int dss_enable_iommu(struct platform_device *pdev, struct dss_hw_ctx *ctx)
> 	{
> 		struct device *dev = NULL;
> 
> 		dev = &pdev->dev;
> 
> 		/* create iommu domain */
> 		ctx->mmu_domain = iommu_domain_alloc(dev->bus);
> 		if (!ctx->mmu_domain) {
> 			pr_err("iommu_domain_alloc failed!\n");
> 			return -EINVAL;
> 		}
> 
> 		iommu_attach_device(ctx->mmu_domain, dev);
> 
> 		return 0;
> 	}
> 
> The only place where the IOMMU domain is used is on this part of the
> code(error part simplified here) [1]:
> 
> 	void hisi_dss_smmu_on(struct dss_hw_ctx *ctx)
> 	{
> 		uint64_t fama_phy_pgd_base;
> 		uint32_t phy_pgd_base;
> ...
> 		fama_phy_pgd_base = iommu_iova_to_phys(ctx->mmu_domain, 0);
> 		phy_pgd_base = (uint32_t)fama_phy_pgd_base;
> 		if (WARN_ON(!phy_pgd_base))
> 			return;
> 
> 		set_reg(smmu_base + SMMU_CB_TTBR0, phy_pgd_base, 32, 0);
> 	}
> 
> [1] https://github.com/mchehab/linux/commit/36da105e719b47bbe9d6cb7e5619b30c7f3eb1bd
> 
> In other words, the driver needs to get the physical address of the frame
> buffer (mapped via iommu) in order to set some DRM-specific register.
> 
> Yeah, the above code is somewhat hackish. I would love to replace
> this part by a more standard approach.

OK, so from a quick look at that, my impression is that your display 
controller has its own MMU and you don't need to pretend to use the 
IOMMU API at all. Just have the DRM driver use io-pgtable directly to 
run its own set of ARM_32_LPAE_S1 pagetables - see Panfrost for an 
example (but try to ignore the wacky "Mali LPAE" format).

Robin.