* [PATCH v3 0/3] iommu/tegra-smmu: Add PCI support @ 2020-09-30 8:42 Nicolin Chen 2020-09-30 8:42 ` [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen ` (2 more replies) 0 siblings, 3 replies; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 8:42 UTC (permalink / raw) To: thierry.reding, joro, krzk, digetx Cc: linux-tegra, linux-kernel, iommu, jonathanh This series is to add PCI support with three changes: PATCH-1 adds a helper function to get mc pointer PATCH-2 adds support for clients that don't exist in DTB PATCH-3 adds PCI support accordingly Changelog (Detail in each patch) v2->v3 * Replaced with devm_tegra_get_memory_controller * Updated changes by following Dmitry's comments v1->v2 * Added PATCH-1 suggested by Dmitry * Reworked PATCH-2 to unify certain code Dmitry Osipenko (1): memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen (2): iommu/tegra-smmu: Rework .probe_device and .attach_dev iommu/tegra-smmu: Add PCI support drivers/iommu/tegra-smmu.c | 179 +++++++++++++------------------------ drivers/memory/tegra/mc.c | 39 ++++++++ include/soc/tegra/mc.h | 17 ++++ 3 files changed, 118 insertions(+), 117 deletions(-) -- 2.17.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 8:42 [PATCH v3 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen @ 2020-09-30 8:42 ` Nicolin Chen 2020-09-30 9:07 ` Krzysztof Kozlowski ` (2 more replies) 2020-09-30 8:42 ` [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev Nicolin Chen 2020-09-30 8:42 ` [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2 siblings, 3 replies; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 8:42 UTC (permalink / raw) To: thierry.reding, joro, krzk, digetx Cc: linux-tegra, linux-kernel, iommu, jonathanh 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 8:42 ` [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen @ 2020-09-30 9:07 ` Krzysztof Kozlowski 2020-09-30 9:41 ` Nicolin Chen 2020-09-30 14:41 ` Dmitry Osipenko 2020-09-30 15:23 ` Thierry Reding 2 siblings, 1 reply; 66+ messages in thread From: Krzysztof Kozlowski @ 2020-09-30 9:07 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, jonathanh, thierry.reding, linux-tegra, digetx "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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 9:07 ` Krzysztof Kozlowski @ 2020-09-30 9:41 ` Nicolin Chen 2020-09-30 10:27 ` Krzysztof Kozlowski 0 siblings, 1 reply; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 9:41 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: linux-kernel, iommu, jonathanh, thierry.reding, linux-tegra, digetx 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! _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 9:41 ` Nicolin Chen @ 2020-09-30 10:27 ` Krzysztof Kozlowski 0 siblings, 0 replies; 66+ messages in thread From: Krzysztof Kozlowski @ 2020-09-30 10:27 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, jonathanh, thierry.reding, linux-tegra, digetx 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 8:42 ` [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen 2020-09-30 9:07 ` Krzysztof Kozlowski @ 2020-09-30 14:41 ` Dmitry Osipenko 2020-09-30 14:45 ` Krzysztof Kozlowski 2020-09-30 15:23 ` Thierry Reding 2 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 14:41 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro, krzk Cc: linux-tegra, linux-kernel, iommu, jonathanh ... > +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 :) _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 14:41 ` Dmitry Osipenko @ 2020-09-30 14:45 ` Krzysztof Kozlowski 2020-09-30 15:22 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Krzysztof Kozlowski @ 2020-09-30 14:45 UTC (permalink / raw) To: Dmitry Osipenko Cc: linux-kernel, iommu, jonathanh, thierry.reding, linux-tegra 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 14:45 ` Krzysztof Kozlowski @ 2020-09-30 15:22 ` Dmitry Osipenko 0 siblings, 0 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 15:22 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: linux-kernel, iommu, jonathanh, thierry.reding, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 8:42 ` [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen 2020-09-30 9:07 ` Krzysztof Kozlowski 2020-09-30 14:41 ` Dmitry Osipenko @ 2020-09-30 15:23 ` Thierry Reding 2020-09-30 15:27 ` Dmitry Osipenko 2020-09-30 15:53 ` Dmitry Osipenko 2 siblings, 2 replies; 66+ messages in thread From: Thierry Reding @ 2020-09-30 15:23 UTC (permalink / raw) To: Nicolin Chen; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, digetx [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 15:23 ` Thierry Reding @ 2020-09-30 15:27 ` Dmitry Osipenko 2020-09-30 15:53 ` Dmitry Osipenko 1 sibling, 0 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 15:27 UTC (permalink / raw) To: Thierry Reding, Nicolin Chen Cc: linux-kernel, krzk, jonathanh, iommu, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 15:23 ` Thierry Reding 2020-09-30 15:27 ` Dmitry Osipenko @ 2020-09-30 15:53 ` Dmitry Osipenko 2020-09-30 16:03 ` Thierry Reding 1 sibling, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 15:53 UTC (permalink / raw) To: Thierry Reding, Nicolin Chen Cc: linux-kernel, krzk, jonathanh, iommu, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 15:53 ` Dmitry Osipenko @ 2020-09-30 16:03 ` Thierry Reding 2020-09-30 16:06 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-09-30 16:03 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 16:03 ` Thierry Reding @ 2020-09-30 16:06 ` Dmitry Osipenko 2020-09-30 16:15 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 16:06 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra 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(). _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 16:06 ` Dmitry Osipenko @ 2020-09-30 16:15 ` Thierry Reding 2020-09-30 16:26 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-09-30 16:15 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 16:15 ` Thierry Reding @ 2020-09-30 16:26 ` Dmitry Osipenko 2020-09-30 16:38 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 16:26 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 16:26 ` Dmitry Osipenko @ 2020-09-30 16:38 ` Thierry Reding 2020-09-30 17:32 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-09-30 16:38 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() 2020-09-30 16:38 ` Thierry Reding @ 2020-09-30 17:32 ` Dmitry Osipenko 0 siblings, 0 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 17:32 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra ... >> 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 8:42 [PATCH v3 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2020-09-30 8:42 ` [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen @ 2020-09-30 8:42 ` Nicolin Chen 2020-09-30 9:21 ` Krzysztof Kozlowski ` (3 more replies) 2020-09-30 8:42 ` [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2 siblings, 4 replies; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 8:42 UTC (permalink / raw) To: thierry.reding, joro, krzk, digetx Cc: linux-tegra, linux-kernel, iommu, jonathanh 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 8:42 ` [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev Nicolin Chen @ 2020-09-30 9:21 ` Krzysztof Kozlowski 2020-09-30 9:40 ` Nicolin Chen 2020-09-30 14:41 ` Dmitry Osipenko ` (2 subsequent siblings) 3 siblings, 1 reply; 66+ messages in thread From: Krzysztof Kozlowski @ 2020-09-30 9:21 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, jonathanh, thierry.reding, linux-tegra, digetx 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 9:21 ` Krzysztof Kozlowski @ 2020-09-30 9:40 ` Nicolin Chen 2020-09-30 10:19 ` Krzysztof Kozlowski 0 siblings, 1 reply; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 9:40 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: linux-kernel, iommu, jonathanh, thierry.reding, linux-tegra, digetx 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 9:40 ` Nicolin Chen @ 2020-09-30 10:19 ` Krzysztof Kozlowski 0 siblings, 0 replies; 66+ messages in thread From: Krzysztof Kozlowski @ 2020-09-30 10:19 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, jonathanh, thierry.reding, linux-tegra, digetx 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 8:42 ` [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev Nicolin Chen 2020-09-30 9:21 ` Krzysztof Kozlowski @ 2020-09-30 14:41 ` Dmitry Osipenko 2020-09-30 15:09 ` Dmitry Osipenko 2020-09-30 15:31 ` Thierry Reding 3 siblings, 0 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 14:41 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro, krzk Cc: linux-tegra, linux-kernel, iommu, jonathanh ... > + 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 8:42 ` [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev Nicolin Chen 2020-09-30 9:21 ` Krzysztof Kozlowski 2020-09-30 14:41 ` Dmitry Osipenko @ 2020-09-30 15:09 ` Dmitry Osipenko 2020-09-30 20:51 ` Nicolin Chen 2020-09-30 15:31 ` Thierry Reding 3 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 15:09 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro, krzk Cc: linux-tegra, linux-kernel, iommu, jonathanh ... > 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 15:09 ` Dmitry Osipenko @ 2020-09-30 20:51 ` Nicolin Chen 0 siblings, 0 replies; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 20:51 UTC (permalink / raw) To: Dmitry Osipenko Cc: linux-kernel, iommu, krzk, jonathanh, thierry.reding, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 8:42 ` [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev Nicolin Chen ` (2 preceding siblings ...) 2020-09-30 15:09 ` Dmitry Osipenko @ 2020-09-30 15:31 ` Thierry Reding 2020-09-30 15:36 ` Dmitry Osipenko 2020-09-30 20:36 ` Nicolin Chen 3 siblings, 2 replies; 66+ messages in thread From: Thierry Reding @ 2020-09-30 15:31 UTC (permalink / raw) To: Nicolin Chen; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, digetx [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 15:31 ` Thierry Reding @ 2020-09-30 15:36 ` Dmitry Osipenko 2020-09-30 16:06 ` Thierry Reding 2020-09-30 16:10 ` Thierry Reding 2020-09-30 20:36 ` Nicolin Chen 1 sibling, 2 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 15:36 UTC (permalink / raw) To: Thierry Reding, Nicolin Chen Cc: linux-kernel, krzk, jonathanh, iommu, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 15:36 ` Dmitry Osipenko @ 2020-09-30 16:06 ` Thierry Reding 2020-09-30 16:25 ` Dmitry Osipenko 2020-09-30 16:10 ` Thierry Reding 1 sibling, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-09-30 16:06 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 16:06 ` Thierry Reding @ 2020-09-30 16:25 ` Dmitry Osipenko 2020-09-30 16:47 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 16:25 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 16:25 ` Dmitry Osipenko @ 2020-09-30 16:47 ` Thierry Reding 2020-10-01 2:11 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-09-30 16:47 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 16:47 ` Thierry Reding @ 2020-10-01 2:11 ` Dmitry Osipenko 2020-10-01 7:58 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-10-01 2:11 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 2:11 ` Dmitry Osipenko @ 2020-10-01 7:58 ` Thierry Reding 2020-10-01 19:04 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-10-01 7:58 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 7:58 ` Thierry Reding @ 2020-10-01 19:04 ` Dmitry Osipenko 2020-10-05 9:16 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-10-01 19:04 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra ... >> 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! _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 19:04 ` Dmitry Osipenko @ 2020-10-05 9:16 ` Thierry Reding 2020-10-05 9:38 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-10-05 9:16 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-05 9:16 ` Thierry Reding @ 2020-10-05 9:38 ` Dmitry Osipenko 2020-10-05 10:27 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-10-05 9:38 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-05 9:38 ` Dmitry Osipenko @ 2020-10-05 10:27 ` Thierry Reding 0 siblings, 0 replies; 66+ messages in thread From: Thierry Reding @ 2020-10-05 10:27 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 15:36 ` Dmitry Osipenko 2020-09-30 16:06 ` Thierry Reding @ 2020-09-30 16:10 ` Thierry Reding 2020-09-30 16:29 ` Dmitry Osipenko 1 sibling, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-09-30 16:10 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 16:10 ` Thierry Reding @ 2020-09-30 16:29 ` Dmitry Osipenko 2020-10-01 7:59 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 16:29 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra ... >> 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? _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 16:29 ` Dmitry Osipenko @ 2020-10-01 7:59 ` Thierry Reding 0 siblings, 0 replies; 66+ messages in thread From: Thierry Reding @ 2020-10-01 7:59 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 15:31 ` Thierry Reding 2020-09-30 15:36 ` Dmitry Osipenko @ 2020-09-30 20:36 ` Nicolin Chen 2020-09-30 21:24 ` Dmitry Osipenko ` (2 more replies) 1 sibling, 3 replies; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 20:36 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, digetx 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(). _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 20:36 ` Nicolin Chen @ 2020-09-30 21:24 ` Dmitry Osipenko 2020-09-30 21:32 ` Nicolin Chen 2020-10-01 9:47 ` Thierry Reding 2020-10-01 10:46 ` Thierry Reding 2 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 21:24 UTC (permalink / raw) To: Nicolin Chen, Thierry Reding Cc: linux-kernel, krzk, jonathanh, iommu, linux-tegra ... >> 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 21:24 ` Dmitry Osipenko @ 2020-09-30 21:32 ` Nicolin Chen 2020-09-30 21:56 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 21:32 UTC (permalink / raw) To: Dmitry Osipenko Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote: > ... > >> It looks to me like the only reason why you need this new global API is > >> because PCI devices may not have a device tree node with a phandle to > >> the IOMMU. However, SMMU support for PCI will only be enabled if the > >> root complex has an iommus property, right? In that case, can't we > >> simply do something like this: > >> > >> if (dev_is_pci(dev)) > >> np = find_host_bridge(dev)->of_node; > >> else > >> np = dev->of_node; > >> > >> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty > >> sure that exists. > >> > >> Once we have that we can still iterate over the iommus property and do > >> not need to rely on this global variable. > > > > I agree that it'd work. But I was hoping to simplify the code > > here if it's possible. Looks like we have an argument on this > > so I will choose to go with your suggestion above for now. > > This patch removed more lines than were added. If this will be opposite > for the Thierry's suggestion, then it's probably not a great suggestion. Sorry, I don't quite understand this comments. Would you please elaborate what's this "it" being "not a great suggestion"? _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 21:32 ` Nicolin Chen @ 2020-09-30 21:56 ` Dmitry Osipenko 2020-10-01 1:26 ` Nicolin Chen 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 21:56 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 21:56 ` Dmitry Osipenko @ 2020-10-01 1:26 ` Nicolin Chen 2020-10-01 2:06 ` Dmitry Osipenko 2020-10-01 9:51 ` Thierry Reding 0 siblings, 2 replies; 66+ messages in thread From: Nicolin Chen @ 2020-10-01 1:26 UTC (permalink / raw) To: Dmitry Osipenko Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra 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) _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 1:26 ` Nicolin Chen @ 2020-10-01 2:06 ` Dmitry Osipenko 2020-10-01 2:48 ` Nicolin Chen 2020-10-01 9:54 ` Thierry Reding 2020-10-01 9:51 ` Thierry Reding 1 sibling, 2 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-10-01 2:06 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 2:06 ` Dmitry Osipenko @ 2020-10-01 2:48 ` Nicolin Chen 2020-10-01 4:04 ` Dmitry Osipenko 2020-10-01 10:23 ` Thierry Reding 2020-10-01 9:54 ` Thierry Reding 1 sibling, 2 replies; 66+ messages in thread From: Nicolin Chen @ 2020-10-01 2:48 UTC (permalink / raw) To: Dmitry Osipenko Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra On Thu, Oct 01, 2020 at 05:06:19AM +0300, Dmitry Osipenko wrote: > 01.10.2020 04:26, Nicolin Chen пишет: > > On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote: > >> 01.10.2020 00:32, Nicolin Chen пишет: > >>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote: > >>>> ... > >>>>>> It looks to me like the only reason why you need this new global API is > >>>>>> because PCI devices may not have a device tree node with a phandle to > >>>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the > >>>>>> root complex has an iommus property, right? In that case, can't we > >>>>>> simply do something like this: > >>>>>> > >>>>>> if (dev_is_pci(dev)) > >>>>>> np = find_host_bridge(dev)->of_node; > >>>>>> else > >>>>>> np = dev->of_node; > >>>>>> > >>>>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty > >>>>>> sure that exists. > >>>>>> > >>>>>> Once we have that we can still iterate over the iommus property and do > >>>>>> not need to rely on this global variable. > >>>>> > >>>>> I agree that it'd work. But I was hoping to simplify the code > >>>>> here if it's possible. Looks like we have an argument on this > >>>>> so I will choose to go with your suggestion above for now. > >>>> > >>>> This patch removed more lines than were added. If this will be opposite > >>>> for the Thierry's suggestion, then it's probably not a great suggestion. > >>> > >>> Sorry, I don't quite understand this comments. Would you please > >>> elaborate what's this "it" being "not a great suggestion"? > >>> > >> > >> I meant that you should try to implement Thierry's solution, but if the > >> end result will be worse than the current patch, then you shouldn't make > >> a v4, but get back to this discussion in order to choose the best option > >> and make everyone agree on it. > > > > I see. Thanks for the reply. And here is a sample implementation: > > That's what I supposed to happen :) The new variant adds code and > complexity, while old did the opposite. Hence the old variant is clearly > more attractive, IMO. I personally am not a fan of adding a path for PCI device either, since PCI/IOMMU cores could have taken care of it while the same path can't be used for other buses. If we can't come to an agreement on globalizing mc pointer, would it be possible to pass tegra_mc_driver through tegra_smmu_probe() so we can continue to use driver_find_device_by_fwnode() as v1? v1: https://lkml.org/lkml/2020/9/26/68 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 2:48 ` Nicolin Chen @ 2020-10-01 4:04 ` Dmitry Osipenko 2020-10-01 10:23 ` Thierry Reding 1 sibling, 0 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-10-01 4:04 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 2:48 ` Nicolin Chen 2020-10-01 4:04 ` Dmitry Osipenko @ 2020-10-01 10:23 ` Thierry Reding 2020-10-01 11:04 ` Nicolin Chen 1 sibling, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-10-01 10:23 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, Dmitry Osipenko [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 10:23 ` Thierry Reding @ 2020-10-01 11:04 ` Nicolin Chen 2020-10-01 20:33 ` Dmitry Osipenko 0 siblings, 1 reply; 66+ messages in thread From: Nicolin Chen @ 2020-10-01 11:04 UTC (permalink / raw) To: Thierry Reding Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, Dmitry Osipenko On Thu, Oct 01, 2020 at 12:23:16PM +0200, Thierry Reding wrote: > > >>>>>> It looks to me like the only reason why you need this new global API is > > > >>>>>> because PCI devices may not have a device tree node with a phandle to > > > >>>>>> the IOMMU. However, SMMU support for PCI will only be enabled if the > > > >>>>>> root complex has an iommus property, right? In that case, can't we > > > >>>>>> simply do something like this: > > > >>>>>> > > > >>>>>> if (dev_is_pci(dev)) > > > >>>>>> np = find_host_bridge(dev)->of_node; > > > >>>>>> else > > > >>>>>> np = dev->of_node; > > I personally am not a fan of adding a path for PCI device either, > > since PCI/IOMMU cores could have taken care of it while the same > > path can't be used for other buses. > > There's already plenty of other drivers that do something similar to > this. Take a look at the arm-smmu driver, for example, which seems to be > doing exactly the same thing to finding the right device tree node to > look at (see dev_get_dev_node() in drivers/iommu/arm-smmu/arm-smmu.c). Hmm..okay..that is quite convincing then... > > If we can't come to an agreement on globalizing mc pointer, would > > it be possible to pass tegra_mc_driver through tegra_smmu_probe() > > so we can continue to use driver_find_device_by_fwnode() as v1? > > > > v1: https://lkml.org/lkml/2020/9/26/68 > > tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean > tegra_smmu_probe_device()? I don't think we can do that because it isn't I was saying to have a global parent_driver pointer: similar to my v1, yet rather than "extern" the tegra_mc_driver, we pass it through egra_smmu_probe() and store it in a static global value so as to call tegra_smmu_get_by_fwnode() in ->probe_device(). Though I agree that creating a global device pointer (mc) might be controversial, yet having a global parent_driver pointer may not be against the rule, considering that it is common in iommu drivers to call driver_find_device_by_fwnode in probe_device(). > known at that point whether MC really is the SMMU. That's in fact the > whole reason why we have to go through this whole dance of iterating > over the iommus entries to find the SMMU. Hmm..I don't quite get the meaning of: "it isn't known at that point whether MC really is the SMMU". Are you saying the stage of bus_set_iommu()? So because at that point either SMMU probe() or MC probe() hasn't finished yet? Thanks! _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 11:04 ` Nicolin Chen @ 2020-10-01 20:33 ` Dmitry Osipenko 2020-10-02 1:07 ` Nicolin Chen 2020-10-05 7:13 ` Thierry Reding 0 siblings, 2 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-10-01 20:33 UTC (permalink / raw) To: Nicolin Chen, Thierry Reding Cc: linux-kernel, krzk, jonathanh, iommu, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 20:33 ` Dmitry Osipenko @ 2020-10-02 1:07 ` Nicolin Chen 2020-10-02 1:55 ` Dmitry Osipenko 2020-10-05 7:13 ` Thierry Reding 1 sibling, 1 reply; 66+ messages in thread From: Nicolin Chen @ 2020-10-02 1:07 UTC (permalink / raw) To: Dmitry Osipenko Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote: > >>> If we can't come to an agreement on globalizing mc pointer, would > >>> it be possible to pass tegra_mc_driver through tegra_smmu_probe() > >>> so we can continue to use driver_find_device_by_fwnode() as v1? > >>> > >>> v1: https://lkml.org/lkml/2020/9/26/68 > >> > >> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean > >> tegra_smmu_probe_device()? I don't think we can do that because it isn't > > > > I was saying to have a global parent_driver pointer: similar to > > my v1, yet rather than "extern" the tegra_mc_driver, we pass it > > through egra_smmu_probe() and store it in a static global value > > so as to call tegra_smmu_get_by_fwnode() in ->probe_device(). > > > > Though I agree that creating a global device pointer (mc) might > > be controversial, yet having a global parent_driver pointer may > > not be against the rule, considering that it is common in iommu > > drivers to call driver_find_device_by_fwnode in probe_device(). > > You don't need the global pointer if you have SMMU OF node. > > You could also get driver pointer from mc->dev->driver. > > But I don't think you need to do this at all. The probe_device() could > be invoked only for the tegra_smmu_ops and then seems you could use > dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver > does. Getting iommu device pointer using driver_find_device_by_fwnode() is a common practice in ->probe_device() of other iommu drivers. But this requires a device_driver pointer that tegra-smmu doesn't have. So passing tegra_mc_driver through tegra_smmu_probe() will address it. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-02 1:07 ` Nicolin Chen @ 2020-10-02 1:55 ` Dmitry Osipenko 2020-10-02 2:54 ` Nicolin Chen 2020-10-05 7:24 ` Thierry Reding 0 siblings, 2 replies; 66+ messages in thread From: Dmitry Osipenko @ 2020-10-02 1:55 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-02 1:55 ` Dmitry Osipenko @ 2020-10-02 2:54 ` Nicolin Chen 2020-10-05 7:24 ` Thierry Reding 1 sibling, 0 replies; 66+ messages in thread From: Nicolin Chen @ 2020-10-02 2:54 UTC (permalink / raw) To: Dmitry Osipenko Cc: linux-kernel, iommu, krzk, jonathanh, Thierry Reding, linux-tegra On Fri, Oct 02, 2020 at 04:55:34AM +0300, Dmitry Osipenko wrote: > 02.10.2020 04:07, Nicolin Chen пишет: > > On Thu, Oct 01, 2020 at 11:33:38PM +0300, Dmitry Osipenko wrote: > >>>>> If we can't come to an agreement on globalizing mc pointer, would > >>>>> it be possible to pass tegra_mc_driver through tegra_smmu_probe() > >>>>> so we can continue to use driver_find_device_by_fwnode() as v1? > >>>>> > >>>>> v1: https://lkml.org/lkml/2020/9/26/68 > >>>> > >>>> tegra_smmu_probe() already takes a struct tegra_mc *. Did you mean > >>>> tegra_smmu_probe_device()? I don't think we can do that because it isn't > >>> > >>> I was saying to have a global parent_driver pointer: similar to > >>> my v1, yet rather than "extern" the tegra_mc_driver, we pass it > >>> through egra_smmu_probe() and store it in a static global value > >>> so as to call tegra_smmu_get_by_fwnode() in ->probe_device(). > >>> > >>> Though I agree that creating a global device pointer (mc) might > >>> be controversial, yet having a global parent_driver pointer may > >>> not be against the rule, considering that it is common in iommu > >>> drivers to call driver_find_device_by_fwnode in probe_device(). > >> > >> You don't need the global pointer if you have SMMU OF node. > >> > >> You could also get driver pointer from mc->dev->driver. > >> > >> But I don't think you need to do this at all. The probe_device() could > >> be invoked only for the tegra_smmu_ops and then seems you could use > >> dev_iommu_priv_set() in tegra_smmu_of_xlate(), like sun50i-iommu driver > >> does. > > > > Getting iommu device pointer using driver_find_device_by_fwnode() > > is a common practice in ->probe_device() of other iommu drivers. > > Please give me a full list of the IOMMU drivers which use this method. > > > But this requires a device_driver pointer that tegra-smmu doesn't > > have. So passing tegra_mc_driver through tegra_smmu_probe() will > > address it. > > > > If you're borrowing code and ideas from other drivers, then at least > please borrow them from a modern good-looking drivers. And I already > pointed out that following cargo cult is not always a good idea. > > ARM-SMMU isn't a modern driver and it has legacy code. You shouldn't > copy it blindly. The sun50i-iommu driver was added half year ago, you > may use it as a reference. I took a closer look at sun50i-iommu driver. It's a good idea. I think I can come up with a cleaner one. Will send v4. Thanks for the advice. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-02 1:55 ` Dmitry Osipenko 2020-10-02 2:54 ` Nicolin Chen @ 2020-10-05 7:24 ` Thierry Reding 1 sibling, 0 replies; 66+ messages in thread From: Thierry Reding @ 2020-10-05 7:24 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 20:33 ` Dmitry Osipenko 2020-10-02 1:07 ` Nicolin Chen @ 2020-10-05 7:13 ` Thierry Reding 2020-10-05 8:14 ` Dmitry Osipenko 1 sibling, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-10-05 7:13 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-05 7:13 ` Thierry Reding @ 2020-10-05 8:14 ` Dmitry Osipenko 2020-10-05 9:31 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-10-05 8:14 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra 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 :) _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-05 8:14 ` Dmitry Osipenko @ 2020-10-05 9:31 ` Thierry Reding 0 siblings, 0 replies; 66+ messages in thread From: Thierry Reding @ 2020-10-05 9:31 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 2:06 ` Dmitry Osipenko 2020-10-01 2:48 ` Nicolin Chen @ 2020-10-01 9:54 ` Thierry Reding 1 sibling, 0 replies; 66+ messages in thread From: Thierry Reding @ 2020-10-01 9:54 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 1:26 ` Nicolin Chen 2020-10-01 2:06 ` Dmitry Osipenko @ 2020-10-01 9:51 ` Thierry Reding 2020-10-01 10:33 ` Nicolin Chen 1 sibling, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-10-01 9:51 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, Dmitry Osipenko [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 9:51 ` Thierry Reding @ 2020-10-01 10:33 ` Nicolin Chen 2020-10-01 10:42 ` Thierry Reding 0 siblings, 1 reply; 66+ messages in thread From: Nicolin Chen @ 2020-10-01 10:33 UTC (permalink / raw) To: Thierry Reding Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, Dmitry Osipenko 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? _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 10:33 ` Nicolin Chen @ 2020-10-01 10:42 ` Thierry Reding 0 siblings, 0 replies; 66+ messages in thread From: Thierry Reding @ 2020-10-01 10:42 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, Dmitry Osipenko [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 20:36 ` Nicolin Chen 2020-09-30 21:24 ` Dmitry Osipenko @ 2020-10-01 9:47 ` Thierry Reding 2020-10-01 10:46 ` Thierry Reding 2 siblings, 0 replies; 66+ messages in thread From: Thierry Reding @ 2020-10-01 9:47 UTC (permalink / raw) To: Nicolin Chen; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, digetx [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-09-30 20:36 ` Nicolin Chen 2020-09-30 21:24 ` Dmitry Osipenko 2020-10-01 9:47 ` Thierry Reding @ 2020-10-01 10:46 ` Thierry Reding 2020-10-02 1:29 ` Nicolin Chen 2 siblings, 1 reply; 66+ messages in thread From: Thierry Reding @ 2020-10-01 10:46 UTC (permalink / raw) To: Nicolin Chen; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, digetx [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev 2020-10-01 10:46 ` Thierry Reding @ 2020-10-02 1:29 ` Nicolin Chen 0 siblings, 0 replies; 66+ messages in thread From: Nicolin Chen @ 2020-10-02 1:29 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-kernel, iommu, krzk, jonathanh, linux-tegra, digetx [-- 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 [-- Attachment #3: Type: text/plain, Size: 156 bytes --] _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 66+ messages in thread
* [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support 2020-09-30 8:42 [PATCH v3 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2020-09-30 8:42 ` [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen 2020-09-30 8:42 ` [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev Nicolin Chen @ 2020-09-30 8:42 ` Nicolin Chen 2020-09-30 14:53 ` Dmitry Osipenko 2 siblings, 1 reply; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 8:42 UTC (permalink / raw) To: thierry.reding, joro, krzk, digetx Cc: linux-tegra, linux-kernel, iommu, jonathanh 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support 2020-09-30 8:42 ` [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen @ 2020-09-30 14:53 ` Dmitry Osipenko 2020-09-30 20:03 ` Nicolin Chen 0 siblings, 1 reply; 66+ messages in thread From: Dmitry Osipenko @ 2020-09-30 14:53 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro, krzk Cc: linux-tegra, linux-kernel, iommu, jonathanh ... > +#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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support 2020-09-30 14:53 ` Dmitry Osipenko @ 2020-09-30 20:03 ` Nicolin Chen 0 siblings, 0 replies; 66+ messages in thread From: Nicolin Chen @ 2020-09-30 20:03 UTC (permalink / raw) To: Dmitry Osipenko Cc: linux-kernel, iommu, krzk, jonathanh, thierry.reding, linux-tegra 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. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 66+ messages in thread
end of thread, other threads:[~2020-10-05 10:27 UTC | newest] Thread overview: 66+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-30 8:42 [PATCH v3 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2020-09-30 8:42 ` [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen 2020-09-30 9:07 ` Krzysztof Kozlowski 2020-09-30 9:41 ` Nicolin Chen 2020-09-30 10:27 ` Krzysztof Kozlowski 2020-09-30 14:41 ` Dmitry Osipenko 2020-09-30 14:45 ` Krzysztof Kozlowski 2020-09-30 15:22 ` Dmitry Osipenko 2020-09-30 15:23 ` Thierry Reding 2020-09-30 15:27 ` Dmitry Osipenko 2020-09-30 15:53 ` Dmitry Osipenko 2020-09-30 16:03 ` Thierry Reding 2020-09-30 16:06 ` Dmitry Osipenko 2020-09-30 16:15 ` Thierry Reding 2020-09-30 16:26 ` Dmitry Osipenko 2020-09-30 16:38 ` Thierry Reding 2020-09-30 17:32 ` Dmitry Osipenko 2020-09-30 8:42 ` [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev Nicolin Chen 2020-09-30 9:21 ` Krzysztof Kozlowski 2020-09-30 9:40 ` Nicolin Chen 2020-09-30 10:19 ` Krzysztof Kozlowski 2020-09-30 14:41 ` Dmitry Osipenko 2020-09-30 15:09 ` Dmitry Osipenko 2020-09-30 20:51 ` Nicolin Chen 2020-09-30 15:31 ` Thierry Reding 2020-09-30 15:36 ` Dmitry Osipenko 2020-09-30 16:06 ` Thierry Reding 2020-09-30 16:25 ` Dmitry Osipenko 2020-09-30 16:47 ` Thierry Reding 2020-10-01 2:11 ` Dmitry Osipenko 2020-10-01 7:58 ` Thierry Reding 2020-10-01 19:04 ` Dmitry Osipenko 2020-10-05 9:16 ` Thierry Reding 2020-10-05 9:38 ` Dmitry Osipenko 2020-10-05 10:27 ` Thierry Reding 2020-09-30 16:10 ` Thierry Reding 2020-09-30 16:29 ` Dmitry Osipenko 2020-10-01 7:59 ` Thierry Reding 2020-09-30 20:36 ` Nicolin Chen 2020-09-30 21:24 ` Dmitry Osipenko 2020-09-30 21:32 ` Nicolin Chen 2020-09-30 21:56 ` Dmitry Osipenko 2020-10-01 1:26 ` Nicolin Chen 2020-10-01 2:06 ` Dmitry Osipenko 2020-10-01 2:48 ` Nicolin Chen 2020-10-01 4:04 ` Dmitry Osipenko 2020-10-01 10:23 ` Thierry Reding 2020-10-01 11:04 ` Nicolin Chen 2020-10-01 20:33 ` Dmitry Osipenko 2020-10-02 1:07 ` Nicolin Chen 2020-10-02 1:55 ` Dmitry Osipenko 2020-10-02 2:54 ` Nicolin Chen 2020-10-05 7:24 ` Thierry Reding 2020-10-05 7:13 ` Thierry Reding 2020-10-05 8:14 ` Dmitry Osipenko 2020-10-05 9:31 ` Thierry Reding 2020-10-01 9:54 ` Thierry Reding 2020-10-01 9:51 ` Thierry Reding 2020-10-01 10:33 ` Nicolin Chen 2020-10-01 10:42 ` Thierry Reding 2020-10-01 9:47 ` Thierry Reding 2020-10-01 10:46 ` Thierry Reding 2020-10-02 1:29 ` Nicolin Chen 2020-09-30 8:42 ` [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2020-09-30 14:53 ` Dmitry Osipenko 2020-09-30 20:03 ` Nicolin Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).