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

[Yoshihiro Shimoda]

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?

> 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).

> > 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 an attribute to a USB controller such as:
 
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 741d9d2..9dcf76ac 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1253,6 +1253,7 @@
 			compatible = "renesas,usb2-phy-r8a7795";
 			reg = <0 0xee080200 0 0x700>;
 			interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
+			otg-hcds = <&ehci0>, <&ohci0>;
 			clocks = <&cpg CPG_MOD 703>;
 			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
 			#phy-cells = <0>;
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
index 41e762a..9a78482 100644
--- a/drivers/usb/common/usb-otg.c
+++ b/drivers/usb/common/usb-otg.c
@@ -258,7 +258,8 @@ static void usb_otg_flush_wait(struct device *otg_dev)
 
 	dev_dbg(otg_dev, "otg: registering pending host/gadget\n");
 	gadget = &wait->gcd;
-	if (gadget)
+	/* I'm not sure but gadget->gadget is possible to be NULL */
+	if (gadget && gadget->gadget)
 		usb_otg_register_gadget(gadget->gadget, gadget->ops);
 
 	host = &wait->primary_hcd;
@@ -567,6 +568,8 @@ struct usb_otg *usb_otg_register(struct device *dev,
 {
 	struct usb_otg *otg;
 	struct otg_wait_data *wait;
+	struct device_node *np;
+	struct platform_device *pdev;
 	int ret = 0;
 
 	if (!dev || !config || !config->fsm_ops)
@@ -616,6 +619,40 @@ struct usb_otg *usb_otg_register(struct device *dev,
 	list_add_tail(&otg->list, &otg_list);
 	mutex_unlock(&otg_list_mutex);
 
+	/* Get primary hcd device (mandatory) */
+	np = of_parse_phandle(dev->of_node, "otg-hcds", 0);
+	if (!np) {
+		dev_err(dev, "no otg-hcds\n");
+		ret = -EINVAL;
+		goto err_dt;
+	}
+	pdev = of_find_device_by_node(np);
+	of_node_put(np);
+	if (!pdev) {
+		dev_err(dev, "coulnd't get primary hcd device\n");
+		ret = -ENODEV;
+		goto err_dt;
+	}
+	otg->primary_dev = &pdev->dev;
+
+	/* Get shared hcd device (optional) */
+	np = of_parse_phandle(dev->of_node, "otg-hcds", 1);
+	if (np) {
+		pdev = of_find_device_by_node(np);
+		of_node_put(np);
+		if (!pdev) {
+			dev_err(dev, "coulnd't get shared hcd device\n");
+			ret = -ENODEV;
+			goto err_dt;
+		}
+		otg->shared_dev = &pdev->dev;
+	} else {
+		dev_dbg(dev, "no shared hcd\n");
+	}
+
+	if (otg->primary_dev != otg->shared_dev)
+		otg->flags |= OTG_FLAG_SHARED_IS_2ND_PRIMARY;
+
 	/* were we in wait list? */
 	mutex_lock(&wait_list_mutex);
 	wait = usb_otg_get_wait(dev);
@@ -627,6 +664,8 @@ struct usb_otg *usb_otg_register(struct device *dev,
 
 	return otg;
 
+err_dt:
+	destroy_workqueue(otg->wq);
 err_wq:
 	kfree(otg);
 unlock:
@@ -822,50 +861,42 @@ 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)) {
-			dev_err(otg_dev, "otg: primary host already registered\n");
-			goto err;
+	if (!otg->primary_hcd.hcd) {
+		dev_info(hcd_dev, "%s: primary %s, now %s\n", __func__,
+			dev_name(otg->primary_dev),
+			dev_name(hcd->self.controller));
+		if (otg->primary_dev == hcd->self.controller) {
+			otg->primary_hcd.hcd = hcd;
+			otg->primary_hcd.irqnum = irqnum;
+			otg->primary_hcd.irqflags = irqflags;
+			otg->primary_hcd.ops = ops;
+			otg->hcd_ops = ops;
+			dev_info(otg_dev, "otg: primary host %s registered\n",
+				 dev_name(hcd->self.controller));
 		}
+	}
 
-		if (hcd->shared_hcd == otg->primary_hcd.hcd) {
-			if (otg->shared_hcd.hcd) {
-				dev_err(otg_dev, "otg: shared host already registered\n");
-				goto err;
-			}
-
+	if (!otg->shared_hcd.hcd && (!usb_otg_hcd_is_primary_hcd(hcd) ||
+			otg->flags & OTG_FLAG_SHARED_IS_2ND_PRIMARY)) {
+		dev_info(hcd_dev, "%s: shared %s, now %s\n", __func__,
+			dev_name(otg->shared_dev),
+			dev_name(hcd->self.controller));
+		if (otg->shared_dev == hcd->self.controller) {
 			otg->shared_hcd.hcd = hcd;
 			otg->shared_hcd.irqnum = irqnum;
 			otg->shared_hcd.irqflags = irqflags;
 			otg->shared_hcd.ops = ops;
 			dev_info(otg_dev, "otg: shared host %s registered\n",
 				 dev_name(hcd->self.controller));
-		} else {
-			dev_err(otg_dev, "otg: invalid shared host %s\n",
-				dev_name(hcd->self.controller));
-			goto err;
-		}
-	} else {
-		if (!usb_otg_hcd_is_primary_hcd(hcd)) {
-			dev_err(otg_dev, "otg: primary host must be registered first\n");
-			goto err;
 		}
-
-		otg->primary_hcd.hcd = hcd;
-		otg->primary_hcd.irqnum = irqnum;
-		otg->primary_hcd.irqflags = irqflags;
-		otg->primary_hcd.ops = ops;
-		otg->hcd_ops = ops;
-		dev_info(otg_dev, "otg: primary host %s registered\n",
-			 dev_name(hcd->self.controller));
 	}
 
 	/*
 	 * 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->primary_hcd.hcd && (!otg->shared_dev ||
+	    (otg->shared_dev && otg->shared_hcd.hcd))) {
 		otg->host = hcd_to_bus(hcd);
 		/* FIXME: set bus->otg_port if this is true OTG port with HNP */
 
@@ -878,10 +909,6 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
 	mutex_unlock(&otg->fsm.lock);
 
 	return 0;
-
-err:
-	mutex_unlock(&otg->fsm.lock);
-	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(usb_otg_register_hcd);
 
diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index b094352..ed08865 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -51,6 +51,8 @@ struct otg_hcd {
  * @fsm: otg finite state machine
  * @hcd_ops: host controller interface
  * ------- internal use only -------
+ * @primary_dev: primary host device for matching
+ * @shared_dev: (optional) shared or other primary host device for matching
  * @primary_hcd: primary host state and interface
  * @shared_hcd: shared host state and interface
  * @gadget_ops: gadget controller interface
@@ -75,6 +77,8 @@ struct usb_otg {
 	struct otg_hcd_ops	*hcd_ops;
 
 	/* internal use only */
+	struct device *primary_dev;
+	struct device *shared_dev;
 	struct otg_hcd primary_hcd;
 	struct otg_hcd shared_hcd;
 	struct otg_gadget_ops *gadget_ops;
@@ -84,6 +88,7 @@ struct usb_otg {
 	u32 flags;
 #define OTG_FLAG_GADGET_RUNNING (1 << 0)
 #define OTG_FLAG_HOST_RUNNING (1 << 1)
+#define OTG_FLAG_SHARED_IS_2ND_PRIMARY (1 << 2)
 	/* use otg->fsm.lock for serializing access */
 
 /*------------- deprecated interface -----------------------------*/