On Tue, Mar 16, 2021 at 12:16:43PM +0100, Thierry Reding wrote:
> > +struct tegra_smmu_group_debug {
> > + const struct tegra_smmu_swgroup *group;
> > + void *priv;
>
> This always stores the address space, so why not make this:
>
> struct tegra_smmu_as *as;
>
> ? While at it, perhaps throw in a const to make sure we don't modify
> this structure in the debugfs code.
OK. I will try to change that.
> > @@ -334,7 +350,7 @@ static void tegra_smmu_domain_free(struct iommu_domain *domain)
> > }
> >
> > static const struct tegra_smmu_swgroup *
> > -tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> > +tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int *index)
> > {
> > const struct tegra_smmu_swgroup *group = NULL;
> > unsigned int i;
> > @@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> > for (i = 0; i < smmu->soc->num_swgroups; i++) {
> > if (smmu->soc->swgroups[i].swgroup == swgroup) {
> > group = &smmu->soc->swgroups[i];
> > + if (index)
> > + *index = i;
>
> This doesn't look like the right place for this. And this also makes
> things hard to follow because it passes out-of-band data in the index
> parameter.
>
> I'm thinking that this could benefit from a bit of refactoring where
> we could for example embed struct tegra_smmu_group_debug into struct
> tegra_smmu_group and then reference that when necessary instead of
> carrying all that data in an orthogonal array. That should also make
> it easier to track this.
>
> Come to think of it, everything that's currently in your new struct
> tegra_smmu_group_debug would be useful in struct tegra_smmu_group,
> irrespective of debugfs support.
Will try to embed it or see what I can do following the suggestion.
> > +static int tegra_smmu_mappings_show(struct seq_file *s, void *data)
> > +{
> > + seq_printf(s, "\nSWGROUP: %s\nASID: %d\nreg: 0x%x\n",
> > + group->name, as->id, group->reg);
>
> Is group->reg really that useful here?
Can drop it.
> > +
> > + smmu_writel(smmu, as->id & 0x7f, SMMU_PTB_ASID);
> > + ptb_reg = smmu_readl(smmu, SMMU_PTB_DATA);
> > +
> > + seq_printf(s, "PTB_ASID: 0x%x\nas->pd_dma: %pad\n",
> > + ptb_reg, &as->pd_dma);
>
> This looks somewhat redundant because as->pd_dma is already part of the
> PTB_ASID register value. Instead, perhaps decode the upper bits of that
> register and simply output as->pdma so that we don't duplicate the base
> address of the page table?
That's a good idea. Will change that too.
