| Message ID | 20200901132005.RFC.1.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] USB: misc: Add usb_hub_pwr driver | expand |
Hi, On Tue, Sep 1, 2020 at 1:21 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > The driver combo usb_hub_pwr/usb_hub_psupply allows to control > the power supply of an onboard USB hub. > > The drivers address two issues: > - a USB hub needs to be powered before it can be discovered > - battery powered devices may want to switch the USB hub off > during suspend to extend battery life > > The regulator of the hub is controlled by the usb_hub_psupply > platform driver. The regulator is switched on when the platform > device is initialized, which enables discovery of the hub. The > driver provides an external interface to enable/disable the > power supply which is used by the usb_hub_pwr driver. > > The usb_hub_pwr extends the generic USB hub driver. The device is > initialized when the hub is discovered by the USB subsystem. It > uses the usb_hub_psupply interface to make its own request to > enable the regulator (increasing the use count to 2). > > During system suspend usb_hub_pwr checks if any wakeup capable > devices are connected to the hub. If not it 'disables' the hub > regulator (decreasing the use count to 1, hence the regulator > stays enabled for now). When the usb_hub_psupply device suspends > it disables the hub regulator unconditionally (decreasing the use > count to 0 or 1, depending on the actions of usb_hub_pwr). This > is done to allow the usb_hub_pwr device to control the state of > the regulator during system suspend. > > Upon resume usb_hub_psupply enables the regulator again, the > usb_hub_pwr device does the same if it disabled the regulator > during resume. > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > The driver currently only supports a single power supply. This should > work for most/many configurations/hubs, support for multiple power > supplies can be added later if needed. > > No DT bindings are included since this is just a RFC. Here is a DT > example: > > usb_hub_psupply: usb-hub-psupply { > compatible = "linux,usb_hub_psupply"; > vdd-supply = <&pp3300_hub>; > }; Definitely bikeshedding, but I would name this differently. The name/compatible you have makes this sound as if it's a software concept that we're sticking into DT. That's generally discouraged. ...if we name it slightly different then I think the driver can work the same but be more in the spirit of DT describing hardware. Specifically, I think it'd be better as: usb_hub: usb-hub { compatible = "realtek,rts5411", "onboard-usb-hub"; vdd-supply = <&pp3300_hub>; }; Now we're describing hardware that's on the board. We have a RTS5411 hub and we've described its power supply. There's also precedent for describing an on-board USB hub in a lop-level node like this (smsc,usb3503). Now, I know what you're saying: we're already describing this hub underneath the USB port node below. I think this is OK to have different aspects of the device described in two places in the DT, though of course I could be corrected by someone more knowledgeable. > &usb_1_dwc3 { > /* 2.0 hub on port 1 */ > hub@1 { > compatible = "usbbda,5411"; What is "usbbda"? I would probably just call this: compatible = "realtek,rts5411-usb2", "onboard-usb-hub-usb2" > reg = <1>; > psupply = <&usb_hub_psupply>; Calling this psupply sounds a bit too much like you're referring to a regulator (with the -supply suffix). Given that I've proposed calling the main device "usb-hub" what about just saying "hub = <&usb_hub>;" > }; > > /* 3.0 hub on port 2 */ > hub@2 { > compatible = "usbbda,411"; Similar to above: compatible = "realtek,rts5411-usb3", "onboard-usb-hub-usb3" > reg = <2>; > psupply = <&usb_hub_psupply>; > }; > }; > > drivers/usb/misc/Kconfig | 14 +++ > drivers/usb/misc/Makefile | 1 + > drivers/usb/misc/usb_hub_psupply.c | 112 ++++++++++++++++++ > drivers/usb/misc/usb_hub_psupply.h | 9 ++ > drivers/usb/misc/usb_hub_pwr.c | 177 +++++++++++++++++++++++++++++ > 5 files changed, 313 insertions(+) > create mode 100644 drivers/usb/misc/usb_hub_psupply.c > create mode 100644 drivers/usb/misc/usb_hub_psupply.h > create mode 100644 drivers/usb/misc/usb_hub_pwr.c > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > index 6818ea689cd9..79ed50e6a7bf 100644 > --- a/drivers/usb/misc/Kconfig > +++ b/drivers/usb/misc/Kconfig > @@ -275,3 +275,17 @@ config USB_CHAOSKEY > > To compile this driver as a module, choose M here: the > module will be called chaoskey. > + > +config USB_HUB_PWR > + tristate "Control power supply for onboard USB hubs" > + depends on PM > + help > + Say Y here if you want to control the power supply of an > + onboard USB hub. The driver switches the power supply of the > + hub on, to make sure the hub can be discovered. During system > + suspend the power supply is switched off, unless a wakeup > + capable device is connected to the hub. This may reduce power > + consumption on battery powered devices. > + > + To compile this driver as a module, choose M here: the > + module will be called usb_hub_pwr. > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile > index da39bddb0604..2bd02388ca62 100644 > --- a/drivers/usb/misc/Makefile > +++ b/drivers/usb/misc/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o > > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/ > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o > +obj-$(CONFIG_USB_HUB_PWR) += usb_hub_pwr.o usb_hub_psupply.o > diff --git a/drivers/usb/misc/usb_hub_psupply.c b/drivers/usb/misc/usb_hub_psupply.c > new file mode 100644 > index 000000000000..6a155ae1f831 > --- /dev/null > +++ b/drivers/usb/misc/usb_hub_psupply.c > @@ -0,0 +1,112 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2020, Google LLC > + */ > + > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > + > +struct usb_hub_psupply_dev { > + struct regulator *vdd; > +}; Until someone has a need for more, I'd just pass your "struct regulator *" as the driver data and get rid of this pointless wrapper. > +int usb_hub_psupply_on(struct device *dev) > +{ > + struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev); > + int err; > + > + err = regulator_enable(usb_hub_psupply->vdd); > + if (err) { > + dev_err(dev, "failed to enable regulator: %d\n", err); > + return err; > + } > + > + return 0; > + nit: blank line > +} > +EXPORT_SYMBOL_GPL(usb_hub_psupply_on); > + > +int usb_hub_psupply_off(struct device *dev) > +{ > + struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev); > + int err; > + > + err = regulator_disable(usb_hub_psupply->vdd); > + if (err) { > + dev_err(dev, "failed to enable regulator: %d\n", err); > + return err; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(usb_hub_psupply_off); > + > +static int usb_hub_psupply_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct usb_hub_psupply_dev *usb_hub_psupply; > + > + usb_hub_psupply = devm_kzalloc(dev, sizeof(*usb_hub_psupply), GFP_KERNEL); > + if (!usb_hub_psupply) > + return -ENOMEM; > + > + dev_set_drvdata(dev, usb_hub_psupply); > + > + usb_hub_psupply->vdd = devm_regulator_get(dev, "vdd"); > + if (IS_ERR(usb_hub_psupply->vdd)) > + return PTR_ERR(usb_hub_psupply->vdd); > + > + return usb_hub_psupply_on(dev); > +} > + > +static int usb_hub_psupply_remove(struct platform_device *pdev) > +{ > + return usb_hub_psupply_off(&pdev->dev); > +} > + > +static int usb_hub_psupply_suspend(struct platform_device *pdev, pm_message_t msg) > +{ > + return usb_hub_psupply_off(&pdev->dev); > +} > + > +static int usb_hub_psupply_resume(struct platform_device *pdev) > +{ > + return usb_hub_psupply_on(&pdev->dev); > +} > + > +static const struct of_device_id usb_hub_psupply_match[] = { > + { .compatible = "linux,usb_hub_psupply" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, usb_hub_psupply_match); > + > +static struct platform_driver usb_hub_psupply_driver = { > + .probe = usb_hub_psupply_probe, > + .remove = usb_hub_psupply_remove, > + .suspend = usb_hub_psupply_suspend, SET_SYSTEM_SLEEP_PM_OPS? Then you also need to add "__maybe_unused" to your suspend/resume functions. > + .resume = usb_hub_psupply_resume, > + .driver = { > + .name = "usb-hub-psupply", > + .of_match_table = usb_hub_psupply_match, > + }, > +}; > + > +static int __init usb_hub_psupply_init(void) > +{ > + return platform_driver_register(&usb_hub_psupply_driver); > +} > +device_initcall(usb_hub_psupply_init); > + > +static void __exit usb_hub_psupply_exit(void) > +{ > + platform_driver_unregister(&usb_hub_psupply_driver); > +} > +module_exit(usb_hub_psupply_exit); > + > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > +MODULE_DESCRIPTION("USB Hub Power Supply"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/usb/misc/usb_hub_psupply.h b/drivers/usb/misc/usb_hub_psupply.h > new file mode 100644 > index 000000000000..284e88f45fcf > --- /dev/null > +++ b/drivers/usb/misc/usb_hub_psupply.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef _USB_HUB_PSUPPLY_H > +#define _USB_HUB_PSUPPLY_H > + > +int usb_hub_psupply_on(struct device *dev); > +int usb_hub_psupply_off(struct device *dev); > + > +#endif /* _USB_HUB_PSUPPLY_H */ > diff --git a/drivers/usb/misc/usb_hub_pwr.c b/drivers/usb/misc/usb_hub_pwr.c > new file mode 100644 > index 000000000000..33945ca4a8c0 > --- /dev/null > +++ b/drivers/usb/misc/usb_hub_pwr.c > @@ -0,0 +1,177 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * USB hub power control > + * > + * Copyright (c) 2020, Google LLC > + */ > + > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/power_supply.h> > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > +#include <linux/usb.h> > +#include <linux/usb/hcd.h> > +#include "../core/usb.h" > +#include "usb_hub_psupply.h" > + > +#define VENDOR_ID_REALTEK 0x0bda > + > +struct usb_hub_pwr_dev { > + struct regulator *vdd; You don't use the regulator directly, right? So get rid of "vdd" here? > + struct device *psupply_dev; > + bool powered_off; > +}; > + > +static struct device *usb_pwr_find_psupply_dev(struct device *dev) > +{ > + const phandle *ph; > + struct device_node *np; > + struct platform_device *pdev; > + > + ph = of_get_property(dev->of_node, "psupply", NULL); > + if (!ph) { > + dev_err(dev, "failed to read 'psupply' property\n"); > + return ERR_PTR(-EINVAL); > + } > + > + np = of_find_node_by_phandle(be32_to_cpu(*ph)); > + if (!np) { > + dev_err(dev, "failed find device node for power supply\n"); > + return ERR_PTR(-EINVAL); > + } > + > + pdev = of_find_device_by_node(np); > + of_node_put(np); > + if (!pdev) > + return ERR_PTR(-EPROBE_DEFER); > + > + return &pdev->dev; > +} > + > +static int usb_hub_pwr_probe(struct usb_device *udev) > +{ > + struct device *dev = &udev->dev; > + struct usb_hub_pwr_dev *uhpw; > + struct device *psupply_dev; > + int err; > + > + /* ignore supported hubs without device tree node */ > + if (!dev->of_node) > + return -ENODEV; I can dig through the code if you don't know, but I'm hoping that -ENODEV means it'll fall back to the regular hub driver? > + psupply_dev = usb_pwr_find_psupply_dev(dev); > + if (IS_ERR(psupply_dev)) > + return PTR_ERR(psupply_dev); > + > + err = usb_generic_driver_probe(udev); > + if (err) { > + put_device(psupply_dev); > + return err; > + } > + > + uhpw = devm_kzalloc(dev, sizeof(*uhpw), GFP_KERNEL); > + if (!uhpw) { > + put_device(psupply_dev); > + return -ENOMEM; > + } > + > + dev_set_drvdata(&udev->dev, uhpw); > + > + uhpw->psupply_dev = psupply_dev; > + > + err = usb_hub_psupply_on(psupply_dev); > + if (err) { > + dev_err(dev, "failed to enable regulator: %d\n", err); > + put_device(psupply_dev); > + return err; > + } > + > + return 0; > +} > + > +static void usb_hub_pwr_disconnect(struct usb_device *udev) > +{ > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > + > + usb_hub_psupply_off(uhpw->psupply_dev); > + put_device(uhpw->psupply_dev); > +} > + > +static int usb_hub_pwr_suspend(struct usb_device *udev, pm_message_t msg) > +{ > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > + int err; > + > + err = usb_generic_driver_suspend(udev, msg); > + if (err) > + return err; > + > + if (!usb_wakeup_enabled_descendants(udev)) { > + usb_port_disable(udev); > + > + err = usb_hub_psupply_off(uhpw->psupply_dev); > + if (err) > + return err; > + > + uhpw->powered_off = true; > + } > + > + return 0; > +} > + > +static int usb_hub_pwr_resume(struct usb_device *udev, pm_message_t msg) > +{ > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > + int err; > + > + if (uhpw->powered_off) { > + err = usb_hub_psupply_on(uhpw->psupply_dev); > + if (err) > + return err; > + > + uhpw->powered_off = false; > + } > + > + return usb_generic_driver_resume(udev, msg); > +} > + > +static const struct usb_device_id hub_id_table[] = { > + { .idVendor = VENDOR_ID_REALTEK, > + .idProduct = 0x0411, /* RTS5411 USB 3.0 */ > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > + { .idVendor = VENDOR_ID_REALTEK, > + .idProduct = 0x5411, /* RTS5411 USB 2.0 */ > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(usb, hub_id_table); > + > +static struct usb_device_driver usb_hub_pwr_driver = { > + > + .name = "usb-hub-pwr", > + .probe = usb_hub_pwr_probe, > + .disconnect = usb_hub_pwr_disconnect, > + .suspend = usb_hub_pwr_suspend, > + .resume = usb_hub_pwr_resume, > + .id_table = hub_id_table, I'm not an expert, but do you need "supports_autosuspend"? I think we want autosuspend enabled so nothing is plugged into the hub (or only things that can autosuspend) then we can save power. I can dig more too, if you don't know. > +}; > + > +static int __init usb_hub_pwr_driver_init(void) > +{ > + return usb_register_device_driver(&usb_hub_pwr_driver, THIS_MODULE); > +} > + > +static void __exit usb_hub_pwr_driver_exit(void) > +{ > + usb_deregister_device_driver(&usb_hub_pwr_driver); > +} > + > +module_init(usb_hub_pwr_driver_init); > +module_exit(usb_hub_pwr_driver_exit); > + > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > +MODULE_DESCRIPTION("USB Hub Power Control"); > +MODULE_LICENSE("GPL v2"); All the above is mostly just nits. To me the concept here seems sane. -Doug
Hi Doug, On Tue, Sep 01, 2020 at 02:21:49PM -0700, Doug Anderson wrote: > Hi, > > On Tue, Sep 1, 2020 at 1:21 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > The driver combo usb_hub_pwr/usb_hub_psupply allows to control > > the power supply of an onboard USB hub. > > > > The drivers address two issues: > > - a USB hub needs to be powered before it can be discovered > > - battery powered devices may want to switch the USB hub off > > during suspend to extend battery life > > > > The regulator of the hub is controlled by the usb_hub_psupply > > platform driver. The regulator is switched on when the platform > > device is initialized, which enables discovery of the hub. The > > driver provides an external interface to enable/disable the > > power supply which is used by the usb_hub_pwr driver. > > > > The usb_hub_pwr extends the generic USB hub driver. The device is > > initialized when the hub is discovered by the USB subsystem. It > > uses the usb_hub_psupply interface to make its own request to > > enable the regulator (increasing the use count to 2). > > > > During system suspend usb_hub_pwr checks if any wakeup capable > > devices are connected to the hub. If not it 'disables' the hub > > regulator (decreasing the use count to 1, hence the regulator > > stays enabled for now). When the usb_hub_psupply device suspends > > it disables the hub regulator unconditionally (decreasing the use > > count to 0 or 1, depending on the actions of usb_hub_pwr). This > > is done to allow the usb_hub_pwr device to control the state of > > the regulator during system suspend. > > > > Upon resume usb_hub_psupply enables the regulator again, the > > usb_hub_pwr device does the same if it disabled the regulator > > during resume. > > > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > The driver currently only supports a single power supply. This should > > work for most/many configurations/hubs, support for multiple power > > supplies can be added later if needed. > > > > No DT bindings are included since this is just a RFC. Here is a DT > > example: > > > > usb_hub_psupply: usb-hub-psupply { > > compatible = "linux,usb_hub_psupply"; > > vdd-supply = <&pp3300_hub>; > > }; > > Definitely bikeshedding, but I would name this differently. The > name/compatible you have makes this sound as if it's a software > concept that we're sticking into DT. That's generally discouraged. > ...if we name it slightly different then I think the driver can work > the same but be more in the spirit of DT describing hardware. > Specifically, I think it'd be better as: > > usb_hub: usb-hub { > compatible = "realtek,rts5411", "onboard-usb-hub"; > vdd-supply = <&pp3300_hub>; > }; > > Now we're describing hardware that's on the board. We have a RTS5411 > hub and we've described its power supply. There's also precedent for > describing an on-board USB hub in a lop-level node like this > (smsc,usb3503). You are right, it definitely describes existing hardware, not some linux-ism. I also envisioned that there might be eventually device specific compatible strings to support multiple regulators or even hub specific power on/off sequences. > Now, I know what you're saying: we're already describing this hub > underneath the USB port node below. I think this is OK to have > different aspects of the device described in two places in the DT, > though of course I could be corrected by someone more knowledgeable. > > > > &usb_1_dwc3 { > > /* 2.0 hub on port 1 */ > > hub@1 { > > compatible = "usbbda,5411"; > > What is "usbbda"? I would probably just call this: > > compatible = "realtek,rts5411-usb2", "onboard-usb-hub-usb2" That's from the USB bindings: Required properties for device nodes: - compatible: "usbVID,PID", where VID is the vendor id and PID the product id. The textual representation of VID and PID shall be in lower case hexadecimal with leading zeroes suppressed. The other compatible strings from the above standard binding could also be used, but a device adhering to this binding may leave out all except for "usbVID,PID". https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/usb/usb-device.txt > > > > reg = <1>; > > psupply = <&usb_hub_psupply>; > > Calling this psupply sounds a bit too much like you're referring to a > regulator (with the -supply suffix). Given that I've proposed calling > the main device "usb-hub" what about just saying "hub = <&usb_hub>;" I envisioned it as an abstract regulator (potentially consisting of multiple regulators), but modelling it with the regulator framework also didn't seem the right thing to do. Using the hub abstraction sounds good to me. Maybe we should also come up with a better name for 'usb_hub_psupply', I never was overly fond of it. > > }; > > > > /* 3.0 hub on port 2 */ > > hub@2 { > > compatible = "usbbda,411"; > > Similar to above: > > compatible = "realtek,rts5411-usb3", "onboard-usb-hub-usb3" See above, need to follow the binding. At some point I experimented with using device class matching instead of VID/PID, but discarded it since I anticipated that the driver would have to handle eventually device specific regulator handling. Now that that is handled by the 'usb_hub_psupply' driver (initially there was a single driver) it might be feasible again (e.g. manage all hubs that have a DT node and a 'hub' property). > > reg = <2>; > > psupply = <&usb_hub_psupply>; > > }; > > }; > > > > drivers/usb/misc/Kconfig | 14 +++ > > drivers/usb/misc/Makefile | 1 + > > drivers/usb/misc/usb_hub_psupply.c | 112 ++++++++++++++++++ > > drivers/usb/misc/usb_hub_psupply.h | 9 ++ > > drivers/usb/misc/usb_hub_pwr.c | 177 +++++++++++++++++++++++++++++ > > 5 files changed, 313 insertions(+) > > create mode 100644 drivers/usb/misc/usb_hub_psupply.c > > create mode 100644 drivers/usb/misc/usb_hub_psupply.h > > create mode 100644 drivers/usb/misc/usb_hub_pwr.c > > > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > > index 6818ea689cd9..79ed50e6a7bf 100644 > > --- a/drivers/usb/misc/Kconfig > > +++ b/drivers/usb/misc/Kconfig > > @@ -275,3 +275,17 @@ config USB_CHAOSKEY > > > > To compile this driver as a module, choose M here: the > > module will be called chaoskey. > > + > > +config USB_HUB_PWR > > + tristate "Control power supply for onboard USB hubs" > > + depends on PM > > + help > > + Say Y here if you want to control the power supply of an > > + onboard USB hub. The driver switches the power supply of the > > + hub on, to make sure the hub can be discovered. During system > > + suspend the power supply is switched off, unless a wakeup > > + capable device is connected to the hub. This may reduce power > > + consumption on battery powered devices. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called usb_hub_pwr. > > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile > > index da39bddb0604..2bd02388ca62 100644 > > --- a/drivers/usb/misc/Makefile > > +++ b/drivers/usb/misc/Makefile > > @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o > > > > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/ > > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o > > +obj-$(CONFIG_USB_HUB_PWR) += usb_hub_pwr.o usb_hub_psupply.o > > diff --git a/drivers/usb/misc/usb_hub_psupply.c b/drivers/usb/misc/usb_hub_psupply.c > > new file mode 100644 > > index 000000000000..6a155ae1f831 > > --- /dev/null > > +++ b/drivers/usb/misc/usb_hub_psupply.c > > @@ -0,0 +1,112 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2020, Google LLC > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/regulator/consumer.h> > > + > > +struct usb_hub_psupply_dev { > > + struct regulator *vdd; > > +}; > > Until someone has a need for more, I'd just pass your "struct > regulator *" as the driver data and get rid of this pointless wrapper. Sounds good, initially there was at least another field, but as of now the struct is not needed. > > +int usb_hub_psupply_on(struct device *dev) > > +{ > > + struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev); > > + int err; > > + > > + err = regulator_enable(usb_hub_psupply->vdd); > > + if (err) { > > + dev_err(dev, "failed to enable regulator: %d\n", err); > > + return err; > > + } > > + > > + return 0; > > + > > nit: blank line > ok > > +} > > +EXPORT_SYMBOL_GPL(usb_hub_psupply_on); > > + > > +int usb_hub_psupply_off(struct device *dev) > > +{ > > + struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev); > > + int err; > > + > > + err = regulator_disable(usb_hub_psupply->vdd); > > + if (err) { > > + dev_err(dev, "failed to enable regulator: %d\n", err); > > + return err; > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(usb_hub_psupply_off); > > + > > +static int usb_hub_psupply_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct usb_hub_psupply_dev *usb_hub_psupply; > > + > > + usb_hub_psupply = devm_kzalloc(dev, sizeof(*usb_hub_psupply), GFP_KERNEL); > > + if (!usb_hub_psupply) > > + return -ENOMEM; > > + > > + dev_set_drvdata(dev, usb_hub_psupply); > > + > > + usb_hub_psupply->vdd = devm_regulator_get(dev, "vdd"); > > + if (IS_ERR(usb_hub_psupply->vdd)) > > + return PTR_ERR(usb_hub_psupply->vdd); > > + > > + return usb_hub_psupply_on(dev); > > +} > > + > > +static int usb_hub_psupply_remove(struct platform_device *pdev) > > +{ > > + return usb_hub_psupply_off(&pdev->dev); > > +} > > + > > +static int usb_hub_psupply_suspend(struct platform_device *pdev, pm_message_t msg) > > +{ > > + return usb_hub_psupply_off(&pdev->dev); > > +} > > + > > +static int usb_hub_psupply_resume(struct platform_device *pdev) > > +{ > > + return usb_hub_psupply_on(&pdev->dev); > > +} > > + > > +static const struct of_device_id usb_hub_psupply_match[] = { > > + { .compatible = "linux,usb_hub_psupply" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, usb_hub_psupply_match); > > + > > +static struct platform_driver usb_hub_psupply_driver = { > > + .probe = usb_hub_psupply_probe, > > + .remove = usb_hub_psupply_remove, > > + .suspend = usb_hub_psupply_suspend, > > SET_SYSTEM_SLEEP_PM_OPS? ok > Then you also need to add "__maybe_unused" > to your suspend/resume functions. Currently there is a dependency on CONFIG_PM, however that isn't strictly needed, since the driver(s) is also useful besides suspend/resume to make sure the hub is powered. > > + .resume = usb_hub_psupply_resume, > > + .driver = { > > + .name = "usb-hub-psupply", > > + .of_match_table = usb_hub_psupply_match, > > + }, > > +}; > > + > > +static int __init usb_hub_psupply_init(void) > > +{ > > + return platform_driver_register(&usb_hub_psupply_driver); > > +} > > +device_initcall(usb_hub_psupply_init); > > + > > +static void __exit usb_hub_psupply_exit(void) > > +{ > > + platform_driver_unregister(&usb_hub_psupply_driver); > > +} > > +module_exit(usb_hub_psupply_exit); > > + > > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > > +MODULE_DESCRIPTION("USB Hub Power Supply"); > > +MODULE_LICENSE("GPL v2"); > > diff --git a/drivers/usb/misc/usb_hub_psupply.h b/drivers/usb/misc/usb_hub_psupply.h > > new file mode 100644 > > index 000000000000..284e88f45fcf > > --- /dev/null > > +++ b/drivers/usb/misc/usb_hub_psupply.h > > @@ -0,0 +1,9 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > + > > +#ifndef _USB_HUB_PSUPPLY_H > > +#define _USB_HUB_PSUPPLY_H > > + > > +int usb_hub_psupply_on(struct device *dev); > > +int usb_hub_psupply_off(struct device *dev); > > + > > +#endif /* _USB_HUB_PSUPPLY_H */ > > diff --git a/drivers/usb/misc/usb_hub_pwr.c b/drivers/usb/misc/usb_hub_pwr.c > > new file mode 100644 > > index 000000000000..33945ca4a8c0 > > --- /dev/null > > +++ b/drivers/usb/misc/usb_hub_pwr.c > > @@ -0,0 +1,177 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * USB hub power control > > + * > > + * Copyright (c) 2020, Google LLC > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_platform.h> > > +#include <linux/power_supply.h> > > +#include <linux/regulator/consumer.h> > > +#include <linux/slab.h> > > +#include <linux/usb.h> > > +#include <linux/usb/hcd.h> > > +#include "../core/usb.h" > > +#include "usb_hub_psupply.h" > > + > > +#define VENDOR_ID_REALTEK 0x0bda > > + > > +struct usb_hub_pwr_dev { > > + struct regulator *vdd; > > You don't use the regulator directly, right? So get rid of "vdd" here? Right, that's still a remainder from the early days, when there was a single driver. > > + struct device *psupply_dev; > > + bool powered_off; > > +}; > > + > > +static struct device *usb_pwr_find_psupply_dev(struct device *dev) > > +{ > > + const phandle *ph; > > + struct device_node *np; > > + struct platform_device *pdev; > > + > > + ph = of_get_property(dev->of_node, "psupply", NULL); > > + if (!ph) { > > + dev_err(dev, "failed to read 'psupply' property\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + np = of_find_node_by_phandle(be32_to_cpu(*ph)); > > + if (!np) { > > + dev_err(dev, "failed find device node for power supply\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + pdev = of_find_device_by_node(np); > > + of_node_put(np); > > + if (!pdev) > > + return ERR_PTR(-EPROBE_DEFER); > > + > > + return &pdev->dev; > > +} > > + > > +static int usb_hub_pwr_probe(struct usb_device *udev) > > +{ > > + struct device *dev = &udev->dev; > > + struct usb_hub_pwr_dev *uhpw; > > + struct device *psupply_dev; > > + int err; > > + > > + /* ignore supported hubs without device tree node */ > > + if (!dev->of_node) > > + return -ENODEV; > > I can dig through the code if you don't know, but I'm hoping that > -ENODEV means it'll fall back to the regular hub driver? exactly > > + psupply_dev = usb_pwr_find_psupply_dev(dev); > > + if (IS_ERR(psupply_dev)) > > + return PTR_ERR(psupply_dev); > > + > > + err = usb_generic_driver_probe(udev); > > + if (err) { > > + put_device(psupply_dev); > > + return err; > > + } > > + > > + uhpw = devm_kzalloc(dev, sizeof(*uhpw), GFP_KERNEL); > > + if (!uhpw) { > > + put_device(psupply_dev); > > + return -ENOMEM; > > + } > > + > > + dev_set_drvdata(&udev->dev, uhpw); > > + > > + uhpw->psupply_dev = psupply_dev; > > + > > + err = usb_hub_psupply_on(psupply_dev); > > + if (err) { > > + dev_err(dev, "failed to enable regulator: %d\n", err); > > + put_device(psupply_dev); > > + return err; > > + } > > + > > + return 0; > > +} > > + > > +static void usb_hub_pwr_disconnect(struct usb_device *udev) > > +{ > > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > > + > > + usb_hub_psupply_off(uhpw->psupply_dev); > > + put_device(uhpw->psupply_dev); > > +} > > + > > +static int usb_hub_pwr_suspend(struct usb_device *udev, pm_message_t msg) > > +{ > > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > > + int err; > > + > > + err = usb_generic_driver_suspend(udev, msg); > > + if (err) > > + return err; > > + > > + if (!usb_wakeup_enabled_descendants(udev)) { > > + usb_port_disable(udev); > > + > > + err = usb_hub_psupply_off(uhpw->psupply_dev); > > + if (err) > > + return err; > > + > > + uhpw->powered_off = true; > > + } > > + > > + return 0; > > +} > > + > > +static int usb_hub_pwr_resume(struct usb_device *udev, pm_message_t msg) > > +{ > > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > > + int err; > > + > > + if (uhpw->powered_off) { > > + err = usb_hub_psupply_on(uhpw->psupply_dev); > > + if (err) > > + return err; > > + > > + uhpw->powered_off = false; > > + } > > + > > + return usb_generic_driver_resume(udev, msg); > > +} > > + > > +static const struct usb_device_id hub_id_table[] = { > > + { .idVendor = VENDOR_ID_REALTEK, > > + .idProduct = 0x0411, /* RTS5411 USB 3.0 */ > > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > > + { .idVendor = VENDOR_ID_REALTEK, > > + .idProduct = 0x5411, /* RTS5411 USB 2.0 */ > > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > > + {}, > > +}; > > + > > +MODULE_DEVICE_TABLE(usb, hub_id_table); > > + > > +static struct usb_device_driver usb_hub_pwr_driver = { > > + > > + .name = "usb-hub-pwr", > > + .probe = usb_hub_pwr_probe, > > + .disconnect = usb_hub_pwr_disconnect, > > + .suspend = usb_hub_pwr_suspend, > > + .resume = usb_hub_pwr_resume, > > + .id_table = hub_id_table, > > I'm not an expert, but do you need "supports_autosuspend"? I think we > want autosuspend enabled so nothing is plugged into the hub (or only > things that can autosuspend) then we can save power. I can dig more > too, if you don't know. Good point, I naively expected the generic driver would still handle this automatically, but it doesn't. This will require some filtering in the suspend/resume handling, it turns out the handlers are called for runtime suspend/resume and we don't want to turn off/on the regulator(s) in this case (even though it's a NOP since the 'psupply' driver keeps them enabled). > > +}; > > + > > +static int __init usb_hub_pwr_driver_init(void) > > +{ > > + return usb_register_device_driver(&usb_hub_pwr_driver, THIS_MODULE); > > +} > > + > > +static void __exit usb_hub_pwr_driver_exit(void) > > +{ > > + usb_deregister_device_driver(&usb_hub_pwr_driver); > > +} > > + > > +module_init(usb_hub_pwr_driver_init); > > +module_exit(usb_hub_pwr_driver_exit); > > + > > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > > +MODULE_DESCRIPTION("USB Hub Power Control"); > > +MODULE_LICENSE("GPL v2"); > > All the above is mostly just nits. To me the concept here seems sane. Thanks for the review! m.
On 20-09-01 13:21:43, Matthias Kaehlcke wrote: > The driver combo usb_hub_pwr/usb_hub_psupply allows to control > the power supply of an onboard USB hub. > > The drivers address two issues: > - a USB hub needs to be powered before it can be discovered > - battery powered devices may want to switch the USB hub off > during suspend to extend battery life > > The regulator of the hub is controlled by the usb_hub_psupply > platform driver. The regulator is switched on when the platform > device is initialized, which enables discovery of the hub. The > driver provides an external interface to enable/disable the > power supply which is used by the usb_hub_pwr driver. > > The usb_hub_pwr extends the generic USB hub driver. The device is > initialized when the hub is discovered by the USB subsystem. It > uses the usb_hub_psupply interface to make its own request to > enable the regulator (increasing the use count to 2). > > During system suspend usb_hub_pwr checks if any wakeup capable > devices are connected to the hub. If not it 'disables' the hub > regulator (decreasing the use count to 1, hence the regulator > stays enabled for now). When the usb_hub_psupply device suspends > it disables the hub regulator unconditionally (decreasing the use > count to 0 or 1, depending on the actions of usb_hub_pwr). This > is done to allow the usb_hub_pwr device to control the state of > the regulator during system suspend. > > Upon resume usb_hub_psupply enables the regulator again, the > usb_hub_pwr device does the same if it disabled the regulator > during resume. Hi Matthias, I did similar several years ago [1], but the concept (power sequence) doesn't be accepted by power maintainer. Your patch introduce an new way to fix this long-term issue, I have an idea to fix it more generally. - Create a table (say usb_pm_table) for USB device which needs to do initial power on and power management during suspend suspend/resume based on VID and PID, example: usb/core/quirks.c - After hub (both roothub and intermediate hub) device is created, search the DT node under this hub, and see if the device is in usb_pm_table. If it is in it, create a platform device, say usb-power-supply, and the related driver is like your usb_hub_psupply.c, the parent of this device is controller device. - After this usb-power-supply device is probed, do initial power on at probe, eg, clock, regulator, reset-gpio. - This usb-power-supply device system suspend operation should be called after onboard device has suspended since it is created before it. No runtime PM ops are needed for it. - When the hub is removed, delete this platform device. What's your opinion? [1] https://lore.kernel.org/lkml/1498027328-25078-1-git-send-email-peter.chen@nxp.com/ Peter > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > The driver currently only supports a single power supply. This should > work for most/many configurations/hubs, support for multiple power > supplies can be added later if needed. > > No DT bindings are included since this is just a RFC. Here is a DT > example: > > usb_hub_psupply: usb-hub-psupply { > compatible = "linux,usb_hub_psupply"; > vdd-supply = <&pp3300_hub>; > }; > > &usb_1_dwc3 { > /* 2.0 hub on port 1 */ > hub@1 { > compatible = "usbbda,5411"; > reg = <1>; > psupply = <&usb_hub_psupply>; > }; > > /* 3.0 hub on port 2 */ > hub@2 { > compatible = "usbbda,411"; > reg = <2>; > psupply = <&usb_hub_psupply>; > }; > }; > > drivers/usb/misc/Kconfig | 14 +++ > drivers/usb/misc/Makefile | 1 + > drivers/usb/misc/usb_hub_psupply.c | 112 ++++++++++++++++++ > drivers/usb/misc/usb_hub_psupply.h | 9 ++ > drivers/usb/misc/usb_hub_pwr.c | 177 +++++++++++++++++++++++++++++ > 5 files changed, 313 insertions(+) > create mode 100644 drivers/usb/misc/usb_hub_psupply.c > create mode 100644 drivers/usb/misc/usb_hub_psupply.h > create mode 100644 drivers/usb/misc/usb_hub_pwr.c > > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig > index 6818ea689cd9..79ed50e6a7bf 100644 > --- a/drivers/usb/misc/Kconfig > +++ b/drivers/usb/misc/Kconfig > @@ -275,3 +275,17 @@ config USB_CHAOSKEY > > To compile this driver as a module, choose M here: the > module will be called chaoskey. > + > +config USB_HUB_PWR > + tristate "Control power supply for onboard USB hubs" > + depends on PM > + help > + Say Y here if you want to control the power supply of an > + onboard USB hub. The driver switches the power supply of the > + hub on, to make sure the hub can be discovered. During system > + suspend the power supply is switched off, unless a wakeup > + capable device is connected to the hub. This may reduce power > + consumption on battery powered devices. > + > + To compile this driver as a module, choose M here: the > + module will be called usb_hub_pwr. > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile > index da39bddb0604..2bd02388ca62 100644 > --- a/drivers/usb/misc/Makefile > +++ b/drivers/usb/misc/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o > > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/ > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o > +obj-$(CONFIG_USB_HUB_PWR) += usb_hub_pwr.o usb_hub_psupply.o > diff --git a/drivers/usb/misc/usb_hub_psupply.c b/drivers/usb/misc/usb_hub_psupply.c > new file mode 100644 > index 000000000000..6a155ae1f831 > --- /dev/null > +++ b/drivers/usb/misc/usb_hub_psupply.c > @@ -0,0 +1,112 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2020, Google LLC > + */ > + > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > + > +struct usb_hub_psupply_dev { > + struct regulator *vdd; > +}; > + > +int usb_hub_psupply_on(struct device *dev) > +{ > + struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev); > + int err; > + > + err = regulator_enable(usb_hub_psupply->vdd); > + if (err) { > + dev_err(dev, "failed to enable regulator: %d\n", err); > + return err; > + } > + > + return 0; > + > +} > +EXPORT_SYMBOL_GPL(usb_hub_psupply_on); > + > +int usb_hub_psupply_off(struct device *dev) > +{ > + struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev); > + int err; > + > + err = regulator_disable(usb_hub_psupply->vdd); > + if (err) { > + dev_err(dev, "failed to enable regulator: %d\n", err); > + return err; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(usb_hub_psupply_off); > + > +static int usb_hub_psupply_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct usb_hub_psupply_dev *usb_hub_psupply; > + > + usb_hub_psupply = devm_kzalloc(dev, sizeof(*usb_hub_psupply), GFP_KERNEL); > + if (!usb_hub_psupply) > + return -ENOMEM; > + > + dev_set_drvdata(dev, usb_hub_psupply); > + > + usb_hub_psupply->vdd = devm_regulator_get(dev, "vdd"); > + if (IS_ERR(usb_hub_psupply->vdd)) > + return PTR_ERR(usb_hub_psupply->vdd); > + > + return usb_hub_psupply_on(dev); > +} > + > +static int usb_hub_psupply_remove(struct platform_device *pdev) > +{ > + return usb_hub_psupply_off(&pdev->dev); > +} > + > +static int usb_hub_psupply_suspend(struct platform_device *pdev, pm_message_t msg) > +{ > + return usb_hub_psupply_off(&pdev->dev); > +} > + > +static int usb_hub_psupply_resume(struct platform_device *pdev) > +{ > + return usb_hub_psupply_on(&pdev->dev); > +} > + > +static const struct of_device_id usb_hub_psupply_match[] = { > + { .compatible = "linux,usb_hub_psupply" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, usb_hub_psupply_match); > + > +static struct platform_driver usb_hub_psupply_driver = { > + .probe = usb_hub_psupply_probe, > + .remove = usb_hub_psupply_remove, > + .suspend = usb_hub_psupply_suspend, > + .resume = usb_hub_psupply_resume, > + .driver = { > + .name = "usb-hub-psupply", > + .of_match_table = usb_hub_psupply_match, > + }, > +}; > + > +static int __init usb_hub_psupply_init(void) > +{ > + return platform_driver_register(&usb_hub_psupply_driver); > +} > +device_initcall(usb_hub_psupply_init); > + > +static void __exit usb_hub_psupply_exit(void) > +{ > + platform_driver_unregister(&usb_hub_psupply_driver); > +} > +module_exit(usb_hub_psupply_exit); > + > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > +MODULE_DESCRIPTION("USB Hub Power Supply"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/usb/misc/usb_hub_psupply.h b/drivers/usb/misc/usb_hub_psupply.h > new file mode 100644 > index 000000000000..284e88f45fcf > --- /dev/null > +++ b/drivers/usb/misc/usb_hub_psupply.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef _USB_HUB_PSUPPLY_H > +#define _USB_HUB_PSUPPLY_H > + > +int usb_hub_psupply_on(struct device *dev); > +int usb_hub_psupply_off(struct device *dev); > + > +#endif /* _USB_HUB_PSUPPLY_H */ > diff --git a/drivers/usb/misc/usb_hub_pwr.c b/drivers/usb/misc/usb_hub_pwr.c > new file mode 100644 > index 000000000000..33945ca4a8c0 > --- /dev/null > +++ b/drivers/usb/misc/usb_hub_pwr.c > @@ -0,0 +1,177 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * USB hub power control > + * > + * Copyright (c) 2020, Google LLC > + */ > + > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/power_supply.h> > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > +#include <linux/usb.h> > +#include <linux/usb/hcd.h> > +#include "../core/usb.h" > +#include "usb_hub_psupply.h" > + > +#define VENDOR_ID_REALTEK 0x0bda > + > +struct usb_hub_pwr_dev { > + struct regulator *vdd; > + struct device *psupply_dev; > + bool powered_off; > +}; > + > +static struct device *usb_pwr_find_psupply_dev(struct device *dev) > +{ > + const phandle *ph; > + struct device_node *np; > + struct platform_device *pdev; > + > + ph = of_get_property(dev->of_node, "psupply", NULL); > + if (!ph) { > + dev_err(dev, "failed to read 'psupply' property\n"); > + return ERR_PTR(-EINVAL); > + } > + > + np = of_find_node_by_phandle(be32_to_cpu(*ph)); > + if (!np) { > + dev_err(dev, "failed find device node for power supply\n"); > + return ERR_PTR(-EINVAL); > + } > + > + pdev = of_find_device_by_node(np); > + of_node_put(np); > + if (!pdev) > + return ERR_PTR(-EPROBE_DEFER); > + > + return &pdev->dev; > +} > + > +static int usb_hub_pwr_probe(struct usb_device *udev) > +{ > + struct device *dev = &udev->dev; > + struct usb_hub_pwr_dev *uhpw; > + struct device *psupply_dev; > + int err; > + > + /* ignore supported hubs without device tree node */ > + if (!dev->of_node) > + return -ENODEV; > + > + psupply_dev = usb_pwr_find_psupply_dev(dev); > + if (IS_ERR(psupply_dev)) > + return PTR_ERR(psupply_dev); > + > + err = usb_generic_driver_probe(udev); > + if (err) { > + put_device(psupply_dev); > + return err; > + } > + > + uhpw = devm_kzalloc(dev, sizeof(*uhpw), GFP_KERNEL); > + if (!uhpw) { > + put_device(psupply_dev); > + return -ENOMEM; > + } > + > + dev_set_drvdata(&udev->dev, uhpw); > + > + uhpw->psupply_dev = psupply_dev; > + > + err = usb_hub_psupply_on(psupply_dev); > + if (err) { > + dev_err(dev, "failed to enable regulator: %d\n", err); > + put_device(psupply_dev); > + return err; > + } > + > + return 0; > +} > + > +static void usb_hub_pwr_disconnect(struct usb_device *udev) > +{ > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > + > + usb_hub_psupply_off(uhpw->psupply_dev); > + put_device(uhpw->psupply_dev); > +} > + > +static int usb_hub_pwr_suspend(struct usb_device *udev, pm_message_t msg) > +{ > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > + int err; > + > + err = usb_generic_driver_suspend(udev, msg); > + if (err) > + return err; > + > + if (!usb_wakeup_enabled_descendants(udev)) { > + usb_port_disable(udev); > + > + err = usb_hub_psupply_off(uhpw->psupply_dev); > + if (err) > + return err; > + > + uhpw->powered_off = true; > + } > + > + return 0; > +} > + > +static int usb_hub_pwr_resume(struct usb_device *udev, pm_message_t msg) > +{ > + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); > + int err; > + > + if (uhpw->powered_off) { > + err = usb_hub_psupply_on(uhpw->psupply_dev); > + if (err) > + return err; > + > + uhpw->powered_off = false; > + } > + > + return usb_generic_driver_resume(udev, msg); > +} > + > +static const struct usb_device_id hub_id_table[] = { > + { .idVendor = VENDOR_ID_REALTEK, > + .idProduct = 0x0411, /* RTS5411 USB 3.0 */ > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > + { .idVendor = VENDOR_ID_REALTEK, > + .idProduct = 0x5411, /* RTS5411 USB 2.0 */ > + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, > + {}, > +}; > + > +MODULE_DEVICE_TABLE(usb, hub_id_table); > + > +static struct usb_device_driver usb_hub_pwr_driver = { > + > + .name = "usb-hub-pwr", > + .probe = usb_hub_pwr_probe, > + .disconnect = usb_hub_pwr_disconnect, > + .suspend = usb_hub_pwr_suspend, > + .resume = usb_hub_pwr_resume, > + .id_table = hub_id_table, > +}; > + > +static int __init usb_hub_pwr_driver_init(void) > +{ > + return usb_register_device_driver(&usb_hub_pwr_driver, THIS_MODULE); > +} > + > +static void __exit usb_hub_pwr_driver_exit(void) > +{ > + usb_deregister_device_driver(&usb_hub_pwr_driver); > +} > + > +module_init(usb_hub_pwr_driver_init); > +module_exit(usb_hub_pwr_driver_exit); > + > +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); > +MODULE_DESCRIPTION("USB Hub Power Control"); > +MODULE_LICENSE("GPL v2"); > -- > 2.28.0.402.g5ffc5be6b7-goog >
On Tue, Sep 01, 2020 at 01:21:43PM -0700, Matthias Kaehlcke wrote: > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile > index da39bddb0604..2bd02388ca62 100644 > --- a/drivers/usb/misc/Makefile > +++ b/drivers/usb/misc/Makefile > @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o > > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/ > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o > +obj-$(CONFIG_USB_HUB_PWR) += usb_hub_pwr.o usb_hub_psupply.o Why is this 2 files? Why can't it just be one? And isn't this much the same thing as many of the other "misc" hub controller drivers we have? And to make it easier to review, can you split out the device tree descriptions so that the DT maintainers can review that? thanks, greg k-h
Hi Peter, On Wed, Sep 02, 2020 at 05:31:06AM +0000, Peter Chen wrote: > On 20-09-01 13:21:43, Matthias Kaehlcke wrote: > > The driver combo usb_hub_pwr/usb_hub_psupply allows to control > > the power supply of an onboard USB hub. > > > > The drivers address two issues: > > - a USB hub needs to be powered before it can be discovered > > - battery powered devices may want to switch the USB hub off > > during suspend to extend battery life > > > > The regulator of the hub is controlled by the usb_hub_psupply > > platform driver. The regulator is switched on when the platform > > device is initialized, which enables discovery of the hub. The > > driver provides an external interface to enable/disable the > > power supply which is used by the usb_hub_pwr driver. > > > > The usb_hub_pwr extends the generic USB hub driver. The device is > > initialized when the hub is discovered by the USB subsystem. It > > uses the usb_hub_psupply interface to make its own request to > > enable the regulator (increasing the use count to 2). > > > > During system suspend usb_hub_pwr checks if any wakeup capable > > devices are connected to the hub. If not it 'disables' the hub > > regulator (decreasing the use count to 1, hence the regulator > > stays enabled for now). When the usb_hub_psupply device suspends > > it disables the hub regulator unconditionally (decreasing the use > > count to 0 or 1, depending on the actions of usb_hub_pwr). This > > is done to allow the usb_hub_pwr device to control the state of > > the regulator during system suspend. > > > > Upon resume usb_hub_psupply enables the regulator again, the > > usb_hub_pwr device does the same if it disabled the regulator > > during resume. > > Hi Matthias, > > I did similar several years ago [1], but the concept (power sequence) > doesn't be accepted by power maintainer. Yeah, I saw that, I think I even reviewed or tested some early version of it :) > Your patch introduce an new way to fix this long-term issue, I have an > idea to fix it more generally. > - Create a table (say usb_pm_table) for USB device which needs to do > initial power on and power management during suspend suspend/resume based > on VID and PID, example: usb/core/quirks.c > - After hub (both roothub and intermediate hub) device is created, search > the DT node under this hub, and see if the device is in usb_pm_table. If > it is in it, create a platform device, say usb-power-supply, and the > related driver is like your usb_hub_psupply.c, the parent of this device > is controller device. This part isn't clear to me. How would the DT look like? Would it have a single node per physical hub chip or one node for each 'logical' hub? Similarly, would there be a single plaform device or multiple? I guess when registering the platform device we could pass it the corresponding DT node, to allow it to determine its regulators, GPIOs, etc during probe. > - After this usb-power-supply device is probed, do initial power on at > probe, eg, clock, regulator, reset-gpio. > - This usb-power-supply device system suspend operation should be called after > onboard device has suspended since it is created before it. No runtime PM ops > are needed for it. > - When the hub is removed, delete this platform device. What exactly is removal? It seems once the hub is 'removed' it could never be probed again because the platform device that is in charge of the initialization is only created when the USB controller is initialized. I have the impression that the platform device would have to exist as long as the USB controller.
Hi Greg, On Wed, Sep 02, 2020 at 08:07:44AM +0200, Greg Kroah-Hartman wrote: > On Tue, Sep 01, 2020 at 01:21:43PM -0700, Matthias Kaehlcke wrote: > > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile > > index da39bddb0604..2bd02388ca62 100644 > > --- a/drivers/usb/misc/Makefile > > +++ b/drivers/usb/misc/Makefile > > @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o > > > > obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/ > > obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o > > +obj-$(CONFIG_USB_HUB_PWR) += usb_hub_pwr.o usb_hub_psupply.o > > Why is this 2 files? Why can't it just be one? It's effectively two drivers that work together, it seemed cleaner to me to have a file for every driver. The 'usb_hub_psupply' driver (which probably should be named 'onboard_usb_hub' or similar) would even be useful on its own (taking care of powering the hub on and potentially other setup actions) with a bit of rework. > And isn't this much the same thing as many of the other "misc" hub > controller drivers we have? There are some commonalities, however most of these drivers seem to target USB hubs with an I2C bus, using custom 'commands' for initialization and putting the hub in/out of standby. The drivers in this patch have two goals: - provide a generic (configurable) solution for powering up/initializing a USB hub - enable support for powering down a USB hub during system suspend > And to make it easier to review, can you split out the device tree > descriptions so that the DT maintainers can review that? Sure, I wasn't sure if this is the right approach, hence I only sent an RFC without formal bindings to get initial feedback. As of now it seems that you are at least not frontally opposed it ;-)
On 20-09-02 10:45:36, Matthias Kaehlcke wrote: > > > > Hi Matthias, > > > > I did similar several years ago [1], but the concept (power sequence) > > doesn't be accepted by power maintainer. > > Yeah, I saw that, I think I even reviewed or tested some early version > of it :) > > > Your patch introduce an new way to fix this long-term issue, I have an > > idea to fix it more generally. > > > - Create a table (say usb_pm_table) for USB device which needs to do > > initial power on and power management during suspend suspend/resume based > > on VID and PID, example: usb/core/quirks.c > > - After hub (both roothub and intermediate hub) device is created, search > > the DT node under this hub, and see if the device is in usb_pm_table. If > > it is in it, create a platform device, say usb-power-supply, and the > > related driver is like your usb_hub_psupply.c, the parent of this device > > is controller device. > > This part isn't clear to me. How would the DT look like? Would it have a > single node per physical hub chip or one node for each 'logical' hub? > Similarly, would there be a single plaform device or multiple? One power supply platform device for one physical device, and DT only describes physical device. HUB driver only probes non-SS HUB port to avoid create two power supply device for SS HUB, there should be no SS-only HUB. Below is the example of DT entry, there is a dwc3 host, and one USB HUB on it, there is a onboard USB ethernet chip on HUB's port 1. &usb_1_dwc3 { status = "okay"; usb2415: hub@1 { compatible = "usb424,2514"; reg = <1>; clocks = <&clk, IMX6QDL_CLK_CKO>; reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>; reset-duration-us = <3000>; vdd-supply = <®_vdd_hub_usb2415>; genesys: ethernet@1 { compatible = "usb5e3,608"; reg = <1>; vdd-supply = <®_vdd_genesys>; }; }; > > I guess when registering the platform device we could pass it the > corresponding DT node, to allow it to determine its regulators, GPIOs, > etc during probe. > > > - After this usb-power-supply device is probed, do initial power on at > > probe, eg, clock, regulator, reset-gpio. > > - This usb-power-supply device system suspend operation should be called after > > onboard device has suspended since it is created before it. No runtime PM ops > > are needed for it. > > - When the hub is removed, delete this platform device. > > What exactly is removal? It seems once the hub is 'removed' it could never be > probed again because the platform device that is in charge of the > initialization is only created when the USB controller is initialized. I have > the impression that the platform device would have to exist as long as the USB > controller. This USB power supply device services USB devices on HUB ports (HUB is also one of USB devices). For USB devices under the roothub, it will be powered on when the roothub is probed.[1] [1] https://lore.kernel.org/lkml/1498027328-25078-5-git-send-email-peter.chen@nxp.com/
Hi Peter, sorry for the delayed reply, I got distracted by other things and ran into issues with my mail setup. On Thu, Sep 03, 2020 at 01:46:18AM +0000, Peter Chen wrote: > On 20-09-02 10:45:36, Matthias Kaehlcke wrote: > > > > > > Hi Matthias, > > > > > > I did similar several years ago [1], but the concept (power sequence) > > > doesn't be accepted by power maintainer. > > > > Yeah, I saw that, I think I even reviewed or tested some early version > > of it :) > > > > > Your patch introduce an new way to fix this long-term issue, I have an > > > idea to fix it more generally. > > > > > - Create a table (say usb_pm_table) for USB device which needs to do > > > initial power on and power management during suspend suspend/resume based > > > on VID and PID, example: usb/core/quirks.c > > > - After hub (both roothub and intermediate hub) device is created, search > > > the DT node under this hub, and see if the device is in usb_pm_table. If > > > it is in it, create a platform device, say usb-power-supply, and the > > > related driver is like your usb_hub_psupply.c, the parent of this device > > > is controller device. > > > > This part isn't clear to me. How would the DT look like? Would it have a > > single node per physical hub chip or one node for each 'logical' hub? > > Similarly, would there be a single plaform device or multiple? > > One power supply platform device for one physical device, and DT only > describes physical device. HUB driver only probes non-SS HUB port to > avoid create two power supply device for SS HUB, there should be no > SS-only HUB. I agree that there should be only one platform device per physical device. Probing only the non-SS hub should work to avoid multiple instances, however it doesn't work for the extended use case, where the hub is powered off during system suspend, but only when no wakeup capable devices are connected to any of the 'logical' hubs. For this to work the driver that controls the regulators, GPIOs, ... needs to have knowledge of all 'logical' hubs. I just sent v1 of this driver, which reworks things a bit, but for now there is still one platform device instantiated through the DT, and one DT entry for every 'logical' hub. I'm open to keep discussing alternative designs, as long as they can also cover the use case of conditionally powering the hub off during system suspend. We can probably continue the discussion on v1, unless it takes me longer than
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig index 6818ea689cd9..79ed50e6a7bf 100644 --- a/drivers/usb/misc/Kconfig +++ b/drivers/usb/misc/Kconfig @@ -275,3 +275,17 @@ config USB_CHAOSKEY To compile this driver as a module, choose M here: the module will be called chaoskey. + +config USB_HUB_PWR + tristate "Control power supply for onboard USB hubs" + depends on PM + help + Say Y here if you want to control the power supply of an + onboard USB hub. The driver switches the power supply of the + hub on, to make sure the hub can be discovered. During system + suspend the power supply is switched off, unless a wakeup + capable device is connected to the hub. This may reduce power + consumption on battery powered devices. + + To compile this driver as a module, choose M here: the + module will be called usb_hub_pwr. diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile index da39bddb0604..2bd02388ca62 100644 --- a/drivers/usb/misc/Makefile +++ b/drivers/usb/misc/Makefile @@ -31,3 +31,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/ obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o +obj-$(CONFIG_USB_HUB_PWR) += usb_hub_pwr.o usb_hub_psupply.o diff --git a/drivers/usb/misc/usb_hub_psupply.c b/drivers/usb/misc/usb_hub_psupply.c new file mode 100644 index 000000000000..6a155ae1f831 --- /dev/null +++ b/drivers/usb/misc/usb_hub_psupply.c @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2020, Google LLC + */ + +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> + +struct usb_hub_psupply_dev { + struct regulator *vdd; +}; + +int usb_hub_psupply_on(struct device *dev) +{ + struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev); + int err; + + err = regulator_enable(usb_hub_psupply->vdd); + if (err) { + dev_err(dev, "failed to enable regulator: %d\n", err); + return err; + } + + return 0; + +} +EXPORT_SYMBOL_GPL(usb_hub_psupply_on); + +int usb_hub_psupply_off(struct device *dev) +{ + struct usb_hub_psupply_dev *usb_hub_psupply = dev_get_drvdata(dev); + int err; + + err = regulator_disable(usb_hub_psupply->vdd); + if (err) { + dev_err(dev, "failed to enable regulator: %d\n", err); + return err; + } + + return 0; +} +EXPORT_SYMBOL_GPL(usb_hub_psupply_off); + +static int usb_hub_psupply_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct usb_hub_psupply_dev *usb_hub_psupply; + + usb_hub_psupply = devm_kzalloc(dev, sizeof(*usb_hub_psupply), GFP_KERNEL); + if (!usb_hub_psupply) + return -ENOMEM; + + dev_set_drvdata(dev, usb_hub_psupply); + + usb_hub_psupply->vdd = devm_regulator_get(dev, "vdd"); + if (IS_ERR(usb_hub_psupply->vdd)) + return PTR_ERR(usb_hub_psupply->vdd); + + return usb_hub_psupply_on(dev); +} + +static int usb_hub_psupply_remove(struct platform_device *pdev) +{ + return usb_hub_psupply_off(&pdev->dev); +} + +static int usb_hub_psupply_suspend(struct platform_device *pdev, pm_message_t msg) +{ + return usb_hub_psupply_off(&pdev->dev); +} + +static int usb_hub_psupply_resume(struct platform_device *pdev) +{ + return usb_hub_psupply_on(&pdev->dev); +} + +static const struct of_device_id usb_hub_psupply_match[] = { + { .compatible = "linux,usb_hub_psupply" }, + {} +}; +MODULE_DEVICE_TABLE(of, usb_hub_psupply_match); + +static struct platform_driver usb_hub_psupply_driver = { + .probe = usb_hub_psupply_probe, + .remove = usb_hub_psupply_remove, + .suspend = usb_hub_psupply_suspend, + .resume = usb_hub_psupply_resume, + .driver = { + .name = "usb-hub-psupply", + .of_match_table = usb_hub_psupply_match, + }, +}; + +static int __init usb_hub_psupply_init(void) +{ + return platform_driver_register(&usb_hub_psupply_driver); +} +device_initcall(usb_hub_psupply_init); + +static void __exit usb_hub_psupply_exit(void) +{ + platform_driver_unregister(&usb_hub_psupply_driver); +} +module_exit(usb_hub_psupply_exit); + +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); +MODULE_DESCRIPTION("USB Hub Power Supply"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/usb/misc/usb_hub_psupply.h b/drivers/usb/misc/usb_hub_psupply.h new file mode 100644 index 000000000000..284e88f45fcf --- /dev/null +++ b/drivers/usb/misc/usb_hub_psupply.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _USB_HUB_PSUPPLY_H +#define _USB_HUB_PSUPPLY_H + +int usb_hub_psupply_on(struct device *dev); +int usb_hub_psupply_off(struct device *dev); + +#endif /* _USB_HUB_PSUPPLY_H */ diff --git a/drivers/usb/misc/usb_hub_pwr.c b/drivers/usb/misc/usb_hub_pwr.c new file mode 100644 index 000000000000..33945ca4a8c0 --- /dev/null +++ b/drivers/usb/misc/usb_hub_pwr.c @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * USB hub power control + * + * Copyright (c) 2020, Google LLC + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/power_supply.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> +#include <linux/usb.h> +#include <linux/usb/hcd.h> +#include "../core/usb.h" +#include "usb_hub_psupply.h" + +#define VENDOR_ID_REALTEK 0x0bda + +struct usb_hub_pwr_dev { + struct regulator *vdd; + struct device *psupply_dev; + bool powered_off; +}; + +static struct device *usb_pwr_find_psupply_dev(struct device *dev) +{ + const phandle *ph; + struct device_node *np; + struct platform_device *pdev; + + ph = of_get_property(dev->of_node, "psupply", NULL); + if (!ph) { + dev_err(dev, "failed to read 'psupply' property\n"); + return ERR_PTR(-EINVAL); + } + + np = of_find_node_by_phandle(be32_to_cpu(*ph)); + if (!np) { + dev_err(dev, "failed find device node for power supply\n"); + return ERR_PTR(-EINVAL); + } + + pdev = of_find_device_by_node(np); + of_node_put(np); + if (!pdev) + return ERR_PTR(-EPROBE_DEFER); + + return &pdev->dev; +} + +static int usb_hub_pwr_probe(struct usb_device *udev) +{ + struct device *dev = &udev->dev; + struct usb_hub_pwr_dev *uhpw; + struct device *psupply_dev; + int err; + + /* ignore supported hubs without device tree node */ + if (!dev->of_node) + return -ENODEV; + + psupply_dev = usb_pwr_find_psupply_dev(dev); + if (IS_ERR(psupply_dev)) + return PTR_ERR(psupply_dev); + + err = usb_generic_driver_probe(udev); + if (err) { + put_device(psupply_dev); + return err; + } + + uhpw = devm_kzalloc(dev, sizeof(*uhpw), GFP_KERNEL); + if (!uhpw) { + put_device(psupply_dev); + return -ENOMEM; + } + + dev_set_drvdata(&udev->dev, uhpw); + + uhpw->psupply_dev = psupply_dev; + + err = usb_hub_psupply_on(psupply_dev); + if (err) { + dev_err(dev, "failed to enable regulator: %d\n", err); + put_device(psupply_dev); + return err; + } + + return 0; +} + +static void usb_hub_pwr_disconnect(struct usb_device *udev) +{ + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); + + usb_hub_psupply_off(uhpw->psupply_dev); + put_device(uhpw->psupply_dev); +} + +static int usb_hub_pwr_suspend(struct usb_device *udev, pm_message_t msg) +{ + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); + int err; + + err = usb_generic_driver_suspend(udev, msg); + if (err) + return err; + + if (!usb_wakeup_enabled_descendants(udev)) { + usb_port_disable(udev); + + err = usb_hub_psupply_off(uhpw->psupply_dev); + if (err) + return err; + + uhpw->powered_off = true; + } + + return 0; +} + +static int usb_hub_pwr_resume(struct usb_device *udev, pm_message_t msg) +{ + struct usb_hub_pwr_dev *uhpw = dev_get_drvdata(&udev->dev); + int err; + + if (uhpw->powered_off) { + err = usb_hub_psupply_on(uhpw->psupply_dev); + if (err) + return err; + + uhpw->powered_off = false; + } + + return usb_generic_driver_resume(udev, msg); +} + +static const struct usb_device_id hub_id_table[] = { + { .idVendor = VENDOR_ID_REALTEK, + .idProduct = 0x0411, /* RTS5411 USB 3.0 */ + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, + { .idVendor = VENDOR_ID_REALTEK, + .idProduct = 0x5411, /* RTS5411 USB 2.0 */ + .match_flags = USB_DEVICE_ID_MATCH_DEVICE }, + {}, +}; + +MODULE_DEVICE_TABLE(usb, hub_id_table); + +static struct usb_device_driver usb_hub_pwr_driver = { + + .name = "usb-hub-pwr", + .probe = usb_hub_pwr_probe, + .disconnect = usb_hub_pwr_disconnect, + .suspend = usb_hub_pwr_suspend, + .resume = usb_hub_pwr_resume, + .id_table = hub_id_table, +}; + +static int __init usb_hub_pwr_driver_init(void) +{ + return usb_register_device_driver(&usb_hub_pwr_driver, THIS_MODULE); +} + +static void __exit usb_hub_pwr_driver_exit(void) +{ + usb_deregister_device_driver(&usb_hub_pwr_driver); +} + +module_init(usb_hub_pwr_driver_init); +module_exit(usb_hub_pwr_driver_exit); + +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); +MODULE_DESCRIPTION("USB Hub Power Control"); +MODULE_LICENSE("GPL v2");