[PATCH v2] ARM: dt: tegra20: Add GART device

[PATCH v2] ARM: dt: tegra20: Add GART device

[Thierry Reding]

This commit adds the device node required to probe NVIDIA Tegra 20 GART
hardware from the device tree.

Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v2:
- drop unneeded of_dev_auxdata entry

 arch/arm/boot/dts/tegra20.dtsi |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 55b28fd..cf3ff41 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -212,5 +212,11 @@
 		interrupts = < 0 97 0x04 >;
 		phy_type = "utmi";
 	};
+
+	gart: gart@7000f000 {
+		compatible = "nvidia,tegra20-gart";
+		reg = < 0x7000f000 0x00000100    /* controller registers */
+		        0x58000000 0x02000000 >; /* GART aperture */
+	};
 };
 
-- 
1.7.10

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Stephen Warren]

On 04/16/2012 09:04 AM, Thierry Reding wrote:
> This commit adds the device node required to probe NVIDIA Tegra 20 GART
> hardware from the device tree.
> 
> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>

Applied, thanks.

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Stephen Warren]

On 04/16/2012 09:04 AM, Thierry Reding wrote:
> This commit adds the device node required to probe NVIDIA Tegra 20 GART
> hardware from the device tree.

> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi

> +	gart: gart@7000f000 {
> +		compatible = "nvidia,tegra20-gart";
> +		reg = < 0x7000f000 0x00000100    /* controller registers */
> +		        0x58000000 0x02000000 >; /* GART aperture */
> +	};

Thierry, Hiroshi, Olof,

I have already applied this to Tegra's for inclusion in 3.5, but I'm
considering dropping it. Further thought/discussions related to adding
DT to the Tegra SMMU have raised an issue with a similar binding there.

I'd like to hear peoples' thoughts on all this, and get a resolution,
before we go ahead with any more GART/SMMU/MC patches.

The Tegra20 register block at 7000f000 is not (just) the GART, but the
MC (Memory Controller). This register block contains both general
MC-related registers, and the GART registers. The situation is identical
on Tegra30, except that the register block contains both MC and SMMU
registers.

(Note: MC/Memory-Controller is a different module to the
EMC/External-Memory-Controller)

As such, I think this node should be more like:

mc: mc@7000f000 {
    compatible = "nvidia,tegra20-mc";
    reg = <0x7000f000 0x00000100>;
};

Then, one of following options can be taken:

Option 1:

Just rename the node to "MC", but have a single driver that both does
whatever MC-related functionality is needed, but also registers as
either the GART or SMMU driver. This would be the simplest; we'd just
need to rename a few compatible flags and update the binding
documentation etc. The only disadvantage is that it merges the plain MC
and GART/SMMU code into a single driver, but perhaps that's sane since
the HW merges all that into a single HW module.

Option 2:

We add an explicit child node to MC in order to instantiate the GART
itself, and have the MC call of_platform_populate() to instantiate it,
just like a bus driver:

mc: mc@7000f000 {
    compatible = "nvidia,tegra20-mc";
    reg = <0x7000f000 0x00000100>;

    ranges;
    #address-cells = <1>;
    #size-cells = <1>;

    gart: gart {
        compatible = "nvidia,tegra20-gart";
        reg = <0x7000f000 0x00000100   /* controller registers */
               0x58000000 0x02000000>; /* GART aperture */
    };
};

The question here is: If the MC and GART drivers both need to access
basically the same register block, they can't both call
request_mem_region() on the registers, which is rather unfortunate.

Also, there aren't really separate HW modules, so writing the device
tree as if there were perhaps feels a little wrong.

Option 3:

The MC driver knows that its register range includes the GART registers,
and hence whenever it's probed, it internally causes the GART driver to
be probed somehow (either by manually registering a platform device to
do this, or by removing all the regular device probing code from the
GART driver, and having the MC driver call some pseudo-probe code in the
GART driver)

tegra-mc.c:

tegra_mc_probe()
{
    /* All the usual setup for the MC driver itself */
    mc->regs = ...;

    /* Instantiate SMMU; passing the already-mapped regs to it */
    tegra30_smmu_probe(&pdev->dev, mc->regs);
}

tegra-smmu.c:

int tegra30_smmu_probe(struct device *dev, void __iomem *regs)

This would be just like current tegra30_smmu_probe, except that:

a) No need to map registers; they're passed in.

b) The SMMU doesn't have its own struct device, but shares with the
parent MC controller. In this case, I don't think that's an issue, since
the "struct smmu_device" is stored globally, so we can use that instead
of dev_get/set_drvdata().

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Stephen Warren]

On 05/03/2012 12:28 PM, Stephen Warren wrote:
> On 04/16/2012 09:04 AM, Thierry Reding wrote:
>> This commit adds the device node required to probe NVIDIA Tegra 20 GART
>> hardware from the device tree.
> 
>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> 
>> +	gart: gart@7000f000 {
>> +		compatible = "nvidia,tegra20-gart";
>> +		reg = < 0x7000f000 0x00000100    /* controller registers */
>> +		        0x58000000 0x02000000 >; /* GART aperture */
>> +	};
...
> As such, I think this node should be more like:
> 
> mc: mc@7000f000 {
>     compatible = "nvidia,tegra20-mc";
>     reg = <0x7000f000 0x00000100>;
> };
> 
> Then, one of following options can be taken:
[to represent the fact that MC and GART are essentially the same HW module]
...

Yet another option is to explicitly use the MFD subsystem for this; I
guess options 1 and 3 that I gave were essentially open-coding MFD anyway.

Offline, Hiroshi mentioned that he had looked at MFD, and ended up
thinking it was a rather heavy-weight solution in this case though.

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Hiroshi Doyu]

On Thu, 3 May 2012 20:58:39 +0200
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 05/03/2012 12:28 PM, Stephen Warren wrote:
> > On 04/16/2012 09:04 AM, Thierry Reding wrote:
> >> This commit adds the device node required to probe NVIDIA Tegra 20 GART
> >> hardware from the device tree.
> > 
> >> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> > 
> >> +	gart: gart@7000f000 {
> >> +		compatible = "nvidia,tegra20-gart";
> >> +		reg = < 0x7000f000 0x00000100    /* controller registers */
> >> +		        0x58000000 0x02000000 >; /* GART aperture */
> >> +	};
> ...
> > As such, I think this node should be more like:
> > 
> > mc: mc@7000f000 {
> >     compatible = "nvidia,tegra20-mc";
> >     reg = <0x7000f000 0x00000100>;
> > };
> > 
> > Then, one of following options can be taken:
> [to represent the fact that MC and GART are essentially the same HW module]
> ...
> 
> Yet another option is to explicitly use the MFD subsystem for this; I
> guess options 1 and 3 that I gave were essentially open-coding MFD anyway.
> 
> Offline, Hiroshi mentioned that he had looked at MFD, and ended up
> thinking it was a rather heavy-weight solution in this case though.

It's because we just spawns a single child device(SMMU) from MC. I
guess that MFD framework would be more beneficial when it spawns
multiple/many children(?). 

Here's my MFD experiment:

>From 720331a8d54052da3ee214dd80193b9c853b9d93 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Wed, 2 May 2012 13:36:10 +0300
Subject: [PATCH 1/1] ARM: tegra: Memory Controller(MC) adds IOMMU device via
 MFD

Tegra IOMMU devices(GART/SMMU) share some resources with MC. Those
IOMMUs can be considered as a part of MC. MC adds GART/SMMU as its
child device at probe().

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/tegra-mc.c |   31 +++++++++++++++++++-----
 drivers/iommu/tegra-smmu.c     |   50 +++++++++++++++++++++++++++++----------
 2 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-tegra/tegra-mc.c b/arch/arm/mach-tegra/tegra-mc.c
index 7af2a99..eb95dce 100644
--- a/arch/arm/mach-tegra/tegra-mc.c
+++ b/arch/arm/mach-tegra/tegra-mc.c
@@ -25,6 +25,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/mfd/core.h>
 
 #define DRV_NAME "tegra-mc"
 
@@ -107,6 +108,7 @@ struct tegra_mc_info {
 	const char * const *client;
 	int num_client;
 	u32 id_mask;
+	struct mfd_cell iommu;
 };
 
 struct tegra_mc {
@@ -228,6 +230,9 @@ static struct tegra_mc_info tegra20_mc_info = {
 	.client = tegra20_mc_client,
 	.num_client = ARRAY_SIZE(tegra20_mc_client),
 	.id_mask = 0x3f,
+	.iommu = {
+		.name	= "tegra-gart",
+	},
 };
 #else
 static struct tegra_mc_info tegra20_mc_info = {};
@@ -354,6 +359,9 @@ static struct tegra_mc_info tegra30_mc_info = {
 	.client = tegra30_mc_client,
 	.num_client = ARRAY_SIZE(tegra30_mc_client),
 	.id_mask = 0x7f,
+	.iommu = {
+		.name	= "tegra-smmu",
+	},
 };
 
 static u32 tegra_mc_ctx[] = {
@@ -437,7 +445,7 @@ static irqreturn_t tegra_mc_isr(int irq, void *data)
 
 static int __devinit tegra_mc_probe(struct platform_device *pdev)
 {
-	struct resource *res;
+	struct resource *mem, *irq;
 	struct tegra_mc *mc;
 	size_t bytes;
 	int err;
@@ -450,17 +458,18 @@ static int __devinit tegra_mc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	mc->dev = &pdev->dev;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem)
 		return -ENODEV;
-	mc->regs = devm_request_and_ioremap(&pdev->dev, res);
+	mc->regs = devm_request_and_ioremap(&pdev->dev, mem);
 	if (!mc->regs)
 		return -EBUSY;
 
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!res)
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!irq)
 		return -ENODEV;
-	err = devm_request_irq(&pdev->dev, res->start, tegra_mc_isr,
+
+	err = devm_request_irq(&pdev->dev, irq->start, tegra_mc_isr,
 			       IRQF_SHARED, dev_name(&pdev->dev), mc);
 	if (err)
 		return -ENODEV;
@@ -473,6 +482,14 @@ static int __devinit tegra_mc_probe(struct platform_device *pdev)
 	info = id->data;
 	if (!info)
 		return -EINVAL;
+
+	if (&info->iommu) {
+		err = mfd_add_devices(&pdev->dev, pdev->id, &info->iommu, 1,
+				      NULL, irq->start);
+		if (err)
+			return -ENODEV;
+	}
+
 	mc->info = info;
 	info->intmask |= MC_INT_DECERR_EMEM | MC_INT_SECURITY_VIOLATION;
 	mc_writel(mc, info->intmask, MC_INTMASK);
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index c70e4e7..21c15bb 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -31,6 +31,7 @@
 #include <linux/iommu.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 
 #include <asm/page.h>
 #include <asm/cacheflush.h>
@@ -113,7 +114,6 @@
 
 #define SMMU_PDE_NEXT_SHIFT		28
 
-#define SMMU_NUM_ASIDS				4
 #define SMMU_TLB_FLUSH_VA_SECTION__MASK		0xffc00000
 #define SMMU_TLB_FLUSH_VA_SECTION__SHIFT	12 /* right shift */
 #define SMMU_TLB_FLUSH_VA_GROUP__MASK		0xffffc000
@@ -872,21 +872,38 @@ static int tegra_smmu_resume(struct device *dev)
 static int tegra_smmu_probe(struct platform_device *pdev)
 {
 	struct smmu_device *smmu;
-	struct resource *regs, *window;
 	struct device *dev = &pdev->dev;
-	int i, err = 0;
+	int i, asids, err = 0;
+	dma_addr_t base;
+	size_t size;
+	const void *prop;
+	struct device_node *mc;
+	struct resource *res;
 
 	if (smmu_handle)
 		return -EIO;
 
 	BUILD_BUG_ON(PAGE_SHIFT != SMMU_PAGE_SHIFT);
 
-	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	window = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (!regs || !window) {
-		dev_err(dev, "No SMMU resources\n");
+	mc = pdev->dev.parent->of_node;
+	if (!mc)
+		return -ENODEV;
+
+	err = of_get_dma_window(mc, "dma-window", 0, NULL,
+				&base, &size);
+	if (err)
+		return -ENODEV;
+
+	size >>= SMMU_PAGE_SHIFT;
+	if (!size)
+		return -ENODEV;
+
+	prop = of_get_property(mc, "nvidia,#asids", NULL);
+	if (!prop)
+		return -ENODEV;
+	asids = be32_to_cpup(prop);
+	if (!asids)
 		return -ENODEV;
-	}
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
 	if (!smmu) {
@@ -895,17 +912,23 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	}
 
 	smmu->dev = dev;
-	smmu->num_as = SMMU_NUM_ASIDS;
-	smmu->iovmm_base = (unsigned long)window->start;
-	smmu->page_count = resource_size(window) >> SMMU_PAGE_SHIFT;
-	smmu->regs = devm_ioremap(dev, regs->start, resource_size(regs));
+	smmu->num_as = asids;
+	smmu->iovmm_base = base;
+	smmu->page_count = size;
+
+	res = platform_get_resource(to_platform_device(pdev->dev.parent),
+				    IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	smmu->regs = devm_ioremap(&pdev->dev, res->start, resource_size(res));
 	if (!smmu->regs) {
 		dev_err(dev, "failed to remap SMMU registers\n");
 		err = -ENXIO;
 		goto fail;
 	}
 
-	smmu->ahb = of_parse_phandle(pdev->dev.of_node, "ahb", 0);
+	smmu->ahb = of_parse_phandle(mc, "ahb", 0);
 	if (!smmu->ahb)
 		return -ENODEV;
 
@@ -1019,4 +1042,5 @@ module_exit(tegra_smmu_exit);
 
 MODULE_DESCRIPTION("IOMMU API for SMMU in Tegra30");
 MODULE_AUTHOR("Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
+MODULE_ALIAS("platform:tegra-smmu");
 MODULE_LICENSE("GPL v2");
-- 
1.7.5.4

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Hiroshi Doyu]

From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Subject: Re: [PATCH v2] ARM: dt: tegra20: Add GART device
Date: Thu, 3 May 2012 20:28:02 +0200
Message-ID: <4FA2CE32.7010406-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

> On 04/16/2012 09:04 AM, Thierry Reding wrote:
> > This commit adds the device node required to probe NVIDIA Tegra 20 GART
> > hardware from the device tree.
> 
> > diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> 
> > +	gart: gart@7000f000 {
> > +		compatible = "nvidia,tegra20-gart";
> > +		reg = < 0x7000f000 0x00000100    /* controller registers */
> > +		        0x58000000 0x02000000 >; /* GART aperture */
> > +	};
> 
> Thierry, Hiroshi, Olof,
> 
> I have already applied this to Tegra's for inclusion in 3.5, but I'm
> considering dropping it. Further thought/discussions related to adding
> DT to the Tegra SMMU have raised an issue with a similar binding there.
> 
> I'd like to hear peoples' thoughts on all this, and get a resolution,
> before we go ahead with any more GART/SMMU/MC patches.
> 
> The Tegra20 register block at 7000f000 is not (just) the GART, but the
> MC (Memory Controller). This register block contains both general
> MC-related registers, and the GART registers. The situation is identical
> on Tegra30, except that the register block contains both MC and SMMU
> registers.

For the furthur info, GART occupies 1 block within MC register range,
and SMMU occupies 3 blocks. In both Tegra{20,30}, MC driver only uses
MC registers, GART uses GART registers in its own block, and SMMU uses
SMMU registers in its own blocks *exclusively*.

IRQ is handled in MC. They include IOMMU page access fault too, but
MC ISR just prints fault address and other info. IRQ handling
registers belong to MC, and GART/SMMU doesn't have to touch it.

 Tegra20:
 	7000f000-7000f3ff : /mc@7000f000	1KB
 	  7000f024-7000f02f : gart
 
 Tegra30:
 	7000f000-7000f3ff : /mc@7000f000	1KB
 	  7000f010-7000f03b : smmu, register block 1
 	  7000f1f0-7000f1ff : smmu, register block 2
 	  7000f228-7000f283 : smmu, register block 3
 	  ....

GART/SMMU register offsets are defined in TRM from the beginning of
MC(0x7000f000), instead of the beginning of their block(s). I think
that 'ioremap()' is done by unit of 4KB in the end because of MMU
address translation, and it's ok that SMMU/GART ioremap's 4KB from
0x7000f000.

> (Note: MC/Memory-Controller is a different module to the
> EMC/External-Memory-Controller)
> 
> As such, I think this node should be more like:
> 
> mc: mc@7000f000 {
>     compatible = "nvidia,tegra20-mc";
>     reg = <0x7000f000 0x00000100>;
> };
> 
> Then, one of following options can be taken:
> 
> Option 1:
> 
> Just rename the node to "MC", but have a single driver that both does
> whatever MC-related functionality is needed, but also registers as
> either the GART or SMMU driver. This would be the simplest; we'd just
> need to rename a few compatible flags and update the binding
> documentation etc. The only disadvantage is that it merges the plain MC
> and GART/SMMU code into a single driver, but perhaps that's sane since
> the HW merges all that into a single HW module.

>From their functionality POV, MC and IOMMU(GART/SMMU) drivers are
totally independent. Those drivers can be kept independent, except
register offset(0x7000f000) in TRM.

> Option 2:
> 
> We add an explicit child node to MC in order to instantiate the GART
> itself, and have the MC call of_platform_populate() to instantiate it,
> just like a bus driver:
> 
> mc: mc@7000f000 {
>     compatible = "nvidia,tegra20-mc";
>     reg = <0x7000f000 0x00000100>;
> 
>     ranges;
>     #address-cells = <1>;
>     #size-cells = <1>;
> 
>     gart: gart {
>         compatible = "nvidia,tegra20-gart";
>         reg = <0x7000f000 0x00000100   /* controller registers */
>                0x58000000 0x02000000>; /* GART aperture */
>     };
> };
> 
> The question here is: If the MC and GART drivers both need to access
> basically the same register block, they can't both call
> request_mem_region() on the registers, which is rather unfortunate.

Can "request_resource(mc_resource, gart_resource)" or
"insert_resource(mc_resource, gart_resource)" create the following
resource tree?

 Tegra20:
 	7000f000-7000f3ff : /mc@7000f000	1KB
 	  7000f024-7000f02f : gart

gart/smmu may be able to get the parent resources for ioremap()?

+	mc = pdev->dev.parent->of_node;
  or
+	res = platform_get_resource(to_platform_device(pdev->dev.parent), IORESOURCE_MEM, 0);


> Also, there aren't really separate HW modules, so writing the device
> tree as if there were perhaps feels a little wrong.
> 
> Option 3:
> 
> The MC driver knows that its register range includes the GART registers,
> and hence whenever it's probed, it internally causes the GART driver to
> be probed somehow (either by manually registering a platform device to
> do this, or by removing all the regular device probing code from the
> GART driver, and having the MC driver call some pseudo-probe code in the
> GART driver)
> 
> tegra-mc.c:
> 
> tegra_mc_probe()
> {
>     /* All the usual setup for the MC driver itself */
>     mc->regs = ...;
> 
>     /* Instantiate SMMU; passing the already-mapped regs to it */
>     tegra30_smmu_probe(&pdev->dev, mc->regs);
> }
> 
> tegra-smmu.c:
> 
> int tegra30_smmu_probe(struct device *dev, void __iomem *regs)
> 
> This would be just like current tegra30_smmu_probe, except that:
> 
> a) No need to map registers; they're passed in.
> 
> b) The SMMU doesn't have its own struct device, but shares with the
> parent MC controller. In this case, I don't think that's an issue, since
> the "struct smmu_device" is stored globally, so we can use that instead
> of dev_get/set_drvdata().

Option 4:

GART/SMMU only needs MC register range(resource). Their register
blocks are statically exclusively accessed although they are located
within MC register range. GART/SMMU and MC are quite independent from
functionarity/driver POV. So if we can ignore the exact H/W module
connections, what about the following info being provided?

	mc: mc@7000f000 {
		compatible = "nvidia,tegra20-mc";
		reg = <0x7000f000 0x400>;	/* controller registers */
		interrupts = <0 77 0x04>;
	};

	gart: gart {
		compatible = "nvidia,tegra20-gart";
		reg = <0x7000f024 0xc   /* controller registers */
		               ^^^^^^
		       0x58000000 0x02000000>; /* GART aperture */
		mc = &mc;
		^^^^^^^^^ 
	};

GART driver can get MC register range by phandle "mc", and make its
reservation as child of "mc". "GART aperture" just goes straight
because it's also visible to CPU. We could keep those drivers mostly
independent if DT allows this kind of phandle.

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Stephen Warren]

