RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi,

> From: Roger Quadros
> Sent: Monday, April 11, 2016 7:55 PM
> 
> On 08/04/16 14:22, Yoshihiro Shimoda wrote:
> > Hi,
> >
> >> From: Roger Quadros
> >> Sent: Thursday, April 07, 2016 8:45 PM
> >>
> >> Hi,
> >>
> >> On 07/04/16 11:52, Yoshihiro Shimoda wrote:
> >>> Hi,
> >>>
> >>>> From: Roger Quadros
> >>>> Sent: Tuesday, April 05, 2016 11:05 PM
> > < snip >
> >>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> >>> index 41e762a..4d7f043 100644
> >>> --- a/drivers/usb/common/usb-otg.c
> >>> +++ b/drivers/usb/common/usb-otg.c
> >>> @@ -825,11 +825,16 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
> >>>  	if (otg->primary_hcd.hcd) {
> >>>  		/* probably a shared HCD ? */
> >>>  		if (usb_otg_hcd_is_primary_hcd(hcd)) {
> >>> +			if (hcd->driver->flags & HCD_USB11) {
> >>> +				dev_info(otg_dev, "this assumes usb 1.1 hc is as shared_hcd\n");
> >>> +				goto check_shared_hcd;
> >>> +			}
> >>>  			dev_err(otg_dev, "otg: primary host already registered\n");
> >>>  			goto err;
> >>>  		}
> >>>
> >>>  		if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> >>> +check_shared_hcd:
> >>>  			if (otg->shared_hcd.hcd) {
> >>>  				dev_err(otg_dev, "otg: shared host already registered\n");
> >>>  				goto err;
> >>>
> >>> What do you think this local hack?
> >>
> >> Is it guaranteed that EHCI hcd registers before OHCI hcd?
> >
> > Thank you for the comment. No, it is not guaranteed.
> >
> >> If not we need to improve the code still.
> >> We will also need to remove the constraint that primary_hcd must be registered
> >> first in usb_otg_register_hcd(). I think that constraint is no longer
> >> needed anyways.
> >
> > I got it.
> > So, I made a patch to avoid the constraint using an additional property "otg-hcds".
> > The patch is in this end of email. What do you think about the patch?
> 
> This might only solve the issue for device tree but what about non-device tree?
> We need to deal with both cases.

Thank you for the comment. I got it.

> >> If EHCI & OHCI HCDs register before OTG driver then things will break unless
> >> we fix usb_otg_hcd_wait_add(). We need to add this HCD_USB11 check there to
> >> populate wait->shared_hcd.
> >
> > I see. In my environment, since EHCI & OHCI HCDs need phy driver and phy driver
> > will calls usb_otg_register(), the order is right. In other words,
> > EHCI & OHCI HCDs never register before OTG driver because even if EHCI driver
> > is probed first, the driver will be deferred (EHCI driver needs the phy driver).
> 
> But still we cannot assume this is true for all platforms. OTG driver can also
> be a separate entity than PHY driver.

I understood it.

> >>> I also wonder if array of hcd may be good for both xHCI and EHCI/OHCI.
> >>> For example of xHCI:
> >>>  - otg->hcds[0] = primary_hcd
> >>>  - otg->hcds[1] = shared_hcd
> >>>
> >>> For example of EHCI/OHCI:
> >>>  - otg->hcds[0] = primary_hcd of EHCI
> >>>  - otg->hcds[1] = primary_hcd of OHCI
> >>
> >> The bigger problem is that how do we know in the OHCI/EHCI case that we need to wait
> >> for both of them before starting the OTG FSM?
> >> Some systems might use just OHCI or just EHCI.
> >>
> >> There is no guarantee that OTG driver registers before the HCDs so this piece
> >> of information must come from the HCD itself. i.e. whether it needs a companion or not.
> >
> > I understood these problems. I wonder if my patch can resolve these problems.
> >
> > Best regards,
> > Yoshihiro Shimoda
> > ---
> > diff --git a/Documentation/devicetree/bindings/usb/generic.txt b/Documentation/devicetree/bindings/usb/generic.txt
> > index f6866c1..518b9eb 100644
> > --- a/Documentation/devicetree/bindings/usb/generic.txt
> > +++ b/Documentation/devicetree/bindings/usb/generic.txt
> > @@ -27,6 +27,12 @@ Optional properties:
> >   - otg-controller: phandle to otg controller. Host or gadget controllers can
> >  			contain this property to link it to a particular OTG
> >  			controller.
> > + - otg-hcds: phandle to host controller(s). An OTG controller can contain this
> > +	     property. The first one is as "primary", and (optional) the second
> > +	     one is as "shared".
> > +	     - For example for xHCI:		otg-hcds = <&xhci>, <&xhci>;
> > +	     - For example for EHCI/OHCI:	otg-hcds = <&ehci>, <&ohci>;
> > +	     - For example for just EHCI:	otg-hcds = <&ehci>;
> >
> 
> This is kind of duplicating the information. We are already specifying in the
> hcd nodes as to which otg controller it is linked to.
> 
> Instead, how about just adding a boolean property in the otg node saying that
> HCD needs companion. e.g.
> 	hcd-needs-compainion: must be present if otg controller is dealing with
> 			EHCI host controller that needs a companion OHCI host controller.
> 
> This can also be added in struct usb_otg_config so that it can deal with non-DT cases.
> 
> So if this property is true, the OTG core will wait for 2 primary HCDs to be registered.

Thank you for your suggestion. I agree with you.
So, I made such a patch which. It is simple than my previous patch.
What do you think?

Best regards,
Yoshihiro Shimoda

---
 Documentation/devicetree/bindings/usb/generic.txt |  3 +++
 drivers/usb/common/common.c                       |  2 ++
 drivers/usb/common/usb-otg.c                      | 11 +++++++----
 include/linux/usb/otg.h                           |  2 ++
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/generic.txt b/Documentation/devicetree/bindings/usb/generic.txt
index f6866c1..1db1c33 100644
--- a/Documentation/devicetree/bindings/usb/generic.txt
+++ b/Documentation/devicetree/bindings/usb/generic.txt
@@ -27,6 +27,9 @@ Optional properties:
  - otg-controller: phandle to otg controller. Host or gadget controllers can
 			contain this property to link it to a particular OTG
 			controller.
+ - hcd-needs-companion: must be present if otg controller is dealing with
+			EHCI host controller that needs a companion OHCI host
+			controller.
 
 This is an attribute to a USB controller such as:
 
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index e3d0161..8b74715 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -233,6 +233,8 @@ int of_usb_update_otg_caps(struct device_node *np,
 	if (of_find_property(np, "adp-disable", NULL) ||
 				(otg_caps->otg_rev < 0x0200))
 		otg_caps->adp_support = false;
+	if (of_find_property(np, "hcd-needs-companion", NULL))
+		otg_caps->needs_companion = true;
 
 	return 0;
 }
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
index 41e762a..e0df839 100644
--- a/drivers/usb/common/usb-otg.c
+++ b/drivers/usb/common/usb-otg.c
@@ -823,13 +823,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
 	/* HCD will be started by OTG fsm when needed */
 	mutex_lock(&otg->fsm.lock);
 	if (otg->primary_hcd.hcd) {
-		/* probably a shared HCD ? */
-		if (usb_otg_hcd_is_primary_hcd(hcd)) {
+		/* probably a shared HCD or a companion OHCI HCD ? */
+		if (!otg->caps->needs_companion &&
+		    usb_otg_hcd_is_primary_hcd(hcd)) {
 			dev_err(otg_dev, "otg: primary host already registered\n");
 			goto err;
 		}
 
-		if (hcd->shared_hcd == otg->primary_hcd.hcd) {
+		if (otg->caps->needs_companion ||
+		    (hcd->shared_hcd == otg->primary_hcd.hcd)) {
 			if (otg->shared_hcd.hcd) {
 				dev_err(otg_dev, "otg: shared host already registered\n");
 				goto err;
@@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
 	 * we're ready only if we have shared HCD
 	 * or we don't need shared HCD.
 	 */
-	if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
+	if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
+				    !otg->primary_hcd.hcd->shared_hcd)) {
 		otg->host = hcd_to_bus(hcd);
 		/* FIXME: set bus->otg_port if this is true OTG port with HNP */
 
diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index b094352..64a7db8 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -112,12 +112,14 @@ struct usb_otg {
  * @hnp_support: Indicates if the device supports HNP.
  * @srp_support: Indicates if the device supports SRP.
  * @adp_support: Indicates if the device supports ADP.
+ * @needs_companion: Indicates if the device needs a companion controller.
  */
 struct usb_otg_caps {
 	u16 otg_rev;
 	bool hnp_support;
 	bool srp_support;
 	bool adp_support;
+	bool needs_companion;
 };
 
 /**
-- 
1.9.1