On 01/18/2013 02:05 AM, Hiroshi Doyu wrote:
> Stephen Warren <[email protected]> wrote @ Wed, 16 Jan 2013 22:12:24
> +0100:
>
>> On 01/15/2013 01:17 AM, Hiroshi Doyu wrote:
>>> There are 3 SMMU MMIO register blocks. They may get bigger as new
>>> Tegra SoC comes. This patch enables to support variable size of those
>>> register blocks.
>>
>> Why would the register blocks move around? In the HW, there's one single
>> chunk of memory containing all the SMMU registers, and we simply carve
>> out a few holes since some unrelated registers are stuck in the middle,
>> thus leaving us with 3 register ranges. If the size of those carved out
>> chunks changes, then doesn't that mean all the registers moved around
>> within the single chunk, and hence all the register offsets in the
>> driver become invalid?
>
> 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.
I would hope that was guaranteed; the only reason to move one of the
ranges would be a HW design change, and if there is a HW design change,
HW should take that opportunity to fix the interleaved register mess
instead of making it worse, and hence we could just have a single
register range after that point.
> 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
> should be done in the accessors as below(*1) if necessary. This way
> may sacrifice some perf because a new accessor prevents compiler
> optimization of register offset calculation, but I think that SMMU
> register accesses are not so frequent and it's acceptable in order to
> unify "tegra-smmu" over Tegra SoCs.
>
> *1:
>
> /*
> * SMMU register accessors
> */
> static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
> {
> void __iommu *addr = smmu->regbase + offs;
> #ifdef DEBUG
> int i;
>
> for (i = 0; i < smmu->num_regblks; i++) {
> BUG_ON(addr < smmu->reg[i].start);
> if (addr <= smmu->reg[i].end)
> break;
> }
> #endfi
> return readl(addr);
> }
Yes, that kind of checking looks much better if this is all going to be
dynamic.
I would suggest enabling the checking all the time rather than hiding it
inside an ifdef; how many people build that file with DEBUG enabled?
Or perhaps you could check each register in probe() somehow.
> 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
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.
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
