[v16,2/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

Commit Message

Vivek Gautam Aug. 30, 2018, 2:45 p.m. UTC

From: Sricharan R <[e-mail]>

The smmu device probe/remove and add/remove master device callbacks
gets called when the smmu is not linked to its master, that is without
the context of the master device. So calling runtime apis in those places
separately.
Global locks are also initialized before enabling runtime pm as the
runtime_resume() calls device_reset() which does tlb_sync_global()
that ultimately requires locks to be initialized.

Signed-off-by: Sricharan R <[e-mail]>
[vivek: Cleanup pm runtime calls]
Signed-off-by: Vivek Gautam <[e-mail]>
Reviewed-by: Tomasz Figa <[e-mail]>
Tested-by: Srinivas Kandagatla <[e-mail]>
---
 drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 81 insertions(+), 8 deletions(-)

Comments

Tomasz Figa Sept. 7, 2018, 9:16 a.m. UTC | #1

Hi Vivek,

On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam
<[e-mail]> wrote:
>
> From: Sricharan R <[e-mail]>
>
> The smmu device probe/remove and add/remove master device callbacks
> gets called when the smmu is not linked to its master, that is without
> the context of the master device. So calling runtime apis in those places
> separately.
> Global locks are also initialized before enabling runtime pm as the
> runtime_resume() calls device_reset() which does tlb_sync_global()
> that ultimately requires locks to be initialized.
>
> Signed-off-by: Sricharan R <[e-mail]>
> [vivek: Cleanup pm runtime calls]
> Signed-off-by: Vivek Gautam <[e-mail]>
> Reviewed-by: Tomasz Figa <[e-mail]>
> Tested-by: Srinivas Kandagatla <[e-mail]>
> ---
>  drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 81 insertions(+), 8 deletions(-)
[snip]
> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>         if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
>                 dev_err(&pdev->dev, "removing device with active domains!\n");
>
> +       arm_smmu_rpm_get(smmu);
>         /* Turn the thing off */
>         writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> +       arm_smmu_rpm_put(smmu);
> +
> +       if (pm_runtime_enabled(smmu->dev))
> +               pm_runtime_force_suspend(smmu->dev);
> +       else
> +               clk_bulk_disable(smmu->num_clks, smmu->clks);
>
> -       clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> +       clk_bulk_unprepare(smmu->num_clks, smmu->clks);

Aren't we missing pm_runtime_disable() here? We'll have the enable
count unbalanced if the driver is removed and probed again.

Also, if we add pm_runtime_disable(), we can reorder things a bit and
simplify into:

       arm_smmu_rpm_get(smmu);

       /* Turn the thing off */
       writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);

       if (pm_runtime_enabled())
               pm_runtime_disable();
        arm_smmu_rpm_put(smmu);

        clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);

Best regards,
Tomasz

Vivek Gautam Sept. 7, 2018, 9:38 a.m. UTC | #2

Hi Tomasz,


On 9/7/2018 2:46 PM, Tomasz Figa wrote:
> Hi Vivek,
>
> On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam
> <[e-mail]> wrote:
>> From: Sricharan R <[e-mail]>
>>
>> The smmu device probe/remove and add/remove master device callbacks
>> gets called when the smmu is not linked to its master, that is without
>> the context of the master device. So calling runtime apis in those places
>> separately.
>> Global locks are also initialized before enabling runtime pm as the
>> runtime_resume() calls device_reset() which does tlb_sync_global()
>> that ultimately requires locks to be initialized.
>>
>> Signed-off-by: Sricharan R <[e-mail]>
>> [vivek: Cleanup pm runtime calls]
>> Signed-off-by: Vivek Gautam <[e-mail]>
>> Reviewed-by: Tomasz Figa <[e-mail]>
>> Tested-by: Srinivas Kandagatla <[e-mail]>
>> ---
>>   drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 81 insertions(+), 8 deletions(-)
> [snip]
>> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>>          if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
>>                  dev_err(&pdev->dev, "removing device with active domains!\n");
>>
>> +       arm_smmu_rpm_get(smmu);
>>          /* Turn the thing off */
>>          writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>> +       arm_smmu_rpm_put(smmu);
>> +
>> +       if (pm_runtime_enabled(smmu->dev))
>> +               pm_runtime_force_suspend(smmu->dev);
>> +       else
>> +               clk_bulk_disable(smmu->num_clks, smmu->clks);
>>
>> -       clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
>> +       clk_bulk_unprepare(smmu->num_clks, smmu->clks);
> Aren't we missing pm_runtime_disable() here? We'll have the enable
> count unbalanced if the driver is removed and probed again.

pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not 
wrong.
And, as mentioned in a previous thread [1], we were seeing a warning 
which we avoided
by keeping force_suspend().

[1] https://lkml.org/lkml/2018/7/8/124

Thanks
Vivek
>
> Also, if we add pm_runtime_disable(), we can reorder things a bit and
> simplify into:
>
>         arm_smmu_rpm_get(smmu);
>
>         /* Turn the thing off */
>         writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>
>         if (pm_runtime_enabled())
>                 pm_runtime_disable();
>          arm_smmu_rpm_put(smmu);
>
>          clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
>
> Best regards,
> Tomasz

Tomasz Figa Sept. 7, 2018, 9:52 a.m. UTC | #3

