From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754247AbcFHH6a (ORCPT ); Wed, 8 Jun 2016 03:58:30 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:56043 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753815AbcFHH61 (ORCPT ); Wed, 8 Jun 2016 03:58:27 -0400 Subject: Re: [PATCH v10 2/7] usb: mux: add generic code for dual role port mux To: Felipe Balbi , Lu Baolu , Jun Li , Peter Chen References: <1464831449-8973-1-git-send-email-baolu.lu@linux.intel.com> <1464831449-8973-3-git-send-email-baolu.lu@linux.intel.com> <20160603074113.GA30006@shlinux2> <5751AAEE.2090001@linux.intel.com> <20160604022838.GA26936@shlinux2> <5753CCFC.2060504@linux.intel.com> <20160606012557.GA16012@shlinux2> <5754E850.1020707@linux.intel.com> <57552006.7080108@ti.com> <5756999B.9020809@linux.intel.com> <5756C4DB.1050400@ti.com> <87a8ixb15w.fsf@linux.intel.com> <5756D410.30003@ti.com> <874m95avka.fsf@linux.intel.com> CC: Mathias Nyman , Greg Kroah-Hartman , Lee Jones , Heikki Krogerus , Liam Girdwood , Mark Brown , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" From: Roger Quadros Message-ID: <5757D008.3090903@ti.com> Date: Wed, 8 Jun 2016 10:58:00 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <874m95avka.fsf@linux.intel.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="3EPbVAE9oQiunSht6an3Pm4Iv1qFiKBkR" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --3EPbVAE9oQiunSht6an3Pm4Iv1qFiKBkR Content-Type: multipart/mixed; boundary="bJFw4tbqulMKtWAt1eObaAbuMJUt8TjAe" From: Roger Quadros To: Felipe Balbi , Lu Baolu , Jun Li , Peter Chen Cc: Mathias Nyman , Greg Kroah-Hartman , Lee Jones , Heikki Krogerus , Liam Girdwood , Mark Brown , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" Message-ID: <5757D008.3090903@ti.com> Subject: Re: [PATCH v10 2/7] usb: mux: add generic code for dual role port mux References: <1464831449-8973-1-git-send-email-baolu.lu@linux.intel.com> <1464831449-8973-3-git-send-email-baolu.lu@linux.intel.com> <20160603074113.GA30006@shlinux2> <5751AAEE.2090001@linux.intel.com> <20160604022838.GA26936@shlinux2> <5753CCFC.2060504@linux.intel.com> <20160606012557.GA16012@shlinux2> <5754E850.1020707@linux.intel.com> <57552006.7080108@ti.com> <5756999B.9020809@linux.intel.com> <5756C4DB.1050400@ti.com> <87a8ixb15w.fsf@linux.intel.com> <5756D410.30003@ti.com> <874m95avka.fsf@linux.intel.com> In-Reply-To: <874m95avka.fsf@linux.intel.com> --bJFw4tbqulMKtWAt1eObaAbuMJUt8TjAe Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 07/06/16 18:05, Felipe Balbi wrote: >=20 > Hi, >=20 > Roger Quadros writes: >>> I might be able to find some time to implement a proof of concept whi= ch >>> would allow your platforms to get dual-role with code we already have= , >>> but I need DWC3's OTG support which, I'm assuming, you already have := -) >>> >>> If you wanna try something offline, just ping me ;-) I'll be happy to= >>> help. >> >> What you are proposing is a dwc3 only solution. With the otg/dual-role= >> series we are trying to be generic as much as possible. >=20 > Well, if there is a need for that, sure. Take MUSB for instance. It > makes use of nothing of the sorts, because it doesn't have to. >=20 >> Whether controller drivers want to use it or not is upto the driver >> maintainers but we should at least ensure that user space ABI if any, >> is consistent across different implementations. >=20 > Role decisions should not be exposed to userspace unless as debug > feature (using e.g. DebugFS). That should be done either by the HW or > within the kernel. >=20 > If we're discussing userspace ABI here, there's something very wrong > with OTG/DRD layer design. Not really. There can be a need for user space application to control the= port role. Consider Apple carplay for instance or even full OTG support which has user selectable role. >=20 >>>> How are you switching the port mux between host and peripheral? Only= >>>> by sysfs or do you have a GPIO for ID pin as well? >>> >>> depends. Some SoCs have GPIO-controller muxes while some just have mu= x's >>> select signals (one for ID, one for VBUS) mapped on xHCI's address >>> space. >>> >>>> What happens to the gadget controller when the port is muxed to the >>>> host controller? Is it stopped or it continues to run? >>> >>> it continues running, but that's pretty irrelevant for Intel's dual-r= ole >> >> Isn't that unnecessary waste of power? Or you have firmware assisted >> low power mode? >=20 > that's an implementation detail which brings nothing to this discussion= , > right? :-) >=20 > We can, certainly, put the other side to D3. >=20 >>> setup. We have an actual physical (inside the die, though) mux which >>> muxes USB signals to XHCI (not DWC3's XHCI) or to a peripheral-only >>> DWC3. >>> >> >> Probably irrelevant for Intel's dual-role but many platforms that >> share the port can't have device controller running when port is in >> host mode and vice versa. >=20 > but that doesn't mean we need an entire new layer added to the kernel > ;-) >=20 > DWC3 already gives us all the information necessary to make a decision > on which role we should assume. Just consider your options. Here's how > things would look like without any OTG/DRD layer: >=20 > -> DWC3 OTG IRQ > -> readl(OSTS); > -> if (OSTS & BIT(4)) > -> dwc3_host_exit(); __dwc3_gadget_start(); > -> else > -> __dwc3_gadget_stop(); dwc3_host_init(); >=20 > Can you draw something similar for your proposed OTG/DRD layer? What about B_IDLE state? We don't want either peripheral or host to run w= hen no cable is inserted. Have you tested if it works? I'd be happy to test if you can prepare a pa= tch to get dual-role working on dwc3 without the OTG/DRD layer :). >=20 > I remember there were at least two schedule_work(). IIRC it looked > something like below: >=20 > -> DWC3 OTG IRQ > -> readl(OSTS); > -> if (OSTS & BIT(4)) > -> otg_set_mode(PERIPHERAL); > -> schedule_work(); > -> otg_ops->stop_host(); > -> usb_del_hcd(); > -> otg_ops->start_peripheral(); > -> usb_gadget_add_udc(); > -> else > -> otg_set_mode(HOST); > -> schedule_work(); > -> otg_ops->stop_peripheral(); > -> usb_gadget_del_udc(); > -> otg_ops->start_host(); > -> usb_add_hcd(); >=20 > I'm probably missing some steps there. =20 As a user you just need to do this reg =3D read(OSTS); dwc->otg->fsm.id =3D !!(reg & STS_ID); dwc->otg->fsm.b_sess_vld =3D !!(reg &STS_BSESVLD); usb_otg_sync_inputs(dwc->otg); And the layer does the rest. But as I said earlier. I have absolutely no issues if dwc3 doesn't use th= at layer as long as dual-role works on our platforms. >=20 >> So there has to be a central point of control where the respective >> controllers are started/stopped. >=20 > some implementations might need this, yes. DWC3 and MUSB don't seem to > be this type of system. OK. >=20 >> That is the other point we are trying to address with the common >> otg/dual-role code. >> >> Even in the TI dwc3 implementation we use dwc3's XHCI so I guess we ne= ed >> to stop the host controller for device mode, right? >=20 > yes, see above. We already have that code. Just code is not enough. We need to know if it works :). >=20 >> If so then who will deal with start/stop of the controllers then? >=20 > dwc3 itself. >=20 OK. cheers, -roger --bJFw4tbqulMKtWAt1eObaAbuMJUt8TjAe-- --3EPbVAE9oQiunSht6an3Pm4Iv1qFiKBkR Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXV9AMAAoJENJaa9O+djCTuD0P/1JTUIQjIpHM/G4DcQu2Zufm /gO7rsmzx3O+I8mnXQDMGysd3SMdBKLS5wOu2Z4ux09JFpY8z509u5s/65AXcEM3 0siF3IKl+x9igPVLXKCuIq1zcWsIDbR02/izAw2tP9vJWX2QDlI/Ua7tDJ/YfcyJ QMzDi+DFFbdIP7GGJQmjZgaz1aLk09NGwfbgiJGvADH5FYFrk5gVMAOBt+s+c7s3 sN4K4szzoovTBh2t4tc0DSqaE8YAueaGb/ENlaTed4cDjH8Fb+H3LNyAVTQEIyga h+IT1M/yiy3D6SUfW8VuYru525MiLd40WVRl+oxgXZfrtTUd3Yi2AvV/oFQCqCPx 7rj1FJ7Htg17zIDWCXq9NScblyUuGFOhrTL2A6FkQPBKLyauHfDpM3tO3g7I8A4d yMb4P+LwJesQLzN0RGf7ozAmQGdVGJyin/W+xmYPTbZMxw4Vf443z7tlBOLRAxsC DFFtV572lsqsicTH4R6a4qMcATlnKPT8VGQ3O4I8Gx3Rg6SUwLBFfN/C12PC0H2X aBIqJVLlL7ZwTpY8ly+xRX2YaA1UjSthka2pnBt58ONDdCtROngJAEYDxb0oD3QK IOkjV9GMtn4HTDJaTCAHg1RwVxDna6ChNBJIoy8KwQ4dqN4oFWuB4oge5RT+WDQ6 Wj45aD2k7EKtnq8y2ef3 =3b0k -----END PGP SIGNATURE----- --3EPbVAE9oQiunSht6an3Pm4Iv1qFiKBkR--