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

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

[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

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

[Nicolin Chen]

This can be used by both tegra-smmu and tegra20-devfreq drivers.

Suggested-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

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

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

[Dmitry Osipenko]

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.

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

[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

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

[Dmitry Osipenko]

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.

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

[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

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

[Dmitry Osipenko]

30.09.2020 09:38, 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
> 

Please don't forget to add a stub for !MC as well since devfreq drivers
use COMPILE_TEST and don't directly depend on the MC driver.

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

[Krzysztof Kozlowski]

On Wed, 30 Sep 2020 at 02:35, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> This can be used by both tegra-smmu and tegra20-devfreq drivers.
>
> Suggested-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>
> 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.

Best regards,
Krzysztof

> +       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
>

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

[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 <nicoleotsuka@gmail.com> wrote:
> >
> > This can be used by both tegra-smmu and tegra20-devfreq drivers.
> >
> > Suggested-by: Dmitry Osipenko <digetx@gmail.com>
> > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > ---
> >
> > 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

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

[Krzysztof Kozlowski]

On Wed, 30 Sep 2020 at 09:31, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> 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 <nicoleotsuka@gmail.com> wrote:
> > >
> > > This can be used by both tegra-smmu and tegra20-devfreq drivers.
> > >
> > > Suggested-by: Dmitry Osipenko <digetx@gmail.com>
> > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > > ---
> > >
> > > 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?

His version is almost the same as yours so it does not matter - you
declare an exported function, so you need to document it. kerneldoc
goes to the C file.
https://elixir.bootlin.com/linux/latest/source/Documentation/doc-guide/kernel-doc.rst

Best regards,
Krzysztof

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

[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 <nicoleotsuka@gmail.com>
---

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,
-					   &args)) {
-		unsigned int swgroup = args.args[0];
-
-		if (args.np != smmu->dev->of_node) {
-			of_node_put(args.np);
-			continue;
-		}
-
-		of_node_put(args.np);
+	if (!fwspec || fwspec->ops != &tegra_smmu_ops)
+		return -ENOENT;
 
+	for (index = 0; index < fwspec->num_ids; index++) {
 		err = tegra_smmu_as_prepare(smmu, as);
-		if (err < 0)
-			return err;
+		if (err)
+			goto disable;
 
-		tegra_smmu_enable(smmu, swgroup, as->id);
-		index++;
+		tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
 	}
 
 	if (index == 0)
 		return -ENODEV;
 
 	return 0;
+
+disable:
+	while (index--) {
+		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+		tegra_smmu_as_unprepare(smmu, as);
+	}
+
+	return err;
 }
 
 static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
 {
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct tegra_smmu_as *as = to_smmu_as(domain);
-	struct device_node *np = dev->of_node;
 	struct tegra_smmu *smmu = as->smmu;
-	struct of_phandle_args args;
 	unsigned int index = 0;
 
-	while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
-					   &args)) {
-		unsigned int swgroup = args.args[0];
-
-		if (args.np != smmu->dev->of_node) {
-			of_node_put(args.np);
-			continue;
-		}
-
-		of_node_put(args.np);
+	if (!fwspec || fwspec->ops != &tegra_smmu_ops)
+		return;
 
-		tegra_smmu_disable(smmu, swgroup, as->id);
+	for (index = 0; index < fwspec->num_ids; index++) {
+		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
 		tegra_smmu_as_unprepare(smmu, as);
-		index++;
 	}
 }
 
@@ -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_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, &dev->of_node->fwnode, ops);
-	if (err < 0) {
-		dev_err(dev, "failed to initialize fwspec: %d\n", err);
-		return err;
-	}
-
-	err = ops->of_xlate(dev, args);
-	if (err < 0) {
-		dev_err(dev, "failed to parse SW group ID: %d\n", err);
-		iommu_fwspec_free(dev);
-		return err;
-	}
-
-	return 0;
-}
-
 static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 {
-	struct device_node *np = dev->of_node;
-	struct tegra_smmu *smmu = NULL;
-	struct of_phandle_args args;
-	unsigned int index = 0;
-	int err;
-
-	while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
-					  &args) == 0) {
-		smmu = tegra_smmu_find(args.np);
-		if (smmu) {
-			err = tegra_smmu_configure(smmu, dev, &args);
-			of_node_put(args.np);
-
-			if (err < 0)
-				return ERR_PTR(err);
-
-			/*
-			 * Only a single IOMMU master interface is currently
-			 * supported by the Linux kernel, so abort after the
-			 * first match.
-			 */
-			dev_iommu_priv_set(dev, smmu);
-
-			break;
-		}
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct tegra_mc *mc = tegra_get_memory_controller();
 
-		of_node_put(args.np);
-		index++;
-	}
+	/* An invalid mc pointer means mc and smmu drivers are not ready */
+	if (IS_ERR_OR_NULL(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 != &tegra_smmu_ops)
 		return ERR_PTR(-ENODEV);
 
-	return &smmu->iommu;
+	dev_iommu_priv_set(dev, mc->smmu);
+
+	return &mc->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 the IOMMU registration process will attempt to add
-	 * all devices to the IOMMU when bus_set_iommu() is called. In order
-	 * not to rely on global variables to track the IOMMU instance, we
-	 * set it here so that it can be looked up from the .probe_device()
-	 * callback via the IOMMU device's .drvdata field.
-	 */
-	mc->smmu = smmu;
-
 	size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
 
 	smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
-- 
2.17.1

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

[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.

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

[Dmitry Osipenko]

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.

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

[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!

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

[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?

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

[Dmitry Osipenko]

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.

> +		return ERR_PTR(-EPROBE_DEFER);

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

[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.

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

[Dmitry Osipenko]

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.

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

[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
readability....but I'd take a step back and use IS_ERR().

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

[Dmitry Osipenko]

30.09.2020 09:13, 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
> readability....but I'd take a step back and use IS_ERR().
> 

The tegra_get_memory_controller() doesn't return NULL, hence the
NULL-check is misleading.

If I was reading that code for the first time and notice such a thing,
then instantly I'd have a much lower credibility to the whole code.

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

[Dmitry Osipenko]

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 != &tegra_smmu_ops)
>  		return ERR_PTR(-ENODEV);

The !mc->smmu can't be true.

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

[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 != &tegra_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.

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

[Dmitry Osipenko]

30.09.2020 08:39, 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 != &tegra_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.
> 

I missed that.

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

[Dmitry Osipenko]

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,
> -					   &args)) {
> -		unsigned int swgroup = args.args[0];
> -
> -		if (args.np != smmu->dev->of_node) {
> -			of_node_put(args.np);
> -			continue;
> -		}
> -
> -		of_node_put(args.np);
> +	if (!fwspec || fwspec->ops != &tegra_smmu_ops)
> +		return -ENOENT;

s/&tegra_smmu_ops/smmu->iommu.ops/

Secondly, is it even possible that fwspec could be NULL here or that
fwspec->ops != smmu->ops?

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

[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,
> > -					   &args)) {
> > -		unsigned int swgroup = args.args[0];
> > -
> > -		if (args.np != smmu->dev->of_node) {
> > -			of_node_put(args.np);
> > -			continue;
> > -		}
> > -
> > -		of_node_put(args.np);
> > +	if (!fwspec || fwspec->ops != &tegra_smmu_ops)
> > +		return -ENOENT;
> 
> s/&tegra_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.

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

[Dmitry Osipenko]

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,
>>> -					   &args)) {
>>> -		unsigned int swgroup = args.args[0];
>>> -
>>> -		if (args.np != smmu->dev->of_node) {
>>> -			of_node_put(args.np);
>>> -			continue;
>>> -		}
>>> -
>>> -		of_node_put(args.np);
>>> +	if (!fwspec || fwspec->ops != &tegra_smmu_ops)
>>> +		return -ENOENT;
>>
>> s/&tegra_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.

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

[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,
> >>> -					   &args)) {
> >>> -		unsigned int swgroup = args.args[0];
> >>> -
> >>> -		if (args.np != smmu->dev->of_node) {
> >>> -			of_node_put(args.np);
> >>> -			continue;
> >>> -		}
> >>> -
> >>> -		of_node_put(args.np);
> >>> +	if (!fwspec || fwspec->ops != &tegra_smmu_ops)
> >>> +		return -ENOENT;
> >>
> >> s/&tegra_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.

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

[Nicolin Chen]

This patch simply adds support for PCI devices. Also reverses
bus_set_iommu in tegra_smmu_remove().

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

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 <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -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(&smmu->lock);
@@ -1079,26 +1081,39 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 	iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
 
 	err = iommu_device_register(&smmu->iommu);
-	if (err) {
-		iommu_device_sysfs_remove(&smmu->iommu);
-		return ERR_PTR(err);
-	}
+	if (err)
+		goto err_sysfs;
 
 	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
-	if (err < 0) {
-		iommu_device_unregister(&smmu->iommu);
-		iommu_device_sysfs_remove(&smmu->iommu);
-		return ERR_PTR(err);
+	if (err < 0)
+		goto err_unregister;
+
+#ifdef CONFIG_PCI
+	if (!iommu_present(&pci_bus_type)) {
+		err = bus_set_iommu(&pci_bus_type, &tegra_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(&platform_bus_type, NULL);
+err_unregister:
+	iommu_device_unregister(&smmu->iommu);
+err_sysfs:
+	iommu_device_sysfs_remove(&smmu->iommu);
+
+	return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
 {
+	bus_set_iommu(&platform_bus_type, NULL);
 	iommu_device_unregister(&smmu->iommu);
 	iommu_device_sysfs_remove(&smmu->iommu);
 
-- 
2.17.1

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

[Dmitry Osipenko]

30.09.2020 03:30, Nicolin Chen пишет:
...
> +#ifdef CONFIG_PCI
> +	if (!iommu_present(&pci_bus_type)) {

Could you please explain why this check is needed?

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

[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(&pci_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.

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

[Dmitry Osipenko]

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);

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

[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...

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

[Dmitry Osipenko]

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.

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.

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

[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

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

[Dmitry Osipenko]

30.09.2020 03:30, Nicolin Chen пишет:
>  void tegra_smmu_remove(struct tegra_smmu *smmu)
>  {
> +	bus_set_iommu(&platform_bus_type, NULL);

Why only platform_bus? Is this really needed at all?

>  	iommu_device_unregister(&smmu->iommu);
>  	iommu_device_sysfs_remove(&smmu->iommu);

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

[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(&platform_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?

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

[Dmitry Osipenko]

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(&platform_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.

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

[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(&platform_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.