[UBOOT,RFC,09/13] common: cmd_dfu: add an API that takes controller index for handling interrupts

Message ID 1408372115-4570-10-git-send-email-kishon@ti.com
State New
Headers show

Commit Message

Kishon Vijay Abraham I Aug. 18, 2014, 2:28 p.m.
Since there can be multiple USB controllers in the system,
usb_gadget_handle_interrupts should take controller index as arguments.
However such an API can only be added in board file so added
board_usb_gadget_handle_interrupts(). usb_gadget_handle_interrupts() should
be deprecated.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 common/cmd_dfu.c | 1 +
 include/usb.h    | 7 +++++++
 2 files changed, 8 insertions(+)

Comments

Lukasz Majewski Aug. 19, 2014, 8:53 a.m. | #1
Hi Kishon,

> Since there can be multiple USB controllers in the system,
> usb_gadget_handle_interrupts should take controller index as
> arguments.

The controller index is passed in the ums/dfu/thor command. Then PHY
subsystem is properly configured.
Since we only handle one USB controller transmission at a time (u-boot
doesn't use interrupts - we only pool) the
usb_gadget_handle_interrupts() was enough for our purpose.

To start transmission from other usb controller you need to end current
transmission and then invoke modified command with different controller
number.

It would be enough to extend
usb_gadget_handle_interrupts() to accept controller index argument
(only six boards use it - it shall be easy to modify them all). 

> However such an API can only be added in board file so
> added board_usb_gadget_handle_interrupts().
> usb_gadget_handle_interrupts() should be deprecated.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  common/cmd_dfu.c | 1 +
>  include/usb.h    | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> index 1e7d50c..f27ba52 100644
> --- a/common/cmd_dfu.c
> +++ b/common/cmd_dfu.c
> @@ -52,6 +52,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) goto exit;
>  
>  		usb_gadget_handle_interrupts();
> +		board_usb_gadget_handle_interrupts(controller_index);

NAK, for the reason stated above. 

>  	}
>  exit:
>  	g_dnl_unregister();
> diff --git a/include/usb.h b/include/usb.h
> index d9fedee..26c6462 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -197,6 +197,13 @@ int board_usb_init(int index, enum usb_init_type
> init); */
>  int board_usb_cleanup(int index, enum usb_init_type init);
>  
> +/*
> + * Can be use to handle interrupts from a specific controller.
> + *
> + * @index index of the USB controller
> + */
> +int board_usb_gadget_handle_interrupts(int index);
> +
>  #ifdef CONFIG_USB_STORAGE
>  
>  #define USB_MAX_STOR_DEV 5
Kishon Vijay Abraham I Aug. 19, 2014, 4:09 p.m. | #2
Hi,

On Tuesday 19 August 2014 02:23 PM, Lukasz Majewski wrote:
> Hi Kishon,
> 
>> Since there can be multiple USB controllers in the system,
>> usb_gadget_handle_interrupts should take controller index as
>> arguments.
> 
> The controller index is passed in the ums/dfu/thor command. Then PHY
> subsystem is properly configured.
> Since we only handle one USB controller transmission at a time (u-boot
> doesn't use interrupts - we only pool) the
> usb_gadget_handle_interrupts() was enough for our purpose.

yeah, if we have only one USB controller in the system, controller index is not
needed.

-Kishon
> 
> To start transmission from other usb controller you need to end current
> transmission and then invoke modified command with different controller
> number.
> 
> It would be enough to extend
> usb_gadget_handle_interrupts() to accept controller index argument
> (only six boards use it - it shall be easy to modify them all). 
> 
>> However such an API can only be added in board file so
>> added board_usb_gadget_handle_interrupts().
>> usb_gadget_handle_interrupts() should be deprecated.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  common/cmd_dfu.c | 1 +
>>  include/usb.h    | 7 +++++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
>> index 1e7d50c..f27ba52 100644
>> --- a/common/cmd_dfu.c
>> +++ b/common/cmd_dfu.c
>> @@ -52,6 +52,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int
>> argc, char * const argv[]) goto exit;
>>  
>>  		usb_gadget_handle_interrupts();
>> +		board_usb_gadget_handle_interrupts(controller_index);
> 
> NAK, for the reason stated above. 
> 
>>  	}
>>  exit:
>>  	g_dnl_unregister();
>> diff --git a/include/usb.h b/include/usb.h
>> index d9fedee..26c6462 100644
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -197,6 +197,13 @@ int board_usb_init(int index, enum usb_init_type
>> init); */
>>  int board_usb_cleanup(int index, enum usb_init_type init);
>>  
>> +/*
>> + * Can be use to handle interrupts from a specific controller.
>> + *
>> + * @index index of the USB controller
>> + */
>> +int board_usb_gadget_handle_interrupts(int index);
>> +
>>  #ifdef CONFIG_USB_STORAGE
>>  
>>  #define USB_MAX_STOR_DEV 5
> 
> 
>
Lukasz Majewski Aug. 20, 2014, 7:37 a.m. | #3
Hi Kishon,

