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

[Mark Rutland]

On Wed, Sep 18, 2013 at 03:21:18PM +0100, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Aug 28, 2013 at 05:01:51PM +0100, Mark Rutland wrote:
> > > > 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
> 
> well, it is :-)
> 
> > 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.
> 
> right, but what 'brings up' the PHY is usb_phy_init(). If we don't add
> usb3phy to DTS or skip usb3phy in case maximum-speed < superspeed, then
> we're screwed :-)

:(

> 
> > > 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...
> 
> hmm, the idea of the patch was:
> 
> 	switch (maximum-speed) {
> 	case SUPER:
> 		grab_and_initialize_usb3_phy();
> 		grab_and_initialize_usb2_phy();
> 		break;
> 	case HIGH:
> 	case FULL:
> 	case LOW:
> 		grab_and_initialize_usb2_phy();
> 		break;
> 	}
> 
> now, imagine someone wants to run his dwc3 in highspeed mode, we
> wouldn't initialize USB3 PHY which could cause the whole IP to break.

When you say wants to run it in highspeed mode, you mean that they want
this configured at run-time, or they deliberately omit a phy when
describing the hardware in the DT?

For the former I appreciate that it's a problem, but for the latter I'd
argue that's their fault. As far as I can see we initialise both PHYs in
the probe path and never uninitialise them, so the only problem would be
if someone's trying to be too clever. As we never check the return value
of usb_phy_init, they can still attempt to work around our best
efforts...

I appreciate that we should not break existing DTs. More on that below.

> 
> I tried poking around on device's registers to see if there was any way
> to detect that the IP needs somethin back from USB3 PHY, but couldn't
> find anything.
> 
> Since we can't know how the IP was integrated, it's best to leave it
> alone and require NOP xceiv to be used.

Agreed for the existing systems, but I still think we can work around
this for new DTs...

> 
> > > > 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?
> 
> you're missing the point regarding the IP possibly needing something
> back from the PHY (e.g. a clock which PHY generates).

I'm not sure why that detracts from the usb-phy-names idea -- if not
present, we stick with the current behaviour and require both PHYs or
fail early. No existing dts suddently explode, though none gain new
functionality either.

If someone's explicitly placed usb-phy-names, we know that they're
up-to-date on the lastest binding, something like:

 - usb-phy: A list of phandles to PHYs. If usb-phy-names is not
	    present, the USB2/HS PHY should be first, followed by the
	    USB3/SS PHY. If usb-phy-names is present, it defines the
	    order of PHYs.

 - usb-phy-names: The names of PHYs described in the usb-phy property.
		  Valid values are "usb2" and "usb3". Should be used iff
		  a subset of PHYs are connected.

		  Compatibility note: The DWC3 IP can be integrated in
		  such a way as to require outputs from particular PHYs
		  for *any* level of operation. This cannot be detected
		  by the OS, and on such systems all required PHYs must
		  be described.

Given that, if they list fewer PHYs than present and their system really
requires a particular PHY, we can quite happily allow their system to
explode in the knowledge it's their fault, not ours :)

If they try to use their new DT on an old platform then the kernel will
refuse to use the DWC3, which would currently be the case anyway.

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/