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

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

[Mauro Carvalho Chehab]

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
  dt: add an spec for the Kirin36x0 SMMU
  dt: hi3670-hikey970.dts: load the SMMU driver on Hikey970
  staging: hikey9xx: add an item about the iommu driver

 .../iommu/hisilicon,kirin36x0-smmu.yaml       |  55 ++
 .../boot/dts/hisilicon/hi3670-hikey970.dts    |   3 +
 drivers/staging/hikey9xx/Kconfig              |   9 +
 drivers/staging/hikey9xx/Makefile             |   1 +
 drivers/staging/hikey9xx/TODO                 |   1 +
 drivers/staging/hikey9xx/hisi_smmu.h          | 196 ++++++
 drivers/staging/hikey9xx/hisi_smmu_lpae.c     | 648 ++++++++++++++++++
 7 files changed, 913 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/hisilicon,kirin36x0-smmu.yaml
 create mode 100644 drivers/staging/hikey9xx/hisi_smmu.h
 create mode 100644 drivers/staging/hikey9xx/hisi_smmu_lpae.c

-- 
2.26.2


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 14/16] dt: add an spec for the Kirin36x0 SMMU

[Mauro Carvalho Chehab]

Describe the properties expected by the IOMMU driver used on
Hikey960 and Hikey970 boards.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../iommu/hisilicon,kirin36x0-smmu.yaml       | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/hisilicon,kirin36x0-smmu.yaml

diff --git a/Documentation/devicetree/bindings/iommu/hisilicon,kirin36x0-smmu.yaml b/Documentation/devicetree/bindings/iommu/hisilicon,kirin36x0-smmu.yaml
new file mode 100644
index 000000000000..ec4c98faf3a5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/hisilicon,kirin36x0-smmu.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/hisilicon,kirin36x0-smmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hisilicon support for HI3660/HI3670 SMMU
+
+maintainers:
+  - Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
+
+description: |+
+  Huawei's Hisilicon Kirin 3660/3670 contains a System MMU that enables
+  scattered physical memory chunks visible as a contiguous region to
+  DMA-capable peripheral devices like GPU and ISP.
+
+  The IOMMU domains are described via iommu_info settings.
+
+properties:
+  compatible:
+    const: hisilicon,hisi-smmu-lpae
+
+  iommu_info:
+    type: object
+
+    properties:
+      start-addr:
+        maxItems: 1
+        description: Memory start address (32 bits)
+
+      size:
+        maxItems: 1
+        description: size of the I/O MMU block (32 bits)
+
+      iova-align:
+        minItems: 2
+        maxItems: 2
+        description: DMA address alignment of the mapped memory (64 bits)
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    smmu_lpae {
+      compatible = "hisilicon,smmu-lpae";
+
+      iommu_info {
+        start-addr = <0x40000>;
+        size = <0xbffc0000>;
+        iova-align = <0x0 0x8000>;
+      };
+    };
-- 
2.26.2

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Christoph Hellwig]

On Mon, Aug 17, 2020 at 09:49:59AM +0200, 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.

Please don't add iommu drivers to staging, and just work with the
maintainers to properly clean it up.

I also don't think adding a totally out of date not compiling version
is a good idea.  Please do a proper rollup, and if required (probably
not in this case), split it into useful chunks.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mauro Carvalho Chehab]

Hi Christoph,

Em Mon, 17 Aug 2020 09:21:06 +0100
Christoph Hellwig <hch@infradead.org> escreveu:

> On Mon, Aug 17, 2020 at 09:49:59AM +0200, 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.  
> 
> Please don't add iommu drivers to staging, and just work with the
> maintainers to properly clean it up.

I need to start from the original patch in order to preserve its
authorship.

My plan is to work with the iommu subsystem maintainers after
have this (and another pending patch series for DRM) merged.

> I also don't think adding a totally out of date not compiling version
> is a good idea.  Please do a proper rollup, and if required (probably
> not in this case), split it into useful chunks.

This series make this driver working as expected.

