Patchwork [v2,14/22] iommu/tegra: smmu: Register platform_device to IOMMU dynamically

login
register
mail settings
Submitter Hiroshi Doyu
Date July 5, 2013, 10:44 a.m.
Message ID <1373021097-32420-15-git-send-email-hdoyu@nvidia.com>
Download mbox | patch
Permalink /patch/257088/
State Superseded, archived
Headers show

Comments

Hiroshi Doyu - July 5, 2013, 10:44 a.m.
Register platform_devices to IOMMU dynamically via
ops->{add,remove}_device().

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 drivers/iommu/tegra-smmu.c |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
Stephen Warren - July 18, 2013, 8:17 p.m.
On 07/05/2013 04:44 AM, Hiroshi Doyu wrote:
> Register platform_devices to IOMMU dynamically via
> ops->{add,remove}_device().

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

> +/*
> + * ASID[0] for the system default
> + * ASID[1] for PPCS, which has SDMMC

I have no idea what PPCS is. The patch description for 21/22 implies
much more than SDMMC is "in PPCS"...

> + * ASID[2][3].. open for drivers, first come, first served.
> + */
> +enum {
> +	SYSTEM_DEFAULT,
> +	SYSTEM_PROTECTED,
> +};

Why hard-code this mapping? Can't devices be assigned to ASIDs based on
a DT property? I assume there's nothing in the SMMU HW that requires
specific ASIDs to be used?

I don't see anything in this series which implements the "open for
drivers" part of the description for ASID 2/3/...

Perhaps not so relevant here since this patch only uses SYSTEM_DEFAULT
as the ASID number, but don't you need to validate that smmu->num_as is
large enough to support all values in the enum definition above?

Since this patch doesn't actually use anything other than
SYSTEM_DEFAULT, I wonder if this patch shouldn't be squashed into patch
21/22, where multiple ASIDs are actually used?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hiroshi Doyu - July 29, 2013, 11:27 a.m.
Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 18 Jul 2013 22:17:21 +0200:

> On 07/05/2013 04:44 AM, Hiroshi Doyu wrote:
> > Register platform_devices to IOMMU dynamically via
> > ops->{add,remove}_device().
> 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> 
> > +/*
> > + * ASID[0] for the system default
> > + * ASID[1] for PPCS, which has SDMMC
> 
> I have no idea what PPCS is. The patch description for 21/22 implies
> much more than SDMMC is "in PPCS"...

"PPCS" is all AHB bus children/(client).

> > + * ASID[2][3].. open for drivers, first come, first served.
> > + */
> > +enum {
> > +	SYSTEM_DEFAULT,
> > +	SYSTEM_PROTECTED,
> > +};
> 
> Why hard-code this mapping? Can't devices be assigned to ASIDs based on
> a DT property? I assume there's nothing in the SMMU HW that requires
> specific ASIDs to be used?

Right. Where should those policy be passed, board DT files?

> I don't see anything in this series which implements the "open for
> drivers" part of the description for ASID 2/3/...

I expect that some smart HW(host1x) will use AS[2/3] for their smarter usage.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - July 29, 2013, 5:54 p.m.
On 07/29/2013 05:27 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 18 Jul 2013 22:17:21 +0200:
> 
>> On 07/05/2013 04:44 AM, Hiroshi Doyu wrote:
>>> Register platform_devices to IOMMU dynamically via
>>> ops->{add,remove}_device().
>>
>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>
>>> +/*
>>> + * ASID[0] for the system default
>>> + * ASID[1] for PPCS, which has SDMMC
>>
>> I have no idea what PPCS is. The patch description for 21/22 implies
>> much more than SDMMC is "in PPCS"...
> 
> "PPCS" is all AHB bus children/(client).

Just to be clear: I was hoping that the comment would be fixed.

>>> + * ASID[2][3].. open for drivers, first come, first served.
>>> + */
>>> +enum {
>>> +	SYSTEM_DEFAULT,
>>> +	SYSTEM_PROTECTED,
>>> +};
>>
>> Why hard-code this mapping? Can't devices be assigned to ASIDs based on
>> a DT property? I assume there's nothing in the SMMU HW that requires
>> specific ASIDs to be used?
> 
> Right. Where should those policy be passed, board DT files?

