[RFC usb-next v5 0/3] initialize (multiple) PHYs on the roothub

[RFC usb-next v5 0/3] initialize (multiple) PHYs on the roothub

[Martin Blumenstingl]

This series is the outcome of a discussion with Felipe Balbi,
see [0] and [1] as well as Mathias Nyman, see [7] and [8].
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 "roothub PHY wrapper". This can be configured
through devicetree by passing a child-node with "reg = <0>" (in other
words: it describes the roothub) 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 roothub PHY wrapper into hcd.c
which automatically enables it for all USB controller drivers (tested
on an Amlogic Meson GXL SoC which uses a dwc3 controller).


Changes since v4 at [9]:
- renamed the subject of the cover-letter (old name was:
  "initialize (multiple) PHYs in xhci-plat")
- back into RFC status (see below for the reasons)
- dropped Tested-by from Chunfeng Yun (same reasons as RFC status)
- reworded cover-letter and commit messages from "platform-roothub"
  to "roothub PHY wrapper"
- moved code from drivers/usb/host/platform-roothub.* to
  drivers/usb/core/phy.* and the changes to
  drivers/usb/host/xhci-plat.c to drivers/usb/core/hcd.c as suggested
  by Mathias Nyman (as a benefit this will enable the new logic for
  non-xHCI controllers as well - however this was not tested yet)
- rename the structs, function names, etc from platform_roothub_* to
  usb_phy_roothub*

Changes since RFCv3 at [6]:
- moved the DT binding change from patch #3 to patch #1 as suggested
  by Rob Herring (and slightly adjusted the commit message to account
  for that)
- added Tested-by from Chunfeng Yun (who confirmed that the whole
  concept and implementation works fine on Mediatek SoCs - many thanks
  again!) to patch #2
- added Rob Herring's ACK to patches 1 and 3
- dropped RFC status (RFCv3 -> PATCH v4)

Changes since RFCv2 at [5]:
- split phy_{init,exit} and phy_power_{on,off} handling. up until RFCv2
  I called phy_init plus phy_power_on in platform_roothub_power_on and
  phy_power_off plus phy_exit in platform_roothub_power_off. However,
  Chunfeng Yun (a Mediatek SoC developer - many thanks for testing my
  series and providing great feedback) reported that only using
  phy_power_off (and omitting phy_exit) during system suspend fixes an
  issue where USB devices would be re-enumerated when resuming. His
  original problem description: "In order to keep link state on mt8173,
  we just power off all phys(not exit) when system enter suspend, then
  power on them again (needn't init, otherwise device will be
  disconnected) when system resume, this can avoid re-enumerating
  device.". This fix affects patch #2 and #3 as we now have
  platform_roothub_init (which calls phy_init internally),
  platform_roothub_power_on (which calls phy_power_on internally),
  platform_roothub_power_off (which calls phy_power_off internally) and
  platform_roothub_exit (which calls phy_exit internally). suspend and
  resume only call platform_roothub_power_{on,off} to prevent the issue
  described by Chunfeng Yun (unfortunately I cannot test this because
  the Amlogic platform currently does not support system suspend).
- dropped two struct forward declarations from platform-roothub.h which
  are not used in the header file (thanks to Chunfeng Yun for spotting
  this)

Changes since RFCv1 at [4]:
- split the usb-xhci dt-binding documentation into a separate patch
- fixed a typo ("usb-phy" -> "phys" in the dt-binding example)
- rebased to apply against latest usb-next


[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
[4] http://marc.info/?l=linux-usb&m=148414866303604&w=2
[5] https://www.spinics.net/lists/linux-usb/msg158967.html
[6] https://www.spinics.net/lists/devicetree/msg190426.html
[7] http://lists.infradead.org/pipermail/linux-amlogic/2017-October/004881.html
[8] http://lists.infradead.org/pipermail/linux-amlogic/2017-October/004920.html
[9] http://lists.infradead.org/pipermail/linux-amlogic/2017-September/004685.html


Martin Blumenstingl (3):
  dt-bindings: usb: add the documentation for USB root-hub
  usb: core: add a wrapper for the USB PHYs on the root-hub
  usb: core: hcd: integrate the PHY roothub wrapper

 .../devicetree/bindings/usb/usb-roothub.txt        |  46 ++++++
 Documentation/devicetree/bindings/usb/usb-xhci.txt |   7 +
 drivers/usb/core/Makefile                          |   2 +-
 drivers/usb/core/hcd.c                             |  30 +++-
 drivers/usb/core/phy.c                             | 184 +++++++++++++++++++++
 drivers/usb/core/phy.h                             |   7 +
 include/linux/usb/hcd.h                            |   1 +
 7 files changed, 275 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usb-roothub.txt
 create mode 100644 drivers/usb/core/phy.c
 create mode 100644 drivers/usb/core/phy.h

-- 
2.14.2

--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Martin Blumenstingl]

A USB root-hub may have several PHYs which need to be configured before
the root-hub starts working.
This adds the documentation for such a USB root-hub as well as a hint
regarding the child-nodes on XHCI controllers which can include the
roothub.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/usb/usb-roothub.txt        | 46 ++++++++++++++++++++++
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  7 ++++
 2 files changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb-roothub.txt

diff --git a/Documentation/devicetree/bindings/usb/usb-roothub.txt b/Documentation/devicetree/bindings/usb/usb-roothub.txt
new file mode 100644
index 000000000000..fc0797d7cee9
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb-roothub.txt
@@ -0,0 +1,46 @@
+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
+
+Required sub-nodes:
+a sub-node per actual USB port is required. each sub-node supports the
+following properties:
+  Required properties:
+    - reg: the port number on the root-hub (mandatory)
+  Optional properties:
+    - 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>;
+				phys = <&usb2_phy1>, <&usb3_phy1>;
+				phy-names = "usb2-phy", "usb3-phy";
+			};
+
+			port@2 {
+				reg = <2>;
+				phys = <&usb2_phy2>, <&usb3_phy2>;
+				phy-names = "usb2-phy", "usb3-phy";
+			};
+		};
+	}
diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index ae6e484a8d7c..5b49ba9f2f9a 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -30,6 +30,13 @@ Optional properties:
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
 
+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";
-- 
2.14.2

--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Arnd Bergmann]

On Sun, Oct 8, 2017 at 11:17 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> A USB root-hub may have several PHYs which need to be configured before
> the root-hub starts working.
> This adds the documentation for such a USB root-hub as well as a hint
> regarding the child-nodes on XHCI controllers which can include the
> roothub.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Have you checked that this still works with DT properties on a USB device
that is listed in the DT? A common use-case is to provide the MAC address
of a soldered-down USB-ethernet that lacks its own eeprom, and it seems
you are changing the way the parent devices of that get represented,
so the dev->of_node pointer in the USB device might no longer refer
to the correct device.

       Arnd
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Martin Blumenstingl]

Hi Arnd,

thank you for reviewing this patch!

On Mon, Oct 9, 2017 at 12:24 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Sun, Oct 8, 2017 at 11:17 PM, Martin Blumenstingl
> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>> A USB root-hub may have several PHYs which need to be configured before
>> the root-hub starts working.
>> This adds the documentation for such a USB root-hub as well as a hint
>> regarding the child-nodes on XHCI controllers which can include the
>> roothub.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
> Have you checked that this still works with DT properties on a USB device
> that is listed in the DT? A common use-case is to provide the MAC address
> of a soldered-down USB-ethernet that lacks its own eeprom, and it seems
> you are changing the way the parent devices of that get represented,
> so the dev->of_node pointer in the USB device might no longer refer
> to the correct device.
I haven't tested the described use-case. however, this patch is not
supposed to change the binding for actual devices.
USB device numbering starts at 1, while 0 is reserved for the root-hub
(at least from what I know). before this patch there was no way to
describe the root-hub via .dts. this however is required for some
platforms that need to set up a PHY for each port on the root-hub
(Amlogic Meson GXL platform is one example: there are two ports
enabled on dwc3's root-hub with 2x USB2 and 1x USB3 PHYs) - so this
patch uses a similar binding as we already have (to describe the
devices) to describe the root-hub


Regards,
Martin
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Alan Stern]

On Thu, 12 Oct 2017, Martin Blumenstingl wrote:

> I haven't tested the described use-case. however, this patch is not
> supposed to change the binding for actual devices.
> USB device numbering starts at 1, while 0 is reserved for the root-hub
> (at least from what I know).

Actually 1 is reserved for the root hub.  External devices are numbered 
starting from 2.  (We can't use 0 for the root hub because, as you 
said, USB device numbering starts at 1!)

Alan Stern

> before this patch there was no way to
> describe the root-hub via .dts. this however is required for some
> platforms that need to set up a PHY for each port on the root-hub
> (Amlogic Meson GXL platform is one example: there are two ports
> enabled on dwc3's root-hub with 2x USB2 and 1x USB3 PHYs) - so this
> patch uses a similar binding as we already have (to describe the
> devices) to describe the root-hub
> 
> 
> Regards,
> Martin
> --
> 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
> 
> 

--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Martin Blumenstingl]

Hi Alan,

On Thu, Oct 12, 2017 at 11:17 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Thu, 12 Oct 2017, Martin Blumenstingl wrote:
>
>> I haven't tested the described use-case. however, this patch is not
>> supposed to change the binding for actual devices.
>> USB device numbering starts at 1, while 0 is reserved for the root-hub
>> (at least from what I know).
>
> Actually 1 is reserved for the root hub.  External devices are numbered
> starting from 2.  (We can't use 0 for the root hub because, as you
> said, USB device numbering starts at 1!)
I just had a look at lsusb again:
$ lsusb -t
/:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M

however, for the devicetree bindings the devices are supposed to use 1-31: [0]

Alan: just to make sure I understood you correctly: do you agree with
this patch?

> Alan Stern
>
>> before this patch there was no way to
>> describe the root-hub via .dts. this however is required for some
>> platforms that need to set up a PHY for each port on the root-hub
>> (Amlogic Meson GXL platform is one example: there are two ports
>> enabled on dwc3's root-hub with 2x USB2 and 1x USB3 PHYs) - so this
>> patch uses a similar binding as we already have (to describe the
>> devices) to describe the root-hub
>>
>>
>> Regards,
>> Martin
>> --
>> 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
>>
>>
>


Regards,
Martin


[0] http://elixir.free-electrons.com/linux/v4.14-rc4/source/Documentation/devicetree/bindings/usb/usb-device.txt
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Alan Stern]

On Thu, 12 Oct 2017, Martin Blumenstingl wrote:

> Hi Alan,
> 
> On Thu, Oct 12, 2017 at 11:17 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> > On Thu, 12 Oct 2017, Martin Blumenstingl wrote:
> >
> >> I haven't tested the described use-case. however, this patch is not
> >> supposed to change the binding for actual devices.
> >> USB device numbering starts at 1, while 0 is reserved for the root-hub
> >> (at least from what I know).
> >
> > Actually 1 is reserved for the root hub.  External devices are numbered
> > starting from 2.  (We can't use 0 for the root hub because, as you
> > said, USB device numbering starts at 1!)
> I just had a look at lsusb again:
> $ lsusb -t
> /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
> 
> however, for the devicetree bindings the devices are supposed to use 1-31: [0]
> 
> Alan: just to make sure I understood you correctly: do you agree with
> this patch?

I have no idea.  I haven't read the patch and I'm not familiar with the
details of DeviceTree.  I was just trying to clear up your
misunderstanding of USB device numbering.

Alan Stern

--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Martin Blumenstingl]

Hi Alan,

