Re: [RFC usb-next v5 2/3] usb: core: add a wrapper for the USB PHYs on the root-hub

[Martin Blumenstingl]

On Sun, Oct 8, 2017 at 11:17 PM, Martin Blumenstingl
 wrote:
[snip]
> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> +{
> +   struct device_node *roothub_np, *port_np;
> +   struct usb_phy_roothub *phy_roothub;
> +   struct usb_phy_roothub *roothub_entry;
> +   struct list_head *head;
> +   int err;
> +
> +   roothub_np = usb_of_get_child_node(dev->of_node, ROOTHUB_PORTNUM);
> +   if (!of_device_is_available(roothub_np)) {
> +   dev_err(dev, "no roothub node found\n");
Xiaolong Ye's kbuild test robot found this dev_err() which shouldn't
be there. I'll remove it in the next version - thanks for spotting
this!

> +   return NULL;
> +   }
> +
[snip]
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC usb-next v5 2/3] usb: core: add a wrapper for the USB PHYs on the root-hub

[Martin Blumenstingl]

Hi Alan,

thank you for taking the time to review my patches!

On Mon, Oct 9, 2017 at 7:16 PM, Alan Stern  wrote:
> On Sun, 8 Oct 2017, 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.
>
> ...
>
>> --- /dev/null
>> +++ b/drivers/usb/core/phy.h
>> @@ -0,0 +1,7 @@
>> +struct usb_phy_roothub;
>> +
>> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
>> +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
>> +
>> +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>> +int usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
>
> Have you considered the possibility that a phy might need three power
> states, for on, off, and suspended?
I have not considered this yet - on the Mediatek platform (tested by
Chunfeng Yun) _power_{on,off} works fine for a suspend/resume cycle.
the other platform this was tested on is Amlogic Meson GXL/GXM,
however we don't have suspend support there

the underlying code relies on the generic PHY framework, which has
*runtime PM* support, but no suspend/resume PM functions - see [0]

> Alan Stern
>


Regards,
Martin


[0] 
http://elixir.free-electrons.com/linux/v4.14-rc4/source/include/linux/phy/phy.h#L131
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC usb-next v5 2/3] usb: core: add a wrapper for the USB PHYs on the root-hub

[Alan Stern]

On Sun, 8 Oct 2017, 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.

...

> --- /dev/null
> +++ b/drivers/usb/core/phy.h
> @@ -0,0 +1,7 @@
> +struct usb_phy_roothub;
> +
> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
> +
> +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
> +int usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);

Have you considered the possibility that a phy might need three power
states, for on, off, and suspended?

Alan Stern

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

[RFC usb-next v5 2/3] usb: core: add a wrapper for the USB PHYs on the root-hub

[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
(for the root-hub) yet which were only introduced recently (documentation
is available in devicetree/bindings/usb/usb-device.txt).

With this new wrapper the usb2-phy and usb3-phy can be specified directly
in the child-node of the corresponding port of the roothub via
devicetree.
This allows SoCs like the Amlogic Meson GXL family to operate correctly
because all USB PHYs are initialized (instead of the first USB PHY
only).

Signed-off-by: Martin Blumenstingl 
---
 drivers/usb/core/Makefile |   2 +-
 drivers/usb/core/phy.c| 184 ++
 drivers/usb/core/phy.h|   7 ++
 3 files changed, 192 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/core/phy.c
 create mode 100644 drivers/usb/core/phy.h

diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 250ec1d662d9..b6e181d08bf6 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -5,7 +5,7 @@
 usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
 usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
 usbcore-y += devio.o notify.o generic.o quirks.o devices.o
-usbcore-y += port.o
+usbcore-y += phy.o port.o
 
 usbcore-$(CONFIG_OF)   += of.o
 usbcore-$(CONFIG_USB_PCI)  += hcd-pci.o
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
new file mode 100644
index ..d90a55b2ae08
--- /dev/null
+++ b/drivers/usb/core/phy.c
@@ -0,0 +1,184 @@
+/*
+ * USB PHY roothub driver - a wrapper for multiple PHYs which passes all phy_*
+ * function calls to multiple (actual) PHY devices. This is comes handy when
+ * initializing all PHYs on a root-hub and to keep them all in the same state.
+ *
+ * Copyright (C) 2017 Martin Blumenstingl 
+ *
+ * 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 .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "phy.h"
+
+#define ROOTHUB_PORTNUM0
+
+struct usb_phy_roothub {
+   struct phy  *phy;
+   struct list_headlist;
+};
+
+static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
+{
+   struct usb_phy_roothub *roothub_entry;
+
+   roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
+   if (!roothub_entry)
+   return ERR_PTR(-ENOMEM);
+
+   INIT_LIST_HEAD(_entry->list);
+
+   return roothub_entry;
+}
+
+static int usb_phy_roothub_add_phy(struct device *dev,
+  struct device_node *port_np,
+  const char *con_id, struct list_head *list)
+{
+   struct usb_phy_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 = usb_phy_roothub_alloc(dev);
+   if (IS_ERR(roothub_entry))
+   return PTR_ERR(roothub_entry);
+
+   roothub_entry->phy = phy;
+
+   list_add_tail(_entry->list, list);
+
+   return 0;
+}
+
+struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
+{
+   struct device_node *roothub_np, *port_np;
+   struct usb_phy_roothub *phy_roothub;
+   struct usb_phy_roothub *roothub_entry;
+   struct list_head *head;
+   int err;
+
+   roothub_np = usb_of_get_child_node(dev->of_node, ROOTHUB_PORTNUM);
+   if (!of_device_is_available(roothub_np)) {
+   dev_err(dev, "no roothub node found\n");
+   return NULL;
+   }
+
+   phy_roothub = usb_phy_roothub_alloc(dev);
+   if (IS_ERR(phy_roothub))
+   return phy_roothub;
+
+   for_each_available_child_of_node(roothub_np, port_np) {
+   err = usb_phy_roothub_add_phy(dev, port_np, "usb2-phy",
+ _roothub->list);
+   if (err)
+