Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-02-24 Thread Nicolin Chen
On Tue, Feb 23, 2021 at 08:10:41AM +0300, Dmitry Osipenko wrote:
> 23.02.2021 05:13, Nicolin Chen пишет:
> > Hi Dmitry,
> > 
> > On Sat, Feb 20, 2021 at 08:16:22AM +0300, Dmitry Osipenko wrote:
> >> 19.02.2021 01:07, Nicolin Chen пишет:
> >>> Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> >>> removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
> >>> of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
> >>> tegra_smmu_configure() that are typically done in the IOMMU core also.
> >>>
> >>> This approach works for both existing devices that have DT nodes and other
> >>> devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
> >>> upon testing. However, Page Fault errors are reported on tegra124-Nyan:
> >>>
> >>>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
> >>>EMEM address decode error (SMMU translation error [--S])
> >>>   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
> >>>Page fault (SMMU translation error [--S])
> >>>
> >>> After debugging, I found that the mentioned commit changed some function
> >>> callback sequence of tegra-smmu's, resulting in enabling SMMU for display
> >>> client before display driver gets initialized. I couldn't reproduce exact
> >>> same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
> >>
> >> Hello Nicolin,
> >>
> >> Could you please explain in a more details what exactly makes the
> >> difference for the callback sequence?
> > 
> > Here is a log with 5.11.0-rc6:
> > https://lava.collabora.co.uk/scheduler/job/3187849
> > [dump_stack was added in some tegra-smmu functions]
> > 
> > And here is a corresponding log with reverting the original commit:
> > https://lava.collabora.co.uk/scheduler/job/3187851
> > 
> > Here is a log with 5.11.0-rc7-next-20210210:
> > https://lava.collabora.co.uk/scheduler/job/3210245
> > 
> > And here is a corresponding log with reverting the original commit:
> > https://lava.collabora.co.uk/scheduler/job/3210596
> > 
> > Both failing logs show that mc errors started right after client DC
> > got enabled by ->attach_dev() callback that in the passing logs was
> > not called until Host1x driver init. And note that two failing logs
> > show that ->attach_dev() could be called from two different sources,
> > of_dma_configure_id() or arch_setup_dma_ops().
> > 
> > The reason why ->attach_dev() gets called is probably related to the
> > following reasons (sorry, can't be 100% sure as I don't have Tegra124
> > or other 32bit Tegra board to test):
> > 1) With the commit reverted, all clients are probed in "arch" stage,
> >which is even prior to iommu core initialization -- including it
> >setting default domain type. This probably messed up the type of
> >allocating domains against the default domain type. Also internal
> >group is somehow affected. So some condition check in iommu core
> >failed and then it bypassed ->attach_dev callback in really_probe
> >stage, until Host1x driver does attach_dev again.
> > 
> > 2) 32bit ARM has arch_setup_dma_ops() does an additional set of iommu
> >domain allocation + attach_dev(), after of_dma_configure_id() did
> >once. This isn't reproducible for me on Tegra210.
> > 
> > As debugging online isn't very efficient, and given that Thierry has
> > been working on the linear mapping of framebuffer carveout, I choose
> > to partially revert as a quick fix.
> 
> The partially revert should be okay, but it's not clear to me what makes
> difference for T124 since I don't see that problem on T30, which also
> has active display at a boot time.

Hmm..do you see ->attach_dev() is called from host1x_client_iommu_attach
or from of_dma_configure_id/arch_setup_dma_ops?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-02-22 Thread Nicolin Chen
Hi Dmitry,

On Sat, Feb 20, 2021 at 08:16:22AM +0300, Dmitry Osipenko wrote:
> 19.02.2021 01:07, Nicolin Chen пишет:
> > Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
> > removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
> > of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
> > tegra_smmu_configure() that are typically done in the IOMMU core also.
> > 
> > This approach works for both existing devices that have DT nodes and other
> > devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
> > upon testing. However, Page Fault errors are reported on tegra124-Nyan:
> > 
> >   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
> >  EMEM address decode error (SMMU translation error [--S])
> >   tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
> >  Page fault (SMMU translation error [--S])
> > 
> > After debugging, I found that the mentioned commit changed some function
> > callback sequence of tegra-smmu's, resulting in enabling SMMU for display
> > client before display driver gets initialized. I couldn't reproduce exact
> > same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.
> 
> Hello Nicolin,
> 
> Could you please explain in a more details what exactly makes the
> difference for the callback sequence?

Here is a log with 5.11.0-rc6:
https://lava.collabora.co.uk/scheduler/job/3187849
[dump_stack was added in some tegra-smmu functions]

And here is a corresponding log with reverting the original commit:
https://lava.collabora.co.uk/scheduler/job/3187851

Here is a log with 5.11.0-rc7-next-20210210:
https://lava.collabora.co.uk/scheduler/job/3210245

And here is a corresponding log with reverting the original commit:
https://lava.collabora.co.uk/scheduler/job/3210596

Both failing logs show that mc errors started right after client DC
got enabled by ->attach_dev() callback that in the passing logs was
not called until Host1x driver init. And note that two failing logs
show that ->attach_dev() could be called from two different sources,
of_dma_configure_id() or arch_setup_dma_ops().

The reason why ->attach_dev() gets called is probably related to the
following reasons (sorry, can't be 100% sure as I don't have Tegra124
or other 32bit Tegra board to test):
1) With the commit reverted, all clients are probed in "arch" stage,
   which is even prior to iommu core initialization -- including it
   setting default domain type. This probably messed up the type of
   allocating domains against the default domain type. Also internal
   group is somehow affected. So some condition check in iommu core
   failed and then it bypassed ->attach_dev callback in really_probe
   stage, until Host1x driver does attach_dev again.

2) 32bit ARM has arch_setup_dma_ops() does an additional set of iommu
   domain allocation + attach_dev(), after of_dma_configure_id() did
   once. This isn't reproducible for me on Tegra210.

As debugging online isn't very efficient, and given that Thierry has
been working on the linear mapping of framebuffer carveout, I choose
to partially revert as a quick fix.

Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

2021-02-18 Thread Nicolin Chen
Commit 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
removed certain hack in the tegra_smmu_probe() by relying on IOMMU core to
of_xlate SMMU's SID per device, so as to get rid of tegra_smmu_find() and
tegra_smmu_configure() that are typically done in the IOMMU core also.

This approach works for both existing devices that have DT nodes and other
devices (like PCI device) that don't exist in DT, on Tegra210 and Tegra3
upon testing. However, Page Fault errors are reported on tegra124-Nyan:

  tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
 EMEM address decode error (SMMU translation error [--S])
  tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
 Page fault (SMMU translation error [--S])

After debugging, I found that the mentioned commit changed some function
callback sequence of tegra-smmu's, resulting in enabling SMMU for display
client before display driver gets initialized. I couldn't reproduce exact
same issue on Tegra210 as Tegra124 (arm-32) differs at arch-level code.

Actually this Page Fault is a known issue, as on most of Tegra platforms,
display gets enabled by the bootloader for the splash screen feature, so
it keeps filling the framebuffer memory. A proper fix to this issue is to
1:1 linear map the framebuffer memory to IOVA space so the SMMU will have
the same address as the physical address in its page table. Yet, Thierry
has been working on the solution above for a year, and it hasn't merged.

Therefore, let's partially revert the mentioned commit to fix the errors.

The reason why we do a partial revert here is that we can still set priv
in ->of_xlate() callback for PCI devices. Meanwhile, devices existing in
DT, like display, will go through tegra_smmu_configure() at the stage of
bus_set_iommu() when SMMU gets probed(), as what it did before we merged
the mentioned commit.

Once we have the linear map solution for framebuffer memory, this change
can be cleaned away.

[Big thank to Guillaume who reported and helped debugging/verification]

Fixes: 25938c73cd79 ("iommu/tegra-smmu: Rework tegra_smmu_probe_device()")
Reported-by: Guillaume Tucker 
Signed-off-by: Nicolin Chen 
---

Guillaume, would you please give a "Tested-by" to this change? Thanks!

 drivers/iommu/tegra-smmu.c | 72 +-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 4a3f095a1c26..97eb62f667d2 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -798,10 +798,70 @@ 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)
+{
+   struct platform_device *pdev;
+   struct tegra_mc *mc;
+
+   pdev = of_find_device_by_node(np);
+   if (!pdev)
+   return NULL;
+
+   mc = platform_get_drvdata(pdev);
+   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, >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 tegra_smmu *smmu = dev_iommu_priv_get(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,
+ ) == 0) {
+   smmu = tegra_smmu_find(args.np);
+   if (smmu) {
+   err = tegra_smmu_configure(smmu, dev, );
+   of_node_put(args.np);
 
+   if (err < 0)
+   return ERR_PTR(err);
+
+   break;
+   }
+
+   of_node_put(args.np);
+   index++;
+   }
+
+   smmu = dev_iommu_priv_get(dev);
if (!smmu)
return ERR_PTR(-ENODEV);
 
@@ -1028,6 +1088,16 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
if (!smmu)
return ERR_PTR(-ENOMEM);
 
+   /*
+* This is a bit of a hack. Ideally we'd wan

Re: [PATCH RESEND v2 4/5] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2021-02-18 Thread Nicolin Chen
Hi Guillaume,

Thank you for the test results! And sorry for my belated reply.

On Thu, Feb 11, 2021 at 03:50:05PM +, Guillaume Tucker wrote:
> > On Sat, Feb 06, 2021 at 01:40:13PM +, Guillaume Tucker wrote:
> >>> It'd be nicer if I can get both logs of the vanilla kernel (failing)
> >>> and the commit-reverted version (passing), each applying this patch.
> >>
> >> Sure, I've run 3 jobs:
> >>
> >> * v5.11-rc6 as a reference, to see the original issue:
> >>   https://lava.collabora.co.uk/scheduler/job/3187848
> >>
> >> * + your debug patch:
> >>   https://lava.collabora.co.uk/scheduler/job/3187849
> >>
> >> * + the "breaking" commit reverted, passing the tests:
> >>   https://lava.collabora.co.uk/scheduler/job/3187851
> > 
> > Thanks for the help!
> > 
> > I am able to figure out what's probably wrong, yet not so sure
> > about the best solution at this point.
> > 
> > Would it be possible for you to run one more time with another
> > debugging patch? I'd like to see the same logs as previous:
> > 1. Vanilla kernel + debug patch
> > 2. Vanilla kernel + Reverted + debug patch
> 
> As it turns out, next-20210210 is passing all the tests again so
> it looks like this got fixed in the meantime:
> 
>   https://lava.collabora.co.uk/scheduler/job/3210192

I checked this passing log, however, found that the regression is
still there though test passed, as the prints below aren't normal:
  tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
 EMEM address decode error (SMMU translation error [--S])
  tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
 Page fault (SMMU translation error [--S])

I was trying to think of a simpler solution than a revert. However,
given the fact that the callback sequence could change -- guessing
likely a recent change in iommu core, I feel it safer to revert my
previous change, not necessarily being a complete revert though.

I attached my partial reverting change in this email. Would it be
possible for you to run one more test for me to confirm it? It'd
keep the tests passing while eliminating all error prints above.

If the fix works, I'll re-send it to mail list by adding a commit
message.

Thanks!
Nicolin
>From a9950b6e76e279f19d2c9d06aef1e222b020a9e2 Mon Sep 17 00:00:00 2001
From: Nicolin Chen 
Date: Thu, 18 Feb 2021 01:11:59 -0800
Subject: [PATCH] iommu/tegra-smmu: Fix mc errors on tegra124-nyan

  tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
	 EMEM address decode error (SMMU translation error [--S])
  tegra-mc 70019000.memory-controller: display0a: read @0xfe056b40:
	 Page fault (SMMU translation error [--S])

Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 72 +-
 1 file changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 4a3f095a1c26..97eb62f667d2 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -798,10 +798,70 @@ 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)
+{
+	struct platform_device *pdev;
+	struct tegra_mc *mc;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev)
+		return NULL;
+
+	mc = platform_get_drvdata(pdev);
+	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, >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 tegra_smmu *smmu = dev_iommu_priv_get(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,
+	  ) == 0) {
+		smmu = tegra_smmu_find(args.np);
+		if (smmu) {
+			err = tegra_smmu_configure(smmu, dev, );
+			of_node_put(args.np);
 
+			if (err < 0)
+return ERR_PTR(err);
+
+			break;
+		}
+
+		of_node_put(args.np);
+		index++;
+	}
+
+	smmu = dev_iommu_priv_get(dev);
 	if (!smmu)
 		return ERR_PTR(-ENODEV);
 
@@ -1028,6 +1088,16 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 	if (!smmu)
 		return ERR_PTR(

Re: [PATCH RESEND v2 4/5] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2021-02-10 Thread Nicolin Chen
Hi Guillaume,

On Sat, Feb 06, 2021 at 01:40:13PM +, Guillaume Tucker wrote:
> > It'd be nicer if I can get both logs of the vanilla kernel (failing)
> > and the commit-reverted version (passing), each applying this patch.
> 
> Sure, I've run 3 jobs:
> 
> * v5.11-rc6 as a reference, to see the original issue:
>   https://lava.collabora.co.uk/scheduler/job/3187848
> 
> * + your debug patch:
>   https://lava.collabora.co.uk/scheduler/job/3187849
> 
> * + the "breaking" commit reverted, passing the tests:
>   https://lava.collabora.co.uk/scheduler/job/3187851

Thanks for the help!

I am able to figure out what's probably wrong, yet not so sure
about the best solution at this point.

Would it be possible for you to run one more time with another
debugging patch? I'd like to see the same logs as previous:
1. Vanilla kernel + debug patch
2. Vanilla kernel + Reverted + debug patch

Thank you
Nicolin
>From dce84731e12900b0e98dffc0ff981638a0bb3405 Mon Sep 17 00:00:00 2001
From: Nicolin Chen 
Date: Fri, 5 Feb 2021 01:41:07 -0800
Subject: [PATCH] iommu: debug tegra-smmu v2

Signed-off-by: Nicolin Chen 
---
 arch/arm/mm/dma-mapping.c   |  6 +++--
 drivers/gpu/drm/tegra/dc.c  |  1 +
 drivers/gpu/drm/tegra/drm.c |  4 +++
 drivers/iommu/iommu.c   | 47 +++--
 drivers/iommu/tegra-smmu.c  | 31 +-
 drivers/memory/tegra/tegra124.c |  2 ++
 6 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c4b8df2ad328..6a6715817707 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2136,13 +2136,15 @@ static int __arm_iommu_attach_device(struct device *dev,
 	int err;
 
 	err = iommu_attach_device(mapping->domain, dev);
-	if (err)
+	if (err) {
+		dev_alert(dev, "--%s failed with err: %d", __func__, err);
 		return err;
+	}
 
 	kref_get(>kref);
 	to_dma_iommu_mapping(dev) = mapping;
 
-	pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev));
+	pr_alert("Attached IOMMU controller to %s device.\n", dev_name(dev));
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 0ae3a025efe9..96af9186e81a 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -2033,6 +2033,7 @@ static int tegra_dc_init(struct host1x_client *client)
 	struct drm_plane *cursor = NULL;
 	int err;
 
+	dev_alert(dc->dev, "---%s, %d\n", __func__, __LINE__);
 	/*
 	 * XXX do not register DCs with no window groups because we cannot
 	 * assign a primary plane to them, which in turn will cause KMS to
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index e9ce7d6992d2..ea6bf26f4df3 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -906,6 +906,7 @@ int host1x_client_iommu_attach(struct host1x_client *client)
 	struct iommu_group *group = NULL;
 	int err;
 
+	dev_alert(client->dev, "---%s, %d\n", __func__, __LINE__);
 	/*
 	 * If the host1x client is already attached to an IOMMU domain that is
 	 * not the shared IOMMU domain, don't try to attach it to a different
@@ -914,7 +915,9 @@ int host1x_client_iommu_attach(struct host1x_client *client)
 	if (domain && domain != tegra->domain)
 		return 0;
 
+	dev_alert(client->dev, "---%s, %d\n", __func__, __LINE__);
 	if (tegra->domain) {
+	dev_alert(client->dev, "---%s, %d\n", __func__, __LINE__);
 		group = iommu_group_get(client->dev);
 		if (!group)
 			return -ENODEV;
@@ -932,6 +935,7 @@ int host1x_client_iommu_attach(struct host1x_client *client)
 
 	client->group = group;
 
+	dev_alert(client->dev, "---%s, %d\n", __func__, __LINE__);
 	return 0;
 }
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d0b0a15dba84..890c7c7ecf94 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -220,6 +220,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group)) {
+		dev_alert(dev, "-%s, %d\n", __func__, __LINE__);
 		ret = PTR_ERR(group);
 		goto out_release;
 	}
@@ -268,7 +269,9 @@ int iommu_probe_device(struct device *dev)
 	 */
 	iommu_alloc_default_domain(group, dev);
 
+	dev_alert(dev, "-%s, %d\n", __func__, __LINE__);
 	if (group->default_domain) {
+		dev_alert(dev, "-%s, %d\n", __func__, __LINE__);
 		ret = __iommu_attach_device(group->default_domain, dev);
 		if (ret) {
 			iommu_group_put(group);
@@ -852,8 +855,11 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 
 	mutex_lock(>mutex);
 	list_add_tail(>list, >devices);
-	if (group->domain  && !iommu_is_attach_deferred(group->domain, dev))
+	d

Re: [PATCH RESEND v2 4/5] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2021-02-05 Thread Nicolin Chen
Hi Guillaume,

On Thu, Feb 04, 2021 at 09:24:23PM -0800, Nicolin Chen wrote:
> > Please let us know if you need any help debugging this issue or
> > to try a fix on this platform.
> 
> Yes, I don't have any Tegra124 platform to run. It'd be very nice
> if you can run some debugging patch (I can provide you) and a fix
> after I root cause the issue.

Would it be possible for you to run with the given debugging patch?

It'd be nicer if I can get both logs of the vanilla kernel (failing)
and the commit-reverted version (passing), each applying this patch.

Thanks in advance!
Nicolin
>From 80f288d7101101fca0412c5c200cea7e203a675d Mon Sep 17 00:00:00 2001
From: Nicolin Chen 
Date: Fri, 5 Feb 2021 01:41:07 -0800
Subject: [PATCH] iommu: debug tegra-smmu

Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 4a3f095a1c26..796b7df54b8f 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -363,6 +363,7 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
 		value |= SMMU_ASID_VALUE(asid);
 		value |= SMMU_ASID_ENABLE;
 		smmu_writel(smmu, value, group->reg);
+		pr_alert("%s, swgroup %d: writing %x to reg1 %x\n", __func__, swgroup, value, group->reg);
 	} else {
 		pr_warn("%s group from swgroup %u not found\n", __func__,
 swgroup);
@@ -379,6 +380,7 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
 		value = smmu_readl(smmu, client->smmu.reg);
 		value |= BIT(client->smmu.bit);
 		smmu_writel(smmu, value, client->smmu.reg);
+		pr_alert("%s, swgroup %d: writing %x to reg2 %x\n", __func__, swgroup, value, client->smmu.reg);
 	}
 }
 
