| Message ID | 20180607181214.30338-1-mka@chromium.org |
|---|---|
| Headers | show |
| Series | Add throttler driver for non-thermal throttling | expand |
Hi Matthias, On 07/06/18 20:12, Matthias Kaehlcke wrote: > The driver subscribes to throttling events from the Chrome OS > embedded controller and enables/disables system throttling based > on these events. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > Changes in v2: > - added SPDX line instead of license boiler-plate > - use macro to avoid splitting line > - changed variable name for throttler from 'cte' to 'ce_thr' > - formatting fixes > - Kconfig: removed odd dashes around 'help' > - added 'Reviewed-by' tag > > Note: I finally decided to keep 'Chrome OS' instead of changing it > to 'ChromeOS'. Both are currently used in the kernel, the latter is > currently more prevalent, however the official name is 'Chrome OS', > so there is no good reason to keep introducing the 'alternative' name. > I am pretty sure that somebody from Google told me the contrary, so ¯\_(ツ)_/¯ Anyway, you will probably know better than me :) > drivers/misc/throttler/Kconfig | 14 +++ > drivers/misc/throttler/Makefile | 1 + > drivers/misc/throttler/cros_ec_throttler.c | 116 +++++++++++++++++++++ > 3 files changed, 131 insertions(+) > create mode 100644 drivers/misc/throttler/cros_ec_throttler.c > > diff --git a/drivers/misc/throttler/Kconfig b/drivers/misc/throttler/Kconfig > index e561f1df5085..da6fb70b96d9 100644 > --- a/drivers/misc/throttler/Kconfig > +++ b/drivers/misc/throttler/Kconfig > @@ -12,3 +12,17 @@ menuconfig THROTTLER > Note that you also need a event monitor module usually called > *_throttler. > > +if THROTTLER > + > +config CROS_EC_THROTTLER > + tristate "Throttler event monitor for the Chrome OS Embedded Controller" > + depends on MFD_CROS_EC > + help > + This driver adds support to throttle the system in reaction to > + Chrome OS EC events. > + > + To compile this driver as a module, choose M here: the > + module will be called cros_ec_throttler. > + > +endif # THROTTLER > + > diff --git a/drivers/misc/throttler/Makefile b/drivers/misc/throttler/Makefile > index c8d920cee315..d9b2a77dabc9 100644 > --- a/drivers/misc/throttler/Makefile > +++ b/drivers/misc/throttler/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_THROTTLER) += core.o > +obj-$(CONFIG_CROS_EC_THROTTLER) += cros_ec_throttler.o > diff --git a/drivers/misc/throttler/cros_ec_throttler.c b/drivers/misc/throttler/cros_ec_throttler.c > new file mode 100644 > index 000000000000..432135c55600 > --- /dev/null > +++ b/drivers/misc/throttler/cros_ec_throttler.c > @@ -0,0 +1,116 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for throttling triggered by events from the Chrome OS Embedded > + * Controller. > + * > + * Copyright (C) 2018 Google, Inc. > + */ > + > +#include <linux/kernel.h> > +#include <linux/mfd/cros_ec.h> > +#include <linux/module.h> > +#include <linux/notifier.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/throttler.h> > + > +#define nb_to_ce_thr(nb) container_of(nb, struct cros_ec_throttler, nb) > + > +struct cros_ec_throttler { > + struct cros_ec_device *ec; > + struct throttler *throttler; > + struct notifier_block nb; > +}; > + > +static int cros_ec_throttler_event(struct notifier_block *nb, > + unsigned long queued_during_suspend, void *_notify) > +{ > + struct cros_ec_throttler *ce_thr = nb_to_ce_thr(nb); > + u32 host_event; > + > + host_event = cros_ec_get_host_event(ce_thr->ec); > + if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_THROTTLE_START)) { > + throttler_set_level(ce_thr->throttler, 1); > + > + return NOTIFY_OK; > + } else if (host_event & > + EC_HOST_EVENT_MASK(EC_HOST_EVENT_THROTTLE_STOP)) { > + throttler_set_level(ce_thr->throttler, 0); > + > + return NOTIFY_OK; > + } > + > + return NOTIFY_DONE; > +} > + > +static int cros_ec_throttler_probe(struct platform_device *pdev) > +{ > + struct cros_ec_throttler *ce_thr; > + struct device *dev = &pdev->dev; > + struct device_node *np = pdev->dev.of_node; > + int ret; > + > + if (!np) { > + /* should never happen */ > + return -EINVAL; > + } > + > + ce_thr = devm_kzalloc(dev, sizeof(*ce_thr), GFP_KERNEL); > + if (!ce_thr) > + return -ENOMEM; > + > + ce_thr->ec = dev_get_drvdata(pdev->dev.parent); > + > + ce_thr->throttler = throttler_setup(dev); > + if (IS_ERR(ce_thr->throttler)) > + return PTR_ERR(ce_thr->throttler); > + > + dev_set_drvdata(dev, ce_thr); > + > + ce_thr->nb.notifier_call = cros_ec_throttler_event; > + ret = blocking_notifier_chain_register(&ce_thr->ec->event_notifier, > + &ce_thr->nb); > + if (ret < 0) { > + dev_err(dev, "failed to register notifier\n"); > + throttler_teardown(ce_thr->throttler); > + return ret; > + } > + > + return 0; > +} > + > +static int cros_ec_throttler_remove(struct platform_device *pdev) > +{ > + struct cros_ec_throttler *ce_thr = platform_get_drvdata(pdev); > + > + blocking_notifier_chain_unregister(&ce_thr->ec->event_notifier, > + &ce_thr->nb); > + > + throttler_teardown(ce_thr->throttler); > + > + return 0; > +} > + > +#ifdef CONFIG_OF > +static const struct of_device_id cros_ec_throttler_of_match[] = { > + { .compatible = "google,cros-ec-throttler" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, cros_ec_throttler_of_match); > +#endif /* CONFIG_OF */ > + > +static struct platform_driver cros_ec_throttler_driver = { > + .driver = { > + .name = "cros-ec-throttler", > + .of_match_table = of_match_ptr(cros_ec_throttler_of_match), > + }, > + .probe = cros_ec_throttler_probe, > + .remove = cros_ec_throttler_remove, > +}; > + > +module_platform_driver(cros_ec_throttler_driver); > + > +MODULE_LICENSE("GPL"); Something that I learnt recently. To match the SPDX-license tag you should set this to "GPL v2" [1] * "GPL" [GNU Public License v2 or later] * "GPL v2" [GNU Public License v2] [1] https://elixir.bootlin.com/linux/v4.17/source/include/linux/module.h#L172 > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > +MODULE_DESCRIPTION("Chrome OS EC Throttler"); > Best regards, Enric -- 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 Enric, On Fri, Jun 08, 2018 at 04:09:18PM +0200, Enric Balletbo i Serra wrote: > On 07/06/18 20:12, Matthias Kaehlcke wrote: > > The driver subscribes to throttling events from the Chrome OS > > embedded controller and enables/disables system throttling based > > on these events. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > --- > > Changes in v2: > > - added SPDX line instead of license boiler-plate > > - use macro to avoid splitting line > > - changed variable name for throttler from 'cte' to 'ce_thr' > > - formatting fixes > > - Kconfig: removed odd dashes around 'help' > > - added 'Reviewed-by' tag > > > > Note: I finally decided to keep 'Chrome OS' instead of changing it > > to 'ChromeOS'. Both are currently used in the kernel, the latter is > > currently more prevalent, however the official name is 'Chrome OS', > > so there is no good reason to keep introducing the 'alternative' name. > > > > I am pretty sure that somebody from Google told me the contrary, so > > ¯\_(ツ)_/¯ > > Anyway, you will probably know better than me :) The different names in the kernel code indicate that even folks at Google disagree on this ;-) The name might have evolved over time. chrome://chrome on a Chromebook names it 'Chrome OS', as do official pages like https://www.google.com/chromebook/ and https://www.chromium.org/chromium-os so I think it is better to use this name. > > +MODULE_LICENSE("GPL"); > > Something that I learnt recently. To match the SPDX-license tag you should set > this to "GPL v2" [1] > > * "GPL" [GNU Public License v2 or later] > * "GPL v2" [GNU Public License v2] > > [1] https://elixir.bootlin.com/linux/v4.17/source/include/linux/module.h#L172 Will update in the next revision Thanks! Matthias -- 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! A few comments, but I didn't get to finish a thorough review yet. 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/MAINTAINERS b/MAINTAINERS > index 92e47b5b0480..f9550e5680ce 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13938,6 +13938,13 @@ T: git git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git > S: Maintained > F: drivers/platform/x86/thinkpad_acpi.c > > +THROTTLER DRIVERS > +M: Matthias Kaehlcke <mka@chromium.org> > +L: linux-pm@vger.kernel.org > +S: Maintained > +F: drivers/misc/throttler/ > +F: include/linux/throttler.h > + > THUNDERBOLT DRIVER > M: Andreas Noever <andreas.noever@gmail.com> > M: Michael Jamet <michael.jamet@intel.com> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 5d713008749b..691d9625d83c 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -513,4 +513,5 @@ source "drivers/misc/echo/Kconfig" > source "drivers/misc/cxl/Kconfig" > source "drivers/misc/ocxl/Kconfig" > source "drivers/misc/cardreader/Kconfig" > +source "drivers/misc/throttler/Kconfig" > endmenu > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 20be70c3f118..b549ccad5215 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -57,3 +57,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o > obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o > obj-$(CONFIG_OCXL) += ocxl/ > obj-$(CONFIG_MISC_RTSX) += cardreader/ > +obj-$(CONFIG_THROTTLER) += throttler/ > 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. > + help > + This option enables core support for non-thermal throttling of CPUs > + and devfreq devices. > + > + Note that you also need a event monitor module usually called > + *_throttler. > + > diff --git a/drivers/misc/throttler/Makefile b/drivers/misc/throttler/Makefile > new file mode 100644 > index 000000000000..c8d920cee315 > --- /dev/null > +++ b/drivers/misc/throttler/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_THROTTLER) += core.o > 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 > @@ -0,0 +1,642 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Core code for non-thermal throttling > + * > + * Copyright (C) 2018 Google, Inc. > + */ > + > +#include <linux/cpufreq.h> > +#include <linux/cpumask.h> > +#include <linux/devfreq.h> > +#include <linux/kernel.h> > +#include <linux/list.h> > +#include <linux/mutex.h> > +#include <linux/notifier.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/pm_opp.h> > +#include <linux/sort.h> > +#include <linux/slab.h> > +#include <linux/throttler.h> > + > +/* > + * Non-thermal throttling: throttling of system components in response to > + * external events (e.g. high battery discharge current). > + * > + * The throttler supports throttling through cpufreq and devfreq. Multiple > + * levels of throttling can be configured. At level 0 no throttling is > + * active on behalf of the throttler, for values > 0 throttling is typically > + * configured to be increasingly aggressive with each level. > + * The number of throttling levels is not limited by the throttler (though > + * it is likely limited by the throttling devices). It is not necessary to > + * configure the same number of levels for all throttling devices. If the > + * requested throttling level for a device is higher than the maximum level > + * of the device the throttler will select the maximum throttling level of > + * the device. > + * > + * Non-thermal throttling is split in two parts: > + * > + * - throttler core > + * - parses the thermal policy > + * - applies throttling settings for a requested level of throttling > + * > + * - event monitor driver > + * - monitors events that trigger throttling > + * - determines the throttling level (often limited to on/off) > + * - asks throttler core to apply throttling settings > + * > + * It is possible for a system to have more than one throttler and the > + * throttlers may make use of the same throttling devices, in case of > + * conflicting settings for a device the more aggressive values will be > + * applied. > + * > + */ > + > +#define ci_to_throttler(ci) \ > + container_of(ci, struct throttler, devfreq.class_iface) > + > +// #define DEBUG_THROTTLER Did you mean to leave your debug code in? Seems like you have some related dead code under #ifdefs. (If you do want this, maybe it'd be better under debugfs, until somebody really wants to formalize and document it.) > + > +struct thr_freq_table { > + uint32_t *freqs; > + int n_entries; > +}; > + > +struct cpufreq_thrdev { > + uint32_t cpu; > + struct thr_freq_table freq_table; > + struct list_head node; > +}; > + > +struct devfreq_thrdev { > + struct devfreq *devfreq; > + struct thr_freq_table freq_table; > + struct throttler *thr; > + struct notifier_block nb; > + struct list_head node; > +}; > + > +struct __thr_cpufreq { > + struct list_head list; > + cpumask_t cm_initialized; > + cpumask_t cm_ignore; > + struct notifier_block nb; > +}; > + > +struct __thr_devfreq { > + struct list_head list; > + struct class_interface class_iface; > +}; > + > +struct throttler { > + struct device *dev; > + int level; > + struct __thr_cpufreq cpufreq; > + struct __thr_devfreq devfreq; > + struct mutex lock; > +}; > + > +static inline int cmp_freqs(const void *a, const void *b) > +{ > + const uint32_t *pa = a, *pb = b; > + > + if (*pa < *pb) > + return 1; > + else if (*pa > *pb) > + return -1; > + > + return 0; > +} > + > +static int thr_handle_devfreq_event(struct notifier_block *nb, > + unsigned long event, void *data); > + > +static unsigned long thr_get_throttling_freq(struct thr_freq_table *ft, > + int level) > +{ > + if (level == 0) { > + WARN(true, "level == 0"); > + return ULONG_MAX; > + } > + > + if (level <= ft->n_entries) > + return ft->freqs[level - 1]; > + else > + return ft->freqs[ft->n_entries - 1]; > +} > + > +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. > + u64 rate; > + > + err = of_property_read_u64(np_opp, "opp-hz", > + &rate); > + if (!err) { > + freqs[nfreqs] = rate; > + nfreqs++; > + } else { > + dev_err(thr->dev, > + "opp-hz not found: %s\n", > + np_opp->full_name); > + } > + } > + > + of_node_put(np_thr); > + put_device(&pdev->dev); > + } > + } > + > + if (nfreqs > 0) { > + /* sort frequencies in descending order */ > + sort(freqs, nfreqs, sizeof(*freqs), cmp_freqs, NULL); > + > + ft->n_entries = nfreqs; > + ft->freqs = devm_kzalloc(thr->dev, > + nfreqs * sizeof(*freqs), GFP_KERNEL); > + if (!ft->freqs) { > + err = -ENOMEM; > + goto out_free; > + } > + > + memcpy(ft->freqs, freqs, nfreqs * sizeof(*freqs)); > + } else { > + err = -ENODEV; > + } > + > +out_free: > + kfree(freqs); > + > +out_node_put: > + of_node_put(np_opp_desc); > + > + return err; > +} [...] Brian -- 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 Mon, Jun 11, 2018 at 06:49:13PM -0700, Brian Norris wrote: > Hi! > > A few comments, but I didn't get to finish a thorough review yet. > > > 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/MAINTAINERS b/MAINTAINERS > > index 92e47b5b0480..f9550e5680ce 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -13938,6 +13938,13 @@ T: git git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git > > S: Maintained > > F: drivers/platform/x86/thinkpad_acpi.c > > > > +THROTTLER DRIVERS > > +M: Matthias Kaehlcke <mka@chromium.org> > > +L: linux-pm@vger.kernel.org > > +S: Maintained > > +F: drivers/misc/throttler/ > > +F: include/linux/throttler.h > > + > > THUNDERBOLT DRIVER > > M: Andreas Noever <andreas.noever@gmail.com> > > M: Michael Jamet <michael.jamet@intel.com> > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > index 5d713008749b..691d9625d83c 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -513,4 +513,5 @@ source "drivers/misc/echo/Kconfig" > > source "drivers/misc/cxl/Kconfig" > > source "drivers/misc/ocxl/Kconfig" > > source "drivers/misc/cardreader/Kconfig" > > +source "drivers/misc/throttler/Kconfig" > > endmenu > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > index 20be70c3f118..b549ccad5215 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -57,3 +57,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o > > obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o > > obj-$(CONFIG_OCXL) += ocxl/ > > obj-$(CONFIG_MISC_RTSX) += cardreader/ > > +obj-$(CONFIG_THROTTLER) += throttler/ > > 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. 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. > > 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. > > +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 Thanks Matthias -- 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 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 -- 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 Tue, Jun 12, 2018 at 01:00:14PM -0700, Brian Norris wrote: > 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? Ok, I'll remove them > > 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? The build error occured with 'openrisc', from a quick grep in drivers/cpufreq it seems indeed that there is no driver for this architecture. > > > > 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. Ok, will change to debugfs with CONFIG_* option. > > > > +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. There is actually a better option, I was so focussed on the of_find_device_by_node() way that I didn't see the obvious solution right away: We can just check if 'np_thr == thr->dev->of_node' ... Thanks for pushing me to give it another look! -- 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 Thu 2018-06-07 11:12:03, Matthias Kaehlcke wrote: > This series adds the throttler driver, for non-thermal throttling of > CPUs and devfreq devices. A use case for non-thermal throttling could > be the detection of a high battery discharge voltage, close to the voltage -> current. Pavel