Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Dmitry Osipenko <digetx@gmail.com>,
	Sowjanya Komatineni <skomatineni@nvidia.com>,
	jason@lakedaemon.net, jonathanh@nvidia.com,
	linus.walleij@linaro.org, marc.zyngier@arm.com,
	mark.rutland@arm.com, stefan@agner.ch, tglx@linutronix.de,
	pdeschrijver@nvidia.com, pgaikwad@nvidia.com,
	linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org,
	jckuo@nvidia.com, josephl@nvidia.com, talho@nvidia.com,
	linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
	mperttunen@nvidia.com, spatra@nvidia.com, robh+dt@kernel.org,
	devicetree@vger.kernel.org, rjw@rjwysocki.net,
	viresh.kumar@linaro.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v9 07/22] clk: Add API to get index of the clock parent
Date: Fri, 08 Nov 2019 10:12:49 -0800
Message-ID: <20191108181249.E284E214DB@mail.kernel.org> (raw)
In-Reply-To: <20191108101116.GA2583136@ulmo>

Quoting Thierry Reding (2019-11-08 02:11:16)
> On Thu, Nov 07, 2019 at 11:19:32AM -0800, Stephen Boyd wrote:
> > Quoting Thierry Reding (2019-11-07 07:21:15)
> > > On Thu, Nov 07, 2019 at 03:54:03AM +0300, Dmitry Osipenko wrote:
> > > > 07.11.2019 02:10, Stephen Boyd пишет:
> > > > > Quoting Sowjanya Komatineni (2019-08-16 12:41:52)
> > > > >> This patch adds an API clk_hw_get_parent_index to get index of the
> > > > >> clock parent to use during the clock restore operations on system
> > > > >> resume.
> > > > >  
> > > > > Is there a reason we can't save the clk hw index at suspend time by
> > > > > reading the hardware to understand the current parent? The parent index
> > > > > typically doesn't matter unless we're trying to communicate something
> > > > > from the framework to the provider driver. Put another way, I would
> > > > > think the provider driver can figure out the index itself without having
> > > > > to go through the framework to do so.
> > > > 
> > > > Isn't it a bit wasteful to duplicate information about the parent within
> > > > a provider if framework already has that info? The whole point of this
> > > > new API is to allow providers to avoid that unnecessary duplication.
> > > > 
> > > > Please note that clk_hw_get_parent_index is getting used only at the
> > > > resume time and not at suspend.
> > > 
> > > I agree with this. All of the information that we need is already cached
> > > in the framework. Doing this in the driver would mean essentially adding
> > > a "saved parent" field along with code to read the value at suspend time
> > > to the three types of clocks that currently use this core helper.
> > 
> > Don't we already have a "saved parent" field by storing the pointer to
> > the clk_hw?
> > 
> > > 
> > > That's certainly something that we *can* do, but it doesn't sound like a
> > > better option than simply querying the framework for the value that we
> > > need.
> > > 
> > 
> > Let me say this another way. Why does this driver want to know the index
> > that the framework uses for some clk_hw pointer? Perhaps it happens to
> > align with the same value that hardware uses, but I still don't
> > understand why the driver wants to know what the framework has decided
> > is the index for some clk_hw pointer.
> > 
> > Or is this something like "give me the index for the parent that the
> > framework thinks I currently have but in reality don't have anymore
> > because the register contents were wiped and we need to reparent it"?
> 
> Yeah, that's exactly what this is being used for. It's used to restore
> the parent/child relationship during resume after the registers have
> been wiped during supend.

Ok cool. Our whole suspend/resume and save/restore story hasn't really
been well thought out so we may want to pull all this logic into the
core one day. For now it's OK to do the heavy lifting from provider
drivers until someone gets a better grasp on how this should all work.

> 
> > A generic API to get any index for this question is overkill and we should
> > consider adding some sort of API like clk_hw_get_current_parent_index(),
> > or a framework flag that tells the framework this parent is incorrect
> > and we need to call the .set_parent() op again to reconfigure it.
> 
> Okay, I think I see what you're saying. The current implementation does
> carry a bit of a risk because users could be calling this function with
> any arbitrary pair of struct clk_hw *, even completely unrelated ones.
> 
> How about we turn it into this instead:
> 
>         /**
>          * clk_hw_get_parent_index - return the index of the parent clock
>          * @hw: clk_hw associated with the clk being consumed
>          *
>          * Fetches and returns the index of parent clock. Returns -EINVAL if the given
>          * clock does not have a current parent.
>          */
>         int clk_hw_get_parent_index(struct clk_hw *hw)
>         {
>                 struct clk_hw *parent = clk_hw_get_parent(hw);
> 
>                 if (!parent)
>                         return -EINVAL;
> 
>                 return clk_fetch_parent_index(hw->core, parent->core);
>         }
>         EXPORT_SYMBOL_GPL(clk_hw_get_parent_index);
> 
> I think that has the advantage that we can't pass it a parent that's not
> really a parent. There's still the slightly weird case where the clock
> doesn't have a current parent, but hopefully that's something we are not
> going to encounter much. After all this only makes sense to be called on
> mux clocks and they always do have a parent by definition.

Right.

> 
> Perhaps we should be more explicit and wrap that !parent conditional in
> a WARN_ON()? In my local patches I do that at the call sites because
> they are all functions returning void, so we'd be silently ignoring the
> cases, but I think it may make sense to have it in the core.
> 

Sure a WARN_ON() sounds fair. That will not take the whole task down
and makes sure that drivers aren't doing something incorrect. Otherwise,
this looks good and we can optimize by caching the parent index later if
we really need to.


  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-16 19:41 [PATCH v9 00/22] SC7 entry and exit support for Tegra210 Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 01/22] pinctrl: tegra: Fix write barrier placement in pmx_writel Sowjanya Komatineni
2019-08-18 22:20   ` Linus Walleij
2019-08-16 19:41 ` [PATCH v9 02/22] pinctrl: tegra: Flush pinctrl writes during resume Sowjanya Komatineni
2019-08-18 22:20   ` Linus Walleij
2019-08-16 19:41 ` [PATCH v9 03/22] clk: tegra: divider: Save and restore divider rate Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 04/22] clk: tegra: pllout: Save and restore pllout context Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 05/22] clk: tegra: pll: Save and restore pll context Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 06/22] clk: tegra: Support for OSC context save and restore Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 07/22] clk: Add API to get index of the clock parent Sowjanya Komatineni
2019-11-06 23:10   ` Stephen Boyd
2019-11-07  0:54     ` Dmitry Osipenko
2019-11-07 15:21       ` Thierry Reding
2019-11-07 19:19         ` Stephen Boyd
2019-11-08 10:11           ` Thierry Reding
2019-11-08 18:12             ` Stephen Boyd [this message]
2019-11-08 18:55               ` Thierry Reding
2019-11-08 21:15                 ` Stephen Boyd
2019-08-16 19:41 ` [PATCH v9 08/22] clk: tegra: periph: Add restore_context support Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 09/22] clk: tegra: clk-super: Fix to enable PLLP branches to CPU Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 10/22] clk: tegra: clk-super: Add restore-context support Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 11/22] clk: tegra: clk-dfll: Add suspend and resume support Sowjanya Komatineni
2019-11-08 21:20   ` Stephen Boyd
2019-11-08 23:38     ` Dmitry Osipenko
2019-08-16 19:41 ` [PATCH v9 12/22] cpufreq: tegra124: " Sowjanya Komatineni
2019-11-02 14:42   ` Thierry Reding
2019-08-16 19:41 ` [PATCH v9 13/22] clk: tegra210: Use fence_udelay during PLLU init Sowjanya Komatineni
2019-08-16 19:41 ` [PATCH v9 14/22] clk: tegra: Share clk and rst register defines with Tegra clock driver Sowjanya Komatineni
2019-08-16 19:42 ` [PATCH v9 15/22] clk: tegra210: Add suspend and resume support Sowjanya Komatineni
2019-08-19 16:47   ` Dmitry Osipenko
2019-08-16 19:42 ` [PATCH v9 16/22] soc/tegra: pmc: Allow to support more tegras wake Sowjanya Komatineni
2019-08-16 19:42 ` [PATCH v9 17/22] soc/tegra: pmc: Add pmc wake support for tegra210 Sowjanya Komatineni
2019-08-16 19:42 ` [PATCH v9 18/22] arm64: tegra: Enable wake from deep sleep on RTC alarm Sowjanya Komatineni
2019-08-16 19:42 ` [PATCH v9 19/22] soc/tegra: pmc: Configure core power request polarity Sowjanya Komatineni
2019-08-16 19:42 ` [PATCH v9 20/22] soc/tegra: pmc: Configure deep sleep control settings Sowjanya Komatineni
2019-08-19 16:48   ` Dmitry Osipenko
2019-08-19 18:20     ` Sowjanya Komatineni
2019-08-19 19:07       ` Sowjanya Komatineni
2019-08-19 19:33         ` Dmitry Osipenko
2019-08-16 19:42 ` [PATCH v9 21/22] arm64: dts: tegra210-p2180: Jetson TX1 SC7 timings Sowjanya Komatineni
2019-08-16 19:42 ` [PATCH v9 22/22] arm64: dts: tegra210-p3450: Jetson Nano " Sowjanya Komatineni

Reply instructions:

You may reply publically 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=20191108181249.E284E214DB@mail.kernel.org \
    --to=sboyd@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=digetx@gmail.com \
    --cc=jason@lakedaemon.net \
    --cc=jckuo@nvidia.com \
    --cc=jonathanh@nvidia.com \
    --cc=josephl@nvidia.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=mperttunen@nvidia.com \
    --cc=pdeschrijver@nvidia.com \
    --cc=pgaikwad@nvidia.com \
    --cc=rjw@rjwysocki.net \
    --cc=robh+dt@kernel.org \
    --cc=skomatineni@nvidia.com \
    --cc=spatra@nvidia.com \
    --cc=stefan@agner.ch \
    --cc=talho@nvidia.com \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.com \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git