* Re: [PATCH v2 06/10] mfd / platform: cros_ec: Reorganize platform and mfd includes
From: Chanwoo Choi @ 2019-06-20 10:28 UTC (permalink / raw)
To: Enric Balletbo i Serra, linux-kernel
Cc: gwendal, Guenter Roeck, Benson Leung, Lee Jones, kernel, dtor,
Andy Shevchenko, Mark Brown, Wolfram Sang, Neil Armstrong,
Alexandre Belloni, Benjamin Tissoires, Dmitry Torokhov,
Sebastian Reichel, Mauro Carvalho Chehab, alsa-devel,
Alessandro Zummo, linux-iio, Fabien Lahoudere, linux-i2c,
linux-rtc, Brian Norris, Gustavo A. R. Silva, Sebastian Reichel,
Rushikesh S Kadam, linux-input, Lars-Peter Clausen,
Jonathan Cameron, Peter Meerwald-Stadler, linux-media, linux-pwm,
Thierry Reding, linux-pm, Takashi Iwai, Liam Girdwood,
MyungJoo Ham, Evan Green, Hartmut Knaack, Cheng-Yi Chiang,
Jaroslav Kysela, Colin Ian King, Hans Verkuil, Jiri Kosina
In-Reply-To: <20190614163635.22413-7-enric.balletbo@collabora.com>
Hi Enric,
For extcon part,
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Best Regards,
Chanwoo choi
On 19. 6. 15. 오전 1:36, Enric Balletbo i Serra wrote:
> There is a bit of mess between cros-ec mfd includes and platform
> includes. For example, we have a linux/mfd/cros_ec.h include that
> exports the interface implemented in platform/chrome/cros_ec_proto.c. Or
> we have a linux/mfd/cros_ec_commands.h file that is non related to the
> multifunction device (in the sense that is not exporting any function of
> the mfd device). This causes crossed includes between mfd and
> platform/chrome subsystems and makes the code difficult to read, apart
> from creating 'curious' situations where a platform/chrome driver includes
> a linux/mfd/cros_ec.h file just to get the exported functions that are
> implemented in another platform/chrome driver.
>
> In order to have a better separation on what the cros-ec multifunction
> driver does and what the cros-ec core provides move and rework the
> affected includes doing:
>
> - Move cros_ec_commands.h to include/linux/platform_data/cros_ec_commands.h
> - Get rid of the parts that are implemented in the platform/chrome/cros_ec_proto.c
> driver from include/linux/mfd/cros_ec.h to a new file
> include/linux/platform_data/cros_ec_proto.h
> - Update all the drivers with the new includes, so
> - Drivers that only need to know about the protocol include
> - linux/platform_data/cros_ec_proto.h
> - linux/platform_data/cros_ec_commands.h
> - Drivers that need to know about the cros-ec mfd device also include
> - linux/mfd/cros_ec.h
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>
> Changes in v2: None
>
> drivers/extcon/extcon-usbc-cros-ec.c | 3 +-
> drivers/hid/hid-google-hammer.c | 4 +-
> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 4 +-
> drivers/iio/accel/cros_ec_accel_legacy.c | 3 +-
> .../common/cros_ec_sensors/cros_ec_sensors.c | 3 +-
> .../cros_ec_sensors/cros_ec_sensors_core.c | 3 +-
> drivers/iio/light/cros_ec_light_prox.c | 3 +-
> drivers/iio/pressure/cros_ec_baro.c | 3 +-
> drivers/input/keyboard/cros_ec_keyb.c | 4 +-
> .../media/platform/cros-ec-cec/cros-ec-cec.c | 4 +-
> drivers/mfd/cros_ec_dev.c | 3 +-
> drivers/platform/chrome/cros_ec.c | 3 +-
> drivers/platform/chrome/cros_ec_debugfs.c | 3 +-
> drivers/platform/chrome/cros_ec_i2c.c | 4 +-
> drivers/platform/chrome/cros_ec_lightbar.c | 3 +-
> drivers/platform/chrome/cros_ec_lpc.c | 4 +-
> drivers/platform/chrome/cros_ec_lpc_reg.c | 4 +-
> drivers/platform/chrome/cros_ec_proto.c | 3 +-
> drivers/platform/chrome/cros_ec_rpmsg.c | 4 +-
> drivers/platform/chrome/cros_ec_spi.c | 4 +-
> drivers/platform/chrome/cros_ec_sysfs.c | 3 +-
> drivers/platform/chrome/cros_ec_trace.c | 2 +-
> drivers/platform/chrome/cros_ec_trace.h | 4 +-
> drivers/platform/chrome/cros_ec_vbc.c | 3 +-
> drivers/platform/chrome/cros_usbpd_logger.c | 5 +-
> drivers/power/supply/cros_usbpd-charger.c | 5 +-
> drivers/pwm/pwm-cros-ec.c | 4 +-
> drivers/rtc/rtc-cros-ec.c | 3 +-
> .../linux/iio/common/cros_ec_sensors_core.h | 3 +-
> include/linux/mfd/cros_ec.h | 306 -----------------
> .../{mfd => platform_data}/cros_ec_commands.h | 0
> include/linux/platform_data/cros_ec_proto.h | 315 ++++++++++++++++++
> sound/soc/codecs/cros_ec_codec.c | 4 +-
> 33 files changed, 377 insertions(+), 349 deletions(-)
> rename include/linux/{mfd => platform_data}/cros_ec_commands.h (100%)
> create mode 100644 include/linux/platform_data/cros_ec_proto.h
>
> diff --git a/drivers/extcon/extcon-usbc-cros-ec.c b/drivers/extcon/extcon-usbc-cros-ec.c
> index 43c0a936ab82..5290cc2d19d9 100644
> --- a/drivers/extcon/extcon-usbc-cros-ec.c
> +++ b/drivers/extcon/extcon-usbc-cros-ec.c
> @@ -6,10 +6,11 @@
>
> #include <linux/extcon-provider.h>
> #include <linux/kernel.h>
> -#include <linux/mfd/cros_ec.h>
> #include <linux/module.h>
> #include <linux/notifier.h>
> #include <linux/of.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/sched.h>
> diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
> index ee5e0bdcf078..84f8c127ebdc 100644
> --- a/drivers/hid/hid-google-hammer.c
> +++ b/drivers/hid/hid-google-hammer.c
> @@ -16,9 +16,9 @@
> #include <linux/acpi.h>
> #include <linux/hid.h>
> #include <linux/leds.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/pm_wakeup.h>
> #include <asm/unaligned.h>
> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index 82bcd9a78759..c551aa96a2e3 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -5,8 +5,8 @@
>
> #include <linux/module.h>
> #include <linux/i2c.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
>
> diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> index 46bb2e421bb9..fd9a634f741e 100644
> --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> @@ -18,9 +18,10 @@
> #include <linux/iio/triggered_buffer.h>
> #include <linux/kernel.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
>
> #define DRV_NAME "cros-ec-accel-legacy"
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index 17af4e0fd5f8..40dc24ff0ee5 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -17,8 +17,9 @@
> #include <linux/iio/triggered_buffer.h>
> #include <linux/kernel.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
>
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 719a0df5aeeb..fd63315399ac 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -14,9 +14,10 @@
> #include <linux/iio/trigger_consumer.h>
> #include <linux/kernel.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
>
> static char *cros_ec_loc[] = {
> diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> index 308ee6ff2e22..437e0eae9178 100644
> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -15,8 +15,9 @@
> #include <linux/iio/trigger_consumer.h>
> #include <linux/kernel.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
>
> diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> index 034ce98d6e97..956dc01f1295 100644
> --- a/drivers/iio/pressure/cros_ec_baro.c
> +++ b/drivers/iio/pressure/cros_ec_baro.c
> @@ -15,9 +15,10 @@
> #include <linux/iio/trigger_consumer.h>
> #include <linux/kernel.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
>
> /*
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index d56001181598..2b71c5a51f90 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -22,8 +22,8 @@
> #include <linux/slab.h>
> #include <linux/sysrq.h>
> #include <linux/input/matrix_keypad.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
>
> #include <asm/unaligned.h>
>
> diff --git a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> index 068df9888dbf..2e4e263a4a94 100644
> --- a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> +++ b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> @@ -16,8 +16,8 @@
> #include <linux/interrupt.h>
> #include <media/cec.h>
> #include <media/cec-notifier.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
>
> #define DRV_NAME "cros-ec-cec"
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index d465bcde9fc4..7572fe096c72 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -19,11 +19,12 @@
>
> #include <linux/mfd/core.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> #include <linux/mod_devicetable.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/slab.h>
>
> #define DRV_NAME "cros-ec-dev"
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index 11fced7917fc..9800597ccd96 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -21,7 +21,8 @@
> #include <linux/interrupt.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> -#include <linux/mfd/cros_ec.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/suspend.h>
> #include <asm/unaligned.h>
>
> diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
> index 4c2a27f6a6d0..b088d91be9c9 100644
> --- a/drivers/platform/chrome/cros_ec_debugfs.c
> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
> @@ -8,9 +8,10 @@
> #include <linux/delay.h>
> #include <linux/fs.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/poll.h>
> #include <linux/sched.h>
> diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
> index 6bb82dfa7dae..9bd97bc8454b 100644
> --- a/drivers/platform/chrome/cros_ec_i2c.c
> +++ b/drivers/platform/chrome/cros_ec_i2c.c
> @@ -9,8 +9,8 @@
> #include <linux/module.h>
> #include <linux/i2c.h>
> #include <linux/interrupt.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
>
> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
> index d30a6650b0b5..caa26da2c788 100644
> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> @@ -9,8 +9,9 @@
> #include <linux/fs.h>
> #include <linux/kobject.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/sched.h>
> #include <linux/types.h>
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index 2c7e654cf89c..0c976e95998a 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -16,9 +16,9 @@
> #include <linux/delay.h>
> #include <linux/io.h>
> #include <linux/interrupt.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/printk.h>
> #include <linux/suspend.h>
> diff --git a/drivers/platform/chrome/cros_ec_lpc_reg.c b/drivers/platform/chrome/cros_ec_lpc_reg.c
> index 0f5cd0ac8b49..dec9a779e209 100644
> --- a/drivers/platform/chrome/cros_ec_lpc_reg.c
> +++ b/drivers/platform/chrome/cros_ec_lpc_reg.c
> @@ -4,8 +4,8 @@
> // Copyright (C) 2016 Google, Inc
>
> #include <linux/io.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
>
> #include "cros_ec_lpc_mec.h"
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 3d2325197a68..f659f96bda12 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -3,10 +3,11 @@
> //
> // Copyright (C) 2015 Google, Inc
>
> -#include <linux/mfd/cros_ec.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/slab.h>
> #include <asm/unaligned.h>
>
> diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
> index 520e507bfa54..9633e5417686 100644
> --- a/drivers/platform/chrome/cros_ec_rpmsg.c
> +++ b/drivers/platform/chrome/cros_ec_rpmsg.c
> @@ -6,9 +6,9 @@
> #include <linux/delay.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/of.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/rpmsg.h>
> #include <linux/slab.h>
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index 02f9e8257581..a4167dfd85bf 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -6,9 +6,9 @@
> #include <linux/delay.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/of.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/spi/spi.h>
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index fe0b7614ae1b..0caeb8d0989d 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -9,8 +9,9 @@
> #include <linux/fs.h>
> #include <linux/kobject.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/printk.h>
> #include <linux/slab.h>
> diff --git a/drivers/platform/chrome/cros_ec_trace.c b/drivers/platform/chrome/cros_ec_trace.c
> index 0a76412095a9..6f80ff4532ae 100644
> --- a/drivers/platform/chrome/cros_ec_trace.c
> +++ b/drivers/platform/chrome/cros_ec_trace.c
> @@ -6,7 +6,7 @@
> #define TRACE_SYMBOL(a) {a, #a}
>
> // Generate the list using the following script:
> -// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p' include/linux/mfd/cros_ec_commands.h
> +// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p' include/linux/platform_data/cros_ec_commands.h
> #define EC_CMDS \
> TRACE_SYMBOL(EC_CMD_PROTO_VERSION), \
> TRACE_SYMBOL(EC_CMD_HELLO), \
> diff --git a/drivers/platform/chrome/cros_ec_trace.h b/drivers/platform/chrome/cros_ec_trace.h
> index 7ae3b89c78b9..0dd4df30fa89 100644
> --- a/drivers/platform/chrome/cros_ec_trace.h
> +++ b/drivers/platform/chrome/cros_ec_trace.h
> @@ -11,8 +11,10 @@
> #if !defined(_CROS_EC_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ)
> #define _CROS_EC_TRACE_H_
>
> +#include <linux/bits.h>
> #include <linux/types.h>
> -#include <linux/mfd/cros_ec.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
>
> #include <linux/tracepoint.h>
>
> diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
> index 8392a1ec33a7..cffe119e7a7a 100644
> --- a/drivers/platform/chrome/cros_ec_vbc.c
> +++ b/drivers/platform/chrome/cros_ec_vbc.c
> @@ -7,8 +7,9 @@
> #include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/slab.h>
>
> #define DRV_NAME "cros-ec-vbc"
> diff --git a/drivers/platform/chrome/cros_usbpd_logger.c b/drivers/platform/chrome/cros_usbpd_logger.c
> index 7c7b267626a0..c549a9b49b56 100644
> --- a/drivers/platform/chrome/cros_usbpd_logger.c
> +++ b/drivers/platform/chrome/cros_usbpd_logger.c
> @@ -6,10 +6,11 @@
> */
>
> #include <linux/ktime.h>
> -#include <linux/math64.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/math64.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/rtc.h>
>
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> index 7e9c3984ef6a..ed8eca28c195 100644
> --- a/drivers/power/supply/cros_usbpd-charger.c
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -5,9 +5,10 @@
> * Copyright (c) 2014 - 2018 Google, Inc
> */
>
> -#include <linux/module.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/power_supply.h>
> #include <linux/slab.h>
> diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> index 98f6ac6cf6ab..85bea2d40b7d 100644
> --- a/drivers/pwm/pwm-cros-ec.c
> +++ b/drivers/pwm/pwm-cros-ec.c
> @@ -6,8 +6,8 @@
> */
>
> #include <linux/module.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/pwm.h>
> #include <linux/slab.h>
> diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
> index 4d6bf9304ceb..6909e01936d9 100644
> --- a/drivers/rtc/rtc-cros-ec.c
> +++ b/drivers/rtc/rtc-cros-ec.c
> @@ -6,8 +6,9 @@
>
> #include <linux/kernel.h>
> #include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/rtc.h>
> #include <linux/slab.h>
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index ce16445411ac..8a91669f5bed 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -18,7 +18,8 @@
>
> #include <linux/iio/iio.h>
> #include <linux/irqreturn.h>
> -#include <linux/mfd/cros_ec.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
>
> enum {
> CROS_EC_SENSOR_X,
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 2a1372d167b9..e0bae49535e1 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -16,184 +16,7 @@
> #ifndef __LINUX_MFD_CROS_EC_H
> #define __LINUX_MFD_CROS_EC_H
>
> -#include <linux/cdev.h>
> #include <linux/device.h>
> -#include <linux/notifier.h>
> -#include <linux/mfd/cros_ec_commands.h>
> -#include <linux/mutex.h>
> -
> -#define CROS_EC_DEV_NAME "cros_ec"
> -#define CROS_EC_DEV_FP_NAME "cros_fp"
> -#define CROS_EC_DEV_PD_NAME "cros_pd"
> -#define CROS_EC_DEV_TP_NAME "cros_tp"
> -#define CROS_EC_DEV_ISH_NAME "cros_ish"
> -
> -/*
> - * The EC is unresponsive for a time after a reboot command. Add a
> - * simple delay to make sure that the bus stays locked.
> - */
> -#define EC_REBOOT_DELAY_MS 50
> -
> -/*
> - * Max bus-specific overhead incurred by request/responses.
> - * I2C requires 1 additional byte for requests.
> - * I2C requires 2 additional bytes for responses.
> - * SPI requires up to 32 additional bytes for responses.
> - */
> -#define EC_PROTO_VERSION_UNKNOWN 0
> -#define EC_MAX_REQUEST_OVERHEAD 1
> -#define EC_MAX_RESPONSE_OVERHEAD 32
> -
> -/*
> - * Command interface between EC and AP, for LPC, I2C and SPI interfaces.
> - */
> -enum {
> - EC_MSG_TX_HEADER_BYTES = 3,
> - EC_MSG_TX_TRAILER_BYTES = 1,
> - EC_MSG_TX_PROTO_BYTES = EC_MSG_TX_HEADER_BYTES +
> - EC_MSG_TX_TRAILER_BYTES,
> - EC_MSG_RX_PROTO_BYTES = 3,
> -
> - /* Max length of messages for proto 2*/
> - EC_PROTO2_MSG_BYTES = EC_PROTO2_MAX_PARAM_SIZE +
> - EC_MSG_TX_PROTO_BYTES,
> -
> - EC_MAX_MSG_BYTES = 64 * 1024,
> -};
> -
> -/**
> - * struct cros_ec_command - Information about a ChromeOS EC command.
> - * @version: Command version number (often 0).
> - * @command: Command to send (EC_CMD_...).
> - * @outsize: Outgoing length in bytes.
> - * @insize: Max number of bytes to accept from the EC.
> - * @result: EC's response to the command (separate from communication failure).
> - * @data: Where to put the incoming data from EC and outgoing data to EC.
> - */
> -struct cros_ec_command {
> - uint32_t version;
> - uint32_t command;
> - uint32_t outsize;
> - uint32_t insize;
> - uint32_t result;
> - uint8_t data[0];
> -};
> -
> -/**
> - * struct cros_ec_device - Information about a ChromeOS EC device.
> - * @phys_name: Name of physical comms layer (e.g. 'i2c-4').
> - * @dev: Device pointer for physical comms device
> - * @was_wake_device: True if this device was set to wake the system from
> - * sleep at the last suspend.
> - * @cros_class: The class structure for this device.
> - * @cmd_readmem: Direct read of the EC memory-mapped region, if supported.
> - * @offset: Is within EC_LPC_ADDR_MEMMAP region.
> - * @bytes: Number of bytes to read. zero means "read a string" (including
> - * the trailing '\0'). At most only EC_MEMMAP_SIZE bytes can be
> - * read. Caller must ensure that the buffer is large enough for the
> - * result when reading a string.
> - * @max_request: Max size of message requested.
> - * @max_response: Max size of message response.
> - * @max_passthru: Max sice of passthru message.
> - * @proto_version: The protocol version used for this device.
> - * @priv: Private data.
> - * @irq: Interrupt to use.
> - * @id: Device id.
> - * @din: Input buffer (for data from EC). This buffer will always be
> - * dword-aligned and include enough space for up to 7 word-alignment
> - * bytes also, so we can ensure that the body of the message is always
> - * dword-aligned (64-bit). We use this alignment to keep ARM and x86
> - * happy. Probably word alignment would be OK, there might be a small
> - * performance advantage to using dword.
> - * @dout: Output buffer (for data to EC). This buffer will always be
> - * dword-aligned and include enough space for up to 7 word-alignment
> - * bytes also, so we can ensure that the body of the message is always
> - * dword-aligned (64-bit). We use this alignment to keep ARM and x86
> - * happy. Probably word alignment would be OK, there might be a small
> - * performance advantage to using dword.
> - * @din_size: Size of din buffer to allocate (zero to use static din).
> - * @dout_size: Size of dout buffer to allocate (zero to use static dout).
> - * @wake_enabled: True if this device can wake the system from sleep.
> - * @suspended: True if this device had been suspended.
> - * @cmd_xfer: Send command to EC and get response.
> - * Returns the number of bytes received if the communication
> - * succeeded, but that doesn't mean the EC was happy with the
> - * command. The caller should check msg.result for the EC's result
> - * code.
> - * @pkt_xfer: Send packet to EC and get response.
> - * @lock: One transaction at a time.
> - * @mkbp_event_supported: True if this EC supports the MKBP event protocol.
> - * @host_sleep_v1: True if this EC supports the sleep v1 command.
> - * @event_notifier: Interrupt event notifier for transport devices.
> - * @event_data: Raw payload transferred with the MKBP event.
> - * @event_size: Size in bytes of the event data.
> - * @host_event_wake_mask: Mask of host events that cause wake from suspend.
> - * @ec: The platform_device used by the mfd driver to interface with the
> - * main EC.
> - * @pd: The platform_device used by the mfd driver to interface with the
> - * PD behind an EC.
> - */
> -struct cros_ec_device {
> - /* These are used by other drivers that want to talk to the EC */
> - const char *phys_name;
> - struct device *dev;
> - bool was_wake_device;
> - struct class *cros_class;
> - int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
> - unsigned int bytes, void *dest);
> -
> - /* These are used to implement the platform-specific interface */
> - u16 max_request;
> - u16 max_response;
> - u16 max_passthru;
> - u16 proto_version;
> - void *priv;
> - int irq;
> - u8 *din;
> - u8 *dout;
> - int din_size;
> - int dout_size;
> - bool wake_enabled;
> - bool suspended;
> - int (*cmd_xfer)(struct cros_ec_device *ec,
> - struct cros_ec_command *msg);
> - int (*pkt_xfer)(struct cros_ec_device *ec,
> - struct cros_ec_command *msg);
> - struct mutex lock;
> - bool mkbp_event_supported;
> - bool host_sleep_v1;
> - struct blocking_notifier_head event_notifier;
> -
> - struct ec_response_get_next_event_v1 event_data;
> - int event_size;
> - u32 host_event_wake_mask;
> -
> - /* The platform devices used by the mfd driver */
> - struct platform_device *ec;
> - struct platform_device *pd;
> -};
> -
> -/**
> - * struct cros_ec_sensor_platform - ChromeOS EC sensor platform information.
> - * @sensor_num: Id of the sensor, as reported by the EC.
> - */
> -struct cros_ec_sensor_platform {
> - u8 sensor_num;
> -};
> -
> -/**
> - * struct cros_ec_platform - ChromeOS EC platform information.
> - * @ec_name: Name of EC device (e.g. 'cros-ec', 'cros-pd', ...)
> - * used in /dev/ and sysfs.
> - * @cmd_offset: Offset to apply for each command. Set when
> - * registering a device behind another one.
> - */
> -struct cros_ec_platform {
> - const char *ec_name;
> - u16 cmd_offset;
> -};
> -
> -struct cros_ec_debugfs;
>
> /**
> * struct cros_ec_dev - ChromeOS EC device entry point.
> @@ -217,133 +40,4 @@ struct cros_ec_dev {
>
> #define to_cros_ec_dev(dev) container_of(dev, struct cros_ec_dev, class_dev)
>
> -/**
> - * cros_ec_suspend() - Handle a suspend operation for the ChromeOS EC device.
> - * @ec_dev: Device to suspend.
> - *
> - * This can be called by drivers to handle a suspend event.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_suspend(struct cros_ec_device *ec_dev);
> -
> -/**
> - * cros_ec_resume() - Handle a resume operation for the ChromeOS EC device.
> - * @ec_dev: Device to resume.
> - *
> - * This can be called by drivers to handle a resume event.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_resume(struct cros_ec_device *ec_dev);
> -
> -/**
> - * cros_ec_prepare_tx() - Prepare an outgoing message in the output buffer.
> - * @ec_dev: Device to register.
> - * @msg: Message to write.
> - *
> - * This is intended to be used by all ChromeOS EC drivers, but at present
> - * only SPI uses it. Once LPC uses the same protocol it can start using it.
> - * I2C could use it now, with a refactor of the existing code.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
> - struct cros_ec_command *msg);
> -
> -/**
> - * cros_ec_check_result() - Check ec_msg->result.
> - * @ec_dev: EC device.
> - * @msg: Message to check.
> - *
> - * This is used by ChromeOS EC drivers to check the ec_msg->result for
> - * errors and to warn about them.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_check_result(struct cros_ec_device *ec_dev,
> - struct cros_ec_command *msg);
> -
> -/**
> - * cros_ec_cmd_xfer() - Send a command to the ChromeOS EC.
> - * @ec_dev: EC device.
> - * @msg: Message to write.
> - *
> - * Call this to send a command to the ChromeOS EC. This should be used
> - * instead of calling the EC's cmd_xfer() callback directly.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> - struct cros_ec_command *msg);
> -
> -/**
> - * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
> - * @ec_dev: EC device.
> - * @msg: Message to write.
> - *
> - * This function is identical to cros_ec_cmd_xfer, except it returns success
> - * status only if both the command was transmitted successfully and the EC
> - * replied with success status. It's not necessary to check msg->result when
> - * using this function.
> - *
> - * Return: The number of bytes transferred on success or negative error code.
> - */
> -int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> - struct cros_ec_command *msg);
> -
> -/**
> - * cros_ec_register() - Register a new ChromeOS EC, using the provided info.
> - * @ec_dev: Device to register.
> - *
> - * Before calling this, allocate a pointer to a new device and then fill
> - * in all the fields up to the --private-- marker.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_register(struct cros_ec_device *ec_dev);
> -
> -/**
> - * cros_ec_unregister() - Remove a ChromeOS EC.
> - * @ec_dev: Device to unregister.
> - *
> - * Call this to deregister a ChromeOS EC, then clean up any private data.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_unregister(struct cros_ec_device *ec_dev);
> -
> -/**
> - * cros_ec_query_all() - Query the protocol version supported by the
> - * ChromeOS EC.
> - * @ec_dev: Device to register.
> - *
> - * Return: 0 on success or negative error code.
> - */
> -int cros_ec_query_all(struct cros_ec_device *ec_dev);
> -
> -/**
> - * cros_ec_get_next_event() - Fetch next event from the ChromeOS EC.
> - * @ec_dev: Device to fetch event from.
> - * @wake_event: Pointer to a bool set to true upon return if the event might be
> - * treated as a wake event. Ignored if null.
> - *
> - * Return: negative error code on errors; 0 for no data; or else number of
> - * bytes received (i.e., an event was retrieved successfully). Event types are
> - * written out to @ec_dev->event_data.event_type on success.
> - */
> -int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
> -
> -/**
> - * cros_ec_get_host_event() - Return a mask of event set by the ChromeOS EC.
> - * @ec_dev: Device to fetch event from.
> - *
> - * When MKBP is supported, when the EC raises an interrupt, we collect the
> - * events raised and call the functions in the ec notifier. This function
> - * is a helper to know which events are raised.
> - *
> - * Return: 0 on error or non-zero bitmask of one or more EC_HOST_EVENT_*.
> - */
> -u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
> -
> #endif /* __LINUX_MFD_CROS_EC_H */
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> similarity index 100%
> rename from include/linux/mfd/cros_ec_commands.h
> rename to include/linux/platform_data/cros_ec_commands.h
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> new file mode 100644
> index 000000000000..34dd9e5c1779
> --- /dev/null
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -0,0 +1,315 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ChromeOS Embedded Controller protocol interface.
> + *
> + * Copyright (C) 2012 Google, Inc
> + */
> +
> +#ifndef __LINUX_CROS_EC_PROTO_H
> +#define __LINUX_CROS_EC_PROTO_H
> +
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +
> +#define CROS_EC_DEV_NAME "cros_ec"
> +#define CROS_EC_DEV_FP_NAME "cros_fp"
> +#define CROS_EC_DEV_ISH_NAME "cros_ish"
> +#define CROS_EC_DEV_PD_NAME "cros_pd"
> +#define CROS_EC_DEV_TP_NAME "cros_tp"
> +
> +/*
> + * The EC is unresponsive for a time after a reboot command. Add a
> + * simple delay to make sure that the bus stays locked.
> + */
> +#define EC_REBOOT_DELAY_MS 50
> +
> +/*
> + * Max bus-specific overhead incurred by request/responses.
> + * I2C requires 1 additional byte for requests.
> + * I2C requires 2 additional bytes for responses.
> + * SPI requires up to 32 additional bytes for responses.
> + */
> +#define EC_PROTO_VERSION_UNKNOWN 0
> +#define EC_MAX_REQUEST_OVERHEAD 1
> +#define EC_MAX_RESPONSE_OVERHEAD 32
> +
> +/*
> + * Command interface between EC and AP, for LPC, I2C and SPI interfaces.
> + */
> +enum {
> + EC_MSG_TX_HEADER_BYTES = 3,
> + EC_MSG_TX_TRAILER_BYTES = 1,
> + EC_MSG_TX_PROTO_BYTES = EC_MSG_TX_HEADER_BYTES +
> + EC_MSG_TX_TRAILER_BYTES,
> + EC_MSG_RX_PROTO_BYTES = 3,
> +
> + /* Max length of messages for proto 2*/
> + EC_PROTO2_MSG_BYTES = EC_PROTO2_MAX_PARAM_SIZE +
> + EC_MSG_TX_PROTO_BYTES,
> +
> + EC_MAX_MSG_BYTES = 64 * 1024,
> +};
> +
> +/**
> + * struct cros_ec_command - Information about a ChromeOS EC command.
> + * @version: Command version number (often 0).
> + * @command: Command to send (EC_CMD_...).
> + * @outsize: Outgoing length in bytes.
> + * @insize: Max number of bytes to accept from the EC.
> + * @result: EC's response to the command (separate from communication failure).
> + * @data: Where to put the incoming data from EC and outgoing data to EC.
> + */
> +struct cros_ec_command {
> + uint32_t version;
> + uint32_t command;
> + uint32_t outsize;
> + uint32_t insize;
> + uint32_t result;
> + uint8_t data[0];
> +};
> +
> +/**
> + * struct cros_ec_device - Information about a ChromeOS EC device.
> + * @phys_name: Name of physical comms layer (e.g. 'i2c-4').
> + * @dev: Device pointer for physical comms device
> + * @was_wake_device: True if this device was set to wake the system from
> + * sleep at the last suspend.
> + * @cros_class: The class structure for this device.
> + * @cmd_readmem: Direct read of the EC memory-mapped region, if supported.
> + * @offset: Is within EC_LPC_ADDR_MEMMAP region.
> + * @bytes: Number of bytes to read. zero means "read a string" (including
> + * the trailing '\0'). At most only EC_MEMMAP_SIZE bytes can be
> + * read. Caller must ensure that the buffer is large enough for the
> + * result when reading a string.
> + * @max_request: Max size of message requested.
> + * @max_response: Max size of message response.
> + * @max_passthru: Max sice of passthru message.
> + * @proto_version: The protocol version used for this device.
> + * @priv: Private data.
> + * @irq: Interrupt to use.
> + * @id: Device id.
> + * @din: Input buffer (for data from EC). This buffer will always be
> + * dword-aligned and include enough space for up to 7 word-alignment
> + * bytes also, so we can ensure that the body of the message is always
> + * dword-aligned (64-bit). We use this alignment to keep ARM and x86
> + * happy. Probably word alignment would be OK, there might be a small
> + * performance advantage to using dword.
> + * @dout: Output buffer (for data to EC). This buffer will always be
> + * dword-aligned and include enough space for up to 7 word-alignment
> + * bytes also, so we can ensure that the body of the message is always
> + * dword-aligned (64-bit). We use this alignment to keep ARM and x86
> + * happy. Probably word alignment would be OK, there might be a small
> + * performance advantage to using dword.
> + * @din_size: Size of din buffer to allocate (zero to use static din).
> + * @dout_size: Size of dout buffer to allocate (zero to use static dout).
> + * @wake_enabled: True if this device can wake the system from sleep.
> + * @suspended: True if this device had been suspended.
> + * @cmd_xfer: Send command to EC and get response.
> + * Returns the number of bytes received if the communication
> + * succeeded, but that doesn't mean the EC was happy with the
> + * command. The caller should check msg.result for the EC's result
> + * code.
> + * @pkt_xfer: Send packet to EC and get response.
> + * @lock: One transaction at a time.
> + * @mkbp_event_supported: True if this EC supports the MKBP event protocol.
> + * @host_sleep_v1: True if this EC supports the sleep v1 command.
> + * @event_notifier: Interrupt event notifier for transport devices.
> + * @event_data: Raw payload transferred with the MKBP event.
> + * @event_size: Size in bytes of the event data.
> + * @host_event_wake_mask: Mask of host events that cause wake from suspend.
> + * @ec: The platform_device used by the mfd driver to interface with the
> + * main EC.
> + * @pd: The platform_device used by the mfd driver to interface with the
> + * PD behind an EC.
> + */
> +struct cros_ec_device {
> + /* These are used by other drivers that want to talk to the EC */
> + const char *phys_name;
> + struct device *dev;
> + bool was_wake_device;
> + struct class *cros_class;
> + int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
> + unsigned int bytes, void *dest);
> +
> + /* These are used to implement the platform-specific interface */
> + u16 max_request;
> + u16 max_response;
> + u16 max_passthru;
> + u16 proto_version;
> + void *priv;
> + int irq;
> + u8 *din;
> + u8 *dout;
> + int din_size;
> + int dout_size;
> + bool wake_enabled;
> + bool suspended;
> + int (*cmd_xfer)(struct cros_ec_device *ec,
> + struct cros_ec_command *msg);
> + int (*pkt_xfer)(struct cros_ec_device *ec,
> + struct cros_ec_command *msg);
> + struct mutex lock;
> + bool mkbp_event_supported;
> + bool host_sleep_v1;
> + struct blocking_notifier_head event_notifier;
> +
> + struct ec_response_get_next_event_v1 event_data;
> + int event_size;
> + u32 host_event_wake_mask;
> +
> + /* The platform devices used by the mfd driver */
> + struct platform_device *ec;
> + struct platform_device *pd;
> +};
> +
> +/**
> + * struct cros_ec_sensor_platform - ChromeOS EC sensor platform information.
> + * @sensor_num: Id of the sensor, as reported by the EC.
> + */
> +struct cros_ec_sensor_platform {
> + u8 sensor_num;
> +};
> +
> +/**
> + * struct cros_ec_platform - ChromeOS EC platform information.
> + * @ec_name: Name of EC device (e.g. 'cros-ec', 'cros-pd', ...)
> + * used in /dev/ and sysfs.
> + * @cmd_offset: Offset to apply for each command. Set when
> + * registering a device behind another one.
> + */
> +struct cros_ec_platform {
> + const char *ec_name;
> + u16 cmd_offset;
> +};
> +
> +/**
> + * cros_ec_suspend() - Handle a suspend operation for the ChromeOS EC device.
> + * @ec_dev: Device to suspend.
> + *
> + * This can be called by drivers to handle a suspend event.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_suspend(struct cros_ec_device *ec_dev);
> +
> +/**
> + * cros_ec_resume() - Handle a resume operation for the ChromeOS EC device.
> + * @ec_dev: Device to resume.
> + *
> + * This can be called by drivers to handle a resume event.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_resume(struct cros_ec_device *ec_dev);
> +
> +/**
> + * cros_ec_prepare_tx() - Prepare an outgoing message in the output buffer.
> + * @ec_dev: Device to register.
> + * @msg: Message to write.
> + *
> + * This is intended to be used by all ChromeOS EC drivers, but at present
> + * only SPI uses it. Once LPC uses the same protocol it can start using it.
> + * I2C could use it now, with a refactor of the existing code.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
> + struct cros_ec_command *msg);
> +
> +/**
> + * cros_ec_check_result() - Check ec_msg->result.
> + * @ec_dev: EC device.
> + * @msg: Message to check.
> + *
> + * This is used by ChromeOS EC drivers to check the ec_msg->result for
> + * errors and to warn about them.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_check_result(struct cros_ec_device *ec_dev,
> + struct cros_ec_command *msg);
> +
> +/**
> + * cros_ec_cmd_xfer() - Send a command to the ChromeOS EC.
> + * @ec_dev: EC device.
> + * @msg: Message to write.
> + *
> + * Call this to send a command to the ChromeOS EC. This should be used
> + * instead of calling the EC's cmd_xfer() callback directly.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> + struct cros_ec_command *msg);
> +
> +/**
> + * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
> + * @ec_dev: EC device.
> + * @msg: Message to write.
> + *
> + * This function is identical to cros_ec_cmd_xfer, except it returns success
> + * status only if both the command was transmitted successfully and the EC
> + * replied with success status. It's not necessary to check msg->result when
> + * using this function.
> + *
> + * Return: The number of bytes transferred on success or negative error code.
> + */
> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> + struct cros_ec_command *msg);
> +
> +/**
> + * cros_ec_register() - Register a new ChromeOS EC, using the provided info.
> + * @ec_dev: Device to register.
> + *
> + * Before calling this, allocate a pointer to a new device and then fill
> + * in all the fields up to the --private-- marker.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_register(struct cros_ec_device *ec_dev);
> +
> +/**
> + * cros_ec_unregister() - Remove a ChromeOS EC.
> + * @ec_dev: Device to unregister.
> + *
> + * Call this to deregister a ChromeOS EC, then clean up any private data.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_unregister(struct cros_ec_device *ec_dev);
> +
> +/**
> + * cros_ec_query_all() - Query the protocol version supported by the
> + * ChromeOS EC.
> + * @ec_dev: Device to register.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_query_all(struct cros_ec_device *ec_dev);
> +
> +/**
> + * cros_ec_get_next_event() - Fetch next event from the ChromeOS EC.
> + * @ec_dev: Device to fetch event from.
> + * @wake_event: Pointer to a bool set to true upon return if the event might be
> + * treated as a wake event. Ignored if null.
> + *
> + * Return: negative error code on errors; 0 for no data; or else number of
> + * bytes received (i.e., an event was retrieved successfully). Event types are
> + * written out to @ec_dev->event_data.event_type on success.
> + */
> +int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
> +
> +/**
> + * cros_ec_get_host_event() - Return a mask of event set by the ChromeOS EC.
> + * @ec_dev: Device to fetch event from.
> + *
> + * When MKBP is supported, when the EC raises an interrupt, we collect the
> + * events raised and call the functions in the ec notifier. This function
> + * is a helper to know which events are raised.
> + *
> + * Return: 0 on error or non-zero bitmask of one or more EC_HOST_EVENT_*.
> + */
> +u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
> +
> +#endif /* __LINUX_CROS_EC_PROTO_H */
> diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
> index 87830ed5ebf4..79bb4081d3c2 100644
> --- a/sound/soc/codecs/cros_ec_codec.c
> +++ b/sound/soc/codecs/cros_ec_codec.c
> @@ -9,9 +9,9 @@
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> -#include <linux/mfd/cros_ec.h>
> -#include <linux/mfd/cros_ec_commands.h>
> #include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <sound/pcm.h>
> #include <sound/pcm_params.h>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH RFC 1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor
From: Hsin-Yi Wang @ 2019-06-20 9:43 UTC (permalink / raw)
To: Sibi Sankar
Cc: Chanwoo Choi,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Saravana Kannan, MyungJoo Ham, Kyungmin Park, Matthias Brugger,
linux-pm, linux-mediatek, lkml, Andrew-sh . Cheng,
linux-kernel-owner
In-Reply-To: <04bbd518efef8296e450e984e6afdba2@codeaurora.org>
Thanks!
On Thu, Jun 20, 2019 at 5:41 PM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Hsin-Yi, Chanwoo
>
> On 2019-06-20 15:02, Hsin-Yi Wang wrote:
> > Hi Chanwoo Choi, Saravana Kannan and Sibi Sankar,
> >
> > I've also tested Sibi Sankar's patch[1] locally with mt8183-cci, and
> > it works fine too!
> > It'd be great if Sibi Sankar or anyone who is familiar with the
> > original design can finish this implementation. But if no one has time
> > to do that, I think I can also help on address the comments. Thanks!
>
> Now that we have a user :) I am happy
> to repost the patch with the comments
> addressed.
>
> https://lkml.org/lkml/2019/6/14/4
> Also with ^^ patch and few more in the
> series the dt parsing of required-opps
> should get further simplified.
>
> >
> >
> > [1]
> > [RFC,2/9] OPP: Export a number of helpers to prevent code duplication
> > - https://patchwork.kernel.org/patch/10875199/
> > [RFC,3/9] PM / devfreq: Add cpu based scaling support to
> > passive_governor
> > - https://patchwork.kernel.org/patch/10875195/
> >
> > Hsin-Yi
> >
> > On Thu, Jun 20, 2019 at 2:56 PM Chanwoo Choi <cw00.choi@samsung.com>
> > wrote:
> >>
> >> + Sibi Sankar
> >>
> >> Hi, Hsin-Yi Wang, Saravana Kannan and Sibi Sankar
> >>
> >> I summarized the history of the related patch about this title.
> >>
> >> Firstly,
> >> As I knew, Saravana sent the patch[1] which contains
> >> 'governor_cpufreq_map.c' last year. According to the Myungoo's
> >> comment,
> >>
> >> Secondly,
> >> Sibi Sankar modified the 'governor_passive.c'[2] in order to support
> >> the mapping between cpu frequency and device frequency.
> >> Unfortunately, Sibi Sankar stopped the development about this
> >> because he had found the other method to get his purpose as I knew.
> >>
> >> Thirdly,
> >> Hsin-Yi Wang send the original patch of Saravana without modification.
> >>
> >>
> >> Sincerely, I think that the mapping between cpu frequency and device
> >> frequency is necessary. And I prefer the Sibi's approach which
> >> implements
> >> stuff to the existing 'passive' governor.
> >>
> >> We need to discuss about how to implement them by whom.
> >>
> >>
> >> [1] [v3,1/2] PM / devfreq: Generic CPU frequency to device frequency
> >> mapping governor
> >> - https://patchwork.kernel.org/patch/10553171/
> >>
> >> [2]
> >> [PATCH RFC 0/9] Add CPU based scaling support to Passive governor
> >> -
> >> https://lore.kernel.org/lkml/08c3cff8c39e3d82e044db93e992da72@codeaurora.org/T/
> >> [PATCH RFC 3/9] PM / devfreq: Add cpu based scaling support to
> >> passive_governor
> >> -
> >> https://lore.kernel.org/lkml/08c3cff8c39e3d82e044db93e992da72@codeaurora.org/T/#m1cafb7baf687d2a680d39c85d3ec7d1b590b68fc
> >>
> >>
> >> Best Regards,
> >> Chanwoo Choi
> >>
> >> On 19. 6. 18. 오후 1:14, Hsin-Yi Wang wrote:
> >> > From: Saravana Kannan <skannan@codeaurora.org>
> >> >
> >> > From: Saravana Kannan <skannan@codeaurora.org>
> >> >
> >> > Many CPU architectures have caches that can scale independent of the CPUs.
> >> > Frequency scaling of the caches is necessary to make sure the cache is not
> >> > a performance bottleneck that leads to poor performance and power. The same
> >> > idea applies for RAM/DDR.
> >> >
> >> > To achieve this, this patch adds a generic devfreq governor that takes the
> >> > current frequency of each CPU frequency domain and then adjusts the
> >> > frequency of the cache (or any devfreq device) based on the frequency of
> >> > the CPUs. It listens to CPU frequency transition notifiers to keep itself
> >> > up to date on the current CPU frequency.
> >> >
> >> > To decide the frequency of the device, the governor does one of the
> >> > following:
> >> >
> >> > * Uses a CPU frequency to device frequency mapping table
> >> > - Either one mapping table used for all CPU freq policies (typically used
> >> > for system with homogeneous cores/clusters that have the same OPPs).
> >> > - One mapping table per CPU freq policy (typically used for ASMP systems
> >> > with heterogeneous CPUs with different OPPs)
> >> >
> >> > OR
> >> >
> >> > * Scales the device frequency in proportion to the CPU frequency. So, if
> >> > the CPUs are running at their max frequency, the device runs at its max
> >> > frequency. If the CPUs are running at their min frequency, the device
> >> > runs at its min frequency. And interpolated for frequencies in between.
> >> >
> >> > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> >> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> >> > ---
> >> > .../bindings/devfreq/devfreq-cpufreq-map.txt | 53 ++
> >> > drivers/devfreq/Kconfig | 8 +
> >> > drivers/devfreq/Makefile | 1 +
> >> > drivers/devfreq/governor_cpufreq_map.c | 583 ++++++++++++++++++
> >> > 4 files changed, 645 insertions(+)
> >> > create mode 100644 Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> >> > create mode 100644 drivers/devfreq/governor_cpufreq_map.c
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> >> > new file mode 100644
> >> > index 000000000000..982a30bcfc86
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> >> > @@ -0,0 +1,53 @@
> >> > +Devfreq CPUfreq governor
> >> > +
> >> > +devfreq-cpufreq-map is a parent device that contains one or more child devices.
> >> > +Each child device provides CPU frequency to device frequency mapping for a
> >> > +specific device. Examples of devices that could use this are: DDR, cache and
> >> > +CCI.
> >> > +
> >> > +Parent device name shall be "devfreq-cpufreq-map".
> >> > +
> >> > +Required child device properties:
> >> > +- cpu-to-dev-map, or cpu-to-dev-map-<X>:
> >> > + A list of tuples where each tuple consists of a
> >> > + CPU frequency (KHz) and the corresponding device
> >> > + frequency. CPU frequencies not listed in the table
> >> > + will use the device frequency that corresponds to the
> >> > + next rounded up CPU frequency.
> >> > + Use "cpu-to-dev-map" if all CPUs in the system should
> >> > + share same mapping.
> >> > + Use cpu-to-dev-map-<cpuid> to describe different
> >> > + mappings for different CPUs. The property should be
> >> > + listed only for the first CPU if multiple CPUs are
> >> > + synchronous.
> >> > +- target-dev: Phandle to device that this mapping applies to.
> >> > +
> >> > +Example:
> >> > + devfreq-cpufreq-map {
> >> > + cpubw-cpufreq {
> >> > + target-dev = <&cpubw>;
> >> > + cpu-to-dev-map =
> >> > + < 300000 1144000 >,
> >> > + < 422400 2288000 >,
> >> > + < 652800 3051000 >,
> >> > + < 883200 5996000 >,
> >> > + < 1190400 8056000 >,
> >> > + < 1497600 10101000 >,
> >> > + < 1728000 12145000 >,
> >> > + < 2649600 16250000 >;
> >> > + };
> >> > +
> >> > + cache-cpufreq {
> >> > + target-dev = <&cache>;
> >> > + cpu-to-dev-map =
> >> > + < 300000 300000 >,
> >> > + < 422400 422400 >,
> >> > + < 652800 499200 >,
> >> > + < 883200 576000 >,
> >> > + < 960000 960000 >,
> >> > + < 1497600 1036800 >,
> >> > + < 1574400 1574400 >,
> >> > + < 1728000 1651200 >,
> >> > + < 2649600 1728000 >;
> >> > + };
> >> > + };
> >> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> >> > index 0c8204d6b78a..0303f5a400b6 100644
> >> > --- a/drivers/devfreq/Kconfig
> >> > +++ b/drivers/devfreq/Kconfig
> >> > @@ -74,6 +74,14 @@ config DEVFREQ_GOV_PASSIVE
> >> > through sysfs entries. The passive governor recommends that
> >> > devfreq device uses the OPP table to get the frequency/voltage.
> >> >
> >> > +config DEVFREQ_GOV_CPUFREQ_MAP
> >> > + tristate "CPUfreq Map"
> >> > + depends on CPU_FREQ
> >> > + help
> >> > + Chooses frequency based on the online CPUs' current frequency and a
> >> > + CPU frequency to device frequency mapping table(s). This governor
> >> > + can be useful for controlling devices such as DDR, cache, CCI, etc.
> >> > +
> >> > comment "DEVFREQ Drivers"
> >> >
> >> > config ARM_EXYNOS_BUS_DEVFREQ
> >> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> >> > index 817dde779f16..81141e2c784f 100644
> >> > --- a/drivers/devfreq/Makefile
> >> > +++ b/drivers/devfreq/Makefile
> >> > @@ -6,6 +6,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE) += governor_performance.o
> >> > obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
> >> > obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
> >> > obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
> >> > +obj-$(CONFIG_DEVFREQ_GOV_CPUFREQ_MAP) += governor_cpufreq_map.o
> >> >
> >> > # DEVFREQ Drivers
> >> > obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> >> > diff --git a/drivers/devfreq/governor_cpufreq_map.c b/drivers/devfreq/governor_cpufreq_map.c
> >> > new file mode 100644
> >> > index 000000000000..084a3ffb8f54
> >> > --- /dev/null
> >> > +++ b/drivers/devfreq/governor_cpufreq_map.c
> >> > @@ -0,0 +1,583 @@
> >> > +// SPDX-License-Identifier: GPL-2.0
> >> > +/*
> >> > + * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights reserved.
> >> > + */
> >> > +
> >> > +#define pr_fmt(fmt) "dev-cpufreq-map: " fmt
> >> > +
> >> > +#include <linux/devfreq.h>
> >> > +#include <linux/cpu.h>
> >> > +#include <linux/cpufreq.h>
> >> > +#include <linux/cpumask.h>
> >> > +#include <linux/slab.h>
> >> > +#include <linux/platform_device.h>
> >> > +#include <linux/of.h>
> >> > +#include <linux/module.h>
> >> > +#include "governor.h"
> >> > +
> >> > +struct cpu_state {
> >> > + unsigned int freq;
> >> > + unsigned int min_freq;
> >> > + unsigned int max_freq;
> >> > + unsigned int first_cpu;
> >> > +};
> >> > +static struct cpu_state *state[NR_CPUS];
> >> > +static int cpufreq_cnt;
> >> > +
> >> > +struct freq_map {
> >> > + unsigned int cpu_khz;
> >> > + unsigned int target_freq;
> >> > +};
> >> > +
> >> > +struct devfreq_node {
> >> > + struct devfreq *df;
> >> > + void *orig_data;
> >> > + struct device *dev;
> >> > + struct device_node *of_node;
> >> > + struct list_head list;
> >> > + struct freq_map **map;
> >> > + struct freq_map *common_map;
> >> > +};
> >> > +static LIST_HEAD(devfreq_list);
> >> > +static DEFINE_MUTEX(state_lock);
> >> > +static DEFINE_MUTEX(cpufreq_reg_lock);
> >> > +
> >> > +static void update_all_devfreqs(void)
> >> > +{
> >> > + struct devfreq_node *node;
> >> > +
> >> > + list_for_each_entry(node, &devfreq_list, list) {
> >> > + struct devfreq *df = node->df;
> >> > +
> >> > + if (!node->df)
> >> > + continue;
> >> > + mutex_lock(&df->lock);
> >> > + update_devfreq(df);
> >> > + mutex_unlock(&df->lock);
> >> > +
> >> > + }
> >> > +}
> >> > +
> >> > +static struct devfreq_node *find_devfreq_node(struct device *dev)
> >> > +{
> >> > + struct devfreq_node *node;
> >> > +
> >> > + list_for_each_entry(node, &devfreq_list, list)
> >> > + if (node->dev == dev || node->of_node == dev->of_node)
> >> > + return node;
> >> > +
> >> > + return NULL;
> >> > +}
> >> > +
> >> > +/* ==================== cpufreq part ==================== */
> >> > +static struct cpu_state *add_policy(struct cpufreq_policy *policy)
> >> > +{
> >> > + struct cpu_state *new_state;
> >> > + unsigned int cpu, first_cpu;
> >> > +
> >> > + new_state = kzalloc(sizeof(struct cpu_state), GFP_KERNEL);
> >> > + if (!new_state)
> >> > + return NULL;
> >> > +
> >> > + first_cpu = cpumask_first(policy->related_cpus);
> >> > + new_state->first_cpu = first_cpu;
> >> > + new_state->freq = policy->cur;
> >> > + new_state->min_freq = policy->cpuinfo.min_freq;
> >> > + new_state->max_freq = policy->cpuinfo.max_freq;
> >> > +
> >> > + for_each_cpu(cpu, policy->related_cpus)
> >> > + state[cpu] = new_state;
> >> > +
> >> > + return new_state;
> >> > +}
> >> > +
> >> > +static int cpufreq_trans_notifier(struct notifier_block *nb,
> >> > + unsigned long event, void *data)
> >> > +{
> >> > + struct cpufreq_freqs *freq = data;
> >> > + struct cpu_state *s;
> >> > + struct cpufreq_policy *policy = NULL;
> >> > +
> >> > + if (event != CPUFREQ_POSTCHANGE)
> >> > + return 0;
> >> > +
> >> > + mutex_lock(&state_lock);
> >> > +
> >> > + s = state[freq->cpu];
> >> > + if (!s) {
> >> > + policy = cpufreq_cpu_get(freq->cpu);
> >> > + if (policy) {
> >> > + s = add_policy(policy);
> >> > + cpufreq_cpu_put(policy);
> >> > + }
> >> > + }
> >> > + if (!s)
> >> > + goto out;
> >> > +
> >> > + if (s->freq != freq->new || policy) {
> >> > + s->freq = freq->new;
> >> > + update_all_devfreqs();
> >> > + }
> >> > +
> >> > +out:
> >> > + mutex_unlock(&state_lock);
> >> > + return 0;
> >> > +}
> >> > +
> >> > +static struct notifier_block cpufreq_trans_nb = {
> >> > + .notifier_call = cpufreq_trans_notifier
> >> > +};
> >> > +
> >> > +static int register_cpufreq(void)
> >> > +{
> >> > + int ret = 0;
> >> > + unsigned int cpu;
> >> > + struct cpufreq_policy *policy;
> >> > +
> >> > + mutex_lock(&cpufreq_reg_lock);
> >> > +
> >> > + if (cpufreq_cnt)
> >> > + goto cnt_not_zero;
> >> > +
> >> > + get_online_cpus();
> >> > + ret = cpufreq_register_notifier(&cpufreq_trans_nb,
> >> > + CPUFREQ_TRANSITION_NOTIFIER);
> >> > + if (ret)
> >> > + goto out;
> >> > +
> >> > + for_each_online_cpu(cpu) {
> >> > + policy = cpufreq_cpu_get(cpu);
> >> > + if (policy) {
> >> > + add_policy(policy);
> >> > + cpufreq_cpu_put(policy);
> >> > + }
> >> > + }
> >> > +out:
> >> > + put_online_cpus();
> >> > +cnt_not_zero:
> >> > + if (!ret)
> >> > + cpufreq_cnt++;
> >> > + mutex_unlock(&cpufreq_reg_lock);
> >> > + return ret;
> >> > +}
> >> > +
> >> > +static int unregister_cpufreq(void)
> >> > +{
> >> > + int ret = 0;
> >> > + int cpu;
> >> > +
> >> > + mutex_lock(&cpufreq_reg_lock);
> >> > +
> >> > + if (cpufreq_cnt > 1)
> >> > + goto out;
> >> > +
> >> > + cpufreq_unregister_notifier(&cpufreq_trans_nb,
> >> > + CPUFREQ_TRANSITION_NOTIFIER);
> >> > +
> >> > + for (cpu = ARRAY_SIZE(state) - 1; cpu >= 0; cpu--) {
> >> > + if (!state[cpu])
> >> > + continue;
> >> > + if (state[cpu]->first_cpu == cpu)
> >> > + kfree(state[cpu]);
> >> > + state[cpu] = NULL;
> >> > + }
> >> > +
> >> > +out:
> >> > + cpufreq_cnt--;
> >> > + mutex_unlock(&cpufreq_reg_lock);
> >> > + return ret;
> >> > +}
> >> > +
> >> > +/* ==================== devfreq part ==================== */
> >> > +
> >> > +static unsigned int interpolate_freq(struct devfreq *df, unsigned int cpu)
> >> > +{
> >> > + unsigned long *freq_table = df->profile->freq_table;
> >> > + unsigned int cpu_min = state[cpu]->min_freq;
> >> > + unsigned int cpu_max = state[cpu]->max_freq;
> >> > + unsigned int cpu_freq = state[cpu]->freq;
> >> > + unsigned int dev_min, dev_max, cpu_percent;
> >> > +
> >> > + if (freq_table) {
> >> > + dev_min = freq_table[0];
> >> > + dev_max = freq_table[df->profile->max_state - 1];
> >> > + } else {
> >> > + if (df->max_freq <= df->min_freq)
> >> > + return 0;
> >> > + dev_min = df->min_freq;
> >> > + dev_max = df->max_freq;
> >> > + }
> >> > +
> >> > + cpu_percent = ((cpu_freq - cpu_min) * 100) / (cpu_max - cpu_min);
> >> > + return dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> >> > +}
> >> > +
> >> > +static unsigned int cpu_to_dev_freq(struct devfreq *df, unsigned int cpu)
> >> > +{
> >> > + struct freq_map *map = NULL;
> >> > + unsigned int cpu_khz = 0, freq;
> >> > + struct devfreq_node *n = df->data;
> >> > +
> >> > + if (!state[cpu] || state[cpu]->first_cpu != cpu) {
> >> > + freq = 0;
> >> > + goto out;
> >> > + }
> >> > +
> >> > + if (n->common_map)
> >> > + map = n->common_map;
> >> > + else if (n->map)
> >> > + map = n->map[cpu];
> >> > +
> >> > + cpu_khz = state[cpu]->freq;
> >> > +
> >> > + if (!map) {
> >> > + freq = interpolate_freq(df, cpu);
> >> > + goto out;
> >> > + }
> >> > +
> >> > + while (map->cpu_khz && map->cpu_khz < cpu_khz)
> >> > + map++;
> >> > + if (!map->cpu_khz)
> >> > + map--;
> >> > + freq = map->target_freq;
> >> > +
> >> > +out:
> >> > + dev_dbg(df->dev.parent, "CPU%u: %d -> dev: %u\n", cpu, cpu_khz, freq);
> >> > + return freq;
> >> > +}
> >> > +
> >> > +static int devfreq_cpufreq_get_freq(struct devfreq *df,
> >> > + unsigned long *freq)
> >> > +{
> >> > + unsigned int cpu, tgt_freq = 0;
> >> > + struct devfreq_node *node;
> >> > +
> >> > + node = df->data;
> >> > + if (!node) {
> >> > + pr_err("Unable to find devfreq node!\n");
> >> > + return -ENODEV;
> >> > + }
> >> > +
> >> > + for_each_possible_cpu(cpu)
> >> > + tgt_freq = max(tgt_freq, cpu_to_dev_freq(df, cpu));
> >> > +
> >> > + *freq = tgt_freq;
> >> > + return 0;
> >> > +}
> >> > +
> >> > +static unsigned int show_table(char *buf, unsigned int len,
> >> > + struct freq_map *map)
> >> > +{
> >> > + unsigned int cnt = 0;
> >> > +
> >> > + cnt += snprintf(buf + cnt, len - cnt, "CPU freq\tDevice freq\n");
> >> > +
> >> > + while (map->cpu_khz && cnt < len) {
> >> > + cnt += snprintf(buf + cnt, len - cnt, "%8u\t%11u\n",
> >> > + map->cpu_khz, map->target_freq);
> >> > + map++;
> >> > + }
> >> > + if (cnt < len)
> >> > + cnt += snprintf(buf + cnt, len - cnt, "\n");
> >> > +
> >> > + return cnt;
> >> > +}
> >> > +
> >> > +static ssize_t freq_map_show(struct device *dev, struct device_attribute *attr,
> >> > + char *buf)
> >> > +{
> >> > + struct devfreq *df = to_devfreq(dev);
> >> > + struct devfreq_node *n = df->data;
> >> > + struct freq_map *map;
> >> > + unsigned int cnt = 0, cpu;
> >> > +
> >> > + mutex_lock(&state_lock);
> >> > + if (n->common_map) {
> >> > + map = n->common_map;
> >> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> >> > + "Common table for all CPUs:\n");
> >> > + cnt += show_table(buf + cnt, PAGE_SIZE - cnt, map);
> >> > + } else if (n->map) {
> >> > + for_each_possible_cpu(cpu) {
> >> > + map = n->map[cpu];
> >> > + if (!map)
> >> > + continue;
> >> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> >> > + "CPU %u:\n", cpu);
> >> > + if (cnt >= PAGE_SIZE)
> >> > + break;
> >> > + cnt += show_table(buf + cnt, PAGE_SIZE - cnt, map);
> >> > + if (cnt >= PAGE_SIZE)
> >> > + break;
> >> > + }
> >> > + } else {
> >> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> >> > + "Device freq interpolated based on CPU freq\n");
> >> > + }
> >> > + mutex_unlock(&state_lock);
> >> > +
> >> > + return cnt;
> >> > +}
> >> > +
> >> > +static DEVICE_ATTR_RO(freq_map);
> >> > +static struct attribute *dev_attr[] = {
> >> > + &dev_attr_freq_map.attr,
> >> > + NULL,
> >> > +};
> >> > +
> >> > +static struct attribute_group dev_attr_group = {
> >> > + .name = "cpufreq-map",
> >> > + .attrs = dev_attr,
> >> > +};
> >> > +
> >> > +static int devfreq_cpufreq_gov_start(struct devfreq *devfreq)
> >> > +{
> >> > + int ret = 0;
> >> > + struct devfreq_node *node;
> >> > + bool alloc = false;
> >> > +
> >> > + ret = register_cpufreq();
> >> > + if (ret)
> >> > + return ret;
> >> > +
> >> > + ret = sysfs_create_group(&devfreq->dev.kobj, &dev_attr_group);
> >> > + if (ret) {
> >> > + unregister_cpufreq();
> >> > + return ret;
> >> > + }
> >> > +
> >> > + mutex_lock(&state_lock);
> >> > +
> >> > + node = find_devfreq_node(devfreq->dev.parent);
> >> > + if (node == NULL) {
> >> > + node = kzalloc(sizeof(struct devfreq_node), GFP_KERNEL);
> >> > + if (!node) {
> >> > + ret = -ENOMEM;
> >> > + goto alloc_fail;
> >> > + }
> >> > + alloc = true;
> >> > + node->dev = devfreq->dev.parent;
> >> > + list_add_tail(&node->list, &devfreq_list);
> >> > + }
> >> > + node->df = devfreq;
> >> > + node->orig_data = devfreq->data;
> >> > + devfreq->data = node;
> >> > +
> >> > + mutex_lock(&devfreq->lock);
> >> > + ret = update_devfreq(devfreq);
> >> > + mutex_unlock(&devfreq->lock);
> >> > + if (ret) {
> >> > + pr_err("Freq update failed!\n");
> >> > + goto update_fail;
> >> > + }
> >> > +
> >> > + mutex_unlock(&state_lock);
> >> > + return 0;
> >> > +
> >> > +update_fail:
> >> > + devfreq->data = node->orig_data;
> >> > + if (alloc) {
> >> > + list_del(&node->list);
> >> > + kfree(node);
> >> > + }
> >> > +alloc_fail:
> >> > + mutex_unlock(&state_lock);
> >> > + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
> >> > + unregister_cpufreq();
> >> > + return ret;
> >> > +}
> >> > +
> >> > +static void devfreq_cpufreq_gov_stop(struct devfreq *devfreq)
> >> > +{
> >> > + struct devfreq_node *node = devfreq->data;
> >> > +
> >> > + mutex_lock(&state_lock);
> >> > + devfreq->data = node->orig_data;
> >> > + if (node->map || node->common_map) {
> >> > + node->df = NULL;
> >> > + } else {
> >> > + list_del(&node->list);
> >> > + kfree(node);
> >> > + }
> >> > + mutex_unlock(&state_lock);
> >> > +
> >> > + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
> >> > + unregister_cpufreq();
> >> > +}
> >> > +
> >> > +static int devfreq_cpufreq_ev_handler(struct devfreq *devfreq,
> >> > + unsigned int event, void *data)
> >> > +{
> >> > + int ret;
> >> > +
> >> > + switch (event) {
> >> > + case DEVFREQ_GOV_START:
> >> > +
> >> > + ret = devfreq_cpufreq_gov_start(devfreq);
> >> > + if (ret) {
> >> > + pr_err("Governor start failed!\n");
> >> > + return ret;
> >> > + }
> >> > + pr_debug("Enabled CPUfreq-map governor\n");
> >> > + break;
> >> > +
> >> > + case DEVFREQ_GOV_STOP:
> >> > +
> >> > + devfreq_cpufreq_gov_stop(devfreq);
> >> > + pr_debug("Disabled dev CPUfreq-map governor\n");
> >> > + break;
> >> > + }
> >> > +
> >> > + return 0;
> >> > +}
> >> > +
> >> > +static struct devfreq_governor devfreq_cpufreq = {
> >> > + .name = "cpufreq-map",
> >> > + .get_target_freq = devfreq_cpufreq_get_freq,
> >> > + .event_handler = devfreq_cpufreq_ev_handler,
> >> > +};
> >> > +
> >> > +#define NUM_COLS 2
> >> > +static struct freq_map *read_tbl(struct device_node *of_node, char *prop_name)
> >> > +{
> >> > + int len, nf, i, j;
> >> > + u32 data;
> >> > + struct freq_map *tbl;
> >> > +
> >> > + if (!of_find_property(of_node, prop_name, &len))
> >> > + return NULL;
> >> > + len /= sizeof(data);
> >> > +
> >> > + if (len % NUM_COLS || len == 0)
> >> > + return NULL;
> >> > + nf = len / NUM_COLS;
> >> > +
> >> > + tbl = kzalloc((nf + 1) * sizeof(*tbl), GFP_KERNEL);
> >> > + if (!tbl)
> >> > + return NULL;
> >> > +
> >> > + for (i = 0, j = 0; i < nf; i++, j += 2) {
> >> > + of_property_read_u32_index(of_node, prop_name, j, &data);
> >> > + tbl[i].cpu_khz = data;
> >> > +
> >> > + of_property_read_u32_index(of_node, prop_name, j + 1, &data);
> >> > + tbl[i].target_freq = data;
> >> > + }
> >> > + tbl[i].cpu_khz = 0;
> >> > +
> >> > + return tbl;
> >> > +}
> >> > +
> >> > +#define PROP_TARGET "target-dev"
> >> > +#define PROP_TABLE "cpu-to-dev-map"
> >> > +static int add_table_from_of(struct device_node *of_node)
> >> > +{
> >> > + struct device_node *target_of_node;
> >> > + struct devfreq_node *node;
> >> > + struct freq_map *common_tbl;
> >> > + struct freq_map **tbl_list = NULL;
> >> > + static char prop_name[] = PROP_TABLE "-999999";
> >> > + int cpu, ret, cnt = 0, prop_sz = ARRAY_SIZE(prop_name);
> >> > +
> >> > + target_of_node = of_parse_phandle(of_node, PROP_TARGET, 0);
> >> > + if (!target_of_node)
> >> > + return -EINVAL;
> >> > +
> >> > + node = kzalloc(sizeof(struct devfreq_node), GFP_KERNEL);
> >> > + if (!node)
> >> > + return -ENOMEM;
> >> > +
> >> > + common_tbl = read_tbl(of_node, PROP_TABLE);
> >> > + if (!common_tbl) {
> >> > + tbl_list = kzalloc(sizeof(*tbl_list) * NR_CPUS, GFP_KERNEL);
> >> > + if (!tbl_list) {
> >> > + ret = -ENOMEM;
> >> > + goto err_list;
> >> > + }
> >> > +
> >> > + for_each_possible_cpu(cpu) {
> >> > + ret = snprintf(prop_name, prop_sz, "%s-%d",
> >> > + PROP_TABLE, cpu);
> >> > + if (ret >= prop_sz) {
> >> > + pr_warn("More CPUs than I can handle!\n");
> >> > + pr_warn("Skipping rest of the tables!\n");
> >> > + break;
> >> > + }
> >> > + tbl_list[cpu] = read_tbl(of_node, prop_name);
> >> > + if (tbl_list[cpu])
> >> > + cnt++;
> >> > + }
> >> > + }
> >> > + if (!common_tbl && !cnt) {
> >> > + ret = -EINVAL;
> >> > + goto err_tbl;
> >> > + }
> >> > +
> >> > + mutex_lock(&state_lock);
> >> > + node->of_node = target_of_node;
> >> > + node->map = tbl_list;
> >> > + node->common_map = common_tbl;
> >> > + list_add_tail(&node->list, &devfreq_list);
> >> > + mutex_unlock(&state_lock);
> >> > +
> >> > + return 0;
> >> > +err_tbl:
> >> > + kfree(tbl_list);
> >> > +err_list:
> >> > + kfree(node);
> >> > + return ret;
> >> > +}
> >> > +
> >> > +static int __init devfreq_cpufreq_init(void)
> >> > +{
> >> > + int ret;
> >> > + struct device_node *of_par, *of_child;
> >> > +
> >> > + of_par = of_find_node_by_name(NULL, "devfreq-cpufreq-map");
> >> > + if (of_par) {
> >> > + for_each_child_of_node(of_par, of_child) {
> >> > + ret = add_table_from_of(of_child);
> >> > + if (ret)
> >> > + pr_err("Parsing %s failed!\n", of_child->name);
> >> > + else
> >> > + pr_debug("Parsed %s.\n", of_child->name);
> >> > + }
> >> > + of_node_put(of_par);
> >> > + } else {
> >> > + pr_info("No tables parsed from DT.\n");
> >> > + }
> >> > +
> >> > + ret = devfreq_add_governor(&devfreq_cpufreq);
> >> > + if (ret) {
> >> > + pr_err("cpufreq-map governor add failed!\n");
> >> > + return ret;
> >> > + }
> >> > +
> >> > + return 0;
> >> > +}
> >> > +subsys_initcall(devfreq_cpufreq_init);
> >> > +
> >> > +static void __exit devfreq_cpufreq_exit(void)
> >> > +{
> >> > + int ret, cpu;
> >> > + struct devfreq_node *node, *tmp;
> >> > +
> >> > + ret = devfreq_remove_governor(&devfreq_cpufreq);
> >> > + if (ret)
> >> > + pr_err("cpufreq-map governor remove failed!\n");
> >> > +
> >> > + mutex_lock(&state_lock);
> >> > + list_for_each_entry_safe(node, tmp, &devfreq_list, list) {
> >> > + kfree(node->common_map);
> >> > + for_each_possible_cpu(cpu)
> >> > + kfree(node->map[cpu]);
> >> > + kfree(node->map);
> >> > + list_del(&node->list);
> >> > + kfree(node);
> >> > + }
> >> > + mutex_unlock(&state_lock);
> >> > +}
> >> > +module_exit(devfreq_cpufreq_exit);
> >> > +
> >> > +MODULE_DESCRIPTION("devfreq gov that sets dev freq based on current CPU freq");
> >> > +MODULE_LICENSE("GPL v2");
> >> >
>
> --
> -- Sibi Sankar --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH RFC 1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor
From: Sibi Sankar @ 2019-06-20 9:41 UTC (permalink / raw)
To: Hsin-Yi Wang
Cc: Chanwoo Choi,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Saravana Kannan, MyungJoo Ham, Kyungmin Park, Matthias Brugger,
linux-pm, linux-mediatek, lkml, Andrew-sh . Cheng,
linux-kernel-owner
In-Reply-To: <CAJMQK-jELbhCUb5LYOnGMsBHjhZHnXMYQYDxWyEmriJf8OSpSQ@mail.gmail.com>
Hey Hsin-Yi, Chanwoo
On 2019-06-20 15:02, Hsin-Yi Wang wrote:
> Hi Chanwoo Choi, Saravana Kannan and Sibi Sankar,
>
> I've also tested Sibi Sankar's patch[1] locally with mt8183-cci, and
> it works fine too!
> It'd be great if Sibi Sankar or anyone who is familiar with the
> original design can finish this implementation. But if no one has time
> to do that, I think I can also help on address the comments. Thanks!
Now that we have a user :) I am happy
to repost the patch with the comments
addressed.
https://lkml.org/lkml/2019/6/14/4
Also with ^^ patch and few more in the
series the dt parsing of required-opps
should get further simplified.
>
>
> [1]
> [RFC,2/9] OPP: Export a number of helpers to prevent code duplication
> - https://patchwork.kernel.org/patch/10875199/
> [RFC,3/9] PM / devfreq: Add cpu based scaling support to
> passive_governor
> - https://patchwork.kernel.org/patch/10875195/
>
> Hsin-Yi
>
> On Thu, Jun 20, 2019 at 2:56 PM Chanwoo Choi <cw00.choi@samsung.com>
> wrote:
>>
>> + Sibi Sankar
>>
>> Hi, Hsin-Yi Wang, Saravana Kannan and Sibi Sankar
>>
>> I summarized the history of the related patch about this title.
>>
>> Firstly,
>> As I knew, Saravana sent the patch[1] which contains
>> 'governor_cpufreq_map.c' last year. According to the Myungoo's
>> comment,
>>
>> Secondly,
>> Sibi Sankar modified the 'governor_passive.c'[2] in order to support
>> the mapping between cpu frequency and device frequency.
>> Unfortunately, Sibi Sankar stopped the development about this
>> because he had found the other method to get his purpose as I knew.
>>
>> Thirdly,
>> Hsin-Yi Wang send the original patch of Saravana without modification.
>>
>>
>> Sincerely, I think that the mapping between cpu frequency and device
>> frequency is necessary. And I prefer the Sibi's approach which
>> implements
>> stuff to the existing 'passive' governor.
>>
>> We need to discuss about how to implement them by whom.
>>
>>
>> [1] [v3,1/2] PM / devfreq: Generic CPU frequency to device frequency
>> mapping governor
>> - https://patchwork.kernel.org/patch/10553171/
>>
>> [2]
>> [PATCH RFC 0/9] Add CPU based scaling support to Passive governor
>> -
>> https://lore.kernel.org/lkml/08c3cff8c39e3d82e044db93e992da72@codeaurora.org/T/
>> [PATCH RFC 3/9] PM / devfreq: Add cpu based scaling support to
>> passive_governor
>> -
>> https://lore.kernel.org/lkml/08c3cff8c39e3d82e044db93e992da72@codeaurora.org/T/#m1cafb7baf687d2a680d39c85d3ec7d1b590b68fc
>>
>>
>> Best Regards,
>> Chanwoo Choi
>>
>> On 19. 6. 18. 오후 1:14, Hsin-Yi Wang wrote:
>> > From: Saravana Kannan <skannan@codeaurora.org>
>> >
>> > From: Saravana Kannan <skannan@codeaurora.org>
>> >
>> > Many CPU architectures have caches that can scale independent of the CPUs.
>> > Frequency scaling of the caches is necessary to make sure the cache is not
>> > a performance bottleneck that leads to poor performance and power. The same
>> > idea applies for RAM/DDR.
>> >
>> > To achieve this, this patch adds a generic devfreq governor that takes the
>> > current frequency of each CPU frequency domain and then adjusts the
>> > frequency of the cache (or any devfreq device) based on the frequency of
>> > the CPUs. It listens to CPU frequency transition notifiers to keep itself
>> > up to date on the current CPU frequency.
>> >
>> > To decide the frequency of the device, the governor does one of the
>> > following:
>> >
>> > * Uses a CPU frequency to device frequency mapping table
>> > - Either one mapping table used for all CPU freq policies (typically used
>> > for system with homogeneous cores/clusters that have the same OPPs).
>> > - One mapping table per CPU freq policy (typically used for ASMP systems
>> > with heterogeneous CPUs with different OPPs)
>> >
>> > OR
>> >
>> > * Scales the device frequency in proportion to the CPU frequency. So, if
>> > the CPUs are running at their max frequency, the device runs at its max
>> > frequency. If the CPUs are running at their min frequency, the device
>> > runs at its min frequency. And interpolated for frequencies in between.
>> >
>> > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
>> > ---
>> > .../bindings/devfreq/devfreq-cpufreq-map.txt | 53 ++
>> > drivers/devfreq/Kconfig | 8 +
>> > drivers/devfreq/Makefile | 1 +
>> > drivers/devfreq/governor_cpufreq_map.c | 583 ++++++++++++++++++
>> > 4 files changed, 645 insertions(+)
>> > create mode 100644 Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
>> > create mode 100644 drivers/devfreq/governor_cpufreq_map.c
>> >
>> > diff --git a/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
>> > new file mode 100644
>> > index 000000000000..982a30bcfc86
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
>> > @@ -0,0 +1,53 @@
>> > +Devfreq CPUfreq governor
>> > +
>> > +devfreq-cpufreq-map is a parent device that contains one or more child devices.
>> > +Each child device provides CPU frequency to device frequency mapping for a
>> > +specific device. Examples of devices that could use this are: DDR, cache and
>> > +CCI.
>> > +
>> > +Parent device name shall be "devfreq-cpufreq-map".
>> > +
>> > +Required child device properties:
>> > +- cpu-to-dev-map, or cpu-to-dev-map-<X>:
>> > + A list of tuples where each tuple consists of a
>> > + CPU frequency (KHz) and the corresponding device
>> > + frequency. CPU frequencies not listed in the table
>> > + will use the device frequency that corresponds to the
>> > + next rounded up CPU frequency.
>> > + Use "cpu-to-dev-map" if all CPUs in the system should
>> > + share same mapping.
>> > + Use cpu-to-dev-map-<cpuid> to describe different
>> > + mappings for different CPUs. The property should be
>> > + listed only for the first CPU if multiple CPUs are
>> > + synchronous.
>> > +- target-dev: Phandle to device that this mapping applies to.
>> > +
>> > +Example:
>> > + devfreq-cpufreq-map {
>> > + cpubw-cpufreq {
>> > + target-dev = <&cpubw>;
>> > + cpu-to-dev-map =
>> > + < 300000 1144000 >,
>> > + < 422400 2288000 >,
>> > + < 652800 3051000 >,
>> > + < 883200 5996000 >,
>> > + < 1190400 8056000 >,
>> > + < 1497600 10101000 >,
>> > + < 1728000 12145000 >,
>> > + < 2649600 16250000 >;
>> > + };
>> > +
>> > + cache-cpufreq {
>> > + target-dev = <&cache>;
>> > + cpu-to-dev-map =
>> > + < 300000 300000 >,
>> > + < 422400 422400 >,
>> > + < 652800 499200 >,
>> > + < 883200 576000 >,
>> > + < 960000 960000 >,
>> > + < 1497600 1036800 >,
>> > + < 1574400 1574400 >,
>> > + < 1728000 1651200 >,
>> > + < 2649600 1728000 >;
>> > + };
>> > + };
>> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> > index 0c8204d6b78a..0303f5a400b6 100644
>> > --- a/drivers/devfreq/Kconfig
>> > +++ b/drivers/devfreq/Kconfig
>> > @@ -74,6 +74,14 @@ config DEVFREQ_GOV_PASSIVE
>> > through sysfs entries. The passive governor recommends that
>> > devfreq device uses the OPP table to get the frequency/voltage.
>> >
>> > +config DEVFREQ_GOV_CPUFREQ_MAP
>> > + tristate "CPUfreq Map"
>> > + depends on CPU_FREQ
>> > + help
>> > + Chooses frequency based on the online CPUs' current frequency and a
>> > + CPU frequency to device frequency mapping table(s). This governor
>> > + can be useful for controlling devices such as DDR, cache, CCI, etc.
>> > +
>> > comment "DEVFREQ Drivers"
>> >
>> > config ARM_EXYNOS_BUS_DEVFREQ
>> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>> > index 817dde779f16..81141e2c784f 100644
>> > --- a/drivers/devfreq/Makefile
>> > +++ b/drivers/devfreq/Makefile
>> > @@ -6,6 +6,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE) += governor_performance.o
>> > obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
>> > obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
>> > obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
>> > +obj-$(CONFIG_DEVFREQ_GOV_CPUFREQ_MAP) += governor_cpufreq_map.o
>> >
>> > # DEVFREQ Drivers
>> > obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
>> > diff --git a/drivers/devfreq/governor_cpufreq_map.c b/drivers/devfreq/governor_cpufreq_map.c
>> > new file mode 100644
>> > index 000000000000..084a3ffb8f54
>> > --- /dev/null
>> > +++ b/drivers/devfreq/governor_cpufreq_map.c
>> > @@ -0,0 +1,583 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +/*
>> > + * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights reserved.
>> > + */
>> > +
>> > +#define pr_fmt(fmt) "dev-cpufreq-map: " fmt
>> > +
>> > +#include <linux/devfreq.h>
>> > +#include <linux/cpu.h>
>> > +#include <linux/cpufreq.h>
>> > +#include <linux/cpumask.h>
>> > +#include <linux/slab.h>
>> > +#include <linux/platform_device.h>
>> > +#include <linux/of.h>
>> > +#include <linux/module.h>
>> > +#include "governor.h"
>> > +
>> > +struct cpu_state {
>> > + unsigned int freq;
>> > + unsigned int min_freq;
>> > + unsigned int max_freq;
>> > + unsigned int first_cpu;
>> > +};
>> > +static struct cpu_state *state[NR_CPUS];
>> > +static int cpufreq_cnt;
>> > +
>> > +struct freq_map {
>> > + unsigned int cpu_khz;
>> > + unsigned int target_freq;
>> > +};
>> > +
>> > +struct devfreq_node {
>> > + struct devfreq *df;
>> > + void *orig_data;
>> > + struct device *dev;
>> > + struct device_node *of_node;
>> > + struct list_head list;
>> > + struct freq_map **map;
>> > + struct freq_map *common_map;
>> > +};
>> > +static LIST_HEAD(devfreq_list);
>> > +static DEFINE_MUTEX(state_lock);
>> > +static DEFINE_MUTEX(cpufreq_reg_lock);
>> > +
>> > +static void update_all_devfreqs(void)
>> > +{
>> > + struct devfreq_node *node;
>> > +
>> > + list_for_each_entry(node, &devfreq_list, list) {
>> > + struct devfreq *df = node->df;
>> > +
>> > + if (!node->df)
>> > + continue;
>> > + mutex_lock(&df->lock);
>> > + update_devfreq(df);
>> > + mutex_unlock(&df->lock);
>> > +
>> > + }
>> > +}
>> > +
>> > +static struct devfreq_node *find_devfreq_node(struct device *dev)
>> > +{
>> > + struct devfreq_node *node;
>> > +
>> > + list_for_each_entry(node, &devfreq_list, list)
>> > + if (node->dev == dev || node->of_node == dev->of_node)
>> > + return node;
>> > +
>> > + return NULL;
>> > +}
>> > +
>> > +/* ==================== cpufreq part ==================== */
>> > +static struct cpu_state *add_policy(struct cpufreq_policy *policy)
>> > +{
>> > + struct cpu_state *new_state;
>> > + unsigned int cpu, first_cpu;
>> > +
>> > + new_state = kzalloc(sizeof(struct cpu_state), GFP_KERNEL);
>> > + if (!new_state)
>> > + return NULL;
>> > +
>> > + first_cpu = cpumask_first(policy->related_cpus);
>> > + new_state->first_cpu = first_cpu;
>> > + new_state->freq = policy->cur;
>> > + new_state->min_freq = policy->cpuinfo.min_freq;
>> > + new_state->max_freq = policy->cpuinfo.max_freq;
>> > +
>> > + for_each_cpu(cpu, policy->related_cpus)
>> > + state[cpu] = new_state;
>> > +
>> > + return new_state;
>> > +}
>> > +
>> > +static int cpufreq_trans_notifier(struct notifier_block *nb,
>> > + unsigned long event, void *data)
>> > +{
>> > + struct cpufreq_freqs *freq = data;
>> > + struct cpu_state *s;
>> > + struct cpufreq_policy *policy = NULL;
>> > +
>> > + if (event != CPUFREQ_POSTCHANGE)
>> > + return 0;
>> > +
>> > + mutex_lock(&state_lock);
>> > +
>> > + s = state[freq->cpu];
>> > + if (!s) {
>> > + policy = cpufreq_cpu_get(freq->cpu);
>> > + if (policy) {
>> > + s = add_policy(policy);
>> > + cpufreq_cpu_put(policy);
>> > + }
>> > + }
>> > + if (!s)
>> > + goto out;
>> > +
>> > + if (s->freq != freq->new || policy) {
>> > + s->freq = freq->new;
>> > + update_all_devfreqs();
>> > + }
>> > +
>> > +out:
>> > + mutex_unlock(&state_lock);
>> > + return 0;
>> > +}
>> > +
>> > +static struct notifier_block cpufreq_trans_nb = {
>> > + .notifier_call = cpufreq_trans_notifier
>> > +};
>> > +
>> > +static int register_cpufreq(void)
>> > +{
>> > + int ret = 0;
>> > + unsigned int cpu;
>> > + struct cpufreq_policy *policy;
>> > +
>> > + mutex_lock(&cpufreq_reg_lock);
>> > +
>> > + if (cpufreq_cnt)
>> > + goto cnt_not_zero;
>> > +
>> > + get_online_cpus();
>> > + ret = cpufreq_register_notifier(&cpufreq_trans_nb,
>> > + CPUFREQ_TRANSITION_NOTIFIER);
>> > + if (ret)
>> > + goto out;
>> > +
>> > + for_each_online_cpu(cpu) {
>> > + policy = cpufreq_cpu_get(cpu);
>> > + if (policy) {
>> > + add_policy(policy);
>> > + cpufreq_cpu_put(policy);
>> > + }
>> > + }
>> > +out:
>> > + put_online_cpus();
>> > +cnt_not_zero:
>> > + if (!ret)
>> > + cpufreq_cnt++;
>> > + mutex_unlock(&cpufreq_reg_lock);
>> > + return ret;
>> > +}
>> > +
>> > +static int unregister_cpufreq(void)
>> > +{
>> > + int ret = 0;
>> > + int cpu;
>> > +
>> > + mutex_lock(&cpufreq_reg_lock);
>> > +
>> > + if (cpufreq_cnt > 1)
>> > + goto out;
>> > +
>> > + cpufreq_unregister_notifier(&cpufreq_trans_nb,
>> > + CPUFREQ_TRANSITION_NOTIFIER);
>> > +
>> > + for (cpu = ARRAY_SIZE(state) - 1; cpu >= 0; cpu--) {
>> > + if (!state[cpu])
>> > + continue;
>> > + if (state[cpu]->first_cpu == cpu)
>> > + kfree(state[cpu]);
>> > + state[cpu] = NULL;
>> > + }
>> > +
>> > +out:
>> > + cpufreq_cnt--;
>> > + mutex_unlock(&cpufreq_reg_lock);
>> > + return ret;
>> > +}
>> > +
>> > +/* ==================== devfreq part ==================== */
>> > +
>> > +static unsigned int interpolate_freq(struct devfreq *df, unsigned int cpu)
>> > +{
>> > + unsigned long *freq_table = df->profile->freq_table;
>> > + unsigned int cpu_min = state[cpu]->min_freq;
>> > + unsigned int cpu_max = state[cpu]->max_freq;
>> > + unsigned int cpu_freq = state[cpu]->freq;
>> > + unsigned int dev_min, dev_max, cpu_percent;
>> > +
>> > + if (freq_table) {
>> > + dev_min = freq_table[0];
>> > + dev_max = freq_table[df->profile->max_state - 1];
>> > + } else {
>> > + if (df->max_freq <= df->min_freq)
>> > + return 0;
>> > + dev_min = df->min_freq;
>> > + dev_max = df->max_freq;
>> > + }
>> > +
>> > + cpu_percent = ((cpu_freq - cpu_min) * 100) / (cpu_max - cpu_min);
>> > + return dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
>> > +}
>> > +
>> > +static unsigned int cpu_to_dev_freq(struct devfreq *df, unsigned int cpu)
>> > +{
>> > + struct freq_map *map = NULL;
>> > + unsigned int cpu_khz = 0, freq;
>> > + struct devfreq_node *n = df->data;
>> > +
>> > + if (!state[cpu] || state[cpu]->first_cpu != cpu) {
>> > + freq = 0;
>> > + goto out;
>> > + }
>> > +
>> > + if (n->common_map)
>> > + map = n->common_map;
>> > + else if (n->map)
>> > + map = n->map[cpu];
>> > +
>> > + cpu_khz = state[cpu]->freq;
>> > +
>> > + if (!map) {
>> > + freq = interpolate_freq(df, cpu);
>> > + goto out;
>> > + }
>> > +
>> > + while (map->cpu_khz && map->cpu_khz < cpu_khz)
>> > + map++;
>> > + if (!map->cpu_khz)
>> > + map--;
>> > + freq = map->target_freq;
>> > +
>> > +out:
>> > + dev_dbg(df->dev.parent, "CPU%u: %d -> dev: %u\n", cpu, cpu_khz, freq);
>> > + return freq;
>> > +}
>> > +
>> > +static int devfreq_cpufreq_get_freq(struct devfreq *df,
>> > + unsigned long *freq)
>> > +{
>> > + unsigned int cpu, tgt_freq = 0;
>> > + struct devfreq_node *node;
>> > +
>> > + node = df->data;
>> > + if (!node) {
>> > + pr_err("Unable to find devfreq node!\n");
>> > + return -ENODEV;
>> > + }
>> > +
>> > + for_each_possible_cpu(cpu)
>> > + tgt_freq = max(tgt_freq, cpu_to_dev_freq(df, cpu));
>> > +
>> > + *freq = tgt_freq;
>> > + return 0;
>> > +}
>> > +
>> > +static unsigned int show_table(char *buf, unsigned int len,
>> > + struct freq_map *map)
>> > +{
>> > + unsigned int cnt = 0;
>> > +
>> > + cnt += snprintf(buf + cnt, len - cnt, "CPU freq\tDevice freq\n");
>> > +
>> > + while (map->cpu_khz && cnt < len) {
>> > + cnt += snprintf(buf + cnt, len - cnt, "%8u\t%11u\n",
>> > + map->cpu_khz, map->target_freq);
>> > + map++;
>> > + }
>> > + if (cnt < len)
>> > + cnt += snprintf(buf + cnt, len - cnt, "\n");
>> > +
>> > + return cnt;
>> > +}
>> > +
>> > +static ssize_t freq_map_show(struct device *dev, struct device_attribute *attr,
>> > + char *buf)
>> > +{
>> > + struct devfreq *df = to_devfreq(dev);
>> > + struct devfreq_node *n = df->data;
>> > + struct freq_map *map;
>> > + unsigned int cnt = 0, cpu;
>> > +
>> > + mutex_lock(&state_lock);
>> > + if (n->common_map) {
>> > + map = n->common_map;
>> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
>> > + "Common table for all CPUs:\n");
>> > + cnt += show_table(buf + cnt, PAGE_SIZE - cnt, map);
>> > + } else if (n->map) {
>> > + for_each_possible_cpu(cpu) {
>> > + map = n->map[cpu];
>> > + if (!map)
>> > + continue;
>> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
>> > + "CPU %u:\n", cpu);
>> > + if (cnt >= PAGE_SIZE)
>> > + break;
>> > + cnt += show_table(buf + cnt, PAGE_SIZE - cnt, map);
>> > + if (cnt >= PAGE_SIZE)
>> > + break;
>> > + }
>> > + } else {
>> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
>> > + "Device freq interpolated based on CPU freq\n");
>> > + }
>> > + mutex_unlock(&state_lock);
>> > +
>> > + return cnt;
>> > +}
>> > +
>> > +static DEVICE_ATTR_RO(freq_map);
>> > +static struct attribute *dev_attr[] = {
>> > + &dev_attr_freq_map.attr,
>> > + NULL,
>> > +};
>> > +
>> > +static struct attribute_group dev_attr_group = {
>> > + .name = "cpufreq-map",
>> > + .attrs = dev_attr,
>> > +};
>> > +
>> > +static int devfreq_cpufreq_gov_start(struct devfreq *devfreq)
>> > +{
>> > + int ret = 0;
>> > + struct devfreq_node *node;
>> > + bool alloc = false;
>> > +
>> > + ret = register_cpufreq();
>> > + if (ret)
>> > + return ret;
>> > +
>> > + ret = sysfs_create_group(&devfreq->dev.kobj, &dev_attr_group);
>> > + if (ret) {
>> > + unregister_cpufreq();
>> > + return ret;
>> > + }
>> > +
>> > + mutex_lock(&state_lock);
>> > +
>> > + node = find_devfreq_node(devfreq->dev.parent);
>> > + if (node == NULL) {
>> > + node = kzalloc(sizeof(struct devfreq_node), GFP_KERNEL);
>> > + if (!node) {
>> > + ret = -ENOMEM;
>> > + goto alloc_fail;
>> > + }
>> > + alloc = true;
>> > + node->dev = devfreq->dev.parent;
>> > + list_add_tail(&node->list, &devfreq_list);
>> > + }
>> > + node->df = devfreq;
>> > + node->orig_data = devfreq->data;
>> > + devfreq->data = node;
>> > +
>> > + mutex_lock(&devfreq->lock);
>> > + ret = update_devfreq(devfreq);
>> > + mutex_unlock(&devfreq->lock);
>> > + if (ret) {
>> > + pr_err("Freq update failed!\n");
>> > + goto update_fail;
>> > + }
>> > +
>> > + mutex_unlock(&state_lock);
>> > + return 0;
>> > +
>> > +update_fail:
>> > + devfreq->data = node->orig_data;
>> > + if (alloc) {
>> > + list_del(&node->list);
>> > + kfree(node);
>> > + }
>> > +alloc_fail:
>> > + mutex_unlock(&state_lock);
>> > + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
>> > + unregister_cpufreq();
>> > + return ret;
>> > +}
>> > +
>> > +static void devfreq_cpufreq_gov_stop(struct devfreq *devfreq)
>> > +{
>> > + struct devfreq_node *node = devfreq->data;
>> > +
>> > + mutex_lock(&state_lock);
>> > + devfreq->data = node->orig_data;
>> > + if (node->map || node->common_map) {
>> > + node->df = NULL;
>> > + } else {
>> > + list_del(&node->list);
>> > + kfree(node);
>> > + }
>> > + mutex_unlock(&state_lock);
>> > +
>> > + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
>> > + unregister_cpufreq();
>> > +}
>> > +
>> > +static int devfreq_cpufreq_ev_handler(struct devfreq *devfreq,
>> > + unsigned int event, void *data)
>> > +{
>> > + int ret;
>> > +
>> > + switch (event) {
>> > + case DEVFREQ_GOV_START:
>> > +
>> > + ret = devfreq_cpufreq_gov_start(devfreq);
>> > + if (ret) {
>> > + pr_err("Governor start failed!\n");
>> > + return ret;
>> > + }
>> > + pr_debug("Enabled CPUfreq-map governor\n");
>> > + break;
>> > +
>> > + case DEVFREQ_GOV_STOP:
>> > +
>> > + devfreq_cpufreq_gov_stop(devfreq);
>> > + pr_debug("Disabled dev CPUfreq-map governor\n");
>> > + break;
>> > + }
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static struct devfreq_governor devfreq_cpufreq = {
>> > + .name = "cpufreq-map",
>> > + .get_target_freq = devfreq_cpufreq_get_freq,
>> > + .event_handler = devfreq_cpufreq_ev_handler,
>> > +};
>> > +
>> > +#define NUM_COLS 2
>> > +static struct freq_map *read_tbl(struct device_node *of_node, char *prop_name)
>> > +{
>> > + int len, nf, i, j;
>> > + u32 data;
>> > + struct freq_map *tbl;
>> > +
>> > + if (!of_find_property(of_node, prop_name, &len))
>> > + return NULL;
>> > + len /= sizeof(data);
>> > +
>> > + if (len % NUM_COLS || len == 0)
>> > + return NULL;
>> > + nf = len / NUM_COLS;
>> > +
>> > + tbl = kzalloc((nf + 1) * sizeof(*tbl), GFP_KERNEL);
>> > + if (!tbl)
>> > + return NULL;
>> > +
>> > + for (i = 0, j = 0; i < nf; i++, j += 2) {
>> > + of_property_read_u32_index(of_node, prop_name, j, &data);
>> > + tbl[i].cpu_khz = data;
>> > +
>> > + of_property_read_u32_index(of_node, prop_name, j + 1, &data);
>> > + tbl[i].target_freq = data;
>> > + }
>> > + tbl[i].cpu_khz = 0;
>> > +
>> > + return tbl;
>> > +}
>> > +
>> > +#define PROP_TARGET "target-dev"
>> > +#define PROP_TABLE "cpu-to-dev-map"
>> > +static int add_table_from_of(struct device_node *of_node)
>> > +{
>> > + struct device_node *target_of_node;
>> > + struct devfreq_node *node;
>> > + struct freq_map *common_tbl;
>> > + struct freq_map **tbl_list = NULL;
>> > + static char prop_name[] = PROP_TABLE "-999999";
>> > + int cpu, ret, cnt = 0, prop_sz = ARRAY_SIZE(prop_name);
>> > +
>> > + target_of_node = of_parse_phandle(of_node, PROP_TARGET, 0);
>> > + if (!target_of_node)
>> > + return -EINVAL;
>> > +
>> > + node = kzalloc(sizeof(struct devfreq_node), GFP_KERNEL);
>> > + if (!node)
>> > + return -ENOMEM;
>> > +
>> > + common_tbl = read_tbl(of_node, PROP_TABLE);
>> > + if (!common_tbl) {
>> > + tbl_list = kzalloc(sizeof(*tbl_list) * NR_CPUS, GFP_KERNEL);
>> > + if (!tbl_list) {
>> > + ret = -ENOMEM;
>> > + goto err_list;
>> > + }
>> > +
>> > + for_each_possible_cpu(cpu) {
>> > + ret = snprintf(prop_name, prop_sz, "%s-%d",
>> > + PROP_TABLE, cpu);
>> > + if (ret >= prop_sz) {
>> > + pr_warn("More CPUs than I can handle!\n");
>> > + pr_warn("Skipping rest of the tables!\n");
>> > + break;
>> > + }
>> > + tbl_list[cpu] = read_tbl(of_node, prop_name);
>> > + if (tbl_list[cpu])
>> > + cnt++;
>> > + }
>> > + }
>> > + if (!common_tbl && !cnt) {
>> > + ret = -EINVAL;
>> > + goto err_tbl;
>> > + }
>> > +
>> > + mutex_lock(&state_lock);
>> > + node->of_node = target_of_node;
>> > + node->map = tbl_list;
>> > + node->common_map = common_tbl;
>> > + list_add_tail(&node->list, &devfreq_list);
>> > + mutex_unlock(&state_lock);
>> > +
>> > + return 0;
>> > +err_tbl:
>> > + kfree(tbl_list);
>> > +err_list:
>> > + kfree(node);
>> > + return ret;
>> > +}
>> > +
>> > +static int __init devfreq_cpufreq_init(void)
>> > +{
>> > + int ret;
>> > + struct device_node *of_par, *of_child;
>> > +
>> > + of_par = of_find_node_by_name(NULL, "devfreq-cpufreq-map");
>> > + if (of_par) {
>> > + for_each_child_of_node(of_par, of_child) {
>> > + ret = add_table_from_of(of_child);
>> > + if (ret)
>> > + pr_err("Parsing %s failed!\n", of_child->name);
>> > + else
>> > + pr_debug("Parsed %s.\n", of_child->name);
>> > + }
>> > + of_node_put(of_par);
>> > + } else {
>> > + pr_info("No tables parsed from DT.\n");
>> > + }
>> > +
>> > + ret = devfreq_add_governor(&devfreq_cpufreq);
>> > + if (ret) {
>> > + pr_err("cpufreq-map governor add failed!\n");
>> > + return ret;
>> > + }
>> > +
>> > + return 0;
>> > +}
>> > +subsys_initcall(devfreq_cpufreq_init);
>> > +
>> > +static void __exit devfreq_cpufreq_exit(void)
>> > +{
>> > + int ret, cpu;
>> > + struct devfreq_node *node, *tmp;
>> > +
>> > + ret = devfreq_remove_governor(&devfreq_cpufreq);
>> > + if (ret)
>> > + pr_err("cpufreq-map governor remove failed!\n");
>> > +
>> > + mutex_lock(&state_lock);
>> > + list_for_each_entry_safe(node, tmp, &devfreq_list, list) {
>> > + kfree(node->common_map);
>> > + for_each_possible_cpu(cpu)
>> > + kfree(node->map[cpu]);
>> > + kfree(node->map);
>> > + list_del(&node->list);
>> > + kfree(node);
>> > + }
>> > + mutex_unlock(&state_lock);
>> > +}
>> > +module_exit(devfreq_cpufreq_exit);
>> > +
>> > +MODULE_DESCRIPTION("devfreq gov that sets dev freq based on current CPU freq");
>> > +MODULE_LICENSE("GPL v2");
>> >
--
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH RFC 1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor
From: Hsin-Yi Wang @ 2019-06-20 9:32 UTC (permalink / raw)
To: Chanwoo Choi
Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Saravana Kannan, Sibi Sankar, MyungJoo Ham, Kyungmin Park,
Matthias Brugger, linux-pm, linux-mediatek, lkml,
Andrew-sh . Cheng
In-Reply-To: <958b27ff-6e64-b4e2-44e6-bc342e3606bc@samsung.com>
Hi Chanwoo Choi, Saravana Kannan and Sibi Sankar,
I've also tested Sibi Sankar's patch[1] locally with mt8183-cci, and
it works fine too!
It'd be great if Sibi Sankar or anyone who is familiar with the
original design can finish this implementation. But if no one has time
to do that, I think I can also help on address the comments. Thanks!
[1]
[RFC,2/9] OPP: Export a number of helpers to prevent code duplication
- https://patchwork.kernel.org/patch/10875199/
[RFC,3/9] PM / devfreq: Add cpu based scaling support to passive_governor
- https://patchwork.kernel.org/patch/10875195/
Hsin-Yi
On Thu, Jun 20, 2019 at 2:56 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>
> + Sibi Sankar
>
> Hi, Hsin-Yi Wang, Saravana Kannan and Sibi Sankar
>
> I summarized the history of the related patch about this title.
>
> Firstly,
> As I knew, Saravana sent the patch[1] which contains
> 'governor_cpufreq_map.c' last year. According to the Myungoo's comment,
>
> Secondly,
> Sibi Sankar modified the 'governor_passive.c'[2] in order to support
> the mapping between cpu frequency and device frequency.
> Unfortunately, Sibi Sankar stopped the development about this
> because he had found the other method to get his purpose as I knew.
>
> Thirdly,
> Hsin-Yi Wang send the original patch of Saravana without modification.
>
>
> Sincerely, I think that the mapping between cpu frequency and device
> frequency is necessary. And I prefer the Sibi's approach which implements
> stuff to the existing 'passive' governor.
>
> We need to discuss about how to implement them by whom.
>
>
> [1] [v3,1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor
> - https://patchwork.kernel.org/patch/10553171/
>
> [2]
> [PATCH RFC 0/9] Add CPU based scaling support to Passive governor
> - https://lore.kernel.org/lkml/08c3cff8c39e3d82e044db93e992da72@codeaurora.org/T/
> [PATCH RFC 3/9] PM / devfreq: Add cpu based scaling support to passive_governor
> - https://lore.kernel.org/lkml/08c3cff8c39e3d82e044db93e992da72@codeaurora.org/T/#m1cafb7baf687d2a680d39c85d3ec7d1b590b68fc
>
>
> Best Regards,
> Chanwoo Choi
>
> On 19. 6. 18. 오후 1:14, Hsin-Yi Wang wrote:
> > From: Saravana Kannan <skannan@codeaurora.org>
> >
> > From: Saravana Kannan <skannan@codeaurora.org>
> >
> > Many CPU architectures have caches that can scale independent of the CPUs.
> > Frequency scaling of the caches is necessary to make sure the cache is not
> > a performance bottleneck that leads to poor performance and power. The same
> > idea applies for RAM/DDR.
> >
> > To achieve this, this patch adds a generic devfreq governor that takes the
> > current frequency of each CPU frequency domain and then adjusts the
> > frequency of the cache (or any devfreq device) based on the frequency of
> > the CPUs. It listens to CPU frequency transition notifiers to keep itself
> > up to date on the current CPU frequency.
> >
> > To decide the frequency of the device, the governor does one of the
> > following:
> >
> > * Uses a CPU frequency to device frequency mapping table
> > - Either one mapping table used for all CPU freq policies (typically used
> > for system with homogeneous cores/clusters that have the same OPPs).
> > - One mapping table per CPU freq policy (typically used for ASMP systems
> > with heterogeneous CPUs with different OPPs)
> >
> > OR
> >
> > * Scales the device frequency in proportion to the CPU frequency. So, if
> > the CPUs are running at their max frequency, the device runs at its max
> > frequency. If the CPUs are running at their min frequency, the device
> > runs at its min frequency. And interpolated for frequencies in between.
> >
> > Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > .../bindings/devfreq/devfreq-cpufreq-map.txt | 53 ++
> > drivers/devfreq/Kconfig | 8 +
> > drivers/devfreq/Makefile | 1 +
> > drivers/devfreq/governor_cpufreq_map.c | 583 ++++++++++++++++++
> > 4 files changed, 645 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> > create mode 100644 drivers/devfreq/governor_cpufreq_map.c
> >
> > diff --git a/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> > new file mode 100644
> > index 000000000000..982a30bcfc86
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> > @@ -0,0 +1,53 @@
> > +Devfreq CPUfreq governor
> > +
> > +devfreq-cpufreq-map is a parent device that contains one or more child devices.
> > +Each child device provides CPU frequency to device frequency mapping for a
> > +specific device. Examples of devices that could use this are: DDR, cache and
> > +CCI.
> > +
> > +Parent device name shall be "devfreq-cpufreq-map".
> > +
> > +Required child device properties:
> > +- cpu-to-dev-map, or cpu-to-dev-map-<X>:
> > + A list of tuples where each tuple consists of a
> > + CPU frequency (KHz) and the corresponding device
> > + frequency. CPU frequencies not listed in the table
> > + will use the device frequency that corresponds to the
> > + next rounded up CPU frequency.
> > + Use "cpu-to-dev-map" if all CPUs in the system should
> > + share same mapping.
> > + Use cpu-to-dev-map-<cpuid> to describe different
> > + mappings for different CPUs. The property should be
> > + listed only for the first CPU if multiple CPUs are
> > + synchronous.
> > +- target-dev: Phandle to device that this mapping applies to.
> > +
> > +Example:
> > + devfreq-cpufreq-map {
> > + cpubw-cpufreq {
> > + target-dev = <&cpubw>;
> > + cpu-to-dev-map =
> > + < 300000 1144000 >,
> > + < 422400 2288000 >,
> > + < 652800 3051000 >,
> > + < 883200 5996000 >,
> > + < 1190400 8056000 >,
> > + < 1497600 10101000 >,
> > + < 1728000 12145000 >,
> > + < 2649600 16250000 >;
> > + };
> > +
> > + cache-cpufreq {
> > + target-dev = <&cache>;
> > + cpu-to-dev-map =
> > + < 300000 300000 >,
> > + < 422400 422400 >,
> > + < 652800 499200 >,
> > + < 883200 576000 >,
> > + < 960000 960000 >,
> > + < 1497600 1036800 >,
> > + < 1574400 1574400 >,
> > + < 1728000 1651200 >,
> > + < 2649600 1728000 >;
> > + };
> > + };
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 0c8204d6b78a..0303f5a400b6 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -74,6 +74,14 @@ config DEVFREQ_GOV_PASSIVE
> > through sysfs entries. The passive governor recommends that
> > devfreq device uses the OPP table to get the frequency/voltage.
> >
> > +config DEVFREQ_GOV_CPUFREQ_MAP
> > + tristate "CPUfreq Map"
> > + depends on CPU_FREQ
> > + help
> > + Chooses frequency based on the online CPUs' current frequency and a
> > + CPU frequency to device frequency mapping table(s). This governor
> > + can be useful for controlling devices such as DDR, cache, CCI, etc.
> > +
> > comment "DEVFREQ Drivers"
> >
> > config ARM_EXYNOS_BUS_DEVFREQ
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index 817dde779f16..81141e2c784f 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -6,6 +6,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE) += governor_performance.o
> > obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
> > obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
> > obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
> > +obj-$(CONFIG_DEVFREQ_GOV_CPUFREQ_MAP) += governor_cpufreq_map.o
> >
> > # DEVFREQ Drivers
> > obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> > diff --git a/drivers/devfreq/governor_cpufreq_map.c b/drivers/devfreq/governor_cpufreq_map.c
> > new file mode 100644
> > index 000000000000..084a3ffb8f54
> > --- /dev/null
> > +++ b/drivers/devfreq/governor_cpufreq_map.c
> > @@ -0,0 +1,583 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#define pr_fmt(fmt) "dev-cpufreq-map: " fmt
> > +
> > +#include <linux/devfreq.h>
> > +#include <linux/cpu.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/cpumask.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/module.h>
> > +#include "governor.h"
> > +
> > +struct cpu_state {
> > + unsigned int freq;
> > + unsigned int min_freq;
> > + unsigned int max_freq;
> > + unsigned int first_cpu;
> > +};
> > +static struct cpu_state *state[NR_CPUS];
> > +static int cpufreq_cnt;
> > +
> > +struct freq_map {
> > + unsigned int cpu_khz;
> > + unsigned int target_freq;
> > +};
> > +
> > +struct devfreq_node {
> > + struct devfreq *df;
> > + void *orig_data;
> > + struct device *dev;
> > + struct device_node *of_node;
> > + struct list_head list;
> > + struct freq_map **map;
> > + struct freq_map *common_map;
> > +};
> > +static LIST_HEAD(devfreq_list);
> > +static DEFINE_MUTEX(state_lock);
> > +static DEFINE_MUTEX(cpufreq_reg_lock);
> > +
> > +static void update_all_devfreqs(void)
> > +{
> > + struct devfreq_node *node;
> > +
> > + list_for_each_entry(node, &devfreq_list, list) {
> > + struct devfreq *df = node->df;
> > +
> > + if (!node->df)
> > + continue;
> > + mutex_lock(&df->lock);
> > + update_devfreq(df);
> > + mutex_unlock(&df->lock);
> > +
> > + }
> > +}
> > +
> > +static struct devfreq_node *find_devfreq_node(struct device *dev)
> > +{
> > + struct devfreq_node *node;
> > +
> > + list_for_each_entry(node, &devfreq_list, list)
> > + if (node->dev == dev || node->of_node == dev->of_node)
> > + return node;
> > +
> > + return NULL;
> > +}
> > +
> > +/* ==================== cpufreq part ==================== */
> > +static struct cpu_state *add_policy(struct cpufreq_policy *policy)
> > +{
> > + struct cpu_state *new_state;
> > + unsigned int cpu, first_cpu;
> > +
> > + new_state = kzalloc(sizeof(struct cpu_state), GFP_KERNEL);
> > + if (!new_state)
> > + return NULL;
> > +
> > + first_cpu = cpumask_first(policy->related_cpus);
> > + new_state->first_cpu = first_cpu;
> > + new_state->freq = policy->cur;
> > + new_state->min_freq = policy->cpuinfo.min_freq;
> > + new_state->max_freq = policy->cpuinfo.max_freq;
> > +
> > + for_each_cpu(cpu, policy->related_cpus)
> > + state[cpu] = new_state;
> > +
> > + return new_state;
> > +}
> > +
> > +static int cpufreq_trans_notifier(struct notifier_block *nb,
> > + unsigned long event, void *data)
> > +{
> > + struct cpufreq_freqs *freq = data;
> > + struct cpu_state *s;
> > + struct cpufreq_policy *policy = NULL;
> > +
> > + if (event != CPUFREQ_POSTCHANGE)
> > + return 0;
> > +
> > + mutex_lock(&state_lock);
> > +
> > + s = state[freq->cpu];
> > + if (!s) {
> > + policy = cpufreq_cpu_get(freq->cpu);
> > + if (policy) {
> > + s = add_policy(policy);
> > + cpufreq_cpu_put(policy);
> > + }
> > + }
> > + if (!s)
> > + goto out;
> > +
> > + if (s->freq != freq->new || policy) {
> > + s->freq = freq->new;
> > + update_all_devfreqs();
> > + }
> > +
> > +out:
> > + mutex_unlock(&state_lock);
> > + return 0;
> > +}
> > +
> > +static struct notifier_block cpufreq_trans_nb = {
> > + .notifier_call = cpufreq_trans_notifier
> > +};
> > +
> > +static int register_cpufreq(void)
> > +{
> > + int ret = 0;
> > + unsigned int cpu;
> > + struct cpufreq_policy *policy;
> > +
> > + mutex_lock(&cpufreq_reg_lock);
> > +
> > + if (cpufreq_cnt)
> > + goto cnt_not_zero;
> > +
> > + get_online_cpus();
> > + ret = cpufreq_register_notifier(&cpufreq_trans_nb,
> > + CPUFREQ_TRANSITION_NOTIFIER);
> > + if (ret)
> > + goto out;
> > +
> > + for_each_online_cpu(cpu) {
> > + policy = cpufreq_cpu_get(cpu);
> > + if (policy) {
> > + add_policy(policy);
> > + cpufreq_cpu_put(policy);
> > + }
> > + }
> > +out:
> > + put_online_cpus();
> > +cnt_not_zero:
> > + if (!ret)
> > + cpufreq_cnt++;
> > + mutex_unlock(&cpufreq_reg_lock);
> > + return ret;
> > +}
> > +
> > +static int unregister_cpufreq(void)
> > +{
> > + int ret = 0;
> > + int cpu;
> > +
> > + mutex_lock(&cpufreq_reg_lock);
> > +
> > + if (cpufreq_cnt > 1)
> > + goto out;
> > +
> > + cpufreq_unregister_notifier(&cpufreq_trans_nb,
> > + CPUFREQ_TRANSITION_NOTIFIER);
> > +
> > + for (cpu = ARRAY_SIZE(state) - 1; cpu >= 0; cpu--) {
> > + if (!state[cpu])
> > + continue;
> > + if (state[cpu]->first_cpu == cpu)
> > + kfree(state[cpu]);
> > + state[cpu] = NULL;
> > + }
> > +
> > +out:
> > + cpufreq_cnt--;
> > + mutex_unlock(&cpufreq_reg_lock);
> > + return ret;
> > +}
> > +
> > +/* ==================== devfreq part ==================== */
> > +
> > +static unsigned int interpolate_freq(struct devfreq *df, unsigned int cpu)
> > +{
> > + unsigned long *freq_table = df->profile->freq_table;
> > + unsigned int cpu_min = state[cpu]->min_freq;
> > + unsigned int cpu_max = state[cpu]->max_freq;
> > + unsigned int cpu_freq = state[cpu]->freq;
> > + unsigned int dev_min, dev_max, cpu_percent;
> > +
> > + if (freq_table) {
> > + dev_min = freq_table[0];
> > + dev_max = freq_table[df->profile->max_state - 1];
> > + } else {
> > + if (df->max_freq <= df->min_freq)
> > + return 0;
> > + dev_min = df->min_freq;
> > + dev_max = df->max_freq;
> > + }
> > +
> > + cpu_percent = ((cpu_freq - cpu_min) * 100) / (cpu_max - cpu_min);
> > + return dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> > +}
> > +
> > +static unsigned int cpu_to_dev_freq(struct devfreq *df, unsigned int cpu)
> > +{
> > + struct freq_map *map = NULL;
> > + unsigned int cpu_khz = 0, freq;
> > + struct devfreq_node *n = df->data;
> > +
> > + if (!state[cpu] || state[cpu]->first_cpu != cpu) {
> > + freq = 0;
> > + goto out;
> > + }
> > +
> > + if (n->common_map)
> > + map = n->common_map;
> > + else if (n->map)
> > + map = n->map[cpu];
> > +
> > + cpu_khz = state[cpu]->freq;
> > +
> > + if (!map) {
> > + freq = interpolate_freq(df, cpu);
> > + goto out;
> > + }
> > +
> > + while (map->cpu_khz && map->cpu_khz < cpu_khz)
> > + map++;
> > + if (!map->cpu_khz)
> > + map--;
> > + freq = map->target_freq;
> > +
> > +out:
> > + dev_dbg(df->dev.parent, "CPU%u: %d -> dev: %u\n", cpu, cpu_khz, freq);
> > + return freq;
> > +}
> > +
> > +static int devfreq_cpufreq_get_freq(struct devfreq *df,
> > + unsigned long *freq)
> > +{
> > + unsigned int cpu, tgt_freq = 0;
> > + struct devfreq_node *node;
> > +
> > + node = df->data;
> > + if (!node) {
> > + pr_err("Unable to find devfreq node!\n");
> > + return -ENODEV;
> > + }
> > +
> > + for_each_possible_cpu(cpu)
> > + tgt_freq = max(tgt_freq, cpu_to_dev_freq(df, cpu));
> > +
> > + *freq = tgt_freq;
> > + return 0;
> > +}
> > +
> > +static unsigned int show_table(char *buf, unsigned int len,
> > + struct freq_map *map)
> > +{
> > + unsigned int cnt = 0;
> > +
> > + cnt += snprintf(buf + cnt, len - cnt, "CPU freq\tDevice freq\n");
> > +
> > + while (map->cpu_khz && cnt < len) {
> > + cnt += snprintf(buf + cnt, len - cnt, "%8u\t%11u\n",
> > + map->cpu_khz, map->target_freq);
> > + map++;
> > + }
> > + if (cnt < len)
> > + cnt += snprintf(buf + cnt, len - cnt, "\n");
> > +
> > + return cnt;
> > +}
> > +
> > +static ssize_t freq_map_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct devfreq *df = to_devfreq(dev);
> > + struct devfreq_node *n = df->data;
> > + struct freq_map *map;
> > + unsigned int cnt = 0, cpu;
> > +
> > + mutex_lock(&state_lock);
> > + if (n->common_map) {
> > + map = n->common_map;
> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> > + "Common table for all CPUs:\n");
> > + cnt += show_table(buf + cnt, PAGE_SIZE - cnt, map);
> > + } else if (n->map) {
> > + for_each_possible_cpu(cpu) {
> > + map = n->map[cpu];
> > + if (!map)
> > + continue;
> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> > + "CPU %u:\n", cpu);
> > + if (cnt >= PAGE_SIZE)
> > + break;
> > + cnt += show_table(buf + cnt, PAGE_SIZE - cnt, map);
> > + if (cnt >= PAGE_SIZE)
> > + break;
> > + }
> > + } else {
> > + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> > + "Device freq interpolated based on CPU freq\n");
> > + }
> > + mutex_unlock(&state_lock);
> > +
> > + return cnt;
> > +}
> > +
> > +static DEVICE_ATTR_RO(freq_map);
> > +static struct attribute *dev_attr[] = {
> > + &dev_attr_freq_map.attr,
> > + NULL,
> > +};
> > +
> > +static struct attribute_group dev_attr_group = {
> > + .name = "cpufreq-map",
> > + .attrs = dev_attr,
> > +};
> > +
> > +static int devfreq_cpufreq_gov_start(struct devfreq *devfreq)
> > +{
> > + int ret = 0;
> > + struct devfreq_node *node;
> > + bool alloc = false;
> > +
> > + ret = register_cpufreq();
> > + if (ret)
> > + return ret;
> > +
> > + ret = sysfs_create_group(&devfreq->dev.kobj, &dev_attr_group);
> > + if (ret) {
> > + unregister_cpufreq();
> > + return ret;
> > + }
> > +
> > + mutex_lock(&state_lock);
> > +
> > + node = find_devfreq_node(devfreq->dev.parent);
> > + if (node == NULL) {
> > + node = kzalloc(sizeof(struct devfreq_node), GFP_KERNEL);
> > + if (!node) {
> > + ret = -ENOMEM;
> > + goto alloc_fail;
> > + }
> > + alloc = true;
> > + node->dev = devfreq->dev.parent;
> > + list_add_tail(&node->list, &devfreq_list);
> > + }
> > + node->df = devfreq;
> > + node->orig_data = devfreq->data;
> > + devfreq->data = node;
> > +
> > + mutex_lock(&devfreq->lock);
> > + ret = update_devfreq(devfreq);
> > + mutex_unlock(&devfreq->lock);
> > + if (ret) {
> > + pr_err("Freq update failed!\n");
> > + goto update_fail;
> > + }
> > +
> > + mutex_unlock(&state_lock);
> > + return 0;
> > +
> > +update_fail:
> > + devfreq->data = node->orig_data;
> > + if (alloc) {
> > + list_del(&node->list);
> > + kfree(node);
> > + }
> > +alloc_fail:
> > + mutex_unlock(&state_lock);
> > + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
> > + unregister_cpufreq();
> > + return ret;
> > +}
> > +
> > +static void devfreq_cpufreq_gov_stop(struct devfreq *devfreq)
> > +{
> > + struct devfreq_node *node = devfreq->data;
> > +
> > + mutex_lock(&state_lock);
> > + devfreq->data = node->orig_data;
> > + if (node->map || node->common_map) {
> > + node->df = NULL;
> > + } else {
> > + list_del(&node->list);
> > + kfree(node);
> > + }
> > + mutex_unlock(&state_lock);
> > +
> > + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
> > + unregister_cpufreq();
> > +}
> > +
> > +static int devfreq_cpufreq_ev_handler(struct devfreq *devfreq,
> > + unsigned int event, void *data)
> > +{
> > + int ret;
> > +
> > + switch (event) {
> > + case DEVFREQ_GOV_START:
> > +
> > + ret = devfreq_cpufreq_gov_start(devfreq);
> > + if (ret) {
> > + pr_err("Governor start failed!\n");
> > + return ret;
> > + }
> > + pr_debug("Enabled CPUfreq-map governor\n");
> > + break;
> > +
> > + case DEVFREQ_GOV_STOP:
> > +
> > + devfreq_cpufreq_gov_stop(devfreq);
> > + pr_debug("Disabled dev CPUfreq-map governor\n");
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct devfreq_governor devfreq_cpufreq = {
> > + .name = "cpufreq-map",
> > + .get_target_freq = devfreq_cpufreq_get_freq,
> > + .event_handler = devfreq_cpufreq_ev_handler,
> > +};
> > +
> > +#define NUM_COLS 2
> > +static struct freq_map *read_tbl(struct device_node *of_node, char *prop_name)
> > +{
> > + int len, nf, i, j;
> > + u32 data;
> > + struct freq_map *tbl;
> > +
> > + if (!of_find_property(of_node, prop_name, &len))
> > + return NULL;
> > + len /= sizeof(data);
> > +
> > + if (len % NUM_COLS || len == 0)
> > + return NULL;
> > + nf = len / NUM_COLS;
> > +
> > + tbl = kzalloc((nf + 1) * sizeof(*tbl), GFP_KERNEL);
> > + if (!tbl)
> > + return NULL;
> > +
> > + for (i = 0, j = 0; i < nf; i++, j += 2) {
> > + of_property_read_u32_index(of_node, prop_name, j, &data);
> > + tbl[i].cpu_khz = data;
> > +
> > + of_property_read_u32_index(of_node, prop_name, j + 1, &data);
> > + tbl[i].target_freq = data;
> > + }
> > + tbl[i].cpu_khz = 0;
> > +
> > + return tbl;
> > +}
> > +
> > +#define PROP_TARGET "target-dev"
> > +#define PROP_TABLE "cpu-to-dev-map"
> > +static int add_table_from_of(struct device_node *of_node)
> > +{
> > + struct device_node *target_of_node;
> > + struct devfreq_node *node;
> > + struct freq_map *common_tbl;
> > + struct freq_map **tbl_list = NULL;
> > + static char prop_name[] = PROP_TABLE "-999999";
> > + int cpu, ret, cnt = 0, prop_sz = ARRAY_SIZE(prop_name);
> > +
> > + target_of_node = of_parse_phandle(of_node, PROP_TARGET, 0);
> > + if (!target_of_node)
> > + return -EINVAL;
> > +
> > + node = kzalloc(sizeof(struct devfreq_node), GFP_KERNEL);
> > + if (!node)
> > + return -ENOMEM;
> > +
> > + common_tbl = read_tbl(of_node, PROP_TABLE);
> > + if (!common_tbl) {
> > + tbl_list = kzalloc(sizeof(*tbl_list) * NR_CPUS, GFP_KERNEL);
> > + if (!tbl_list) {
> > + ret = -ENOMEM;
> > + goto err_list;
> > + }
> > +
> > + for_each_possible_cpu(cpu) {
> > + ret = snprintf(prop_name, prop_sz, "%s-%d",
> > + PROP_TABLE, cpu);
> > + if (ret >= prop_sz) {
> > + pr_warn("More CPUs than I can handle!\n");
> > + pr_warn("Skipping rest of the tables!\n");
> > + break;
> > + }
> > + tbl_list[cpu] = read_tbl(of_node, prop_name);
> > + if (tbl_list[cpu])
> > + cnt++;
> > + }
> > + }
> > + if (!common_tbl && !cnt) {
> > + ret = -EINVAL;
> > + goto err_tbl;
> > + }
> > +
> > + mutex_lock(&state_lock);
> > + node->of_node = target_of_node;
> > + node->map = tbl_list;
> > + node->common_map = common_tbl;
> > + list_add_tail(&node->list, &devfreq_list);
> > + mutex_unlock(&state_lock);
> > +
> > + return 0;
> > +err_tbl:
> > + kfree(tbl_list);
> > +err_list:
> > + kfree(node);
> > + return ret;
> > +}
> > +
> > +static int __init devfreq_cpufreq_init(void)
> > +{
> > + int ret;
> > + struct device_node *of_par, *of_child;
> > +
> > + of_par = of_find_node_by_name(NULL, "devfreq-cpufreq-map");
> > + if (of_par) {
> > + for_each_child_of_node(of_par, of_child) {
> > + ret = add_table_from_of(of_child);
> > + if (ret)
> > + pr_err("Parsing %s failed!\n", of_child->name);
> > + else
> > + pr_debug("Parsed %s.\n", of_child->name);
> > + }
> > + of_node_put(of_par);
> > + } else {
> > + pr_info("No tables parsed from DT.\n");
> > + }
> > +
> > + ret = devfreq_add_governor(&devfreq_cpufreq);
> > + if (ret) {
> > + pr_err("cpufreq-map governor add failed!\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +subsys_initcall(devfreq_cpufreq_init);
> > +
> > +static void __exit devfreq_cpufreq_exit(void)
> > +{
> > + int ret, cpu;
> > + struct devfreq_node *node, *tmp;
> > +
> > + ret = devfreq_remove_governor(&devfreq_cpufreq);
> > + if (ret)
> > + pr_err("cpufreq-map governor remove failed!\n");
> > +
> > + mutex_lock(&state_lock);
> > + list_for_each_entry_safe(node, tmp, &devfreq_list, list) {
> > + kfree(node->common_map);
> > + for_each_possible_cpu(cpu)
> > + kfree(node->map[cpu]);
> > + kfree(node->map);
> > + list_del(&node->list);
> > + kfree(node);
> > + }
> > + mutex_unlock(&state_lock);
> > +}
> > +module_exit(devfreq_cpufreq_exit);
> > +
> > +MODULE_DESCRIPTION("devfreq gov that sets dev freq based on current CPU freq");
> > +MODULE_LICENSE("GPL v2");
> >
^ permalink raw reply
* Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure
From: Chanwoo Choi @ 2019-06-20 8:04 UTC (permalink / raw)
To: Ezequiel Garcia, Kyungmin Park, MyungJoo Ham
Cc: kernel, linux-pm, Enric Balletbo i Serra
In-Reply-To: <7320aa6c-4a49-aae2-4db5-5c1f0d6ce76e@samsung.com>
On 19. 6. 20. 오후 4:59, Chanwoo Choi wrote:
> Hi,
>
> On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote:
>> A bit unexpectedly (but still documented), request_module may
>> return a positive value, in case of a modprobe error.
>> This is currently causing issues in the devfreq framework.
>>
>> When a request_module exits with a positive value, we currently
>> return that via ERR_PTR. However, because the value is positive,
>> it's not a ERR_VALUE proper, and is therefore treated as a
>> valid struct devfreq_governor pointer, leading to a kernel oops.
>>
>> The right way to fix this is hinted in __request_module documentation:
>>
>> """
>> [snip] The function returns
>> zero on success or a negative errno code or positive exit code from
>> "modprobe" on failure. Note that a successful module load does not mean
>> the module did not then unload and exit on an error of its own. Callers
>> must check that the service they requested is now available not blindly
>> invoke it.
>> """
>>
>> Therefore, drop the return value check, which is not useful, and instead
>> just re-try to find the (hopefully now loaded) governor.
>>
>> Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> ---
>> drivers/devfreq/devfreq.c | 8 ++------
>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 6b6991f0e873..8868ad9472d2 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
>> static struct devfreq_governor *try_then_request_governor(const char *name)
>> {
>> struct devfreq_governor *governor;
>> - int err = 0;
>>
>> if (IS_ERR_OR_NULL(name)) {
>> pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
>> @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>>
>> if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>> DEVFREQ_NAME_LEN))
>> - err = request_module("governor_%s", "simpleondemand");
>> + request_module("governor_%s", "simpleondemand");
>
> I don't agree to remove the exception handling. Even if request_module()
> returns positive value,
Sorry, I wrote the wrong comment. It have to handle the positive return value
for exception handling.
>
>> else
>> - err = request_module("governor_%s", name);
>> - /* Restore previous state before return */
>> + request_module("governor_%s", name);
>
> ditto.
>
>> mutex_lock(&devfreq_list_lock);
>> - if (err)> - return ERR_PTR(err);
>
> You better to modify it as following:
>
> if (err < 0)
> return ERR_PTR(err);
> else if (err > 0)
> return ERR_PTR(-EINVAL);
>
>
>>
>> governor = find_devfreq_governor(name);
>> }
>>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 1/2] PM / devfreq: Fix governor module load failure
From: Chanwoo Choi @ 2019-06-20 7:59 UTC (permalink / raw)
To: Ezequiel Garcia, Kyungmin Park, MyungJoo Ham
Cc: kernel, linux-pm, Enric Balletbo i Serra
In-Reply-To: <20190605190053.19177-1-ezequiel@collabora.com>
Hi,
On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote:
> A bit unexpectedly (but still documented), request_module may
> return a positive value, in case of a modprobe error.
> This is currently causing issues in the devfreq framework.
>
> When a request_module exits with a positive value, we currently
> return that via ERR_PTR. However, because the value is positive,
> it's not a ERR_VALUE proper, and is therefore treated as a
> valid struct devfreq_governor pointer, leading to a kernel oops.
>
> The right way to fix this is hinted in __request_module documentation:
>
> """
> [snip] The function returns
> zero on success or a negative errno code or positive exit code from
> "modprobe" on failure. Note that a successful module load does not mean
> the module did not then unload and exit on an error of its own. Callers
> must check that the service they requested is now available not blindly
> invoke it.
> """
>
> Therefore, drop the return value check, which is not useful, and instead
> just re-try to find the (hopefully now loaded) governor.
>
> Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.")
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> drivers/devfreq/devfreq.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 6b6991f0e873..8868ad9472d2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
> static struct devfreq_governor *try_then_request_governor(const char *name)
> {
> struct devfreq_governor *governor;
> - int err = 0;
>
> if (IS_ERR_OR_NULL(name)) {
> pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>
> if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
> DEVFREQ_NAME_LEN))
> - err = request_module("governor_%s", "simpleondemand");
> + request_module("governor_%s", "simpleondemand");
I don't agree to remove the exception handling. Even if request_module()
returns positive value, it have to be handled the negative code for error case.
> else
> - err = request_module("governor_%s", name);
> - /* Restore previous state before return */
> + request_module("governor_%s", name);
ditto.
> mutex_lock(&devfreq_list_lock);
> - if (err)> - return ERR_PTR(err);
You better to modify it as following:
if (err < 0)
return ERR_PTR(err);
else if (err > 0)
return ERR_PTR(-EINVAL);
>
> governor = find_devfreq_governor(name);
> }
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* [PATCH V4 1/6] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()
From: Viresh Kumar @ 2019-06-20 7:35 UTC (permalink / raw)
To: Rafael Wysocki, Len Brown, Pavel Machek, Kevin Hilman,
Ulf Hansson
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
juri.lelli, linux-kernel
In-Reply-To: <cover.1561014965.git.viresh.kumar@linaro.org>
In order to use the same set of routines to register notifiers for
different request types, update the existing
dev_pm_qos_{add|remove}_notifier() routines with an additional
parameter: request-type.
For now, it only supports resume-latency request type but will be
extended to frequency limit (min/max) constraints later on.
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Documentation/power/pm_qos_interface.txt | 10 ++++++----
drivers/base/power/domain.c | 8 +++++---
drivers/base/power/qos.c | 14 ++++++++++++--
include/linux/pm_qos.h | 12 ++++++++----
4 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index 19c5f7b1a7ba..ec7d662d1707 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -164,12 +164,14 @@ directory.
Notification mechanisms:
The per-device PM QoS framework has a per-device notification tree.
-int dev_pm_qos_add_notifier(device, notifier):
-Adds a notification callback function for the device.
+int dev_pm_qos_add_notifier(device, notifier, type):
+Adds a notification callback function for the device for a particular request
+type.
+
The callback is called when the aggregated value of the device constraints list
-is changed (for resume latency device PM QoS only).
+is changed.
-int dev_pm_qos_remove_notifier(device, notifier):
+int dev_pm_qos_remove_notifier(device, notifier, type):
Removes the notification callback function for the device.
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 33c30c1e6a30..b063bc41b0a9 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1536,7 +1536,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
if (ret)
genpd_free_dev_data(dev, gpd_data);
else
- dev_pm_qos_add_notifier(dev, &gpd_data->nb);
+ dev_pm_qos_add_notifier(dev, &gpd_data->nb,
+ DEV_PM_QOS_RESUME_LATENCY);
return ret;
}
@@ -1569,7 +1570,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
pdd = dev->power.subsys_data->domain_data;
gpd_data = to_gpd_data(pdd);
- dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
+ dev_pm_qos_remove_notifier(dev, &gpd_data->nb,
+ DEV_PM_QOS_RESUME_LATENCY);
genpd_lock(genpd);
@@ -1597,7 +1599,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
out:
genpd_unlock(genpd);
- dev_pm_qos_add_notifier(dev, &gpd_data->nb);
+ dev_pm_qos_add_notifier(dev, &gpd_data->nb, DEV_PM_QOS_RESUME_LATENCY);
return ret;
}
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 6c91f8df1d59..cfd463212513 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -467,6 +467,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
*
* @dev: target device for the constraint
* @notifier: notifier block managed by caller.
+ * @type: request type.
*
* Will register the notifier into a notification chain that gets called
* upon changes to the target value for the device.
@@ -474,10 +475,14 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
* If the device's constraints object doesn't exist when this routine is called,
* it will be created (or error code will be returned if that fails).
*/
-int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
+int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
+ enum dev_pm_qos_req_type type)
{
int ret = 0;
+ if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
+ return -EINVAL;
+
mutex_lock(&dev_pm_qos_mtx);
if (IS_ERR(dev->power.qos))
@@ -500,15 +505,20 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
*
* @dev: target device for the constraint
* @notifier: notifier block to be removed.
+ * @type: request type.
*
* Will remove the notifier from the notification chain that gets called
* upon changes to the target value.
*/
int dev_pm_qos_remove_notifier(struct device *dev,
- struct notifier_block *notifier)
+ struct notifier_block *notifier,
+ enum dev_pm_qos_req_type type)
{
int retval = 0;
+ if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
+ return -EINVAL;
+
mutex_lock(&dev_pm_qos_mtx);
/* Silently return if the constraints object is not present. */
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 6ea1ae373d77..1f4d456e8fff 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -146,9 +146,11 @@ int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
int dev_pm_qos_remove_request(struct dev_pm_qos_request *req);
int dev_pm_qos_add_notifier(struct device *dev,
- struct notifier_block *notifier);
+ struct notifier_block *notifier,
+ enum dev_pm_qos_req_type type);
int dev_pm_qos_remove_notifier(struct device *dev,
- struct notifier_block *notifier);
+ struct notifier_block *notifier,
+ enum dev_pm_qos_req_type type);
void dev_pm_qos_constraints_init(struct device *dev);
void dev_pm_qos_constraints_destroy(struct device *dev);
int dev_pm_qos_add_ancestor_request(struct device *dev,
@@ -202,10 +204,12 @@ static inline int dev_pm_qos_update_request(struct dev_pm_qos_request *req,
static inline int dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
{ return 0; }
static inline int dev_pm_qos_add_notifier(struct device *dev,
- struct notifier_block *notifier)
+ struct notifier_block *notifier,
+ enum dev_pm_qos_req_type type);
{ return 0; }
static inline int dev_pm_qos_remove_notifier(struct device *dev,
- struct notifier_block *notifier)
+ struct notifier_block *notifier,
+ enum dev_pm_qos_req_type type)
{ return 0; }
static inline void dev_pm_qos_constraints_init(struct device *dev)
{
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V4 2/6] PM / QOS: Rename __dev_pm_qos_read_value() and dev_pm_qos_raw_read_value()
From: Viresh Kumar @ 2019-06-20 7:35 UTC (permalink / raw)
To: Rafael Wysocki, Kevin Hilman, Ulf Hansson, Pavel Machek,
Len Brown, Daniel Lezcano
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
juri.lelli, linux-kernel
In-Reply-To: <cover.1561014965.git.viresh.kumar@linaro.org>
dev_pm_qos_read_value() will soon need to support more constraint types
(min/max frequency) and will have another argument to it, i.e. type of
the constraint. While that is fine for the existing users of
dev_pm_qos_read_value(), but not that optimal for the callers of
__dev_pm_qos_read_value() and dev_pm_qos_raw_read_value() as all the
callers of these two routines are only looking for resume latency
constraint.
Lets make these two routines care only about the resume latency
constraint and rename them to __dev_pm_qos_resume_latency() and
dev_pm_qos_raw_resume_latency().
Suggested-by: Rafael J. Wysocki <rjw@rjwysocki.net>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/domain_governor.c | 2 +-
drivers/base/power/qos.c | 13 +++++++++----
drivers/base/power/runtime.c | 2 +-
drivers/cpuidle/governor.c | 2 +-
include/linux/pm_qos.h | 8 ++++----
5 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 3838045c9277..20e56a5be01f 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -66,7 +66,7 @@ static bool default_suspend_ok(struct device *dev)
td->constraint_changed = false;
td->cached_suspend_ok = false;
td->effective_constraint_ns = 0;
- constraint_ns = __dev_pm_qos_read_value(dev);
+ constraint_ns = __dev_pm_qos_resume_latency(dev);
spin_unlock_irqrestore(&dev->power.lock, flags);
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index cfd463212513..7a0d197f0809 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -90,16 +90,16 @@ enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev, s32 mask)
EXPORT_SYMBOL_GPL(dev_pm_qos_flags);
/**
- * __dev_pm_qos_read_value - Get PM QoS constraint for a given device.
+ * __dev_pm_qos_resume_latency - Get resume latency constraint for a given device.
* @dev: Device to get the PM QoS constraint value for.
*
* This routine must be called with dev->power.lock held.
*/
-s32 __dev_pm_qos_read_value(struct device *dev)
+s32 __dev_pm_qos_resume_latency(struct device *dev)
{
lockdep_assert_held(&dev->power.lock);
- return dev_pm_qos_raw_read_value(dev);
+ return dev_pm_qos_raw_resume_latency(dev);
}
/**
@@ -112,7 +112,12 @@ s32 dev_pm_qos_read_value(struct device *dev)
s32 ret;
spin_lock_irqsave(&dev->power.lock, flags);
- ret = __dev_pm_qos_read_value(dev);
+
+ if (IS_ERR_OR_NULL(dev->power.qos))
+ ret = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+ else
+ ret = pm_qos_read_value(&dev->power.qos->resume_latency);
+
spin_unlock_irqrestore(&dev->power.lock, flags);
return ret;
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 952a1e7057c7..b75335508d2c 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -275,7 +275,7 @@ static int rpm_check_suspend_allowed(struct device *dev)
|| (dev->power.request_pending
&& dev->power.request == RPM_REQ_RESUME))
retval = -EAGAIN;
- else if (__dev_pm_qos_read_value(dev) == 0)
+ else if (__dev_pm_qos_resume_latency(dev) == 0)
retval = -EPERM;
else if (dev->power.runtime_status == RPM_SUSPENDED)
retval = 1;
diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
index 9fddf828a76f..2e3e14192bee 100644
--- a/drivers/cpuidle/governor.c
+++ b/drivers/cpuidle/governor.c
@@ -110,7 +110,7 @@ int cpuidle_governor_latency_req(unsigned int cpu)
{
int global_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
struct device *device = get_cpu_device(cpu);
- int device_req = dev_pm_qos_raw_read_value(device);
+ int device_req = dev_pm_qos_raw_resume_latency(device);
return device_req < global_req ? device_req : global_req;
}
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 1f4d456e8fff..06aa88496761 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -139,7 +139,7 @@ s32 pm_qos_read_value(struct pm_qos_constraints *c);
#ifdef CONFIG_PM
enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev, s32 mask);
enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev, s32 mask);
-s32 __dev_pm_qos_read_value(struct device *dev);
+s32 __dev_pm_qos_resume_latency(struct device *dev);
s32 dev_pm_qos_read_value(struct device *dev);
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
enum dev_pm_qos_req_type type, s32 value);
@@ -176,7 +176,7 @@ static inline s32 dev_pm_qos_requested_flags(struct device *dev)
return dev->power.qos->flags_req->data.flr.flags;
}
-static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
+static inline s32 dev_pm_qos_raw_resume_latency(struct device *dev)
{
return IS_ERR_OR_NULL(dev->power.qos) ?
PM_QOS_RESUME_LATENCY_NO_CONSTRAINT :
@@ -189,7 +189,7 @@ static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
static inline enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev,
s32 mask)
{ return PM_QOS_FLAGS_UNDEFINED; }
-static inline s32 __dev_pm_qos_read_value(struct device *dev)
+static inline s32 __dev_pm_qos_resume_latency(struct device *dev)
{ return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
static inline s32 dev_pm_qos_read_value(struct device *dev)
{ return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
@@ -245,7 +245,7 @@ static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev)
return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
}
static inline s32 dev_pm_qos_requested_flags(struct device *dev) { return 0; }
-static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
+static inline s32 dev_pm_qos_raw_resume_latency(struct device *dev)
{
return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
}
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V4 3/6] PM / QOS: Pass request type to dev_pm_qos_read_value()
From: Viresh Kumar @ 2019-06-20 7:35 UTC (permalink / raw)
To: Rafael Wysocki, Len Brown, Pavel Machek, Kevin Hilman,
Ulf Hansson
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
juri.lelli, linux-kernel
In-Reply-To: <cover.1561014965.git.viresh.kumar@linaro.org>
In order to allow dev_pm_qos_read_value() to read values for different
QoS requests, pass request type as a parameter to these routines.
For now, it only supports resume-latency request type but will be
extended to frequency limit (min/max) constraints later on.
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Documentation/power/pm_qos_interface.txt | 2 +-
drivers/base/power/domain_governor.c | 2 +-
drivers/base/power/qos.c | 17 ++++++++++++-----
include/linux/pm_qos.h | 16 +++++++++++++---
4 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index ec7d662d1707..cfcb1df39799 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -123,7 +123,7 @@ Will remove the element. After removal it will update the aggregate target and
call the notification trees if the target was changed as a result of removing
the request.
-s32 dev_pm_qos_read_value(device):
+s32 dev_pm_qos_read_value(device, type):
Returns the aggregated value for a given device's constraints list.
enum pm_qos_flags_status dev_pm_qos_flags(device, mask)
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 20e56a5be01f..daa8c7689f7e 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -33,7 +33,7 @@ static int dev_update_qos_constraint(struct device *dev, void *data)
* take its current PM QoS constraint (that's the only thing
* known at this point anyway).
*/
- constraint_ns = dev_pm_qos_read_value(dev);
+ constraint_ns = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
constraint_ns *= NSEC_PER_USEC;
}
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 7a0d197f0809..2461fed0efa0 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -105,18 +105,25 @@ s32 __dev_pm_qos_resume_latency(struct device *dev)
/**
* dev_pm_qos_read_value - Get PM QoS constraint for a given device (locked).
* @dev: Device to get the PM QoS constraint value for.
+ * @type: QoS request type.
*/
-s32 dev_pm_qos_read_value(struct device *dev)
+s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
{
+ struct dev_pm_qos *qos = dev->power.qos;
unsigned long flags;
s32 ret;
spin_lock_irqsave(&dev->power.lock, flags);
- if (IS_ERR_OR_NULL(dev->power.qos))
- ret = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
- else
- ret = pm_qos_read_value(&dev->power.qos->resume_latency);
+ switch (type) {
+ case DEV_PM_QOS_RESUME_LATENCY:
+ ret = IS_ERR_OR_NULL(qos) ? PM_QOS_RESUME_LATENCY_NO_CONSTRAINT
+ : pm_qos_read_value(&qos->resume_latency);
+ break;
+ default:
+ WARN_ON(1);
+ ret = 0;
+ }
spin_unlock_irqrestore(&dev->power.lock, flags);
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 06aa88496761..5ce1c310373e 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -140,7 +140,7 @@ s32 pm_qos_read_value(struct pm_qos_constraints *c);
enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev, s32 mask);
enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev, s32 mask);
s32 __dev_pm_qos_resume_latency(struct device *dev);
-s32 dev_pm_qos_read_value(struct device *dev);
+s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type);
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
enum dev_pm_qos_req_type type, s32 value);
int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
@@ -191,8 +191,18 @@ static inline enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev,
{ return PM_QOS_FLAGS_UNDEFINED; }
static inline s32 __dev_pm_qos_resume_latency(struct device *dev)
{ return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
-static inline s32 dev_pm_qos_read_value(struct device *dev)
- { return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
+static inline s32 dev_pm_qos_read_value(struct device *dev,
+ enum dev_pm_qos_req_type type)
+{
+ switch type {
+ case DEV_PM_QOS_RESUME_LATENCY:
+ return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+ default:
+ WARN_ON(1);
+ return 0;
+ }
+}
+
static inline int dev_pm_qos_add_request(struct device *dev,
struct dev_pm_qos_request *req,
enum dev_pm_qos_req_type type,
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V4 4/6] PM / QoS: Add support for MIN/MAX frequency constraints
From: Viresh Kumar @ 2019-06-20 7:35 UTC (permalink / raw)
To: Rafael Wysocki, Pavel Machek, Len Brown
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
juri.lelli, Ulf Hansson, linux-kernel
In-Reply-To: <cover.1561014965.git.viresh.kumar@linaro.org>
This patch introduces the min-frequency and max-frequency device
constraints, which will be used by the cpufreq core to begin with.
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/qos.c | 111 ++++++++++++++++++++++++++++++++++-----
include/linux/pm_qos.h | 12 +++++
2 files changed, 109 insertions(+), 14 deletions(-)
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 2461fed0efa0..6c90fd7e2ff8 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -120,6 +120,14 @@ s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
ret = IS_ERR_OR_NULL(qos) ? PM_QOS_RESUME_LATENCY_NO_CONSTRAINT
: pm_qos_read_value(&qos->resume_latency);
break;
+ case DEV_PM_QOS_MIN_FREQUENCY:
+ ret = IS_ERR_OR_NULL(qos) ? PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE
+ : pm_qos_read_value(&qos->min_frequency);
+ break;
+ case DEV_PM_QOS_MAX_FREQUENCY:
+ ret = IS_ERR_OR_NULL(qos) ? PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE
+ : pm_qos_read_value(&qos->max_frequency);
+ break;
default:
WARN_ON(1);
ret = 0;
@@ -161,6 +169,14 @@ static int apply_constraint(struct dev_pm_qos_request *req,
req->dev->power.set_latency_tolerance(req->dev, value);
}
break;
+ case DEV_PM_QOS_MIN_FREQUENCY:
+ ret = pm_qos_update_target(&qos->min_frequency,
+ &req->data.pnode, action, value);
+ break;
+ case DEV_PM_QOS_MAX_FREQUENCY:
+ ret = pm_qos_update_target(&qos->max_frequency,
+ &req->data.pnode, action, value);
+ break;
case DEV_PM_QOS_FLAGS:
ret = pm_qos_update_flags(&qos->flags, &req->data.flr,
action, value);
@@ -189,12 +205,11 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
if (!qos)
return -ENOMEM;
- n = kzalloc(sizeof(*n), GFP_KERNEL);
+ n = kzalloc(3 * sizeof(*n), GFP_KERNEL);
if (!n) {
kfree(qos);
return -ENOMEM;
}
- BLOCKING_INIT_NOTIFIER_HEAD(n);
c = &qos->resume_latency;
plist_head_init(&c->list);
@@ -203,6 +218,7 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
c->type = PM_QOS_MIN;
c->notifiers = n;
+ BLOCKING_INIT_NOTIFIER_HEAD(n);
c = &qos->latency_tolerance;
plist_head_init(&c->list);
@@ -211,6 +227,24 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
c->no_constraint_value = PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT;
c->type = PM_QOS_MIN;
+ c = &qos->min_frequency;
+ plist_head_init(&c->list);
+ c->target_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
+ c->default_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
+ c->no_constraint_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
+ c->type = PM_QOS_MAX;
+ c->notifiers = ++n;
+ BLOCKING_INIT_NOTIFIER_HEAD(n);
+
+ c = &qos->max_frequency;
+ plist_head_init(&c->list);
+ c->target_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
+ c->default_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
+ c->no_constraint_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
+ c->type = PM_QOS_MIN;
+ c->notifiers = ++n;
+ BLOCKING_INIT_NOTIFIER_HEAD(n);
+
INIT_LIST_HEAD(&qos->flags.list);
spin_lock_irq(&dev->power.lock);
@@ -264,11 +298,25 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
}
+
c = &qos->latency_tolerance;
plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
}
+
+ c = &qos->min_frequency;
+ plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+ apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE);
+ memset(req, 0, sizeof(*req));
+ }
+
+ c = &qos->max_frequency;
+ plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+ apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
+ memset(req, 0, sizeof(*req));
+ }
+
f = &qos->flags;
list_for_each_entry_safe(req, tmp, &f->list, data.flr.node) {
apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
@@ -380,6 +428,8 @@ static int __dev_pm_qos_update_request(struct dev_pm_qos_request *req,
switch(req->type) {
case DEV_PM_QOS_RESUME_LATENCY:
case DEV_PM_QOS_LATENCY_TOLERANCE:
+ case DEV_PM_QOS_MIN_FREQUENCY:
+ case DEV_PM_QOS_MAX_FREQUENCY:
curr_value = req->data.pnode.prio;
break;
case DEV_PM_QOS_FLAGS:
@@ -492,9 +542,6 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
{
int ret = 0;
- if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
- return -EINVAL;
-
mutex_lock(&dev_pm_qos_mtx);
if (IS_ERR(dev->power.qos))
@@ -502,10 +549,28 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
else if (!dev->power.qos)
ret = dev_pm_qos_constraints_allocate(dev);
- if (!ret)
+ if (ret)
+ goto unlock;
+
+ switch (type) {
+ case DEV_PM_QOS_RESUME_LATENCY:
ret = blocking_notifier_chain_register(dev->power.qos->resume_latency.notifiers,
notifier);
+ break;
+ case DEV_PM_QOS_MIN_FREQUENCY:
+ ret = blocking_notifier_chain_register(dev->power.qos->min_frequency.notifiers,
+ notifier);
+ break;
+ case DEV_PM_QOS_MAX_FREQUENCY:
+ ret = blocking_notifier_chain_register(dev->power.qos->max_frequency.notifiers,
+ notifier);
+ break;
+ default:
+ WARN_ON(1);
+ ret = -EINVAL;
+ }
+unlock:
mutex_unlock(&dev_pm_qos_mtx);
return ret;
}
@@ -526,20 +591,35 @@ int dev_pm_qos_remove_notifier(struct device *dev,
struct notifier_block *notifier,
enum dev_pm_qos_req_type type)
{
- int retval = 0;
-
- if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
- return -EINVAL;
+ int ret = 0;
mutex_lock(&dev_pm_qos_mtx);
/* Silently return if the constraints object is not present. */
- if (!IS_ERR_OR_NULL(dev->power.qos))
- retval = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers,
- notifier);
+ if (IS_ERR_OR_NULL(dev->power.qos))
+ goto unlock;
+
+ switch (type) {
+ case DEV_PM_QOS_RESUME_LATENCY:
+ ret = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers,
+ notifier);
+ break;
+ case DEV_PM_QOS_MIN_FREQUENCY:
+ ret = blocking_notifier_chain_unregister(dev->power.qos->min_frequency.notifiers,
+ notifier);
+ break;
+ case DEV_PM_QOS_MAX_FREQUENCY:
+ ret = blocking_notifier_chain_unregister(dev->power.qos->max_frequency.notifiers,
+ notifier);
+ break;
+ default:
+ WARN_ON(1);
+ ret = -EINVAL;
+ }
+unlock:
mutex_unlock(&dev_pm_qos_mtx);
- return retval;
+ return ret;
}
EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
@@ -599,6 +679,9 @@ static void __dev_pm_qos_drop_user_request(struct device *dev,
req = dev->power.qos->flags_req;
dev->power.qos->flags_req = NULL;
break;
+ default:
+ WARN_ON(1);
+ return;
}
__dev_pm_qos_remove_request(req);
kfree(req);
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 5ce1c310373e..17132b10b233 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -40,6 +40,8 @@ enum pm_qos_flags_status {
#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT PM_QOS_LATENCY_ANY
#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS PM_QOS_LATENCY_ANY_NS
#define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE 0
+#define PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE 0
+#define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE (-1)
#define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT (-1)
#define PM_QOS_FLAG_NO_POWER_OFF (1 << 0)
@@ -58,6 +60,8 @@ struct pm_qos_flags_request {
enum dev_pm_qos_req_type {
DEV_PM_QOS_RESUME_LATENCY = 1,
DEV_PM_QOS_LATENCY_TOLERANCE,
+ DEV_PM_QOS_MIN_FREQUENCY,
+ DEV_PM_QOS_MAX_FREQUENCY,
DEV_PM_QOS_FLAGS,
};
@@ -99,10 +103,14 @@ struct pm_qos_flags {
struct dev_pm_qos {
struct pm_qos_constraints resume_latency;
struct pm_qos_constraints latency_tolerance;
+ struct pm_qos_constraints min_frequency;
+ struct pm_qos_constraints max_frequency;
struct pm_qos_flags flags;
struct dev_pm_qos_request *resume_latency_req;
struct dev_pm_qos_request *latency_tolerance_req;
struct dev_pm_qos_request *flags_req;
+ struct dev_pm_qos_request *min_frequency_req;
+ struct dev_pm_qos_request *max_frequency_req;
};
/* Action requested to pm_qos_update_target */
@@ -197,6 +205,10 @@ static inline s32 dev_pm_qos_read_value(struct device *dev,
switch type {
case DEV_PM_QOS_RESUME_LATENCY:
return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+ case DEV_PM_QOS_MIN_FREQUENCY:
+ return PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
+ case DEV_PM_QOS_MAX_FREQUENCY:
+ return PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
default:
WARN_ON(1);
return 0;
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V4 6/6] cpufreq: Add QoS requests for userspace constraints
From: Viresh Kumar @ 2019-06-20 7:35 UTC (permalink / raw)
To: Rafael Wysocki
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
juri.lelli, Ulf Hansson, linux-kernel
In-Reply-To: <cover.1561014965.git.viresh.kumar@linaro.org>
This implements QoS requests to manage userspace configuration of min
and max frequency.
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 87 ++++++++++++++++++++-------------------
include/linux/cpufreq.h | 8 +---
2 files changed, 46 insertions(+), 49 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b47a6c094171..5d050b4a3806 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -718,23 +718,15 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
static ssize_t store_##file_name \
(struct cpufreq_policy *policy, const char *buf, size_t count) \
{ \
- int ret, temp; \
- struct cpufreq_policy new_policy; \
+ unsigned long val; \
+ int ret; \
\
- memcpy(&new_policy, policy, sizeof(*policy)); \
- new_policy.min = policy->user_policy.min; \
- new_policy.max = policy->user_policy.max; \
- \
- ret = sscanf(buf, "%u", &new_policy.object); \
+ ret = sscanf(buf, "%lu", &val); \
if (ret != 1) \
return -EINVAL; \
\
- temp = new_policy.object; \
- ret = cpufreq_set_policy(policy, &new_policy); \
- if (!ret) \
- policy->user_policy.object = temp; \
- \
- return ret ? ret : count; \
+ ret = dev_pm_qos_update_request(policy->object##_freq_req, val);\
+ return ret >= 0 ? count : ret; \
}
store_one(scaling_min_freq, min);
@@ -1126,8 +1118,6 @@ static void reeval_frequency_limits(struct cpufreq_policy *policy)
new_policy = *policy;
pr_debug("updating policy for CPU %u\n", policy->cpu);
- new_policy.min = policy->user_policy.min;
- new_policy.max = policy->user_policy.max;
cpufreq_set_policy(policy, &new_policy);
}
@@ -1237,6 +1227,12 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
goto err_min_qos_notifier;
}
+ policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req),
+ GFP_KERNEL);
+ if (!policy->min_freq_req)
+ goto err_max_qos_notifier;
+
+ policy->max_freq_req = policy->min_freq_req + 1;
INIT_LIST_HEAD(&policy->policy_list);
init_rwsem(&policy->rwsem);
spin_lock_init(&policy->transition_lock);
@@ -1247,6 +1243,9 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
policy->cpu = cpu;
return policy;
+err_max_qos_notifier:
+ dev_pm_qos_remove_notifier(dev, &policy->nb_max,
+ DEV_PM_QOS_MAX_FREQUENCY);
err_min_qos_notifier:
dev_pm_qos_remove_notifier(dev, &policy->nb_min,
DEV_PM_QOS_MIN_FREQUENCY);
@@ -1282,6 +1281,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
DEV_PM_QOS_MAX_FREQUENCY);
dev_pm_qos_remove_notifier(dev, &policy->nb_min,
DEV_PM_QOS_MIN_FREQUENCY);
+ dev_pm_qos_remove_request(policy->max_freq_req);
+ dev_pm_qos_remove_request(policy->min_freq_req);
+ kfree(policy->min_freq_req);
cpufreq_policy_put_kobj(policy);
free_cpumask_var(policy->real_cpus);
@@ -1360,16 +1362,30 @@ static int cpufreq_online(unsigned int cpu)
cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
if (new_policy) {
- policy->user_policy.min = policy->min;
- policy->user_policy.max = policy->max;
+ struct device *dev = get_cpu_device(cpu);
for_each_cpu(j, policy->related_cpus) {
per_cpu(cpufreq_cpu_data, j) = policy;
add_cpu_dev_symlink(policy, j);
}
- } else {
- policy->min = policy->user_policy.min;
- policy->max = policy->user_policy.max;
+
+ ret = dev_pm_qos_add_request(dev, policy->min_freq_req,
+ DEV_PM_QOS_MIN_FREQUENCY,
+ policy->min);
+ if (ret < 0) {
+ dev_err(dev, "Failed to add min-freq constraint (%d)\n",
+ ret);
+ goto out_destroy_policy;
+ }
+
+ ret = dev_pm_qos_add_request(dev, policy->max_freq_req,
+ DEV_PM_QOS_MAX_FREQUENCY,
+ policy->max);
+ if (ret < 0) {
+ dev_err(dev, "Failed to add max-freq constraint (%d)\n",
+ ret);
+ goto out_destroy_policy;
+ }
}
if (cpufreq_driver->get && has_target()) {
@@ -2345,7 +2361,6 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
{
struct cpufreq_governor *old_gov;
struct device *cpu_dev = get_cpu_device(policy->cpu);
- unsigned long min, max;
int ret;
pr_debug("setting new policy for CPU %u: %u - %u kHz\n",
@@ -2353,24 +2368,12 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
memcpy(&new_policy->cpuinfo, &policy->cpuinfo, sizeof(policy->cpuinfo));
- /*
- * This check works well when we store new min/max freq attributes,
- * because new_policy is a copy of policy with one field updated.
- */
- if (new_policy->min > new_policy->max)
- return -EINVAL;
-
/*
* PM QoS framework collects all the requests from users and provide us
* the final aggregated value here.
*/
- min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
- max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
-
- if (min > new_policy->min)
- new_policy->min = min;
- if (max < new_policy->max)
- new_policy->max = max;
+ new_policy->min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
+ new_policy->max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
/* verify the cpu speed can be set within this limit */
ret = cpufreq_driver->verify(new_policy);
@@ -2459,10 +2462,9 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
* @cpu: CPU to re-evaluate the policy for.
*
* Update the current frequency for the cpufreq policy of @cpu and use
- * cpufreq_set_policy() to re-apply the min and max limits saved in the
- * user_policy sub-structure of that policy, which triggers the evaluation
- * of policy notifiers and the cpufreq driver's ->verify() callback for the
- * policy in question, among other things.
+ * cpufreq_set_policy() to re-apply the min and max limits, which triggers the
+ * evaluation of policy notifiers and the cpufreq driver's ->verify() callback
+ * for the policy in question, among other things.
*/
void cpufreq_update_policy(unsigned int cpu)
{
@@ -2522,10 +2524,9 @@ static int cpufreq_boost_set_sw(int state)
break;
}
- down_write(&policy->rwsem);
- policy->user_policy.max = policy->max;
- cpufreq_governor_limits(policy);
- up_write(&policy->rwsem);
+ ret = dev_pm_qos_update_request(policy->max_freq_req, policy->max);
+ if (ret)
+ break;
}
return ret;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d8622cf3f46c..b6245f512a26 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -50,11 +50,6 @@ struct cpufreq_cpuinfo {
unsigned int transition_latency;
};
-struct cpufreq_user_policy {
- unsigned int min; /* in kHz */
- unsigned int max; /* in kHz */
-};
-
struct cpufreq_policy {
/* CPUs sharing clock, require sw coordination */
cpumask_var_t cpus; /* Online CPUs only */
@@ -84,7 +79,8 @@ struct cpufreq_policy {
struct work_struct update; /* if update_policy() needs to be
* called, but you're in IRQ context */
- struct cpufreq_user_policy user_policy;
+ struct dev_pm_qos_request *min_freq_req;
+ struct dev_pm_qos_request *max_freq_req;
struct cpufreq_frequency_table *freq_table;
enum cpufreq_table_sorting freq_table_sorted;
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V4 5/6] cpufreq: Register notifiers with the PM QoS framework
From: Viresh Kumar @ 2019-06-20 7:35 UTC (permalink / raw)
To: Rafael Wysocki
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
juri.lelli, Ulf Hansson, linux-kernel
In-Reply-To: <cover.1561014965.git.viresh.kumar@linaro.org>
This registers the notifiers for min/max frequency constraints with the
PM QoS framework. The constraints are also taken into consideration in
cpufreq_set_policy().
This also relocates cpufreq_policy_put_kobj() as it is required to be
called from cpufreq_policy_alloc() now.
reeval_frequency_limits() is updated to have proper locking in place and
avoid calling cpufreq_set_policy() for inactive policies.
No constraints are added until now though.
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 135 ++++++++++++++++++++++++++++++--------
include/linux/cpufreq.h | 3 +
2 files changed, 109 insertions(+), 29 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 0a73de7aae54..b47a6c094171 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -26,6 +26,7 @@
#include <linux/kernel_stat.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/pm_qos.h>
#include <linux/slab.h>
#include <linux/suspend.h>
#include <linux/syscore_ops.h>
@@ -999,7 +1000,7 @@ static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu)
{
struct device *dev = get_cpu_device(cpu);
- if (!dev)
+ if (unlikely(!dev))
return;
if (cpumask_test_and_set_cpu(cpu, policy->real_cpus))
@@ -1117,14 +1118,20 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp
static void reeval_frequency_limits(struct cpufreq_policy *policy)
{
- struct cpufreq_policy new_policy = *policy;
+ struct cpufreq_policy new_policy;
- pr_debug("updating policy for CPU %u\n", policy->cpu);
+ down_write(&policy->rwsem);
+
+ if (!policy_is_inactive(policy)) {
+ new_policy = *policy;
+ pr_debug("updating policy for CPU %u\n", policy->cpu);
- new_policy.min = policy->user_policy.min;
- new_policy.max = policy->user_policy.max;
+ new_policy.min = policy->user_policy.min;
+ new_policy.max = policy->user_policy.max;
+ cpufreq_set_policy(policy, &new_policy);
+ }
- cpufreq_set_policy(policy, &new_policy);
+ up_write(&policy->rwsem);
}
static void handle_update(struct work_struct *work)
@@ -1136,11 +1143,55 @@ static void handle_update(struct work_struct *work)
reeval_frequency_limits(policy);
}
+static int cpufreq_notifier_min(struct notifier_block *nb, unsigned long freq,
+ void *data)
+{
+ struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_min);
+
+ schedule_work(&policy->update);
+ return 0;
+}
+
+static int cpufreq_notifier_max(struct notifier_block *nb, unsigned long freq,
+ void *data)
+{
+ struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_max);
+
+ schedule_work(&policy->update);
+ return 0;
+}
+
+static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
+{
+ struct kobject *kobj;
+ struct completion *cmp;
+
+ down_write(&policy->rwsem);
+ cpufreq_stats_free_table(policy);
+ kobj = &policy->kobj;
+ cmp = &policy->kobj_unregister;
+ up_write(&policy->rwsem);
+ kobject_put(kobj);
+
+ /*
+ * We need to make sure that the underlying kobj is
+ * actually not referenced anymore by anybody before we
+ * proceed with unloading.
+ */
+ pr_debug("waiting for dropping of refcount\n");
+ wait_for_completion(cmp);
+ pr_debug("wait complete\n");
+}
+
static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
{
struct cpufreq_policy *policy;
+ struct device *dev = get_cpu_device(cpu);
int ret;
+ if (!dev)
+ return NULL;
+
policy = kzalloc(sizeof(*policy), GFP_KERNEL);
if (!policy)
return NULL;
@@ -1157,7 +1208,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
cpufreq_global_kobject, "policy%u", cpu);
if (ret) {
- pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
+ dev_err(dev, "%s: failed to init policy->kobj: %d\n", __func__, ret);
/*
* The entire policy object will be freed below, but the extra
* memory allocated for the kobject name needs to be freed by
@@ -1167,6 +1218,25 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
goto err_free_real_cpus;
}
+ policy->nb_min.notifier_call = cpufreq_notifier_min;
+ policy->nb_max.notifier_call = cpufreq_notifier_max;
+
+ ret = dev_pm_qos_add_notifier(dev, &policy->nb_min,
+ DEV_PM_QOS_MIN_FREQUENCY);
+ if (ret) {
+ dev_err(dev, "Failed to register MIN QoS notifier: %d (%*pbl)\n",
+ ret, cpumask_pr_args(policy->cpus));
+ goto err_kobj_remove;
+ }
+
+ ret = dev_pm_qos_add_notifier(dev, &policy->nb_max,
+ DEV_PM_QOS_MAX_FREQUENCY);
+ if (ret) {
+ dev_err(dev, "Failed to register MAX QoS notifier: %d (%*pbl)\n",
+ ret, cpumask_pr_args(policy->cpus));
+ goto err_min_qos_notifier;
+ }
+
INIT_LIST_HEAD(&policy->policy_list);
init_rwsem(&policy->rwsem);
spin_lock_init(&policy->transition_lock);
@@ -1177,6 +1247,11 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
policy->cpu = cpu;
return policy;
+err_min_qos_notifier:
+ dev_pm_qos_remove_notifier(dev, &policy->nb_min,
+ DEV_PM_QOS_MIN_FREQUENCY);
+err_kobj_remove:
+ cpufreq_policy_put_kobj(policy);
err_free_real_cpus:
free_cpumask_var(policy->real_cpus);
err_free_rcpumask:
@@ -1189,30 +1264,9 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
return NULL;
}
-static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
-{
- struct kobject *kobj;
- struct completion *cmp;
-
- down_write(&policy->rwsem);
- cpufreq_stats_free_table(policy);
- kobj = &policy->kobj;
- cmp = &policy->kobj_unregister;
- up_write(&policy->rwsem);
- kobject_put(kobj);
-
- /*
- * We need to make sure that the underlying kobj is
- * actually not referenced anymore by anybody before we
- * proceed with unloading.
- */
- pr_debug("waiting for dropping of refcount\n");
- wait_for_completion(cmp);
- pr_debug("wait complete\n");
-}
-
static void cpufreq_policy_free(struct cpufreq_policy *policy)
{
+ struct device *dev = get_cpu_device(policy->cpu);
unsigned long flags;
int cpu;
@@ -1224,6 +1278,11 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
per_cpu(cpufreq_cpu_data, cpu) = NULL;
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+ dev_pm_qos_remove_notifier(dev, &policy->nb_max,
+ DEV_PM_QOS_MAX_FREQUENCY);
+ dev_pm_qos_remove_notifier(dev, &policy->nb_min,
+ DEV_PM_QOS_MIN_FREQUENCY);
+
cpufreq_policy_put_kobj(policy);
free_cpumask_var(policy->real_cpus);
free_cpumask_var(policy->related_cpus);
@@ -2285,6 +2344,8 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
struct cpufreq_policy *new_policy)
{
struct cpufreq_governor *old_gov;
+ struct device *cpu_dev = get_cpu_device(policy->cpu);
+ unsigned long min, max;
int ret;
pr_debug("setting new policy for CPU %u: %u - %u kHz\n",
@@ -2299,11 +2360,27 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
if (new_policy->min > new_policy->max)
return -EINVAL;
+ /*
+ * PM QoS framework collects all the requests from users and provide us
+ * the final aggregated value here.
+ */
+ min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
+ max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
+
+ if (min > new_policy->min)
+ new_policy->min = min;
+ if (max < new_policy->max)
+ new_policy->max = max;
+
/* verify the cpu speed can be set within this limit */
ret = cpufreq_driver->verify(new_policy);
if (ret)
return ret;
+ /*
+ * The notifier-chain shall be removed once all the users of
+ * CPUFREQ_ADJUST are moved to use the QoS framework.
+ */
/* adjust if necessary - all reasons */
blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
CPUFREQ_ADJUST, new_policy);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d01a74fbc4db..d8622cf3f46c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -147,6 +147,9 @@ struct cpufreq_policy {
/* Pointer to the cooling device if used for thermal mitigation */
struct thermal_cooling_device *cdev;
+
+ struct notifier_block nb_min;
+ struct notifier_block nb_max;
};
struct cpufreq_freqs {
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V4 0/6] cpufreq: Use QoS layer to manage freq-constraints
From: Viresh Kumar @ 2019-06-20 7:35 UTC (permalink / raw)
To: Rafael Wysocki, Daniel Lezcano, Kevin Hilman, Len Brown,
Pavel Machek, Ulf Hansson
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
juri.lelli, linux-kernel
Hello,
This patchset attempts to manage CPU frequency constraints using the PM
QoS framework. It only does the basic stuff right now and moves the
userspace constraints to use the QoS infrastructure.
This is rebased over pm/linux-next and another cleanup series [1].
Todo:
- Migrate all users to the QoS framework and get rid of cpufreq specific
notifiers.
- Make PM QoS learn about the relation of CPUs in a policy, so a single
list of constraints is managed for all of them instead of per-cpu
constraints.
V3->V4:
- Few commit logs updated as suggested during reviews.
- Separate commit (2/6) to create resume-latency specific routines
- Reused earlier work ("update") for notifiers as well.
- Kept Reviewed-by tags as is as the patches normally got better only.
Please take them back if you find any issues.
V2->V3:
- Add a comment in cpufreq.c as suggested by Qais.
- Rebased on latest pm/linux-next.
V1->V2:
- The previous version introduced a completely new framework, this one
moves to PM QoS instead.
- Lots of changes because of this.
--
viresh
[1] http://lore.kernel.org/lkml/cover.1560999838.git.viresh.kumar@linaro.org
Viresh Kumar (6):
PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()
PM / QOS: Rename __dev_pm_qos_read_value() and
dev_pm_qos_raw_read_value()
PM / QOS: Pass request type to dev_pm_qos_read_value()
PM / QoS: Add support for MIN/MAX frequency constraints
cpufreq: Register notifiers with the PM QoS framework
cpufreq: Add QoS requests for userspace constraints
Documentation/power/pm_qos_interface.txt | 12 +-
drivers/base/power/domain.c | 8 +-
drivers/base/power/domain_governor.c | 4 +-
drivers/base/power/qos.c | 135 ++++++++++++++--
drivers/base/power/runtime.c | 2 +-
drivers/cpufreq/cpufreq.c | 198 ++++++++++++++++-------
drivers/cpuidle/governor.c | 2 +-
include/linux/cpufreq.h | 11 +-
include/linux/pm_qos.h | 48 ++++--
9 files changed, 316 insertions(+), 104 deletions(-)
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply
* Re: [PATCH 2/2] PM / devfreq: Sanitize prints
From: Chanwoo Choi @ 2019-06-20 7:23 UTC (permalink / raw)
To: Ezequiel Garcia, Kyungmin Park, MyungJoo Ham
Cc: kernel, linux-pm, Enric Balletbo i Serra
In-Reply-To: <20190605190053.19177-2-ezequiel@collabora.com>
Hi,
Frankly, I don't like the existing 'DEVFREQ: ' prefix
because it is not used on all error log and it is not necessary.
Instead of this patch, I just prefer to delete the 'DEVFREQ: ' prefix
On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote:
> This commit is a simple cosmetic cleanup, where pr_fmt is used to avoid
> the "DEVFREQ" prefix in some prints.
>
> Also, messages are changed to not start with a capital. This is just
> a cosmetic change, meant to sanitize all prints from this file.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> drivers/devfreq/devfreq.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 8868ad9472d2..44392fa1c570 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -10,6 +10,8 @@
> * published by the Free Software Foundation.
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/kernel.h>
> #include <linux/kmod.h>
> #include <linux/sched.h>
> @@ -59,7 +61,7 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> struct devfreq *tmp_devfreq;
>
> if (IS_ERR_OR_NULL(dev)) {
> - pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> + pr_err("%s: invalid parameters\n", __func__);
> return ERR_PTR(-EINVAL);
> }
> WARN(!mutex_is_locked(&devfreq_list_lock),
> @@ -208,7 +210,7 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
> struct devfreq_governor *tmp_governor;
>
> if (IS_ERR_OR_NULL(name)) {
> - pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> + pr_err("%s: invalid parameters\n", __func__);
> return ERR_PTR(-EINVAL);
> }
> WARN(!mutex_is_locked(&devfreq_list_lock),
> @@ -238,7 +240,7 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
> struct devfreq_governor *governor;
>
> if (IS_ERR_OR_NULL(name)) {
> - pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
> + pr_err("%s: invalid parameters\n", __func__);
> return ERR_PTR(-EINVAL);
> }
> WARN(!mutex_is_locked(&devfreq_list_lock),
> @@ -1001,7 +1003,7 @@ int devfreq_add_governor(struct devfreq_governor *governor)
> int err = 0;
>
> if (!governor) {
> - pr_err("%s: Invalid parameters.\n", __func__);
> + pr_err("%s: invalid parameters.\n", __func__);
> return -EINVAL;
> }
>
> @@ -1066,7 +1068,7 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
> int err = 0;
>
> if (!governor) {
> - pr_err("%s: Invalid parameters.\n", __func__);
> + pr_err("%s: invalid parameters.\n", __func__);
> return -EINVAL;
> }
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH RFC 1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor
From: Chanwoo Choi @ 2019-06-20 6:58 UTC (permalink / raw)
To: Hsin-Yi Wang, linux-arm-kernel, Saravana Kannan, Sibi Sankar
Cc: MyungJoo Ham, Kyungmin Park, Matthias Brugger, linux-pm,
linux-mediatek, linux-kernel, Andrew-sh . Cheng
In-Reply-To: <20190618041433.245629-2-hsinyi@chromium.org>
+ Sibi Sankar
Hi, Hsin-Yi Wang, Saravana Kannan and Sibi Sankar
I summarized the history of the related patch about this title.
Firstly,
As I knew, Saravana sent the patch[1] which contains
'governor_cpufreq_map.c' last year. According to the Myungoo's comment,
Secondly,
Sibi Sankar modified the 'governor_passive.c'[2] in order to support
the mapping between cpu frequency and device frequency.
Unfortunately, Sibi Sankar stopped the development about this
because he had found the other method to get his purpose as I knew.
Thirdly,
Hsin-Yi Wang send the original patch of Saravana without modification.
Sincerely, I think that the mapping between cpu frequency and device
frequency is necessary. And I prefer the Sibi's approach which implements
stuff to the existing 'passive' governor.
We need to discuss about how to implement them by whom.
[1] [v3,1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor
- https://patchwork.kernel.org/patch/10553171/
[2]
[PATCH RFC 0/9] Add CPU based scaling support to Passive governor
- https://lore.kernel.org/lkml/08c3cff8c39e3d82e044db93e992da72@codeaurora.org/T/
[PATCH RFC 3/9] PM / devfreq: Add cpu based scaling support to passive_governor
- https://lore.kernel.org/lkml/08c3cff8c39e3d82e044db93e992da72@codeaurora.org/T/#m1cafb7baf687d2a680d39c85d3ec7d1b590b68fc
Best Regards,
Chanwoo Choi
On 19. 6. 18. 오후 1:14, Hsin-Yi Wang wrote:
> From: Saravana Kannan <skannan@codeaurora.org>
>
> From: Saravana Kannan <skannan@codeaurora.org>
>
> Many CPU architectures have caches that can scale independent of the CPUs.
> Frequency scaling of the caches is necessary to make sure the cache is not
> a performance bottleneck that leads to poor performance and power. The same
> idea applies for RAM/DDR.
>
> To achieve this, this patch adds a generic devfreq governor that takes the
> current frequency of each CPU frequency domain and then adjusts the
> frequency of the cache (or any devfreq device) based on the frequency of
> the CPUs. It listens to CPU frequency transition notifiers to keep itself
> up to date on the current CPU frequency.
>
> To decide the frequency of the device, the governor does one of the
> following:
>
> * Uses a CPU frequency to device frequency mapping table
> - Either one mapping table used for all CPU freq policies (typically used
> for system with homogeneous cores/clusters that have the same OPPs).
> - One mapping table per CPU freq policy (typically used for ASMP systems
> with heterogeneous CPUs with different OPPs)
>
> OR
>
> * Scales the device frequency in proportion to the CPU frequency. So, if
> the CPUs are running at their max frequency, the device runs at its max
> frequency. If the CPUs are running at their min frequency, the device
> runs at its min frequency. And interpolated for frequencies in between.
>
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> .../bindings/devfreq/devfreq-cpufreq-map.txt | 53 ++
> drivers/devfreq/Kconfig | 8 +
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/governor_cpufreq_map.c | 583 ++++++++++++++++++
> 4 files changed, 645 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> create mode 100644 drivers/devfreq/governor_cpufreq_map.c
>
> diff --git a/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> new file mode 100644
> index 000000000000..982a30bcfc86
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> @@ -0,0 +1,53 @@
> +Devfreq CPUfreq governor
> +
> +devfreq-cpufreq-map is a parent device that contains one or more child devices.
> +Each child device provides CPU frequency to device frequency mapping for a
> +specific device. Examples of devices that could use this are: DDR, cache and
> +CCI.
> +
> +Parent device name shall be "devfreq-cpufreq-map".
> +
> +Required child device properties:
> +- cpu-to-dev-map, or cpu-to-dev-map-<X>:
> + A list of tuples where each tuple consists of a
> + CPU frequency (KHz) and the corresponding device
> + frequency. CPU frequencies not listed in the table
> + will use the device frequency that corresponds to the
> + next rounded up CPU frequency.
> + Use "cpu-to-dev-map" if all CPUs in the system should
> + share same mapping.
> + Use cpu-to-dev-map-<cpuid> to describe different
> + mappings for different CPUs. The property should be
> + listed only for the first CPU if multiple CPUs are
> + synchronous.
> +- target-dev: Phandle to device that this mapping applies to.
> +
> +Example:
> + devfreq-cpufreq-map {
> + cpubw-cpufreq {
> + target-dev = <&cpubw>;
> + cpu-to-dev-map =
> + < 300000 1144000 >,
> + < 422400 2288000 >,
> + < 652800 3051000 >,
> + < 883200 5996000 >,
> + < 1190400 8056000 >,
> + < 1497600 10101000 >,
> + < 1728000 12145000 >,
> + < 2649600 16250000 >;
> + };
> +
> + cache-cpufreq {
> + target-dev = <&cache>;
> + cpu-to-dev-map =
> + < 300000 300000 >,
> + < 422400 422400 >,
> + < 652800 499200 >,
> + < 883200 576000 >,
> + < 960000 960000 >,
> + < 1497600 1036800 >,
> + < 1574400 1574400 >,
> + < 1728000 1651200 >,
> + < 2649600 1728000 >;
> + };
> + };
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 0c8204d6b78a..0303f5a400b6 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -74,6 +74,14 @@ config DEVFREQ_GOV_PASSIVE
> through sysfs entries. The passive governor recommends that
> devfreq device uses the OPP table to get the frequency/voltage.
>
> +config DEVFREQ_GOV_CPUFREQ_MAP
> + tristate "CPUfreq Map"
> + depends on CPU_FREQ
> + help
> + Chooses frequency based on the online CPUs' current frequency and a
> + CPU frequency to device frequency mapping table(s). This governor
> + can be useful for controlling devices such as DDR, cache, CCI, etc.
> +
> comment "DEVFREQ Drivers"
>
> config ARM_EXYNOS_BUS_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 817dde779f16..81141e2c784f 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE) += governor_performance.o
> obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
> obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
> obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
> +obj-$(CONFIG_DEVFREQ_GOV_CPUFREQ_MAP) += governor_cpufreq_map.o
>
> # DEVFREQ Drivers
> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> diff --git a/drivers/devfreq/governor_cpufreq_map.c b/drivers/devfreq/governor_cpufreq_map.c
> new file mode 100644
> index 000000000000..084a3ffb8f54
> --- /dev/null
> +++ b/drivers/devfreq/governor_cpufreq_map.c
> @@ -0,0 +1,583 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt) "dev-cpufreq-map: " fmt
> +
> +#include <linux/devfreq.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include "governor.h"
> +
> +struct cpu_state {
> + unsigned int freq;
> + unsigned int min_freq;
> + unsigned int max_freq;
> + unsigned int first_cpu;
> +};
> +static struct cpu_state *state[NR_CPUS];
> +static int cpufreq_cnt;
> +
> +struct freq_map {
> + unsigned int cpu_khz;
> + unsigned int target_freq;
> +};
> +
> +struct devfreq_node {
> + struct devfreq *df;
> + void *orig_data;
> + struct device *dev;
> + struct device_node *of_node;
> + struct list_head list;
> + struct freq_map **map;
> + struct freq_map *common_map;
> +};
> +static LIST_HEAD(devfreq_list);
> +static DEFINE_MUTEX(state_lock);
> +static DEFINE_MUTEX(cpufreq_reg_lock);
> +
> +static void update_all_devfreqs(void)
> +{
> + struct devfreq_node *node;
> +
> + list_for_each_entry(node, &devfreq_list, list) {
> + struct devfreq *df = node->df;
> +
> + if (!node->df)
> + continue;
> + mutex_lock(&df->lock);
> + update_devfreq(df);
> + mutex_unlock(&df->lock);
> +
> + }
> +}
> +
> +static struct devfreq_node *find_devfreq_node(struct device *dev)
> +{
> + struct devfreq_node *node;
> +
> + list_for_each_entry(node, &devfreq_list, list)
> + if (node->dev == dev || node->of_node == dev->of_node)
> + return node;
> +
> + return NULL;
> +}
> +
> +/* ==================== cpufreq part ==================== */
> +static struct cpu_state *add_policy(struct cpufreq_policy *policy)
> +{
> + struct cpu_state *new_state;
> + unsigned int cpu, first_cpu;
> +
> + new_state = kzalloc(sizeof(struct cpu_state), GFP_KERNEL);
> + if (!new_state)
> + return NULL;
> +
> + first_cpu = cpumask_first(policy->related_cpus);
> + new_state->first_cpu = first_cpu;
> + new_state->freq = policy->cur;
> + new_state->min_freq = policy->cpuinfo.min_freq;
> + new_state->max_freq = policy->cpuinfo.max_freq;
> +
> + for_each_cpu(cpu, policy->related_cpus)
> + state[cpu] = new_state;
> +
> + return new_state;
> +}
> +
> +static int cpufreq_trans_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct cpufreq_freqs *freq = data;
> + struct cpu_state *s;
> + struct cpufreq_policy *policy = NULL;
> +
> + if (event != CPUFREQ_POSTCHANGE)
> + return 0;
> +
> + mutex_lock(&state_lock);
> +
> + s = state[freq->cpu];
> + if (!s) {
> + policy = cpufreq_cpu_get(freq->cpu);
> + if (policy) {
> + s = add_policy(policy);
> + cpufreq_cpu_put(policy);
> + }
> + }
> + if (!s)
> + goto out;
> +
> + if (s->freq != freq->new || policy) {
> + s->freq = freq->new;
> + update_all_devfreqs();
> + }
> +
> +out:
> + mutex_unlock(&state_lock);
> + return 0;
> +}
> +
> +static struct notifier_block cpufreq_trans_nb = {
> + .notifier_call = cpufreq_trans_notifier
> +};
> +
> +static int register_cpufreq(void)
> +{
> + int ret = 0;
> + unsigned int cpu;
> + struct cpufreq_policy *policy;
> +
> + mutex_lock(&cpufreq_reg_lock);
> +
> + if (cpufreq_cnt)
> + goto cnt_not_zero;
> +
> + get_online_cpus();
> + ret = cpufreq_register_notifier(&cpufreq_trans_nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> + if (ret)
> + goto out;
> +
> + for_each_online_cpu(cpu) {
> + policy = cpufreq_cpu_get(cpu);
> + if (policy) {
> + add_policy(policy);
> + cpufreq_cpu_put(policy);
> + }
> + }
> +out:
> + put_online_cpus();
> +cnt_not_zero:
> + if (!ret)
> + cpufreq_cnt++;
> + mutex_unlock(&cpufreq_reg_lock);
> + return ret;
> +}
> +
> +static int unregister_cpufreq(void)
> +{
> + int ret = 0;
> + int cpu;
> +
> + mutex_lock(&cpufreq_reg_lock);
> +
> + if (cpufreq_cnt > 1)
> + goto out;
> +
> + cpufreq_unregister_notifier(&cpufreq_trans_nb,
> + CPUFREQ_TRANSITION_NOTIFIER);
> +
> + for (cpu = ARRAY_SIZE(state) - 1; cpu >= 0; cpu--) {
> + if (!state[cpu])
> + continue;
> + if (state[cpu]->first_cpu == cpu)
> + kfree(state[cpu]);
> + state[cpu] = NULL;
> + }
> +
> +out:
> + cpufreq_cnt--;
> + mutex_unlock(&cpufreq_reg_lock);
> + return ret;
> +}
> +
> +/* ==================== devfreq part ==================== */
> +
> +static unsigned int interpolate_freq(struct devfreq *df, unsigned int cpu)
> +{
> + unsigned long *freq_table = df->profile->freq_table;
> + unsigned int cpu_min = state[cpu]->min_freq;
> + unsigned int cpu_max = state[cpu]->max_freq;
> + unsigned int cpu_freq = state[cpu]->freq;
> + unsigned int dev_min, dev_max, cpu_percent;
> +
> + if (freq_table) {
> + dev_min = freq_table[0];
> + dev_max = freq_table[df->profile->max_state - 1];
> + } else {
> + if (df->max_freq <= df->min_freq)
> + return 0;
> + dev_min = df->min_freq;
> + dev_max = df->max_freq;
> + }
> +
> + cpu_percent = ((cpu_freq - cpu_min) * 100) / (cpu_max - cpu_min);
> + return dev_min + mult_frac(dev_max - dev_min, cpu_percent, 100);
> +}
> +
> +static unsigned int cpu_to_dev_freq(struct devfreq *df, unsigned int cpu)
> +{
> + struct freq_map *map = NULL;
> + unsigned int cpu_khz = 0, freq;
> + struct devfreq_node *n = df->data;
> +
> + if (!state[cpu] || state[cpu]->first_cpu != cpu) {
> + freq = 0;
> + goto out;
> + }
> +
> + if (n->common_map)
> + map = n->common_map;
> + else if (n->map)
> + map = n->map[cpu];
> +
> + cpu_khz = state[cpu]->freq;
> +
> + if (!map) {
> + freq = interpolate_freq(df, cpu);
> + goto out;
> + }
> +
> + while (map->cpu_khz && map->cpu_khz < cpu_khz)
> + map++;
> + if (!map->cpu_khz)
> + map--;
> + freq = map->target_freq;
> +
> +out:
> + dev_dbg(df->dev.parent, "CPU%u: %d -> dev: %u\n", cpu, cpu_khz, freq);
> + return freq;
> +}
> +
> +static int devfreq_cpufreq_get_freq(struct devfreq *df,
> + unsigned long *freq)
> +{
> + unsigned int cpu, tgt_freq = 0;
> + struct devfreq_node *node;
> +
> + node = df->data;
> + if (!node) {
> + pr_err("Unable to find devfreq node!\n");
> + return -ENODEV;
> + }
> +
> + for_each_possible_cpu(cpu)
> + tgt_freq = max(tgt_freq, cpu_to_dev_freq(df, cpu));
> +
> + *freq = tgt_freq;
> + return 0;
> +}
> +
> +static unsigned int show_table(char *buf, unsigned int len,
> + struct freq_map *map)
> +{
> + unsigned int cnt = 0;
> +
> + cnt += snprintf(buf + cnt, len - cnt, "CPU freq\tDevice freq\n");
> +
> + while (map->cpu_khz && cnt < len) {
> + cnt += snprintf(buf + cnt, len - cnt, "%8u\t%11u\n",
> + map->cpu_khz, map->target_freq);
> + map++;
> + }
> + if (cnt < len)
> + cnt += snprintf(buf + cnt, len - cnt, "\n");
> +
> + return cnt;
> +}
> +
> +static ssize_t freq_map_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct devfreq *df = to_devfreq(dev);
> + struct devfreq_node *n = df->data;
> + struct freq_map *map;
> + unsigned int cnt = 0, cpu;
> +
> + mutex_lock(&state_lock);
> + if (n->common_map) {
> + map = n->common_map;
> + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> + "Common table for all CPUs:\n");
> + cnt += show_table(buf + cnt, PAGE_SIZE - cnt, map);
> + } else if (n->map) {
> + for_each_possible_cpu(cpu) {
> + map = n->map[cpu];
> + if (!map)
> + continue;
> + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> + "CPU %u:\n", cpu);
> + if (cnt >= PAGE_SIZE)
> + break;
> + cnt += show_table(buf + cnt, PAGE_SIZE - cnt, map);
> + if (cnt >= PAGE_SIZE)
> + break;
> + }
> + } else {
> + cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> + "Device freq interpolated based on CPU freq\n");
> + }
> + mutex_unlock(&state_lock);
> +
> + return cnt;
> +}
> +
> +static DEVICE_ATTR_RO(freq_map);
> +static struct attribute *dev_attr[] = {
> + &dev_attr_freq_map.attr,
> + NULL,
> +};
> +
> +static struct attribute_group dev_attr_group = {
> + .name = "cpufreq-map",
> + .attrs = dev_attr,
> +};
> +
> +static int devfreq_cpufreq_gov_start(struct devfreq *devfreq)
> +{
> + int ret = 0;
> + struct devfreq_node *node;
> + bool alloc = false;
> +
> + ret = register_cpufreq();
> + if (ret)
> + return ret;
> +
> + ret = sysfs_create_group(&devfreq->dev.kobj, &dev_attr_group);
> + if (ret) {
> + unregister_cpufreq();
> + return ret;
> + }
> +
> + mutex_lock(&state_lock);
> +
> + node = find_devfreq_node(devfreq->dev.parent);
> + if (node == NULL) {
> + node = kzalloc(sizeof(struct devfreq_node), GFP_KERNEL);
> + if (!node) {
> + ret = -ENOMEM;
> + goto alloc_fail;
> + }
> + alloc = true;
> + node->dev = devfreq->dev.parent;
> + list_add_tail(&node->list, &devfreq_list);
> + }
> + node->df = devfreq;
> + node->orig_data = devfreq->data;
> + devfreq->data = node;
> +
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
> + if (ret) {
> + pr_err("Freq update failed!\n");
> + goto update_fail;
> + }
> +
> + mutex_unlock(&state_lock);
> + return 0;
> +
> +update_fail:
> + devfreq->data = node->orig_data;
> + if (alloc) {
> + list_del(&node->list);
> + kfree(node);
> + }
> +alloc_fail:
> + mutex_unlock(&state_lock);
> + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
> + unregister_cpufreq();
> + return ret;
> +}
> +
> +static void devfreq_cpufreq_gov_stop(struct devfreq *devfreq)
> +{
> + struct devfreq_node *node = devfreq->data;
> +
> + mutex_lock(&state_lock);
> + devfreq->data = node->orig_data;
> + if (node->map || node->common_map) {
> + node->df = NULL;
> + } else {
> + list_del(&node->list);
> + kfree(node);
> + }
> + mutex_unlock(&state_lock);
> +
> + sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
> + unregister_cpufreq();
> +}
> +
> +static int devfreq_cpufreq_ev_handler(struct devfreq *devfreq,
> + unsigned int event, void *data)
> +{
> + int ret;
> +
> + switch (event) {
> + case DEVFREQ_GOV_START:
> +
> + ret = devfreq_cpufreq_gov_start(devfreq);
> + if (ret) {
> + pr_err("Governor start failed!\n");
> + return ret;
> + }
> + pr_debug("Enabled CPUfreq-map governor\n");
> + break;
> +
> + case DEVFREQ_GOV_STOP:
> +
> + devfreq_cpufreq_gov_stop(devfreq);
> + pr_debug("Disabled dev CPUfreq-map governor\n");
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static struct devfreq_governor devfreq_cpufreq = {
> + .name = "cpufreq-map",
> + .get_target_freq = devfreq_cpufreq_get_freq,
> + .event_handler = devfreq_cpufreq_ev_handler,
> +};
> +
> +#define NUM_COLS 2
> +static struct freq_map *read_tbl(struct device_node *of_node, char *prop_name)
> +{
> + int len, nf, i, j;
> + u32 data;
> + struct freq_map *tbl;
> +
> + if (!of_find_property(of_node, prop_name, &len))
> + return NULL;
> + len /= sizeof(data);
> +
> + if (len % NUM_COLS || len == 0)
> + return NULL;
> + nf = len / NUM_COLS;
> +
> + tbl = kzalloc((nf + 1) * sizeof(*tbl), GFP_KERNEL);
> + if (!tbl)
> + return NULL;
> +
> + for (i = 0, j = 0; i < nf; i++, j += 2) {
> + of_property_read_u32_index(of_node, prop_name, j, &data);
> + tbl[i].cpu_khz = data;
> +
> + of_property_read_u32_index(of_node, prop_name, j + 1, &data);
> + tbl[i].target_freq = data;
> + }
> + tbl[i].cpu_khz = 0;
> +
> + return tbl;
> +}
> +
> +#define PROP_TARGET "target-dev"
> +#define PROP_TABLE "cpu-to-dev-map"
> +static int add_table_from_of(struct device_node *of_node)
> +{
> + struct device_node *target_of_node;
> + struct devfreq_node *node;
> + struct freq_map *common_tbl;
> + struct freq_map **tbl_list = NULL;
> + static char prop_name[] = PROP_TABLE "-999999";
> + int cpu, ret, cnt = 0, prop_sz = ARRAY_SIZE(prop_name);
> +
> + target_of_node = of_parse_phandle(of_node, PROP_TARGET, 0);
> + if (!target_of_node)
> + return -EINVAL;
> +
> + node = kzalloc(sizeof(struct devfreq_node), GFP_KERNEL);
> + if (!node)
> + return -ENOMEM;
> +
> + common_tbl = read_tbl(of_node, PROP_TABLE);
> + if (!common_tbl) {
> + tbl_list = kzalloc(sizeof(*tbl_list) * NR_CPUS, GFP_KERNEL);
> + if (!tbl_list) {
> + ret = -ENOMEM;
> + goto err_list;
> + }
> +
> + for_each_possible_cpu(cpu) {
> + ret = snprintf(prop_name, prop_sz, "%s-%d",
> + PROP_TABLE, cpu);
> + if (ret >= prop_sz) {
> + pr_warn("More CPUs than I can handle!\n");
> + pr_warn("Skipping rest of the tables!\n");
> + break;
> + }
> + tbl_list[cpu] = read_tbl(of_node, prop_name);
> + if (tbl_list[cpu])
> + cnt++;
> + }
> + }
> + if (!common_tbl && !cnt) {
> + ret = -EINVAL;
> + goto err_tbl;
> + }
> +
> + mutex_lock(&state_lock);
> + node->of_node = target_of_node;
> + node->map = tbl_list;
> + node->common_map = common_tbl;
> + list_add_tail(&node->list, &devfreq_list);
> + mutex_unlock(&state_lock);
> +
> + return 0;
> +err_tbl:
> + kfree(tbl_list);
> +err_list:
> + kfree(node);
> + return ret;
> +}
> +
> +static int __init devfreq_cpufreq_init(void)
> +{
> + int ret;
> + struct device_node *of_par, *of_child;
> +
> + of_par = of_find_node_by_name(NULL, "devfreq-cpufreq-map");
> + if (of_par) {
> + for_each_child_of_node(of_par, of_child) {
> + ret = add_table_from_of(of_child);
> + if (ret)
> + pr_err("Parsing %s failed!\n", of_child->name);
> + else
> + pr_debug("Parsed %s.\n", of_child->name);
> + }
> + of_node_put(of_par);
> + } else {
> + pr_info("No tables parsed from DT.\n");
> + }
> +
> + ret = devfreq_add_governor(&devfreq_cpufreq);
> + if (ret) {
> + pr_err("cpufreq-map governor add failed!\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +subsys_initcall(devfreq_cpufreq_init);
> +
> +static void __exit devfreq_cpufreq_exit(void)
> +{
> + int ret, cpu;
> + struct devfreq_node *node, *tmp;
> +
> + ret = devfreq_remove_governor(&devfreq_cpufreq);
> + if (ret)
> + pr_err("cpufreq-map governor remove failed!\n");
> +
> + mutex_lock(&state_lock);
> + list_for_each_entry_safe(node, tmp, &devfreq_list, list) {
> + kfree(node->common_map);
> + for_each_possible_cpu(cpu)
> + kfree(node->map[cpu]);
> + kfree(node->map);
> + list_del(&node->list);
> + kfree(node);
> + }
> + mutex_unlock(&state_lock);
> +}
> +module_exit(devfreq_cpufreq_exit);
> +
> +MODULE_DESCRIPTION("devfreq gov that sets dev freq based on current CPU freq");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* Re: [PATCH v4 0/3] nvme: add thermal zone devices
From: Christoph Hellwig @ 2019-06-20 6:36 UTC (permalink / raw)
To: Akinobu Mita
Cc: linux-nvme, linux-pm, Zhang Rui, Eduardo Valentin, Daniel Lezcano,
Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
Minwoo Im, Kenneth Heitke, Chaitanya Kulkarni
In-Reply-To: <1560439238-4054-1-git-send-email-akinobu.mita@gmail.com>
So the code looks perfectly fine to me. But I'm still not convinced we
have an important enough use case, and I'm worried about the AEN uevent
implication.
Keith, Sagi, Jens, any opinion?
^ permalink raw reply
* [PATCH V2 5/5] cpufreq: Avoid calling cpufreq_verify_current_freq() from handle_update()
From: Viresh Kumar @ 2019-06-20 3:05 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1560999838.git.viresh.kumar@linaro.org>
On some occasions cpufreq_verify_current_freq() schedules a work whose
callback is handle_update(), which further calls cpufreq_update_policy()
which may end up calling cpufreq_verify_current_freq() again.
On the other hand, when cpufreq_update_policy() is called from
handle_update(), the pointer to the cpufreq policy is already available
but we still call cpufreq_cpu_acquire() to get it in
cpufreq_update_policy(), which should be avoided as well.
Fix both the issues by creating another helper
reeval_frequency_limits().
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4556a53fc764..0a73de7aae54 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1115,13 +1115,25 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp
return ret;
}
+static void reeval_frequency_limits(struct cpufreq_policy *policy)
+{
+ struct cpufreq_policy new_policy = *policy;
+
+ pr_debug("updating policy for CPU %u\n", policy->cpu);
+
+ new_policy.min = policy->user_policy.min;
+ new_policy.max = policy->user_policy.max;
+
+ cpufreq_set_policy(policy, &new_policy);
+}
+
static void handle_update(struct work_struct *work)
{
struct cpufreq_policy *policy =
container_of(work, struct cpufreq_policy, update);
- unsigned int cpu = policy->cpu;
- pr_debug("handle_update for cpu %u called\n", cpu);
- cpufreq_update_policy(cpu);
+
+ pr_debug("handle_update for cpu %u called\n", policy->cpu);
+ reeval_frequency_limits(policy);
}
static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
@@ -2378,7 +2390,6 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
void cpufreq_update_policy(unsigned int cpu)
{
struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
- struct cpufreq_policy new_policy;
if (!policy)
return;
@@ -2391,12 +2402,7 @@ void cpufreq_update_policy(unsigned int cpu)
(cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
goto unlock;
- pr_debug("updating policy for CPU %u\n", cpu);
- memcpy(&new_policy, policy, sizeof(*policy));
- new_policy.min = policy->user_policy.min;
- new_policy.max = policy->user_policy.max;
-
- cpufreq_set_policy(policy, &new_policy);
+ reeval_frequency_limits(policy);
unlock:
cpufreq_cpu_release(policy);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V2 3/5] cpufreq: Use has_target() instead of !setpolicy
From: Viresh Kumar @ 2019-06-20 3:05 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1560999838.git.viresh.kumar@linaro.org>
For code consistency, use has_target() instead of !setpolicy everywhere,
as it is already done at several places. Maybe we should also use
"!has_target()" instead of "cpufreq_driver->setpolicy" where we need to
check if the driver supports setpolicy, so to use only one expression
for this kind of differentiation.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 41ac701e324f..5f5c7a516c74 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -632,7 +632,7 @@ static int cpufreq_parse_policy(char *str_governor,
}
/**
- * cpufreq_parse_governor - parse a governor string only for !setpolicy
+ * cpufreq_parse_governor - parse a governor string only for has_target()
*/
static int cpufreq_parse_governor(char *str_governor,
struct cpufreq_policy *policy)
@@ -1301,7 +1301,7 @@ static int cpufreq_online(unsigned int cpu)
policy->max = policy->user_policy.max;
}
- if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
+ if (cpufreq_driver->get && has_target()) {
policy->cur = cpufreq_driver->get(policy->cpu);
if (!policy->cur) {
pr_err("%s: ->get() failed\n", __func__);
@@ -2401,7 +2401,7 @@ void cpufreq_update_policy(unsigned int cpu)
* BIOS might change freq behind our back
* -> ask driver for current freq and notify governors about a change
*/
- if (cpufreq_driver->get && !cpufreq_driver->setpolicy &&
+ if (cpufreq_driver->get && has_target() &&
(cpufreq_suspended || WARN_ON(!cpufreq_update_current_freq(policy))))
goto unlock;
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V2 4/5] cpufreq: Reuse cpufreq_update_current_freq() in __cpufreq_get()
From: Viresh Kumar @ 2019-06-20 3:05 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1560999838.git.viresh.kumar@linaro.org>
Their implementations are quite similar, lets modify
cpufreq_update_current_freq() a little and use it from __cpufreq_get().
Also rename cpufreq_update_current_freq() to
cpufreq_verify_current_freq(), as that's what it is doing.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 70 ++++++++++++++++-----------------------
1 file changed, 28 insertions(+), 42 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 5f5c7a516c74..4556a53fc764 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1547,6 +1547,30 @@ static void cpufreq_out_of_sync(struct cpufreq_policy *policy,
cpufreq_freq_transition_end(policy, &freqs, 0);
}
+static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, bool update)
+{
+ unsigned int new_freq;
+
+ new_freq = cpufreq_driver->get(policy->cpu);
+ if (!new_freq)
+ return 0;
+
+ /*
+ * If fast frequency switching is used with the given policy, the check
+ * against policy->cur is pointless, so skip it in that case.
+ */
+ if (policy->fast_switch_enabled || !has_target())
+ return new_freq;
+
+ if (policy->cur != new_freq) {
+ cpufreq_out_of_sync(policy, new_freq);
+ if (update)
+ schedule_work(&policy->update);
+ }
+
+ return new_freq;
+}
+
/**
* cpufreq_quick_get - get the CPU frequency (in kHz) from policy->cur
* @cpu: CPU number
@@ -1602,30 +1626,10 @@ EXPORT_SYMBOL(cpufreq_quick_get_max);
static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
{
- unsigned int ret_freq = 0;
-
if (unlikely(policy_is_inactive(policy)))
- return ret_freq;
-
- ret_freq = cpufreq_driver->get(policy->cpu);
-
- /*
- * If fast frequency switching is used with the given policy, the check
- * against policy->cur is pointless, so skip it in that case too.
- */
- if (policy->fast_switch_enabled)
- return ret_freq;
-
- if (has_target() && ret_freq && policy->cur) {
- /* verify no discrepancy between actual and
- saved value exists */
- if (unlikely(ret_freq != policy->cur)) {
- cpufreq_out_of_sync(policy, ret_freq);
- schedule_work(&policy->update);
- }
- }
+ return 0;
- return ret_freq;
+ return cpufreq_verify_current_freq(policy, true);
}
/**
@@ -1652,24 +1656,6 @@ unsigned int cpufreq_get(unsigned int cpu)
}
EXPORT_SYMBOL(cpufreq_get);
-static unsigned int cpufreq_update_current_freq(struct cpufreq_policy *policy)
-{
- unsigned int new_freq;
-
- new_freq = cpufreq_driver->get(policy->cpu);
- if (!new_freq)
- return 0;
-
- if (!policy->cur) {
- pr_debug("cpufreq: Driver did not initialize current freq\n");
- policy->cur = new_freq;
- } else if (policy->cur != new_freq && has_target()) {
- cpufreq_out_of_sync(policy, new_freq);
- }
-
- return new_freq;
-}
-
static struct subsys_interface cpufreq_interface = {
.name = "cpufreq",
.subsys = &cpu_subsys,
@@ -2151,7 +2137,7 @@ static int cpufreq_start_governor(struct cpufreq_policy *policy)
pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
if (cpufreq_driver->get)
- cpufreq_update_current_freq(policy);
+ cpufreq_verify_current_freq(policy, false);
if (policy->governor->start) {
ret = policy->governor->start(policy);
@@ -2402,7 +2388,7 @@ void cpufreq_update_policy(unsigned int cpu)
* -> ask driver for current freq and notify governors about a change
*/
if (cpufreq_driver->get && has_target() &&
- (cpufreq_suspended || WARN_ON(!cpufreq_update_current_freq(policy))))
+ (cpufreq_suspended || WARN_ON(!cpufreq_verify_current_freq(policy, false))))
goto unlock;
pr_debug("updating policy for CPU %u\n", cpu);
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V2 2/5] cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()
From: Viresh Kumar @ 2019-06-20 3:05 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1560999838.git.viresh.kumar@linaro.org>
CPUFREQ_CONST_LOOPS was introduced in a very old commit from pre-2.6
kernel release commit 6a4a93f9c0d5 ("[CPUFREQ] Fix 'out of sync'
issue").
Probably the initial idea was to just avoid these checks for set_policy
type drivers and then things got changed over the years. And it is very
unclear why these checks are there at all.
Replace the CPUFREQ_CONST_LOOPS check with has_target(), which makes
more sense now.
cpufreq_notify_transition() is only called for has_target() type driver
and not for set_policy type, and the check is simply redundant. Remove
it as well.
Also remove () around freq comparison statement as they aren't required
and checkpatch also warns for them.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 54befd775bd6..41ac701e324f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -359,12 +359,10 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
* which is not equal to what the cpufreq core thinks is
* "old frequency".
*/
- if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
- if (policy->cur && (policy->cur != freqs->old)) {
- pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
- freqs->old, policy->cur);
- freqs->old = policy->cur;
- }
+ if (policy->cur && policy->cur != freqs->old) {
+ pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
+ freqs->old, policy->cur);
+ freqs->old = policy->cur;
}
srcu_notifier_call_chain(&cpufreq_transition_notifier_list,
@@ -1618,8 +1616,7 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
if (policy->fast_switch_enabled)
return ret_freq;
- if (ret_freq && policy->cur &&
- !(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
+ if (has_target() && ret_freq && policy->cur) {
/* verify no discrepancy between actual and
saved value exists */
if (unlikely(ret_freq != policy->cur)) {
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V2 1/5] cpufreq: Remove the redundant !setpolicy check
From: Viresh Kumar @ 2019-06-20 3:05 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
In-Reply-To: <cover.1560999838.git.viresh.kumar@linaro.org>
cpufreq_start_governor() is only called for !setpolicy case, checking it
again is not required.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 85ff958e01f1..54befd775bd6 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2153,7 +2153,7 @@ static int cpufreq_start_governor(struct cpufreq_policy *policy)
pr_debug("%s: for CPU %u\n", __func__, policy->cpu);
- if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
+ if (cpufreq_driver->get)
cpufreq_update_current_freq(policy);
if (policy->governor->start) {
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH V2 0/5] cpufreq: cleanups
From: Viresh Kumar @ 2019-06-20 3:05 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
Hi Rafael,
I accumulated these while reworking the freq-constraint series and it
would be nice if these can get in before I send the next version of
freq-constraint stuff.
These are mostly cleanups and code consolidation for better management
of code. Compile and boot tested only.
Thanks.
V1->V2:
- Merged patch 2/6 and 3/6 (now called 2/5).
- Updated commit log of 3/5 as it wasn't clear enough earlier.
Viresh Kumar (5):
cpufreq: Remove the redundant !setpolicy check
cpufreq: Replace few CPUFREQ_CONST_LOOPS checks with has_target()
cpufreq: Use has_target() instead of !setpolicy
cpufreq: Reuse cpufreq_update_current_freq() in __cpufreq_get()
cpufreq: Avoid calling cpufreq_verify_current_freq() from
handle_update()
drivers/cpufreq/cpufreq.c | 115 +++++++++++++++++---------------------
1 file changed, 52 insertions(+), 63 deletions(-)
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
From: Joel Fernandes @ 2019-06-19 20:52 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki,
Sandeep Patil, Viresh Kumar, Hridya Valsaraju, Linux PM,
Cc: Android Kernel, LKML, Alexei Starovoitov, Steven Rostedt,
Alexei Starovoitov
In-Reply-To: <CAGETcx9yWAvp0UYHQxfCkPi1ooBuYA1ZzZXVUvfyBh8XUTcbMg@mail.gmail.com>
On Wed, Jun 19, 2019 at 4:41 PM 'Saravana Kannan' via kernel-team
<kernel-team@android.com> wrote:
> > > On Wed, Jun 19, 2019, 11:55 AM 'Joel Fernandes' via kernel-team <kernel-team@android.com> wrote:
> > >>
> > >> On Wed, Jun 19, 2019 at 2:35 PM Greg Kroah-Hartman
> > >> <gregkh@linuxfoundation.org> wrote:
> > >> >
> > >> > On Wed, Jun 19, 2019 at 02:01:36PM -0400, Joel Fernandes wrote:
> > >> > > On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
> > >> > > <gregkh@linuxfoundation.org> wrote:
> > >> > > >
> > >> > > > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> > >> > > > > > It is conceivable to have a "wakeup_sources" directory under
> > >> > > > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> > >> > > > >
> > >> > > > > One of the "issues" with this is, now if you have say 100 wake up
> > >> > > > > sources, with 10 entries each, then we're talking about a 1000 sysfs
> > >> > > > > files. Each one has to be opened, and read individually. This adds
> > >> > > > > overhead and it is more convenient to read from a single file. The
> > >> > > > > problem is this single file is not ABI. So the question I guess is,
> > >> > > > > how do we solve this in both an ABI friendly way while keeping the
> > >> > > > > overhead low.
> > >> > > >
> > >> > > > How much overhead? Have you measured it, reading from virtual files is
> > >> > > > fast :)
> > >> > >
> > >> > > I measured, and it is definitely not free. If you create and read a
> > >> > > 1000 files and just return a string back, it can take up to 11-13
> > >> > > milliseconds (did not lock CPU frequencies, was just looking for
> > >> > > average ball park). This is assuming that the counter reading is just
> > >> > > doing that, and nothing else is being done to return the sysfs data
> > >> > > which is probably not always true in practice.
> > >> > >
> > >> > > Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
> > >> > > CPU scheduling competion reading sysfs can hurt the deadline. There's
> > >> > > also the question of power - we definitely have spent time in the past
> > >> > > optimizing other virtual files such as /proc/pid/smaps for this reason
> > >> > > where it spent lots of CPU time.
> > >> >
> > >> > smaps was "odd", but that was done after measurements were actually made
> > >> > to prove it was needed. That hasn't happened yet :)
> > >> >
> > >> > And is there a reason you have to do this every 16ms?
> > >>
> > >> Not every, I was just saying whenever it happens and a frame delivery
> > >> deadline is missed, then a frame drop can occur which can result in a
> > >> poor user experience.
> > >
> > >
> > > But this is not done in the UI thread context. So some thread running for more than 16ms shouldn't cause a frame drop. If it does, we have bigger problems.
> > >
> >
> > Not really. That depends on the priority of the other thread and other
> > things. It can obviously time share the same CPU as the UI thread if
> > it is not configured correctly. Even with CFS it can reduce the time
> > consumed by other "real-time" CFS threads. I am not sure what you are
> > proposing, there are also (obviously) power issues with things running
> > for long times pointlessly. We should try to do better if we can. As
> > Greg said, some study/research can be done on the use case before
> > settling for a solution (sysfs or other).
> >
>
> Agree, power and optimization is good. Just saying that the UI example
> is not a real one. If the UI thread is that poorly configured that
> some thread running for a second can cause frame drops in a multicore
> system, that's a problem with the UI framework design.
We do know that historically there are problems with the UI thread's
scheduling and folks are looking into DL scheduling for that. I was
just giving UI thread as an example, there are also other low latency
threads (audio etc). Anyway, I think we know the next steps here so we
can park this discussion for now.
^ permalink raw reply
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
From: Saravana Kannan @ 2019-06-19 20:40 UTC (permalink / raw)
To: Joel Fernandes
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki,
Sandeep Patil, Viresh Kumar, Hridya Valsaraju, Linux PM,
Cc: Android Kernel, LKML, Alexei Starovoitov, Steven Rostedt,
Alexei Starovoitov
In-Reply-To: <CAJWu+ooaDBCF06QAeddFig5myfUABd6qebJ14nd6pKaBwQq8MA@mail.gmail.com>
On Wed, Jun 19, 2019 at 1:09 PM 'Joel Fernandes' via kernel-team
<kernel-team@android.com> wrote:
>
> On Wed, Jun 19, 2019 at 3:59 PM 'Saravana Kannan' via kernel-team
> <kernel-team@android.com> wrote:
> >
> >
> >
> > On Wed, Jun 19, 2019, 11:55 AM 'Joel Fernandes' via kernel-team <kernel-team@android.com> wrote:
> >>
> >> On Wed, Jun 19, 2019 at 2:35 PM Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> >
> >> > On Wed, Jun 19, 2019 at 02:01:36PM -0400, Joel Fernandes wrote:
> >> > > On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
> >> > > <gregkh@linuxfoundation.org> wrote:
> >> > > >
> >> > > > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> >> > > > > > It is conceivable to have a "wakeup_sources" directory under
> >> > > > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> >> > > > >
> >> > > > > One of the "issues" with this is, now if you have say 100 wake up
> >> > > > > sources, with 10 entries each, then we're talking about a 1000 sysfs
> >> > > > > files. Each one has to be opened, and read individually. This adds
> >> > > > > overhead and it is more convenient to read from a single file. The
> >> > > > > problem is this single file is not ABI. So the question I guess is,
> >> > > > > how do we solve this in both an ABI friendly way while keeping the
> >> > > > > overhead low.
> >> > > >
> >> > > > How much overhead? Have you measured it, reading from virtual files is
> >> > > > fast :)
> >> > >
> >> > > I measured, and it is definitely not free. If you create and read a
> >> > > 1000 files and just return a string back, it can take up to 11-13
> >> > > milliseconds (did not lock CPU frequencies, was just looking for
> >> > > average ball park). This is assuming that the counter reading is just
> >> > > doing that, and nothing else is being done to return the sysfs data
> >> > > which is probably not always true in practice.
> >> > >
> >> > > Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
> >> > > CPU scheduling competion reading sysfs can hurt the deadline. There's
> >> > > also the question of power - we definitely have spent time in the past
> >> > > optimizing other virtual files such as /proc/pid/smaps for this reason
> >> > > where it spent lots of CPU time.
> >> >
> >> > smaps was "odd", but that was done after measurements were actually made
> >> > to prove it was needed. That hasn't happened yet :)
> >> >
> >> > And is there a reason you have to do this every 16ms?
> >>
> >> Not every, I was just saying whenever it happens and a frame delivery
> >> deadline is missed, then a frame drop can occur which can result in a
> >> poor user experience.
> >
> >
> > But this is not done in the UI thread context. So some thread running for more than 16ms shouldn't cause a frame drop. If it does, we have bigger problems.
> >
>
> Not really. That depends on the priority of the other thread and other
> things. It can obviously time share the same CPU as the UI thread if
> it is not configured correctly. Even with CFS it can reduce the time
> consumed by other "real-time" CFS threads. I am not sure what you are
> proposing, there are also (obviously) power issues with things running
> for long times pointlessly. We should try to do better if we can. As
> Greg said, some study/research can be done on the use case before
> settling for a solution (sysfs or other).
>
Agree, power and optimization is good. Just saying that the UI example
is not a real one. If the UI thread is that poorly configured that
some thread running for a second can cause frame drops in a multicore
system, that's a problem with the UI framework design.
-Saravana
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>
^ permalink raw reply
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
From: Joel Fernandes @ 2019-06-19 20:09 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki,
Sandeep Patil, Viresh Kumar, Hridya Valsaraju, Linux PM,
Cc: Android Kernel, LKML, Alexei Starovoitov, Steven Rostedt,
Alexei Starovoitov
In-Reply-To: <CAGETcx-ZZRc_jtBws2cFTe1wjiWeBowdqfqOhcCJV_7AUyBEVw@mail.gmail.com>
On Wed, Jun 19, 2019 at 3:59 PM 'Saravana Kannan' via kernel-team
<kernel-team@android.com> wrote:
>
>
>
> On Wed, Jun 19, 2019, 11:55 AM 'Joel Fernandes' via kernel-team <kernel-team@android.com> wrote:
>>
>> On Wed, Jun 19, 2019 at 2:35 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> >
>> > On Wed, Jun 19, 2019 at 02:01:36PM -0400, Joel Fernandes wrote:
>> > > On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
>> > > <gregkh@linuxfoundation.org> wrote:
>> > > >
>> > > > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
>> > > > > > It is conceivable to have a "wakeup_sources" directory under
>> > > > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
>> > > > >
>> > > > > One of the "issues" with this is, now if you have say 100 wake up
>> > > > > sources, with 10 entries each, then we're talking about a 1000 sysfs
>> > > > > files. Each one has to be opened, and read individually. This adds
>> > > > > overhead and it is more convenient to read from a single file. The
>> > > > > problem is this single file is not ABI. So the question I guess is,
>> > > > > how do we solve this in both an ABI friendly way while keeping the
>> > > > > overhead low.
>> > > >
>> > > > How much overhead? Have you measured it, reading from virtual files is
>> > > > fast :)
>> > >
>> > > I measured, and it is definitely not free. If you create and read a
>> > > 1000 files and just return a string back, it can take up to 11-13
>> > > milliseconds (did not lock CPU frequencies, was just looking for
>> > > average ball park). This is assuming that the counter reading is just
>> > > doing that, and nothing else is being done to return the sysfs data
>> > > which is probably not always true in practice.
>> > >
>> > > Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
>> > > CPU scheduling competion reading sysfs can hurt the deadline. There's
>> > > also the question of power - we definitely have spent time in the past
>> > > optimizing other virtual files such as /proc/pid/smaps for this reason
>> > > where it spent lots of CPU time.
>> >
>> > smaps was "odd", but that was done after measurements were actually made
>> > to prove it was needed. That hasn't happened yet :)
>> >
>> > And is there a reason you have to do this every 16ms?
>>
>> Not every, I was just saying whenever it happens and a frame delivery
>> deadline is missed, then a frame drop can occur which can result in a
>> poor user experience.
>
>
> But this is not done in the UI thread context. So some thread running for more than 16ms shouldn't cause a frame drop. If it does, we have bigger problems.
>
Not really. That depends on the priority of the other thread and other
things. It can obviously time share the same CPU as the UI thread if
it is not configured correctly. Even with CFS it can reduce the time
consumed by other "real-time" CFS threads. I am not sure what you are
proposing, there are also (obviously) power issues with things running
for long times pointlessly. We should try to do better if we can. As
Greg said, some study/research can be done on the use case before
settling for a solution (sysfs or other).
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox