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

[PATCH v3 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 (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

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

[Nicolin Chen]

From: Dmitry Osipenko <digetx@gmail.com>

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 <digetx@gmail.com>
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

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

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

[Krzysztof Kozlowski]

"On Wed, 30 Sep 2020 at 10:48, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> From: Dmitry Osipenko <digetx@gmail.com>
>
> 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 <digetx@gmail.com>
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
>
> 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.

Best regards,
Krzysztof

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

[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 <nicoleotsuka@gmail.com> wrote:
> >
> > From: Dmitry Osipenko <digetx@gmail.com>
> >
> > 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 <digetx@gmail.com>
> > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > ---
> >
> > 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!

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

[Krzysztof Kozlowski]

On Wed, Sep 30, 2020 at 02:41:45AM -0700, Nicolin Chen wrote:
> On Wed, Sep 30, 2020 at 11:07:32AM +0200, Krzysztof Kozlowski wrote:
> > "On Wed, 30 Sep 2020 at 10:48, Nicolin Chen <nicoleotsuka@gmail.com> wrote:
> > >
> > > From: Dmitry Osipenko <digetx@gmail.com>
> > >
> > > 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 <digetx@gmail.com>
> > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > > ---
> > >
> > > 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()

My bad here, this is not a "put" helper so the previous name was
actually good. No need to change.

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

This can be simpler with devm_add_action_or_reset.

Best regards,
Krzysztof

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

[Dmitry Osipenko]

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

This should be put_device(&pdev->dev). Please always be careful while
copying someones else code :)

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

[Krzysztof Kozlowski]

On Wed, 30 Sep 2020 at 16:41, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> ...
> > +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);
>
> This should be put_device(&pdev->dev). Please always be careful while
> copying someones else code :)

Good catch. I guess devm_add_action_or_reset() would also work... or
running Smatch on new code. Smatch should point it out.

Best regards,
Krzysztof

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

[Dmitry Osipenko]

30.09.2020 17:45, Krzysztof Kozlowski пишет:
> On Wed, 30 Sep 2020 at 16:41, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> ...
>>> +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);
>>
>> This should be put_device(&pdev->dev). Please always be careful while
>> copying someones else code :)
> 
> Good catch. I guess devm_add_action_or_reset() would also work... or
> running Smatch on new code. Smatch should point it out.

The devm_add_action_or_reset() shouldn't help much in this particular
case because it's too early for the devm_add_action here.

Having an explicit put_device() in all error code paths also helps with
improving readability of the code a tad, IMO.

Smatch indeed should catch this bug, but Smatch usually isn't a part of
the developers workflow. More often Smatch is a part of maintainers
workflow, hence such problems usually are getting caught before patches
are applied.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
> From: Dmitry Osipenko <digetx@gmail.com>
> 
> 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 <digetx@gmail.com>
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
> 
> 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(+)

Let's not add this helper, please. If a device needs a reference to the
memory controller, it should have a phandle to the memory controller in
device tree so that it can be looked up explicitly.

Adding this helper is officially sanctioning that it's okay not to have
that reference and that's a bad idea.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

30.09.2020 18:23, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>> From: Dmitry Osipenko <digetx@gmail.com>
>>
>> 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 <digetx@gmail.com>
>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>> ---
>>
>> 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(+)
> 
> Let's not add this helper, please. If a device needs a reference to the
> memory controller, it should have a phandle to the memory controller in
> device tree so that it can be looked up explicitly.
> 
> Adding this helper is officially sanctioning that it's okay not to have
> that reference and that's a bad idea.

Maybe that's because the reference isn't really needed for the lookup? :)

Secondly, we could use the reference and then fall back to the
of-matching for devices that don't have the reference, like all Tegra20
devices + Tegra30/124 ACTMON devices.

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

[Dmitry Osipenko]

30.09.2020 18:23, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>> From: Dmitry Osipenko <digetx@gmail.com>
>>
>> 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 <digetx@gmail.com>
>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>> ---
>>
>> 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(+)
> 
> Let's not add this helper, please. If a device needs a reference to the
> memory controller, it should have a phandle to the memory controller in
> device tree so that it can be looked up explicitly.
> 
> Adding this helper is officially sanctioning that it's okay not to have
> that reference and that's a bad idea.

And please explain why it's a bad idea, I don't see anything bad here at
all.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
> 30.09.2020 18:23, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
> >> From: Dmitry Osipenko <digetx@gmail.com>
> >>
> >> 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 <digetx@gmail.com>
> >> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> >> ---
> >>
> >> 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(+)
> > 
> > Let's not add this helper, please. If a device needs a reference to the
> > memory controller, it should have a phandle to the memory controller in
> > device tree so that it can be looked up explicitly.
> > 
> > Adding this helper is officially sanctioning that it's okay not to have
> > that reference and that's a bad idea.
> 
> And please explain why it's a bad idea, I don't see anything bad here at
> all.

Well, you said yourself in a recent comment that we should avoid global
variables. devm_tegra_get_memory_controller() is nothing but a glorified
global variable.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

30.09.2020 19:03, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 18:23, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>>>> From: Dmitry Osipenko <digetx@gmail.com>
>>>>
>>>> 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 <digetx@gmail.com>
>>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>>>> ---
>>>>
>>>> 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(+)
>>>
>>> Let's not add this helper, please. If a device needs a reference to the
>>> memory controller, it should have a phandle to the memory controller in
>>> device tree so that it can be looked up explicitly.
>>>
>>> Adding this helper is officially sanctioning that it's okay not to have
>>> that reference and that's a bad idea.
>>
>> And please explain why it's a bad idea, I don't see anything bad here at
>> all.
> 
> Well, you said yourself in a recent comment that we should avoid global
> variables. devm_tegra_get_memory_controller() is nothing but a glorified
> global variable.

This is not a variable, but a common helper function which will remove
the duplicated code and will help to avoid common mistakes like a missed
put_device().

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2647 bytes --]

On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:03, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 18:23, Thierry Reding пишет:
> >>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
> >>>> From: Dmitry Osipenko <digetx@gmail.com>
> >>>>
> >>>> 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 <digetx@gmail.com>
> >>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> >>>> ---
> >>>>
> >>>> 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(+)
> >>>
> >>> Let's not add this helper, please. If a device needs a reference to the
> >>> memory controller, it should have a phandle to the memory controller in
> >>> device tree so that it can be looked up explicitly.
> >>>
> >>> Adding this helper is officially sanctioning that it's okay not to have
> >>> that reference and that's a bad idea.
> >>
> >> And please explain why it's a bad idea, I don't see anything bad here at
> >> all.
> > 
> > Well, you said yourself in a recent comment that we should avoid global
> > variables. devm_tegra_get_memory_controller() is nothing but a glorified
> > global variable.
> 
> This is not a variable, but a common helper function which will remove
> the duplicated code and will help to avoid common mistakes like a missed
> put_device().

Yeah, you're right: this is actually much worse than a global variable.
It's a helper function that needs 50+ lines in order to effectively
access a global variable.

You could write this much simpler by doing something like this:

	static struct tegra_mc *global_mc;

	int tegra_mc_probe(...)
	{
		...

		global_mc = mc;

		...
	}

	struct tegra_mc *tegra_get_memory_controller(void)
	{
		return global_mc;
	}

The result is *exactly* the same, except that this is actually more
honest. Nicolin's patch *pretends* that it isn't using a global variable
by wrapping a lot of complicated code around it.

But that doesn't change the fact that this accesses a singleton object
without actually being able to tie it to the device in the first place.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

30.09.2020 19:15, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 19:03, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
>>>> 30.09.2020 18:23, Thierry Reding пишет:
>>>>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>>>>>> From: Dmitry Osipenko <digetx@gmail.com>
>>>>>>
>>>>>> 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 <digetx@gmail.com>
>>>>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>>>>>> ---
>>>>>>
>>>>>> 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(+)
>>>>>
>>>>> Let's not add this helper, please. If a device needs a reference to the
>>>>> memory controller, it should have a phandle to the memory controller in
>>>>> device tree so that it can be looked up explicitly.
>>>>>
>>>>> Adding this helper is officially sanctioning that it's okay not to have
>>>>> that reference and that's a bad idea.
>>>>
>>>> And please explain why it's a bad idea, I don't see anything bad here at
>>>> all.
>>>
>>> Well, you said yourself in a recent comment that we should avoid global
>>> variables. devm_tegra_get_memory_controller() is nothing but a glorified
>>> global variable.
>>
>> This is not a variable, but a common helper function which will remove
>> the duplicated code and will help to avoid common mistakes like a missed
>> put_device().
> 
> Yeah, you're right: this is actually much worse than a global variable.
> It's a helper function that needs 50+ lines in order to effectively
> access a global variable.
> 
> You could write this much simpler by doing something like this:
> 
> 	static struct tegra_mc *global_mc;
> 
> 	int tegra_mc_probe(...)
> 	{
> 		...
> 
> 		global_mc = mc;
> 
> 		...
> 	}
> 
> 	struct tegra_mc *tegra_get_memory_controller(void)
> 	{
> 		return global_mc;
> 	}
> 
> The result is *exactly* the same, except that this is actually more
> honest. Nicolin's patch *pretends* that it isn't using a global variable
> by wrapping a lot of complicated code around it.
> 
> But that doesn't change the fact that this accesses a singleton object
> without actually being able to tie it to the device in the first place.

I don't think that the MC driver will stay built-in forever, although
its modularization is complicated right now. Hence something shall keep
the reference to the MC device resources while they are in use and this
patch takes care of doing that.

Secondly, the Nicolin's patch doesn't pretend on anything, but rather
brings the already existing duplicated code to a single common place.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4311 bytes --]

On Wed, Sep 30, 2020 at 07:26:00PM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:15, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 19:03, Thierry Reding пишет:
> >>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
> >>>> 30.09.2020 18:23, Thierry Reding пишет:
> >>>>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
> >>>>>> From: Dmitry Osipenko <digetx@gmail.com>
> >>>>>>
> >>>>>> 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 <digetx@gmail.com>
> >>>>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> >>>>>> ---
> >>>>>>
> >>>>>> 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(+)
> >>>>>
> >>>>> Let's not add this helper, please. If a device needs a reference to the
> >>>>> memory controller, it should have a phandle to the memory controller in
> >>>>> device tree so that it can be looked up explicitly.
> >>>>>
> >>>>> Adding this helper is officially sanctioning that it's okay not to have
> >>>>> that reference and that's a bad idea.
> >>>>
> >>>> And please explain why it's a bad idea, I don't see anything bad here at
> >>>> all.
> >>>
> >>> Well, you said yourself in a recent comment that we should avoid global
> >>> variables. devm_tegra_get_memory_controller() is nothing but a glorified
> >>> global variable.
> >>
> >> This is not a variable, but a common helper function which will remove
> >> the duplicated code and will help to avoid common mistakes like a missed
> >> put_device().
> > 
> > Yeah, you're right: this is actually much worse than a global variable.
> > It's a helper function that needs 50+ lines in order to effectively
> > access a global variable.
> > 
> > You could write this much simpler by doing something like this:
> > 
> > 	static struct tegra_mc *global_mc;
> > 
> > 	int tegra_mc_probe(...)
> > 	{
> > 		...
> > 
> > 		global_mc = mc;
> > 
> > 		...
> > 	}
> > 
> > 	struct tegra_mc *tegra_get_memory_controller(void)
> > 	{
> > 		return global_mc;
> > 	}
> > 
> > The result is *exactly* the same, except that this is actually more
> > honest. Nicolin's patch *pretends* that it isn't using a global variable
> > by wrapping a lot of complicated code around it.
> > 
> > But that doesn't change the fact that this accesses a singleton object
> > without actually being able to tie it to the device in the first place.
> 
> I don't think that the MC driver will stay built-in forever, although
> its modularization is complicated right now. Hence something shall keep
> the reference to the MC device resources while they are in use and this
> patch takes care of doing that.

It looks to me like all the other places where we get a reference to the
MC also keep a reference to the device. That's obviously not going to be
enough once the code is turned into a module. At that point we need to
make sure to also grab a reference to the module. But that's orthogonal
to this discussion.

> Secondly, the Nicolin's patch doesn't pretend on anything, but rather

Yes, the patch does pretend to "look up" the memory controller device,
but in reality it will always return a singleton object, which can just
as easily be achieved by using a global variable.

> brings the already existing duplicated code to a single common place.

Where exactly is that duplicated code? The only places I see where we
get a reference to the memory controller are from the EMC drivers and
they properly look up the MC via the nvidia,memory-controller device
tree property.

But that's not what this new helper does. This code will use the OF
lookup table to find any match and then returns that, completely
ignoring any links established by the device tree.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

...
>> I don't think that the MC driver will stay built-in forever, although
>> its modularization is complicated right now. Hence something shall keep
>> the reference to the MC device resources while they are in use and this
>> patch takes care of doing that.
> 
> It looks to me like all the other places where we get a reference to the
> MC also keep a reference to the device. That's obviously not going to be
> enough once the code is turned into a module. At that point we need to
> make sure to also grab a reference to the module. But that's orthogonal
> to this discussion.
> 
>> Secondly, the Nicolin's patch doesn't pretend on anything, but rather
> 
> Yes, the patch does pretend to "look up" the memory controller device,
> but in reality it will always return a singleton object, which can just
> as easily be achieved by using a global variable.
> 
>> brings the already existing duplicated code to a single common place.
> 
> Where exactly is that duplicated code? The only places I see where we
> get a reference to the memory controller are from the EMC drivers and
> they properly look up the MC via the nvidia,memory-controller device
> tree property.

Yours observation is correct, all the drivers *do the lookup*. My point
is that the nvidia,memory-controller usage isn't strictly necessary.

The tegra20-devfreq driver doesn't use the phandle lookup because
Tegra20 DTs don't have it, instead it uses the compatible lookup. Hence
this patch doesn't really change the already existing behaviour for the
drivers. The phandle usage is optional.

This patch adds a common API that is usable by *all* the already
existing drivers, starting from the Tegra20 drivers.

> But that's not what this new helper does. This code will use the OF
> lookup table to find any match and then returns that, completely
> ignoring any links established by the device tree.

As I already said in the other reply, it should be fine to add lookup by
the phandle and then fall back to the compatible matching. On the other
hand, this is not strictly necessary because we always have only a
single MC device at a time.

Please note that I don't have any objections to improving this patch. In
the end either way will work, so we just need to choose the best option.
I was merely explaining the rationale behind this patch and not trying
to defend it.

Yours suggestion about using static mc variable is also good to me since
currently MC driver is built-in and at least that won't be a
globally-visible kernel variable, which doesn't feel great to have.

I think we can follow approach of the static mc variable for the starter
and improve it once there will be a real need. This should be the
simplest option right now.

But again, we may slightly future-proof the API by adding the
resource-managed variant. Either way will be good, IMO :) Currently I
don't have a strong preference.

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

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,
-					   &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 tegra_mc *mc = devm_tegra_get_memory_controller(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 
-		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 != &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 v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

[Krzysztof Kozlowski]

On Wed, 30 Sep 2020 at 10:48, Nicolin Chen <nicoleotsuka@gmail.com> 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 <nicoleotsuka@gmail.com>
> ---
>
> 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.

Best regards,
Krzysztof

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

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

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

[Krzysztof Kozlowski]

On Wed, Sep 30, 2020 at 02:40:32AM -0700, Nicolin Chen wrote:
> On Wed, Sep 30, 2020 at 11:21:14AM +0200, Krzysztof Kozlowski wrote:
> > On Wed, 30 Sep 2020 at 10:48, Nicolin Chen <nicoleotsuka@gmail.com> 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 <nicoleotsuka@gmail.com>
> > > ---
> > >
> > > 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.

OK, good point. Thanks for explanation.

Best regards,
Krzysztof

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

[Dmitry Osipenko]

...
> +	struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>  
> -		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 != &tegra_smmu_ops)
>  		return ERR_PTR(-ENODEV);
>  
> -	return &smmu->iommu;
> +	dev_iommu_priv_set(dev, mc->smmu);
> +
> +	return &mc->smmu->iommu;
>  }

Is it really okay to use devm_tegra_get_memory_controller() here?

I assume it should be more preferred to do it only for devices that have
fwspec->ops == &tegra_smmu_ops.

Secondly, it also looks to me that a non-devm variant should be more
appropriate here because tegra_smmu_probe_device() isn't invoked by the
devices themselves.

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

[Dmitry Osipenko]

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

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.

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

[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,
> > -					   &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;
> 
> 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.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8617 bytes --]

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

> -		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 != &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;

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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

 I'...
>> +	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.

This sounds more complicated than the current variant.

Secondly, I'm already about to use the new tegra_get_memory_controller()
API for all the T20/30/124/210 EMC and devfreq drivers.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
>  I'...
> >> +	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.
> 
> This sounds more complicated than the current variant.
> 
> Secondly, I'm already about to use the new tegra_get_memory_controller()
> API for all the T20/30/124/210 EMC and devfreq drivers.

Why do we need it there? They seem to work fine without it right now. If
it is required for new functionality, we can always make the dependent
on a DT reference via phandle without breaking any existing code.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

