Re: [PATCH 00/16] IOMMU driver for Kirin 960/970

[Mauro Carvalho Chehab]

Hi Robin,

Em Tue, 18 Aug 2020 15:47:55 +0100
Robin Murphy <robin.murphy@xxxxxxx> 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.

Thanks,
Mauro