diff mbox

[RFC] usb: dwc3: Remove dwc3 dependency on gadget.

Message ID 1356357513-8892-1-git-send-email-gautam.vivek@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vivek Gautam Dec. 24, 2012, 1:58 p.m. UTC
DWC3 controller curretly depends on CONFIG_USB and CONFIG_USB_GADGET.
Some hardware may like to use only host feature on dwc3.
So, removing the dependency of USB_DWC3 on USB_GADGET
and further modulating the dwc3 core to enable gadget features
only with USB_GADGET.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Doug Anderson <dianders@chromium.org>
---
 drivers/usb/dwc3/Kconfig   |    4 ++--
 drivers/usb/dwc3/Makefile  |    2 +-
 drivers/usb/dwc3/core.h    |    7 +++++++
 drivers/usb/dwc3/debugfs.c |    6 ++++--
 drivers/usb/dwc3/gadget.h  |    8 ++++++++
 5 files changed, 22 insertions(+), 5 deletions(-)

Comments

Felipe Balbi Jan. 10, 2013, 1:02 p.m. UTC | #1
Hi,

On Mon, Dec 24, 2012 at 07:28:33PM +0530, Vivek Gautam wrote:
> DWC3 controller curretly depends on CONFIG_USB and CONFIG_USB_GADGET.
> Some hardware may like to use only host feature on dwc3.
> So, removing the dependency of USB_DWC3 on USB_GADGET
> and further modulating the dwc3 core to enable gadget features
> only with USB_GADGET.
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> CC: Doug Anderson <dianders@chromium.org>

right, right... Eventually we need to do it, but you're only making
gadget side optional. Host side should be optional too, but then you
need to make sure we don't build dwc3 without gadget and host.

Maybe make a mode selection for Host-only, Peripheral-only, Dual-Role
Device ??

More comments below...

> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index f6a6e07..b70dcf1 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -1,7 +1,7 @@
>  config USB_DWC3
>  	tristate "DesignWare USB3 DRD Core Support"
> -	depends on (USB && USB_GADGET)
> -	select USB_OTG_UTILS
> +	depends on USB
> +	select USB_OTG_UTILS if USB_GADGET

we want USB_OTG_UTILS also on host-only builds because host side,
eventually, will have to learn how to control its PHYs.

>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>  	help
>  	  Say Y or M here if your system has a Dual Role SuperSpeed
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index 4502648..8f469cb 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -5,7 +5,7 @@ obj-$(CONFIG_USB_DWC3)			+= dwc3.o
>  
>  dwc3-y					:= core.o
>  dwc3-y					+= host.o
> -dwc3-y					+= gadget.o ep0.o
> +obj-$(CONFIG_USB_GADGET)               += gadget.o ep0.o

this is wrong. CONFIG_USB_GADGET is tristate, so this should be:

ifneq($(CONFIG_USB_GADGET),)
 dwc3-y	+= gadget.o ep0.o
endif

or something similar

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4999563..4dc7ef2 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -865,7 +865,14 @@ int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc);
>  int dwc3_host_init(struct dwc3 *dwc);
>  void dwc3_host_exit(struct dwc3 *dwc);
>  
> +#ifdef CONFIG_USB_GADGET

not taking into account module builds.

