| Message ID | 20181122101119.29194-1-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/2] power: supply: add input voltage limit property. | expand |
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
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
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 >
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
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
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
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
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,