* [U-Boot] [RFC 01/10] New board-specific USB initialization interface [not found] <52027F05.3010309@samsung.com> @ 2013-08-07 17:09 ` Mateusz Zalega 0 siblings, 0 replies; 5+ messages in thread From: Mateusz Zalega @ 2013-08-07 17:09 UTC (permalink / raw) To: u-boot On 08/07/13 18:23, Stephen Warren wrote: >> This commit unifies board-specific USB initialization implementations >> under one symbol (usb_board_init), declaration of which is available in >> usb.h. > >> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c > >> -int board_usb_init(const void *blob) >> +int usb_process_devicetree(const void *blob) >> { >> int node_list[USB_PORTS_MAX]; >> int count, err = 0; > > With just this patch applied, nothing calls that function. This breaks > "git bisect". > Yup, I suppose I should squash this RFC PATCH. It's not so large anyway. Thanks, -- Mateusz Zalega Samsung R&D Institute Poland (SRPOL) | Kernel and System Framework group ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [RFC 00/10] New board-specific USB initialization interface @ 2013-08-06 10:50 Mateusz Zalega 2013-08-06 10:50 ` [U-Boot] [RFC 01/10] " Mateusz Zalega 0 siblings, 1 reply; 5+ messages in thread From: Mateusz Zalega @ 2013-08-06 10:50 UTC (permalink / raw) To: u-boot Current implementation of do_dfu() and do_usb_mass_storage() requires board-specific board_usb_init() which performs USB hardware initialization. I noticed that several boards have such a function defined, named either usb_board_init() (which binds to ohci-hcd.c driver and had been used solely by it) or board_usb_init() (as in ehci-omap.c). I _assumed_ that these functions do what's required by do_*() and renamed the earlier in order to unify the naming convention. It would: + make enabling DFU and UMS in these boards easier + clean up the namespace + improve code reusability + enable devs to postpone hardware init until it's needed (as in do_dfu()) Declarations of these functions were scattered all over the codebase. Seeing it as a bad practice, I added declaration of usb_board_init() in usb.h and removed the other. I added a weak symbol for usb_board_init() in usb.c so that boards which do not require USB initialization or have to initialize hardware earlier would compile and run. For boards which offer both USB device and host capabilities, you can specify which part, host or device, to initialize, via function argument. Does this approach sound good and/or reasonable? Code compiles successfully for all ARM boards and was tested on Samsung Goni. Mateusz Zalega (10): New board-specific USB initialization interface nvidia, tegra: new USB hardware init interface Voipac PXA270: new USB hardware init interface Trizeps IV: new USB hardware init interface Colibri PXA270: new USB hardware init interface icpdas lp8x4x: new USB hardware init interface esd pmc440: new USB hardware init interface esd apc405: new USB hardware init interface balloon3: new USB hardware init interface canyonlands: new USB hardware init interface arch/arm/include/asm/arch-tegra/usb.h | 3 +-- board/amcc/canyonlands/canyonlands.c | 5 +++-- board/balloon3/balloon3.c | 5 +++-- board/esd/apc405/apc405.c | 5 +++-- board/esd/pmc440/pmc440.c | 5 +++-- board/icpdas/lp8x4x/lp8x4x.c | 5 +++-- board/nvidia/common/board.c | 14 ++++++++++---- board/toradex/colibri_pxa270/colibri_pxa270.c | 5 +++-- board/trizepsiv/conxs.c | 5 +++-- board/vpac270/vpac270.c | 5 +++-- common/cmd_dfu.c | 5 ++--- common/cmd_usb_mass_storage.c | 3 ++- common/usb.c | 5 +++++ drivers/usb/host/ehci-omap.c | 8 +------- drivers/usb/host/ehci-tegra.c | 2 +- drivers/usb/host/ohci-hcd.c | 4 ++-- drivers/usb/host/ohci.h | 12 +++++------- include/g_dnl.h | 2 -- include/usb.h | 17 ++++++++++++++++- include/usb_mass_storage.h | 12 +++++------- 20 files changed, 74 insertions(+), 53 deletions(-) -- 1.8.2.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [RFC 01/10] New board-specific USB initialization interface 2013-08-06 10:50 [U-Boot] [RFC 00/10] " Mateusz Zalega @ 2013-08-06 10:50 ` Mateusz Zalega 2013-08-07 16:23 ` Stephen Warren 2013-08-11 18:04 ` Marek Vasut 0 siblings, 2 replies; 5+ messages in thread From: Mateusz Zalega @ 2013-08-06 10:50 UTC (permalink / raw) To: u-boot This commit unifies board-specific USB initialization implementations under one symbol (usb_board_init), declaration of which is available in usb.h. Signed-off-by: Mateusz Zalega <m.zalega@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> Cc: Marek Vasut <marex@denx.de> --- common/cmd_dfu.c | 5 ++--- common/cmd_usb_mass_storage.c | 3 ++- common/usb.c | 5 +++++ drivers/usb/host/ehci-omap.c | 8 +------- drivers/usb/host/ehci-tegra.c | 2 +- drivers/usb/host/ohci-hcd.c | 4 ++-- drivers/usb/host/ohci.h | 12 +++++------- include/g_dnl.h | 2 -- include/usb.h | 17 ++++++++++++++++- include/usb_mass_storage.h | 12 +++++------- 10 files changed, 39 insertions(+), 31 deletions(-) diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index db066ac..c701ebc 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -14,6 +14,7 @@ #include <dfu.h> #include <asm/errno.h> #include <g_dnl.h> +#include <usb.h> static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { @@ -43,9 +44,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) goto done; } -#ifdef CONFIG_TRATS - board_usb_init(); -#endif + board_usb_init(USB_INIT_DEVICE); g_dnl_register(s); while (1) { diff --git a/common/cmd_usb_mass_storage.c b/common/cmd_usb_mass_storage.c index 33a4715..86135b4 100644 --- a/common/cmd_usb_mass_storage.c +++ b/common/cmd_usb_mass_storage.c @@ -9,6 +9,7 @@ #include <common.h> #include <command.h> #include <g_dnl.h> +#include <usb.h> #include <usb_mass_storage.h> int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag, @@ -33,7 +34,7 @@ int do_usb_mass_storage(cmd_tbl_t *cmdtp, int flag, goto fail; } - board_usb_init(); + board_usb_init(USB_INIT_DEVICE); ums_info = board_ums_init(dev_num, offset, part_size); if (!ums_info) { diff --git a/common/usb.c b/common/usb.c index f740e5e..793fb35 100644 --- a/common/usb.c +++ b/common/usb.c @@ -982,4 +982,9 @@ int usb_new_device(struct usb_device *dev) return 0; } +__attribute__((weak)) +int board_usb_init(enum board_usb_init_type what_to_init) +{ + return 0; +} /* EOF */ diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c index a47e078..61b7c8d 100644 --- a/drivers/usb/host/ehci-omap.c +++ b/drivers/usb/host/ehci-omap.c @@ -86,12 +86,6 @@ static void omap_ehci_soft_phy_reset(int port) ulpi_reset(&ulpi_vp); } -inline int __board_usb_init(void) -{ - return 0; -} -int board_usb_init(void) __attribute__((weak, alias("__board_usb_init"))); - #if defined(CONFIG_OMAP_EHCI_PHY1_RESET_GPIO) || \ defined(CONFIG_OMAP_EHCI_PHY2_RESET_GPIO) /* controls PHY(s) reset signal(s) */ @@ -150,7 +144,7 @@ int omap_ehci_hcd_init(struct omap_usbhs_board_data *usbhs_pdata, debug("Initializing OMAP EHCI\n"); - ret = board_usb_init(); + ret = board_usb_init(USB_INIT_HOST); if (ret < 0) return ret; diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c index c6da449..cc23133 100644 --- a/drivers/usb/host/ehci-tegra.c +++ b/drivers/usb/host/ehci-tegra.c @@ -699,7 +699,7 @@ static int process_usb_nodes(const void *blob, int node_list[], int count) return 0; } -int board_usb_init(const void *blob) +int usb_process_devicetree(const void *blob) { int node_list[USB_PORTS_MAX]; int count, err = 0; diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c index c33c487..e6a5623 100644 --- a/drivers/usb/host/ohci-hcd.c +++ b/drivers/usb/host/ohci-hcd.c @@ -1861,7 +1861,7 @@ int usb_lowlevel_init(int index, void **controller) #ifdef CONFIG_SYS_USB_OHCI_BOARD_INIT /* board dependant init */ - if (usb_board_init()) + if (board_usb_init(USB_INIT_HOST)) return -1; #endif memset(&gohci, 0, sizeof(ohci_t)); @@ -1918,7 +1918,7 @@ int usb_lowlevel_init(int index, void **controller) err ("can't reset usb-%s", gohci.slot_name); #ifdef CONFIG_SYS_USB_OHCI_BOARD_INIT /* board dependant cleanup */ - usb_board_init_fail(); + board_usb_init_fail(); #endif #ifdef CONFIG_SYS_USB_OHCI_CPU_INIT diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h index d977e8f..9f7f961 100644 --- a/drivers/usb/host/ohci.h +++ b/drivers/usb/host/ohci.h @@ -19,14 +19,12 @@ #endif /* CONFIG_SYS_OHCI_SWAP_REG_ACCESS */ /* functions for doing board or CPU specific setup/cleanup */ -extern int usb_board_init(void); -extern int usb_board_stop(void); -extern int usb_board_init_fail(void); - -extern int usb_cpu_init(void); -extern int usb_cpu_stop(void); -extern int usb_cpu_init_fail(void); +int usb_board_stop(void); +int board_usb_init_fail(void); +int usb_cpu_init(void); +int usb_cpu_stop(void); +int usb_cpu_init_fail(void); static int cc_to_error[16] = { diff --git a/include/g_dnl.h b/include/g_dnl.h index 2b2f11a..b6c4dd4 100644 --- a/include/g_dnl.h +++ b/include/g_dnl.h @@ -14,6 +14,4 @@ int g_dnl_bind_fixup(struct usb_device_descriptor *); int g_dnl_register(const char *s); void g_dnl_unregister(void); -/* USB initialization declaration - board specific */ -void board_usb_init(void); #endif /* __G_DOWNLOAD_H_ */ diff --git a/include/usb.h b/include/usb.h index 60db897..29f67bf 100644 --- a/include/usb.h +++ b/include/usb.h @@ -165,10 +165,25 @@ int submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer, extern void udc_disconnect(void); -#else +#elif !defined(CONFIG_USB_GADGET) #error USB Lowlevel not defined #endif +/* You can initialize platform's USB host, device or both + * capabilities by passing this enum as an argument to + * board_usb_init(). + */ +enum board_usb_init_type { + USB_INIT_ALL, + USB_INIT_HOST, + USB_INIT_DEVICE +}; + +/* board-specific hardware initialization, called by + * usb drivers and u-boot commands + */ +int board_usb_init(enum board_usb_init_type what_to_init); + #ifdef CONFIG_USB_STORAGE #define USB_MAX_STOR_DEV 5 diff --git a/include/usb_mass_storage.h b/include/usb_mass_storage.h index 35cdcc3..de31b0e 100644 --- a/include/usb_mass_storage.h +++ b/include/usb_mass_storage.h @@ -30,13 +30,11 @@ struct ums_board_info { struct ums_device ums_dev; }; -extern void board_usb_init(void); - -extern int fsg_init(struct ums_board_info *); -extern void fsg_cleanup(void); -extern struct ums_board_info *board_ums_init(unsigned int, +int fsg_init(struct ums_board_info *); +void fsg_cleanup(void); +struct ums_board_info *board_ums_init(unsigned int, unsigned int, unsigned int); -extern int usb_gadget_handle_interrupts(void); -extern int fsg_main_thread(void *); +int usb_gadget_handle_interrupts(void); +int fsg_main_thread(void *); #endif /* __USB_MASS_STORAGE_H__ */ -- 1.8.2.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [RFC 01/10] New board-specific USB initialization interface 2013-08-06 10:50 ` [U-Boot] [RFC 01/10] " Mateusz Zalega @ 2013-08-07 16:23 ` Stephen Warren 2013-08-11 18:04 ` Marek Vasut 1 sibling, 0 replies; 5+ messages in thread From: Stephen Warren @ 2013-08-07 16:23 UTC (permalink / raw) To: u-boot On 08/06/2013 04:50 AM, Mateusz Zalega wrote: > This commit unifies board-specific USB initialization implementations > under one symbol (usb_board_init), declaration of which is available in > usb.h. > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c > -int board_usb_init(const void *blob) > +int usb_process_devicetree(const void *blob) > { > int node_list[USB_PORTS_MAX]; > int count, err = 0; With just this patch applied, nothing calls that function. This breaks "git bisect". ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [RFC 01/10] New board-specific USB initialization interface 2013-08-06 10:50 ` [U-Boot] [RFC 01/10] " Mateusz Zalega 2013-08-07 16:23 ` Stephen Warren @ 2013-08-11 18:04 ` Marek Vasut 2013-08-12 9:33 ` Mateusz Zalega 1 sibling, 1 reply; 5+ messages in thread From: Marek Vasut @ 2013-08-11 18:04 UTC (permalink / raw) To: u-boot Dear Mateusz Zalega, > This commit unifies board-specific USB initialization implementations > under one symbol (usb_board_init), declaration of which is available in > usb.h. > > Signed-off-by: Mateusz Zalega <m.zalega@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > Reviewed-by: Lukasz Majewski <l.majewski@samsung.com> > Cc: Marek Vasut <marex@denx.de> > --- > common/cmd_dfu.c | 5 ++--- > common/cmd_usb_mass_storage.c | 3 ++- > common/usb.c | 5 +++++ > drivers/usb/host/ehci-omap.c | 8 +------- > drivers/usb/host/ehci-tegra.c | 2 +- > drivers/usb/host/ohci-hcd.c | 4 ++-- > drivers/usb/host/ohci.h | 12 +++++------- > include/g_dnl.h | 2 -- > include/usb.h | 17 ++++++++++++++++- > include/usb_mass_storage.h | 12 +++++------- > 10 files changed, 39 insertions(+), 31 deletions(-) [...] > --- a/include/usb.h > +++ b/include/usb.h > @@ -165,10 +165,25 @@ int submit_int_msg(struct usb_device *dev, unsigned > long pipe, void *buffer, > > extern void udc_disconnect(void); > > -#else > +#elif !defined(CONFIG_USB_GADGET) > #error USB Lowlevel not defined > #endif > > +/* You can initialize platform's USB host, device or both > + * capabilities by passing this enum as an argument to > + * board_usb_init(). > + */ The comment style is wrong, please fix. Did the patchset pass checkpatch ? /* * multi * line * comment */ > +enum board_usb_init_type { > + USB_INIT_ALL, > + USB_INIT_HOST, > + USB_INIT_DEVICE > +}; > + > +/* board-specific hardware initialization, called by > + * usb drivers and u-boot commands > + */ > +int board_usb_init(enum board_usb_init_type what_to_init); > + > #ifdef CONFIG_USB_STORAGE > > #define USB_MAX_STOR_DEV 5 > diff --git a/include/usb_mass_storage.h b/include/usb_mass_storage.h > index 35cdcc3..de31b0e 100644 > --- a/include/usb_mass_storage.h > +++ b/include/usb_mass_storage.h > @@ -30,13 +30,11 @@ struct ums_board_info { > struct ums_device ums_dev; > }; > > -extern void board_usb_init(void); > - > -extern int fsg_init(struct ums_board_info *); > -extern void fsg_cleanup(void); > -extern struct ums_board_info *board_ums_init(unsigned int, > +int fsg_init(struct ums_board_info *); > +void fsg_cleanup(void); > +struct ums_board_info *board_ums_init(unsigned int, > unsigned int, unsigned int); > -extern int usb_gadget_handle_interrupts(void); > -extern int fsg_main_thread(void *); > +int usb_gadget_handle_interrupts(void); > +int fsg_main_thread(void *); > > #endif /* __USB_MASS_STORAGE_H__ */ Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [RFC 01/10] New board-specific USB initialization interface 2013-08-11 18:04 ` Marek Vasut @ 2013-08-12 9:33 ` Mateusz Zalega 0 siblings, 0 replies; 5+ messages in thread From: Mateusz Zalega @ 2013-08-12 9:33 UTC (permalink / raw) To: u-boot On 08/11/13 20:04, Marek Vasut wrote: >> +/* You can initialize platform's USB host, device or both >> + * capabilities by passing this enum as an argument to >> + * board_usb_init(). >> + */ > > The comment style is wrong, please fix. Did the patchset pass checkpatch ? > > /* > * multi > * line > * comment > */ Yes, it did: ---8<--- $ ./tools/checkpatch.pl 0001-New-board-specific-USB-initialization-interface.patch total: 0 errors, 0 warnings, 0 checks, 154 lines checked NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE USLEEP_RANGE 0001-New-board-specific-USB-initialization-interface.patch has no obvious style problems and is ready for submission. --->8--- NETWORKING_BLOCK_COMMENT_STYLE - ...and, in our case, this option is the culprit. Will fix. Thanks, -- Mateusz Zalega Samsung R&D Institute Poland (SRPOL) | Kernel and System Framework group ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-08-12 9:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <52027F05.3010309@samsung.com>
2013-08-07 17:09 ` [U-Boot] [RFC 01/10] New board-specific USB initialization interface Mateusz Zalega
2013-08-06 10:50 [U-Boot] [RFC 00/10] " Mateusz Zalega
2013-08-06 10:50 ` [U-Boot] [RFC 01/10] " Mateusz Zalega
2013-08-07 16:23 ` Stephen Warren
2013-08-11 18:04 ` Marek Vasut
2013-08-12 9:33 ` Mateusz Zalega
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox