[v16,1/5] iommu/arm-smmu: Add pm_runtime/sleep ops

Commit Message

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

From: Sricharan R <[e-mail]>

The smmu needs to be functional only when the respective
master's using it are active. The device_link feature
helps to track such functional dependencies, so that the
iommu gets powered when the master device enables itself
using pm_runtime. So by adapting the smmu driver for
runtime pm, above said dependency can be addressed.

This patch adds the pm runtime/sleep callbacks to the
driver and also the functions to parse the smmu clocks
from DT and enable them in resume/suspend.

Also, while we enable the runtime pm add a pm sleep suspend
callback that pushes devices to low power state by turning
the clocks off in a system sleep.
Also add corresponding clock enable path in resume callback.

Signed-off-by: Sricharan R <[e-mail]>
Signed-off-by: Archit Taneja <[e-mail]>
[vivek: rework for clock and pm ops]
Signed-off-by: Vivek Gautam <[e-mail]>
Reviewed-by: Tomasz Figa <[e-mail]>
Tested-by: Srinivas Kandagatla <[e-mail]>
---
 drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 74 insertions(+), 3 deletions(-)

Comments

Robin Murphy Sept. 26, 2018, 3:27 p.m. UTC | #1

On 30/08/18 15:45, Vivek Gautam wrote:
> From: Sricharan R <[e-mail]>
> 
> The smmu needs to be functional only when the respective
> master's using it are active. The device_link feature
> helps to track such functional dependencies, so that the
> iommu gets powered when the master device enables itself
> using pm_runtime. So by adapting the smmu driver for
> runtime pm, above said dependency can be addressed.
> 
> This patch adds the pm runtime/sleep callbacks to the
> driver and also the functions to parse the smmu clocks
> from DT and enable them in resume/suspend.
> 
> Also, while we enable the runtime pm add a pm sleep suspend
> callback that pushes devices to low power state by turning
> the clocks off in a system sleep.
> Also add corresponding clock enable path in resume callback.
> 
> Signed-off-by: Sricharan R <[e-mail]>
> Signed-off-by: Archit Taneja <[e-mail]>
> [vivek: rework for clock and pm ops]
> Signed-off-by: Vivek Gautam <[e-mail]>
> Reviewed-by: Tomasz Figa <[e-mail]>
> Tested-by: Srinivas Kandagatla <[e-mail]>
> ---
>   drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 74 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index fd1b80ef9490..d900e007c3c9 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,6 +48,7 @@
>   #include <linux/of_iommu.h>
>   #include <linux/pci.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
>   
> @@ -205,6 +206,8 @@ struct arm_smmu_device {
>   	u32				num_global_irqs;
>   	u32				num_context_irqs;
>   	unsigned int			*irqs;
> +	struct clk_bulk_data		*clks;
> +	int				num_clks;
>   
>   	u32				cavium_id_base; /* Specific to Cavium */
>   
> @@ -1896,10 +1899,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>   struct arm_smmu_match_data {
>   	enum arm_smmu_arch_version version;
>   	enum arm_smmu_implementation model;
> +	const char * const *clks;
> +	int num_clks;
>   };
>   
>   #define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +static const struct arm_smmu_match_data name = { .version = ver, .model = imp }
>   
>   ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>   ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> @@ -1918,6 +1923,23 @@ static const struct of_device_id arm_smmu_of_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>   
> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
> +				   const char * const *clks)
> +{
> +	int i;
> +
> +	if (smmu->num_clks < 1)
> +		return;
> +
> +	smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
> +				  sizeof(*smmu->clks), GFP_KERNEL);
> +	if (!smmu->clks)
> +		return;
> +
> +	for (i = 0; i < smmu->num_clks; i++)
> +		smmu->clks[i].id = clks[i];
> +}
> +
>   #ifdef CONFIG_ACPI
>   static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
>   {
> @@ -2000,6 +2022,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>   	data = of_device_get_match_data(dev);
>   	smmu->version = data->version;
>   	smmu->model = data->model;
> +	smmu->num_clks = data->num_clks;
> +
> +	arm_smmu_fill_clk_data(smmu, data->clks);
>   
>   	parse_driver_options(smmu);
>   
> @@ -2098,6 +2123,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   		smmu->irqs[i] = irq;
>   	}
>   
> +	err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
> +	if (err)
> +		return err;
> +
> +	err = clk_bulk_prepare_enable(smmu->num_clks, smmu->clks);
> +	if (err)
> +		return err;
> +

Hmm, if we error out beyond here it looks like we should strictly 
balance that prepare/enable before devres does the clk_bulk_put(), 
however the probe error path is starting to look like it needs a bit of 
love in general, so I might just spin a cleanup patch on top (and even 
then only for the sake of not being a bad example; SMMU probe failure is 
never a realistic situation for the system to actually recover from).

Otherwise,

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

>   	err = arm_smmu_device_cfg_probe(smmu);
>   	if (err)
>   		return err;
> @@ -2184,6 +2217,9 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
>   
>   	/* Turn the thing off */
>   	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> +
> +	clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> +
>   	return 0;
>   }
>   
> @@ -2192,15 +2228,50 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
>   	arm_smmu_device_remove(pdev);
>   }
>   
> -static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
>   {
>   	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
> +	if (ret)
> +		return ret;
>   
>   	arm_smmu_device_reset(smmu);
> +
>   	return 0;
>   }
>   
> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> +{
> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable(smmu->num_clks, smmu->clks);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> +{
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	return arm_smmu_runtime_resume(dev);
> +}
> +
> +static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
> +{
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	return arm_smmu_runtime_suspend(dev);
> +}
> +
> +static const struct dev_pm_ops arm_smmu_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_pm_suspend, arm_smmu_pm_resume)
> +	SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
> +			   arm_smmu_runtime_resume, NULL)
> +};
>   
>   static struct platform_driver arm_smmu_driver = {
>   	.driver	= {
>

Vivek Gautam Sept. 27, 2018, 8:31 a.m. UTC | #2

On Wed, Sep 26, 2018 at 8:57 PM Robin Murphy <[e-mail]> wrote:
>
> On 30/08/18 15:45, Vivek Gautam wrote:
> > From: Sricharan R <[e-mail]>
> >
> > The smmu needs to be functional only when the respective
> > master's using it are active. The device_link feature
> > helps to track such functional dependencies, so that the
> > iommu gets powered when the master device enables itself
> > using pm_runtime. So by adapting the smmu driver for
> > runtime pm, above said dependency can be addressed.
> >
> > This patch adds the pm runtime/sleep callbacks to the
> > driver and also the functions to parse the smmu clocks
> > from DT and enable them in resume/suspend.
> >
> > Also, while we enable the runtime pm add a pm sleep suspend
> > callback that pushes devices to low power state by turning
> > the clocks off in a system sleep.
> > Also add corresponding clock enable path in resume callback.
> >
> > Signed-off-by: Sricharan R <[e-mail]>
> > Signed-off-by: Archit Taneja <[e-mail]>
> > [vivek: rework for clock and pm ops]
> > Signed-off-by: Vivek Gautam <[e-mail]>
> > Reviewed-by: Tomasz Figa <[e-mail]>
> > Tested-by: Srinivas Kandagatla <[e-mail]>
> > ---
> >   drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 74 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index fd1b80ef9490..d900e007c3c9 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -48,6 +48,7 @@
> >   #include <linux/of_iommu.h>
> >   #include <linux/pci.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >   #include <linux/slab.h>
> >   #include <linux/spinlock.h>
> >
> > @@ -205,6 +206,8 @@ struct arm_smmu_device {
> >       u32                             num_global_irqs;
> >       u32                             num_context_irqs;
> >       unsigned int                    *irqs;
> > +     struct clk_bulk_data            *clks;
> > +     int                             num_clks;
> >
> >       u32                             cavium_id_base; /* Specific to Cavium */
> >
> > @@ -1896,10 +1899,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >   struct arm_smmu_match_data {
> >       enum arm_smmu_arch_version version;
> >       enum arm_smmu_implementation model;
> > +     const char * const *clks;
> > +     int num_clks;
> >   };
> >
> >   #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
> > -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> > +static const struct arm_smmu_match_data name = { .version = ver, .model = imp }
> >
> >   ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> >   ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> > @@ -1918,6 +1923,23 @@ static const struct of_device_id arm_smmu_of_match[] = {
> >   };
> >   MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
> >
> > +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
> > +                                const char * const *clks)
> > +{
> > +     int i;
> > +
> > +     if (smmu->num_clks < 1)
> > +             return;
> > +
> > +     smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
> > +                               sizeof(*smmu->clks), GFP_KERNEL);
> > +     if (!smmu->clks)
> > +             return;
> > +
> > +     for (i = 0; i < smmu->num_clks; i++)
> > +             smmu->clks[i].id = clks[i];
> > +}
> > +
> >   #ifdef CONFIG_ACPI
> >   static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> >   {
> > @@ -2000,6 +2022,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
> >       data = of_device_get_match_data(dev);
> >       smmu->version = data->version;
> >       smmu->model = data->model;
> > +     smmu->num_clks = data->num_clks;
> > +
> > +     arm_smmu_fill_clk_data(smmu, data->clks);
> >
> >       parse_driver_options(smmu);
> >
> > @@ -2098,6 +2123,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> >               smmu->irqs[i] = irq;
> >       }
> >
> > +     err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
> > +     if (err)
> > +             return err;
> > +
> > +     err = clk_bulk_prepare_enable(smmu->num_clks, smmu->clks);
> > +     if (err)
> > +             return err;
> > +
>
> Hmm, if we error out beyond here it looks like we should strictly
> balance that prepare/enable before devres does the clk_bulk_put(),
> however the probe error path is starting to look like it needs a bit of
> love in general, so I might just spin a cleanup patch on top (and even
> then only for the sake of not being a bad example; SMMU probe failure is
> never a realistic situation for the system to actually recover from).

Sure Robin. Thanks for the review on the series.
Let me know, I can spin a change for probe failure path cleanup.

Best regards
Vivek

>
> Otherwise,
>
> Reviewed-by: Robin Murphy <[e-mail]>
>
> >       err = arm_smmu_device_cfg_probe(smmu);
> >       if (err)
> >               return err;
> > @@ -2184,6 +2217,9 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
> >
> >       /* Turn the thing off */
> >       writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> > +
> > +     clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> > +
> >       return 0;
> >   }
> >
> > @@ -2192,15 +2228,50 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
> >       arm_smmu_device_remove(pdev);
> >   }
> >
> > -static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> > +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
> >   {
> >       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > +     int ret;
> > +
> > +     ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
> > +     if (ret)
> > +             return ret;
> >
> >       arm_smmu_device_reset(smmu);
> > +
> >       return 0;
> >   }
> >
> > -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> > +{
> > +     struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > +
> > +     clk_bulk_disable(smmu->num_clks, smmu->clks);
> > +
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> > +{
> > +     if (pm_runtime_suspended(dev))
> > +             return 0;
> > +
> > +     return arm_smmu_runtime_resume(dev);
> > +}
> > +
> > +static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
> > +{
> > +     if (pm_runtime_suspended(dev))
> > +             return 0;
> > +
> > +     return arm_smmu_runtime_suspend(dev);
> > +}
> > +
> > +static const struct dev_pm_ops arm_smmu_pm_ops = {
> > +     SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_pm_suspend, arm_smmu_pm_resume)
> > +     SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
> > +                        arm_smmu_runtime_resume, NULL)
> > +};
> >
> >   static struct platform_driver arm_smmu_driver = {
> >       .driver = {
> >
> _______________________________________________
> iommu mailing list
> [e-mail]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Ulf Hansson Sept. 28, 2018, 11:59 a.m. UTC | #3

On 30 August 2018 at 16:45, Vivek Gautam <[e-mail]> wrote:
> From: Sricharan R <[e-mail]>
>
> The smmu needs to be functional only when the respective
> master's using it are active. The device_link feature
> helps to track such functional dependencies, so that the
> iommu gets powered when the master device enables itself
> using pm_runtime. So by adapting the smmu driver for
> runtime pm, above said dependency can be addressed.
>
> This patch adds the pm runtime/sleep callbacks to the
> driver and also the functions to parse the smmu clocks
> from DT and enable them in resume/suspend.
>
> Also, while we enable the runtime pm add a pm sleep suspend
> callback that pushes devices to low power state by turning
> the clocks off in a system sleep.
> Also add corresponding clock enable path in resume callback.
>
> Signed-off-by: Sricharan R <[e-mail]>
> Signed-off-by: Archit Taneja <[e-mail]>
> [vivek: rework for clock and pm ops]
> Signed-off-by: Vivek Gautam <[e-mail]>
> Reviewed-by: Tomasz Figa <[e-mail]>
> Tested-by: Srinivas Kandagatla <[e-mail]>
> ---
>  drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 74 insertions(+), 3 deletions(-)
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c

[...]

> -static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
>  {
>         struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
> +       if (ret)
> +               return ret;
>
>         arm_smmu_device_reset(smmu);
> +
>         return 0;
>  }
>
> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> +{
> +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> +       clk_bulk_disable(smmu->num_clks, smmu->clks);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> +{
> +       if (pm_runtime_suspended(dev))
> +               return 0;

Looks like you should be able use pm_runtime_force_resume(), instead
of using this local trick. Unless I am missing something, of course.

In other words, just assign the system sleep callbacks for resume, to
pm_runtime_force_resume(). And vice verse for the system suspend
callbacks, pm_runtime_force_suspend(), of course.

> +
> +       return arm_smmu_runtime_resume(dev);
> +}
> +
> +static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
> +{
> +       if (pm_runtime_suspended(dev))
> +               return 0;
> +
> +       return arm_smmu_runtime_suspend(dev);
> +}
> +
> +static const struct dev_pm_ops arm_smmu_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_pm_suspend, arm_smmu_pm_resume)

I am wondering if using the ->suspend|resume() callback is really
"late/early" enough in the device suspend phase?

Others is using the noirq phase and some is even using the syscore
ops. Of course it depends on the behavior of the consumers of iommu
device, and I guess not everyone is using device links, which for sure
improves things in this regards as well.

> +       SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
> +                          arm_smmu_runtime_resume, NULL)
> +};
>
>  static struct platform_driver arm_smmu_driver = {
>         .driver = {
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>

BTW, apologize for very late review comments.

Besides the above comments, the series looks good to me.

Kind regards
Uffe

Vivek Gautam Oct. 1, 2018, 5:49 a.m. UTC | #4

HI Ulf,

On Fri, Sep 28, 2018 at 5:30 PM Ulf Hansson <[e-mail]> wrote:
>
> On 30 August 2018 at 16:45, Vivek Gautam <[e-mail]> wrote:
> > From: Sricharan R <[e-mail]>
> >
> > The smmu needs to be functional only when the respective
> > master's using it are active. The device_link feature
> > helps to track such functional dependencies, so that the
> > iommu gets powered when the master device enables itself
> > using pm_runtime. So by adapting the smmu driver for
> > runtime pm, above said dependency can be addressed.
> >
> > This patch adds the pm runtime/sleep callbacks to the
> > driver and also the functions to parse the smmu clocks
> > from DT and enable them in resume/suspend.
> >
> > Also, while we enable the runtime pm add a pm sleep suspend
> > callback that pushes devices to low power state by turning
> > the clocks off in a system sleep.
> > Also add corresponding clock enable path in resume callback.
> >
> > Signed-off-by: Sricharan R <[e-mail]>
> > Signed-off-by: Archit Taneja <[e-mail]>
> > [vivek: rework for clock and pm ops]
> > Signed-off-by: Vivek Gautam <[e-mail]>
> > Reviewed-by: Tomasz Figa <[e-mail]>
> > Tested-by: Srinivas Kandagatla <[e-mail]>
> > ---
> >  drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 74 insertions(+), 3 deletions(-)
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>
> [...]
>
> > -static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> > +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
> >  {
> >         struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > +       int ret;
> > +
> > +       ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
> > +       if (ret)
> > +               return ret;
> >
> >         arm_smmu_device_reset(smmu);
> > +
> >         return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> > +{
> > +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > +
> > +       clk_bulk_disable(smmu->num_clks, smmu->clks);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> > +{
> > +       if (pm_runtime_suspended(dev))
> > +               return 0;
>
> Looks like you should be able use pm_runtime_force_resume(), instead
> of using this local trick. Unless I am missing something, of course.
>
> In other words, just assign the system sleep callbacks for resume, to
> pm_runtime_force_resume(). And vice verse for the system suspend
> callbacks, pm_runtime_force_suspend(), of course.

Thanks for the review. I will change this as suggested.

>
> > +
> > +       return arm_smmu_runtime_resume(dev);
> > +}
> > +
> > +static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
> > +{
> > +       if (pm_runtime_suspended(dev))
> > +               return 0;
> > +
> > +       return arm_smmu_runtime_suspend(dev);
> > +}
> > +
> > +static const struct dev_pm_ops arm_smmu_pm_ops = {
> > +       SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_pm_suspend, arm_smmu_pm_resume)
>
> I am wondering if using the ->suspend|resume() callback is really
> "late/early" enough in the device suspend phase?
>
> Others is using the noirq phase and some is even using the syscore
> ops. Of course it depends on the behavior of the consumers of iommu
> device, and I guess not everyone is using device links, which for sure
> improves things in this regards as well.

Well yes, as you said the device links should be able to take care of
maintaining the correct suspend/resume order of smmu and its clients,
or am I missing your point here?
Let me know and I will be happy to incorporate any suggestions.
Thanks

Regards
Vivek

>
> > +       SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
> > +                          arm_smmu_runtime_resume, NULL)
> > +};
> >
> >  static struct platform_driver arm_smmu_driver = {
> >         .driver = {
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> > of Code Aurora Forum, hosted by The Linux Foundation
> >
>
> BTW, apologize for very late review comments.
>
> Besides the above comments, the series looks good to me.
>
> Kind regards
> Uffe
> _______________________________________________
> iommu mailing list
> [e-mail]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Ulf Hansson Oct. 1, 2018, 9:38 a.m. UTC | #5

On 1 October 2018 at 07:49, Vivek Gautam <[e-mail]> wrote:
> HI Ulf,
>
> On Fri, Sep 28, 2018 at 5:30 PM Ulf Hansson <[e-mail]> wrote:
>>
>> On 30 August 2018 at 16:45, Vivek Gautam <[e-mail]> wrote:
>> > From: Sricharan R <[e-mail]>
>> >
>> > The smmu needs to be functional only when the respective
>> > master's using it are active. The device_link feature
>> > helps to track such functional dependencies, so that the
>> > iommu gets powered when the master device enables itself
>> > using pm_runtime. So by adapting the smmu driver for
>> > runtime pm, above said dependency can be addressed.
>> >
>> > This patch adds the pm runtime/sleep callbacks to the
>> > driver and also the functions to parse the smmu clocks
>> > from DT and enable them in resume/suspend.
>> >
>> > Also, while we enable the runtime pm add a pm sleep suspend
>> > callback that pushes devices to low power state by turning
>> > the clocks off in a system sleep.
>> > Also add corresponding clock enable path in resume callback.
>> >
>> > Signed-off-by: Sricharan R <[e-mail]>
>> > Signed-off-by: Archit Taneja <[e-mail]>
>> > [vivek: rework for clock and pm ops]
>> > Signed-off-by: Vivek Gautam <[e-mail]>
>> > Reviewed-by: Tomasz Figa <[e-mail]>
>> > Tested-by: Srinivas Kandagatla <[e-mail]>
>> > ---
>> >  drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
>> >  1 file changed, 74 insertions(+), 3 deletions(-)
>> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>
>> [...]
>>
>> > -static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
>> > +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
>> >  {
>> >         struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> > +       int ret;
>> > +
>> > +       ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
>> > +       if (ret)
>> > +               return ret;
>> >
>> >         arm_smmu_device_reset(smmu);
>> > +
>> >         return 0;
>> >  }
>> >
>> > -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
>> > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
>> > +{
>> > +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> > +
>> > +       clk_bulk_disable(smmu->num_clks, smmu->clks);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
>> > +{
>> > +       if (pm_runtime_suspended(dev))
>> > +               return 0;
>>
>> Looks like you should be able use pm_runtime_force_resume(), instead
>> of using this local trick. Unless I am missing something, of course.
>>
>> In other words, just assign the system sleep callbacks for resume, to
>> pm_runtime_force_resume(). And vice verse for the system suspend
>> callbacks, pm_runtime_force_suspend(), of course.
>
> Thanks for the review. I will change this as suggested.
>
>>
>> > +
>> > +       return arm_smmu_runtime_resume(dev);
>> > +}
>> > +
>> > +static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
>> > +{
>> > +       if (pm_runtime_suspended(dev))
>> > +               return 0;
>> > +
>> > +       return arm_smmu_runtime_suspend(dev);
>> > +}
>> > +
>> > +static const struct dev_pm_ops arm_smmu_pm_ops = {
>> > +       SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_pm_suspend, arm_smmu_pm_resume)
>>
>> I am wondering if using the ->suspend|resume() callback is really
>> "late/early" enough in the device suspend phase?
>>
>> Others is using the noirq phase and some is even using the syscore
>> ops. Of course it depends on the behavior of the consumers of iommu
>> device, and I guess not everyone is using device links, which for sure
>> improves things in this regards as well.
>
> Well yes, as you said the device links should be able to take care of
> maintaining the correct suspend/resume order of smmu and its clients,
> or am I missing your point here?
> Let me know and I will be happy to incorporate any suggestions.
> Thanks

If it works fine, then you may keep it as is.

Just wanted to point out that if any consumers relies on the iommu to
operational to say until the suspend-late phase, then this doesn't
play. Then you need to move your callbacks to the corresponding same
phase.

[...]

Kind regards
Uffe

Vivek Gautam Oct. 1, 2018, 10:21 a.m. UTC | #6

On Mon, Oct 1, 2018 at 3:09 PM Ulf Hansson <[e-mail]> wrote:
>
> On 1 October 2018 at 07:49, Vivek Gautam <[e-mail]> wrote:
> > HI Ulf,
> >
> > On Fri, Sep 28, 2018 at 5:30 PM Ulf Hansson <[e-mail]> wrote:
> >>
> >> On 30 August 2018 at 16:45, Vivek Gautam <[e-mail]> wrote:
> >> > From: Sricharan R <[e-mail]>
> >> >
> >> > The smmu needs to be functional only when the respective
> >> > master's using it are active. The device_link feature
> >> > helps to track such functional dependencies, so that the
> >> > iommu gets powered when the master device enables itself
> >> > using pm_runtime. So by adapting the smmu driver for
> >> > runtime pm, above said dependency can be addressed.
> >> >
> >> > This patch adds the pm runtime/sleep callbacks to the
> >> > driver and also the functions to parse the smmu clocks
> >> > from DT and enable them in resume/suspend.
> >> >
> >> > Also, while we enable the runtime pm add a pm sleep suspend
> >> > callback that pushes devices to low power state by turning
> >> > the clocks off in a system sleep.
> >> > Also add corresponding clock enable path in resume callback.
> >> >
> >> > Signed-off-by: Sricharan R <[e-mail]>
> >> > Signed-off-by: Archit Taneja <[e-mail]>
> >> > [vivek: rework for clock and pm ops]
> >> > Signed-off-by: Vivek Gautam <[e-mail]>
> >> > Reviewed-by: Tomasz Figa <[e-mail]>
> >> > Tested-by: Srinivas Kandagatla <[e-mail]>
> >> > ---
> >> >  drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
> >> >  1 file changed, 74 insertions(+), 3 deletions(-)
> >> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >>
> >> [...]
> >>
> >> > -static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> >> > +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
> >> >  {
> >> >         struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> >> > +       int ret;
> >> > +
> >> > +       ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
> >> > +       if (ret)
> >> > +               return ret;
> >> >
> >> >         arm_smmu_device_reset(smmu);
> >> > +
> >> >         return 0;
> >> >  }
> >> >
> >> > -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> >> > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> >> > +{
> >> > +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> >> > +
> >> > +       clk_bulk_disable(smmu->num_clks, smmu->clks);
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> >> > +{
> >> > +       if (pm_runtime_suspended(dev))
> >> > +               return 0;
> >>
> >> Looks like you should be able use pm_runtime_force_resume(), instead
> >> of using this local trick. Unless I am missing something, of course.
> >>
> >> In other words, just assign the system sleep callbacks for resume, to
> >> pm_runtime_force_resume(). And vice verse for the system suspend
> >> callbacks, pm_runtime_force_suspend(), of course.
> >
> > Thanks for the review. I will change this as suggested.
> >
> >>
> >> > +
> >> > +       return arm_smmu_runtime_resume(dev);
> >> > +}
> >> > +
> >> > +static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
> >> > +{
> >> > +       if (pm_runtime_suspended(dev))
> >> > +               return 0;
> >> > +
> >> > +       return arm_smmu_runtime_suspend(dev);
> >> > +}
> >> > +
> >> > +static const struct dev_pm_ops arm_smmu_pm_ops = {
> >> > +       SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_pm_suspend, arm_smmu_pm_resume)
> >>
> >> I am wondering if using the ->suspend|resume() callback is really
> >> "late/early" enough in the device suspend phase?
> >>
> >> Others is using the noirq phase and some is even using the syscore
> >> ops. Of course it depends on the behavior of the consumers of iommu
> >> device, and I guess not everyone is using device links, which for sure
> >> improves things in this regards as well.
> >
> > Well yes, as you said the device links should be able to take care of
> > maintaining the correct suspend/resume order of smmu and its clients,
> > or am I missing your point here?
> > Let me know and I will be happy to incorporate any suggestions.
> > Thanks
>
> If it works fine, then you may keep it as is.
>
> Just wanted to point out that if any consumers relies on the iommu to
> operational to say until the suspend-late phase, then this doesn't
> play. Then you need to move your callbacks to the corresponding same
> phase.

Although I have no means to test the suspend-late phase, tests with graphics
and display on db820 haven't shown any anomaly.

[snip]

Best regards
Vivek

Vivek Gautam Oct. 1, 2018, 10:32 a.m. UTC | #7

On Mon, Oct 1, 2018 at 11:19 AM Vivek Gautam
<[e-mail]> wrote:
>
> HI Ulf,
>
> On Fri, Sep 28, 2018 at 5:30 PM Ulf Hansson <[e-mail]> wrote:
> >
> > On 30 August 2018 at 16:45, Vivek Gautam <[e-mail]> wrote:
> > > From: Sricharan R <[e-mail]>
> > >
> > > The smmu needs to be functional only when the respective
> > > master's using it are active. The device_link feature
> > > helps to track such functional dependencies, so that the
> > > iommu gets powered when the master device enables itself
> > > using pm_runtime. So by adapting the smmu driver for
> > > runtime pm, above said dependency can be addressed.
> > >
> > > This patch adds the pm runtime/sleep callbacks to the
> > > driver and also the functions to parse the smmu clocks
> > > from DT and enable them in resume/suspend.
> > >
> > > Also, while we enable the runtime pm add a pm sleep suspend
> > > callback that pushes devices to low power state by turning
> > > the clocks off in a system sleep.
> > > Also add corresponding clock enable path in resume callback.
> > >
> > > Signed-off-by: Sricharan R <[e-mail]>
> > > Signed-off-by: Archit Taneja <[e-mail]>
> > > [vivek: rework for clock and pm ops]
> > > Signed-off-by: Vivek Gautam <[e-mail]>
> > > Reviewed-by: Tomasz Figa <[e-mail]>
> > > Tested-by: Srinivas Kandagatla <[e-mail]>
> > > ---
> > >  drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 74 insertions(+), 3 deletions(-)
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >
> > [...]
> >
> > > -static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> > > +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
> > >  {
> > >         struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > > +       int ret;
> > > +
> > > +       ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
> > > +       if (ret)
> > > +               return ret;
> > >
> > >         arm_smmu_device_reset(smmu);
> > > +
> > >         return 0;
> > >  }
> > >
> > > -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> > > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> > > +{
> > > +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > > +
> > > +       clk_bulk_disable(smmu->num_clks, smmu->clks);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
> > > +{
> > > +       if (pm_runtime_suspended(dev))
> > > +               return 0;
> >
> > Looks like you should be able use pm_runtime_force_resume(), instead
> > of using this local trick. Unless I am missing something, of course.
> >
> > In other words, just assign the system sleep callbacks for resume, to
> > pm_runtime_force_resume(). And vice verse for the system suspend
> > callbacks, pm_runtime_force_suspend(), of course.
>
> Thanks for the review. I will change this as suggested.

Coming back at this - actually Rafael suggested _not_ to use
pm_runtime_force_suspend/resume() when Marek had suggested
the same [1].
He also mentioned few caveats/limitations of using these APIs
for system sleep ops.
Let me know your opinion. Thanks.

[1] https://lkml.org/lkml/2018/7/11/978
[2] https://lkml.org/lkml/2018/7/23/334

Best regards
Vivek

Ulf Hansson Oct. 1, 2018, 12:02 p.m. UTC | #8

On 1 October 2018 at 12:32, Vivek Gautam <[e-mail]> wrote:
> On Mon, Oct 1, 2018 at 11:19 AM Vivek Gautam
> <[e-mail]> wrote:
>>
>> HI Ulf,
>>
>> On Fri, Sep 28, 2018 at 5:30 PM Ulf Hansson <[e-mail]> wrote:
>> >
>> > On 30 August 2018 at 16:45, Vivek Gautam <[e-mail]> wrote:
>> > > From: Sricharan R <[e-mail]>
>> > >
>> > > The smmu needs to be functional only when the respective
>> > > master's using it are active. The device_link feature
>> > > helps to track such functional dependencies, so that the
>> > > iommu gets powered when the master device enables itself
>> > > using pm_runtime. So by adapting the smmu driver for
>> > > runtime pm, above said dependency can be addressed.
>> > >
>> > > This patch adds the pm runtime/sleep callbacks to the
>> > > driver and also the functions to parse the smmu clocks
>> > > from DT and enable them in resume/suspend.
>> > >
>> > > Also, while we enable the runtime pm add a pm sleep suspend
>> > > callback that pushes devices to low power state by turning
>> > > the clocks off in a system sleep.
>> > > Also add corresponding clock enable path in resume callback.
>> > >
>> > > Signed-off-by: Sricharan R <[e-mail]>
>> > > Signed-off-by: Archit Taneja <[e-mail]>
>> > > [vivek: rework for clock and pm ops]
>> > > Signed-off-by: Vivek Gautam <[e-mail]>
>> > > Reviewed-by: Tomasz Figa <[e-mail]>
>> > > Tested-by: Srinivas Kandagatla <[e-mail]>
>> > > ---
>> > >  drivers/iommu/arm-smmu.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++--
>> > >  1 file changed, 74 insertions(+), 3 deletions(-)
>> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> >
>> > [...]
>> >
>> > > -static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
>> > > +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
>> > >  {
>> > >         struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> > > +       int ret;
>> > > +
>> > > +       ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
>> > > +       if (ret)
>> > > +               return ret;
>> > >
>> > >         arm_smmu_device_reset(smmu);
>> > > +
>> > >         return 0;
>> > >  }
>> > >
>> > > -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
>> > > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
>> > > +{
>> > > +       struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>> > > +
>> > > +       clk_bulk_disable(smmu->num_clks, smmu->clks);
>> > > +
>> > > +       return 0;
>> > > +}
>> > > +
>> > > +static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
>> > > +{
>> > > +       if (pm_runtime_suspended(dev))
>> > > +               return 0;
>> >
>> > Looks like you should be able use pm_runtime_force_resume(), instead
>> > of using this local trick. Unless I am missing something, of course.
>> >
>> > In other words, just assign the system sleep callbacks for resume, to
>> > pm_runtime_force_resume(). And vice verse for the system suspend
>> > callbacks, pm_runtime_force_suspend(), of course.
>>
>> Thanks for the review. I will change this as suggested.
>
> Coming back at this - actually Rafael suggested _not_ to use
> pm_runtime_force_suspend/resume() when Marek had suggested
> the same [1].

I see.

> He also mentioned few caveats/limitations of using these APIs
> for system sleep ops.
> Let me know your opinion. Thanks.

>
> [1] https://lkml.org/lkml/2018/7/11/978
> [2] https://lkml.org/lkml/2018/7/23/334

Me and Rafael have been discussing these topics historically as well.
I don't want to get that discussion started again here.

If your device is attached to the PCI bus or the ACPI PM domain (and
also gets runtime PM enabled), then I suggest you to stick to the
currently suggested approach. Otherwise it should be perfectly fine to
switch to the *force helpers.

Kind regards
Uffe

Patch

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index fd1b80ef9490..d900e007c3c9 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -48,6 +48,7 @@ 
 #include <linux/of_iommu.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -205,6 +206,8 @@  struct arm_smmu_device {
 	u32				num_global_irqs;
 	u32				num_context_irqs;
 	unsigned int			*irqs;
+	struct clk_bulk_data		*clks;
+	int				num_clks;
 
 	u32				cavium_id_base; /* Specific to Cavium */
 
@@ -1896,10 +1899,12 @@  static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 struct arm_smmu_match_data {
 	enum arm_smmu_arch_version version;
 	enum arm_smmu_implementation model;
+	const char * const *clks;
+	int num_clks;
 };
 
 #define ARM_SMMU_MATCH_DATA(name, ver, imp)	\
-static struct arm_smmu_match_data name = { .version = ver, .model = imp }
+static const struct arm_smmu_match_data name = { .version = ver, .model = imp }
 
 ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
 ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
@@ -1918,6 +1923,23 @@  static const struct of_device_id arm_smmu_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
+static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
+				   const char * const *clks)
+{
+	int i;
+
+	if (smmu->num_clks < 1)
+		return;
+
+	smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
+				  sizeof(*smmu->clks), GFP_KERNEL);
+	if (!smmu->clks)
+		return;
+
+	for (i = 0; i < smmu->num_clks; i++)
+		smmu->clks[i].id = clks[i];
+}
+
 #ifdef CONFIG_ACPI
 static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
 {
@@ -2000,6 +2022,9 @@  static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 	data = of_device_get_match_data(dev);
 	smmu->version = data->version;
 	smmu->model = data->model;
+	smmu->num_clks = data->num_clks;
+
+	arm_smmu_fill_clk_data(smmu, data->clks);
 
 	parse_driver_options(smmu);
 
@@ -2098,6 +2123,14 @@  static int arm_smmu_device_probe(struct platform_device *pdev)
 		smmu->irqs[i] = irq;
 	}
 
+	err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
+	if (err)
+		return err;
+
+	err = clk_bulk_prepare_enable(smmu->num_clks, smmu->clks);
+	if (err)
+		return err;
+
 	err = arm_smmu_device_cfg_probe(smmu);
 	if (err)
 		return err;
@@ -2184,6 +2217,9 @@  static int arm_smmu_device_remove(struct platform_device *pdev)
 
 	/* Turn the thing off */
 	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
+
+	clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
+
 	return 0;
 }
 
@@ -2192,15 +2228,50 @@  static void arm_smmu_device_shutdown(struct platform_device *pdev)
 	arm_smmu_device_remove(pdev);
 }
 
-static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
+static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
 {
 	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_bulk_enable(smmu->num_clks, smmu->clks);
+	if (ret)
+		return ret;
 
 	arm_smmu_device_reset(smmu);
+
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
+static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	clk_bulk_disable(smmu->num_clks, smmu->clks);
+
+	return 0;
+}
+
+static int __maybe_unused arm_smmu_pm_resume(struct device *dev)
+{
+	if (pm_runtime_suspended(dev))
+		return 0;
+
+	return arm_smmu_runtime_resume(dev);
+}
+
+static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
+{
+	if (pm_runtime_suspended(dev))
+		return 0;
+
+	return arm_smmu_runtime_suspend(dev);
+}
+
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_pm_suspend, arm_smmu_pm_resume)
+	SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
+			   arm_smmu_runtime_resume, NULL)
+};
 
 static struct platform_driver arm_smmu_driver = {
 	.driver	= {