On Fri, Oct 13, 2017 at 4:15 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Thu, 12 Oct 2017, Martin Blumenstingl wrote:
>
>> Hi Alan,
>>
>> On Thu, Oct 12, 2017 at 11:17 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
>> > On Thu, 12 Oct 2017, Martin Blumenstingl wrote:
>> >
>> >> I haven't tested the described use-case. however, this patch is not
>> >> supposed to change the binding for actual devices.
>> >> USB device numbering starts at 1, while 0 is reserved for the root-hub
>> >> (at least from what I know).
>> >
>> > Actually 1 is reserved for the root hub.  External devices are numbered
>> > starting from 2.  (We can't use 0 for the root hub because, as you
>> > said, USB device numbering starts at 1!)
>> I just had a look at lsusb again:
>> $ lsusb -t
>> /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>>
>> however, for the devicetree bindings the devices are supposed to use 1-31: [0]
>>
>> Alan: just to make sure I understood you correctly: do you agree with
>> this patch?
>
> I have no idea.  I haven't read the patch and I'm not familiar with the
> details of DeviceTree.  I was just trying to clear up your
> misunderstanding of USB device numbering.
thank you for clarifying this.
I tested it and the devicetree numbering is "off by one" compared to
the lsusb output - this however has nothing to do with my patch.
so my previous statements that basically said "0 is used for the
roothub" should be "0 is used for the roothub in .dts/.dtsi files"


Regards,
Martin
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Arnd Bergmann]

On Thu, Oct 12, 2017 at 10:56 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> Hi Arnd,
>
> thank you for reviewing this patch!
>
> On Mon, Oct 9, 2017 at 12:24 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Sun, Oct 8, 2017 at 11:17 PM, Martin Blumenstingl
>> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>> A USB root-hub may have several PHYs which need to be configured before
>>> the root-hub starts working.
>>> This adds the documentation for such a USB root-hub as well as a hint
>>> regarding the child-nodes on XHCI controllers which can include the
>>> roothub.
>>>
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>
>> Have you checked that this still works with DT properties on a USB device
>> that is listed in the DT? A common use-case is to provide the MAC address
>> of a soldered-down USB-ethernet that lacks its own eeprom, and it seems
>> you are changing the way the parent devices of that get represented,
>> so the dev->of_node pointer in the USB device might no longer refer
>> to the correct device.
> I haven't tested the described use-case. however, this patch is not
> supposed to change the binding for actual devices.
> USB device numbering starts at 1, while 0 is reserved for the root-hub
> (at least from what I know). before this patch there was no way to
> describe the root-hub via .dts. this however is required for some
> platforms that need to set up a PHY for each port on the root-hub
> (Amlogic Meson GXL platform is one example: there are two ports
> enabled on dwc3's root-hub with 2x USB2 and 1x USB3 PHYs) - so this
> patch uses a similar binding as we already have (to describe the
> devices) to describe the root-hub

My point is that the DT binding does depend on the hierarchy, there
is no way to find a particular device unless each parent up the
chain is described correctly in DT as well and has a valid of_node
pointer.

It's possible that this has never worked on XHCI because of the lack
of the root-hub in DT. Either way, we should ensure that it does work
now and you didn't break it, so please at least test it with your patches.

The patch below should be sufficient to see if any device has an
of_node pointer when you add it to your DT:

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 17681d5638ac..498d0aa0a5c0 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -647,6 +647,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
                }
                dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
                                raw_port);
+               dev_info(&dev->dev, "of_node %p parent %p\n",
dev->dev.of_node, parent->dev.of_node);

                /* hub driver sets up TT records */
        }


       Arnd
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Martin Blumenstingl]

[-- Attachment #1: Type: text/plain, Size: 4875 bytes --]

Hi Arnd,

On Fri, Oct 13, 2017 at 9:37 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thu, Oct 12, 2017 at 10:56 PM, Martin Blumenstingl
> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>> Hi Arnd,
>>
>> thank you for reviewing this patch!
>>
>> On Mon, Oct 9, 2017 at 12:24 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>> On Sun, Oct 8, 2017 at 11:17 PM, Martin Blumenstingl
>>> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>>> A USB root-hub may have several PHYs which need to be configured before
>>>> the root-hub starts working.
>>>> This adds the documentation for such a USB root-hub as well as a hint
>>>> regarding the child-nodes on XHCI controllers which can include the
>>>> roothub.
>>>>
>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>
>>> Have you checked that this still works with DT properties on a USB device
>>> that is listed in the DT? A common use-case is to provide the MAC address
>>> of a soldered-down USB-ethernet that lacks its own eeprom, and it seems
>>> you are changing the way the parent devices of that get represented,
>>> so the dev->of_node pointer in the USB device might no longer refer
>>> to the correct device.
>> I haven't tested the described use-case. however, this patch is not
>> supposed to change the binding for actual devices.
>> USB device numbering starts at 1, while 0 is reserved for the root-hub
>> (at least from what I know). before this patch there was no way to
>> describe the root-hub via .dts. this however is required for some
>> platforms that need to set up a PHY for each port on the root-hub
>> (Amlogic Meson GXL platform is one example: there are two ports
>> enabled on dwc3's root-hub with 2x USB2 and 1x USB3 PHYs) - so this
>> patch uses a similar binding as we already have (to describe the
>> devices) to describe the root-hub
>
> My point is that the DT binding does depend on the hierarchy, there
> is no way to find a particular device unless each parent up the
> chain is described correctly in DT as well and has a valid of_node
> pointer.
>
> It's possible that this has never worked on XHCI because of the lack
> of the root-hub in DT. Either way, we should ensure that it does work
> now and you didn't break it, so please at least test it with your patches.
>
> The patch below should be sufficient to see if any device has an
> of_node pointer when you add it to your DT:
I slightly modified your patch (see the attached version) and tried it:
# lsusb -t
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/0p, 5000M
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
   |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
       |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage, 480M

the roothub (bus 1 port 1 dev 1) and a soldered down hub (child of
that root-hub, port 1 dev 2) have an entry in the .dts

# dmesg | grep usb
[    0.174097] usbcore: registered new interface driver usbfs
[    0.174147] usbcore: registered new interface driver hub
[    0.174217] usbcore: registered new device driver usb
[    1.354280] usb usb2: We don't know the algorithms for LPM for this
host, disabling LPM.
[    1.373297] usbcore: registered new interface driver usb-storage
[    1.512840] usbcore: registered new interface driver dvb_usb_rtl28xxu
[    1.550506] usb 1-1: of_node /soc/usb@c9000000/hub@1 parent /soc/usb@c9000000
^ this is the soldered down hub
[    1.552579] usbcore: registered new interface driver bcm203x
[    1.712033] usbcore: registered new interface driver usbhid
[    1.716994] usbhid: USB HID core driver
[    1.738827] usb 1-1: new high-speed USB device number 2 using xhci-hcd
[    2.142392] usb 1-1.3: of_node <no-node> parent /soc/usb@c9000000/hub@1
^ this is the thumb drive plugged into the hub (not specified in the .dts)
[    2.220399] usb 1-1.3: new high-speed USB device number 3 using xhci-hcd
[    2.326144] usb-storage 1-1.3:1.0: USB Mass Storage device detected
[    2.328352] scsi host0: usb-storage 1-1.3:1.0

so for me it seems to be working fine

is there anything else you want me to test?

> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 17681d5638ac..498d0aa0a5c0 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -647,6 +647,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>                 }
>                 dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
>                                 raw_port);
> +               dev_info(&dev->dev, "of_node %p parent %p\n",
> dev->dev.of_node, parent->dev.of_node);
>
>                 /* hub driver sets up TT records */
>         }
>
>
>        Arnd


