Re: [PATCH] usb: dwc3: core: clarify usb-phy array binding

[Mark Rutland]

On Tue, Aug 27, 2013 at 07:53:29PM +0100, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Aug 13, 2013 at 02:34:10PM +0100, Mark Rutland wrote:
> > On Mon, Aug 12, 2013 at 07:05:53PM +0100, Felipe Balbi wrote:
> > > On Fri, Aug 09, 2013 at 01:42:15PM -0500, Kumar Gala wrote:
> > > > 
> > > > On Aug 9, 2013, at 11:28 AM, Mark Rutland wrote:
> > > > 
> > > > > On Fri, Aug 09, 2013 at 04:40:32PM +0100, Kumar Gala wrote:
> > > > >> The binding spec wasn't clear that the order of the phandles in the
> > > > >> usb-phy array has meaning.  Clarify this point in the binding that
> > > > >> it should be <USB2-HS-PHY, USB3-SS-PHY>.
> > > > >> 
> > > > >> Signed-off-by: Kumar Gala <galak@xxxxxxxxxxxxxx>
> > > > >> ---
> > > > >> Documentation/devicetree/bindings/usb/dwc3.txt | 4 +++-
> > > > >> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >> 
> > > > >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > >> index 7a95c65..8a9770b 100644
> > > > >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > >> @@ -6,7 +6,9 @@ Required properties:
> > > > >>  - compatible: must be "synopsys,dwc3"
> > > > >>  - reg : Address and length of the register set for the device
> > > > >>  - interrupts: Interrupts used by the dwc3 controller.
> > > > >> - - usb-phy : array of phandle for the PHY device
> > > > >> + - usb-phy : array of phandle for the PHY device.  The first element
> > > > >> +   in the array is expected to be a handle to the USB2/HS PHY and
> > > > >> +   the second element is expected to be a handle to the USB3/SS PHY
> > > > > 
> > > > > Just to check - it's not valid to have a USB3 phy without a USB2 phy?
> > > > 
> > > > Don't know, hopefully Felipe will chime in on that.
> > > 
> > > that'd be a really non-standard implementation. Per-spec, USB3 is
> > > *always* backwards compatible with USB2.
> > 
> > I'm slightly confused here. From a quick look at the driver, it expects
> > two separate phys to be present -- one handling only USB2, and one
> > handling USB3 (with USB2 backwards compatibility).
> > 
> > So it's not physically possible for someone to just wire up a single phy
> > to the device, either USB2-only or USB3?
> 
> of course it is :-) In fact, TI has done it. But it causes a whole bunch
> of other problems to support that sort of model. For one, in some
> systems, a clock generated by the USB3 PHY is backfed into the IP and if
> USB3 PHY isn't running, the dwc3 IP won't start.

That sounds like a mess. But unless I've misunderstood, that doesn't
sound like a general problem with having one phy, but rather an
integration issue on a specific system? Presumably in that case as long
as the phy was brought up before poking the rest of the IP, the unit
would function correctly.

> 
> I even wrote a patch making USB3 PHY optional, but didn't push it
> exactly because it broke some other systems and I can't guarantee users
> won't mess up their DTS/pdata.

Does that mean that their dts or pdata are wrong at the moment, but
they're spared because the driver bails out due to a missing phy? If
their pdata's wrong, that should be fixable relatively easily, but if
the dt is wrong then I'm not sure how we can support those systems
sensibly. That sounds like an ideal candidate for a dt quirks
mechanism...

> 
> > You can describe the USB2-only case in the dt by only listing the first
> > phy (though the driver won't support it as it expects both to be
> > present), but it's impossible to describe that you've wired up a single
> > phy that's USB3 capable.
> 
> you might be right there...

Would it be possible to have something like (an optional) usb-phy-names?
If not present, we assume the current situation, if present then we use
it to figure out which phys are present. Maybe I'm missing something
that makes that impossible?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/