> Hi,
> 
> On Tuesday 19 August 2014 02:23 PM, Lukasz Majewski wrote:
> > Hi Kishon,
> > 
> >> Since there can be multiple USB controllers in the system,
> >> usb_gadget_handle_interrupts should take controller index as
> >> arguments.
> > 
> > The controller index is passed in the ums/dfu/thor command. Then PHY
> > subsystem is properly configured.
> > Since we only handle one USB controller transmission at a time
> > (u-boot doesn't use interrupts - we only pool) the
> > usb_gadget_handle_interrupts() was enough for our purpose.
> 
> yeah, if we have only one USB controller in the system, controller
> index is not needed.

To sum up:
1. You should be able to specify the USB controller when calling dfu or
ums commands (as it is now).

2. If this is not enough for some reason (and good justification is
welcome here), then usb_gadget_handle_interrupts() should be extended
to accept the controller index argument.

> 
> -Kishon
> > 
> > To start transmission from other usb controller you need to end
> > current transmission and then invoke modified command with
> > different controller number.
> > 
> > It would be enough to extend
> > usb_gadget_handle_interrupts() to accept controller index argument
> > (only six boards use it - it shall be easy to modify them all). 
> > 
> >> However such an API can only be added in board file so
> >> added board_usb_gadget_handle_interrupts().
> >> usb_gadget_handle_interrupts() should be deprecated.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  common/cmd_dfu.c | 1 +
> >>  include/usb.h    | 7 +++++++
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> >> index 1e7d50c..f27ba52 100644
> >> --- a/common/cmd_dfu.c
> >> +++ b/common/cmd_dfu.c
> >> @@ -52,6 +52,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int
> >> argc, char * const argv[]) goto exit;
> >>  
> >>  		usb_gadget_handle_interrupts();
> >> +
> >> board_usb_gadget_handle_interrupts(controller_index);
> > 
> > NAK, for the reason stated above. 
> > 
> >>  	}
> >>  exit:
> >>  	g_dnl_unregister();
> >> diff --git a/include/usb.h b/include/usb.h
> >> index d9fedee..26c6462 100644
> >> --- a/include/usb.h
> >> +++ b/include/usb.h
> >> @@ -197,6 +197,13 @@ int board_usb_init(int index, enum
> >> usb_init_type init); */
> >>  int board_usb_cleanup(int index, enum usb_init_type init);
> >>  
> >> +/*
> >> + * Can be use to handle interrupts from a specific controller.
> >> + *
> >> + * @index index of the USB controller
> >> + */
> >> +int board_usb_gadget_handle_interrupts(int index);
> >> +
> >>  #ifdef CONFIG_USB_STORAGE
> >>  
> >>  #define USB_MAX_STOR_DEV 5
> > 
> > 
> >

Patch

diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
index 1e7d50c..f27ba52 100644
--- a/common/cmd_dfu.c
+++ b/common/cmd_dfu.c
@@ -52,6 +52,7 @@  static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			goto exit;
 
 		usb_gadget_handle_interrupts();
+		board_usb_gadget_handle_interrupts(controller_index);
 	}
 exit:
 	g_dnl_unregister();
diff --git a/include/usb.h b/include/usb.h
index d9fedee..26c6462 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -197,6 +197,13 @@  int board_usb_init(int index, enum usb_init_type init);
  */
 int board_usb_cleanup(int index, enum usb_init_type init);
 
+/*
+ * Can be use to handle interrupts from a specific controller.
+ *
+ * @index index of the USB controller
+ */
+int board_usb_gadget_handle_interrupts(int index);
+
 #ifdef CONFIG_USB_STORAGE
 
 #define USB_MAX_STOR_DEV 5