Regards,
Martin

[-- Attachment #2: khadas-vim-soldered-usb-hub-print.patch --]
[-- Type: text/x-patch, Size: 885 bytes --]

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index cfbfbfeff85d..7677ce77d122 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -81,6 +81,11 @@
 					phy-names = "usb2-phy";
 				};
 			};
+
+			hub@1 {
+				compatible = "usb1a40,801";
+				reg = <1>;
+			};
 		};
 	};
 };
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 17681d5638ac..893f4e8b0733 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -647,6 +647,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
 		}
 		dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
 				raw_port);
+		dev_info(&dev->dev, "of_node %s parent %s\n", of_node_full_name(dev->dev.of_node), of_node_full_name(parent->dev.of_node));
 
 		/* hub driver sets up TT records */
 	}

Re: [RFC usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Arnd Bergmann]

On Tue, Oct 17, 2017 at 11:00 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> On Fri, Oct 13, 2017 at 9:37 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Thu, Oct 12, 2017 at 10:56 PM, Martin Blumenstingl
>> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>
>> It's possible that this has never worked on XHCI because of the lack
>> of the root-hub in DT. Either way, we should ensure that it does work
>> now and you didn't break it, so please at least test it with your patches.
>>
>> The patch below should be sufficient to see if any device has an
>> of_node pointer when you add it to your DT:
> I slightly modified your patch (see the attached version) and tried it:
> # lsusb -t
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/0p, 5000M
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
>    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>        |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>
> the roothub (bus 1 port 1 dev 1) and a soldered down hub (child of
> that root-hub, port 1 dev 2) have an entry in the .dts
>
> # dmesg | grep usb
> [    0.174097] usbcore: registered new interface driver usbfs
> [    0.174147] usbcore: registered new interface driver hub
> [    0.174217] usbcore: registered new device driver usb
> [    1.354280] usb usb2: We don't know the algorithms for LPM for this
> host, disabling LPM.
> [    1.373297] usbcore: registered new interface driver usb-storage
> [    1.512840] usbcore: registered new interface driver dvb_usb_rtl28xxu
> [    1.550506] usb 1-1: of_node /soc/usb@c9000000/hub@1 parent /soc/usb@c9000000
> ^ this is the soldered down hub
> [    1.552579] usbcore: registered new interface driver bcm203x
> [    1.712033] usbcore: registered new interface driver usbhid
> [    1.716994] usbhid: USB HID core driver
> [    1.738827] usb 1-1: new high-speed USB device number 2 using xhci-hcd
> [    2.142392] usb 1-1.3: of_node <no-node> parent /soc/usb@c9000000/hub@1
> ^ this is the thumb drive plugged into the hub (not specified in the .dts)
> [    2.220399] usb 1-1.3: new high-speed USB device number 3 using xhci-hcd
> [    2.326144] usb-storage 1-1.3:1.0: USB Mass Storage device detected
> [    2.328352] scsi host0: usb-storage 1-1.3:1.0
>
> so for me it seems to be working fine

Ok, very good!

> is there anything else you want me to test?

What about the same dtb when run on a kernel without your
patch series? Does that work as well, or are your patches
required to make it work?

       Arnd
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Martin Blumenstingl]

Hi Arnd,

On Tue, Oct 17, 2017 at 11:10 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Tue, Oct 17, 2017 at 11:00 PM, Martin Blumenstingl
> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>> On Fri, Oct 13, 2017 at 9:37 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>> On Thu, Oct 12, 2017 at 10:56 PM, Martin Blumenstingl
>>> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>>
>>> It's possible that this has never worked on XHCI because of the lack
>>> of the root-hub in DT. Either way, we should ensure that it does work
>>> now and you didn't break it, so please at least test it with your patches.
>>>
>>> The patch below should be sufficient to see if any device has an
>>> of_node pointer when you add it to your DT:
>> I slightly modified your patch (see the attached version) and tried it:
>> # lsusb -t
>> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/0p, 5000M
>> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
>>    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>>        |__ Port 3: Dev 3, If 0, Class=Mass Storage, Driver=usb-storage, 480M
>>
>> the roothub (bus 1 port 1 dev 1) and a soldered down hub (child of
>> that root-hub, port 1 dev 2) have an entry in the .dts
>>
>> # dmesg | grep usb
>> [    0.174097] usbcore: registered new interface driver usbfs
>> [    0.174147] usbcore: registered new interface driver hub
>> [    0.174217] usbcore: registered new device driver usb
>> [    1.354280] usb usb2: We don't know the algorithms for LPM for this
>> host, disabling LPM.
>> [    1.373297] usbcore: registered new interface driver usb-storage
>> [    1.512840] usbcore: registered new interface driver dvb_usb_rtl28xxu
>> [    1.550506] usb 1-1: of_node /soc/usb@c9000000/hub@1 parent /soc/usb@c9000000
>> ^ this is the soldered down hub
>> [    1.552579] usbcore: registered new interface driver bcm203x
>> [    1.712033] usbcore: registered new interface driver usbhid
>> [    1.716994] usbhid: USB HID core driver
>> [    1.738827] usb 1-1: new high-speed USB device number 2 using xhci-hcd
>> [    2.142392] usb 1-1.3: of_node <no-node> parent /soc/usb@c9000000/hub@1
>> ^ this is the thumb drive plugged into the hub (not specified in the .dts)
>> [    2.220399] usb 1-1.3: new high-speed USB device number 3 using xhci-hcd
>> [    2.326144] usb-storage 1-1.3:1.0: USB Mass Storage device detected
>> [    2.328352] scsi host0: usb-storage 1-1.3:1.0
>>
>> so for me it seems to be working fine
>
> Ok, very good!
>
>> is there anything else you want me to test?
>
> What about the same dtb when run on a kernel without your
> patch series? Does that work as well, or are your patches
> required to make it work?
this is the only device I have which uses devicetree and a xHCI
controller. I can test it with a dwc2 based device though if you want


