diff mbox

[v2,9/9] iommu/arm-smmu: add support for non-pci devices

Message ID 1436239822-14132-10-git-send-email-thunder.leizhen@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zhen Lei July 7, 2015, 3:30 a.m. UTC 
This patch support a master with multiple stream IDs, but doesn't support a
master behinds more than one SMMUs.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

--
1.8.0

Comments

Robin Murphy July 8, 2015, 1:22 p.m. UTC | #1 
On 07/07/15 04:30, Zhen Lei wrote:
> This patch support a master with multiple stream IDs, but doesn't support a
> master behinds more than one SMMUs.

This should probably include a binding documentation update to make it 
clear what values of #iommu-cells the driver supports in what 
circumstances, and that the cells themselves should contain raw stream IDs.

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 274d059..582cd1e 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -30,6 +30,7 @@
>   #include <linux/of_address.h>
>   #include <linux/pci.h>
>   #include <linux/platform_device.h>
> +#include <linux/amba/bus.h>
>   #include <linux/of_iommu.h>
>   #include <linux/of_platform.h>
>
> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>   		return -EINVAL;
>   	}
>
> -	/* We only support PCI, for now */
>   	if (!dev_is_pci(dev)) {
> -		return -ENODEV;
> +		int i;
> +		struct iommu_group *group;
> +
> +		group = iommu_group_get(dev);
> +		if (!group) {
> +			group = iommu_group_alloc();
> +			if (IS_ERR(group)) {
> +				dev_err(dev, "failed to allocate IOMMU group\n");
> +				return PTR_ERR(group);
> +			}
> +
> +			ret = iommu_group_add_device(group, dev);
> +			if (ret) {
> +				dev_err(dev, "failed to add device into IOMMU group\n");
> +				return PTR_ERR(group);

This leaks the group.

> +			}
> +		}
> +		iommu_group_put(group);
> +
> +		for (i = 0; i < args->args_count; i++) {

I'm dubious of the value of looping here - having n>1 #iommu-cells per 
phandle means that every platform device behind one SMMU must have the 
same number of stream IDs, or you still have to have repeated phandles 
for every device with some greater multiple of n stream IDs each, but 
ruling out any device with <n. I'm not sure how realistic that is, and 
whether there's any real benefit beyond saving a handful of bytes in the 
DTB.

> +			ret = arm_smmu_add_device(dev, args->args[i]);
> +			if (ret) {
> +				dev_err(dev,
> +					"failed to add sid=%d into SMMU\n",
> +					args->args[i]);
> +				return ret;
> +			}
> +		}
>   	} else {
>   		u32 sid;
>   		struct device_node *of_root;
> @@ -2725,6 +2752,14 @@ static int __init arm_smmu_init(void)
>   	if (ret)
>   		return ret;
>
> +	if (!iommu_present(&platform_bus_type))
> +		bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
> +
> +#ifdef CONFIG_ARM_AMBA
> +	if (!iommu_present(&amba_bustype))
> +		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
> +#endif
> +
>   	return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>   }
>
> --
> 1.8.0
>
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
Zhen Lei July 9, 2015, 1:56 a.m. UTC | #2 
On 2015/7/8 21:22, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
>> This patch support a master with multiple stream IDs, but doesn't support a
>> master behinds more than one SMMUs.
> 
> This should probably include a binding documentation update to make it clear what values of #iommu-cells the driver supports in what circumstances, and that the cells themselves should contain raw
> stream IDs.

OK.

> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 274d059..582cd1e 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -30,6 +30,7 @@
>>   #include <linux/of_address.h>
>>   #include <linux/pci.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/amba/bus.h>
>>   #include <linux/of_iommu.h>
>>   #include <linux/of_platform.h>
>>
>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>>           return -EINVAL;
>>       }
>>
>> -    /* We only support PCI, for now */
>>       if (!dev_is_pci(dev)) {
>> -        return -ENODEV;
>> +        int i;
>> +        struct iommu_group *group;
>> +
>> +        group = iommu_group_get(dev);
>> +        if (!group) {
>> +            group = iommu_group_alloc();
>> +            if (IS_ERR(group)) {
>> +                dev_err(dev, "failed to allocate IOMMU group\n");
>> +                return PTR_ERR(group);
>> +            }
>> +
>> +            ret = iommu_group_add_device(group, dev);
>> +            if (ret) {
>> +                dev_err(dev, "failed to add device into IOMMU group\n");
>> +                return PTR_ERR(group);
> 
> This leaks the group.

OK, I got it. Thanks.

> 
>> +            }
>> +        }
>> +        iommu_group_put(group);
>> +
>> +        for (i = 0; i < args->args_count; i++) {
> 
> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
> saving a handful of bytes in the DTB.

As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
#iommu-cells = <1>;
iommus = <&{/iommu} 23>, <&{/iommu} 24>;

On my hardware platform, a master only have one streamID. But I tested two cases:
#iommu-cells = <1>;
1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
All of these two cases worked well.

Well, if each master contains two streamIDs(or more but equal), this for loop is needed.

> 
>> +            ret = arm_smmu_add_device(dev, args->args[i]);
>> +            if (ret) {
>> +                dev_err(dev,
>> +                    "failed to add sid=%d into SMMU\n",
>> +                    args->args[i]);
>> +                return ret;
>> +            }
>> +        }
>>       } else {
>>           u32 sid;
>>           struct device_node *of_root;
>> @@ -2725,6 +2752,14 @@ static int __init arm_smmu_init(void)
>>       if (ret)
>>           return ret;
>>
>> +    if (!iommu_present(&platform_bus_type))
>> +        bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
>> +
>> +#ifdef CONFIG_ARM_AMBA
>> +    if (!iommu_present(&amba_bustype))
>> +        bus_set_iommu(&amba_bustype, &arm_smmu_ops);
>> +#endif
>> +
>>       return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>>   }
>>
>> -- 
>> 1.8.0
>>
>>
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 
> 
> .
>
Robin Murphy July 9, 2015, 12:01 p.m. UTC | #3 
On 09/07/15 02:56, leizhen wrote:
[...]
>>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
[...]
>>> +        for (i = 0; i < args->args_count; i++) {
>>
>> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
>> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
>> saving a handful of bytes in the DTB.
>
> As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
> #iommu-cells = <1>;
> iommus = <&{/iommu} 23>, <&{/iommu} 24>;
>
> On my hardware platform, a master only have one streamID. But I tested two cases:
> #iommu-cells = <1>;
> 1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
> 2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
> All of these two cases worked well.
>
> Well, if each master contains two streamIDs(or more but equal), this for loop is needed.

My point is that args_count == #iommu-cells here. If you mandate 
#iommu-cells == 1 as per your example (which in my opinion *is* the 
right thing to do), then looping over something which is guaranteed to 
be a single item is pointless.

You'd only need the loop if args_count > 1, for example:

	#iommu-cells = <2>;
	1: iommus = <&smmu0 sid1 sid2>;
	2: iommus = <&smmu0 sid1 sid2>, <&smmu0 sid3 sid4>;

but then you'd be also stuck describing any device with an odd number of 
stream IDs on that SMMU:

	3: iommus = <&smmu0 sid1 what-do-I-put-here?>;

which is why I don't think it's worth trying to accommodate anything 
other than #iommu-cells == 1, and for that case you know you will only 
ever have to deal with args[0] in each of_xlate() call.

Robin.
Zhen Lei July 10, 2015, 12:34 a.m. UTC | #4 
On 2015/7/9 20:01, Robin Murphy wrote:
> On 09/07/15 02:56, leizhen wrote:
> [...]
>>>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> [...]
>>>> +        for (i = 0; i < args->args_count; i++) {
>>>
>>> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
>>> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
>>> saving a handful of bytes in the DTB.
>>
>> As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
>> #iommu-cells = <1>;
>> iommus = <&{/iommu} 23>, <&{/iommu} 24>;
>>
>> On my hardware platform, a master only have one streamID. But I tested two cases:
>> #iommu-cells = <1>;
>> 1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
>> 2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
>> All of these two cases worked well.
>>
>> Well, if each master contains two streamIDs(or more but equal), this for loop is needed.
> 
> My point is that args_count == #iommu-cells here. If you mandate #iommu-cells == 1 as per your example (which in my opinion *is* the right thing to do), then looping over something which is guaranteed
> to be a single item is pointless.
> 
> You'd only need the loop if args_count > 1, for example:
> 
>     #iommu-cells = <2>;
>     1: iommus = <&smmu0 sid1 sid2>;
>     2: iommus = <&smmu0 sid1 sid2>, <&smmu0 sid3 sid4>;
> 
> but then you'd be also stuck describing any device with an odd number of stream IDs on that SMMU:
> 
>     3: iommus = <&smmu0 sid1 what-do-I-put-here?>;
> 
> which is why I don't think it's worth trying to accommodate anything other than #iommu-cells == 1, and for that case you know you will only ever have to deal with args[0] in each of_xlate() call.

OK, I will consider it in v3.

> 
> Robin.
> 
> 
> .
>
diff mbox

Patch

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 274d059..582cd1e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -30,6 +30,7 @@ 
 #include <linux/of_address.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/amba/bus.h>
 #include <linux/of_iommu.h>
 #include <linux/of_platform.h>

@@ -1928,9 +1929,35 @@  static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 		return -EINVAL;
 	}

-	/* We only support PCI, for now */
 	if (!dev_is_pci(dev)) {
-		return -ENODEV;
+		int i;
+		struct iommu_group *group;
+
+		group = iommu_group_get(dev);
+		if (!group) {
+			group = iommu_group_alloc();
+			if (IS_ERR(group)) {
+				dev_err(dev, "failed to allocate IOMMU group\n");
+				return PTR_ERR(group);
+			}
+
+			ret = iommu_group_add_device(group, dev);
+			if (ret) {
+				dev_err(dev, "failed to add device into IOMMU group\n");
+				return PTR_ERR(group);
+			}
+		}
+		iommu_group_put(group);
+
+		for (i = 0; i < args->args_count; i++) {
+			ret = arm_smmu_add_device(dev, args->args[i]);
+			if (ret) {
+				dev_err(dev,
+					"failed to add sid=%d into SMMU\n",
+					args->args[i]);
+				return ret;
+			}
+		}
 	} else {
 		u32 sid;
 		struct device_node *of_root;
@@ -2725,6 +2752,14 @@  static int __init arm_smmu_init(void)
 	if (ret)
 		return ret;

+	if (!iommu_present(&platform_bus_type))
+		bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
+
+#ifdef CONFIG_ARM_AMBA
+	if (!iommu_present(&amba_bustype))
+		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
+#endif
+
 	return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
 }