On Sat, Sep 26, 2020 at 01:07:17AM -0700, Nicolin Chen wrote: > The tegra_smmu_probe_device() function searches in DT for the iommu > phandler to get "smmu" pointer. This works for most of SMMU clients > that exist in the DTB. But a PCI device will not be added to iommu, > since it doesn't have a DT node. > > Fortunately, for a client with a DT node, tegra_smmu_probe_device() > calls tegra_smmu_of_xlate() via tegra_smmu_configure(), while for a > PCI device, of_pci_iommu_init() in the IOMMU core calls .of_xlate() > as well, even before running tegra_smmu_probe_device(). And in both > cases, tegra_smmu_of_xlate() prepares a valid iommu_fwspec pointer > that allows us to get the mc->smmu pointer via dev_get_drvdata() by > calling driver_find_device_by_fwnode(). > > So this patch uses iommu_fwspec in .probe_device() and related code > for a client that does not exist in the DTB, especially a PCI one. > > Signed-off-by: Nicolin Chen > --- > drivers/iommu/tegra-smmu.c | 89 +++++++++++++++++++++++--------------- > drivers/memory/tegra/mc.c | 2 +- > include/soc/tegra/mc.h | 2 + > 3 files changed, 56 insertions(+), 37 deletions(-) > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > index b10e02073610..97a7185b4578 100644 > --- a/drivers/iommu/tegra-smmu.c > +++ b/drivers/iommu/tegra-smmu.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include Why is this needed? I don't see any of the symbols declared in that file used here. > #include > > #include > @@ -61,6 +62,8 @@ struct tegra_smmu_as { > u32 attr; > }; > > +static const struct iommu_ops tegra_smmu_ops; > + > static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom) > { > return container_of(dom, struct tegra_smmu_as, domain); > @@ -484,60 +487,49 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu, > static int tegra_smmu_attach_dev(struct iommu_domain *domain, > struct device *dev) > { > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > struct tegra_smmu *smmu = dev_iommu_priv_get(dev); > struct tegra_smmu_as *as = to_smmu_as(domain); > - struct device_node *np = dev->of_node; > - struct of_phandle_args args; > - unsigned int index = 0; > - int err = 0; > - > - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index, > - &args)) { > - unsigned int swgroup = args.args[0]; > - > - if (args.np != smmu->dev->of_node) { > - of_node_put(args.np); > - continue; > - } > + int index, err = 0; > > - of_node_put(args.np); > + if (!fwspec || fwspec->ops != &tegra_smmu_ops) > + return -ENOENT; > > + for (index = 0; index < fwspec->num_ids; index++) { > err = tegra_smmu_as_prepare(smmu, as); > - if (err < 0) > - return err; > + if (err) > + goto err_disable; I'd personally drop the err_ prefix here because it's pretty obvious that we're going to do this as a result of an error happening. > > - tegra_smmu_enable(smmu, swgroup, as->id); > - index++; > + tegra_smmu_enable(smmu, fwspec->ids[index], as->id); > } > > if (index == 0) > return -ENODEV; > > return 0; > + > +err_disable: > + for (index--; index >= 0; index--) { > + tegra_smmu_disable(smmu, fwspec->ids[index], as->id); > + tegra_smmu_as_unprepare(smmu, as); > + } I think a more idiomatic version of doing this would be: while (index--) { ... } > + > + return err; > } > > static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *dev) > { > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > struct tegra_smmu_as *as = to_smmu_as(domain); > - struct device_node *np = dev->of_node; > struct tegra_smmu *smmu = as->smmu; > - struct of_phandle_args args; > unsigned int index = 0; > > - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index, > - &args)) { > - unsigned int swgroup = args.args[0]; > - > - if (args.np != smmu->dev->of_node) { > - of_node_put(args.np); > - continue; > - } > - > - of_node_put(args.np); > + if (!fwspec || fwspec->ops != &tegra_smmu_ops) > + return; > > - tegra_smmu_disable(smmu, swgroup, as->id); > + for (index = 0; index < fwspec->num_ids; index++) { > + tegra_smmu_disable(smmu, fwspec->ids[index], as->id); > tegra_smmu_as_unprepare(smmu, as); > - index++; > } > } > > @@ -845,10 +837,25 @@ static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev, > return 0; > } > > +static struct tegra_smmu *tegra_smmu_get_by_fwnode(struct fwnode_handle *fwnode) > +{ > + struct device *dev = driver_find_device_by_fwnode(&tegra_mc_driver.driver, fwnode); > + struct tegra_mc *mc; > + > + if (!dev) > + return NULL; > + > + put_device(dev); > + mc = dev_get_drvdata(dev); > + > + return mc->smmu; > +} > + As I mentioned in another reply, I think tegra_smmu_find() should be all you need in this case. > static struct iommu_device *tegra_smmu_probe_device(struct device *dev) > { > struct device_node *np = dev->of_node; > struct tegra_smmu *smmu = NULL; > + struct iommu_fwspec *fwspec; > struct of_phandle_args args; > unsigned int index = 0; > int err; > @@ -868,8 +875,6 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev) > * supported by the Linux kernel, so abort after the > * first match. > */ > - dev_iommu_priv_set(dev, smmu); > - > break; > } > > @@ -877,8 +882,20 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev) > index++; > } > > - if (!smmu) > - return ERR_PTR(-ENODEV); > + /* Any device should be able to get smmu pointer using fwspec */ > + if (!smmu) { > + fwspec = dev_iommu_fwspec_get(dev); > + if (!fwspec || fwspec->ops != &tegra_smmu_ops) > + return ERR_PTR(-ENODEV); > + > + smmu = tegra_smmu_get_by_fwnode(fwspec->iommu_fwnode); > + } > + > + /* NULL smmu pointer means that SMMU driver is not probed yet */ > + if (unlikely(!smmu)) > + return ERR_PTR(-EPROBE_DEFER); > + > + dev_iommu_priv_set(dev, smmu); Can this not be unified with the OF code above? Basically in all cases where we use Tegra SMMU, a fwnode_node should correspond 1:1 to a struct device_node, which makes the code you added here effectively the same as what's already there. Thierry