From: Thierry Reding <thierry.reding@gmail.com>
To: Nicolin Chen <nicoleotsuka@gmail.com>
Cc: joro@8bytes.org, krzk@kernel.org, vdumpa@nvidia.com,
jonathanh@nvidia.com, linux-tegra@vger.kernel.org,
iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/5] iommu/tegra-smmu: Use iommu_fwspec in .probe_/.attach_device()
Date: Mon, 28 Sep 2020 09:52:12 +0200 [thread overview]
Message-ID: <20200928075212.GF2837573@ulmo> (raw)
In-Reply-To: <20200926080719.6822-4-nicoleotsuka@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6605 bytes --]
On Sat, Sep 26, 2020 at 01:07:17AM -0700, Nicolin Chen wrote:
> The tegra_smmu_probe_device() function searches in DT for the iommu
> phandler to get "smmu" pointer. This works for most of SMMU clients
> that exist in the DTB. But a PCI device will not be added to iommu,
> since it doesn't have a DT node.
>
> Fortunately, for a client with a DT node, tegra_smmu_probe_device()
> calls tegra_smmu_of_xlate() via tegra_smmu_configure(), while for a
> PCI device, of_pci_iommu_init() in the IOMMU core calls .of_xlate()
> as well, even before running tegra_smmu_probe_device(). And in both
> cases, tegra_smmu_of_xlate() prepares a valid iommu_fwspec pointer
> that allows us to get the mc->smmu pointer via dev_get_drvdata() by
> calling driver_find_device_by_fwnode().
>
> So this patch uses iommu_fwspec in .probe_device() and related code
> for a client that does not exist in the DTB, especially a PCI one.
>
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
> drivers/iommu/tegra-smmu.c | 89 +++++++++++++++++++++++---------------
> drivers/memory/tegra/mc.c | 2 +-
> include/soc/tegra/mc.h | 2 +
> 3 files changed, 56 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index b10e02073610..97a7185b4578 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -13,6 +13,7 @@
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> +#include <linux/dma-iommu.h>
Why is this needed? I don't see any of the symbols declared in that file
used here.
> #include <linux/dma-mapping.h>
>
> #include <soc/tegra/ahb.h>
> @@ -61,6 +62,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 +487,49 @@ 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;
> - }
> + int index, err = 0;
>
> - 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 err_disable;
I'd personally drop the err_ prefix here because it's pretty obvious
that we're going to do this as a result of an error happening.
>
> - tegra_smmu_enable(smmu, swgroup, as->id);
> - index++;
> + tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
> }
>
> if (index == 0)
> return -ENODEV;
>
> return 0;
> +
> +err_disable:
> + for (index--; index >= 0; index--) {
> + tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
> + tegra_smmu_as_unprepare(smmu, as);
> + }
I think a more idiomatic version of doing this would be:
while (index--) {
...
}
> +
> + 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++;
> }
> }
>
> @@ -845,10 +837,25 @@ static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
> return 0;
> }
>
> +static struct tegra_smmu *tegra_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct device *dev = driver_find_device_by_fwnode(&tegra_mc_driver.driver, fwnode);
> + struct tegra_mc *mc;
> +
> + if (!dev)
> + return NULL;
> +
> + put_device(dev);
> + mc = dev_get_drvdata(dev);
> +
> + return mc->smmu;
> +}
> +
As I mentioned in another reply, I think tegra_smmu_find() should be all
you need in this case.
> static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> {
> struct device_node *np = dev->of_node;
> struct tegra_smmu *smmu = NULL;
> + struct iommu_fwspec *fwspec;
> struct of_phandle_args args;
> unsigned int index = 0;
> int err;
> @@ -868,8 +875,6 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> * supported by the Linux kernel, so abort after the
> * first match.
> */
> - dev_iommu_priv_set(dev, smmu);
> -
> break;
> }
>
> @@ -877,8 +882,20 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> index++;
> }
>
> - if (!smmu)
> - return ERR_PTR(-ENODEV);
> + /* Any device should be able to get smmu pointer using fwspec */
> + if (!smmu) {
> + fwspec = dev_iommu_fwspec_get(dev);
> + if (!fwspec || fwspec->ops != &tegra_smmu_ops)
> + return ERR_PTR(-ENODEV);
> +
> + smmu = tegra_smmu_get_by_fwnode(fwspec->iommu_fwnode);
> + }
> +
> + /* NULL smmu pointer means that SMMU driver is not probed yet */
> + if (unlikely(!smmu))
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + dev_iommu_priv_set(dev, smmu);
Can this not be unified with the OF code above? Basically in all cases
where we use Tegra SMMU, a fwnode_node should correspond 1:1 to a struct
device_node, which makes the code you added here effectively the same as
what's already there.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-09-28 7:52 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-26 8:07 [PATCH 0/5] iommu/tegra-smmu: Adding PCI support and mappings debugfs node Nicolin Chen
2020-09-26 8:07 ` [PATCH 1/5] iommu/tegra-smmu: Unwrap tegra_smmu_group_get Nicolin Chen
2020-09-28 7:13 ` Thierry Reding
2020-09-28 19:33 ` Nicolin Chen
2020-09-26 8:07 ` [PATCH 2/5] iommu/tegra-smmu: Expend mutex protection range Nicolin Chen
2020-09-26 8:07 ` [PATCH 3/5] iommu/tegra-smmu: Use iommu_fwspec in .probe_/.attach_device() Nicolin Chen
2020-09-26 14:48 ` Dmitry Osipenko
2020-09-26 20:42 ` Nicolin Chen
2020-09-28 7:29 ` Thierry Reding
2020-09-28 7:52 ` Thierry Reding [this message]
2020-09-28 22:18 ` Nicolin Chen
2020-09-29 4:06 ` Dmitry Osipenko
2020-09-29 5:36 ` Nicolin Chen
2020-09-26 8:07 ` [PATCH 4/5] iommu/tegra-smmu: Add PCI support Nicolin Chen
2020-09-26 22:18 ` Dmitry Osipenko
2020-09-28 22:31 ` Nicolin Chen
2020-09-28 7:55 ` Thierry Reding
2020-09-28 22:33 ` Nicolin Chen
2020-09-26 8:07 ` [PATCH 5/5] iommu/tegra-smmu: Add pagetable mappings to debugfs Nicolin Chen
2020-09-26 14:48 ` Dmitry Osipenko
2020-09-26 20:47 ` Nicolin Chen
2020-09-26 21:41 ` Dmitry Osipenko
2020-09-26 21:24 ` Dmitry Osipenko
2020-09-26 21:37 ` Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200928075212.GF2837573@ulmo \
--to=thierry.reding@gmail.com \
--cc=iommu@lists.linux-foundation.org \
--cc=jonathanh@nvidia.com \
--cc=joro@8bytes.org \
--cc=krzk@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=nicoleotsuka@gmail.com \
--cc=vdumpa@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).