[PATCH v3 1/2] power: supply: add input voltage limit property

[PATCH v3 1/2] power: supply: add input voltage limit property

[Enric Balletbo i Serra]

This is part of the Pixel C's thermal management strategy to effectively
limit the input power to 5V 3A when the screen is on. When the screen is
on, the display, the CPU, and the GPU all contribute more heat to the
system than while the screen is off, and we made a tradeoff to throttle
the charger in order to give more of the thermal budget to those other
components.

So there's nothing fundamentally broken about the hardware that would
cause the Pixel C to malfunction if we were charging at 9V or 12V instead
of 5V when the screen is on, ie if userspace doesn't change this.

What would happen is that you wouldn't meet Google's skin temperature
targets on the system if the charger was allowed to run at 9V or 12V with
the screen on.

For folks hacking on Pixel Cs (which is now outside of Google's official
support window for Android) and customizing their own kernel and userspace
this would be acceptable, but we wanted to expose this feature in the
power supply properties because the feature does exist in the Emedded
Controller firmware of the Pixel C and all of Google's Chromebooks with
USB-C made since 2015 in case someone running an up to date kernel wanted
to limit the charging power for thermal or other reasons.

This patch exposes a new property, similar to input current limit, to
re-configure the maximum voltage from the external supply at runtime
based on system-level knowledge or user input.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
---

Changes in v3:
- Improve commit log and documentation with Benson comments.

Changes in v2:
- Document the new property in ABI/testing/sysfs-class-power.
- Add the Reviewed-by Guenter Roeck tag.

 Documentation/ABI/testing/sysfs-class-power | 15 +++++++++++++++
 Documentation/power/power_supply_class.txt  |  2 ++
 drivers/power/supply/power_supply_sysfs.c   |  1 +
 include/linux/power_supply.h                |  1 +
 4 files changed, 19 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index 5e23e22dce1b..6dee5c105a28 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -335,6 +335,21 @@ Description:
 		Access: Read, Write
 		Valid values: Represented in microamps
 
+What:		/sys/class/power_supply/<supply_name>/input_voltage_limit
+Date:		Nov 2018
+Contact:	linux-pm@vger.kernel.org
+Description:
+		This entry configures the incoming VBUS voltage limit currently
+		set in the supply. Normally this is configured based on
+		system-level knowledge or user input (e.g. This is part of the
+		Pixel C's thermal management strategy to effectively limit the
+		input power to 5V when the screen is on to meet Google's skin
+		temperature targets). Note that this feature should not be
+		used for safety critical things.
+
+		Access: Read, Write
+		Valid values: Represented in microvolts
+
 What:		/sys/class/power_supply/<supply_name>/online,
 Date:		May 2007
 Contact:	linux-pm@vger.kernel.org
diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
index 300d37896e51..7b4be615b4f8 100644
--- a/Documentation/power/power_supply_class.txt
+++ b/Documentation/power/power_supply_class.txt
@@ -137,6 +137,8 @@ power supply object.
 
 INPUT_CURRENT_LIMIT - input current limit programmed by charger. Indicates
 the current drawn from a charging source.
+INPUT_VOLTAGE_LIMIT - input voltage limit programmed by charger. Indicates
+the voltage limit from a charging source.
 
 CHARGE_CONTROL_LIMIT - current charge control limit setting
 CHARGE_CONTROL_LIMIT_MAX - maximum charge control limit setting
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index dce24f596160..5848742ebb59 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -275,6 +275,7 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(charge_control_limit),
 	POWER_SUPPLY_ATTR(charge_control_limit_max),
 	POWER_SUPPLY_ATTR(input_current_limit),
+	POWER_SUPPLY_ATTR(input_voltage_limit),
 	POWER_SUPPLY_ATTR(energy_full_design),
 	POWER_SUPPLY_ATTR(energy_empty_design),
 	POWER_SUPPLY_ATTR(energy_full),
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index f80769175c56..608ba88e32ee 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -122,6 +122,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
 	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
-- 
2.19.2

[PATCH v3 2/2] power: supply: cros: allow to set input voltage and current limit

[Enric Balletbo i Serra]

This patch allows reading and writing the input voltage and current
limit through the POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT and
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT sysfs properties. This allows
userspace to see current values and to re-configure these values at
runtime based on system-level knowledge or user input.

By default there is no limit, this is reported as a -1 when reading from
userspace. Writing a value will set the current or voltage limit in uA
or uV, and writing any negative value will remove that limit.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

Changes in v3: None
Changes in v2:
- Fix the upper limit that can be set.
- Remove unnecessary else.

 drivers/power/supply/cros_usbpd-charger.c | 116 ++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
index 7e9c3984ef6a..3a9ea94c3de3 100644
--- a/drivers/power/supply/cros_usbpd-charger.c
+++ b/drivers/power/supply/cros_usbpd-charger.c
@@ -53,6 +53,8 @@ struct charger_data {
 };
 
 static enum power_supply_property cros_usbpd_charger_props[] = {
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CURRENT_MAX,
@@ -80,6 +82,10 @@ static enum power_supply_usb_type cros_usbpd_charger_usb_types[] = {
 	POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID
 };
 
+/* Input voltage/current limit in mV/mA. Default to none. */
+static u16 input_voltage_limit = EC_POWER_LIMIT_NONE;
+static u16 input_current_limit = EC_POWER_LIMIT_NONE;
+
 static bool cros_usbpd_charger_port_is_dedicated(struct port_data *port)
 {
 	return port->port_number >= port->charger->num_usbpd_ports;
@@ -324,6 +330,26 @@ static int cros_usbpd_charger_get_port_status(struct port_data *port,
 	return ret;
 }
 
+static int cros_usbpd_charger_set_ext_power_limit(struct charger_data *charger,
+						  u16 current_lim,
+						  u16 voltage_lim)
+{
+	struct ec_params_external_power_limit_v1 req;
+	int ret;
+
+	req.current_lim = current_lim;
+	req.voltage_lim = voltage_lim;
+
+	ret = cros_usbpd_charger_ec_command(charger, 0,
+					    EC_CMD_EXTERNAL_POWER_LIMIT,
+					    &req, sizeof(req), NULL, 0);
+	if (ret < 0)
+		dev_err(charger->dev,
+			"Unable to set the 'External Power Limit': %d\n", ret);
+
+	return ret;
+}
+
 static void cros_usbpd_charger_power_changed(struct power_supply *psy)
 {
 	struct port_data *port = power_supply_get_drvdata(psy);
@@ -396,6 +422,18 @@ static int cros_usbpd_charger_get_prop(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_USB_TYPE:
 		val->intval = port->psy_usb_type;
 		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		if (input_current_limit == EC_POWER_LIMIT_NONE)
+			val->intval = -1;
+		else
+			val->intval = input_current_limit * 1000;
+		break;
+	case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
+		if (input_voltage_limit == EC_POWER_LIMIT_NONE)
+			val->intval = -1;
+		else
+			val->intval = input_voltage_limit * 1000;
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = port->model_name;
 		break;
@@ -409,6 +447,81 @@ static int cros_usbpd_charger_get_prop(struct power_supply *psy,
 	return 0;
 }
 
+static int cros_usbpd_charger_set_prop(struct power_supply *psy,
+				       enum power_supply_property psp,
+				       const union power_supply_propval *val)
+{
+	struct port_data *port = power_supply_get_drvdata(psy);
+	struct charger_data *charger = port->charger;
+	struct device *dev = charger->dev;
+	u16 intval;
+	int ret;
+
+	/* U16_MAX in mV/mA is the maximum supported value */
+	if (val->intval >= U16_MAX * 1000)
+		return -EINVAL;
+	/* A negative number is used to clear the limit */
+	if (val->intval < 0)
+		intval = EC_POWER_LIMIT_NONE;
+	else	/* Convert from uA/uV to mA/mV */
+		intval = val->intval / 1000;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = cros_usbpd_charger_set_ext_power_limit(charger, intval,
+							input_voltage_limit);
+		if (ret < 0)
+			break;
+
+		input_current_limit = intval;
+		if (input_current_limit == EC_POWER_LIMIT_NONE)
+			dev_info(dev,
+			  "External Current Limit cleared for all ports\n");
+		else
+			dev_info(dev,
+			  "External Current Limit set to %dmA for all ports\n",
+			  input_current_limit);
+		break;
+	case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
+		ret = cros_usbpd_charger_set_ext_power_limit(charger,
+							input_current_limit,
+							intval);
+		if (ret < 0)
+			break;
+
+		input_voltage_limit = intval;
+		if (input_voltage_limit == EC_POWER_LIMIT_NONE)
+			dev_info(dev,
+			  "External Voltage Limit cleared for all ports\n");
+		else
+			dev_info(dev,
+			  "External Voltage Limit set to %dmV for all ports\n",
+			  input_voltage_limit);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int cros_usbpd_charger_property_is_writeable(struct power_supply *psy,
+						enum power_supply_property psp)
+{
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+	case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
+		ret = 1;
+		break;
+	default:
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static int cros_usbpd_charger_ec_event(struct notifier_block *nb,
 				       unsigned long queued_during_suspend,
 				       void *_notify)
@@ -525,6 +638,9 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
 
 		psy_desc = &port->psy_desc;
 		psy_desc->get_property = cros_usbpd_charger_get_prop;
+		psy_desc->set_property = cros_usbpd_charger_set_prop;
+		psy_desc->property_is_writeable =
+				cros_usbpd_charger_property_is_writeable;
 		psy_desc->external_power_changed =
 					cros_usbpd_charger_power_changed;
 		psy_cfg.drv_data = port;
-- 
2.19.2

RE: [PATCH v3 1/2] power: supply: add input voltage limit property

[Adam Thomson]

On 13 December 2018 12:10, Enric Balletbo i Serra wrote:

> This is part of the Pixel C's thermal management strategy to effectively limit the
> input power to 5V 3A when the screen is on. When the screen is on, the display,
> the CPU, and the GPU all contribute more heat to the system than while the
> screen is off, and we made a tradeoff to throttle the charger in order to give
> more of the thermal budget to those other components.
> 
> So there's nothing fundamentally broken about the hardware that would cause
> the Pixel C to malfunction if we were charging at 9V or 12V instead of 5V when
> the screen is on, ie if userspace doesn't change this.
> 
> What would happen is that you wouldn't meet Google's skin temperature targets
> on the system if the charger was allowed to run at 9V or 12V with the screen on.
> 
> For folks hacking on Pixel Cs (which is now outside of Google's official support
> window for Android) and customizing their own kernel and userspace this would
> be acceptable, but we wanted to expose this feature in the power supply
> properties because the feature does exist in the Emedded Controller firmware of
> the Pixel C and all of Google's Chromebooks with USB-C made since 2015 in case
> someone running an up to date kernel wanted to limit the charging power for
> thermal or other reasons.
> 
> This patch exposes a new property, similar to input current limit, to re-configure
> the maximum voltage from the external supply at runtime based on system-level
> knowledge or user input.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>

Acked-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
> 
> Changes in v3:
> - Improve commit log and documentation with Benson comments.
> 
> Changes in v2:
> - Document the new property in ABI/testing/sysfs-class-power.
> - Add the Reviewed-by Guenter Roeck tag.
> 
>  Documentation/ABI/testing/sysfs-class-power | 15 +++++++++++++++
> Documentation/power/power_supply_class.txt  |  2 ++
>  drivers/power/supply/power_supply_sysfs.c   |  1 +
>  include/linux/power_supply.h                |  1 +
>  4 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-power
> b/Documentation/ABI/testing/sysfs-class-power
> index 5e23e22dce1b..6dee5c105a28 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -335,6 +335,21 @@ Description:
>  		Access: Read, Write
>  		Valid values: Represented in microamps
> 
> +What:		/sys/class/power_supply/<supply_name>/input_voltage_limit
> +Date:		Nov 2018
> +Contact:	linux-pm@vger.kernel.org
> +Description:
> +		This entry configures the incoming VBUS voltage limit currently
> +		set in the supply. Normally this is configured based on
> +		system-level knowledge or user input (e.g. This is part of the
> +		Pixel C's thermal management strategy to effectively limit the
> +		input power to 5V when the screen is on to meet Google's skin
> +		temperature targets). Note that this feature should not be
> +		used for safety critical things.
> +
> +		Access: Read, Write
> +		Valid values: Represented in microvolts
> +
>  What:		/sys/class/power_supply/<supply_name>/online,
>  Date:		May 2007
>  Contact:	linux-pm@vger.kernel.org
> diff --git a/Documentation/power/power_supply_class.txt
> b/Documentation/power/power_supply_class.txt
> index 300d37896e51..7b4be615b4f8 100644
> --- a/Documentation/power/power_supply_class.txt
> +++ b/Documentation/power/power_supply_class.txt
> @@ -137,6 +137,8 @@ power supply object.
> 
>  INPUT_CURRENT_LIMIT - input current limit programmed by charger. Indicates
> the current drawn from a charging source.
> +INPUT_VOLTAGE_LIMIT - input voltage limit programmed by charger.
> +Indicates the voltage limit from a charging source.
> 
>  CHARGE_CONTROL_LIMIT - current charge control limit setting
> CHARGE_CONTROL_LIMIT_MAX - maximum charge control limit setting diff --git
> a/drivers/power/supply/power_supply_sysfs.c
> b/drivers/power/supply/power_supply_sysfs.c
> index dce24f596160..5848742ebb59 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -275,6 +275,7 @@ static struct device_attribute power_supply_attrs[] = {
>  	POWER_SUPPLY_ATTR(charge_control_limit),
>  	POWER_SUPPLY_ATTR(charge_control_limit_max),
>  	POWER_SUPPLY_ATTR(input_current_limit),
> +	POWER_SUPPLY_ATTR(input_voltage_limit),
>  	POWER_SUPPLY_ATTR(energy_full_design),
>  	POWER_SUPPLY_ATTR(energy_empty_design),
>  	POWER_SUPPLY_ATTR(energy_full),
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h index
> f80769175c56..608ba88e32ee 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -122,6 +122,7 @@ enum power_supply_property {
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
>  	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
>  	POWER_SUPPLY_PROP_ENERGY_FULL,
> --
> 2.19.2

Re: [PATCH v3 2/2] power: supply: cros: allow to set input voltage and current limit

[Guenter Roeck]

On Thu, Dec 13, 2018 at 4:09 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> This patch allows reading and writing the input voltage and current
> limit through the POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT and
> POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT sysfs properties. This allows
> userspace to see current values and to re-configure these values at
> runtime based on system-level knowledge or user input.
>
> By default there is no limit, this is reported as a -1 when reading from
> userspace. Writing a value will set the current or voltage limit in uA
> or uV, and writing any negative value will remove that limit.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>
> Changes in v3: None
> Changes in v2:
> - Fix the upper limit that can be set.
> - Remove unnecessary else.
>
>  drivers/power/supply/cros_usbpd-charger.c | 116 ++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
>
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> index 7e9c3984ef6a..3a9ea94c3de3 100644
> --- a/drivers/power/supply/cros_usbpd-charger.c
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -53,6 +53,8 @@ struct charger_data {
>  };
>
>  static enum power_supply_property cros_usbpd_charger_props[] = {
> +       POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +       POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
>         POWER_SUPPLY_PROP_ONLINE,
>         POWER_SUPPLY_PROP_STATUS,
>         POWER_SUPPLY_PROP_CURRENT_MAX,
> @@ -80,6 +82,10 @@ static enum power_supply_usb_type cros_usbpd_charger_usb_types[] = {
>         POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID
>  };
>
> +/* Input voltage/current limit in mV/mA. Default to none. */
> +static u16 input_voltage_limit = EC_POWER_LIMIT_NONE;
> +static u16 input_current_limit = EC_POWER_LIMIT_NONE;
> +
>  static bool cros_usbpd_charger_port_is_dedicated(struct port_data *port)
>  {
>         return port->port_number >= port->charger->num_usbpd_ports;
> @@ -324,6 +330,26 @@ static int cros_usbpd_charger_get_port_status(struct port_data *port,
>         return ret;
>  }
>
> +static int cros_usbpd_charger_set_ext_power_limit(struct charger_data *charger,
> +                                                 u16 current_lim,
> +                                                 u16 voltage_lim)
> +{
> +       struct ec_params_external_power_limit_v1 req;
> +       int ret;
> +
> +       req.current_lim = current_lim;
> +       req.voltage_lim = voltage_lim;
> +
> +       ret = cros_usbpd_charger_ec_command(charger, 0,
> +                                           EC_CMD_EXTERNAL_POWER_LIMIT,
> +                                           &req, sizeof(req), NULL, 0);
> +       if (ret < 0)
> +               dev_err(charger->dev,
> +                       "Unable to set the 'External Power Limit': %d\n", ret);
> +
> +       return ret;
> +}
> +
>  static void cros_usbpd_charger_power_changed(struct power_supply *psy)
>  {
>         struct port_data *port = power_supply_get_drvdata(psy);
> @@ -396,6 +422,18 @@ static int cros_usbpd_charger_get_prop(struct power_supply *psy,
>         case POWER_SUPPLY_PROP_USB_TYPE:
>                 val->intval = port->psy_usb_type;
>                 break;
> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +               if (input_current_limit == EC_POWER_LIMIT_NONE)
> +                       val->intval = -1;
> +               else
> +                       val->intval = input_current_limit * 1000;
> +               break;
> +       case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
> +               if (input_voltage_limit == EC_POWER_LIMIT_NONE)
> +                       val->intval = -1;
> +               else
> +                       val->intval = input_voltage_limit * 1000;
> +               break;
>         case POWER_SUPPLY_PROP_MODEL_NAME:
>                 val->strval = port->model_name;
>                 break;
> @@ -409,6 +447,81 @@ static int cros_usbpd_charger_get_prop(struct power_supply *psy,
>         return 0;
>  }
>
> +static int cros_usbpd_charger_set_prop(struct power_supply *psy,
> +                                      enum power_supply_property psp,
> +                                      const union power_supply_propval *val)
> +{
> +       struct port_data *port = power_supply_get_drvdata(psy);
> +       struct charger_data *charger = port->charger;
> +       struct device *dev = charger->dev;
> +       u16 intval;
> +       int ret;
> +
> +       /* U16_MAX in mV/mA is the maximum supported value */
> +       if (val->intval >= U16_MAX * 1000)
> +               return -EINVAL;
> +       /* A negative number is used to clear the limit */
> +       if (val->intval < 0)
> +               intval = EC_POWER_LIMIT_NONE;
> +       else    /* Convert from uA/uV to mA/mV */
> +               intval = val->intval / 1000;
> +
> +       switch (psp) {
> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +               ret = cros_usbpd_charger_set_ext_power_limit(charger, intval,
> +                                                       input_voltage_limit);
> +               if (ret < 0)
> +                       break;
> +
> +               input_current_limit = intval;
> +               if (input_current_limit == EC_POWER_LIMIT_NONE)
> +                       dev_info(dev,
> +                         "External Current Limit cleared for all ports\n");
> +               else
> +                       dev_info(dev,
> +                         "External Current Limit set to %dmA for all ports\n",
> +                         input_current_limit);
> +               break;
> +       case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
> +               ret = cros_usbpd_charger_set_ext_power_limit(charger,
> +                                                       input_current_limit,
> +                                                       intval);
> +               if (ret < 0)
> +                       break;
> +
> +               input_voltage_limit = intval;
> +               if (input_voltage_limit == EC_POWER_LIMIT_NONE)
> +                       dev_info(dev,
> +                         "External Voltage Limit cleared for all ports\n");
> +               else
> +                       dev_info(dev,
> +                         "External Voltage Limit set to %dmV for all ports\n",
> +                         input_voltage_limit);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +       }
> +
> +       return ret;
> +}
> +
> +static int cros_usbpd_charger_property_is_writeable(struct power_supply *psy,
> +                                               enum power_supply_property psp)
> +{
> +       int ret;
> +
> +       switch (psp) {
> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +       case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
> +               ret = 1;
> +               break;
> +       default:
> +               ret = 0;
> +       }
> +
> +       return ret;
> +}
> +
>  static int cros_usbpd_charger_ec_event(struct notifier_block *nb,
>                                        unsigned long queued_during_suspend,
>                                        void *_notify)
> @@ -525,6 +638,9 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
>
>                 psy_desc = &port->psy_desc;
>                 psy_desc->get_property = cros_usbpd_charger_get_prop;
> +               psy_desc->set_property = cros_usbpd_charger_set_prop;
> +               psy_desc->property_is_writeable =
> +                               cros_usbpd_charger_property_is_writeable;
>                 psy_desc->external_power_changed =
>                                         cros_usbpd_charger_power_changed;
>                 psy_cfg.drv_data = port;
> --
> 2.19.2
>

Re: [PATCH v3 1/2] power: supply: add input voltage limit property

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5516 bytes --]

Hi!

> This is part of the Pixel C's thermal management strategy to effectively
> limit the input power to 5V 3A when the screen is on. When the screen is
> on, the display, the CPU, and the GPU all contribute more heat to the
> system than while the screen is off, and we made a tradeoff to throttle
> the charger in order to give more of the thermal budget to those other
> components.
> 
> So there's nothing fundamentally broken about the hardware that would
> cause the Pixel C to malfunction if we were charging at 9V or 12V instead
> of 5V when the screen is on, ie if userspace doesn't change this.
> 
> What would happen is that you wouldn't meet Google's skin temperature
> targets on the system if the charger was allowed to run at 9V or 12V with
> the screen on.
> 
> For folks hacking on Pixel Cs (which is now outside of Google's official
> support window for Android) and customizing their own kernel and userspace
> this would be acceptable, but we wanted to expose this feature in the
> power supply properties because the feature does exist in the Emedded
> Controller firmware of the Pixel C and all of Google's Chromebooks with
> USB-C made since 2015 in case someone running an up to date kernel wanted
> to limit the charging power for thermal or other reasons.
> 
> This patch exposes a new property, similar to input current limit, to
> re-configure the maximum voltage from the external supply at runtime
> based on system-level knowledge or user input.

Could we get power input limit, instead?

That is what you really want, it is more generally useful, and it is
what current input limit should have been, in the first place...

Thanks,
									Pavel

> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
> 
> Changes in v3:
> - Improve commit log and documentation with Benson comments.
> 
> Changes in v2:
> - Document the new property in ABI/testing/sysfs-class-power.
> - Add the Reviewed-by Guenter Roeck tag.
> 
>  Documentation/ABI/testing/sysfs-class-power | 15 +++++++++++++++
>  Documentation/power/power_supply_class.txt  |  2 ++
>  drivers/power/supply/power_supply_sysfs.c   |  1 +
>  include/linux/power_supply.h                |  1 +
>  4 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> index 5e23e22dce1b..6dee5c105a28 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -335,6 +335,21 @@ Description:
>  		Access: Read, Write
>  		Valid values: Represented in microamps
>  
> +What:		/sys/class/power_supply/<supply_name>/input_voltage_limit
> +Date:		Nov 2018
> +Contact:	linux-pm@vger.kernel.org
> +Description:
> +		This entry configures the incoming VBUS voltage limit currently
> +		set in the supply. Normally this is configured based on
> +		system-level knowledge or user input (e.g. This is part of the
> +		Pixel C's thermal management strategy to effectively limit the
> +		input power to 5V when the screen is on to meet Google's skin
> +		temperature targets). Note that this feature should not be
> +		used for safety critical things.
> +
> +		Access: Read, Write
> +		Valid values: Represented in microvolts
> +
>  What:		/sys/class/power_supply/<supply_name>/online,
>  Date:		May 2007
>  Contact:	linux-pm@vger.kernel.org
> diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
> index 300d37896e51..7b4be615b4f8 100644
> --- a/Documentation/power/power_supply_class.txt
> +++ b/Documentation/power/power_supply_class.txt
> @@ -137,6 +137,8 @@ power supply object.
>  
>  INPUT_CURRENT_LIMIT - input current limit programmed by charger. Indicates
>  the current drawn from a charging source.
> +INPUT_VOLTAGE_LIMIT - input voltage limit programmed by charger. Indicates
> +the voltage limit from a charging source.
>  
>  CHARGE_CONTROL_LIMIT - current charge control limit setting
>  CHARGE_CONTROL_LIMIT_MAX - maximum charge control limit setting
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index dce24f596160..5848742ebb59 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -275,6 +275,7 @@ static struct device_attribute power_supply_attrs[] = {
>  	POWER_SUPPLY_ATTR(charge_control_limit),
>  	POWER_SUPPLY_ATTR(charge_control_limit_max),
>  	POWER_SUPPLY_ATTR(input_current_limit),
> +	POWER_SUPPLY_ATTR(input_voltage_limit),
>  	POWER_SUPPLY_ATTR(energy_full_design),
>  	POWER_SUPPLY_ATTR(energy_empty_design),
>  	POWER_SUPPLY_ATTR(energy_full),
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index f80769175c56..608ba88e32ee 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -122,6 +122,7 @@ enum power_supply_property {
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
>  	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
>  	POWER_SUPPLY_PROP_ENERGY_FULL,

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 1/2] power: supply: add input voltage limit property

[Enric Balletbo i Serra]

Hi Pavel,

On 13/12/18 23:20, Pavel Machek wrote:
> Hi!
> 
>> This is part of the Pixel C's thermal management strategy to effectively
>> limit the input power to 5V 3A when the screen is on. When the screen is
>> on, the display, the CPU, and the GPU all contribute more heat to the
>> system than while the screen is off, and we made a tradeoff to throttle
>> the charger in order to give more of the thermal budget to those other
>> components.
>>
>> So there's nothing fundamentally broken about the hardware that would
>> cause the Pixel C to malfunction if we were charging at 9V or 12V instead
>> of 5V when the screen is on, ie if userspace doesn't change this.
>>
>> What would happen is that you wouldn't meet Google's skin temperature
>> targets on the system if the charger was allowed to run at 9V or 12V with
>> the screen on.
>>
>> For folks hacking on Pixel Cs (which is now outside of Google's official
>> support window for Android) and customizing their own kernel and userspace
>> this would be acceptable, but we wanted to expose this feature in the
>> power supply properties because the feature does exist in the Emedded
>> Controller firmware of the Pixel C and all of Google's Chromebooks with
>> USB-C made since 2015 in case someone running an up to date kernel wanted
>> to limit the charging power for thermal or other reasons.
>>
>> This patch exposes a new property, similar to input current limit, to
>> re-configure the maximum voltage from the external supply at runtime
>> based on system-level knowledge or user input.
> 
> Could we get power input limit, instead?
> 

I'm open but I have some concerns, so lets discuss a bit about it :)

According to the USB PD 2.0 specs if we limit the source at 15W we can get 5V/3A
or 9V/1.67A, if I am not mistaken the higher voltage caused problem since the
conversion to lower internal voltages generated more heat, so in this case
9V/1.67A is not a valid value for us (maybe someone from ChromeOS can confirm
this?).

There is also the USB Power Delivery Specification revision 3.0, who defines a
programmable power supply protocol that allows granular control over VBUS power
in 20 mV steps to facilitate constant current or constant voltage charging. So,
maybe we might be interested on set a specific constant current or a specific
contant voltage. I think that in this case would be interesting have the
possibility to set voltage or current. What do you think?

Thanks,
 Enric


> That is what you really want, it is more generally useful, and it is
> what current input limit should have been, in the first place...
> 
> Thanks,
> 									Pavel
> 
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> Reviewed-by: Guenter Roeck <groeck@chromium.org>
>> ---
>>
>> Changes in v3:
>> - Improve commit log and documentation with Benson comments.
>>
>> Changes in v2:
>> - Document the new property in ABI/testing/sysfs-class-power.
>> - Add the Reviewed-by Guenter Roeck tag.
>>
>>  Documentation/ABI/testing/sysfs-class-power | 15 +++++++++++++++
>>  Documentation/power/power_supply_class.txt  |  2 ++
>>  drivers/power/supply/power_supply_sysfs.c   |  1 +
>>  include/linux/power_supply.h                |  1 +
>>  4 files changed, 19 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
>> index 5e23e22dce1b..6dee5c105a28 100644
>> --- a/Documentation/ABI/testing/sysfs-class-power
>> +++ b/Documentation/ABI/testing/sysfs-class-power
>> @@ -335,6 +335,21 @@ Description:
>>  		Access: Read, Write
>>  		Valid values: Represented in microamps
>>  
>> +What:		/sys/class/power_supply/<supply_name>/input_voltage_limit
>> +Date:		Nov 2018
>> +Contact:	linux-pm@vger.kernel.org
>> +Description:
>> +		This entry configures the incoming VBUS voltage limit currently
>> +		set in the supply. Normally this is configured based on
>> +		system-level knowledge or user input (e.g. This is part of the
>> +		Pixel C's thermal management strategy to effectively limit the
>> +		input power to 5V when the screen is on to meet Google's skin
>> +		temperature targets). Note that this feature should not be
>> +		used for safety critical things.
>> +
>> +		Access: Read, Write
>> +		Valid values: Represented in microvolts
>> +
>>  What:		/sys/class/power_supply/<supply_name>/online,
>>  Date:		May 2007
>>  Contact:	linux-pm@vger.kernel.org
>> diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
>> index 300d37896e51..7b4be615b4f8 100644
>> --- a/Documentation/power/power_supply_class.txt
>> +++ b/Documentation/power/power_supply_class.txt
>> @@ -137,6 +137,8 @@ power supply object.
>>  
>>  INPUT_CURRENT_LIMIT - input current limit programmed by charger. Indicates
>>  the current drawn from a charging source.
>> +INPUT_VOLTAGE_LIMIT - input voltage limit programmed by charger. Indicates
>> +the voltage limit from a charging source.
>>  
>>  CHARGE_CONTROL_LIMIT - current charge control limit setting
>>  CHARGE_CONTROL_LIMIT_MAX - maximum charge control limit setting
>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>> index dce24f596160..5848742ebb59 100644
>> --- a/drivers/power/supply/power_supply_sysfs.c
>> +++ b/drivers/power/supply/power_supply_sysfs.c
>> @@ -275,6 +275,7 @@ static struct device_attribute power_supply_attrs[] = {
>>  	POWER_SUPPLY_ATTR(charge_control_limit),
>>  	POWER_SUPPLY_ATTR(charge_control_limit_max),
>>  	POWER_SUPPLY_ATTR(input_current_limit),
>> +	POWER_SUPPLY_ATTR(input_voltage_limit),
>>  	POWER_SUPPLY_ATTR(energy_full_design),
>>  	POWER_SUPPLY_ATTR(energy_empty_design),
>>  	POWER_SUPPLY_ATTR(energy_full),
>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> index f80769175c56..608ba88e32ee 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -122,6 +122,7 @@ enum power_supply_property {
>>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
>>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
>>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>> +	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
>>  	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
>>  	POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
>>  	POWER_SUPPLY_PROP_ENERGY_FULL,
> 

Re: [PATCH v3 1/2] power: supply: add input voltage limit property

[Enric Balletbo Serra]

Hi Pavel,

Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del
dia dt., 18 de des. 2018 a les 17:32:
>
> Hi Pavel,
>
> On 13/12/18 23:20, Pavel Machek wrote:
> > Hi!
> >
> >> This is part of the Pixel C's thermal management strategy to effectively
> >> limit the input power to 5V 3A when the screen is on. When the screen is
> >> on, the display, the CPU, and the GPU all contribute more heat to the
> >> system than while the screen is off, and we made a tradeoff to throttle
> >> the charger in order to give more of the thermal budget to those other
> >> components.
> >>
> >> So there's nothing fundamentally broken about the hardware that would
> >> cause the Pixel C to malfunction if we were charging at 9V or 12V instead
> >> of 5V when the screen is on, ie if userspace doesn't change this.
> >>
> >> What would happen is that you wouldn't meet Google's skin temperature
> >> targets on the system if the charger was allowed to run at 9V or 12V with
> >> the screen on.
> >>
> >> For folks hacking on Pixel Cs (which is now outside of Google's official
> >> support window for Android) and customizing their own kernel and userspace
> >> this would be acceptable, but we wanted to expose this feature in the
> >> power supply properties because the feature does exist in the Emedded
> >> Controller firmware of the Pixel C and all of Google's Chromebooks with
> >> USB-C made since 2015 in case someone running an up to date kernel wanted
> >> to limit the charging power for thermal or other reasons.
> >>
> >> This patch exposes a new property, similar to input current limit, to
> >> re-configure the maximum voltage from the external supply at runtime
> >> based on system-level knowledge or user input.
> >
> > Could we get power input limit, instead?
> >
>
> I'm open but I have some concerns, so lets discuss a bit about it :)
>
> According to the USB PD 2.0 specs if we limit the source at 15W we can get 5V/3A
> or 9V/1.67A, if I am not mistaken the higher voltage caused problem since the
> conversion to lower internal voltages generated more heat, so in this case
> 9V/1.67A is not a valid value for us (maybe someone from ChromeOS can confirm
> this?).
>
> There is also the USB Power Delivery Specification revision 3.0, who defines a
> programmable power supply protocol that allows granular control over VBUS power
> in 20 mV steps to facilitate constant current or constant voltage charging. So,
> maybe we might be interested on set a specific constant current or a specific
> contant voltage. I think that in this case would be interesting have the
> possibility to set voltage or current. What do you think?
>

Around Xmas are bad dates to start a discussion. I don't want this
patch will be forgotten, so a gentle ping on your thoughts on this :)
(just in case)

Cheers,
 Enric

> Thanks,
>  Enric
>
>
> > That is what you really want, it is more generally useful, and it is
> > what current input limit should have been, in the first place...
> >
> > Thanks,
> >                                                                       Pavel
> >
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> >> ---
> >>
> >> Changes in v3:
> >> - Improve commit log and documentation with Benson comments.
> >>
> >> Changes in v2:
> >> - Document the new property in ABI/testing/sysfs-class-power.
> >> - Add the Reviewed-by Guenter Roeck tag.
> >>
> >>  Documentation/ABI/testing/sysfs-class-power | 15 +++++++++++++++
> >>  Documentation/power/power_supply_class.txt  |  2 ++
> >>  drivers/power/supply/power_supply_sysfs.c   |  1 +
> >>  include/linux/power_supply.h                |  1 +
> >>  4 files changed, 19 insertions(+)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> >> index 5e23e22dce1b..6dee5c105a28 100644
> >> --- a/Documentation/ABI/testing/sysfs-class-power
> >> +++ b/Documentation/ABI/testing/sysfs-class-power
> >> @@ -335,6 +335,21 @@ Description:
> >>              Access: Read, Write
> >>              Valid values: Represented in microamps
> >>
> >> +What:               /sys/class/power_supply/<supply_name>/input_voltage_limit
> >> +Date:               Nov 2018
> >> +Contact:    linux-pm@vger.kernel.org
> >> +Description:
> >> +            This entry configures the incoming VBUS voltage limit currently
> >> +            set in the supply. Normally this is configured based on
> >> +            system-level knowledge or user input (e.g. This is part of the
> >> +            Pixel C's thermal management strategy to effectively limit the
> >> +            input power to 5V when the screen is on to meet Google's skin
> >> +            temperature targets). Note that this feature should not be
> >> +            used for safety critical things.
> >> +
> >> +            Access: Read, Write
> >> +            Valid values: Represented in microvolts
> >> +
> >>  What:               /sys/class/power_supply/<supply_name>/online,
> >>  Date:               May 2007
> >>  Contact:    linux-pm@vger.kernel.org
> >> diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
> >> index 300d37896e51..7b4be615b4f8 100644
> >> --- a/Documentation/power/power_supply_class.txt
> >> +++ b/Documentation/power/power_supply_class.txt
> >> @@ -137,6 +137,8 @@ power supply object.
> >>
> >>  INPUT_CURRENT_LIMIT - input current limit programmed by charger. Indicates
> >>  the current drawn from a charging source.
> >> +INPUT_VOLTAGE_LIMIT - input voltage limit programmed by charger. Indicates
> >> +the voltage limit from a charging source.
> >>
> >>  CHARGE_CONTROL_LIMIT - current charge control limit setting
> >>  CHARGE_CONTROL_LIMIT_MAX - maximum charge control limit setting
> >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> >> index dce24f596160..5848742ebb59 100644
> >> --- a/drivers/power/supply/power_supply_sysfs.c
> >> +++ b/drivers/power/supply/power_supply_sysfs.c
> >> @@ -275,6 +275,7 @@ static struct device_attribute power_supply_attrs[] = {
> >>      POWER_SUPPLY_ATTR(charge_control_limit),
> >>      POWER_SUPPLY_ATTR(charge_control_limit_max),
> >>      POWER_SUPPLY_ATTR(input_current_limit),
> >> +    POWER_SUPPLY_ATTR(input_voltage_limit),
> >>      POWER_SUPPLY_ATTR(energy_full_design),
> >>      POWER_SUPPLY_ATTR(energy_empty_design),
> >>      POWER_SUPPLY_ATTR(energy_full),
> >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> >> index f80769175c56..608ba88e32ee 100644
> >> --- a/include/linux/power_supply.h
> >> +++ b/include/linux/power_supply.h
> >> @@ -122,6 +122,7 @@ enum power_supply_property {
> >>      POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
> >>      POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
> >>      POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> >> +    POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
> >>      POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
> >>      POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
> >>      POWER_SUPPLY_PROP_ENERGY_FULL,
> >

Re: [PATCH v3 1/2] power: supply: add input voltage limit property

[Guenter Roeck]

On Tue, Jan 8, 2019 at 9:19 AM Enric Balletbo Serra <eballetbo@gmail.com> wrote:
>
> Hi Pavel,
>
> Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del
> dia dt., 18 de des. 2018 a les 17:32:
> >
> > Hi Pavel,
> >
> > On 13/12/18 23:20, Pavel Machek wrote:
> > > Hi!
> > >
> > >> This is part of the Pixel C's thermal management strategy to effectively
> > >> limit the input power to 5V 3A when the screen is on. When the screen is
> > >> on, the display, the CPU, and the GPU all contribute more heat to the
> > >> system than while the screen is off, and we made a tradeoff to throttle
> > >> the charger in order to give more of the thermal budget to those other
> > >> components.
> > >>
> > >> So there's nothing fundamentally broken about the hardware that would
> > >> cause the Pixel C to malfunction if we were charging at 9V or 12V instead
> > >> of 5V when the screen is on, ie if userspace doesn't change this.
> > >>
> > >> What would happen is that you wouldn't meet Google's skin temperature
> > >> targets on the system if the charger was allowed to run at 9V or 12V with
> > >> the screen on.
> > >>
> > >> For folks hacking on Pixel Cs (which is now outside of Google's official
> > >> support window for Android) and customizing their own kernel and userspace
> > >> this would be acceptable, but we wanted to expose this feature in the
> > >> power supply properties because the feature does exist in the Emedded
> > >> Controller firmware of the Pixel C and all of Google's Chromebooks with
> > >> USB-C made since 2015 in case someone running an up to date kernel wanted
> > >> to limit the charging power for thermal or other reasons.
> > >>
> > >> This patch exposes a new property, similar to input current limit, to
> > >> re-configure the maximum voltage from the external supply at runtime
> > >> based on system-level knowledge or user input.
> > >
> > > Could we get power input limit, instead?
> > >
> >
> > I'm open but I have some concerns, so lets discuss a bit about it :)
> >
> > According to the USB PD 2.0 specs if we limit the source at 15W we can get 5V/3A
> > or 9V/1.67A, if I am not mistaken the higher voltage caused problem since the
> > conversion to lower internal voltages generated more heat, so in this case
> > 9V/1.67A is not a valid value for us (maybe someone from ChromeOS can confirm
> > this?).
> >
> > There is also the USB Power Delivery Specification revision 3.0, who defines a
> > programmable power supply protocol that allows granular control over VBUS power
> > in 20 mV steps to facilitate constant current or constant voltage charging. So,
> > maybe we might be interested on set a specific constant current or a specific
> > contant voltage. I think that in this case would be interesting have the
> > possibility to set voltage or current. What do you think?
> >
>
> Around Xmas are bad dates to start a discussion. I don't want this
> patch will be forgotten, so a gentle ping on your thoughts on this :)
> (just in case)
>

FWIW, if upstream implements power limit instead of voltage limit,
we'll have to carry a private patch anyway since we need to be able to
set the voltage limit. Seems to me that would make the upstream change
worthless for us, and it should then be dropped unless someone else
has a valid use case.

I recently encountered another possible use case, though I am not sure
if it applies directly (the power controller/EC might do that on its
own). At least on some systems, power consumption in idle/suspend is
higher if a high input voltage is provided, compared to 5V. In that
situation, it is desirable to be able to set a temporary input voltage
limit. Again, being able to limit the input _power_ won't help in that
situation.

Thanks,
Guenter

> Cheers,
>  Enric
>
> > Thanks,
> >  Enric
> >
> >
> > > That is what you really want, it is more generally useful, and it is
> > > what current input limit should have been, in the first place...
> > >
> > > Thanks,
> > >                                                                       Pavel
> > >
> > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > >> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> > >> ---
> > >>
> > >> Changes in v3:
> > >> - Improve commit log and documentation with Benson comments.
> > >>
> > >> Changes in v2:
> > >> - Document the new property in ABI/testing/sysfs-class-power.
> > >> - Add the Reviewed-by Guenter Roeck tag.
> > >>
> > >>  Documentation/ABI/testing/sysfs-class-power | 15 +++++++++++++++
> > >>  Documentation/power/power_supply_class.txt  |  2 ++
> > >>  drivers/power/supply/power_supply_sysfs.c   |  1 +
> > >>  include/linux/power_supply.h                |  1 +
> > >>  4 files changed, 19 insertions(+)
> > >>
> > >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> > >> index 5e23e22dce1b..6dee5c105a28 100644
> > >> --- a/Documentation/ABI/testing/sysfs-class-power
> > >> +++ b/Documentation/ABI/testing/sysfs-class-power
> > >> @@ -335,6 +335,21 @@ Description:
> > >>              Access: Read, Write
> > >>              Valid values: Represented in microamps
> > >>
> > >> +What:               /sys/class/power_supply/<supply_name>/input_voltage_limit
> > >> +Date:               Nov 2018
> > >> +Contact:    linux-pm@vger.kernel.org
> > >> +Description:
> > >> +            This entry configures the incoming VBUS voltage limit currently
> > >> +            set in the supply. Normally this is configured based on
> > >> +            system-level knowledge or user input (e.g. This is part of the
> > >> +            Pixel C's thermal management strategy to effectively limit the
> > >> +            input power to 5V when the screen is on to meet Google's skin
> > >> +            temperature targets). Note that this feature should not be
> > >> +            used for safety critical things.
> > >> +
> > >> +            Access: Read, Write
> > >> +            Valid values: Represented in microvolts
> > >> +
> > >>  What:               /sys/class/power_supply/<supply_name>/online,
> > >>  Date:               May 2007
> > >>  Contact:    linux-pm@vger.kernel.org
> > >> diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
> > >> index 300d37896e51..7b4be615b4f8 100644
> > >> --- a/Documentation/power/power_supply_class.txt
> > >> +++ b/Documentation/power/power_supply_class.txt
> > >> @@ -137,6 +137,8 @@ power supply object.
> > >>
> > >>  INPUT_CURRENT_LIMIT - input current limit programmed by charger. Indicates
> > >>  the current drawn from a charging source.
> > >> +INPUT_VOLTAGE_LIMIT - input voltage limit programmed by charger. Indicates
> > >> +the voltage limit from a charging source.
> > >>
> > >>  CHARGE_CONTROL_LIMIT - current charge control limit setting
> > >>  CHARGE_CONTROL_LIMIT_MAX - maximum charge control limit setting
> > >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > >> index dce24f596160..5848742ebb59 100644
> > >> --- a/drivers/power/supply/power_supply_sysfs.c
> > >> +++ b/drivers/power/supply/power_supply_sysfs.c
> > >> @@ -275,6 +275,7 @@ static struct device_attribute power_supply_attrs[] = {
> > >>      POWER_SUPPLY_ATTR(charge_control_limit),
> > >>      POWER_SUPPLY_ATTR(charge_control_limit_max),
> > >>      POWER_SUPPLY_ATTR(input_current_limit),
> > >> +    POWER_SUPPLY_ATTR(input_voltage_limit),
> > >>      POWER_SUPPLY_ATTR(energy_full_design),
> > >>      POWER_SUPPLY_ATTR(energy_empty_design),
> > >>      POWER_SUPPLY_ATTR(energy_full),
> > >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > >> index f80769175c56..608ba88e32ee 100644
> > >> --- a/include/linux/power_supply.h
> > >> +++ b/include/linux/power_supply.h
> > >> @@ -122,6 +122,7 @@ enum power_supply_property {
> > >>      POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
> > >>      POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
> > >>      POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> > >> +    POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
> > >>      POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
> > >>      POWER_SUPPLY_PROP_ENERGY_EMPTY_DESIGN,
> > >>      POWER_SUPPLY_PROP_ENERGY_FULL,
> > >

Re: [PATCH v3 1/2] power: supply: add input voltage limit property

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]

Hi!

> > >> For folks hacking on Pixel Cs (which is now outside of Google's official
> > >> support window for Android) and customizing their own kernel and userspace
> > >> this would be acceptable, but we wanted to expose this feature in the
> > >> power supply properties because the feature does exist in the Emedded
> > >> Controller firmware of the Pixel C and all of Google's Chromebooks with
> > >> USB-C made since 2015 in case someone running an up to date kernel wanted
> > >> to limit the charging power for thermal or other reasons.
> > >>
> > >> This patch exposes a new property, similar to input current limit, to
> > >> re-configure the maximum voltage from the external supply at runtime
> > >> based on system-level knowledge or user input.
> > >
> > > Could we get power input limit, instead?
> > >
> >
> > I'm open but I have some concerns, so lets discuss a bit about it :)
> >
> > According to the USB PD 2.0 specs if we limit the source at 15W we can get 5V/3A
> > or 9V/1.67A, if I am not mistaken the higher voltage caused problem since the
> > conversion to lower internal voltages generated more heat, so in this case
> > 9V/1.67A is not a valid value for us (maybe someone from ChromeOS can confirm
> > this?).

> Around Xmas are bad dates to start a discussion. I don't want this
> patch will be forgotten, so a gentle ping on your thoughts on this :)
> (just in case)

If someone can indeed confirm it is _voltage_ that is problematic,
then approach is ok with me. [But I'd not expect buck convertor
efficiency to differ greatly based on input voltage.]

Best regards,
									Pavel
									

> > >> +What:               /sys/class/power_supply/<supply_name>/input_voltage_limit
> > >> +Date:               Nov 2018
> > >> +Contact:    linux-pm@vger.kernel.org
> > >> +Description:
> > >> +            This entry configures the incoming VBUS voltage limit currently
> > >> +            set in the supply. Normally this is configured based on
> > >> +            system-level knowledge or user input (e.g. This is part of the
> > >> +            Pixel C's thermal management strategy to effectively limit the
> > >> +            input power to 5V when the screen is on to meet Google's skin
> > >> +            temperature targets). Note that this feature should not be
> > >> +            used for safety critical things.


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]