On 05/04/2012 03:13 AM, Hiroshi Doyu wrote:
> From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Subject: Re: [PATCH v2] ARM: dt: tegra20: Add GART device
> Date: Thu, 3 May 2012 20:28:02 +0200
> Message-ID: <4FA2CE32.7010406-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> 
>> On 04/16/2012 09:04 AM, Thierry Reding wrote:
>>> This commit adds the device node required to probe NVIDIA Tegra 20 GART
>>> hardware from the device tree.
>>
>>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
>>
>>> +	gart: gart@7000f000 {
>>> +		compatible = "nvidia,tegra20-gart";
>>> +		reg = < 0x7000f000 0x00000100    /* controller registers */
>>> +		        0x58000000 0x02000000 >; /* GART aperture */
>>> +	};
>>
>> Thierry, Hiroshi, Olof,
>>
>> I have already applied this to Tegra's for inclusion in 3.5, but I'm
>> considering dropping it. Further thought/discussions related to adding
>> DT to the Tegra SMMU have raised an issue with a similar binding there.
>>
>> I'd like to hear peoples' thoughts on all this, and get a resolution,
>> before we go ahead with any more GART/SMMU/MC patches.
>>
>> The Tegra20 register block at 7000f000 is not (just) the GART, but the
>> MC (Memory Controller). This register block contains both general
>> MC-related registers, and the GART registers. The situation is identical
>> on Tegra30, except that the register block contains both MC and SMMU
>> registers.
> 
> For the furthur info, GART occupies 1 block within MC register range,
> and SMMU occupies 3 blocks. In both Tegra{20,30}, MC driver only uses
> MC registers, GART uses GART registers in its own block, and SMMU uses
> SMMU registers in its own blocks *exclusively*.
> 
> IRQ is handled in MC. They include IOMMU page access fault too, but
> MC ISR just prints fault address and other info. IRQ handling
> registers belong to MC, and GART/SMMU doesn't have to touch it.
> 
>  Tegra20:
>  	7000f000-7000f3ff : /mc@7000f000	1KB
>  	  7000f024-7000f02f : gart
>  
>  Tegra30:
>  	7000f000-7000f3ff : /mc@7000f000	1KB
>  	  7000f010-7000f03b : smmu, register block 1
>  	  7000f1f0-7000f1ff : smmu, register block 2
>  	  7000f228-7000f283 : smmu, register block 3
>  	  ....

True, it appears that each register is logically part of MC or GART/SMMU
functionality. In that case, chunks of registers can be assigned to
specific drivers. However, the memory map is rather interleaved:

Tegra20:

00c-01c: MC
024-034: GART
03c-270: MC

Tegra30:

010-038: SMMU
050-220: MC
228-280: SMMU
29c-36c: MC

Perhaps the simplest way is to represent those chunks as separate reg
entries:

    mc@7000f010 {
        compatible = "nvidia,tegra30-mc";
        reg = <0x7000f010 0x1d4 0x70000228 0xd4>;
    };

    smmu@0x70000050 {
        compatible = "nvidia,tegra30-smmu";
        reg = <0x70000010 0x2c 0x70000228 0x5c>;
    };

This could all be hidden inside e.g. smmu_read():

static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
{
	BUG_ON(offs < 0x10);
	if (offs < 0x40)
        	return readl(smmu->regsa + offs - 0x10);
	BUG_ON(offs < 0x228);
	if (offs < 0x370)
        	return readl(smmu->regsb + offs - 0x228);
	BUG();
}

This would optimize out, since most likely "offs" would be a #define for
the register address.

Then there would be no need for the DT nodes or devices/drivers to
interact at all.

We should really talk to our HW engineers and make sure that future
chips don't get even more interleaved register maps - either make sure
the current layout is perpetuated exactly, or fix it to make them
entirely separate. Can you please start an internal thread with them on
this topic, Hiroshi?

>> Option 2:
>>
>> We add an explicit child node to MC in order to instantiate the GART
>> itself, and have the MC call of_platform_populate() to instantiate it,
>> just like a bus driver:
>>
>> mc: mc@7000f000 {
>>     compatible = "nvidia,tegra20-mc";
>>     reg = <0x7000f000 0x00000100>;
>>
>>     ranges;
>>     #address-cells = <1>;
>>     #size-cells = <1>;
>>
>>     gart: gart {
>>         compatible = "nvidia,tegra20-gart";
>>         reg = <0x7000f000 0x00000100   /* controller registers */
>>                0x58000000 0x02000000>; /* GART aperture */
>>     };
>> };
>>
>> The question here is: If the MC and GART drivers both need to access
>> basically the same register block, they can't both call
>> request_mem_region() on the registers, which is rather unfortunate.
> 
> Can "request_resource(mc_resource, gart_resource)" or
> "insert_resource(mc_resource, gart_resource)" create the following
> resource tree?
> 
>  Tegra20:
>  	7000f000-7000f3ff : /mc@7000f000	1KB
>  	  7000f024-7000f02f : gart
> 
> gart/smmu may be able to get the parent resources for ioremap()?
> 
> +	mc = pdev->dev.parent->of_node;
>   or
> +	res = platform_get_resource(to_platform_device(pdev->dev.parent), IORESOURCE_MEM, 0);

I'd certainly rather avoid drivers reaching into other devices to get
register windows, unless this is more explicitly managed by passing the
data from one device to another via an API for this purpose. The code
above feels quite fragile/dangerous.

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Hiroshi Doyu]

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

From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Subject: Re: [PATCH v2] ARM: dt: tegra20: Add GART device
Date: Fri, 4 May 2012 19:24:45 +0200
Message-ID: <4FA410DD.3090105-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

> On 05/04/2012 03:13 AM, Hiroshi Doyu wrote:
> > From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > Subject: Re: [PATCH v2] ARM: dt: tegra20: Add GART device
> > Date: Thu, 3 May 2012 20:28:02 +0200
> > Message-ID: <4FA2CE32.7010406-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > 
> >> On 04/16/2012 09:04 AM, Thierry Reding wrote:
> >>> This commit adds the device node required to probe NVIDIA Tegra 20 GART
> >>> hardware from the device tree.
> >>
> >>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> >>
> >>> +	gart: gart@7000f000 {
> >>> +		compatible = "nvidia,tegra20-gart";
> >>> +		reg = < 0x7000f000 0x00000100    /* controller registers */
> >>> +		        0x58000000 0x02000000 >; /* GART aperture */
> >>> +	};
> >>
> >> Thierry, Hiroshi, Olof,
> >>
> >> I have already applied this to Tegra's for inclusion in 3.5, but I'm
> >> considering dropping it. Further thought/discussions related to adding
> >> DT to the Tegra SMMU have raised an issue with a similar binding there.
> >>
> >> I'd like to hear peoples' thoughts on all this, and get a resolution,
> >> before we go ahead with any more GART/SMMU/MC patches.
> >>
> >> The Tegra20 register block at 7000f000 is not (just) the GART, but the
> >> MC (Memory Controller). This register block contains both general
> >> MC-related registers, and the GART registers. The situation is identical
> >> on Tegra30, except that the register block contains both MC and SMMU
> >> registers.
> > 
> > For the furthur info, GART occupies 1 block within MC register range,
> > and SMMU occupies 3 blocks. In both Tegra{20,30}, MC driver only uses
> > MC registers, GART uses GART registers in its own block, and SMMU uses
> > SMMU registers in its own blocks *exclusively*.
> > 
> > IRQ is handled in MC. They include IOMMU page access fault too, but
> > MC ISR just prints fault address and other info. IRQ handling
> > registers belong to MC, and GART/SMMU doesn't have to touch it.
> > 
> >  Tegra20:
> >  	7000f000-7000f3ff : /mc@7000f000	1KB
> >  	  7000f024-7000f02f : gart
> >  
> >  Tegra30:
> >  	7000f000-7000f3ff : /mc@7000f000	1KB
> >  	  7000f010-7000f03b : smmu, register block 1
> >  	  7000f1f0-7000f1ff : smmu, register block 2
> >  	  7000f228-7000f283 : smmu, register block 3
> >  	  ....
> 
> True, it appears that each register is logically part of MC or GART/SMMU
> functionality. In that case, chunks of registers can be assigned to
> specific drivers. However, the memory map is rather interleaved:
> 
> Tegra20:
> 
> 00c-01c: MC
> 024-034: GART
> 03c-270: MC
> 
> Tegra30:
> 
> 010-038: SMMU
> 050-220: MC
> 228-280: SMMU
> 29c-36c: MC
> 
> Perhaps the simplest way is to represent those chunks as separate reg
> entries:
> 
>     mc@7000f010 {
>         compatible = "nvidia,tegra30-mc";
>         reg = <0x7000f010 0x1d4 0x70000228 0xd4>;
>     };
> 
>     smmu@0x70000050 {
>         compatible = "nvidia,tegra30-smmu";
>         reg = <0x70000010 0x2c 0x70000228 0x5c>;
>     };
> 
> This could all be hidden inside e.g. smmu_read():
> 
> static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
> {
> 	BUG_ON(offs < 0x10);
> 	if (offs < 0x40)
>         	return readl(smmu->regsa + offs - 0x10);
> 	BUG_ON(offs < 0x228);
> 	if (offs < 0x370)
>         	return readl(smmu->regsb + offs - 0x228);
> 	BUG();
> }
> 
> This would optimize out, since most likely "offs" would be a #define for
> the register address.
> 
> Then there would be no need for the DT nodes or devices/drivers to
> interact at all.