30.09.2020 19:06, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
>>  I'...
>>>> +	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.
>>
>> This sounds more complicated than the current variant.
>>
>> Secondly, I'm already about to use the new tegra_get_memory_controller()
>> API for all the T20/30/124/210 EMC and devfreq drivers.
> 
> Why do we need it there? They seem to work fine without it right now.

All the Tegra30/124/210 EMC drivers are already duplicating that MC
lookup code and only the recent T210 driver does it properly.

> If it is required for new functionality, we can always make the dependent
> on a DT reference via phandle without breaking any existing code.

That's correct, it will be also needed for the new functionality as
well, hence even more drivers will need to perform the MC lookup.

I don't quite understand why you're asking for the phandle reference,
it's absolutely not needed for the MC lookup and won't work for the
older DTs if DT change will be needed. Please give a detailed explanation.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2934 bytes --]

On Wed, Sep 30, 2020 at 07:25:41PM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:06, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
> >>  I'...
> >>>> +	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.
> >>
> >> This sounds more complicated than the current variant.
> >>
> >> Secondly, I'm already about to use the new tegra_get_memory_controller()
> >> API for all the T20/30/124/210 EMC and devfreq drivers.
> > 
> > Why do we need it there? They seem to work fine without it right now.
> 
> All the Tegra30/124/210 EMC drivers are already duplicating that MC
> lookup code and only the recent T210 driver does it properly.
> 
> > If it is required for new functionality, we can always make the dependent
> > on a DT reference via phandle without breaking any existing code.
> 
> That's correct, it will be also needed for the new functionality as
> well, hence even more drivers will need to perform the MC lookup.

I don't have any issues with adding a helper if we need it from several
different locations. But the helper should be working off of a given
device and look up the device via the device tree node referenced by
phandle. We already have those phandles in place for the EMC devices,
and any other device that needs to interoperate with the MC should also
get such a reference.

> I don't quite understand why you're asking for the phandle reference,
> it's absolutely not needed for the MC lookup and won't work for the

We need that phandle in order to establish a link between the devices.
Yes, you can probably do it without the phandle and just match by
compatible string. But we don't do that for other types of devices
either, right? For a display driver we reference the attached panel via
phandle, but we could also just look it up via name or absolute path or
some other heuristic. But a phandle is just a much more explicit way of
linking the devices, so why not use it?

> older DTs if DT change will be needed. Please give a detailed explanation.

New functionality doesn't have to work with older DTs.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

30.09.2020 19:47, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 07:25:41PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 19:06, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
>>>>  I'...
>>>>>> +	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.
>>>>
>>>> This sounds more complicated than the current variant.
>>>>
>>>> Secondly, I'm already about to use the new tegra_get_memory_controller()
>>>> API for all the T20/30/124/210 EMC and devfreq drivers.
>>>
>>> Why do we need it there? They seem to work fine without it right now.
>>
>> All the Tegra30/124/210 EMC drivers are already duplicating that MC
>> lookup code and only the recent T210 driver does it properly.
>>
>>> If it is required for new functionality, we can always make the dependent
>>> on a DT reference via phandle without breaking any existing code.
>>
>> That's correct, it will be also needed for the new functionality as
>> well, hence even more drivers will need to perform the MC lookup.
> 
> I don't have any issues with adding a helper if we need it from several
> different locations. But the helper should be working off of a given
> device and look up the device via the device tree node referenced by
> phandle. We already have those phandles in place for the EMC devices,
> and any other device that needs to interoperate with the MC should also
> get such a reference.
> 
>> I don't quite understand why you're asking for the phandle reference,
>> it's absolutely not needed for the MC lookup and won't work for the
> 
> We need that phandle in order to establish a link between the devices.
> Yes, you can probably do it without the phandle and just match by
> compatible string. But we don't do that for other types of devices
> either, right? For a display driver we reference the attached panel via
> phandle, but we could also just look it up via name or absolute path or
> some other heuristic. But a phandle is just a much more explicit way of
> linking the devices, so why not use it?

There are dozens variants of the panels and system could easily have
more than one panel, hence a direct lookup by phandle is a natural
choice for the panels.

While all Tegra SoCs have a single fixed MC in the system, and thus,
there is no real need to use phandle because we can't mix up MC with
anything else.

>> older DTs if DT change will be needed. Please give a detailed explanation.
> 
> New functionality doesn't have to work with older DTs.

This is fine in general, but I'm afraid that in this particular case we
will need to have a fall back anyways because otherwise it should break
the old functionality.

So I don't think that using phandle for the MC device finding is really
warrant.

Phandle is kinda more applicable for the cases where only the DT node
lookup is needed (not the lookup of the MC device driver), but even then
it is also not mandatory.

I hope you agree.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7275 bytes --]

On Thu, Oct 01, 2020 at 05:11:30AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:47, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 07:25:41PM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 19:06, Thierry Reding пишет:
> >>> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
> >>>>  I'...
> >>>>>> +	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.
> >>>>
> >>>> This sounds more complicated than the current variant.
> >>>>
> >>>> Secondly, I'm already about to use the new tegra_get_memory_controller()
> >>>> API for all the T20/30/124/210 EMC and devfreq drivers.
> >>>
> >>> Why do we need it there? They seem to work fine without it right now.
> >>
> >> All the Tegra30/124/210 EMC drivers are already duplicating that MC
> >> lookup code and only the recent T210 driver does it properly.
> >>
> >>> If it is required for new functionality, we can always make the dependent
> >>> on a DT reference via phandle without breaking any existing code.
> >>
> >> That's correct, it will be also needed for the new functionality as
> >> well, hence even more drivers will need to perform the MC lookup.
> > 
> > I don't have any issues with adding a helper if we need it from several
> > different locations. But the helper should be working off of a given
> > device and look up the device via the device tree node referenced by
> > phandle. We already have those phandles in place for the EMC devices,
> > and any other device that needs to interoperate with the MC should also
> > get such a reference.
> > 
> >> I don't quite understand why you're asking for the phandle reference,
> >> it's absolutely not needed for the MC lookup and won't work for the
> > 
> > We need that phandle in order to establish a link between the devices.
> > Yes, you can probably do it without the phandle and just match by
> > compatible string. But we don't do that for other types of devices
> > either, right? For a display driver we reference the attached panel via
> > phandle, but we could also just look it up via name or absolute path or
> > some other heuristic. But a phandle is just a much more explicit way of
> > linking the devices, so why not use it?
> 
> There are dozens variants of the panels and system could easily have
> more than one panel, hence a direct lookup by phandle is a natural
> choice for the panels.

Not really, there's typically only just one panel. But that's just one
example. EMC would be another. There's only a single EMC on Tegra and
yet for something like interconnects we still reference it by phandle.
PMC is another case and so is CAR, GPIO (at least on early Tegra SoCs)
and pinmux, etc.

The example of GPIO shows very well how this is important. If we had
made the assumption from the beginning that there was only ever going to
be a single GPIO controller, then we would've had a big problem when the
first SoC shipped that had multiple GPIO controllers.

> While all Tegra SoCs have a single fixed MC in the system, and thus,
> there is no real need to use phandle because we can't mix up MC with
> anything else.

The same is true for the SMMU, and yet the iommus property references
the SMMU by phandle. There are a *lot* of cases where you could imply
dependencies because you have intimate knowledge about the hardware
within drivers. But the point is to avoid this wherever possible so
that the DTB is as self-describing as possible.

> >> older DTs if DT change will be needed. Please give a detailed explanation.
> > 
> > New functionality doesn't have to work with older DTs.
> 
> This is fine in general, but I'm afraid that in this particular case we
> will need to have a fall back anyways because otherwise it should break
> the old functionality.

It looks like tegra20-devfreq is the only one that currently does this
lookup via compatible string. And looking at the driver, what it does is
pretty horrible, to be honest. It gets a reference to the memory
controller and then simply accesses registers within the memory
controller without any type of protection against concurrent accesses or
reference counting to make sure the registers it accesses are still
valid. At the very least this should've been a regmap. And not
coincidentally, regmaps are usually passed around by referencing their
provider via phandle.

That's exactly the kind of hack that I want to prevent from happening.
If you can just grab a pointer to the memory controller with a global
function pointer it makes people think that it's okay to use this kind
of shortcut. But it isn't.

Given the above, the lookup-by-compatible fallback should stay limited
to tegra20-devfreq. Everything else should move to something saner. So
this new helper should look up by phandle and not have a fallback, but
instead the tegra20-devfreq should fall back if the new helper doesn't
return anything useful (probably something like -ENOENT, meaning that
there's no phandle and that we're using an old device tree). Bonus
points for updating the DT bindings for tegra20-devfreq to also allow
the memory controller to be specified by phandle and use a regmap for
the shared registers.

> So I don't think that using phandle for the MC device finding is really
> warrant.
> 
> Phandle is kinda more applicable for the cases where only the DT node
> lookup is needed (not the lookup of the MC device driver), but even then
> it is also not mandatory.
> 
> I hope you agree.

I strongly disagree. We look up a bunch of devices via phandle, and in
only very rare cases do we care only about the DT node. When we look up
clocks, resets or GPIOs, we always get some sort of resource handle. It
makes sense to do so because we need to operate on these resources and
having only a DT node doesn't allow us to do that.

Ultimately this is a matter of principle. Yes, it's true that there is
only a single MC on Tegra and we can "cheat" by taking advantage of that
knowledge. But we don't have to, and it's not even very difficult to not
rely on that knowledge. Treating the MC differently from everything else
also makes it more difficult to understand the associated code and on
top of that it sets a bad example because other people seeing this code
will think it's a good idea.

The big advantage of uniformity is that it drastically simplifies things
and causes less suprises.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

...
>> There are dozens variants of the panels and system could easily have
>> more than one panel, hence a direct lookup by phandle is a natural
>> choice for the panels.
> 
> Not really, there's typically only just one panel. But that's just one
> example. EMC would be another. There's only a single EMC on Tegra and
> yet for something like interconnects we still reference it by phandle.

Interconnect is a generic API.

> PMC is another case and so is CAR, GPIO (at least on early Tegra SoCs)
> and pinmux, etc.
> 
> The example of GPIO shows very well how this is important. If we had
> made the assumption from the beginning that there was only ever going to
> be a single GPIO controller, then we would've had a big problem when the
> first SoC shipped that had multiple GPIO controllers.

This is true, but only if all these words are applied to the generic APIs.

>> While all Tegra SoCs have a single fixed MC in the system, and thus,
>> there is no real need to use phandle because we can't mix up MC with
>> anything else.
> 
> The same is true for the SMMU, and yet the iommus property references
> the SMMU by phandle. There are a *lot* of cases where you could imply
> dependencies because you have intimate knowledge about the hardware
> within drivers. But the point is to avoid this wherever possible so
> that the DTB is as self-describing as possible.
> 
>>>> older DTs if DT change will be needed. Please give a detailed explanation.
>>>
>>> New functionality doesn't have to work with older DTs.
>>
>> This is fine in general, but I'm afraid that in this particular case we
>> will need to have a fall back anyways because otherwise it should break
>> the old functionality.
> 
> It looks like tegra20-devfreq is the only one that currently does this
> lookup via compatible string. And looking at the driver, what it does is
> pretty horrible, to be honest. It gets a reference to the memory
> controller and then simply accesses registers within the memory
> controller without any type of protection against concurrent accesses or
> reference counting to make sure the registers it accesses are still
> valid. At the very least this should've been a regmap.

Regmap is about abstracting accesses to devices that may sit on
different types of buses, like I2C or SPI for example. Or devices that
have a non-trivial registers mapping, or have slow IO and need caching.

I think you meant regmap in regards to protecting IO accesses, but this
is not what regmap is about if IO accesses are atomic by nature.

The tegra20-devfreq functionality is very separated from the rest of the
memory controller, hence there are no conflicts in regards to hardware
accesses, so there is nothing to protect.

Also, Regmap API itself doesn't manage refcounting of the mappings.

> And not
> coincidentally, regmaps are usually passed around by referencing their
> provider via phandle.

Any real-world examples? I think you're mixing up regmap with something
else.

The devfreq driver works just like the SMMU and GART. The devfreq device
is supposed to be created only once both MC and EMC drivers are loaded
and we know that they can't go away [1].

[1]
https://patchwork.ozlabs.org/project/linux-tegra/patch/20200814000621.8415-32-digetx@gmail.com/

Hence the tegra20-devfreq driver is horrible as much as the SMMU and
GART drivers. Perhaps not much could be done about it unless MC driver
is converted to MFD. But MFD won't work for tegra20-devfreq driver
anyways because it depends on presence of both MC and EMC drivers
simultaneously :)

Besides you didn't want the MFD couple years ago [2].

[2]
https://patchwork.ozlabs.org/project/linux-tegra/patch/675f74f82378b5f7d8f61d35e929614a0e156141.1523301400.git.digetx@gmail.com/#1902020

> That's exactly the kind of hack that I want to prevent from happening.
> If you can just grab a pointer to the memory controller with a global
> function pointer it makes people think that it's okay to use this kind
> of shortcut. But it isn't.
> Given the above, the lookup-by-compatible fallback should stay limited
> to tegra20-devfreq. Everything else should move to something saner. So
> this new helper should look up by phandle and not have a fallback, but
> instead the tegra20-devfreq should fall back if the new helper doesn't
> return anything useful (probably something like -ENOENT, meaning that
> there's no phandle and that we're using an old device tree). Bonus
> points for updating the DT bindings for tegra20-devfreq to also allow
> the memory controller to be specified by phandle and use a regmap for
> the shared registers.
The tegra20-devfreq driver doesn't share registers with other drivers.
MC statistics collection is a part of the MC, but it has no connection
to the other functions of the MC, at least from SW perspective.

Apparently you're missing that it's still not a problem to change the
T20 DT because all the MC-related drivers are still inactive in the
upstream kernel and awaiting the interconnect support addition.

Secondly, you're missing that tegra30-devfreq driver will also need to
perform the MC lookup [3] and then driver will no longer work for the
older DTs if phandle becomes mandatory because these DTs do not have the
MC phandle in the ACTMON node.

[3]
https://github.com/grate-driver/linux/commit/441d19281f9b3428a4db1ecb3a02e1ec02a8ad7f

So we will need the fall back for T30/124 as well.

>> So I don't think that using phandle for the MC device finding is really
>> warrant.
>>
>> Phandle is kinda more applicable for the cases where only the DT node
>> lookup is needed (not the lookup of the MC device driver), but even then
>> it is also not mandatory.
>>
>> I hope you agree.
> 
> I strongly disagree. We look up a bunch of devices via phandle, and in
> only very rare cases do we care only about the DT node. When we look up
> clocks, resets or GPIOs, we always get some sort of resource handle. It
> makes sense to do so because we need to operate on these resources and
> having only a DT node doesn't allow us to do that.

We don't have this problem because MC drivers do not support unloading.
Hence this is not something to really care about for now.

Maybe MC drivers won't ever support unloading and then why do we need to
discuss it at all? Should be better to get back to this once there will
be a real need.

> Ultimately this is a matter of principle. Yes, it's true that there is
> only a single MC on Tegra and we can "cheat" by taking advantage of that
> knowledge. But we don't have to, and it's not even very difficult to not
> rely on that knowledge. Treating the MC differently from everything else
> also makes it more difficult to understand the associated code and on
> top of that it sets a bad example because other people seeing this code
> will think it's a good idea.
> 
> The big advantage of uniformity is that it drastically simplifies things
> and causes less suprises.

But we aren't talking about the uniformity, at least I'm not.

To be honest, it feels to me that you're talking from perspective of
yours generic memory controller API series and want to see everything
tailored for it, which is okay :)

While I'm merely wanting to improve the existing codebase with minimal
efforts and without a need to change DTs.

Alright, I'll consider to add the phandles as a part of the interconnect
series since you're so concentrated on it :) But the series is already
40+ patches.. although, the vast majority of the patches are very
trivial, so maybe okay to add couple more trivial patches. I'm very
hoping that you will manage to allocate time for reviewing of the
upcoming v6 and that it will be good for merging into to the v5.11!

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11707 bytes --]

On Thu, Oct 01, 2020 at 10:04:30PM +0300, Dmitry Osipenko wrote:
> ...
> >> There are dozens variants of the panels and system could easily have
> >> more than one panel, hence a direct lookup by phandle is a natural
> >> choice for the panels.
> > 
> > Not really, there's typically only just one panel. But that's just one
> > example. EMC would be another. There's only a single EMC on Tegra and
> > yet for something like interconnects we still reference it by phandle.
> 
> Interconnect is a generic API.
> 
> > PMC is another case and so is CAR, GPIO (at least on early Tegra SoCs)
> > and pinmux, etc.
> > 
> > The example of GPIO shows very well how this is important. If we had
> > made the assumption from the beginning that there was only ever going to
> > be a single GPIO controller, then we would've had a big problem when the
> > first SoC shipped that had multiple GPIO controllers.
> 
> This is true, but only if all these words are applied to the generic APIs.

The reason why this is true for generic APIs is because people actually
think about generic APIs a bit more than about custom APIs because they
need to be more future-proof.

