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

Stephen Warren <swarren@wwwdotorg.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?

From 71de7ddfed8b3341f18e97b92bdf76c193e5127a Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu@nvidia.com>
Date: Wed, 9 Jan 2013 12:38:47 +0200
Subject: [PATCH 1/1] iommu/tegra: smmu: Support variable MMIO ranges/blocks

Presently there are 3 register blocks. In the future chips over some
generations, some of register block "size" can be extended to the end
and also more new register blocks will be added, which is expected at
most a few blocks. Usually the starting address of each block won't
change. Ideally SMMU register blocks should be in one block, but it
was a bit too late to change this design(or H/W). Considering this
situation, in this driver, number of register blocks should be
allocated dynamically, based on the info from DT. Its range checks
can be done in the accessors. This way may sacrifice some perf because
a new accessors prevents compiler optimization of register offset
calculation, but I think that SMMU register accesses are not so
frequent and it's acceptable, but it's ifdef'ed out. This patch is
necessary to unify "tegra-smmu.ko" over Tegra SoCs.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 drivers/iommu/tegra-smmu.c |   62 ++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 28 deletions(-)

On 01/21/2013 12:36 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.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);
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html