Regards,
Martin
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Arnd Bergmann]

On Tue, Oct 17, 2017 at 11:19 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>> Ok, very good!
>>
>>> is there anything else you want me to test?
>>
>> What about the same dtb when run on a kernel without your
>> patch series? Does that work as well, or are your patches
>> required to make it work?
>
> this is the only device I have which uses devicetree and a xHCI
> controller. I can test it with a dwc2 based device though if you want

Does dwc2 also use separate nodes for the roothub? From your
description it sounds like it would not be affected by your patch.

To rephrase what I'm interested in, can you check that with your
patch series, we correctly associate a device node in DT with the
struct device in the kernel both with and without the optional
roothub node in the dtb?

Since you used a dtb that already listed an endpoint device below
an xhci, that would answer my earlier question of whether it worked
before your patch series, and you tested that it still works with your
patches applied and the roothub node added in the dtb.  Now we
just need to make sure we don't break existing dtb files that don't
have the roothub node but do have endpoint device nodes.

       Arnd
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Neil Armstrong]

On 18/10/2017 11:05, Arnd Bergmann wrote:
> On Tue, Oct 17, 2017 at 11:19 PM, Martin Blumenstingl
> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>> Ok, very good!
>>>
>>>> is there anything else you want me to test?
>>>
>>> What about the same dtb when run on a kernel without your
>>> patch series? Does that work as well, or are your patches
>>> required to make it work?
>>
>> this is the only device I have which uses devicetree and a xHCI
>> controller. I can test it with a dwc2 based device though if you want
> 
> Does dwc2 also use separate nodes for the roothub? From your
> description it sounds like it would not be affected by your patch.
> 
> To rephrase what I'm interested in, can you check that with your
> patch series, we correctly associate a device node in DT with the
> struct device in the kernel both with and without the optional
> roothub node in the dtb?
> 
> Since you used a dtb that already listed an endpoint device below
> an xhci, that would answer my earlier question of whether it worked
> before your patch series, and you tested that it still works with your
> patches applied and the roothub node added in the dtb.  Now we
> just need to make sure we don't break existing dtb files that don't
> have the roothub node but do have endpoint device nodes.
> 
>        Arnd
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 

Hi Arnd,

I tested in the same conditions :
- On Odroid-C2 using DWC2, but not using the roothub functionnality
- patch v6 applied
- node for on-board hub
- printk in usb core

And it works :
[    8.767964] usb 1-1: of_node /soc/usb@c9100000/hub@1 parent /soc/usb@c9100000
[    8.955901] usb 1-1: new high-speed USB device number 2 using dwc2
[    9.165641] hub 1-1:1.0: USB hub found
[    9.165939] hub 1-1:1.0: 4 ports detected

Neil
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Martin Blumenstingl]

Hi Arnd,

On Wed, Oct 18, 2017 at 11:05 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Tue, Oct 17, 2017 at 11:19 PM, Martin Blumenstingl
> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>> Ok, very good!
>>>
>>>> is there anything else you want me to test?
>>>
>>> What about the same dtb when run on a kernel without your
>>> patch series? Does that work as well, or are your patches
>>> required to make it work?
>>
>> this is the only device I have which uses devicetree and a xHCI
>> controller. I can test it with a dwc2 based device though if you want
>
> Does dwc2 also use separate nodes for the roothub? From your
> description it sounds like it would not be affected by your patch.
currently it doesn't use separate notes for the roothub - however,
with this patch it could (although I haven't explicitly tested this)

> To rephrase what I'm interested in, can you check that with your
> patch series, we correctly associate a device node in DT with the
> struct device in the kernel both with and without the optional
> roothub node in the dtb?
I can do the same tests that Neil did with SoCs that use a dwc2
controller (Amlogic Meson8m2 and Meson8b).
the only SoC I have which uses a dwc3 controller is an Amlogic Meson
GXL - however this won't see any devices (apart from the root-hub)
without this patch

> Since you used a dtb that already listed an endpoint device below
> an xhci, that would answer my earlier question of whether it worked
> before your patch series, and you tested that it still works with your
> patches applied and the roothub node added in the dtb.  Now we
> just need to make sure we don't break existing dtb files that don't
> have the roothub node but do have endpoint device nodes.
the endpoint you're seeing is the root-hub - you can see the full .dts here: [0]

maybe my patch description or documentation is not clear - could you
please explain what makes you think that specifying the root-hub
didn't work before (so I can update the comments where needed)?
to sum up what this series does: find the node with reg = <0>; (= the
root-hub) and get all PHY instances from the child-nodes
this should not change any existing behavior except if someone had a
node with reg = <0>; below any USB controller node (which was
undocumented behavior before my patches)


Regards,
Martin


[0] https://github.com/xdarklight/linux/blob/meson-gxl-usb-v6/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi#L55
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Arnd Bergmann]

On Thu, Oct 19, 2017 at 11:25 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>> Does dwc2 also use separate nodes for the roothub? From your
>> description it sounds like it would not be affected by your patch.
> currently it doesn't use separate notes for the roothub - however,
> with this patch it could (although I haven't explicitly tested this)

Ok.

>> Since you used a dtb that already listed an endpoint device below
>> an xhci, that would answer my earlier question of whether it worked
>> before your patch series, and you tested that it still works with your
>> patches applied and the roothub node added in the dtb.  Now we
>> just need to make sure we don't break existing dtb files that don't
>> have the roothub node but do have endpoint device nodes.
> the endpoint you're seeing is the root-hub - you can see the full .dts here: [0]
>
> maybe my patch description or documentation is not clear - could you
> please explain what makes you think that specifying the root-hub
> didn't work before (so I can update the comments where needed)?
> to sum up what this series does: find the node with reg = <0>; (= the
> root-hub) and get all PHY instances from the child-nodes
> this should not change any existing behavior except if someone had a
> node with reg = <0>; below any USB controller node (which was
> undocumented behavior before my patches)

Maybe I misunderstand what the actual change to the hierarchy
is. Quoting from your example for the new code

+       &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>;
+                               phys = <&usb2_phy1>, <&usb3_phy1>;
+                               phy-names = "usb2-phy", "usb3-phy";
+                       };