That doesn't make it wrong to think hard about using custom APIs because
we also want those to be somewhat future-proof.

> >> While all Tegra SoCs have a single fixed MC in the system, and thus,
> >> there is no real need to use phandle because we can't mix up MC with
> >> anything else.
> > 
> > The same is true for the SMMU, and yet the iommus property references
> > the SMMU by phandle. There are a *lot* of cases where you could imply
> > dependencies because you have intimate knowledge about the hardware
> > within drivers. But the point is to avoid this wherever possible so
> > that the DTB is as self-describing as possible.
> > 
> >>>> older DTs if DT change will be needed. Please give a detailed explanation.
> >>>
> >>> New functionality doesn't have to work with older DTs.
> >>
> >> This is fine in general, but I'm afraid that in this particular case we
> >> will need to have a fall back anyways because otherwise it should break
> >> the old functionality.
> > 
> > It looks like tegra20-devfreq is the only one that currently does this
> > lookup via compatible string. And looking at the driver, what it does is
> > pretty horrible, to be honest. It gets a reference to the memory
> > controller and then simply accesses registers within the memory
> > controller without any type of protection against concurrent accesses or
> > reference counting to make sure the registers it accesses are still
> > valid. At the very least this should've been a regmap.
> 
> Regmap is about abstracting accesses to devices that may sit on
> different types of buses, like I2C or SPI for example. Or devices that
> have a non-trivial registers mapping, or have slow IO and need caching.

Those are common uses, yes.

> I think you meant regmap in regards to protecting IO accesses, but this
> is not what regmap is about if IO accesses are atomic by nature.

Then why is there regmap-mmio?

> The tegra20-devfreq functionality is very separated from the rest of the
> memory controller, hence there are no conflicts in regards to hardware
> accesses, so there is nothing to protect.
> 
> Also, Regmap API itself doesn't manage refcounting of the mappings.

That may be true now, but at least it is something formal rather than
just dereferencing some pointer and accessing registers through it. If
this ever becomes a problem it's something that we can more easily
address.

> > And not
> > coincidentally, regmaps are usually passed around by referencing their
> > provider via phandle.
> 
> Any real-world examples? I think you're mixing up regmap with something
> else.

syscon is the most obvious that comes to mind. It is meant to address
the kind of use-case that tegra20-devfreq apparently needs here, where
you have registers for certain functionality that are located in a
completely different IP block from the rest of that functionality. Often
there are better alternatives to solve this, by reusing existing
infrastructure, such as pinmux.

In cases where no subsystem exists we typically use syscon, which are
implemented via regmaps, to gain access to shared registers.

> The devfreq driver works just like the SMMU and GART. The devfreq device
> is supposed to be created only once both MC and EMC drivers are loaded
> and we know that they can't go away [1].
> 
> [1]
> https://patchwork.ozlabs.org/project/linux-tegra/patch/20200814000621.8415-32-digetx@gmail.com/

Huh... why is the tegra20-devfreq device instantiated from the EMC
driver? That doesn't make any sense to me. If there aren't any registers
that the driver accesses, then it would make more sense to subsume that
functionality under some different driver (tegra20-mc most likely by
the looks of things).

On a side-note: once we move tegra20-devfreq into tegra20-mc, there's no
need for this look up at all anymore.

> Hence the tegra20-devfreq driver is horrible as much as the SMMU and
> GART drivers. Perhaps not much could be done about it unless MC driver
> is converted to MFD. But MFD won't work for tegra20-devfreq driver
> anyways because it depends on presence of both MC and EMC drivers
> simultaneously :)
> 
> Besides you didn't want the MFD couple years ago [2].
> 
> [2]
> https://patchwork.ozlabs.org/project/linux-tegra/patch/675f74f82378b5f7d8f61d35e929614a0e156141.1523301400.git.digetx@gmail.com/#1902020

That's because MFD is a misguided effort to split drivers into parts
that fit nicely into Linux' subsystems.

See the above devfreq vs. memory-controller mess for why that's a bad
idea. If you have to jump through hoops like that (i.e. create a virtual
device for a driver to bind against and then look up the device tree
node of the originator of that virtual device to get at its resources)
there's something seriously wrong.

I'm glad we caught this before this made it in.

> > That's exactly the kind of hack that I want to prevent from happening.
> > If you can just grab a pointer to the memory controller with a global
> > function pointer it makes people think that it's okay to use this kind
> > of shortcut. But it isn't.
> > Given the above, the lookup-by-compatible fallback should stay limited
> > to tegra20-devfreq. Everything else should move to something saner. So
> > this new helper should look up by phandle and not have a fallback, but
> > instead the tegra20-devfreq should fall back if the new helper doesn't
> > return anything useful (probably something like -ENOENT, meaning that
> > there's no phandle and that we're using an old device tree). Bonus
> > points for updating the DT bindings for tegra20-devfreq to also allow
> > the memory controller to be specified by phandle and use a regmap for
> > the shared registers.
> The tegra20-devfreq driver doesn't share registers with other drivers.
> MC statistics collection is a part of the MC, but it has no connection
> to the other functions of the MC, at least from SW perspective.
> 
> Apparently you're missing that it's still not a problem to change the
> T20 DT because all the MC-related drivers are still inactive in the
> upstream kernel and awaiting the interconnect support addition.

Great, that means it's not too late and we can still add that phandle.

> Secondly, you're missing that tegra30-devfreq driver will also need to
> perform the MC lookup [3] and then driver will no longer work for the
> older DTs if phandle becomes mandatory because these DTs do not have the
> MC phandle in the ACTMON node.
> 
> [3]
> https://github.com/grate-driver/linux/commit/441d19281f9b3428a4db1ecb3a02e1ec02a8ad7f
> 
> So we will need the fall back for T30/124 as well.

No, we don't need the fallback because this is new functionality which
can and should be gated on the existence of the new phandle. If there's
no phandle then we have no choice but to use the old code to ensure old
behaviour.

> 
> >> So I don't think that using phandle for the MC device finding is really
> >> warrant.
> >>
> >> Phandle is kinda more applicable for the cases where only the DT node
> >> lookup is needed (not the lookup of the MC device driver), but even then
> >> it is also not mandatory.
> >>
> >> I hope you agree.
> > 
> > I strongly disagree. We look up a bunch of devices via phandle, and in
> > only very rare cases do we care only about the DT node. When we look up
> > clocks, resets or GPIOs, we always get some sort of resource handle. It
> > makes sense to do so because we need to operate on these resources and
> > having only a DT node doesn't allow us to do that.
> 
> We don't have this problem because MC drivers do not support unloading.
> Hence this is not something to really care about for now.
> 
> Maybe MC drivers won't ever support unloading and then why do we need to
> discuss it at all? Should be better to get back to this once there will
> be a real need.

We need to discuss this now because it has implications for the DT
bindings, which, once established, we will never be able to change
again. At least not in a backwards-incompatible way.

That implies that any code we add based on that assumption will have to
be maintained in the kernel basically forever. As you see for the case
of tegra30-devfreq that means we need the existing fallback to remain
backwards-compatible with older DTBs that don't have the link to the
memory controller to get at the frequency table.

Now, this is obviously something that we should've done since the
beginning but we didn't and now we're stuck with it. But we shouldn't
further compound the problem by doing things by halves now.

> > Ultimately this is a matter of principle. Yes, it's true that there is
> > only a single MC on Tegra and we can "cheat" by taking advantage of that
> > knowledge. But we don't have to, and it's not even very difficult to not
> > rely on that knowledge. Treating the MC differently from everything else
> > also makes it more difficult to understand the associated code and on
> > top of that it sets a bad example because other people seeing this code
> > will think it's a good idea.
> > 
> > The big advantage of uniformity is that it drastically simplifies things
> > and causes less suprises.
> 
> But we aren't talking about the uniformity, at least I'm not.
> 
> To be honest, it feels to me that you're talking from perspective of
> yours generic memory controller API series and want to see everything
> tailored for it, which is okay :)

I had actually given up on that idea because it wasn't gaining any
momentum. But your point is valid for a different reason. Just because
there's currently no generic framework for memory controllers doesn't
mean there never will be, so we should design things with some
genericity in mind so that if something generic ever comes around we
have a simpler way of transitioning to it.

> While I'm merely wanting to improve the existing codebase with minimal
> efforts and without a need to change DTs.
> 
> Alright, I'll consider to add the phandles as a part of the interconnect
> series since you're so concentrated on it :) But the series is already
> 40+ patches.. although, the vast majority of the patches are very
> trivial, so maybe okay to add couple more trivial patches. I'm very
> hoping that you will manage to allocate time for reviewing of the
> upcoming v6 and that it will be good for merging into to the v5.11!

