| Message ID | 1516362223-22946-1-git-send-email-vivek.gautam@codeaurora.org |
|---|---|
| Headers | show |
| Series | iommu/arm-smmu: Add runtime pm/sleep support | expand |
On 19/01/18 11:43, 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> > --- > drivers/iommu/arm-smmu.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 53 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 78d4c6b8f1ba..21acffe91a1c 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,9 @@ struct arm_smmu_device { > u32 num_global_irqs; > u32 num_context_irqs; > unsigned int *irqs; > + struct clk_bulk_data *clocks; > + int num_clks; > + const char * const *clk_names; This seems unnecessary, as we use it a grand total of of once, during initialisation when we have the source data directly to hand. Just pass data->clks into arm_smmu_init_clks() as an additional argument. Otherwise, I think this looks reasonable; it's about as unobtrusive as it's going to get. Robin. > u32 cavium_id_base; /* Specific to Cavium */ > > @@ -1685,6 +1689,25 @@ static int arm_smmu_id_size_to_bits(int size) > } > } > > +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu) > +{ > + int i; > + int num = smmu->num_clks; > + > + if (num < 1) > + return 0; > + > + smmu->clocks = devm_kcalloc(smmu->dev, num, > + sizeof(*smmu->clocks), GFP_KERNEL); > + if (!smmu->clocks) > + return -ENOMEM; > + > + for (i = 0; i < num; i++) > + smmu->clocks[i].id = smmu->clk_names[i]; > + > + return devm_clk_bulk_get(smmu->dev, num, smmu->clocks); > +} > + > static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) > { > unsigned long size; > @@ -1897,10 +1920,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); > @@ -2001,6 +2026,8 @@ 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->clk_names = data->clks; > + smmu->num_clks = data->num_clks; > > parse_driver_options(smmu); > > @@ -2099,6 +2126,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > smmu->irqs[i] = irq; > } > > + err = arm_smmu_init_clocks(smmu); > + if (err) > + return err; > + > err = arm_smmu_device_cfg_probe(smmu); > if (err) > return err; > @@ -2197,7 +2228,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_prepare_enable(smmu->num_clks, smmu->clocks); > +} > + > +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev) > +{ > + struct arm_smmu_device *smmu = dev_get_drvdata(dev); > + > + clk_bulk_disable_unprepare(smmu->num_clks, smmu->clocks); > + > + return 0; > +} > + > +static const struct dev_pm_ops arm_smmu_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) > + SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend, > + arm_smmu_runtime_resume, NULL) > +}; > > static struct platform_driver arm_smmu_driver = { > .driver = { > -- 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 19/01/18 11:43, 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> > --- > drivers/iommu/arm-smmu.c | 45 +++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 41 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 21acffe91a1c..95478bfb182c 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -914,11 +914,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 = pm_runtime_get_sync(smmu->dev); > + if (ret) > + return; > + > /* > * Disable the context bank and free the page tables before freeing > * it. > @@ -933,6 +937,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); > + > + pm_runtime_put_sync(smmu->dev); > } > > static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) > @@ -1408,12 +1414,20 @@ static int arm_smmu_add_device(struct device *dev) > while (i--) > cfg->smendx[i] = INVALID_SMENDX; > > - ret = arm_smmu_master_alloc_smes(dev); > + ret = pm_runtime_get_sync(smmu->dev); > if (ret) > goto out_cfg_free; > > + ret = arm_smmu_master_alloc_smes(dev); > + if (ret) { > + pm_runtime_put_sync(smmu->dev); > + goto out_cfg_free; Please keep to the existing pattern and put this on the cleanup path with a new label, rather than inline. > + } > + > iommu_device_link(&smmu->iommu, dev); > > + pm_runtime_put_sync(smmu->dev); > + > return 0; > > out_cfg_free: > @@ -1428,7 +1442,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; > @@ -1436,8 +1450,21 @@ static void arm_smmu_remove_device(struct device *dev) > cfg = fwspec->iommu_priv; > smmu = cfg->smmu; > > + /* > + * The device link between the master device and > + * smmu is already purged at this point. > + * So enable the power to smmu explicitly. > + */ I don't understand this comment, especially since we don't even introduce device links until the following patch... :/ > + > + ret = pm_runtime_get_sync(smmu->dev); > + if (ret) > + return; > + > iommu_device_unlink(&smmu->iommu, dev); > arm_smmu_master_free_smes(fwspec); > + > + pm_runtime_put_sync(smmu->dev); > + > iommu_group_remove_device(dev); > kfree(fwspec->iommu_priv); > iommu_fwspec_free(dev); > @@ -2130,6 +2157,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev) > if (err) > return err; > > + platform_set_drvdata(pdev, smmu); > + > + pm_runtime_enable(dev); > + > + err = pm_runtime_get_sync(dev); > + if (err) > + return err; > + > err = arm_smmu_device_cfg_probe(smmu); > if (err) > return err; > @@ -2171,9 +2206,9 @@ 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); > + pm_runtime_put_sync(dev); > > /* > * For ACPI and generic DT bindings, an SMMU will be probed before > @@ -2212,6 +2247,8 @@ 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); > + pm_runtime_force_suspend(smmu->dev); Why do we need this? I guess it might be a Qualcomm-ism as I don't see anyone else calling it from .remove other than a couple of other qcom_* drivers. Given that we only get here during system shutdown (or the root user intentionally pissing about with driver unbinding), it doesn't seem like a point where power saving really matters all that much. I'd also naively expect that anything this device was the last consumer off would get turned off by core code anyway once it's removed, but maybe things aren't that slick; I dunno :/ Robin. > + > return 0; > } > > -- 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 19/01/18 11:43, 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. Don't we need to balance this with a device_link_del() in .remove_device (like exynos-iommu does)? Robin. > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > --- > drivers/iommu/arm-smmu.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 95478bfb182c..33bbcfedb896 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev) > struct arm_smmu_device *smmu; > struct arm_smmu_master_cfg *cfg; > struct iommu_fwspec *fwspec = dev->iommu_fwspec; > + struct device_link *link; > int i, ret; > > if (using_legacy_binding) { > @@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev) > > pm_runtime_put_sync(smmu->dev); > > + /* > + * Establish the link between smmu and master, so that the > + * smmu gets runtime enabled/disabled as per the master's > + * needs. > + */ > + link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME); > + if (!link) > + dev_warn(smmu->dev, "Unable to create device link between %s and %s\n", > + dev_name(smmu->dev), dev_name(dev)); > + > return 0; > > out_cfg_free: > -- 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 1/31/2018 5:53 PM, Robin Murphy wrote: > On 19/01/18 11:43, 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> >> --- >> drivers/iommu/arm-smmu.c | 55 >> ++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 53 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 78d4c6b8f1ba..21acffe91a1c 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,9 @@ struct arm_smmu_device { >> u32 num_global_irqs; >> u32 num_context_irqs; >> unsigned int *irqs; >> + struct clk_bulk_data *clocks; >> + int num_clks; >> + const char * const *clk_names; > > This seems unnecessary, as we use it a grand total of of once, during > initialisation when we have the source data directly to hand. Just > pass data->clks into arm_smmu_init_clks() as an additional argument. Sure, will do that. > > Otherwise, I think this looks reasonable; it's about as unobtrusive as > it's going to get. Thanks for reviewing. regards Vivek > > Robin. > >> u32 cavium_id_base; /* Specific to Cavium */ >> @@ -1685,6 +1689,25 @@ static int arm_smmu_id_size_to_bits(int size) >> } >> } >> +static int arm_smmu_init_clocks(struct arm_smmu_device *smmu) >> +{ >> + int i; >> + int num = smmu->num_clks; >> + >> + if (num < 1) >> + return 0; >> + >> + smmu->clocks = devm_kcalloc(smmu->dev, num, >> + sizeof(*smmu->clocks), GFP_KERNEL); >> + if (!smmu->clocks) >> + return -ENOMEM; >> + >> + for (i = 0; i < num; i++) >> + smmu->clocks[i].id = smmu->clk_names[i]; >> + >> + return devm_clk_bulk_get(smmu->dev, num, smmu->clocks); >> +} >> + >> static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu) >> { >> unsigned long size; >> @@ -1897,10 +1920,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); >> @@ -2001,6 +2026,8 @@ 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->clk_names = data->clks; >> + smmu->num_clks = data->num_clks; >> parse_driver_options(smmu); >> @@ -2099,6 +2126,10 @@ static int arm_smmu_device_probe(struct >> platform_device *pdev) >> smmu->irqs[i] = irq; >> } >> + err = arm_smmu_init_clocks(smmu); >> + if (err) >> + return err; >> + >> err = arm_smmu_device_cfg_probe(smmu); >> if (err) >> return err; >> @@ -2197,7 +2228,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_prepare_enable(smmu->num_clks, smmu->clocks); >> +} >> + >> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev) >> +{ >> + struct arm_smmu_device *smmu = dev_get_drvdata(dev); >> + >> + clk_bulk_disable_unprepare(smmu->num_clks, smmu->clocks); >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops arm_smmu_pm_ops = { >> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume) >> + SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend, >> + arm_smmu_runtime_resume, NULL) >> +}; >> static struct platform_driver arm_smmu_driver = { >> .driver = { >> -- 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 1/31/2018 6:39 PM, Robin Murphy wrote: > On 19/01/18 11:43, 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. > > Don't we need to balance this with a device_link_del() in > .remove_device (like exynos-iommu does)? Right. Will add device_link_del() call. Thanks for pointing out. regards Vivek > > Robin. > >> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >> --- >> drivers/iommu/arm-smmu.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 95478bfb182c..33bbcfedb896 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev) >> struct arm_smmu_device *smmu; >> struct arm_smmu_master_cfg *cfg; >> struct iommu_fwspec *fwspec = dev->iommu_fwspec; >> + struct device_link *link; >> int i, ret; >> if (using_legacy_binding) { >> @@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device >> *dev) >> pm_runtime_put_sync(smmu->dev); >> + /* >> + * Establish the link between smmu and master, so that the >> + * smmu gets runtime enabled/disabled as per the master's >> + * needs. >> + */ >> + link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME); >> + if (!link) >> + dev_warn(smmu->dev, "Unable to create device link between %s >> and %s\n", >> + dev_name(smmu->dev), dev_name(dev)); >> + >> return 0; >> out_cfg_free: >> -- 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 Robin, On 1/31/2018 6:36 PM, Robin Murphy wrote: > On 19/01/18 11:43, 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> >> --- >> drivers/iommu/arm-smmu.c | 45 +++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 41 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 21acffe91a1c..95478bfb182c 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -914,11 +914,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 = pm_runtime_get_sync(smmu->dev); >> + if (ret) >> + return; >> + >> /* >> * Disable the context bank and free the page tables before freeing >> * it. >> @@ -933,6 +937,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); >> + >> + pm_runtime_put_sync(smmu->dev); >> } >> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) >> @@ -1408,12 +1414,20 @@ static int arm_smmu_add_device(struct device *dev) >> while (i--) >> cfg->smendx[i] = INVALID_SMENDX; >> - ret = arm_smmu_master_alloc_smes(dev); >> + ret = pm_runtime_get_sync(smmu->dev); >> if (ret) >> goto out_cfg_free; >> + ret = arm_smmu_master_alloc_smes(dev); >> + if (ret) { >> + pm_runtime_put_sync(smmu->dev); >> + goto out_cfg_free; > > Please keep to the existing pattern and put this on the cleanup path with a new label, rather than inline. ok. > >> + } >> + >> iommu_device_link(&smmu->iommu, dev); >> + pm_runtime_put_sync(smmu->dev); >> + >> return 0; >> out_cfg_free: >> @@ -1428,7 +1442,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; >> @@ -1436,8 +1450,21 @@ static void arm_smmu_remove_device(struct device *dev) >> cfg = fwspec->iommu_priv; >> smmu = cfg->smmu; >> + /* >> + * The device link between the master device and >> + * smmu is already purged at this point. >> + * So enable the power to smmu explicitly. >> + */ > > I don't understand this comment, especially since we don't even introduce device links until the following patch... :/ > This is because the core device_del callback, does a device_links_purge for that device, before calling the remove_device notifier. As a result, have to explicitly turn on the power to iommu. Probably the comment should be removed, rest of the places we don't explain why we are turning on explicitly. >> + >> + ret = pm_runtime_get_sync(smmu->dev); >> + if (ret) >> + return; >> + >> iommu_device_unlink(&smmu->iommu, dev); >> arm_smmu_master_free_smes(fwspec); >> + >> + pm_runtime_put_sync(smmu->dev); >> + >> iommu_group_remove_device(dev); >> kfree(fwspec->iommu_priv); >> iommu_fwspec_free(dev); >> @@ -2130,6 +2157,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >> if (err) >> return err; >> + platform_set_drvdata(pdev, smmu); >> + >> + pm_runtime_enable(dev); >> + >> + err = pm_runtime_get_sync(dev); >> + if (err) >> + return err; >> + >> err = arm_smmu_device_cfg_probe(smmu); >> if (err) >> return err; >> @@ -2171,9 +2206,9 @@ 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); >> + pm_runtime_put_sync(dev); >> /* >> * For ACPI and generic DT bindings, an SMMU will be probed before >> @@ -2212,6 +2247,8 @@ 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); >> + pm_runtime_force_suspend(smmu->dev); > > Why do we need this? I guess it might be a Qualcomm-ism as I don't see anyone else calling it from .remove other than a couple of other qcom_* drivers. Given that we only get here during system shutdown (or the root user intentionally pissing about with driver unbinding), it doesn't seem like a point where power saving really matters all that much. > > I'd also naively expect that anything this device was the last consumer off would get turned off by core code anyway once it's removed, but maybe things aren't that slick; I dunno :/ hmm, that should not be needed. with turning of all consumers taken care by device_link code before the supplier (iommu) remove gets called should ensure that. So the above force_suspend should not be needed/can be removed. But one more thing is, we do touch the register in the above code. So that should require a additional get/put sync around that writel. Regards, Sricharan
On 2/1/2018 5:03 PM, Sricharan R wrote: > Hi Robin, > > On 1/31/2018 6:36 PM, Robin Murphy wrote: >> On 19/01/18 11:43, 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> >>> --- >>> drivers/iommu/arm-smmu.c | 45 +++++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 41 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 21acffe91a1c..95478bfb182c 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -914,11 +914,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 = pm_runtime_get_sync(smmu->dev); >>> + if (ret) >>> + return; >>> + >>> /* >>> * Disable the context bank and free the page tables before freeing >>> * it. >>> @@ -933,6 +937,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); >>> + >>> + pm_runtime_put_sync(smmu->dev); >>> } >>> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) >>> @@ -1408,12 +1414,20 @@ static int arm_smmu_add_device(struct device *dev) >>> while (i--) >>> cfg->smendx[i] = INVALID_SMENDX; >>> - ret = arm_smmu_master_alloc_smes(dev); >>> + ret = pm_runtime_get_sync(smmu->dev); >>> if (ret) >>> goto out_cfg_free; >>> + ret = arm_smmu_master_alloc_smes(dev); >>> + if (ret) { >>> + pm_runtime_put_sync(smmu->dev); >>> + goto out_cfg_free; >> Please keep to the existing pattern and put this on the cleanup path with a new label, rather than inline. > ok. > >>> + } >>> + >>> iommu_device_link(&smmu->iommu, dev); >>> + pm_runtime_put_sync(smmu->dev); >>> + >>> return 0; >>> out_cfg_free: >>> @@ -1428,7 +1442,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; >>> @@ -1436,8 +1450,21 @@ static void arm_smmu_remove_device(struct device *dev) >>> cfg = fwspec->iommu_priv; >>> smmu = cfg->smmu; >>> + /* >>> + * The device link between the master device and >>> + * smmu is already purged at this point. >>> + * So enable the power to smmu explicitly. >>> + */ >> I don't understand this comment, especially since we don't even introduce device links until the following patch... :/ >> > This is because the core device_del callback, does a device_links_purge for that device, > before calling the remove_device notifier. As a result, have to explicitly turn on the > power to iommu. Probably the comment should be removed, rest of the places we don't > explain why we are turning on explicitly. Yes, will remove the comment here. > >>> + >>> + ret = pm_runtime_get_sync(smmu->dev); >>> + if (ret) >>> + return; >>> + >>> iommu_device_unlink(&smmu->iommu, dev); >>> arm_smmu_master_free_smes(fwspec); >>> + >>> + pm_runtime_put_sync(smmu->dev); >>> + >>> iommu_group_remove_device(dev); >>> kfree(fwspec->iommu_priv); >>> iommu_fwspec_free(dev); >>> @@ -2130,6 +2157,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev) >>> if (err) >>> return err; >>> + platform_set_drvdata(pdev, smmu); >>> + >>> + pm_runtime_enable(dev); >>> + >>> + err = pm_runtime_get_sync(dev); >>> + if (err) >>> + return err; >>> + >>> err = arm_smmu_device_cfg_probe(smmu); >>> if (err) >>> return err; >>> @@ -2171,9 +2206,9 @@ 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); >>> + pm_runtime_put_sync(dev); >>> /* >>> * For ACPI and generic DT bindings, an SMMU will be probed before >>> @@ -2212,6 +2247,8 @@ 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); >>> + pm_runtime_force_suspend(smmu->dev); >> Why do we need this? I guess it might be a Qualcomm-ism as I don't see anyone else calling it from .remove other than a couple of other qcom_* drivers. Given that we only get here during system shutdown (or the root user intentionally pissing about with driver unbinding), it doesn't seem like a point where power saving really matters all that much. >> >> I'd also naively expect that anything this device was the last consumer off would get turned off by core code anyway once it's removed, but maybe things aren't that slick; I dunno :/ > hmm, that should not be needed. with turning of all consumers taken care by device_link code before > the supplier (iommu) remove gets called should ensure that. So the above force_suspend should > not be needed/can be removed. But one more thing is, we do touch the register in the above code. > So that should require a additional get/put sync around that writel. Possibly we can replace the force_suspend() with a pm_runtime_disable() to complement pm_runtime_enable in the probe. I will test the scenario where we are writing the SMMU register in .remove path. regards Vivek > > Regards, > Sricharan > -- 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 Robin/Vivek, On 2/1/2018 2:23 PM, Vivek Gautam wrote: > Hi, > > > On 1/31/2018 6:39 PM, Robin Murphy wrote: >> On 19/01/18 11:43, 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. >> >> Don't we need to balance this with a device_link_del() in .remove_device (like exynos-iommu does)? > > Right. Will add device_link_del() call. Thanks for pointing out. The reason for not adding device_link_del from .remove_device was, the core device_del which calls the .remove_device from notifier, calls device_links_purge before that. That does the same thing as device_link_del. So by the time .remove_device is called, device_links for that device is already cleaned up. Vivek, you may want to check once that calling device_link_del from .remove_device has no effect, just to confirm once more. Regards, Sricharan > > regards > Vivek > >> >> Robin. >> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>> --- >>> drivers/iommu/arm-smmu.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 95478bfb182c..33bbcfedb896 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -1367,6 +1367,7 @@ static int arm_smmu_add_device(struct device *dev) >>> struct arm_smmu_device *smmu; >>> struct arm_smmu_master_cfg *cfg; >>> struct iommu_fwspec *fwspec = dev->iommu_fwspec; >>> + struct device_link *link; >>> int i, ret; >>> if (using_legacy_binding) { >>> @@ -1428,6 +1429,16 @@ static int arm_smmu_add_device(struct device *dev) >>> pm_runtime_put_sync(smmu->dev); >>> + /* >>> + * Establish the link between smmu and master, so that the >>> + * smmu gets runtime enabled/disabled as per the master's >>> + * needs. >>> + */ >>> + link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME); >>> + if (!link) >>> + dev_warn(smmu->dev, "Unable to create device link between %s and %s\n", >>> + dev_name(smmu->dev), dev_name(dev)); >>> + >>> return 0; >>> out_cfg_free: >>> >
On 02/02/18 05:40, Sricharan R wrote: > Hi Robin/Vivek, > > On 2/1/2018 2:23 PM, Vivek Gautam wrote: >> Hi, >> >> >> On 1/31/2018 6:39 PM, Robin Murphy wrote: >>> On 19/01/18 11:43, 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. >>> >>> Don't we need to balance this with a device_link_del() in .remove_device (like exynos-iommu does)? >> >> Right. Will add device_link_del() call. Thanks for pointing out. > > The reason for not adding device_link_del from .remove_device was, the core device_del > which calls the .remove_device from notifier, calls device_links_purge before that. > That does the same thing as device_link_del. So by the time .remove_device is called, > device_links for that device is already cleaned up. Vivek, you may want to check once that > calling device_link_del from .remove_device has no effect, just to confirm once more. There is at least one path in which .remove_device is not called via the notifier from device_del(), which is in the cleanup path of iommu_bus_init(). AFAICS any links created by .add_device during that process would be left dangling, because the device(s) would be live but otherwise disassociated from the IOMMU afterwards. From a maintenance perspective it's easier to have the call in its logical place even if it does nothing 99% of the time; that way we shouldn't have to keep an eye out for subtle changes in the power management code or driver core that might invalidate the device_del() reasoning above, and the power management guys shouldn't have to comprehend the internals of the IOMMU API to make sense of the unbalanced call if they ever want to change their API. Thanks, Robin. -- 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 Robin, On 2/2/2018 5:01 PM, Robin Murphy wrote: > On 02/02/18 05:40, Sricharan R wrote: >> Hi Robin/Vivek, >> >> On 2/1/2018 2:23 PM, Vivek Gautam wrote: >>> Hi, >>> >>> >>> On 1/31/2018 6:39 PM, Robin Murphy wrote: >>>> On 19/01/18 11:43, 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. >>>> >>>> Don't we need to balance this with a device_link_del() in .remove_device (like exynos-iommu does)? >>> >>> Right. Will add device_link_del() call. Thanks for pointing out. >> >> The reason for not adding device_link_del from .remove_device was, the core device_del >> which calls the .remove_device from notifier, calls device_links_purge before that. >> That does the same thing as device_link_del. So by the time .remove_device is called, >> device_links for that device is already cleaned up. Vivek, you may want to check once that >> calling device_link_del from .remove_device has no effect, just to confirm once more. > > There is at least one path in which .remove_device is not called via the notifier from device_del(), which is in the cleanup path of iommu_bus_init(). AFAICS any links created by .add_device during that process would be left dangling, because the device(s) would be live but otherwise disassociated from the IOMMU afterwards. > > From a maintenance perspective it's easier to have the call in its logical place even if it does nothing 99% of the time; that way we shouldn't have to keep an eye out for subtle changes in the power management code or driver core that might invalidate the device_del() reasoning above, and the power management guys shouldn't have to comprehend the internals of the IOMMU API to make sense of the unbalanced call if they ever want to change their API. Ha, for a moment was thinking that with probe deferral add/remove_iommu_group in iommu_bus_init is dummy. But that may not be true for all Archs. Surely agree for the maintainability reason as well. Thanks. Regards, Sricharan
This series provides the support for turning on the arm-smmu's clocks/power domains using runtime pm. This is done using the recently introduced device links patches, which lets the smmu's runtime to follow the master's runtime pm, so the smmu remains powered only when the masters use it. It also adds support for Qcom's arm-smmu-v2 variant that has different clocks and power requirements. Took some reference from the exynos runtime patches [1]. After much discussion [3] over the use of pm_runtime_get/put() in .unmap op path for the arm-smmu, and after disussing over more than a couple of approaches to address this, we are putting forward the changes *without* using pm_runtime APIs in 'unmap'. Rather, letting the client device take the control of powering on/off the connected iommu through pm_runtime_get(put)_suppliers() APIs for the scnerios when the iommu power can't be directly controlled by clients through device links. Rafael has agreed to export the suppliers APIs [4]. Hi Robin, Will, please consider reviewing this series. [V6] * Added Ack given by Rafael to first patch in the series. * Addressed Rob Herring's comment for adding soc specific compatible string as well besides 'qcom,smmu-v2'. [V5] * Dropped runtime pm calls from "arm_smmu_unmap" op as discussed over the list [3] for the last patch series. * Added a patch to export pm_runtime_get/put_suppliers() APIs to the series as agreed with Rafael [4]. * Added the related patch for msm drm iommu layer to use pm_runtime_get/put_suppliers() APIs in msm_mmu_funcs. * Dropped arm-mmu500 clock patch since that would break existing platforms. * Changed compatible 'qcom,msm8996-smmu-v2' to 'qcom,smmu-v2' to reflect the IP version rather than the platform on which it is used. The same IP is used across multiple platforms including msm8996, and sdm845 etc. * Using clock bulk APIs to handle the clocks available to the IP as suggested by Stephen Boyd. * The first patch in v4 version of the patch-series: ("iommu/arm-smmu: Fix the error path in arm_smmu_add_device") has already made it to mainline. [V4] * Reworked the clock handling part. We now take clock names as data in the driver for supported compatible versions, and loop over them to get, enable, and disable the clocks. * Using qcom,msm8996 based compatibles for bindings instead of a generic qcom compatible. * Refactor MMU500 patch to just add the necessary clock names data and corresponding bindings. * Added the pm_runtime_get/put() calls in .unmap iommu op (fix added by Stanimir on top of previous patch version. * Added a patch to fix error path in arm_smmu_add_device() * Removed patch 3/5 of V3 patch series that added qcom,smmu-v2 bindings. [V3] * Reworked the patches to keep the clocks init/enabling function separately for each compatible. * Added clocks bindings for MMU40x/500. * Added a new compatible for qcom,smmu-v2 implementation and the clock bindings for the same. * Rebased on top of 4.11-rc1 [V2] * Split the patches little differently. * Addressed comments. * Removed the patch #4 [2] from previous post for arm-smmu context save restore. Planning to post this separately after reworking/addressing Robin's feedback. * Reversed the sequence to disable clocks than enabling. This was required for those cases where the clocks are populated in a dependent order from DT. [1] https://lkml.org/lkml/2016/10/20/70 [2] https://patchwork.kernel.org/patch/9389717/ [3] https://patchwork.kernel.org/patch/9827825/ [4] https://patchwork.kernel.org/patch/10102445/ Sricharan R (3): iommu/arm-smmu: Add pm_runtime/sleep ops iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device iommu/arm-smmu: Add the device_link between masters and smmu Vivek Gautam (3): base: power: runtime: Export pm_runtime_get/put_suppliers iommu/arm-smmu: Add support for qcom,smmu-v2 variant drm/msm: iommu: Replace runtime calls with runtime suppliers .../devicetree/bindings/iommu/arm,smmu.txt | 43 +++++++ drivers/base/power/runtime.c | 2 + drivers/gpu/drm/msm/msm_iommu.c | 16 +-- drivers/iommu/arm-smmu.c | 124 ++++++++++++++++++++- 4 files changed, 171 insertions(+), 14 deletions(-)