[v4,18/18] iommu: exynos: add callback for initializing devices from device tree

Commit Message

Marek Szyprowski Jan. 23, 2015, 12:40 p.m. UTC

Hello,

On 2015-01-19 16:27, Javier Martinez Canillas wrote:
> I wanted to test your IOMMU series on an Exynos5420 Peach Pit but the
> kernel hangs with your series + dependencies on top of 3.19-rc5.
>
> Bisecting I found that $subject is the offending commit. I've pushed
> my test branch [0] in case I missed something.
>
> On Fri, Jan 16, 2015 at 10:13 AM, Marek Szyprowski
> <[e-mail]> wrote:
>> This patch adds implementation of of_xlate callback, which prepares
>> masters device for attaching to IOMMU. This callback is called during
>> creating devices from device tree.
>>
>> Signed-off-by: Marek Szyprowski <[e-mail]>
>> ---
>>   drivers/iommu/exynos-iommu.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index ea2659159e63..5432b443abfc 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -1077,6 +1077,33 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *iommu_domain,
>>          return phys;
>>   }
>>
>> +static int exynos_iommu_of_xlate(struct device *dev,
>> +                                struct of_phandle_args *spec)
>> +{
>> +       struct exynos_iommu_owner *owner = dev->archdata.iommu;
>> +       struct platform_device *sysmmu = of_find_device_by_node(spec->np);
>> +       struct sysmmu_drvdata *data;
>> +
>> +       if (!sysmmu)
>> +               return -ENODEV;
>> +
>> +       data = platform_get_drvdata(sysmmu);
>> +       if (!data)
>> +               return -ENODEV;
>> +
>> +       if (!owner) {
>> +               owner = kzalloc(sizeof(*owner), GFP_KERNEL);
>> +               if (!owner)
>> +                       return -ENOMEM;
>> +
>> +               INIT_LIST_HEAD(&owner->clients);
>> +               dev->archdata.iommu = owner;
>> +       }
>> +
>> +       list_add_tail(&data->owner_node, &owner->clients);
> This is the line that causes the kernel to hang, if I comment the
> list_add_tail() call then the kernel boots.
>
> I checked that neither data nor owner are NULL and that the
> owner->clients list_head is initialized. Do you have any ideas what
> could be happening?

This is really strange. However the hang is definitely not caused by
adding the controller to the list, but rather the fact that it is later
being initialized, probably in exynos_iommu_attach_device().

Just a quick question - does bootloader on Exynos5420 Peach Pit sets
any image on the display?

If so then we will get IOMMU page fault on init (DMA engine of FIMD is
left enabled from bootloader) and such case is not yet handled.
Besides that I have no idea for any other reason for such failure.

To check if this is caused by io page fault, please temporarily add the
following hack:
--->8---
                  dev_name(dev));

--->8---

Best regards

Comments

Javier Martinez Canillas Jan. 23, 2015, 1:48 p.m. UTC | #1

Hello Marek,

On Fri, Jan 23, 2015 at 1:40 PM, Marek Szyprowski
<[e-mail]> wrote:
>>> +
>>> +       list_add_tail(&data->owner_node, &owner->clients);
>>
>> This is the line that causes the kernel to hang, if I comment the
>> list_add_tail() call then the kernel boots.
>>
>> I checked that neither data nor owner are NULL and that the
>> owner->clients list_head is initialized. Do you have any ideas what
>> could be happening?
>
>
> This is really strange. However the hang is definitely not caused by
> adding the controller to the list, but rather the fact that it is later
> being initialized, probably in exynos_iommu_attach_device().
>

Yes, I knew adding to the list was not the issue but a side effect of
being in the list. I'm not familiar with Exynos sysmmu/iommu to figure
out though.

> Just a quick question - does bootloader on Exynos5420 Peach Pit sets
> any image on the display?
>

Yes u-boot does initialize the display, I see the boot messages and
have an u-boot prompt.

> If so then we will get IOMMU page fault on init (DMA engine of FIMD is
> left enabled from bootloader) and such case is not yet handled.
> Besides that I have no idea for any other reason for such failure.
>

I see, that's a reasonable explanation and in fact your patch makes at
least the kernel to start booting.

