Hi, On 16/8/19 19:02, Duncan Laurie wrote: > On Fri, Aug 16, 2019 at 2:12 AM Greg Kroah-Hartman > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: >> >> On Thu, Aug 15, 2019 at 05:42:05PM -0600, Duncan Laurie wrote: >>> On Wed, Aug 14, 2019 at 6:08 PM Nick Crews <ncrews@xxxxxxxxxxxx> wrote: >>>> >>>> Adding Duncan Laurie who I think has some more intimate knowledge >>>> of how this is implemented in HW. Duncan, could you correct or elaborate >>>> on my answers below as you see fit? Also, sorry if I make some beginner >>>> mistakes here, I'm just getting familiar with the USB subsystem, and thanks for >>>> your patience. >>>> >>>> On Wed, Aug 14, 2019 at 3:20 PM Greg Kroah-Hartman >>>> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: >>>>> >>>>> On Wed, Aug 14, 2019 at 02:12:07PM -0600, Nick Crews wrote: >>>>>> Thanks for the fast response! >>>>>> >>>>>> On Tue, Aug 13, 2019 at 12:02 AM Greg Kroah-Hartman >>>>>> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: >>>>>>> >>>>>>> On Mon, Aug 12, 2019 at 06:08:43PM -0600, Nick Crews wrote: >>>>>>>> Hi Greg! >>>>>>> >>>>>>> Hi! >>>>>>> >>>>>>> First off, please fix your email client to not send html so that vger >>>>>>> does not reject your messages :) >>>>>> >>>>>> Thanks, should be good now. >>>>>> >>>>>>> >>>>>>>> I am working on a Chrome OS device that supports a policy called "USB Power >>>>>>>> Share," which allows users to turn the laptop into a charge pack for their >>>>>>>> phone. When the policy is enabled, power will be supplied to the USB ports >>>>>>>> even when the system is in low power states such as S3 and S5. When >>>>>>>> disabled, then no power will be supplied in S3 and S5. I wrote a driver >>>>>>>> <https://lore.kernel.org/patchwork/patch/1062995/> for this already as part >>>>>>>> of drivers/platform/chrome/, but Enric Balletbo i Serra, the maintainer, >>>>>>>> had the reasonable suggestion of trying to move this into the USB subsystem. >>>>>>> >>>>>>> Correct suggestion. >>>>>>> >>>>>>>> Has anything like this been done before? Do you have any preliminary >>>>>>>> thoughts on this before I start writing code? A few things that I haven't >>>>>>>> figured out yet: >>>>>>>> - How to make this feature only available on certain devices. Using device >>>>>>>> tree? Kconfig? Making a separate driver just for this device that plugs >>>>>>>> into the USB core? >>>>>>>> - The feature is only supported on some USB ports, so we need a way of >>>>>>>> filtering on a per-port basis. >>>>>>> >>>>>>> Look at the drivers/usb/typec/ code, I think that should do everything >>>>>>> you need here as this is a typec standard functionality, right? >>>>>> >>>>>> Unfortunately this is for USB 2.0 ports, so it's not type-C. >>>>>> Is the type-C code still worth looking at? >>>>> >>>>> If this is for USB 2, does it use the "non-standard" hub commands to >>>>> turn on and off power? If so, why not just use the usbreset userspace >>>>> program for that? >>>> >>>> It does not use the standard hub commands. The USB ports are controlled >>>> by an Embedded Controller (EC), so to control this policy we send a command >>>> to the EC. Since the command to send to the EC is very specific, this would need >>>> to go into a "hub driver" unique for these Wilco devices. We would make it so >>>> that the normal hub registration is intercepted by something that sees this is a >>>> Wilco device, and instead register the hub as a "wilco-hub", which has its own >>>> special "power_share" sysfs attribute, but still is treated as a normal USB hub >>>> otherwise? >>>> >>> >>> >>> I would say it is somewhat similar to the USB port power control which >>> eventually calls into usb_acpi_set_power_state() but in this case it only >>> affects the behavior when the system is NOT running. >> >> Ok, if this is when the system is not running, why does Linux need to be >> involved at all? >> >> And if Linux is running, why not just follow the USB spec and not create >> your own craziness? >> >>> This design has a standalone USB charge power controller on the board >>> that passes through the USB2 D+/D- pins from one port and is able to do >>> BC1.2 negotiation when the host controller is not powered, assuming >>> the chip has been enabled by the Embedded Controller. >> >> So it does follow the spec? Or does not? I can't determine here. >> > > > I didn't realize the part had a public datasheet: > https://www.dialog-semiconductor.com/sites/default/files/xslgc55544cr105_09292017.pdf > > It is really only concerned with following the BC1.2 spec and not > interfering with the USB protocol part. > Without knowing the internal design, but having more infor now, looks to me that should be modelled more as a kind of power supply? Maybe something similar to UCS1002-2 device (drivers/power/supply/ucs1002_power.c) but behind the EC? Cheers, Enric > >> If the EC is in charge of all of this, why does Linux need to get >> involved? >> > > Only because we are looking to expose a policy to control the behavior > of this chip at the OS level. > > Most systems would put this in as an option in the BIOS but we do > not have setup menus on Chrome OS and we want to have the policy > controlled directly, preferably without resorting to an opaque interface > to a userspace utility. > > To that end we have added a number of different EC controls and are > looking to fit them into the appropriate subsystems wherever possible. > As you can see they don't always fit naturally.. > > >>>>> And how are you turning a USB 2 port into a power source? That feels >>>>> really odd given the spec. Is this part of the standard somewhere or >>>>> just a firmware/hardware hack that you are adding to a device? >>>> >>>> The EC twiddles something in the port' HW so that the port turns into a >>>> DCP (Dedicated Charging Port) and only supplies power, not data. So I >>>> think yes, this is a bit of a hack that does not conform to the spec. >>>> >>>>> >>>>> Is there some port information in the firmware that describes this >>>>> functionality? If so, can you expose it through sysfs to the port that >>>>> way? >>>> >>>> [I'm not sure I'm answering your question, but] I believe that we could >>>> make the BIOS firmware describe the USB ports' capabilities, and the >>>> kernel's behavior would be gated upon what the firmware reports. I see >>>> that struct usb_port already contains a "quirks" field, should we add a >>>> POWER_SHARE quirk to include/linux/usb/quirks.h? I would guess that >>>> should that should be reserved for quirks shared between many USB >>>> devices/hubs? >>> >>> We could add a Device Property to the affected USB port in ACPI and >>> describe it that way, similar to other properties like 'vcc-supply', 'clocks', >>> 'vbus-detect', etc and hook it into the phy-generic driver. >>> >>> However I'm not clear on whether the phy driver binding works with XHCI >>> when using ACPI, so this may not be an appropriate place either. >> >> Why would you have DT involved if you are using acpi? :) >> > > This would come in via the _DSD method of passing parameters to > specific ACPI devices. > > > -duncan >
