Re: [PATCH 4/6] iommu/tegra: smmu: Support variable MMIO range

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 01/21/2013 12:36 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Fri, 18 Jan 2013 17:44:13 +0100:
> 
>>> Even the checks if "offs" is in some of register blocks could be
>>> ifdef'ed out with DEBUG. "smmu->regbase" can be calculated in probe()
>>> as below. I don't think that we don't need to access "mc" DT entry to
>>> get this address. since "smmu"'s 1st reg block always starts at 0x10.
>>>
>>>   /* same as "mc"'s 1st reg block */
>>>   smmu->regbase = smmu->reg[0] & PAGE_MASK;
>>
>> I don't see regbase in the existing driver or your patch. Are you
> 
> I attached the update one below just for "regbase".
> 
>> proposing to simply make readl/writel add the offset onto a base address
>> that's calculated like that? That may not work in general; if the SMMU
>> register ranges cross a page boundary, and the various separate ranges
>> end up getting mapped to non-contiguous virtual addresses, using a
>> single base address won't work.
> 
> That's not in general, but I think that this works because the 1st
> SMMU register block offset is fixed(0x10) against MC base. If this is
> not acceptable, then, I think that SMMU driver needs to access to MC
> node to get the its base. What do you think?

>  drivers/iommu/tegra-smmu.c |   62 ++++++++++++++++++++++++--------------------
>  1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

> +#ifdef DEBUG
> +static inline void smmu_check_reg_range(size_t offs)

Like I said before, when is DEBUG defined? Rarely I suspect. It'd be
best to simply enable smmu_check_reg_range() all the time. As such ...

> +{
> +	int i;
> +
> +	for (i = 0; i < smmu->nregs; i++) {
> +		BUG_ON(offs < smmu->regs[i] - smmu->regbase);
> +		if (offs <= smmu->rege[i] - smmu->regbase)
> +			break;
> +	}
> +}
> +#else
> +static inline void smmu_check_reg_range(size_t offs) { }
> +#endif
> +
>  static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
>  {
> -	BUG_ON(offs < 0x10);
> -	if (offs < 0x3c)
> -		return readl(smmu->regs[0] + offs - 0x10);
> -	BUG_ON(offs < 0x1f0);
> -	if (offs < 0x200)
> -		return readl(smmu->regs[1] + offs - 0x1f0);
> -	BUG_ON(offs < 0x228);
> -	if (offs < 0x284)
> -		return readl(smmu->regs[2] + offs - 0x228);
> -	BUG();
> +	smmu_check_reg_range(offs);

... here, you'd be doing the loop every access anyway, so you may as
well not calculate regbase at all, move the body of
smmu_check_reg_range() into smmu_read()/smmu_write(), and do the access
inside the if statement inside the loop, with the per-range mapping.

> +	return readl(smmu->regbase + offs);
>  }