[v3,00/10] A3700 USB S2RAM support

Hello,

As part of an effort to bring suspend to RAM support to Armada 3700
SoCs (main target: ESPRESSObin), this series handles the work around
the USB2 and USB32 IPs.

First, a change in the core adds support for the new PHY framework by
following the phy_set_mode()/phy_power_on() sequence (patch 1). The
second change needed is to update the xHCI MVEBU driver (patch 2) with
the XHCI_RESET_ON_RESUME quirk that will do most of the
reconfiguration work when resuming.

Then, because of the asynchronous suspend feature implemented lately,
the xHCI driver was blocking during the S2RAM operation, probably due
to a register access while the clock was not enabled. A first patch
has been written for that, adding a new quirk to ignore the
asynchronous setting [1] which has become useless once clock links
with consumer have been contributed [2] (series not merged yet).

Then, the OHCI Orion (USB2 host controller) driver is updated to avoid
doing twice the PHY management (patch 3) and upgraded with traditional
S2RAM callbacks (patch 4).

The last missing peace is a UTMI PHY driver for the USB2 part of each
controller, which is added in patch 5 (see patch 6 for the bindings).

Finally, the A3700 device tree is updated (patch 7, 8, 9) to reference
the PHYs. xHCI bindings already document the PHY so no update is needed
on this regard.

[1] http://code.bulix.org/s2ccd4-511198
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/623885.html

Thanks,
Miquèl

Changes since v2:
=================
* Typos
* UTMI PHY bindings:
  > New name for the USB2 host/PHY syscons (+ changes in the DT).
  > Clearly state that the PHYs are different between OTG and
    host-only controllers.
* UTMI PHY driver:
  > Use GENMASK macro.
  > Removed useless checks on driver data retrieval.
  > Dropped the empty ->remove() hook.
  > Updated the licence macro to match the SPDX tag.
  > Used the of_device_get_match_data() helper.
  > Removed the use of a local variable in the ->probe() for the
    driver's operations.
  > Fixed the driver co-author: Igal, not Evan.
* xHCI mvebu driver:
  > Used Armada 38x naming convention:
    s/Armada 3700 SoC/Armada 37xx SoC/
  > Added Gregory's Reviewed-by tag.

Changes since v1:
=================
* Add UTMI PHY driver/bindings/DT changes to bring S2RAM support to
  both USB ports available on the A3700 SoC.
* Updates to the OHCI Orion driver to avoid doing the PHY
  initialization twice.
* Upgrade of the OHCI Orion driver with S2RAM callbacks.
* Added a reference to the A3700 SoC in the USB Host Kconfig prompt
  (not only in the help section).
* Rebased on top of v5.0-rc1.


Miquel Raynal (9):
  usb: core: comply to PHY framework
  usb: ehci-orion: avoid double PHY initialization
  usb: ehci-orion: add S2RAM support
  phy: add A3700 UTMI PHY driver
  dt-bindings: phy: mvebu-utmi: add UTMI PHY bindings
  MAINTAINERS: phy: fill Armada 3700 PHY drivers entry
  ARM64: dts: marvell: armada-37xx: fix USB2 memory region
  ARM64: dts: marvell: armada-37xx: declare USB2 UTMI PHYs
  ARM64: dts: marvell: armada-37xx: link USB hosts with their PHYs

Ofer Heifetz (1):
  usb: host: xhci: mvebu: add reset on resume quirk

 .../bindings/phy/phy-mvebu-utmi.txt           |  38 +++
 MAINTAINERS                                   |   4 +-
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |  34 ++-
 drivers/phy/marvell/Kconfig                   |   9 +
 drivers/phy/marvell/Makefile                  |   1 +
 drivers/phy/marvell/phy-mvebu-a3700-utmi.c    | 278 ++++++++++++++++++
 drivers/usb/core/hcd.c                        |   5 +
 drivers/usb/core/phy.c                        |  28 ++
 drivers/usb/core/phy.h                        |   2 +
 drivers/usb/host/Kconfig                      |   4 +-
 drivers/usb/host/ehci-orion.c                 |  51 ++--
 drivers/usb/host/xhci-mvebu.c                 |  11 +
 drivers/usb/host/xhci-mvebu.h                 |   6 +
 drivers/usb/host/xhci-plat.c                  |   7 +
 14 files changed, 451 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
 create mode 100644 drivers/phy/marvell/phy-mvebu-a3700-utmi.c

On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:
> Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Hi Rob,
 
 On lun., janv. 21 2019, Rob Herring <robh@kernel.org> wrote:

> On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:
>> Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.
>> 
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
>>

I found "marvell,usb2-host-miscellaneous" too generic, I would prefer
that we call it "marvell,armada-3700-usb2-host-miscellaneous" as we do
for all the other IP.

Gregory

>
> Reviewed-by: Rob Herring <robh@kernel.org>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Jan 21, 2019 at 9:49 AM Gregory CLEMENT
<gregory.clement@bootlin.com> wrote:
>
> Hi Rob,
>
>  On lun., janv. 21 2019, Rob Herring <robh@kernel.org> wrote:
>
> > On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:
> >> Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.
> >>
> >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >> ---
> >>  .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
> >>  1 file changed, 38 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
> >>
>
> I found "marvell,usb2-host-miscellaneous" too generic, I would prefer
> that we call it "marvell,armada-3700-usb2-host-miscellaneous" as we do
> for all the other IP.

Ah yes, you are right. Perhaps just '-misc' to shorten it a little bit.

Rob

Hello,

Rob Herring <robh@kernel.org> wrote on Tue, 22 Jan 2019 10:08:41 -0600:

