From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <57398C16.5050805@ti.com> References: <1463133808-10630-1-git-send-email-rogerq@ti.com> <1463133808-10630-9-git-send-email-rogerq@ti.com> <57398C16.5050805@ti.com> From: Peter Chen Date: Wed, 18 May 2016 15:45:51 +0800 Message-ID: Subject: Re: [PATCH v8 08/14] usb: otg: add OTG/dual-role core Content-Type: multipart/alternative; boundary=001a11c372dc23c1e50533190d24 To: Roger Quadros Cc: Peter Chen , balbi@kernel.org, tony@atomide.com, Greg Kroah-Hartman , dan.j.williams@intel.com, mathias.nyman@linux.intel.com, Joao.Pinto@synopsys.com, sergei.shtylyov@cogentembedded.com, jun.li@freescale.com, grygorii.strashko@ti.com, yoshihiro.shimoda.uh@renesas.com, robh@kernel.org, nsekhar@ti.com, b-liu@ti.com, "linux-usb@vger.kernel.org" , linux-omap@vger.kernel.org, lkml , devicetree@vger.kernel.org List-ID: --001a11c372dc23c1e50533190d24 Content-Type: text/plain; charset=UTF-8 On Mon, May 16, 2016 at 5:00 PM, Roger Quadros 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 > > --- > > 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. 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- BR, Peter Chen --001a11c372dc23c1e50533190d24 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Mon, May 16, 2016 at 5:00 PM, Roger Quadros <rogerq@ti.com> 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 <roge= rq@ti.com>
> ---
>=C2=A0 drivers/usb/common/Makefile=C2=A0 |=C2=A0 =C2=A0 2 +-
>=C2=A0 drivers/usb/common/usb-otg.c | 1042 ++++++++++++++++++++++++++++= ++++++++++++++
>=C2=A0 drivers/usb/core/Kconfig=C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A0 4 +-<= br> >=C2=A0 include/linux/usb/gadget.h=C2=A0 =C2=A0|=C2=A0 =C2=A0 2 +
>=C2=A0 include/linux/usb/hcd.h=C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A0 1 + >=C2=A0 include/linux/usb/otg-fsm.h=C2=A0 |=C2=A0 =C2=A0 7 +
>=C2=A0 include/linux/usb/otg.h=C2=A0 =C2=A0 =C2=A0 |=C2=A0 154 ++++++-<= br> >=C2=A0 7 files changed, 1206 insertions(+), 6 deletions(-)
>=C2=A0 create mode 100644 drivers/usb/common/usb-otg.c


This patch causes the following build issues when CONFIG_USB_GA= DGET=3Dm, CONFIG_USB=3Dm,
CONFIG_USB_COMMON=3Dm and CONFIG_USB_OTG=3Dy

ERROR: "usb_otg_register_gadget" [drivers/usb/gadget/udc/udc-core= .ko] undefined!
ERROR: "usb_otg_unregister_gadget" [drivers/usb/gadget/udc/udc-co= re.ko] undefined!
ERROR: "usb_otg_register_hcd" [drivers/usb/core/usbcore.ko] undef= ined!
ERROR: "usb_otg_unregister_hcd" [drivers/usb/core/usbcore.ko] und= efined!
ERROR: "otg_statemachine" [drivers/usb/chipidea/ci_hdrc.ko] undef= ined!
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= 9;
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)=C2=A0 =C2=A0 =C2=A0 =C2= =A0+=3D renesas_usbhs/
=C2=A0obj-$(CONFIG_USB_GADGET)=C2=A0 =C2=A0 =C2=A0 =C2=A0+=3D gadget/

=C2=A0obj-$(CONFIG_USB_COMMON)=C2=A0 =C2=A0 =C2=A0 =C2=A0+=3D common/
+obj-$(CONFIG_USB_OTG)=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 +=3D common/

=C2=A0obj-$(CONFIG_USBIP_CORE)=C2=A0 =C2=A0 =C2=A0 =C2=A0+=3D 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 *hc= d)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return hcd =3D=3D hcd->prim= ary_hcd;
=C2=A0}

+static const char *otg_state_string(enum usb_otg_state state)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0static const char *const names[] =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_A_IDLE] = =3D "a_idle",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_A_WAIT_V= RISE] =3D "a_wait_vrise",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_A_WAIT_B= CON] =3D "a_wait_bcon",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_A_HOST] = =3D "a_host",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_A_SUSPEN= D] =3D "a_suspend",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_A_PERIPH= ERAL] =3D "a_peripheral",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_A_WAIT_V= FALL] =3D "a_wait_vfall",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_A_VBUS_E= RR] =3D "a_vbus_err",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_B_IDLE] = =3D "b_idle",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_B_SRP_IN= IT] =3D "b_srp_init",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_B_PERIPH= ERAL] =3D "b_peripheral",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_B_WAIT_A= CON] =3D "b_wait_acon",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0[OTG_STATE_B_HOST] = =3D "b_host",
+=C2=A0 =C2=A0 =C2=A0 =C2=A0};
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (state < 0 || state >=3D ARRAY_SIZE(na= mes))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return "UNDEFI= NED";
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return names[state];
+}
+


From my point, make an= other copy for otg stuff is not a good way,
could we make folder = under usb/ named otg for dedicated otg stuffs,
in that case, buil= d otg stuffs can not depend on USB_COMMON.

Peter
=C2=A0
=C2=A0/**
=C2=A0 * Check if the OTG device is in our wait list and r= eturn
=C2=A0 * otg_wait_data, else NULL.
@@ -433,7 +457,7 @@ static void drd_set_state(struct otg_fsm *fsm, enum usb= _otg_state new_state)

=C2=A0 =C2=A0 =C2=A0 =C2=A0 fsm->state_changed =3D 1;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_dbg(otg->dev, "otg= : set state: %s\n",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0usb_otg_stat= e_string(new_state));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0otg_state_string(ne= w_state));
=C2=A0 =C2=A0 =C2=A0 =C2=A0 switch (new_state) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 case OTG_STATE_B_IDLE:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 drd_set_protocol(fs= m, PROTO_UNDEF);
@@ -449,7 +473,7 @@ static void drd_set_state(struct otg_fsm *fsm, enum usb= _otg_state new_state)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 default:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 de= v_warn(otg->dev, "%s: otg: invalid state: %s\n",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 __func__, usb_otg_state_string(new_state));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 __func__, otg_state_string(new_state));
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

@@ -507,7 +531,7 @@ int drd_statemachine(struct usb_otg *otg)

=C2=A0 =C2=A0 =C2=A0 =C2=A0 default:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 de= v_err(otg->dev, "%s: otg: invalid usb-drd state: %s\n",
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0__func__, usb_otg_state_string(state));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0__func__, otg_state_string(state));
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

--
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb&qu= ot; in
the body of a message to major= domo@vger.kernel.org
More majordomo info at=C2=A0 http://vger.kernel.org/majord= omo-info.html



--
=
BR,
Peter Chen
--001a11c372dc23c1e50533190d24--