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

From: Roger Quadros
Date: Mon Apr 11 2016 - 06:55:04 EST

Next message: Chen Feng: "Re: [PATCH 1/2] arm64: mem-model: add flatmem model for arm64"
Previous message: Luis de Bethencourt: "[RESEND] block: swim3: Fix module autoload for OF platform driver"
In reply to: Yoshihiro Shimoda: "RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core"
Next in thread: Yoshihiro Shimoda: "RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

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

cheers,
-roger

> 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 -----------------------------*/
>
>

Next message: Chen Feng: "Re: [PATCH 1/2] arm64: mem-model: add flatmem model for arm64"
Previous message: Luis de Bethencourt: "[RESEND] block: swim3: Fix module autoload for OF platform driver"
In reply to: Yoshihiro Shimoda: "RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core"
Next in thread: Yoshihiro Shimoda: "RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]