[PATCH 1/2] dt-bindings: usb: Add binding for onboard USB hubs

[PATCH 1/2] dt-bindings: usb: Add binding for onboard USB hubs

[Matthias Kaehlcke]

Onboard USB hubs need to be powered and may require initiaization of
other resources (like GPIOs or clocks) to work properly. This adds
a device tree binding for these hubs.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---

 .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml

diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
new file mode 100644
index 000000000000..f82d8f459eed
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binding for onboard USB hubs
+
+maintainers:
+  - Matthias Kaehlcke <mka@chromium.org>
+
+allOf:
+  - $ref: /schemas/usb/onboard_usb_hub.yaml#
+
+properties:
+  compatible:
+    enum:
+      - onboard-usb-hub
+      - realtek,rts5411
+
+  power-off-in-suspend:
+    description:
+      The hub should be powered off during system suspend. When the
+      "wakeup-source" property is also provided the hub is only powered
+      off during suspend when no wakeup capable descendants are connected.
+    type: boolean
+
+  vdd-supply:
+    description:
+      phandle to the regulator that provides power to the hub.
+
+  wakeup-source:
+    description:
+      Wakeup capable USB devices connected to this hub can be used as
+      wakeup source.
+    type: boolean
+
+required:
+  - compatible
+  - vdd-supply
+
+examples:
+  - |
+    usb_hub: usb-hub {
+        compatible = "realtek,rts5411", "onboard-usb-hub";
+        vdd-supply = <&pp3300_hub>;
+        power-off-in-suspend;
+        wakeup-source;
+    };
+
+    &usb_1_dwc3 {
+	dr_mode = "host";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.0 hub on port 1 */
+	hub@1 {
+		compatible = "usbbda,5411";
+		reg = <1>;
+		hub = <&usb_hub>;
+	};
+
+	/* 3.0 hub on port 2 */
+	hub@2 {
+		compatible = "usbbda,411";
+		reg = <2>;
+		hub = <&usb_hub>;
+	};
+
+...
-- 
2.28.0.618.gf4bc123cb7-goog

[PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

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).

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

 drivers/usb/misc/Kconfig           |  15 ++
 drivers/usb/misc/Makefile          |   1 +
 drivers/usb/misc/onboard_usb_hub.c | 306 +++++++++++++++++++++++++++++
 3 files changed, 322 insertions(+)
 create mode 100644 drivers/usb/misc/onboard_usb_hub.c

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");
-- 
2.28.0.618.gf4bc123cb7-goog

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

Hi,

I just noticed that building this results in a compilation error, due
to a missing brace. I tested this patch, but with another patch on top
with debug logs, the other patch adds the brace.

I'll still hold off a bit before sending v2, for if others have
comments.

On Mon, Sep 14, 2020 at 11:27:49AM -0700, Matthias Kaehlcke wrote:

> 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
>
> +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;

missing brace here:	}

> +		}
> +
> +		mutex_unlock(&hub->lock);
> +	}
> +
> +	if (!hub->has_wakeup_capable_descendants)
> +		rc = onboard_hub_power_off(hub);
> +
> +	return rc;
> +}

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Alan Stern]

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

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

Hi Alan,

thanks for your feedback!

On Mon, Sep 14, 2020 at 04:14:03PM -0400, Alan Stern wrote:
> 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.

ok, I'll change it in the next version.

> >  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.

Thanks for the pointer! I'll change the driver to use pm_ptr as suggested.

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Peter Chen]

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.

> +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?

Peter


> +
> +	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");
> -- 
> 2.28.0.618.gf4bc123cb7-goog
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

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

RE: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Peter Chen]

  
> > > +	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

Re: [PATCH 1/2] dt-bindings: usb: Add binding for onboard USB hubs

[Rob Herring]

On Mon, Sep 14, 2020 at 11:27:48AM -0700, Matthias Kaehlcke wrote:
> Onboard USB hubs need to be powered and may require initiaization of
> other resources (like GPIOs or clocks) to work properly. This adds
> a device tree binding for these hubs.

We already have bindings for these. 2 in fact as I2C controlled hubs are 
often described under the I2C bus.

> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
>  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> new file mode 100644
> index 000000000000..f82d8f459eed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for onboard USB hubs
> +
> +maintainers:
> +  - Matthias Kaehlcke <mka@chromium.org>
> +
> +allOf:
> +  - $ref: /schemas/usb/onboard_usb_hub.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - onboard-usb-hub
> +      - realtek,rts5411
> +
> +  power-off-in-suspend:
> +    description:
> +      The hub should be powered off during system suspend. When the
> +      "wakeup-source" property is also provided the hub is only powered
> +      off during suspend when no wakeup capable descendants are connected.
> +    type: boolean
> +
> +  vdd-supply:
> +    description:
> +      phandle to the regulator that provides power to the hub.
> +
> +  wakeup-source:
> +    description:
> +      Wakeup capable USB devices connected to this hub can be used as
> +      wakeup source.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - vdd-supply
> +
> +examples:
> +  - |
> +    usb_hub: usb-hub {
> +        compatible = "realtek,rts5411", "onboard-usb-hub";
> +        vdd-supply = <&pp3300_hub>;
> +        power-off-in-suspend;
> +        wakeup-source;

This is the hub device?

> +    };
> +
> +    &usb_1_dwc3 {
> +	dr_mode = "host";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	/* 2.0 hub on port 1 */
> +	hub@1 {
> +		compatible = "usbbda,5411";
> +		reg = <1>;
> +		hub = <&usb_hub>;

Or this node is?

> +	};
> +
> +	/* 3.0 hub on port 2 */
> +	hub@2 {
> +		compatible = "usbbda,411";
> +		reg = <2>;
> +		hub = <&usb_hub>;

Or this node is?

The hub node belongs here.

If you really have it connected to 2 upstream ports, then just do 
one node with 'reg = <1 2>;'.

Rob

> +	};
> +
> +...
> -- 
> 2.28.0.618.gf4bc123cb7-goog
> 

Re: [PATCH 1/2] dt-bindings: usb: Add binding for onboard USB hubs

[Rob Herring]

On Mon, 14 Sep 2020 11:27:48 -0700, Matthias Kaehlcke wrote:
> Onboard USB hubs need to be powered and may require initiaization of
> other resources (like GPIOs or clocks) to work properly. This adds
> a device tree binding for these hubs.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
>  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 773, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 848, in _ruamel_yaml.CParser._compose_sequence_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning a block scalar
  in "<unicode string>", line 43, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 52, column 1
make[1]: *** [Documentation/devicetree/bindings/Makefile:18: Documentation/devicetree/bindings/usb/onboard_usb_hub.example.dts] Error 1
make[1]: *** Deleting file 'Documentation/devicetree/bindings/usb/onboard_usb_hub.example.dts'
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml:  while scanning a block scalar
  in "<unicode string>", line 43, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 52, column 1
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
make: *** [Makefile:1366: dt_binding_check] Error 2


See https://patchwork.ozlabs.org/patch/1363875

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

Hi Peter,

On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote:
>   
> > > > +	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.

I conceptually agree, but in practice there are some conflicting details:

wakeup for the hubs on my system is by default disabled, yet USB wakeup works
regardless, so the flag doesn't really provide useful information. I guess we
could still use it if there is no better way, but it doesn't seem ideal.

Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup
support. Please let me know if there is a reliable way to check if wakeup is
enabled on the controller of a device.

> 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.

Ok, I think you are suggesting a sysfs attribute instead of a DT property, that
sounds good to me.

Re: [PATCH 1/2] dt-bindings: usb: Add binding for onboard USB hubs

[Matthias Kaehlcke]

Hi Rob,

On Tue, Sep 15, 2020 at 08:21:45AM -0600, Rob Herring wrote:
> On Mon, Sep 14, 2020 at 11:27:48AM -0700, Matthias Kaehlcke wrote:
> > Onboard USB hubs need to be powered and may require initiaization of
> > other resources (like GPIOs or clocks) to work properly. This adds
> > a device tree binding for these hubs.
> 
> We already have bindings for these. 2 in fact as I2C controlled hubs are 
> often described under the I2C bus.

Yes, these are I2C controlled hubs, which need hub specific drivers. This
driver is for hubs without an additional bus that share similar
initialization requirements and can benefit from common functionality.

> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > 
> >  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > new file mode 100644
> > index 000000000000..f82d8f459eed
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Binding for onboard USB hubs
> > +
> > +maintainers:
> > +  - Matthias Kaehlcke <mka@chromium.org>
> > +
> > +allOf:
> > +  - $ref: /schemas/usb/onboard_usb_hub.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - onboard-usb-hub
> > +      - realtek,rts5411
> > +
> > +  power-off-in-suspend:
> > +    description:
> > +      The hub should be powered off during system suspend. When the
> > +      "wakeup-source" property is also provided the hub is only powered
> > +      off during suspend when no wakeup capable descendants are connected.
> > +    type: boolean
> > +
> > +  vdd-supply:
> > +    description:
> > +      phandle to the regulator that provides power to the hub.
> > +
> > +  wakeup-source:
> > +    description:
> > +      Wakeup capable USB devices connected to this hub can be used as
> > +      wakeup source.
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - vdd-supply
> > +
> > +examples:
> > +  - |
> > +    usb_hub: usb-hub {
> > +        compatible = "realtek,rts5411", "onboard-usb-hub";
> > +        vdd-supply = <&pp3300_hub>;
> > +        power-off-in-suspend;
> > +        wakeup-source;
> 
> This is the hub device?

This is the physical hub device on the platform bus, which is the
equivalent to this entry for a usb2512b hub on an I2C bus:

    usb2512b@2c {
        compatible = "microchip,usb2512b";
	reg = <0x2c>;
	reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
    };

(source: Documentation/devicetree/bindings/usb/usb251xb.txt)

It doesn't have an I2C, SPI or other bus, hence the platform bus is
used.

> > +    };
> > +
> > +    &usb_1_dwc3 {
> > +	dr_mode = "host";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	/* 2.0 hub on port 1 */
> > +	hub@1 {
> > +		compatible = "usbbda,5411";
> > +		reg = <1>;
> > +		hub = <&usb_hub>;
> 
> Or this node is?

It is the USB 2.0 part of the hub. The device is instantiated by
Linux even without this node, but the system associates the node
with the device, which suggests it 'exists'.

The usb2512b mentioned above implicitly also has a node here, it just
isn't specified since the USB controller autodetects it.

> > +	};
> > +
> > +	/* 3.0 hub on port 2 */
> > +	hub@2 {
> > +		compatible = "usbbda,411";
> > +		reg = <2>;
> > +		hub = <&usb_hub>;
> 
> Or this node is?

It is the USB 3.0 part of the hub.

> The hub node belongs here.

The platform device isn't probed when the node is inside the USB
controller node. I haven't investigated why that's the case.

> If you really have it connected to 2 upstream ports, then just do
> one node with 'reg = <1 2>;'.

Yes, it is connected to two upstream ports. The platform driver needs a
reference to both/all hubs, to be able to determine whether to keep the
hub powered during system suspend or not.

Technically the hub with product id 0x5411 is connected to port 1 and the
one with product id 0x411 to port 2, so I would say the above is more
accurate than pretending one of the hubs is connected to both ports.

I would argue that the two hub nodes are similar to a SDIO BT/WiFi combo,
where you have one chip/module with multiple functions. The DT has entries
for both functions, even though they reside in the same chip and share the
same bus.

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Alan Stern]

On Tue, Sep 15, 2020 at 04:03:45PM -0700, Matthias Kaehlcke wrote:
> Hi Peter,
> 
> On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote:

> > 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.
> 
> I conceptually agree, but in practice there are some conflicting details:
> 
> wakeup for the hubs on my system is by default disabled, yet USB wakeup works
> regardless, so the flag doesn't really provide useful information. I guess we
> could still use it if there is no better way, but it doesn't seem ideal.

The wakeup setting for USB hubs affects only the following events: port 
connect, port disconnect, and port overcurrent.  It does not refer to 
forwarding wakeup requests from downstream USB devices; that is always 
enabled.  So maybe your wakeup flag really is accurate and you didn't 
realize it.

> Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup
> support. Please let me know if there is a reliable way to check if wakeup is
> enabled on the controller of a device.

The host controller's sysfs wakeup setting should always be correct.  If 
it isn't, that indicates there is a bug in the host controller driver or 
the corresponding platform-specific code.  What driver does your system 
use?

Alan Stern

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Peter Chen]

On 20-09-15 16:03:45, Matthias Kaehlcke wrote:
> Hi Peter,
> 
> On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote:
> >   
> > > > > +	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.
> 
> I conceptually agree, but in practice there are some conflicting details:
> 
> wakeup for the hubs on my system is by default disabled, yet USB wakeup works
> regardless, so the flag doesn't really provide useful information. I guess we
> could still use it if there is no better way, but it doesn't seem ideal.
> 
> Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup
> support. Please let me know if there is a reliable way to check if wakeup is
> enabled on the controller of a device.

Then, how could your code work, you use usb_wakeup_enabled_descendants
to get if HUB or the descendants under the HUB has wakeup enabled?

If you use dwc3, you need to enable xhci-plat.c's wakeup entry if your
system needs xHCI connect/disconnect wakeup event. I have one pending
patch to do it:

https://www.spinics.net/lists/linux-usb/msg199406.html

> 
> > 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.
> 
> Ok, I think you are suggesting a sysfs attribute instead of a DT property, that
> sounds good to me.

Yes.

-- 

Thanks,
Peter Chen

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

Hi Peter,

On Wed, Sep 16, 2020 at 08:19:07AM +0000, Peter Chen wrote:
> On 20-09-15 16:03:45, Matthias Kaehlcke wrote:
> > Hi Peter,
> > 
> > On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote:
> > >   
> > > > > > +	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.
> > 
> > I conceptually agree, but in practice there are some conflicting details:
> > 
> > wakeup for the hubs on my system is by default disabled, yet USB wakeup works
> > regardless, so the flag doesn't really provide useful information. I guess we
> > could still use it if there is no better way, but it doesn't seem ideal.
> > 
> > Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup
> > support. Please let me know if there is a reliable way to check if wakeup is
> > enabled on the controller of a device.
> 
> Then, how could your code work, you use usb_wakeup_enabled_descendants
> to get if HUB or the descendants under the HUB has wakeup enabled?

Doing just that would not allow to switch the hub off when wakeup enabled
descendants are connected, which might be desirable in some configurations.

> If you use dwc3, you need to enable xhci-plat.c's wakeup entry if your
> system needs xHCI connect/disconnect wakeup event. I have one pending
> patch to do it:
> 
> https://www.spinics.net/lists/linux-usb/msg199406.html

Thanks, my system has indeed a dwc3(-qcom) controller, your patch adds
the missing wakeup entry to sysfs. So it seems your patch should solve
my problem (sharp timing!), however you mention specifically the 'xHCI
connect/disconnect wakeup event', so I wonder if the xHCI wakeup flag
isn't applicable to other wakeup events. I know the dwc3-qcom platform
device has its own wakeup flag. The driver currently enables wakeup
interrupts unconditionally, I sent a patch to change that
(https://lore.kernel.org/patchwork/patch/1305894/), however I now wonder
if it should evaluate the xHCI wakeup flag instead of its own.

Thanks

Matthias

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

On Tue, Sep 15, 2020 at 10:14:21PM -0400, Alan Stern wrote:
> On Tue, Sep 15, 2020 at 04:03:45PM -0700, Matthias Kaehlcke wrote:
> > Hi Peter,
> > 
> > On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote:
> 
> > > 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.
> > 
> > I conceptually agree, but in practice there are some conflicting details:
> > 
> > wakeup for the hubs on my system is by default disabled, yet USB wakeup works
> > regardless, so the flag doesn't really provide useful information. I guess we
> > could still use it if there is no better way, but it doesn't seem ideal.
> 
> The wakeup setting for USB hubs affects only the following events: port 
> connect, port disconnect, and port overcurrent.  It does not refer to 
> forwarding wakeup requests from downstream USB devices; that is always 
> enabled.  So maybe your wakeup flag really is accurate and you didn't 
> realize it.

Thanks for the clarification!

> > Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup
> > support. Please let me know if there is a reliable way to check if wakeup is
> > enabled on the controller of a device.
> 
> The host controller's sysfs wakeup setting should always be correct.  If 
> it isn't, that indicates there is a bug in the host controller driver or 
> the corresponding platform-specific code.

Good to know :)

> What driver does your system use?

The driver is dwc3-qcom, Peter pointed me to a patch he recently sent to add
the missing wakeup entry (https://patchwork.kernel.org/patch/11717835/). It
seems that should solve the problem, except for some confusion on my side
about the wakeup flag of the xHCI device vs. that of the platform device
(details in my reply to Peter).

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Peter Chen]

On 20-09-16 12:16:07, Matthias Kaehlcke wrote:
> Hi Peter,
> 
> On Wed, Sep 16, 2020 at 08:19:07AM +0000, Peter Chen wrote:
> > On 20-09-15 16:03:45, Matthias Kaehlcke wrote:
> > > Hi Peter,
> > > 
> > > On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote:
> > > >   
> > > > > > > +	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.
> > > 
> > > I conceptually agree, but in practice there are some conflicting details:
> > > 
> > > wakeup for the hubs on my system is by default disabled, yet USB wakeup works
> > > regardless, so the flag doesn't really provide useful information. I guess we
> > > could still use it if there is no better way, but it doesn't seem ideal.
> > > 
> > > Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup
> > > support. Please let me know if there is a reliable way to check if wakeup is
> > > enabled on the controller of a device.
> > 
> > Then, how could your code work, you use usb_wakeup_enabled_descendants
> > to get if HUB or the descendants under the HUB has wakeup enabled?
> 
> Doing just that would not allow to switch the hub off when wakeup enabled
> descendants are connected, which might be desirable in some configurations.
> 
> > If you use dwc3, you need to enable xhci-plat.c's wakeup entry if your
> > system needs xHCI connect/disconnect wakeup event. I have one pending
> > patch to do it:
> > 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-usb%2Fmsg199406.html&data=02%7C01%7Cpeter.chen%40nxp.com%7C02c4cc75e26a47d0224d08d85a74f945%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637358805725394858&sdata=cjZhSmQiXVJoLsN5PjFACsLwsikH%2BeRTztPhsckJFNs%3D&reserved=0
> 
> Thanks, my system has indeed a dwc3(-qcom) controller, your patch adds
> the missing wakeup entry to sysfs. So it seems your patch should solve
> my problem (sharp timing!), however you mention specifically the 'xHCI
> connect/disconnect wakeup event', so I wonder if the xHCI wakeup flag
> isn't applicable to other wakeup events. I know the dwc3-qcom platform
> device has its own wakeup flag. The driver currently enables wakeup
> interrupts unconditionally, I sent a patch to change that
> (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1305894%2F&data=02%7C01%7Cpeter.chen%40nxp.com%7C02c4cc75e26a47d0224d08d85a74f945%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637358805725394858&sdata=6IjiiHJql%2FW4vzDla9q3qdfiiOzOQy1Vk7ryUhKOOTc%3D&reserved=0), however I now wonder
> if it should evaluate the xHCI wakeup flag instead of its own.
> 

You may need both (glue & xhci), it depends on system design, and
usually, these two kinds of wakeup setting isn't conflict.

-- 

Thanks,
Peter Chen

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

On Thu, Sep 17, 2020 at 12:27:29AM +0000, Peter Chen wrote:
> On 20-09-16 12:16:07, Matthias Kaehlcke wrote:
> > Hi Peter,
> > 
> > On Wed, Sep 16, 2020 at 08:19:07AM +0000, Peter Chen wrote:
> > > On 20-09-15 16:03:45, Matthias Kaehlcke wrote:
> > > > Hi Peter,
> > > > 
> > > > On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote:
> > > > >   
> > > > > > > > +	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.
> > > > 
> > > > I conceptually agree, but in practice there are some conflicting details:
> > > > 
> > > > wakeup for the hubs on my system is by default disabled, yet USB wakeup works
> > > > regardless, so the flag doesn't really provide useful information. I guess we
> > > > could still use it if there is no better way, but it doesn't seem ideal.
> > > > 
> > > > Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup
> > > > support. Please let me know if there is a reliable way to check if wakeup is
> > > > enabled on the controller of a device.
> > > 
> > > Then, how could your code work, you use usb_wakeup_enabled_descendants
> > > to get if HUB or the descendants under the HUB has wakeup enabled?
> > 
> > Doing just that would not allow to switch the hub off when wakeup enabled
> > descendants are connected, which might be desirable in some configurations.
> > 
> > > If you use dwc3, you need to enable xhci-plat.c's wakeup entry if your
> > > system needs xHCI connect/disconnect wakeup event. I have one pending
> > > patch to do it:
> > > 
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-usb%2Fmsg199406.html&data=02%7C01%7Cpeter.chen%40nxp.com%7C02c4cc75e26a47d0224d08d85a74f945%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637358805725394858&sdata=cjZhSmQiXVJoLsN5PjFACsLwsikH%2BeRTztPhsckJFNs%3D&reserved=0
> > 
> > Thanks, my system has indeed a dwc3(-qcom) controller, your patch adds
> > the missing wakeup entry to sysfs. So it seems your patch should solve
> > my problem (sharp timing!), however you mention specifically the 'xHCI
> > connect/disconnect wakeup event', so I wonder if the xHCI wakeup flag
> > isn't applicable to other wakeup events. I know the dwc3-qcom platform
> > device has its own wakeup flag. The driver currently enables wakeup
> > interrupts unconditionally, I sent a patch to change that
> > (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1305894%2F&data=02%7C01%7Cpeter.chen%40nxp.com%7C02c4cc75e26a47d0224d08d85a74f945%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637358805725394858&sdata=6IjiiHJql%2FW4vzDla9q3qdfiiOzOQy1Vk7ryUhKOOTc%3D&reserved=0), however I now wonder
> > if it should evaluate the xHCI wakeup flag instead of its own.
> > 
> 
> You may need both (glue & xhci), it depends on system design, and
> usually, these two kinds of wakeup setting isn't conflict.

Ok, thanks. So if I understand correctly the onboard hub driver should
check the wakeup state of the xHCI to determine if remote wakeup is
enabled for the controller (after all it doesn't know anything about
the platform device). Wakeup might not work properly if it is disabled
for the platform device, but it's the responsability of the board
software/config to make sure it is enabled (possibly this could be done
by making the dwc3-qcom driver understand the 'wakeup-source' property,
as the xhci-mtk driver does).

RE: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Peter Chen]

