Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
On 09/15/13 16:21, Marek Vasut wrote: I suppose this thread can be concluded by droping the INIT_ALL stuff entirely. Afterall, we do not want to init _ALL_ ports at once, but we want to init them selectively. Best regards, Marek Vasut +1 -- Mateusz Zalega Samsung RD Institute Poland ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
Dear Mateusz Zalega, On 09/06/13 13:40, Marek Vasut wrote: Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Despite our greatest ambitions, I don't think we're likely to use more than 2^31-1 USB controllers at a time. Besides, negative values look better both in code and debugger session. Thinking of it further, instead of using negative value here, like I mentioned above, why not make the board_usb_init_type into a field of flags , then add flag to init all controllers at once ? That's unnecessary. It wouldn't lead to any practical advantage over existing interface. The advantage would be you won't be mixing two things (value AND value with special meaning) into the index parameter. Alright, provide a use-case. The only 'special' value we have now doesn't interfere with controller index. Why write code or interfaces that won't ever be used? Look, abusing the index field with a special value is moronic, especially if you I wouldn't call a de-facto standard abusive or moronic. On the other hand, contributing to unnecessary code bloat would be. _do_ have another field that can very well be turned into a block of flags Have you provided another use-case for that, as I asked? instead of enum right next to it. function(int foo , enum bar) | ^ `-' flags go here Then int foo can be turned into unsigned int foo _and_ be used for it's one singular purpose. Likewise, enum bar will now be unsigned int flags . Do you see the separation now ? Read your mail carefully - at this point I don't have any problems understanding what you wish to do, but I see your API changes as unjustified. Can you justify them without appealing to your authority and maybe inserting a couple of ad-hominems here and there? I suppose this thread can be concluded by droping the INIT_ALL stuff entirely. Afterall, we do not want to init _ALL_ ports at once, but we want to init them selectively. Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
On 09/05/13 19:51, Marek Vasut wrote: Why not wrap board_usb_init() and board_usb_init_fail() into single call. You now pass some flags to board_usb_init() already, so just add another for the fail case. How does it sound to you? Like overengineering. It would lead to board_usb_init(USB_INIT_ALL, USB_INIT_DEVICE, USB_CLEANUP) calls, which are not very readable. This is not what I mean, see this: int board_usb_init(int index, enum board_usb_init_type init) Add a new init type (or maybe change the init field to be flags) that will say OK, do a fail init ? As for providing a way to do a selective cleanup if anything gone wrong, it's a good idea, but adding such functionality to board_usb_init() would be confusing, especially for newcomers. Why not do this in board_usb_init_fail(int index, enum board_usb_init_type)? ...and maybe rename board_usb_init_fail to board_usb_cleanup. Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Despite our greatest ambitions, I don't think we're likely to use more than 2^31-1 USB controllers at a time. Besides, negative values look better both in code and debugger session. Thinking of it further, instead of using negative value here, like I mentioned above, why not make the board_usb_init_type into a field of flags , then add flag to init all controllers at once ? That's unnecessary. It wouldn't lead to any practical advantage over existing interface. Best Regards, -- Mateusz Zalega Samsung RD Institute Poland ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
Dear Mateusz Zalega, On 09/05/13 19:51, Marek Vasut wrote: Why not wrap board_usb_init() and board_usb_init_fail() into single call. You now pass some flags to board_usb_init() already, so just add another for the fail case. How does it sound to you? Like overengineering. It would lead to board_usb_init(USB_INIT_ALL, USB_INIT_DEVICE, USB_CLEANUP) calls, which are not very readable. This is not what I mean, see this: int board_usb_init(int index, enum board_usb_init_type init) Add a new init type (or maybe change the init field to be flags) that will say OK, do a fail init ? As for providing a way to do a selective cleanup if anything gone wrong, it's a good idea, but adding such functionality to board_usb_init() would be confusing, especially for newcomers. Why not do this in board_usb_init_fail(int index, enum board_usb_init_type)? ...and maybe rename board_usb_init_fail to board_usb_cleanup. Cleanup is nice name, yeah. Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Despite our greatest ambitions, I don't think we're likely to use more than 2^31-1 USB controllers at a time. Besides, negative values look better both in code and debugger session. Thinking of it further, instead of using negative value here, like I mentioned above, why not make the board_usb_init_type into a field of flags , then add flag to init all controllers at once ? That's unnecessary. It wouldn't lead to any practical advantage over existing interface. The advantage would be you won't be mixing two things (value AND value with special meaning) into the index parameter. Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
On 09/06/13 13:24, Marek Vasut wrote: Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Despite our greatest ambitions, I don't think we're likely to use more than 2^31-1 USB controllers at a time. Besides, negative values look better both in code and debugger session. Thinking of it further, instead of using negative value here, like I mentioned above, why not make the board_usb_init_type into a field of flags , then add flag to init all controllers at once ? That's unnecessary. It wouldn't lead to any practical advantage over existing interface. The advantage would be you won't be mixing two things (value AND value with special meaning) into the index parameter. Alright, provide a use-case. The only 'special' value we have now doesn't interfere with controller index. Why write code or interfaces that won't ever be used? Best regards, -- Mateusz Zalega Samsung RD Institute Poland ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
Dear Mateusz Zalega, On 09/06/13 13:24, Marek Vasut wrote: Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Despite our greatest ambitions, I don't think we're likely to use more than 2^31-1 USB controllers at a time. Besides, negative values look better both in code and debugger session. Thinking of it further, instead of using negative value here, like I mentioned above, why not make the board_usb_init_type into a field of flags , then add flag to init all controllers at once ? That's unnecessary. It wouldn't lead to any practical advantage over existing interface. The advantage would be you won't be mixing two things (value AND value with special meaning) into the index parameter. Alright, provide a use-case. The only 'special' value we have now doesn't interfere with controller index. Why write code or interfaces that won't ever be used? Look, abusing the index field with a special value is moronic, especially if you _do_ have another field that can very well be turned into a block of flags instead of enum right next to it. function(int foo , enum bar) | ^ | | `-' flags go here Then int foo can be turned into unsigned int foo _and_ be used for it's one singular purpose. Likewise, enum bar will now be unsigned int flags . Do you see the separation now ? Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
On 09/06/13 13:40, Marek Vasut wrote: Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Despite our greatest ambitions, I don't think we're likely to use more than 2^31-1 USB controllers at a time. Besides, negative values look better both in code and debugger session. Thinking of it further, instead of using negative value here, like I mentioned above, why not make the board_usb_init_type into a field of flags , then add flag to init all controllers at once ? That's unnecessary. It wouldn't lead to any practical advantage over existing interface. The advantage would be you won't be mixing two things (value AND value with special meaning) into the index parameter. Alright, provide a use-case. The only 'special' value we have now doesn't interfere with controller index. Why write code or interfaces that won't ever be used? Look, abusing the index field with a special value is moronic, especially if you I wouldn't call a de-facto standard abusive or moronic. On the other hand, contributing to unnecessary code bloat would be. _do_ have another field that can very well be turned into a block of flags Have you provided another use-case for that, as I asked? instead of enum right next to it. function(int foo , enum bar) | ^ | | `-' flags go here Then int foo can be turned into unsigned int foo _and_ be used for it's one singular purpose. Likewise, enum bar will now be unsigned int flags . Do you see the separation now ? Read your mail carefully - at this point I don't have any problems understanding what you wish to do, but I see your API changes as unjustified. Can you justify them without appealing to your authority and maybe inserting a couple of ad-hominems here and there? -- Mateusz Zalega Samsung RD Institute Poland ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
On 09/05/13 17:50, Marek Vasut wrote: v3 changes: - added 'index' argument to perform selective port initialization OK, a few general ideas again: Why not wrap board_usb_init() and board_usb_init_fail() into single call. You now pass some flags to board_usb_init() already, so just add another for the fail case. How does it sound to you? Like overengineering. It would lead to board_usb_init(USB_INIT_ALL, USB_INIT_DEVICE, USB_CLEANUP) calls, which are not very readable. Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Despite our greatest ambitions, I don't think we're likely to use more than 2^31-1 USB controllers at a time. Besides, negative values look better both in code and debugger session. Best Regards, -- Mateusz Zalega Samsung RD Institute Poland ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
Dear Mateusz Zalega, On 09/05/13 17:50, Marek Vasut wrote: v3 changes: - added 'index' argument to perform selective port initialization OK, a few general ideas again: Why not wrap board_usb_init() and board_usb_init_fail() into single call. You now pass some flags to board_usb_init() already, so just add another for the fail case. How does it sound to you? Like overengineering. It would lead to board_usb_init(USB_INIT_ALL, USB_INIT_DEVICE, USB_CLEANUP) calls, which are not very readable. This is not what I mean, see this: int board_usb_init(int index, enum board_usb_init_type init) Add a new init type (or maybe change the init field to be flags) that will say OK, do a fail init ? Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Despite our greatest ambitions, I don't think we're likely to use more than 2^31-1 USB controllers at a time. Besides, negative values look better both in code and debugger session. Thinking of it further, instead of using negative value here, like I mentioned above, why not make the board_usb_init_type into a field of flags , then add flag to init all controllers at once ? Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: new board-specific USB init interface
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.zal...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com Reviewed-by: Lukasz Majewski l.majew...@samsung.com Cc: Minkyu Kang mk7.k...@samsung.com Cc: Marek Vasut ma...@denx.de --- Changes since RFC (v1): - NVIDIA Tegra doesn't postpone its USB init anymore - board_usb_init()'s sole argument name was shortened - networking code comment style (/* blurb...) dropped - squashed RFC changes so that patch won't break bisect v2 changes: - commit message fixup v3 changes: - added 'index' argument to perform selective port initialization OK, a few general ideas again: Why not wrap board_usb_init() and board_usb_init_fail() into single call. You now pass some flags to board_usb_init() already, so just add another for the fail case. How does it sound to you? Moreover, the 'int index' should likely be unsigned int and the special value to init all controllers at once should probably then be 0x Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3] usb: new board-specific USB init interface
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.zal...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
Reviewed-by: Lukasz Majewski l.majew...@samsung.com
Cc: Minkyu Kang mk7.k...@samsung.com
Cc: Marek Vasut ma...@denx.de
---
Changes since RFC (v1):
- NVIDIA Tegra doesn't postpone its USB init anymore
- board_usb_init()'s sole argument name was shortened
- networking code comment style (/* blurb...) dropped
- squashed RFC changes so that patch won't break bisect
v2 changes:
- commit message fixup
v3 changes:
- added 'index' argument to perform selective port initialization
---
arch/arm/include/asm/arch-tegra/usb.h | 3 +--
arch/arm/include/asm/ehci-omap.h | 4 ++--
board/amcc/canyonlands/canyonlands.c | 5 +++--
board/balloon3/balloon3.c | 5 +++--
board/compulab/cm_t35/cm_t35.c| 2 +-
board/esd/apc405/apc405.c | 5 +++--
board/esd/pmc440/pmc440.c | 5 +++--
board/htkw/mcx/mcx.c | 2 +-
board/icpdas/lp8x4x/lp8x4x.c | 5 +++--
board/nvidia/common/board.c | 4 +++-
board/samsung/trats/trats.c | 5 +++--
board/technexion/twister/twister.c| 2 +-
board/teejet/mt_ventoux/mt_ventoux.c | 2 +-
board/ti/beagle/beagle.c | 2 +-
board/ti/panda/panda.c| 2 +-
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 | 12 +++-
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 | 28 ++-
include/usb_mass_storage.h| 14 ++
28 files changed, 92 insertions(+), 63 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/usb.h
b/arch/arm/include/asm/arch-tegra/usb.h
index f66257c..a1efd07 100644
--- a/arch/arm/include/asm/arch-tegra/usb.h
+++ b/arch/arm/include/asm/arch-tegra/usb.h
@@ -131,8 +131,7 @@
/* USB3_IF_USB_PHY_VBUS_SENSORS_0 */
#define VBUS_VLD_STS (1 26)
-
/* Setup USB on the board */
-int board_usb_init(const void *blob);
+int usb_process_devicetree(const void *blob);
#endif /* _TEGRA_USB_H_ */
diff --git a/arch/arm/include/asm/ehci-omap.h b/arch/arm/include/asm/ehci-omap.h
index ac83a53..c7bca05 100644
--- a/arch/arm/include/asm/ehci-omap.h
+++ b/arch/arm/include/asm/ehci-omap.h
@@ -145,8 +145,8 @@ struct omap_ehci {
struct ehci_hccr;
struct ehci_hcor;
-int omap_ehci_hcd_init(struct omap_usbhs_board_data *usbhs_pdata,
- struct ehci_hccr **hccr, struct ehci_hcor **hcor);
+int omap_ehci_hcd_init(int index, struct omap_usbhs_board_data *usbhs_pdata,
+ struct ehci_hccr **hccr, struct ehci_hcor **hcor);
int omap_ehci_hcd_stop(void);
#endif /* _OMAP_COMMON_EHCI_H_ */
diff --git a/board/amcc/canyonlands/canyonlands.c
b/board/amcc/canyonlands/canyonlands.c
index cc36f45..8e85bee 100644
--- a/board/amcc/canyonlands/canyonlands.c
+++ b/board/amcc/canyonlands/canyonlands.c
@@ -16,6 +16,7 @@
#include asm/4xx_pcie.h
#include asm/ppc4xx-gpio.h
#include asm/errno.h
+#include usb.h
extern flash_info_t flash_info[CONFIG_SYS_MAX_FLASH_BANKS]; /* info for FLASH
chips */
@@ -188,7 +189,7 @@ int board_early_init_f(void)
}
#if defined(CONFIG_USB_OHCI_NEW) defined(CONFIG_SYS_USB_OHCI_BOARD_INIT)
-int usb_board_init(void)
+int board_usb_init(int index, enum board_usb_init_type init)
{
struct board_bcsr *bcsr_data =
(struct board_bcsr *)CONFIG_SYS_BCSR_BASE;
@@ -229,7 +230,7 @@ int usb_board_stop(void)
return 0;
}
-int usb_board_init_fail(void)
+int board_usb_init_fail(void)
{
return usb_board_stop();
}
diff --git a/board/balloon3/balloon3.c b/board/balloon3/balloon3.c
index ecbac16..891e48a 100644
--- a/board/balloon3/balloon3.c
+++ b/board/balloon3/balloon3.c
@@ -13,6 +13,7 @@
#include asm/io.h
#include spartan3.h
#include command.h
+#include usb.h
DECLARE_GLOBAL_DATA_PTR;
@@ -59,7 +60,7 @@ void dram_init_banksize(void)
}
#ifdef CONFIG_CMD_USB
-int usb_board_init(void)
+int board_usb_init(int index, enum board_usb_init_type init)
{
writel((readl(UHCHR) | UHCHR_PCPL | UHCHR_PSPL)
~(UHCHR_SSEP0 | UHCHR_SSEP1 | UHCHR_SSEP2 | UHCHR_SSE),
@@
