Message ID | 20180708173413.1965-1-vivek.gautam@codeaurora.org |
---|---|
Headers | show |
Series | iommu/arm-smmu: Add runtime pm/sleep support | expand |
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday, July 8, 2018 7:34:11 PM CEST Vivek Gautam wrote: > From: Sricharan R <sricharan@codeaurora.org> > > The smmu device probe/remove and add/remove master device callbacks > gets called when the smmu is not linked to its master, that is without > the context of the master device. So calling runtime apis in those places > separately. > > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > [vivek: Cleanup pm runtime calls] > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > --- > > - Change since v11 > * Replaced pm_runtime_disable() with pm_runtime_force_suspend() > to avoid warning about " Unpreparing enabled clock". > Full warning text mentioned in cover patch. > > drivers/iommu/arm-smmu.c | 92 +++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 84 insertions(+), 8 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index a01d0dde21dd..09265e206e2d 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = { > { 0, NULL}, > }; > > +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) > +{ > + if (pm_runtime_enabled(smmu->dev)) Why do you need the pm_runtime_enabled() checks here and below? pm_runtime_get_sync() and pm_runtime_put() should work just fine if runtime PM is not enabled. > + return pm_runtime_get_sync(smmu->dev); > + > + return 0; > +} > + > +static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu) > +{ > + if (pm_runtime_enabled(smmu->dev)) > + pm_runtime_put(smmu->dev); > +} > + > static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) > { > return container_of(dom, struct arm_smmu_domain, domain); > @@ -913,11 +927,15 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_device *smmu = smmu_domain->smmu; > struct arm_smmu_cfg *cfg = &smmu_domain->cfg; > - int irq; > + int ret, irq; > > if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY) > return; > > + ret = arm_smmu_rpm_get(smmu); > + if (ret < 0) > + return; > + > /* > * Disable the context bank and free the page tables before freeing > * it. > @@ -932,6 +950,8 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain) > > free_io_pgtable_ops(smmu_domain->pgtbl_ops); > __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx); > + > + arm_smmu_rpm_put(smmu); > } > > static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) > @@ -1213,10 +1233,15 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > return -ENODEV; > > smmu = fwspec_smmu(fwspec); > + > + ret = arm_smmu_rpm_get(smmu); > + if (ret < 0) > + return ret; > + > /* Ensure that the domain is finalised */ > ret = arm_smmu_init_domain_context(domain, smmu); > if (ret < 0) > - return ret; > + goto rpm_put; > > /* > * Sanity check the domain. We don't support domains across > @@ -1226,33 +1251,50 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > dev_err(dev, > "cannot attach to SMMU %s whilst already attached to domain on SMMU %s\n", > dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev)); > - return -EINVAL; > + ret = -EINVAL; > + goto rpm_put; > } > > /* Looks ok, so add the device to the domain */ > - return arm_smmu_domain_add_master(smmu_domain, fwspec); > + ret = arm_smmu_domain_add_master(smmu_domain, fwspec); > + > +rpm_put: > + arm_smmu_rpm_put(smmu); > + return ret; > } > > static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova, > phys_addr_t paddr, size_t size, int prot) > { > struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops; > + struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu; > + int ret; > > if (!ops) > return -ENODEV; > > - return ops->map(ops, iova, paddr, size, prot); > + arm_smmu_rpm_get(smmu); > + ret = ops->map(ops, iova, paddr, size, prot); > + arm_smmu_rpm_put(smmu); > + > + return ret; > } > > static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova, > size_t size) > { > struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops; > + struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu; > + size_t ret; > > if (!ops) > return 0; > > - return ops->unmap(ops, iova, size); > + arm_smmu_rpm_get(smmu); > + ret = ops->unmap(ops, iova, size); > + arm_smmu_rpm_put(smmu); > + > + return ret; > } > > static void arm_smmu_iotlb_sync(struct iommu_domain *domain) > @@ -1407,7 +1449,13 @@ static int arm_smmu_add_device(struct device *dev) > while (i--) > cfg->smendx[i] = INVALID_SMENDX; > > + ret = arm_smmu_rpm_get(smmu); > + if (ret < 0) > + goto out_cfg_free; > + > ret = arm_smmu_master_alloc_smes(dev); > + arm_smmu_rpm_put(smmu); > + > if (ret) > goto out_cfg_free; > > @@ -1427,7 +1475,7 @@ static void arm_smmu_remove_device(struct device *dev) > struct iommu_fwspec *fwspec = dev->iommu_fwspec; > struct arm_smmu_master_cfg *cfg; > struct arm_smmu_device *smmu; > - > + int ret; > > if (!fwspec || fwspec->ops != &arm_smmu_ops) > return; > @@ -1435,8 +1483,15 @@ static void arm_smmu_remove_device(struct device *dev) > cfg = fwspec->iommu_priv; > smmu = cfg->smmu; > > + ret = arm_smmu_rpm_get(smmu); > + if (ret < 0) > + return; > + > iommu_device_unlink(&smmu->iommu, dev); > arm_smmu_master_free_smes(fwspec); > + > + arm_smmu_rpm_put(smmu); > + > iommu_group_remove_device(dev); > kfree(fwspec->iommu_priv); > iommu_fwspec_free(dev); > @@ -2124,6 +2179,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > smmu->irqs[i] = irq; > } > > + platform_set_drvdata(pdev, smmu); > + > err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks); > if (err) > return err; > @@ -2132,6 +2189,19 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > if (err) > return err; > > + /* > + * We want to avoid touching dev->power.lock in fastpaths unless > + * it's really going to do something useful - pm_runtime_enabled() > + * can serve as an ideal proxy for that decision. So, conditionally > + * enable pm_runtime. > + */ > + if (dev->pm_domain) > + pm_runtime_enable(dev); > + > + err = arm_smmu_rpm_get(smmu); > + if (err < 0) > + return err; > + > err = arm_smmu_device_cfg_probe(smmu); > if (err) > return err; > @@ -2173,10 +2243,11 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > return err; > } > > - platform_set_drvdata(pdev, smmu); > arm_smmu_device_reset(smmu); > arm_smmu_test_smr_masks(smmu); > > + arm_smmu_rpm_put(smmu); > + > /* > * For ACPI and generic DT bindings, an SMMU will be probed before > * any device which might need it, so we want the bus ops in place > @@ -2212,8 +2283,13 @@ static int arm_smmu_device_remove(struct platform_device *pdev) > if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS)) > dev_err(&pdev->dev, "removing device with active domains!\n"); > > + arm_smmu_rpm_get(smmu); > /* Turn the thing off */ > writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0); > + arm_smmu_rpm_put(smmu); > + > + if (pm_runtime_enabled(smmu->dev)) > + pm_runtime_force_suspend(smmu->dev); > > clk_bulk_unprepare(smmu->num_clks, smmu->clks); > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: > From: Sricharan R <sricharan@codeaurora.org> > > Finally add the device link between the master device and > smmu, so that the smmu gets runtime enabled/disabled only when the > master needs it. This is done from add_device callback which gets > called once when the master is added to the smmu. > > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Lukas Wunner <lukas@wunner.de> > --- > > - Change since v11 > * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. > > drivers/iommu/arm-smmu.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 09265e206e2d..916cde4954d2 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) > > iommu_device_link(&smmu->iommu, dev); > > + if (pm_runtime_enabled(smmu->dev) && Why does the creation of the link depend on whether or not runtime PM is enabled for the MMU device? What about system-wide PM and system shutdown? Are they always guaranteed to happen in the right order without the link? > + !device_link_add(dev, smmu->dev, > + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { > + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", > + dev_name(dev)); > + ret = -ENODEV; > + goto out_unlink; > + } > + > return 0; > > +out_unlink: > + iommu_device_unlink(&smmu->iommu, dev); > + arm_smmu_master_free_smes(fwspec); > out_cfg_free: > kfree(cfg); > out_free: > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, Thanks for review. On Wed, Jul 11, 2018 at 6:53 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Sunday, July 8, 2018 7:34:11 PM CEST Vivek Gautam wrote: > > From: Sricharan R <sricharan@codeaurora.org> > > > > The smmu device probe/remove and add/remove master device callbacks > > gets called when the smmu is not linked to its master, that is without > > the context of the master device. So calling runtime apis in those places > > separately. > > > > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > > [vivek: Cleanup pm runtime calls] > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > --- > > > > - Change since v11 > > * Replaced pm_runtime_disable() with pm_runtime_force_suspend() > > to avoid warning about " Unpreparing enabled clock". > > Full warning text mentioned in cover patch. > > > > drivers/iommu/arm-smmu.c | 92 +++++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 84 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > > index a01d0dde21dd..09265e206e2d 100644 > > --- a/drivers/iommu/arm-smmu.c > > +++ b/drivers/iommu/arm-smmu.c > > @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = { > > { 0, NULL}, > > }; > > > > +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) > > +{ > > + if (pm_runtime_enabled(smmu->dev)) > > Why do you need the pm_runtime_enabled() checks here and below? > > pm_runtime_get_sync() and pm_runtime_put() should work just fine if > runtime PM is not enabled. Because pm_runtime_get_sync() acquires a spin lock, even if only for the short time of checking if runtime PM is enabled and SMMU driver maintainers didn't want any spin locks in certain IOMMU API code paths on hardware implementations that don't need runtime PM, while we still need to be able to control runtime PM there on hardware implementations that need so. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/11/2018 3:23 PM, Rafael J. Wysocki wrote: > On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >> From: Sricharan R <sricharan@codeaurora.org> >> >> Finally add the device link between the master device and >> smmu, so that the smmu gets runtime enabled/disabled only when the >> master needs it. This is done from add_device callback which gets >> called once when the master is added to the smmu. >> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Lukas Wunner <lukas@wunner.de> >> --- >> >> - Change since v11 >> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >> >> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 09265e206e2d..916cde4954d2 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >> >> iommu_device_link(&smmu->iommu, dev); >> >> + if (pm_runtime_enabled(smmu->dev) && > Why does the creation of the link depend on whether or not runtime PM > is enabled for the MMU device? The main purpose of this device link is to handle the runtime PM synchronization between the supplier (iommu) and consumer (client devices, such as GPU/display). Moreover, the runtime pm is conditionally enabled for smmu devices that support such [1]. > > What about system-wide PM and system shutdown? Are they always guaranteed > to happen in the right order without the link? When there's no runtime PM, there's no clocks, and other resources to be handled. So, we don't need device link for system-wide PM and system shutdown to work correctly. That's the case with current arm-smmu driver. Is it something that i am missing here? [1] https://lkml.org/lkml/2018/3/8/775 Thanks Vivek >> + !device_link_add(dev, smmu->dev, >> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { >> + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", >> + dev_name(dev)); >> + ret = -ENODEV; >> + goto out_unlink; >> + } >> + >> return 0; >> >> +out_unlink: >> + iommu_device_unlink(&smmu->iommu, dev); >> + arm_smmu_master_free_smes(fwspec); >> out_cfg_free: >> kfree(cfg); >> out_free: >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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:05 PM, Tomasz Figa <tfiga@chromium.org> wrote: > Hi Rafael, > > Thanks for review. > > On Wed, Jul 11, 2018 at 6:53 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> >> On Sunday, July 8, 2018 7:34:11 PM CEST Vivek Gautam wrote: >> > From: Sricharan R <sricharan@codeaurora.org> >> > >> > The smmu device probe/remove and add/remove master device callbacks >> > gets called when the smmu is not linked to its master, that is without >> > the context of the master device. So calling runtime apis in those places >> > separately. >> > >> > Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> > [vivek: Cleanup pm runtime calls] >> > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> > --- >> > >> > - Change since v11 >> > * Replaced pm_runtime_disable() with pm_runtime_force_suspend() >> > to avoid warning about " Unpreparing enabled clock". >> > Full warning text mentioned in cover patch. >> > >> > drivers/iommu/arm-smmu.c | 92 +++++++++++++++++++++++++++++++++++++++++++----- >> > 1 file changed, 84 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> > index a01d0dde21dd..09265e206e2d 100644 >> > --- a/drivers/iommu/arm-smmu.c >> > +++ b/drivers/iommu/arm-smmu.c >> > @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = { >> > { 0, NULL}, >> > }; >> > >> > +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) >> > +{ >> > + if (pm_runtime_enabled(smmu->dev)) >> >> Why do you need the pm_runtime_enabled() checks here and below? >> >> pm_runtime_get_sync() and pm_runtime_put() should work just fine if >> runtime PM is not enabled. > > Because pm_runtime_get_sync() acquires a spin lock, even if only for > the short time of checking if runtime PM is enabled and SMMU driver > maintainers didn't want any spin locks in certain IOMMU API code paths > on hardware implementations that don't need runtime PM, while we still > need to be able to control runtime PM there on hardware > implementations that need so. OK, so it is an optimization. It would be good to put a comment in there to that effect. -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/11/2018 4:29 PM, Rafael J. Wysocki wrote: > On Wed, Jul 11, 2018 at 12:05 PM, Tomasz Figa <tfiga@chromium.org> wrote: >> Hi Rafael, >> >> Thanks for review. >> >> On Wed, Jul 11, 2018 at 6:53 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >>> On Sunday, July 8, 2018 7:34:11 PM CEST Vivek Gautam wrote: >>>> From: Sricharan R <sricharan@codeaurora.org> >>>> >>>> The smmu device probe/remove and add/remove master device callbacks >>>> gets called when the smmu is not linked to its master, that is without >>>> the context of the master device. So calling runtime apis in those places >>>> separately. >>>> >>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>> [vivek: Cleanup pm runtime calls] >>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>> --- >>>> >>>> - Change since v11 >>>> * Replaced pm_runtime_disable() with pm_runtime_force_suspend() >>>> to avoid warning about " Unpreparing enabled clock". >>>> Full warning text mentioned in cover patch. >>>> >>>> drivers/iommu/arm-smmu.c | 92 +++++++++++++++++++++++++++++++++++++++++++----- >>>> 1 file changed, 84 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index a01d0dde21dd..09265e206e2d 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c >>>> @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = { >>>> { 0, NULL}, >>>> }; >>>> >>>> +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu) >>>> +{ >>>> + if (pm_runtime_enabled(smmu->dev)) >>> Why do you need the pm_runtime_enabled() checks here and below? >>> >>> pm_runtime_get_sync() and pm_runtime_put() should work just fine if >>> runtime PM is not enabled. >> Because pm_runtime_get_sync() acquires a spin lock, even if only for >> the short time of checking if runtime PM is enabled and SMMU driver >> maintainers didn't want any spin locks in certain IOMMU API code paths >> on hardware implementations that don't need runtime PM, while we still >> need to be able to control runtime PM there on hardware >> implementations that need so. > OK, so it is an optimization. It would be good to put a comment in > there to that effect. Yea, actually there's a comment placed in arm_smmu_device_probe() Â where the runtime PM is conditionally enabled. I can add comments for these wrappers too if you would like. Thanks & Regards Vivek -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 devicetree" 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 devicetree" 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
Hi Rafael, On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > Hi Rafael, > > > > On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >> >> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>> >>> From: Sricharan R <sricharan@codeaurora.org> >>> >>> Finally add the device link between the master device and >>> smmu, so that the smmu gets runtime enabled/disabled only when the >>> master needs it. This is done from add_device callback which gets >>> called once when the master is added to the smmu. >>> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Lukas Wunner <lukas@wunner.de> >>> --- >>> >>> - Change since v11 >>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>> >>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 09265e206e2d..916cde4954d2 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>> iommu_device_link(&smmu->iommu, dev); >>> + if (pm_runtime_enabled(smmu->dev) && >> >> Why does the creation of the link depend on whether or not runtime PM >> is enabled for the MMU device? > > > The main purpose of this device link is to handle the runtime PM > synchronization > between the supplier (iommu) and consumer (client devices, such as > GPU/display). > Moreover, the runtime pm is conditionally enabled for smmu devices that > support > such [1]. Is there something you would like me to modify in this patch? Best regards Vivek >> >> >> What about system-wide PM and system shutdown? Are they always guaranteed >> to happen in the right order without the link? > > > When there's no runtime PM, there's no clocks, and other resources to be > handled. > So, we don't need device link for system-wide PM and system shutdown to work > correctly. > That's the case with current arm-smmu driver. > Is it something that i am missing here? > > [1] https://lkml.org/lkml/2018/3/8/775 > > Thanks > Vivek >>> >>> + !device_link_add(dev, smmu->dev, >>> + DL_FLAG_PM_RUNTIME | >>> DL_FLAG_AUTOREMOVE_SUPPLIER)) { >>> + dev_err(smmu->dev, "Unable to add link to the consumer >>> %s\n", >>> + dev_name(dev)); >>> + ret = -ENODEV; >>> + goto out_unlink; >>> + } >>> + >>> return 0; >>> +out_unlink: >>> + iommu_device_unlink(&smmu->iommu, dev); >>> + arm_smmu_master_free_smes(fwspec); >>> out_cfg_free: >>> kfree(cfg); >>> out_free: >>> >> > > -- > 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 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > Hi Rafael, > > > On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> Hi Rafael, >> >> >> >> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>> >>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>> >>>> From: Sricharan R <sricharan@codeaurora.org> >>>> >>>> Finally add the device link between the master device and >>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>> master needs it. This is done from add_device callback which gets >>>> called once when the master is added to the smmu. >>>> >>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>> Cc: Lukas Wunner <lukas@wunner.de> >>>> --- >>>> >>>> - Change since v11 >>>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>>> >>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index 09265e206e2d..916cde4954d2 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c >>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>>> iommu_device_link(&smmu->iommu, dev); >>>> + if (pm_runtime_enabled(smmu->dev) && >>> >>> Why does the creation of the link depend on whether or not runtime PM >>> is enabled for the MMU device? >> >> >> The main purpose of this device link is to handle the runtime PM >> synchronization >> between the supplier (iommu) and consumer (client devices, such as >> GPU/display). >> Moreover, the runtime pm is conditionally enabled for smmu devices that >> support >> such [1]. > > Is there something you would like me to modify in this patch? Not really, as long as you are sure that it is correct. :-) You need to remember, however, that if you add system-wide PM callbacks to the driver, the ordering between them and the client device callbacks during system-wide suspend matters as well. Don't you need the link the ensure the correct system-wide suspend ordering too? -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 devicetree" 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote: > On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> Hi Rafael, >> >> >> On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >>> Hi Rafael, >>> >>> >>> >>> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>>> From: Sricharan R <sricharan@codeaurora.org> >>>>> >>>>> Finally add the device link between the master device and >>>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>>> master needs it. This is done from add_device callback which gets >>>>> called once when the master is added to the smmu. >>>>> >>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>>> Cc: Lukas Wunner <lukas@wunner.de> >>>>> --- >>>>> >>>>> - Change since v11 >>>>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>>>> >>>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>>> 1 file changed, 12 insertions(+) >>>>> >>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>> index 09265e206e2d..916cde4954d2 100644 >>>>> --- a/drivers/iommu/arm-smmu.c >>>>> +++ b/drivers/iommu/arm-smmu.c >>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>>>> iommu_device_link(&smmu->iommu, dev); >>>>> + if (pm_runtime_enabled(smmu->dev) && >>>> Why does the creation of the link depend on whether or not runtime PM >>>> is enabled for the MMU device? >>> >>> The main purpose of this device link is to handle the runtime PM >>> synchronization >>> between the supplier (iommu) and consumer (client devices, such as >>> GPU/display). >>> Moreover, the runtime pm is conditionally enabled for smmu devices that >>> support >>> such [1]. >> Is there something you would like me to modify in this patch? > Not really, as long as you are sure that it is correct. :-) > > You need to remember, however, that if you add system-wide PM > callbacks to the driver, the ordering between them and the client > device callbacks during system-wide suspend matters as well. Don't > you need the link the ensure the correct system-wide suspend ordering > too? The fact that currently we handle clocks only through runtime pm callbacks, would it be better to call runtime pm put/get in system-wide PM callbacks. This would be same as i mentioned in the other thread. 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 16, 2018 at 1:46 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > > On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote: >> >> On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >>> >>> Hi Rafael, >>> >>> >>> On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam >>> <vivek.gautam@codeaurora.org> wrote: >>>> >>>> Hi Rafael, >>>> >>>> >>>> >>>> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>>>> >>>>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>>>> >>>>>> From: Sricharan R <sricharan@codeaurora.org> >>>>>> >>>>>> Finally add the device link between the master device and >>>>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>>>> master needs it. This is done from add_device callback which gets >>>>>> called once when the master is added to the smmu. >>>>>> >>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>>>> Cc: Lukas Wunner <lukas@wunner.de> >>>>>> --- >>>>>> >>>>>> - Change since v11 >>>>>> * Replaced DL_FLAG_AUTOREMOVE flag with >>>>>> DL_FLAG_AUTOREMOVE_SUPPLIER. >>>>>> >>>>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>>>> 1 file changed, 12 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>> index 09265e206e2d..916cde4954d2 100644 >>>>>> --- a/drivers/iommu/arm-smmu.c >>>>>> +++ b/drivers/iommu/arm-smmu.c >>>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device >>>>>> *dev) >>>>>> iommu_device_link(&smmu->iommu, dev); >>>>>> + if (pm_runtime_enabled(smmu->dev) && >>>>> >>>>> Why does the creation of the link depend on whether or not runtime PM >>>>> is enabled for the MMU device? >>>> >>>> >>>> The main purpose of this device link is to handle the runtime PM >>>> synchronization >>>> between the supplier (iommu) and consumer (client devices, such as >>>> GPU/display). >>>> Moreover, the runtime pm is conditionally enabled for smmu devices that >>>> support >>>> such [1]. >>> >>> Is there something you would like me to modify in this patch? >> >> Not really, as long as you are sure that it is correct. :-) >> >> You need to remember, however, that if you add system-wide PM >> callbacks to the driver, the ordering between them and the client >> device callbacks during system-wide suspend matters as well. Don't >> you need the link the ensure the correct system-wide suspend ordering >> too? > > > The fact that currently we handle clocks only through runtime pm callbacks, > would it be better to call runtime pm put/get in system-wide PM callbacks. > This would be same as i mentioned in the other thread. Well, my point is that there's no reason for system-wide suspend to depend directly on runtime PM (which can be effectively disabled by user space as mentioned for multiple times in this thread). It simply is not efficient to let the clock run while the system as a whole is sleeping (even if power/control has been set to "on" for this particular device) and it should not be too hard to prevent that from happening. You may need an additional flag in the driver for that or similar, but it definitely should be doable. Now, that's my advice and I'm not the maintainer of that code, so it is your call (as long as the maintainer agrees with it). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/17/2018 1:16 PM, Rafael J. Wysocki wrote: > On Mon, Jul 16, 2018 at 1:46 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> >> On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote: >>> On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam >>> <vivek.gautam@codeaurora.org> wrote: >>>> Hi Rafael, >>>> >>>> >>>> On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam >>>> <vivek.gautam@codeaurora.org> wrote: >>>>> Hi Rafael, >>>>> >>>>> >>>>> >>>>> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>>>>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>>>>> From: Sricharan R <sricharan@codeaurora.org> >>>>>>> >>>>>>> Finally add the device link between the master device and >>>>>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>>>>> master needs it. This is done from add_device callback which gets >>>>>>> called once when the master is added to the smmu. >>>>>>> >>>>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>>>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>>>>> Cc: Lukas Wunner <lukas@wunner.de> >>>>>>> --- >>>>>>> >>>>>>> - Change since v11 >>>>>>> * Replaced DL_FLAG_AUTOREMOVE flag with >>>>>>> DL_FLAG_AUTOREMOVE_SUPPLIER. >>>>>>> >>>>>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>>>>> 1 file changed, 12 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>>> index 09265e206e2d..916cde4954d2 100644 >>>>>>> --- a/drivers/iommu/arm-smmu.c >>>>>>> +++ b/drivers/iommu/arm-smmu.c >>>>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device >>>>>>> *dev) >>>>>>> iommu_device_link(&smmu->iommu, dev); >>>>>>> + if (pm_runtime_enabled(smmu->dev) && >>>>>> Why does the creation of the link depend on whether or not runtime PM >>>>>> is enabled for the MMU device? >>>>> >>>>> The main purpose of this device link is to handle the runtime PM >>>>> synchronization >>>>> between the supplier (iommu) and consumer (client devices, such as >>>>> GPU/display). >>>>> Moreover, the runtime pm is conditionally enabled for smmu devices that >>>>> support >>>>> such [1]. >>>> Is there something you would like me to modify in this patch? >>> Not really, as long as you are sure that it is correct. :-) >>> >>> You need to remember, however, that if you add system-wide PM >>> callbacks to the driver, the ordering between them and the client >>> device callbacks during system-wide suspend matters as well. Don't >>> you need the link the ensure the correct system-wide suspend ordering >>> too? >> >> The fact that currently we handle clocks only through runtime pm callbacks, >> would it be better to call runtime pm put/get in system-wide PM callbacks. >> This would be same as i mentioned in the other thread. > Well, my point is that there's no reason for system-wide suspend to > depend directly on runtime PM (which can be effectively disabled by > user space as mentioned for multiple times in this thread). > > It simply is not efficient to let the clock run while the system as a > whole is sleeping (even if power/control has been set to "on" for this > particular device) and it should not be too hard to prevent that from > happening. You may need an additional flag in the driver for that or > similar, but it definitely should be doable. Right, I will modify the things are required. Thanks again for pointing this out. Best regards Vivek > > Now, that's my advice and I'm not the maintainer of that code, so it > is your call (as long as the maintainer agrees with it). > > Thanks, > Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 3:23 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >> From: Sricharan R <sricharan@codeaurora.org> >> >> Finally add the device link between the master device and >> smmu, so that the smmu gets runtime enabled/disabled only when the >> master needs it. This is done from add_device callback which gets >> called once when the master is added to the smmu. >> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Lukas Wunner <lukas@wunner.de> >> --- >> >> - Change since v11 >> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >> >> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 09265e206e2d..916cde4954d2 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >> >> iommu_device_link(&smmu->iommu, dev); >> >> + if (pm_runtime_enabled(smmu->dev) && > > Why does the creation of the link depend on whether or not runtime PM > is enabled for the MMU device? > > What about system-wide PM and system shutdown? Are they always guaranteed > to happen in the right order without the link? Hi Robin, As Rafael pointed, we should the device link creation should not depend on runtime PM being enabled or not, as we would also want to guarantee that system wide PM callbacks are called in the right order for smmu and clients. Does this change of removing the check for pm_runtime_enabled() from here looks okay to you? FYI, as discussed in the first patch [1] of this series, I will add a system wide suspend callback - arm_smmu_pm_suspend, that would do clock disable, and will add corresponding clock enable calls in arm_smmu_pm_resume(). [1] https://lore.kernel.org/patchwork/patch/960460/ Best regards Vivek > >> + !device_link_add(dev, smmu->dev, >> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { >> + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", >> + dev_name(dev)); >> + ret = -ENODEV; >> + goto out_unlink; >> + } >> + >> return 0; >> >> +out_unlink: >> + iommu_device_unlink(&smmu->iommu, dev); >> + arm_smmu_master_free_smes(fwspec); >> out_cfg_free: >> kfree(cfg); >> out_free: >> > >
On 18/07/18 10:30, Vivek Gautam wrote: > On Wed, Jul 11, 2018 at 3:23 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>> From: Sricharan R <sricharan@codeaurora.org> >>> >>> Finally add the device link between the master device and >>> smmu, so that the smmu gets runtime enabled/disabled only when the >>> master needs it. This is done from add_device callback which gets >>> called once when the master is added to the smmu. >>> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Lukas Wunner <lukas@wunner.de> >>> --- >>> >>> - Change since v11 >>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>> >>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 09265e206e2d..916cde4954d2 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>> >>> iommu_device_link(&smmu->iommu, dev); >>> >>> + if (pm_runtime_enabled(smmu->dev) && >> >> Why does the creation of the link depend on whether or not runtime PM >> is enabled for the MMU device? >> >> What about system-wide PM and system shutdown? Are they always guaranteed >> to happen in the right order without the link? > > Hi Robin, > > As Rafael pointed, we should the device link creation should not depend on > runtime PM being enabled or not, as we would also want to guarantee > that system wide PM callbacks are called in the right order for smmu > and clients. > > Does this change of removing the check for pm_runtime_enabled() from here > looks okay to you? FWIW the existing system PM ops make no claim to be perfect, and I wouldn't be at all surprised if it was only by coincidence that my devices happened to put on the relevant lists in the right order to start with. If we no longer need to worry about explicit device_link housekeeping in the SMMU driver, then creating them unconditionally sounds like the sensible thing to do. I'd be inclined to treat failure as non-fatal like we do for the sysfs link, though, since it's another thing that correct SMMU operation doesn't actually depend on (at this point we don't necessarily know if this consumer even has a driver at all). > FYI, as discussed in the first patch [1] of this series, I will add a > system wide > suspend callback - arm_smmu_pm_suspend, that would do clock disable, and will > add corresponding clock enable calls in arm_smmu_pm_resume(). OK, I still don't really understand the finer points of how system PM and runtime PM interact, but if making it robust is just a case of calling the runtime suspend/resume hooks as appropriate from the system ones, that sounds reasonable. Robin. > > [1] https://lore.kernel.org/patchwork/patch/960460/ > > > Best regards > Vivek > >> >>> + !device_link_add(dev, smmu->dev, >>> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { >>> + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", >>> + dev_name(dev)); >>> + ret = -ENODEV; >>> + goto out_unlink; >>> + } >>> + >>> return 0; >>> >>> +out_unlink: >>> + iommu_device_unlink(&smmu->iommu, dev); >>> + arm_smmu_master_free_smes(fwspec); >>> out_cfg_free: >>> kfree(cfg); >>> out_free: >>> >> >> > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 18, 2018 at 6:13 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 18/07/18 10:30, Vivek Gautam wrote: >> >> On Wed, Jul 11, 2018 at 3:23 PM, Rafael J. Wysocki <rjw@rjwysocki.net> >> wrote: >>> >>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>> >>>> From: Sricharan R <sricharan@codeaurora.org> >>>> >>>> Finally add the device link between the master device and >>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>> master needs it. This is done from add_device callback which gets >>>> called once when the master is added to the smmu. >>>> >>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>> Cc: Lukas Wunner <lukas@wunner.de> >>>> --- >>>> >>>> - Change since v11 >>>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>>> >>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index 09265e206e2d..916cde4954d2 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c >>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device >>>> *dev) >>>> >>>> iommu_device_link(&smmu->iommu, dev); >>>> >>>> + if (pm_runtime_enabled(smmu->dev) && >>> >>> >>> Why does the creation of the link depend on whether or not runtime PM >>> is enabled for the MMU device? >>> >>> What about system-wide PM and system shutdown? Are they always >>> guaranteed >>> to happen in the right order without the link? >> >> >> Hi Robin, >> >> As Rafael pointed, we should the device link creation should not depend on >> runtime PM being enabled or not, as we would also want to guarantee >> that system wide PM callbacks are called in the right order for smmu >> and clients. >> >> Does this change of removing the check for pm_runtime_enabled() from here >> looks okay to you? > > > FWIW the existing system PM ops make no claim to be perfect, and I wouldn't > be at all surprised if it was only by coincidence that my devices happened > to put on the relevant lists in the right order to start with. If we no > longer need to worry about explicit device_link housekeeping in the SMMU > driver, then creating them unconditionally sounds like the sensible thing to > do. I'd be inclined to treat failure as non-fatal like we do for the sysfs > link, though, since it's another thing that correct SMMU operation doesn't > actually depend on (at this point we don't necessarily know if this consumer > even has a driver at all). Thanks. I will then respin the patch taking care of treating failure as non-fatal. >> FYI, as discussed in the first patch [1] of this series, I will add a >> system wide >> suspend callback - arm_smmu_pm_suspend, that would do clock disable, and >> will >> add corresponding clock enable calls in arm_smmu_pm_resume(). > > > OK, I still don't really understand the finer points of how system PM and > runtime PM interact, but if making it robust is just a case of calling the > runtime suspend/resume hooks as appropriate from the system ones, that > sounds reasonable. Sure. Thanks. Best regards Vivek > > Robin. > >> >> [1] https://lore.kernel.org/patchwork/patch/960460/ >> >> >> Best regards >> Vivek >> >>> >>>> + !device_link_add(dev, smmu->dev, >>>> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) >>>> { >>>> + dev_err(smmu->dev, "Unable to add link to the consumer >>>> %s\n", >>>> + dev_name(dev)); >>>> + ret = -ENODEV; >>>> + goto out_unlink; >>>> + } >>>> + >>>> return 0; >>>> >>>> +out_unlink: >>>> + iommu_device_unlink(&smmu->iommu, dev); >>>> + arm_smmu_master_free_smes(fwspec); >>>> out_cfg_free: >>>> kfree(cfg); >>>> out_free: >>>> >>> >>> >> >> >> > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html