| Message ID | 1358237848-968-4-git-send-email-hdoyu@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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? It may help if you provide an explicit example of what the register layout change is... > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > static inline u32 smmu_read(struct smmu_device *smmu, size_t offs) > { > BUG_ON(offs < 0x10); > - if (offs < 0x3c) > + if (offs < 0x10 + smmu->regsz[0]) > return readl(smmu->regs[0] + offs - 0x10); > BUG_ON(offs < 0x1f0); > - if (offs < 0x200) > + if (offs < 0x1f0 + smmu->regsz[1]) Wouldn't you need to adjust that BUG_ON() in a similar way to how the if condition was adjusted? -- 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
Stephen Warren <swarren@wwwdotorg.org> 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. 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); } 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; -- 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
On 01/18/2013 02:05 AM, Hiroshi Doyu wrote: > Stephen Warren <swarren@wwwdotorg.org> 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. -- 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
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index e80312c..4f3a2a5 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -222,6 +222,7 @@ struct smmu_debugfs_info { */ struct smmu_device { void __iomem *regs[NUM_SMMU_REG_BANKS]; + resource_size_t regsz[NUM_SMMU_REG_BANKS]; unsigned long iovmm_base; /* remappable base address */ unsigned long page_count; /* total remappable size */ spinlock_t lock; @@ -257,13 +258,13 @@ static struct smmu_device *smmu_handle; /* unique for a system */ static inline u32 smmu_read(struct smmu_device *smmu, size_t offs) { BUG_ON(offs < 0x10); - if (offs < 0x3c) + if (offs < 0x10 + smmu->regsz[0]) return readl(smmu->regs[0] + offs - 0x10); BUG_ON(offs < 0x1f0); - if (offs < 0x200) + if (offs < 0x1f0 + smmu->regsz[1]) return readl(smmu->regs[1] + offs - 0x1f0); BUG_ON(offs < 0x228); - if (offs < 0x284) + if (offs < 0x228 + smmu->regsz[2]) return readl(smmu->regs[2] + offs - 0x228); BUG(); } @@ -271,17 +272,17 @@ static inline u32 smmu_read(struct smmu_device *smmu, size_t offs) static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs) { BUG_ON(offs < 0x10); - if (offs < 0x3c) { + if (offs < 0x10 + smmu->regsz[0]) { writel(val, smmu->regs[0] + offs - 0x10); return; } BUG_ON(offs < 0x1f0); - if (offs < 0x200) { + if (offs < 0x1f0 + smmu->regsz[1]) { writel(val, smmu->regs[1] + offs - 0x1f0); return; } BUG_ON(offs < 0x228); - if (offs < 0x284) { + if (offs < 0x228 + smmu->regsz[2]) { writel(val, smmu->regs[2] + offs - 0x228); return; } @@ -1117,6 +1118,7 @@ static int tegra_smmu_probe(struct platform_device *pdev) smmu->regs[i] = devm_request_and_ioremap(&pdev->dev, res); if (!smmu->regs[i]) return -EBUSY; + smmu->regsz[i] = resource_size(res); } err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base, &size);
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. Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> --- drivers/iommu/tegra-smmu.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)