From: will.deacon@arm•com (Will Deacon)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 1/6] iommu/arm-smmu: add support for specifying clocks
Date: Wed, 10 Sep 2014 19:27:39 +0100 [thread overview]
Message-ID: <20140910182739.GM1710@arm.com> (raw)
In-Reply-To: <vnkwa968b6ux.fsf@mitchelh-linux.qualcomm.com>
On Wed, Sep 10, 2014 at 02:29:42AM +0100, Mitchel Humpherys wrote:
> On Tue, Aug 26 2014 at 07:27:58 AM, Will Deacon <will.deacon@arm•com> wrote:
> > On Tue, Aug 19, 2014 at 08:03:09PM +0100, Olav Haugan wrote:
> >> Clients of the SMMU driver are required to vote for clocks and power
> >> when they know they need to use the SMMU. However, the clock and power
> >> needed to be on for the SMMU to service bus masters aren't necessarily
> >> the same as the ones needed to read/write registers...See below.
> >
> > The case I'm thinking of is where a device masters through the IOMMU, but
> > doesn't make use of any translations. In this case, its transactions will
> > bypass the SMMU and I want to ensure that continues to happen, regardless of
> > the power state of the SMMU.
>
> Then I assume the driver for such a device wouldn't be attaching to (or
> detaching from) the IOMMU, so we won't be touching it at all either
> way. Or am I missing something?
As long as its only the register file that gets powered down, then there's
no issue. However, that's making assumptions about what these clocks are
controlling. Is there a way for the driver to know which aspects of the
device are controlled by which clock?
> >> > What stops theses from racing with each other when there are multiple
> >> > clocks? I also assume that the clk API ignores calls to clk_enable_prepare
> >> > for a clk that's already enabled? I couldn't find that code...
> >>
> >> All the clock APIs are reference counted yes. Not sure what you mean by
> >> racing with each other? When you call to enable a clock the call does
> >> not return until the clock is already ON (or OFF).
> >
> > I was thinking of an interrupt handler racing with normal code, but actually
> > you balance the clk enable/disable in the interrupt handlers. However, it's
> > not safe to call these clk functions from irq context anyway, since
> > clk_prepare may sleep.
>
> Ah yes. You okay with moving to a threaded IRQ?
A threaded IRQ already makes sense for context interrupts (if anybody has a
platform that can do stalls properly), but it seems a bit weird for the
global fault handler. Is there no way to fix the race instead?
> >> >> @@ -2061,12 +2164,16 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> >> >> spin_unlock(&arm_smmu_devices_lock);
> >> >>
> >> >> arm_smmu_device_reset(smmu);
> >> >> + arm_smmu_disable_clocks(smmu);
> >> >
> >> > I wonder if this is really the right thing to do. Rather than the
> >> > fine-grained clock enable/disable you have, why don't we just enable in
> >> > domain_init and disable in domain_destroy, with refcounting for the clocks?
> >> >
> >>
> >> So the whole point of all of this is that we try to save power. As Mitch
> >> wrote in the commit text we want to only leave the clock and power on
> >> for as short period of time as possible.
> >
> > Understood, but if the clocks are going up and down like yo-yos, then it's
> > not obvious that you end up saving any power at all. Have you tried
> > measuring the power consumption with different granularities for the
> > clocks?
>
> This has been profiled extensively and for some use cases it's a huge
> win. Unfortunately we don't have any numbers for public sharing :( but
> you can imagine a use case where some multimedia framework maps a bunch
> of buffers into an SMMU at the beginning of some interactive user
> session and doesn't unmap them until the (human) user decides they are
> done. This could be a long time, all the while these clocks could be
> off, saving power.
Ok, I can see that. I wonder whether we could wrap all of the iommu_ops
functions with the clock enable/disable code, instead of putting it directly
into the drivers?
> > The code you're proposing seems to take the approach of `we're going to
> > access registers so enable the clocks, access the registers then disable the
> > clocks', which is simple but may not be particularly effective.
>
> Yes, that's a good summary of the approach here. It has been effective
> in saving power for us in the past...
Mike, do you have any experience with this sort of stuff?
Will
next prev parent reply other threads:[~2014-09-10 18:27 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-13 0:51 [PATCH 0/6] iommu/arm-smmu: misc features, new DT bindings Mitchel Humpherys
2014-08-13 0:51 ` [PATCH 1/6] iommu/arm-smmu: add support for specifying clocks Mitchel Humpherys
2014-08-13 21:07 ` Mitchel Humpherys
2014-08-19 12:58 ` Will Deacon
2014-08-19 19:03 ` Olav Haugan
2014-08-26 14:27 ` Will Deacon
2014-09-10 1:29 ` Mitchel Humpherys
2014-09-10 18:27 ` Will Deacon [this message]
2014-09-10 19:09 ` Mitchel Humpherys
2014-09-15 18:38 ` Mitchel Humpherys
2014-08-19 19:28 ` Mitchel Humpherys
2014-08-13 0:51 ` [PATCH 2/6] iommu/arm-smmu: add support for specifying regulators Mitchel Humpherys
2014-08-13 21:17 ` Mitchel Humpherys
2014-08-19 13:00 ` Will Deacon
2014-08-13 0:51 ` [PATCH 3/6] iommu/arm-smmu: add support for iova_to_phys through ATS1PR Mitchel Humpherys
2014-08-19 12:44 ` Will Deacon
2014-08-19 18:12 ` Mitchel Humpherys
2014-08-26 13:54 ` Will Deacon
2014-09-01 16:15 ` Will Deacon
2014-08-13 0:51 ` [PATCH 4/6] iommu/arm-smmu: implement generic DT bindings Mitchel Humpherys
2014-08-13 16:53 ` Mitchel Humpherys
2014-08-19 12:54 ` Will Deacon
2014-08-19 15:54 ` Hiroshi Doyu
2014-08-20 3:18 ` Arnd Bergmann
2014-08-13 0:51 ` [PATCH 5/6] iommu/arm-smmu: support buggy implementations with invalidate-on-map Mitchel Humpherys
2014-11-12 18:26 ` Will Deacon
2014-11-12 18:58 ` Mitchel Humpherys
2014-11-13 9:48 ` Will Deacon
2014-11-14 23:08 ` Mitchel Humpherys
2014-08-13 0:51 ` [PATCH 6/6] iommu/arm-smmu: add .domain_{set, get}_attr for coherent walk control Mitchel Humpherys
2014-08-19 12:48 ` [PATCH 6/6] iommu/arm-smmu: add .domain_{set,get}_attr " Will Deacon
2014-08-19 19:19 ` [PATCH 6/6] iommu/arm-smmu: add .domain_{set, get}_attr " Mitchel Humpherys
2014-08-13 17:22 ` [PATCH 0/6] iommu/arm-smmu: misc features, new DT bindings Mitchel Humpherys
2014-08-15 17:25 ` Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140910182739.GM1710@arm.com \
--to=will.deacon@arm$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox