[RFC PATCH 0/2] initialize (multiple) PHYs in xhci-plat

[RFC PATCH 0/2] initialize (multiple) PHYs in xhci-plat

[Martin Blumenstingl]

This series is the outcome of a discussion with Felipe Balbi,
see [0] and [1].
The quick-summary of this is:
- dwc3 already takes one USB2 and one USB3 PHY and initializes these
  correct
- some other HCI platform drivers (like ehci-platform.c, xhci-mtk.c and
  ohci-platform.c) do not have a limitation on the number of PHYs - they
  support one PHY per actual host port
- Amlogic Meson GXL and GXM SoCs come with a dwc3 IP block which has two
  or three USB2 ports enabled on the internal root-hub. The SoCs also
  provide separate USB2 PHYs, one per port. All USB2 PHYs (which are
  internally "connected" to the dwc3 roothub) need to be powered on,
  otherwise USB devices cannot be enumerated (even if just one PHY is
  disabled and if the device is plugged into another, enabled port)

In my first attempt to get USB supported on the GXL and GXM SoCs I tried
to work-around the problem that I could not pass multiple PHYs to the
dwc3 controller.
This was rejected by Rob Herring (which was definitely the thing to do in
my opinion), see [2]

This series adds a new "platform-roothub". This can be configured through
devicetree by passing a child-node with "reg = <0>" to the USB
controller. Additionally there has to be a child-node for each port on
the root-hub. Each of the child-nodes takes a "phys" and "phy-names"
property. This allows modeling the root-hub in devicetree similar to the
USB device binding (documented in devicetree/bindings/usb/usb-device.txt)
This avoids and backwards-compatibility problems (which was a concern
regardless of the solution, see [3]) since the binding for the root-hub
was previously not specified (and we're not using the "phys" property of
the controller, which might have served different purposes before,
depending on the drivers).

Additionally this integrates the new platform-roothub into xhci-plat.c
which automatically enables it for the dwc3 driver (in host-mode).


[0] http://lists.infradead.org/pipermail/linux-amlogic/2017-January/001945.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2017-January/001947.html
[2] http://lists.infradead.org/pipermail/linux-amlogic/2016-November/001818.html
[3] http://lists.infradead.org/pipermail/linux-amlogic/2017-January/001948.html

Martin Blumenstingl (2):
  usb: host: add a generic platform USB roothub driver
  usb: host: xhci: plat: integrate the platform-roothub

 .../devicetree/bindings/usb/usb-roothub.txt        |  41 ++++++
 Documentation/devicetree/bindings/usb/usb-xhci.txt |   7 +
 drivers/usb/host/Kconfig                           |   4 +
 drivers/usb/host/Makefile                          |   2 +
 drivers/usb/host/platform-roothub.c                | 146 +++++++++++++++++++++
 drivers/usb/host/platform-roothub.h                |  14 ++
 drivers/usb/host/xhci-plat.c                       |  37 +++++-
 drivers/usb/host/xhci.h                            |   3 +
 8 files changed, 251 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usb-roothub.txt
 create mode 100644 drivers/usb/host/platform-roothub.c
 create mode 100644 drivers/usb/host/platform-roothub.h

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/2] usb: host: add a generic platform USB roothub driver

[Martin Blumenstingl]

Many SoC platforms have separate devices for the USB PHY which are
registered through the generic PHY framework. These PHYs have to be
enabled to make the USB controller actually work. They also have to be
disabled again on shutdown/suspend.

Currently (at least) the following HCI platform drivers are using custom
code to obtain all PHYs via devicetree for the roothub/controller and
disable/enable them when required:
- ehci-platform.c has ehci_platform_power_{on,off}
- xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
- ohci-platform.c has ohci_platform_power_{on,off}

These drivers are not using the generic devicetree USB device bindings
yet which were only introduced recently (documentation is available in
devicetree/bindings/usb/usb-device.txt).
With this new driver the usb2-phy and usb3-phy can be specified directly
in the child-node of the corresponding port of the roothub via
devicetree. This can be extended by not just parsing PHYs (some of the
other drivers listed above are for example also parsing a list of clocks
as well) when required.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 .../devicetree/bindings/usb/usb-roothub.txt        |  41 ++++++
 drivers/usb/host/Kconfig                           |   3 +
 drivers/usb/host/Makefile                          |   2 +
 drivers/usb/host/platform-roothub.c                | 146 +++++++++++++++++++++
 drivers/usb/host/platform-roothub.h                |  14 ++
 5 files changed, 206 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb-roothub.txt
 create mode 100644 drivers/usb/host/platform-roothub.c
 create mode 100644 drivers/usb/host/platform-roothub.h

