diff mbox series

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

Message ID 20190415220049.14924-1-enric.balletbo@collabora.com (mailing list archive)
State Not Applicable, archived
Headers show
Series [RESEND,v3,1/2] power: supply: add input voltage limit property | expand

Commit Message

Enric Balletbo i Serra April 15, 2019, 10 p.m. UTC 
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(+)

Comments

Pavel Machek April 16, 2019, 7:19 a.m. UTC | #1 
Hi!

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

Well, and I suspect it should expose input power limit, not input
voltage limit.

DC-DC convertor efficiency normally does not much depend on input
voltage....

								Pavel
Enric Balletbo i Serra April 16, 2019, 8:42 a.m. UTC | #2 
Hi Pavel,

On 16/4/19 9:19, Pavel Machek wrote:
> Hi!
> 
>> 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.
> 
> Well, and I suspect it should expose input power limit, not input
> voltage limit.
> 

Oh, ok, I thought we were agree that input voltage had sense after had some
discussion in v3. Seems that no, let me try to give you another example...

> DC-DC convertor efficiency normally does not much depend on input
> voltage....
> 
> 								Pavel
> 

As we said we have a heat "problem" due the internal voltage conversions.

Lets assume you have a linear regulator instead with a Vin range from 5V to 9V
and we want an output of 3.3V/1A

For 9V:
 Input power : P(in) = 9V x 1A = 9W
 Output power: P(out) = 3.3V x 1A = 3.3W
 Regulator power dissipated: P(reg) = P(in) - P(out) = 9W - 3.3W = 5.7W

For 5V:
 Input power : P(in) = 5V x 1A = 5W
 Output power: P(out) = 3.3V x 1A = 3.3W
 Regulator power dissipated: P(reg) = P(in) - P(out) = 5W - 3.3W = 1,7W

In the first case the regulator needs to dissipate more power, hence the
temperature is greater than the second case.

Thanks,
 Enric
Sebastian Reichel May 2, 2019, 9:01 p.m. UTC | #3 
Hi,

On Tue, Apr 16, 2019 at 10:42:30AM +0200, Enric Balletbo i Serra wrote:
> On 16/4/19 9:19, Pavel Machek wrote:
> >> 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.
> > 
> > Well, and I suspect it should expose input power limit, not input
> > voltage limit.
> 
> Oh, ok, I thought we were agree that input voltage had sense after had some
> discussion in v3. Seems that no, let me try to give you another example...
> 
> > DC-DC convertor efficiency normally does not much depend on input
> > voltage....
> 
> As we said we have a heat "problem" due the internal voltage conversions.
> 
> Lets assume you have a linear regulator instead with a Vin range from 5V to 9V
> and we want an output of 3.3V/1A
>
> For 9V:
>  Input power : P(in) = 9V x 1A = 9W
>  Output power: P(out) = 3.3V x 1A = 3.3W
>  Regulator power dissipated: P(reg) = P(in) - P(out) = 9W - 3.3W = 5.7W
> 
> For 5V:
>  Input power : P(in) = 5V x 1A = 5W
>  Output power: P(out) = 3.3V x 1A = 3.3W
>  Regulator power dissipated: P(reg) = P(in) - P(out) = 5W - 3.3W = 1,7W
> 
> In the first case the regulator needs to dissipate more power, hence the
> temperature is greater than the second case.

I would be surprised, if a linear regulator is being used in this
place :) That would basically render functionality of higher voltage
completley useless and a good reason to always limit to 5V. For the
generic case I agree with Pavel, that control over the power (voltage
* current) is the better choice. Still I believe it makes sense to
have a control knob for the voltage available, since some hardware
designs suck.

For example the bad hardware design might be the remote side,
that has issues providing some voltages and this would make it
possible to debug that.

-- Sebastian
Pavel Machek May 2, 2019, 9:12 p.m. UTC | #4 
Hi!

> On Tue, Apr 16, 2019 at 10:42:30AM +0200, Enric Balletbo i Serra wrote:
> > On 16/4/19 9:19, Pavel Machek wrote:
> > >> 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.
> > > 
> > > Well, and I suspect it should expose input power limit, not input
> > > voltage limit.
> > 
> > Oh, ok, I thought we were agree that input voltage had sense after had some
> > discussion in v3. Seems that no, let me try to give you another example...
> > 
> > > DC-DC convertor efficiency normally does not much depend on input
> > > voltage....
> > 
> > As we said we have a heat "problem" due the internal voltage conversions.
> > 
> > Lets assume you have a linear regulator instead with a Vin range from 5V to 9V
> > and we want an output of 3.3V/1A
> >
> > For 9V:
> >  Input power : P(in) = 9V x 1A = 9W
> >  Output power: P(out) = 3.3V x 1A = 3.3W
> >  Regulator power dissipated: P(reg) = P(in) - P(out) = 9W - 3.3W = 5.7W
> > 
> > For 5V:
> >  Input power : P(in) = 5V x 1A = 5W
> >  Output power: P(out) = 3.3V x 1A = 3.3W
> >  Regulator power dissipated: P(reg) = P(in) - P(out) = 5W - 3.3W = 1,7W
> > 
> > In the first case the regulator needs to dissipate more power, hence the
> > temperature is greater than the second case.
> 
> I would be surprised, if a linear regulator is being used in this
> place :) That would basically render functionality of higher voltage
> completley useless and a good reason to always limit to 5V. For the
> generic case I agree with Pavel, that control over the power (voltage
> * current) is the better choice. Still I believe it makes sense to
> have a control knob for the voltage available, since some hardware
> designs suck.
> 
> For example the bad hardware design might be the remote side,
> that has issues providing some voltages and this would make it
> possible to debug that.

Ok, I agree it might be useful _somewhere_, mostly for hardware
debugging. But before if we add voltage_limit, lets add power_limit,
too; and for problems that can be solved using power_limit, lets use
power_limit...

Best regards,
									Pavel
diff mbox series

Patch

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 2f9c201a54d1..5cb0d0b5eb19 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,