> To check if this is caused by io page fault, please temporarily add the
> following hack:
> --->8---
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 7864797609b3..5e70cf7eb31b 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2011,6 +2011,9 @@ static bool arm_setup_iommu_dma_ops(struct device
> *dev, u64 dma_base, u64 size,
>          return false;
>      }
>
> +    iommu_map(mapping->domain, 0x40000000, 0x40000000, 0x80000000,
> +          IOMMU_READ | IOMMU_WRITE);
> +
>      if (arm_iommu_attach_device(dev, mapping)) {

The kernel still hangs but the boot does indeed go further. Here is my
boot log [0] although I couldn't find an evident cause.

Best regards,
Javier

[0]: http://paste.debian.net/plain/141968

Marek Szyprowski Jan. 23, 2015, 4:15 p.m. UTC | #2

Hello Javier,

On 2015-01-23 14:48, Javier Martinez Canillas wrote:
> On Fri, Jan 23, 2015 at 1:40 PM, Marek Szyprowski
> <[e-mail]> wrote:
>>>> +
>>>> +       list_add_tail(&data->owner_node, &owner->clients);
>>> This is the line that causes the kernel to hang, if I comment the
>>> list_add_tail() call then the kernel boots.
>>>
>>> I checked that neither data nor owner are NULL and that the
>>> owner->clients list_head is initialized. Do you have any ideas what
>>> could be happening?
>>
>> This is really strange. However the hang is definitely not caused by
>> adding the controller to the list, but rather the fact that it is later
>> being initialized, probably in exynos_iommu_attach_device().
>>
> Yes, I knew adding to the list was not the issue but a side effect of
> being in the list. I'm not familiar with Exynos sysmmu/iommu to figure
> out though.
>
>> Just a quick question - does bootloader on Exynos5420 Peach Pit sets
>> any image on the display?
>>
> Yes u-boot does initialize the display, I see the boot messages and
> have an u-boot prompt.
>
>> If so then we will get IOMMU page fault on init (DMA engine of FIMD is
>> left enabled from bootloader) and such case is not yet handled.
>> Besides that I have no idea for any other reason for such failure.
>>
> I see, that's a reasonable explanation and in fact your patch makes at
> least the kernel to start booting.
>
>> To check if this is caused by io page fault, please temporarily add the
>> following hack:
>> --->8---
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index 7864797609b3..5e70cf7eb31b 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -2011,6 +2011,9 @@ static bool arm_setup_iommu_dma_ops(struct device
>> *dev, u64 dma_base, u64 size,
>>           return false;
>>       }
>>
>> +    iommu_map(mapping->domain, 0x40000000, 0x40000000, 0x80000000,
>> +          IOMMU_READ | IOMMU_WRITE);
>> +
>>       if (arm_iommu_attach_device(dev, mapping)) {
> The kernel still hangs but the boot does indeed go further. Here is my
> boot log [0] although I couldn't find an evident cause.

Frankly, this freeze looks really strange, because there is no error message
or any other failure indicator.

Thanks for testing the above hack. It confirmed that the previous boot 
failure
was caused by display left enabled in bootloader. Such configuration is 
not yet
supported, although I will do my best so find how to add support for it. 
For the
time being - please don't enable IOMMU support on Exynos5420 Peach Pit. It
shouldn't be a big issue, because IOMMU support was already non-functional
all the time on Exynos platform.

Best regards

Javier Martinez Canillas Jan. 23, 2015, 4:44 p.m. UTC | #3

Hello Marek,

On Fri, Jan 23, 2015 at 5:15 PM, Marek Szyprowski
<[e-mail]> wrote:
>>
>>> To check if this is caused by io page fault, please temporarily add the
>>> following hack:
>>> --->8---
>>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>>> index 7864797609b3..5e70cf7eb31b 100644
>>> --- a/arch/arm/mm/dma-mapping.c
>>> +++ b/arch/arm/mm/dma-mapping.c
>>> @@ -2011,6 +2011,9 @@ static bool arm_setup_iommu_dma_ops(struct device
>>> *dev, u64 dma_base, u64 size,
>>>           return false;
>>>       }
>>>
>>> +    iommu_map(mapping->domain, 0x40000000, 0x40000000, 0x80000000,
>>> +          IOMMU_READ | IOMMU_WRITE);
>>> +
>>>       if (arm_iommu_attach_device(dev, mapping)) {
>>
>> The kernel still hangs but the boot does indeed go further. Here is my
>> boot log [0] although I couldn't find an evident cause.
>
>
> Frankly, this freeze looks really strange, because there is no error message
> or any other failure indicator.
>
> Thanks for testing the above hack. It confirmed that the previous boot

You are welcome

> failure
> was caused by display left enabled in bootloader. Such configuration is not
> yet
> supported, although I will do my best so find how to add support for it. For
> the

Great, thanks a lot.

> time being - please don't enable IOMMU support on Exynos5420 Peach Pit. It
> shouldn't be a big issue, because IOMMU support was already non-functional
> all the time on Exynos platform.
>

Agreed but keep in mind that CONFIG_EXYNOS_IOMMU is currently enabled
in arch/arm/configs/exynos_defconfig so you should include in your
series a patch to disable that config option to prevent the boot hang.
Or at least add a of_machine_is_compatible("google,peach") check
although I don't know if there are other Exynos machines that have the
same issue.

Best regards,
Javier

Patch

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7864797609b3..5e70cf7eb31b 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2011,6 +2011,9 @@  static bool arm_setup_iommu_dma_ops(struct device 
*dev, u64 dma_base, u64 size,
          return false;
      }

+    iommu_map(mapping->domain, 0x40000000, 0x40000000, 0x80000000,
+          IOMMU_READ | IOMMU_WRITE);
+
      if (arm_iommu_attach_device(dev, mapping)) {
          pr_warn("Failed to attached device %s to IOMMU_mapping\n",