| Message ID | 20181127101145.7682-1-vivek.gautam@codeaurora.org |
|---|---|
| Headers | show |
| Series | iommu/arm-smmu: Add runtime pm/sleep support | expand |
On 11/27/18 4:11 AM, 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. > We pull all the information about clocks from device tree. > > Also, while we enable the runtime pm add a pm sleep suspend > callback that pushes devices to low power state by turning > the clocks off in a system sleep. > Also add corresponding clock enable path in resume callback. > > Signed-off-by: Sricharan R <sricharan@codeaurora.org> > Signed-off-by: Archit Taneja <architt@codeaurora.org> > [Thor: Rework to get clocks from device tree] > Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com> > [vivek: rework for clock and pm ops] > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > Reviewed-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/iommu/arm-smmu.c | 100 +++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 97 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 5a28ae892504..e47c840fc6a8 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -44,10 +44,12 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_address.h> > +#include <linux/of_clk.h> > #include <linux/of_device.h> > #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> > <snip> Thanks! Tested the device tree clock portions on Intel SOCFPGA Stratix10 DevKit. Tested-by: Thor Thayer <thor.thayer@linux.intel.com>
Quoting Vivek Gautam (2018-11-27 02:11:41) > @@ -1966,6 +1970,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]; Is this clk_bulk_get_all()?
On 28/11/2018 16:24, Stephen Boyd wrote: > Quoting Vivek Gautam (2018-11-27 02:11:41) >> @@ -1966,6 +1970,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]; > > Is this clk_bulk_get_all()? Ooh, did that finally get merged while we weren't looking? Great! Much as I don't want to drag this series out to a v19, it *would* be neat if we no longer need to open-code that bit... Robin.
On Wed, Nov 28, 2018 at 10:07 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 28/11/2018 16:24, Stephen Boyd wrote: > > Quoting Vivek Gautam (2018-11-27 02:11:41) > >> @@ -1966,6 +1970,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]; > > > > Is this clk_bulk_get_all()? >From what I remember, and now I could go back to v7 and check [1], we parked clk_bulk_get out of OF's sole purview as we also have arm_smmu_device_acpi_probe() besides arm_smmu_device_dt_probe(). arm_smmu_device_dt_probe() could get the clocks from dt and fill in the clock bulk data, and similarly, arm_smmu_device_acpi_probe() could fill the clock bulk data by getting it from ACPI. clk_bulk_get_all() seems like going only the OF way. Is there another way here to have something common between ACPI and OF, and then do the clk_bulk_get? [1] https://lore.kernel.org/patchwork/patch/881365/ Thanks & regards Vivek > > Ooh, did that finally get merged while we weren't looking? Great! > > Much as I don't want to drag this series out to a v19, it *would* be > neat if we no longer need to open-code that bit... > > Robin. > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Thu, Nov 29, 2018 at 08:25:20PM +0530, Vivek Gautam wrote: > On Wed, Nov 28, 2018 at 10:07 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > On 28/11/2018 16:24, Stephen Boyd wrote: > > > Quoting Vivek Gautam (2018-11-27 02:11:41) > > >> @@ -1966,6 +1970,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]; > > > > > > Is this clk_bulk_get_all()? > > From what I remember, and now I could go back to v7 and check [1], we parked > clk_bulk_get out of OF's sole purview as we also have > arm_smmu_device_acpi_probe() besides arm_smmu_device_dt_probe(). > > arm_smmu_device_dt_probe() could get the clocks from dt and fill in > the clock bulk data, and > similarly, arm_smmu_device_acpi_probe() could fill the clock bulk data > by getting it from ACPI. > > clk_bulk_get_all() seems like going only the OF way. > Is there another way here to have something common between ACPI > and OF, and then do the clk_bulk_get? I'd say just go with clk_bulk_get_all() and if somebody really wants to mess with the SMMU clocks on a system booted via ACPI, then it's their problem to solve. My understanding is that the design of IORT makes this next to impossible to solve anyway, because a static table is used and therefore we're unable to run whatever ASL methods need to be invoked to mess with the clocks. Will
On Fri, Nov 30, 2018 at 11:45 PM Will Deacon <will.deacon@arm.com> wrote: > > On Thu, Nov 29, 2018 at 08:25:20PM +0530, Vivek Gautam wrote: > > On Wed, Nov 28, 2018 at 10:07 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > > > On 28/11/2018 16:24, Stephen Boyd wrote: > > > > Quoting Vivek Gautam (2018-11-27 02:11:41) > > > >> @@ -1966,6 +1970,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]; > > > > > > > > Is this clk_bulk_get_all()? > > > > From what I remember, and now I could go back to v7 and check [1], we parked > > clk_bulk_get out of OF's sole purview as we also have > > arm_smmu_device_acpi_probe() besides arm_smmu_device_dt_probe(). > > > > arm_smmu_device_dt_probe() could get the clocks from dt and fill in > > the clock bulk data, and > > similarly, arm_smmu_device_acpi_probe() could fill the clock bulk data > > by getting it from ACPI. > > > > clk_bulk_get_all() seems like going only the OF way. > > Is there another way here to have something common between ACPI > > and OF, and then do the clk_bulk_get? > > I'd say just go with clk_bulk_get_all() and if somebody really wants to > mess with the SMMU clocks on a system booted via ACPI, then it's their > problem to solve. My understanding is that the design of IORT makes this > next to impossible to solve anyway, because a static table is used and > therefore we're unable to run whatever ASL methods need to be invoked to > mess with the clocks. Sure then. I will respin this patch-series. > > Will
Quoting Vivek Gautam (2018-12-02 22:43:38) > On Fri, Nov 30, 2018 at 11:45 PM Will Deacon <will.deacon@arm.com> wrote: > > > > On Thu, Nov 29, 2018 at 08:25:20PM +0530, Vivek Gautam wrote: > > > clk_bulk_get_all() seems like going only the OF way. > > > Is there another way here to have something common between ACPI > > > and OF, and then do the clk_bulk_get? > > > > I'd say just go with clk_bulk_get_all() and if somebody really wants to > > mess with the SMMU clocks on a system booted via ACPI, then it's their > > problem to solve. My understanding is that the design of IORT makes this > > next to impossible to solve anyway, because a static table is used and > > therefore we're unable to run whatever ASL methods need to be invoked to > > mess with the clocks. > > Sure then. I will respin this patch-series. > Right. The idea is to add non-OF support to clk_bulk_get_all() if/when we get the requirement. Sounds like we can keep waiting a little longer for that to happen.