Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1682250 > unrolled thread

[PATCH V4 0/6] iommu/arm-smmu: Add runtime pm/sleep support

Back to article view | Back to linux.kernel

Contents

  [PATCH V4 0/6] iommu/arm-smmu: Add runtime pm/sleep support Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
    [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:00 +0200
        Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:10 +0200
          Re: [PATCH V4 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 06:00 +0200
    [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:00 +0200
        Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 07:20 +0200
          Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Sricharan R <sricharan@codeaurora.org> - 2017-07-13 07:40 +0200
            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-13 14:00 +0200
              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Marek Szyprowski <m.szyprowski@samsung.com> - 2017-07-13 14:10 +0200
                Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-13 14:20 +0200
                  Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Marek Szyprowski <m.szyprowski@samsung.com> - 2017-07-13 14:30 +0200
              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Sricharan R <sricharan@codeaurora.org> - 2017-07-13 16:00 +0200
                Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-13 17:00 +0200
                  Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Will Deacon <will.deacon@arm.com> - 2017-07-14 19:10 +0200
                    Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-14 19:50 +0200
                      Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Will Deacon <will.deacon@arm.com> - 2017-07-14 20:10 +0200
                        Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-14 20:30 +0200
                          Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Will Deacon <will.deacon@arm.com> - 2017-07-14 21:10 +0200
                            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-14 21:40 +0200
                              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Sricharan R <sricharan@codeaurora.org> - 2017-07-17 13:50 +0200
                                Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Sricharan R <sricharan@codeaurora.org> - 2017-07-17 14:30 +0200
                            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Will Deacon <will.deacon@arm.com> - 2017-07-14 21:40 +0200
                            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-14 21:40 +0200
            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 16:00 +0200
              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 16:10 +0200
          Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 08:50 +0200
            Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Robin Murphy <robin.murphy@arm.com> - 2017-07-13 12:00 +0200
              Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe,  add/remove device Rob Clark <robdclark@gmail.com> - 2017-07-13 14:00 +0200
    [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between masters and smmu Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between  masters and smmu Stephen Boyd <sboyd@codeaurora.org> - 2017-07-13 01:00 +0200
        Re: [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between  masters and smmu Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-13 06:00 +0200
    [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks Rob Herring <robh@kernel.org> - 2017-07-10 05:40 +0200
        Re: [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-11 07:20 +0200
    [PATCH V4 6/6] iommu/arm-smmu: Add support for qcom,msm8996-smmu-v2 clocks Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-06 11:40 +0200
      Re: [PATCH V4 6/6] iommu/arm-smmu: Add support for  qcom,msm8996-smmu-v2 clocks Rob Herring <robh@kernel.org> - 2017-07-10 05:50 +0200
        Re: [PATCH V4 6/6] iommu/arm-smmu: Add support for  qcom,msm8996-smmu-v2 clocks Vivek Gautam <vivek.gautam@codeaurora.org> - 2017-07-10 08:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1687622 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Fri, Jul 14, 2017 at 3:36 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Jul 14, 2017 at 03:34:42PM -0400, Rob Clark wrote:
>> On Fri, Jul 14, 2017 at 3:01 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Fri, Jul 14, 2017 at 02:25:45PM -0400, Rob Clark wrote:
>> >> On Fri, Jul 14, 2017 at 2:06 PM, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Fri, Jul 14, 2017 at 01:42:13PM -0400, Rob Clark wrote:
>> >> >> On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
>> >> >> > On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
>> >> >> >> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> >> >> >> > Hi,
>> >> >> >> >
>> >> >> >> > On 7/13/2017 5:20 PM, Rob Clark wrote:
>> >> >> >> >> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> >> >> >> >>> Hi Vivek,
>> >> >> >> >>>
>> >> >> >> >>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>> >> >> >> >>>> Hi Stephen,
>> >> >> >> >>>>
>> >> >> >> >>>>
>> >> >> >> >>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>> >> >> >> >>>>> On 07/06, Vivek Gautam wrote:
>> >> >> >> >>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>> >> >> >> >>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>> >> >> >> >>>>>>                    size_t size)
>> >> >> >> >>>>>>   {
>> >> >> >> >>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>> >> >> >> >>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> >> >> >> >>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>> >> >> >> >>>>>> +    size_t ret;
>> >> >> >> >>>>>>         if (!ops)
>> >> >> >> >>>>>>           return 0;
>> >> >> >> >>>>>>   -    return ops->unmap(ops, iova, size);
>> >> >> >> >>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>> >> >> >> >>>>> Can these map/unmap ops be called from an atomic context? I seem
>> >> >> >> >>>>> to recall that being a problem before.
>> >> >> >> >>>>
>> >> >> >> >>>> That's something which was dropped in the following patch merged in master:
>> >> >> >> >>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>> >> >> >> >>>>
>> >> >> >> >>>> Looks like we don't  need locks here anymore?
>> >> >> >> >>>
>> >> >> >> >>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>> >> >> >> >>>  from unmap. Somehow looks like some path in the master using that
>> >> >> >> >>>  should have enabled the pm ?
>> >> >> >> >>>
>> >> >> >> >>
>> >> >> >> >> Yes, there are a bunch of scenarios where unmap can happen with
>> >> >> >> >> disabled master (but not in atomic context).  On the gpu side we
>> >> >> >> >> opportunistically keep a buffer mapping until the buffer is freed
>> >> >> >> >> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>> >> >> >> >> an exported dmabuf while some other driver holds a reference to it
>> >> >> >> >> (which can be dropped when the v4l2 device is suspended).
>> >> >> >> >>
>> >> >> >> >> Since unmap triggers tbl flush which touches iommu regs, the iommu
>> >> >> >> >> driver *definitely* needs a pm_runtime_get_sync().
>> >> >> >> >
>> >> >> >> >  Ok, with that being the case, there are two things here,
>> >> >> >> >
>> >> >> >> >  1) If the device links are still intact at these places where unmap is called,
>> >> >> >> >     then pm_runtime from the master would setup the all the clocks. That would
>> >> >> >> >     avoid reintroducing the locking indirectly here.
>> >> >> >> >
>> >> >> >> >  2) If not, then doing it here is the only way. But for both cases, since
>> >> >> >> >     the unmap can be called from atomic context, resume handler here should
>> >> >> >> >     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
>> >> >> >> >
>> >> >> >>
>> >> >> >> I do kinda like the approach Marek suggested.. of deferring the tlb
>> >> >> >> flush until resume.  I'm wondering if we could combine that with
>> >> >> >> putting the mmu in a stalled state when we suspend (and not resume the
>> >> >> >> mmu until after the pending tlb flush)?
>> >> >> >
>> >> >> > I'm not sure that a stalled state is what we're after here, because we need
>> >> >> > to take care to prevent any table walks if we've freed the underlying pages.
>> >> >> > What we could try to do is disable the SMMU (put into global bypass) and
>> >> >> > invalidate the TLB when performing a suspend operation, then we just ignore
>> >> >> > invalidation whilst the clocks are stopped and, on resume, enable the SMMU
>> >> >> > again.
>> >> >>
>> >> >> wouldn't stalled just block any memory transactions by device(s) using
>> >> >> the context bank?  Putting it in bypass isn't really a good thing if
>> >> >> there is any chance the device can sneak in a memory access before
>> >> >> we've taking it back out of bypass (ie. makes gpu a giant userspace
>> >> >> controlled root hole).
>> >> >
>> >> > If it doesn't deadlock, then yes, it will stall transactions. However, that
>> >> > doesn't mean it necessarily prevents page table walks.
>> >>
>> >> btw, I guess the concern about pagetable walk is that the unmap could
>> >> have removed some sub-level of the pt that the tlb walk would hit?
>> >> Would deferring freeing those pages help?
>> >
>> > Could do, but it sounds like a lot of complication that I think we can fix
>> > by making the suspend operation put the SMMU into a "clean" state.
>> >
>> >> > Instead of bypass, we
>> >> > could configure all the streams to terminate, but this race still worries me
>> >> > somewhat. I thought that the SMMU would only be suspended if all of its
>> >> > masters were suspended, so if the GPU wants to come out of suspend then the
>> >> > SMMU should be resumed first.
>> >>
>> >> I believe this should be true.. on the gpu side, I'm mostly trying to
>> >> avoid having to power the gpu back on to free buffers.  (On the v4l2
>> >> side, somewhere in the core videobuf code would also need to be made
>> >> to wrap it's dma_unmap_sg() with pm_runtime_get/put()..)
>> >
>> > Right, and we shouldn't have to resume it if we suspend it in a clean state,
>> > with the TLBs invalidated.
>> >
>>
>> I guess if the device_link() stuff ensured the attached device
>> (gpu/etc) was suspended before suspending the iommu, then I guess I
>> can't see how temporarily putting the iommu in bypass would be a
>> problem.  I haven't looked at the device_link() stuff too closely, but
>> iommu being resumed first and suspended last seems like the only thing
>> that would make sense.  I'm mostly just nervous about iommu in bypass
>> vs gpu since userspace has so much control over what address gpu
>> writes to / reads from, so getting it wrong w/ the iommu would be a
>> rather bad thing ;-)
>
> Right, but we can also configure it to terminate if you don't want bypass.
>

ok, terminate wfm

BR,
-R

[toc] | [prev] | [next] | [standalone]

#1688947 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

Hi,

On 7/15/2017 1:09 AM, Rob Clark wrote:
> On Fri, Jul 14, 2017 at 3:36 PM, Will Deacon <will.deacon@arm.com> wrote:
>> On Fri, Jul 14, 2017 at 03:34:42PM -0400, Rob Clark wrote:
>>> On Fri, Jul 14, 2017 at 3:01 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>> On Fri, Jul 14, 2017 at 02:25:45PM -0400, Rob Clark wrote:
>>>>> On Fri, Jul 14, 2017 at 2:06 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>>>> On Fri, Jul 14, 2017 at 01:42:13PM -0400, Rob Clark wrote:
>>>>>>> On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>>>>>> On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
>>>>>>>>> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 7/13/2017 5:20 PM, Rob Clark wrote:
>>>>>>>>>>> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>>>>>>>>>>>> Hi Vivek,
>>>>>>>>>>>>
>>>>>>>>>>>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>>>>>>>>>>>> Hi Stephen,
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>>>>>>>>>>>> On 07/06, Vivek Gautam wrote:
>>>>>>>>>>>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>>>>>>>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>>>>>>>>>>>>                    size_t size)
>>>>>>>>>>>>>>>   {
>>>>>>>>>>>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>>>>>>>>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>>>>>>>>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>>>>>>>>>>>> +    size_t ret;
>>>>>>>>>>>>>>>         if (!ops)
>>>>>>>>>>>>>>>           return 0;
>>>>>>>>>>>>>>>   -    return ops->unmap(ops, iova, size);
>>>>>>>>>>>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>>>>>>>>>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>>>>>>>>>>>> to recall that being a problem before.
>>>>>>>>>>>>>
>>>>>>>>>>>>> That's something which was dropped in the following patch merged in master:
>>>>>>>>>>>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>>>>>>>>>>>
>>>>>>>>>>>>> Looks like we don't  need locks here anymore?
>>>>>>>>>>>>
>>>>>>>>>>>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>>>>>>>>>>>>  from unmap. Somehow looks like some path in the master using that
>>>>>>>>>>>>  should have enabled the pm ?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yes, there are a bunch of scenarios where unmap can happen with
>>>>>>>>>>> disabled master (but not in atomic context).  On the gpu side we
>>>>>>>>>>> opportunistically keep a buffer mapping until the buffer is freed
>>>>>>>>>>> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>>>>>>>>>>> an exported dmabuf while some other driver holds a reference to it
>>>>>>>>>>> (which can be dropped when the v4l2 device is suspended).
>>>>>>>>>>>
>>>>>>>>>>> Since unmap triggers tbl flush which touches iommu regs, the iommu
>>>>>>>>>>> driver *definitely* needs a pm_runtime_get_sync().
>>>>>>>>>>
>>>>>>>>>>  Ok, with that being the case, there are two things here,
>>>>>>>>>>
>>>>>>>>>>  1) If the device links are still intact at these places where unmap is called,
>>>>>>>>>>     then pm_runtime from the master would setup the all the clocks. That would
>>>>>>>>>>     avoid reintroducing the locking indirectly here.
>>>>>>>>>>
>>>>>>>>>>  2) If not, then doing it here is the only way. But for both cases, since
>>>>>>>>>>     the unmap can be called from atomic context, resume handler here should
>>>>>>>>>>     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I do kinda like the approach Marek suggested.. of deferring the tlb
>>>>>>>>> flush until resume.  I'm wondering if we could combine that with
>>>>>>>>> putting the mmu in a stalled state when we suspend (and not resume the
>>>>>>>>> mmu until after the pending tlb flush)?
>>>>>>>>
>>>>>>>> I'm not sure that a stalled state is what we're after here, because we need
>>>>>>>> to take care to prevent any table walks if we've freed the underlying pages.
>>>>>>>> What we could try to do is disable the SMMU (put into global bypass) and
>>>>>>>> invalidate the TLB when performing a suspend operation, then we just ignore
>>>>>>>> invalidation whilst the clocks are stopped and, on resume, enable the SMMU
>>>>>>>> again.
>>>>>>>
>>>>>>> wouldn't stalled just block any memory transactions by device(s) using
>>>>>>> the context bank?  Putting it in bypass isn't really a good thing if
>>>>>>> there is any chance the device can sneak in a memory access before
>>>>>>> we've taking it back out of bypass (ie. makes gpu a giant userspace
>>>>>>> controlled root hole).
>>>>>>
>>>>>> If it doesn't deadlock, then yes, it will stall transactions. However, that
>>>>>> doesn't mean it necessarily prevents page table walks.
>>>>>
>>>>> btw, I guess the concern about pagetable walk is that the unmap could
>>>>> have removed some sub-level of the pt that the tlb walk would hit?
>>>>> Would deferring freeing those pages help?
>>>>
>>>> Could do, but it sounds like a lot of complication that I think we can fix
>>>> by making the suspend operation put the SMMU into a "clean" state.
>>>>
>>>>>> Instead of bypass, we
>>>>>> could configure all the streams to terminate, but this race still worries me
>>>>>> somewhat. I thought that the SMMU would only be suspended if all of its
>>>>>> masters were suspended, so if the GPU wants to come out of suspend then the
>>>>>> SMMU should be resumed first.
>>>>>
>>>>> I believe this should be true.. on the gpu side, I'm mostly trying to
>>>>> avoid having to power the gpu back on to free buffers.  (On the v4l2
>>>>> side, somewhere in the core videobuf code would also need to be made
>>>>> to wrap it's dma_unmap_sg() with pm_runtime_get/put()..)
>>>>
>>>> Right, and we shouldn't have to resume it if we suspend it in a clean state,
>>>> with the TLBs invalidated.
>>>>
>>>
>>> I guess if the device_link() stuff ensured the attached device
>>> (gpu/etc) was suspended before suspending the iommu, then I guess I
>>> can't see how temporarily putting the iommu in bypass would be a
>>> problem.  I haven't looked at the device_link() stuff too closely, but
>>> iommu being resumed first and suspended last seems like the only thing
>>> that would make sense.  I'm mostly just nervous about iommu in bypass
>>> vs gpu since userspace has so much control over what address gpu
>>> writes to / reads from, so getting it wrong w/ the iommu would be a
>>> rather bad thing ;-)
>>
>> Right, but we can also configure it to terminate if you don't want bypass.
>>
> 

 But one thing here is, with devicelinks in picture, iommu suspend/resume
 is called along with the master. That means, we can end up cleaning even
 active entries on the suspend path ?, if suspend is going to
 put the smmu in to a clean state every time. So if the master's are following
 the pm_runtime sequence before a dma_map/unmap operation, that seems better.

Regards,
 Sricharan


-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[toc] | [prev] | [next] | [standalone]

#1688987 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

Hi,

On 7/17/2017 5:16 PM, Sricharan R wrote:
> Hi,
> 
> On 7/15/2017 1:09 AM, Rob Clark wrote:
>> On Fri, Jul 14, 2017 at 3:36 PM, Will Deacon <will.deacon@arm.com> wrote:
>>> On Fri, Jul 14, 2017 at 03:34:42PM -0400, Rob Clark wrote:
>>>> On Fri, Jul 14, 2017 at 3:01 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>>> On Fri, Jul 14, 2017 at 02:25:45PM -0400, Rob Clark wrote:
>>>>>> On Fri, Jul 14, 2017 at 2:06 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>>>>> On Fri, Jul 14, 2017 at 01:42:13PM -0400, Rob Clark wrote:
>>>>>>>> On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
>>>>>>>>> On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
>>>>>>>>>> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 7/13/2017 5:20 PM, Rob Clark wrote:
>>>>>>>>>>>> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>>>>>>>>>>>>> Hi Vivek,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>>>>>>>>>>>>> Hi Stephen,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>>>>>>>>>>>>> On 07/06, Vivek Gautam wrote:
>>>>>>>>>>>>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>>>>>>>>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>>>>>>>>>>>>>                    size_t size)
>>>>>>>>>>>>>>>>   {
>>>>>>>>>>>>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>>>>>>>>>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>>>>>>>>>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>>>>>>>>>>>>> +    size_t ret;
>>>>>>>>>>>>>>>>         if (!ops)
>>>>>>>>>>>>>>>>           return 0;
>>>>>>>>>>>>>>>>   -    return ops->unmap(ops, iova, size);
>>>>>>>>>>>>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>>>>>>>>>>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>>>>>>>>>>>>> to recall that being a problem before.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> That's something which was dropped in the following patch merged in master:
>>>>>>>>>>>>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Looks like we don't  need locks here anymore?
>>>>>>>>>>>>>
>>>>>>>>>>>>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>>>>>>>>>>>>>  from unmap. Somehow looks like some path in the master using that
>>>>>>>>>>>>>  should have enabled the pm ?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, there are a bunch of scenarios where unmap can happen with
>>>>>>>>>>>> disabled master (but not in atomic context).  On the gpu side we
>>>>>>>>>>>> opportunistically keep a buffer mapping until the buffer is freed
>>>>>>>>>>>> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>>>>>>>>>>>> an exported dmabuf while some other driver holds a reference to it
>>>>>>>>>>>> (which can be dropped when the v4l2 device is suspended).
>>>>>>>>>>>>
>>>>>>>>>>>> Since unmap triggers tbl flush which touches iommu regs, the iommu
>>>>>>>>>>>> driver *definitely* needs a pm_runtime_get_sync().
>>>>>>>>>>>
>>>>>>>>>>>  Ok, with that being the case, there are two things here,
>>>>>>>>>>>
>>>>>>>>>>>  1) If the device links are still intact at these places where unmap is called,
>>>>>>>>>>>     then pm_runtime from the master would setup the all the clocks. That would
>>>>>>>>>>>     avoid reintroducing the locking indirectly here.
>>>>>>>>>>>
>>>>>>>>>>>  2) If not, then doing it here is the only way. But for both cases, since
>>>>>>>>>>>     the unmap can be called from atomic context, resume handler here should
>>>>>>>>>>>     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I do kinda like the approach Marek suggested.. of deferring the tlb
>>>>>>>>>> flush until resume.  I'm wondering if we could combine that with
>>>>>>>>>> putting the mmu in a stalled state when we suspend (and not resume the
>>>>>>>>>> mmu until after the pending tlb flush)?
>>>>>>>>>
>>>>>>>>> I'm not sure that a stalled state is what we're after here, because we need
>>>>>>>>> to take care to prevent any table walks if we've freed the underlying pages.
>>>>>>>>> What we could try to do is disable the SMMU (put into global bypass) and
>>>>>>>>> invalidate the TLB when performing a suspend operation, then we just ignore
>>>>>>>>> invalidation whilst the clocks are stopped and, on resume, enable the SMMU
>>>>>>>>> again.
>>>>>>>>
>>>>>>>> wouldn't stalled just block any memory transactions by device(s) using
>>>>>>>> the context bank?  Putting it in bypass isn't really a good thing if
>>>>>>>> there is any chance the device can sneak in a memory access before
>>>>>>>> we've taking it back out of bypass (ie. makes gpu a giant userspace
>>>>>>>> controlled root hole).
>>>>>>>
>>>>>>> If it doesn't deadlock, then yes, it will stall transactions. However, that
>>>>>>> doesn't mean it necessarily prevents page table walks.
>>>>>>
>>>>>> btw, I guess the concern about pagetable walk is that the unmap could
>>>>>> have removed some sub-level of the pt that the tlb walk would hit?
>>>>>> Would deferring freeing those pages help?
>>>>>
>>>>> Could do, but it sounds like a lot of complication that I think we can fix
>>>>> by making the suspend operation put the SMMU into a "clean" state.
>>>>>
>>>>>>> Instead of bypass, we
>>>>>>> could configure all the streams to terminate, but this race still worries me
>>>>>>> somewhat. I thought that the SMMU would only be suspended if all of its
>>>>>>> masters were suspended, so if the GPU wants to come out of suspend then the
>>>>>>> SMMU should be resumed first.
>>>>>>
>>>>>> I believe this should be true.. on the gpu side, I'm mostly trying to
>>>>>> avoid having to power the gpu back on to free buffers.  (On the v4l2
>>>>>> side, somewhere in the core videobuf code would also need to be made
>>>>>> to wrap it's dma_unmap_sg() with pm_runtime_get/put()..)
>>>>>
>>>>> Right, and we shouldn't have to resume it if we suspend it in a clean state,
>>>>> with the TLBs invalidated.
>>>>>
>>>>
>>>> I guess if the device_link() stuff ensured the attached device
>>>> (gpu/etc) was suspended before suspending the iommu, then I guess I
>>>> can't see how temporarily putting the iommu in bypass would be a
>>>> problem.  I haven't looked at the device_link() stuff too closely, but
>>>> iommu being resumed first and suspended last seems like the only thing
>>>> that would make sense.  I'm mostly just nervous about iommu in bypass
>>>> vs gpu since userspace has so much control over what address gpu
>>>> writes to / reads from, so getting it wrong w/ the iommu would be a
>>>> rather bad thing ;-)
>>>
>>> Right, but we can also configure it to terminate if you don't want bypass.
>>>
>>
> 
>  But one thing here is, with devicelinks in picture, iommu suspend/resume
>  is called along with the master. That means, we can end up cleaning even
>  active entries on the suspend path ?, if suspend is going to
>  put the smmu in to a clean state every time. So if the master's are following
>  the pm_runtime sequence before a dma_map/unmap operation, that seems better.
> 

 Also, for the usecase of unmap being done from master's like GPU while it is already
 suspended, then following the Marek's approach of checking for the smmu state while
 in unmap and defer the TLB flush till resume seems correct way. All of the above
 true if we want to use device_link.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[toc] | [prev] | [next] | [standalone]