This may be quite simple and enough ok at the moment, instead of
trying to make some tricky connections between devices now.

For reference, attached the above patch to this mail.

> We should really talk to our HW engineers and make sure that future
> chips don't get even more interleaved register maps - either make sure
> the current layout is perpetuated exactly, or fix it to make them
> entirely separate. Can you please start an internal thread with them on
> this topic, Hiroshi?

Ok, Done.

> >> Option 2:
> >>
> >> We add an explicit child node to MC in order to instantiate the GART
> >> itself, and have the MC call of_platform_populate() to instantiate it,
> >> just like a bus driver:
> >>
> >> mc: mc@7000f000 {
> >>     compatible = "nvidia,tegra20-mc";
> >>     reg = <0x7000f000 0x00000100>;
> >>
> >>     ranges;
> >>     #address-cells = <1>;
> >>     #size-cells = <1>;
> >>
> >>     gart: gart {
> >>         compatible = "nvidia,tegra20-gart";
> >>         reg = <0x7000f000 0x00000100   /* controller registers */
> >>                0x58000000 0x02000000>; /* GART aperture */
> >>     };
> >> };
> >>
> >> The question here is: If the MC and GART drivers both need to access
> >> basically the same register block, they can't both call
> >> request_mem_region() on the registers, which is rather unfortunate.
> > 
> > Can "request_resource(mc_resource, gart_resource)" or
> > "insert_resource(mc_resource, gart_resource)" create the following
> > resource tree?
> > 
> >  Tegra20:
> >  	7000f000-7000f3ff : /mc@7000f000	1KB
> >  	  7000f024-7000f02f : gart
> > 
> > gart/smmu may be able to get the parent resources for ioremap()?
> > 
> > +	mc = pdev->dev.parent->of_node;
> >   or
> > +	res = platform_get_resource(to_platform_device(pdev->dev.parent), IORESOURCE_MEM, 0);
> 
> I'd certainly rather avoid drivers reaching into other devices to get
> register windows, unless this is more explicitly managed by passing the
> data from one device to another via an API for this purpose. The code
> above feels quite fragile/dangerous.

Ok, let's start with the simplest way, and improve later with nice
framework.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-iommu-tegra-smmu-Add-DT-support-for-SMMU.patch --]
[-- Type: text/x-patch; name="0001-iommu-tegra-smmu-Add-DT-support-for-SMMU.patch", Size: 9705 bytes --]

From b8c18283190e83cd77285a56175325131baadf86 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu@nvidia.com>
Date: Wed, 18 Apr 2012 15:26:39 +0300
Subject: [PATCH 1/1] iommu/tegra: smmu: Add DT support for SMMU

Add DT support for SMMU. The necessary info is expected to pass from
DT with more precise resource reservation, independet of the parent MC
regiter range.

Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
---
 drivers/iommu/Kconfig      |    1 +
 drivers/iommu/tegra-smmu.c |  156 +++++++++++++++++++++++++++++---------------
 2 files changed, 103 insertions(+), 54 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index c698437..24308f4 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -155,6 +155,7 @@ config TEGRA_IOMMU_GART
 config TEGRA_IOMMU_SMMU
 	bool "Tegra SMMU IOMMU Support"
 	depends on ARCH_TEGRA_3x_SOC
+	select TEGRA_AHB
 	select IOMMU_API
 	help
 	  Enables support for remapping discontiguous physical memory
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ecd6790..0a1954e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -30,12 +30,15 @@
 #include <linux/sched.h>
 #include <linux/iommu.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 #include <asm/page.h>
 #include <asm/cacheflush.h>
 
 #include <mach/iomap.h>
 #include <mach/smmu.h>
+#include <mach/tegra-ahb.h>
 
 /* bitmap of the page sizes currently supported */
 #define SMMU_IOMMU_PGSIZES	(SZ_4K)
@@ -111,12 +114,6 @@
 
 #define SMMU_PDE_NEXT_SHIFT		28
 
-/* AHB Arbiter Registers */
-#define AHB_XBAR_CTRL				0xe0
-#define AHB_XBAR_CTRL_SMMU_INIT_DONE_DONE	1
-#define AHB_XBAR_CTRL_SMMU_INIT_DONE_SHIFT	17
-
-#define SMMU_NUM_ASIDS				4
 #define SMMU_TLB_FLUSH_VA_SECTION__MASK		0xffc00000
 #define SMMU_TLB_FLUSH_VA_SECTION__SHIFT	12 /* right shift */
 #define SMMU_TLB_FLUSH_VA_GROUP__MASK		0xffffc000
@@ -177,6 +174,8 @@
 #define SMMU_ASID_DISABLE	0
 #define SMMU_ASID_ASID(n)	((n) & ~SMMU_ASID_ENABLE(0))
 
+#define NUM_SMMU_REG_BANKS	3
+
 #define smmu_client_enable_hwgrp(c, m)	smmu_client_set_hwgrp(c, m, 1)
 #define smmu_client_disable_hwgrp(c)	smmu_client_set_hwgrp(c, 0, 0)
 #define __smmu_client_enable_hwgrp(c, m) __smmu_client_set_hwgrp(c, m, 1)