Is the HW module (== SW group?) to ASID mapping policy or is a
particular mapping required by HW?

Can the ASID mapping just happen dynamically in SW rather than DT
dictating a particular mapping?

I assume the mapping must be set up before any HW is used, and can't be
modified later, so there's no much chance of deferring any policy
decisions to user-space etc.?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hiroshi Doyu - July 30, 2013, 5:18 a.m.
On Mon, 29 Jul 2013 19:54:09 +0200
Stephen Warren <swarren@wwwdotorg.org> wrote:
...
> >> Why hard-code this mapping? Can't devices be assigned to ASIDs based on
> >> a DT property? I assume there's nothing in the SMMU HW that requires
> >> specific ASIDs to be used?
> > 
> > Right. Where should those policy be passed, board DT files?
> 
> Is the HW module (== SW group?) to ASID mapping policy or is a
> particular mapping required by HW?

only policy.
But swgroup can have multiple HWs, they need to be set in the same swgroup.

> Can the ASID mapping just happen dynamically in SW rather than DT
> dictating a particular mapping?

No info about ASID in DT, at least, right now.
If a board DT file is the place to put policy, then it can be an option.
 
> I assume the mapping must be set up before any HW is used, and can't be
> modified later, so there's no much chance of deferring any policy
> decisions to user-space etc.?

I think that both cases are needed.

Some traditional devices(MMC) can be changed later as it may have rootfs on it.
For some smarter devices like host1x, it want to change ASID dynamically per request/context.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 1617c90..35f4a16 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -39,6 +39,7 @@ 
 
 #include <asm/page.h>
 #include <asm/cacheflush.h>
+#include <asm/dma-iommu.h>
 
 enum smmu_hwgrp {
 	HWGRP_AFI,
@@ -949,6 +950,40 @@  static void smmu_iommu_domain_destroy(struct iommu_domain *domain)
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
 }
 
+/*
+ * ASID[0] for the system default
+ * ASID[1] for PPCS, which has SDMMC
+ * ASID[2][3].. open for drivers, first come, first served.
+ */
+enum {
+	SYSTEM_DEFAULT,
+	SYSTEM_PROTECTED,
+};
+
+static int smmu_iommu_add_device(struct device *dev)
+{
+	int err;
+	struct dma_iommu_mapping *map = smmu_handle->map[SYSTEM_DEFAULT];
+
+	if (!map)
+		goto out;
+
+	err = arm_iommu_attach_device(dev, map);
+	if (err) {
+		dev_err(dev, "Failed to attach to map %p\n", map);
+		return err;
+	}
+out:
+	dev_dbg(dev, "Attached to map %p\n", map);
+	return 0;
+}
+
+static void smmu_iommu_remove_device(struct device *dev)
+{
+	dev_dbg(dev, "Detaching from map %p\n", to_dma_iommu_mapping(dev));
+	arm_iommu_detach_device(dev);
+}
+
 static struct iommu_ops smmu_iommu_ops = {
 	.domain_init	= smmu_iommu_domain_init,
 	.domain_destroy	= smmu_iommu_domain_destroy,
@@ -958,6 +993,8 @@  static struct iommu_ops smmu_iommu_ops = {
 	.unmap		= smmu_iommu_unmap,
 	.iova_to_phys	= smmu_iommu_iova_to_phys,
 	.domain_has_cap	= smmu_iommu_domain_has_cap,
+	.add_device	= smmu_iommu_add_device,
+	.remove_device	= smmu_iommu_remove_device,
 	.pgsize_bitmap	= SMMU_IOMMU_PGSIZES,
 };