#1687623 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Fri, Jul 14, 2017 at 03:34:42PM -0400, Rob Clark wrote:
> On Fri, Jul 14, 2017 at 3:01 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Jul 14, 2017 at 02:25:45PM -0400, Rob Clark wrote:
> >> On Fri, Jul 14, 2017 at 2:06 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Fri, Jul 14, 2017 at 01:42:13PM -0400, Rob Clark wrote:
> >> >> On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> >> > On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
> >> >> >> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> >> >> >> > Hi,
> >> >> >> >
> >> >> >> > On 7/13/2017 5:20 PM, Rob Clark wrote:
> >> >> >> >> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> >> >> >> >>> Hi Vivek,
> >> >> >> >>>
> >> >> >> >>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
> >> >> >> >>>> Hi Stephen,
> >> >> >> >>>>
> >> >> >> >>>>
> >> >> >> >>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
> >> >> >> >>>>> On 07/06, Vivek Gautam wrote:
> >> >> >> >>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
> >> >> >> >>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
> >> >> >> >>>>>>                    size_t size)
> >> >> >> >>>>>>   {
> >> >> >> >>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> >> >> >> >>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> >> >> >> >>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> >> >> >> >>>>>> +    size_t ret;
> >> >> >> >>>>>>         if (!ops)
> >> >> >> >>>>>>           return 0;
> >> >> >> >>>>>>   -    return ops->unmap(ops, iova, size);
> >> >> >> >>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
> >> >> >> >>>>> Can these map/unmap ops be called from an atomic context? I seem
> >> >> >> >>>>> to recall that being a problem before.
> >> >> >> >>>>
> >> >> >> >>>> That's something which was dropped in the following patch merged in master:
> >> >> >> >>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
> >> >> >> >>>>
> >> >> >> >>>> Looks like we don't  need locks here anymore?
> >> >> >> >>>
> >> >> >> >>>  Apart from the locking, wonder why a explicit pm_runtime is needed
> >> >> >> >>>  from unmap. Somehow looks like some path in the master using that
> >> >> >> >>>  should have enabled the pm ?
> >> >> >> >>>
> >> >> >> >>
> >> >> >> >> Yes, there are a bunch of scenarios where unmap can happen with
> >> >> >> >> disabled master (but not in atomic context).  On the gpu side we
> >> >> >> >> opportunistically keep a buffer mapping until the buffer is freed
> >> >> >> >> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
> >> >> >> >> an exported dmabuf while some other driver holds a reference to it
> >> >> >> >> (which can be dropped when the v4l2 device is suspended).
> >> >> >> >>
> >> >> >> >> Since unmap triggers tbl flush which touches iommu regs, the iommu
> >> >> >> >> driver *definitely* needs a pm_runtime_get_sync().
> >> >> >> >
> >> >> >> >  Ok, with that being the case, there are two things here,
> >> >> >> >
> >> >> >> >  1) If the device links are still intact at these places where unmap is called,
> >> >> >> >     then pm_runtime from the master would setup the all the clocks. That would
> >> >> >> >     avoid reintroducing the locking indirectly here.
> >> >> >> >
> >> >> >> >  2) If not, then doing it here is the only way. But for both cases, since
> >> >> >> >     the unmap can be called from atomic context, resume handler here should
> >> >> >> >     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
> >> >> >> >
> >> >> >>
> >> >> >> I do kinda like the approach Marek suggested.. of deferring the tlb
> >> >> >> flush until resume.  I'm wondering if we could combine that with
> >> >> >> putting the mmu in a stalled state when we suspend (and not resume the
> >> >> >> mmu until after the pending tlb flush)?
> >> >> >
> >> >> > I'm not sure that a stalled state is what we're after here, because we need
> >> >> > to take care to prevent any table walks if we've freed the underlying pages.
> >> >> > What we could try to do is disable the SMMU (put into global bypass) and
> >> >> > invalidate the TLB when performing a suspend operation, then we just ignore
> >> >> > invalidation whilst the clocks are stopped and, on resume, enable the SMMU
> >> >> > again.
> >> >>
> >> >> wouldn't stalled just block any memory transactions by device(s) using
> >> >> the context bank?  Putting it in bypass isn't really a good thing if
> >> >> there is any chance the device can sneak in a memory access before
> >> >> we've taking it back out of bypass (ie. makes gpu a giant userspace
> >> >> controlled root hole).
> >> >
> >> > If it doesn't deadlock, then yes, it will stall transactions. However, that
> >> > doesn't mean it necessarily prevents page table walks.
> >>
> >> btw, I guess the concern about pagetable walk is that the unmap could
> >> have removed some sub-level of the pt that the tlb walk would hit?
> >> Would deferring freeing those pages help?
> >
> > Could do, but it sounds like a lot of complication that I think we can fix
> > by making the suspend operation put the SMMU into a "clean" state.
> >
> >> > Instead of bypass, we
> >> > could configure all the streams to terminate, but this race still worries me
> >> > somewhat. I thought that the SMMU would only be suspended if all of its
> >> > masters were suspended, so if the GPU wants to come out of suspend then the
> >> > SMMU should be resumed first.
> >>
> >> I believe this should be true.. on the gpu side, I'm mostly trying to
> >> avoid having to power the gpu back on to free buffers.  (On the v4l2
> >> side, somewhere in the core videobuf code would also need to be made
> >> to wrap it's dma_unmap_sg() with pm_runtime_get/put()..)
> >
> > Right, and we shouldn't have to resume it if we suspend it in a clean state,
> > with the TLBs invalidated.
> >
> 
> I guess if the device_link() stuff ensured the attached device
> (gpu/etc) was suspended before suspending the iommu, then I guess I
> can't see how temporarily putting the iommu in bypass would be a
> problem.  I haven't looked at the device_link() stuff too closely, but
> iommu being resumed first and suspended last seems like the only thing
> that would make sense.  I'm mostly just nervous about iommu in bypass
> vs gpu since userspace has so much control over what address gpu
> writes to / reads from, so getting it wrong w/ the iommu would be a
> rather bad thing ;-)