On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam <[e-mail]> wrote:
>
> Hi Tomasz,
>
>
> On 9/7/2018 2:46 PM, Tomasz Figa wrote:
> > Hi Vivek,
> >
> > On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam
> > <[e-mail]> wrote:
> >> From: Sricharan R <[e-mail]>
> >>
> >> The smmu device probe/remove and add/remove master device callbacks
> >> gets called when the smmu is not linked to its master, that is without
> >> the context of the master device. So calling runtime apis in those places
> >> separately.
> >> Global locks are also initialized before enabling runtime pm as the
> >> runtime_resume() calls device_reset() which does tlb_sync_global()
> >> that ultimately requires locks to be initialized.
> >>
> >> Signed-off-by: Sricharan R <[e-mail]>
> >> [vivek: Cleanup pm runtime calls]
> >> Signed-off-by: Vivek Gautam <[e-mail]>
> >> Reviewed-by: Tomasz Figa <[e-mail]>
> >> Tested-by: Srinivas Kandagatla <[e-mail]>
> >> ---
> >>   drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
> >>   1 file changed, 81 insertions(+), 8 deletions(-)
> > [snip]
> >> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> >>          if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
> >>                  dev_err(&pdev->dev, "removing device with active domains!\n");
> >>
> >> +       arm_smmu_rpm_get(smmu);
> >>          /* Turn the thing off */
> >>          writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> >> +       arm_smmu_rpm_put(smmu);
> >> +
> >> +       if (pm_runtime_enabled(smmu->dev))
> >> +               pm_runtime_force_suspend(smmu->dev);
> >> +       else
> >> +               clk_bulk_disable(smmu->num_clks, smmu->clks);
> >>
> >> -       clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> >> +       clk_bulk_unprepare(smmu->num_clks, smmu->clks);
> > Aren't we missing pm_runtime_disable() here? We'll have the enable
> > count unbalanced if the driver is removed and probed again.
>
> pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not
> wrong.
> And, as mentioned in a previous thread [1], we were seeing a warning
> which we avoided
> by keeping force_suspend().
>
> [1] https://lkml.org/lkml/2018/7/8/124

I see, thanks. I didn't realize that pm_runtime_force_suspend()
already disables runtime PM indeed. Sorry for the noise.

Best regards,
Tomasz

Vivek Gautam Sept. 7, 2018, 10:22 a.m. UTC | #4

On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa <[e-mail]> wrote:
>
> On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam <[e-mail]> wrote:
> >
> > Hi Tomasz,
> >
> >
> > On 9/7/2018 2:46 PM, Tomasz Figa wrote:
> > > Hi Vivek,
> > >
> > > On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam
> > > <[e-mail]> wrote:
> > >> From: Sricharan R <[e-mail]>
> > >>
> > >> The smmu device probe/remove and add/remove master device callbacks
> > >> gets called when the smmu is not linked to its master, that is without
> > >> the context of the master device. So calling runtime apis in those places
> > >> separately.
> > >> Global locks are also initialized before enabling runtime pm as the
> > >> runtime_resume() calls device_reset() which does tlb_sync_global()
> > >> that ultimately requires locks to be initialized.
> > >>
> > >> Signed-off-by: Sricharan R <[e-mail]>
> > >> [vivek: Cleanup pm runtime calls]
> > >> Signed-off-by: Vivek Gautam <[e-mail]>
> > >> Reviewed-by: Tomasz Figa <[e-mail]>
> > >> Tested-by: Srinivas Kandagatla <[e-mail]>
> > >> ---
> > >>   drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
> > >>   1 file changed, 81 insertions(+), 8 deletions(-)
> > > [snip]
> > >> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> > >>          if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
> > >>                  dev_err(&pdev->dev, "removing device with active domains!\n");
> > >>
> > >> +       arm_smmu_rpm_get(smmu);
> > >>          /* Turn the thing off */
> > >>          writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> > >> +       arm_smmu_rpm_put(smmu);
> > >> +
> > >> +       if (pm_runtime_enabled(smmu->dev))
> > >> +               pm_runtime_force_suspend(smmu->dev);
> > >> +       else
> > >> +               clk_bulk_disable(smmu->num_clks, smmu->clks);
> > >>
> > >> -       clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> > >> +       clk_bulk_unprepare(smmu->num_clks, smmu->clks);
> > > Aren't we missing pm_runtime_disable() here? We'll have the enable
> > > count unbalanced if the driver is removed and probed again.
> >
> > pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not
> > wrong.
> > And, as mentioned in a previous thread [1], we were seeing a warning
> > which we avoided
> > by keeping force_suspend().
> >
> > [1] https://lkml.org/lkml/2018/7/8/124
>
> I see, thanks. I didn't realize that pm_runtime_force_suspend()
> already disables runtime PM indeed. Sorry for the noise.

Hi Tomasz,
No problem. Thanks for looking back at it.

Hi Robin,
If you are fine with this series, then can you please consider giving
Reviewed-by, so that we are certain that this series will go in the next merge
window.
Thanks

Best regards
Vivek

Vivek Gautam Sept. 18, 2018, 3:11 a.m. UTC | #5

Hi Robin,

On Fri, Sep 7, 2018 at 3:52 PM Vivek Gautam <[e-mail]> wrote:
>
> On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa <[e-mail]> wrote:
> >
> > On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam <[e-mail]> wrote:
> > >
> > > Hi Tomasz,
> > >
> > >
> > > On 9/7/2018 2:46 PM, Tomasz Figa wrote:
> > > > Hi Vivek,
> > > >
> > > > On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam
> > > > <[e-mail]> wrote:
> > > >> From: Sricharan R <[e-mail]>
> > > >>
> > > >> The smmu device probe/remove and add/remove master device callbacks
> > > >> gets called when the smmu is not linked to its master, that is without
> > > >> the context of the master device. So calling runtime apis in those places
> > > >> separately.
> > > >> Global locks are also initialized before enabling runtime pm as the
> > > >> runtime_resume() calls device_reset() which does tlb_sync_global()
> > > >> that ultimately requires locks to be initialized.
> > > >>
> > > >> Signed-off-by: Sricharan R <[e-mail]>
> > > >> [vivek: Cleanup pm runtime calls]
> > > >> Signed-off-by: Vivek Gautam <[e-mail]>
> > > >> Reviewed-by: Tomasz Figa <[e-mail]>
> > > >> Tested-by: Srinivas Kandagatla <[e-mail]>
> > > >> ---
> > > >>   drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
> > > >>   1 file changed, 81 insertions(+), 8 deletions(-)
> > > > [snip]
> > > >> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> > > >>          if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
> > > >>                  dev_err(&pdev->dev, "removing device with active domains!\n");
> > > >>
> > > >> +       arm_smmu_rpm_get(smmu);
> > > >>          /* Turn the thing off */
> > > >>          writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> > > >> +       arm_smmu_rpm_put(smmu);
> > > >> +
> > > >> +       if (pm_runtime_enabled(smmu->dev))
> > > >> +               pm_runtime_force_suspend(smmu->dev);
> > > >> +       else
> > > >> +               clk_bulk_disable(smmu->num_clks, smmu->clks);
> > > >>
> > > >> -       clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> > > >> +       clk_bulk_unprepare(smmu->num_clks, smmu->clks);
> > > > Aren't we missing pm_runtime_disable() here? We'll have the enable
> > > > count unbalanced if the driver is removed and probed again.
> > >
> > > pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not
> > > wrong.
> > > And, as mentioned in a previous thread [1], we were seeing a warning
> > > which we avoided
> > > by keeping force_suspend().
> > >
> > > [1] https://lkml.org/lkml/2018/7/8/124
> >
> > I see, thanks. I didn't realize that pm_runtime_force_suspend()
> > already disables runtime PM indeed. Sorry for the noise.
>
> Hi Tomasz,
> No problem. Thanks for looking back at it.
>
> Hi Robin,
> If you are fine with this series, then can you please consider giving
> Reviewed-by, so that we are certain that this series will go in the next merge
> window.
> Thanks

Gentle ping.
You ack will be very helpful in letting Will pull this series for 4.20.
Thanks.

Best regards
Vivek


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

Vivek Gautam Sept. 25, 2018, 5:56 a.m. UTC | #6

Hi Robin, Will,

On Tue, Sep 18, 2018 at 8:41 AM Vivek Gautam
<[e-mail]> wrote:
>
> Hi Robin,
>
> On Fri, Sep 7, 2018 at 3:52 PM Vivek Gautam <[e-mail]> wrote:
> >
> > On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa <[e-mail]> wrote:
> > >
> > > On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam <[e-mail]> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > >
> > > > On 9/7/2018 2:46 PM, Tomasz Figa wrote:
> > > > > Hi Vivek,
> > > > >
> > > > > On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam
> > > > > <[e-mail]> wrote:
> > > > >> From: Sricharan R <[e-mail]>
> > > > >>
> > > > >> The smmu device probe/remove and add/remove master device callbacks
> > > > >> gets called when the smmu is not linked to its master, that is without
> > > > >> the context of the master device. So calling runtime apis in those places
> > > > >> separately.
> > > > >> Global locks are also initialized before enabling runtime pm as the
> > > > >> runtime_resume() calls device_reset() which does tlb_sync_global()
> > > > >> that ultimately requires locks to be initialized.
> > > > >>
> > > > >> Signed-off-by: Sricharan R <[e-mail]>
> > > > >> [vivek: Cleanup pm runtime calls]
> > > > >> Signed-off-by: Vivek Gautam <[e-mail]>
> > > > >> Reviewed-by: Tomasz Figa <[e-mail]>
> > > > >> Tested-by: Srinivas Kandagatla <[e-mail]>
> > > > >> ---
> > > > >>   drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
> > > > >>   1 file changed, 81 insertions(+), 8 deletions(-)
> > > > > [snip]
> > > > >> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> > > > >>          if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
> > > > >>                  dev_err(&pdev->dev, "removing device with active domains!\n");
> > > > >>
> > > > >> +       arm_smmu_rpm_get(smmu);
> > > > >>          /* Turn the thing off */
> > > > >>          writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> > > > >> +       arm_smmu_rpm_put(smmu);
> > > > >> +
> > > > >> +       if (pm_runtime_enabled(smmu->dev))
> > > > >> +               pm_runtime_force_suspend(smmu->dev);
> > > > >> +       else
> > > > >> +               clk_bulk_disable(smmu->num_clks, smmu->clks);
> > > > >>
> > > > >> -       clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> > > > >> +       clk_bulk_unprepare(smmu->num_clks, smmu->clks);
> > > > > Aren't we missing pm_runtime_disable() here? We'll have the enable
> > > > > count unbalanced if the driver is removed and probed again.
> > > >
> > > > pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not
> > > > wrong.
> > > > And, as mentioned in a previous thread [1], we were seeing a warning
> > > > which we avoided
> > > > by keeping force_suspend().
> > > >
> > > > [1] https://lkml.org/lkml/2018/7/8/124
> > >
> > > I see, thanks. I didn't realize that pm_runtime_force_suspend()
> > > already disables runtime PM indeed. Sorry for the noise.
> >
> > Hi Tomasz,
> > No problem. Thanks for looking back at it.
> >
> > Hi Robin,
> > If you are fine with this series, then can you please consider giving
> > Reviewed-by, so that we are certain that this series will go in the next merge
> > window.
> > Thanks
>
> Gentle ping.
> You ack will be very helpful in letting Will pull this series for 4.20.
> Thanks.

I would really appreciate if you could provide your ack for this series.
Or if there are any concerns, I am willing to address them.
Thanks.

Best regards
Vivek



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



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

Robin Murphy Sept. 25, 2018, 6:55 p.m. UTC | #7

Hi Vivek,

On 2018-09-25 6:56 AM, Vivek Gautam wrote:
> Hi Robin, Will,
> 
> On Tue, Sep 18, 2018 at 8:41 AM Vivek Gautam
> <[e-mail]> wrote:
>>
>> Hi Robin,
>>
>> On Fri, Sep 7, 2018 at 3:52 PM Vivek Gautam <[e-mail]> wrote:
>>>
>>> On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa <[e-mail]> wrote:
>>>>
>>>> On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam <[e-mail]> wrote:
>>>>>
>>>>> Hi Tomasz,
>>>>>
>>>>>
>>>>> On 9/7/2018 2:46 PM, Tomasz Figa wrote:
>>>>>> Hi Vivek,
>>>>>>
>>>>>> On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam
>>>>>> <[e-mail]> wrote:
>>>>>>> From: Sricharan R <[e-mail]>
>>>>>>>
>>>>>>> The smmu device probe/remove and add/remove master device callbacks
>>>>>>> gets called when the smmu is not linked to its master, that is without
>>>>>>> the context of the master device. So calling runtime apis in those places
>>>>>>> separately.
>>>>>>> Global locks are also initialized before enabling runtime pm as the
>>>>>>> runtime_resume() calls device_reset() which does tlb_sync_global()
>>>>>>> that ultimately requires locks to be initialized.
>>>>>>>
>>>>>>> Signed-off-by: Sricharan R <[e-mail]>
>>>>>>> [vivek: Cleanup pm runtime calls]
>>>>>>> Signed-off-by: Vivek Gautam <[e-mail]>
>>>>>>> Reviewed-by: Tomasz Figa <[e-mail]>
>>>>>>> Tested-by: Srinivas Kandagatla <[e-mail]>
>>>>>>> ---
>>>>>>>    drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
>>>>>>>    1 file changed, 81 insertions(+), 8 deletions(-)
>>>>>> [snip]
>>>>>>> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>>>>>>>           if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
>>>>>>>                   dev_err(&pdev->dev, "removing device with active domains!\n");
>>>>>>>
>>>>>>> +       arm_smmu_rpm_get(smmu);
>>>>>>>           /* Turn the thing off */
>>>>>>>           writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>>>>>>> +       arm_smmu_rpm_put(smmu);
>>>>>>> +
>>>>>>> +       if (pm_runtime_enabled(smmu->dev))
>>>>>>> +               pm_runtime_force_suspend(smmu->dev);
>>>>>>> +       else
>>>>>>> +               clk_bulk_disable(smmu->num_clks, smmu->clks);
>>>>>>>
>>>>>>> -       clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
>>>>>>> +       clk_bulk_unprepare(smmu->num_clks, smmu->clks);
>>>>>> Aren't we missing pm_runtime_disable() here? We'll have the enable
>>>>>> count unbalanced if the driver is removed and probed again.
>>>>>
>>>>> pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not
>>>>> wrong.
>>>>> And, as mentioned in a previous thread [1], we were seeing a warning
>>>>> which we avoided
>>>>> by keeping force_suspend().
>>>>>
>>>>> [1] https://lkml.org/lkml/2018/7/8/124
>>>>
>>>> I see, thanks. I didn't realize that pm_runtime_force_suspend()
>>>> already disables runtime PM indeed. Sorry for the noise.
>>>
>>> Hi Tomasz,
>>> No problem. Thanks for looking back at it.
>>>
>>> Hi Robin,
>>> If you are fine with this series, then can you please consider giving
>>> Reviewed-by, so that we are certain that this series will go in the next merge
>>> window.
>>> Thanks
>>
>> Gentle ping.
>> You ack will be very helpful in letting Will pull this series for 4.20.
>> Thanks.
> 
> I would really appreciate if you could provide your ack for this series.
> Or if there are any concerns, I am willing to address them.