diff --git a/Documentation/devicetree/bindings/usb/usb-roothub.txt b/Documentation/devicetree/bindings/usb/usb-roothub.txt
new file mode 100644
index 000000000000..96e152d3901c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb-roothub.txt
@@ -0,0 +1,41 @@
+Generic USB root-hub Properties
+
+similar to the USB device bindings (documented in usb-device.txt from the
+current directory) this provides support for configuring the root-hub.
+
+Required properties:
+- compatible: should be at least one of "usb1d6b,3", "usb1d6b,2"
+- reg: must be 0.
+- address-cells: must be 1
+- size-cells: must be 0
+- a sub-node per port supports the following properties:
+  - reg: the port number on the root-hub (mandatory)
+  - phys: optional, from the *Generic PHY* bindings (mandatory needed when
+    phy-names is given)
+  - phy-names: optional, from the *Generic PHY* bindings; supported names
+    are "usb2-phy" or "usb3-phy"
+
+Example:
+	&usb1 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		roothub@0 {
+			compatible = "usb1d6b,3", "usb1d6b,2";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+
+			port@1 {
+				reg = <1>;
+				usb-phy = <&usb2_phy1>, <&usb3_phy1>;
+				phy-names = "usb2-phy", "usb3-phy";
+			};
+
+			port@2 {
+				reg = <2>;
+				usb-phy = <&usb2_phy2>, <&usb3_phy2>;
+				phy-names = "usb2-phy", "usb3-phy";
+			};
+		};
+	}
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 6361fc739306..eb7009460322 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -797,6 +797,9 @@ config USB_HCD_SSB
 
 	  If unsure, say N.
 
+config USB_PLATFORM_ROOTHUB
+	bool
+
 config USB_HCD_TEST_MODE
 	bool "HCD test mode support"
 	---help---
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 2644537b7bcf..1f1e19c4a826 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -31,6 +31,8 @@ ifneq ($(CONFIG_USB), )
 	obj-$(CONFIG_PCI)	+= pci-quirks.o
 endif
 
+obj-$(CONFIG_USB_PLATFORM_ROOTHUB)	+= platform-roothub.o
+
 obj-$(CONFIG_USB_EHCI_HCD)	+= ehci-hcd.o
 obj-$(CONFIG_USB_EHCI_PCI)	+= ehci-pci.o
 obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)	+= ehci-platform.o