> On Mon, Jan 21, 2019 at 9:49 AM Gregory CLEMENT
> <gregory.clement@bootlin.com> wrote:
> >
> > Hi Rob,
> >
> >  On lun., janv. 21 2019, Rob Herring <robh@kernel.org> wrote:
> >  
> > > On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:  
> > >> Add bindings for Marvell Armada 3700 USB2 UTMI+ PHY.
> > >>
> > >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > >> ---
> > >>  .../bindings/phy/phy-mvebu-utmi.txt           | 38 +++++++++++++++++++
> > >>  1 file changed, 38 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/phy/phy-mvebu-utmi.txt
> > >>  
> >
> > I found "marvell,usb2-host-miscellaneous" too generic, I would prefer
> > that we call it "marvell,armada-3700-usb2-host-miscellaneous" as we do
> > for all the other IP.  
> 
> Ah yes, you are right. Perhaps just '-misc' to shorten it a little bit.
> 
> Rob

Sure, no problem.

I will wait for other comments on the rest of the series and then I
will update the bindings/DT patch. Perhaps if I don't get more
feedback I will just resend the concerned patches (6/10 and 9/10).


Thanks,
Miquèl

Hello,

On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:

> +- compatible: Should be one of:
> +	      * "marvell,a3700-utmi-host-phy" for the PHY connected to
> +	        the USB2 host-only controller.
> +	      * "marvell,a3700-utmi-otg-phy" for the PHY connected to
> +	        the USB3 and USB2 OTG capable controller.

Do we really need different compatible strings for those ? I assume the
IP block is exactly the same for the two PHYs, right ? If so, we should
use the same compatible string.

Didn't we discuss that we can lookup from the PHY whether the USB
controller it is connected to is in host mode or otg mode ?

> +Example:
> +
> +	usb2_utmi_host_phy: phy@5f000 {
> +		compatible = "marvell,armada-3700-utmi-host-phy";
> +		reg = <0x5f000 0x800>;
> +		marvell,usb-misc-reg = <&usb2_syscon>;
> +		#phy-cells = <0>;
> +	};
> +
> +	usb2_syscon: system-controller@5f800 {
> +		compatible = "marvell,usb2-host-miscellaneous", "syscon";
> +		reg = <0x5f800 0x800>;
> +	};

Those registers are contiguous to the register range of the PHY itself.
What was the criteria used to decide that we need two separate DT nodes
for these ?

Best regards,

Thomas

Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Tue, 22 Jan
2019 18:38:33 +0100:

> Hello,
> 
> On Mon, 21 Jan 2019 12:23:32 +0100, Miquel Raynal wrote:
> 
> > +- compatible: Should be one of:
> > +	      * "marvell,a3700-utmi-host-phy" for the PHY connected to
> > +	        the USB2 host-only controller.
> > +	      * "marvell,a3700-utmi-otg-phy" for the PHY connected to
> > +	        the USB3 and USB2 OTG capable controller.  
> 
> Do we really need different compatible strings for those ? I assume the
> IP block is exactly the same for the two PHYs, right ? If so, we should
> use the same compatible string.

For what I understand, the PHYs are different. At least this is how
they are described in the specification. I can list at least two
differences visible in the register set:
* one has OTG registers, the other does not.
* one has charger detection capabilities (and registers), the other
  does not.

> 
> Didn't we discuss that we can lookup from the PHY whether the USB
> controller it is connected to is in host mode or otg mode ?

Yes, but having two different PHYs meant having two different
compatibles, hence there was no need in the driver to use this
lookup approach. Anyway this is an implementation detail and it is
always possible to go back. 

> 
> > +Example:
> > +
> > +	usb2_utmi_host_phy: phy@5f000 {
> > +		compatible = "marvell,armada-3700-utmi-host-phy";
> > +		reg = <0x5f000 0x800>;
> > +		marvell,usb-misc-reg = <&usb2_syscon>;
> > +		#phy-cells = <0>;
> > +	};
> > +
> > +	usb2_syscon: system-controller@5f800 {
> > +		compatible = "marvell,usb2-host-miscellaneous", "syscon";
> > +		reg = <0x5f800 0x800>;
> > +	};  
> 
> Those registers are contiguous to the register range of the PHY itself.
> What was the criteria used to decide that we need two separate DT nodes
> for these ?

Because this area contains a bunch of registers, most of them are
there to manage the PHY, but others are related to the USB controller
(ie. a software reset). I know the current USB controller driver does
not access this area but what if one day we decide to do it?


Thanks,
Miquèl

Hello Miquel,

On Tue, 22 Jan 2019 20:16:35 +0100, Miquel Raynal wrote:

> > Do we really need different compatible strings for those ? I assume the
> > IP block is exactly the same for the two PHYs, right ? If so, we should
> > use the same compatible string.  
> 
> For what I understand, the PHYs are different. At least this is how
> they are described in the specification. I can list at least two
> differences visible in the register set:
> * one has OTG registers, the other does not.
> * one has charger detection capabilities (and registers), the other
>   does not.

OK, then indeed it makes sense to have two different compatible strings.

> > Those registers are contiguous to the register range of the PHY itself.
> > What was the criteria used to decide that we need two separate DT nodes
> > for these ?  
> 
> Because this area contains a bunch of registers, most of them are
> there to manage the PHY, but others are related to the USB controller
> (ie. a software reset). I know the current USB controller driver does
> not access this area but what if one day we decide to do it?

OK, if indeed this register area contains register used both for the
PHY and for something else, your DT representation makes sense.

Thanks for those clarifications!

Thomas