LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Douglas Anderson <dianders@chromium.org>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>
Subject: Re: [PATCH v2 09/11] misc: throttler: Add core support for non-thermal throttling
Date: Tue, 12 Jun 2018 13:00:14 -0700	[thread overview]
Message-ID: <20180612200012.GA99983@rodete-desktop-imager.corp.google.com> (raw)
In-Reply-To: <20180612171140.GH88063@google.com>

Hi,

On Tue, Jun 12, 2018 at 10:11:40AM -0700, Matthias Kaehlcke wrote:
> On Mon, Jun 11, 2018 at 06:49:13PM -0700, Brian Norris wrote:
> > On Thu, Jun 07, 2018 at 11:12:12AM -0700, Matthias Kaehlcke wrote:
> > > The purpose of the throttler is to provide support for non-thermal
> > > throttling. Throttling is triggered by external event, e.g. the
> > > detection of a high battery discharge current, close to the OCP limit
> > > of the battery. The throttler is only in charge of the throttling, not
> > > the monitoring, which is done by another (possibly platform specific)
> > > driver.
> > > 
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > > Changes in v2:
> > > - completely reworked the driver to support configuration through OPPs, which
> > >   requires a more dynamic handling
> > > - added sysfs attribute to set the level for debugging and testing
> > > - Makefile: depend on Kconfig option to traverse throttler directory
> > > - Kconfig: removed 'default n'
> > > - added SPDX line instead of license boiler-plate
> > > - added entry to MAINTAINERS file
> > > 
> > > 
> > >  MAINTAINERS                     |   7 +
> > >  drivers/misc/Kconfig            |   1 +
> > >  drivers/misc/Makefile           |   1 +
> > >  drivers/misc/throttler/Kconfig  |  14 +
> > >  drivers/misc/throttler/Makefile |   1 +
> > >  drivers/misc/throttler/core.c   | 642 ++++++++++++++++++++++++++++++++
> > >  include/linux/throttler.h       |  11 +
> > >  7 files changed, 677 insertions(+)
> > >  create mode 100644 drivers/misc/throttler/Kconfig
> > >  create mode 100644 drivers/misc/throttler/Makefile
> > >  create mode 100644 drivers/misc/throttler/core.c
> > >  create mode 100644 include/linux/throttler.h
> > > 

...

> > > diff --git a/drivers/misc/throttler/Kconfig b/drivers/misc/throttler/Kconfig
> > > new file mode 100644
> > > index 000000000000..e561f1df5085
> > > --- /dev/null
> > > +++ b/drivers/misc/throttler/Kconfig
> > > @@ -0,0 +1,14 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +
> > > +menuconfig THROTTLER
> > > +	bool "Throttler support"
> > > +	depends on OF
> > > +	select CPU_FREQ
> > > +	select PM_DEVFREQ
> > 
> > I'm curious whether we're actually truly compile-time dependent on
> > {CPU,DEV}FREQ? It seems like those subsystems mostly stub out stuff so
> > they fall back to no-ops if not built in.
> >
> > I know that's not very useful for your existing throttler, since it
> > wouldn't be very effective if one or both were disabled.
> 
> The idea is not to depend on both options being enabled, since
> throttling of one type might be all that is needed.

OK, then if you fix the build errors...don't do these 'select's here?

> As the build bot pointed out cpufreq doesn't stub out all functions.
> Probably the cleanest way to work around this is to define a stub for
> cpufreq_update_policy() in the throttler when CONFIG_CPU_FREQ is not
> defined.

Might make sense.

Also, how is it that CONFIG_CPU_FREQ managed to not be defined, even
though you 'select'ed it? Was the kbuild error on some oddball
architecture that doesn't support CPU_FREQ?

> > > diff --git a/drivers/misc/throttler/core.c b/drivers/misc/throttler/core.c
> > > new file mode 100644
> > > index 000000000000..15b50c111032
> > > --- /dev/null
> > > +++ b/drivers/misc/throttler/core.c
> > > ...
> > > +// #define DEBUG_THROTTLER
> > 
> > Did you mean to leave your debug code in? Seems like you have some
> > related dead code under #ifdefs.
> 
> Yes, I left it in intentionally.
> 
> > (If you do want this, maybe it'd be better under debugfs, until somebody
> > really wants to formalize and document it.)
> 
> Since it is code that is never enabled in an official kernel build I
> found it simpler to use sysfs with a direct association with the
> device, instead of having <debugfs>/throttler/<throttler_name>/level.
> 
> If folks have strong opinions about using debugfs or not having the
> debug code at all I'm fine with changing/dropping it.

If you ever expect this to actually get merged, I'd think we should go
with one way or the other. Dead code is not appreciated in mainline, so
either make it something people can actually have a chance of using
(e.g., a CONFIG_* option or debugfs), or else drop it.

> > > +static int thr_init_freq_table(struct throttler *thr, struct device *opp_dev,
> > > +			       struct thr_freq_table *ft)
> > > +{
> > > +	struct device_node *np_opp_desc, *np_opp;
> > > +	int nchilds;
> > > +	uint32_t *freqs;
> > > +	int nfreqs = 0;
> > > +	int err = 0;
> > > +
> > > +	np_opp_desc = dev_pm_opp_of_get_opp_desc_node(opp_dev);
> > > +	if (!np_opp_desc)
> > > +		return -EINVAL;
> > > +
> > > +	nchilds = of_get_child_count(np_opp_desc);
> > > +	if (!nchilds) {
> > > +		err = -EINVAL;
> > > +		goto out_node_put;
> > > +	}
> > > +
> > > +	freqs = kzalloc(nchilds * sizeof(uint32_t), GFP_KERNEL);
> > > +	if (!freqs) {
> > > +		err = -ENOMEM;
> > > +		goto out_node_put;
> > > +	}
> > > +
> > > +	/* determine which OPPs are used by this throttler (if any) */
> > > +	for_each_child_of_node(np_opp_desc, np_opp) {
> > > +		int num_thr;
> > > +		int i;
> > > +
> > > +		num_thr = of_property_count_u32_elems(np_opp, "opp-throttlers");
> > > +		if (num_thr < 0)
> > > +			continue;
> > > +
> > > +		for (i = 0; i < num_thr; i++) {
> > > +			struct device_node *np_thr;
> > > +			struct platform_device *pdev;
> > > +
> > > +			np_thr = of_parse_phandle(np_opp, "opp-throttlers", i);
> > > +			if (!np_thr) {
> > > +				dev_err(thr->dev,
> > > +					"failed to parse phandle %d: %s\n", i,
> > > +					np_opp->full_name);
> > > +				continue;
> > > +			}
> > > +
> > > +			pdev = of_find_device_by_node(np_thr);
> > > +			if (!pdev) {
> > > +				dev_err(thr->dev,
> > > +					"could not find throttler dev: %s\n",
> > > +					 np_thr->full_name);
> > > +				of_node_put(np_thr);
> > > +				continue;
> > > +			}
> > > +
> > > +			/* OPP is used by this throttler */
> > > +			if (&pdev->dev == thr->dev) {
> > 
> > So you're assuming that all throttlers are platform devices? Seems
> > slightly shaky; I could easily imagine a similar device that's a SPI or
> > I2C device.
> 
> As of now that's the only existing throttler. Adding handling for
> throttlers on other buses that might never be implemented seems
> premature. If throttlers with other bus types are added the core
> code can be extended to support this using
> of_find_i2c_device_by_node(), of_find_spi_device_by_node(), etc

I suppose...but it feels like there should be a better way to do this
that isn't specific to a particular bus.

If you're not going to fix this, then maybe you should force callers to
pass a plaform_device instead of device, to make it extra clear that
other devices are not supported.

Brian

  reply	other threads:[~2018-06-12 20:00 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-07 18:12 [PATCH v2 00/11] Add throttler driver " Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 01/11] PM / devfreq: Init user limits from OPP limits, not viceversa Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 02/11] PM / devfreq: Fix handling of min/max_freq == 0 Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 03/11] PM / devfreq: Don't adjust to user limits in governors Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 04/11] PM / devfreq: Add struct devfreq_policy Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 05/11] PM / devfreg: Add support for policy notifiers Matthias Kaehlcke
2018-06-08 23:21   ` kbuild test robot
2018-06-09  1:16   ` kbuild test robot
2018-06-07 18:12 ` [PATCH v2 06/11] PM / devfreq: Make update_devfreq() public Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 07/11] PM / devfreq: export devfreq_class Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 08/11] dt-bindings: PM / OPP: add opp-throttlers property Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 09/11] misc: throttler: Add core support for non-thermal throttling Matthias Kaehlcke
2018-06-09  4:34   ` kbuild test robot
2018-06-12  1:49   ` Brian Norris
2018-06-12 17:11     ` Matthias Kaehlcke
2018-06-12 20:00       ` Brian Norris [this message]
2018-06-13  1:48         ` Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 10/11] dt-bindings: misc: add bindings for cros_ec_throttler Matthias Kaehlcke
2018-06-12 19:10   ` Rob Herring
2018-06-12 20:40     ` Brian Norris
2018-06-13  2:00       ` Matthias Kaehlcke
2018-06-07 18:12 ` [PATCH v2 11/11] misc: throttler: Add Chrome OS EC throttler Matthias Kaehlcke
2018-06-08 14:09   ` Enric Balletbo i Serra
2018-06-08 15:56     ` Matthias Kaehlcke
2018-06-28 18:52 ` [PATCH v2 00/11] Add throttler driver for non-thermal throttling Pavel Machek

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=20180612200012.GA99983@rodete-desktop-imager.corp.google.com \
    --to=briannorris@chromium.org \
    --cc=arnd@arndb.de \
    --cc=cw00.choi@samsung.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=enric.balletbo@collabora.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mka@chromium.org \
    --cc=myungjoo.ham@samsung.com \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v2 09/11] misc: throttler: Add core support for non-thermal throttling' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).