Apologies, I thought I'd replied to say I'd be getting to this shortly, 
but apparently not :(

FWIW, "shortly" is now tomorrow - I don't *think* there's anything 
outstanding, but given the number of subtleties we've turned up so far I 
do just want one last thorough double-check to make sure.

Thanks,
Robin.

Vivek Gautam Sept. 26, 2018, 6:22 a.m. UTC | #8

Hi Robin,

On Wed, Sep 26, 2018 at 12:25 AM Robin Murphy <[e-mail]> wrote:
>
> Hi Vivek,
>
> On 2018-09-25 6:56 AM, Vivek Gautam wrote:
> > Hi Robin, Will,
> >
> > On Tue, Sep 18, 2018 at 8:41 AM Vivek Gautam
> > <[e-mail]> wrote:
> >>
> >> Hi Robin,
> >>
> >> On Fri, Sep 7, 2018 at 3:52 PM Vivek Gautam <[e-mail]> wrote:
> >>>
> >>> On Fri, Sep 7, 2018 at 3:22 PM Tomasz Figa <[e-mail]> wrote:
> >>>>
> >>>> On Fri, Sep 7, 2018 at 6:38 PM Vivek Gautam <[e-mail]> wrote:
> >>>>>
> >>>>> Hi Tomasz,
> >>>>>
> >>>>>
> >>>>> On 9/7/2018 2:46 PM, Tomasz Figa wrote:
> >>>>>> Hi Vivek,
> >>>>>>
> >>>>>> On Thu, Aug 30, 2018 at 11:46 PM Vivek Gautam
> >>>>>> <[e-mail]> wrote:
> >>>>>>> From: Sricharan R <[e-mail]>
> >>>>>>>
> >>>>>>> The smmu device probe/remove and add/remove master device callbacks
> >>>>>>> gets called when the smmu is not linked to its master, that is without
> >>>>>>> the context of the master device. So calling runtime apis in those places
> >>>>>>> separately.
> >>>>>>> Global locks are also initialized before enabling runtime pm as the
> >>>>>>> runtime_resume() calls device_reset() which does tlb_sync_global()
> >>>>>>> that ultimately requires locks to be initialized.
> >>>>>>>
> >>>>>>> Signed-off-by: Sricharan R <[e-mail]>
> >>>>>>> [vivek: Cleanup pm runtime calls]
> >>>>>>> Signed-off-by: Vivek Gautam <[e-mail]>
> >>>>>>> Reviewed-by: Tomasz Figa <[e-mail]>
> >>>>>>> Tested-by: Srinivas Kandagatla <[e-mail]>
> >>>>>>> ---
> >>>>>>>    drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
> >>>>>>>    1 file changed, 81 insertions(+), 8 deletions(-)
> >>>>>> [snip]
> >>>>>>> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> >>>>>>>           if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
> >>>>>>>                   dev_err(&pdev->dev, "removing device with active domains!\n");
> >>>>>>>
> >>>>>>> +       arm_smmu_rpm_get(smmu);
> >>>>>>>           /* Turn the thing off */
> >>>>>>>           writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> >>>>>>> +       arm_smmu_rpm_put(smmu);
> >>>>>>> +
> >>>>>>> +       if (pm_runtime_enabled(smmu->dev))
> >>>>>>> +               pm_runtime_force_suspend(smmu->dev);
> >>>>>>> +       else
> >>>>>>> +               clk_bulk_disable(smmu->num_clks, smmu->clks);
> >>>>>>>
> >>>>>>> -       clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> >>>>>>> +       clk_bulk_unprepare(smmu->num_clks, smmu->clks);
> >>>>>> Aren't we missing pm_runtime_disable() here? We'll have the enable
> >>>>>> count unbalanced if the driver is removed and probed again.
> >>>>>
> >>>>> pm_runtime_force_suspend() does a pm_runtime_disable() also if i am not
> >>>>> wrong.
> >>>>> And, as mentioned in a previous thread [1], we were seeing a warning
> >>>>> which we avoided
> >>>>> by keeping force_suspend().
> >>>>>
> >>>>> [1] https://lkml.org/lkml/2018/7/8/124
> >>>>
> >>>> I see, thanks. I didn't realize that pm_runtime_force_suspend()
> >>>> already disables runtime PM indeed. Sorry for the noise.
> >>>
> >>> Hi Tomasz,
> >>> No problem. Thanks for looking back at it.
> >>>
> >>> Hi Robin,
> >>> If you are fine with this series, then can you please consider giving
> >>> Reviewed-by, so that we are certain that this series will go in the next merge
> >>> window.
> >>> Thanks
> >>
> >> Gentle ping.
> >> You ack will be very helpful in letting Will pull this series for 4.20.
> >> Thanks.
> >
> > I would really appreciate if you could provide your ack for this series.
> > Or if there are any concerns, I am willing to address them.
>
> Apologies, I thought I'd replied to say I'd be getting to this shortly,
> but apparently not :(
>
> FWIW, "shortly" is now tomorrow - I don't *think* there's anything
> outstanding, but given the number of subtleties we've turned up so far I
> do just want one last thorough double-check to make sure.

Cool. TIA for the review. I hope we have something that we can land :),
and then work further to take care of addressing other needs of this driver.
Thanks.

Best regards
Vivek
>
> Thanks,
> Robin.
> _______________________________________________
> iommu mailing list
> [e-mail]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Robin Murphy Sept. 26, 2018, 3:42 p.m. UTC | #9

On 30/08/18 15:45, Vivek Gautam wrote:
> From: Sricharan R <[e-mail]>
> 
> The smmu device probe/remove and add/remove master device callbacks
> gets called when the smmu is not linked to its master, that is without
> the context of the master device. So calling runtime apis in those places
> separately.
> Global locks are also initialized before enabling runtime pm as the
> runtime_resume() calls device_reset() which does tlb_sync_global()
> that ultimately requires locks to be initialized.

To the best of my knowledge in this stuff (which is still not quite 
enough to be *truly* confident...),

Reviewed-by: Robin Murphy <[e-mail]>

> Signed-off-by: Sricharan R <[e-mail]>
> [vivek: Cleanup pm runtime calls]
> Signed-off-by: Vivek Gautam <[e-mail]>
> Reviewed-by: Tomasz Figa <[e-mail]>
> Tested-by: Srinivas Kandagatla <[e-mail]>
> ---
>   drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 81 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index d900e007c3c9..1bf542010be7 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>   	{ 0, NULL},
>   };
>   
> +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> +{
> +	if (pm_runtime_enabled(smmu->dev))
> +		return pm_runtime_get_sync(smmu->dev);
> +
> +	return 0;
> +}
> +
> +static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> +{
> +	if (pm_runtime_enabled(smmu->dev))
> +		pm_runtime_put(smmu->dev);
> +}
> +
>   static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
>   {
>   	return container_of(dom, struct arm_smmu_domain, domain);
> @@ -913,11 +927,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>   	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> -	int irq;
> +	int ret, irq;
>   
>   	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
>   		return;
>   
> +	ret = arm_smmu_rpm_get(smmu);
> +	if (ret < 0)
> +		return;
> +
>   	/*
>   	 * Disable the context bank and free the page tables before freeing
>   	 * it.
> @@ -932,6 +950,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>   
>   	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>   	__arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
> +
> +	arm_smmu_rpm_put(smmu);
>   }
>   
>   static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> @@ -1213,10 +1233,15 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   		return -ENODEV;
>   
>   	smmu = fwspec_smmu(fwspec);
> +
> +	ret = arm_smmu_rpm_get(smmu);
> +	if (ret < 0)
> +		return ret;
> +
>   	/* Ensure that the domain is finalised */
>   	ret = arm_smmu_init_domain_context(domain, smmu);
>   	if (ret < 0)
> -		return ret;
> +		goto rpm_put;
>   
>   	/*
>   	 * Sanity check the domain. We don't support domains across
> @@ -1226,33 +1251,50 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   		dev_err(dev,
>   			"cannot attach to SMMU %s whilst already attached to domain on SMMU %s\n",
>   			dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev));
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto rpm_put;
>   	}
>   
>   	/* Looks ok, so add the device to the domain */
> -	return arm_smmu_domain_add_master(smmu_domain, fwspec);
> +	ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
> +
> +rpm_put:
> +	arm_smmu_rpm_put(smmu);
> +	return ret;
>   }
>   
>   static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>   			phys_addr_t paddr, size_t size, int prot)
>   {
>   	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> +	struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
> +	int ret;
>   
>   	if (!ops)
>   		return -ENODEV;
>   
> -	return ops->map(ops, iova, paddr, size, prot);
> +	arm_smmu_rpm_get(smmu);
> +	ret = ops->map(ops, iova, paddr, size, prot);
> +	arm_smmu_rpm_put(smmu);
> +
> +	return ret;
>   }
>   
>   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_device *smmu = to_smmu_domain(domain)->smmu;
> +	size_t ret;
>   
>   	if (!ops)
>   		return 0;
>   
> -	return ops->unmap(ops, iova, size);
> +	arm_smmu_rpm_get(smmu);
> +	ret = ops->unmap(ops, iova, size);
> +	arm_smmu_rpm_put(smmu);
> +
> +	return ret;
>   }
>   
>   static void arm_smmu_iotlb_sync(struct iommu_domain *domain)
> @@ -1407,7 +1449,13 @@ static int arm_smmu_add_device(struct device *dev)
>   	while (i--)
>   		cfg->smendx[i] = INVALID_SMENDX;
>   
> +	ret = arm_smmu_rpm_get(smmu);
> +	if (ret < 0)
> +		goto out_cfg_free;
> +
>   	ret = arm_smmu_master_alloc_smes(dev);
> +	arm_smmu_rpm_put(smmu);
> +
>   	if (ret)
>   		goto out_cfg_free;
>   
> @@ -1427,7 +1475,7 @@ static void arm_smmu_remove_device(struct device *dev)
>   	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>   	struct arm_smmu_master_cfg *cfg;
>   	struct arm_smmu_device *smmu;
> -
> +	int ret;
>   
>   	if (!fwspec || fwspec->ops != &arm_smmu_ops)
>   		return;
> @@ -1435,8 +1483,15 @@ static void arm_smmu_remove_device(struct device *dev)
>   	cfg  = fwspec->iommu_priv;
>   	smmu = cfg->smmu;
>   
> +	ret = arm_smmu_rpm_get(smmu);
> +	if (ret < 0)
> +		return;
> +
>   	iommu_device_unlink(&smmu->iommu, dev);
>   	arm_smmu_master_free_smes(fwspec);
> +
> +	arm_smmu_rpm_put(smmu);
> +
>   	iommu_group_remove_device(dev);
>   	kfree(fwspec->iommu_priv);
>   	iommu_fwspec_free(dev);
> @@ -2180,6 +2235,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   	arm_smmu_device_reset(smmu);
>   	arm_smmu_test_smr_masks(smmu);
>   
> +	/*
> +	 * We want to avoid touching dev->power.lock in fastpaths unless
> +	 * it's really going to do something useful - pm_runtime_enabled()
> +	 * can serve as an ideal proxy for that decision. So, conditionally
> +	 * enable pm_runtime.
> +	 */
> +	if (dev->pm_domain) {
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);
> +	}
> +
>   	/*
>   	 * For ACPI and generic DT bindings, an SMMU will be probed before
>   	 * any device which might need it, so we want the bus ops in place
> @@ -2215,10 +2281,17 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>   	if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
>   		dev_err(&pdev->dev, "removing device with active domains!\n");
>   
> +	arm_smmu_rpm_get(smmu);
>   	/* Turn the thing off */
>   	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> +	arm_smmu_rpm_put(smmu);
> +
> +	if (pm_runtime_enabled(smmu->dev))
> +		pm_runtime_force_suspend(smmu->dev);
> +	else
> +		clk_bulk_disable(smmu->num_clks, smmu->clks);
>   
> -	clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> +	clk_bulk_unprepare(smmu->num_clks, smmu->clks);
>   
>   	return 0;
>   }
>