diff --git a/drivers/usb/host/platform-roothub.c b/drivers/usb/host/platform-roothub.c
new file mode 100644
index 000000000000..84837e42b006
--- /dev/null
+++ b/drivers/usb/host/platform-roothub.c
@@ -0,0 +1,146 @@
+/*
+ * platform roothub driver - a virtual PHY device which passes all phy_*
+ * function calls to multiple (actual) PHY devices. This is comes handy when
+ * initializing all PHYs on a root-hub (to keep them all in the same state).
+ *
+ * Copyright (C) 2017 Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/phy/phy.h>
+#include <linux/of.h>
+#include <linux/usb/of.h>
+
+#include "platform-roothub.h"
+
+#define ROOTHUB_PORTNUM		0
+
+struct platform_roothub {
+	struct phy		*phy;
+	struct list_head	list;
+};
+
+static struct platform_roothub *platform_roothub_alloc(struct device *dev)
+{
+	struct platform_roothub *roothub_entry;
+
+	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
+	if (!roothub_entry)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&roothub_entry->list);
+
+	return roothub_entry;
+}
+
+static int platform_roothub_add_phy(struct device *dev,
+				    struct device_node *port_np,
+				    const char *con_id, struct list_head *list)
+{
+	struct platform_roothub *roothub_entry;
+	struct phy *phy = devm_of_phy_get(dev, port_np, con_id);
+
+	if (IS_ERR_OR_NULL(phy)) {
+		if (!phy || PTR_ERR(phy) == -ENODEV)
+			return 0;
+		else
+			return PTR_ERR(phy);
+	}
+
+	roothub_entry = platform_roothub_alloc(dev);
+	if (IS_ERR(roothub_entry))
+		return PTR_ERR(roothub_entry);
+
+	roothub_entry->phy = phy;
+
+	list_add_tail(&roothub_entry->list, list);
+
+	return 0;
+}
+
+struct platform_roothub *platform_roothub_init(struct device *dev)
+{
+	struct device_node *roothub_np, *port_np;
+	struct platform_roothub *plat_roothub;
+	int err;
+
+	roothub_np = usb_of_get_child_node(dev->of_node, ROOTHUB_PORTNUM);
+	if (!of_device_is_available(roothub_np))
+		return NULL;
+
+	plat_roothub = platform_roothub_alloc(dev);
+	if (IS_ERR(plat_roothub))
+		return plat_roothub;
+
+	for_each_available_child_of_node(roothub_np, port_np) {
+		err = platform_roothub_add_phy(dev, port_np, "usb2-phy",
+					       &plat_roothub->list);
+		if (err)
+			return ERR_PTR(err);
+
+		err = platform_roothub_add_phy(dev, port_np, "usb3-phy",
+					       &plat_roothub->list);
+		if (err)
+			return ERR_PTR(err);
+	}
+
+	return plat_roothub;
+}
+EXPORT_SYMBOL_GPL(platform_roothub_init);
+
+int platform_roothub_power_on(struct platform_roothub *plat_roothub)
+{
+	struct platform_roothub *roothub_entry;
+	struct list_head *head;
+	int err;
+
+	if (!plat_roothub)
+		return 0;
+
+	head = &plat_roothub->list;
+
+	list_for_each_entry(roothub_entry, head, list) {
+		err = phy_init(roothub_entry->phy);
+		if (err)
+			goto err_out;
+
+		err = phy_power_on(roothub_entry->phy);
+		if (err) {
+			phy_exit(roothub_entry->phy);
+			goto err_out;
+		}
+	}
+
+	return 0;
+
+err_out:
+	list_for_each_entry_continue_reverse(roothub_entry, head, list) {
+		phy_power_off(roothub_entry->phy);
+		phy_exit(roothub_entry->phy);
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(platform_roothub_power_on);
+
+void platform_roothub_power_off(struct platform_roothub *plat_roothub)
+{
+	struct platform_roothub *roothub_entry;
+
+	if (!plat_roothub)
+		return;
+
+	list_for_each_entry_reverse(roothub_entry, &plat_roothub->list, list) {
+		phy_power_off(roothub_entry->phy);
+		phy_exit(roothub_entry->phy);
+	}
+}
+EXPORT_SYMBOL_GPL(platform_roothub_power_off);
diff --git a/drivers/usb/host/platform-roothub.h b/drivers/usb/host/platform-roothub.h
new file mode 100644
index 000000000000..bde0bf299e3b
--- /dev/null
+++ b/drivers/usb/host/platform-roothub.h
@@ -0,0 +1,14 @@
+#ifndef USB_HOST_PLATFORM_ROOTHUB_H
+#define USB_HOST_PLATFORM_ROOTHUB_H
+
+struct phy;
+struct device_node;
+
+struct platform_roothub;
+
+struct platform_roothub *platform_roothub_init(struct device *dev);
+
+int platform_roothub_power_on(struct platform_roothub *plat_roothub);
+void platform_roothub_power_off(struct platform_roothub *plat_roothub);
+
+#endif /* USB_HOST_PLATFORM_ROOTHUB_H */
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 2/2] usb: host: xhci: plat: integrate the platform-roothub

[Martin Blumenstingl]

This enables the platform-roothub for the xhci-plat driver. This allows
specifying a PHY for each port via devicetree. All PHYs will then be
enabled/disabled by the platform-roothub driver.

One example where this is required is the Amlogic GXL and GXM SoCs:
They are using a dwc3 USB controller with up to three ports enabled on
the internal roothub. Using only the top-level "phy" properties does not
work here since one can only specify one "usb2-phy" and one "usb3-phy",
while actually at least two "usb2-phy" have to be specified.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  7 ++++
 drivers/usb/host/Kconfig                           |  1 +
 drivers/usb/host/xhci-plat.c                       | 37 ++++++++++++++++++++--
 drivers/usb/host/xhci.h                            |  3 ++
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index 0b7d8576001c..f05b2306ae19 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -28,6 +28,13 @@ Optional properties:
   - clocks: reference to a clock
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
 