@@ -491,13 +493,19 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 	unsigned int index;
 	int err;
 
+	dev_alert(dev, "---%s: smmu %s\n", __func__, smmu ? "valid" : "NULL");
+	dump_stack();
 	if (!fwspec)
 		return -ENOENT;
 
+	dev_alert(dev, "---%s: fwspec->num_ids %d\n", __func__, fwspec->num_ids);
 	for (index = 0; index < fwspec->num_ids; index++) {
 		err = tegra_smmu_as_prepare(smmu, as);
-		if (err)
+		if (err) {
+			dev_err(dev, "failed to prepare as(%d) for fwspec %d",
+as->id, fwspec->ids[index]);
 			goto disable;
+		}
 
 		tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
 	}
@@ -805,6 +813,8 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 	if (!smmu)
 		return ERR_PTR(-ENODEV);
 
+	dev_alert(dev, "%s, %d\n", __func__, __LINE__);
+	dump_stack();
 	return >iommu;
 }
 
@@ -904,6 +914,8 @@ static int tegra_smmu_of_xlate(struct device *dev,
 
 	dev_iommu_priv_set(dev, mc->smmu);
 
+	dev_alert(dev, "---%s: id %d", __func__, id);
+	dump_stack();
 	return iommu_fwspec_add_ids(dev, , 1);
 }
 
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH RESEND v2 4/5] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2021-02-04 Thread Nicolin Chen
Hi Guillaume,

On Thu, Feb 04, 2021 at 11:10:15AM +, Guillaume Tucker wrote:
> Hi Nicolin,
> 
> A regression was detected by kernelci.org in IGT's drm_read tests
> on mainline, it was first seen on 17th December 2020.  You can
> find some details here:
> 
>   https://kernelci.org/test/case/id/600b82dc1e3208f123d3dffc/

Thanks for reporting the issue. We did test on Tegra210 and Tegra30
yet not on Tegra124. I am wondering what could go wrong...

> Please let us know if you need any help debugging this issue or
> to try a fix on this platform.

Yes, I don't have any Tegra124 platform to run. It'd be very nice
if you can run some debugging patch (I can provide you) and a fix
after I root cause the issue.

Thanks
Nicolin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND v2 4/5] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-11-25 Thread Nicolin Chen
The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".

This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.

Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.

So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.

Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().

This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
   ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
   tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
   pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
   takes care of it.

This also fixes a problem that previously we could add clients to
iommu groups before iommu core initializes its default domain:
ubuntu@jetson:~$ dmesg | grep iommu
platform 5000.host1x: Adding to iommu group 1
platform 5700.gpu: Adding to iommu group 2
iommu: Default domain type: Translated
platform 5420.dc: Adding to iommu group 3
platform 5424.dc: Adding to iommu group 3
platform 5434.vic: Adding to iommu group 4

Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
warnings if switching to IOMMU_DOMAIN_DMA:
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA

Now, bypassing the first probe_device() call from bus_set_iommu()
fixes the sequence:
ubuntu@jetson:~$ dmesg | grep iommu
iommu: Default domain type: Translated
tegra-host1x 5000.host1x: Adding to iommu group 0
tegra-dc 5420.dc: Adding to iommu group 1
tegra-dc 5424.dc: Adding to iommu group 1
tegra-vic 5434.vic: Adding to iommu group 2
nouveau 5700.gpu: Adding to iommu group 3

Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Acked-by: Thierry Reding 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 96 ++
 1 file changed, 15 insertions(+), 81 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 297d49f3f80e..f45ed43cf8db 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ 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)
-{
-   struct platform_device *pdev;
-   struct tegra_mc *mc;
-
-   pdev = of_find_device_by_node(np);
-   if (!pdev)
-   return NULL;
-
-   mc = platform_get_drvdata(pdev);
-   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, >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

[PATCH RESEND v2 5/5] iommu/tegra-smmu: Add PCI support

2020-11-25 Thread Nicolin Chen
This patch simply adds support for PCI devices.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Acked-by: Thierry Reding 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index f45ed43cf8db..4a3f095a1c26 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
group->smmu = smmu;
group->soc = soc;
 
-   group->group = iommu_group_alloc();
+   if (dev_is_pci(dev))
+   group->group = pci_device_group(dev);
+   else
+   group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1075,22 +1080,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
 
err = iommu_device_register(>iommu);
-   if (err) {
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err)
+   goto remove_sysfs;
 
err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err < 0)
+   goto unregister;
+
+#ifdef CONFIG_PCI
+   err = bus_set_iommu(_bus_type, _smmu_ops);
+   if (err < 0)
+   goto unset_platform_bus;
+#endif
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
+
+unset_platform_bus: __maybe_unused;
+   bus_set_iommu(_bus_type, NULL);
+unregister:
+   iommu_device_unregister(>iommu);
+remove_sysfs:
+   iommu_device_sysfs_remove(>iommu);
+
+   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND v2 3/5] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-11-25 Thread Nicolin Chen
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.

So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Acked-by: Thierry Reding 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 56 --
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..297d49f3f80e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,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,
-  )) {
-   unsigned int swgroup = args.args[0];
-
-   if (args.np != smmu->dev->of_node) {
-   of_node_put(args.np);
-   continue;
-   }
+   unsigned int index;
+   int err;
 
-   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,
-  )) {
-   unsigned int swgroup = args.args[0];
+   unsigned int index;
 
-   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++;
}
 }
 
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND v2 2/5] iommu/tegra-smmu: Expand mutex protection range

2020-11-25 Thread Nicolin Chen
This is used to protect potential race condition at use_count.
since probes of client drivers, calling attach_dev(), may run
concurrently.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Acked-by: Thierry Reding 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 34 +-
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ec4c9dafff95..6a3ecc334481 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -256,26 +256,19 @@ static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, 
unsigned int *idp)
 {
unsigned long id;
 
-   mutex_lock(>lock);
-
id = find_first_zero_bit(smmu->asids, smmu->soc->num_asids);
-   if (id >= smmu->soc->num_asids) {
-   mutex_unlock(>lock);
+   if (id >= smmu->soc->num_asids)
return -ENOSPC;
-   }
 
set_bit(id, smmu->asids);
*idp = id;
 
-   mutex_unlock(>lock);
return 0;
 }
 
 static void tegra_smmu_free_asid(struct tegra_smmu *smmu, unsigned int id)
 {
-   mutex_lock(>lock);
clear_bit(id, smmu->asids);
-   mutex_unlock(>lock);
 }
 
 static bool tegra_smmu_capable(enum iommu_cap cap)
@@ -420,17 +413,21 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
 struct tegra_smmu_as *as)
 {
u32 value;
-   int err;
+   int err = 0;
+
+   mutex_lock(>lock);
 
if (as->use_count > 0) {
as->use_count++;
-   return 0;
+   goto unlock;
}
 
as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD,
  DMA_TO_DEVICE);
-   if (dma_mapping_error(smmu->dev, as->pd_dma))
-   return -ENOMEM;
+   if (dma_mapping_error(smmu->dev, as->pd_dma)) {
+   err = -ENOMEM;
+   goto unlock;
+   }
 
/* We can't handle 64-bit DMA addresses */
if (!smmu_dma_addr_valid(smmu, as->pd_dma)) {
@@ -453,24 +450,35 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
as->smmu = smmu;
as->use_count++;
 
+   mutex_unlock(>lock);
+
return 0;
 
 err_unmap:
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
+unlock:
+   mutex_unlock(>lock);
+
return err;
 }
 
 static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
struct tegra_smmu_as *as)
 {
-   if (--as->use_count > 0)
+   mutex_lock(>lock);
+
+   if (--as->use_count > 0) {
+   mutex_unlock(>lock);
return;
+   }
 
tegra_smmu_free_asid(smmu, as->id);
 
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
 
as->smmu = NULL;
+
+   mutex_unlock(>lock);
 }
 
 static int tegra_smmu_attach_dev(struct iommu_domain *domain,
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND v2 1/5] iommu/tegra-smmu: Unwrap tegra_smmu_group_get

2020-11-25 Thread Nicolin Chen
The tegra_smmu_group_get was added to group devices in different
SWGROUPs and it'd return a NULL group pointer upon a mismatch at
tegra_smmu_find_group(), so for most of clients/devices, it very
likely would mismatch and need a fallback generic_device_group().

But now tegra_smmu_group_get handles devices in same SWGROUP too,
which means that it would allocate a group for every new SWGROUP
or would directly return an existing one upon matching a SWGROUP,
i.e. any device will go through this function.

So possibility of having a NULL group pointer in device_group()
is upon failure of either devm_kzalloc() or iommu_group_alloc().
In either case, calling generic_device_group() no longer makes a
sense. Especially for devm_kzalloc() failing case, it'd cause a
problem if it fails at devm_kzalloc() yet succeeds at a fallback
generic_device_group(), because it does not create a group->list
for other devices to match.

This patch simply unwraps the function to clean it up.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Acked-by: Thierry Reding 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 0becdbfea306..ec4c9dafff95 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -903,10 +903,12 @@ static void tegra_smmu_group_release(void *iommu_data)
mutex_unlock(>lock);
 }
 
-static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
-   unsigned int swgroup)
+static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 {
+   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
const struct tegra_smmu_group_soc *soc;
+   unsigned int swgroup = fwspec->ids[0];
struct tegra_smmu_group *group;
struct iommu_group *grp;
 
@@ -950,19 +952,6 @@ static struct iommu_group *tegra_smmu_group_get(struct 
tegra_smmu *smmu,
return group->group;
 }
 
-static struct iommu_group *tegra_smmu_device_group(struct device *dev)
-{
-   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
-   struct iommu_group *group;
-
-   group = tegra_smmu_group_get(smmu, fwspec->ids[0]);
-   if (!group)
-   group = generic_device_group(dev);
-
-   return group;
-}
-
 static int tegra_smmu_of_xlate(struct device *dev,
   struct of_phandle_args *args)
 {
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND v2 0/5] iommu/tegra-smmu: Some pending reviewed changes

2020-11-25 Thread Nicolin Chen
Changelog
v1->v2:
 * Added Thierry's acks to PATCH-3~5

This is a merged set of resend for previously two series of patches
that were reviewed/acked a month ago yet have not got applied.

Series-1: https://lkml.org/lkml/2020/9/29/73
"[PATCH v4 0/2] iommu/tegra-smmu: Two followup changes"

Series-2: https://lkml.org/lkml/2020/10/9/808
"[PATCH v7 0/3] iommu/tegra-smmu: Add PCI support"

Nicolin Chen (5):
  iommu/tegra-smmu: Unwrap tegra_smmu_group_get
  iommu/tegra-smmu: Expand mutex protection range
  iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
  iommu/tegra-smmu: Rework tegra_smmu_probe_device()
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 240 ++---
 1 file changed, 88 insertions(+), 152 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH RESEND 0/5] iommu/tegra-smmu: Some pending reviewed changes

2020-11-25 Thread Nicolin Chen
On Wed, Nov 25, 2020 at 09:55:10AM +, Will Deacon wrote:
> On Tue, Nov 24, 2020 at 03:12:16PM -0800, Nicolin Chen wrote:
> > On Wed, Nov 25, 2020 at 02:05:14AM +0300, Dmitry Osipenko wrote:
> > > 25.11.2020 00:21, Nicolin Chen пишет:
> > > > These five patches were acked by Thierry and acked-n-tested by
> > > > Dmitry a while ago. Would it be possible for you to apply them?
> > > > 
> > > > Thanks!
> > > 
> > > You probably should try to ping Will Deacon.
> > > 
> > > https://lkml.org/lkml/2020/11/17/243
> > 
> > Thank you, Dmitry.
> > --
> > 
> > Will, would it be possible for you to take these changes?
> > 
> > I sent them on Nov 11 to the following lists:
> > linux-ker...@vger.kernel.org
> > iommu@lists.linux-foundation.org
> > 
> > If you need me to resend it again by adding you in To line,
> > please kindly let me know.
> 
> Weird, this ended up in my spam. If you wouldn't mind resending with me
> on To: then please do that. Then I know I'm looking at the correct version.

Resending.

Thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH RESEND 0/5] iommu/tegra-smmu: Some pending reviewed changes

2020-11-24 Thread Nicolin Chen
On Wed, Nov 25, 2020 at 02:05:14AM +0300, Dmitry Osipenko wrote:
> 25.11.2020 00:21, Nicolin Chen пишет:
> > Hi Joerg,
> > 
> > These five patches were acked by Thierry and acked-n-tested by
> > Dmitry a while ago. Would it be possible for you to apply them?
> > 
> > Thanks!
> 
> Hi,
> 
> You probably should try to ping Will Deacon.
> 
> https://lkml.org/lkml/2020/11/17/243

Thank you, Dmitry.
--

Will, would it be possible for you to take these changes?

I sent them on Nov 11 to the following lists:
linux-ker...@vger.kernel.org
iommu@lists.linux-foundation.org

If you need me to resend it again by adding you in To line,
please kindly let me know.

Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH RESEND 0/5] iommu/tegra-smmu: Some pending reviewed changes

2020-11-24 Thread Nicolin Chen
Hi Joerg,

These five patches were acked by Thierry and acked-n-tested by
Dmitry a while ago. Would it be possible for you to apply them?

Thanks!

On Wed, Nov 11, 2020 at 02:21:24PM -0800, Nicolin Chen wrote:
> This is a merged set of resend for previously two series of patches
> that were reviewed/acked a month ago yet have not got applied.
> 
> Series-1: https://lkml.org/lkml/2020/9/29/73
> "[PATCH v4 0/2] iommu/tegra-smmu: Two followup changes"
> 
> Series-2: https://lkml.org/lkml/2020/10/9/808
> "[PATCH v7 0/3] iommu/tegra-smmu: Add PCI support"
> 
> Nicolin Chen (5):
>   iommu/tegra-smmu: Unwrap tegra_smmu_group_get
>   iommu/tegra-smmu: Expand mutex protection range
>   iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
>   iommu/tegra-smmu: Rework tegra_smmu_probe_device()
>   iommu/tegra-smmu: Add PCI support
> 
>  drivers/iommu/tegra-smmu.c | 240 ++---
>  1 file changed, 88 insertions(+), 152 deletions(-)
> 
> -- 
> 2.17.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND 3/5] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-11-11 Thread Nicolin Chen
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.

So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 56 --
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..297d49f3f80e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,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,
-  )) {
-   unsigned int swgroup = args.args[0];
-
-   if (args.np != smmu->dev->of_node) {
-   of_node_put(args.np);
-   continue;
-   }
+   unsigned int index;
+   int err;
 
-   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,
-  )) {
-   unsigned int swgroup = args.args[0];
+   unsigned int index;
 
-   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++;
}
 }
 
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND 4/5] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-11-11 Thread Nicolin Chen
The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".

This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.

Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.

So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.

Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().

This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
   ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
   tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
   pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
   takes care of it.

This also fixes a problem that previously we could add clients to
iommu groups before iommu core initializes its default domain:
ubuntu@jetson:~$ dmesg | grep iommu
platform 5000.host1x: Adding to iommu group 1
platform 5700.gpu: Adding to iommu group 2
iommu: Default domain type: Translated
platform 5420.dc: Adding to iommu group 3
platform 5424.dc: Adding to iommu group 3
platform 5434.vic: Adding to iommu group 4

Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
warnings if switching to IOMMU_DOMAIN_DMA:
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA

Now, bypassing the first probe_device() call from bus_set_iommu()
fixes the sequence:
ubuntu@jetson:~$ dmesg | grep iommu
iommu: Default domain type: Translated
tegra-host1x 5000.host1x: Adding to iommu group 0
tegra-dc 5420.dc: Adding to iommu group 1
tegra-dc 5424.dc: Adding to iommu group 1
tegra-vic 5434.vic: Adding to iommu group 2
nouveau 5700.gpu: Adding to iommu group 3

Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 96 ++
 1 file changed, 15 insertions(+), 81 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 297d49f3f80e..f45ed43cf8db 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ 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)
-{
-   struct platform_device *pdev;
-   struct tegra_mc *mc;
-
-   pdev = of_find_device_by_node(np);
-   if (!pdev)
-   return NULL;
-
-   mc = platform_get_drvdata(pdev);
-   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, >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 

[PATCH RESEND 5/5] iommu/tegra-smmu: Add PCI support

2020-11-11 Thread Nicolin Chen
This patch simply adds support for PCI devices.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index f45ed43cf8db..4a3f095a1c26 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
group->smmu = smmu;
group->soc = soc;
 
-   group->group = iommu_group_alloc();
+   if (dev_is_pci(dev))
+   group->group = pci_device_group(dev);
+   else
+   group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1075,22 +1080,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
 
err = iommu_device_register(>iommu);
-   if (err) {
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err)
+   goto remove_sysfs;
 
err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err < 0)
+   goto unregister;
+
+#ifdef CONFIG_PCI
+   err = bus_set_iommu(_bus_type, _smmu_ops);
+   if (err < 0)
+   goto unset_platform_bus;
+#endif
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
+
+unset_platform_bus: __maybe_unused;
+   bus_set_iommu(_bus_type, NULL);
+unregister:
+   iommu_device_unregister(>iommu);
+remove_sysfs:
+   iommu_device_sysfs_remove(>iommu);
+
+   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND 1/5] iommu/tegra-smmu: Unwrap tegra_smmu_group_get

2020-11-11 Thread Nicolin Chen
The tegra_smmu_group_get was added to group devices in different
SWGROUPs and it'd return a NULL group pointer upon a mismatch at
tegra_smmu_find_group(), so for most of clients/devices, it very
likely would mismatch and need a fallback generic_device_group().

But now tegra_smmu_group_get handles devices in same SWGROUP too,
which means that it would allocate a group for every new SWGROUP
or would directly return an existing one upon matching a SWGROUP,
i.e. any device will go through this function.

So possibility of having a NULL group pointer in device_group()
is upon failure of either devm_kzalloc() or iommu_group_alloc().
In either case, calling generic_device_group() no longer makes a
sense. Especially for devm_kzalloc() failing case, it'd cause a
problem if it fails at devm_kzalloc() yet succeeds at a fallback
generic_device_group(), because it does not create a group->list
for other devices to match.

This patch simply unwraps the function to clean it up.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Acked-by: Thierry Reding 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 0becdbfea306..ec4c9dafff95 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -903,10 +903,12 @@ static void tegra_smmu_group_release(void *iommu_data)
mutex_unlock(>lock);
 }
 
-static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
-   unsigned int swgroup)
+static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 {
+   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
const struct tegra_smmu_group_soc *soc;
+   unsigned int swgroup = fwspec->ids[0];
struct tegra_smmu_group *group;
struct iommu_group *grp;
 
@@ -950,19 +952,6 @@ static struct iommu_group *tegra_smmu_group_get(struct 
tegra_smmu *smmu,
return group->group;
 }
 
-static struct iommu_group *tegra_smmu_device_group(struct device *dev)
-{
-   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
-   struct iommu_group *group;
-
-   group = tegra_smmu_group_get(smmu, fwspec->ids[0]);
-   if (!group)
-   group = generic_device_group(dev);
-
-   return group;
-}
-
 static int tegra_smmu_of_xlate(struct device *dev,
   struct of_phandle_args *args)
 {
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND 0/5] iommu/tegra-smmu: Some pending reviewed changes

2020-11-11 Thread Nicolin Chen
This is a merged set of resend for previously two series of patches
that were reviewed/acked a month ago yet have not got applied.

Series-1: https://lkml.org/lkml/2020/9/29/73
"[PATCH v4 0/2] iommu/tegra-smmu: Two followup changes"

Series-2: https://lkml.org/lkml/2020/10/9/808
"[PATCH v7 0/3] iommu/tegra-smmu: Add PCI support"

Nicolin Chen (5):
  iommu/tegra-smmu: Unwrap tegra_smmu_group_get
  iommu/tegra-smmu: Expand mutex protection range
  iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
  iommu/tegra-smmu: Rework tegra_smmu_probe_device()
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 240 ++---
 1 file changed, 88 insertions(+), 152 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH RESEND 2/5] iommu/tegra-smmu: Expand mutex protection range

2020-11-11 Thread Nicolin Chen
This is used to protect potential race condition at use_count.
since probes of client drivers, calling attach_dev(), may run
concurrently.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Acked-by: Thierry Reding 
Signed-off-by: Nicolin Chen 
---
 drivers/iommu/tegra-smmu.c | 34 +-
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ec4c9dafff95..6a3ecc334481 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -256,26 +256,19 @@ static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, 
unsigned int *idp)
 {
unsigned long id;
 
-   mutex_lock(>lock);
-
id = find_first_zero_bit(smmu->asids, smmu->soc->num_asids);
-   if (id >= smmu->soc->num_asids) {
-   mutex_unlock(>lock);
+   if (id >= smmu->soc->num_asids)
return -ENOSPC;
-   }
 
set_bit(id, smmu->asids);
*idp = id;
 
-   mutex_unlock(>lock);
return 0;
 }
 
 static void tegra_smmu_free_asid(struct tegra_smmu *smmu, unsigned int id)
 {
-   mutex_lock(>lock);
clear_bit(id, smmu->asids);
-   mutex_unlock(>lock);
 }
 
 static bool tegra_smmu_capable(enum iommu_cap cap)
@@ -420,17 +413,21 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
 struct tegra_smmu_as *as)
 {
u32 value;
-   int err;
+   int err = 0;
+
+   mutex_lock(>lock);
 
if (as->use_count > 0) {
as->use_count++;
-   return 0;
+   goto unlock;
}
 
as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD,
  DMA_TO_DEVICE);
-   if (dma_mapping_error(smmu->dev, as->pd_dma))
-   return -ENOMEM;
+   if (dma_mapping_error(smmu->dev, as->pd_dma)) {
+   err = -ENOMEM;
+   goto unlock;
+   }
 
/* We can't handle 64-bit DMA addresses */
if (!smmu_dma_addr_valid(smmu, as->pd_dma)) {
@@ -453,24 +450,35 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
as->smmu = smmu;
as->use_count++;
 
+   mutex_unlock(>lock);
+
return 0;
 
 err_unmap:
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
+unlock:
+   mutex_unlock(>lock);
+
return err;
 }
 
 static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
struct tegra_smmu_as *as)
 {
-   if (--as->use_count > 0)
+   mutex_lock(>lock);
+
+   if (--as->use_count > 0) {
+   mutex_unlock(>lock);
return;
+   }
 
tegra_smmu_free_asid(smmu, as->id);
 
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
 
as->smmu = NULL;
+
+   mutex_unlock(>lock);
 }
 
 static int tegra_smmu_attach_dev(struct iommu_domain *domain,
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v7 0/3] iommu/tegra-smmu: Add PCI support

2020-11-07 Thread Nicolin Chen
On Fri, Oct 09, 2020 at 09:19:33AM -0700, Nicolin Chen wrote:
> This series is to add PCI support in tegra-smmu driver.

This v7 has fixed all the existing comments and been in the
maillist for nearly a month. Can anyone please apply them?

Thanks
Nic

> Changelog (Detail in each patch)
> v6->v7
>  * Added comments for put_device in PATCH-2
>  * Renamed goto labels in PATCH-3
>  * Kept Dmitry's Reviewed-by and Tested-by as no function change
> v5->v6
>  * Dropped a NULL check, per Dmitry's comments
>  * Added Dmitry's Reviewed-by and Tested-by
> v4->v5
>  * PATCH-1 Cleaned two variables inits
>  * PATCH-2 Fixed put() in ->of_xlate() and Updated commit message
>  * PATCH-3 Added Dmitry's Reviewed-by
> v3->v4
>  * Dropped helper function, per Thierry's comments
>  * Found another way to get smmu pointer
> v2->v3
>  * Replaced with devm_tegra_get_memory_controller
>  * Updated changes by following Dmitry's comments
> v1->v2
>  * Added PATCH-1 suggested by Dmitry
>  * Reworked PATCH-2 to unify certain code
> 
> Nicolin Chen (3):
>   iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
>   iommu/tegra-smmu: Rework tegra_smmu_probe_device()
>   iommu/tegra-smmu: Add PCI support
> 
>  drivers/iommu/tegra-smmu.c | 187 +
>  1 file changed, 63 insertions(+), 124 deletions(-)
> 
> -- 
> 2.17.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v4 0/2] iommu/tegra-smmu: Two followup changes

2020-11-07 Thread Nicolin Chen
On Mon, Sep 28, 2020 at 11:13:23PM -0700, Nicolin Chen wrote:
> Two followup patches for tegra-smmu:
> PATCH-1 is a clean-up patch for the recently applied SWGROUP change.
> PATCH-2 fixes a potential race condition

These two changes have Acked-by and Reviewed-by for a month already.
Who can apply them?

Thanks
Nic

> Changelog
> v3->v4:
>  * PATCH-2: Fixed typo in subject
> v2->v3:
>  * PATCH-2: renamed "err_unlock" to "unlock"
> v1->v2:
>  * Separated first two changs of V1 so they may get applied first,
>since the other three changes need some extra time to finalize.
> 
> Nicolin Chen (2):
>   iommu/tegra-smmu: Unwrap tegra_smmu_group_get
>   iommu/tegra-smmu: Expand mutex protection range
> 
>  drivers/iommu/tegra-smmu.c | 53 ++
>  1 file changed, 25 insertions(+), 28 deletions(-)
> 
> -- 
> 2.17.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

2020-10-16 Thread Nicolin Chen
On Fri, Oct 16, 2020 at 03:10:26PM +0100, Robin Murphy wrote:
> On 2020-10-16 04:53, Nicolin Chen wrote:
> > On Thu, Oct 15, 2020 at 10:55:52AM +0100, Robin Murphy wrote:
> > > On 2020-10-15 05:13, Nicolin Chen wrote:
> > > > On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
> > > > > On 2020-10-09 17:19, Nicolin Chen wrote:
> > > > > > This patch simply adds support for PCI devices.
> > > > > > 
> > > > > > Reviewed-by: Dmitry Osipenko 
> > > > > > Tested-by: Dmitry Osipenko 
> > > > > > Signed-off-by: Nicolin Chen 
> > > > > > ---
> > > > > > 
> > > > > > Changelog
> > > > > > v6->v7
> > > > > > * Renamed goto labels, suggested by Thierry.
> > > > > > v5->v6
> > > > > > * Added Dmitry's Reviewed-by and Tested-by.
> > > > > > v4->v5
> > > > > > * Added Dmitry's Reviewed-by
> > > > > > v3->v4
> > > > > > * Dropped !iommu_present() check
> > > > > > * Added CONFIG_PCI check in the exit path
> > > > > > v2->v3
> > > > > > * Replaced ternary conditional operator with if-else in 
> > > > > > .device_group()
> > > > > > * Dropped change in tegra_smmu_remove()
> > > > > > v1->v2
> > > > > > * Added error-out labels in tegra_smmu_probe()
> > > > > > * Dropped pci_request_acs() since IOMMU core would call it.
> > > > > > 
> > > > > > drivers/iommu/tegra-smmu.c | 35 
> > > > > > +--
> > > > > > 1 file changed, 25 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > > > > > index be29f5977145..2941d6459076 100644
> > > > > > --- a/drivers/iommu/tegra-smmu.c
> > > > > > +++ b/drivers/iommu/tegra-smmu.c
> > > > > > @@ -10,6 +10,7 @@
> > > > > > #include 
> > > > > > #include 
> > > > > > #include 
> > > > > > +#include 
> > > > > > #include 
> > > > > > #include 
> > > > > > #include 
> > > > > > @@ -865,7 +866,11 @@ static struct iommu_group 
> > > > > > *tegra_smmu_device_group(struct device *dev)
> > > > > > group->smmu = smmu;
> > > > > > group->soc = soc;
> > > > > > -   group->group = iommu_group_alloc();
> > > > > > +   if (dev_is_pci(dev))
> > > > > > +   group->group = pci_device_group(dev);
> > > > > 
> > > > > Just to check, is it OK to have two or more swgroups "owning" the same
> > > > > iommu_group if an existing one gets returned here? It looks like that 
> > > > > might
> > > > > not play nice with the use of iommu_group_set_iommudata().
> > > > 
> > > > Do you mean by "gets returned here" the "IS_ERR" check below?
> > > 
> > > I mean that unlike iommu_group_alloc()/generic_device_group(),
> > > pci_device_group() may give you back a group that already contains another
> > > device and has already been set up from that device's perspective. This 
> > > can
> > > happen for topological reasons like requester ID aliasing through a 
> > > PCI-PCIe
> > > bridge or lack of isolation between functions.
> > 
> > Okay..but we don't really have two swgroups owning the same groups
> > in case of PCI devices. For Tegra210, all PCI devices inherit the
> > same swgroup from the PCI controller. And I'd think previous chips
> > do the same. The only use case currently of 2+ swgroups owning the
> > same iommu_group is for display controller.
> > 
> > Or do you suggest we need an additional check for pci_device_group?
> 
> Ah, OK - I still don't have the best comprehension of what exactly swgroups

The "swgroup" stands for "software group", which associates with
an ASID (Address Space Identifier) for SMMU to determine whether
this client is going through SMMU translation or not.

> are, and the path through .of_xlate looked like you might be using the PCI
> requester ID as the swgroup identifier, but I guess that ultimately depends
> on what your "iommu-map" is supposed to look like. If pci_device_group()

This is copied from pcie node in our downstream dtsi file:

iommus = < TEGRA_SWGROUP_AFI>;
iommu-map = <0x0  TEGRA_SWGROUP_AFI 0x1000>;
iommu-map-mask = <0x0>;

> will effectively only ever get called once regardless of how many endpoints
> exist, then indeed this won't be a concern (although if that's *guaranteed*
> to be the case then you may as well just stick with calling
> iommu_group_alloc() directly). Thanks for clarifying.

All PCI devices are supposed to get this swgroup of the pcie node
above. So the function will return the existing group of the pcie
controller, before giving a chance to call iommu_group_alloc().

Thanks for the review and input.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

2020-10-15 Thread Nicolin Chen
On Thu, Oct 15, 2020 at 10:55:52AM +0100, Robin Murphy wrote:
> On 2020-10-15 05:13, Nicolin Chen wrote:
> > On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
> > > On 2020-10-09 17:19, Nicolin Chen wrote:
> > > > This patch simply adds support for PCI devices.
> > > > 
> > > > Reviewed-by: Dmitry Osipenko 
> > > > Tested-by: Dmitry Osipenko 
> > > > Signed-off-by: Nicolin Chen 
> > > > ---
> > > > 
> > > > Changelog
> > > > v6->v7
> > > >* Renamed goto labels, suggested by Thierry.
> > > > v5->v6
> > > >* Added Dmitry's Reviewed-by and Tested-by.
> > > > v4->v5
> > > >* Added Dmitry's Reviewed-by
> > > > v3->v4
> > > >* Dropped !iommu_present() check
> > > >* Added CONFIG_PCI check in the exit path
> > > > v2->v3
> > > >* Replaced ternary conditional operator with if-else in 
> > > > .device_group()
> > > >* Dropped change in tegra_smmu_remove()
> > > > v1->v2
> > > >* Added error-out labels in tegra_smmu_probe()
> > > >* Dropped pci_request_acs() since IOMMU core would call it.
> > > > 
> > > >drivers/iommu/tegra-smmu.c | 35 +--
> > > >1 file changed, 25 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > > > index be29f5977145..2941d6459076 100644
> > > > --- a/drivers/iommu/tegra-smmu.c
> > > > +++ b/drivers/iommu/tegra-smmu.c
> > > > @@ -10,6 +10,7 @@
> > > >#include 
> > > >#include 
> > > >#include 
> > > > +#include 
> > > >#include 
> > > >#include 
> > > >#include 
> > > > @@ -865,7 +866,11 @@ static struct iommu_group 
> > > > *tegra_smmu_device_group(struct device *dev)
> > > > group->smmu = smmu;
> > > > group->soc = soc;
> > > > -   group->group = iommu_group_alloc();
> > > > +   if (dev_is_pci(dev))
> > > > +   group->group = pci_device_group(dev);
> > > 
> > > Just to check, is it OK to have two or more swgroups "owning" the same
> > > iommu_group if an existing one gets returned here? It looks like that 
> > > might
> > > not play nice with the use of iommu_group_set_iommudata().
> > 
> > Do you mean by "gets returned here" the "IS_ERR" check below?
> 
> I mean that unlike iommu_group_alloc()/generic_device_group(),
> pci_device_group() may give you back a group that already contains another
> device and has already been set up from that device's perspective. This can
> happen for topological reasons like requester ID aliasing through a PCI-PCIe
> bridge or lack of isolation between functions.

Okay..but we don't really have two swgroups owning the same groups
in case of PCI devices. For Tegra210, all PCI devices inherit the
same swgroup from the PCI controller. And I'd think previous chips
do the same. The only use case currently of 2+ swgroups owning the
same iommu_group is for display controller.

Or do you suggest we need an additional check for pci_device_group?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

2020-10-14 Thread Nicolin Chen
On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
> On 2020-10-09 17:19, Nicolin Chen wrote:
> > This patch simply adds support for PCI devices.
> > 
> > Reviewed-by: Dmitry Osipenko 
> > Tested-by: Dmitry Osipenko 
> > Signed-off-by: Nicolin Chen 
> > ---
> > 
> > Changelog
> > v6->v7
> >   * Renamed goto labels, suggested by Thierry.
> > v5->v6
> >   * Added Dmitry's Reviewed-by and Tested-by.
> > v4->v5
> >   * Added Dmitry's Reviewed-by
> > v3->v4
> >   * Dropped !iommu_present() check
> >   * Added CONFIG_PCI check in the exit path
> > v2->v3
> >   * Replaced ternary conditional operator with if-else in .device_group()
> >   * Dropped change in tegra_smmu_remove()
> > v1->v2
> >   * Added error-out labels in tegra_smmu_probe()
> >   * Dropped pci_request_acs() since IOMMU core would call it.
> > 
> >   drivers/iommu/tegra-smmu.c | 35 +--
> >   1 file changed, 25 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index be29f5977145..2941d6459076 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -10,6 +10,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -865,7 +866,11 @@ static struct iommu_group 
> > *tegra_smmu_device_group(struct device *dev)
> > group->smmu = smmu;
> > group->soc = soc;
> > -   group->group = iommu_group_alloc();
> > +   if (dev_is_pci(dev))
> > +   group->group = pci_device_group(dev);
> 
> Just to check, is it OK to have two or more swgroups "owning" the same
> iommu_group if an existing one gets returned here? It looks like that might
> not play nice with the use of iommu_group_set_iommudata().

Do you mean by "gets returned here" the "IS_ERR" check below?

> Robin.
> 
> > +   else
> > +   group->group = generic_device_group(dev);
> > +
> > if (IS_ERR(group->group)) {
> > devm_kfree(smmu->dev, group);
> > mutex_unlock(>lock);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v7 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-09 Thread Nicolin Chen
The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".

This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.

Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.

So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.

Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().

This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
   ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
   tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
   pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
   takes care of it.

This also fixes a problem that previously we could add clients to
iommu groups before iommu core initializes its default domain:
ubuntu@jetson:~$ dmesg | grep iommu
platform 5000.host1x: Adding to iommu group 1
platform 5700.gpu: Adding to iommu group 2
iommu: Default domain type: Translated
platform 5420.dc: Adding to iommu group 3
platform 5424.dc: Adding to iommu group 3
platform 5434.vic: Adding to iommu group 4

Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
warnings if switching to IOMMU_DOMAIN_DMA:
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA

Now, bypassing the first probe_device() call from bus_set_iommu()
fixes the sequence:
ubuntu@jetson:~$ dmesg | grep iommu
iommu: Default domain type: Translated
tegra-host1x 5000.host1x: Adding to iommu group 0
tegra-dc 5420.dc: Adding to iommu group 1
tegra-dc 5424.dc: Adding to iommu group 1
tegra-vic 5434.vic: Adding to iommu group 2
nouveau 5700.gpu: Adding to iommu group 3

Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---

Changelog
v6->v7
 * Added comments for put_device(), suggested by Thierry.
v5->v6
 * Dropped NULL mc/smmu pointer check, suggested by Dmitry.
 * Updated commit message with test results with vanilla DTB.
 * Added Dmitry's Reviewed-by and Tested-by.
v4->v5
 * Replaced of_node_put() with put_device() in of_xlate().
 * Added test result in commit message.
v3->v4
 * Moved dev_iommu_priv_set() to of_xlate() so we don't need
   to poll DTB for smmu pointer.
 * Removed the hack in tegra_smmu_probe() by returning ERR_PTR(
   -ENODEV) in tegra_smmu_probe_device() to let iommu core call
   in again.
 * Removed tegra_smmu_find() and tegra_smmu_configure() as iommu
   core takes care of fwspec.
v2->v3
 * Used devm_tegra_get_memory_controller() to get mc pointer
 * Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
v1->v2
 * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
   with tegra_get_memory_controller call.
 * Dropped the hack in tegra_smmu_probe().

 drivers/iommu/tegra-smmu.c | 96 ++
 1 file changed, 15 insertions(+), 81 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 297d49f3f80e..be29f5977145 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ 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 devi

[PATCH v7 0/3] iommu/tegra-smmu: Add PCI support

2020-10-09 Thread Nicolin Chen
This series is to add PCI support in tegra-smmu driver.

Changelog (Detail in each patch)
v6->v7
 * Added comments for put_device in PATCH-2
 * Renamed goto labels in PATCH-3
 * Kept Dmitry's Reviewed-by and Tested-by as no function change
v5->v6
 * Dropped a NULL check, per Dmitry's comments
 * Added Dmitry's Reviewed-by and Tested-by
v4->v5
 * PATCH-1 Cleaned two variables inits
 * PATCH-2 Fixed put() in ->of_xlate() and Updated commit message
 * PATCH-3 Added Dmitry's Reviewed-by
v3->v4
 * Dropped helper function, per Thierry's comments
 * Found another way to get smmu pointer
v2->v3
 * Replaced with devm_tegra_get_memory_controller
 * Updated changes by following Dmitry's comments
v1->v2
 * Added PATCH-1 suggested by Dmitry
 * Reworked PATCH-2 to unify certain code

Nicolin Chen (3):
  iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
  iommu/tegra-smmu: Rework tegra_smmu_probe_device()
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 187 +
 1 file changed, 63 insertions(+), 124 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v7 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-09 Thread Nicolin Chen
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.

So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---

Changelog
v6->v7:
 * No change
v5->v6:
 * Added Dmitry's Reviewed-by and Tested-by
v4->v5:
 * Removed "index" and "err" assigning to 0
v3->v4:
 * Seperated the change, as a cleanup, from the rework patch
v1->v3:
 * N/A

 drivers/iommu/tegra-smmu.c | 56 --
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..297d49f3f80e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,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,
-  )) {
-   unsigned int swgroup = args.args[0];
-
-   if (args.np != smmu->dev->of_node) {
-   of_node_put(args.np);
-   continue;
-   }
+   unsigned int index;
+   int err;
 
-   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,
-  )) {
-   unsigned int swgroup = args.args[0];
+   unsigned int index;
 
-   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++;
}
 }
 
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

2020-10-09 Thread Nicolin Chen
This patch simply adds support for PCI devices.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---

Changelog
v6->v7
 * Renamed goto labels, suggested by Thierry.
v5->v6
 * Added Dmitry's Reviewed-by and Tested-by.
v4->v5
 * Added Dmitry's Reviewed-by
v3->v4
 * Dropped !iommu_present() check
 * Added CONFIG_PCI check in the exit path
v2->v3
 * Replaced ternary conditional operator with if-else in .device_group()
 * Dropped change in tegra_smmu_remove()
v1->v2
 * Added error-out labels in tegra_smmu_probe()
 * Dropped pci_request_acs() since IOMMU core would call it.

 drivers/iommu/tegra-smmu.c | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index be29f5977145..2941d6459076 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
group->smmu = smmu;
group->soc = soc;
 
-   group->group = iommu_group_alloc();
+   if (dev_is_pci(dev))
+   group->group = pci_device_group(dev);
+   else
+   group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1075,22 +1080,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
 
err = iommu_device_register(>iommu);
-   if (err) {
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err)
+   goto remove_sysfs;
 
err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err < 0)
+   goto unregister;
+
+#ifdef CONFIG_PCI
+   err = bus_set_iommu(_bus_type, _smmu_ops);
+   if (err < 0)
+   goto unset_platform_bus;
+#endif
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
+
+unset_platform_bus: __maybe_unused;
+   bus_set_iommu(_bus_type, NULL);
+unregister:
+   iommu_device_unregister(>iommu);
+remove_sysfs:
+   iommu_device_sysfs_remove(>iommu);
+
+   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-09 Thread Nicolin Chen
On Fri, Oct 09, 2020 at 02:25:56PM +0200, Thierry Reding wrote:
> On Thu, Oct 08, 2020 at 02:12:10PM -0700, Nicolin Chen wrote:
> > On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote:
> > > On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> > > > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > > > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > > > >  static int tegra_smmu_of_xlate(struct device *dev,
> > > > > > > >struct of_phandle_args *args)
> > > > > > > >  {
> > > > > > > > +   struct platform_device *iommu_pdev = 
> > > > > > > > of_find_device_by_node(args->np);
> > > > > > > > +   struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > > > > > > u32 id = args->args[0];
> > > > > > > >  
> > > > > > > > +   of_node_put(args->np);
> > > > > > > 
> > > > > > > of_find_device_by_node() takes device reference and not the np
> > > > > > > reference. This is a bug, please remove of_node_put().
> > > > > > 
> > > > > > Looks like so. Replacing it with put_device(_pdev->dev);
> > > > > 
> > > > > Putting the put_device() here is wrong, though. You need to make sure
> > > > > you keep a reference to it as long as you keep accessing the data that
> > > > > is owned by it.
> > > > 
> > > > I am confused. You said in the other reply (to Dmitry) that we do
> > > > need to put_device(mc->dev), where mc->dev should be the same as
> > > > iommu_pdev->dev. But here your comments sounds that we should not
> > > > put_device at all since ->probe_device/group_device/attach_dev()
> > > > will use it later.
> > > 
> > > You need to call put_device() at some point to release the reference
> > > that you acquired by calling of_find_device_by_node(). If you don't
> > > release it, you're leaking the reference and the kernel isn't going to
> > > know when it's safe to delete the device.
> > > 
> > > So what I'm saying is that we either release it here, which isn't quite
> > > right because we do reference data relating to the device later on. And
> > 
> > I see. A small question here by the way: By looking at other IOMMU
> > drivers that are calling driver_find_device_by_fwnode() function,
> > I found that most of them put_device right after the function call,
> > and dev_get_drvdata() after putting the device..
> > 
> > Feels like they are doing it wrongly?
> 
> Well, like I said this is somewhat academic because these are all
> referencing the IOMMU that by definition still needs to be around
> when this code is called, and there's locks in place to ensure
> these don't go away. So it's not like these drivers are doing it
> wrong, they're just not doing it pedantically right.
> 
> > 
> > > because it isn't quite right there should be a reason to justify it,
> > > which is that the SMMU parent device is the same as the MC, so the
> > > reference count isn't strictly necessary. But that's not quite obvious,
> > > so highlighting it in a comment makes sense.
> > > 
> > > The other alternative is to not call put_device() here and keep on to
> > > the reference as long as you keep using "mc". This might be difficult to
> > > implement because it may not be obvious where to release it. I think
> > > this is the better alternative, but if it's too complicated to implement
> > > it might not be worth it.
> > 
> > I feel so too. The dev is got at of_xlate() that does not have an
> > obvious counterpart function. So I'll just remove put_device() and
> > put a line of comments, as you suggested.
> 
> I think you misunderstood. Not calling put_device() would be wrong
> because that leaks a reference to the SMMU that you can't get back. My
> suggestion was rather to keep put_device() here, but add a comment as to
> why it's okay to call the put_device() here, even though you keep using
> its private data later beyond this point, which typically would be wrong
> to do.

I see. Thanks for clarification! Will send v6 soon.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-08 Thread Nicolin Chen
On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote:
> On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > >  static int tegra_smmu_of_xlate(struct device *dev,
> > > > > >struct of_phandle_args *args)
> > > > > >  {
> > > > > > +   struct platform_device *iommu_pdev = 
> > > > > > of_find_device_by_node(args->np);
> > > > > > +   struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > > > > u32 id = args->args[0];
> > > > > >  
> > > > > > +   of_node_put(args->np);
> > > > > 
> > > > > of_find_device_by_node() takes device reference and not the np
> > > > > reference. This is a bug, please remove of_node_put().
> > > > 
> > > > Looks like so. Replacing it with put_device(_pdev->dev);
> > > 
> > > Putting the put_device() here is wrong, though. You need to make sure
> > > you keep a reference to it as long as you keep accessing the data that
> > > is owned by it.
> > 
> > I am confused. You said in the other reply (to Dmitry) that we do
> > need to put_device(mc->dev), where mc->dev should be the same as
> > iommu_pdev->dev. But here your comments sounds that we should not
> > put_device at all since ->probe_device/group_device/attach_dev()
> > will use it later.
> 
> You need to call put_device() at some point to release the reference
> that you acquired by calling of_find_device_by_node(). If you don't
> release it, you're leaking the reference and the kernel isn't going to
> know when it's safe to delete the device.
> 
> So what I'm saying is that we either release it here, which isn't quite
> right because we do reference data relating to the device later on. And

I see. A small question here by the way: By looking at other IOMMU
drivers that are calling driver_find_device_by_fwnode() function,
I found that most of them put_device right after the function call,
and dev_get_drvdata() after putting the device..

Feels like they are doing it wrongly?

> because it isn't quite right there should be a reason to justify it,
> which is that the SMMU parent device is the same as the MC, so the
> reference count isn't strictly necessary. But that's not quite obvious,
> so highlighting it in a comment makes sense.
> 
> The other alternative is to not call put_device() here and keep on to
> the reference as long as you keep using "mc". This might be difficult to
> implement because it may not be obvious where to release it. I think
> this is the better alternative, but if it's too complicated to implement
> it might not be worth it.

I feel so too. The dev is got at of_xlate() that does not have an
obvious counterpart function. So I'll just remove put_device() and
put a line of comments, as you suggested.

> > > Like I said earlier, this is a bit weird in this case because we're
> > > self-referencing, so iommu_pdev->dev is going to stay around as long as
> > > the SMMU is. However, it might be worth to properly track the lifetime
> > > anyway just so that the code can serve as a good example of how to do
> > > things.
> > 
> > What's this "track-the-lifetime"?
> 
> This basically just means that SMMU needs to ensure that MC stays alive
> (by holding a reference to it) as long as SMMU uses it. If the last
> reference to MC is dropped, then the mc pointer and potentially anything
> that it points to will become dangling. If you were to drop the last
> reference at this point, then on the next line the mc pointer could
> already be invalid.
> 
> That's how it generally works, anyway. What's special about this use-
> case is that the SMMU and MC are the same device, so it should be safe
> to omit this additional tracking because the IOMMU tracking should take
> care of that already.

Okay.

> > > If you decide to go for the shortcut and not track this reference
> > > properly, then at least you need to add a comment as to why it is safe
> > > to do in this case. This ensures that readers are away of the
> > > circumstances and don't copy this bad code into a context where the
> > > circumstances are different.
> > 
> > I don't quite get this "shortcut" here either...mind elaborating?
> 
> The shortcut is taking advantage of the knowledge that the SMMU and the
> MC are the same device and therefore not properly track the MC object.
> Given that their code is located in different locations, this isn't
> obvious to the casual reader of the code, so they may assume that this
> is the normal way to do things. To avoid that, the code should have a
> comment explaining why that is.

Got it. Thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-05 Thread Nicolin Chen
On Mon, Oct 05, 2020 at 12:56:38PM +0200, Thierry Reding wrote:
> On Mon, Oct 05, 2020 at 11:41:08AM +0300, Dmitry Osipenko wrote:
> > 05.10.2020 00:57, Nicolin Chen пишет:
> > > On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote:
> > >> 03.10.2020 09:59, Nicolin Chen пишет:
> > >>>  static int tegra_smmu_of_xlate(struct device *dev,
> > >>>struct of_phandle_args *args)
> > >>>  {
> > >>> +   struct platform_device *iommu_pdev = 
> > >>> of_find_device_by_node(args->np);
> > >>> +   struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > >>> u32 id = args->args[0];
> > >>>  
> > >>> +   put_device(_pdev->dev);
> > >>> +
> > >>> +   if (!mc || !mc->smmu)
> > >>> +   return -EPROBE_DEFER;
> > >>
> > >> I'm not very excited by seeing code in the patches that can't be
> > >> explained by the patch authors and will appreciate if you could provide
> > >> a detailed explanation about why this NULL checking is needed because I
> > >> think it is unneeded, especially given that other IOMMU drivers don't
> > >> have such check.
> > > 
> > > This function could be called from of_iommu_configure(), which is
> > > a part of other driver's probe(). So I think it's safer to have a
> > > check. Yet, given mc driver is added to the "arch_initcall" stage,
> > > you are probably right that there's no really need at this moment
> > > because all clients should be called after mc/smmu are inited. So
> > > I'll resend a v6, if that makes you happy.
> > 
> > I wanted to get the explanation :) I'm very curious why it's actually
> > needed because I'm not 100% sure whether it's not needed at all.
> > 
> > I'd assume that the only possible problem could be if some device is
> > created in parallel with the MC probing and there is no locking that
> > could prevent this in the drivers core. It's not apparent to me whether
> > this situation could happen at all in practice.
> > 
> > The MC is created early and at that time everything is sequential, so
> > it's indeed should be safe to remove the check.
> 
> I think I now remember exactly why the "hack" in tegra_smmu_probe()
> exists. The reason is that the MC driver does this:
> 
>   mc->smmu = tegra_smmu_probe(...);
> 
> That means that mc->smmu is going to be NULL until tegra_smmu_probe()
> has finished. But tegra_smmu_probe() calls bus_set_iommu() and that in
> turn calls ->probe_device(). So the purpose of the "hack" in the
> tegra_smmu_probe() function was to make sure mc->smmu was available at
> that point, because, well, it is already known, but we haven't gotten
> around to storing it yet.

Yea, that's exactly what I described in the commit message of a
later version of this patch. Yet, we can drop it now as a device
will probe() and call ->probe_device() again.

> ->of_xlate() can theoretically be called as early as right after
> bus_set_iommu() via of_iommu_configure() if that is called in parallel
> with tegra_smmu_probe(). I think that's very unlikely, but I'm not 100%
> sure that it can't happen.

As my previous reply to Dmitry above, I don't think it'd happen,
given that mc/smmu drivers are inited during arch_initcall stage
while all clients should be probed during device_initcall stage,
once this rework patch gets merged.

> In any case, I do agree with Dmitry that we should have a comment here
> explaining why this is necessary. Even if we're completely certain that
> this is necessary, it's not obvious and therefore should get that
> comment. And if we're not certain that it's necessary, it's probably
> also good to mention that in the comment so that eventually it can be
> determined or the check removed if it proves to be unnecessary.

Well, I have answered him, and also removed the mc/smmu check in
v6 already.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-05 Thread Nicolin Chen
On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > >  static int tegra_smmu_of_xlate(struct device *dev,
> > > >struct of_phandle_args *args)
> > > >  {
> > > > +   struct platform_device *iommu_pdev = 
> > > > of_find_device_by_node(args->np);
> > > > +   struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > > u32 id = args->args[0];
> > > >  
> > > > +   of_node_put(args->np);
> > > 
> > > of_find_device_by_node() takes device reference and not the np
> > > reference. This is a bug, please remove of_node_put().
> > 
> > Looks like so. Replacing it with put_device(_pdev->dev);
> 
> Putting the put_device() here is wrong, though. You need to make sure
> you keep a reference to it as long as you keep accessing the data that
> is owned by it.

I am confused. You said in the other reply (to Dmitry) that we do
need to put_device(mc->dev), where mc->dev should be the same as
iommu_pdev->dev. But here your comments sounds that we should not
put_device at all since ->probe_device/group_device/attach_dev()
will use it later.

> Like I said earlier, this is a bit weird in this case because we're
> self-referencing, so iommu_pdev->dev is going to stay around as long as
> the SMMU is. However, it might be worth to properly track the lifetime
> anyway just so that the code can serve as a good example of how to do
> things.

What's this "track-the-lifetime"?

> If you decide to go for the shortcut and not track this reference
> properly, then at least you need to add a comment as to why it is safe
> to do in this case. This ensures that readers are away of the
> circumstances and don't copy this bad code into a context where the
> circumstances are different.

I don't quite get this "shortcut" here either...mind elaborating?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/3] iommu/tegra-smmu: Add PCI support

2020-10-05 Thread Nicolin Chen
On Mon, Oct 05, 2020 at 12:04:19PM +0200, Thierry Reding wrote:
> > +err_bus_set: __maybe_unused;
> > +   bus_set_iommu(_bus_type, NULL);
> > +err_unregister:
> > +   iommu_device_unregister(>iommu);
> > +err_sysfs:
> > +   iommu_device_sysfs_remove(>iommu);
> 
> Can you please switch to label names that are more consistent with the
> others in this driver? Notably the ones in tegra_smmu_domain_alloc().
> The idea is to describe in the name of the label what's happening at the
> label. Something like this, for example:
> 
> unset_platform_bus:
>   bus_set_iommu(_bus_type, NULL);
> unregister:
>   iommu_device_unregister(>iommu);
> remove_sysfs:
>   iommu_device_sysfs_remove(>iommu);

Okay. Will update in v7.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v6 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-04 Thread Nicolin Chen
The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".

This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.

Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.

So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.

Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().

This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
   ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
   tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
   pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
   takes care of it.

This also fixes a problem that previously we could add clients to
iommu groups before iommu core initializes its default domain:
ubuntu@jetson:~$ dmesg | grep iommu
platform 5000.host1x: Adding to iommu group 1
platform 5700.gpu: Adding to iommu group 2
iommu: Default domain type: Translated
platform 5420.dc: Adding to iommu group 3
platform 5424.dc: Adding to iommu group 3
platform 5434.vic: Adding to iommu group 4

Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
warnings if switching to IOMMU_DOMAIN_DMA:
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA

Now, bypassing the first probe_device() call from bus_set_iommu()
fixes the sequence:
ubuntu@jetson:~$ dmesg | grep iommu
iommu: Default domain type: Translated
tegra-host1x 5000.host1x: Adding to iommu group 0
tegra-dc 5420.dc: Adding to iommu group 1
tegra-dc 5424.dc: Adding to iommu group 1
tegra-vic 5434.vic: Adding to iommu group 2
nouveau 5700.gpu: Adding to iommu group 3

Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---

Changelog
v5->v6
 * Dropped NULL mc/smmu pointer check, suggested by Dmitry.
 * Updated commit message with test results with vanilla DTB.
 * Added Dmitry's Reviewed-by and Tested-by.
v4->v5
 * Replaced of_node_put() with put_device() in of_xlate().
 * Added test result in commit message.
v3->v4
 * Moved dev_iommu_priv_set() to of_xlate() so we don't need
   to poll DTB for smmu pointer.
 * Removed the hack in tegra_smmu_probe() by returning ERR_PTR(
   -ENODEV) in tegra_smmu_probe_device() to let iommu core call
   in again.
 * Removed tegra_smmu_find() and tegra_smmu_configure() as iommu
   core takes care of fwspec.
v2->v3
 * Used devm_tegra_get_memory_controller() to get mc pointer
 * Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
v1->v2
 * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
   with tegra_get_memory_controller call.
 * Dropped the hack in tegra_smmu_probe().

 drivers/iommu/tegra-smmu.c | 89 --
 1 file changed, 8 insertions(+), 81 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 297d49f3f80e..c3a3131543c8 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ 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)
-{
-   struct platform_device *pdev;
-   struct tegra_m

[PATCH v6 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-04 Thread Nicolin Chen
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.

So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---

Changelog
v5->v6:
 * Added Dmitry's Reviewed-by and Tested-by
v4->v5:
 * Removed "index" and "err" assigning to 0
v3->v4:
 * Seperated the change, as a cleanup, from the rework patch
v1->v3:
 * N/A

 drivers/iommu/tegra-smmu.c | 56 --
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..297d49f3f80e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,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,
-  )) {
-   unsigned int swgroup = args.args[0];
-
-   if (args.np != smmu->dev->of_node) {
-   of_node_put(args.np);
-   continue;
-   }
+   unsigned int index;
+   int err;
 
-   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,
-  )) {
-   unsigned int swgroup = args.args[0];
+   unsigned int index;
 
-   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++;
}
 }
 
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v6 3/3] iommu/tegra-smmu: Add PCI support

2020-10-04 Thread Nicolin Chen
This patch simply adds support for PCI devices.

Reviewed-by: Dmitry Osipenko 
Tested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---

Changelog
v5->v6
 * Added Dmitry's Reviewed-by and Tested-by.
v4->v5
 * Added Dmitry's Reviewed-by
v3->v4
 * Dropped !iommu_present() check
 * Added CONFIG_PCI check in the exit path
v2->v3
 * Replaced ternary conditional operator with if-else in .device_group()
 * Dropped change in tegra_smmu_remove()
v1->v2
 * Added error-out labels in tegra_smmu_probe()
 * Dropped pci_request_acs() since IOMMU core would call it.

 drivers/iommu/tegra-smmu.c | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index c3a3131543c8..e54c006dedd1 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
group->smmu = smmu;
group->soc = soc;
 
-   group->group = iommu_group_alloc();
+   if (dev_is_pci(dev))
+   group->group = pci_device_group(dev);
+   else
+   group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1068,22 +1073,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
 
err = iommu_device_register(>iommu);
-   if (err) {
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err)
+   goto err_sysfs;
 
err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err < 0)
+   goto err_unregister;
+
+#ifdef CONFIG_PCI
+   err = bus_set_iommu(_bus_type, _smmu_ops);
+   if (err < 0)
+   goto err_bus_set;
+#endif
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
+
+err_bus_set: __maybe_unused;
+   bus_set_iommu(_bus_type, NULL);
+err_unregister:
+   iommu_device_unregister(>iommu);
+err_sysfs:
+   iommu_device_sysfs_remove(>iommu);
+
+   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v6 0/3] iommu/tegra-smmu: Add PCI support

2020-10-04 Thread Nicolin Chen
This series is to add PCI support in tegra-smmu driver.

Changelog (Detail in each patch)
v5->v6
 * Dropped a NULL check, per Dmitry's comments
 * Added Dmitry's Reviewed-by and Tested-by
v4->v5
 * PATCH-1 Cleaned two variables inits
 * PATCH-2 Fixed put() in ->of_xlate() and Updated commit message
 * PATCH-3 Added Dmitry's Reviewed-by
v3->v4
 * Dropped helper function, per Thierry's comments
 * Found another way to get smmu pointer
v2->v3
 * Replaced with devm_tegra_get_memory_controller
 * Updated changes by following Dmitry's comments
v1->v2
 * Added PATCH-1 suggested by Dmitry
 * Reworked PATCH-2 to unify certain code

Nicolin Chen (3):
  iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
  iommu/tegra-smmu: Rework tegra_smmu_probe_device()
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 180 -
 1 file changed, 56 insertions(+), 124 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support

2020-10-04 Thread Nicolin Chen
On Sat, Oct 03, 2020 at 05:16:20PM +0300, Dmitry Osipenko wrote:
> 03.10.2020 09:59, Nicolin Chen пишет:
> > This patch simply adds support for PCI devices.
> > 
> > Signed-off-by: Nicolin Chen 
> > Reviewed-by: Dmitry Osipenko 
> 
> Small nit: yours s-b tag always should be the last line of the commit
> message because you're "signing up" words that were written by you.
> 
> Tested-by: Dmitry Osipenko 

OK. Thanks for testing, btw.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-04 Thread Nicolin Chen
On Sat, Oct 03, 2020 at 05:05:52PM +0300, Dmitry Osipenko wrote:
> 03.10.2020 09:59, Nicolin Chen пишет:
> > ubuntu@jetson:~$ dmesg | grep iommu
> > iommu: Default domain type: Translated 
> > tegra-i2c 7000c400.i2c: Adding to iommu group 0
> > tegra-i2c 7000c500.i2c: Adding to iommu group 0
> > tegra-i2c 7000d000.i2c: Adding to iommu group 0
> > tegra-pcie 1003000.pcie: Adding to iommu group 1
> 
> Could you please explain how you got I2C into IOMMU?
> 
> Are you testing vanilla upstream kerne? Upstream DT doesn't assign AHB
> group to I2C controllers, nor to APB DMA controller.

I have local DT changes adding iommus property in i2c/pcie nodes
to test group_device() and pci device, yet neither of them is in
vanilla kernel. The log is merely to show to people the sequence
of iommu core prior to clients.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-04 Thread Nicolin Chen
On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote:
> 03.10.2020 09:59, Nicolin Chen пишет:
> >  static int tegra_smmu_of_xlate(struct device *dev,
> >struct of_phandle_args *args)
> >  {
> > +   struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > +   struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > u32 id = args->args[0];
> >  
> > +   put_device(_pdev->dev);
> > +
> > +   if (!mc || !mc->smmu)
> > +   return -EPROBE_DEFER;
> 
> I'm not very excited by seeing code in the patches that can't be
> explained by the patch authors and will appreciate if you could provide
> a detailed explanation about why this NULL checking is needed because I
> think it is unneeded, especially given that other IOMMU drivers don't
> have such check.

This function could be called from of_iommu_configure(), which is
a part of other driver's probe(). So I think it's safer to have a
check. Yet, given mc driver is added to the "arch_initcall" stage,
you are probably right that there's no really need at this moment
because all clients should be called after mc/smmu are inited. So
I'll resend a v6, if that makes you happy.

> I'm asking this question second time now, please don't ignore review
> comments next time.

I think I missed your reply or misunderstood it.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 0/3] iommu/tegra-smmu: Add PCI support

2020-10-03 Thread Nicolin Chen
This series is to add PCI support in tegra-smmu driver.

Changelog (Detail in each patch)
v4->v5
 * PATCH-1 Cleaned two variables inits
 * PATCH-2 Fixed put() in ->of_xlate() and Updated commit message
 * PATCH-3 Added Dmitry's Reviewed-by
v3->v4
 * Dropped helper function
 * Found another way to get smmu pointer
v2->v3
 * Replaced with devm_tegra_get_memory_controller
 * Updated changes by following Dmitry's comments
v1->v2
 * Added PATCH-1 suggested by Dmitry
 * Reworked PATCH-2 to unify certain code

Nicolin Chen (3):
  iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
  iommu/tegra-smmu: Rework tegra_smmu_probe_device()
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 183 -
 1 file changed, 59 insertions(+), 124 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v5 3/3] iommu/tegra-smmu: Add PCI support

2020-10-03 Thread Nicolin Chen
This patch simply adds support for PCI devices.

Signed-off-by: Nicolin Chen 
Reviewed-by: Dmitry Osipenko 
---

Changelog
v4->v5
 * Added Dmitry's Reviewed-by
v3->v4
 * Dropped !iommu_present() check
 * Added CONFIG_PCI check in the exit path
v2->v3
 * Replaced ternary conditional operator with if-else in .device_group()
 * Dropped change in tegra_smmu_remove()
v1->v2
 * Added error-out labels in tegra_smmu_probe()
 * Dropped pci_request_acs() since IOMMU core would call it.

 drivers/iommu/tegra-smmu.c | 35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 73b091facae0..babab6d51360 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
group->smmu = smmu;
group->soc = soc;
 
-   group->group = iommu_group_alloc();
+   if (dev_is_pci(dev))
+   group->group = pci_device_group(dev);
+   else
+   group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1071,22 +1076,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
 
err = iommu_device_register(>iommu);
-   if (err) {
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err)
+   goto err_sysfs;
 
err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err < 0)
+   goto err_unregister;
+
+#ifdef CONFIG_PCI
+   err = bus_set_iommu(_bus_type, _smmu_ops);
+   if (err < 0)
+   goto err_bus_set;
+#endif
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
+
+err_bus_set: __maybe_unused;
+   bus_set_iommu(_bus_type, NULL);
+err_unregister:
+   iommu_device_unregister(>iommu);
+err_sysfs:
+   iommu_device_sysfs_remove(>iommu);
+
+   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-03 Thread Nicolin Chen
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.

So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.

Signed-off-by: Nicolin Chen 
---

Changelog
v4->v5:
 * Removed "index" and "err" assigning to 0
v3->v4:
 * Seperated the change, as a cleanup, from the rework patch
v1->v3:
 * N/A

 drivers/iommu/tegra-smmu.c | 56 --
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..297d49f3f80e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,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,
-  )) {
-   unsigned int swgroup = args.args[0];
-
-   if (args.np != smmu->dev->of_node) {
-   of_node_put(args.np);
-   continue;
-   }
+   unsigned int index;
+   int err;
 
-   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,
-  )) {
-   unsigned int swgroup = args.args[0];
+   unsigned int index;
 
-   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++;
}
 }
 
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-03 Thread Nicolin Chen
The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".

This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.

Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.

So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.

Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().

This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
   ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
   tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
   pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
   takes care of it.

This also fixes a problem that previously we added all clients to
iommu groups before iommu core initializes its default domain:
ubuntu@jetson:~$ dmesg | grep iommu
platform smmu_benchmark: Adding to iommu group 0
platform 1003000.pcie: Adding to iommu group 1
platform 5000.host1x: Adding to iommu group 2
platform 5700.gpu: Adding to iommu group 3
platform 7000c400.i2c: Adding to iommu group 4
platform 7000c500.i2c: Adding to iommu group 4
platform 7000c700.i2c: Adding to iommu group 4
platform 7000d000.i2c: Adding to iommu group 4
iommu: Default domain type: Translated

Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
warnings if switching to IOMMU_DOMAIN_DMA:
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA
iommu: Failed to allocate default IOMMU domain of type 0 for
   group (null) - Falling back to IOMMU_DOMAIN_DMA

Now, bypassing the first probe_device() call from bus_set_iommu()
fixes the sequence:
ubuntu@jetson:~$ dmesg | grep iommu
iommu: Default domain type: Translated 
tegra-i2c 7000c400.i2c: Adding to iommu group 0
tegra-i2c 7000c500.i2c: Adding to iommu group 0
tegra-i2c 7000d000.i2c: Adding to iommu group 0
tegra-pcie 1003000.pcie: Adding to iommu group 1
...

Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.

Signed-off-by: Nicolin Chen 
---

Changelog
v4->v5
 * Replaced of_node_put() with put_device() in of_xlate()
 * Added test result in commit message
v3->v4
 * Moved dev_iommu_priv_set() to of_xlate() so we don't need
   to poll DTB for smmu pointer.
 * Removed the hack in tegra_smmu_probe() by returning ERR_PTR(
   -ENODEV) in tegra_smmu_probe_device() to let iommu core call
   in again.
 * Removed tegra_smmu_find() and tegra_smmu_configure() as iommu
   core takes care of fwspec.
v2->v3
 * Used devm_tegra_get_memory_controller() to get mc pointer
 * Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
v1->v2
 * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
   with tegra_get_memory_controller call.
 * Dropped the hack in tegra_smmu_probe().

 drivers/iommu/tegra-smmu.c | 92 +-
 1 file changed, 11 insertions(+), 81 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 297d49f3f80e..73b091facae0 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ 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)
-{
-   struct platform_device *pdev;
-   struct tegra_mc *mc;
-
-   pdev = of_find_device_by_node(np);
-   if (!pdev)
-   return NULL;
-
-   mc

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Nicolin Chen
On Fri, Oct 02, 2020 at 11:12:18PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 22:45, Nicolin Chen пишет:
> > On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
> >> 02.10.2020 09:08, Nicolin Chen пишет:
> >>>  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,
> >>> -)) {
> >>> - 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;
> >>
> >> Could the !fwspec ever be true here as well?
> > 
> > There are multiple callers of this function. It's really not that
> > straightforward to track every one of them. So I'd rather have it
> > here as other iommu drivers do. We are human beings, so we could
> > have missed something somewhere, especially callers are not from
> > tegra-* drivers.
> > 
> 
> I'm looking at the IOMMU core and it requires device to be in IOMMU
> group before attach_dev() could be called.
> 
> The group can't be assigned to device without the fwspec, see
> tegra_smmu_device_group().
>
> Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
> NULL in attach_dev(), some not checking anything, some check both and
> only arm-smmu checks the fwspec.

As I said a couple of days ago, I don't like to assume that the
callers won't change. And this time, it's from open code. So I
don't want to assume that there won't be a change.

If you are confident that there is no need to add such a check,
please send patches to remove those checks in those drivers to
see if others would agree. I would be willing to remove it after
that. Otherwise, I'd like to keep this.

Thanks for the review.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Nicolin Chen
On Fri, Oct 02, 2020 at 05:52:00PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 17:22, Dmitry Osipenko пишет:
> > 02.10.2020 09:08, Nicolin Chen пишет:
> >>  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;
> > 
> > Looks like there is no need to initialize 'index' and 'err' variables
> > anymore.
> > 
> 
> Same for tegra_smmu_detach_dev().

Can remove them.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Nicolin Chen
On Fri, Oct 02, 2020 at 05:58:29PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 17:22, Dmitry Osipenko пишет:
> > 02.10.2020 09:08, Nicolin Chen пишет:
> >> -static void tegra_smmu_release_device(struct device *dev)
> >> -{
> >> -  dev_iommu_priv_set(dev, NULL);
> >> -}
> >> +static void tegra_smmu_release_device(struct device *dev) {}
> > 
> > Please keep the braces as-is.
> > 
> 
> I noticed that you borrowed this style from the sun50i-iommu driver, but
> this is a bit unusual coding style for the c files. At least to me it's
> unusual to see header-style function stub in a middle of c file. But
> maybe it's just me.

I don't see a rule in ./Documentation/process/coding-style.rst
against this, and there're plenty of drivers doing so. If you
feel uncomfortable with this style, you may add a rule to that
doc so everyone will follow :)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Nicolin Chen
On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> >  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,
> > -  )) {
> > -   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;
> 
> Could the !fwspec ever be true here as well?

There are multiple callers of this function. It's really not that
straightforward to track every one of them. So I'd rather have it
here as other iommu drivers do. We are human beings, so we could
have missed something somewhere, especially callers are not from
tegra-* drivers.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Nicolin Chen
On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> >  static int tegra_smmu_of_xlate(struct device *dev,
> >struct of_phandle_args *args)
> >  {
> > +   struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > +   struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > u32 id = args->args[0];
> >  
> > +   of_node_put(args->np);
> 
> of_find_device_by_node() takes device reference and not the np
> reference. This is a bug, please remove of_node_put().

Looks like so. Replacing it with put_device(_pdev->dev);

Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Nicolin Chen
On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> >  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,
> > - ) == 0) {
> > -   smmu = tegra_smmu_find(args.np);
> > -   if (smmu) {
> > -   err = tegra_smmu_configure(smmu, dev, );
> > -   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;
> > -   }
> > -
> > -   of_node_put(args.np);
> > -   index++;
> > -   }
> > +   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> >  
> > if (!smmu)
> > return ERR_PTR(-ENODEV);
> 
> The !smmu can't ever be true now, isn't it? Then please remove it.

How can you be so sure? Have you read my commit message? The whole
point of removing the hack in tegra_smmu_probe() is to return the
ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function
when mc->smmu is not assigned it, as it's assigned after we return
tegra_smmu_probe() while bus_set_iommu() is still in the middle of
the tegra_smmu_probe().
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/3] iommu/tegra-smmu: Add PCI support

2020-10-02 Thread Nicolin Chen
On Fri, Oct 02, 2020 at 05:35:24PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> > @@ -865,7 +866,11 @@ static struct iommu_group 
> > *tegra_smmu_device_group(struct device *dev)
> > group->smmu = smmu;
> > group->soc = soc;
> >  
> > -   group->group = iommu_group_alloc();
> > +   if (dev_is_pci(dev))
> > +   group->group = pci_device_group(dev);
> > +   else
> > +   group->group = generic_device_group(dev);
> > +
> > if (IS_ERR(group->group)) {
> > devm_kfree(smmu->dev, group);
> > mutex_unlock(>lock);
> > @@ -1069,22 +1074,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device 
> > *dev,
> > iommu_device_set_fwnode(>iommu, dev->fwnode);
> >  
> > err = iommu_device_register(>iommu);
> > -   if (err) {
> > -   iommu_device_sysfs_remove(>iommu);
> > -   return ERR_PTR(err);
> > -   }
> > +   if (err)
> > +   goto err_sysfs;
> >  
> > err = bus_set_iommu(_bus_type, _smmu_ops);
> > -   if (err < 0) {
> > -   iommu_device_unregister(>iommu);
> > -   iommu_device_sysfs_remove(>iommu);
> > -   return ERR_PTR(err);
> > -   }
> > +   if (err < 0)
> > +   goto err_unregister;
> > +
> > +#ifdef CONFIG_PCI
> > +   err = bus_set_iommu(_bus_type, _smmu_ops);
> > +   if (err < 0)
> > +   goto err_bus_set;
> > +#endif
> >  
> > if (IS_ENABLED(CONFIG_DEBUG_FS))
> > tegra_smmu_debugfs_init(smmu);
> >  
> > return smmu;
> > +
> > +err_bus_set: __maybe_unused;
> 
> __maybe_unused?

In order to mute a build warning when CONFIG_PCI=n...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 3/3] iommu/tegra-smmu: Add PCI support

2020-10-02 Thread Nicolin Chen
This patch simply adds support for PCI devices.

Signed-off-by: Nicolin Chen 
---

Changelog
v3->v4
 * Dropped !iommu_present() check
 * Added CONFIG_PCI check in the exit path
v2->v3
 * Replaced ternary conditional operator with if-else in .device_group()
 * Dropped change in tegra_smmu_remove()
v1->v2
 * Added error-out labels in tegra_smmu_probe()
 * Dropped pci_request_acs() since IOMMU core would call it.

 drivers/iommu/tegra-smmu.c | 37 +++--
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 02d02b0c55c4..b701a7b55e84 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
group->smmu = smmu;
group->soc = soc;
 
-   group->group = iommu_group_alloc();
+   if (dev_is_pci(dev))
+   group->group = pci_device_group(dev);
+   else
+   group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1069,22 +1074,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
 
err = iommu_device_register(>iommu);
-   if (err) {
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err)
+   goto err_sysfs;
 
err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err < 0)
+   goto err_unregister;
+
+#ifdef CONFIG_PCI
+   err = bus_set_iommu(_bus_type, _smmu_ops);
+   if (err < 0)
+   goto err_bus_set;
+#endif
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
+
+err_bus_set: __maybe_unused;
+   bus_set_iommu(_bus_type, NULL);
+err_unregister:
+   iommu_device_unregister(>iommu);
+err_sysfs:
+   iommu_device_sysfs_remove(>iommu);
+
+   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

2020-10-02 Thread Nicolin Chen
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.

So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.

Signed-off-by: Nicolin Chen 
---

Changelog
v3->v4:
 * Seperated the change, as a cleanup, from the rework patch
v1->v3:
 * N/A

 drivers/iommu/tegra-smmu.c | 50 +++---
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..a573a5151c69 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,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,
-  )) {
-   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,
-  )) {
-   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++;
}
 }
 
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v4 0/3] iommu/tegra-smmu: Add PCI support

2020-10-02 Thread Nicolin Chen
This series is to add PCI support in tegra-smmu driver.

Changelog (Detail in each patch)
v3->v4
 * Dropped helper function
 * Found another way to get smmu pointer
v2->v3
 * Replaced with devm_tegra_get_memory_controller
 * Updated changes by following Dmitry's comments
v1->v2
 * Added PATCH-1 suggested by Dmitry
 * Reworked PATCH-2 to unify certain code

Nicolin Chen (3):
  iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
  iommu/tegra-smmu: Rework tegra_smmu_probe_device()
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 177 -
 1 file changed, 56 insertions(+), 121 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

2020-10-02 Thread Nicolin Chen
The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".

This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.

Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.

So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.

Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().

This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
   ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
   tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
   pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
   takes care of it.

Signed-off-by: Nicolin Chen 
---

Changelog
v3->v4
 * Moved dev_iommu_priv_set() to of_xlate() so we don't need
   to poll DTB for smmu pointer.
 * Removed the hack in tegra_smmu_probe() by returning ERR_PTR(
   -ENODEV) in tegra_smmu_probe_device() to let iommu core call
   in again.
 * Removed tegra_smmu_find() and tegra_smmu_configure() as iommu
   core takes care of fwspec.
v2->v3
 * Used devm_tegra_get_memory_controller() to get mc pointer
 * Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
v1->v2
 * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
   with tegra_get_memory_controller call.
 * Dropped the hack in tegra_smmu_probe().

 drivers/iommu/tegra-smmu.c | 90 --
 1 file changed, 9 insertions(+), 81 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index a573a5151c69..02d02b0c55c4 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ 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)
-{
-   struct platform_device *pdev;
-   struct tegra_mc *mc;
-
-   pdev = of_find_device_by_node(np);
-   if (!pdev)
-   return NULL;
-
-   mc = platform_get_drvdata(pdev);
-   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, >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,
- ) == 0) {
-   smmu = tegra_smmu_find(args.np);
-   if (smmu) {
-   err = tegra_smmu_configure(smmu, dev, );
-   of_node_put(args.np);
-
-   if (err < 0)
-   return ERR_PTR(err);
-
-   /*
-* Only a

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Nicolin Chen
On Fri, Oct 02, 2020 at 04:55:34AM +0300, Dmitry Osipenko wrote:
> 02.10.2020 04:07, Nicolin Chen пишет:
> > On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote:
> >>>>> If we can't come to an agreement on globalizing mc pointer, would
> >>>>> it be possible to pass tegra_mc_driver through tegra_smmu_probe()
> >>>>> so we can continue to use driver_find_device_by_fwnode() as v1?
> >>>>>
> >>>>> v1: https://lkml.org/lkml/2020/9/26/68
> >>>>
> >>>> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean
> >>>> tegra_smmu_probe_device()? I don't think we can do that because it isn't
> >>>
> >>> I was saying to have a global parent_driver pointer: similar to
> >>> my v1, yet rather than "extern" the tegra_mc_driver, we pass it
> >>> through egra_smmu_probe() and store it in a static global value
> >>> so as to call tegra_smmu_get_by_fwnode() in ->probe_device().
> >>>
> >>> Though I agree that creating a global device pointer (mc) might
> >>> be controversial, yet having a global parent_driver pointer may
> >>> not be against the rule, considering that it is common in iommu
> >>> drivers to call driver_find_device_by_fwnode in probe_device().
> >>
> >> You don't need the global pointer if you have SMMU OF node.
> >>
> >> You could also get driver pointer from mc->dev->driver.
> >>
> >> But I don't think you need to do this at all. The probe_device() could
> >> be invoked only for the tegra_smmu_ops and then seems you could use
> >> dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver
> >> does.
> > 
> > Getting iommu device pointer using driver_find_device_by_fwnode()
> > is a common practice in ->probe_device() of other iommu drivers.
> 
> Please give me a full list of the IOMMU drivers which use this method.
> 
> > But this requires a device_driver pointer that tegra-smmu doesn't
> > have. So passing tegra_mc_driver through tegra_smmu_probe() will
> > address it.
> > 
> 
> If you're borrowing code and ideas from other drivers, then at least
> please borrow them from a modern good-looking drivers. And I already
> pointed out that following cargo cult is not always a good idea.
> 
> ARM-SMMU isn't a modern driver and it has legacy code. You shouldn't
> copy it blindly. The sun50i-iommu driver was added half year ago, you
> may use it as a reference.

I took a closer look at sun50i-iommu driver. It's a good idea.
I think I can come up with a cleaner one. Will send v4.

Thanks for the advice.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Nicolin Chen
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 
> > 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 
> > 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 
Date: Thu, 1 Oct 2020 17:51:26 -0700
Subject: [PATCH] iommu/tegra-smmu: Test

Signed-off-by: Nicolin Chen 
---
 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,
-	   )) {
-		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

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Nicolin Chen
On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote:
> >>> If we can't come to an agreement on globalizing mc pointer, would
> >>> it be possible to pass tegra_mc_driver through tegra_smmu_probe()
> >>> so we can continue to use driver_find_device_by_fwnode() as v1?
> >>>
> >>> v1: https://lkml.org/lkml/2020/9/26/68
> >>
> >> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean
> >> tegra_smmu_probe_device()? I don't think we can do that because it isn't
> > 
> > I was saying to have a global parent_driver pointer: similar to
> > my v1, yet rather than "extern" the tegra_mc_driver, we pass it
> > through egra_smmu_probe() and store it in a static global value
> > so as to call tegra_smmu_get_by_fwnode() in ->probe_device().
> > 
> > Though I agree that creating a global device pointer (mc) might
> > be controversial, yet having a global parent_driver pointer may
> > not be against the rule, considering that it is common in iommu
> > drivers to call driver_find_device_by_fwnode in probe_device().
> 
> You don't need the global pointer if you have SMMU OF node.
> 
> You could also get driver pointer from mc->dev->driver.
> 
> But I don't think you need to do this at all. The probe_device() could
> be invoked only for the tegra_smmu_ops and then seems you could use
> dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver
> does.

Getting iommu device pointer using driver_find_device_by_fwnode()
is a common practice in ->probe_device() of other iommu drivers.
But this requires a device_driver pointer that tegra-smmu doesn't
have. So passing tegra_mc_driver through tegra_smmu_probe() will
address it.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Nicolin Chen
On Thu, Oct 01, 2020 at 12:23:16PM +0200, Thierry Reding wrote:
 > > >> It looks to me like the only reason why you need this new global 
 > > >> API is
> > > >> because PCI devices may not have a device tree node with a phandle 
> > > >> to
> > > >> the IOMMU. However, SMMU support for PCI will only be enabled if 
> > > >> the
> > > >> root complex has an iommus property, right? In that case, can't we
> > > >> simply do something like this:
> > > >>
> > > >>if (dev_is_pci(dev))
> > > >>np = find_host_bridge(dev)->of_node;
> > > >>else
> > > >>np = dev->of_node;

> > I personally am not a fan of adding a path for PCI device either,
> > since PCI/IOMMU cores could have taken care of it while the same
> > path can't be used for other buses.
> 
> There's already plenty of other drivers that do something similar to
> this. Take a look at the arm-smmu driver, for example, which seems to be
> doing exactly the same thing to finding the right device tree node to
> look at (see dev_get_dev_node() in drivers/iommu/arm-smmu/arm-smmu.c).

Hmm..okay..that is quite convincing then...

> > If we can't come to an agreement on globalizing mc pointer, would
> > it be possible to pass tegra_mc_driver through tegra_smmu_probe()
> > so we can continue to use driver_find_device_by_fwnode() as v1?
> > 
> > v1: https://lkml.org/lkml/2020/9/26/68
> 
> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean
> tegra_smmu_probe_device()? I don't think we can do that because it isn't

I was saying to have a global parent_driver pointer: similar to
my v1, yet rather than "extern" the tegra_mc_driver, we pass it
through egra_smmu_probe() and store it in a static global value
so as to call tegra_smmu_get_by_fwnode() in ->probe_device().

Though I agree that creating a global device pointer (mc) might
be controversial, yet having a global parent_driver pointer may
not be against the rule, considering that it is common in iommu
drivers to call driver_find_device_by_fwnode in probe_device().

> known at that point whether MC really is the SMMU. That's in fact the
> whole reason why we have to go through this whole dance of iterating
> over the iommus entries to find the SMMU.

Hmm..I don't quite get the meaning of:
"it isn't known at that point whether MC really is the SMMU".

Are you saying the stage of bus_set_iommu()? So because at that
point either SMMU probe() or MC probe() hasn't finished yet?

Thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-10-01 Thread Nicolin Chen
On Thu, Oct 01, 2020 at 11:51:52AM +0200, Thierry Reding wrote:
> > > >> ...
> > >  It looks to me like the only reason why you need this new global API 
> > >  is
> > >  because PCI devices may not have a device tree node with a phandle to
> > >  the IOMMU. However, SMMU support for PCI will only be enabled if the
> > >  root complex has an iommus property, right? In that case, can't we
> > >  simply do something like this:
> > > 
> > >   if (dev_is_pci(dev))
> > >   np = find_host_bridge(dev)->of_node;
> > >   else
> > >   np = dev->of_node;
> > > 
> > >  ? I'm not sure exactly what find_host_bridge() is called, but I'm 
> > >  pretty
> > >  sure that exists.

> > @@ -814,12 +815,15 @@ static struct tegra_smmu *tegra_smmu_find(struct 
> > device_node *np)
> >  }
> >  
> >  static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device 
> > *dev,
> > -   struct of_phandle_args *args)
> > +   struct of_phandle_args *args, struct 
> > fwnode_handle *fwnode)
> >  {
> > const struct iommu_ops *ops = smmu->iommu.ops;
> > int err;
> >  
> > -   err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
> > +   if (!fwnode)
> > +   return -ENOENT;
> > +
> > +   err = iommu_fwspec_init(dev, fwnode, ops);
> > if (err < 0) {
> > dev_err(dev, "failed to initialize fwspec: %d\n", err);
> > return err;
> > @@ -835,6 +839,19 @@ static int tegra_smmu_configure(struct tegra_smmu 
> > *smmu, struct device *dev,
> > return 0;
> >  }
> >  
> > +static struct device_node *tegra_smmu_find_pci_np(struct pci_dev *pci_dev)
> > +{
> > +   struct pci_bus *bus = pci_dev->bus;
> > +   struct device *dev = >dev;
> > +
> > +   while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> > +   dev = >parent->dev;
> > +   bus = bus->parent;
> > +   }
> > +
> > +   return dev->of_node;
> > +}
> 
> This seems like it's the equivalent of pci_get_host_bridge_device(). Can
> you use that instead? I think you might use the parent of the host
> bridge that's returned from that function, though.

I noticed that one when looking up one of the of_ functions, yet
also found that this pci_get_host_bridge_device() is privated by
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/pci/pci.h?id=975e1ac173058b8710e5979e97fc1397233301f3

Would PCI folks be that willing to (allow to) revert it?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Thu, Oct 01, 2020 at 05:06:19AM +0300, Dmitry Osipenko wrote:
> 01.10.2020 04:26, Nicolin Chen пишет:
> > On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
> >> 01.10.2020 00:32, Nicolin Chen пишет:
> >>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
> >>>> ...
> >>>>>> It looks to me like the only reason why you need this new global API is
> >>>>>> because PCI devices may not have a device tree node with a phandle to
> >>>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >>>>>> root complex has an iommus property, right? In that case, can't we
> >>>>>> simply do something like this:
> >>>>>>
> >>>>>>if (dev_is_pci(dev))
> >>>>>>np = find_host_bridge(dev)->of_node;
> >>>>>>else
> >>>>>>np = dev->of_node;
> >>>>>>
> >>>>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm 
> >>>>>> pretty
> >>>>>> sure that exists.
> >>>>>>
> >>>>>> Once we have that we can still iterate over the iommus property and do
> >>>>>> not need to rely on this global variable.
> >>>>>
> >>>>> I agree that it'd work. But I was hoping to simplify the code
> >>>>> here if it's possible. Looks like we have an argument on this
> >>>>> so I will choose to go with your suggestion above for now.
> >>>>
> >>>> This patch removed more lines than were added. If this will be opposite
> >>>> for the Thierry's suggestion, then it's probably not a great suggestion.
> >>>
> >>> Sorry, I don't quite understand this comments. Would you please
> >>> elaborate what's this "it" being "not a great suggestion"?
> >>>
> >>
> >> I meant that you should try to implement Thierry's solution, but if the
> >> end result will be worse than the current patch, then you shouldn't make
> >> a v4, but get back to this discussion in order to choose the best option
> >> and make everyone agree on it.
> > 
> > I see. Thanks for the reply. And here is a sample implementation:
> 
> That's what I supposed to happen :) The new variant adds code and
> complexity, while old did the opposite. Hence the old variant is clearly
> more attractive, IMO.

I personally am not a fan of adding a path for PCI device either,
since PCI/IOMMU cores could have taken care of it while the same
path can't be used for other buses.

If we can't come to an agreement on globalizing mc pointer, would
it be possible to pass tegra_mc_driver through tegra_smmu_probe()
so we can continue to use driver_find_device_by_fwnode() as v1?

v1: https://lkml.org/lkml/2020/9/26/68
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
> 01.10.2020 00:32, Nicolin Chen пишет:
> > On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
> >> ...
> >>>> It looks to me like the only reason why you need this new global API is
> >>>> because PCI devices may not have a device tree node with a phandle to
> >>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >>>> root complex has an iommus property, right? In that case, can't we
> >>>> simply do something like this:
> >>>>
> >>>>  if (dev_is_pci(dev))
> >>>>  np = find_host_bridge(dev)->of_node;
> >>>>  else
> >>>>  np = dev->of_node;
> >>>>
> >>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> >>>> sure that exists.
> >>>>
> >>>> Once we have that we can still iterate over the iommus property and do
> >>>> not need to rely on this global variable.
> >>>
> >>> I agree that it'd work. But I was hoping to simplify the code
> >>> here if it's possible. Looks like we have an argument on this
> >>> so I will choose to go with your suggestion above for now.
> >>
> >> This patch removed more lines than were added. If this will be opposite
> >> for the Thierry's suggestion, then it's probably not a great suggestion.
> > 
> > Sorry, I don't quite understand this comments. Would you please
> > elaborate what's this "it" being "not a great suggestion"?
> > 
> 
> I meant that you should try to implement Thierry's solution, but if the
> end result will be worse than the current patch, then you shouldn't make
> a v4, but get back to this discussion in order to choose the best option
> and make everyone agree on it.

I see. Thanks for the reply. And here is a sample implementation:

@@ -814,12 +815,15 @@ static struct tegra_smmu *tegra_smmu_find(struct 
device_node *np)
 }
 
 static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
-   struct of_phandle_args *args)
+   struct of_phandle_args *args, struct 
fwnode_handle *fwnode)
 {
const struct iommu_ops *ops = smmu->iommu.ops;
int err;
 
-   err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
+   if (!fwnode)
+   return -ENOENT;
+
+   err = iommu_fwspec_init(dev, fwnode, ops);
if (err < 0) {
dev_err(dev, "failed to initialize fwspec: %d\n", err);
return err;
@@ -835,6 +839,19 @@ static int tegra_smmu_configure(struct tegra_smmu *smmu, 
struct device *dev,
return 0;
 }
 
+static struct device_node *tegra_smmu_find_pci_np(struct pci_dev *pci_dev)
+{
+   struct pci_bus *bus = pci_dev->bus;
+   struct device *dev = >dev;
+
+   while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
+   dev = >parent->dev;
+   bus = bus->parent;
+   }
+
+   return dev->of_node;
+}
+
 static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 {
struct device_node *np = dev->of_node;
@@ -843,11 +860,14 @@ static struct iommu_device 
*tegra_smmu_probe_device(struct device *dev)
unsigned int index = 0;
int err;
 
+   if (dev_is_pci(dev))
+   np = tegra_smmu_find_pci_np(to_pci_dev(dev));
+
while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
  ) == 0) {
smmu = tegra_smmu_find(args.np);
if (smmu) {
-   err = tegra_smmu_configure(smmu, dev, );
+   err = tegra_smmu_configure(smmu, dev, , 
>fwnode);
of_node_put(args.np);
 
if (err < 0)

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
> ...
> >> It looks to me like the only reason why you need this new global API is
> >> because PCI devices may not have a device tree node with a phandle to
> >> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >> root complex has an iommus property, right? In that case, can't we
> >> simply do something like this:
> >>
> >>if (dev_is_pci(dev))
> >>np = find_host_bridge(dev)->of_node;
> >>else
> >>np = dev->of_node;
> >>
> >> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> >> sure that exists.
> >>
> >> Once we have that we can still iterate over the iommus property and do
> >> not need to rely on this global variable.
> > 
> > I agree that it'd work. But I was hoping to simplify the code
> > here if it's possible. Looks like we have an argument on this
> > so I will choose to go with your suggestion above for now.
> 
> This patch removed more lines than were added. If this will be opposite
> for the Thierry's suggestion, then it's probably not a great suggestion.

Sorry, I don't quite understand this comments. Would you please
elaborate what's this "it" being "not a great suggestion"?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 06:09:43PM +0300, Dmitry Osipenko wrote:
> ...
> >  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,
> > -  )) {
> > -   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 != _smmu_ops)
> > +   return -ENOENT;
> 
> In previous reply you said that these fwspec checks are borrowed from
> the arm-smmu driver, but I'm now looking at what other drivers do and I
> don't see them having this check.
> 
> Hence I'm objecting the need to have this check here. You either should
> give a rational to having the check or it should be removed.
> 
> Please never blindly copy foreign code, you should always double-check it.

I will give a test and remove it upon positive result.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 05:31:31PM +0200, Thierry Reding wrote:
> On Wed, Sep 30, 2020 at 01:42:57AM -0700, Nicolin Chen wrote:
> > Previously the driver relies on bus_set_iommu() in .probe() to call
> > in .probe_device() function so each client can poll iommus property
> > in DTB to configure fwspec via tegra_smmu_configure(). According to
> > the comments in .probe(), this is a bit of a hack. And this doesn't
> > work for a client that doesn't exist in DTB, PCI device for example.
> > 
> > Actually when a device/client gets probed, the of_iommu_configure()
> > will call in .probe_device() function again, with a prepared fwspec
> > from of_iommu_configure() that reads the SWGROUP id in DTB as we do
> > in tegra-smmu driver.
> > 
> > Additionally, as a new helper devm_tegra_get_memory_controller() is
> > introduced, there's no need to poll the iommus property in order to
> > get mc->smmu pointers or SWGROUP id.
> > 
> > This patch reworks .probe_device() and .attach_dev() by doing:
> > 1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
> > 2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
> > 3) Calling devm_tegra_get_memory_controller() in .probe_device()
> > 4) Also dropping the hack in .probe() that's no longer needed.
> > 
> > Signed-off-by: Nicolin Chen 
[...]
> >  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,
> > - ) == 0) {
> > -   smmu = tegra_smmu_find(args.np);
> > -   if (smmu) {
> > -   err = tegra_smmu_configure(smmu, dev, );
> > -   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 tegra_mc *mc = devm_tegra_get_memory_controller(dev);
> > +   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> 
> It looks to me like the only reason why you need this new global API is
> because PCI devices may not have a device tree node with a phandle to
> the IOMMU. However, SMMU support for PCI will only be enabled if the
> root complex has an iommus property, right? In that case, can't we
> simply do something like this:
> 
>   if (dev_is_pci(dev))
>   np = find_host_bridge(dev)->of_node;
>   else
>   np = dev->of_node;
> 
> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> sure that exists.
> 
> Once we have that we can still iterate over the iommus property and do
> not need to rely on this global variable.

I agree that it'd work. But I was hoping to simplify the code
here if it's possible. Looks like we have an argument on this
so I will choose to go with your suggestion above for now.

> > -   of_node_put(args.np);
> > -   index++;
> > -   }
> > +   /* An invalid mc pointer means mc and smmu drivers are not ready */
> > +   if (IS_ERR(mc))
> > +   return ERR_PTR(-EPROBE_DEFER);
> >  
> > -   if (!smmu)
> > +   /*
> > +* IOMMU core allows -ENODEV return to carry on. So bypass any call
> > +* from bus_set_iommu() during tegra_smmu_probe(), as a device will
> > +* call in again via of_iommu_configure when fwspec is prepared.
> > +*/
> > +   if (!mc->smmu || !fwspec || fwspec->ops != _smmu_ops)
> > return ERR_PTR(-ENODEV);
> >  
> > -   return >iommu;
> > +   dev_iommu_priv_set(dev, mc->smmu);
> > +
> > +   return >smmu->iommu;
> >  }
> >  
> >  static void tegra_smmu_release_device(struct device *dev)
> > @@ -1089,16 +1027,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device 
> > *dev,
> > if (!smmu)
> > return ERR_PTR(-ENOMEM);
> >  
> > -   /*
> > -* This is a bit of a hack. Ideally we'd want to simply return this
> > -* value. However 

Re: [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 05:53:20PM +0300, Dmitry Osipenko wrote:
> ...
> > +#ifdef CONFIG_PCI
> > +   if (!iommu_present(_bus_type)) {
> 
> 
> In the previous reply you said that you're borrowing this check from the
> arm-smmu driver, but arm-smmu also has a similar check for
> platform_bus_type, while Tegra SMMU driver doesn't have it. Hence I'm
> objecting the necessity of having this check.
> 
> Please give a rationale for having this check in the code. And please
> note that cargo cult isn't a rationale to me.

Okay, okayI will remove it upon testing.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 11:07:32AM +0200, Krzysztof Kozlowski wrote:
> "On Wed, 30 Sep 2020 at 10:48, Nicolin Chen  wrote:
> >
> > From: Dmitry Osipenko 
> >
> > Multiple Tegra drivers need to retrieve Memory Controller and hence there
> > is quite some duplication of the retrieval code among the drivers. Let's
> > add a new common helper for the retrieval of the MC.
> >
> > Signed-off-by: Dmitry Osipenko 
> > Signed-off-by: Nicolin Chen 
> > ---
> >
> > Changelog
> > v2->v3:
> >  * Replaced with Dimtry's devm_tegra_get_memory_controller()
> > v1->v2:
> >  * N/A
> >
> >  drivers/memory/tegra/mc.c | 39 +++
> >  include/soc/tegra/mc.h| 17 +
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> > index ec8403557ed4..dd691dc3738e 100644
> > --- a/drivers/memory/tegra/mc.c
> > +++ b/drivers/memory/tegra/mc.c
> > @@ -42,6 +42,45 @@ static const struct of_device_id tegra_mc_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
> >
> > +static void tegra_mc_devm_action_put_device(void *data)
> 
> devm_tegra_memory_controller_put()
> 
> > +{
> > +   struct tegra_mc *mc = data;
> > +
> > +   put_device(mc->dev);
> > +}
> > +
> > +struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev)
> 
> Usually 'get' is a suffix (e.g. clk, gpiod, iio, led), so:
> devm_tegra_memory_controller_get()
> 
> > +{
> > +   struct platform_device *pdev;
> > +   struct device_node *np;
> > +   struct tegra_mc *mc;
> > +   int err;
> > +
> > +   np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL);
> > +   if (!np)
> > +   return ERR_PTR(-ENOENT);
> > +
> > +   pdev = of_find_device_by_node(np);
> > +   of_node_put(np);
> > +   if (!pdev)
> > +   return ERR_PTR(-ENODEV);
> > +
> > +   mc = platform_get_drvdata(pdev);
> > +   if (!mc) {
> > +   put_device(mc->dev);
> > +   return ERR_PTR(-EPROBE_DEFER);
> > +   }
> > +
> > +   err = devm_add_action(dev, tegra_mc_devm_action_put_device, mc);
> > +   if (err) {
> > +   put_device(mc->dev);
> > +   return ERR_PTR(err);
> > +   }
> > +
> > +   return mc;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_tegra_get_memory_controller);
> > +
> >  static int tegra_mc_block_dma_common(struct tegra_mc *mc,
> >  const struct tegra_mc_reset *rst)
> >  {
> > diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
> > index 1238e35653d1..c05142e3e244 100644
> > --- a/include/soc/tegra/mc.h
> > +++ b/include/soc/tegra/mc.h
> > @@ -184,4 +184,21 @@ struct tegra_mc {
> >  int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long 
> > rate);
> >  unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
> >
> > +#ifdef CONFIG_TEGRA_MC
> > +/**
> > + * devm_tegra_get_memory_controller() - Get the tegra_mc pointer.
> > + * @dev: Device that will be interacted with
> 
> This is not precise enough and there is no interaction with 'dev' in
> devm_tegra_get_memory_controller(). Something like: "Device that owns
> the pointer to tegra memory controller"
> 
> > + *
> > + * Return: ERR_PTR() on error or a valid pointer to a struct tegra_mc.
> > + *
> > + * The mc->dev counter will be automatically put by the device management 
> > code.
> 
> 1. s/mc/tegra_mc/ (it's the first occurence of word mc here)
> 2. "kerneldoc goes to the C file". Not to the header.

I will send v4 after changing all of the places.

Thanks for the comments!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 11:21:14AM +0200, Krzysztof Kozlowski wrote:
> On Wed, 30 Sep 2020 at 10:48, Nicolin Chen  wrote:
> >
> > Previously the driver relies on bus_set_iommu() in .probe() to call
> > in .probe_device() function so each client can poll iommus property
> > in DTB to configure fwspec via tegra_smmu_configure(). According to
> > the comments in .probe(), this is a bit of a hack. And this doesn't
> > work for a client that doesn't exist in DTB, PCI device for example.
> >
> > Actually when a device/client gets probed, the of_iommu_configure()
> > will call in .probe_device() function again, with a prepared fwspec
> > from of_iommu_configure() that reads the SWGROUP id in DTB as we do
> > in tegra-smmu driver.
> >
> > Additionally, as a new helper devm_tegra_get_memory_controller() is
> > introduced, there's no need to poll the iommus property in order to
> > get mc->smmu pointers or SWGROUP id.
> >
> > This patch reworks .probe_device() and .attach_dev() by doing:
> > 1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
> > 2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
> > 3) Calling devm_tegra_get_memory_controller() in .probe_device()
> > 4) Also dropping the hack in .probe() that's no longer needed.
> >
> > Signed-off-by: Nicolin Chen 
> > ---
> >
> > Changelog
> > v2->v3
> >  * Used devm_tegra_get_memory_controller() to get mc pointer
> >  * Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
> > v1->v2
> >  * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
> >with tegra_get_memory_controller call.
> >  * Dropped the hack in tegra_smmu_probe().
> >
> >  drivers/iommu/tegra-smmu.c | 144 ++---
> >  1 file changed, 36 insertions(+), 108 deletions(-)
> >
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index 6a3ecc334481..636dc3b89545 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -61,6 +61,8 @@ struct tegra_smmu_as {
> > u32 attr;
> >  };
> >
> > +static const struct iommu_ops tegra_smmu_ops;
> 
> I cannot find in this patch where this is assigned.

Because it's already set in probe():
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iommu/tegra-smmu.c#n1162

And my PATCH-3 sets it for PCI bus also.

Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
Previously the driver relies on bus_set_iommu() in .probe() to call
in .probe_device() function so each client can poll iommus property
in DTB to configure fwspec via tegra_smmu_configure(). According to
the comments in .probe(), this is a bit of a hack. And this doesn't
work for a client that doesn't exist in DTB, PCI device for example.

Actually when a device/client gets probed, the of_iommu_configure()
will call in .probe_device() function again, with a prepared fwspec
from of_iommu_configure() that reads the SWGROUP id in DTB as we do
in tegra-smmu driver.

Additionally, as a new helper devm_tegra_get_memory_controller() is
introduced, there's no need to poll the iommus property in order to
get mc->smmu pointers or SWGROUP id.

This patch reworks .probe_device() and .attach_dev() by doing:
1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
3) Calling devm_tegra_get_memory_controller() in .probe_device()
4) Also dropping the hack in .probe() that's no longer needed.

Signed-off-by: Nicolin Chen 
---

Changelog
v2->v3
 * Used devm_tegra_get_memory_controller() to get mc pointer
 * Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
v1->v2
 * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
   with tegra_get_memory_controller call.
 * Dropped the hack in tegra_smmu_probe().

 drivers/iommu/tegra-smmu.c | 144 ++---
 1 file changed, 36 insertions(+), 108 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..636dc3b89545 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -61,6 +61,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 +486,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,
-  )) {
-   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 != _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 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,
-  )) {
-   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 != _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++;
}
 }
 
@@ -807,80 +799,26 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct 
iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
 }
 
-static

[PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

2020-09-30 Thread Nicolin Chen
From: Dmitry Osipenko 

Multiple Tegra drivers need to retrieve Memory Controller and hence there
is quite some duplication of the retrieval code among the drivers. Let's
add a new common helper for the retrieval of the MC.

Signed-off-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---

Changelog
v2->v3:
 * Replaced with Dimtry's devm_tegra_get_memory_controller()
v1->v2:
 * N/A

 drivers/memory/tegra/mc.c | 39 +++
 include/soc/tegra/mc.h| 17 +
 2 files changed, 56 insertions(+)

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index ec8403557ed4..dd691dc3738e 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -42,6 +42,45 @@ static const struct of_device_id tegra_mc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
 
+static void tegra_mc_devm_action_put_device(void *data)
+{
+   struct tegra_mc *mc = data;
+
+   put_device(mc->dev);
+}
+
+struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev)
+{
+   struct platform_device *pdev;
+   struct device_node *np;
+   struct tegra_mc *mc;
+   int err;
+
+   np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL);
+   if (!np)
+   return ERR_PTR(-ENOENT);
+
+   pdev = of_find_device_by_node(np);
+   of_node_put(np);
+   if (!pdev)
+   return ERR_PTR(-ENODEV);
+
+   mc = platform_get_drvdata(pdev);
+   if (!mc) {
+   put_device(mc->dev);
+   return ERR_PTR(-EPROBE_DEFER);
+   }
+
+   err = devm_add_action(dev, tegra_mc_devm_action_put_device, mc);
+   if (err) {
+   put_device(mc->dev);
+   return ERR_PTR(err);
+   }
+
+   return mc;
+}
+EXPORT_SYMBOL_GPL(devm_tegra_get_memory_controller);
+
 static int tegra_mc_block_dma_common(struct tegra_mc *mc,
 const struct tegra_mc_reset *rst)
 {
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1238e35653d1..c05142e3e244 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -184,4 +184,21 @@ struct tegra_mc {
 int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
 unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
 
+#ifdef CONFIG_TEGRA_MC
+/**
+ * devm_tegra_get_memory_controller() - Get the tegra_mc pointer.
+ * @dev: Device that will be interacted with
+ *
+ * Return: ERR_PTR() on error or a valid pointer to a struct tegra_mc.
+ *
+ * The mc->dev counter will be automatically put by the device management code.
+ */
+struct tegra_mc *devm_tegra_get_memory_controller(struct device *dev);
+#else
+static inline struct tegra_mc *devm_tegra_get_memory_controller(struct device 
*dev)
+{
+   return ERR_PTR(-ENODEV);
+}
+#endif
+
 #endif /* __SOC_TEGRA_MC_H__ */
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v3 0/3] iommu/tegra-smmu: Add PCI support

2020-09-30 Thread Nicolin Chen
This series is to add PCI support with three changes:
PATCH-1 adds a helper function to get mc pointer
PATCH-2 adds support for clients that don't exist in DTB
PATCH-3 adds PCI support accordingly

Changelog (Detail in each patch)
v2->v3
 * Replaced with devm_tegra_get_memory_controller
 * Updated changes by following Dmitry's comments
v1->v2
 * Added PATCH-1 suggested by Dmitry
 * Reworked PATCH-2 to unify certain code

Dmitry Osipenko (1):
  memory: tegra: Add devm_tegra_get_memory_controller()

Nicolin Chen (2):
  iommu/tegra-smmu: Rework .probe_device and .attach_dev
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 179 +
 drivers/memory/tegra/mc.c  |  39 
 include/soc/tegra/mc.h |  17 
 3 files changed, 118 insertions(+), 117 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v3 3/3] iommu/tegra-smmu: Add PCI support

2020-09-30 Thread Nicolin Chen
This patch simply adds support for PCI devices.

Signed-off-by: Nicolin Chen 
---

Changelog
v2->v3
 * Replaced ternary conditional operator with if-else in .device_group()
 * Dropped change in tegra_smmu_remove()
v1->v2
 * Added error-out labels in tegra_smmu_probe()
 * Dropped pci_request_acs() since IOMMU core would call it.

 drivers/iommu/tegra-smmu.c | 35 ++-
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 636dc3b89545..db98ca334eae 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -882,7 +883,11 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
group->smmu = smmu;
group->soc = soc;
 
-   group->group = iommu_group_alloc();
+   if (dev_is_pci(dev))
+   group->group = pci_device_group(dev);
+   else
+   group->group = generic_device_group(dev);
+
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1079,22 +1084,34 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
 
err = iommu_device_register(>iommu);
-   if (err) {
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err)
+   goto err_sysfs;
 
err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
+   if (err < 0)
+   goto err_unregister;
+
+#ifdef CONFIG_PCI
+   if (!iommu_present(_bus_type)) {
+   err = bus_set_iommu(_bus_type, _smmu_ops);
+   if (err < 0)
+   goto err_bus_set;
}
+#endif
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
+
+err_bus_set:
+   bus_set_iommu(_bus_type, NULL);
+err_unregister:
+   iommu_device_unregister(>iommu);
+err_sysfs:
+   iommu_device_sysfs_remove(>iommu);
+
+   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller

2020-09-30 Thread Nicolin Chen
Hi Krzysztof,

On Wed, Sep 30, 2020 at 09:21:39AM +0200, Krzysztof Kozlowski wrote:
> On Wed, 30 Sep 2020 at 02:35, Nicolin Chen  wrote:
> >
> > This can be used by both tegra-smmu and tegra20-devfreq drivers.
> >
> > Suggested-by: Dmitry Osipenko 
> > Signed-off-by: Nicolin Chen 
> > ---
> >
> > Changelog
> > v1->v2
> >  * N/A
> >
> >  drivers/memory/tegra/mc.c | 23 +++
> >  include/soc/tegra/mc.h|  1 +
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> > index ec8403557ed4..09352ad66dcc 100644
> > --- a/drivers/memory/tegra/mc.c
> > +++ b/drivers/memory/tegra/mc.c
> > @@ -42,6 +42,29 @@ static const struct of_device_id tegra_mc_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
> 
> > +struct tegra_mc *tegra_get_memory_controller(void)
> > +{
> 
> Add kerneldoc and mention dropping of reference to the device after use.

I am abort to use Dmitry's devm_ one in my next version:
https://github.com/grate-driver/linux/commit/2105e7664063772d72fefe9696bdab0b688b9de2

Could I just skip the kerneldoc part? Otherwise, would you please
tell me which kerneldoc file I should update?

Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:58:50AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:29, Nicolin Chen пишет:
> > Hi Dmitry,
> > 
> > On Wed, Sep 30, 2020 at 08:10:07AM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 03:30, Nicolin Chen пишет:
> >>> - group->group = iommu_group_alloc();
> >>> + group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
> >>
> >> This will be nicer to write as:
> >>
> >> if (dev_is_pci(dev))
> >> group->group = pci_device_group(dev);
> >> else
> >> group->group = generic_device_group(dev);
> > 
> > Why is that nicer? I don't feel mine is hard to read at all...
> > 
> 
> Previously you said that you're going to add USB support, so I assume
> there will be something about USB.

I was hoping that it'd work for USB. Yet USB doesn't seem to have
an different function for device_group().

> It's also a kinda common style that majority of Tegra drivers use in
> upstream kernel. But yours variant is good too if there won't be a need
> to change it later on.

Okay..I'll use yours then.

Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 09:01:09AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:34, Nicolin Chen пишет:
> > On Wed, Sep 30, 2020 at 08:10:35AM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 03:30, Nicolin Chen пишет:
> >>>  void tegra_smmu_remove(struct tegra_smmu *smmu)
> >>>  {
> >>> + bus_set_iommu(_bus_type, NULL);
> >>
> >> Why only platform_bus? Is this really needed at all?
> > 
> > I see qcom_iommu.c file set to NULL in remove(), Probably should
> > have added pci_bus_type too though.
> > 
> > Or are you sure that there's no need at all?
> > 
> 
> It wasn't here before this patch and platform_bus is unrelated to the
> topic of this patch. But it probably should be there.
> 
> On the other hand, the tegra_smmu_remove() is unused and maybe it could
> be better to get rid of this unused function at all.

I will drop this line then, since no one is calling it. And maybe
we can submit a clean up patch later.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 09:32:20AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:44, Nicolin Chen пишет:
> > On Wed, Sep 30, 2020 at 08:12:10AM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 03:30, Nicolin Chen пишет:
> >> ...
> >>>  int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long 
> >>> rate);
> >>>  unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
> >>> +struct tegra_mc *tegra_get_memory_controller(void);
> >>>  
> >>>  #endif /* __SOC_TEGRA_MC_H__ */
> >>>
> >>
> >> This will conflict with the tegra20-devfreq driver, you should change it
> >> as well.
> > 
> > Will remove that in v3.
> > 
> > Thanks
> > 
> 
> Please also consider to add a resource-managed variant, similar to what
> I did here:
> 
> https://github.com/grate-driver/linux/commit/2105e7664063772d72fefe9696bdab0b688b9de2
> 
> I was going to use it in the next iteration of the memory interconnect
> series.
> 
> But now it also will be good if you could add the devm variant to yours
> SMMU patchset since you're already about to touch the tegra20-devfreq
> driver. I'll then rebase my patches on top of yours patch.

Just saw this reply. Yea, I think this'd be better. Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:20:50AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:10, Dmitry Osipenko пишет:
> > 30.09.2020 03:30, Nicolin Chen пишет:
> >>  static void tegra_smmu_release_device(struct device *dev)
> > 
> > The tegra_get_memory_controller() uses of_find_device_by_node(), hence
> > tegra_smmu_release_device() should put_device(mc) in order to balance
> > back the refcounting.
> > 
> 
> Actually, the put_device(mc) should be right after
> tegra_get_memory_controller() in tegra_smmu_probe_device() because SMMU
> is a part of MC, hence MC can't just go away.

Hmm..I found that there is no put_device() call in tegra20-devfreq.
Should we just put_device(pdev->dev) in tegra_get_memory_controller?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:59:45AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:41, Nicolin Chen пишет:
> > On Wed, Sep 30, 2020 at 08:39:54AM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 03:30, Nicolin Chen пишет:
> >>>  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,
> >>> -)) {
> >>> - 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 != _smmu_ops)
> >>> + return -ENOENT;
> >>
> >> s/_smmu_ops/smmu->iommu.ops/
> >>
> >> Secondly, is it even possible that fwspec could be NULL here or that
> >> fwspec->ops != smmu->ops?
> > 
> > I am following what's in the arm-smmu driver, as I think it'd be
> > a common practice to do such a check in such a way.
> > 
> 
> Please check whether it's really needed. It looks like it was needed
> sometime ago, but that's not true anymore.