I mean, while patch 01/16 is against Kernel 4.9, the other patches
on this series ports it to upstream, cleans up the driver and
address several issues on it.

This specific IOMMU seems to be an specific part of the SoC dedicated for 
the display engine and by the encoding/decoding images via the ISP. 
With this series, this driver builds and runs as expected, providing
IOMMU support needed by the upcoming KMS/DRM driver.

The only issue on it (as far as I can tell) is that the DT bindings
require some work, as, instead of using dma-ranges, the DRM driver binds
into it with:

	smmu_lpae {
                 compatible = "hisilicon,smmu-lpae";
         };

         dpe: dpe@e8600000 {
                 compatible = "hisilicon,kirin970-dpe";
...
                 iommu_info {
                         start-addr = <0x8000>;
                         size = <0xbfff8000>;
                 };
         };

In order to properly address it, the best would be to also have the
DRM driver merged upstream, as it relies on it. So, a change in DT will 
also mean a change at the way the DRM uses it.

The DRM itself should go via staging, as it has some bugs that I'd
like to fix before moving it to drivers/gpu/drm. Those are more
tricky to solve, as they seem to require using different settings for 
some hardware registers, and the downstream driver also have the same 
issues. Fixing them will likely require some time.

Thanks,
Mauro
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Christoph Hellwig]

On Mon, Aug 17, 2020 at 11:27:25AM +0200, Mauro Carvalho Chehab wrote:
> I need to start from the original patch in order to preserve its
> authorship.

Nom you don't.  We have plenty of example where people submit code
written by other, especially when it is significantly reworked,

> My plan is to work with the iommu subsystem maintainers after
> have this (and another pending patch series for DRM) merged.

Please submit it to the iommu list directly.  Seriously - you did not
even cc the relevant list, and you don't even have a comment from
the maintainers.  Don't do this kind of crap.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Greg Kroah-Hartman]

On Mon, Aug 17, 2020 at 11:27:25AM +0200, Mauro Carvalho Chehab wrote:
> Hi Christoph,
> 
> Em Mon, 17 Aug 2020 09:21:06 +0100
> Christoph Hellwig <hch@infradead.org> escreveu:
> 
> > On Mon, Aug 17, 2020 at 09:49:59AM +0200, 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.  
> > 
> > Please don't add iommu drivers to staging, and just work with the
> > maintainers to properly clean it up.
> 
> I need to start from the original patch in order to preserve its
> authorship.
> 
> My plan is to work with the iommu subsystem maintainers after
> have this (and another pending patch series for DRM) merged.
> 
> > I also don't think adding a totally out of date not compiling version
> > is a good idea.  Please do a proper rollup, and if required (probably
> > not in this case), split it into useful chunks.
> 
> This series make this driver working as expected.
> 
> I mean, while patch 01/16 is against Kernel 4.9, the other patches
> on this series ports it to upstream, cleans up the driver and
> address several issues on it.
> 
> This specific IOMMU seems to be an specific part of the SoC dedicated for 
> the display engine and by the encoding/decoding images via the ISP. 
> With this series, this driver builds and runs as expected, providing
> IOMMU support needed by the upcoming KMS/DRM driver.
> 
> The only issue on it (as far as I can tell) is that the DT bindings
> require some work, as, instead of using dma-ranges, the DRM driver binds
> into it with:
> 
> 	smmu_lpae {
>                  compatible = "hisilicon,smmu-lpae";
>          };
> 
>          dpe: dpe@e8600000 {
>                  compatible = "hisilicon,kirin970-dpe";
> ...
>                  iommu_info {
>                          start-addr = <0x8000>;
>                          size = <0xbfff8000>;
>                  };
>          };
> 
> In order to properly address it, the best would be to also have the
> DRM driver merged upstream, as it relies on it. So, a change in DT will 
> also mean a change at the way the DRM uses it.
> 
> The DRM itself should go via staging, as it has some bugs that I'd
> like to fix before moving it to drivers/gpu/drm. Those are more
> tricky to solve, as they seem to require using different settings for 
> some hardware registers, and the downstream driver also have the same 
> issues. Fixing them will likely require some time.

