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

From: Yoshihiro Shimoda
Date: Thu Apr 14 2016 - 04:37:13 EST

Next message: Russell King - ARM Linux: "Re: [PATCH v7 1/2] drivers: base: add support for registering notifier about deferred probe"
Previous message: Ulf Hansson: "Re: [RFC 0/6] mmc: Field Firmware Update"
In reply to: Roger Quadros: "Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core"
Next in thread: Roger Quadros: "Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Russell King - ARM Linux: "Re: [PATCH v7 1/2] drivers: base: add support for registering notifier about deferred probe"
Previous message: Ulf Hansson: "Re: [RFC 0/6] mmc: Field Firmware Update"
In reply to: Roger Quadros: "Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core"
Next in thread: Roger Quadros: "Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]