Given that most iommu drivers have ->ops check, I'd like to
have it also for safety. If someday that's not true anymore,
I'd expect someone to update all existing drivers.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-30 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 09:10:38AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:49, Nicolin Chen пишет:
> > On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 03:30, Nicolin Chen пишет:
> >>> + /* An invalid mc pointer means mc and smmu drivers are not ready */
> >>> + if (IS_ERR_OR_NULL(mc))
> >>
> >> tegra_get_memory_controller() doesn't return NULL.
> > 
> > Well, I don't want to assume that it'd do that forever, and the
> > NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't
> > hurt to have.
> > 
> 
> I don't see any reasons why it won't do that forever.
> 
> Secondly, public function can't be changed randomly without updating all
> the callers.
> 
> Hence there is no need to handle cases that can't ever happen and it
> hurts readability of the code + original error code is missed.

I don't quite understand why an extra "_OR_NULL" would hurt
readabilitybut I'd take a step back and use IS_ERR().
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-29 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:20:50AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:10, Dmitry Osipenko пишет:
> > 30.09.2020 03:30, Nicolin Chen пишет:
> >>  static void tegra_smmu_release_device(struct device *dev)
> > 
> > The tegra_get_memory_controller() uses of_find_device_by_node(), hence
> > tegra_smmu_release_device() should put_device(mc) in order to balance
> > back the refcounting.
> > 
> 
> Actually, the put_device(mc) should be right after
> tegra_get_memory_controller() in tegra_smmu_probe_device() because SMMU
> is a part of MC, hence MC can't just go away.

Will add it. Thanks for pointing it out!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-29 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> > +   /* An invalid mc pointer means mc and smmu drivers are not ready */
> > +   if (IS_ERR_OR_NULL(mc))
> 
> tegra_get_memory_controller() doesn't return NULL.

Well, I don't want to assume that it'd do that forever, and the
NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't
hurt to have.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller

2020-09-29 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:12:10AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> ...
> >  int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long 
> > rate);
> >  unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
> > +struct tegra_mc *tegra_get_memory_controller(void);
> >  
> >  #endif /* __SOC_TEGRA_MC_H__ */
> > 
> 
> This will conflict with the tegra20-devfreq driver, you should change it
> as well.

Will remove that in v3.

Thanks
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-29 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:39:54AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> >  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,
> > -  )) {
> > -   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 != _smmu_ops)
> > +   return -ENOENT;
> 
> s/_smmu_ops/smmu->iommu.ops/
> 
> Secondly, is it even possible that fwspec could be NULL here or that
> fwspec->ops != smmu->ops?

I am following what's in the arm-smmu driver, as I think it'd be
a common practice to do such a check in such a way.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-29 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:24:02AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> > +   /*
> > +* IOMMU core allows -ENODEV return to carry on. So bypass any call
> > +* from bus_set_iommu() during tegra_smmu_probe(), as a device will
> > +* call in again via of_iommu_configure when fwspec is prepared.
> > +*/
> > +   if (!mc->smmu || !fwspec || fwspec->ops != _smmu_ops)
> > return ERR_PTR(-ENODEV);
> 
> The !mc->smmu can't be true.

Are you sure? I have removed the "mc->smmu = smmu" in probe() with
this change. So the only time "mc->smmu == !NULL" is after probe()
of SMMU driver is returned. As my comments says, tegra_smmu_probe()
calls in this function via bus_set_iommu(), so mc->smmu can be NULL
in this case.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support

2020-09-29 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:10:35AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> >  void tegra_smmu_remove(struct tegra_smmu *smmu)
> >  {
> > +   bus_set_iommu(_bus_type, NULL);
> 
> Why only platform_bus? Is this really needed at all?

I see qcom_iommu.c file set to NULL in remove(), Probably should
have added pci_bus_type too though.

Or are you sure that there's no need at all?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support

2020-09-29 Thread Nicolin Chen
Hi Dmitry,

On Wed, Sep 30, 2020 at 08:10:07AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> > -   group->group = iommu_group_alloc();
> > +   group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
> 
> This will be nicer to write as:
> 
> if (dev_is_pci(dev))
> group->group = pci_device_group(dev);
> else
> group->group = generic_device_group(dev);

Why is that nicer? I don't feel mine is hard to read at all...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support

2020-09-29 Thread Nicolin Chen
On Wed, Sep 30, 2020 at 08:10:00AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> ...
> > +#ifdef CONFIG_PCI
> > +   if (!iommu_present(_bus_type)) {
> 
> Could you please explain why this check is needed?

That's referencing what's in the arm-smmu.c file, since it does
not hurt to have a check.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/2] iommu/tegra-smmu: Expand mutex protection range

2020-09-29 Thread Nicolin Chen
On Tue, Sep 29, 2020 at 08:42:52PM +0300, Dmitry Osipenko wrote:
> 29.09.2020 09:13, Nicolin Chen пишет:
> > This is used to protect potential race condition at use_count.
> > since probes of client drivers, calling attach_dev(), may run
> > concurrently.
> > 
> > Signed-off-by: Nicolin Chen 
> > ---
> 
> It's always better not to mix success and error code paths in order to
> keep code readable, but not a big deal in the case of this particular
> patch since the changed code is quite simple. Please try to avoid doing
> this in the future patches.
> 
> Also, please note that in general it's better to use locked/unlocked
> versions for the functions like it's already done for
> tegra_smmu_map/unmap, this will remove the need to maintain lockings in
> the code. The code touched by this patch doesn't have complicated code
> paths, so it's good enough to me.
> 
> Reviewed-by: Dmitry Osipenko 

Okay. Thanks for the review!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 0/3] iommu/tegra-smmu: Add PCI support

2020-09-29 Thread Nicolin Chen
This series is to add PCI support with three changes:
PATCH-1 adds a helper function to get mc pointer
PATCH-2 adds support for clients that don't exist in DTB
PATCH-3 adds PCI support accordingly

Changelog
v1->v2
 * Added PATCH-1 suggested by Dmitry
 * Reworked PATCH-2 to unify certain code

Prvious versions
v1: https://lkml.org/lkml/2020/9/26/66

Nicolin Chen (3):
  memory: tegra: Add helper function tegra_get_memory_controller
  iommu/tegra-smmu: Rework .probe_device and .attach_dev
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 177 +
 drivers/memory/tegra/mc.c  |  23 +
 include/soc/tegra/mc.h |   1 +
 3 files changed, 84 insertions(+), 117 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

2020-09-29 Thread Nicolin Chen
Previously the driver relies on bus_set_iommu() in .probe() to call
in .probe_device() function so each client can poll iommus property
in DTB to configure fwspec via tegra_smmu_configure(). According to
the comments in .probe(), this is a bit of a hack. And this doesn't
work for a client that doesn't exist in DTB, PCI device for example.

Actually when a device/client gets probed, the of_iommu_configure()
will call in .probe_device() function again, with a prepared fwspec
from of_iommu_configure() that reads the SWGROUP id in DTB as we do
in tegra-smmu driver.

Additionally, as new helper function tegra_get_memory_controller()
is introduced, there's no need to poll the iommus property in order
to get mc->smmu pointers or SWGROUP id.

This patch reworks .probe_device() and .attach_dev() by doing:
1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
3) Calling tegra_get_memory_controller() in .probe_device() instead
4) Also dropping the hack in .probe() that's no longer needed.

Signed-off-by: Nicolin Chen 
---

Changelog
v1->v2
 * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
   with tegra_get_memory_controller call.
 * Dropped the hack in tegra_smmu_probe().

 drivers/iommu/tegra-smmu.c | 144 ++---
 1 file changed, 36 insertions(+), 108 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..cf4981369828 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -61,6 +61,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 +486,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,
-  )) {
-   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 != _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 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,
-  )) {
-   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 != _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++;
}
 }
 
@@ -807,80 +799,26 @@ 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)
-{
-   struct platform_device *pdev;
-   struct tegra_m

[PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller

2020-09-29 Thread Nicolin Chen
This can be used by both tegra-smmu and tegra20-devfreq drivers.

Suggested-by: Dmitry Osipenko 
Signed-off-by: Nicolin Chen 
---

Changelog
v1->v2
 * N/A

 drivers/memory/tegra/mc.c | 23 +++
 include/soc/tegra/mc.h|  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index ec8403557ed4..09352ad66dcc 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -42,6 +42,29 @@ static const struct of_device_id tegra_mc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
 
+struct tegra_mc *tegra_get_memory_controller(void)
+{
+   struct platform_device *pdev;
+   struct device_node *np;
+   struct tegra_mc *mc;
+
+   np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL);
+   if (!np)
+   return ERR_PTR(-ENOENT);
+
+   pdev = of_find_device_by_node(np);
+   of_node_put(np);
+   if (!pdev)
+   return ERR_PTR(-ENODEV);
+
+   mc = platform_get_drvdata(pdev);
+   if (!mc)
+   return ERR_PTR(-EPROBE_DEFER);
+
+   return mc;
+}
+EXPORT_SYMBOL_GPL(tegra_get_memory_controller);
+
 static int tegra_mc_block_dma_common(struct tegra_mc *mc,
 const struct tegra_mc_reset *rst)
 {
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1238e35653d1..c72718fd589f 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -183,5 +183,6 @@ struct tegra_mc {
 
 int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
 unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
+struct tegra_mc *tegra_get_memory_controller(void);
 
 #endif /* __SOC_TEGRA_MC_H__ */
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v2 3/3] iommu/tegra-smmu: Add PCI support

2020-09-29 Thread Nicolin Chen
This patch simply adds support for PCI devices. Also reverses
bus_set_iommu in tegra_smmu_remove().

Signed-off-by: Nicolin Chen 
---

Changelog
v1->v2
 * Added error-out labels in tegra_smmu_probe()
 * Dropped pci_request_acs() since IOMMU core would call it.

 drivers/iommu/tegra-smmu.c | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index cf4981369828..74d84908679e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -856,6 +857,7 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
const struct tegra_smmu_group_soc *soc;
unsigned int swgroup = fwspec->ids[0];
struct tegra_smmu_group *group;
+   bool pci = dev_is_pci(dev);
struct iommu_group *grp;
 
/* Find group_soc associating with swgroup */
@@ -882,7 +884,7 @@ static struct iommu_group *tegra_smmu_device_group(struct 
device *dev)
group->smmu = smmu;
group->soc = soc;
 
-   group->group = iommu_group_alloc();
+   group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1079,26 +1081,39 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
 
err = iommu_device_register(>iommu);
-   if (err) {
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
-   }
+   if (err)
+   goto err_sysfs;
 
err = bus_set_iommu(_bus_type, _smmu_ops);
-   if (err < 0) {
-   iommu_device_unregister(>iommu);
-   iommu_device_sysfs_remove(>iommu);
-   return ERR_PTR(err);
+   if (err < 0)
+   goto err_unregister;
+
+#ifdef CONFIG_PCI
+   if (!iommu_present(_bus_type)) {
+   err = bus_set_iommu(_bus_type, _smmu_ops);
+   if (err < 0)
+   goto err_bus_set;
}
+#endif
 
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
 
return smmu;
+
+err_bus_set:
+   bus_set_iommu(_bus_type, NULL);
+err_unregister:
+   iommu_device_unregister(>iommu);
+err_sysfs:
+   iommu_device_sysfs_remove(>iommu);
+
+   return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
 {
+   bus_set_iommu(_bus_type, NULL);
iommu_device_unregister(>iommu);
iommu_device_sysfs_remove(>iommu);
 
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v4 1/2] iommu/tegra-smmu: Unwrap tegra_smmu_group_get

2020-09-29 Thread Nicolin Chen
The tegra_smmu_group_get was added to group devices in different
SWGROUPs and it'd return a NULL group pointer upon a mismatch at
tegra_smmu_find_group(), so for most of clients/devices, it very
likely would mismatch and need a fallback generic_device_group().

But now tegra_smmu_group_get handles devices in same SWGROUP too,
which means that it would allocate a group for every new SWGROUP
or would directly return an existing one upon matching a SWGROUP,
i.e. any device will go through this function.

So possibility of having a NULL group pointer in device_group()
is upon failure of either devm_kzalloc() or iommu_group_alloc().
In either case, calling generic_device_group() no longer makes a
sense. Especially for devm_kzalloc() failing case, it'd cause a
problem if it fails at devm_kzalloc() yet succeeds at a fallback
generic_device_group(), because it does not create a group->list
for other devices to match.

This patch simply unwraps the function to clean it up.

Signed-off-by: Nicolin Chen 
---

Changelog
v2->v4:
 * N/A
v1->v2:
 * Changed type of swgroup to "unsigned int", following Thierry's
   commnets.

 drivers/iommu/tegra-smmu.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 0becdbfea306..ec4c9dafff95 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -903,10 +903,12 @@ static void tegra_smmu_group_release(void *iommu_data)
mutex_unlock(>lock);
 }
 
-static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
-   unsigned int swgroup)
+static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 {
+   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
const struct tegra_smmu_group_soc *soc;
+   unsigned int swgroup = fwspec->ids[0];
struct tegra_smmu_group *group;
struct iommu_group *grp;
 
@@ -950,19 +952,6 @@ static struct iommu_group *tegra_smmu_group_get(struct 
tegra_smmu *smmu,
return group->group;
 }
 
-static struct iommu_group *tegra_smmu_device_group(struct device *dev)
-{
-   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-   struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
-   struct iommu_group *group;
-
-   group = tegra_smmu_group_get(smmu, fwspec->ids[0]);
-   if (!group)
-   group = generic_device_group(dev);
-
-   return group;
-}
-
 static int tegra_smmu_of_xlate(struct device *dev,
   struct of_phandle_args *args)
 {
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v4 2/2] iommu/tegra-smmu: Expand mutex protection range

2020-09-29 Thread Nicolin Chen
This is used to protect potential race condition at use_count.
since probes of client drivers, calling attach_dev(), may run
concurrently.

Signed-off-by: Nicolin Chen 
---

Changelog
v3->v4:
 * Fixed typo "Expend" => "Expand"
v2->v3:
 * Renamed label "err_unlock" to "unlock"
v1->v2:
 * N/A

 drivers/iommu/tegra-smmu.c | 34 +-
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ec4c9dafff95..6a3ecc334481 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -256,26 +256,19 @@ static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, 
unsigned int *idp)
 {
unsigned long id;
 
-   mutex_lock(>lock);
-
id = find_first_zero_bit(smmu->asids, smmu->soc->num_asids);
-   if (id >= smmu->soc->num_asids) {
-   mutex_unlock(>lock);
+   if (id >= smmu->soc->num_asids)
return -ENOSPC;
-   }
 
set_bit(id, smmu->asids);
*idp = id;
 
-   mutex_unlock(>lock);
return 0;
 }
 
 static void tegra_smmu_free_asid(struct tegra_smmu *smmu, unsigned int id)
 {
-   mutex_lock(>lock);
clear_bit(id, smmu->asids);
-   mutex_unlock(>lock);
 }
 
 static bool tegra_smmu_capable(enum iommu_cap cap)
@@ -420,17 +413,21 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
 struct tegra_smmu_as *as)
 {
u32 value;
-   int err;
+   int err = 0;
+
+   mutex_lock(>lock);
 
if (as->use_count > 0) {
as->use_count++;
-   return 0;
+   goto unlock;
}
 
as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD,
  DMA_TO_DEVICE);
-   if (dma_mapping_error(smmu->dev, as->pd_dma))
-   return -ENOMEM;
+   if (dma_mapping_error(smmu->dev, as->pd_dma)) {
+   err = -ENOMEM;
+   goto unlock;
+   }
 
/* We can't handle 64-bit DMA addresses */
if (!smmu_dma_addr_valid(smmu, as->pd_dma)) {
@@ -453,24 +450,35 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
as->smmu = smmu;
as->use_count++;
 
+   mutex_unlock(>lock);
+
return 0;
 
 err_unmap:
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
+unlock:
+   mutex_unlock(>lock);
+
return err;
 }
 
 static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
struct tegra_smmu_as *as)
 {
-   if (--as->use_count > 0)
+   mutex_lock(>lock);
+
+   if (--as->use_count > 0) {
+   mutex_unlock(>lock);
return;
+   }
 
tegra_smmu_free_asid(smmu, as->id);
 
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
 
as->smmu = NULL;
+
+   mutex_unlock(>lock);
 }
 
 static int tegra_smmu_attach_dev(struct iommu_domain *domain,
-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[PATCH v4 0/2] iommu/tegra-smmu: Two followup changes

2020-09-29 Thread Nicolin Chen
Two followup patches for tegra-smmu:
PATCH-1 is a clean-up patch for the recently applied SWGROUP change.
PATCH-2 fixes a potential race condition

Changelog
v3->v4:
 * PATCH-2: Fixed typo in subject
v2->v3:
 * PATCH-2: renamed "err_unlock" to "unlock"
v1->v2:
 * Separated first two changs of V1 so they may get applied first,
   since the other three changes need some extra time to finalize.

Nicolin Chen (2):
  iommu/tegra-smmu: Unwrap tegra_smmu_group_get
  iommu/tegra-smmu: Expand mutex protection range

 drivers/iommu/tegra-smmu.c | 53 ++
 1 file changed, 25 insertions(+), 28 deletions(-)

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range

2020-09-29 Thread Nicolin Chen
On Tue, Sep 29, 2020 at 07:03:36AM +0100, Christoph Hellwig wrote:
> On Mon, Sep 28, 2020 at 09:52:47PM -0700, Nicolin Chen wrote:
> > This is used to protect potential race condition at use_count.
> > since probes of client drivers, calling attach_dev(), may run
> > concurrently.
> 
> Shouldn't this read "expand" instead of "expend"?

Oops...my poor English :)

Fixing

Thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH 3/5] iommu/tegra-smmu: Use iommu_fwspec in .probe_/.attach_device()

2020-09-28 Thread Nicolin Chen
On Tue, Sep 29, 2020 at 07:06:37AM +0300, Dmitry Osipenko wrote:
> ...
> >> As I mentioned in another reply, I think tegra_smmu_find() should be all
> >> you need in this case.
> > 
> > This function is used by .probe_device() where its dev pointer is
> > an SMMU client. IIUC, tegra_smmu_find() needs np pointer of "mc".
> > For a PCI device that doesn't have a DT node with iommus property,
> > not sure how we can use tegra_smmu_find().
> 
> Perhaps you could get np from struct pci_dev.bus?

Possibly yes...but I was hoping the solution would be potentially
reused by other devices that don't have DT nodes, USB for example,
though I haven't tested with current version.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


  1   2   3   >