+sub-nodes:
+- optionally there can be a node for the root-hub, see usb-roothub.txt in the
+  current directory
+- one or more nodes with reg 1-31 for each port to which a device is connected.
+  See usb-device.txt in the current directory for more information.
+
+
 Example:
 	usb@f0931000 {
 		compatible = "generic-xhci";
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index eb7009460322..b83b5aa89b76 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -36,6 +36,7 @@ config USB_XHCI_PCI
 config USB_XHCI_PLATFORM
 	tristate "Generic xHCI driver for a platform device"
 	select USB_XHCI_RCAR if ARCH_RENESAS
+	select USB_PLATFORM_ROOTHUB
 	---help---
 	  Adds an xHCI host driver for a generic platform device, which
 	  provides a memory space and an irq.
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index e94bfd07e6c3..a0211cebac71 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -248,6 +248,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		goto disable_clk;
 	}
 
+	xhci->platform_roothub = platform_roothub_init(sysdev);
+	if (IS_ERR(xhci->platform_roothub)) {
+		ret = PTR_ERR(xhci->platform_roothub);
+		goto disable_clk;
+	}
+
 	if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
 		xhci->quirks |= XHCI_LPM_SUPPORT;
 
@@ -266,10 +272,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			goto put_usb3_hcd;
 	}
 
-	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
+	ret = platform_roothub_power_on(xhci->platform_roothub);
 	if (ret)
 		goto disable_usb_phy;
 
+	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
+	if (ret)
+		goto disable_platform_roothub;
+
 	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto dealloc_usb2_hcd;
@@ -280,6 +290,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
+disable_platform_roothub:
+	platform_roothub_power_off(xhci->platform_roothub);
+
 disable_usb_phy:
 	usb_phy_shutdown(hcd->usb_phy);
 
@@ -305,6 +318,8 @@ static int xhci_plat_remove(struct platform_device *dev)
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_phy_shutdown(hcd->usb_phy);
 
+	platform_roothub_power_off(xhci->platform_roothub);
+
 	usb_remove_hcd(hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
@@ -320,6 +335,7 @@ static int xhci_plat_suspend(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	int ret;
 
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
@@ -329,15 +345,30 @@ static int xhci_plat_suspend(struct device *dev)
 	 * reconsider this when xhci_plat_suspend enlarges its scope, e.g.,
 	 * also applies to runtime suspend.
 	 */
-	return xhci_suspend(xhci, device_may_wakeup(dev));
+	ret = xhci_suspend(xhci, device_may_wakeup(dev));
+	if (ret)
+		return ret;
+
+	platform_roothub_power_off(xhci->platform_roothub);
+
+	return 0;
 }
 
 static int xhci_plat_resume(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	int ret;
 
-	return xhci_resume(xhci, 0);
+	ret = platform_roothub_power_on(xhci->platform_roothub);
+	if (ret)
+		return ret;
+
+	ret = xhci_resume(xhci, 0);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static const struct dev_pm_ops xhci_plat_pm_ops = {
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2d7b6374b58d..c1d384223419 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -34,6 +34,8 @@
 #include	"xhci-ext-caps.h"
 #include "pci-quirks.h"
 
+#include "platform-roothub.h"
+
 /* xHCI PCI Configuration Registers */
 #define XHCI_SBRN_OFFSET	(0x60)
 
@@ -1559,6 +1561,7 @@ struct xhci_hcd {
 	struct msix_entry	*msix_entries;
 	/* optional clock */
 	struct clk		*clk;
+	struct platform_roothub	*platform_roothub;
 	/* data structures */
 	struct xhci_device_context_array *dcbaa;
 	struct xhci_ring	*cmd_ring;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] usb: host: add a generic platform USB roothub driver

[Rob Herring]

On Wed, Jan 11, 2017 at 04:29:46PM +0100, Martin Blumenstingl wrote:
> Many SoC platforms have separate devices for the USB PHY which are
> registered through the generic PHY framework. These PHYs have to be
> enabled to make the USB controller actually work. They also have to be
> disabled again on shutdown/suspend.
> 
> Currently (at least) the following HCI platform drivers are using custom
> code to obtain all PHYs via devicetree for the roothub/controller and
> disable/enable them when required:
> - ehci-platform.c has ehci_platform_power_{on,off}
> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
> - ohci-platform.c has ohci_platform_power_{on,off}
> 
> These drivers are not using the generic devicetree USB device bindings
> yet which were only introduced recently (documentation is available in
> devicetree/bindings/usb/usb-device.txt).
> With this new driver the usb2-phy and usb3-phy can be specified directly
> in the child-node of the corresponding port of the roothub via
> devicetree. This can be extended by not just parsing PHYs (some of the
> other drivers listed above are for example also parsing a list of clocks
> as well) when required.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/usb-roothub.txt        |  41 ++++++
>  drivers/usb/host/Kconfig                           |   3 +
>  drivers/usb/host/Makefile                          |   2 +
>  drivers/usb/host/platform-roothub.c                | 146 +++++++++++++++++++++
>  drivers/usb/host/platform-roothub.h                |  14 ++
>  5 files changed, 206 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb-roothub.txt
>  create mode 100644 drivers/usb/host/platform-roothub.c
>  create mode 100644 drivers/usb/host/platform-roothub.h
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-roothub.txt b/Documentation/devicetree/bindings/usb/usb-roothub.txt
> new file mode 100644
> index 000000000000..96e152d3901c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb-roothub.txt
> @@ -0,0 +1,41 @@
> +Generic USB root-hub Properties
> +
> +similar to the USB device bindings (documented in usb-device.txt from the
> +current directory) this provides support for configuring the root-hub.
> +
> +Required properties:
> +- compatible: should be at least one of "usb1d6b,3", "usb1d6b,2"
> +- reg: must be 0.
> +- address-cells: must be 1
> +- size-cells: must be 0

> +- a sub-node per port supports the following properties:

Make this another section with required and optional sections.

> +  - reg: the port number on the root-hub (mandatory)
> +  - phys: optional, from the *Generic PHY* bindings (mandatory needed when
> +    phy-names is given)
> +  - phy-names: optional, from the *Generic PHY* bindings; supported names
> +    are "usb2-phy" or "usb3-phy"
> +
> +Example:
> +	&usb1 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		roothub@0 {
> +			compatible = "usb1d6b,3", "usb1d6b,2";

Is this discoverable? IIRC, we had decided that ports on the root hub 
are just children of the USB controller node (rather than 
grandchildren). Why does that not work?

> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0>;
> +
> +			port@1 {

Wouldn't this normally be 0 and 1. This should probably be usb-port 
rather than port to avoid OF graph overlap.

> +				reg = <1>;
> +				usb-phy = <&usb2_phy1>, <&usb3_phy1>;

s/usb-phy/phys/

> +				phy-names = "usb2-phy", "usb3-phy";
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				usb-phy = <&usb2_phy2>, <&usb3_phy2>;
> +				phy-names = "usb2-phy", "usb3-phy";
> +			};
> +		};
> +	}
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] usb: host: add a generic platform USB roothub driver

[Martin Blumenstingl]

Hi Rob,

On Fri, Jan 13, 2017 at 9:08 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Jan 11, 2017 at 04:29:46PM +0100, Martin Blumenstingl wrote:
>> Many SoC platforms have separate devices for the USB PHY which are
>> registered through the generic PHY framework. These PHYs have to be
>> enabled to make the USB controller actually work. They also have to be
>> disabled again on shutdown/suspend.
>>
>> Currently (at least) the following HCI platform drivers are using custom
>> code to obtain all PHYs via devicetree for the roothub/controller and
>> disable/enable them when required:
>> - ehci-platform.c has ehci_platform_power_{on,off}
>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
>> - ohci-platform.c has ohci_platform_power_{on,off}
>>
>> These drivers are not using the generic devicetree USB device bindings
>> yet which were only introduced recently (documentation is available in
>> devicetree/bindings/usb/usb-device.txt).
>> With this new driver the usb2-phy and usb3-phy can be specified directly
>> in the child-node of the corresponding port of the roothub via
>> devicetree. This can be extended by not just parsing PHYs (some of the
>> other drivers listed above are for example also parsing a list of clocks
>> as well) when required.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>>  .../devicetree/bindings/usb/usb-roothub.txt        |  41 ++++++
>>  drivers/usb/host/Kconfig                           |   3 +
>>  drivers/usb/host/Makefile                          |   2 +
>>  drivers/usb/host/platform-roothub.c                | 146 +++++++++++++++++++++
>>  drivers/usb/host/platform-roothub.h                |  14 ++
>>  5 files changed, 206 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/usb-roothub.txt
>>  create mode 100644 drivers/usb/host/platform-roothub.c
>>  create mode 100644 drivers/usb/host/platform-roothub.h
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-roothub.txt b/Documentation/devicetree/bindings/usb/usb-roothub.txt
>> new file mode 100644
>> index 000000000000..96e152d3901c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/usb-roothub.txt
>> @@ -0,0 +1,41 @@
>> +Generic USB root-hub Properties
>> +
>> +similar to the USB device bindings (documented in usb-device.txt from the
>> +current directory) this provides support for configuring the root-hub.
>> +
>> +Required properties:
>> +- compatible: should be at least one of "usb1d6b,3", "usb1d6b,2"
>> +- reg: must be 0.
>> +- address-cells: must be 1
>> +- size-cells: must be 0
>
>> +- a sub-node per port supports the following properties:
>
> Make this another section with required and optional sections.
I can do that, but let's wait for the results if we want the PHYs to
be specified at the grand-child or child level

>> +  - reg: the port number on the root-hub (mandatory)
>> +  - phys: optional, from the *Generic PHY* bindings (mandatory needed when
>> +    phy-names is given)
>> +  - phy-names: optional, from the *Generic PHY* bindings; supported names
>> +    are "usb2-phy" or "usb3-phy"
>> +
>> +Example:
>> +     &usb1 {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +
>> +             roothub@0 {
>> +                     compatible = "usb1d6b,3", "usb1d6b,2";
>
> Is this discoverable? IIRC, we had decided that ports on the root hub
> are just children of the USB controller node (rather than
> grandchildren). Why does that not work?
if I understand you correctly you are thinking of something like this:
&usb1 {
    ...cells...

    port@1 {
        reg = <1>;
        phys = <&phy2>
    }

    port@2 {
        reg = <2>;
        phys = <&phy2>
    }
}

in that case we need a way to differentiate between "actual device at
port 1" and "configuration for root-hub port 1".
in that example I also cannot specify a compatible string since I
don't know which device might be plugged into that port.

>> +                     #address-cells = <1>;
>> +                     #size-cells = <0>;
>> +                     reg = <0>;
>> +
>> +                     port@1 {
>
> Wouldn't this normally be 0 and 1. This should probably be usb-port
> rather than port to avoid OF graph overlap.
the USB subsystem starts counting at 1, there can never be a "device
with device-number 0" - so I think we should stay with 1 and 2 for a
2-port roothub.
I'm fine with changing the name to "usb-port" though

>> +                             reg = <1>;
>> +                             usb-phy = <&usb2_phy1>, <&usb3_phy1>;
>
> s/usb-phy/phys/
thanks for spotting this

>> +                             phy-names = "usb2-phy", "usb3-phy";
>> +                     };
>> +
>> +                     port@2 {
>> +                             reg = <2>;
>> +                             usb-phy = <&usb2_phy2>, <&usb3_phy2>;
>> +                             phy-names = "usb2-phy", "usb3-phy";
>> +                     };
>> +             };
>> +     }
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] usb: host: add a generic platform USB roothub driver

