| Message ID | 20180708173413.1965-2-vivek.gautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: > From: Sricharan R <sricharan@codeaurora.org> > > 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. > > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > Signed-off-by: Archit Taneja <architt@codeaurora.org> > [vivek: Clock rework to request bulk of clocks] > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > --- > > - No change since v11. > > drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 58 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index f7a96bcf94a6..a01d0dde21dd 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 */ > > @@ -1897,10 +1900,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); > @@ -1919,6 +1924,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) > { > @@ -2001,6 +2023,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); > > @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); > + if (err) > + return err; > + > err = arm_smmu_device_cfg_probe(smmu); > if (err) > return err; > @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); > + > return 0; > } > > @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) > return 0; > } > > -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); > +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) > +{ > + struct arm_smmu_device *smmu = dev_get_drvdata(dev); > + > + return clk_bulk_enable(smmu->num_clks, smmu->clks); > +} > + > +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 const struct dev_pm_ops arm_smmu_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) This is suspicious. If you need a runtime suspend method, why do you think that it is not necessary to suspend the device during system-wide transitions? > + SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend, > + arm_smmu_runtime_resume, NULL) > +}; > > static struct platform_driver arm_smmu_driver = { > .driver = { > -- 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
Hi Rafael, On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >> From: Sricharan R <sricharan@codeaurora.org> >> >> 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. >> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> Signed-off-by: Archit Taneja <architt@codeaurora.org> >> [vivek: Clock rework to request bulk of clocks] >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> --- >> >> - No change since v11. >> >> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 58 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index f7a96bcf94a6..a01d0dde21dd 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 */ >> >> @@ -1897,10 +1900,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); >> @@ -1919,6 +1924,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) >> { >> @@ -2001,6 +2023,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); >> >> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); >> + if (err) >> + return err; >> + >> err = arm_smmu_device_cfg_probe(smmu); >> if (err) >> return err; >> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); >> + >> return 0; >> } >> >> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >> return 0; >> } >> >> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >> +{ >> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >> + >> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >> +} >> + >> +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 const struct dev_pm_ops arm_smmu_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) > > This is suspicious. > > If you need a runtime suspend method, why do you think that it is not necessary > to suspend the device during system-wide transitions? Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? In that case the clocks have to be enabled in the resume path too. I remember Tomasz pointed to that we shouldn't need clock enable in resume path [1]. [1] https://lkml.org/lkml/2018/3/15/60 Best regards Vivek > >> + SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend, >> + arm_smmu_runtime_resume, NULL) >> +}; >> >> static struct platform_driver arm_smmu_driver = { >> .driver = { >> > >
On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > Hi Rafael, > > > On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >>> From: Sricharan R <sricharan@codeaurora.org> >>> >>> 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. >>> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >>> [vivek: Clock rework to request bulk of clocks] >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>> --- >>> >>> - No change since v11. >>> >>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 58 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index f7a96bcf94a6..a01d0dde21dd 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 */ >>> >>> @@ -1897,10 +1900,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); >>> @@ -1919,6 +1924,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) >>> { >>> @@ -2001,6 +2023,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); >>> >>> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); >>> + if (err) >>> + return err; >>> + >>> err = arm_smmu_device_cfg_probe(smmu); >>> if (err) >>> return err; >>> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); >>> + >>> return 0; >>> } >>> >>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >>> return 0; >>> } >>> >>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >>> +{ >>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>> + >>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >>> +} >>> + >>> +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 const struct dev_pm_ops arm_smmu_pm_ops = { >>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >> >> This is suspicious. >> >> If you need a runtime suspend method, why do you think that it is not necessary >> to suspend the device during system-wide transitions? > > Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? > In that case the clocks have to be enabled in the resume path too. > > I remember Tomasz pointed to that we shouldn't need clock enable in resume > path [1]. > > [1] https://lkml.org/lkml/2018/3/15/60 Honestly, I just don't know. :-) It just looks odd the way it is done. I think the clock should be gated during system-wide suspend too, because the system can spend much more time in a sleep state than in the working state, on average. And note that you cannot rely on runtime PM to always do it for you, because it may be disabled at a client device or even blocked by user space via power/control in sysfs and that shouldn't matter for system-wide PM. Thanks, Rafael -- 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
On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: > > Hi Rafael, > > > > > > On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: > >>> From: Sricharan R <sricharan@codeaurora.org> > >>> > >>> 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. > >>> > >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> > >>> Signed-off-by: Archit Taneja <architt@codeaurora.org> > >>> [vivek: Clock rework to request bulk of clocks] > >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > >>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> > >>> --- > >>> > >>> - No change since v11. > >>> > >>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- > >>> 1 file changed, 58 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > >>> index f7a96bcf94a6..a01d0dde21dd 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 */ > >>> > >>> @@ -1897,10 +1900,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); > >>> @@ -1919,6 +1924,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) > >>> { > >>> @@ -2001,6 +2023,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); > >>> > >>> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); > >>> + if (err) > >>> + return err; > >>> + > >>> err = arm_smmu_device_cfg_probe(smmu); > >>> if (err) > >>> return err; > >>> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); > >>> + > >>> return 0; > >>> } > >>> > >>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) > >>> return 0; > >>> } > >>> > >>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); > >>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) > >>> +{ > >>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); > >>> + > >>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); > >>> +} > >>> + > >>> +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 const struct dev_pm_ops arm_smmu_pm_ops = { > >>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) > >> > >> This is suspicious. > >> > >> If you need a runtime suspend method, why do you think that it is not necessary > >> to suspend the device during system-wide transitions? > > > > Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? > > In that case the clocks have to be enabled in the resume path too. > > > > I remember Tomasz pointed to that we shouldn't need clock enable in resume > > path [1]. > > > > [1] https://lkml.org/lkml/2018/3/15/60 That was an answer for a different question. I don't remember suggesting having no suspend function. Although, given the PM subsystem internals, the suspend function wouldn't be called on SMMU implementation needed power control (since they would have runtime PM enabled) and on others, it would be called but do nothing (since no clocks). > > Honestly, I just don't know. :-) > > It just looks odd the way it is done. I think the clock should be > gated during system-wide suspend too, because the system can spend > much more time in a sleep state than in the working state, on average. > > And note that you cannot rely on runtime PM to always do it for you, > because it may be disabled at a client device or even blocked by user > space via power/control in sysfs and that shouldn't matter for > system-wide PM. User space blocking runtime PM through sysfs is a good point. I'm not 100% sure how the PM subsystem deals with that in case of system-wide suspend. I guess for consistency and safety, we should have the suspend callback. Best regards, Tomasz -- 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
Hi Tomasz, On 2018-07-11 14:51, Tomasz Figa wrote: > On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >>> On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >>>>> From: Sricharan R <sricharan@codeaurora.org> >>>>> >>>>> 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. >>>>> >>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >>>>> [vivek: Clock rework to request bulk of clocks] >>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>> --- >>>>> >>>>> - No change since v11. >>>>> >>>>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 58 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>> index f7a96bcf94a6..a01d0dde21dd 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 */ >>>>> >>>>> @@ -1897,10 +1900,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); >>>>> @@ -1919,6 +1924,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) >>>>> { >>>>> @@ -2001,6 +2023,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); >>>>> >>>>> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); >>>>> + if (err) >>>>> + return err; >>>>> + >>>>> err = arm_smmu_device_cfg_probe(smmu); >>>>> if (err) >>>>> return err; >>>>> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); >>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >>>>> return 0; >>>>> } >>>>> >>>>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >>>>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >>>>> +{ >>>>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>>>> + >>>>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >>>>> +} >>>>> + >>>>> +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 const struct dev_pm_ops arm_smmu_pm_ops = { >>>>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >>>> This is suspicious. >>>> >>>> If you need a runtime suspend method, why do you think that it is not necessary >>>> to suspend the device during system-wide transitions? >>> Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? >>> In that case the clocks have to be enabled in the resume path too. >>> >>> I remember Tomasz pointed to that we shouldn't need clock enable in resume >>> path [1]. >>> >>> [1] https://lkml.org/lkml/2018/3/15/60 > That was an answer for a different question. I don't remember > suggesting having no suspend function. Although, given the PM > subsystem internals, the suspend function wouldn't be called on SMMU > implementation needed power control (since they would have runtime PM > enabled) and on others, it would be called but do nothing (since no > clocks). > >> Honestly, I just don't know. :-) >> >> It just looks odd the way it is done. I think the clock should be >> gated during system-wide suspend too, because the system can spend >> much more time in a sleep state than in the working state, on average. >> >> And note that you cannot rely on runtime PM to always do it for you, >> because it may be disabled at a client device or even blocked by user >> space via power/control in sysfs and that shouldn't matter for >> system-wide PM. > User space blocking runtime PM through sysfs is a good point. I'm not > 100% sure how the PM subsystem deals with that in case of system-wide > suspend. I guess for consistency and safety, we should have the > suspend callback. Frankly, if there are no other reasons I suggest to wire system suspend/resume to pm_runtime_force_* helpers: SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pm_runtime_force_resume). This way you will have everything related to suspending and resuming in one place and you would not need to bother about all possible cases (like suspending from runtime pm active and suspending from runtime pm suspended cases as well as restoring proper device state on resume). This is especially important in recent kernel releases, where devices are system-suspended regardless their runtime pm states (in older kernels devices were first runtime resumed for system suspend, what made code simpler, but wasn't best from power consumption perspective). If you go this way, You only need to ensure that runtime resume will also restore proper device state besides enabling all the clocks. This will also prepare your driver to properly operate inside power domain, where it is possible for device to loose its internal state after runtime suspend when respective power domain has been turned off. Best regards
On Wed, Jul 11, 2018 at 3:40 PM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hi Tomasz, > > On 2018-07-11 14:51, Tomasz Figa wrote: >> On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >>> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam >>> <vivek.gautam@codeaurora.org> wrote: >>>> On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>>> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >>>>>> From: Sricharan R <sricharan@codeaurora.org> >>>>>> >>>>>> 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. >>>>>> >>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >>>>>> [vivek: Clock rework to request bulk of clocks] >>>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>>> --- >>>>>> >>>>>> - No change since v11. >>>>>> >>>>>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >>>>>> 1 file changed, 58 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>> index f7a96bcf94a6..a01d0dde21dd 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 */ >>>>>> >>>>>> @@ -1897,10 +1900,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); >>>>>> @@ -1919,6 +1924,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) >>>>>> { >>>>>> @@ -2001,6 +2023,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); >>>>>> >>>>>> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); >>>>>> + if (err) >>>>>> + return err; >>>>>> + >>>>>> err = arm_smmu_device_cfg_probe(smmu); >>>>>> if (err) >>>>>> return err; >>>>>> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); >>>>>> + >>>>>> return 0; >>>>>> } >>>>>> >>>>>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >>>>>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >>>>>> +{ >>>>>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>>>>> + >>>>>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >>>>>> +} >>>>>> + >>>>>> +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 const struct dev_pm_ops arm_smmu_pm_ops = { >>>>>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >>>>> This is suspicious. >>>>> >>>>> If you need a runtime suspend method, why do you think that it is not necessary >>>>> to suspend the device during system-wide transitions? >>>> Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? >>>> In that case the clocks have to be enabled in the resume path too. >>>> >>>> I remember Tomasz pointed to that we shouldn't need clock enable in resume >>>> path [1]. >>>> >>>> [1] https://lkml.org/lkml/2018/3/15/60 >> That was an answer for a different question. I don't remember >> suggesting having no suspend function. Although, given the PM >> subsystem internals, the suspend function wouldn't be called on SMMU >> implementation needed power control (since they would have runtime PM >> enabled) and on others, it would be called but do nothing (since no >> clocks). >> >>> Honestly, I just don't know. :-) >>> >>> It just looks odd the way it is done. I think the clock should be >>> gated during system-wide suspend too, because the system can spend >>> much more time in a sleep state than in the working state, on average. >>> >>> And note that you cannot rely on runtime PM to always do it for you, >>> because it may be disabled at a client device or even blocked by user >>> space via power/control in sysfs and that shouldn't matter for >>> system-wide PM. >> User space blocking runtime PM through sysfs is a good point. I'm not >> 100% sure how the PM subsystem deals with that in case of system-wide >> suspend. I guess for consistency and safety, we should have the >> suspend callback. > > Frankly, if there are no other reasons I suggest to wire system > suspend/resume to pm_runtime_force_* helpers: > SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume). Not a good idea at all IMO. Use PM driver flags rather I'd say. > This way you will have everything related to suspending and resuming in > one place and you would not need to bother about all possible cases (like > suspending from runtime pm active and suspending from runtime pm suspended > cases as well as restoring proper device state on resume). This is > especially important in recent kernel releases, where devices are > system-suspended regardless their runtime pm states (in older kernels > devices were first runtime resumed for system suspend, what made code > simpler, but wasn't best from power consumption perspective). > > If you go this way, You only need to ensure that runtime resume will also > restore proper device state besides enabling all the clocks. This will > also prepare your driver to properly operate inside power domain, where it > is possible for device to loose its internal state after runtime suspend > when respective power domain has been turned off. I'm not sure if you are aware of the pm_runtime_force_* limitations, though. Thanks, Rafael -- 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
Hi, On Wed, Jul 11, 2018 at 6:21 PM, Tomasz Figa <tfiga@chromium.org> wrote: > On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >> >> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >> > Hi Rafael, >> > >> > >> > On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> >> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >> >>> From: Sricharan R <sricharan@codeaurora.org> >> >>> >> >>> 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. >> >>> >> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> >>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >> >>> [vivek: Clock rework to request bulk of clocks] >> >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> >>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> >>> --- >> >>> >> >>> - No change since v11. >> >>> >> >>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >> >>> 1 file changed, 58 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> >>> index f7a96bcf94a6..a01d0dde21dd 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 */ >> >>> >> >>> @@ -1897,10 +1900,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); >> >>> @@ -1919,6 +1924,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) >> >>> { >> >>> @@ -2001,6 +2023,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); >> >>> >> >>> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); >> >>> + if (err) >> >>> + return err; >> >>> + >> >>> err = arm_smmu_device_cfg_probe(smmu); >> >>> if (err) >> >>> return err; >> >>> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); >> >>> + >> >>> return 0; >> >>> } >> >>> >> >>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >> >>> return 0; >> >>> } >> >>> >> >>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >> >>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >> >>> +{ >> >>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >> >>> + >> >>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >> >>> +} >> >>> + >> >>> +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 const struct dev_pm_ops arm_smmu_pm_ops = { >> >>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >> >> >> >> This is suspicious. >> >> >> >> If you need a runtime suspend method, why do you think that it is not necessary >> >> to suspend the device during system-wide transitions? >> > >> > Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? >> > In that case the clocks have to be enabled in the resume path too. >> > >> > I remember Tomasz pointed to that we shouldn't need clock enable in resume >> > path [1]. >> > >> > [1] https://lkml.org/lkml/2018/3/15/60 > > That was an answer for a different question. I don't remember > suggesting having no suspend function. My bad, apologies. You are right, we were discussing if we need any additional handling of power for arm_smmu_device_reset() in arm_smmu_pm_resume(). > Although, given the PM > subsystem internals, the suspend function wouldn't be called on SMMU > implementation needed power control (since they would have runtime PM > enabled) and on others, it would be called but do nothing (since no > clocks). > >> >> Honestly, I just don't know. :-) >> >> It just looks odd the way it is done. I think the clock should be >> gated during system-wide suspend too, because the system can spend >> much more time in a sleep state than in the working state, on average. >> >> And note that you cannot rely on runtime PM to always do it for you, >> because it may be disabled at a client device or even blocked by user >> space via power/control in sysfs and that shouldn't matter for >> system-wide PM. > > User space blocking runtime PM through sysfs is a good point. I'm not > 100% sure how the PM subsystem deals with that in case of system-wide > suspend. I guess for consistency and safety, we should have the > suspend callback. Will add the following suspend callback (same as arm_smmu_runtime_suspend): static int __maybe_unused arm_smmu_pm_suspend(struct device *dev) { struct arm_smmu_device *smmu = dev_get_drvdata(dev); clk_bulk_disable(smmu->num_clks, smmu->clks); return 0; } Best regards Vivek > > Best regards, > Tomasz > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Thu, Jul 12, 2018 at 12:57 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > Hi, > > > On Wed, Jul 11, 2018 at 6:21 PM, Tomasz Figa <tfiga@chromium.org> wrote: >> On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >>> >>> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam >>> <vivek.gautam@codeaurora.org> wrote: >>> > Hi Rafael, >>> > >>> > >>> > On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>> >> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >>> >>> From: Sricharan R <sricharan@codeaurora.org> >>> >>> >>> >>> 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. >>> >>> >>> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>> >>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >>> >>> [vivek: Clock rework to request bulk of clocks] >>> >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>> >>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>> >>> --- >>> >>> >>> >>> - No change since v11. >>> >>> >>> >>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >>> >>> 1 file changed, 58 insertions(+), 2 deletions(-) >>> >>> >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> >>> index f7a96bcf94a6..a01d0dde21dd 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 */ >>> >>> >>> >>> @@ -1897,10 +1900,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); >>> >>> @@ -1919,6 +1924,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) >>> >>> { >>> >>> @@ -2001,6 +2023,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); >>> >>> >>> >>> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); >>> >>> + if (err) >>> >>> + return err; >>> >>> + >>> >>> err = arm_smmu_device_cfg_probe(smmu); >>> >>> if (err) >>> >>> return err; >>> >>> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); >>> >>> + >>> >>> return 0; >>> >>> } >>> >>> >>> >>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >>> >>> return 0; >>> >>> } >>> >>> >>> >>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >>> >>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >>> >>> +{ >>> >>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>> >>> + >>> >>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >>> >>> +} >>> >>> + >>> >>> +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 const struct dev_pm_ops arm_smmu_pm_ops = { >>> >>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >>> >> >>> >> This is suspicious. >>> >> >>> >> If you need a runtime suspend method, why do you think that it is not necessary >>> >> to suspend the device during system-wide transitions? >>> > >>> > Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? >>> > In that case the clocks have to be enabled in the resume path too. >>> > >>> > I remember Tomasz pointed to that we shouldn't need clock enable in resume >>> > path [1]. >>> > >>> > [1] https://lkml.org/lkml/2018/3/15/60 >> >> That was an answer for a different question. I don't remember >> suggesting having no suspend function. > > My bad, apologies. You are right, we were discussing if we need any additional > handling of power for arm_smmu_device_reset() in arm_smmu_pm_resume(). > >> Although, given the PM >> subsystem internals, the suspend function wouldn't be called on SMMU >> implementation needed power control (since they would have runtime PM >> enabled) and on others, it would be called but do nothing (since no >> clocks). >> >>> >>> Honestly, I just don't know. :-) >>> >>> It just looks odd the way it is done. I think the clock should be >>> gated during system-wide suspend too, because the system can spend >>> much more time in a sleep state than in the working state, on average. >>> >>> And note that you cannot rely on runtime PM to always do it for you, >>> because it may be disabled at a client device or even blocked by user >>> space via power/control in sysfs and that shouldn't matter for >>> system-wide PM. >> >> User space blocking runtime PM through sysfs is a good point. I'm not >> 100% sure how the PM subsystem deals with that in case of system-wide >> suspend. I guess for consistency and safety, we should have the >> suspend callback. > > Will add the following suspend callback (same as arm_smmu_runtime_suspend): > > static int __maybe_unused arm_smmu_pm_suspend(struct device *dev) > { > struct arm_smmu_device *smmu = dev_get_drvdata(dev); > > clk_bulk_disable(smmu->num_clks, smmu->clks); > > return 0; > } I think you also need to check if the clock has already been disabled by runtime PM. Otherwise you may end up disabling it twice in a row. -- 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
HI Rafael, On 7/16/2018 2:21 PM, Rafael J. Wysocki wrote: > On Thu, Jul 12, 2018 at 12:57 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> Hi, >> >> >> On Wed, Jul 11, 2018 at 6:21 PM, Tomasz Figa <tfiga@chromium.org> wrote: >>> On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >>>> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam >>>> <vivek.gautam@codeaurora.org> wrote: >>>>> Hi Rafael, >>>>> >>>>> >>>>> On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>>>> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >>>>>>> From: Sricharan R <sricharan@codeaurora.org> >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >>>>>>> [vivek: Clock rework to request bulk of clocks] >>>>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>>>> --- >>>>>>> >>>>>>> - No change since v11. >>>>>>> >>>>>>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >>>>>>> 1 file changed, 58 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>>> index f7a96bcf94a6..a01d0dde21dd 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 */ >>>>>>> >>>>>>> @@ -1897,10 +1900,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); >>>>>>> @@ -1919,6 +1924,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) >>>>>>> { >>>>>>> @@ -2001,6 +2023,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); >>>>>>> >>>>>>> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); >>>>>>> + if (err) >>>>>>> + return err; >>>>>>> + >>>>>>> err = arm_smmu_device_cfg_probe(smmu); >>>>>>> if (err) >>>>>>> return err; >>>>>>> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); >>>>>>> + >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >>>>>>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >>>>>>> +{ >>>>>>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>>>>>> + >>>>>>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >>>>>>> +} >>>>>>> + >>>>>>> +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 const struct dev_pm_ops arm_smmu_pm_ops = { >>>>>>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >>>>>> This is suspicious. >>>>>> >>>>>> If you need a runtime suspend method, why do you think that it is not necessary >>>>>> to suspend the device during system-wide transitions? >>>>> Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? >>>>> In that case the clocks have to be enabled in the resume path too. >>>>> >>>>> I remember Tomasz pointed to that we shouldn't need clock enable in resume >>>>> path [1]. >>>>> >>>>> [1] https://lkml.org/lkml/2018/3/15/60 >>> That was an answer for a different question. I don't remember >>> suggesting having no suspend function. >> My bad, apologies. You are right, we were discussing if we need any additional >> handling of power for arm_smmu_device_reset() in arm_smmu_pm_resume(). >> >>> Although, given the PM >>> subsystem internals, the suspend function wouldn't be called on SMMU >>> implementation needed power control (since they would have runtime PM >>> enabled) and on others, it would be called but do nothing (since no >>> clocks). >>> >>>> Honestly, I just don't know. :-) >>>> >>>> It just looks odd the way it is done. I think the clock should be >>>> gated during system-wide suspend too, because the system can spend >>>> much more time in a sleep state than in the working state, on average. >>>> >>>> And note that you cannot rely on runtime PM to always do it for you, >>>> because it may be disabled at a client device or even blocked by user >>>> space via power/control in sysfs and that shouldn't matter for >>>> system-wide PM. >>> User space blocking runtime PM through sysfs is a good point. I'm not >>> 100% sure how the PM subsystem deals with that in case of system-wide >>> suspend. I guess for consistency and safety, we should have the >>> suspend callback. >> Will add the following suspend callback (same as arm_smmu_runtime_suspend): >> >> static int __maybe_unused arm_smmu_pm_suspend(struct device *dev) >> { >> struct arm_smmu_device *smmu = dev_get_drvdata(dev); >> >> clk_bulk_disable(smmu->num_clks, smmu->clks); >> >> return 0; >> } > I think you also need to check if the clock has already been disabled > by runtime PM. Otherwise you may end up disabling it twice in a row. Should I rather call a pm_runtime_put() in suspend callback? Or an expanded form something similar to: https://elixir.bootlin.com/linux/v4.18-rc5/source/drivers/slimbus/qcom-ctrl.c#L695 Best regards Vivek -- 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
Hi, On Mon, Jul 16, 2018 at 12:11 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > HI Rafael, > > > > On 7/16/2018 2:21 PM, Rafael J. Wysocki wrote: >> >> On Thu, Jul 12, 2018 at 12:57 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: [cut] >>>> Although, given the PM >>>> subsystem internals, the suspend function wouldn't be called on SMMU >>>> implementation needed power control (since they would have runtime PM >>>> enabled) and on others, it would be called but do nothing (since no >>>> clocks). >>>> >>>>> Honestly, I just don't know. :-) >>>>> >>>>> It just looks odd the way it is done. I think the clock should be >>>>> gated during system-wide suspend too, because the system can spend >>>>> much more time in a sleep state than in the working state, on average. >>>>> >>>>> And note that you cannot rely on runtime PM to always do it for you, >>>>> because it may be disabled at a client device or even blocked by user >>>>> space via power/control in sysfs and that shouldn't matter for >>>>> system-wide PM. >>>> >>>> User space blocking runtime PM through sysfs is a good point. I'm not >>>> 100% sure how the PM subsystem deals with that in case of system-wide >>>> suspend. I guess for consistency and safety, we should have the >>>> suspend callback. >>> >>> Will add the following suspend callback (same as >>> arm_smmu_runtime_suspend): >>> >>> static int __maybe_unused arm_smmu_pm_suspend(struct device *dev) >>> { >>> struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>> >>> clk_bulk_disable(smmu->num_clks, smmu->clks); >>> >>> return 0; >>> } >> >> I think you also need to check if the clock has already been disabled >> by runtime PM. Otherwise you may end up disabling it twice in a row. > > > Should I rather call a pm_runtime_put() in suspend callback? That wouldn't work as runtime PM may be effectively disabled by user space via sysfs. That's one of the reasons why you need the extra system-wide suspend callback in the first place. :-) > Or an expanded form something similar to: > https://elixir.bootlin.com/linux/v4.18-rc5/source/drivers/slimbus/qcom-ctrl.c#L695 Yes, you can do something like that, but be careful to make sure that the state of the device after system-wide resume is consistent with its runtime PM status in all cases. -- 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
Hi Rafael, On 2018-07-11 22:36, Rafael J. Wysocki wrote: > On Wed, Jul 11, 2018 at 3:40 PM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> Hi Tomasz, >> >> On 2018-07-11 14:51, Tomasz Figa wrote: >>> On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >>>> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam >>>> <vivek.gautam@codeaurora.org> wrote: >>>>> On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>>>>> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote: >>>>>>> From: Sricharan R <sricharan@codeaurora.org> >>>>>>> >>>>>>> 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. >>>>>>> >>>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>>>>> Signed-off-by: Archit Taneja <architt@codeaurora.org> >>>>>>> [vivek: Clock rework to request bulk of clocks] >>>>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>>>> --- >>>>>>> >>>>>>> - No change since v11. >>>>>>> >>>>>>> drivers/iommu/arm-smmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >>>>>>> 1 file changed, 58 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>>> index f7a96bcf94a6..a01d0dde21dd 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 */ >>>>>>> >>>>>>> @@ -1897,10 +1900,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); >>>>>>> @@ -1919,6 +1924,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) >>>>>>> { >>>>>>> @@ -2001,6 +2023,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); >>>>>>> >>>>>>> @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); >>>>>>> + if (err) >>>>>>> + return err; >>>>>>> + >>>>>>> err = arm_smmu_device_cfg_probe(smmu); >>>>>>> if (err) >>>>>>> return err; >>>>>>> @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); >>>>>>> + >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); >>>>>>> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) >>>>>>> +{ >>>>>>> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >>>>>>> + >>>>>>> + return clk_bulk_enable(smmu->num_clks, smmu->clks); >>>>>>> +} >>>>>>> + >>>>>>> +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 const struct dev_pm_ops arm_smmu_pm_ops = { >>>>>>> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >>>>>> This is suspicious. >>>>>> >>>>>> If you need a runtime suspend method, why do you think that it is not necessary >>>>>> to suspend the device during system-wide transitions? >>>>> Okay, so you suggest to put clock disabling in say arm_smmu_pm_suspend()? >>>>> In that case the clocks have to be enabled in the resume path too. >>>>> >>>>> I remember Tomasz pointed to that we shouldn't need clock enable in resume >>>>> path [1]. >>>>> >>>>> [1] https://lkml.org/lkml/2018/3/15/60 >>> That was an answer for a different question. I don't remember >>> suggesting having no suspend function. Although, given the PM >>> subsystem internals, the suspend function wouldn't be called on SMMU >>> implementation needed power control (since they would have runtime PM >>> enabled) and on others, it would be called but do nothing (since no >>> clocks). >>> >>>> Honestly, I just don't know. :-) >>>> >>>> It just looks odd the way it is done. I think the clock should be >>>> gated during system-wide suspend too, because the system can spend >>>> much more time in a sleep state than in the working state, on average. >>>> >>>> And note that you cannot rely on runtime PM to always do it for you, >>>> because it may be disabled at a client device or even blocked by user >>>> space via power/control in sysfs and that shouldn't matter for >>>> system-wide PM. >>> User space blocking runtime PM through sysfs is a good point. I'm not >>> 100% sure how the PM subsystem deals with that in case of system-wide >>> suspend. I guess for consistency and safety, we should have the >>> suspend callback. >> Frankly, if there are no other reasons I suggest to wire system >> suspend/resume to pm_runtime_force_* helpers: >> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >> pm_runtime_force_resume). > Not a good idea at all IMO. > > Use PM driver flags rather I'd say. Frankly, till now I wasn't aware of the DPM_FLAG_* in struct dev_pm_info 'driver_flags'. I've briefly checked them but I don't see the equivalent of using SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume): keep device suspend if it was runtime suspended AND really call pm_runtime_suspend if it was not runtime suspended on system suspend. >> This way you will have everything related to suspending and resuming in >> one place and you would not need to bother about all possible cases (like >> suspending from runtime pm active and suspending from runtime pm suspended >> cases as well as restoring proper device state on resume). This is >> especially important in recent kernel releases, where devices are >> system-suspended regardless their runtime pm states (in older kernels >> devices were first runtime resumed for system suspend, what made code >> simpler, but wasn't best from power consumption perspective). >> >> If you go this way, You only need to ensure that runtime resume will also >> restore proper device state besides enabling all the clocks. This will >> also prepare your driver to properly operate inside power domain, where it >> is possible for device to loose its internal state after runtime suspend >> when respective power domain has been turned off. > I'm not sure if you are aware of the pm_runtime_force_* limitations, though. What are those limitations? Best regards
On Mon, Jul 23, 2018 at 7:59 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hi Rafael, > > On 2018-07-11 22:36, Rafael J. Wysocki wrote: >> On Wed, Jul 11, 2018 at 3:40 PM, Marek Szyprowski >> <m.szyprowski@samsung.com> wrote: [cut] >>> Frankly, if there are no other reasons I suggest to wire system >>> suspend/resume to pm_runtime_force_* helpers: >>> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>> pm_runtime_force_resume). >> Not a good idea at all IMO. >> >> Use PM driver flags rather I'd say. > > Frankly, till now I wasn't aware of the DPM_FLAG_* in struct dev_pm_info > 'driver_flags'. They are a relatively recent addition. > I've briefly checked them but I don't see the equivalent > of using SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume): keep device suspend if it was runtime suspended > AND really call pm_runtime_suspend if it was not runtime suspended on > system suspend. DPM_FLAG_SMART_SUSPEND is expected to cause that to happen. If you want the device to remain in suspend after the system-wide resume from sleep (if possible), you can set DPM_FLAG_LEAVE_SUSPENDED for it too. Currently a caveat is that genpd still doesn't support the flags. I have patches for that, but I haven't got to posting them yet. If you are interested, I can push this work forward relatively quickly, so please let me know. >>> This way you will have everything related to suspending and resuming in >>> one place and you would not need to bother about all possible cases (like >>> suspending from runtime pm active and suspending from runtime pm suspended >>> cases as well as restoring proper device state on resume). This is >>> especially important in recent kernel releases, where devices are >>> system-suspended regardless their runtime pm states (in older kernels >>> devices were first runtime resumed for system suspend, what made code >>> simpler, but wasn't best from power consumption perspective). >>> >>> If you go this way, You only need to ensure that runtime resume will also >>> restore proper device state besides enabling all the clocks. This will >>> also prepare your driver to properly operate inside power domain, where it >>> is possible for device to loose its internal state after runtime suspend >>> when respective power domain has been turned off. >> I'm not sure if you are aware of the pm_runtime_force_* limitations, though. > > What are those limitations? First off, they don't work with middle-layer code implementing its own PM callbacks that actually operate on devices (like the PCI bus type or the generic ACPI PM domain). This means that drivers using them may not work on systems with ACPI, for example. Second, pm_runtime_force_resume() will always try to leave the device in suspend during system-wide resume from sleep which may not be desirable. Finally, they expect the runtime PM status to be updated by system-wide PM callbacks of devices below the one using them (eg. its children and their children etc) which generally is not required and may not take place unless the drivers of those devices use pm_runtime_force_* themselves. So careful there. -- 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
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index f7a96bcf94a6..a01d0dde21dd 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 */ @@ -1897,10 +1900,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); @@ -1919,6 +1924,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) { @@ -2001,6 +2023,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); @@ -2099,6 +2124,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(smmu->num_clks, smmu->clks); + if (err) + return err; + err = arm_smmu_device_cfg_probe(smmu); if (err) return err; @@ -2181,6 +2214,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_unprepare(smmu->num_clks, smmu->clks); + return 0; } @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct device *dev) return 0; } -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume); +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev) +{ + struct arm_smmu_device *smmu = dev_get_drvdata(dev); + + return clk_bulk_enable(smmu->num_clks, smmu->clks); +} + +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 const struct dev_pm_ops arm_smmu_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(NULL, 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 = {