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

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

[Enric Balletbo i Serra]

We have a problem with USBPD chargers which under certain conditions
can result in system overheating if the voltage provided by the USBPD
port is too high. While the preferred means to control this would be
through devicetree or ACPI settings, this is not always possible, and
we need to have a means to set a voltage limit.

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 v2:
- Document the new property in ABI/testing/sysfs-class-power.
- Add the Reviewed-by Guenter Roeck tag.

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

diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index 5e23e22dce1b..4fb24b0a28df 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -335,6 +335,17 @@ 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:
+		Details the incoming VBUS voltage limit currently set in the
+		supply. Normally this is configured based on the type of
+		connection made.
+
+		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.1

[PATCH v2 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 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.1

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

[Guenter Roeck]

On Thu, Nov 22, 2018 at 2:11 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 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.1
>

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

[Adam Thomson]

On 22 November 2018 10:11, Enric Balletbo i Serra wrote:

> We have a problem with USBPD chargers which under certain conditions can
> result in system overheating if the voltage provided by the USBPD port is too
> high. While the preferred means to control this would be through devicetree or
> ACPI settings, this is not always possible, and we need to have a means to set a
> voltage limit.
> 
> 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 v2:
> - Document the new property in ABI/testing/sysfs-class-power.
> - Add the Reviewed-by Guenter Roeck tag.
> 
>  Documentation/ABI/testing/sysfs-class-power | 11 +++++++++++
> Documentation/power/power_supply_class.txt  |  2 ++
>  drivers/power/supply/power_supply_sysfs.c   |  1 +
>  include/linux/power_supply.h                |  1 +
>  4 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-power
> b/Documentation/ABI/testing/sysfs-class-power
> index 5e23e22dce1b..4fb24b0a28df 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -335,6 +335,17 @@ 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:
> +		Details the incoming VBUS voltage limit currently set in the
> +		supply. Normally this is configured based on the type of
> +		connection made.
> +
> +		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.1

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

[Pavel Machek]

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

Hi!

> We have a problem with USBPD chargers which under certain conditions
> can result in system overheating if the voltage provided by the USBPD
> port is too high. While the preferred means to control this would be
> through devicetree or ACPI settings, this is not always possible, and
> we need to have a means to set a voltage limit.
> 
> 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.

First, this should really be handled by dt / ACPI. If it is broken,
that's a hardware bug, and we can do DMI-based blacklists in kernel.

How are you supposed to fsck a system, for example?

> +What:		/sys/class/power_supply/<supply_name>/input_voltage_limit
> +Date:		Nov 2018
> +Contact:	linux-pm@vger.kernel.org
> +Description:
> +		Details the incoming VBUS voltage limit currently set in the
> +		supply. Normally this is configured based on the type of
> +		connection made.

"Details"?

Who can write to this value and when? What is the limit? If USB
charger is plugged in, should it show 5.0V (because that's nominal on
the USB) or 5.25V (because that is the real limit)?

Who can write to this and when. And what happens on write? What
happens if I write value that charger can't provide there?

Does it set the voltage power supply should produce? Or maximal it can
choose to produce?

This really needs better documentation.

								Pavel
-- 
(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 v2 1/2] power: supply: add input voltage limit property.

[Enric Balletbo i Serra]

Hi Pavel,

Thanks for the feedback.

On 24/11/18 0:22, Pavel Machek wrote:
> Hi!
> 
>> We have a problem with USBPD chargers which under certain conditions
>> can result in system overheating if the voltage provided by the USBPD
>> port is too high. While the preferred means to control this would be
>> through devicetree or ACPI settings, this is not always possible, and
>> we need to have a means to set a voltage limit.
>>
>> 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.
> 
> First, this should really be handled by dt / ACPI. If it is broken,
> that's a hardware bug, and we can do DMI-based blacklists in kernel.
> 

I think that handle this via dt / ACPI is not possible for our use case. It can
be a hardware bug or a hardware/user constrain, let me try to explain better
with an example.

On Pixel C's devices, userspace uses this to set a USB input limit of 5V when
the screen is on for thermal reasons, but those go away when the screen is
off/system is sleeping, so we allow 9V and 12V levels when sleeping.

> How are you supposed to fsck a system, for example?
> 

hmm, I'm not sure I get the question, sorry, could you rephrase?

>> +What:		/sys/class/power_supply/<supply_name>/input_voltage_limit
>> +Date:		Nov 2018
>> +Contact:	linux-pm@vger.kernel.org
>> +Description:
>> +		Details the incoming VBUS voltage limit currently set in the
>> +		supply. Normally this is configured based on the type of
>> +		connection made.
> 
> "Details"?
> 
> Who can write to this value and when? What is the limit? If USB
> charger is plugged in, should it show 5.0V (because that's nominal on
> the USB) or 5.25V (because that is the real limit)?
> 

The voltages here refer to the Typical or Nominal values defined by the USB
specification for a Fixed power supply (or Fixed PDO). 5.0V, 5.25V, 4.75V,
5.5V... all of those are considered "5V" according to the USB specifications.

At the 5V level (defined in the USB PD specification as vSafe5V), a nominal
power source is allowed to have a voltage between 4.75V and 5.50V (5V -5%/
+10%). For simplicity, we refer to this range as 5V, the typical value.
For all higher voltage levels other than vSafe5V, MIN voltage is PDO Voltage *
0.95, and MAX voltage is PDO Voltage * 1.05, where PDO Voltage is the Typical
value (9V, 12V, 15V, 20V). [1]

> Who can write to this and when. And what happens on write? What
> happens if I write value that charger can't provide there?
> 
> Does it set the voltage power supply should produce? Or maximal it can
> choose to produce?
> 

This defines a maximal voltage. If you write a value it can't provide,
it should go down to the next lowest level, and barring that, 5V is
always allowed.

That makes me think that maybe I should also improve the implementation.

> This really needs better documentation.

Ack, I'll send a next version improving that.

Make sense to you?

Thanks
 Enric

[1] USB PD Specification revision 2.0, Version 1.3, Table 7-22

> 
> 								Pavel
> 

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

[Pavel Machek]

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

Hi!

> >> We have a problem with USBPD chargers which under certain conditions
> >> can result in system overheating if the voltage provided by the USBPD
> >> port is too high. While the preferred means to control this would be
> >> through devicetree or ACPI settings, this is not always possible, and
> >> we need to have a means to set a voltage limit.
> >>
> >> 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.
> > 
> > First, this should really be handled by dt / ACPI. If it is broken,
> > that's a hardware bug, and we can do DMI-based blacklists in kernel.
> > 
> 
> I think that handle this via dt / ACPI is not possible for our use case. It can
> be a hardware bug or a hardware/user constrain, let me try to explain better
> with an example.
> 
> On Pixel C's devices, userspace uses this to set a USB input limit of 5V when
> the screen is on for thermal reasons, but those go away when the screen is
> off/system is sleeping, so we allow 9V and 12V levels when sleeping.

So, on pixel C, what happens if userland ignores the constraint, keeps
display on and sets charger to 12V?

> > How are you supposed to fsck a system, for example?
> > 
> 
> hmm, I'm not sure I get the question, sorry, could you rephrase?

Ok, so I have just booted with init=/bin/bash, and would like to run
fsck.ext4 on my root filesystem.

> >> +What:		/sys/class/power_supply/<supply_name>/input_voltage_limit
> >> +Date:		Nov 2018
> >> +Contact:	linux-pm@vger.kernel.org
> >> +Description:
> >> +		Details the incoming VBUS voltage limit currently set in the
> >> +		supply. Normally this is configured based on the type of
> >> +		connection made.
> > 
> > "Details"?
> > 
> > Who can write to this value and when? What is the limit? If USB
> > charger is plugged in, should it show 5.0V (because that's nominal on
> > the USB) or 5.25V (because that is the real limit)?
> 
> The voltages here refer to the Typical or Nominal values defined by the USB
> specification for a Fixed power supply (or Fixed PDO). 5.0V, 5.25V, 4.75V,
> 5.5V... all of those are considered "5V" according to the USB specifications.
> 
> At the 5V level (defined in the USB PD specification as vSafe5V), a nominal
> power source is allowed to have a voltage between 4.75V and 5.50V (5V -5%/
> +10%). For simplicity, we refer to this range as 5V, the typical value.
> For all higher voltage levels other than vSafe5V, MIN voltage is PDO Voltage *
> 0.95, and MAX voltage is PDO Voltage * 1.05, where PDO Voltage is the Typical
> value (9V, 12V, 15V, 20V). [1]

> > Who can write to this and when. And what happens on write? What
> > happens if I write value that charger can't provide there?
> > 
> > Does it set the voltage power supply should produce? Or maximal it can
> > choose to produce?
> > 
> 
> This defines a maximal voltage. If you write a value it can't provide,
> it should go down to the next lowest level, and barring that, 5V is
> always allowed.
> 
> That makes me think that maybe I should also improve the implementation.
> 
> > This really needs better documentation.
> 
> Ack, I'll send a next version improving that.

Thanks.
									Pavel
-- 
(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 v2 1/2] power: supply: add input voltage limit property.

[Benson Leung]

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

Hi Pavel,

On Sat, Dec 01, 2018 at 04:09:34PM +0100, Pavel Machek wrote:
> > I think that handle this via dt / ACPI is not possible for our use case. It can
> > be a hardware bug or a hardware/user constrain, let me try to explain better
> > with an example.
> > 
> > On Pixel C's devices, userspace uses this to set a USB input limit of 5V when
> > the screen is on for thermal reasons, but those go away when the screen is
> > off/system is sleeping, so we allow 9V and 12V levels when sleeping.
> 
> So, on pixel C, what happens if userland ignores the constraint, keeps
> display on and sets charger to 12V?

I was the software tech lead for the Google Pixel C and was involved in this
particular code change in 2015 before the release of the product.

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.

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.

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.

Thanks,
Benson

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Sebastian Reichel]

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

Hi,

On Mon, Dec 03, 2018 at 11:58:45AM -0800, Benson Leung wrote:
> On Sat, Dec 01, 2018 at 04:09:34PM +0100, Pavel Machek wrote:
> > > I think that handle this via dt / ACPI is not possible for our use case. It can
> > > be a hardware bug or a hardware/user constrain, let me try to explain better
> > > with an example.
> > > 
> > > On Pixel C's devices, userspace uses this to set a USB input limit of 5V when
> > > the screen is on for thermal reasons, but those go away when the screen is
> > > off/system is sleeping, so we allow 9V and 12V levels when sleeping.
> > 
> > So, on pixel C, what happens if userland ignores the constraint, keeps
> > display on and sets charger to 12V?
> 
> I was the software tech lead for the Google Pixel C and was involved in this
> particular code change in 2015 before the release of the product.
> 
> 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.
> 
> 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.
> 
> 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.

I'm fine with merging this with the above description. I hope
vendors never decide to move safety relevant decisions to userspace.
Enric, can you please integrate the great description from Benson
into the patch description?

Thanks,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pavel Machek]

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

Hi!

> On Sat, Dec 01, 2018 at 04:09:34PM +0100, Pavel Machek wrote:
> > > I think that handle this via dt / ACPI is not possible for our use case. It can
> > > be a hardware bug or a hardware/user constrain, let me try to explain better
> > > with an example.
> > > 
> > > On Pixel C's devices, userspace uses this to set a USB input limit of 5V when
> > > the screen is on for thermal reasons, but those go away when the screen is
> > > off/system is sleeping, so we allow 9V and 12V levels when sleeping.
> > 
> > So, on pixel C, what happens if userland ignores the constraint, keeps
> > display on and sets charger to 12V?
> 
> I was the software tech lead for the Google Pixel C and was involved in this
> particular code change in 2015 before the release of the product.
> 
> 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.
> 
> 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.
> 
> 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.

So... the system is still guaranteed to work. It may be hot but not
dangerously so, and lifetime of internal components is not going to be
decreased by heat...?

Ok, I guess in such case we can do it from userspace.

> 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.

Few concerns:

1) You are basically using voltage limit to limit power. We already
have current limits, but what both you and existing user really want
are power limits. Should we have power limit instead? Would be way
more logical.

(If your charger can do 5V 1A, 5V 2.5A, 5V 3A, 9V 1A, you may want to
limit to first two, not first three).

2) Should the policy be based not on "screen is on or off" but on real
temperatures? We already have a thermal framework, and other machines
have "limit charging power when hot" constraints IIRC...

									Pavel
-- 
(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 --]