[Martin Blumenstingl]

Hi Rob,

On Fri, Jan 13, 2017 at 9:21 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> Hi Rob,
>
> On Fri, Jan 13, 2017 at 9:08 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Wed, Jan 11, 2017 at 04:29:46PM +0100, Martin Blumenstingl wrote:
>>> Many SoC platforms have separate devices for the USB PHY which are
>>> registered through the generic PHY framework. These PHYs have to be
>>> enabled to make the USB controller actually work. They also have to be
>>> disabled again on shutdown/suspend.
>>>
>>> Currently (at least) the following HCI platform drivers are using custom
>>> code to obtain all PHYs via devicetree for the roothub/controller and
>>> disable/enable them when required:
>>> - ehci-platform.c has ehci_platform_power_{on,off}
>>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
>>> - ohci-platform.c has ohci_platform_power_{on,off}
>>>
>>> These drivers are not using the generic devicetree USB device bindings
>>> yet which were only introduced recently (documentation is available in
>>> devicetree/bindings/usb/usb-device.txt).
>>> With this new driver the usb2-phy and usb3-phy can be specified directly
>>> in the child-node of the corresponding port of the roothub via
>>> devicetree. This can be extended by not just parsing PHYs (some of the
>>> other drivers listed above are for example also parsing a list of clocks
>>> as well) when required.
>>>
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>> ---
>>>  .../devicetree/bindings/usb/usb-roothub.txt        |  41 ++++++
>>>  drivers/usb/host/Kconfig                           |   3 +
>>>  drivers/usb/host/Makefile                          |   2 +
>>>  drivers/usb/host/platform-roothub.c                | 146 +++++++++++++++++++++
>>>  drivers/usb/host/platform-roothub.h                |  14 ++
>>>  5 files changed, 206 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/usb/usb-roothub.txt
>>>  create mode 100644 drivers/usb/host/platform-roothub.c
>>>  create mode 100644 drivers/usb/host/platform-roothub.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/usb-roothub.txt b/Documentation/devicetree/bindings/usb/usb-roothub.txt
>>> new file mode 100644
>>> index 000000000000..96e152d3901c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/usb-roothub.txt
>>> @@ -0,0 +1,41 @@
>>> +Generic USB root-hub Properties
>>> +
>>> +similar to the USB device bindings (documented in usb-device.txt from the
>>> +current directory) this provides support for configuring the root-hub.
>>> +
>>> +Required properties:
>>> +- compatible: should be at least one of "usb1d6b,3", "usb1d6b,2"
>>> +- reg: must be 0.
>>> +- address-cells: must be 1
>>> +- size-cells: must be 0
>>
>>> +- a sub-node per port supports the following properties:
>>
>> Make this another section with required and optional sections.
> I can do that, but let's wait for the results if we want the PHYs to
> be specified at the grand-child or child level
>
>>> +  - reg: the port number on the root-hub (mandatory)
>>> +  - phys: optional, from the *Generic PHY* bindings (mandatory needed when
>>> +    phy-names is given)
>>> +  - phy-names: optional, from the *Generic PHY* bindings; supported names
>>> +    are "usb2-phy" or "usb3-phy"
>>> +
>>> +Example:
>>> +     &usb1 {
>>> +             #address-cells = <1>;
>>> +             #size-cells = <0>;
>>> +
>>> +             roothub@0 {
>>> +                     compatible = "usb1d6b,3", "usb1d6b,2";
>>
>> Is this discoverable? IIRC, we had decided that ports on the root hub
>> are just children of the USB controller node (rather than
>> grandchildren). Why does that not work?
> if I understand you correctly you are thinking of something like this:
> &usb1 {
>     ...cells...
>
>     port@1 {
>         reg = <1>;
>         phys = <&phy2>
>     }
>
>     port@2 {
>         reg = <2>;
>         phys = <&phy2>
>     }
> }
>
> in that case we need a way to differentiate between "actual device at
> port 1" and "configuration for root-hub port 1".
> in that example I also cannot specify a compatible string since I
> don't know which device might be plugged into that port.
could you please share your thoughts on this? can you point me to any
documentation or a discussion on a mailing-list which describes how
ports on the root hub should be modeled?

>>> +                     #address-cells = <1>;
>>> +                     #size-cells = <0>;
>>> +                     reg = <0>;
>>> +
>>> +                     port@1 {
>>
>> Wouldn't this normally be 0 and 1. This should probably be usb-port
>> rather than port to avoid OF graph overlap.
> the USB subsystem starts counting at 1, there can never be a "device
> with device-number 0" - so I think we should stay with 1 and 2 for a
> 2-port roothub.
> I'm fine with changing the name to "usb-port" though
>
>>> +                             reg = <1>;
>>> +                             usb-phy = <&usb2_phy1>, <&usb3_phy1>;
>>
>> s/usb-phy/phys/
> thanks for spotting this
>
>>> +                             phy-names = "usb2-phy", "usb3-phy";
>>> +                     };
>>> +
>>> +                     port@2 {
>>> +                             reg = <2>;
>>> +                             usb-phy = <&usb2_phy2>, <&usb3_phy2>;
>>> +                             phy-names = "usb2-phy", "usb3-phy";
>>> +                     };
>>> +             };
>>> +     }
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html