Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1371633 > unrolled thread
| Started by | Roger Quadros <rogerq@ti.com> |
|---|---|
| First post | 2016-04-05 16:10 +0200 |
| Last post | 2016-04-27 15:40 +0200 |
| Articles | 20 on this page of 85 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH v6 00/12] USB OTG/dual-role framework Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
[PATCH v6 10/12] usb: doc: dt-binding: Add otg-controller property Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
[PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Peter Chen <hzpeterchen@gmail.com> - 2016-04-18 09:10 +0200
Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-20 09:00 +0200
RE: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Jun Li <jun.li@nxp.com> - 2016-04-21 08:40 +0200
Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-25 16:10 +0200
RE: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Jun Li <jun.li@nxp.com> - 2016-04-26 02:10 +0200
Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-27 13:30 +0200
Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-28 12:00 +0200
[PATCH v6 12/12] usb: host: xhci-plat: Add otg device to platform data Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
RE: [PATCH v6 12/12] usb: host: xhci-plat: Add otg device to platform data Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-06 05:30 +0200
Re: [PATCH v6 12/12] usb: host: xhci-plat: Add otg device to platform data Roger Quadros <rogerq@ti.com> - 2016-04-06 08:40 +0200
[PATCH v6 06/12] usb: otg: get rid of CONFIG_USB_OTG_FSM in favour of CONFIG_USB_OTG Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
Re: [PATCH v6 06/12] usb: otg: get rid of CONFIG_USB_OTG_FSM in favour of CONFIG_USB_OTG Peter Chen <hzpeterchen@gmail.com> - 2016-04-18 10:20 +0200
Re: [PATCH v6 06/12] usb: otg: get rid of CONFIG_USB_OTG_FSM in favour of CONFIG_USB_OTG Roger Quadros <rogerq@ti.com> - 2016-04-20 10:20 +0200
[PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-07 11:00 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-07 13:50 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-08 13:30 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-11 13:00 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-14 10:40 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-14 13:10 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-14 13:20 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-14 13:40 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-15 12:00 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-15 13:00 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-15 12:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-19 11:30 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-20 07:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-20 09:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-22 08:20 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-22 03:40 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-22 05:50 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> - 2016-04-22 08:00 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-15 11:40 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-15 13:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-18 04:20 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-20 09:00 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-20 11:40 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-19 10:20 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-20 09:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-20 11:50 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Jun Li <jun.li@nxp.com> - 2016-04-26 04:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-26 05:50 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Jun Li <jun.li@nxp.com> - 2016-04-26 07:20 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-26 08:30 +0200
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Jun Li <jun.li@nxp.com> - 2016-04-26 09:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-26 10:30 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-27 05:30 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-27 13:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-28 04:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-28 10:10 +0200
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-27 13:20 +0200
[PATCH v6 05/12] usb: gadget.h: Add OTG to gadget interface Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
[PATCH v6 02/12] usb: hcd.h: Add OTG to HCD interface Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
Re: [PATCH v6 02/12] usb: hcd.h: Add OTG to HCD interface Peter Chen <hzpeterchen@gmail.com> - 2016-04-18 09:50 +0200
[PATCH v6 04/12] usb: otg-fsm: move host controller operations into usb_otg->hcd_ops Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
Re: [PATCH v6 04/12] usb: otg-fsm: move host controller operations into usb_otg->hcd_ops Peter Chen <hzpeterchen@gmail.com> - 2016-04-18 10:10 +0200
[PATCH v6 11/12] usb: core: hub: Notify OTG fsm when A device sets b_hnp_enable Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
Re: [PATCH v6 11/12] usb: core: hub: Notify OTG fsm when A device sets b_hnp_enable Peter Chen <hzpeterchen@gmail.com> - 2016-04-18 09:20 +0200
Re: [PATCH v6 11/12] usb: core: hub: Notify OTG fsm when A device sets b_hnp_enable Roger Quadros <rogerq@ti.com> - 2016-04-27 16:40 +0200
[PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Roger Quadros <rogerq@ti.com> - 2016-04-05 16:10 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Felipe Balbi <balbi@kernel.org> - 2016-04-06 08:20 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Roger Quadros <rogerq@ti.com> - 2016-04-06 08:40 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Peter Chen <hzpeterchen@gmail.com> - 2016-04-07 11:50 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Roger Quadros <rogerq@ti.com> - 2016-04-07 12:50 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Peter Chen <hzpeterchen@gmail.com> - 2016-04-08 03:10 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Roger Quadros <rogerq@ti.com> - 2016-04-08 09:20 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Peter Chen <hzpeterchen@gmail.com> - 2016-04-08 10:00 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Peter Chen <hzpeterchen@gmail.com> - 2016-04-18 04:40 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Alan Stern <stern@rowland.harvard.edu> - 2016-04-18 16:20 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Peter Chen <hzpeterchen@gmail.com> - 2016-04-19 04:10 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Roger Quadros <rogerq@ti.com> - 2016-04-20 10:20 +0200
Re: [PATCH v6 01/12] usb: hcd: Initialize hcd->flags to 0 Peter Chen <hzpeterchen@gmail.com> - 2016-04-20 11:50 +0200
[PATCH v6 08/12] usb: hcd: Adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-05 16:20 +0200
Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Peter Chen <hzpeterchen@gmail.com> - 2016-04-18 08:40 +0200
Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Peter Chen <hzpeterchen@gmail.com> - 2016-04-19 10:30 +0200
Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-20 08:50 +0200
Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-20 08:50 +0200
RE: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Jun Li <jun.li@nxp.com> - 2016-04-27 12:20 +0200
Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-27 13:10 +0200
Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Roger Quadros <rogerq@ti.com> - 2016-04-27 13:20 +0200
RE: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Jun Li <jun.li@nxp.com> - 2016-04-27 15:00 +0200
RE: [PATCH v6 08/12] usb: hcd: Adapt to OTG core Jun Li <jun.li@nxp.com> - 2016-04-27 15:40 +0200
Page 2 of 5 — ← Prev page 1 [2] 3 4 5 Next page →
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2016-04-11 13:00 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rmHJw-4qq-9@gated-at.bofh.it> |
| In reply to | #1374169 |
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 -----------------------------*/
>
>
[toc] | [prev] | [next] | [standalone]
| From | Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> |
|---|---|
| Date | 2016-04-14 10:40 +0200 |
| Subject | RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rnKYF-7jf-1@gated-at.bofh.it> |
| In reply to | #1375684 |
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
[toc] | [prev] | [next] | [standalone]
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2016-04-14 13:10 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rnNjQ-J5-19@gated-at.bofh.it> |
| In reply to | #1378596 |
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
[toc] | [prev] | [next] | [standalone]
| From | Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> |
|---|---|
| Date | 2016-04-14 13:20 +0200 |
| Subject | RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rnNtw-MQ-19@gated-at.bofh.it> |
| In reply to | #1378691 |
Hi,
> From: Roger Quadros
> Sent: Thursday, April 14, 2016 8:00 PM
>
> On 14/04/16 11:36, Yoshihiro Shimoda wrote:
> > Hi,
> >
< snip >
> > 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.
I see. I also think the otg_config is more suitable.
> >
> > 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.
Thank you for the comment.
If we change the "needs_companion" place to the otg_config,
do we need to add a flag into the otg, instead of otg->caps?
> > 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?
I agree with you.
So, I will modify my local patch and test it tomorrow.
Best regards,
Yoshihiro Shimoda
>
> > */
> > struct usb_otg_caps {
> > u16 otg_rev;
> > bool hnp_support;
> > bool srp_support;
> > bool adp_support;
> > + bool needs_companion;
> > };
> >
> > /**
> >
>
> cheers,
> -roger
[toc] | [prev] | [next] | [standalone]
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2016-04-14 13:40 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rnNMS-Ww-15@gated-at.bofh.it> |
| In reply to | #1378698 |
On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Roger Quadros
>> Sent: Thursday, April 14, 2016 8:00 PM
>>
>> On 14/04/16 11:36, Yoshihiro Shimoda wrote:
>>> Hi,
>>>
> < snip >
>>> 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.
>
> I see. I also think the otg_config is more suitable.
>
>>>
>>> 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.
>
> Thank you for the comment.
> If we change the "needs_companion" place to the otg_config,
> do we need to add a flag into the otg, instead of otg->caps?
Yes we can add a flag in struct usb_otg.
cheers,
-roger
>
>>> 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?
>
> I agree with you.
>
> So, I will modify my local patch and test it tomorrow.
>
> Best regards,
> Yoshihiro Shimoda
>
>>
>>> */
>>> struct usb_otg_caps {
>>> u16 otg_rev;
>>> bool hnp_support;
>>> bool srp_support;
>>> bool adp_support;
>>> + bool needs_companion;
>>> };
>>>
>>> /**
>>>
>>
>> cheers,
>> -roger
[toc] | [prev] | [next] | [standalone]
| From | Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> |
|---|---|
| Date | 2016-04-15 12:00 +0200 |
| Subject | RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <ro8HF-YX-17@gated-at.bofh.it> |
| In reply to | #1378706 |
Hi,
> From: Roger Quadros
> Sent: Thursday, April 14, 2016 8:32 PM
>
> On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > Hi,
> >
< snip >
> >>> @@ -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.
> >
> > Thank you for the comment.
> > If we change the "needs_companion" place to the otg_config,
> > do we need to add a flag into the otg, instead of otg->caps?
>
> Yes we can add a flag in struct usb_otg.
Thank you for the comment.
I made a fixed patch.
So, should I send this patch to ML after you sent v7 patches?
Or, would you apply this patch before you send v7 patches?
Best regards,
Yoshihiro Shimoda
---
[toc] | [prev] | [next] | [standalone]
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2016-04-15 13:00 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <ro9DK-1Ev-53@gated-at.bofh.it> |
| In reply to | #1379657 |
On 15/04/16 12:59, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Roger Quadros
>> Sent: Thursday, April 14, 2016 8:32 PM
>>
>> On 14/04/16 14:15, Yoshihiro Shimoda wrote:
>>> Hi,
>>>
> < snip >
>>>>> @@ -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.
>>>
>>> Thank you for the comment.
>>> If we change the "needs_companion" place to the otg_config,
>>> do we need to add a flag into the otg, instead of otg->caps?
>>
>> Yes we can add a flag in struct usb_otg.
>
> Thank you for the comment.
>
> I made a fixed patch.
> So, should I send this patch to ML after you sent v7 patches?
> Or, would you apply this patch before you send v7 patches?
>
Thanks for working on this and testing the series. I will pick your patch
as part of v7.
cheers,
-roger
[toc] | [prev] | [next] | [standalone]
| From | Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> |
|---|---|
| Date | 2016-04-15 12:10 +0200 |
| Subject | RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <ro8Rk-1jQ-17@gated-at.bofh.it> |
| In reply to | #1378706 |
Hi,
> From: Yoshihiro Shimoda
> Sent: Friday, April 15, 2016 6:59 PM
>
> Hi,
>
> > From: Roger Quadros
> > Sent: Thursday, April 14, 2016 8:32 PM
> >
> > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > Hi,
> > >
> < snip >
> > >>> @@ -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.
> > >
> > > Thank you for the comment.
> > > If we change the "needs_companion" place to the otg_config,
> > > do we need to add a flag into the otg, instead of otg->caps?
> >
> > Yes we can add a flag in struct usb_otg.
>
> Thank you for the comment.
>
> I made a fixed patch.
> So, should I send this patch to ML after you sent v7 patches?
> Or, would you apply this patch before you send v7 patches?
Oops, I sent this email without my patch...
---
Subject: [PATCH] usb: otg: add hcd companion support
Since some host controller (e.g. EHCI) needs a companion host controller
(e.g. OHCI), this patch adds such a configuration to use it in the OTG
core.
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
Documentation/devicetree/bindings/usb/generic.txt | 3 +++
drivers/usb/common/usb-otg.c | 17 +++++++++++++----
include/linux/usb/otg.h | 7 ++++++-
3 files changed, 22 insertions(+), 5 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/usb-otg.c b/drivers/usb/common/usb-otg.c
index 41e762a..83c8c96 100644
--- a/drivers/usb/common/usb-otg.c
+++ b/drivers/usb/common/usb-otg.c
@@ -20,6 +20,7 @@
#include <linux/list.h>
#include <linux/of.h>
#include <linux/of_platform.h>
+#include <linux/usb/of.h>
#include <linux/usb/otg.h>
#include <linux/usb/gadget.h>
#include <linux/workqueue.h>
@@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
else
INIT_WORK(&otg->work, usb_otg_work);
+ if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
+ config->hcd_needs_companion) /* needs comanion ? */
+ otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
+
otg->wq = create_singlethread_workqueue("usb_otg");
if (!otg->wq) {
dev_err(dev, "otg: %s: can't create workqueue\n",
@@ -823,13 +828,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->flags & OTG_FLAG_HCD_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->flags & OTG_FLAG_HCD_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 +872,9 @@ 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->flags & OTG_FLAG_HCD_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..6f4ca77 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -57,7 +57,8 @@ struct otg_hcd {
* @list: list of otg controllers
* @work: otg state machine work
* @wq: otg state machine work queue
- * @flags: to track if host/gadget is running
+ * @flags: to track if host/gadget is running, or to indicate if hcd needs
+ * companion
*/
struct usb_otg {
u8 default_a;
@@ -84,6 +85,7 @@ struct usb_otg {
u32 flags;
#define OTG_FLAG_GADGET_RUNNING (1 << 0)
#define OTG_FLAG_HOST_RUNNING (1 << 1)
+#define OTG_FLAG_HCD_NEEDS_COMPANION (1 << 2)
/* use otg->fsm.lock for serializing access */
/*------------- deprecated interface -----------------------------*/
@@ -125,11 +127,14 @@ struct usb_otg_caps {
* @caps: otg capabilities of the controller
* @ops: otg fsm operations
* @otg_work: optional custom otg state machine work function
+ * @hcd_needs_companion: Indicates if host controller needs a companion
+ * controller
*/
struct usb_otg_config {
struct usb_otg_caps *otg_caps;
struct otg_fsm_ops *fsm_ops;
void (*otg_work)(struct work_struct *work);
+ bool hcd_needs_companion;
};
extern const char *usb_otg_state_string(enum usb_otg_state state);
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-04-19 11:30 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rpA8O-4wD-13@gated-at.bofh.it> |
| In reply to | #1379665 |
On Fri, Apr 15, 2016 at 10:03:16AM +0000, Yoshihiro Shimoda wrote:
> Hi,
>
> > From: Yoshihiro Shimoda
> > Sent: Friday, April 15, 2016 6:59 PM
> >
> > Hi,
> >
> > > From: Roger Quadros
> > > Sent: Thursday, April 14, 2016 8:32 PM
> > >
> > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > Hi,
> > > >
> > < snip >
> > > >>> @@ -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.
> > > >
> > > > Thank you for the comment.
> > > > If we change the "needs_companion" place to the otg_config,
> > > > do we need to add a flag into the otg, instead of otg->caps?
> > >
> > > Yes we can add a flag in struct usb_otg.
> >
> > Thank you for the comment.
> >
> > I made a fixed patch.
> > So, should I send this patch to ML after you sent v7 patches?
> > Or, would you apply this patch before you send v7 patches?
>
> Oops, I sent this email without my patch...
>
> ---
> Subject: [PATCH] usb: otg: add hcd companion support
>
> Since some host controller (e.g. EHCI) needs a companion host controller
> (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> core.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
> Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> drivers/usb/common/usb-otg.c | 17 +++++++++++++----
> include/linux/usb/otg.h | 7 ++++++-
> 3 files changed, 22 insertions(+), 5 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/usb-otg.c b/drivers/usb/common/usb-otg.c
> index 41e762a..83c8c96 100644
> --- a/drivers/usb/common/usb-otg.c
> +++ b/drivers/usb/common/usb-otg.c
> @@ -20,6 +20,7 @@
> #include <linux/list.h>
> #include <linux/of.h>
> #include <linux/of_platform.h>
> +#include <linux/usb/of.h>
> #include <linux/usb/otg.h>
> #include <linux/usb/gadget.h>
> #include <linux/workqueue.h>
> @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> else
> INIT_WORK(&otg->work, usb_otg_work);
>
> + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> + config->hcd_needs_companion) /* needs comanion ? */
%s/comanion/companion
I have a little puzzled with companion controller and shared hcd, let me
post a topic for it.
Peter
> + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> +
> otg->wq = create_singlethread_workqueue("usb_otg");
> if (!otg->wq) {
> dev_err(dev, "otg: %s: can't create workqueue\n",
> @@ -823,13 +828,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->flags & OTG_FLAG_HCD_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->flags & OTG_FLAG_HCD_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 +872,9 @@ 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->flags & OTG_FLAG_HCD_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..6f4ca77 100644
> --- a/include/linux/usb/otg.h
> +++ b/include/linux/usb/otg.h
> @@ -57,7 +57,8 @@ struct otg_hcd {
> * @list: list of otg controllers
> * @work: otg state machine work
> * @wq: otg state machine work queue
> - * @flags: to track if host/gadget is running
> + * @flags: to track if host/gadget is running, or to indicate if hcd needs
> + * companion
> */
> struct usb_otg {
> u8 default_a;
> @@ -84,6 +85,7 @@ struct usb_otg {
> u32 flags;
> #define OTG_FLAG_GADGET_RUNNING (1 << 0)
> #define OTG_FLAG_HOST_RUNNING (1 << 1)
> +#define OTG_FLAG_HCD_NEEDS_COMPANION (1 << 2)
> /* use otg->fsm.lock for serializing access */
>
> /*------------- deprecated interface -----------------------------*/
> @@ -125,11 +127,14 @@ struct usb_otg_caps {
> * @caps: otg capabilities of the controller
> * @ops: otg fsm operations
> * @otg_work: optional custom otg state machine work function
> + * @hcd_needs_companion: Indicates if host controller needs a companion
> + * controller
> */
> struct usb_otg_config {
> struct usb_otg_caps *otg_caps;
> struct otg_fsm_ops *fsm_ops;
> void (*otg_work)(struct work_struct *work);
> + bool hcd_needs_companion;
> };
>
> extern const char *usb_otg_state_string(enum usb_otg_state state);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> |
|---|---|
| Date | 2016-04-20 07:10 +0200 |
| Subject | RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rpSyK-2vk-7@gated-at.bofh.it> |
| In reply to | #1382319 |
Hi,
> From: Peter Chen
> Sent: Tuesday, April 19, 2016 6:18 PM
>
> On Fri, Apr 15, 2016 at 10:03:16AM +0000, Yoshihiro Shimoda wrote:
> > Hi,
> >
> > > From: Yoshihiro Shimoda
> > > Sent: Friday, April 15, 2016 6:59 PM
> > >
> > > Hi,
> > >
> > > > From: Roger Quadros
> > > > Sent: Thursday, April 14, 2016 8:32 PM
> > > >
> > > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > > Hi,
> > > > >
> > > < snip >
> > > > >>> @@ -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.
> > > > >
> > > > > Thank you for the comment.
> > > > > If we change the "needs_companion" place to the otg_config,
> > > > > do we need to add a flag into the otg, instead of otg->caps?
> > > >
> > > > Yes we can add a flag in struct usb_otg.
> > >
> > > Thank you for the comment.
> > >
> > > I made a fixed patch.
> > > So, should I send this patch to ML after you sent v7 patches?
> > > Or, would you apply this patch before you send v7 patches?
> >
> > Oops, I sent this email without my patch...
> >
> > ---
> > Subject: [PATCH] usb: otg: add hcd companion support
> >
> > Since some host controller (e.g. EHCI) needs a companion host controller
> > (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> > core.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> > Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> > drivers/usb/common/usb-otg.c | 17 +++++++++++++----
> > include/linux/usb/otg.h | 7 ++++++-
> > 3 files changed, 22 insertions(+), 5 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/usb-otg.c b/drivers/usb/common/usb-otg.c
> > index 41e762a..83c8c96 100644
> > --- a/drivers/usb/common/usb-otg.c
> > +++ b/drivers/usb/common/usb-otg.c
> > @@ -20,6 +20,7 @@
> > #include <linux/list.h>
> > #include <linux/of.h>
> > #include <linux/of_platform.h>
> > +#include <linux/usb/of.h>
> > #include <linux/usb/otg.h>
> > #include <linux/usb/gadget.h>
> > #include <linux/workqueue.h>
> > @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> > else
> > INIT_WORK(&otg->work, usb_otg_work);
> >
> > + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> > + config->hcd_needs_companion) /* needs comanion ? */
>
> %s/comanion/companion
Thank you for pointing it out!
Roger, would you fix this in your v7 patch set?
> I have a little puzzled with companion controller and shared hcd, let me
> post a topic for it.
I looked at the email thread.
It is very useful information to me! :)
Best regards,
Yoshihiro Shimoda
> Peter
>
> > + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> > +
> > otg->wq = create_singlethread_workqueue("usb_otg");
> > if (!otg->wq) {
> > dev_err(dev, "otg: %s: can't create workqueue\n",
> > @@ -823,13 +828,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->flags & OTG_FLAG_HCD_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->flags & OTG_FLAG_HCD_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 +872,9 @@ 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->flags & OTG_FLAG_HCD_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..6f4ca77 100644
> > --- a/include/linux/usb/otg.h
> > +++ b/include/linux/usb/otg.h
> > @@ -57,7 +57,8 @@ struct otg_hcd {
> > * @list: list of otg controllers
> > * @work: otg state machine work
> > * @wq: otg state machine work queue
> > - * @flags: to track if host/gadget is running
> > + * @flags: to track if host/gadget is running, or to indicate if hcd needs
> > + * companion
> > */
> > struct usb_otg {
> > u8 default_a;
> > @@ -84,6 +85,7 @@ struct usb_otg {
> > u32 flags;
> > #define OTG_FLAG_GADGET_RUNNING (1 << 0)
> > #define OTG_FLAG_HOST_RUNNING (1 << 1)
> > +#define OTG_FLAG_HCD_NEEDS_COMPANION (1 << 2)
> > /* use otg->fsm.lock for serializing access */
> >
> > /*------------- deprecated interface -----------------------------*/
> > @@ -125,11 +127,14 @@ struct usb_otg_caps {
> > * @caps: otg capabilities of the controller
> > * @ops: otg fsm operations
> > * @otg_work: optional custom otg state machine work function
> > + * @hcd_needs_companion: Indicates if host controller needs a companion
> > + * controller
> > */
> > struct usb_otg_config {
> > struct usb_otg_caps *otg_caps;
> > struct otg_fsm_ops *fsm_ops;
> > void (*otg_work)(struct work_struct *work);
> > + bool hcd_needs_companion;
> > };
> >
> > extern const char *usb_otg_state_string(enum usb_otg_state state);
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
>
> Best Regards,
> Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2016-04-20 09:10 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rpUqR-3QU-5@gated-at.bofh.it> |
| In reply to | #1383051 |
On 20/04/16 08:08, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Peter Chen
>> Sent: Tuesday, April 19, 2016 6:18 PM
>>
>> On Fri, Apr 15, 2016 at 10:03:16AM +0000, Yoshihiro Shimoda wrote:
>>> Hi,
>>>
>>>> From: Yoshihiro Shimoda
>>>> Sent: Friday, April 15, 2016 6:59 PM
>>>>
>>>> Hi,
>>>>
>>>>> From: Roger Quadros
>>>>> Sent: Thursday, April 14, 2016 8:32 PM
>>>>>
>>>>> On 14/04/16 14:15, Yoshihiro Shimoda wrote:
>>>>>> Hi,
>>>>>>
>>>> < snip >
>>>>>>>> @@ -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.
>>>>>>
>>>>>> Thank you for the comment.
>>>>>> If we change the "needs_companion" place to the otg_config,
>>>>>> do we need to add a flag into the otg, instead of otg->caps?
>>>>>
>>>>> Yes we can add a flag in struct usb_otg.
>>>>
>>>> Thank you for the comment.
>>>>
>>>> I made a fixed patch.
>>>> So, should I send this patch to ML after you sent v7 patches?
>>>> Or, would you apply this patch before you send v7 patches?
>>>
>>> Oops, I sent this email without my patch...
>>>
>>> ---
>>> Subject: [PATCH] usb: otg: add hcd companion support
>>>
>>> Since some host controller (e.g. EHCI) needs a companion host controller
>>> (e.g. OHCI), this patch adds such a configuration to use it in the OTG
>>> core.
>>>
>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>> ---
>>> Documentation/devicetree/bindings/usb/generic.txt | 3 +++
>>> drivers/usb/common/usb-otg.c | 17 +++++++++++++----
>>> include/linux/usb/otg.h | 7 ++++++-
>>> 3 files changed, 22 insertions(+), 5 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/usb-otg.c b/drivers/usb/common/usb-otg.c
>>> index 41e762a..83c8c96 100644
>>> --- a/drivers/usb/common/usb-otg.c
>>> +++ b/drivers/usb/common/usb-otg.c
>>> @@ -20,6 +20,7 @@
>>> #include <linux/list.h>
>>> #include <linux/of.h>
>>> #include <linux/of_platform.h>
>>> +#include <linux/usb/of.h>
>>> #include <linux/usb/otg.h>
>>> #include <linux/usb/gadget.h>
>>> #include <linux/workqueue.h>
>>> @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
>>> else
>>> INIT_WORK(&otg->work, usb_otg_work);
>>>
>>> + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
>>> + config->hcd_needs_companion) /* needs comanion ? */
>>
>> %s/comanion/companion
>
> Thank you for pointing it out!
>
> Roger, would you fix this in your v7 patch set?
Yes, I'll fix it locally. You don't need to post it again.
--
cheers,
-roger
>
>> I have a little puzzled with companion controller and shared hcd, let me
>> post a topic for it.
>
> I looked at the email thread.
> It is very useful information to me! :)
>
> Best regards,
> Yoshihiro Shimoda
>
>> Peter
>>
>>> + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
>>> +
>>> otg->wq = create_singlethread_workqueue("usb_otg");
>>> if (!otg->wq) {
>>> dev_err(dev, "otg: %s: can't create workqueue\n",
>>> @@ -823,13 +828,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->flags & OTG_FLAG_HCD_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->flags & OTG_FLAG_HCD_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 +872,9 @@ 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->flags & OTG_FLAG_HCD_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..6f4ca77 100644
>>> --- a/include/linux/usb/otg.h
>>> +++ b/include/linux/usb/otg.h
>>> @@ -57,7 +57,8 @@ struct otg_hcd {
>>> * @list: list of otg controllers
>>> * @work: otg state machine work
>>> * @wq: otg state machine work queue
>>> - * @flags: to track if host/gadget is running
>>> + * @flags: to track if host/gadget is running, or to indicate if hcd needs
>>> + * companion
>>> */
>>> struct usb_otg {
>>> u8 default_a;
>>> @@ -84,6 +85,7 @@ struct usb_otg {
>>> u32 flags;
>>> #define OTG_FLAG_GADGET_RUNNING (1 << 0)
>>> #define OTG_FLAG_HOST_RUNNING (1 << 1)
>>> +#define OTG_FLAG_HCD_NEEDS_COMPANION (1 << 2)
>>> /* use otg->fsm.lock for serializing access */
>>>
>>> /*------------- deprecated interface -----------------------------*/
>>> @@ -125,11 +127,14 @@ struct usb_otg_caps {
>>> * @caps: otg capabilities of the controller
>>> * @ops: otg fsm operations
>>> * @otg_work: optional custom otg state machine work function
>>> + * @hcd_needs_companion: Indicates if host controller needs a companion
>>> + * controller
>>> */
>>> struct usb_otg_config {
>>> struct usb_otg_caps *otg_caps;
>>> struct otg_fsm_ops *fsm_ops;
>>> void (*otg_work)(struct work_struct *work);
>>> + bool hcd_needs_companion;
>>> };
>>>
>>> extern const char *usb_otg_state_string(enum usb_otg_state state);
>>> --
>>> 1.9.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>>
>> Best Regards,
>> Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-04-22 08:20 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rqCBA-5Qg-13@gated-at.bofh.it> |
| In reply to | #1383090 |
On Wed, Apr 20, 2016 at 10:03:34AM +0300, Roger Quadros wrote:
> On 20/04/16 08:08, Yoshihiro Shimoda wrote:
> > Hi,
> >
> >> From: Peter Chen
> >> Sent: Tuesday, April 19, 2016 6:18 PM
> >>
> >> On Fri, Apr 15, 2016 at 10:03:16AM +0000, Yoshihiro Shimoda wrote:
> >>> Hi,
> >>>
> >>>> From: Yoshihiro Shimoda
> >>>> Sent: Friday, April 15, 2016 6:59 PM
> >>>>
> >>>> Hi,
> >>>>
> >>>>> From: Roger Quadros
> >>>>> Sent: Thursday, April 14, 2016 8:32 PM
> >>>>>
> >>>>> On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> >>>>>> Hi,
> >>>>>>
> >>>> < snip >
> >>>>>>>> @@ -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.
> >>>>>>
> >>>>>> Thank you for the comment.
> >>>>>> If we change the "needs_companion" place to the otg_config,
> >>>>>> do we need to add a flag into the otg, instead of otg->caps?
> >>>>>
> >>>>> Yes we can add a flag in struct usb_otg.
> >>>>
> >>>> Thank you for the comment.
> >>>>
> >>>> I made a fixed patch.
> >>>> So, should I send this patch to ML after you sent v7 patches?
> >>>> Or, would you apply this patch before you send v7 patches?
> >>>
> >>> Oops, I sent this email without my patch...
> >>>
> >>> ---
> >>> Subject: [PATCH] usb: otg: add hcd companion support
> >>>
> >>> Since some host controller (e.g. EHCI) needs a companion host controller
> >>> (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> >>> core.
> >>>
> >>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >>> ---
> >>> Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> >>> drivers/usb/common/usb-otg.c | 17 +++++++++++++----
> >>> include/linux/usb/otg.h | 7 ++++++-
> >>> 3 files changed, 22 insertions(+), 5 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/usb-otg.c b/drivers/usb/common/usb-otg.c
> >>> index 41e762a..83c8c96 100644
> >>> --- a/drivers/usb/common/usb-otg.c
> >>> +++ b/drivers/usb/common/usb-otg.c
> >>> @@ -20,6 +20,7 @@
> >>> #include <linux/list.h>
> >>> #include <linux/of.h>
> >>> #include <linux/of_platform.h>
> >>> +#include <linux/usb/of.h>
> >>> #include <linux/usb/otg.h>
> >>> #include <linux/usb/gadget.h>
> >>> #include <linux/workqueue.h>
> >>> @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> >>> else
> >>> INIT_WORK(&otg->work, usb_otg_work);
> >>>
> >>> + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> >>> + config->hcd_needs_companion) /* needs comanion ? */
> >>
> >> %s/comanion/companion
> >
> > Thank you for pointing it out!
> >
> > Roger, would you fix this in your v7 patch set?
>
> Yes, I'll fix it locally. You don't need to post it again.
>
Roger, I am ok with this patch.
Acked-by: Peter Chen <peter.chen@nxp.com>
Peter
> --
> cheers,
> -roger
>
> >
> >> I have a little puzzled with companion controller and shared hcd, let me
> >> post a topic for it.
> >
> > I looked at the email thread.
> > It is very useful information to me! :)
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> >> Peter
> >>
> >>> + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> >>> +
> >>> otg->wq = create_singlethread_workqueue("usb_otg");
> >>> if (!otg->wq) {
> >>> dev_err(dev, "otg: %s: can't create workqueue\n",
> >>> @@ -823,13 +828,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->flags & OTG_FLAG_HCD_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->flags & OTG_FLAG_HCD_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 +872,9 @@ 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->flags & OTG_FLAG_HCD_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..6f4ca77 100644
> >>> --- a/include/linux/usb/otg.h
> >>> +++ b/include/linux/usb/otg.h
> >>> @@ -57,7 +57,8 @@ struct otg_hcd {
> >>> * @list: list of otg controllers
> >>> * @work: otg state machine work
> >>> * @wq: otg state machine work queue
> >>> - * @flags: to track if host/gadget is running
> >>> + * @flags: to track if host/gadget is running, or to indicate if hcd needs
> >>> + * companion
> >>> */
> >>> struct usb_otg {
> >>> u8 default_a;
> >>> @@ -84,6 +85,7 @@ struct usb_otg {
> >>> u32 flags;
> >>> #define OTG_FLAG_GADGET_RUNNING (1 << 0)
> >>> #define OTG_FLAG_HOST_RUNNING (1 << 1)
> >>> +#define OTG_FLAG_HCD_NEEDS_COMPANION (1 << 2)
> >>> /* use otg->fsm.lock for serializing access */
> >>>
> >>> /*------------- deprecated interface -----------------------------*/
> >>> @@ -125,11 +127,14 @@ struct usb_otg_caps {
> >>> * @caps: otg capabilities of the controller
> >>> * @ops: otg fsm operations
> >>> * @otg_work: optional custom otg state machine work function
> >>> + * @hcd_needs_companion: Indicates if host controller needs a companion
> >>> + * controller
> >>> */
> >>> struct usb_otg_config {
> >>> struct usb_otg_caps *otg_caps;
> >>> struct otg_fsm_ops *fsm_ops;
> >>> void (*otg_work)(struct work_struct *work);
> >>> + bool hcd_needs_companion;
> >>> };
> >>>
> >>> extern const char *usb_otg_state_string(enum usb_otg_state state);
> >>> --
> >>> 1.9.1
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >> --
> >>
> >> Best Regards,
> >> Peter Chen
--
Best Regards,
Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-04-22 03:40 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rqyeC-203-7@gated-at.bofh.it> |
| In reply to | #1379665 |
On Fri, Apr 15, 2016 at 10:03:16AM +0000, Yoshihiro Shimoda wrote:
> Hi,
>
> > From: Yoshihiro Shimoda
> > Sent: Friday, April 15, 2016 6:59 PM
> >
> > Hi,
> >
> > > From: Roger Quadros
> > > Sent: Thursday, April 14, 2016 8:32 PM
> > >
> > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > Hi,
> > > >
> > < snip >
> > > >>> @@ -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.
> > > >
> > > > Thank you for the comment.
> > > > If we change the "needs_companion" place to the otg_config,
> > > > do we need to add a flag into the otg, instead of otg->caps?
> > >
> > > Yes we can add a flag in struct usb_otg.
> >
> > Thank you for the comment.
> >
> > I made a fixed patch.
> > So, should I send this patch to ML after you sent v7 patches?
> > Or, would you apply this patch before you send v7 patches?
>
> Oops, I sent this email without my patch...
>
> ---
> Subject: [PATCH] usb: otg: add hcd companion support
>
> Since some host controller (e.g. EHCI) needs a companion host controller
> (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> core.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
> Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> drivers/usb/common/usb-otg.c | 17 +++++++++++++----
> include/linux/usb/otg.h | 7 ++++++-
> 3 files changed, 22 insertions(+), 5 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/usb-otg.c b/drivers/usb/common/usb-otg.c
> index 41e762a..83c8c96 100644
> --- a/drivers/usb/common/usb-otg.c
> +++ b/drivers/usb/common/usb-otg.c
> @@ -20,6 +20,7 @@
> #include <linux/list.h>
> #include <linux/of.h>
> #include <linux/of_platform.h>
> +#include <linux/usb/of.h>
> #include <linux/usb/otg.h>
> #include <linux/usb/gadget.h>
> #include <linux/workqueue.h>
> @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> else
> INIT_WORK(&otg->work, usb_otg_work);
>
> + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> + config->hcd_needs_companion) /* needs comanion ? */
> + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> +
> otg->wq = create_singlethread_workqueue("usb_otg");
> if (!otg->wq) {
> dev_err(dev, "otg: %s: can't create workqueue\n",
> @@ -823,13 +828,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->flags & OTG_FLAG_HCD_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->flags & OTG_FLAG_HCD_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 +872,9 @@ 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->flags & OTG_FLAG_HCD_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..6f4ca77 100644
> --- a/include/linux/usb/otg.h
> +++ b/include/linux/usb/otg.h
> @@ -57,7 +57,8 @@ struct otg_hcd {
> * @list: list of otg controllers
> * @work: otg state machine work
> * @wq: otg state machine work queue
> - * @flags: to track if host/gadget is running
> + * @flags: to track if host/gadget is running, or to indicate if hcd needs
> + * companion
> */
> struct usb_otg {
> u8 default_a;
> @@ -84,6 +85,7 @@ struct usb_otg {
> u32 flags;
> #define OTG_FLAG_GADGET_RUNNING (1 << 0)
> #define OTG_FLAG_HOST_RUNNING (1 << 1)
> +#define OTG_FLAG_HCD_NEEDS_COMPANION (1 << 2)
> /* use otg->fsm.lock for serializing access */
>
> /*------------- deprecated interface -----------------------------*/
> @@ -125,11 +127,14 @@ struct usb_otg_caps {
> * @caps: otg capabilities of the controller
> * @ops: otg fsm operations
> * @otg_work: optional custom otg state machine work function
> + * @hcd_needs_companion: Indicates if host controller needs a companion
> + * controller
> */
> struct usb_otg_config {
> struct usb_otg_caps *otg_caps;
> struct otg_fsm_ops *fsm_ops;
> void (*otg_work)(struct work_struct *work);
> + bool hcd_needs_companion;
> };
>
> extern const char *usb_otg_state_string(enum usb_otg_state state);
> --
Hi Yoshihiro, the shared_hcd is used for USB3 only [1], this patch may
not be suitable for supporting companion controller at OTG framework.
For companion or other USB2 controllers, both primary hcd and shard hcd
are NULL for HCD core.
[1] http://www.spinics.net/lists/linux-usb/msg139630.html
--
Best Regards,
Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-04-22 05:50 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rqAgp-3AA-3@gated-at.bofh.it> |
| In reply to | #1384662 |
On Fri, Apr 22, 2016 at 09:26:46AM +0800, Peter Chen wrote:
> On Fri, Apr 15, 2016 at 10:03:16AM +0000, Yoshihiro Shimoda wrote:
> > Hi,
> >
> > > From: Yoshihiro Shimoda
> > > Sent: Friday, April 15, 2016 6:59 PM
> > >
> > > Hi,
> > >
> > > > From: Roger Quadros
> > > > Sent: Thursday, April 14, 2016 8:32 PM
> > > >
> > > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > > Hi,
> > > > >
> > > < snip >
> > > > >>> @@ -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.
> > > > >
> > > > > Thank you for the comment.
> > > > > If we change the "needs_companion" place to the otg_config,
> > > > > do we need to add a flag into the otg, instead of otg->caps?
> > > >
> > > > Yes we can add a flag in struct usb_otg.
> > >
> > > Thank you for the comment.
> > >
> > > I made a fixed patch.
> > > So, should I send this patch to ML after you sent v7 patches?
> > > Or, would you apply this patch before you send v7 patches?
> >
> > Oops, I sent this email without my patch...
> >
> > ---
> > Subject: [PATCH] usb: otg: add hcd companion support
> >
> > Since some host controller (e.g. EHCI) needs a companion host controller
> > (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> > core.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> > Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> > drivers/usb/common/usb-otg.c | 17 +++++++++++++----
> > include/linux/usb/otg.h | 7 ++++++-
> > 3 files changed, 22 insertions(+), 5 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/usb-otg.c b/drivers/usb/common/usb-otg.c
> > index 41e762a..83c8c96 100644
> > --- a/drivers/usb/common/usb-otg.c
> > +++ b/drivers/usb/common/usb-otg.c
> > @@ -20,6 +20,7 @@
> > #include <linux/list.h>
> > #include <linux/of.h>
> > #include <linux/of_platform.h>
> > +#include <linux/usb/of.h>
> > #include <linux/usb/otg.h>
> > #include <linux/usb/gadget.h>
> > #include <linux/workqueue.h>
> > @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> > else
> > INIT_WORK(&otg->work, usb_otg_work);
> >
> > + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> > + config->hcd_needs_companion) /* needs comanion ? */
> > + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> > +
> > otg->wq = create_singlethread_workqueue("usb_otg");
> > if (!otg->wq) {
> > dev_err(dev, "otg: %s: can't create workqueue\n",
> > @@ -823,13 +828,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->flags & OTG_FLAG_HCD_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->flags & OTG_FLAG_HCD_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 +872,9 @@ 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->flags & OTG_FLAG_HCD_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..6f4ca77 100644
> > --- a/include/linux/usb/otg.h
> > +++ b/include/linux/usb/otg.h
> > @@ -57,7 +57,8 @@ struct otg_hcd {
> > * @list: list of otg controllers
> > * @work: otg state machine work
> > * @wq: otg state machine work queue
> > - * @flags: to track if host/gadget is running
> > + * @flags: to track if host/gadget is running, or to indicate if hcd needs
> > + * companion
> > */
> > struct usb_otg {
> > u8 default_a;
> > @@ -84,6 +85,7 @@ struct usb_otg {
> > u32 flags;
> > #define OTG_FLAG_GADGET_RUNNING (1 << 0)
> > #define OTG_FLAG_HOST_RUNNING (1 << 1)
> > +#define OTG_FLAG_HCD_NEEDS_COMPANION (1 << 2)
> > /* use otg->fsm.lock for serializing access */
> >
> > /*------------- deprecated interface -----------------------------*/
> > @@ -125,11 +127,14 @@ struct usb_otg_caps {
> > * @caps: otg capabilities of the controller
> > * @ops: otg fsm operations
> > * @otg_work: optional custom otg state machine work function
> > + * @hcd_needs_companion: Indicates if host controller needs a companion
> > + * controller
> > */
> > struct usb_otg_config {
> > struct usb_otg_caps *otg_caps;
> > struct otg_fsm_ops *fsm_ops;
> > void (*otg_work)(struct work_struct *work);
> > + bool hcd_needs_companion;
> > };
> >
> > extern const char *usb_otg_state_string(enum usb_otg_state state);
> > --
>
> Hi Yoshihiro, the shared_hcd is used for USB3 only [1], this patch may
> not be suitable for supporting companion controller at OTG framework.
> For companion or other USB2 controllers, both primary hcd and shard hcd
> are NULL for HCD core.
>
Oh, I may be wrong. It should be ok for companion controller by using
this patch. Yoshihiro, do all below variables are NULL after both hcds
call usb_otg_register_hcd?
otg->primary_hcd.hcd->shared_hcd
otg->primary_hcd.hcd->primary_hcd
otg->shared_hcd.hcd->shared_hcd
otg->shared_hcd.hcd->primary_hcd
--
Best Regards,
Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> |
|---|---|
| Date | 2016-04-22 08:00 +0200 |
| Subject | RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rqCie-5m3-21@gated-at.bofh.it> |
| In reply to | #1384679 |
Hi,
> From: Peter Chen
> Sent: Friday, April 22, 2016 12:34 PM
>
> On Fri, Apr 22, 2016 at 09:26:46AM +0800, Peter Chen wrote:
> > On Fri, Apr 15, 2016 at 10:03:16AM +0000, Yoshihiro Shimoda wrote:
> > > Hi,
> > >
> > > > From: Yoshihiro Shimoda
> > > > Sent: Friday, April 15, 2016 6:59 PM
> > > >
> > > > Hi,
> > > >
> > > > > From: Roger Quadros
> > > > > Sent: Thursday, April 14, 2016 8:32 PM
> > > > >
> > > > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > > > Hi,
> > > > > >
> > > > < snip >
> > > > > >>> @@ -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.
> > > > > >
> > > > > > Thank you for the comment.
> > > > > > If we change the "needs_companion" place to the otg_config,
> > > > > > do we need to add a flag into the otg, instead of otg->caps?
> > > > >
> > > > > Yes we can add a flag in struct usb_otg.
> > > >
> > > > Thank you for the comment.
> > > >
> > > > I made a fixed patch.
> > > > So, should I send this patch to ML after you sent v7 patches?
> > > > Or, would you apply this patch before you send v7 patches?
> > >
> > > Oops, I sent this email without my patch...
> > >
> > > ---
> > > Subject: [PATCH] usb: otg: add hcd companion support
> > >
> > > Since some host controller (e.g. EHCI) needs a companion host controller
> > > (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> > > core.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > > Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> > > drivers/usb/common/usb-otg.c | 17 +++++++++++++----
> > > include/linux/usb/otg.h | 7 ++++++-
> > > 3 files changed, 22 insertions(+), 5 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/usb-otg.c b/drivers/usb/common/usb-otg.c
> > > index 41e762a..83c8c96 100644
> > > --- a/drivers/usb/common/usb-otg.c
> > > +++ b/drivers/usb/common/usb-otg.c
> > > @@ -20,6 +20,7 @@
> > > #include <linux/list.h>
> > > #include <linux/of.h>
> > > #include <linux/of_platform.h>
> > > +#include <linux/usb/of.h>
> > > #include <linux/usb/otg.h>
> > > #include <linux/usb/gadget.h>
> > > #include <linux/workqueue.h>
> > > @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> > > else
> > > INIT_WORK(&otg->work, usb_otg_work);
> > >
> > > + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> > > + config->hcd_needs_companion) /* needs comanion ? */
> > > + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> > > +
> > > otg->wq = create_singlethread_workqueue("usb_otg");
> > > if (!otg->wq) {
> > > dev_err(dev, "otg: %s: can't create workqueue\n",
> > > @@ -823,13 +828,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->flags & OTG_FLAG_HCD_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->flags & OTG_FLAG_HCD_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 +872,9 @@ 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->flags & OTG_FLAG_HCD_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..6f4ca77 100644
> > > --- a/include/linux/usb/otg.h
> > > +++ b/include/linux/usb/otg.h
> > > @@ -57,7 +57,8 @@ struct otg_hcd {
> > > * @list: list of otg controllers
> > > * @work: otg state machine work
> > > * @wq: otg state machine work queue
> > > - * @flags: to track if host/gadget is running
> > > + * @flags: to track if host/gadget is running, or to indicate if hcd needs
> > > + * companion
> > > */
> > > struct usb_otg {
> > > u8 default_a;
> > > @@ -84,6 +85,7 @@ struct usb_otg {
> > > u32 flags;
> > > #define OTG_FLAG_GADGET_RUNNING (1 << 0)
> > > #define OTG_FLAG_HOST_RUNNING (1 << 1)
> > > +#define OTG_FLAG_HCD_NEEDS_COMPANION (1 << 2)
> > > /* use otg->fsm.lock for serializing access */
> > >
> > > /*------------- deprecated interface -----------------------------*/
> > > @@ -125,11 +127,14 @@ struct usb_otg_caps {
> > > * @caps: otg capabilities of the controller
> > > * @ops: otg fsm operations
> > > * @otg_work: optional custom otg state machine work function
> > > + * @hcd_needs_companion: Indicates if host controller needs a companion
> > > + * controller
> > > */
> > > struct usb_otg_config {
> > > struct usb_otg_caps *otg_caps;
> > > struct otg_fsm_ops *fsm_ops;
> > > void (*otg_work)(struct work_struct *work);
> > > + bool hcd_needs_companion;
> > > };
> > >
> > > extern const char *usb_otg_state_string(enum usb_otg_state state);
> > > --
> >
> > Hi Yoshihiro, the shared_hcd is used for USB3 only [1], this patch may
> > not be suitable for supporting companion controller at OTG framework.
> > For companion or other USB2 controllers, both primary hcd and shard hcd
> > are NULL for HCD core.
> >
>
> Oh, I may be wrong. It should be ok for companion controller by using
> this patch. Yoshihiro, do all below variables are NULL after both hcds
> call usb_otg_register_hcd?
>
> otg->primary_hcd.hcd->shared_hcd
> otg->primary_hcd.hcd->primary_hcd
> otg->shared_hcd.hcd->shared_hcd
> otg->shared_hcd.hcd->primary_hcd
Thank you for the review twice! :)
I checked these variables and the variables was set to NULL after both
hcds call usb_otg_register_hcd.
< a patch for the check >
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
index 83c8c96..1fb7ea0 100644
--- a/drivers/usb/common/usb-otg.c
+++ b/drivers/usb/common/usb-otg.c
@@ -884,6 +884,19 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
dev_dbg(otg_dev, "otg: can't start till shared host registers\n");
}
+ if (otg->primary_hcd.hcd) {
+ dev_info(otg_dev, "primary_hcd.hcd->shared_hcd = %p\n",
+ otg->primary_hcd.hcd->shared_hcd);
+ dev_info(otg_dev, "primary_hcd.hcd->primary_hcd = %p\n",
+ otg->primary_hcd.hcd->primary_hcd);
+ }
+ if (otg->shared_hcd.hcd) {
+ dev_info(otg_dev, "shared_hcd.hcd->shared_hcd = %p\n",
+ otg->shared_hcd.hcd->shared_hcd);
+ dev_info(otg_dev, "shared_hcd.hcd->primary_hcd = %p\n",
+ otg->shared_hcd.hcd->primary_hcd);
+ }
+
mutex_unlock(&otg->fsm.lock);
return 0;
< result >
[ 1.797122] phy_rcar_gen3_usb2 ee080200.usb-phy: otg: limiting to dual-role
[ 1.804228] phy_rcar_gen3_usb2 ee080200.usb-phy: otg: primary host ee080100.usb registered
[ 1.812168] phy_rcar_gen3_usb2 ee080200.usb-phy: primary_hcd.hcd->shared_hcd = (null)
[ 1.820520] phy_rcar_gen3_usb2 ee080200.usb-phy: primary_hcd.hcd->primary_hcd = (null)
[ 1.829136] phy_rcar_gen3_usb2 ee080200.usb-phy: otg: shared host ee080000.usb registered
[ 1.836993] phy_rcar_gen3_usb2 ee080200.usb-phy: otg: can't start till gadget registers
[ 1.844675] phy_rcar_gen3_usb2 ee080200.usb-phy: primary_hcd.hcd->shared_hcd = (null)
[ 1.853024] phy_rcar_gen3_usb2 ee080200.usb-phy: primary_hcd.hcd->primary_hcd = (null)
[ 1.861458] phy_rcar_gen3_usb2 ee080200.usb-phy: shared_hcd.hcd->shared_hcd = (null)
[ 1.869724] phy_rcar_gen3_usb2 ee080200.usb-phy: shared_hcd.hcd->primary_hcd = (null)
Best regards,
Yoshihiro Shimoda
> --
>
> Best Regards,
> Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-04-15 11:40 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <ro8oi-S5-13@gated-at.bofh.it> |
| In reply to | #1371645 |
On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> + * @dev: OTG/dual-role controller device.
> + * @config: OTG configuration.
> + *
> + * Registers the OTG/dual-role controller device with the USB OTG core.
> + *
> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> + */
> +struct usb_otg *usb_otg_register(struct device *dev,
> + struct usb_otg_config *config)
> +{
> + struct usb_otg *otg;
> + struct otg_wait_data *wait;
> + int ret = 0;
> +
> + if (!dev || !config || !config->fsm_ops)
> + return ERR_PTR(-EINVAL);
> +
> + /* already in list? */
> + mutex_lock(&otg_list_mutex);
> + if (usb_otg_get_data(dev)) {
> + dev_err(dev, "otg: %s: device already in otg list\n",
> + __func__);
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + /* allocate and add to list */
> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> + if (!otg) {
> + ret = -ENOMEM;
> + goto unlock;
> + }
> +
> + otg->dev = dev;
> + otg->caps = config->otg_caps;
> +
> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> + otg->caps->adp_support) && !config->otg_work)
> + dev_info(dev, "otg: limiting to dual-role\n");
What does above mean? Customized otg_work item may be dual-role,
may be full otg.
> +
> + if (config->otg_work) /* custom otg_work ? */
> + INIT_WORK(&otg->work, config->otg_work);
> + else
> + INIT_WORK(&otg->work, usb_otg_work);
> +
> + otg->wq = create_singlethread_workqueue("usb_otg");
> + if (!otg->wq) {
> + dev_err(dev, "otg: %s: can't create workqueue\n",
> + __func__);
> + ret = -ENOMEM;
> + goto err_wq;
> + }
I found a bug that caused by non-freezable workqueue, change it
as freezable please.
https://marc.ttias.be/linux-stable/2016-03/msg00723.php
I will review the whole set from next week.
Peter
> +
> + /* set otg ops */
> + otg->fsm.ops = config->fsm_ops;
> +
> + mutex_init(&otg->fsm.lock);
> +
> + list_add_tail(&otg->list, &otg_list);
> + mutex_unlock(&otg_list_mutex);
> +
> + /* were we in wait list? */
> + mutex_lock(&wait_list_mutex);
> + wait = usb_otg_get_wait(dev);
> + mutex_unlock(&wait_list_mutex);
> + if (wait) {
> + /* register pending host/gadget and flush from list */
> + usb_otg_flush_wait(dev);
> + }
> +
> + return otg;
> +
> +err_wq:
> + kfree(otg);
> +unlock:
> + mutex_unlock(&otg_list_mutex);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register);
> +
> +/**
> + * usb_otg_unregister() - Unregister the OTG/dual-role device from USB OTG core
> + * @dev: OTG controller device.
> + *
> + * Unregisters the OTG/dual-role controller device from USB OTG core.
> + * Prevents unregistering till both the associated Host and Gadget controllers
> + * have unregistered from the OTG core.
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +int usb_otg_unregister(struct device *dev)
> +{
> + struct usb_otg *otg;
> +
> + mutex_lock(&otg_list_mutex);
> + otg = usb_otg_get_data(dev);
> + if (!otg) {
> + dev_err(dev, "otg: %s: device not in otg list\n",
> + __func__);
> + mutex_unlock(&otg_list_mutex);
> + return -EINVAL;
> + }
> +
> + /* prevent unregister till both host & gadget have unregistered */
> + if (otg->host || otg->gadget) {
> + dev_err(dev, "otg: %s: host/gadget still registered\n",
> + __func__);
> + return -EBUSY;
> + }
> +
> + /* OTG FSM is halted when host/gadget unregistered */
> + destroy_workqueue(otg->wq);
> +
> + /* remove from otg list */
> + list_del(&otg->list);
> + kfree(otg);
> + mutex_unlock(&otg_list_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister);
> +
> +/**
> + * start/kick the OTG FSM if we can
> + * fsm->lock must be held
> + */
> +static void usb_otg_start_fsm(struct usb_otg *otg)
> +{
> + struct otg_fsm *fsm = &otg->fsm;
> +
> + if (fsm->running)
> + goto kick_fsm;
> +
> + if (!otg->host) {
> + dev_info(otg->dev, "otg: can't start till host registers\n");
> + return;
> + }
> +
> + if (!otg->gadget) {
> + dev_info(otg->dev, "otg: can't start till gadget registers\n");
> + return;
> + }
> +
> + fsm->running = true;
> +kick_fsm:
> + queue_work(otg->wq, &otg->work);
> +}
> +
> +/**
> + * stop the OTG FSM. Stops Host & Gadget controllers as well.
> + * fsm->lock must be held
> + */
> +static void usb_otg_stop_fsm(struct usb_otg *otg)
> +{
> + struct otg_fsm *fsm = &otg->fsm;
> +
> + if (!fsm->running)
> + return;
> +
> + /* no more new events queued */
> + fsm->running = false;
> +
> + flush_workqueue(otg->wq);
> + otg->state = OTG_STATE_UNDEFINED;
> +
> + /* stop host/gadget immediately */
> + if (fsm->protocol == PROTO_HOST)
> + otg_start_host(otg, 0);
> + else if (fsm->protocol == PROTO_GADGET)
> + otg_start_gadget(otg, 0);
> + fsm->protocol = PROTO_UNDEF;
> +}
> +
> +/**
> + * usb_otg_sync_inputs - Sync OTG inputs with the OTG state machine
> + * @fsm: OTG FSM instance
> + *
> + * Used by the OTG driver to update the inputs to the OTG
> + * state machine.
> + *
> + * Can be called in IRQ context.
> + */
> +void usb_otg_sync_inputs(struct usb_otg *otg)
> +{
> + /* Don't kick FSM till it has started */
> + if (!otg->fsm.running)
> + return;
> +
> + /* Kick FSM */
> + queue_work(otg->wq, &otg->work);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_sync_inputs);
> +
> +/**
> + * usb_otg_kick_fsm - Kick the OTG state machine
> + * @otg_dev: OTG controller device
> + *
> + * Used by USB host/device stack to sync OTG related
> + * events to the OTG state machine.
> + * e.g. change in host_bus->b_hnp_enable, gadget->b_hnp_enable
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_kick_fsm(struct device *otg_dev)
> +{
> + struct usb_otg *otg;
> +
> + mutex_lock(&otg_list_mutex);
> + otg = usb_otg_get_data(otg_dev);
> + mutex_unlock(&otg_list_mutex);
> + if (!otg) {
> + dev_dbg(otg_dev, "otg: %s: invalid otg device\n",
> + __func__);
> + return -ENODEV;
> + }
> +
> + usb_otg_sync_inputs(otg);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_kick_fsm);
> +
> +/**
> + * usb_otg_register_hcd - Register the host controller to OTG core
> + * @hcd: host controller device
> + * @irqnum: interrupt number
> + * @irqflags: interrupt flags
> + * @ops: HCD ops to interface with the HCD
> + *
> + * This is used by the USB Host stack to register the host controller
> + * to the OTG core. Host controller must not be started by the
> + * caller as it is left upto the OTG state machine to do so.
> + * hcd->otg_dev must contain the related otg controller device.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
> + unsigned long irqflags, struct otg_hcd_ops *ops)
> +{
> + struct usb_otg *otg;
> + struct device *hcd_dev = hcd->self.controller;
> + struct device *otg_dev = hcd->otg_dev;
> +
> + if (!otg_dev)
> + return -EINVAL;
> +
> + /* we're otg but otg controller might not yet be registered */
> + mutex_lock(&otg_list_mutex);
> + otg = usb_otg_get_data(otg_dev);
> + mutex_unlock(&otg_list_mutex);
> + if (!otg) {
> + dev_dbg(hcd_dev,
> + "otg: controller not yet registered. waiting..\n");
> + /*
> + * otg controller might register later. Put the hcd in
> + * wait list and call us back when ready
> + */
> + if (usb_otg_hcd_wait_add(otg_dev, hcd, irqnum, irqflags, ops)) {
> + dev_err(hcd_dev, "otg: failed to add hcd to wait list\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> + }
> +
> + /* 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 (hcd->shared_hcd == otg->primary_hcd.hcd) {
> + if (otg->shared_hcd.hcd) {
> + dev_err(otg_dev, "otg: shared host already registered\n");
> + goto err;
> + }
> +
> + 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) {
> + otg->host = hcd_to_bus(hcd);
> + /* FIXME: set bus->otg_port if this is true OTG port with HNP */
> +
> + /* start FSM */
> + usb_otg_start_fsm(otg);
> + } else {
> + dev_dbg(otg_dev, "otg: can't start till shared host registers\n");
> + }
> +
> + mutex_unlock(&otg->fsm.lock);
> +
> + return 0;
> +
> +err:
> + mutex_unlock(&otg->fsm.lock);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register_hcd);
> +
> +/**
> + * usb_otg_unregister_hcd - Unregister the host controller from OTG core
> + * @hcd: host controller device
> + *
> + * This is used by the USB Host stack to unregister the host controller
> + * from the OTG core. Ensures that host controller is not running
> + * on successful return.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_unregister_hcd(struct usb_hcd *hcd)
> +{
> + struct usb_otg *otg;
> + struct device *hcd_dev = hcd_to_bus(hcd)->controller;
> + struct device *otg_dev = hcd->otg_dev;
> +
> + if (!otg_dev)
> + return -EINVAL; /* we're definitely not OTG */
> +
> + mutex_lock(&otg_list_mutex);
> + otg = usb_otg_get_data(otg_dev);
> + mutex_unlock(&otg_list_mutex);
> + if (!otg) {
> + /* are we in wait list? */
> + if (!usb_otg_hcd_wait_remove(hcd))
> + return 0;
> +
> + dev_dbg(hcd_dev, "otg: host wasn't registered with otg\n");
> + return -EINVAL;
> + }
> +
> + mutex_lock(&otg->fsm.lock);
> + if (hcd == otg->primary_hcd.hcd) {
> + otg->primary_hcd.hcd = NULL;
> + dev_info(otg_dev, "otg: primary host %s unregistered\n",
> + dev_name(hcd_dev));
> + } else if (hcd == otg->shared_hcd.hcd) {
> + otg->shared_hcd.hcd = NULL;
> + dev_info(otg_dev, "otg: shared host %s unregistered\n",
> + dev_name(hcd_dev));
> + } else {
> + dev_err(otg_dev, "otg: host %s wasn't registered with otg\n",
> + dev_name(hcd_dev));
> + mutex_unlock(&otg->fsm.lock);
> + return -EINVAL;
> + }
> +
> + /* stop FSM & Host */
> + usb_otg_stop_fsm(otg);
> + otg->host = NULL;
> +
> + mutex_unlock(&otg->fsm.lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister_hcd);
> +
> +/**
> + * usb_otg_register_gadget - Register the gadget controller to OTG core
> + * @gadget: gadget controller
> + *
> + * This is used by the USB gadget stack to register the gadget controller
> + * to the OTG core. Gadget controller must not be started by the
> + * caller as it is left upto the OTG state machine to do so.
> + *
> + * Gadget core must call this only when all resources required for
> + * gadget controller to run are available.
> + * i.e. gadget function driver is available.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_register_gadget(struct usb_gadget *gadget,
> + struct otg_gadget_ops *ops)
> +{
> + struct usb_otg *otg;
> + struct device *gadget_dev = &gadget->dev;
> + struct device *otg_dev = gadget->otg_dev;
> +
> + if (!otg_dev)
> + return -EINVAL; /* we're definitely not OTG */
> +
> + /* we're otg but otg controller might not yet be registered */
> + mutex_lock(&otg_list_mutex);
> + otg = usb_otg_get_data(otg_dev);
> + mutex_unlock(&otg_list_mutex);
> + if (!otg) {
> + dev_dbg(gadget_dev,
> + "otg: controller not yet registered. waiting..\n");
> + /*
> + * otg controller might register later. Put the gadget in
> + * wait list and call us back when ready
> + */
> + if (usb_otg_gadget_wait_add(otg_dev, gadget, ops)) {
> + dev_err(gadget_dev,
> + "otg: failed to add to gadget to wait list\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> + }
> +
> + mutex_lock(&otg->fsm.lock);
> + if (otg->gadget) {
> + dev_err(otg_dev, "otg: gadget already registered with otg\n");
> + mutex_unlock(&otg->fsm.lock);
> + return -EINVAL;
> + }
> +
> + otg->gadget = gadget;
> + otg->gadget_ops = ops;
> + dev_info(otg_dev, "otg: gadget %s registered\n",
> + dev_name(&gadget->dev));
> +
> + /* start FSM */
> + usb_otg_start_fsm(otg);
> + mutex_unlock(&otg->fsm.lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register_gadget);
> +
> +/**
> + * usb_otg_unregister_gadget - Unregister the gadget controller from OTG core
> + * @gadget: gadget controller
> + *
> + * This is used by the USB gadget stack to unregister the gadget controller
> + * from the OTG core. Ensures that gadget controller is halted
> + * on successful return.
> + *
> + * Returns: 0 on success, error value otherwise.
> + */
> +int usb_otg_unregister_gadget(struct usb_gadget *gadget)
> +{
> + struct usb_otg *otg;
> + struct device *gadget_dev = &gadget->dev;
> + struct device *otg_dev = gadget->otg_dev;
> +
> + if (!otg_dev)
> + return -EINVAL;
> +
> + mutex_lock(&otg_list_mutex);
> + otg = usb_otg_get_data(otg_dev);
> + mutex_unlock(&otg_list_mutex);
> + if (!otg) {
> + /* are we in wait list? */
> + if (!usb_otg_gadget_wait_remove(gadget))
> + return 0;
> +
> + dev_dbg(gadget_dev, "otg: gadget wasn't registered with otg\n");
> + return -EINVAL;
> + }
> +
> + mutex_lock(&otg->fsm.lock);
> + if (otg->gadget != gadget) {
> + dev_err(otg_dev, "otg: gadget %s wasn't registered with otg\n",
> + dev_name(&gadget->dev));
> + mutex_unlock(&otg->fsm.lock);
> + return -EINVAL;
> + }
> +
> + /* Stop FSM & gadget */
> + usb_otg_stop_fsm(otg);
> + otg->gadget = NULL;
> + mutex_unlock(&otg->fsm.lock);
> +
> + dev_info(otg_dev, "otg: gadget %s unregistered\n",
> + dev_name(&gadget->dev));
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister_gadget);
> diff --git a/drivers/usb/common/usb-otg.h b/drivers/usb/common/usb-otg.h
> new file mode 100644
> index 0000000..2bf3fbf
> --- /dev/null
> +++ b/drivers/usb/common/usb-otg.h
> @@ -0,0 +1,71 @@
> +/**
> + * drivers/usb/common/usb-otg.h - USB OTG core local header
> + *
> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Roger Quadros <rogerq@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __DRIVERS_USB_COMMON_USB_OTG_H
> +#define __DRIVERS_USB_COMMON_USB_OTG_H
> +
> +/*
> + * A-DEVICE timing constants
> + */
> +
> +/* Wait for VBUS Rise */
> +#define TA_WAIT_VRISE (100) /* a_wait_vrise: section 7.1.2
> + * a_wait_vrise_tmr: section 7.4.5.1
> + * TA_VBUS_RISE <= 100ms, section 4.4
> + * Table 4-1: Electrical Characteristics
> + * ->DC Electrical Timing
> + */
> +/* Wait for VBUS Fall */
> +#define TA_WAIT_VFALL (1000) /* a_wait_vfall: section 7.1.7
> + * a_wait_vfall_tmr: section: 7.4.5.2
> + */
> +/* Wait for B-Connect */
> +#define TA_WAIT_BCON (10000) /* a_wait_bcon: section 7.1.3
> + * TA_WAIT_BCON: should be between 1100
> + * and 30000 ms, section 5.5, Table 5-1
> + */
> +/* A-Idle to B-Disconnect */
> +#define TA_AIDL_BDIS (5000) /* a_suspend min 200 ms, section 5.2.1
> + * TA_AIDL_BDIS: section 5.5, Table 5-1
> + */
> +/* B-Idle to A-Disconnect */
> +#define TA_BIDL_ADIS (500) /* TA_BIDL_ADIS: section 5.2.1
> + * 500ms is used for B switch to host
> + * for safe
> + */
> +
> +/*
> + * B-device timing constants
> + */
> +
> +/* Data-Line Pulse Time*/
> +#define TB_DATA_PLS (10) /* b_srp_init,continue 5~10ms
> + * section:5.1.3
> + */
> +/* SRP Fail Time */
> +#define TB_SRP_FAIL (6000) /* b_srp_init,fail time 5~6s
> + * section:5.1.6
> + */
> +/* A-SE0 to B-Reset */
> +#define TB_ASE0_BRST (155) /* minimum 155 ms, section:5.3.1 */
> +/* SE0 Time Before SRP */
> +#define TB_SE0_SRP (1000) /* b_idle,minimum 1s, section:5.1.2 */
> +/* SSEND time before SRP */
> +#define TB_SSEND_SRP (1500) /* minimum 1.5 sec, section:5.1.2 */
> +
> +#define TB_SESS_VLD (1000)
> +
> +#endif /* __DRIVERS_USB_COMMON_USB_OTG_H */
> diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
> index ae228d0..b468a9f 100644
> --- a/drivers/usb/core/Kconfig
> +++ b/drivers/usb/core/Kconfig
> @@ -42,7 +42,7 @@ config USB_DYNAMIC_MINORS
> If you are unsure about this, say N here.
>
> config USB_OTG
> - bool "OTG support"
> + bool "OTG/Dual-role support"
> depends on PM
> default n
> help
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 8c0ae64..1878ae1 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -583,6 +583,7 @@ struct usb_gadget_ops {
> * @out_epnum: last used out ep number
> * @in_epnum: last used in ep number
> * @otg_caps: OTG capabilities of this gadget.
> + * @otg_dev: OTG controller device, if needs to be used with OTG core.
> * @sg_supported: true if we can handle scatter-gather
> * @is_otg: True if the USB device port uses a Mini-AB jack, so that the
> * gadget driver must provide a USB OTG descriptor.
> @@ -639,6 +640,7 @@ struct usb_gadget {
> unsigned out_epnum;
> unsigned in_epnum;
> struct usb_otg_caps *otg_caps;
> + struct device *otg_dev;
>
> unsigned sg_supported:1;
> unsigned is_otg:1;
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 861ccaa..2017cd4 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -184,6 +184,7 @@ struct usb_hcd {
> struct mutex *bandwidth_mutex;
> struct usb_hcd *shared_hcd;
> struct usb_hcd *primary_hcd;
> + struct device *otg_dev; /* OTG controller device */
>
>
> #define HCD_BUFFER_POOLS 4
> diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
> index 36f0cf9..ba6755c 100644
> --- a/include/linux/usb/otg-fsm.h
> +++ b/include/linux/usb/otg-fsm.h
> @@ -61,6 +61,11 @@ enum otg_fsm_timer {
> /**
> * struct otg_fsm - OTG state machine according to the OTG spec
> *
> + * DRD mode hardware Inputs
> + *
> + * @id: TRUE for B-device, FALSE for A-device.
> + * @b_sess_vld: VBUS voltage in regulation.
> + *
> * OTG hardware Inputs
> *
> * Common inputs for A and B device
> @@ -133,6 +138,7 @@ enum otg_fsm_timer {
> * a_clr_err: Asserted (by application ?) to clear a_vbus_err due to an
> * overcurrent condition and causes the A-device to transition
> * to a_wait_vfall
> + * running: state machine running/stopped indicator
> */
> struct otg_fsm {
> /* Input */
> @@ -188,6 +194,7 @@ struct otg_fsm {
> int b_ase0_brst_tmout;
> int a_bidl_adis_tmout;
>
> + bool running;
> struct otg_fsm_ops *ops;
>
> /* Current usb protocol used: 0:undefine; 1:host; 2:client */
> diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> index 85b8fb5..b094352 100644
> --- a/include/linux/usb/otg.h
> +++ b/include/linux/usb/otg.h
> @@ -10,10 +10,55 @@
> #define __LINUX_USB_OTG_H
>
> #include <linux/phy/phy.h>
> -#include <linux/usb/phy.h>
> -#include <linux/usb/otg-fsm.h>
> +#include <linux/device.h>
> +#include <linux/hrtimer.h>
> +#include <linux/ktime.h>
> +#include <linux/usb.h>
> #include <linux/usb/hcd.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/otg-fsm.h>
> +#include <linux/usb/phy.h>
>
> +/**
> + * struct otg_hcd - host controller state and interface
> + *
> + * @hcd: host controller
> + * @irqnum: irq number
> + * @irqflags: irq flags
> + * @ops: otg to host controller interface
> + * @ops: otg to host controller interface
> + * @otg_dev: otg controller device
> + */
> +struct otg_hcd {
> + struct usb_hcd *hcd;
> + unsigned int irqnum;
> + unsigned long irqflags;
> + struct otg_hcd_ops *ops;
> + struct device *otg_dev;
> +};
> +
> +/**
> + * struct usb_otg - usb otg controller state
> + *
> + * @default_a: Indicates we are an A device. i.e. Host.
> + * @phy: USB phy interface
> + * @usb_phy: old usb_phy interface
> + * @host: host controller bus
> + * @gadget: gadget device
> + * @state: current otg state
> + * @dev: otg controller device
> + * @caps: otg capabilities revision, hnp, srp, etc
> + * @fsm: otg finite state machine
> + * @hcd_ops: host controller interface
> + * ------- internal use only -------
> + * @primary_hcd: primary host state and interface
> + * @shared_hcd: shared host state and interface
> + * @gadget_ops: gadget controller interface
> + * @list: list of otg controllers
> + * @work: otg state machine work
> + * @wq: otg state machine work queue
> + * @flags: to track if host/gadget is running
> + */
> struct usb_otg {
> u8 default_a;
>
> @@ -24,9 +69,24 @@ struct usb_otg {
> struct usb_gadget *gadget;
>
> enum usb_otg_state state;
> + struct device *dev;
> + struct usb_otg_caps *caps;
> struct otg_fsm fsm;
> struct otg_hcd_ops *hcd_ops;
>
> + /* internal use only */
> + struct otg_hcd primary_hcd;
> + struct otg_hcd shared_hcd;
> + struct otg_gadget_ops *gadget_ops;
> + struct list_head list;
> + struct work_struct work;
> + struct workqueue_struct *wq;
> + u32 flags;
> +#define OTG_FLAG_GADGET_RUNNING (1 << 0)
> +#define OTG_FLAG_HOST_RUNNING (1 << 1)
> + /* use otg->fsm.lock for serializing access */
> +
> +/*------------- deprecated interface -----------------------------*/
> /* bind/unbind the host controller */
> int (*set_host)(struct usb_otg *otg, struct usb_bus *host);
>
> @@ -42,7 +102,7 @@ struct usb_otg {
>
> /* start or continue HNP role switch */
> int (*start_hnp)(struct usb_otg *otg);
> -
> +/*---------------------------------------------------------------*/
> };
>
> /**
> @@ -60,8 +120,92 @@ struct usb_otg_caps {
> bool adp_support;
> };
>
> +/**
> + * struct usb_otg_config - otg controller configuration
> + * @caps: otg capabilities of the controller
> + * @ops: otg fsm operations
> + * @otg_work: optional custom otg state machine work function
> + */
> +struct usb_otg_config {
> + struct usb_otg_caps *otg_caps;
> + struct otg_fsm_ops *fsm_ops;
> + void (*otg_work)(struct work_struct *work);
> +};
> +
> extern const char *usb_otg_state_string(enum usb_otg_state state);
>
> +#if IS_ENABLED(CONFIG_USB_OTG)
> +struct usb_otg *usb_otg_register(struct device *dev,
> + struct usb_otg_config *config);
> +int usb_otg_unregister(struct device *dev);
> +int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
> + unsigned long irqflags, struct otg_hcd_ops *ops);
> +int usb_otg_unregister_hcd(struct usb_hcd *hcd);
> +int usb_otg_register_gadget(struct usb_gadget *gadget,
> + struct otg_gadget_ops *ops);
> +int usb_otg_unregister_gadget(struct usb_gadget *gadget);
> +void usb_otg_sync_inputs(struct usb_otg *otg);
> +int usb_otg_kick_fsm(struct device *otg_dev);
> +int usb_otg_start_host(struct usb_otg *otg, int on);
> +int usb_otg_start_gadget(struct usb_otg *otg, int on);
> +
> +#else /* CONFIG_USB_OTG */
> +
> +static inline struct usb_otg *usb_otg_register(struct device *dev,
> + struct usb_otg_config *config)
> +{
> + return ERR_PTR(-ENOTSUPP);
> +}
> +
> +static inline int usb_otg_unregister(struct device *dev)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
> + unsigned long irqflags,
> + struct otg_hcd_ops *ops)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int usb_otg_unregister_hcd(struct usb_hcd *hcd)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int usb_otg_register_gadget(struct usb_gadget *gadget,
> + struct otg_gadget_ops *ops)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int usb_otg_unregister_gadget(struct usb_gadget *gadget)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline void usb_otg_sync_inputs(struct usb_otg *otg)
> +{
> +}
> +
> +static inline int usb_otg_kick_fsm(struct device *otg_dev)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int usb_otg_start_host(struct usb_otg *otg, int on)
> +{
> + return -ENOTSUPP;
> +}
> +
> +static inline int usb_otg_start_gadget(struct usb_otg *otg, int on)
> +{
> + return -ENOTSUPP;
> +}
> +#endif /* CONFIG_USB_OTG */
> +
> +/*------------- deprecated interface -----------------------------*/
> /* Context: can sleep */
> static inline int
> otg_start_hnp(struct usb_otg *otg)
> @@ -113,6 +257,8 @@ otg_start_srp(struct usb_otg *otg)
> return -ENOTSUPP;
> }
>
> +/*---------------------------------------------------------------*/
> +
> /* for OTG controller drivers (and maybe other stuff) */
> extern int usb_bus_start_enum(struct usb_bus *bus, unsigned port_num);
>
> @@ -237,4 +383,6 @@ static inline int otg_start_gadget(struct usb_otg *otg, int on)
> return otg->fsm.ops->start_gadget(otg, on);
> }
>
> +int drd_statemachine(struct usb_otg *otg);
> +
> #endif /* __LINUX_USB_OTG_H */
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2016-04-15 13:10 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <ro9No-21n-11@gated-at.bofh.it> |
| In reply to | #1379647 |
On 15/04/16 12:25, Peter Chen wrote:
> On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
>> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
>> + * @dev: OTG/dual-role controller device.
>> + * @config: OTG configuration.
>> + *
>> + * Registers the OTG/dual-role controller device with the USB OTG core.
>> + *
>> + * Return: struct usb_otg * if success, ERR_PTR() if error.
>> + */
>> +struct usb_otg *usb_otg_register(struct device *dev,
>> + struct usb_otg_config *config)
>> +{
>> + struct usb_otg *otg;
>> + struct otg_wait_data *wait;
>> + int ret = 0;
>> +
>> + if (!dev || !config || !config->fsm_ops)
>> + return ERR_PTR(-EINVAL);
>> +
>> + /* already in list? */
>> + mutex_lock(&otg_list_mutex);
>> + if (usb_otg_get_data(dev)) {
>> + dev_err(dev, "otg: %s: device already in otg list\n",
>> + __func__);
>> + ret = -EINVAL;
>> + goto unlock;
>> + }
>> +
>> + /* allocate and add to list */
>> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>> + if (!otg) {
>> + ret = -ENOMEM;
>> + goto unlock;
>> + }
>> +
>> + otg->dev = dev;
>> + otg->caps = config->otg_caps;
>> +
>> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
>> + otg->caps->adp_support) && !config->otg_work)
>> + dev_info(dev, "otg: limiting to dual-role\n");
>
> What does above mean? Customized otg_work item may be dual-role,
> may be full otg.
I'm checking for !config->otg_work so we're sure of using the
default dual-role only work function.
>
>> +
>> + if (config->otg_work) /* custom otg_work ? */
>> + INIT_WORK(&otg->work, config->otg_work);
>> + else
>> + INIT_WORK(&otg->work, usb_otg_work);
>> +
>> + otg->wq = create_singlethread_workqueue("usb_otg");
>> + if (!otg->wq) {
>> + dev_err(dev, "otg: %s: can't create workqueue\n",
>> + __func__);
>> + ret = -ENOMEM;
>> + goto err_wq;
>> + }
>
> I found a bug that caused by non-freezable workqueue, change it
> as freezable please.
>
> https://marc.ttias.be/linux-stable/2016-03/msg00723.php
OK.
>
> I will review the whole set from next week.
Thanks.
--
cheers,
-roger
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-04-18 04:20 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rp6X8-66A-3@gated-at.bofh.it> |
| In reply to | #1379729 |
On Fri, Apr 15, 2016 at 02:00:46PM +0300, Roger Quadros wrote:
> On 15/04/16 12:25, Peter Chen wrote:
> > On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> >> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> >> + * @dev: OTG/dual-role controller device.
> >> + * @config: OTG configuration.
> >> + *
> >> + * Registers the OTG/dual-role controller device with the USB OTG core.
> >> + *
> >> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> >> + */
> >> +struct usb_otg *usb_otg_register(struct device *dev,
> >> + struct usb_otg_config *config)
> >> +{
> >> + struct usb_otg *otg;
> >> + struct otg_wait_data *wait;
> >> + int ret = 0;
> >> +
> >> + if (!dev || !config || !config->fsm_ops)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + /* already in list? */
> >> + mutex_lock(&otg_list_mutex);
> >> + if (usb_otg_get_data(dev)) {
> >> + dev_err(dev, "otg: %s: device already in otg list\n",
> >> + __func__);
> >> + ret = -EINVAL;
> >> + goto unlock;
> >> + }
> >> +
> >> + /* allocate and add to list */
> >> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> >> + if (!otg) {
> >> + ret = -ENOMEM;
> >> + goto unlock;
> >> + }
> >> +
> >> + otg->dev = dev;
> >> + otg->caps = config->otg_caps;
> >> +
> >> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> >> + otg->caps->adp_support) && !config->otg_work)
> >> + dev_info(dev, "otg: limiting to dual-role\n");
> >
> > What does above mean? Customized otg_work item may be dual-role,
> > may be full otg.
>
> I'm checking for !config->otg_work so we're sure of using the
> default dual-role only work function.
>
I see. But whether it is dual-role or fully otg should depend on
otg->caps, the work item may be different according to design.
Besides, your code seems to depend on one of the otg capabilities
for dual-role.
--
Best Regards,
Peter Chen
[toc] | [prev] | [next] | [standalone]
| From | Roger Quadros <rogerq@ti.com> |
|---|---|
| Date | 2016-04-20 09:00 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rpUhc-3vS-19@gated-at.bofh.it> |
| In reply to | #1380899 |
On 18/04/16 05:09, Peter Chen wrote:
> On Fri, Apr 15, 2016 at 02:00:46PM +0300, Roger Quadros wrote:
>> On 15/04/16 12:25, Peter Chen wrote:
>>> On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
>>>> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
>>>> + * @dev: OTG/dual-role controller device.
>>>> + * @config: OTG configuration.
>>>> + *
>>>> + * Registers the OTG/dual-role controller device with the USB OTG core.
>>>> + *
>>>> + * Return: struct usb_otg * if success, ERR_PTR() if error.
>>>> + */
>>>> +struct usb_otg *usb_otg_register(struct device *dev,
>>>> + struct usb_otg_config *config)
>>>> +{
>>>> + struct usb_otg *otg;
>>>> + struct otg_wait_data *wait;
>>>> + int ret = 0;
>>>> +
>>>> + if (!dev || !config || !config->fsm_ops)
>>>> + return ERR_PTR(-EINVAL);
>>>> +
>>>> + /* already in list? */
>>>> + mutex_lock(&otg_list_mutex);
>>>> + if (usb_otg_get_data(dev)) {
>>>> + dev_err(dev, "otg: %s: device already in otg list\n",
>>>> + __func__);
>>>> + ret = -EINVAL;
>>>> + goto unlock;
>>>> + }
>>>> +
>>>> + /* allocate and add to list */
>>>> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>>>> + if (!otg) {
>>>> + ret = -ENOMEM;
>>>> + goto unlock;
>>>> + }
>>>> +
>>>> + otg->dev = dev;
>>>> + otg->caps = config->otg_caps;
>>>> +
>>>> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
>>>> + otg->caps->adp_support) && !config->otg_work)
>>>> + dev_info(dev, "otg: limiting to dual-role\n");
>>>
>>> What does above mean? Customized otg_work item may be dual-role,
>>> may be full otg.
>>
>> I'm checking for !config->otg_work so we're sure of using the
>> default dual-role only work function.
>>
>
> I see. But whether it is dual-role or fully otg should depend on
> otg->caps, the work item may be different according to design.
> Besides, your code seems to depend on one of the otg capabilities
> for dual-role.
>
Which capability? Id/vbus status?
Are you OK with this patch?
cheers,
-roger
[toc] | [prev] | [next] | [standalone]
| From | Peter Chen <hzpeterchen@gmail.com> |
|---|---|
| Date | 2016-04-20 11:40 +0200 |
| Subject | Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core |
| Message-ID | <rpWM2-5HI-17@gated-at.bofh.it> |
| In reply to | #1383086 |
On Wed, Apr 20, 2016 at 09:54:49AM +0300, Roger Quadros wrote:
> On 18/04/16 05:09, Peter Chen wrote:
> > On Fri, Apr 15, 2016 at 02:00:46PM +0300, Roger Quadros wrote:
> >> On 15/04/16 12:25, Peter Chen wrote:
> >>> On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> >>>> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> >>>> + * @dev: OTG/dual-role controller device.
> >>>> + * @config: OTG configuration.
> >>>> + *
> >>>> + * Registers the OTG/dual-role controller device with the USB OTG core.
> >>>> + *
> >>>> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> >>>> + */
> >>>> +struct usb_otg *usb_otg_register(struct device *dev,
> >>>> + struct usb_otg_config *config)
> >>>> +{
> >>>> + struct usb_otg *otg;
> >>>> + struct otg_wait_data *wait;
> >>>> + int ret = 0;
> >>>> +
> >>>> + if (!dev || !config || !config->fsm_ops)
> >>>> + return ERR_PTR(-EINVAL);
> >>>> +
> >>>> + /* already in list? */
> >>>> + mutex_lock(&otg_list_mutex);
> >>>> + if (usb_otg_get_data(dev)) {
> >>>> + dev_err(dev, "otg: %s: device already in otg list\n",
> >>>> + __func__);
> >>>> + ret = -EINVAL;
> >>>> + goto unlock;
> >>>> + }
> >>>> +
> >>>> + /* allocate and add to list */
> >>>> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> >>>> + if (!otg) {
> >>>> + ret = -ENOMEM;
> >>>> + goto unlock;
> >>>> + }
> >>>> +
> >>>> + otg->dev = dev;
> >>>> + otg->caps = config->otg_caps;
> >>>> +
> >>>> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> >>>> + otg->caps->adp_support) && !config->otg_work)
> >>>> + dev_info(dev, "otg: limiting to dual-role\n");
> >>>
> >>> What does above mean? Customized otg_work item may be dual-role,
> >>> may be full otg.
> >>
> >> I'm checking for !config->otg_work so we're sure of using the
> >> default dual-role only work function.
> >>
> >
> > I see. But whether it is dual-role or fully otg should depend on
> > otg->caps, the work item may be different according to design.
> > Besides, your code seems to depend on one of the otg capabilities
> > for dual-role.
> >
> Which capability? Id/vbus status?
>
if (otg->caps->hnp_support || otg->caps->srp_support || otg->caps->adp_support)
it is fully otg;
else
it is dual role;
Do you think so?
--
Best Regards,
Peter Chen
[toc] | [prev] | [next] | [standalone]
Page 2 of 5 — ← Prev page 1 [2] 3 4 5 Next page →
Back to top | Article view | linux.kernel
csiph-web