On 14/04/16 11:36, Yoshihiro Shimoda wrote:
> 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;
I'm not sure if otg_caps structure is a right place for this. Maybe Peter can confirm
if this is OK or not.
I was thinking more about adding this bit in the otg_config structure.
>
> 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 */
>
These changes look good to me. Thanks.
> 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.
Description is not exact. How about this.
"Indicates if host controller needs a companion controller"
Is hcd_needs_companion is better than just needs_companion?
> */
> struct usb_otg_caps {
> u16 otg_rev;
> bool hnp_support;
> bool srp_support;
> bool adp_support;
> + bool needs_companion;
> };
>
> /**
>
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