@@ -235,7 +234,7 @@ struct smmu_as {
  * Per SMMU device - IOMMU device
  */
 struct smmu_device {
-	void __iomem	*regs, *regs_ahbarb;
+	void __iomem	*regs[NUM_SMMU_REG_BANKS];
 	unsigned long	iovmm_base;	/* remappable base address */
 	unsigned long	page_count;	/* total remappable size */
 	spinlock_t	lock;
@@ -252,29 +251,57 @@ struct smmu_device {
 	unsigned long translation_enable_1;
 	unsigned long translation_enable_2;
 	unsigned long asid_security;
+
+	struct device_node *ahb;
 };
 
 static struct smmu_device *smmu_handle; /* unique for a system */
 
 /*
- *	SMMU/AHB register accessors
+ *	SMMU register accessors
  */
-static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
-{
-	return readl(smmu->regs + offs);
-}
-static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
+
+static int get_reg_bank(u32 offset)
 {
-	writel(val, smmu->regs + offs);
+	int i;
+	const struct smmu_reg_bank {
+		u32 start;
+		size_t size;
+	} r[] = {
+		{
+			.start = 0x10,
+			.size = 0x2c,
+		},
+		{
+			.start = 0x1f0,
+			.size = 0x10,
+		},
+		{
+			.start = 0x228,
+			.size = 0x5c,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(r); i++) {
+		BUG_ON(offset < r[i].start);
+		if (offset < r[i].start + r[i].size)
+			return i;
+	}
+	BUG();
+	return 0; /* Never reach here */
 }
 
-static inline u32 ahb_read(struct smmu_device *smmu, size_t offs)
+static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
 {
-	return readl(smmu->regs_ahbarb + offs);
+	int i;
+	i = get_reg_bank(offs);
+	return readl(smmu->regs[i] + offs);
 }
-static inline void ahb_write(struct smmu_device *smmu, u32 val, size_t offs)
+static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
 {
-	writel(val, smmu->regs_ahbarb + offs);
+	int i;
+	i = get_reg_bank(offs);
+	writel(val, smmu->regs[i] + offs);
 }
 
 #define VA_PAGE_TO_PA(va, page)	\
@@ -370,7 +397,7 @@ static void smmu_flush_regs(struct smmu_device *smmu, int enable)
 	FLUSH_SMMU_REGS(smmu);
 }
 
-static void smmu_setup_regs(struct smmu_device *smmu)
+static int smmu_setup_regs(struct smmu_device *smmu)
 {
 	int i;
 	u32 val;
@@ -398,10 +425,7 @@ static void smmu_setup_regs(struct smmu_device *smmu)
 
 	smmu_flush_regs(smmu, 1);
 
-	val = ahb_read(smmu, AHB_XBAR_CTRL);
-	val |= AHB_XBAR_CTRL_SMMU_INIT_DONE_DONE <<
-		AHB_XBAR_CTRL_SMMU_INIT_DONE_SHIFT;
-	ahb_write(smmu, val, AHB_XBAR_CTRL);
+	return tegra_ahb_enable_smmu(smmu->ahb);
 }
 
 static void flush_ptc_and_tlb(struct smmu_device *smmu,
@@ -873,59 +897,77 @@ static int tegra_smmu_resume(struct device *dev)
 {
 	struct smmu_device *smmu = dev_get_drvdata(dev);
 	unsigned long flags;
+	int err;
 
 	spin_lock_irqsave(&smmu->lock, flags);
-	smmu_setup_regs(smmu);
+	err = smmu_setup_regs(smmu);
 	spin_unlock_irqrestore(&smmu->lock, flags);
-	return 0;
+	return err;
 }
 
 static int tegra_smmu_probe(struct platform_device *pdev)
 {
 	struct smmu_device *smmu;
-	struct resource *regs, *regs2, *window;
 	struct device *dev = &pdev->dev;
-	int i, err = 0;
+	int i, asids, err = 0;
+	dma_addr_t base;
+	size_t size;
+	const void *prop;
 
 	if (smmu_handle)
 		return -EIO;
 
 	BUILD_BUG_ON(PAGE_SHIFT != SMMU_PAGE_SHIFT);
 
-	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	regs2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	window = platform_get_resource(pdev, IORESOURCE_MEM, 2);
-	if (!regs || !regs2 || !window) {
-		dev_err(dev, "No SMMU resources\n");
-		return -ENODEV;
-	}
-
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
 	if (!smmu) {
 		dev_err(dev, "failed to allocate smmu_device\n");
 		return -ENOMEM;
 	}
 
-	smmu->dev = dev;
-	smmu->num_as = SMMU_NUM_ASIDS;
-	smmu->iovmm_base = (unsigned long)window->start;
-	smmu->page_count = resource_size(window) >> SMMU_PAGE_SHIFT;
-	smmu->regs = devm_ioremap(dev, regs->start, resource_size(regs));
-	smmu->regs_ahbarb = devm_ioremap(dev, regs2->start,
-					 resource_size(regs2));
-	if (!smmu->regs || !smmu->regs_ahbarb) {
-		dev_err(dev, "failed to remap SMMU registers\n");
-		err = -ENXIO;
-		goto fail;
+	for (i = 0; i < ARRAY_SIZE(smmu->regs); i++) {
+		struct resource *res;
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!res)
+			return -ENODEV;
+		smmu->regs[i] = devm_request_and_ioremap(&pdev->dev, res);
+		if (!smmu->regs[i])
+			return -EBUSY;
 	}
 
+	err = of_get_dma_window(dev->of_node, "dma-window", 0, NULL,
+				&base, &size);
+	if (err)
+		return -ENODEV;
+
+	size >>= SMMU_PAGE_SHIFT;
+	if (!size)
+		return -ENODEV;
+
+	prop = of_get_property(dev->of_node, "nvidia,#asids", NULL);
+	if (!prop)
+		return -ENODEV;
+	asids = be32_to_cpup(prop);
+	if (!asids)
+		return -ENODEV;
+
+	smmu->ahb = of_parse_phandle(pdev->dev.of_node, "nvidia,ahb", 0);
+	if (!smmu->ahb)
+		return -ENODEV;
+
+	smmu->dev = dev;
+	smmu->num_as = asids;
+	smmu->iovmm_base = base;
+	smmu->page_count = size;
+
 	smmu->translation_enable_0 = ~0;
 	smmu->translation_enable_1 = ~0;
 	smmu->translation_enable_2 = ~0;
 	smmu->asid_security = 0;
 
 	smmu->as = devm_kzalloc(dev,
-			sizeof(smmu->as[0]) * smmu->num_as, GFP_KERNEL);
+				sizeof(smmu->as[0]) * smmu->num_as, GFP_KERNEL);
 	if (!smmu->as) {
 		dev_err(dev, "failed to allocate smmu_as\n");
 		err = -ENOMEM;
@@ -945,7 +987,9 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 		INIT_LIST_HEAD(&as->client);
 	}
 	spin_lock_init(&smmu->lock);
-	smmu_setup_regs(smmu);
+	err = smmu_setup_regs(smmu);
+	if (err)
+		goto fail;
 	platform_set_drvdata(pdev, smmu);
 
 	smmu->avp_vector_page = alloc_page(GFP_KERNEL);
@@ -958,10 +1002,6 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 fail:
 	if (smmu->avp_vector_page)
 		__free_page(smmu->avp_vector_page);
-	if (smmu->regs)
-		devm_iounmap(dev, smmu->regs);
-	if (smmu->regs_ahbarb)
-		devm_iounmap(dev, smmu->regs_ahbarb);
 	if (smmu && smmu->as) {
 		for (i = 0; i < smmu->num_as; i++) {
 			if (smmu->as[i].pdir_page) {
@@ -993,8 +1033,6 @@ static int tegra_smmu_remove(struct platform_device *pdev)
 		__free_page(smmu->avp_vector_page);
 	if (smmu->regs)
 		devm_iounmap(dev, smmu->regs);
-	if (smmu->regs_ahbarb)
-		devm_iounmap(dev, smmu->regs_ahbarb);
 	devm_kfree(dev, smmu);
 	smmu_handle = NULL;
 	return 0;
@@ -1005,6 +1043,14 @@ const struct dev_pm_ops tegra_smmu_pm_ops = {
 	.resume		= tegra_smmu_resume,
 };
 
+#ifdef CONFIG_OF
+static struct of_device_id tegra_smmu_of_match[] __devinitdata = {
+	{ .compatible = "nvidia,tegra30-smmu", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_smmu_of_match);
+#endif
+
 static struct platform_driver tegra_smmu_driver = {
 	.probe		= tegra_smmu_probe,
 	.remove		= tegra_smmu_remove,
@@ -1012,6 +1058,7 @@ static struct platform_driver tegra_smmu_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "tegra-smmu",
 		.pm	= &tegra_smmu_pm_ops,
+		.of_match_table = of_match_ptr(tegra_smmu_of_match),
 	},
 };
 
@@ -1031,4 +1078,5 @@ module_exit(tegra_smmu_exit);
 
 MODULE_DESCRIPTION("IOMMU API for SMMU in Tegra30");
 MODULE_AUTHOR("Hiroshi DOYU <hdoyu@nvidia.com>");
+MODULE_ALIAS("platform:tegra-smmu");
 MODULE_LICENSE("GPL v2");
-- 
1.7.5.4


[-- Attachment #3: Type: text/plain, Size: 190 bytes --]

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Stephen Warren]

On 05/07/2012 05:47 AM, Hiroshi Doyu wrote:
> Stephen Warren wrote at Fri, 4 May 2012 19:24:45 +0200:
...
>> Perhaps the simplest way is to represent those chunks as separate reg
>> entries:
>>
>>     mc@7000f010 {
>>         compatible = "nvidia,tegra30-mc";
>>         reg = <0x7000f010 0x1d4 0x70000228 0xd4>;
>>     };
>>
>>     smmu@0x70000050 {
>>         compatible = "nvidia,tegra30-smmu";
>>         reg = <0x70000010 0x2c 0x70000228 0x5c>;
>>     };
...
> Ok, let's start with the simplest way, and improve later with nice
> framework.

Uggh. So that patch is for the SMMU, but this thread is about the GART.
Also, attaching patches makes it more difficult to review them, since
them may not appear inline when reading the mail, and certainly don't
when replying. Anyway...

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

> +#include <mach/tegra-ahb.h>

Note that this patch therefor depends on the AHB series that you just
posted, and isn't applied anywhere yet.

> +static int get_reg_bank(u32 offset)
>  {
> -	writel(val, smmu->regs + offs);
> +	int i;
> +	const struct smmu_reg_bank {
> +		u32 start;
> +		size_t size;
> +	} r[] = {
> +		{
> +			.start = 0x10,
> +			.size = 0x2c,
> +		},
> +		{
> +			.start = 0x1f0,
> +			.size = 0x10,
> +		},
> +		{
> +			.start = 0x228,
> +			.size = 0x5c,
> +		},
> +	};
> +
> +	for (i = 0; i < ARRAY_SIZE(r); i++) {
> +		BUG_ON(offset < r[i].start);
> +		if (offset < r[i].start + r[i].size)
> +			return i;
> +	}
> +	BUG();
> +	return 0; /* Never reach here */
>  }

While stylistically nice, I'm not sure that will be as likely to
optimize out as the simple if/else code I gave as an example. I suppose
it's not a big deal though.

>  static int tegra_smmu_probe(struct platform_device *pdev)

> +	err = of_get_dma_window(dev->of_node, "dma-window", 0, NULL,
> +				&base, &size);

So this also relies on the generic DMA window parsing patch too. I don't
think that's checked in anywhere yet right?

> +	size >>= SMMU_PAGE_SHIFT;
> +	if (!size)
> +		return -ENODEV;

It seems like this should validate that !(size & ((1 << SMMU_PAGE_SHIFT)
- 1)) too?

This patch looks like it adds the full implementation of device tree
support, yet doesn't add any documentation for the binding.

Finally, I assume you're going to post a similar patch for the Tegra20
GART, so its register ranges don't overlap the Tegra20 MC?

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Hiroshi Doyu]

From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Subject: Re: [PATCH v2] ARM: dt: tegra20: Add GART device
Date: Mon, 7 May 2012 18:06:46 +0200
Message-ID: <4FA7F316.7070904-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

> On 05/07/2012 05:47 AM, Hiroshi Doyu wrote:
> > Stephen Warren wrote at Fri, 4 May 2012 19:24:45 +0200:
> ...
> >> Perhaps the simplest way is to represent those chunks as separate reg
> >> entries:
> >>
> >>     mc@7000f010 {
> >>         compatible = "nvidia,tegra30-mc";
> >>         reg = <0x7000f010 0x1d4 0x70000228 0xd4>;
> >>     };
> >>
> >>     smmu@0x70000050 {
> >>         compatible = "nvidia,tegra30-smmu";
> >>         reg = <0x70000010 0x2c 0x70000228 0x5c>;
> >>     };
> ...
> > Ok, let's start with the simplest way, and improve later with nice
> > framework.
>
> Uggh. So that patch is for the SMMU, but this thread is about the GART.
> Also, attaching patches makes it more difficult to review them, since
> them may not appear inline when reading the mail, and certainly don't
> when replying. Anyway...
>
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>
> > +#include <mach/tegra-ahb.h>
>
> Note that this patch therefor depends on the AHB series that you just
> posted, and isn't applied anywhere yet.

Right.

FYI: the AHB series:
http://article.gmane.org/gmane.linux.ports.tegra/4657


> > +static int get_reg_bank(u32 offset)
> >  {
> > -	writel(val, smmu->regs + offs);
> > +	int i;
> > +	const struct smmu_reg_bank {
> > +		u32 start;
> > +		size_t size;
> > +	} r[] = {
> > +		{
> > +			.start = 0x10,
> > +			.size = 0x2c,
> > +		},
> > +		{
> > +			.start = 0x1f0,
> > +			.size = 0x10,
> > +		},
> > +		{
> > +			.start = 0x228,
> > +			.size = 0x5c,
> > +		},
> > +	};
> > +
> > +	for (i = 0; i < ARRAY_SIZE(r); i++) {
> > +		BUG_ON(offset < r[i].start);
> > +		if (offset < r[i].start + r[i].size)
> > +			return i;
> > +	}
> > +	BUG();
> > +	return 0; /* Never reach here */
> >  }
>
> While stylistically nice, I'm not sure that will be as likely to
> optimize out as the simple if/else code I gave as an example. I suppose
> it's not a big deal though.

Ok, you meant compiler optimization. I'll compare their output and may
use the original(yours) if the original is better.

> >  static int tegra_smmu_probe(struct platform_device *pdev)
>
> > +	err = of_get_dma_window(dev->of_node, "dma-window", 0, NULL,
> > +				&base, &size);
>
> So this also relies on the generic DMA window parsing patch too. I don't
> think that's checked in anywhere yet right?

Yes. I'll ask the feedback on that patch.

> > +	size >>= SMMU_PAGE_SHIFT;
> > +	if (!size)
> > +		return -ENODEV;
>
> It seems like this should validate that !(size & ((1 << SMMU_PAGE_SHIFT)
> - 1)) too?

Yes

> This patch looks like it adds the full implementation of device tree
> support, yet doesn't add any documentation for the binding.

I'll add the doc.

> Finally, I assume you're going to post a similar patch for the Tegra20
> GART, so its register ranges don't overlap the Tegra20 MC?

Right.

Re: [PATCH v2] ARM: dt: tegra20: Add GART device

[Hiroshi Doyu]

From: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Subject: Re: [PATCH v2] ARM: dt: tegra20: Add GART device
Date: Mon, 7 May 2012 18:06:46 +0200
Message-ID: <4FA7F316.7070904-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

> On 05/07/2012 05:47 AM, Hiroshi Doyu wrote:
> > Stephen Warren wrote at Fri, 4 May 2012 19:24:45 +0200:
> ...
> >> Perhaps the simplest way is to represent those chunks as separate reg
> >> entries:
> >>
> >>     mc@7000f010 {
> >>         compatible = "nvidia,tegra30-mc";
> >>         reg = <0x7000f010 0x1d4 0x70000228 0xd4>;
> >>     };
> >>
> >>     smmu@0x70000050 {
> >>         compatible = "nvidia,tegra30-smmu";
> >>         reg = <0x70000010 0x2c 0x70000228 0x5c>;
> >>     };
> ...
> > Ok, let's start with the simplest way, and improve later with nice
> > framework.
> 
> Uggh. So that patch is for the SMMU, but this thread is about the GART.
> Also, attaching patches makes it more difficult to review them, since
> them may not appear inline when reading the mail, and certainly don't
> when replying. Anyway...
> 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> 
> > +#include <mach/tegra-ahb.h>
> 
> Note that this patch therefor depends on the AHB series that you just
> posted, and isn't applied anywhere yet.
> 
> > +static int get_reg_bank(u32 offset)
> >  {
> > -	writel(val, smmu->regs + offs);
> > +	int i;
> > +	const struct smmu_reg_bank {
> > +		u32 start;
> > +		size_t size;
> > +	} r[] = {
> > +		{
> > +			.start = 0x10,
> > +			.size = 0x2c,
> > +		},
> > +		{
> > +			.start = 0x1f0,
> > +			.size = 0x10,
> > +		},
> > +		{
> > +			.start = 0x228,
> > +			.size = 0x5c,
> > +		},
> > +	};
> > +
> > +	for (i = 0; i < ARRAY_SIZE(r); i++) {
> > +		BUG_ON(offset < r[i].start);
> > +		if (offset < r[i].start + r[i].size)
> > +			return i;
> > +	}
> > +	BUG();
> > +	return 0; /* Never reach here */
> >  }
> 
> While stylistically nice, I'm not sure that will be as likely to
> optimize out as the simple if/else code I gave as an example. I suppose
> it's not a big deal though.
> 
> >  static int tegra_smmu_probe(struct platform_device *pdev)
> 
> > +	err = of_get_dma_window(dev->of_node, "dma-window", 0, NULL,
> > +				&base, &size);
> 
> So this also relies on the generic DMA window parsing patch too. I don't
> think that's checked in anywhere yet right?

Hi Grant and Rob,

Any chance to reivew the following patch?

  http://article.gmane.org/gmane.linux.ports.tegra/4603

Attached the same one as below.

>From e9975c75f3de6902b2e76f3ad69f62bb85f3d684 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Wed, 18 Apr 2012 12:09:03 +0300
Subject: [PATCH 1/1] dt: Add general DMA window parser

This code was based on:
	"arch/microblaze/kernel/prom_parse.c"
	"arch/powerpc/kernel/prom_parse.c"

"ibm," prefix could be supported with some modification.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 drivers/of/Kconfig         |    4 ++
 drivers/of/Makefile        |    1 +
 drivers/of/of_dma.c        |   70 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_address.h |   13 ++++++++
 4 files changed, 88 insertions(+), 0 deletions(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..3b0298b 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,8 @@ config OF_MTD
 	depends on MTD
 	def_bool y
 
+config OF_DMA
+	depends on HAS_DMA
+	def_bool y
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..711ff5b 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_MTD)	+= of_mtd.o
+obj-$(CONFIG_OF_DMA)	+= of_dma.o
diff --git a/drivers/of/of_dma.c b/drivers/of/of_dma.c
new file mode 100644
index 0000000..a89fe61
--- /dev/null
+++ b/drivers/of/of_dma.c
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Based on:
+ *	"arch/microblaze/kernel/prom_parse.c"
+ *	"arch/powerpc/kernel/prom_parse.c"
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/export.h>
+#include <linux/of_address.h>
+
+int of_get_dma_window(struct device_node *dn,
+			const char *propname, int index,
+			unsigned long *busno,
+			dma_addr_t *addr, size_t *size)
+{
+	const __be32 *dma_window, *end;
+	int bytes, cur_index = 0;
+
+	if (!dn || !addr || !size)
+		return -EINVAL;
+
+	if (!propname)
+		propname = "dma-window";
+
+	dma_window = of_get_property(dn, propname, &bytes);
+	if (!dma_window)
+		return -ENODEV;
+	end = dma_window + bytes / sizeof(*dma_window);
+
+	while (dma_window < end) {
+		u32 cells;
+		const void *prop;
+
+		/* busno is always one cell */
+		if (busno)
+			*busno = be32_to_cpup(dma_window++);
+
+		prop = of_get_property(dn, "#dma-address-cells", NULL);
+		if (!prop)
+			prop = of_get_property(dn, "#address-cells", NULL);
+
+		cells = prop ? be32_to_cpup(prop) : of_n_addr_cells(dn);
+		if (!cells)
+			return -EINVAL;
+		*addr = of_read_number(dma_window, cells);
+		dma_window += cells;
+
+		prop = of_get_property(dn, "#dma-size-cells", NULL);
+		cells = prop ? be32_to_cpup(prop) : of_n_size_cells(dn);
+		if (!cells)
+			return -EINVAL;
+		*size = of_read_number(dma_window, cells);
+		dma_window += cells;
+
+		if (cur_index++ == index)
+			break;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(of_get_dma_window);
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 01b925a..9d9da9d 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -21,6 +21,11 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const u32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern int of_get_dma_window(struct device_node *dev,
+			       const char *propname, int index,
+			       unsigned long *busno,
+			       dma_addr_t *phys, size_t *size);
+
 #ifndef pci_address_to_pio
 static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
@@ -48,6 +53,14 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+
+extern int of_get_dma_window(struct device_node *dev,
+			       const char *propname, int index,
+			       unsigned long *busno,
+			       dma_addr_t *phys, size_t *size)
+{
+	return 0;
+}
 #endif /* CONFIG_OF_ADDRESS */
 
 
-- 
1.7.5.4