Hi Mark, On Mon, Dec 8, 2014 at 3:20 PM, Mark Zhang <markz@xxxxxxxxxx> wrote: > This patch adds suspend/resume support for NVIDIA SMMU. > This patch is created on top of Thierry Reding's patch set: > > "[PATCH v7 00/12] NVIDIA Tegra memory controller and IOMMU support" You should have this comment under the "---" as we don't need it to persist once this patch is merged. > > Signed-off-by: Mark Zhang <markz@xxxxxxxxxx> > --- > drivers/iommu/tegra-smmu.c | 79 +++++++++++++++++++++++++++++++++++++++++++++- > drivers/memory/tegra/mc.c | 21 ++++++++++++ > drivers/memory/tegra/mc.h | 4 +++ > 3 files changed, 103 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > index 0909e0bae9ec..ab38805055a4 100644 > --- a/drivers/iommu/tegra-smmu.c > +++ b/drivers/iommu/tegra-smmu.c > @@ -17,6 +17,8 @@ > #include <soc/tegra/ahb.h> > #include <soc/tegra/mc.h> > > +struct tegra_smmu_as; > + > struct tegra_smmu { > void __iomem *regs; > struct device *dev; > @@ -25,9 +27,10 @@ struct tegra_smmu { > const struct tegra_smmu_soc *soc; > > unsigned long *asids; > + struct tegra_smmu_as **as; > struct mutex lock; > > - struct list_head list; > + struct list_head swgroup_asid_list; > }; > > struct tegra_smmu_as { > @@ -40,6 +43,12 @@ struct tegra_smmu_as { > u32 attr; > }; > > +struct tegra_smmu_swgroup_asid { > + struct list_head list; > + unsigned swgroup_id; > + unsigned asid; > +}; > + > static inline void smmu_writel(struct tegra_smmu *smmu, u32 value, > unsigned long offset) > { > @@ -376,6 +385,7 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu, > as->smmu = smmu; > as->use_count++; > > + smmu->as[as->id] = as; > return 0; > } > > @@ -386,6 +396,7 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu, > return; > > tegra_smmu_free_asid(smmu, as->id); > + smmu->as[as->id] = NULL; > as->smmu = NULL; > } > > @@ -398,6 +409,7 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain, > struct of_phandle_args args; > unsigned int index = 0; > int err = 0; > + struct tegra_smmu_swgroup_asid *sa = NULL; This initialization is unneeded. Actually this declaration would probably be better placed in the while() loop below since its usage is local to it. > > while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index, > &args)) { > @@ -411,6 +423,14 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain, > return err; > > tegra_smmu_enable(smmu, swgroup, as->id); > + > + sa = kzalloc(sizeof(struct tegra_smmu_swgroup_asid), > + GFP_KERNEL); > + INIT_LIST_HEAD(&sa->list); You don't need to call INIT_LIST_HEAD on this, list_add_tail() will effectively overwrite any initialization done by this macro (see include/linux/list.h). > + sa->swgroup_id = swgroup; > + sa->asid = as->id; > + list_add_tail(&sa->list, &smmu->swgroup_asid_list); > + > index++; > } > > @@ -427,6 +447,7 @@ static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *de > struct tegra_smmu *smmu = as->smmu; > struct of_phandle_args args; > unsigned int index = 0; > + struct tegra_smmu_swgroup_asid *sa = NULL; Same here, move the declaration into the while() loop and remove the initialization. > > while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index, > &args)) { > @@ -435,6 +456,13 @@ static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *de > if (args.np != smmu->dev->of_node) > continue; > > + list_for_each_entry(sa, &smmu->swgroup_asid_list, list) { > + if (sa->asid == as->id && sa->swgroup_id == swgroup) > + break; > + } > + list_del(&sa->list); > + kfree(sa); > + > tegra_smmu_disable(smmu, swgroup, as->id); > tegra_smmu_as_unprepare(smmu, as); > index++; > @@ -651,6 +679,48 @@ static void tegra_smmu_ahb_enable(void) > } > } > > +void tegra_smmu_resume(struct tegra_smmu *smmu) > +{ > + struct tegra_smmu_as *as = NULL; > + unsigned int bit; > + u32 value; > + struct tegra_smmu_swgroup_asid *sa = NULL; Again no need to initialize to NULL here. > + > + for_each_set_bit(bit, smmu->asids, smmu->soc->num_asids) { > + as = smmu->as[bit]; > + smmu->soc->ops->flush_dcache(as->pd, 0, SMMU_SIZE_PD); > + > + smmu_writel(smmu, as->id & 0x7f, SMMU_PTB_ASID); > + value = SMMU_PTB_DATA_VALUE(as->pd, as->attr); > + smmu_writel(smmu, value, SMMU_PTB_DATA); > + } > + > + list_for_each_entry(sa, &smmu->swgroup_asid_list, list) > + tegra_smmu_enable(smmu, sa->swgroup_id, sa->asid); Actually I wonder if you could not do completely without the list here. How about the following: 1) add a "unsigned swgroup_id" member to tegra_smmu_as, set it in tegra_smmu_attach_dev() instead of sa->asid 2) in the "for_each_bit" loop above, call tegra_smmu_enable(smmu, as->swgroup_id, as->id) after the last smmu_writel() Would that work? It would allow you to get rid of the tegra_smmu_swgroup_asid struct as well as a few memory allocations. I don't know if that kind of 1:1 matching between as and and swgroup makes sense for the SMMU though, maybe Thierry or Hiroshi can confirm? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html