The way I understand it, an endpoing device would now be
located in

 &usb1/roothub@0/port@1/hub@2/device@1

where previously it was in

 &usb1/port@1/hub@2/device@1

Is that correct?

I don't see any code that can deal with both cases and still
assign the correct of_node pointer to the device device@1 in
the end (maybe it's there and you just need to point me
to the right patch).

The reason why we have to be careful here is that the
Linux device hierarchy is not just derived from DT here, but
it gets created from the physical devices under &usb1
and the 'struct device' hierarchy has to match the DT hierarchy
exactly.

      Arnd
--
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 usb-next v5 1/3] dt-bindings: usb: add the documentation for USB root-hub

[Martin Blumenstingl]

Hi Arnd,

thank you for taking 10 minutes to discuss this in person with me!

On Fri, Oct 20, 2017 at 12:10 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thu, Oct 19, 2017 at 11:25 PM, Martin Blumenstingl
> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>>> Does dwc2 also use separate nodes for the roothub? From your
>>> description it sounds like it would not be affected by your patch.
>> currently it doesn't use separate notes for the roothub - however,
>> with this patch it could (although I haven't explicitly tested this)
>
> Ok.
>
>>> Since you used a dtb that already listed an endpoint device below
>>> an xhci, that would answer my earlier question of whether it worked
>>> before your patch series, and you tested that it still works with your
>>> patches applied and the roothub node added in the dtb.  Now we
>>> just need to make sure we don't break existing dtb files that don't
>>> have the roothub node but do have endpoint device nodes.
>> the endpoint you're seeing is the root-hub - you can see the full .dts here: [0]
>>
>> maybe my patch description or documentation is not clear - could you
>> please explain what makes you think that specifying the root-hub
>> didn't work before (so I can update the comments where needed)?
>> to sum up what this series does: find the node with reg = <0>; (= the
>> root-hub) and get all PHY instances from the child-nodes
>> this should not change any existing behavior except if someone had a
>> node with reg = <0>; below any USB controller node (which was
>> undocumented behavior before my patches)
>
> Maybe I misunderstand what the actual change to the hierarchy
> is. Quoting from your example for the new code
>
> +       &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>;
> +                               phys = <&usb2_phy1>, <&usb3_phy1>;
> +                               phy-names = "usb2-phy", "usb3-phy";
> +                       };
>
> The way I understand it, an endpoing device would now be
> located in
>
>  &usb1/roothub@0/port@1/hub@2/device@1
>
> where previously it was in
>
>  &usb1/port@1/hub@2/device@1
>
> Is that correct?
this is not how it's currently implemented - let's find out if this is
a misunderstanding on my side
the second example is still valid with this series, while the first
example won't work with the code as it is

> I don't see any code that can deal with both cases and still
> assign the correct of_node pointer to the device device@1 in
> the end (maybe it's there and you just need to point me
> to the right patch).
it's not you - that code is (currently) not there

> The reason why we have to be careful here is that the
> Linux device hierarchy is not just derived from DT here, but
> it gets created from the physical devices under &usb1
> and the 'struct device' hierarchy has to match the DT hierarchy
> exactly.
yes, I fully agree with you here
I will post an updated version of this series which includes an
updated usb-xhci dt-binding documentation to show how the end result
(after this series) can look like. I'll include Rob in the loop again
so we don't introduce a broken binding.


Regards,
Martin
--
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 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 <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 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 000000000000..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 <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.h>
+#include <linux/usb/hcd.h>
+#include <linux/usb/of.h>
+
+#include "phy.h"
+
+#define ROOTHUB_PORTNUM		0
+
+struct usb_phy_roothub {
+	struct phy		*phy;
+	struct list_head	list;
+};
+
+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(&roothub_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(&roothub_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",
+					      &phy_roothub->list);
+		if (err)
+			goto err_out;
+
+		err = usb_phy_roothub_add_phy(dev, port_np, "usb3-phy",
+					      &phy_roothub->list);
+		if (err)
+			goto err_out;
+	}
+
+	head = &phy_roothub->list;
+
+	list_for_each_entry(roothub_entry, head, list) {
+		err = phy_init(roothub_entry->phy);
+		if (err)
+			goto err_exit_phys;
+	}
+
+	return phy_roothub;
+
+err_exit_phys:
+	list_for_each_entry_continue_reverse(roothub_entry, head, list)
+		phy_exit(roothub_entry->phy);
+
+err_out:
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
+
+int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub)
+{
+	struct usb_phy_roothub *roothub_entry;
+	struct list_head *head;
+	int err, ret = 0;
+
+	if (!phy_roothub)
+		return 0;
+
+	head = &phy_roothub->list;
+
+	list_for_each_entry(roothub_entry, head, list) {
+		err = phy_exit(roothub_entry->phy);
+		if (err)
+			ret = ret;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_exit);
+
+int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub)
+{
+	struct usb_phy_roothub *roothub_entry;
+	struct list_head *head;
+	int err;
+
+	if (!phy_roothub)
+		return 0;
+
+	head = &phy_roothub->list;
+
+	list_for_each_entry(roothub_entry, head, list) {
+		err = phy_power_on(roothub_entry->phy);
+		if (err)
+			goto err_out;
+	}
+
+	return 0;
+
+err_out:
+	list_for_each_entry_continue_reverse(roothub_entry, head, list)
+		phy_power_off(roothub_entry->phy);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_power_on);
+
+int usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
+{
+	struct usb_phy_roothub *roothub_entry;
+	int err, ret = 0;
+
+	if (!phy_roothub)
+		return 0;
+
+	list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list) {
+		err = phy_power_off(roothub_entry->phy);
+		if (err)
+			ret = err;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
new file mode 100644
index 000000000000..aa840dbd078a
--- /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);
-- 
2.14.2