DRM drivers can't go through staging unless you get the DRM developers
to agree with it, and last I heard, they were strongly against it.

It's _always_ faster to just do the work out-of-tree for a week or so
and then merge it correctly to the proper part of the kernel tree.  I'd
recommend doing that here for the iommu driver, as well as the DRM
driver.

There's no issues with authorship and the like, just properly attribute
it when you submit it and you are fine.

Again, merging in staging always takes more work and energy, don't do it
unless there is no other way.

thanks,

greg k-h
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mauro Carvalho Chehab]

Em Mon, 17 Aug 2020 11:37:03 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:

> On Mon, Aug 17, 2020 at 11:27:25AM +0200, Mauro Carvalho Chehab wrote:
> > Hi Christoph,
> > 
> > Em Mon, 17 Aug 2020 09:21:06 +0100
> > Christoph Hellwig <hch@infradead.org> escreveu:
> >   
> > > On Mon, Aug 17, 2020 at 09:49:59AM +0200, 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.    
> > > 
> > > Please don't add iommu drivers to staging, and just work with the
> > > maintainers to properly clean it up.  
> > 
> > I need to start from the original patch in order to preserve its
> > authorship.
> > 
> > My plan is to work with the iommu subsystem maintainers after
> > have this (and another pending patch series for DRM) merged.
> >   
> > > I also don't think adding a totally out of date not compiling version
> > > is a good idea.  Please do a proper rollup, and if required (probably
> > > not in this case), split it into useful chunks.  
> > 
> > This series make this driver working as expected.
> > 
> > I mean, while patch 01/16 is against Kernel 4.9, the other patches
> > on this series ports it to upstream, cleans up the driver and
> > address several issues on it.
> > 
> > This specific IOMMU seems to be an specific part of the SoC dedicated for 
> > the display engine and by the encoding/decoding images via the ISP. 
> > With this series, this driver builds and runs as expected, providing
> > IOMMU support needed by the upcoming KMS/DRM driver.
> > 
> > The only issue on it (as far as I can tell) is that the DT bindings
> > require some work, as, instead of using dma-ranges, the DRM driver binds
> > into it with:
> > 
> > 	smmu_lpae {
> >                  compatible = "hisilicon,smmu-lpae";
> >          };
> > 
> >          dpe: dpe@e8600000 {
> >                  compatible = "hisilicon,kirin970-dpe";
> > ...
> >                  iommu_info {
> >                          start-addr = <0x8000>;
> >                          size = <0xbfff8000>;
> >                  };
> >          };
> > 
> > In order to properly address it, the best would be to also have the
> > DRM driver merged upstream, as it relies on it. So, a change in DT will 
> > also mean a change at the way the DRM uses it.
> > 
> > The DRM itself should go via staging, as it has some bugs that I'd
> > like to fix before moving it to drivers/gpu/drm. Those are more
> > tricky to solve, as they seem to require using different settings for 
> > some hardware registers, and the downstream driver also have the same 
> > issues. Fixing them will likely require some time.  
> 
> DRM drivers can't go through staging unless you get the DRM developers
> to agree with it, and last I heard, they were strongly against it.


Ok, I'll ping them. There's already a thread I opened at the DRM devel
ML about this driver. IMHO, there's nothing on this specific driver
that would prevent having it on staging for a while, until the two
or three remaining bugs gets fixed.

On the other hand, the code already follows the current DRM policies,
as far as I can tell. The bugs are related to some specific register
settings that would need to be better tuned (and maybe some delay
when waiting for EDID data at the already-existing adv7535 driver).
Maybe it would be preferred to have it at drivers/gpu even with
such bugs.

> It's _always_ faster to just do the work out-of-tree for a week or so
> and then merge it correctly to the proper part of the kernel tree.  I'd
> recommend doing that here for the iommu driver, as well as the DRM
> driver.

It is OK for me to working for a week or so, until the iommu people
become happy with that. 

