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 <[email protected]>
Signed-off-by: Ravi Chandra Sadineni <[email protected]>
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
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 <[email protected]>");
+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 <[email protected]>");
+MODULE_DESCRIPTION("USB Hub Power Control");
+MODULE_LICENSE("GPL v2");
--
2.28.0.402.g5ffc5be6b7-goog
Hi,
On Tue, Sep 1, 2020 at 1:21 PM Matthias Kaehlcke <[email protected]> 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 <[email protected]>
> Signed-off-by: Ravi Chandra Sadineni <[email protected]>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> 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 <[email protected]>");
> +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 <[email protected]>");
> +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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Ravi Chandra Sadineni <[email protected]>
> > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > ---
> > 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 <[email protected]>");
> > +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 <[email protected]>");
> > +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/[email protected]/
Peter
>
> Co-developed-by: Ravi Chandra Sadineni <[email protected]>
> Signed-off-by: Ravi Chandra Sadineni <[email protected]>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> 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 <[email protected]>");
> +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 <[email protected]>");
> +MODULE_DESCRIPTION("USB Hub Power Control");
> +MODULE_LICENSE("GPL v2");
> --
> 2.28.0.402.g5ffc5be6b7-goog
>
--
Thanks,
Peter Chen
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 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 ;-)
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.
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/[email protected]/
--
Thanks,
Peter Chen
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