--
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 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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]

On Sun, Oct 8, 2017 at 11:17 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC usb-next v5 3/3] usb: core: hcd: integrate the PHY roothub wrapper

[Martin Blumenstingl]

This integrates the PHY roothub wrapper into the core hcd
infrastructure. Multiple PHYs which are part of the roothub devicetree
node (which is a sub-node of the sysdev's node) are now managed
(= powered on/off when needed), by the new usb_phy_roothub code.

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>
---
 drivers/usb/core/hcd.c  | 30 +++++++++++++++++++++++++++++-
 include/linux/usb/hcd.h |  1 +
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 67aa3d039b9b..56704dd10c15 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -50,6 +50,7 @@
 #include <linux/usb/otg.h>
 
 #include "usb.h"
+#include "phy.h"
 
 
 /*-------------------------------------------------------------------------*/
@@ -2292,7 +2293,11 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"suspend", status);
 	}
-	return status;
+
+	if (status == 0)
+		return usb_phy_roothub_power_off(hcd->phy_roothub);
+	else
+		return status;
 }
 
 int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
@@ -2307,6 +2312,11 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		dev_dbg(&rhdev->dev, "skipped %s of dead bus\n", "resume");
 		return 0;
 	}
+
+	status = usb_phy_roothub_power_on(hcd->phy_roothub);
+	if (status)
+		return status;
+
 	if (!hcd->driver->bus_resume)
 		return -ENOENT;
 	if (HCD_RH_RUNNING(hcd))
@@ -2344,6 +2354,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		}
 	} else {
 		hcd->state = old_state;
+		usb_phy_roothub_power_off(hcd->phy_roothub);
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"resume", status);
 		if (status != -ESHUTDOWN)
@@ -2780,6 +2791,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
 		}
 	}
 
+	hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
+	if (IS_ERR(hcd->phy_roothub)) {
+		retval = PTR_ERR(hcd->phy_roothub);
+		goto err_phy_roothub_init;
+	}
+
+	retval = usb_phy_roothub_power_on(hcd->phy_roothub);
+	if (retval)
+		goto err_usb_phy_roothub_power_on;
+
 	dev_info(hcd->self.controller, "%s\n", hcd->product_desc);
 
 	/* Keep old behaviour if authorized_default is not in [0, 1]. */
@@ -2944,6 +2965,10 @@ int usb_add_hcd(struct usb_hcd *hcd,
 err_register_bus:
 	hcd_buffer_destroy(hcd);
 err_create_buf:
+	usb_phy_roothub_power_off(hcd->phy_roothub);
+err_usb_phy_roothub_power_on:
+	usb_phy_roothub_exit(hcd->phy_roothub);
+err_phy_roothub_init:
 	if (IS_ENABLED(CONFIG_GENERIC_PHY) && hcd->remove_phy && hcd->phy) {
 		phy_power_off(hcd->phy);
 		phy_exit(hcd->phy);
@@ -3028,6 +3053,9 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 	usb_deregister_bus(&hcd->self);
 	hcd_buffer_destroy(hcd);
 
+	usb_phy_roothub_power_off(hcd->phy_roothub);
+	usb_phy_roothub_exit(hcd->phy_roothub);
+
 	if (IS_ENABLED(CONFIG_GENERIC_PHY) && hcd->remove_phy && hcd->phy) {
 		phy_power_off(hcd->phy);
 		phy_exit(hcd->phy);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index a1f03ebfde47..6915b2afc209 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -103,6 +103,7 @@ struct usb_hcd {
 	 */
 	struct usb_phy		*usb_phy;
 	struct phy		*phy;
+	struct usb_phy_roothub	*phy_roothub;
 
 	/* Flags that need to be manipulated atomically because they can
 	 * change while the host controller is running.  Always use
-- 
2.14.2

--
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 usb-next v5 3/3] usb: core: hcd: integrate the PHY roothub wrapper

[Alan Stern]

On Sun, 8 Oct 2017, Martin Blumenstingl wrote:

> This integrates the PHY roothub wrapper into the core hcd
> infrastructure. Multiple PHYs which are part of the roothub devicetree
> node (which is a sub-node of the sysdev's node) are now managed
> (= powered on/off when needed), by the new usb_phy_roothub code.
> 
> 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>
> ---
>  drivers/usb/core/hcd.c  | 30 +++++++++++++++++++++++++++++-
>  include/linux/usb/hcd.h |  1 +
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 67aa3d039b9b..56704dd10c15 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -50,6 +50,7 @@
>  #include <linux/usb/otg.h>
>  
>  #include "usb.h"
> +#include "phy.h"
>  
>  
>  /*-------------------------------------------------------------------------*/
> @@ -2292,7 +2293,11 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"suspend", status);
>  	}
> -	return status;
> +
> +	if (status == 0)
> +		return usb_phy_roothub_power_off(hcd->phy_roothub);

Is this really the right thing to do?  If usb_phy_roothub_power_off() 
fails, what condition does this leave the bus in?  And what condition 
does the kernel _think_ the bus is in?

Alan Stern

--
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 usb-next v5 3/3] usb: core: hcd: integrate the PHY roothub wrapper

[Martin Blumenstingl]

Hi Alan,

On Mon, Oct 9, 2017 at 7:18 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Sun, 8 Oct 2017, Martin Blumenstingl wrote:
>
>> This integrates the PHY roothub wrapper into the core hcd
>> infrastructure. Multiple PHYs which are part of the roothub devicetree
>> node (which is a sub-node of the sysdev's node) are now managed
>> (= powered on/off when needed), by the new usb_phy_roothub code.
>>
>> 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>
>> ---
>>  drivers/usb/core/hcd.c  | 30 +++++++++++++++++++++++++++++-
>>  include/linux/usb/hcd.h |  1 +
>>  2 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 67aa3d039b9b..56704dd10c15 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -50,6 +50,7 @@
>>  #include <linux/usb/otg.h>
>>
>>  #include "usb.h"
>> +#include "phy.h"
>>
>>
>>  /*-------------------------------------------------------------------------*/
>> @@ -2292,7 +2293,11 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>>               dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>>                               "suspend", status);
>>       }
>> -     return status;
>> +
>> +     if (status == 0)
>> +             return usb_phy_roothub_power_off(hcd->phy_roothub);
>
> Is this really the right thing to do?  If usb_phy_roothub_power_off()
> fails, what condition does this leave the bus in?  And what condition
> does the kernel _think_ the bus is in?
indeed, thank you for spotting this!