Okay, sounds good.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

05.10.2020 12:16, Thierry Reding пишет:
...
>> I think you meant regmap in regards to protecting IO accesses, but this
>> is not what regmap is about if IO accesses are atomic by nature.
> 
> Then why is there regmap-mmio?

To protect programming sequences for example, actually that's the only
real use-case I saw in kernel drivers once. In our case there are no
sequences that require protection, at least I'm not aware about that.

>> Secondly, you're missing that tegra30-devfreq driver will also need to
>> perform the MC lookup [3] and then driver will no longer work for the
>> older DTs if phandle becomes mandatory because these DTs do not have the
>> MC phandle in the ACTMON node.
>>
>> [3]
>> https://github.com/grate-driver/linux/commit/441d19281f9b3428a4db1ecb3a02e1ec02a8ad7f
>>
>> So we will need the fall back for T30/124 as well.
> 
> No, we don't need the fallback because this is new functionality which
> can and should be gated on the existence of the new phandle. If there's
> no phandle then we have no choice but to use the old code to ensure old
> behaviour.

You just repeated what I was trying to say:)

Perhaps I spent a bit too much time touching that code to the point that
lost feeling that there is a need to clarify everything in details.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2870 bytes --]

On Mon, Oct 05, 2020 at 12:38:20PM +0300, Dmitry Osipenko wrote:
> 05.10.2020 12:16, Thierry Reding пишет:
> ...
> >> I think you meant regmap in regards to protecting IO accesses, but this
> >> is not what regmap is about if IO accesses are atomic by nature.
> > 
> > Then why is there regmap-mmio?
> 
> To protect programming sequences for example, actually that's the only
> real use-case I saw in kernel drivers once. In our case there are no
> sequences that require protection, at least I'm not aware about that.

True. But I'd still prefer to have a more formal mechanism of handing
out access to registers.

Either way, this isn't very relevant in the case of tegra20-devfreq
because there's really no reason for it to be a separate driver with
device tree lookup since it's all internal to the MC driver. The only
reason (unless I'm missing something) for that is to have the code
located in drivers/devfreq. We can do that without requiring DT lookup
either like we did for tegra-smmu/tegra-mc, or by directly copying the
code into drivers/memory.

It's become fairly common in recent years to group code by IP rather
than functionality. We nowadays have good tools to help with subsystem
wide refactoring that make it much less necessary for subsystem code to
be concentrated in a single directory.

> >> Secondly, you're missing that tegra30-devfreq driver will also need to
> >> perform the MC lookup [3] and then driver will no longer work for the
> >> older DTs if phandle becomes mandatory because these DTs do not have the
> >> MC phandle in the ACTMON node.
> >>
> >> [3]
> >> https://github.com/grate-driver/linux/commit/441d19281f9b3428a4db1ecb3a02e1ec02a8ad7f
> >>
> >> So we will need the fall back for T30/124 as well.
> > 
> > No, we don't need the fallback because this is new functionality which
> > can and should be gated on the existence of the new phandle. If there's
> > no phandle then we have no choice but to use the old code to ensure old
> > behaviour.
> 
> You just repeated what I was trying to say:)
> 
> Perhaps I spent a bit too much time touching that code to the point that
> lost feeling that there is a need to clarify everything in details.

I assumed that by "fall back" you meant the lookup-by-compatible. But
what I was talking about is the fall back to the current code which does
not use the MC device tree node at all. The latter is going to be
required to ensure that the code continues to work as-is. But the former
is not required because we have fall back code that already works with
old device trees. New code that is using the memory controller's timings
nodes can be gated on the existence of the phandle in DT and doing so is
not going to break backwards-compatibility.

But perhaps I was misunderstanding what you were trying to say.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
>  I'...
> >> +	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.
> 
> This sounds more complicated than the current variant.

I don't think so. It's actually very clear and explicit. And yes, this
might be a little more work (and honestly, this is what? a handful of
lines?) than accessing a global variable, but that's a fair price to pay
for proper encapsulation.

> Secondly, I'm already about to use the new tegra_get_memory_controller()
> API for all the T20/30/124/210 EMC and devfreq drivers.

Also, this really proves the point I was trying to make about how this
is going to proliferate...

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

...
>> Secondly, I'm already about to use the new tegra_get_memory_controller()
>> API for all the T20/30/124/210 EMC and devfreq drivers.
> 
> Also, this really proves the point I was trying to make about how this
> is going to proliferate...

Sorry, I'm probably totally missing yours point.. "what" exactly will
proliferate?

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

On Wed, Sep 30, 2020 at 07:29:12PM +0300, Dmitry Osipenko wrote:
> ...
> >> Secondly, I'm already about to use the new tegra_get_memory_controller()
> >> API for all the T20/30/124/210 EMC and devfreq drivers.
> > 
> > Also, this really proves the point I was trying to make about how this
> > is going to proliferate...
> 
> Sorry, I'm probably totally missing yours point.. "what" exactly will
> proliferate?

Making use of this lookup-by-compatible mechanism. If you provide a
function to make that easy, then people are going to use it, without
even thinking about whether or not it is a good idea.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[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 <nicoleotsuka@gmail.com>
[...]
> >  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 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 != &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;
> 
> I don't think this is going to work. I distinctly remember putting this
> here because we needed access to this before ->probe_device() had been
> called for any of the devices.

Do you remember which exact part of code needs to access mc->smmu
before ->probe_device() is called?

What I understood is that IOMMU core didn't allow ERR_PTR(-ENODEV)
return value from ->probe_device(), previously ->add_device(), to
carry on when you added this code/driver:
    commit 8918465163171322c77a19d5258a95f56d89d2e4
    Author: Thierry Reding <treding@nvidia.com>
    Date:   Wed Apr 16 09:24:44 2014 +0200
        memory: Add NVIDIA Tegra memory controller support

..until the core had a change one year later:
    commit 38667f18900afe172a4fe44279b132b4140f920f
    Author: Joerg Roedel <jroedel@suse.de>
    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().

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

[Dmitry Osipenko]

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

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

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

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

[Dmitry Osipenko]

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.

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

[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, &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 = &bus->dev;
+
+	while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
+		dev = &bus->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,
 					  &args) == 0) {
 		smmu = tegra_smmu_find(args.np);
 		if (smmu) {
-			err = tegra_smmu_configure(smmu, dev, &args);
+			err = tegra_smmu_configure(smmu, dev, &args, &np->fwnode);
 			of_node_put(args.np);
 
 			if (err < 0)

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

[Dmitry Osipenko]

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.

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

[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

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

[Dmitry Osipenko]

01.10.2020 05:48, 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
> 

I think we already agreed that it will be good to have a common helper.
So far Thierry only objected that the implementation of the helper could
be improved.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3357 bytes --]

On Wed, Sep 30, 2020 at 07:48:50PM -0700, Nicolin Chen wrote:
> 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.

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

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

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

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

[Dmitry Osipenko]

01.10.2020 14:04, 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...

Not very convincing to me. I don't see a "plenty of other drivers",
there is only one arm-smmu driver.

The dev_get_dev_node() is under CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS (!).
Guys, doesn't it look strange to you? :)

The arm-smmu driver does a similar thing for the modern bindings to what
Nicolin's v3 is doing.

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

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

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

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

[Dmitry Osipenko]

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.

Always consult the IOMMU core code. If you're too unsure about
something, then maybe better to start a new thread and ask Joerg about
the best modern practices that IOMMU drivers should use.

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

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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4051 bytes --]

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.

ARM SMMU and ARM SMMU v3 do this and so does virtio-iommu. Pretty much
all the other drivers for ARM platforms have their own variations of
tegra_smmu_find() using of_find_device_by_node() at some point.

What others do differently is that they call of_find_device_by_node()
from ->of_xlate(), which is notably different from what we do in
tegra-smmu (where we call it from ->probe_device()). It's entirely
possible that we can do that as well, which is what we've been
discussing in a different sub-thread, but like I mentioned there I do
recall that being problematic, otherwise I wouldn't have left all the
comments in the code.

If we can determine that moving this to ->of_xlate() works fine in all
cases, then I think that's something that we should do for tegra-smmu to
become more consistent with other 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.
> > 
> 
> 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.

That's nonsense. There's no such thing as "modern" drivers is Linux
because they are constantly improved. Yes, ARM SMMU may have legacy code
paths, but that's because it has been around for much longer than others
and therefore is much more mature.

I can't say much about sun50i-iommu because I'm not familiar with it,
but I have seen plenty of "modern" drivers that turn out to be much
worse than "old" drivers. New doesn't always mean better.

> Always consult the IOMMU core code. If you're too unsure about
> something, then maybe better to start a new thread and ask Joerg about
> the best modern practices that IOMMU drivers should use.

This doesn't really have anything to do with the IOMMU core code. This
has to do with platform and firmware code, so the IOMMU core is only
marginally involved.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4275 bytes --]

