| Message ID | 1382165367-6618-3-git-send-email-inderpal.singh@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
On Saturday, October 19, 2013 at 08:49:27 AM, Inderpal Singh wrote: > From: Chander Kashyap <chander.kashyap@linaro.org> > > Arndale board has AX88760, which is USB 2.0 Hub & USB 2.0 Ethernet Combo > controller, connected to HSIC Phy of USB host controller via USB3503 hub. > > This patch implements a board specific board_usb_init function in ehci > driver to perform reset sequence for USB3503 hub and enables the relevant > config options for network to work. > > Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > --- > board/samsung/arndale/arndale.c | 23 +++++++++++++++++++++++ > drivers/usb/host/ehci-exynos.c | 9 +++++++++ > include/configs/arndale.h | 6 ++++++ > 3 files changed, 38 insertions(+) > > diff --git a/board/samsung/arndale/arndale.c > b/board/samsung/arndale/arndale.c index 052fecd..44f20b9 100644 > --- a/board/samsung/arndale/arndale.c > +++ b/board/samsung/arndale/arndale.c > @@ -7,10 +7,23 @@ > #include <common.h> > #include <asm/arch/pinmux.h> > #include <asm/arch/dwmmc.h> > +#include <asm/arch/gpio.h> > #include <asm/arch/power.h> > > DECLARE_GLOBAL_DATA_PTR; > > +#ifdef CONFIG_USB_EHCI_EXYNOS > +void board_usb_init(int value) > +{ > + struct exynos5_gpio_part1 *gpio = (struct exynos5_gpio_part1 *) > + samsung_get_base_gpio_part1(); > + > + /* Configure gpios for usb 3503 hub's reset and connect */ > + s5p_gpio_direction_output(&gpio->x3, 5, value); > + s5p_gpio_direction_output(&gpio->d1, 7, value); > +} > +#endif > + > int board_init(void) > { > gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL); > @@ -91,6 +104,16 @@ int board_early_init_f(void) > } > #endif > > +#ifdef CONFIG_BOARD_LATE_INIT > +int board_late_init(void) > +{ > +#ifdef CONFIG_PREBOOT > + setenv("preboot", CONFIG_PREBOOT); > + setenv("usbethaddr", "00:40:5c:26:0a:5b"); Why do you need these 'setenv' calls ? This logic seems completely broken. The 'preboot' env variable is set already by defining the CONFIG_PREBOOT and the usbethaddr shall be set by user. [...]
Hi Marek, Thanks for the review. On 12 December 2013 19:03, Marek Vasut <marex@denx.de> wrote: > On Saturday, October 19, 2013 at 08:49:27 AM, Inderpal Singh wrote: > > From: Chander Kashyap <chander.kashyap@linaro.org> > > > > Arndale board has AX88760, which is USB 2.0 Hub & USB 2.0 Ethernet Combo > > controller, connected to HSIC Phy of USB host controller via USB3503 hub. > > > > This patch implements a board specific board_usb_init function in ehci > > driver to perform reset sequence for USB3503 hub and enables the relevant > > config options for network to work. > > > > Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > > --- > > board/samsung/arndale/arndale.c | 23 +++++++++++++++++++++++ > > drivers/usb/host/ehci-exynos.c | 9 +++++++++ > > include/configs/arndale.h | 6 ++++++ > > 3 files changed, 38 insertions(+) > > > > diff --git a/board/samsung/arndale/arndale.c > > b/board/samsung/arndale/arndale.c index 052fecd..44f20b9 100644 > > --- a/board/samsung/arndale/arndale.c > > +++ b/board/samsung/arndale/arndale.c > > @@ -7,10 +7,23 @@ > > #include <common.h> > > #include <asm/arch/pinmux.h> > > #include <asm/arch/dwmmc.h> > > +#include <asm/arch/gpio.h> > > #include <asm/arch/power.h> > > > > DECLARE_GLOBAL_DATA_PTR; > > > > +#ifdef CONFIG_USB_EHCI_EXYNOS > > +void board_usb_init(int value) > > +{ > > + struct exynos5_gpio_part1 *gpio = (struct exynos5_gpio_part1 *) > > + > samsung_get_base_gpio_part1(); > > + > > + /* Configure gpios for usb 3503 hub's reset and connect */ > > + s5p_gpio_direction_output(&gpio->x3, 5, value); > > + s5p_gpio_direction_output(&gpio->d1, 7, value); > > +} > > +#endif > > + > > int board_init(void) > > { > > gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL); > > @@ -91,6 +104,16 @@ int board_early_init_f(void) > > } > > #endif > > > > +#ifdef CONFIG_BOARD_LATE_INIT > > +int board_late_init(void) > > +{ > > +#ifdef CONFIG_PREBOOT > > + setenv("preboot", CONFIG_PREBOOT); > > + setenv("usbethaddr", "00:40:5c:26:0a:5b"); > > Why do you need these 'setenv' calls ? This logic seems completely broken. > The > 'preboot' env variable is set already by defining the CONFIG_PREBOOT and > the > usbethaddr shall be set by user. > Since arndale is a development board so this was done so that user (developer) does not has to set usbethaddr on every boot. If its redundant then I will remove this in the next version of the patchset. Thanks, Inder > [...] > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On Saturday, December 14, 2013 at 03:00:39 PM, Inderpal Singh wrote: > Hi Marek, > > Thanks for the review. > > On 12 December 2013 19:03, Marek Vasut <marex@denx.de> wrote: > > On Saturday, October 19, 2013 at 08:49:27 AM, Inderpal Singh wrote: > > > From: Chander Kashyap <chander.kashyap@linaro.org> > > > > > > Arndale board has AX88760, which is USB 2.0 Hub & USB 2.0 Ethernet > > > Combo controller, connected to HSIC Phy of USB host controller via > > > USB3503 hub. > > > > > > This patch implements a board specific board_usb_init function in ehci > > > driver to perform reset sequence for USB3503 hub and enables the > > > relevant config options for network to work. > > > > > > Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> > > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > > > --- > > > > > > board/samsung/arndale/arndale.c | 23 +++++++++++++++++++++++ > > > drivers/usb/host/ehci-exynos.c | 9 +++++++++ > > > include/configs/arndale.h | 6 ++++++ > > > 3 files changed, 38 insertions(+) > > > > > > diff --git a/board/samsung/arndale/arndale.c > > > b/board/samsung/arndale/arndale.c index 052fecd..44f20b9 100644 > > > --- a/board/samsung/arndale/arndale.c > > > +++ b/board/samsung/arndale/arndale.c > > > @@ -7,10 +7,23 @@ > > > > > > #include <common.h> > > > #include <asm/arch/pinmux.h> > > > #include <asm/arch/dwmmc.h> > > > > > > +#include <asm/arch/gpio.h> > > > > > > #include <asm/arch/power.h> > > > > > > DECLARE_GLOBAL_DATA_PTR; > > > > > > +#ifdef CONFIG_USB_EHCI_EXYNOS > > > +void board_usb_init(int value) > > > +{ > > > + struct exynos5_gpio_part1 *gpio = (struct exynos5_gpio_part1 *) > > > + > > > > samsung_get_base_gpio_part1(); > > > > > + > > > + /* Configure gpios for usb 3503 hub's reset and connect */ > > > + s5p_gpio_direction_output(&gpio->x3, 5, value); > > > + s5p_gpio_direction_output(&gpio->d1, 7, value); > > > +} > > > +#endif > > > + > > > > > > int board_init(void) > > > { > > > > > > gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL); > > > > > > @@ -91,6 +104,16 @@ int board_early_init_f(void) > > > > > > } > > > #endif > > > > > > +#ifdef CONFIG_BOARD_LATE_INIT > > > +int board_late_init(void) > > > +{ > > > +#ifdef CONFIG_PREBOOT > > > + setenv("preboot", CONFIG_PREBOOT); > > > + setenv("usbethaddr", "00:40:5c:26:0a:5b"); > > > > Why do you need these 'setenv' calls ? This logic seems completely > > broken. The > > 'preboot' env variable is set already by defining the CONFIG_PREBOOT and > > the > > usbethaddr shall be set by user. > > Since arndale is a development board so this was done so that user > (developer) does not has to set usbethaddr on every boot. If its redundant > then I will remove this in the next version of the patchset. Can the user/developer/ ... just run: => setenv usbethaddr 00:11:22:33:44:fg => saveenv on the U-Boot command line? setenv("preboot", CONFIG_PREBOOT); is broken, the user might want to override this variable, yet you don't allow him to as you always reset it back to default value. If the board not loaded with a valid environment, a default env will be used and if CONFIG_PREBOOT is defined in your include/configs/<board>.h , then the 'preboot' variable will also be set to the default value so no need to set it like so explicitly. You can reset a variable using "env default -f preboot'.
On 14 December 2013 23:20, Marek Vasut <marex@denx.de> wrote: > On Saturday, December 14, 2013 at 03:00:39 PM, Inderpal Singh wrote: > > Hi Marek, > > > > Thanks for the review. > > > > On 12 December 2013 19:03, Marek Vasut <marex@denx.de> wrote: > > > On Saturday, October 19, 2013 at 08:49:27 AM, Inderpal Singh wrote: > > > > From: Chander Kashyap <chander.kashyap@linaro.org> > > > > > > > > Arndale board has AX88760, which is USB 2.0 Hub & USB 2.0 Ethernet > > > > Combo controller, connected to HSIC Phy of USB host controller via > > > > USB3503 hub. > > > > > > > > This patch implements a board specific board_usb_init function in > ehci > > > > driver to perform reset sequence for USB3503 hub and enables the > > > > relevant config options for network to work. > > > > > > > > Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> > > > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > > > > --- > > > > > > > > board/samsung/arndale/arndale.c | 23 +++++++++++++++++++++++ > > > > drivers/usb/host/ehci-exynos.c | 9 +++++++++ > > > > include/configs/arndale.h | 6 ++++++ > > > > 3 files changed, 38 insertions(+) > > > > > > > > diff --git a/board/samsung/arndale/arndale.c > > > > b/board/samsung/arndale/arndale.c index 052fecd..44f20b9 100644 > > > > --- a/board/samsung/arndale/arndale.c > > > > +++ b/board/samsung/arndale/arndale.c > > > > @@ -7,10 +7,23 @@ > > > > > > > > #include <common.h> > > > > #include <asm/arch/pinmux.h> > > > > #include <asm/arch/dwmmc.h> > > > > > > > > +#include <asm/arch/gpio.h> > > > > > > > > #include <asm/arch/power.h> > > > > > > > > DECLARE_GLOBAL_DATA_PTR; > > > > > > > > +#ifdef CONFIG_USB_EHCI_EXYNOS > > > > +void board_usb_init(int value) > > > > +{ > > > > + struct exynos5_gpio_part1 *gpio = (struct exynos5_gpio_part1 *) > > > > + > > > > > > samsung_get_base_gpio_part1(); > > > > > > > + > > > > + /* Configure gpios for usb 3503 hub's reset and connect */ > > > > + s5p_gpio_direction_output(&gpio->x3, 5, value); > > > > + s5p_gpio_direction_output(&gpio->d1, 7, value); > > > > +} > > > > +#endif > > > > + > > > > > > > > int board_init(void) > > > > { > > > > > > > > gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL); > > > > > > > > @@ -91,6 +104,16 @@ int board_early_init_f(void) > > > > > > > > } > > > > #endif > > > > > > > > +#ifdef CONFIG_BOARD_LATE_INIT > > > > +int board_late_init(void) > > > > +{ > > > > +#ifdef CONFIG_PREBOOT > > > > + setenv("preboot", CONFIG_PREBOOT); > > > > + setenv("usbethaddr", "00:40:5c:26:0a:5b"); > > > > > > Why do you need these 'setenv' calls ? This logic seems completely > > > broken. The > > > 'preboot' env variable is set already by defining the CONFIG_PREBOOT > and > > > the > > > usbethaddr shall be set by user. > > > > Since arndale is a development board so this was done so that user > > (developer) does not has to set usbethaddr on every boot. If its > redundant > > then I will remove this in the next version of the patchset. > > Can the user/developer/ ... just run: > > => setenv usbethaddr 00:11:22:33:44:fg > => saveenv > > on the U-Boot command line? > > setenv("preboot", CONFIG_PREBOOT); is broken, the user might want to > override > this variable, yet you don't allow him to as you always reset it back to > default > value. If the board not loaded with a valid environment, a default env > will be > used and if CONFIG_PREBOOT is defined in your include/configs/<board>.h , > then > the 'preboot' variable will also be set to the default value so no need to > set > it like so explicitly. You can reset a variable using "env default -f > preboot'. > Thanks a lot Marek for the explanation. Will remove this in the next patchset. Thanks, Inder
On Saturday, December 14, 2013 at 03:28:57 PM, Inderpal Singh wrote: > On 14 December 2013 23:20, Marek Vasut <marex@denx.de> wrote: > > On Saturday, December 14, 2013 at 03:00:39 PM, Inderpal Singh wrote: > > > Hi Marek, > > > > > > Thanks for the review. > > > > > > On 12 December 2013 19:03, Marek Vasut <marex@denx.de> wrote: > > > > On Saturday, October 19, 2013 at 08:49:27 AM, Inderpal Singh wrote: > > > > > From: Chander Kashyap <chander.kashyap@linaro.org> > > > > > > > > > > Arndale board has AX88760, which is USB 2.0 Hub & USB 2.0 Ethernet > > > > > Combo controller, connected to HSIC Phy of USB host controller via > > > > > USB3503 hub. > > > > > > > > > > This patch implements a board specific board_usb_init function in > > > > ehci > > > > > > > driver to perform reset sequence for USB3503 hub and enables the > > > > > relevant config options for network to work. > > > > > > > > > > Signed-off-by: Inderpal Singh <inderpal.singh@linaro.org> > > > > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > > > > > --- > > > > > > > > > > board/samsung/arndale/arndale.c | 23 +++++++++++++++++++++++ > > > > > drivers/usb/host/ehci-exynos.c | 9 +++++++++ > > > > > include/configs/arndale.h | 6 ++++++ > > > > > 3 files changed, 38 insertions(+) > > > > > > > > > > diff --git a/board/samsung/arndale/arndale.c > > > > > b/board/samsung/arndale/arndale.c index 052fecd..44f20b9 100644 > > > > > --- a/board/samsung/arndale/arndale.c > > > > > +++ b/board/samsung/arndale/arndale.c > > > > > @@ -7,10 +7,23 @@ > > > > > > > > > > #include <common.h> > > > > > #include <asm/arch/pinmux.h> > > > > > #include <asm/arch/dwmmc.h> > > > > > > > > > > +#include <asm/arch/gpio.h> > > > > > > > > > > #include <asm/arch/power.h> > > > > > > > > > > DECLARE_GLOBAL_DATA_PTR; > > > > > > > > > > +#ifdef CONFIG_USB_EHCI_EXYNOS > > > > > +void board_usb_init(int value) > > > > > +{ > > > > > + struct exynos5_gpio_part1 *gpio = (struct exynos5_gpio_part1 > > > > > *) + > > > > > > > > samsung_get_base_gpio_part1(); > > > > > > > > > + > > > > > + /* Configure gpios for usb 3503 hub's reset and connect */ > > > > > + s5p_gpio_direction_output(&gpio->x3, 5, value); > > > > > + s5p_gpio_direction_output(&gpio->d1, 7, value); > > > > > +} > > > > > +#endif > > > > > + > > > > > > > > > > int board_init(void) > > > > > { > > > > > > > > > > gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL); > > > > > > > > > > @@ -91,6 +104,16 @@ int board_early_init_f(void) > > > > > > > > > > } > > > > > #endif > > > > > > > > > > +#ifdef CONFIG_BOARD_LATE_INIT > > > > > +int board_late_init(void) > > > > > +{ > > > > > +#ifdef CONFIG_PREBOOT > > > > > + setenv("preboot", CONFIG_PREBOOT); > > > > > + setenv("usbethaddr", "00:40:5c:26:0a:5b"); > > > > > > > > Why do you need these 'setenv' calls ? This logic seems completely > > > > broken. The > > > > 'preboot' env variable is set already by defining the CONFIG_PREBOOT > > > > and > > > > > > the > > > > usbethaddr shall be set by user. > > > > > > Since arndale is a development board so this was done so that user > > > (developer) does not has to set usbethaddr on every boot. If its > > > > redundant > > > > > then I will remove this in the next version of the patchset. > > > > Can the user/developer/ ... just run: > > > > => setenv usbethaddr 00:11:22:33:44:fg > > => saveenv > > > > on the U-Boot command line? > > > > setenv("preboot", CONFIG_PREBOOT); is broken, the user might want to > > override > > this variable, yet you don't allow him to as you always reset it back to > > default > > value. If the board not loaded with a valid environment, a default env > > will be > > used and if CONFIG_PREBOOT is defined in your include/configs/<board>.h , > > then > > the 'preboot' variable will also be set to the default value so no need > > to set > > it like so explicitly. You can reset a variable using "env default -f > > preboot'. > > Thanks a lot Marek for the explanation. > Will remove this in the next patchset. Sure, HTH. Sorry I didn't review your patch, next time please at least add me to the CC or add 'usb:' tag to the patch.
diff --git a/board/samsung/arndale/arndale.c b/board/samsung/arndale/arndale.c index 052fecd..44f20b9 100644 --- a/board/samsung/arndale/arndale.c +++ b/board/samsung/arndale/arndale.c @@ -7,10 +7,23 @@ #include <common.h> #include <asm/arch/pinmux.h> #include <asm/arch/dwmmc.h> +#include <asm/arch/gpio.h> #include <asm/arch/power.h> DECLARE_GLOBAL_DATA_PTR; +#ifdef CONFIG_USB_EHCI_EXYNOS +void board_usb_init(int value) +{ + struct exynos5_gpio_part1 *gpio = (struct exynos5_gpio_part1 *) + samsung_get_base_gpio_part1(); + + /* Configure gpios for usb 3503 hub's reset and connect */ + s5p_gpio_direction_output(&gpio->x3, 5, value); + s5p_gpio_direction_output(&gpio->d1, 7, value); +} +#endif + int board_init(void) { gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL); @@ -91,6 +104,16 @@ int board_early_init_f(void) } #endif +#ifdef CONFIG_BOARD_LATE_INIT +int board_late_init(void) +{ +#ifdef CONFIG_PREBOOT + setenv("preboot", CONFIG_PREBOOT); + setenv("usbethaddr", "00:40:5c:26:0a:5b"); +#endif +} +#endif + #ifdef CONFIG_DISPLAY_BOARDINFO int checkboard(void) { diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index 50fbf36..cba450e 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -170,6 +170,11 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) set_usbhost_phy_ctrl(POWER_USB_HOST_PHY_CTRL_DISABLE); } +inline void __board_usb_init(int value) +{ +} +void board_usb_init(int) __attribute__((weak, alias("__board_usb_init"))); + /* * EHCI-initialization * Create the appropriate control structures to manage @@ -189,8 +194,12 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) ctx->hcd = (struct ehci_hccr *)samsung_get_base_usb_ehci(); #endif + board_usb_init(0); + setup_usb_phy(ctx->usb); + board_usb_init(1); + *hccr = ctx->hcd; *hcor = (struct ehci_hcor *)((uint32_t) *hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); diff --git a/include/configs/arndale.h b/include/configs/arndale.h index ed44a04..ea24661 100644 --- a/include/configs/arndale.h +++ b/include/configs/arndale.h @@ -116,6 +116,10 @@ #define CONFIG_USB_EHCI_EXYNOS #define CONFIG_USB_STORAGE +#define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3 +#define CONFIG_USB_HOST_ETHER +#define CONFIG_USB_ETHER_ASIX +#define CONFIG_PREBOOT "usb start" /* MMC SPL */ #define CONFIG_SPL #define COPY_BL2_FNPTR_ADDR 0x02020030 @@ -179,6 +183,8 @@ #define CONFIG_ENV_IS_IN_MMC #define CONFIG_SECURE_BL1_ONLY +#define CONFIG_BOARD_LATE_INIT + /* Secure FW size configuration */ #ifdef CONFIG_SECURE_BL1_ONLY #define CONFIG_SEC_FW_SIZE (8 << 10) /* 8KB */