Re: [PATCH v8 08/14] usb: otg: add OTG/dual-role core

From: Roger Quadros
Date: Wed May 18 2016 - 09:00:23 EST

Hi Peter,

On 18/05/16 10:45, Peter Chen wrote:
>
>
> On Mon, May 16, 2016 at 5:00 PM, Roger Quadros <rogerq@xxxxxx <mailto:rogerq@xxxxxx>> wrote:
>
> On 13/05/16 13:03, Roger Quadros wrote:
> > It provides APIs for the following tasks
> >
> > - Registering an OTG/dual-role capable controller
> > - Registering Host and Gadget controllers to OTG core
> > - Providing inputs to and kicking the OTG state machine
> >
> > Provide a dual-role device (DRD) state machine.
> > DRD mode is a reduced functionality OTG mode. In this mode
> > we don't support SRP, HNP and dynamic role-swap.
> >
> > In DRD operation, the controller mode (Host or Peripheral)
> > is decided based on the ID pin status. Once a cable plug (Type-A
> > or Type-B) is attached the controller selects the state
> > and doesn't change till the cable in unplugged and a different
> > cable type is inserted.
> >
> > As we don't need most of the complex OTG states and OTG timers
> > we implement a lean DRD state machine in usb-otg.c.
> > The DRD state machine is only interested in 2 hardware inputs
> > 'id' and 'b_sess_vld'.
> >
> > Signed-off-by: Roger Quadros <rogerq@xxxxxx <mailto:rogerq@xxxxxx>>
> > ---
> > drivers/usb/common/Makefile | 2 +-
> > drivers/usb/common/usb-otg.c | 1042 ++++++++++++++++++++++++++++++++++++++++++
> > drivers/usb/core/Kconfig | 4 +-
> > include/linux/usb/gadget.h | 2 +
> > include/linux/usb/hcd.h | 1 +
> > include/linux/usb/otg-fsm.h | 7 +
> > include/linux/usb/otg.h | 154 ++++++-
> > 7 files changed, 1206 insertions(+), 6 deletions(-)
> > create mode 100644 drivers/usb/common/usb-otg.c
>
>
> This patch causes the following build issues when CONFIG_USB_GADGET=m, CONFIG_USB=m,
> CONFIG_USB_COMMON=m and CONFIG_USB_OTG=y
>
> ERROR: "usb_otg_register_gadget" [drivers/usb/gadget/udc/udc-core.ko] undefined!
> ERROR: "usb_otg_unregister_gadget" [drivers/usb/gadget/udc/udc-core.ko] undefined!
> ERROR: "usb_otg_register_hcd" [drivers/usb/core/usbcore.ko] undefined!
> ERROR: "usb_otg_unregister_hcd" [drivers/usb/core/usbcore.ko] undefined!
> ERROR: "otg_statemachine" [drivers/usb/chipidea/ci_hdrc.ko] undefined!
> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
> make[1]: *** [__modpost] Error 1
> Makefile:1141: recipe for target 'modules' failed
> make: *** [modules] Error 2
> make: *** Waiting for unfinished jobs....
>
> drivers/built-in.o: In function `drd_set_state':
> usb-otg.c:(.text+0x2b4242): undefined reference to `usb_otg_state_string'
> drivers/built-in.o: In function `drd_statemachine':
> (.text+0x2b4b4c): undefined reference to `usb_otg_state_string'
> Makefile:937: recipe for target 'vmlinux' failed
>
> I'll fix it up with the following diff.
>
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index dca7856..16a5b55 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -59,5 +59,6 @@ obj-$(CONFIG_USB_RENESAS_USBHS) += renesas_usbhs/
> obj-$(CONFIG_USB_GADGET) += gadget/
>
> obj-$(CONFIG_USB_COMMON) += common/
> +obj-$(CONFIG_USB_OTG) += common/
>
> obj-$(CONFIG_USBIP_CORE) += usbip/
> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> index 77048aa..17e449e 100644
> --- a/drivers/usb/common/usb-otg.c
> +++ b/drivers/usb/common/usb-otg.c
> @@ -56,6 +56,30 @@ static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
> return hcd == hcd->primary_hcd;
> }
>
> +static const char *otg_state_string(enum usb_otg_state state)
> +{
> + static const char *const names[] = {
> + [OTG_STATE_A_IDLE] = "a_idle",
> + [OTG_STATE_A_WAIT_VRISE] = "a_wait_vrise",
> + [OTG_STATE_A_WAIT_BCON] = "a_wait_bcon",
> + [OTG_STATE_A_HOST] = "a_host",
> + [OTG_STATE_A_SUSPEND] = "a_suspend",
> + [OTG_STATE_A_PERIPHERAL] = "a_peripheral",
> + [OTG_STATE_A_WAIT_VFALL] = "a_wait_vfall",
> + [OTG_STATE_A_VBUS_ERR] = "a_vbus_err",
> + [OTG_STATE_B_IDLE] = "b_idle",
> + [OTG_STATE_B_SRP_INIT] = "b_srp_init",
> + [OTG_STATE_B_PERIPHERAL] = "b_peripheral",
> + [OTG_STATE_B_WAIT_ACON] = "b_wait_acon",
> + [OTG_STATE_B_HOST] = "b_host",
> + };
> +
> + if (state < 0 || state >= ARRAY_SIZE(names))
> + return "UNDEFINED";
> +
> + return names[state];
> +}
> +
>
>
>
> From my point, make another copy for otg stuff is not a good way,
> could we make folder under usb/ named otg for dedicated otg stuffs,
> in that case, build otg stuffs can not depend on USB_COMMON.

OK. I can try that. I'll delete otg_state_string from usb-common.c and
move it into usb/otg/usb-otg.c

I'll also move usb-otg-fsm.c to usb/otg/.

cheers,
-roger

>
> Peter
>
>
> /**
> * Check if the OTG device is in our wait list and return
> * otg_wait_data, else NULL.
> @@ -433,7 +457,7 @@ static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)
>
> fsm->state_changed = 1;
> dev_dbg(otg->dev, "otg: set state: %s\n",
> - usb_otg_state_string(new_state));
> + otg_state_string(new_state));
> switch (new_state) {
> case OTG_STATE_B_IDLE:
> drd_set_protocol(fsm, PROTO_UNDEF);
> @@ -449,7 +473,7 @@ static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)
> break;
> default:
> dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
> - __func__, usb_otg_state_string(new_state));
> + __func__, otg_state_string(new_state));
> break;
> }
>
> @@ -507,7 +531,7 @@ int drd_statemachine(struct usb_otg *otg)
>
> default:
> dev_err(otg->dev, "%s: otg: invalid usb-drd state: %s\n",
> - __func__, usb_otg_state_string(state));
> + __func__, otg_state_string(state));
> break;
> }
>
> --
> cheers,
> -roger
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx <mailto:majordomo@xxxxxxxxxxxxxxx>
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
>
> --
> BR,
> Peter Chen