[PATCH v2 0/8] Tegra GART driver clean up and optimization

[PATCH v2 0/8] Tegra GART driver clean up and optimization

[Dmitry Osipenko]

Hello,

This patch-series makes GART driver one step closer to become actually
usable by addressing the following:

1. Thierry noticed that Memory Controller driver uses registers that belong
   to GART in [0] and for now MC driver only reports the fact of GART's page
   fault. The first two patches of the series are addressing this shortcoming
   by providing integration of the MC driver with the GART.

   [0] https://www.spinics.net/lists/linux-tegra/msg33072.html

2. Currently GART has bee kept disabled by the commit c7e3ca515e784
   ("iommu/tegra: gart: Do not register with bus"). If GART is re-enabled than
   all devices in the system are getting assigned to the GART as it is a global
   systems IOMMU provider. This is wrong simply because GART doesn't handle all
   those devices. This series makes GART to accept only devices that are
   explicitly assigned to GART in device tree using 'iommu' phandle.

3. This series makes a generic clean up of the driver, like removing dead code.

4. This series introduces and utilizes iotlb_sync_map() callback that was
   previously suggested by Joerg Roedel in [1], optimizing mapping / unmapping
   performance.

   [1] https://www.spinics.net/lists/linux-tegra/msg32914.html


Changelog:

v2: Addressed review comments from Robin Murphy to v1 by moving devices
    iommu_fwspec check to gart_iommu_add_device().

    Dropped the "Provide single domain and group for all devices" patch from
    the series for now because after some more considering it became not
    exactly apparent whether that is what we need, that was also suggested
    by Robin Murphy in the review comment. Maybe something like a runtime
    IOMMU usage for devices would be a better solution, allowing to implement
    transparent context switching of virtual IOMMU domains.

    Some very minor code cleanups, reworded commit messages.

Dmitry Osipenko (8):
  memory: tegra: Provide facility for integration with the GART driver
  iommu/tegra: gart: Provide access to Memory Controller driver
  iommu/tegra: gart: Clean up drivers module code
  iommu/tegra: gart: Remove pr_fmt and clean up includes
  iommu/tegra: gart: Clean up driver probe errors handling
  iommu/tegra: gart: Ignore devices without IOMMU phandle in DT
  iommu: Introduce iotlb_sync_map callback
  iommu/tegra: gart: Optimize mapping / unmapping performance

 drivers/iommu/iommu.c      |  8 ++-
 drivers/iommu/tegra-gart.c | 99 +++++++++++++++++++++++---------------
 drivers/memory/tegra/mc.c  | 26 ++++++++--
 include/linux/iommu.h      |  1 +
 include/soc/tegra/mc.h     | 13 +++++
 5 files changed, 103 insertions(+), 44 deletions(-)

-- 
2.18.0

[PATCH v2 1/8] memory: tegra: Provide facility for integration with the GART driver

[Dmitry Osipenko]

In order to report clients name and access direction on GART's page fault,
MC driver need to access GART registers. Add facility that provides access
to the GART.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/memory/tegra/mc.c | 26 +++++++++++++++++++++++---
 include/soc/tegra/mc.h    | 13 +++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index e56862495f36..4940d72b5263 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -72,6 +72,8 @@ static const struct of_device_id tegra_mc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
 
+static struct tegra_mc_gart_handle *gart_handle;
+
 static int terga_mc_block_dma_common(struct tegra_mc *mc,
 				     const struct tegra_mc_reset *rst)
 {
@@ -543,6 +545,11 @@ static irqreturn_t tegra_mc_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+void tegra_mc_register_gart(struct tegra_mc_gart_handle *handle)
+{
+	WRITE_ONCE(gart_handle, handle);
+}
+
 static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
 {
 	struct tegra_mc *mc = data;
@@ -565,6 +572,7 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
 		switch (BIT(bit)) {
 		case MC_INT_DECERR_EMEM:
 			reg = MC_DECERR_EMEM_OTHERS_STATUS;
+			addr = mc_readl(mc, reg + sizeof(u32));
 			value = mc_readl(mc, reg);
 
 			id = value & mc->soc->client_id_mask;
@@ -575,11 +583,24 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
 			break;
 
 		case MC_INT_INVALID_GART_PAGE:
-			dev_err_ratelimited(mc->dev, "%s\n", error);
-			continue;
+			if (gart_handle == NULL) {
+				dev_err_ratelimited(mc->dev, "%s\n", error);
+				continue;
+			}
+
+			addr = gart_handle->error_addr(gart_handle);
+			value = gart_handle->error_req(gart_handle);
+
+			id = (value >> 1) & mc->soc->client_id_mask;
+			desc = error_names[2];
+
+			if (value & BIT(0))
+				direction = "write";
+			break;
 
 		case MC_INT_SECURITY_VIOLATION:
 			reg = MC_SECURITY_VIOLATION_STATUS;
+			addr = mc_readl(mc, reg + sizeof(u32));
 			value = mc_readl(mc, reg);
 
 			id = value & mc->soc->client_id_mask;
@@ -596,7 +617,6 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
 		}
 
 		client = mc->soc->clients[id].name;
-		addr = mc_readl(mc, reg + sizeof(u32));
 
 		dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s)\n",
 				    client, secure, direction, &addr, error,
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index b43f37fea096..5bf72eb4dd51 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -162,4 +162,17 @@ struct tegra_mc {
 void tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
 unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
 
+struct tegra_mc_gart_handle {
+	u32 (*error_addr)(struct tegra_mc_gart_handle *handle);
+	u32 (*error_req)(struct tegra_mc_gart_handle *handle);
+};
+
+#ifdef CONFIG_TEGRA_MC
+void tegra_mc_register_gart(struct tegra_mc_gart_handle *handle);
+#else
+static inline void tegra_mc_register_gart(struct tegra_mc_gart_handle *handle)
+{
+}
+#endif
+
 #endif /* __SOC_TEGRA_MC_H__ */
-- 
2.18.0

Re: [PATCH v2 1/8] memory: tegra: Provide facility for integration with the GART driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Sat, Aug 04, 2018 at 05:29:56PM +0300, Dmitry Osipenko wrote:
> In order to report clients name and access direction on GART's page fault,
> MC driver need to access GART registers. Add facility that provides access
> to the GART.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/memory/tegra/mc.c | 26 +++++++++++++++++++++++---
>  include/soc/tegra/mc.h    | 13 +++++++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index e56862495f36..4940d72b5263 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -72,6 +72,8 @@ static const struct of_device_id tegra_mc_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
>  
> +static struct tegra_mc_gart_handle *gart_handle;
> +

Why the global variable? Can't this be part of struct tegra_mc? We
already do a very similar thing with the Tegra SMMU integration, why
invent something completely different? Can't we stick to a similar
mechanism?

Given that struct tegra_smmu is opaque at the memory controller level,
we could even go and store the GART related data in the same pointer.

How about the registration code goes into a struct tegra_gart_probe()
function that is called from tegra_mc_probe() right after the SMMU
block:

	if (IS_ENABLED(CONFIG_TEGRA_IOMMU_SMMU)) {
		mc->smmu = tegra_smmu_probe(...);
		...
	}

	if (IS_ENABLED(CONFIG_TEGRA_IOMMU_GART)) {
		mc->smmu = tegra_gart_probe(...);
		...
	}

?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory Controller driver

[Dmitry Osipenko]

GART contain registers needed by the Memory Controller driver, provide
access to the MC driver by utilizing its GART-integration facility.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/iommu/tegra-gart.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index a004f6da35f2..f8b653e25914 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -31,6 +31,8 @@
 #include <linux/iommu.h>
 #include <linux/of.h>
 
+#include <soc/tegra/mc.h>
+
 #include <asm/cacheflush.h>
 
 /* bitmap of the page sizes currently supported */
@@ -41,6 +43,8 @@
 #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
 #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
 #define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
+#define GART_ERROR_REQ		(0x30 - GART_REG_BASE)
+#define GART_ERROR_ADDR		(0x34 - GART_REG_BASE)
 
 #define GART_PAGE_SHIFT		12
 #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
@@ -63,6 +67,8 @@ struct gart_device {
 	struct device		*dev;
 
 	struct iommu_device	iommu;		/* IOMMU Core handle */
+
+	struct tegra_mc_gart_handle mc_gart_handle;
 };
 
 struct gart_domain {
@@ -408,6 +414,20 @@ static int tegra_gart_resume(struct device *dev)
 	return 0;
 }
 
+static u32 tegra_gart_error_addr(struct tegra_mc_gart_handle *handle)
+{
+	struct gart_device *gart = container_of(handle, struct gart_device,
+						mc_gart_handle);
+	return readl_relaxed(gart->regs + GART_ERROR_ADDR);
+}
+
+static u32 tegra_gart_error_req(struct tegra_mc_gart_handle *handle)
+{
+	struct gart_device *gart = container_of(handle, struct gart_device,
+						mc_gart_handle);
+	return readl_relaxed(gart->regs + GART_ERROR_REQ);
+}
+
 static int tegra_gart_probe(struct platform_device *pdev)
 {
 	struct gart_device *gart;
@@ -464,6 +484,8 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	gart->regs = gart_regs;
 	gart->iovmm_base = (dma_addr_t)res_remap->start;
 	gart->page_count = (resource_size(res_remap) >> GART_PAGE_SHIFT);
+	gart->mc_gart_handle.error_addr = tegra_gart_error_addr;
+	gart->mc_gart_handle.error_req = tegra_gart_error_req;
 
 	gart->savedata = vmalloc(array_size(sizeof(u32), gart->page_count));
 	if (!gart->savedata) {
@@ -475,6 +497,7 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	do_gart_setup(gart, NULL);
 
 	gart_handle = gart;
+	tegra_mc_register_gart(&gart->mc_gart_handle);
 
 	return 0;
 }
-- 
2.18.0

Re: [PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory Controller driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3484 bytes --]

On Sat, Aug 04, 2018 at 05:29:57PM +0300, Dmitry Osipenko wrote:
> GART contain registers needed by the Memory Controller driver, provide
> access to the MC driver by utilizing its GART-integration facility.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/iommu/tegra-gart.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index a004f6da35f2..f8b653e25914 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c
> @@ -31,6 +31,8 @@
>  #include <linux/iommu.h>
>  #include <linux/of.h>
>  
> +#include <soc/tegra/mc.h>
> +
>  #include <asm/cacheflush.h>
>  
>  /* bitmap of the page sizes currently supported */
> @@ -41,6 +43,8 @@
>  #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
>  #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
>  #define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
> +#define GART_ERROR_REQ		(0x30 - GART_REG_BASE)
> +#define GART_ERROR_ADDR		(0x34 - GART_REG_BASE)
>  
>  #define GART_PAGE_SHIFT		12
>  #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
> @@ -63,6 +67,8 @@ struct gart_device {
>  	struct device		*dev;
>  
>  	struct iommu_device	iommu;		/* IOMMU Core handle */
> +
> +	struct tegra_mc_gart_handle mc_gart_handle;
>  };
>  
>  struct gart_domain {
> @@ -408,6 +414,20 @@ static int tegra_gart_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static u32 tegra_gart_error_addr(struct tegra_mc_gart_handle *handle)
> +{
> +	struct gart_device *gart = container_of(handle, struct gart_device,
> +						mc_gart_handle);
> +	return readl_relaxed(gart->regs + GART_ERROR_ADDR);
> +}
> +
> +static u32 tegra_gart_error_req(struct tegra_mc_gart_handle *handle)
> +{
> +	struct gart_device *gart = container_of(handle, struct gart_device,
> +						mc_gart_handle);
> +	return readl_relaxed(gart->regs + GART_ERROR_REQ);
> +}
> +
>  static int tegra_gart_probe(struct platform_device *pdev)
>  {
>  	struct gart_device *gart;
> @@ -464,6 +484,8 @@ static int tegra_gart_probe(struct platform_device *pdev)
>  	gart->regs = gart_regs;
>  	gart->iovmm_base = (dma_addr_t)res_remap->start;
>  	gart->page_count = (resource_size(res_remap) >> GART_PAGE_SHIFT);
> +	gart->mc_gart_handle.error_addr = tegra_gart_error_addr;
> +	gart->mc_gart_handle.error_req = tegra_gart_error_req;
>  
>  	gart->savedata = vmalloc(array_size(sizeof(u32), gart->page_count));
>  	if (!gart->savedata) {
> @@ -475,6 +497,7 @@ static int tegra_gart_probe(struct platform_device *pdev)
>  	do_gart_setup(gart, NULL);
>  
>  	gart_handle = gart;
> +	tegra_mc_register_gart(&gart->mc_gart_handle);
>  
>  	return 0;
>  }

I see now why you've did it this way. We have separate devices unlike
with SMMU where it is properly modeled as part of the memory controller.
I think we should consider breaking ABI at this point and properly merge
both the memory controller and GART nodes. There's really no reason why
they should be separate and we're jumping through multiple hoops to do
what we need to do just because a few years back we made a mistake.

I know we're technically not supposed to break the DT ABI, but I think
in this particular case we can make a good case for it. The current DT
bindings are plainly broken, and obviously so. Also, we don't currently
use the GART upstream for anything, so we can't break any existing
systems either.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory Controller driver

[Dmitry Osipenko]

On Thursday, 9 August 2018 14:17:46 MSK Thierry Reding wrote:
> On Sat, Aug 04, 2018 at 05:29:57PM +0300, Dmitry Osipenko wrote:
> > GART contain registers needed by the Memory Controller driver, provide
> > access to the MC driver by utilizing its GART-integration facility.
> > 
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> > 
> >  drivers/iommu/tegra-gart.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> > index a004f6da35f2..f8b653e25914 100644
> > --- a/drivers/iommu/tegra-gart.c
> > +++ b/drivers/iommu/tegra-gart.c
> > @@ -31,6 +31,8 @@
> > 
> >  #include <linux/iommu.h>
> >  #include <linux/of.h>
> > 
> > +#include <soc/tegra/mc.h>
> > +
> > 
> >  #include <asm/cacheflush.h>
> >  
> >  /* bitmap of the page sizes currently supported */
> > 
> > @@ -41,6 +43,8 @@
> > 
> >  #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
> >  #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
> >  #define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
> > 
> > +#define GART_ERROR_REQ		(0x30 - GART_REG_BASE)
> > +#define GART_ERROR_ADDR		(0x34 - GART_REG_BASE)
> > 
> >  #define GART_PAGE_SHIFT		12
> >  #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
> > 
> > @@ -63,6 +67,8 @@ struct gart_device {
> > 
> >  	struct device		*dev;
> >  	
> >  	struct iommu_device	iommu;		/* IOMMU Core handle */
> > 
> > +
> > +	struct tegra_mc_gart_handle mc_gart_handle;
> > 
> >  };
> >  
> >  struct gart_domain {
> > 
> > @@ -408,6 +414,20 @@ static int tegra_gart_resume(struct device *dev)
> > 
> >  	return 0;
> >  
> >  }
> > 
> > +static u32 tegra_gart_error_addr(struct tegra_mc_gart_handle *handle)
> > +{
> > +	struct gart_device *gart = container_of(handle, struct gart_device,
> > +						mc_gart_handle);
> > +	return readl_relaxed(gart->regs + GART_ERROR_ADDR);
> > +}
> > +
> > +static u32 tegra_gart_error_req(struct tegra_mc_gart_handle *handle)
> > +{
> > +	struct gart_device *gart = container_of(handle, struct gart_device,
> > +						mc_gart_handle);
> > +	return readl_relaxed(gart->regs + GART_ERROR_REQ);
> > +}
> > +
> > 
> >  static int tegra_gart_probe(struct platform_device *pdev)
> >  {
> >  
> >  	struct gart_device *gart;
> > 
> > @@ -464,6 +484,8 @@ static int tegra_gart_probe(struct platform_device
> > *pdev)> 
> >  	gart->regs = gart_regs;
> >  	gart->iovmm_base = (dma_addr_t)res_remap->start;
> >  	gart->page_count = (resource_size(res_remap) >> GART_PAGE_SHIFT);
> > 
> > +	gart->mc_gart_handle.error_addr = tegra_gart_error_addr;
> > +	gart->mc_gart_handle.error_req = tegra_gart_error_req;
> > 
> >  	gart->savedata = vmalloc(array_size(sizeof(u32), gart->page_count));
> >  	if (!gart->savedata) {
> > 
> > @@ -475,6 +497,7 @@ static int tegra_gart_probe(struct platform_device
> > *pdev)> 
> >  	do_gart_setup(gart, NULL);
> >  	
> >  	gart_handle = gart;
> > 
> > +	tegra_mc_register_gart(&gart->mc_gart_handle);
> > 
> >  	return 0;
> >  
> >  }
> 
> I see now why you've did it this way. We have separate devices unlike
> with SMMU where it is properly modeled as part of the memory controller.
> I think we should consider breaking ABI at this point and properly merge
> both the memory controller and GART nodes. There's really no reason why
> they should be separate and we're jumping through multiple hoops to do
> what we need to do just because a few years back we made a mistake.
> 
> I know we're technically not supposed to break the DT ABI, but I think
> in this particular case we can make a good case for it. The current DT
> bindings are plainly broken, and obviously so. Also, we don't currently
> use the GART upstream for anything, so we can't break any existing
> systems either.

IIUC, that will require to break the stable DT ABI of the tegra20-mc, which is 
working fine and does its job. I'm personally not seeing the slight lameness 
of the current DT as a good excuse to break the ABI. Let's then break DT ABI 
on all Tegra's and convert them all to genpd and other goodies like assigned 
clock parents and clock rate.

Re: [PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory Controller driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5394 bytes --]

On Thu, Aug 09, 2018 at 02:39:03PM +0300, Dmitry Osipenko wrote:
> On Thursday, 9 August 2018 14:17:46 MSK Thierry Reding wrote:
> > On Sat, Aug 04, 2018 at 05:29:57PM +0300, Dmitry Osipenko wrote:
> > > GART contain registers needed by the Memory Controller driver, provide
> > > access to the MC driver by utilizing its GART-integration facility.
> > > 
> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > ---
> > > 
> > >  drivers/iommu/tegra-gart.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> > > index a004f6da35f2..f8b653e25914 100644
> > > --- a/drivers/iommu/tegra-gart.c
> > > +++ b/drivers/iommu/tegra-gart.c
> > > @@ -31,6 +31,8 @@
> > > 
> > >  #include <linux/iommu.h>
> > >  #include <linux/of.h>
> > > 
> > > +#include <soc/tegra/mc.h>
> > > +
> > > 
> > >  #include <asm/cacheflush.h>
> > >  
> > >  /* bitmap of the page sizes currently supported */
> > > 
> > > @@ -41,6 +43,8 @@
> > > 
> > >  #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
> > >  #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
> > >  #define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
> > > 
> > > +#define GART_ERROR_REQ		(0x30 - GART_REG_BASE)
> > > +#define GART_ERROR_ADDR		(0x34 - GART_REG_BASE)
> > > 
> > >  #define GART_PAGE_SHIFT		12
> > >  #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
> > > 
> > > @@ -63,6 +67,8 @@ struct gart_device {
> > > 
> > >  	struct device		*dev;
> > >  	
> > >  	struct iommu_device	iommu;		/* IOMMU Core handle */
> > > 
> > > +
> > > +	struct tegra_mc_gart_handle mc_gart_handle;
> > > 
> > >  };
> > >  
> > >  struct gart_domain {
> > > 
> > > @@ -408,6 +414,20 @@ static int tegra_gart_resume(struct device *dev)
> > > 
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > +static u32 tegra_gart_error_addr(struct tegra_mc_gart_handle *handle)
> > > +{
> > > +	struct gart_device *gart = container_of(handle, struct gart_device,
> > > +						mc_gart_handle);
> > > +	return readl_relaxed(gart->regs + GART_ERROR_ADDR);
> > > +}
> > > +
> > > +static u32 tegra_gart_error_req(struct tegra_mc_gart_handle *handle)
> > > +{
> > > +	struct gart_device *gart = container_of(handle, struct gart_device,
> > > +						mc_gart_handle);
> > > +	return readl_relaxed(gart->regs + GART_ERROR_REQ);
> > > +}
> > > +
> > > 
> > >  static int tegra_gart_probe(struct platform_device *pdev)
> > >  {
> > >  
> > >  	struct gart_device *gart;
> > > 
> > > @@ -464,6 +484,8 @@ static int tegra_gart_probe(struct platform_device
> > > *pdev)> 
> > >  	gart->regs = gart_regs;
> > >  	gart->iovmm_base = (dma_addr_t)res_remap->start;
> > >  	gart->page_count = (resource_size(res_remap) >> GART_PAGE_SHIFT);
> > > 
> > > +	gart->mc_gart_handle.error_addr = tegra_gart_error_addr;
> > > +	gart->mc_gart_handle.error_req = tegra_gart_error_req;
> > > 
> > >  	gart->savedata = vmalloc(array_size(sizeof(u32), gart->page_count));
> > >  	if (!gart->savedata) {
> > > 
> > > @@ -475,6 +497,7 @@ static int tegra_gart_probe(struct platform_device
> > > *pdev)> 
> > >  	do_gart_setup(gart, NULL);
> > >  	
> > >  	gart_handle = gart;
> > > 
> > > +	tegra_mc_register_gart(&gart->mc_gart_handle);
> > > 
> > >  	return 0;
> > >  
> > >  }
> > 
> > I see now why you've did it this way. We have separate devices unlike
> > with SMMU where it is properly modeled as part of the memory controller.
> > I think we should consider breaking ABI at this point and properly merge
> > both the memory controller and GART nodes. There's really no reason why
> > they should be separate and we're jumping through multiple hoops to do
> > what we need to do just because a few years back we made a mistake.
> > 
> > I know we're technically not supposed to break the DT ABI, but I think
> > in this particular case we can make a good case for it. The current DT
> > bindings are plainly broken, and obviously so. Also, we don't currently
> > use the GART upstream for anything, so we can't break any existing
> > systems either.
> 
> IIUC, that will require to break the stable DT ABI of the tegra20-mc, which is 
> working fine and does its job. I'm personally not seeing the slight lameness 
> of the current DT as a good excuse to break the ABI. Let's then break DT ABI 
> on all Tegra's and convert them all to genpd and other goodies like assigned 
> clock parents and clock rate.

genpd and assigned clocks are complementary, they can be switched to
without breaking ABI.

And that's also different from the memory controller on Tegra20 where we
just made the mistake of describing what is effectively one device as
two separate devices. From what I can tell, the only reason this was
done was because it mapped better to the Linux driver model where there
is a framework to represent an IOMMU and a misunderstanding of how to
work with the driver model and device tree.

As such, I would describe it as more of a bug in the DT that should be
fixed rather than breaking the ABI.

And, like I said, we are in the somewhat fortunate situation that we
don't actively use the GART, at least in upstream, yet. So even if we
break ABI, nobody will notice anyway. Those are about as good pre-
conditions as you're going to get for fixing ABI.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory Controller driver

[Dmitry Osipenko]

On Thursday, 9 August 2018 16:59:24 MSK Thierry Reding wrote:
> On Thu, Aug 09, 2018 at 02:39:03PM +0300, Dmitry Osipenko wrote:
> > On Thursday, 9 August 2018 14:17:46 MSK Thierry Reding wrote:
> > > On Sat, Aug 04, 2018 at 05:29:57PM +0300, Dmitry Osipenko wrote:
> > > > GART contain registers needed by the Memory Controller driver, provide
> > > > access to the MC driver by utilizing its GART-integration facility.
> > > > 
> > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/iommu/tegra-gart.c | 23 +++++++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > > 
> > > > diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> > > > index a004f6da35f2..f8b653e25914 100644
> > > > --- a/drivers/iommu/tegra-gart.c
> > > > +++ b/drivers/iommu/tegra-gart.c
> > > > @@ -31,6 +31,8 @@
> > > > 
> > > >  #include <linux/iommu.h>
> > > >  #include <linux/of.h>
> > > > 
> > > > +#include <soc/tegra/mc.h>
> > > > +
> > > > 
> > > >  #include <asm/cacheflush.h>
> > > >  
> > > >  /* bitmap of the page sizes currently supported */
> > > > 
> > > > @@ -41,6 +43,8 @@
> > > > 
> > > >  #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
> > > >  #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
> > > >  #define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
> > > > 
> > > > +#define GART_ERROR_REQ		(0x30 - GART_REG_BASE)
> > > > +#define GART_ERROR_ADDR		(0x34 - GART_REG_BASE)
> > > > 
> > > >  #define GART_PAGE_SHIFT		12
> > > >  #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
> > > > 
> > > > @@ -63,6 +67,8 @@ struct gart_device {
> > > > 
> > > >  	struct device		*dev;
> > > >  	
> > > >  	struct iommu_device	iommu;		/* IOMMU Core handle */
> > > > 
> > > > +
> > > > +	struct tegra_mc_gart_handle mc_gart_handle;
> > > > 
> > > >  };
> > > >  
> > > >  struct gart_domain {
> > > > 
> > > > @@ -408,6 +414,20 @@ static int tegra_gart_resume(struct device *dev)
> > > > 
> > > >  	return 0;
> > > >  
> > > >  }
> > > > 
> > > > +static u32 tegra_gart_error_addr(struct tegra_mc_gart_handle *handle)
> > > > +{
> > > > +	struct gart_device *gart = container_of(handle, struct 
gart_device,
> > > > +						mc_gart_handle);
> > > > +	return readl_relaxed(gart->regs + GART_ERROR_ADDR);
> > > > +}
> > > > +
> > > > +static u32 tegra_gart_error_req(struct tegra_mc_gart_handle *handle)
> > > > +{
> > > > +	struct gart_device *gart = container_of(handle, struct 
gart_device,
> > > > +						mc_gart_handle);
> > > > +	return readl_relaxed(gart->regs + GART_ERROR_REQ);
> > > > +}
> > > > +
> > > > 
> > > >  static int tegra_gart_probe(struct platform_device *pdev)
> > > >  {
> > > >  
> > > >  	struct gart_device *gart;
> > > > 
> > > > @@ -464,6 +484,8 @@ static int tegra_gart_probe(struct platform_device
> > > > *pdev)>
> > > > 
> > > >  	gart->regs = gart_regs;
> > > >  	gart->iovmm_base = (dma_addr_t)res_remap->start;
> > > >  	gart->page_count = (resource_size(res_remap) >> GART_PAGE_SHIFT);
> > > > 
> > > > +	gart->mc_gart_handle.error_addr = tegra_gart_error_addr;
> > > > +	gart->mc_gart_handle.error_req = tegra_gart_error_req;
> > > > 
> > > >  	gart->savedata = vmalloc(array_size(sizeof(u32), gart->page_count));
> > > >  	if (!gart->savedata) {
> > > > 
> > > > @@ -475,6 +497,7 @@ static int tegra_gart_probe(struct platform_device
> > > > *pdev)>
> > > > 
> > > >  	do_gart_setup(gart, NULL);
> > > >  	
> > > >  	gart_handle = gart;
> > > > 
> > > > +	tegra_mc_register_gart(&gart->mc_gart_handle);
> > > > 
> > > >  	return 0;
> > > >  
> > > >  }
> > > 
> > > I see now why you've did it this way. We have separate devices unlike
> > > with SMMU where it is properly modeled as part of the memory controller.
> > > I think we should consider breaking ABI at this point and properly merge
> > > both the memory controller and GART nodes. There's really no reason why
> > > they should be separate and we're jumping through multiple hoops to do
> > > what we need to do just because a few years back we made a mistake.
> > > 
> > > I know we're technically not supposed to break the DT ABI, but I think
> > > in this particular case we can make a good case for it. The current DT
> > > bindings are plainly broken, and obviously so. Also, we don't currently
> > > use the GART upstream for anything, so we can't break any existing
> > > systems either.
> > 
> > IIUC, that will require to break the stable DT ABI of the tegra20-mc,
> > which is working fine and does its job. I'm personally not seeing the
> > slight lameness of the current DT as a good excuse to break the ABI.
> > Let's then break DT ABI on all Tegra's and convert them all to genpd and
> > other goodies like assigned clock parents and clock rate.
> 
> genpd and assigned clocks are complementary, they can be switched to
> without breaking ABI.
> 
> And that's also different from the memory controller on Tegra20 where we
> just made the mistake of describing what is effectively one device as
> two separate devices. From what I can tell, the only reason this was
> done was because it mapped better to the Linux driver model where there
> is a framework to represent an IOMMU and a misunderstanding of how to
> work with the driver model and device tree.
> 
> As such, I would describe it as more of a bug in the DT that should be
> fixed rather than breaking the ABI.
> 
> And, like I said, we are in the somewhat fortunate situation that we
> don't actively use the GART, at least in upstream, yet. So even if we
> break ABI, nobody will notice anyway. Those are about as good pre-
> conditions as you're going to get for fixing ABI.

Please tell exactly what you're suggesting.

Current DT:

	mc: memory-controller@7000f000 {
		compatible = "nvidia,tegra20-mc";
		reg = <0x7000f000 0x024
		       0x7000f03c 0x3c4>;
		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
		#reset-cells = <1>;
	};

	gart: iommu@7000f024 {
		compatible = "nvidia,tegra20-gart";
		reg = <0x7000f024 0x00000018	/* controller registers */
		       0x58000000 0x02000000>;	/* GART aperture */
		#iommu-cells = <0>;
	};



Variant 1 (break MC ABI):

	mc: memory-controller@7000f000 {
		compatible = "nvidia,tegra20-mc";
		reg = <0x7000f000 0x400	/* controller registers */
		       0x58000000 0x02000000>;	/* GART aperture */
		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
		#reset-cells = <1>;
		#iommu-cells = <0>;
	};



Variant 2 (don't break MC ABI, extra code churning which seems will be worse 
than what it is now):

	mc: memory-controller@7000f000 {
		compatible = "nvidia,tegra20-mc";
		reg = <0x7000f000 0x024
		       0x7000f03c 0x3c4
		       0x7000f024 0x00000018	/* GART registers */
		       0x58000000 0x02000000>;	/* GART aperture */;
		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
		#reset-cells = <1>;
		#iommu-cells = <0>;
	};

Re: [PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory Controller driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7008 bytes --]

On Thu, Aug 09, 2018 at 05:22:59PM +0300, Dmitry Osipenko wrote:
> On Thursday, 9 August 2018 16:59:24 MSK Thierry Reding wrote:
> > On Thu, Aug 09, 2018 at 02:39:03PM +0300, Dmitry Osipenko wrote:
> > > On Thursday, 9 August 2018 14:17:46 MSK Thierry Reding wrote:
> > > > On Sat, Aug 04, 2018 at 05:29:57PM +0300, Dmitry Osipenko wrote:
> > > > > GART contain registers needed by the Memory Controller driver, provide
> > > > > access to the MC driver by utilizing its GART-integration facility.
> > > > > 
> > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/iommu/tegra-gart.c | 23 +++++++++++++++++++++++
> > > > >  1 file changed, 23 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> > > > > index a004f6da35f2..f8b653e25914 100644
> > > > > --- a/drivers/iommu/tegra-gart.c
> > > > > +++ b/drivers/iommu/tegra-gart.c
> > > > > @@ -31,6 +31,8 @@
> > > > > 
> > > > >  #include <linux/iommu.h>
> > > > >  #include <linux/of.h>
> > > > > 
> > > > > +#include <soc/tegra/mc.h>
> > > > > +
> > > > > 
> > > > >  #include <asm/cacheflush.h>
> > > > >  
> > > > >  /* bitmap of the page sizes currently supported */
> > > > > 
> > > > > @@ -41,6 +43,8 @@
> > > > > 
> > > > >  #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
> > > > >  #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
> > > > >  #define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
> > > > > 
> > > > > +#define GART_ERROR_REQ		(0x30 - GART_REG_BASE)
> > > > > +#define GART_ERROR_ADDR		(0x34 - GART_REG_BASE)
> > > > > 
> > > > >  #define GART_PAGE_SHIFT		12
> > > > >  #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
> > > > > 
> > > > > @@ -63,6 +67,8 @@ struct gart_device {
> > > > > 
> > > > >  	struct device		*dev;
> > > > >  	
> > > > >  	struct iommu_device	iommu;		/* IOMMU Core handle */
> > > > > 
> > > > > +
> > > > > +	struct tegra_mc_gart_handle mc_gart_handle;
> > > > > 
> > > > >  };
> > > > >  
> > > > >  struct gart_domain {
> > > > > 
> > > > > @@ -408,6 +414,20 @@ static int tegra_gart_resume(struct device *dev)
> > > > > 
> > > > >  	return 0;
> > > > >  
> > > > >  }
> > > > > 
> > > > > +static u32 tegra_gart_error_addr(struct tegra_mc_gart_handle *handle)
> > > > > +{
> > > > > +	struct gart_device *gart = container_of(handle, struct 
> gart_device,
> > > > > +						mc_gart_handle);
> > > > > +	return readl_relaxed(gart->regs + GART_ERROR_ADDR);
> > > > > +}
> > > > > +
> > > > > +static u32 tegra_gart_error_req(struct tegra_mc_gart_handle *handle)
> > > > > +{
> > > > > +	struct gart_device *gart = container_of(handle, struct 
> gart_device,
> > > > > +						mc_gart_handle);
> > > > > +	return readl_relaxed(gart->regs + GART_ERROR_REQ);
> > > > > +}
> > > > > +
> > > > > 
> > > > >  static int tegra_gart_probe(struct platform_device *pdev)
> > > > >  {
> > > > >  
> > > > >  	struct gart_device *gart;
> > > > > 
> > > > > @@ -464,6 +484,8 @@ static int tegra_gart_probe(struct platform_device
> > > > > *pdev)>
> > > > > 
> > > > >  	gart->regs = gart_regs;
> > > > >  	gart->iovmm_base = (dma_addr_t)res_remap->start;
> > > > >  	gart->page_count = (resource_size(res_remap) >> GART_PAGE_SHIFT);
> > > > > 
> > > > > +	gart->mc_gart_handle.error_addr = tegra_gart_error_addr;
> > > > > +	gart->mc_gart_handle.error_req = tegra_gart_error_req;
> > > > > 
> > > > >  	gart->savedata = vmalloc(array_size(sizeof(u32), gart->page_count));
> > > > >  	if (!gart->savedata) {
> > > > > 
> > > > > @@ -475,6 +497,7 @@ static int tegra_gart_probe(struct platform_device
> > > > > *pdev)>
> > > > > 
> > > > >  	do_gart_setup(gart, NULL);
> > > > >  	
> > > > >  	gart_handle = gart;
> > > > > 
> > > > > +	tegra_mc_register_gart(&gart->mc_gart_handle);
> > > > > 
> > > > >  	return 0;
> > > > >  
> > > > >  }
> > > > 
> > > > I see now why you've did it this way. We have separate devices unlike
> > > > with SMMU where it is properly modeled as part of the memory controller.
> > > > I think we should consider breaking ABI at this point and properly merge
> > > > both the memory controller and GART nodes. There's really no reason why
> > > > they should be separate and we're jumping through multiple hoops to do
> > > > what we need to do just because a few years back we made a mistake.
> > > > 
> > > > I know we're technically not supposed to break the DT ABI, but I think
> > > > in this particular case we can make a good case for it. The current DT
> > > > bindings are plainly broken, and obviously so. Also, we don't currently
> > > > use the GART upstream for anything, so we can't break any existing
> > > > systems either.
> > > 
> > > IIUC, that will require to break the stable DT ABI of the tegra20-mc,
> > > which is working fine and does its job. I'm personally not seeing the
> > > slight lameness of the current DT as a good excuse to break the ABI.
> > > Let's then break DT ABI on all Tegra's and convert them all to genpd and
> > > other goodies like assigned clock parents and clock rate.
> > 
> > genpd and assigned clocks are complementary, they can be switched to
> > without breaking ABI.
> > 
> > And that's also different from the memory controller on Tegra20 where we
> > just made the mistake of describing what is effectively one device as
> > two separate devices. From what I can tell, the only reason this was
> > done was because it mapped better to the Linux driver model where there
> > is a framework to represent an IOMMU and a misunderstanding of how to
> > work with the driver model and device tree.
> > 
> > As such, I would describe it as more of a bug in the DT that should be
> > fixed rather than breaking the ABI.
> > 
> > And, like I said, we are in the somewhat fortunate situation that we
> > don't actively use the GART, at least in upstream, yet. So even if we
> > break ABI, nobody will notice anyway. Those are about as good pre-
> > conditions as you're going to get for fixing ABI.
> 
> Please tell exactly what you're suggesting.
> 
> Current DT:
> 
> 	mc: memory-controller@7000f000 {
> 		compatible = "nvidia,tegra20-mc";
> 		reg = <0x7000f000 0x024
> 		       0x7000f03c 0x3c4>;
> 		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> 		#reset-cells = <1>;
> 	};
> 
> 	gart: iommu@7000f024 {
> 		compatible = "nvidia,tegra20-gart";
> 		reg = <0x7000f024 0x00000018	/* controller registers */
> 		       0x58000000 0x02000000>;	/* GART aperture */
> 		#iommu-cells = <0>;
> 	};
> 
> 
> 
> Variant 1 (break MC ABI):
> 
> 	mc: memory-controller@7000f000 {
> 		compatible = "nvidia,tegra20-mc";
> 		reg = <0x7000f000 0x400	/* controller registers */
> 		       0x58000000 0x02000000>;	/* GART aperture */
> 		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> 		#reset-cells = <1>;
> 		#iommu-cells = <0>;
> 	};

This is the variant that I would prefer.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/8] iommu/tegra: gart: Provide access to Memory Controller driver

[Dmitry Osipenko]

On Thursday, 9 August 2018 17:52:06 MSK Thierry Reding wrote:
> On Thu, Aug 09, 2018 at 05:22:59PM +0300, Dmitry Osipenko wrote:
> > On Thursday, 9 August 2018 16:59:24 MSK Thierry Reding wrote:
> > > On Thu, Aug 09, 2018 at 02:39:03PM +0300, Dmitry Osipenko wrote:
> > > > On Thursday, 9 August 2018 14:17:46 MSK Thierry Reding wrote:
> > > > > On Sat, Aug 04, 2018 at 05:29:57PM +0300, Dmitry Osipenko wrote:
> > > > > > GART contain registers needed by the Memory Controller driver,
> > > > > > provide
> > > > > > access to the MC driver by utilizing its GART-integration
> > > > > > facility.
> > > > > > 
> > > > > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/iommu/tegra-gart.c | 23 +++++++++++++++++++++++
> > > > > >  1 file changed, 23 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/iommu/tegra-gart.c
> > > > > > b/drivers/iommu/tegra-gart.c
> > > > > > index a004f6da35f2..f8b653e25914 100644
> > > > > > --- a/drivers/iommu/tegra-gart.c
> > > > > > +++ b/drivers/iommu/tegra-gart.c
> > > > > > @@ -31,6 +31,8 @@
> > > > > > 
> > > > > >  #include <linux/iommu.h>
> > > > > >  #include <linux/of.h>
> > > > > > 
> > > > > > +#include <soc/tegra/mc.h>
> > > > > > +
> > > > > > 
> > > > > >  #include <asm/cacheflush.h>
> > > > > >  
> > > > > >  /* bitmap of the page sizes currently supported */
> > > > > > 
> > > > > > @@ -41,6 +43,8 @@
> > > > > > 
> > > > > >  #define GART_ENTRY_ADDR		(0x28 - GART_REG_BASE)
> > > > > >  #define GART_ENTRY_DATA		(0x2c - GART_REG_BASE)
> > > > > >  #define GART_ENTRY_PHYS_ADDR_VALID	(1 << 31)
> > > > > > 
> > > > > > +#define GART_ERROR_REQ		(0x30 - GART_REG_BASE)
> > > > > > +#define GART_ERROR_ADDR		(0x34 - GART_REG_BASE)
> > > > > > 
> > > > > >  #define GART_PAGE_SHIFT		12
> > > > > >  #define GART_PAGE_SIZE		(1 << GART_PAGE_SHIFT)
> > > > > > 
> > > > > > @@ -63,6 +67,8 @@ struct gart_device {
> > > > > > 
> > > > > >  	struct device		*dev;
> > > > > >  	
> > > > > >  	struct iommu_device	iommu;		/* IOMMU Core handle */
> > > > > > 
> > > > > > +
> > > > > > +	struct tegra_mc_gart_handle mc_gart_handle;
> > > > > > 
> > > > > >  };
> > > > > >  
> > > > > >  struct gart_domain {
> > > > > > 
> > > > > > @@ -408,6 +414,20 @@ static int tegra_gart_resume(struct device
> > > > > > *dev)
> > > > > > 
> > > > > >  	return 0;
> > > > > >  
> > > > > >  }
> > > > > > 
> > > > > > +static u32 tegra_gart_error_addr(struct tegra_mc_gart_handle
> > > > > > *handle)
> > > > > > +{
> > > > > > +	struct gart_device *gart = container_of(handle, struct
> > 
> > gart_device,
> > 
> > > > > > +						mc_gart_handle);
> > > > > > +	return readl_relaxed(gart->regs + GART_ERROR_ADDR);
> > > > > > +}
> > > > > > +
> > > > > > +static u32 tegra_gart_error_req(struct tegra_mc_gart_handle
> > > > > > *handle)
> > > > > > +{
> > > > > > +	struct gart_device *gart = container_of(handle, struct
> > 
> > gart_device,
> > 
> > > > > > +						mc_gart_handle);
> > > > > > +	return readl_relaxed(gart->regs + GART_ERROR_REQ);
> > > > > > +}
> > > > > > +
> > > > > > 
> > > > > >  static int tegra_gart_probe(struct platform_device *pdev)
> > > > > >  {
> > > > > >  
> > > > > >  	struct gart_device *gart;
> > > > > > 
> > > > > > @@ -464,6 +484,8 @@ static int tegra_gart_probe(struct
> > > > > > platform_device
> > > > > > *pdev)>
> > > > > > 
> > > > > >  	gart->regs = gart_regs;
> > > > > >  	gart->iovmm_base = (dma_addr_t)res_remap->start;
> > > > > >  	gart->page_count = (resource_size(res_remap) >>
> > > > > >  	GART_PAGE_SHIFT);
> > > > > > 
> > > > > > +	gart->mc_gart_handle.error_addr = tegra_gart_error_addr;
> > > > > > +	gart->mc_gart_handle.error_req = tegra_gart_error_req;
> > > > > > 
> > > > > >  	gart->savedata = vmalloc(array_size(sizeof(u32),
> > > > > >  	gart->page_count));
> > > > > >  	if (!gart->savedata) {
> > > > > > 
> > > > > > @@ -475,6 +497,7 @@ static int tegra_gart_probe(struct
> > > > > > platform_device
> > > > > > *pdev)>
> > > > > > 
> > > > > >  	do_gart_setup(gart, NULL);
> > > > > >  	
> > > > > >  	gart_handle = gart;
> > > > > > 
> > > > > > +	tegra_mc_register_gart(&gart->mc_gart_handle);
> > > > > > 
> > > > > >  	return 0;
> > > > > >  
> > > > > >  }
> > > > > 
> > > > > I see now why you've did it this way. We have separate devices
> > > > > unlike
> > > > > with SMMU where it is properly modeled as part of the memory
> > > > > controller.
> > > > > I think we should consider breaking ABI at this point and properly
> > > > > merge
> > > > > both the memory controller and GART nodes. There's really no reason
> > > > > why
> > > > > they should be separate and we're jumping through multiple hoops to
> > > > > do
> > > > > what we need to do just because a few years back we made a mistake.
> > > > > 
> > > > > I know we're technically not supposed to break the DT ABI, but I
> > > > > think
> > > > > in this particular case we can make a good case for it. The current
> > > > > DT
> > > > > bindings are plainly broken, and obviously so. Also, we don't
> > > > > currently
> > > > > use the GART upstream for anything, so we can't break any existing
> > > > > systems either.
> > > > 
> > > > IIUC, that will require to break the stable DT ABI of the tegra20-mc,
> > > > which is working fine and does its job. I'm personally not seeing the
> > > > slight lameness of the current DT as a good excuse to break the ABI.
> > > > Let's then break DT ABI on all Tegra's and convert them all to genpd
> > > > and
> > > > other goodies like assigned clock parents and clock rate.
> > > 
> > > genpd and assigned clocks are complementary, they can be switched to
> > > without breaking ABI.
> > > 
> > > And that's also different from the memory controller on Tegra20 where we
> > > just made the mistake of describing what is effectively one device as
> > > two separate devices. From what I can tell, the only reason this was
> > > done was because it mapped better to the Linux driver model where there
> > > is a framework to represent an IOMMU and a misunderstanding of how to
> > > work with the driver model and device tree.
> > > 
> > > As such, I would describe it as more of a bug in the DT that should be
> > > fixed rather than breaking the ABI.
> > > 
> > > And, like I said, we are in the somewhat fortunate situation that we
> > > don't actively use the GART, at least in upstream, yet. So even if we
> > > break ABI, nobody will notice anyway. Those are about as good pre-
> > > conditions as you're going to get for fixing ABI.
> > 
> > Please tell exactly what you're suggesting.
> > 
> > Current DT:
> > 	mc: memory-controller@7000f000 {
> > 	
> > 		compatible = "nvidia,tegra20-mc";
> > 		reg = <0x7000f000 0x024
> > 		
> > 		       0x7000f03c 0x3c4>;
> > 		
> > 		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> > 		#reset-cells = <1>;
> > 	
> > 	};
> > 	
> > 	gart: iommu@7000f024 {
> > 	
> > 		compatible = "nvidia,tegra20-gart";
> > 		reg = <0x7000f024 0x00000018	/* controller registers */
> > 		
> > 		       0x58000000 0x02000000>;	/* GART aperture */
> > 		
> > 		#iommu-cells = <0>;
> > 	
> > 	};
> > 
> > Variant 1 (break MC ABI):
> > 	mc: memory-controller@7000f000 {
> > 	
> > 		compatible = "nvidia,tegra20-mc";
> > 		reg = <0x7000f000 0x400	/* controller registers */
> > 		
> > 		       0x58000000 0x02000000>;	/* GART aperture */
> > 		
> > 		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
> > 		#reset-cells = <1>;
> > 		#iommu-cells = <0>;
> > 	
> > 	};
> 
> This is the variant that I would prefer.

Okay, I'll prepare the patches.

[PATCH v2 3/8] iommu/tegra: gart: Clean up drivers module code

[Dmitry Osipenko]

GART driver is built-in, hence it can't be unloaded and the module removal
code is never used. This patch merely removes the dead code and makes
GART driver explicitly built-in by dropping the drivers bind/unbind sysfs
attributes. Lastly disallow to defer drivers probing, since it's a core
driver and actually nothing should cause the deferred probe.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/iommu/tegra-gart.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index f8b653e25914..141ff0c79ee4 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -502,20 +502,6 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int tegra_gart_remove(struct platform_device *pdev)
-{
-	struct gart_device *gart = platform_get_drvdata(pdev);
-
-	iommu_device_unregister(&gart->iommu);
-	iommu_device_sysfs_remove(&gart->iommu);
-
-	writel(0, gart->regs + GART_CONFIG);
-	if (gart->savedata)
-		vfree(gart->savedata);
-	gart_handle = NULL;
-	return 0;
-}
-
 static const struct dev_pm_ops tegra_gart_pm_ops = {
 	.suspend	= tegra_gart_suspend,
 	.resume		= tegra_gart_resume,
@@ -529,26 +515,21 @@ MODULE_DEVICE_TABLE(of, tegra_gart_of_match);
 
 static struct platform_driver tegra_gart_driver = {
 	.probe		= tegra_gart_probe,
-	.remove		= tegra_gart_remove,
 	.driver = {
 		.name	= "tegra-gart",
 		.pm	= &tegra_gart_pm_ops,
 		.of_match_table = tegra_gart_of_match,
+		.suppress_bind_attrs = true,
 	},
+	.prevent_deferred_probe = true,
 };
 
-static int tegra_gart_init(void)
+static int __init tegra_gart_init(void)
 {
 	return platform_driver_register(&tegra_gart_driver);
 }
-
-static void __exit tegra_gart_exit(void)
-{
-	platform_driver_unregister(&tegra_gart_driver);
-}
-
 subsys_initcall(tegra_gart_init);
-module_exit(tegra_gart_exit);
+
 module_param(gart_debug, bool, 0644);
 
 MODULE_PARM_DESC(gart_debug, "Enable GART debugging");
-- 
2.18.0

[PATCH v2 4/8] iommu/tegra: gart: Remove pr_fmt and clean up includes

[Dmitry Osipenko]

Remove unneeded headers inclusion and sort the headers in alphabet order.
Remove pr_fmt macro since there is no pr_*() in the code and it doesn't
affect dev_*() functions.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/iommu/tegra-gart.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 141ff0c79ee4..9a427cb35340 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -17,24 +17,17 @@
  * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
  */
 
-#define pr_fmt(fmt)	"%s(): " fmt, __func__
-
+#include <linux/io.h>
+#include <linux/iommu.h>
+#include <linux/list.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/spinlock.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/vmalloc.h>
-#include <linux/mm.h>
-#include <linux/list.h>
-#include <linux/device.h>
-#include <linux/io.h>
-#include <linux/iommu.h>
-#include <linux/of.h>
 
 #include <soc/tegra/mc.h>
 
-#include <asm/cacheflush.h>
-
 /* bitmap of the page sizes currently supported */
 #define GART_IOMMU_PGSIZES	(SZ_4K)
 
-- 
2.18.0

[PATCH v2 5/8] iommu/tegra: gart: Clean up driver probe errors handling

[Dmitry Osipenko]

Properly clean up allocated resources on the drivers probe failure and
remove unneeded checks.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/iommu/tegra-gart.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 9a427cb35340..def570bc88c4 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -429,9 +429,6 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
-	if (gart_handle)
-		return -EIO;
-
 	BUILD_BUG_ON(PAGE_SHIFT != GART_PAGE_SHIFT);
 
 	/* the GART memory aperture is required */
@@ -466,8 +463,7 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	ret = iommu_device_register(&gart->iommu);
 	if (ret) {
 		dev_err(dev, "Failed to register IOMMU\n");
-		iommu_device_sysfs_remove(&gart->iommu);
-		return ret;
+		goto remove_sysfs;
 	}
 
 	gart->dev = &pdev->dev;
@@ -483,7 +479,8 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	gart->savedata = vmalloc(array_size(sizeof(u32), gart->page_count));
 	if (!gart->savedata) {
 		dev_err(dev, "failed to allocate context save area\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto unregister_iommu;
 	}
 
 	platform_set_drvdata(pdev, gart);
@@ -493,6 +490,13 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	tegra_mc_register_gart(&gart->mc_gart_handle);
 
 	return 0;
+
+unregister_iommu:
+	iommu_device_unregister(&gart->iommu);
+remove_sysfs:
+	iommu_device_sysfs_remove(&gart->iommu);
+
+	return ret;
 }
 
 static const struct dev_pm_ops tegra_gart_pm_ops = {
-- 
2.18.0

[PATCH v2 6/8] iommu/tegra: gart: Ignore devices without IOMMU phandle in DT

[Dmitry Osipenko]

GART can't handle all devices, hence ignore devices that aren't related
to GART. IOMMU phandle must be explicitly assign to devices in the device
tree.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/iommu/tegra-gart.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index def570bc88c4..bcdb8973a0ad 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -348,8 +348,12 @@ static bool gart_iommu_capable(enum iommu_cap cap)
 
 static int gart_iommu_add_device(struct device *dev)
 {
-	struct iommu_group *group = iommu_group_get_for_dev(dev);
+	struct iommu_group *group;
 
+	if (!dev->iommu_fwspec)
+		return -ENODEV;
+
+	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group))
 		return PTR_ERR(group);
 
@@ -366,6 +370,12 @@ static void gart_iommu_remove_device(struct device *dev)
 	iommu_device_unlink(&gart_handle->iommu, dev);
 }
 
+static int gart_iommu_of_xlate(struct device *dev,
+			       struct of_phandle_args *args)
+{
+	return 0;
+}
+
 static const struct iommu_ops gart_iommu_ops = {
 	.capable	= gart_iommu_capable,
 	.domain_alloc	= gart_iommu_domain_alloc,
@@ -380,6 +390,7 @@ static const struct iommu_ops gart_iommu_ops = {
 	.unmap		= gart_iommu_unmap,
 	.iova_to_phys	= gart_iommu_iova_to_phys,
 	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
+	.of_xlate	= gart_iommu_of_xlate,
 };
 
 static int tegra_gart_suspend(struct device *dev)
@@ -459,6 +470,7 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	}
 
 	iommu_device_set_ops(&gart->iommu, &gart_iommu_ops);
+	iommu_device_set_fwnode(&gart->iommu, dev->fwnode);
 
 	ret = iommu_device_register(&gart->iommu);
 	if (ret) {
-- 
2.18.0

[PATCH v2 7/8] iommu: Introduce iotlb_sync_map callback

[Dmitry Osipenko]

Introduce iotlb_sync_map() callback that is invoked in the end of
iommu_map(). This new callback allows IOMMU drivers to avoid syncing
after mapping of each contiguous chunk and sync only when the whole
mapping is completed, optimizing performance of the mapping operation.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c | 8 ++++++--
 include/linux/iommu.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f3698006cb53..056282f23b10 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1545,13 +1545,14 @@ static size_t iommu_pgsize(struct iommu_domain *domain,
 int iommu_map(struct iommu_domain *domain, unsigned long iova,
 	      phys_addr_t paddr, size_t size, int prot)
 {
+	const struct iommu_ops *ops = domain->ops;
 	unsigned long orig_iova = iova;
 	unsigned int min_pagesz;
 	size_t orig_size = size;
 	phys_addr_t orig_paddr = paddr;
 	int ret = 0;
 
-	if (unlikely(domain->ops->map == NULL ||
+	if (unlikely(ops->map == NULL ||
 		     domain->pgsize_bitmap == 0UL))
 		return -ENODEV;
 
@@ -1580,7 +1581,7 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		pr_debug("mapping: iova 0x%lx pa %pa pgsize 0x%zx\n",
 			 iova, &paddr, pgsize);
 
-		ret = domain->ops->map(domain, iova, paddr, pgsize, prot);
+		ret = ops->map(domain, iova, paddr, pgsize, prot);
 		if (ret)
 			break;
 
@@ -1589,6 +1590,9 @@ int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		size -= pgsize;
 	}
 
+	if (ops->iotlb_sync_map)
+		ops->iotlb_sync_map(domain);
+
 	/* unroll mapping in case something went wrong */
 	if (ret)
 		iommu_unmap(domain, orig_iova, orig_size - size);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7447b0b0579a..7ed115526fb5 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -206,6 +206,7 @@ struct iommu_ops {
 	void (*flush_iotlb_all)(struct iommu_domain *domain);
 	void (*iotlb_range_add)(struct iommu_domain *domain,
 				unsigned long iova, size_t size);
+	void (*iotlb_sync_map)(struct iommu_domain *domain);
 	void (*iotlb_sync)(struct iommu_domain *domain);
 	phys_addr_t (*iova_to_phys)(struct iommu_domain *domain, dma_addr_t iova);
 	int (*add_device)(struct device *dev);
-- 
2.18.0

[PATCH v2 8/8] iommu/tegra: gart: Optimize mapping / unmapping performance

[Dmitry Osipenko]

Currently GART writes one page entry at a time. More optimal would be to
aggregate the writes and flush BUS buffer in the end, this gives map/unmap
10-40% performance boost (depending on size of mapping) in comparison to
flushing after each page entry update.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/iommu/tegra-gart.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index bcdb8973a0ad..5e3fd32fce4a 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -293,7 +293,6 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		}
 	}
 	gart_set_pte(gart, iova, GART_PTE(pfn));
-	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
 	return 0;
 }
@@ -310,7 +309,6 @@ static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 
 	spin_lock_irqsave(&gart->pte_lock, flags);
 	gart_set_pte(gart, iova, 0);
-	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
 	return bytes;
 }
@@ -376,6 +374,14 @@ static int gart_iommu_of_xlate(struct device *dev,
 	return 0;
 }
 
+static void gart_iommu_sync(struct iommu_domain *domain)
+{
+	struct gart_domain *gart_domain = to_gart_domain(domain);
+	struct gart_device *gart = gart_domain->gart;
+
+	FLUSH_GART_REGS(gart);
+}
+
 static const struct iommu_ops gart_iommu_ops = {
 	.capable	= gart_iommu_capable,
 	.domain_alloc	= gart_iommu_domain_alloc,
@@ -391,6 +397,8 @@ static const struct iommu_ops gart_iommu_ops = {
 	.iova_to_phys	= gart_iommu_iova_to_phys,
 	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
 	.of_xlate	= gart_iommu_of_xlate,
+	.iotlb_sync_map	= gart_iommu_sync,
+	.iotlb_sync	= gart_iommu_sync,
 };
 
 static int tegra_gart_suspend(struct device *dev)
-- 
2.18.0

Re: [PATCH v2 0/8] Tegra GART driver clean up and optimization

[Joerg Roedel]

Hey Thierry,

On Sat, Aug 04, 2018 at 05:29:55PM +0300, Dmitry Osipenko wrote:
> Dmitry Osipenko (8):
>   memory: tegra: Provide facility for integration with the GART driver
>   iommu/tegra: gart: Provide access to Memory Controller driver
>   iommu/tegra: gart: Clean up drivers module code
>   iommu/tegra: gart: Remove pr_fmt and clean up includes
>   iommu/tegra: gart: Clean up driver probe errors handling
>   iommu/tegra: gart: Ignore devices without IOMMU phandle in DT
>   iommu: Introduce iotlb_sync_map callback
>   iommu/tegra: gart: Optimize mapping / unmapping performance
> 
>  drivers/iommu/iommu.c      |  8 ++-
>  drivers/iommu/tegra-gart.c | 99 +++++++++++++++++++++++---------------
>  drivers/memory/tegra/mc.c  | 26 ++++++++--
>  include/linux/iommu.h      |  1 +
>  include/soc/tegra/mc.h     | 13 +++++
>  5 files changed, 103 insertions(+), 44 deletions(-)

Can you have a look please whether this is safe to go upstream?

Thanks,

	Joerg