[U-Boot] [PATCH 2/2] exynos5: arndale: Add network support

[Marek Vasut]

On Saturday, December 14, 2013 at 03:28:57 PM, Inderpal Singh wrote:
> On 14 December 2013 23:20, Marek Vasut <marex at 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 at denx.de> wrote:
> > > > On Saturday, October 19, 2013 at 08:49:27 AM, Inderpal Singh wrote:
> > > > > From: Chander Kashyap <chander.kashyap at 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 at linaro.org>
> > > > > Signed-off-by: Chander Kashyap <chander.kashyap at 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.