[PATCH v2 0/7] musb: sunxi: Add support for run-time changing dr-mode through sysfs

[b-liu@ti.com (Bin Liu)]

Hi,

On Mon, Aug 22, 2016 at 05:50:25PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 22-08-16 16:08, Bin Liu wrote:
> >Hi,
> >
> >On Sun, Aug 21, 2016 at 11:29:02AM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 19-08-16 23:25, Bin Liu wrote:
> >>>Hi,
> >>>
> >>>On Mon, Aug 15, 2016 at 09:21:25PM +0200, Hans de Goede wrote:
> >>>>Hi All,
> >>>>
> >>>>Here is a patch series which implements run-time changing the dr-mode
> >>>>of sunxi musb controllers through the (already existing) musb "mode"
> >>>>sysfs attribute.
> >>>>
> >>>>This is useful on boards where there is no id pin, e.g. some tv-boxes
> >>>>use the musb controller to get an extra usb A port without needing
> >>>>a hub chip. Except for the missing id pin when using a usb A<->A cable
> >>>>these ports can do peripheral mode just fine. This series makes it
> >>>>possible to do e.g. this by doing echo "peripheral" > mode before
> >>>>plugging in the usb A<->A cable.
> >>>
> >>>Well, this is an illegal usecase. A-A cable is invalid by USB Spec.
> >>
> >>Yet they exist and there are USB devices (e.g. harddisk enclosures)
> >>which use a USB-A connector even though they are a device not
> >>a host. I own several such devices myself. I agree that this is
> >>wrong but these devices exist, typical case of reality trumping
> >>the spec.
> >
> >I know those products exist, but they are different from yours. Those
> >existing devices are usb peripheral-only, which never source vbus so do
> >not hurt anything other than violate the USB Spec. But your usecase
> >allows the device switching to host mode, which could damage the usb
> >host on the other end of the connection due to vbus contention.
> 
> Actually the sun4i phy driver avoids this by checking vbus-detect for
> external vbus presence before enabling the boards own Vbus.
> 
> More importantly, not supporting peripheral mode on musb attached to an
> USB-A receptacle will not stop users from trying to use it and insert
> an A<->A cable at which point the damage is already done.
> 
> I understand why you dislike this, but we cannot stop a user
> from trying this. So it is better to actually be prepared for this
> (e.g. do the vbus-detect check we're already doing) then it is
> to not support this.
> 
> For example:
> 
> Lets say we do not have vbus-detect and the user plugs in
> an A<->A cable causing Vbus being driven from 2 sides.
> 
> Not good, now we have current flowing between the 2
> power supplies and things are heating up.
> 
> Now there are 2 possible follow-up scenarios:
> 
> 1) We support peripheral mode, the user does
> "echo peripheral > mode" in sysfs and we disable
> the boards Vbus, no more current, things are cooling
> down again before components burn up.
> 
> 2) We do not support peripheral mode (which is what you're
> suggesting), the user does "echo peripheral > mode" in sysfs,
> nothing happens. Now we can only happen the user unplugs
> the A<->A cable soon, instead of trying a bunch of other things.
> 
> Neither is ideal, but do you see how not allowing peripheral
> mode is not helping here ? As soon as the user plugs in
> an A<->A cable bad things may happen, we cannot avoid that
> from the software side.
> 
> >>>With type-A receptacle the controller should be in host-only mode,
> >>>switching to peripheral mode should not be allowed.
> >>
> >>Peripheral mode can still be quite useful, e.g. to do ethernet
> >>over USB (some of these devices lack wired ethernet).
> >
> >For such usecase, a micro-AB connector should be used, or use two
> >connectors - type-A and type-B which are mux'd to the same usb port.
> >
> >>
> >>Also the manual of these devices typically instructs users to
> >>use an A<->A cable for firmware updates, since the bootROM in
> >
> >(Nobody reads the manual, we all know it...) This does not prevent users
> >to connect two hosts together then damage the hardware. So this design
> >should not be allowed.
> 
> Nothing prevents an user from doing that, as soon as they have
> an A<->A cable they can easily do that, one can only hope they
> will not do it.
> 
> >>the SoC does not care anmd will happily put the device in
> >>Allwinner's custom DFU mode using the USB-A connector.
> >>
> >>Although I agree with this being not ideal, the fact is that
> >>an USB-A connector combined with a A<->A cable is electrically
> >>100% identical to a USB-B connector with its id-pin not connected
> >>(on the PCB side), which also happens see below.
> >
> >they are two different cables. A-B cable does not provide a chance to
> >connect two hosts together, but A-A cable does.
> 
> Right, but an A-B cable with its id-pin wrongly connected to ground
> (some so called otg charging hubs do this) will cause the exact same issue.
> 
> This is why usb-ports and especially otg ports should have current limiting
> on their Vbus and why the sun4i phy driver checks vbus-detect not reporting
> an external vbus before enabling Vbus.
> 
> >>>>This series has both sun4i-usb-phy driver and sunxi-musb-glue changes,
> >>>>both are necessary for the run-time changing to work, but they can be
> >>>>merged independently without breaking anything.
> >>>>
> >>>>Changed in v2:
> >>>>
> >>>>After sleeping on it a night I realized that always passing port_mode =
> >>>>DUAL_ROLE to the musb-core was wrong. There is a distintion between
> >>>>the id-pin not working properly which we can work around with software
> >>>>mode switching (and we want to register both the host and udc driver
> >>>>in this case) vs cases where we really only want to register a host
> >>>>(wifi module connected to musb soldered onto the PCB).
> >>>>
> >>>>So v2 of this series drops the
> >>>>"musb: sunxi: Always register both host and udc when build with dual-role support"
> >>>>patch.
> >>>>
> >>>>Instead systems which are dual-role capable, but lack an id-pin should use
> >>>>dr_mode = "otg" in the dts file. There is one problem with this, some
> >>>>systems are dual-role capable but use a female USB-A connector connected
> >>>>to the musb controller. These should come up in host mode by default,
> >>>
> >>>This is not a problem. With a type-A connector, the dual-role controller
> >>>should work in host-only mode.
> >>>Role switching should only be allowed if an AB connector is used.
> >>
> >>Yet people may want to use it in peripheral mode, I strongly believe
> >>that we should not be telling people what they can and cannot do
> >>with their hardware. That is policy and the kernel does not set
> >
> >I agree with the policy, but we are responsible to tell people don't do
> >something which is not correct.
> >
> >>such policy, that is up to userspace. OTOH the port should clearly
> >>default to host mode hence the new property.
> >
> >Due to the problem with A-A cable I explained above, A-A cable should
> >not be used in role-switching cases (it should be used at the first
> >place), so this new property is unnecessary.
> >
> >>
> >>>Using the sysfs entry to switch roles for generic purpose is really a
> >>>bad idea, it opens up ton of problems.
> >>
> >>Yet the sysfs entry exists already, and the problems depend on how
> >>you hook it up. I'm merely using the sysfs entry as an id-pin
> >>for cases where there is no id pin hooked up. If you look at the
> >>patches you will see that all that is changed by writing the
> >>sysfs entry is the phy reporting a different id-pin value to
> >>the musb ip. Everything else is handled the same as for any normal
> >>otg mode port.
> >>
> >>>For systems which lack of id-pin should use a discrete circuit (for
> >>>example GPIO) to detect the id pin in the AB receptacle, then the USB
> >>>driver will handle the role switching transparently.
> >>
> >>You're again talking theory here in reality there is hardware where
> >>for whatever reasons the PCB uses a mini or micro usb receptacle,
> >>but they did not bother to _physically_ hook up the id pin.
> >
> >Who are they?
> 
> One example of such a device is the quite popular mk802 tv stick
> (the original version) with A10s SoC.
> 
> > the manufactures who use musb controller to make products,
> >so as Linux or musb drivers, we have control, it is our responsibility
> >to tell those manufactures to do things in the right way - design the hw
> >correctly at the first place. We can't compromise by using an A-A cable
> >to provide a change to damage the usb host.
> >
> >This is different from the cases, for example, from the xhci controller
> >perspective, the xhci driver has to compromise any usb thumb drives
> >which do not follow the USB Specs, to make them work on PCs.
> 
> That makes no sense, why would the xhci driver have to compromise
> but we don't ? Either actually having things working is more important
> then following the spec or the spec is more important...
> 
> And in reality as your xhci example points out having things
> working (and that means offering all possible functionality) is
> more important.
> 
> >>As said before reality trumps the Spec / theory.
> >>
> >>>>rather then peripheral mode which is the default for systems which lack an
> >>>>id-pin. This patch set introduces:
> >>>>
> >>>>"phy-sun4i-usb: Add "allwinner,usb0-usb-a-connector" dt property"
> >>>>
> >>>>Which allows specifying the use of a female USB-A connector for the
> >>>>musb controller in the phy dt node, the presence of this dt property
> >>>>changes the default to host mode.
> >>>
> >>>This is unnecessary, if using a type-A connector, dr_mode should be
> >>>"host" in DT.
> >>
> >>Which means we're telling users what they can and cannot do with
> >>their hardware, which we should not be doing.
> >
> >To summary up my opinion from my lengthy comments above,
> >
> >A-A cable should not be used, especially in role-switching cases, so
> >the DT prop is unnecessary;
> 
> A<_>A cables exist, users will try to plug them in, those are facts.
> 
> So we'd better be prepared to deal with this, denying this scenario
> exists is not helpful, not supporting it is equally unhelpful.

Well, I will stop arguing on this from here. It is up to the DT
maintainer to decide on this DT prop.

> 
> Regards,
> 
> Hans

Regards,
-Bin.