On Thu, Oct 01, 2020 at 12:46:14PM +0200, Thierry Reding wrote:
> > > > - /*
> > > > - * This is a bit of a hack. Ideally we'd want to simply return this
> > > > - * value. However the IOMMU registration process will attempt to add
> > > > - * all devices to the IOMMU when bus_set_iommu() is called. In order
> > > > - * not to rely on global variables to track the IOMMU instance, we
> > > > - * set it here so that it can be looked up from the .probe_device()
> > > > - * callback via the IOMMU device's .drvdata field.
> > > > - */
> > > > - mc->smmu = smmu;
> > >
> > > I don't think this is going to work. I distinctly remember putting this
> > > here because we needed access to this before ->probe_device() had been
> > > called for any of the devices.
> >
> > Do you remember which exact part of code needs to access mc->smmu
> > before ->probe_device() is called?
> >
> > What I understood is that IOMMU core didn't allow ERR_PTR(-ENODEV)
> > return value from ->probe_device(), previously ->add_device(), to
> > carry on when you added this code/driver:
> > commit 8918465163171322c77a19d5258a95f56d89d2e4
> > Author: Thierry Reding <treding@xxxxxxxxxx>
> > Date: Wed Apr 16 09:24:44 2014 +0200
> > memory: Add NVIDIA Tegra memory controller support
> >
> > ..until the core had a change one year later:
> > commit 38667f18900afe172a4fe44279b132b4140f920f
> > Author: Joerg Roedel <jroedel@xxxxxxx>
> > Date: Mon Jun 29 10:16:08 2015 +0200
> > iommu: Ignore -ENODEV errors from add_device call-back
> >
> > As my commit message of this change states, ->probe_device() will
> > be called in from both bus_set_iommu() and really_probe() of each
> > device through of_iommu_configure() -- the later one initializes
> > an fwspec by polling the iommus property in the IOMMU core, same
> > as what we do here in tegra-smmu. If this works, we can probably
> > drop the hack here and get rid of tegra_smmu_configure().
>
> Looking at this a bit more, I notice that tegra_smmu_configure() does a
> lot of what's already done during of_iommu_configure(), so it'd indeed
> be nice if we could somehow get rid of that. However, like I said, I do
> recall that for DMA/IOMMU we need this prior to ->probe_device(), so it
> isn't clear to me if we can do that.
>
> So I think in order to make progress we need to check that dropping this
> does indeed still work when we enable DMA/IOMMU (and the preliminary
> patches to pass 1:1 mappings via reserved-memory regions). If so, I
> think it should be safe to remove this.
I am attaching a patch that works with both IOMMU_DOMAIN_UNMANAGED
and IOMMU_DOMAIN_DMA. Would it be possible for you to give a test?
The implementation of getting mc->smmu is using a parent_driver as
I was asking you in the other reply. Yet, it could let us give it a
try.
>From 01693c8d4af5abb38bb5ede4b22590a647909868 Mon Sep 17 00:00:00 2001
From: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
Date: Thu, 1 Oct 2020 17:51:26 -0700
Subject: [PATCH] iommu/tegra-smmu: Test
Signed-off-by: Nicolin Chen <nicoleotsuka@xxxxxxxxx>
---
drivers/iommu/tegra-smmu.c | 141 ++++++++++++-------------------------
drivers/memory/tegra/mc.c | 5 +-
include/soc/tegra/mc.h | 4 +-
3 files changed, 51 insertions(+), 99 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..ade952d3143c 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -61,6 +61,9 @@ struct tegra_smmu_as {
u32 attr;
};
+static const struct iommu_ops tegra_smmu_ops;
+static struct device_driver *parent_driver;
+
static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
{
return container_of(dom, struct tegra_smmu_as, domain);
@@ -484,60 +487,50 @@ 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;
- }
-
- of_node_put(args.np);
+ if (!fwspec)
+ 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 disable;
- tegra_smmu_enable(smmu, swgroup, as->id);
- index++;
+ tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
}
if (index == 0)
return -ENODEV;
return 0;
+
+disable:
+ while (index--) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+ tegra_smmu_as_unprepare(smmu, as);
+ }
+
+ 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)
+ 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++;
}
}
@@ -807,79 +800,40 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
}
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
+static struct tegra_smmu *tegra_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
{
- struct platform_device *pdev;
struct tegra_mc *mc;
+ struct device *dev;
+
+ if (!parent_driver || !fwnode)
+ return NULL;
- pdev = of_find_device_by_node(np);
- if (!pdev)
+ dev = driver_find_device_by_fwnode(parent_driver, fwnode);
+ if (!dev)
return NULL;
- mc = platform_get_drvdata(pdev);
+ put_device(dev);
+ mc = dev_get_drvdata(dev);
if (!mc)
return NULL;
return mc->smmu;
}
-static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
- struct of_phandle_args *args)
-{
- const struct iommu_ops *ops = smmu->iommu.ops;
- int err;
-
- err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
- if (err < 0) {
- dev_err(dev, "failed to initialize fwspec: %d\n", err);
- return err;
- }
-
- err = ops->of_xlate(dev, args);
- if (err < 0) {
- dev_err(dev, "failed to parse SW group ID: %d\n", err);
- iommu_fwspec_free(dev);
- return err;
- }
-
- return 0;
-}
-
static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
{
- struct device_node *np = dev->of_node;
- struct tegra_smmu *smmu = NULL;
- struct of_phandle_args args;
- unsigned int index = 0;
- int err;
-
- while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args) == 0) {
- smmu = tegra_smmu_find(args.np);
- if (smmu) {
- err = tegra_smmu_configure(smmu, dev, &args);
- of_node_put(args.np);
-
- if (err < 0)
- return ERR_PTR(err);
-
- /*
- * Only a single IOMMU master interface is currently
- * supported by the Linux kernel, so abort after the
- * first match.
- */
- dev_iommu_priv_set(dev, smmu);
-
- break;
- }
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct tegra_smmu *smmu;
- of_node_put(args.np);
- index++;
- }
+ if (!fwspec)
+ return ERR_PTR(-ENODEV);
+ smmu = tegra_smmu_get_by_fwnode(fwspec->iommu_fwnode);
if (!smmu)
return ERR_PTR(-ENODEV);
+ dev_iommu_priv_set(dev, smmu);
+
return &smmu->iommu;
}
@@ -1078,27 +1032,22 @@ static void tegra_smmu_debugfs_exit(struct tegra_smmu *smmu)
struct tegra_smmu *tegra_smmu_probe(struct device *dev,
const struct tegra_smmu_soc *soc,
- struct tegra_mc *mc)
+ struct tegra_mc *mc,
+ struct platform_driver *driver)
{
struct tegra_smmu *smmu;
size_t size;
u32 value;
int err;
+ if (!driver)
+ return ERR_PTR(-ENODEV);
+ parent_driver = &driver->driver;
+
smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
if (!smmu)
return ERR_PTR(-ENOMEM);
- /*
- * This is a bit of a hack. Ideally we'd want to simply return this
- * value. However the IOMMU registration process will attempt to add
- * all devices to the IOMMU when bus_set_iommu() is called. In order
- * not to rely on global variables to track the IOMMU instance, we
- * set it here so that it can be looked up from the .probe_device()
- * callback via the IOMMU device's .drvdata field.
- */
- mc->smmu = smmu;
-
size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index ec8403557ed4..586f9134c5b5 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -19,6 +19,8 @@
#include "mc.h"
+static struct platform_driver tegra_mc_driver;
+
static const struct of_device_id tegra_mc_of_match[] = {
#ifdef CONFIG_ARCH_TEGRA_2x_SOC
{ .compatible = "nvidia,tegra20-mc-gart", .data = &tegra20_mc_soc },
@@ -682,7 +684,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
err);
if (IS_ENABLED(CONFIG_TEGRA_IOMMU_SMMU) && mc->soc->smmu) {
- mc->smmu = tegra_smmu_probe(&pdev->dev, mc->soc->smmu, mc);
+ mc->smmu = tegra_smmu_probe(&pdev->dev, mc->soc->smmu, mc,
+ &tegra_mc_driver);
if (IS_ERR(mc->smmu)) {
dev_err(&pdev->dev, "failed to probe SMMU: %ld\n",
PTR_ERR(mc->smmu));
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1238e35653d1..763738f0a5ee 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -80,12 +80,12 @@ struct gart_device;
#ifdef CONFIG_TEGRA_IOMMU_SMMU
struct tegra_smmu *tegra_smmu_probe(struct device *dev,
const struct tegra_smmu_soc *soc,
- struct tegra_mc *mc);
+ struct tegra_mc *mc, struct platform_driver *driver);
void tegra_smmu_remove(struct tegra_smmu *smmu);
#else
static inline struct tegra_smmu *
tegra_smmu_probe(struct device *dev, const struct tegra_smmu_soc *soc,
- struct tegra_mc *mc)
+ struct tegra_mc *mc, struct platform_driver *driver)
{
return NULL;
}
--
2.17.1