On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote:
> 01.10.2020 14:04, 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...
> 
> Not very convincing to me. I don't see a "plenty of other drivers",
> there is only one arm-smmu driver.

There's ARM SMMU, ARM SMMU v3 and at least FSL PAMU. Even some of the
x86 platforms use dev_is_pci() to special-case PCI devices. That's just
because PCI is fundamentally different from fixed devices such as those
on a platform bus.

> The dev_get_dev_node() is under CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS (!).
> Guys, doesn't it look strange to you? :)

See above, there are other cases where PCI devices are treated special.
For example, pretty much every driver that supports PCI differentiates
between PCI and other devices in their ->device_group() callback.

> The arm-smmu driver does a similar thing for the modern bindings to what
> Nicolin's v3 is doing.

I don't really have any objections to doing something similar to what
ARM SMMU does. My main objection is to the use of a global pointer that
is used to look up the SMMU. As you see, the ARM SMMU driver also does
this lookup via driver_find_device_by_fwnode() rather than storing a
global pointer.

Also you can't quite equate ARM SMMU with Tegra SMMU. ARM SMMU can
properly deal with devices behind a PCI host bridge, whereas on Tegra
all those devices are thrown in the same bucket with the same IOMMU
domain. So it's to be expected that some things will have to be
different between the two drivers.

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

Have you also seen that sun50i-iommu does look up the SMMU from a
phandle using of_find_device_by_node()? So I think you've shown yourself
that even "modern" drivers avoid global pointers and look up via
phandle.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

05.10.2020 10:13, Thierry Reding пишет:
...
> Have you also seen that sun50i-iommu does look up the SMMU from a
> phandle using of_find_device_by_node()? So I think you've shown yourself
> that even "modern" drivers avoid global pointers and look up via
> phandle.

I have no problem with the lookup by phandle and I'm all for it. It's
now apparent to me that you completely missed my point, but that should
be my fault that I haven't conveyed it properly from the start. I just
wanted to avoid the incompatible DT changes which could break older DTs
+ I simply wanted to improve the older code without introducing new
features, that's it.

Anyways, after yours comments I started to look at how the interconnect
patches could be improved and found new things, like that OPPs now
support ICC and that EMC has a working EMC_STAT, I also discovered
syscon and simple-mfd. This means that we won't need the global pointers
at all neither for SMMU, nor for interconnect, nor for EMC drivers :)

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On Mon, Oct 05, 2020 at 11:14:27AM +0300, Dmitry Osipenko wrote:
> 05.10.2020 10:13, Thierry Reding пишет:
> ...
> > Have you also seen that sun50i-iommu does look up the SMMU from a
> > phandle using of_find_device_by_node()? So I think you've shown yourself
> > that even "modern" drivers avoid global pointers and look up via
> > phandle.
> 
> I have no problem with the lookup by phandle and I'm all for it. It's
> now apparent to me that you completely missed my point, but that should
> be my fault that I haven't conveyed it properly from the start. I just
> wanted to avoid the incompatible DT changes which could break older DTs
> + I simply wanted to improve the older code without introducing new
> features, that's it.
> 
> Anyways, after yours comments I started to look at how the interconnect
> patches could be improved and found new things, like that OPPs now
> support ICC and that EMC has a working EMC_STAT, I also discovered
> syscon and simple-mfd. This means that we won't need the global pointers
> at all neither for SMMU, nor for interconnect, nor for EMC drivers :)

Well, evidently discussion on mailing lists actually works. =)

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]

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.

Surely code size can't be the only measure of good code. You can fit the
above on even fewer lines if you sacrifice readability. In this case you
can strip away those lines because you're effectively using a global
variable.

So there's always a compromise and I think in this case it's not a good
one because we sacrifice explicit code that clearly documents what's
going on with less code that's a bit handwavy about what's happening.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]

On Wed, Sep 30, 2020 at 06:26:30PM -0700, Nicolin Chen wrote:
> 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, &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 = &bus->dev;
> +
> +	while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> +		dev = &bus->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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[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, &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 = &bus->dev;
> > +
> > +	while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> > +		dev = &bus->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?

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2836 bytes --]

On Thu, Oct 01, 2020 at 03:33:19AM -0700, Nicolin Chen wrote:
> 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, &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 = &bus->dev;
> > > +
> > > +	while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
> > > +		dev = &bus->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?

Yeah, sounds like that would be useful. If you do, perhaps also take the
opportunity to replace open-coded variants, such as the one in arm-smmu.

Either that, or open-code this in tegra-smmu, like arm-smmu does.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6370 bytes --]

On Wed, Sep 30, 2020 at 01:36:18PM -0700, Nicolin Chen wrote:
> 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 <nicoleotsuka@gmail.com>
> [...]
> > >  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 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 != &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;
> > 
> > I don't think this is going to work. I distinctly remember putting this
> > here because we needed access to this before ->probe_device() had been
> > called for any of the devices.
> 
> Do you remember which exact part of code needs to access mc->smmu
> before ->probe_device() is called?
> 
> What I understood is that IOMMU core didn't allow ERR_PTR(-ENODEV)
> return value from ->probe_device(), previously ->add_device(), to
> carry on when you added this code/driver:
>     commit 8918465163171322c77a19d5258a95f56d89d2e4
>     Author: Thierry Reding <treding@nvidia.com>
>     Date:   Wed Apr 16 09:24:44 2014 +0200
>         memory: Add NVIDIA Tegra memory controller support
> 
> ..until the core had a change one year later:
>     commit 38667f18900afe172a4fe44279b132b4140f920f
>     Author: Joerg Roedel <jroedel@suse.de>
>     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().

I think this may have been important primarily for DMA/IOMMU integration
and may not currently exhibit because we don't enable that yet.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6816 bytes --]

On Wed, Sep 30, 2020 at 01:36:18PM -0700, Nicolin Chen wrote:
> 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 <nicoleotsuka@gmail.com>
> [...]
> > >  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 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 != &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;
> > 
> > I don't think this is going to work. I distinctly remember putting this
> > here because we needed access to this before ->probe_device() had been
> > called for any of the devices.
> 
> Do you remember which exact part of code needs to access mc->smmu
> before ->probe_device() is called?
> 
> What I understood is that IOMMU core didn't allow ERR_PTR(-ENODEV)
> return value from ->probe_device(), previously ->add_device(), to
> carry on when you added this code/driver:
>     commit 8918465163171322c77a19d5258a95f56d89d2e4
>     Author: Thierry Reding <treding@nvidia.com>
>     Date:   Wed Apr 16 09:24:44 2014 +0200
>         memory: Add NVIDIA Tegra memory controller support
> 
> ..until the core had a change one year later:
>     commit 38667f18900afe172a4fe44279b132b4140f920f
>     Author: Joerg Roedel <jroedel@suse.de>
>     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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Nicolin Chen]

[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]

On Thu, Oct 01, 2020 at 12:46:14PM +0200, Thierry Reding wrote:
> > > > -	/*
> > > > -	 * This is a bit of a hack. Ideally we'd want to simply return this
> > > > -	 * value. However the IOMMU registration process will attempt to add
> > > > -	 * all devices to the IOMMU when bus_set_iommu() is called. In order
> > > > -	 * not to rely on global variables to track the IOMMU instance, we
> > > > -	 * set it here so that it can be looked up from the .probe_device()
> > > > -	 * callback via the IOMMU device's .drvdata field.
> > > > -	 */
> > > > -	mc->smmu = smmu;
> > > 
> > > I don't think this is going to work. I distinctly remember putting this
> > > here because we needed access to this before ->probe_device() had been
> > > called for any of the devices.
> > 
> > Do you remember which exact part of code needs to access mc->smmu
> > before ->probe_device() is called?
> > 
> > What I understood is that IOMMU core didn't allow ERR_PTR(-ENODEV)
> > return value from ->probe_device(), previously ->add_device(), to
> > carry on when you added this code/driver:
> >     commit 8918465163171322c77a19d5258a95f56d89d2e4
> >     Author: Thierry Reding <treding@nvidia.com>
> >     Date:   Wed Apr 16 09:24:44 2014 +0200
> >         memory: Add NVIDIA Tegra memory controller support
> > 
> > ..until the core had a change one year later:
> >     commit 38667f18900afe172a4fe44279b132b4140f920f
> >     Author: Joerg Roedel <jroedel@suse.de>
> >     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.