>  int dwc3_gadget_init(struct dwc3 *dwc);
>  void dwc3_gadget_exit(struct dwc3 *dwc);
> +#else
> +static inline int dwc3_gadget_init(struct dwc3 *dwc)
> +{ return -EINVAL; }
> +static inline void dwc3_gadget_exit(struct dwc3 *dwc)
> +{ }
> +#endif
>  
>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index d4a30f1..553bbaa 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -535,7 +535,8 @@ static ssize_t dwc3_testmode_write(struct file *file,
>  		testmode = 0;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
> -	dwc3_gadget_set_test_mode(dwc, testmode);
> +	if (dwc3_gadget_set_test_mode(dwc, testmode))
> +		dev_dbg(dwc->dev, "host: Invalid request\n");
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
>  	return count;

wrong, if you don't have gadget mode, you just don't create this file.

> @@ -638,7 +639,8 @@ static ssize_t dwc3_link_state_write(struct file *file,
>  		return -EINVAL;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
> -	dwc3_gadget_set_link_state(dwc, state);
> +	if (dwc3_gadget_set_link_state(dwc, state))
> +		dev_dbg(dwc->dev, "host: Invalid request\n");

likewise.

>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
>  	return count;
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 99e6d72..28e82db 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -105,8 +105,16 @@ static inline void dwc3_gadget_move_request_queued(struct dwc3_request *req)
>  void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
>  		int status);
>  
> +#ifdef CONFIG_USB_GADGET

not taking into account CONFIG_USB_GADGET=m
Vivek Gautam Jan. 11, 2013, 1:43 p.m. UTC | #2
Hi Felipe,


On Thu, Jan 10, 2013 at 6:32 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Dec 24, 2012 at 07:28:33PM +0530, Vivek Gautam wrote:
>> DWC3 controller curretly depends on CONFIG_USB and CONFIG_USB_GADGET.
>> Some hardware may like to use only host feature on dwc3.
>> So, removing the dependency of USB_DWC3 on USB_GADGET
>> and further modulating the dwc3 core to enable gadget features
>> only with USB_GADGET.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> CC: Doug Anderson <dianders@chromium.org>
>
> right, right... Eventually we need to do it, but you're only making
> gadget side optional. Host side should be optional too, but then you
> need to make sure we don't build dwc3 without gadget and host.
>

Yes, true we need to make host side also optional, build dwc3 only when
either of host or gadget are built.

> Maybe make a mode selection for Host-only, Peripheral-only, Dual-Role
> Device ??
>

True this will be good idea to use modes: host only, gadget only or dual role.
May be the platform glue layers can use them later to put the controller
in a specific mode ?

> More comments below...
>
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index f6a6e07..b70dcf1 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -1,7 +1,7 @@
>>  config USB_DWC3
>>       tristate "DesignWare USB3 DRD Core Support"
>> -     depends on (USB && USB_GADGET)
>> -     select USB_OTG_UTILS
>> +     depends on USB
>> +     select USB_OTG_UTILS if USB_GADGET
>
> we want USB_OTG_UTILS also on host-only builds because host side,
> eventually, will have to learn how to control its PHYs.
>

Ok.

>>       select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>       help
>>         Say Y or M here if your system has a Dual Role SuperSpeed
>> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
>> index 4502648..8f469cb 100644
>> --- a/drivers/usb/dwc3/Makefile
>> +++ b/drivers/usb/dwc3/Makefile
>> @@ -5,7 +5,7 @@ obj-$(CONFIG_USB_DWC3)                        += dwc3.o
>>
>>  dwc3-y                                       := core.o
>>  dwc3-y                                       += host.o
>> -dwc3-y                                       += gadget.o ep0.o
>> +obj-$(CONFIG_USB_GADGET)               += gadget.o ep0.o
>
> this is wrong. CONFIG_USB_GADGET is tristate, so this should be:
>
> ifneq($(CONFIG_USB_GADGET),)
>  dwc3-y += gadget.o ep0.o
> endif
>
> or something similar
>

True, missed taking into account that CONFIG_USB_GADGET is tristate :-(
Will amend this as suggested.

>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 4999563..4dc7ef2 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -865,7 +865,14 @@ int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc);
>>  int dwc3_host_init(struct dwc3 *dwc);
>>  void dwc3_host_exit(struct dwc3 *dwc);
>>
>> +#ifdef CONFIG_USB_GADGET
>
> not taking into account module builds.
>

ditto

>>  int dwc3_gadget_init(struct dwc3 *dwc);
>>  void dwc3_gadget_exit(struct dwc3 *dwc);
>> +#else
>> +static inline int dwc3_gadget_init(struct dwc3 *dwc)
>> +{ return -EINVAL; }
>> +static inline void dwc3_gadget_exit(struct dwc3 *dwc)
>> +{ }
>> +#endif
>>
>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>> index d4a30f1..553bbaa 100644
>> --- a/drivers/usb/dwc3/debugfs.c
>> +++ b/drivers/usb/dwc3/debugfs.c
>> @@ -535,7 +535,8 @@ static ssize_t dwc3_testmode_write(struct file *file,
>>               testmode = 0;
>>
>>       spin_lock_irqsave(&dwc->lock, flags);
>> -     dwc3_gadget_set_test_mode(dwc, testmode);
>> +     if (dwc3_gadget_set_test_mode(dwc, testmode))
>> +             dev_dbg(dwc->dev, "host: Invalid request\n");
>>       spin_unlock_irqrestore(&dwc->lock, flags);
>>
>>       return count;
>
> wrong, if you don't have gadget mode, you just don't create this file.
>

dwc3-core makes call to dwc3_debugfs_init() invariably depending on DEBUG_FS.
Will this not go ahead and create this file ?
I think i am missing here something.  :-(

>> @@ -638,7 +639,8 @@ static ssize_t dwc3_link_state_write(struct file *file,
>>               return -EINVAL;
>>
>>       spin_lock_irqsave(&dwc->lock, flags);
>> -     dwc3_gadget_set_link_state(dwc, state);
>> +     if (dwc3_gadget_set_link_state(dwc, state))
>> +             dev_dbg(dwc->dev, "host: Invalid request\n");
>
> likewise.
>
>>       spin_unlock_irqrestore(&dwc->lock, flags);
>>
>>       return count;
>> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
>> index 99e6d72..28e82db 100644
>> --- a/drivers/usb/dwc3/gadget.h
>> +++ b/drivers/usb/dwc3/gadget.h
>> @@ -105,8 +105,16 @@ static inline void dwc3_gadget_move_request_queued(struct dwc3_request *req)
>>  void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
>>               int status);
>>
>> +#ifdef CONFIG_USB_GADGET
>
> not taking into account CONFIG_USB_GADGET=m
>
missed taking into account that CONFIG_USB_GADGET is tristate :-(

Will amend this patch accordingly.
Felipe Balbi Jan. 11, 2013, 1:59 p.m. UTC | #3
Hi,

On Fri, Jan 11, 2013 at 07:13:55PM +0530, Vivek Gautam wrote:
> On Thu, Jan 10, 2013 at 6:32 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Mon, Dec 24, 2012 at 07:28:33PM +0530, Vivek Gautam wrote:
> >> DWC3 controller curretly depends on CONFIG_USB and CONFIG_USB_GADGET.
> >> Some hardware may like to use only host feature on dwc3.
> >> So, removing the dependency of USB_DWC3 on USB_GADGET
> >> and further modulating the dwc3 core to enable gadget features
> >> only with USB_GADGET.
> >>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> CC: Doug Anderson <dianders@chromium.org>
> >
> > right, right... Eventually we need to do it, but you're only making
> > gadget side optional. Host side should be optional too, but then you
> > need to make sure we don't build dwc3 without gadget and host.
> >
> 
> Yes, true we need to make host side also optional, build dwc3 only when
> either of host or gadget are built.

btw, make the default Dual-Role, if user/defconfig doesn't select
anything we want to build with all features.

> > Maybe make a mode selection for Host-only, Peripheral-only, Dual-Role
> > Device ??
> >
> 
> True this will be good idea to use modes: host only, gadget only or dual role.
> May be the platform glue layers can use them later to put the controller
> in a specific mode ?

probably not as that's likely to change from board to board.

> >>  int dwc3_gadget_init(struct dwc3 *dwc);
> >>  void dwc3_gadget_exit(struct dwc3 *dwc);
> >> +#else
> >> +static inline int dwc3_gadget_init(struct dwc3 *dwc)
> >> +{ return -EINVAL; }
> >> +static inline void dwc3_gadget_exit(struct dwc3 *dwc)
> >> +{ }
> >> +#endif
> >>
> >>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
> >> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> >> index d4a30f1..553bbaa 100644
> >> --- a/drivers/usb/dwc3/debugfs.c
> >> +++ b/drivers/usb/dwc3/debugfs.c
> >> @@ -535,7 +535,8 @@ static ssize_t dwc3_testmode_write(struct file *file,
> >>               testmode = 0;
> >>
> >>       spin_lock_irqsave(&dwc->lock, flags);
> >> -     dwc3_gadget_set_test_mode(dwc, testmode);
> >> +     if (dwc3_gadget_set_test_mode(dwc, testmode))
> >> +             dev_dbg(dwc->dev, "host: Invalid request\n");
> >>       spin_unlock_irqrestore(&dwc->lock, flags);
> >>
> >>       return count;
> >
> > wrong, if you don't have gadget mode, you just don't create this file.
> >
> 
> dwc3-core makes call to dwc3_debugfs_init() invariably depending on DEBUG_FS.
> Will this not go ahead and create this file ?
> I think i am missing here something.  :-(

right, you can change dwc3_debugfs_init() to take into account the fact
that you're a gadget/drd or host-only.

In case of host only, you stil want regdump to be available :-)
Vivek Gautam Jan. 11, 2013, 2:28 p.m. UTC | #4
Hi,


On Fri, Jan 11, 2013 at 7:29 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Fri, Jan 11, 2013 at 07:13:55PM +0530, Vivek Gautam wrote:
>> On Thu, Jan 10, 2013 at 6:32 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > Hi,
>> >
>> > On Mon, Dec 24, 2012 at 07:28:33PM +0530, Vivek Gautam wrote:
>> >> DWC3 controller curretly depends on CONFIG_USB and CONFIG_USB_GADGET.
>> >> Some hardware may like to use only host feature on dwc3.
>> >> So, removing the dependency of USB_DWC3 on USB_GADGET
>> >> and further modulating the dwc3 core to enable gadget features
>> >> only with USB_GADGET.
>> >>
>> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >> CC: Doug Anderson <dianders@chromium.org>
>> >
>> > right, right... Eventually we need to do it, but you're only making
>> > gadget side optional. Host side should be optional too, but then you
>> > need to make sure we don't build dwc3 without gadget and host.
>> >
>>
>> Yes, true we need to make host side also optional, build dwc3 only when
>> either of host or gadget are built.
>
> btw, make the default Dual-Role, if user/defconfig doesn't select
> anything we want to build with all features.
>

Yes we can try something like this ?

         if (USB || USB_GADGET)
         menuconfig USB_DWC3
                 tristate "DesignWare USB3 DRD Core Support"
          ...

          if USB_DWC3
          choice
          default USB_DWC3_DUAL_ROLE_MODE
          ...

          config USB_DWC3_HOST_MODE
          ...

          config USB_DWC3_DEVICE_MODE
          ...

          config USB_DWC3_DUAL_ROLE_MODE
          ...

         endchoice

         ..
         ..
         endif
         endif


>> > Maybe make a mode selection for Host-only, Peripheral-only, Dual-Role
>> > Device ??
>> >
>>
>> True this will be good idea to use modes: host only, gadget only or dual role.
>> May be the platform glue layers can use them later to put the controller
>> in a specific mode ?
>
> probably not as that's likely to change from board to board.
>
>> >>  int dwc3_gadget_init(struct dwc3 *dwc);
>> >>  void dwc3_gadget_exit(struct dwc3 *dwc);
>> >> +#else
>> >> +static inline int dwc3_gadget_init(struct dwc3 *dwc)
>> >> +{ return -EINVAL; }
>> >> +static inline void dwc3_gadget_exit(struct dwc3 *dwc)
>> >> +{ }
>> >> +#endif
>> >>
>> >>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>> >> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>> >> index d4a30f1..553bbaa 100644
>> >> --- a/drivers/usb/dwc3/debugfs.c
>> >> +++ b/drivers/usb/dwc3/debugfs.c
>> >> @@ -535,7 +535,8 @@ static ssize_t dwc3_testmode_write(struct file *file,
>> >>               testmode = 0;
>> >>
>> >>       spin_lock_irqsave(&dwc->lock, flags);
>> >> -     dwc3_gadget_set_test_mode(dwc, testmode);
>> >> +     if (dwc3_gadget_set_test_mode(dwc, testmode))
>> >> +             dev_dbg(dwc->dev, "host: Invalid request\n");
>> >>       spin_unlock_irqrestore(&dwc->lock, flags);
>> >>
>> >>       return count;
>> >
>> > wrong, if you don't have gadget mode, you just don't create this file.
>> >
>>
>> dwc3-core makes call to dwc3_debugfs_init() invariably depending on DEBUG_FS.
>> Will this not go ahead and create this file ?
>> I think i am missing here something.  :-(
>
> right, you can change dwc3_debugfs_init() to take into account the fact
> that you're a gadget/drd or host-only.
>
> In case of host only, you stil want regdump to be available :-)
>

So in dwc3_debugfs_init() we shall actually create just 'regdump' file
in case of host only mode, otherwise keep the dwc3_debugfs_init() happy
with creating other files too.
Right ?
Felipe Balbi Jan. 14, 2013, 7:57 a.m. UTC | #5
On Fri, Jan 11, 2013 at 07:58:23PM +0530, Vivek Gautam wrote:
> Hi,
> 
> 
> On Fri, Jan 11, 2013 at 7:29 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Fri, Jan 11, 2013 at 07:13:55PM +0530, Vivek Gautam wrote:
> >> On Thu, Jan 10, 2013 at 6:32 PM, Felipe Balbi <balbi@ti.com> wrote:
> >> > Hi,
> >> >
> >> > On Mon, Dec 24, 2012 at 07:28:33PM +0530, Vivek Gautam wrote:
> >> >> DWC3 controller curretly depends on CONFIG_USB and CONFIG_USB_GADGET.
> >> >> Some hardware may like to use only host feature on dwc3.
> >> >> So, removing the dependency of USB_DWC3 on USB_GADGET
> >> >> and further modulating the dwc3 core to enable gadget features
> >> >> only with USB_GADGET.
> >> >>
> >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> >> CC: Doug Anderson <dianders@chromium.org>
> >> >
> >> > right, right... Eventually we need to do it, but you're only making
> >> > gadget side optional. Host side should be optional too, but then you
> >> > need to make sure we don't build dwc3 without gadget and host.
> >> >
> >>
> >> Yes, true we need to make host side also optional, build dwc3 only when
> >> either of host or gadget are built.
> >
> > btw, make the default Dual-Role, if user/defconfig doesn't select
> > anything we want to build with all features.
> >
> 
> Yes we can try something like this ?
> 
>          if (USB || USB_GADGET)

no need for this if, actually...

>          menuconfig USB_DWC3
>                  tristate "DesignWare USB3 DRD Core Support"

make it a "depends on (USB || USB_GAGDGET)" here

other than that, it looks correct. Just make sure to compile test in all
options.

>           if USB_DWC3
>           choice
>           default USB_DWC3_DUAL_ROLE_MODE
>           ...
> 
>           config USB_DWC3_HOST_MODE

this one should depend on USB

>           ...
> 
>           config USB_DWC3_DEVICE_MODE

this one should depend on USB_GADGET

>           ...
> 
>           config USB_DWC3_DUAL_ROLE_MODE

should depend on USB && USB_GADGET

> >> >>  int dwc3_gadget_init(struct dwc3 *dwc);
> >> >>  void dwc3_gadget_exit(struct dwc3 *dwc);
> >> >> +#else
> >> >> +static inline int dwc3_gadget_init(struct dwc3 *dwc)
> >> >> +{ return -EINVAL; }
> >> >> +static inline void dwc3_gadget_exit(struct dwc3 *dwc)
> >> >> +{ }
> >> >> +#endif
> >> >>
> >> >>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
> >> >> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> >> >> index d4a30f1..553bbaa 100644
> >> >> --- a/drivers/usb/dwc3/debugfs.c
> >> >> +++ b/drivers/usb/dwc3/debugfs.c
> >> >> @@ -535,7 +535,8 @@ static ssize_t dwc3_testmode_write(struct file *file,
> >> >>               testmode = 0;
> >> >>
> >> >>       spin_lock_irqsave(&dwc->lock, flags);
> >> >> -     dwc3_gadget_set_test_mode(dwc, testmode);
> >> >> +     if (dwc3_gadget_set_test_mode(dwc, testmode))
> >> >> +             dev_dbg(dwc->dev, "host: Invalid request\n");
> >> >>       spin_unlock_irqrestore(&dwc->lock, flags);
> >> >>
> >> >>       return count;
> >> >
> >> > wrong, if you don't have gadget mode, you just don't create this file.
> >> >
> >>
> >> dwc3-core makes call to dwc3_debugfs_init() invariably depending on DEBUG_FS.
> >> Will this not go ahead and create this file ?
> >> I think i am missing here something.  :-(
> >
> > right, you can change dwc3_debugfs_init() to take into account the fact
> > that you're a gadget/drd or host-only.
> >
> > In case of host only, you stil want regdump to be available :-)
> >
> 
> So in dwc3_debugfs_init() we shall actually create just 'regdump' file
> in case of host only mode, otherwise keep the dwc3_debugfs_init() happy
> with creating other files too.
> Right ?

right.
Vivek Gautam Jan. 14, 2013, 9:12 a.m. UTC | #6
Hi Felipe,


On Mon, Jan 14, 2013 at 1:27 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Jan 11, 2013 at 07:58:23PM +0530, Vivek Gautam wrote:
>> Hi,
>>
>>
>> On Fri, Jan 11, 2013 at 7:29 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > Hi,
>> >
>> > On Fri, Jan 11, 2013 at 07:13:55PM +0530, Vivek Gautam wrote:
>> >> On Thu, Jan 10, 2013 at 6:32 PM, Felipe Balbi <balbi@ti.com> wrote:
>> >> > Hi,
>> >> >
>> >> > On Mon, Dec 24, 2012 at 07:28:33PM +0530, Vivek Gautam wrote:
>> >> >> DWC3 controller curretly depends on CONFIG_USB and CONFIG_USB_GADGET.
>> >> >> Some hardware may like to use only host feature on dwc3.
>> >> >> So, removing the dependency of USB_DWC3 on USB_GADGET
>> >> >> and further modulating the dwc3 core to enable gadget features
>> >> >> only with USB_GADGET.
>> >> >>
>> >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >> >> CC: Doug Anderson <dianders@chromium.org>
>> >> >
>> >> > right, right... Eventually we need to do it, but you're only making
>> >> > gadget side optional. Host side should be optional too, but then you
>> >> > need to make sure we don't build dwc3 without gadget and host.
>> >> >
>> >>
>> >> Yes, true we need to make host side also optional, build dwc3 only when
>> >> either of host or gadget are built.
>> >
>> > btw, make the default Dual-Role, if user/defconfig doesn't select
>> > anything we want to build with all features.
>> >
>>
>> Yes we can try something like this ?
>>
>>          if (USB || USB_GADGET)
>
> no need for this if, actually...
>
>>          menuconfig USB_DWC3
>>                  tristate "DesignWare USB3 DRD Core Support"
>
> make it a "depends on (USB || USB_GAGDGET)" here
>
Yeah, sure.

> other than that, it looks correct. Just make sure to compile test in all
> options.
>

Yes ofcourse, i shall compile test all possible options.

>>           if USB_DWC3
>>           choice
>>           default USB_DWC3_DUAL_ROLE_MODE
>>           ...
>>
>>           config USB_DWC3_HOST_MODE
>
> this one should depend on USB
>
yes true.

>>           ...
>>
>>           config USB_DWC3_DEVICE_MODE
>
> this one should depend on USB_GADGET
>
Ok,

>>           ...
>>
>>           config USB_DWC3_DUAL_ROLE_MODE
>
> should depend on USB && USB_GADGET
>

Ok

>> >> >>  int dwc3_gadget_init(struct dwc3 *dwc);
>> >> >>  void dwc3_gadget_exit(struct dwc3 *dwc);
>> >> >> +#else
>> >> >> +static inline int dwc3_gadget_init(struct dwc3 *dwc)
>> >> >> +{ return -EINVAL; }
>> >> >> +static inline void dwc3_gadget_exit(struct dwc3 *dwc)
>> >> >> +{ }
>> >> >> +#endif
>> >> >>
>> >> >>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>> >> >> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>> >> >> index d4a30f1..553bbaa 100644
>> >> >> --- a/drivers/usb/dwc3/debugfs.c
>> >> >> +++ b/drivers/usb/dwc3/debugfs.c
>> >> >> @@ -535,7 +535,8 @@ static ssize_t dwc3_testmode_write(struct file *file,
>> >> >>               testmode = 0;
>> >> >>
>> >> >>       spin_lock_irqsave(&dwc->lock, flags);
>> >> >> -     dwc3_gadget_set_test_mode(dwc, testmode);
>> >> >> +     if (dwc3_gadget_set_test_mode(dwc, testmode))
>> >> >> +             dev_dbg(dwc->dev, "host: Invalid request\n");
>> >> >>       spin_unlock_irqrestore(&dwc->lock, flags);
>> >> >>
>> >> >>       return count;
>> >> >
>> >> > wrong, if you don't have gadget mode, you just don't create this file.
>> >> >
>> >>
>> >> dwc3-core makes call to dwc3_debugfs_init() invariably depending on DEBUG_FS.
>> >> Will this not go ahead and create this file ?
>> >> I think i am missing here something.  :-(
>> >
>> > right, you can change dwc3_debugfs_init() to take into account the fact
>> > that you're a gadget/drd or host-only.
>> >
>> > In case of host only, you stil want regdump to be available :-)
>> >
>>
>> So in dwc3_debugfs_init() we shall actually create just 'regdump' file
>> in case of host only mode, otherwise keep the dwc3_debugfs_init() happy
>> with creating other files too.
>> Right ?
>
> right.
>
Ok so we shall change dwc3_debugfs_init().

Thanks for reviewing and suggesting changes.
diff mbox

Patch

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index f6a6e07..b70dcf1 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -1,7 +1,7 @@ 
 config USB_DWC3
 	tristate "DesignWare USB3 DRD Core Support"
-	depends on (USB && USB_GADGET)
-	select USB_OTG_UTILS
+	depends on USB
+	select USB_OTG_UTILS if USB_GADGET
 	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
 	help
 	  Say Y or M here if your system has a Dual Role SuperSpeed
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index 4502648..8f469cb 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -5,7 +5,7 @@  obj-$(CONFIG_USB_DWC3)			+= dwc3.o
 
 dwc3-y					:= core.o
 dwc3-y					+= host.o
-dwc3-y					+= gadget.o ep0.o
+obj-$(CONFIG_USB_GADGET)               += gadget.o ep0.o
 
 ifneq ($(CONFIG_DEBUG_FS),)
 	dwc3-y				+= debugfs.o
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4999563..4dc7ef2 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -865,7 +865,14 @@  int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc);
 int dwc3_host_init(struct dwc3 *dwc);
 void dwc3_host_exit(struct dwc3 *dwc);
 
+#ifdef CONFIG_USB_GADGET
 int dwc3_gadget_init(struct dwc3 *dwc);
 void dwc3_gadget_exit(struct dwc3 *dwc);
+#else
+static inline int dwc3_gadget_init(struct dwc3 *dwc)
+{ return -EINVAL; }
+static inline void dwc3_gadget_exit(struct dwc3 *dwc)
+{ }
+#endif
 
 #endif /* __DRIVERS_USB_DWC3_CORE_H */
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index d4a30f1..553bbaa 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -535,7 +535,8 @@  static ssize_t dwc3_testmode_write(struct file *file,
 		testmode = 0;
 
 	spin_lock_irqsave(&dwc->lock, flags);
-	dwc3_gadget_set_test_mode(dwc, testmode);
+	if (dwc3_gadget_set_test_mode(dwc, testmode))
+		dev_dbg(dwc->dev, "host: Invalid request\n");
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return count;
@@ -638,7 +639,8 @@  static ssize_t dwc3_link_state_write(struct file *file,
 		return -EINVAL;
 
 	spin_lock_irqsave(&dwc->lock, flags);
-	dwc3_gadget_set_link_state(dwc, state);
+	if (dwc3_gadget_set_link_state(dwc, state))
+		dev_dbg(dwc->dev, "host: Invalid request\n");
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return count;
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 99e6d72..28e82db 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -105,8 +105,16 @@  static inline void dwc3_gadget_move_request_queued(struct dwc3_request *req)
 void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
 		int status);
 
+#ifdef CONFIG_USB_GADGET
 int dwc3_gadget_set_test_mode(struct dwc3 *dwc, int mode);
 int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state);
+#else
+static inline int dwc3_gadget_set_test_mode(struct dwc3 *dwc, int mode)
+{ return -EINVAL; }
+static inline int dwc3_gadget_set_link_state(struct dwc3 *dwc,
+						enum dwc3_link_state state)
+{ return -EINVAL; }
+#endif
 
 void dwc3_ep0_interrupt(struct dwc3 *dwc,
 		const struct dwc3_event_depevt *event);