Right, but we can also configure it to terminate if you don't want bypass.

Will

[toc] | [prev] | [next] | [standalone]

#1687625 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Fri, Jul 14, 2017 at 3:01 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Jul 14, 2017 at 02:25:45PM -0400, Rob Clark wrote:
>> On Fri, Jul 14, 2017 at 2:06 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Fri, Jul 14, 2017 at 01:42:13PM -0400, Rob Clark wrote:
>> >> On Fri, Jul 14, 2017 at 1:07 PM, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Thu, Jul 13, 2017 at 10:55:10AM -0400, Rob Clark wrote:
>> >> >> On Thu, Jul 13, 2017 at 9:53 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> >> >> > Hi,
>> >> >> >
>> >> >> > On 7/13/2017 5:20 PM, Rob Clark wrote:
>> >> >> >> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> >> >> >>> Hi Vivek,
>> >> >> >>>
>> >> >> >>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>> >> >> >>>> Hi Stephen,
>> >> >> >>>>
>> >> >> >>>>
>> >> >> >>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>> >> >> >>>>> On 07/06, Vivek Gautam wrote:
>> >> >> >>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>> >> >> >>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>> >> >> >>>>>>                    size_t size)
>> >> >> >>>>>>   {
>> >> >> >>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>> >> >> >>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> >> >> >>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>> >> >> >>>>>> +    size_t ret;
>> >> >> >>>>>>         if (!ops)
>> >> >> >>>>>>           return 0;
>> >> >> >>>>>>   -    return ops->unmap(ops, iova, size);
>> >> >> >>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>> >> >> >>>>> Can these map/unmap ops be called from an atomic context? I seem
>> >> >> >>>>> to recall that being a problem before.
>> >> >> >>>>
>> >> >> >>>> That's something which was dropped in the following patch merged in master:
>> >> >> >>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>> >> >> >>>>
>> >> >> >>>> Looks like we don't  need locks here anymore?
>> >> >> >>>
>> >> >> >>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>> >> >> >>>  from unmap. Somehow looks like some path in the master using that
>> >> >> >>>  should have enabled the pm ?
>> >> >> >>>
>> >> >> >>
>> >> >> >> Yes, there are a bunch of scenarios where unmap can happen with
>> >> >> >> disabled master (but not in atomic context).  On the gpu side we
>> >> >> >> opportunistically keep a buffer mapping until the buffer is freed
>> >> >> >> (which can happen after gpu is disabled).  Likewise, v4l2 won't unmap
>> >> >> >> an exported dmabuf while some other driver holds a reference to it
>> >> >> >> (which can be dropped when the v4l2 device is suspended).
>> >> >> >>
>> >> >> >> Since unmap triggers tbl flush which touches iommu regs, the iommu
>> >> >> >> driver *definitely* needs a pm_runtime_get_sync().
>> >> >> >
>> >> >> >  Ok, with that being the case, there are two things here,
>> >> >> >
>> >> >> >  1) If the device links are still intact at these places where unmap is called,
>> >> >> >     then pm_runtime from the master would setup the all the clocks. That would
>> >> >> >     avoid reintroducing the locking indirectly here.
>> >> >> >
>> >> >> >  2) If not, then doing it here is the only way. But for both cases, since
>> >> >> >     the unmap can be called from atomic context, resume handler here should
>> >> >> >     avoid doing clk_prepare_enable , instead move the clk_prepare to the init.
>> >> >> >
>> >> >>
>> >> >> I do kinda like the approach Marek suggested.. of deferring the tlb
>> >> >> flush until resume.  I'm wondering if we could combine that with
>> >> >> putting the mmu in a stalled state when we suspend (and not resume the
>> >> >> mmu until after the pending tlb flush)?
>> >> >
>> >> > I'm not sure that a stalled state is what we're after here, because we need
>> >> > to take care to prevent any table walks if we've freed the underlying pages.
>> >> > What we could try to do is disable the SMMU (put into global bypass) and
>> >> > invalidate the TLB when performing a suspend operation, then we just ignore
>> >> > invalidation whilst the clocks are stopped and, on resume, enable the SMMU
>> >> > again.
>> >>
>> >> wouldn't stalled just block any memory transactions by device(s) using
>> >> the context bank?  Putting it in bypass isn't really a good thing if
>> >> there is any chance the device can sneak in a memory access before
>> >> we've taking it back out of bypass (ie. makes gpu a giant userspace
>> >> controlled root hole).
>> >
>> > If it doesn't deadlock, then yes, it will stall transactions. However, that
>> > doesn't mean it necessarily prevents page table walks.
>>
>> btw, I guess the concern about pagetable walk is that the unmap could
>> have removed some sub-level of the pt that the tlb walk would hit?
>> Would deferring freeing those pages help?
>
> Could do, but it sounds like a lot of complication that I think we can fix
> by making the suspend operation put the SMMU into a "clean" state.
>
>> > Instead of bypass, we
>> > could configure all the streams to terminate, but this race still worries me
>> > somewhat. I thought that the SMMU would only be suspended if all of its
>> > masters were suspended, so if the GPU wants to come out of suspend then the
>> > SMMU should be resumed first.
>>
>> I believe this should be true.. on the gpu side, I'm mostly trying to
>> avoid having to power the gpu back on to free buffers.  (On the v4l2
>> side, somewhere in the core videobuf code would also need to be made
>> to wrap it's dma_unmap_sg() with pm_runtime_get/put()..)
>
> Right, and we shouldn't have to resume it if we suspend it in a clean state,
> with the TLBs invalidated.
>

I guess if the device_link() stuff ensured the attached device
(gpu/etc) was suspended before suspending the iommu, then I guess I
can't see how temporarily putting the iommu in bypass would be a
problem.  I haven't looked at the device_link() stuff too closely, but
iommu being resumed first and suspended last seems like the only thing
that would make sense.  I'm mostly just nervous about iommu in bypass
vs gpu since userspace has so much control over what address gpu
writes to / reads from, so getting it wrong w/ the iommu would be a
rather bad thing ;-)

BR,
-R

[toc] | [prev] | [next] | [standalone]

#1686540 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Thu, Jul 13, 2017 at 11:05 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> Hi Vivek,
>
> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>> Hi Stephen,
>>
>>
>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>> On 07/06, Vivek Gautam wrote:
>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>                    size_t size)
>>>>   {
>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>> +    size_t ret;
>>>>         if (!ops)
>>>>           return 0;
>>>>   -    return ops->unmap(ops, iova, size);
>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>> Can these map/unmap ops be called from an atomic context? I seem
>>> to recall that being a problem before.
>>
>> That's something which was dropped in the following patch merged in master:
>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>
>> Looks like we don't  need locks here anymore?
>
>  Apart from the locking, wonder why a explicit pm_runtime is needed
>  from unmap. Somehow looks like some path in the master using that
>  should have enabled the pm ?

Right, the master should have done a runtime_get(), and with
device links the iommu will also resume.

The master will call the unmap when it is attached to the iommu
and therefore the iommu should be in resume state.
We shouldn't have an unmap without the master attached anyways.
Will investigate this further if we need the pm_runtime() calls
around unmap or not.

Best regards
Vivek

>
> Regards,
>  Sricharan
>
> --
> "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
> ---
> This email has been checked for viruses by Avast antivirus software.
> https://www.avast.com/antivirus
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686547 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Thu, Jul 13, 2017 at 7:27 PM, Vivek Gautam
<vivek.gautam@codeaurora.org> wrote:
> On Thu, Jul 13, 2017 at 11:05 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> Hi Vivek,
>>
>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>> Hi Stephen,
>>>
>>>
>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>> On 07/06, Vivek Gautam wrote:
>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>   static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>>                    size_t size)
>>>>>   {
>>>>> -    struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>> +    struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>> +    struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>> +    size_t ret;
>>>>>         if (!ops)
>>>>>           return 0;
>>>>>   -    return ops->unmap(ops, iova, size);
>>>>> +    pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>> to recall that being a problem before.
>>>
>>> That's something which was dropped in the following patch merged in master:
>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>
>>> Looks like we don't  need locks here anymore?
>>
>>  Apart from the locking, wonder why a explicit pm_runtime is needed
>>  from unmap. Somehow looks like some path in the master using that
>>  should have enabled the pm ?
>
> Right, the master should have done a runtime_get(), and with
> device links the iommu will also resume.
>
> The master will call the unmap when it is attached to the iommu
> and therefore the iommu should be in resume state.
> We shouldn't have an unmap without the master attached anyways.
> Will investigate this further if we need the pm_runtime() calls
> around unmap or not.

My apologies. My email client didn't update the thread. So please ignore
this comment.

>
> Best regards
> Vivek
>
>>
>> Regards,
>>  Sricharan
>>
>> --
>> "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>>
>> ---
>> This email has been checked for viruses by Avast antivirus software.
>> https://www.avast.com/antivirus
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project



-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686294 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On 07/13, Vivek Gautam wrote:
> Hi Stephen,
> 
> 
> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
> >On 07/06, Vivek Gautam wrote:
> >>@@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
> >>  static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
> >>  			     size_t size)
> >>  {
> >>-	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> >>+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> >>+	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> >>+	size_t ret;
> >>  	if (!ops)
> >>  		return 0;
> >>-	return ops->unmap(ops, iova, size);
> >>+	pm_runtime_get_sync(smmu_domain->smmu->dev);
> >Can these map/unmap ops be called from an atomic context? I seem
> >to recall that being a problem before.
> 
> That's something which was dropped in the following patch merged in master:
> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
> 
> Looks like we don't  need locks here anymore?
> 

While removing the spinlock around the map/unmap path may be one
thing, I'm not sure that's all of them. Is there a path from an
atomic DMA allocation (GFP_ATOMIC sort of thing) mapped into an
IOMMU for a device that can eventually get down to here and
attempt to turn a clk on?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686419 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On 13/07/17 07:48, Stephen Boyd wrote:
> On 07/13, Vivek Gautam wrote:
>> Hi Stephen,
>>
>>
>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>> On 07/06, Vivek Gautam wrote:
>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>  static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>  			     size_t size)
>>>>  {
>>>> -	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>> +	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>> +	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>> +	size_t ret;
>>>>  	if (!ops)
>>>>  		return 0;
>>>> -	return ops->unmap(ops, iova, size);
>>>> +	pm_runtime_get_sync(smmu_domain->smmu->dev);
>>> Can these map/unmap ops be called from an atomic context? I seem
>>> to recall that being a problem before.
>>
>> That's something which was dropped in the following patch merged in master:
>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>
>> Looks like we don't  need locks here anymore?
>>
> 
> While removing the spinlock around the map/unmap path may be one
> thing, I'm not sure that's all of them. Is there a path from an
> atomic DMA allocation (GFP_ATOMIC sort of thing) mapped into an
> IOMMU for a device that can eventually get down to here and
> attempt to turn a clk on?

Yes, in the DMA path map/unmap will frequently be called from IRQ
handlers (think e.g. network packets). The whole point of removing the
lock was to allow multiple maps/unmaps to execute in parallel (since we
know they will be safely operating on different areas of the pagetable).
AFAICS this change is going to largely reintroduce that bottleneck via
dev->power_lock, which is anything but what we want :(

Robin.

[toc] | [prev] | [next] | [standalone]

#1686474 — Re: [PATCH V4 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

On Thu, Jul 13, 2017 at 5:50 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 13/07/17 07:48, Stephen Boyd wrote:
>> On 07/13, Vivek Gautam wrote:
>>> Hi Stephen,
>>>
>>>
>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>> On 07/06, Vivek Gautam wrote:
>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>  static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>>                         size_t size)
>>>>>  {
>>>>> -  struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>> +  struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>> +  struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>> +  size_t ret;
>>>>>    if (!ops)
>>>>>            return 0;
>>>>> -  return ops->unmap(ops, iova, size);
>>>>> +  pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>> to recall that being a problem before.
>>>
>>> That's something which was dropped in the following patch merged in master:
>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>
>>> Looks like we don't  need locks here anymore?
>>>
>>
>> While removing the spinlock around the map/unmap path may be one
>> thing, I'm not sure that's all of them. Is there a path from an
>> atomic DMA allocation (GFP_ATOMIC sort of thing) mapped into an
>> IOMMU for a device that can eventually get down to here and
>> attempt to turn a clk on?
>
> Yes, in the DMA path map/unmap will frequently be called from IRQ
> handlers (think e.g. network packets). The whole point of removing the
> lock was to allow multiple maps/unmaps to execute in parallel (since we
> know they will be safely operating on different areas of the pagetable).
> AFAICS this change is going to largely reintroduce that bottleneck via
> dev->power_lock, which is anything but what we want :(
>

Maybe __pm_runtime_resume() needs some sort of fast-path if already
enabled?  Or otherwise we need some sort of flag to tell the iommu
that it cannot rely on the unmapping device to be resumed?

BR,
-R

[toc] | [prev] | [next] | [standalone]

#1682256 — [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

From: Sricharan R <sricharan@codeaurora.org>

Finally add the device link between the master device and
smmu, so that the smmu gets runtime enabled/disabled only when the
master needs it. This is done from add_device callback which gets
called once when the master is added to the smmu.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index ddbfa8ab69e6..75567d9698ab 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1348,6 +1348,7 @@ static int arm_smmu_add_device(struct device *dev)
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_master_cfg *cfg;
 	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+	struct device_link *link = NULL;
 	int i, ret;
 
 	if (using_legacy_binding) {
@@ -1403,6 +1404,16 @@ static int arm_smmu_add_device(struct device *dev)
 
 	pm_runtime_put_sync(smmu->dev);
 
+	/*
+	 * Establish the link between smmu and master, so that the
+	 * smmu gets runtime enabled/disabled as per the master's
+	 * needs.
+	 */
+	link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
+	if (!link)
+		dev_warn(smmu->dev, "Unable to create device link between %s and %s\n",
+			 dev_name(smmu->dev), dev_name(dev));
+
 	return 0;
 
 out_cfg_free:
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686099 — Re: [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

On 07/06, Vivek Gautam wrote:
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index ddbfa8ab69e6..75567d9698ab 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1348,6 +1348,7 @@ static int arm_smmu_add_device(struct device *dev)
>  	struct arm_smmu_device *smmu;
>  	struct arm_smmu_master_cfg *cfg;
>  	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +	struct device_link *link = NULL;

Unnecessary initialization?

>  	int i, ret;
>  
>  	if (using_legacy_binding) {
> @@ -1403,6 +1404,16 @@ static int arm_smmu_add_device(struct device *dev)
>  
>  	pm_runtime_put_sync(smmu->dev);
>  
> +	/*
> +	 * Establish the link between smmu and master, so that the
> +	 * smmu gets runtime enabled/disabled as per the master's
> +	 * needs.
> +	 */
> +	link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
> +	if (!link)
> +		dev_warn(smmu->dev, "Unable to create device link between %s and %s\n",
> +			 dev_name(smmu->dev), dev_name(dev));
> +
>  	return 0;
>  

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1686221 — Re: [PATCH V4 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

On 07/13/2017 04:25 AM, Stephen Boyd wrote:
> On 07/06, Vivek Gautam wrote:
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index ddbfa8ab69e6..75567d9698ab 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1348,6 +1348,7 @@ static int arm_smmu_add_device(struct device *dev)
>>   	struct arm_smmu_device *smmu;
>>   	struct arm_smmu_master_cfg *cfg;
>>   	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> +	struct device_link *link = NULL;
> Unnecessary initialization?

Right, will drop this.
Thanks.

>
>>   	int i, ret;
>>   
>>   	if (using_legacy_binding) {
>> @@ -1403,6 +1404,16 @@ static int arm_smmu_add_device(struct device *dev)
>>   
>>   	pm_runtime_put_sync(smmu->dev);
>>   
>> +	/*
>> +	 * Establish the link between smmu and master, so that the
>> +	 * smmu gets runtime enabled/disabled as per the master's
>> +	 * needs.
>> +	 */
>> +	link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>> +	if (!link)
>> +		dev_warn(smmu->dev, "Unable to create device link between %s and %s\n",
>> +			 dev_name(smmu->dev), dev_name(dev));
>> +
>>   	return 0;
>>   

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1682259 — [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks

From: Sricharan R <sricharan@codeaurora.org>

The MMU400x/500 is the implementation of the SMMUv2
arch specification. It is split in to two blocks
TBU, TCU. TBU caches the page table, instantiated
for each master locally, clocked by the TBUn_clk.
TCU manages the address translation with PTW and has
the programming interface as well, clocked using the
TCU_CLK. The TBU can also be sharing the same clock
domain as TCU, in which case both are clocked using
the TCU_CLK.

This defines the clock bindings for the same and adds
the clock names to compatible data.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
[vivek: clock rework and cleanup]
Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         | 24 ++++++++++++++++++++++
 drivers/iommu/arm-smmu.c                           | 12 ++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 8a6ffce12af5..00331752d355 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -71,6 +71,26 @@ conditions.
                   or using stream matching with #iommu-cells = <2>, and
                   may be ignored if present in such cases.
 
+- clock-names:    Should be "tcu" and "iface" for "arm,mmu-400",
+                  "arm,mmu-401" and "arm,mmu-500"
+
+                  "tcu" clock is required for smmu's register access using the
+                  programming interface and ptw for downstream bus access. This
+                  clock is also used for access to the TBU connected to the
+                  master locally. Sometimes however, TBU is clocked along with
+                  the master.
+
+                  "iface" clock is required to access the TCU's programming
+                  interface, apart from the "tcu" clock.
+
+- clocks:         Phandles for respective clocks described by clock-names.
+
+- power-domains:  Phandles to SMMU's power domain specifier. This is
+                  required even if SMMU belongs to the master's power
+                  domain, as the SMMU will have to be enabled and
+                  accessed before master gets enabled and linked to its
+                  SMMU.
+
 ** Deprecated properties:
 
 - mmu-masters (deprecated in favour of the generic "iommus" binding) :
@@ -95,6 +115,10 @@ conditions.
                              <0 36 4>,
                              <0 37 4>;
                 #iommu-cells = <1>;
+                clocks = <&gcc GCC_SMMU_CFG_CLK>,
+                         <&gcc GCC_APSS_TCU_CLK>;
+
+		clock-names = "iface", "tcu";
         };
 
         /* device with two stream IDs, 0 and 7 */
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 75567d9698ab..7bb09280fa11 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1947,9 +1947,19 @@ struct arm_smmu_match_data {
 ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
 ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
 ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
-ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
 ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
 
+static const char * const arm_mmu500_clks[] = {
+	"tcu", "iface",
+};
+
+static const struct arm_smmu_match_data arm_mmu500 = {
+	.version = ARM_SMMU_V2,
+	.model = ARM_MMU500,
+	.clks = arm_mmu500_clks,
+	.num_clks = ARRAY_SIZE(arm_mmu500_clks),
+};
+
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
 	{ .compatible = "arm,smmu-v2", .data = &smmu_generic_v2 },
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1683935 — Re: [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks

On Thu, Jul 06, 2017 at 03:07:04PM +0530, Vivek Gautam wrote:
> From: Sricharan R <sricharan@codeaurora.org>
> 
> The MMU400x/500 is the implementation of the SMMUv2
> arch specification. It is split in to two blocks
> TBU, TCU. TBU caches the page table, instantiated
> for each master locally, clocked by the TBUn_clk.
> TCU manages the address translation with PTW and has
> the programming interface as well, clocked using the
> TCU_CLK. The TBU can also be sharing the same clock
> domain as TCU, in which case both are clocked using
> the TCU_CLK.

No TBU clock below. When is it shared or not? If that's an integration 
option then the binding should always have a TBU clock with the same 
parent as the TCU_CLK.

> This defines the clock bindings for the same and adds
> the clock names to compatible data.
> 
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> [vivek: clock rework and cleanup]
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt         | 24 ++++++++++++++++++++++
>  drivers/iommu/arm-smmu.c                           | 12 ++++++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 8a6ffce12af5..00331752d355 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -71,6 +71,26 @@ conditions.
>                    or using stream matching with #iommu-cells = <2>, and
>                    may be ignored if present in such cases.
>  
> +- clock-names:    Should be "tcu" and "iface" for "arm,mmu-400",
> +                  "arm,mmu-401" and "arm,mmu-500"
> +
> +                  "tcu" clock is required for smmu's register access using the
> +                  programming interface and ptw for downstream bus access. This
> +                  clock is also used for access to the TBU connected to the
> +                  master locally. Sometimes however, TBU is clocked along with
> +                  the master.
> +
> +                  "iface" clock is required to access the TCU's programming
> +                  interface, apart from the "tcu" clock.
> +
> +- clocks:         Phandles for respective clocks described by clock-names.
> +
> +- power-domains:  Phandles to SMMU's power domain specifier. This is
> +                  required even if SMMU belongs to the master's power
> +                  domain, as the SMMU will have to be enabled and
> +                  accessed before master gets enabled and linked to its
> +                  SMMU.
> +
>  ** Deprecated properties:
>  
>  - mmu-masters (deprecated in favour of the generic "iommus" binding) :
> @@ -95,6 +115,10 @@ conditions.
>                               <0 36 4>,
>                               <0 37 4>;
>                  #iommu-cells = <1>;
> +                clocks = <&gcc GCC_SMMU_CFG_CLK>,
> +                         <&gcc GCC_APSS_TCU_CLK>;
> +
> +		clock-names = "iface", "tcu";
>          };
>  
>          /* device with two stream IDs, 0 and 7 */
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 75567d9698ab..7bb09280fa11 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1947,9 +1947,19 @@ struct arm_smmu_match_data {
>  ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>  ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>  ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
>  ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
>  
> +static const char * const arm_mmu500_clks[] = {
> +	"tcu", "iface",
> +};
> +
> +static const struct arm_smmu_match_data arm_mmu500 = {
> +	.version = ARM_SMMU_V2,
> +	.model = ARM_MMU500,
> +	.clks = arm_mmu500_clks,
> +	.num_clks = ARRAY_SIZE(arm_mmu500_clks),
> +};
> +
>  static const struct of_device_id arm_smmu_of_match[] = {
>  	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
>  	{ .compatible = "arm,smmu-v2", .data = &smmu_generic_v2 },
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[toc] | [prev] | [next] | [standalone]

#1684779 — Re: [PATCH V4 5/6] iommu/arm-smmu: Add support for MMU40x/500 clocks

Hi Rob,


On 07/10/2017 09:07 AM, Rob Herring wrote:
> On Thu, Jul 06, 2017 at 03:07:04PM +0530, Vivek Gautam wrote:
>> From: Sricharan R<sricharan@codeaurora.org>
>>
>> The MMU400x/500 is the implementation of the SMMUv2
>> arch specification. It is split in to two blocks
>> TBU, TCU. TBU caches the page table, instantiated
>> for each master locally, clocked by the TBUn_clk.
>> TCU manages the address translation with PTW and has
>> the programming interface as well, clocked using the
>> TCU_CLK. The TBU can also be sharing the same clock
>> domain as TCU, in which case both are clocked using
>> the TCU_CLK.
> No TBU clock below. When is it shared or not? If that's an integration
> option then the binding should always have a TBU clock with the same
> parent as the TCU_CLK.

Right. This is something that the ARM spec also says.
The TBU clock can either be in the same clock and power domain as
the TCU clock, or in a separate.

As you said, we should have the TBU clock as well, and based on the
integration the TBU clock can either have same parent as TCU or
different.

I will change these bindings to include the TBU clock as well.


Best Regards
Vivek

>> This defines the clock bindings for the same and adds
>> the clock names to compatible data.
>>
>> Signed-off-by: Sricharan R<sricharan@codeaurora.org>
>> [vivek: clock rework and cleanup]
>> Signed-off-by: Vivek Gautam<vivek.gautam@codeaurora.org>
>> ---
>>   .../devicetree/bindings/iommu/arm,smmu.txt         | 24 ++++++++++++++++++++++
>>   drivers/iommu/arm-smmu.c                           | 12 ++++++++++-
>>   2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> index 8a6ffce12af5..00331752d355 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> @@ -71,6 +71,26 @@ conditions.
>>                     or using stream matching with #iommu-cells = <2>, and
>>                     may be ignored if present in such cases.
>>   
>> +- clock-names:    Should be "tcu" and "iface" for "arm,mmu-400",
>> +                  "arm,mmu-401" and "arm,mmu-500"
>> +
>> +                  "tcu" clock is required for smmu's register access using the
>> +                  programming interface and ptw for downstream bus access. This
>> +                  clock is also used for access to the TBU connected to the
>> +                  master locally. Sometimes however, TBU is clocked along with
>> +                  the master.
>> +
>> +                  "iface" clock is required to access the TCU's programming
>> +                  interface, apart from the "tcu" clock.
>> +
>> +- clocks:         Phandles for respective clocks described by clock-names.
>> +
>> +- power-domains:  Phandles to SMMU's power domain specifier. This is
>> +                  required even if SMMU belongs to the master's power
>> +                  domain, as the SMMU will have to be enabled and
>> +                  accessed before master gets enabled and linked to its
>> +                  SMMU.
>> +
>>   ** Deprecated properties:
>>   
>>   - mmu-masters (deprecated in favour of the generic "iommus" binding) :
>> @@ -95,6 +115,10 @@ conditions.
>>                                <0 36 4>,
>>                                <0 37 4>;
>>                   #iommu-cells = <1>;
>> +                clocks = <&gcc GCC_SMMU_CFG_CLK>,
>> +                         <&gcc GCC_APSS_TCU_CLK>;
>> +
>> +		clock-names = "iface", "tcu";
>>           };
>>   
>>           /* device with two stream IDs, 0 and 7 */
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 75567d9698ab..7bb09280fa11 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1947,9 +1947,19 @@ struct arm_smmu_match_data {
>>   ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>   ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>   ARM_SMMU_MATCH_DATA(arm_mmu401, ARM_SMMU_V1_64K, GENERIC_SMMU);
>> -ARM_SMMU_MATCH_DATA(arm_mmu500, ARM_SMMU_V2, ARM_MMU500);
>>   ARM_SMMU_MATCH_DATA(cavium_smmuv2, ARM_SMMU_V2, CAVIUM_SMMUV2);
>>   
>> +static const char * const arm_mmu500_clks[] = {
>> +	"tcu", "iface",
>> +};
>> +
>> +static const struct arm_smmu_match_data arm_mmu500 = {
>> +	.version = ARM_SMMU_V2,
>> +	.model = ARM_MMU500,
>> +	.clks = arm_mmu500_clks,
>> +	.num_clks = ARRAY_SIZE(arm_mmu500_clks),
>> +};
>> +
>>   static const struct of_device_id arm_smmu_of_match[] = {
>>   	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
>>   	{ .compatible = "arm,smmu-v2", .data = &smmu_generic_v2 },
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message tomajordomo@vger.kernel.org
> More majordomo info athttp://vger.kernel.org/majordomo-info.html

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1682260 — [PATCH V4 6/6] iommu/arm-smmu: Add support for qcom,msm8996-smmu-v2 clocks

qcom,msm8996-smmu-v2 is an arm,smmu-v2 implementation with
specific clock and power requirements. This smmu core is used
with multiple masters on msm8996, viz. mdss, video, etc.
Add bindings for the same.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 18 ++++++++++++++++++
 drivers/iommu/arm-smmu.c                             | 13 +++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 00331752d355..5d8e79775fae 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -17,6 +17,7 @@ conditions.
                         "arm,mmu-401"
                         "arm,mmu-500"
                         "cavium,smmu-v2"
+                        "qcom,msm8996-smmu-v2"
 
                   depending on the particular implementation and/or the
                   version of the architecture implemented.
@@ -74,11 +75,16 @@ conditions.
 - clock-names:    Should be "tcu" and "iface" for "arm,mmu-400",
                   "arm,mmu-401" and "arm,mmu-500"
 
+                  Should be "bus", and "iface" for "qcom,msm8996-smmu-v2"
+                  implementation.
+
                   "tcu" clock is required for smmu's register access using the
                   programming interface and ptw for downstream bus access. This
                   clock is also used for access to the TBU connected to the
                   master locally. Sometimes however, TBU is clocked along with
                   the master.
+                  "bus" clock for "qcom,msm8996-smmu-v2" is requierd for downstream
+                  bus access and for the smmu ptw.
 
                   "iface" clock is required to access the TCU's programming
                   interface, apart from the "tcu" clock.
@@ -161,3 +167,15 @@ conditions.
                 iommu-map = <0 &smmu3 0 0x400>;
                 ...
         };
+
+	/* Qcom's arm,smmu-v2 implementation for msm8996 */
+	smmu4: iommu {
+		compatible = "qcom,msm8996-smmu-v2";
+		...
+		#iommu-cells = <1>;
+		power-domains = <&mmcc MDSS_GDSC>;
+
+		clocks = <&mmcc SMMU_MDP_AXI_CLK>,
+			 <&mmcc SMMU_MDP_AHB_CLK>;
+		clock-names = "bus", "iface";
+	};
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 7bb09280fa11..fe8e7fd61282 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -110,6 +110,7 @@ enum arm_smmu_implementation {
 	GENERIC_SMMU,
 	ARM_MMU500,
 	CAVIUM_SMMUV2,
+	QCOM_MSM8996_SMMUV2,
 };
 
 /* Until ACPICA headers cover IORT rev. C */
@@ -1960,6 +1961,17 @@ struct arm_smmu_match_data {
 	.num_clks = ARRAY_SIZE(arm_mmu500_clks),
 };
 
+static const char * const qcom_msm8996_smmuv2_clks[] = {
+	"bus", "iface",
+};
+
+static const struct arm_smmu_match_data qcom_msm8996_smmuv2 = {
+	.version = ARM_SMMU_V2,
+	.model = QCOM_MSM8996_SMMUV2,
+	.clks = qcom_msm8996_smmuv2_clks,
+	.num_clks = ARRAY_SIZE(qcom_msm8996_smmuv2_clks),
+};
+
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", .data = &smmu_generic_v1 },
 	{ .compatible = "arm,smmu-v2", .data = &smmu_generic_v2 },
@@ -1967,6 +1979,7 @@ struct arm_smmu_match_data {
 	{ .compatible = "arm,mmu-401", .data = &arm_mmu401 },
 	{ .compatible = "arm,mmu-500", .data = &arm_mmu500 },
 	{ .compatible = "cavium,smmu-v2", .data = &cavium_smmuv2 },
+	{ .compatible = "qcom,msm8996-smmu-v2", .data = &qcom_msm8996_smmuv2 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1683939 — Re: [PATCH V4 6/6] iommu/arm-smmu: Add support for qcom,msm8996-smmu-v2 clocks

On Thu, Jul 06, 2017 at 03:07:05PM +0530, Vivek Gautam wrote:
> qcom,msm8996-smmu-v2 is an arm,smmu-v2 implementation with
> specific clock and power requirements. This smmu core is used
> with multiple masters on msm8996, viz. mdss, video, etc.
> Add bindings for the same.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu.txt | 18 ++++++++++++++++++
>  drivers/iommu/arm-smmu.c                             | 13 +++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 00331752d355..5d8e79775fae 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -17,6 +17,7 @@ conditions.
>                          "arm,mmu-401"
>                          "arm,mmu-500"
>                          "cavium,smmu-v2"
> +                        "qcom,msm8996-smmu-v2"
>  
>                    depending on the particular implementation and/or the
>                    version of the architecture implemented.
> @@ -74,11 +75,16 @@ conditions.
>  - clock-names:    Should be "tcu" and "iface" for "arm,mmu-400",
>                    "arm,mmu-401" and "arm,mmu-500"
>  
> +                  Should be "bus", and "iface" for "qcom,msm8996-smmu-v2"
> +                  implementation.
> +
>                    "tcu" clock is required for smmu's register access using the
>                    programming interface and ptw for downstream bus access. This
>                    clock is also used for access to the TBU connected to the
>                    master locally. Sometimes however, TBU is clocked along with
>                    the master.
> +                  "bus" clock for "qcom,msm8996-smmu-v2" is requierd for downstream

s/requierd/required/

> +                  bus access and for the smmu ptw.
>  
>                    "iface" clock is required to access the TCU's programming
>                    interface, apart from the "tcu" clock.
> @@ -161,3 +167,15 @@ conditions.
>                  iommu-map = <0 &smmu3 0 0x400>;
>                  ...
>          };
> +
> +	/* Qcom's arm,smmu-v2 implementation for msm8996 */
> +	smmu4: iommu {
> +		compatible = "qcom,msm8996-smmu-v2";

No registers?

> +		...
> +		#iommu-cells = <1>;
> +		power-domains = <&mmcc MDSS_GDSC>;
> +
> +		clocks = <&mmcc SMMU_MDP_AXI_CLK>,
> +			 <&mmcc SMMU_MDP_AHB_CLK>;
> +		clock-names = "bus", "iface";
> +	};

[toc] | [prev] | [next] | [standalone]

#1683991 — Re: [PATCH V4 6/6] iommu/arm-smmu: Add support for qcom,msm8996-smmu-v2 clocks

Hi Rob,


On Mon, Jul 10, 2017 at 9:10 AM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Jul 06, 2017 at 03:07:05PM +0530, Vivek Gautam wrote:
>> qcom,msm8996-smmu-v2 is an arm,smmu-v2 implementation with
>> specific clock and power requirements. This smmu core is used
>> with multiple masters on msm8996, viz. mdss, video, etc.
>> Add bindings for the same.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/iommu/arm,smmu.txt | 18 ++++++++++++++++++
>>  drivers/iommu/arm-smmu.c                             | 13 +++++++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> index 00331752d355..5d8e79775fae 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> @@ -17,6 +17,7 @@ conditions.
>>                          "arm,mmu-401"
>>                          "arm,mmu-500"
>>                          "cavium,smmu-v2"
>> +                        "qcom,msm8996-smmu-v2"
>>
>>                    depending on the particular implementation and/or the
>>                    version of the architecture implemented.
>> @@ -74,11 +75,16 @@ conditions.
>>  - clock-names:    Should be "tcu" and "iface" for "arm,mmu-400",
>>                    "arm,mmu-401" and "arm,mmu-500"
>>
>> +                  Should be "bus", and "iface" for "qcom,msm8996-smmu-v2"
>> +                  implementation.
>> +
>>                    "tcu" clock is required for smmu's register access using the
>>                    programming interface and ptw for downstream bus access. This
>>                    clock is also used for access to the TBU connected to the
>>                    master locally. Sometimes however, TBU is clocked along with
>>                    the master.
>> +                  "bus" clock for "qcom,msm8996-smmu-v2" is requierd for downstream
>
> s/requierd/required/

sure, will correct it.

>
>> +                  bus access and for the smmu ptw.
>>
>>                    "iface" clock is required to access the TCU's programming
>>                    interface, apart from the "tcu" clock.
>> @@ -161,3 +167,15 @@ conditions.
>>                  iommu-map = <0 &smmu3 0 0x400>;
>>                  ...
>>          };
>> +
>> +     /* Qcom's arm,smmu-v2 implementation for msm8996 */
>> +     smmu4: iommu {
>> +             compatible = "qcom,msm8996-smmu-v2";
>
> No registers?

It does have registers. Will add the complete binding example.

Thank you for the review.

Best Regards
Vivek

[snip]


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web