[-- Attachment #2: 0001-iommu-tegra-smmu-Test.patch --]
[-- Type: text/x-diff, Size: 7820 bytes --]

From 01693c8d4af5abb38bb5ede4b22590a647909868 Mon Sep 17 00:00:00 2001
From: Nicolin Chen <nicoleotsuka@gmail.com>
Date: Thu, 1 Oct 2020 17:51:26 -0700
Subject: [PATCH] iommu/tegra-smmu: Test

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 drivers/iommu/tegra-smmu.c | 141 ++++++++++++-------------------------
 drivers/memory/tegra/mc.c  |   5 +-
 include/soc/tegra/mc.h     |   4 +-
 3 files changed, 51 insertions(+), 99 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..ade952d3143c 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -61,6 +61,9 @@ struct tegra_smmu_as {
 	u32 attr;
 };
 
+static const struct iommu_ops tegra_smmu_ops;
+static struct device_driver *parent_driver;
+
 static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
 {
 	return container_of(dom, struct tegra_smmu_as, domain);
@@ -484,60 +487,50 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
 static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
 	struct tegra_smmu_as *as = to_smmu_as(domain);
-	struct device_node *np = dev->of_node;
-	struct of_phandle_args args;
 	unsigned int index = 0;
 	int err = 0;
 
-	while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
-					   &args)) {
-		unsigned int swgroup = args.args[0];
-
-		if (args.np != smmu->dev->of_node) {
-			of_node_put(args.np);
-			continue;
-		}
-
-		of_node_put(args.np);
+	if (!fwspec)
+		return -ENOENT;
 
+	for (index = 0; index < fwspec->num_ids; index++) {
 		err = tegra_smmu_as_prepare(smmu, as);
-		if (err < 0)
-			return err;
+		if (err)
+			goto disable;
 
-		tegra_smmu_enable(smmu, swgroup, as->id);
-		index++;
+		tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
 	}
 
 	if (index == 0)
 		return -ENODEV;
 
 	return 0;
+
+disable:
+	while (index--) {
+		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+		tegra_smmu_as_unprepare(smmu, as);
+	}
+
+	return err;
 }
 
 static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
 {
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct tegra_smmu_as *as = to_smmu_as(domain);
-	struct device_node *np = dev->of_node;
 	struct tegra_smmu *smmu = as->smmu;
-	struct of_phandle_args args;
 	unsigned int index = 0;
 
-	while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
-					   &args)) {
-		unsigned int swgroup = args.args[0];
-
-		if (args.np != smmu->dev->of_node) {
-			of_node_put(args.np);
-			continue;
-		}
-
-		of_node_put(args.np);
+	if (!fwspec)
+		return;
 
-		tegra_smmu_disable(smmu, swgroup, as->id);
+	for (index = 0; index < fwspec->num_ids; index++) {
+		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
 		tegra_smmu_as_unprepare(smmu, as);
-		index++;
 	}
 }
 
@@ -807,79 +800,40 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct iommu_domain *domain,
 	return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
 }
 
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
+static struct tegra_smmu *tegra_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
 {
-	struct platform_device *pdev;
 	struct tegra_mc *mc;
+	struct device *dev;
+
+	if (!parent_driver || !fwnode)
+		return NULL;
 
-	pdev = of_find_device_by_node(np);
-	if (!pdev)
+	dev = driver_find_device_by_fwnode(parent_driver, fwnode);
+	if (!dev)
 		return NULL;
 
-	mc = platform_get_drvdata(pdev);
+	put_device(dev);
+	mc = dev_get_drvdata(dev);
 	if (!mc)
 		return NULL;
 
 	return mc->smmu;
 }
 
-static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
-				struct of_phandle_args *args)
-{
-	const struct iommu_ops *ops = smmu->iommu.ops;
-	int err;
-
-	err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
-	if (err < 0) {
-		dev_err(dev, "failed to initialize fwspec: %d\n", err);
-		return err;
-	}
-
-	err = ops->of_xlate(dev, args);
-	if (err < 0) {
-		dev_err(dev, "failed to parse SW group ID: %d\n", err);
-		iommu_fwspec_free(dev);
-		return err;
-	}
-
-	return 0;
-}
-
 static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 {
-	struct device_node *np = dev->of_node;
-	struct tegra_smmu *smmu = NULL;
-	struct of_phandle_args args;
-	unsigned int index = 0;
-	int err;
-
-	while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
-					  &args) == 0) {
-		smmu = tegra_smmu_find(args.np);
-		if (smmu) {
-			err = tegra_smmu_configure(smmu, dev, &args);
-			of_node_put(args.np);
-
-			if (err < 0)
-				return ERR_PTR(err);
-
-			/*
-			 * Only a single IOMMU master interface is currently
-			 * supported by the Linux kernel, so abort after the
-			 * first match.
-			 */
-			dev_iommu_priv_set(dev, smmu);
-
-			break;
-		}
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct tegra_smmu *smmu;
 
-		of_node_put(args.np);
-		index++;
-	}
+	if (!fwspec)
+		return ERR_PTR(-ENODEV);
 
+	smmu = tegra_smmu_get_by_fwnode(fwspec->iommu_fwnode);
 	if (!smmu)
 		return ERR_PTR(-ENODEV);
 
+	dev_iommu_priv_set(dev, smmu);
+
 	return &smmu->iommu;
 }
 
@@ -1078,27 +1032,22 @@ static void tegra_smmu_debugfs_exit(struct tegra_smmu *smmu)
 
 struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 				    const struct tegra_smmu_soc *soc,
-				    struct tegra_mc *mc)
+				    struct tegra_mc *mc,
+				    struct platform_driver *driver)
 {
 	struct tegra_smmu *smmu;
 	size_t size;
 	u32 value;
 	int err;
 
+	if (!driver)
+		return ERR_PTR(-ENODEV);
+	parent_driver = &driver->driver;
+
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
 	if (!smmu)
 		return ERR_PTR(-ENOMEM);
 
-	/*
-	 * This is a bit of a hack. Ideally we'd want to simply return this
-	 * value. However the IOMMU registration process will attempt to add
-	 * all devices to the IOMMU when bus_set_iommu() is called. In order
-	 * not to rely on global variables to track the IOMMU instance, we
-	 * set it here so that it can be looked up from the .probe_device()
-	 * callback via the IOMMU device's .drvdata field.
-	 */
-	mc->smmu = smmu;
-
 	size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
 
 	smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index ec8403557ed4..586f9134c5b5 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -19,6 +19,8 @@
 
 #include "mc.h"
 
+static struct platform_driver tegra_mc_driver;
+
 static const struct of_device_id tegra_mc_of_match[] = {
 #ifdef CONFIG_ARCH_TEGRA_2x_SOC
 	{ .compatible = "nvidia,tegra20-mc-gart", .data = &tegra20_mc_soc },
@@ -682,7 +684,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
 			err);
 
 	if (IS_ENABLED(CONFIG_TEGRA_IOMMU_SMMU) && mc->soc->smmu) {
-		mc->smmu = tegra_smmu_probe(&pdev->dev, mc->soc->smmu, mc);
+		mc->smmu = tegra_smmu_probe(&pdev->dev, mc->soc->smmu, mc,
+					    &tegra_mc_driver);
 		if (IS_ERR(mc->smmu)) {
 			dev_err(&pdev->dev, "failed to probe SMMU: %ld\n",
 				PTR_ERR(mc->smmu));
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1238e35653d1..763738f0a5ee 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -80,12 +80,12 @@ struct gart_device;
 #ifdef CONFIG_TEGRA_IOMMU_SMMU
 struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 				    const struct tegra_smmu_soc *soc,
-				    struct tegra_mc *mc);
+				    struct tegra_mc *mc, struct platform_driver *driver);
 void tegra_smmu_remove(struct tegra_smmu *smmu);
 #else
 static inline struct tegra_smmu *
 tegra_smmu_probe(struct device *dev, const struct tegra_smmu_soc *soc,
-		 struct tegra_mc *mc)
+		 struct tegra_mc *mc, struct platform_driver *driver)
 {
 	return NULL;
 }
-- 
2.17.1

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

[Nicolin Chen]

This patch simply adds support for PCI devices.

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

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 <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>
@@ -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(&smmu->lock);
@@ -1079,22 +1084,34 @@ 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)
-- 
2.17.1

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

[Dmitry Osipenko]

...
> +#ifdef CONFIG_PCI
> +	if (!iommu_present(&pci_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.

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

[Nicolin Chen]

On Wed, Sep 30, 2020 at 05:53:20PM +0300, Dmitry Osipenko wrote:
> ...
> > +#ifdef CONFIG_PCI
> > +	if (!iommu_present(&pci_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, okay....I will remove it upon testing.