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

[Stephen Warren <swarren-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?

> +	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?