| Message ID | 20200914112716.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: usb: Add binding for onboard USB hubs | expand |
On Mon, Sep 14, 2020 at 11:27:49AM -0700, Matthias Kaehlcke wrote: > The main issue this driver addresses is that a USB hub needs to be > powered before it can be discovered. For onboard hubs this is often > solved by supplying the hub with an 'always-on' regulator, which is > kind of a hack. Some onboard hubs may require further initialization > steps, like changing the state of a GPIO or enabling a clock, which > requires further hacks. This driver creates a platform device > representing the hub which performs the necessary initialization. > Currently it only supports switching on a single regulator, support > for multiple regulators or other actions can be added as needed. > Different initialization sequences can be supported based on the > compatible string. > > Besides performing the initialization the driver can be configured > to power the hub off during system suspend. This can help to extend > battery life on battery powered devices, which have no requirements > to keep the hub powered during suspend. The driver can also be > configured to leave the hub powered when a wakeup capable USB device > is connected when suspending, and keeping it powered otherwise. > > Technically the driver consists of two drivers, the platform driver > described above and a very thin USB driver that subclasses the > generic hub driver. Actually it subclasses the generic usb device driver, not the hub driver. > The purpose of this driver is to provide the > platform driver with the USB devices corresponding to the hub(s) > (a hub controller may provide multiple 'logical' hubs, e.g. one > to support USB 2.0 and another for USB 3.x). > > 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> > --- > This is an evolution of '[RFC] USB: misc: Add usb_hub_pwr driver' > (https://lore.kernel.org/patchwork/patch/1299239/). > > Changes in v1: > - renamed the driver to 'onboard_usb_hub' > - single file for platform and USB driver > - USB hub devices register with the platform device > - the DT includes a phandle of the platform device > - the platform device now controls when power is turned off > - the USB driver became a very thin subclass of the generic hub > driver > - enabled autosuspend support See https://marc.info/?l=linux-usb&m=159914635920888&w=2 and the accompanying submissions. You'll probably want to include those updates in your driver. Alan Stern
Hi Peter, thanks for your comments! On Tue, Sep 15, 2020 at 02:55:06AM +0000, Peter Chen wrote: > On 20-09-14 11:27:49, Matthias Kaehlcke wrote: > > The main issue this driver addresses is that a USB hub needs to be > > powered before it can be discovered. For onboard hubs this is often > > solved by supplying the hub with an 'always-on' regulator, which is > > kind of a hack. Some onboard hubs may require further initialization > > steps, like changing the state of a GPIO or enabling a clock, which > > requires further hacks. This driver creates a platform device > > representing the hub which performs the necessary initialization. > > Currently it only supports switching on a single regulator, support > > for multiple regulators or other actions can be added as needed. > > Different initialization sequences can be supported based on the > > compatible string. > > > > Besides performing the initialization the driver can be configured > > to power the hub off during system suspend. This can help to extend > > battery life on battery powered devices, which have no requirements > > to keep the hub powered during suspend. The driver can also be > > configured to leave the hub powered when a wakeup capable USB device > > is connected when suspending, and keeping it powered otherwise. > > > > Technically the driver consists of two drivers, the platform driver > > described above and a very thin USB driver that subclasses the > > generic hub driver. The purpose of this driver is to provide the > > platform driver with the USB devices corresponding to the hub(s) > > (a hub controller may provide multiple 'logical' hubs, e.g. one > > to support USB 2.0 and another for USB 3.x). > > I agree with Alan, you may change this driver to apply for generic > onboard USB devices. I interpreted that Alan only corrected my terminology and didn't suggest to extend the driver to generic onboard devices. Actually I like that we now have a abstraction for a specific physical 'device', rather than the initial usb_hub_pwr/usb_hub_psupply split, which seemed a bit contrived (thanks Doug!). > > +static int onboard_hub_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct onboard_hub *hub; > > + > > + hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); > > + if (!hub) > > + return -ENOMEM; > > + > > + hub->vdd = devm_regulator_get(dev, "vdd"); > > + if (IS_ERR(hub->vdd)) > > + return PTR_ERR(hub->vdd); > > + > > + hub->dev = dev; > > + mutex_init(&hub->lock); > > + INIT_LIST_HEAD(&hub->udev_list); > > + > > + hub->cfg.power_off_in_suspend = of_property_read_bool(dev->of_node, "power-off-in-suspend"); > > + hub->cfg.wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source"); > > Do you really need these two properties? If the device (and its children > if existed) has wakeup enabled, you keep power in suspend, otherwise, > you could close it, any exceptions? That would work for my use case, but I'm not sure it's a universally good configuration. I don't have a specific USB device in mind, but you could have a device that shouldn't lose it's context during suspend or keep operating autonomously (e.g. a sensor with a large buffer collecting samples). Not sure if something like this exists in the real though. I'm not an expert, but it seems there are USB controllers with wakeup support which is always enabled. A board with such a controller then couldn't have a policy to power down the hub regardless of wakeup capable devices being connected. Thanks Matthias
> > > + hub->cfg.power_off_in_suspend = > of_property_read_bool(dev->of_node, "power-off-in-suspend"); > > > + hub->cfg.wakeup_source = of_property_read_bool(dev->of_node, > > > +"wakeup-source"); > > > > Do you really need these two properties? If the device (and its > > children if existed) has wakeup enabled, you keep power in suspend, > > otherwise, you could close it, any exceptions? > > That would work for my use case, but I'm not sure it's a universally good > configuration. > > I don't have a specific USB device in mind, but you could have a device that > shouldn't lose it's context during suspend or keep operating autonomously (e.g. > a sensor with a large buffer collecting samples). Not sure if something like this > exists in the real though. > > I'm not an expert, but it seems there are USB controllers with wakeup support > which is always enabled. A board with such a controller then couldn't have a > policy to power down the hub regardless of wakeup capable devices being > connected. > Whether or not it is a wakeup_source, it could get through its or its children's /sys/../power/wakeup value, you have already used usb_wakeup_enabled_descendants to know it. If the onboard HUB needs to reflect wakeup signal, it should not power off its regulator. For another property power-off-in-suspend, I think it is also a user option, but not a hardware feature. If (wakeup-source || ! power-off-in-suspend) power off; else keep power; Peter
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig index 6818ea689cd9..e941244e24e5 100644 --- a/drivers/usb/misc/Kconfig +++ b/drivers/usb/misc/Kconfig @@ -275,3 +275,18 @@ config USB_CHAOSKEY To compile this driver as a module, choose M here: the module will be called chaoskey. + +config USB_ONBOARD_HUB + tristate "Onboard USB hub support" + depends on OF + help + Say Y here if you want to support onboard USB hubs. The driver + powers supported hubs on and may perform other initialization + steps. + + The driver can also switch off the power of the hub during + system suspend if it is configured accordingly. This may + reduce power consumption while the system is suspended. + + To compile this driver as a module, choose M here: the + module will be called onboard_usb_hub. diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile index da39bddb0604..6f10a1c6f7e9 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_ONBOARD_HUB) += onboard_usb_hub.o diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c new file mode 100644 index 000000000000..e5a816d0b124 --- /dev/null +++ b/drivers/usb/misc/onboard_usb_hub.c @@ -0,0 +1,306 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Driver for onboard USB hubs + * + * Copyright (c) 2020, Google LLC + */ + +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/suspend.h> +#include <linux/usb.h> +#include <linux/usb/hcd.h> +#include "../core/usb.h" + +/************************** Platform driver **************************/ + +struct udev_node { + struct usb_device *udev; + struct list_head list; +}; + +struct onboard_hub { + struct regulator *vdd; + struct device *dev; + struct { + bool power_off_in_suspend; + bool wakeup_source; + } cfg; + struct list_head udev_list; + struct mutex lock; + bool has_wakeup_capable_descendants; +}; + +static int onboard_hub_power_on(struct onboard_hub *hub) +{ + int err; + + err = regulator_enable(hub->vdd); + if (err) { + dev_err(hub->dev, "failed to enable regulator: %d\n", err); + return err; + } + + return 0; +} + +static int onboard_hub_power_off(struct onboard_hub *hub) +{ + int err; + + err = regulator_disable(hub->vdd); + if (err) { + dev_err(hub->dev, "failed to enable regulator: %d\n", err); + return err; + } + + return 0; +} + +#ifdef CONFIG_PM + +static int onboard_hub_suspend(struct platform_device *pdev, pm_message_t msg) +{ + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); + int rc = 0; + + if (!hub->cfg.power_off_in_suspend) + return 0; + + hub->has_wakeup_capable_descendants = false; + + if (hub->cfg.wakeup_source) { + struct udev_node *node; + + mutex_lock(&hub->lock); + + list_for_each_entry(node, &hub->udev_list, list) { + if (usb_wakeup_enabled_descendants(node->udev)) { + hub->has_wakeup_capable_descendants = true; + break; + } + + mutex_unlock(&hub->lock); + } + + if (!hub->has_wakeup_capable_descendants) + rc = onboard_hub_power_off(hub); + + return rc; +} + +static int onboard_hub_resume(struct platform_device *pdev) +{ + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); + int rc = 0; + + if (hub->cfg.power_off_in_suspend && !hub->has_wakeup_capable_descendants) + rc = onboard_hub_power_on(hub); + + return rc; +} + +#endif + +static int onboard_hub_add_usbdev(struct onboard_hub *hub, struct usb_device *udev) +{ + struct udev_node *node; + + node = devm_kzalloc(hub->dev, sizeof(*node), GFP_KERNEL); + if (!node) + return -ENOMEM; + + node->udev = udev; + + mutex_lock(&hub->lock); + list_add(&node->list, &hub->udev_list); + mutex_unlock(&hub->lock); + + return 0; +} + +static int onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev) +{ + struct udev_node *node; + + mutex_lock(&hub->lock); + + list_for_each_entry(node, &hub->udev_list, list) { + if (node->udev == udev) { + list_del(&node->list); + devm_kfree(hub->dev, node); + break; + } + } + + mutex_unlock(&hub->lock); + + if (node == NULL) + return -EINVAL; + + return 0; +} + +static int onboard_hub_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct onboard_hub *hub; + + hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); + if (!hub) + return -ENOMEM; + + hub->vdd = devm_regulator_get(dev, "vdd"); + if (IS_ERR(hub->vdd)) + return PTR_ERR(hub->vdd); + + hub->dev = dev; + mutex_init(&hub->lock); + INIT_LIST_HEAD(&hub->udev_list); + + hub->cfg.power_off_in_suspend = of_property_read_bool(dev->of_node, "power-off-in-suspend"); + hub->cfg.wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source"); + + dev_set_drvdata(dev, hub); + + return onboard_hub_power_on(hub); +} + +static int onboard_hub_remove(struct platform_device *pdev) +{ + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); + + return onboard_hub_power_off(hub); +} + +static const struct of_device_id onboard_hub_match[] = { + { .compatible = "onboard-usb-hub" }, + { .compatible = "realtek,rts5411" }, + {} +}; +MODULE_DEVICE_TABLE(of, onboard_hub_match); + +static struct platform_driver onboard_hub_driver = { + .probe = onboard_hub_probe, + .remove = onboard_hub_remove, +#ifdef CONFIG_PM + .suspend = onboard_hub_suspend, + .resume = onboard_hub_resume, +#endif + .driver = { + .name = "onboard-usb-hub", + .of_match_table = onboard_hub_match, + }, +}; + +/************************** USB driver **************************/ + +#define VENDOR_ID_REALTEK 0x0bda + +static struct onboard_hub *_find_onboard_hub(struct device *dev) +{ + const phandle *ph; + struct device_node *np; + struct platform_device *pdev; + + ph = of_get_property(dev->of_node, "hub", NULL); + if (!ph) { + dev_err(dev, "failed to read 'hub' 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 onboard hub\n"); + return ERR_PTR(-EINVAL); + } + + pdev = of_find_device_by_node(np); + of_node_put(np); + if (!pdev) + return ERR_PTR(-EPROBE_DEFER); + + return dev_get_drvdata(&pdev->dev); +} + +static int onboard_hub_usbdev_probe(struct usb_device *udev) +{ + struct device *dev = &udev->dev; + struct onboard_hub *hub; + + /* ignore supported hubs without device tree node */ + if (!dev->of_node) + return -ENODEV; + + hub = _find_onboard_hub(dev); + if (IS_ERR(hub)) + return PTR_ERR(dev); + + dev_set_drvdata(dev, hub); + + onboard_hub_add_usbdev(hub, udev); + + return 0; +} + +static void onboard_hub_usbdev_disconnect(struct usb_device *udev) +{ + struct onboard_hub *hub = dev_get_drvdata(&udev->dev); + + onboard_hub_remove_usbdev(hub, udev); + + put_device(hub->dev); +} + +static const struct usb_device_id onboard_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, onboard_hub_id_table); + +static struct usb_device_driver onboard_hub_usbdev_driver = { + + .name = "onboard-usb-hub", + .probe = onboard_hub_usbdev_probe, + .disconnect = onboard_hub_usbdev_disconnect, + .generic_subclass = 1, + .supports_autosuspend = 1, + .id_table = onboard_hub_id_table, +}; + +/************************** Driver (de)registration **************************/ + +static int __init onboard_hub_init(void) +{ + int rc; + + rc = platform_driver_register(&onboard_hub_driver); + if (rc) + return rc; + + return usb_register_device_driver(&onboard_hub_usbdev_driver, THIS_MODULE); +} +device_initcall(onboard_hub_init); + +static void __exit onboard_hub_exit(void) +{ + usb_deregister_device_driver(&onboard_hub_usbdev_driver); + platform_driver_unregister(&onboard_hub_driver); +} +module_exit(onboard_hub_exit); + +MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>"); +MODULE_DESCRIPTION("Onboard USB Hub driver"); +MODULE_LICENSE("GPL v2");