> > >
> >
> > You may need both (glue & xhci), it depends on system design, and
> > usually, these two kinds of wakeup setting isn't conflict.
> 
> Ok, thanks. So if I understand correctly the onboard hub driver should check the
> wakeup state of the xHCI to determine if remote wakeup is enabled for the
> controller (after all it doesn't know anything about the platform device).
> Wakeup might not work properly if it is disabled for the platform device, but it's
> the responsability of the board software/config to make sure it is enabled
> (possibly this could be done by making the dwc3-qcom driver understand the
> 'wakeup-source' property, as the xhci-mtk driver does).

No, every level should handle its own wakeup setting. You may have to do this since the USB bus and platform bus
are two different buses, you should not visit device structure across the bus. And you don't need a device tree property
to do it. For platform driver, you could use device_may_wakeup, for onboard hub power driver, you could use
usb_wakeup_enabled_descendants since you need to cover descendants.

The purpose of these two wakeup logic is different, for USB bus, it is used to tell USB devices to enable remote wakeup
and do not power off its regulator; for platform bus, it is used to tell the controller to enable its wakeup setting and keep
the regulator for its USB controller and USB PHY (if needed).

Peter

Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Matthias Kaehlcke]

On Thu, Sep 17, 2020 at 01:24:30AM +0000, Peter Chen wrote:
> > > >
> > >
> > > You may need both (glue & xhci), it depends on system design, and
> > > usually, these two kinds of wakeup setting isn't conflict.
> > 
> > Ok, thanks. So if I understand correctly the onboard hub driver should check the
> > wakeup state of the xHCI to determine if remote wakeup is enabled for the
> > controller (after all it doesn't know anything about the platform device).
> > Wakeup might not work properly if it is disabled for the platform device, but it's
> > the responsability of the board software/config to make sure it is enabled
> > (possibly this could be done by making the dwc3-qcom driver understand the
> > 'wakeup-source' property, as the xhci-mtk driver does).
> 
> No, every level should handle its own wakeup setting. You may have to do this since the USB bus and platform bus
> are two different buses, you should not visit device structure across the bus. And you don't need a device tree property
> to do it. For platform driver, you could use device_may_wakeup, for onboard hub power driver, you could use
> usb_wakeup_enabled_descendants since you need to cover descendants.
> 
> The purpose of these two wakeup logic is different, for USB bus, it is used to tell USB devices to enable remote wakeup
> and do not power off its regulator; for platform bus, it is used to tell the controller to enable its wakeup setting and keep
> the regulator for its USB controller and USB PHY (if needed).

Sorry, I didn't express myself clearly. With the platform device I was
referring to the dwc3-qcom ('glue') in this case, which could check
it's own 'wakeup-source' attribute to enable wakeup at boot. The driver
currently enables wakeup statically. The onboard_hub driver is agnostic
of this device.

Re: [PATCH 1/2] dt-bindings: usb: Add binding for onboard USB hubs

[Rob Herring]

+Florian

On Tue, Sep 15, 2020 at 6:00 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Rob,
>
> On Tue, Sep 15, 2020 at 08:21:45AM -0600, Rob Herring wrote:
> > On Mon, Sep 14, 2020 at 11:27:48AM -0700, Matthias Kaehlcke wrote:
> > > Onboard USB hubs need to be powered and may require initiaization of
> > > other resources (like GPIOs or clocks) to work properly. This adds
> > > a device tree binding for these hubs.
> >
> > We already have bindings for these. 2 in fact as I2C controlled hubs are
> > often described under the I2C bus.
>
> Yes, these are I2C controlled hubs, which need hub specific drivers. This
> driver is for hubs without an additional bus that share similar
> initialization requirements and can benefit from common functionality.

Yes, as I said, there's already 2 ways to do this. The second is
defining the USB bus under the USB host. I'm sure there's some
examples in the tree.

> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >
> > >  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > new file mode 100644
> > > index 000000000000..f82d8f459eed
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > @@ -0,0 +1,70 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Binding for onboard USB hubs
> > > +
> > > +maintainers:
> > > +  - Matthias Kaehlcke <mka@chromium.org>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/usb/onboard_usb_hub.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - onboard-usb-hub
> > > +      - realtek,rts5411
> > > +
> > > +  power-off-in-suspend:
> > > +    description:
> > > +      The hub should be powered off during system suspend. When the
> > > +      "wakeup-source" property is also provided the hub is only powered
> > > +      off during suspend when no wakeup capable descendants are connected.
> > > +    type: boolean
> > > +
> > > +  vdd-supply:
> > > +    description:
> > > +      phandle to the regulator that provides power to the hub.
> > > +
> > > +  wakeup-source:
> > > +    description:
> > > +      Wakeup capable USB devices connected to this hub can be used as
> > > +      wakeup source.
> > > +    type: boolean
> > > +
> > > +required:
> > > +  - compatible
> > > +  - vdd-supply
> > > +
> > > +examples:
> > > +  - |
> > > +    usb_hub: usb-hub {
> > > +        compatible = "realtek,rts5411", "onboard-usb-hub";
> > > +        vdd-supply = <&pp3300_hub>;
> > > +        power-off-in-suspend;
> > > +        wakeup-source;
> >
> > This is the hub device?
>
> This is the physical hub device on the platform bus, which is the

How is a USB hub connected to the 'platform bus'? There's no such
thing as 'platform bus' in DT.

> equivalent to this entry for a usb2512b hub on an I2C bus:
>
>     usb2512b@2c {
>         compatible = "microchip,usb2512b";
>         reg = <0x2c>;
>         reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
>     };
>
> (source: Documentation/devicetree/bindings/usb/usb251xb.txt)
>
> It doesn't have an I2C, SPI or other bus, hence the platform bus is
> used.
>
> > > +    };
> > > +
> > > +    &usb_1_dwc3 {
> > > +   dr_mode = "host";
> > > +   #address-cells = <1>;
> > > +   #size-cells = <0>;
> > > +
> > > +   /* 2.0 hub on port 1 */
> > > +   hub@1 {
> > > +           compatible = "usbbda,5411";
> > > +           reg = <1>;
> > > +           hub = <&usb_hub>;
> >
> > Or this node is?
>
> It is the USB 2.0 part of the hub. The device is instantiated by
> Linux even without this node, but the system associates the node
> with the device, which suggests it 'exists'.
>
> The usb2512b mentioned above implicitly also has a node here, it just
> isn't specified since the USB controller autodetects it.

Like other probe-able buses, we describe the devices in DT when they
have extra resources/config which are not probe-able.

> > > +   };
> > > +
> > > +   /* 3.0 hub on port 2 */
> > > +   hub@2 {
> > > +           compatible = "usbbda,411";
> > > +           reg = <2>;
> > > +           hub = <&usb_hub>;
> >
> > Or this node is?
>
> It is the USB 3.0 part of the hub.
>
> > The hub node belongs here.
>
> The platform device isn't probed when the node is inside the USB
> controller node. I haven't investigated why that's the case.

It shouldn't be a platform device, but associating a device_node with
the usb device. I think at least that is there in the kernel.

Though if you need setup to happen before the device is probe-able
which appears to be the case here, then that is a common problem which
isn't really a solved issue. There's the mmc-pwrseq stuff, but I don't
want to see a repeat of that. There was an #armlinux irc discussion I
had with Florian on this just 2 days ago[1]. If the only thing you
have to configure is 'reset-gpios', then you probably can do that
generically scanning all the child USB DT devices and deassert reset.
But as soon as you have device specific things and need specific
ordering and timing, then you'll need device specific code to handle
it.

>
> > If you really have it connected to 2 upstream ports, then just do
> > one node with 'reg = <1 2>;'.
>
> Yes, it is connected to two upstream ports. The platform driver needs a
> reference to both/all hubs, to be able to determine whether to keep the
> hub powered during system suspend or not.
>
> Technically the hub with product id 0x5411 is connected to port 1 and the
> one with product id 0x411 to port 2, so I would say the above is more
> accurate than pretending one of the hubs is connected to both ports.
>
> I would argue that the two hub nodes are similar to a SDIO BT/WiFi combo,
> where you have one chip/module with multiple functions. The DT has entries
> for both functions, even though they reside in the same chip and share the
> same bus.

That has generally worked because little is shared between BT and WiFi
and what is shared is refcounted (clks for example). That could work
here, we just have to be careful.

Rob

[1] https://pastebin.com/iepyfe9c

Re: [PATCH 1/2] dt-bindings: usb: Add binding for onboard USB hubs

[Matthias Kaehlcke]

Hi Rob,

On Fri, Sep 18, 2020 at 10:05:21AM -0600, Rob Herring wrote:
> +Florian
> 
> On Tue, Sep 15, 2020 at 6:00 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > On Tue, Sep 15, 2020 at 08:21:45AM -0600, Rob Herring wrote:
> > > On Mon, Sep 14, 2020 at 11:27:48AM -0700, Matthias Kaehlcke wrote:
> > > > Onboard USB hubs need to be powered and may require initiaization of
> > > > other resources (like GPIOs or clocks) to work properly. This adds
> > > > a device tree binding for these hubs.
> > >
> > > We already have bindings for these. 2 in fact as I2C controlled hubs are
> > > often described under the I2C bus.
> >
> > Yes, these are I2C controlled hubs, which need hub specific drivers. This
> > driver is for hubs without an additional bus that share similar
> > initialization requirements and can benefit from common functionality.
> 
> Yes, as I said, there's already 2 ways to do this. The second is
> defining the USB bus under the USB host. I'm sure there's some
> examples in the tree.

I suppose you refer to something like the hub@1 and hub@2 nodes in the
example of this patch. This alone is not a suitable method in this case,
since the hub needs to be powered/initialized before it can be detected
by the USB bus and a driver could perform any initialization (a
chicken-egg problem)

> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > ---
> > > >
> > > >  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
> > > >  1 file changed, 70 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > > new file mode 100644
> > > > index 000000000000..f82d8f459eed
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > > @@ -0,0 +1,70 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Binding for onboard USB hubs
> > > > +
> > > > +maintainers:
> > > > +  - Matthias Kaehlcke <mka@chromium.org>
> > > > +
> > > > +allOf:
> > > > +  - $ref: /schemas/usb/onboard_usb_hub.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - onboard-usb-hub
> > > > +      - realtek,rts5411
> > > > +
> > > > +  power-off-in-suspend:
> > > > +    description:
> > > > +      The hub should be powered off during system suspend. When the
> > > > +      "wakeup-source" property is also provided the hub is only powered
> > > > +      off during suspend when no wakeup capable descendants are connected.
> > > > +    type: boolean
> > > > +
> > > > +  vdd-supply:
> > > > +    description:
> > > > +      phandle to the regulator that provides power to the hub.
> > > > +
> > > > +  wakeup-source:
> > > > +    description:
> > > > +      Wakeup capable USB devices connected to this hub can be used as
> > > > +      wakeup source.
> > > > +    type: boolean
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - vdd-supply
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    usb_hub: usb-hub {
> > > > +        compatible = "realtek,rts5411", "onboard-usb-hub";
> > > > +        vdd-supply = <&pp3300_hub>;
> > > > +        power-off-in-suspend;
> > > > +        wakeup-source;
> > >
> > > This is the hub device?
> >
> > This is the physical hub device on the platform bus, which is the
> 
> How is a USB hub connected to the 'platform bus'? There's no such
> thing as 'platform bus' in DT.

I suppose in DT lingo you would say the 'system bus'. Under Linux
such devices are often represented as platform devices, 'connected'
to the pseudo 'platform' bus. It is the physical chip that has pins
to supply power, assert a reset or receive an input clock.

> > equivalent to this entry for a usb2512b hub on an I2C bus:
> >
> >     usb2512b@2c {
> >         compatible = "microchip,usb2512b";
> >         reg = <0x2c>;
> >         reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
> >     };
> >
> > (source: Documentation/devicetree/bindings/usb/usb251xb.txt)
> >
> > It doesn't have an I2C, SPI or other bus, hence the platform bus is
> > used.
> >
> > > > +    };
> > > > +
> > > > +    &usb_1_dwc3 {
> > > > +   dr_mode = "host";
> > > > +   #address-cells = <1>;
> > > > +   #size-cells = <0>;
> > > > +
> > > > +   /* 2.0 hub on port 1 */
> > > > +   hub@1 {
> > > > +           compatible = "usbbda,5411";
> > > > +           reg = <1>;
> > > > +           hub = <&usb_hub>;
> > >
> > > Or this node is?
> >
> > It is the USB 2.0 part of the hub. The device is instantiated by
> > Linux even without this node, but the system associates the node
> > with the device, which suggests it 'exists'.
> >
> > The usb2512b mentioned above implicitly also has a node here, it just
> > isn't specified since the USB controller autodetects it.
> 
> Like other probe-able buses, we describe the devices in DT when they
> have extra resources/config which are not probe-able.

The problem is that the device is not probe-able without being
powered first.

> > > > +   };
> > > > +
> > > > +   /* 3.0 hub on port 2 */
> > > > +   hub@2 {
> > > > +           compatible = "usbbda,411";
> > > > +           reg = <2>;
> > > > +           hub = <&usb_hub>;
> > >
> > > Or this node is?
> >
> > It is the USB 3.0 part of the hub.
> >
> > > The hub node belongs here.
> >
> > The platform device isn't probed when the node is inside the USB
> > controller node. I haven't investigated why that's the case.
> 
> It shouldn't be a platform device, but associating a device_node with
> the usb device. I think at least that is there in the kernel.
> 
> Though if you need setup to happen before the device is probe-able
> which appears to be the case here, then that is a common problem which
> isn't really a solved issue. There's the mmc-pwrseq stuff, but I don't
> want to see a repeat of that. There was an #armlinux irc discussion I
> had with Florian on this just 2 days ago[1]. If the only thing you
> have to configure is 'reset-gpios', then you probably can do that
> generically scanning all the child USB DT devices and deassert reset.
> But as soon as you have device specific things and need specific
> ordering and timing, then you'll need device specific code to handle
> it.

Yeah, there have been a few attempts to solve this in the past,
unfortunately none of them was accepted, hopefully we can break this
pattern :)

For my specific use case only a single regulator needs to be turned on,
however that could be different for other hubs/configurations. So yes,
I think device specific code will be needed, but preferably not a
driver for every hub model/family.

Also the driver can optionally turn the power of the hub off during
system suspend, either always or only when no wakeup capable
descendants are connected, hence the driver needs to be aware of all
the USB hub devices that correspond to the hub chip.

> >
> > > If you really have it connected to 2 upstream ports, then just do
> > > one node with 'reg = <1 2>;'.
> >
> > Yes, it is connected to two upstream ports. The platform driver needs a
> > reference to both/all hubs, to be able to determine whether to keep the
> > hub powered during system suspend or not.
> >
> > Technically the hub with product id 0x5411 is connected to port 1 and the
> > one with product id 0x411 to port 2, so I would say the above is more
> > accurate than pretending one of the hubs is connected to both ports.
> >
> > I would argue that the two hub nodes are similar to a SDIO BT/WiFi combo,
> > where you have one chip/module with multiple functions. The DT has entries
> > for both functions, even though they reside in the same chip and share the
> > same bus.
> 
> That has generally worked because little is shared between BT and WiFi
> and what is shared is refcounted (clks for example). That could work
> here, we just have to be careful.

The USB portion of the driver is currently a very thin layer, it essentially
(un)registers the USB devices with the main driver (the platform device)
and that's it. I received some feedback related with refcounting on the driver
patch, which I will incorporate.

Thanks

Matthias