The main reason of submitting via staging is that I need to preserve
the patch that added this driver as-is, in order to preserve its
SoB and not causing legal issues.

It it is OK for iommu to accept a submission like that, I can
re-submit it, doing the changes at drivers/iommu.

If not, besides this series, the only alternative I can think of is to 
merge it first at the staging, with the incremental changes, and with
a final patch moving the code out of staging.

Thanks,
Mauro
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Greg Kroah-Hartman]

On Mon, Aug 17, 2020 at 12:46:17PM +0200, Mauro Carvalho Chehab wrote:
> The main reason of submitting via staging is that I need to preserve
> the patch that added this driver as-is, in order to preserve its
> SoB and not causing legal issues.
> 
> It it is OK for iommu to accept a submission like that, I can
> re-submit it, doing the changes at drivers/iommu.

You can always do this just fine, as one single patch.  You do know
about the co-developed-by: line, right?

thanks,

greg k-h
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Joerg Roedel]

On Mon, Aug 17, 2020 at 12:53:45PM +0200, Greg Kroah-Hartman wrote:
> You can always do this just fine, as one single patch.  You do know
> about the co-developed-by: line, right?

Agreed. Please keep the main iommu driver in one patch and use
co-developed-by. This makes it easier for me to review it and provide
feedback. And please Cc me on the whole patch-set for v2.

Thanks,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

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 ;)

Robin.

>    dt: add an spec for the Kirin36x0 SMMU
>    dt: hi3670-hikey970.dts: load the SMMU driver on Hikey970
>    staging: hikey9xx: add an item about the iommu driver
> 
>   .../iommu/hisilicon,kirin36x0-smmu.yaml       |  55 ++
>   .../boot/dts/hisilicon/hi3670-hikey970.dts    |   3 +
>   drivers/staging/hikey9xx/Kconfig              |   9 +
>   drivers/staging/hikey9xx/Makefile             |   1 +
>   drivers/staging/hikey9xx/TODO                 |   1 +
>   drivers/staging/hikey9xx/hisi_smmu.h          | 196 ++++++
>   drivers/staging/hikey9xx/hisi_smmu_lpae.c     | 648 ++++++++++++++++++
>   7 files changed, 913 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/iommu/hisilicon,kirin36x0-smmu.yaml
>   create mode 100644 drivers/staging/hikey9xx/hisi_smmu.h
>   create mode 100644 drivers/staging/hikey9xx/hisi_smmu_lpae.c
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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@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.

Thanks,
Mauro
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[John Stultz]

On Tue, Aug 18, 2020 at 9:26 AM Robin Murphy <robin.murphy@arm.com> wrote:
> On 2020-08-18 16:29, Mauro Carvalho Chehab wrote:
> > Em Tue, 18 Aug 2020 15:47:55 +0100
> > 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).

Yea. For the HiKey960, there was originally a similar patch series but
it was refactored out and the (still out of tree) DRM driver I'm
carrying doesn't seem to need it (though looking we still have the
iommu_info subnode in the dts that maybe needs to be cleaned up).

thanks
-john
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 2020-08-18 23:02, John Stultz wrote:
> On Tue, Aug 18, 2020 at 9:26 AM Robin Murphy <robin.murphy@arm.com> wrote:
>> On 2020-08-18 16:29, Mauro Carvalho Chehab wrote:
>>> Em Tue, 18 Aug 2020 15:47:55 +0100
>>> 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).
> 
> Yea. For the HiKey960, there was originally a similar patch series but
> it was refactored out and the (still out of tree) DRM driver I'm
> carrying doesn't seem to need it (though looking we still have the
> iommu_info subnode in the dts that maybe needs to be cleaned up).

Indeed, I'd assume it's possible to leave the MMU off and just use CMA 
buffers instead, but wiring it up properly without the downstream 
mis-design should be pretty clean, so maybe that could ultimately be 
shared with 960 too (assuming the hardware isn't wildly dissimilar).

I notice there's already a whole load of MMU configuration hard-coded 
into the DRM driver - does iommu_info even need to be in the DT, or 
could that also be decided directly by the driver? (Most other MMU-aware 
DRM drivers seem to hard-code their drm_mm dimensions.) I can't imagine 
the *virtual* address space limits need to vary on a per-board basis, 
and they could easily be tied to the compatible if they legitimately 
differ across SoCs and a simple lowest-common-denominator approach 
wouldn't suffice for whatever reason.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Mauro Carvalho Chehab]

Em Tue, 18 Aug 2020 15:02:54 -0700
John Stultz <john.stultz@linaro.org> escreveu:

> On Tue, Aug 18, 2020 at 9:26 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > On 2020-08-18 16:29, Mauro Carvalho Chehab wrote:  
> > > Em Tue, 18 Aug 2020 15:47:55 +0100
> > > 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).  
> 
> Yea. For the HiKey960, there was originally a similar patch series but
> it was refactored out and the (still out of tree) DRM driver I'm
> carrying doesn't seem to need it (though looking we still have the
> iommu_info subnode in the dts that maybe needs to be cleaned up).

Funny... while the Hikey 970 DRM driver has such IOMMU code, it
doesn't actually use it!

The driver has a function called hisi_dss_smmu_config() with
sets the registers on a different way in order to use IOMMU
or not, at the hisi_fb_pan_display() function. It can also
use a mode called "afbcd".

Well, this function sets both to false:

	bool afbcd = false;
	bool mmu_enable = false;

I ended commenting out the code which depends at the iommu
driver and everything is working as before.

So, I'll just forget about this iommu driver, as we can live
without that.

For now, I'll keep the mmu code there commented out, as
it could be useful on a future port for it to use io-pgtable.

-

Robin,

Can the Panfrost driver use io-pgtable while the KMS driver
won't be using it? Or this would cause it to not work?

My end goal here is to be able to test the Panfrost driver ;-)

Thanks,
Mauro
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Robin Murphy]

On 2020-08-19 11:28, Mauro Carvalho Chehab wrote:
> Em Tue, 18 Aug 2020 15:02:54 -0700
> John Stultz <john.stultz@linaro.org> escreveu:
> 
>> On Tue, Aug 18, 2020 at 9:26 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>> On 2020-08-18 16:29, Mauro Carvalho Chehab wrote:
>>>> Em Tue, 18 Aug 2020 15:47:55 +0100
>>>> 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).
>>
>> Yea. For the HiKey960, there was originally a similar patch series but
>> it was refactored out and the (still out of tree) DRM driver I'm
>> carrying doesn't seem to need it (though looking we still have the
>> iommu_info subnode in the dts that maybe needs to be cleaned up).
> 
> Funny... while the Hikey 970 DRM driver has such IOMMU code, it
> doesn't actually use it!
> 
> The driver has a function called hisi_dss_smmu_config() with
> sets the registers on a different way in order to use IOMMU
> or not, at the hisi_fb_pan_display() function. It can also
> use a mode called "afbcd".
> 
> Well, this function sets both to false:
> 
> 	bool afbcd = false;
> 	bool mmu_enable = false;
> 
> I ended commenting out the code which depends at the iommu
> driver and everything is working as before.
> 
> So, I'll just forget about this iommu driver, as we can live
> without that.
> 
> For now, I'll keep the mmu code there commented out, as
> it could be useful on a future port for it to use io-pgtable.
> 
> -
> 
> Robin,
> 
> Can the Panfrost driver use io-pgtable while the KMS driver
> won't be using it? Or this would cause it to not work?
> 
> My end goal here is to be able to test the Panfrost driver ;-)

Yup, the GPU has its own independent MMU, so Panfrost can import display 
buffers regardless of whether they're physically contiguous or not. 
Since Mesa master has recently landed AFBC support, there's probably 
more immediate benefit in getting that AFBC decoder working before the 
display MMU (although ultimately things are likely to work better under 
memory pressure if you don't have to rely on CMA, so it should still be 
worth coming back to at some point).

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu