cros_usbpd-charger / power_manager / metrics - Need to split out Apple proprietary "brick" method from Mains |
||||||||||||
Issue descriptionWhat steps will reproduce the problem? 1. Plug in an Apple 1A, 2A, or 2.4A charger into one of our Chromebook systems via A-to-C cable 2. check /var/log/power_manager/powerd.LATEST 3. What is the expected result? Apple proprietary types are distinct from other types, like "USB" (SDP), "USB CDP" "USB DCP" etc. What happens instead of that? Always shows up as "Mains" Please provide any additional information below. Attach a screenshot if possible. UserAgentString: Mozilla/5.0 (X11; CrOS x86_64 9202.43.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.85 Safari/537.36 Work needs to be done in the kernel and in the power manager. In the kernel : Power supply types (which were last tweaked by me here : https://bugs.chromium.org/p/chromium/issues/detail?id=531628) need to be tweaked again to add Apple type : POWER_SUPPLY_TYPE_USB_APPLE_BRICK cros_usbpd-charger needs to make a small change to return the new APPLE_BRICK type instead of MAINS. Thankfully the EC already has a type USB_CHG_TYPE_PROPRIETARY and is already reporting the chargers as proprietary chargers. The kernel driver just needs to express it to the upper layers. In power manager : another type needs to be added (handled the same as mains) and metrics adjusted to report it as well. I can have this in by M-59. Bonus points (for me) for getting the new type upstreamed like last time.
,
Mar 3 2017
This will need to be done for chromeos-3.14, chromeos-3.18, and chromeos-4.4 to hit all of our Type-C Chromebooks.
,
Mar 3 2017
I'm happy to make the powerd changes. Is the new type going to show up in sysfs as "USB_APPLE_BRICK"?
,
Mar 3 2017
What about Samsung and other proprietary chargers? Under the assumption that the Apple bricks far outnumber them, do we want to just lump them all under a USB_PROPRIETARY?
,
Mar 3 2017
I think from the EC we don't have the way to distinguish between the two. Do you happen to know if the BC 1.2 chip (pericom is it?) does itself?
,
Mar 3 2017
,
Mar 3 2017
,
Mar 3 2017
Oh, it looks like the Samsung chargers are recognized as DCP by our ICs. So yeah, either APPLE_BRICK or PROPRIETARY is fine by me. If we think we might have to take stats for QC2 or something for some reason (even if we don't support it) anytime in the future, we may want an available category to lump it into, which would make PROPRIETARY preferable.
,
Mar 6 2017
I'd rather be specific as possible in the kernel with the power supply types, so let's use APPLE_BRICK. If we come up with a situation where we can detect some other proprietary method using one of our chips, we can just add another one distinct from APPLE_BRICK at that time. By the way, I talked to Shawnn@ and he also mentioned that some of the "Mains" category might be an artifact of the way our driver deals with chargers when they are first attached. We use Mains as a placeholder until we determine it's one of the other types, for example if USB PD takes a little bit to determine, so that's an extra bit of noise in this data that we need to either disregard when we analyze it or account for.
,
Mar 6 2017
sgtm. The whole "mains" thing is a huge source of noise, but once we pull Apple out of that category, isn't it just going to be a transient state? Is it safe to ignore it entirely when analyzing the histogram?
,
May 4 2017
I'm going to do this for M-60. Sorry for the delay everyone.
,
May 6 2017
,
May 8 2017
Hey Dan, Kernel CLs for this change in v4.4 (Kevin, Reef, Eve) are posted here: https://chromium-review.googlesource.com/498058 https://chromium-review.googlesource.com/498187 https://chromium-review.googlesource.com/498059 The new string to look for in the "type" node is "BrickID"
,
May 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/17c3a61f0cf073a39f4d538c442b5885ce2b06ac commit 17c3a61f0cf073a39f4d538c442b5885ce2b06ac Author: derat <derat@chromium.org> Date: Thu May 11 03:50:38 2017 metrics: Add BrickID charger type for Chrome OS. Update the PowerSupplyType enum to include BrickID, a new value used for proprietary Apple chargers. BUG= 698133 Review-Url: https://codereview.chromium.org/2875523002 Cr-Commit-Position: refs/heads/master@{#470798} [modify] https://crrev.com/17c3a61f0cf073a39f4d538c442b5885ce2b06ac/tools/metrics/histograms/enums.xml
,
May 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/3a973c5b8974b04aff3e1f41c38768df8b865bc4 commit 3a973c5b8974b04aff3e1f41c38768df8b865bc4 Author: Daniel Erat <derat@chromium.org> Date: Thu May 11 16:40:29 2017 power: Add metrics reporting for BrickID charger types. Add support for reporting chargers with type "BrickID", used for Apple Type-C chargers. BUG= chromium:698133 TEST=tests pass Change-Id: I1906319a79cf9ea0edd1a2b080fa7e9f390fab3c Reviewed-on: https://chromium-review.googlesource.com/500890 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> [modify] https://crrev.com/3a973c5b8974b04aff3e1f41c38768df8b865bc4/power_manager/powerd/system/power_supply.cc [modify] https://crrev.com/3a973c5b8974b04aff3e1f41c38768df8b865bc4/power_manager/powerd/system/power_supply.h [modify] https://crrev.com/3a973c5b8974b04aff3e1f41c38768df8b865bc4/power_manager/powerd/metrics_collector_unittest.cc [modify] https://crrev.com/3a973c5b8974b04aff3e1f41c38768df8b865bc4/power_manager/common/metrics_constants.h
,
May 11 2017
The powerd and Chrome sides of this should be in now.
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/2f7067553600eec86a4aa7349c3cd24e3eedac06 commit 2f7067553600eec86a4aa7349c3cd24e3eedac06 Author: Benson Leung <bleung@chromium.org> Date: Thu Jul 27 20:08:32 2017 BACKPORT: FROMLIST: power_supply: Add Apple Brick ID power supply type Apple currently supports three very common USB chargers: https://www.apple.com/power-adapters/ These chargers implement a proprietary Apple method for advertising 1A, 2.1A, and 2.4A at 5V called "Brick ID". In addition, 3rd parties implement the same charging method in many charging accessories that work with iOS devices. Devices that have charger detection chips such as the Pericom PI3USB9281, eg. Google Chromebook Pixel 2015, are capable of detecting these chargers, so let's add a type to facilicate passing that info up to userspace. This adds a separate power supply type for Apple's proprietary "Brick ID" charging method. Signed-off-by: Benson Leung <bleung@chromium.org> (bleung: am from https://patchwork.kernel.org/patch/9716765 Fixed up due to moved new drivers/power/supply and some other small context) BUG= chromium:698133 TEST=With an Apple 12W (2.4A), 10W (2.1A), or 5W (1A) power adapter attached, cat /sys/class/power_supply/CROS_USB_PD_CHARGER1/type Check that "BrickID" is returned. Change-Id: I75164c3c9e61ecba4a82f1c7ffe2cbf17a0ee7e7 Reviewed-on: https://chromium-review.googlesource.com/499528 Commit-Ready: Benson Leung <bleung@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Shawn N <shawnn@chromium.org> [modify] https://crrev.com/2f7067553600eec86a4aa7349c3cd24e3eedac06/drivers/power/power_supply_sysfs.c [modify] https://crrev.com/2f7067553600eec86a4aa7349c3cd24e3eedac06/include/linux/power_supply.h
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/d69e46ddd380f51d7d3a7082742663fd55f6b874 commit d69e46ddd380f51d7d3a7082742663fd55f6b874 Author: Benson Leung <bleung@chromium.org> Date: Thu Jul 27 20:08:35 2017 CHROMIUM: cros_usbpd-charger - Use new Apple Brick ID type Use the new Apple Brick power supply type when "USB_CHG_TYPE_PROPRIETARY" is provided by the ec. On most of our Chromebooks (Samus, and Glados generation), that type will correspond directly to Apple 1.0A, 2.0A, or 2.4A methods being detected. This will allow us to gather better metrics in the power manager when Apple Brick ID method is used on our Type-C Chromebooks. Signed-off-by: Benson Leung <bleung@chromium.org> BUG= chromium:698133 TEST=With an Apple 12W (2.4A), 10W (2.1A), or 5W (1A) power adapter attached, cat /sys/class/power_supply/CROS_USB_PD_CHARGER1/type Check that "BrickID" is returned. Change-Id: Ic6722194adde42d505feb890fa62661a6a27c4fa Reviewed-on: https://chromium-review.googlesource.com/499530 Commit-Ready: Benson Leung <bleung@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Shawn N <shawnn@chromium.org> [modify] https://crrev.com/d69e46ddd380f51d7d3a7082742663fd55f6b874/drivers/power/cros_usbpd-charger.c
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/f0bf72a44ebb8fc93f8494850442bf32679e488b commit f0bf72a44ebb8fc93f8494850442bf32679e488b Author: Benson Leung <bleung@chromium.org> Date: Thu Jul 27 20:08:28 2017 BACKPORT: FROMLIST: power_supply: Add Apple Brick ID power supply type Apple currently supports three very common USB chargers: https://www.apple.com/power-adapters/ These chargers implement a proprietary Apple method for advertising 1A, 2.1A, and 2.4A at 5V called "Brick ID". In addition, 3rd parties implement the same charging method in many charging accessories that work with iOS devices. Devices that have charger detection chips such as the Pericom PI3USB9281, eg. Google Chromebook Pixel 2015, are capable of detecting these chargers, so let's add a type to facilicate passing that info up to userspace. This adds a separate power supply type for Apple's proprietary "Brick ID" charging method. Signed-off-by: Benson Leung <bleung@chromium.org> (bleung: am from https://patchwork.kernel.org/patch/9716765 Fixed up due to moved new drivers/power/supply and some other small context) BUG= chromium:698133 TEST=With an Apple 12W (2.4A), 10W (2.1A), or 5W (1A) power adapter attached, cat /sys/class/power_supply/CROS_USB_PD_CHARGER1/type Check that "BrickID" is returned. Change-Id: I75164c3c9e61ecba4a82f1c7ffe2cbf17a0ee7e7 Reviewed-on: https://chromium-review.googlesource.com/498058 Commit-Ready: Benson Leung <bleung@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Todd Broch <tbroch@chromium.org> [modify] https://crrev.com/f0bf72a44ebb8fc93f8494850442bf32679e488b/drivers/power/power_supply_sysfs.c [modify] https://crrev.com/f0bf72a44ebb8fc93f8494850442bf32679e488b/include/linux/power_supply.h
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ab2d669e3aa15afd2dd379cd4079584c21f49c70 commit ab2d669e3aa15afd2dd379cd4079584c21f49c70 Author: Benson Leung <bleung@chromium.org> Date: Thu Jul 27 20:08:31 2017 CHROMIUM: cros_usbpd-charger - Use new Apple Brick ID type Use the new Apple Brick power supply type when "USB_CHG_TYPE_PROPRIETARY" is provided by the ec. On most of our Chromebooks (Samus, and Glados generation), that type will correspond directly to Apple 1.0A, 2.0A, or 2.4A methods being detected. This will allow us to gather better metrics in the power manager when Apple Brick ID method is used on our Type-C Chromebooks. Signed-off-by: Benson Leung <bleung@chromium.org> BUG= chromium:698133 TEST=With an Apple 12W (2.4A), 10W (2.1A), or 5W (1A) power adapter attached, cat /sys/class/power_supply/CROS_USB_PD_CHARGER1/type Check that "BrickID" is returned. Change-Id: Ic6722194adde42d505feb890fa62661a6a27c4fa Reviewed-on: https://chromium-review.googlesource.com/498059 Commit-Ready: Benson Leung <bleung@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Shawn N <shawnn@chromium.org> [modify] https://crrev.com/ab2d669e3aa15afd2dd379cd4079584c21f49c70/drivers/power/cros_usbpd-charger.c
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/734ccb9b7f03746998881578fb815e9c36a86670 commit 734ccb9b7f03746998881578fb815e9c36a86670 Author: Benson Leung <bleung@chromium.org> Date: Thu Jul 27 20:08:36 2017 BACKPORT: FROMLIST: power_supply: Add Apple Brick ID power supply type Apple currently supports three very common USB chargers: https://www.apple.com/power-adapters/ These chargers implement a proprietary Apple method for advertising 1A, 2.1A, and 2.4A at 5V called "Brick ID". In addition, 3rd parties implement the same charging method in many charging accessories that work with iOS devices. Devices that have charger detection chips such as the Pericom PI3USB9281, eg. Google Chromebook Pixel 2015, are capable of detecting these chargers, so let's add a type to facilicate passing that info up to userspace. This adds a separate power supply type for Apple's proprietary "Brick ID" charging method. Signed-off-by: Benson Leung <bleung@chromium.org> (bleung: am from https://patchwork.kernel.org/patch/9716765 Fixed up due to moved new drivers/power/supply and some other small context) BUG= chromium:698133 TEST=With an Apple 12W (2.4A), 10W (2.1A), or 5W (1A) power adapter attached, cat /sys/class/power_supply/CROS_USB_PD_CHARGER1/type Check that "BrickID" is returned. Change-Id: I75164c3c9e61ecba4a82f1c7ffe2cbf17a0ee7e7 Reviewed-on: https://chromium-review.googlesource.com/499627 Commit-Ready: Benson Leung <bleung@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Shawn N <shawnn@chromium.org> [modify] https://crrev.com/734ccb9b7f03746998881578fb815e9c36a86670/drivers/power/power_supply_sysfs.c [modify] https://crrev.com/734ccb9b7f03746998881578fb815e9c36a86670/include/linux/power_supply.h
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/001edd0c86183aa5f20c62e76136349a1bc0efa7 commit 001edd0c86183aa5f20c62e76136349a1bc0efa7 Author: Benson Leung <bleung@chromium.org> Date: Thu Jul 27 22:44:00 2017 CHROMIUM: cros_usbpd-charger - Use new Apple Brick ID type Use the new Apple Brick power supply type when "USB_CHG_TYPE_PROPRIETARY" is provided by the ec. On most of our Chromebooks (Samus, and Glados generation), that type will correspond directly to Apple 1.0A, 2.0A, or 2.4A methods being detected. This will allow us to gather better metrics in the power manager when Apple Brick ID method is used on our Type-C Chromebooks. Signed-off-by: Benson Leung <bleung@chromium.org> BUG= chromium:698133 TEST=With an Apple 12W (2.4A), 10W (2.1A), or 5W (1A) power adapter attached, cat /sys/class/power_supply/CROS_USB_PD_CHARGER1/type Check that "BrickID" is returned. Change-Id: Ic6722194adde42d505feb890fa62661a6a27c4fa Reviewed-on: https://chromium-review.googlesource.com/499629 Commit-Ready: Benson Leung <bleung@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Shawn N <shawnn@chromium.org> [modify] https://crrev.com/001edd0c86183aa5f20c62e76136349a1bc0efa7/drivers/power/cros_usbpd-charger.c
,
Jul 27 2017
Landed in M-62. Sorry for the delay everyone.
,
Sep 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4764eb8cf97b6c7298d07a77e2a19070c54e4b6f commit 4764eb8cf97b6c7298d07a77e2a19070c54e4b6f Author: Guenter Roeck <groeck@chromium.org> Date: Thu Sep 28 02:03:00 2017 CHROMIUM: cros_usbpd-charger - Use new Apple Brick ID type Use the new Apple Brick power supply type when "USB_CHG_TYPE_PROPRIETARY" is provided by the ec. On most of our Chromebooks (Samus, and Glados generation), that type will correspond directly to Apple 1.0A, 2.0A, or 2.4A methods being detected. This will allow us to gather better metrics in the power manager when Apple Brick ID method is used on our Type-C Chromebooks. Signed-off-by: Benson Leung <bleung@chromium.org> BUG= chromium:698133 TEST=With an Apple 12W (2.4A), 10W (2.1A), or 5W (1A) power adapter attached, cat /sys/class/power_supply/CROS_USB_PD_CHARGER1/type Check that "BrickID" is returned. Change-Id: Ic6722194adde42d505feb890fa62661a6a27c4fa Reviewed-on: https://chromium-review.googlesource.com/498059 Commit-Ready: Benson Leung <bleung@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Shawn N <shawnn@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/688107 Commit-Ready: Guenter Roeck <groeck@chromium.org> Tested-by: Guenter Roeck <groeck@chromium.org> Reviewed-by: Dmitry Torokhov <dtor@chromium.org> [modify] https://crrev.com/4764eb8cf97b6c7298d07a77e2a19070c54e4b6f/drivers/power/supply/cros_usbpd-charger.c
,
Sep 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/c34f2e426c6f8c8374df5b76f6a8d7ea585db18d commit c34f2e426c6f8c8374df5b76f6a8d7ea585db18d Author: Guenter Roeck <groeck@chromium.org> Date: Thu Sep 28 02:02:59 2017 BACKPORT: FROMLIST: power_supply: Add Apple Brick ID power supply type Apple currently supports three very common USB chargers: https://www.apple.com/power-adapters/ These chargers implement a proprietary Apple method for advertising 1A, 2.1A, and 2.4A at 5V called "Brick ID". In addition, 3rd parties implement the same charging method in many charging accessories that work with iOS devices. Devices that have charger detection chips such as the Pericom PI3USB9281, eg. Google Chromebook Pixel 2015, are capable of detecting these chargers, so let's add a type to facilicate passing that info up to userspace. This adds a separate power supply type for Apple's proprietary "Brick ID" charging method. Signed-off-by: Benson Leung <bleung@chromium.org> (bleung: am from https://patchwork.kernel.org/patch/9716765 Fixed up due to moved new drivers/power/supply and some other small context) BUG= chromium:698133 TEST=With an Apple 12W (2.4A), 10W (2.1A), or 5W (1A) power adapter attached, cat /sys/class/power_supply/CROS_USB_PD_CHARGER1/type Check that "BrickID" is returned. Change-Id: I75164c3c9e61ecba4a82f1c7ffe2cbf17a0ee7e7 Reviewed-on: https://chromium-review.googlesource.com/498058 Commit-Ready: Benson Leung <bleung@chromium.org> Tested-by: Benson Leung <bleung@chromium.org> Reviewed-by: Todd Broch <tbroch@chromium.org> [rebase412(groeck): sysfs file moved; context conflicts in include/linux/power_supply.h] Signed-off-by: Guenter Roeck <groeck@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/688113 Reviewed-by: Dmitry Torokhov <dtor@chromium.org> [modify] https://crrev.com/c34f2e426c6f8c8374df5b76f6a8d7ea585db18d/drivers/power/supply/power_supply_sysfs.c [modify] https://crrev.com/c34f2e426c6f8c8374df5b76f6a8d7ea585db18d/include/linux/power_supply.h |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by bleung@chromium.org
, Mar 3 2017