Will Deacon Oct. 1, 2018, 12:58 p.m. UTC | #10

Hi Vivek,

On Thu, Aug 30, 2018 at 08:15:38PM +0530, Vivek Gautam wrote:
> From: Sricharan R <[e-mail]>
> 
> The smmu device probe/remove and add/remove master device callbacks
> gets called when the smmu is not linked to its master, that is without
> the context of the master device. So calling runtime apis in those places
> separately.
> Global locks are also initialized before enabling runtime pm as the
> runtime_resume() calls device_reset() which does tlb_sync_global()
> that ultimately requires locks to be initialized.
> 
> Signed-off-by: Sricharan R <[e-mail]>
> [vivek: Cleanup pm runtime calls]
> Signed-off-by: Vivek Gautam <[e-mail]>
> Reviewed-by: Tomasz Figa <[e-mail]>
> Tested-by: Srinivas Kandagatla <[e-mail]>
> ---
>  drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 81 insertions(+), 8 deletions(-)

This doesn't apply on my tree[1], possibly because I've got Robin's non-strict
invalidation queued there. However, that got me thinking -- how does this
work in conjunction with the timer-based TLB invalidation? Do we need to
rpm_{get,put} around flush_iotlb_all()? If so, do we still need the calls
in map/unmap when non-strict mode is in use?

Will

[1] https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-joerg/arm-smmu/updates

Vivek Gautam Oct. 2, 2018, 4:14 a.m. UTC | #11

Hi Will,

On Mon, Oct 1, 2018 at 6:29 PM Will Deacon <[e-mail]> wrote:
>
> Hi Vivek,
>
> On Thu, Aug 30, 2018 at 08:15:38PM +0530, Vivek Gautam wrote:
> > From: Sricharan R <[e-mail]>
> >
> > The smmu device probe/remove and add/remove master device callbacks
> > gets called when the smmu is not linked to its master, that is without
> > the context of the master device. So calling runtime apis in those places
> > separately.
> > Global locks are also initialized before enabling runtime pm as the
> > runtime_resume() calls device_reset() which does tlb_sync_global()
> > that ultimately requires locks to be initialized.
> >
> > Signed-off-by: Sricharan R <[e-mail]>
> > [vivek: Cleanup pm runtime calls]
> > Signed-off-by: Vivek Gautam <[e-mail]>
> > Reviewed-by: Tomasz Figa <[e-mail]>
> > Tested-by: Srinivas Kandagatla <[e-mail]>
> > ---
> >  drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 81 insertions(+), 8 deletions(-)
>
> This doesn't apply on my tree[1], possibly because I've got Robin's non-strict
> invalidation queued there. However, that got me thinking -- how does this
> work in conjunction with the timer-based TLB invalidation? Do we need to
> rpm_{get,put} around flush_iotlb_all()? If so, do we still need the calls
> in map/unmap when non-strict mode is in use?

I haven't tested things with flush queues, but from what it looks like
both .flush_iotlb_all, and .iotlb_sync callbacks need rpm_get/put().
I will respin the patches.

Thanks
Vivek
>
> Will
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-joerg/arm-smmu/updates

Vivek Gautam Oct. 2, 2018, 5:28 a.m. UTC | #12

On Tue, Oct 2, 2018 at 9:44 AM Vivek Gautam <[e-mail]> wrote:
>
> Hi Will,
>
> On Mon, Oct 1, 2018 at 6:29 PM Will Deacon <[e-mail]> wrote:
> >
> > Hi Vivek,
> >
> > On Thu, Aug 30, 2018 at 08:15:38PM +0530, Vivek Gautam wrote:
> > > From: Sricharan R <[e-mail]>
> > >
> > > The smmu device probe/remove and add/remove master device callbacks
> > > gets called when the smmu is not linked to its master, that is without
> > > the context of the master device. So calling runtime apis in those places
> > > separately.
> > > Global locks are also initialized before enabling runtime pm as the
> > > runtime_resume() calls device_reset() which does tlb_sync_global()
> > > that ultimately requires locks to be initialized.
> > >
> > > Signed-off-by: Sricharan R <[e-mail]>
> > > [vivek: Cleanup pm runtime calls]
> > > Signed-off-by: Vivek Gautam <[e-mail]>
> > > Reviewed-by: Tomasz Figa <[e-mail]>
> > > Tested-by: Srinivas Kandagatla <[e-mail]>
> > > ---
> > >  drivers/iommu/arm-smmu.c | 89 +++++++++++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 81 insertions(+), 8 deletions(-)
> >
> > This doesn't apply on my tree[1], possibly because I've got Robin's non-strict
> > invalidation queued there. However, that got me thinking -- how does this
> > work in conjunction with the timer-based TLB invalidation? Do we need to
> > rpm_{get,put} around flush_iotlb_all()? If so, do we still need the calls
> > in map/unmap when non-strict mode is in use?

For map/unmap(), i think there would be no harm in having additional
power.usage_count even for the non-strict mode.
So, I will just add rpm{get,put} in arm_smmu_flush_iotlb_all(), and
arm_smmu_iotlb_sync().

Regards
Vivek

>
> I haven't tested things with flush queues, but from what it looks like
> both .flush_iotlb_all, and .iotlb_sync callbacks need rpm_get/put().
> I will respin the patches.
>
> Thanks
> Vivek
> >
> > Will
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=for-joerg/arm-smmu/updates
>
>
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

Patch

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d900e007c3c9..1bf542010be7 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -268,6 +268,20 @@  static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ 0, NULL},
 };
 
+static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
+{
+	if (pm_runtime_enabled(smmu->dev))
+		return pm_runtime_get_sync(smmu->dev);
+
+	return 0;
+}
+
+static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
+{
+	if (pm_runtime_enabled(smmu->dev))
+		pm_runtime_put(smmu->dev);
+}
+
 static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 {
 	return container_of(dom, struct arm_smmu_domain, domain);
@@ -913,11 +927,15 @@  static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
-	int irq;
+	int ret, irq;
 
 	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
 		return;
 
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return;
+
 	/*
 	 * Disable the context bank and free the page tables before freeing
 	 * it.
@@ -932,6 +950,8 @@  static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
 	__arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
+
+	arm_smmu_rpm_put(smmu);
 }
 
 static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
@@ -1213,10 +1233,15 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return -ENODEV;
 
 	smmu = fwspec_smmu(fwspec);
+
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return ret;
+
 	/* Ensure that the domain is finalised */
 	ret = arm_smmu_init_domain_context(domain, smmu);
 	if (ret < 0)
-		return ret;
+		goto rpm_put;
 
 	/*
 	 * Sanity check the domain. We don't support domains across
@@ -1226,33 +1251,50 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		dev_err(dev,
 			"cannot attach to SMMU %s whilst already attached to domain on SMMU %s\n",
 			dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev));
-		return -EINVAL;
+		ret = -EINVAL;
+		goto rpm_put;
 	}
 
 	/* Looks ok, so add the device to the domain */
-	return arm_smmu_domain_add_master(smmu_domain, fwspec);
+	ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
+
+rpm_put:
+	arm_smmu_rpm_put(smmu);
+	return ret;
 }
 
 static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
 			phys_addr_t paddr, size_t size, int prot)
 {
 	struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
+	struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
+	int ret;
 
 	if (!ops)
 		return -ENODEV;
 
-	return ops->map(ops, iova, paddr, size, prot);
+	arm_smmu_rpm_get(smmu);
+	ret = ops->map(ops, iova, paddr, size, prot);
+	arm_smmu_rpm_put(smmu);
+
+	return ret;
 }
 
 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_device *smmu = to_smmu_domain(domain)->smmu;
+	size_t ret;
 
 	if (!ops)
 		return 0;
 
-	return ops->unmap(ops, iova, size);
+	arm_smmu_rpm_get(smmu);
+	ret = ops->unmap(ops, iova, size);
+	arm_smmu_rpm_put(smmu);
+
+	return ret;
 }
 
 static void arm_smmu_iotlb_sync(struct iommu_domain *domain)
@@ -1407,7 +1449,13 @@  static int arm_smmu_add_device(struct device *dev)
 	while (i--)
 		cfg->smendx[i] = INVALID_SMENDX;
 
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		goto out_cfg_free;
+
 	ret = arm_smmu_master_alloc_smes(dev);
+	arm_smmu_rpm_put(smmu);
+
 	if (ret)
 		goto out_cfg_free;
 
@@ -1427,7 +1475,7 @@  static void arm_smmu_remove_device(struct device *dev)
 	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 	struct arm_smmu_master_cfg *cfg;
 	struct arm_smmu_device *smmu;
-
+	int ret;
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops)
 		return;
@@ -1435,8 +1483,15 @@  static void arm_smmu_remove_device(struct device *dev)
 	cfg  = fwspec->iommu_priv;
 	smmu = cfg->smmu;
 
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return;
+
 	iommu_device_unlink(&smmu->iommu, dev);
 	arm_smmu_master_free_smes(fwspec);
+
+	arm_smmu_rpm_put(smmu);
+
 	iommu_group_remove_device(dev);
 	kfree(fwspec->iommu_priv);
 	iommu_fwspec_free(dev);
@@ -2180,6 +2235,17 @@  static int arm_smmu_device_probe(struct platform_device *pdev)
 	arm_smmu_device_reset(smmu);
 	arm_smmu_test_smr_masks(smmu);
 
+	/*
+	 * We want to avoid touching dev->power.lock in fastpaths unless
+	 * it's really going to do something useful - pm_runtime_enabled()
+	 * can serve as an ideal proxy for that decision. So, conditionally
+	 * enable pm_runtime.
+	 */
+	if (dev->pm_domain) {
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+
 	/*
 	 * For ACPI and generic DT bindings, an SMMU will be probed before
 	 * any device which might need it, so we want the bus ops in place
@@ -2215,10 +2281,17 @@  static int arm_smmu_device_remove(struct platform_device *pdev)
 	if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
 		dev_err(&pdev->dev, "removing device with active domains!\n");
 
+	arm_smmu_rpm_get(smmu);
 	/* Turn the thing off */
 	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
+	arm_smmu_rpm_put(smmu);
+
+	if (pm_runtime_enabled(smmu->dev))
+		pm_runtime_force_suspend(smmu->dev);
+	else
+		clk_bulk_disable(smmu->num_clks, smmu->clks);
 
-	clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
+	clk_bulk_unprepare(smmu->num_clks, smmu->clks);
 
 	return 0;
 }