On 15/07/16 14:55, Lorenzo Pieralisi wrote:

On Fri, Jul 01, 2016 at 05:50:15PM +0100, Robin Murphy wrote:

[...]

quoted

+static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	int ret;
+
+	/* We only support PCI, for now */
+	if (!dev_is_pci(dev))
+		return -ENODEV;

Given that a) the check above is removed in a later patch and b)
code below does not depend on SMMU v3, I think the aim should
be to make this a core function (ie I am asking this since I will
need it in IORT based translation and I do not want to add yet another
*_xlate hook to iommu_op), iommu_fwspec_xlate() ?

Indeed, this is only tied to OF by the current datatypes, and that's
straightforward to change. Ultimately the purpose is just for
firmware/bus code to pass in some words of configuration data, and the
driver to respond with what corresponding runtime data it wants to
associate with the device. As I suggested over on the fsl-mc discussion,
the caller might not even really be 'firmware' at all.

What I will do with my next RFC is move the iommu_fwspec out of
OF_IOMMU code in a separate compilation unit and we will take the
discussion from there.

Sounds good. If the end result starts looking clear, it might be an idea
to squash some patches and skip this intermediate OF-specific step
entirely (I was just hesitant to do that myself without a clear view of
the IORT side).

quoted

+
+	ret = iommu_fwspec_init(dev, args->np);
+	if (!ret)
+		ret = iommu_fwspec_add_ids(dev, &args->args[0], 1);
+
+	return ret;
+}
+
 static struct iommu_ops arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,

@@ -1947,6 +1894,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.device_group		= pci_device_group,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
+	.of_xlate		= arm_smmu_of_xlate,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 };
 

@@ -2697,6 +2645,22 @@ static void __exit arm_smmu_exit(void)
 subsys_initcall(arm_smmu_init);
 module_exit(arm_smmu_exit);
 
+static int __init arm_smmu_of_init(struct device_node *np)
+{
+	static bool registered;
+
+	if (!registered)
+		registered = !arm_smmu_init();

We also need a static variable in arm_smmu_init() to make sure
we do not try to execute it multiple times :( (here and
subsys_initcall).

Strictly, yes, although since there didn't seem to be any real issue
with just letting the initcall fail when register_driver() detects the
collision, I'd hoped we might be able to keep this bodge together in one
place. I guess it might end up printing some unwanted failure message
though, so I'll take another look.

Thanks,
Robin.

Thanks,
Lorenzo

quoted

+
+	if (!of_platform_device_create(np, NULL, platform_bus_type.dev_root))
+		return -ENODEV;
+
+	of_iommu_set_ops(np, &arm_smmu_ops);
+
+	return 0;
+}
+IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", arm_smmu_of_init);
+
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
 MODULE_AUTHOR("Will Deacon [off-list ref]");
 MODULE_LICENSE("GPL v2");
-- 
2.8.1.dirty