do you have any suggestions how to improve this?
maybe I should move usb_phy_roothub_power_off a few lines up and only
call it after the "rhdev->do_remote_wakeup" block if status is 0. if
usb_phy_roothub_power_off then returns an error I could call
"hcd_bus_resume(rhdev, PMSG_AUTO_RESUME);". what do you think about
this?

> Alan Stern
>


Regards,
Martin
--
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 usb-next v5 3/3] usb: core: hcd: integrate the PHY roothub wrapper

[Alan Stern]

On Thu, 12 Oct 2017, Martin Blumenstingl wrote:

> Hi Alan,
> 
> On Mon, Oct 9, 2017 at 7:18 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> > On Sun, 8 Oct 2017, Martin Blumenstingl wrote:
> >
> >> This integrates the PHY roothub wrapper into the core hcd
> >> infrastructure. Multiple PHYs which are part of the roothub devicetree
> >> node (which is a sub-node of the sysdev's node) are now managed
> >> (= powered on/off when needed), by the new usb_phy_roothub code.
> >>
> >> 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>
> >> ---
> >>  drivers/usb/core/hcd.c  | 30 +++++++++++++++++++++++++++++-
> >>  include/linux/usb/hcd.h |  1 +
> >>  2 files changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >> index 67aa3d039b9b..56704dd10c15 100644
> >> --- a/drivers/usb/core/hcd.c
> >> +++ b/drivers/usb/core/hcd.c
> >> @@ -50,6 +50,7 @@
> >>  #include <linux/usb/otg.h>
> >>
> >>  #include "usb.h"
> >> +#include "phy.h"
> >>
> >>
> >>  /*-------------------------------------------------------------------------*/
> >> @@ -2292,7 +2293,11 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
> >>               dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
> >>                               "suspend", status);
> >>       }
> >> -     return status;
> >> +
> >> +     if (status == 0)
> >> +             return usb_phy_roothub_power_off(hcd->phy_roothub);
> >
> > Is this really the right thing to do?  If usb_phy_roothub_power_off()
> > fails, what condition does this leave the bus in?  And what condition
> > does the kernel _think_ the bus is in?
> indeed, thank you for spotting this!
> 
> do you have any suggestions how to improve this?
> maybe I should move usb_phy_roothub_power_off a few lines up and only
> call it after the "rhdev->do_remote_wakeup" block if status is 0. if
> usb_phy_roothub_power_off then returns an error I could call
> "hcd_bus_resume(rhdev, PMSG_AUTO_RESUME);". what do you think about
> this?

Or you could just throw away the return code from 
usb_phy_roothub_power_off().  Maybe print it out in a warning message, 
but do not report it to the caller.

After all, given the choice between leaving the entire USB bus at full 
power and leaving just the phy at full power, which would you prefer?

Alan Stern

--
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 usb-next v5 3/3] usb: core: hcd: integrate the PHY roothub wrapper

[Martin Blumenstingl]

On Fri, Oct 13, 2017 at 4:07 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Thu, 12 Oct 2017, Martin Blumenstingl wrote:
>
>> Hi Alan,
>>
>> On Mon, Oct 9, 2017 at 7:18 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
>> > On Sun, 8 Oct 2017, Martin Blumenstingl wrote:
>> >
>> >> This integrates the PHY roothub wrapper into the core hcd
>> >> infrastructure. Multiple PHYs which are part of the roothub devicetree
>> >> node (which is a sub-node of the sysdev's node) are now managed
>> >> (= powered on/off when needed), by the new usb_phy_roothub code.
>> >>
>> >> 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>
>> >> ---
>> >>  drivers/usb/core/hcd.c  | 30 +++++++++++++++++++++++++++++-
>> >>  include/linux/usb/hcd.h |  1 +
>> >>  2 files changed, 30 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> >> index 67aa3d039b9b..56704dd10c15 100644
>> >> --- a/drivers/usb/core/hcd.c
>> >> +++ b/drivers/usb/core/hcd.c
>> >> @@ -50,6 +50,7 @@
>> >>  #include <linux/usb/otg.h>
>> >>
>> >>  #include "usb.h"
>> >> +#include "phy.h"
>> >>
>> >>
>> >>  /*-------------------------------------------------------------------------*/
>> >> @@ -2292,7 +2293,11 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
>> >>               dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>> >>                               "suspend", status);
>> >>       }
>> >> -     return status;
>> >> +
>> >> +     if (status == 0)
>> >> +             return usb_phy_roothub_power_off(hcd->phy_roothub);
>> >
>> > Is this really the right thing to do?  If usb_phy_roothub_power_off()
>> > fails, what condition does this leave the bus in?  And what condition
>> > does the kernel _think_ the bus is in?
>> indeed, thank you for spotting this!
>>
>> do you have any suggestions how to improve this?
>> maybe I should move usb_phy_roothub_power_off a few lines up and only
>> call it after the "rhdev->do_remote_wakeup" block if status is 0. if
>> usb_phy_roothub_power_off then returns an error I could call
>> "hcd_bus_resume(rhdev, PMSG_AUTO_RESUME);". what do you think about
>> this?
>
> Or you could just throw away the return code from
> usb_phy_roothub_power_off().  Maybe print it out in a warning message,
> but do not report it to the caller.
phy_power_off is already printing a warning message for each PHY that
failed to turn off

> After all, given the choice between leaving the entire USB bus at full
> power and leaving just the phy at full power, which would you prefer?
I see, let's keep it simple for now and power off the bus (the code
can still be updated later on if real world shows that we're hitting a
problem here)

as you suggested I'll make it void in the next version - thanks for that!


Regards,
Martin
--
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