Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-06 Thread Markus Hubig 
On Mon, Aug 06, 2012 at 11:05:03AM +0200, Markus Hubig wrote:
 This adds support for the AT91SAM9G20 boards by taskit GmbH.
 Both boards, Stamp9G20 and PortuxG20, are integrated in one
 file. PortuxG20 is basically a SBC built around the Stamp9G20.

Ignore this. I started a new thread with [PATCH v4] in the subject.

Cheers, Markus
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-06 Thread Andreas Bießmann 
Dear Markus Hubig,

On 03.08.2012 14:05, Markus Hubig wrote:
 On Thu, Aug 02, 2012 at 03:28:30PM +0200, Markus Hubig wrote:
 On Wed, Aug 01, 2012 at 10:21:04PM +0200, Andreas Bießmann wrote:
 On 01.08.12 21:28, Markus Hubig wrote:
 On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
 +/* Need to reset PHY - 500ms reset */
 +writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
 +AT91_RSTC_MR_URSTEN, rstc-mr);

 Hmm ... is it OK to generate the user reset here? I know this is the
 same in at least at91sam9263ek, can you please check if we should
 instead delete that bit in MR?

 MR? Sorry I don't get this one. Please explain a bit ...

 I talked about URSTEN bit in RSTC_MR (Reset Controller Mode Register;
 p99 in at91sam9g20 datasheet). The URSTEN bit set to 1 means disable low
 level detection on NRST pin. Which in fact disables external reset with
 the reset key. One have to check if this is true or maybe I'm wrong here.

 Hmm ok I'll investigate this a bit further ...
 
 OK I looked this up in the at91sam9g20 datasheet, but as fahr as I understand
 it, setting AT91_RSTC_MR_URSTEN enables the *detection* of a low level on NRST
 to trigger USER RESET, while in order to perform the PHY Reset, NRST just hast
 to get low. To get NRST low, one has to set EXTRST in RSTC_CR.

correct.

 
 So to perform a PHY Reset, the correct code should be:
 
 | /* Reset PHY for 500ms */
 | writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13)
 |  ~AT91_RSTC_MR_URSTEN, rstc-mr);
 | writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST, rstc-cr);
 
 I tested it with my PortuxG20 and Network is still working!
 
 Unfortunatlie the Portux has no reset button, but I think for all at91sam9g20
 boards with a reset button the AT91_RSTC_MR_URSTEN has to be high by default,
 so somewhere in arch/arm/cpu/arm926ejs/at91/ there has to be some code like
 this:
 
 |#ifdef CONFIG_AT91_RESET_BUTTON
 |struct at91_rstc *rstc = (struct at91_rstc *)ATMEL_BASE_RSTC;
 |writel(AT91_RSTC_KEY | AT91_RSTC_MR_URSTEN, rstc-mr);
 |#endif

Well, I think it is up to the board maintainer to verify the reset
switch is working.

Best regards

Andreas Bießmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-06 Thread Andreas Bießmann 
Dear Markus Hubig,

On 02.08.2012 16:14, Markus Hubig wrote:
 On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
 On 30.07.12 20:01, Markus Hubig wrote:

 
 snipp
 
 +int board_early_init_f(void)
 +{
 +   struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
 +
 +   /* Enable clocks for all PIOs */
 +   writel((1  ATMEL_ID_PIOA) | (1  ATMEL_ID_PIOB) |
 +   (1  ATMEL_ID_PIOC), pmc-pcer);

 you should initialize seriald_hw here to avoid strange characters on
 serial line when switching from at91bootstrap to u-boot.
 
 snip
 
 +   /* adress of boot parameters */
 +   gd-bd-bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
 +
 +   at91_set_gpio_output(AT91_PIN_PC9, 1);
 +   at91_set_gpio_output(AT91_PIN_PC5, 1);

 Can you please add some comment why switching these pins?
 
 OK now I now that PC5 switches the red LED on and there since is a
 LED framework in u-boot I will use that in a later patch.
 
 PC9 is somewhat strange. If I set it to 0 I don't have a console!

can you please ask at taskit what function this pin has and document it
here?

 So I tried to put both
 
 | at91_set_gpio_output(AT91_PIN_PC9, 1);
 | at91_seriald_hw_init();
 
 into board_early_init_f() to avoid the strange characters at boot time,
 but again no console output ...
 
 Further tests showed that It seems that I can't use at91_set_gpio_output()
 inside board_early_init_f(). Switching on the red LED with PC5 also do not
 work in board_early_init_f() ...
 
 Any ideas?

Not currently. Maybe the PC9 has some vital functionality for UART to
work (some switch, power, ...) and is reset by another part running
after board_early_init_f()?

 
 Cheers, Markus

Best regards

Andreas Bießmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-06 Thread Markus Hubig 
On Mon, Aug 06, 2012 at 02:49:51PM +0200, Andreas Bießmann wrote:
 On 02.08.2012 16:14, Markus Hubig wrote:
  On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
  On 30.07.12 20:01, Markus Hubig wrote:
  
  PC9 is somewhat strange. If I set it to 0 I don't have a console!
 
 can you please ask at taskit what function this pin has and document it
 here?

I'm already waiting for an aswer ...

  So I tried to put both
  
  | at91_set_gpio_output(AT91_PIN_PC9, 1);
  | at91_seriald_hw_init();
  
  into board_early_init_f() to avoid the strange characters at boot time,
  but again no console output ...
  
  Further tests showed that It seems that I can't use at91_set_gpio_output()
  inside board_early_init_f(). Switching on the red LED with PC5 also do not
  work in board_early_init_f() ...
  
  Any ideas?
 
 Not currently. Maybe the PC9 has some vital functionality for UART to
 work (some switch, power, ...) and is reset by another part running
 after board_early_init_f()?

But how can I reset this PIN without using 'AT91_PIN_PC9'? I'm grep'ed all
over the source code but don't find nothing! Is there another way of setting
the PIO pins?

Cheers, Markus
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-03 Thread Markus Hubig 
On Thu, Aug 02, 2012 at 03:28:30PM +0200, Markus Hubig wrote:
 On Wed, Aug 01, 2012 at 10:21:04PM +0200, Andreas Bießmann wrote:
  On 01.08.12 21:28, Markus Hubig wrote:
   On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
   +   /* Need to reset PHY - 500ms reset */
   +   writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
   +   AT91_RSTC_MR_URSTEN, rstc-mr);
  
   Hmm ... is it OK to generate the user reset here? I know this is the
   same in at least at91sam9263ek, can you please check if we should
   instead delete that bit in MR?
   
   MR? Sorry I don't get this one. Please explain a bit ...
  
  I talked about URSTEN bit in RSTC_MR (Reset Controller Mode Register;
  p99 in at91sam9g20 datasheet). The URSTEN bit set to 1 means disable low
  level detection on NRST pin. Which in fact disables external reset with
  the reset key. One have to check if this is true or maybe I'm wrong here.
 
 Hmm ok I'll investigate this a bit further ...

OK I looked this up in the at91sam9g20 datasheet, but as fahr as I understand
it, setting AT91_RSTC_MR_URSTEN enables the *detection* of a low level on NRST
to trigger USER RESET, while in order to perform the PHY Reset, NRST just hast
to get low. To get NRST low, one has to set EXTRST in RSTC_CR.

So to perform a PHY Reset, the correct code should be:

| /* Reset PHY for 500ms */
| writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13)
|~AT91_RSTC_MR_URSTEN, rstc-mr);
| writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST, rstc-cr);

I tested it with my PortuxG20 and Network is still working!

Unfortunatlie the Portux has no reset button, but I think for all at91sam9g20
boards with a reset button the AT91_RSTC_MR_URSTEN has to be high by default,
so somewhere in arch/arm/cpu/arm926ejs/at91/ there has to be some code like
this:

|#ifdef CONFIG_AT91_RESET_BUTTON
|struct at91_rstc *rstc = (struct at91_rstc *)ATMEL_BASE_RSTC;
|writel(AT91_RSTC_KEY | AT91_RSTC_MR_URSTEN, rstc-mr);
|#endif

Cheers, Markus
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-02 Thread Markus Hubig 
On Wed, Aug 01, 2012 at 10:21:04PM +0200, Andreas Bießmann wrote:
 On 01.08.12 21:28, Markus Hubig wrote:
  On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
  + /* Need to reset PHY - 500ms reset */
  + writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
  + AT91_RSTC_MR_URSTEN, rstc-mr);
 
  Hmm ... is it OK to generate the user reset here? I know this is the
  same in at least at91sam9263ek, can you please check if we should
  instead delete that bit in MR?
  
  MR? Sorry I don't get this one. Please explain a bit ...
 
 I talked about URSTEN bit in RSTC_MR (Reset Controller Mode Register;
 p99 in at91sam9g20 datasheet). The URSTEN bit set to 1 means disable low
 level detection on NRST pin. Which in fact disables external reset with
 the reset key. One have to check if this is true or maybe I'm wrong here.

Hmm ok I'll investigate this a bit further ...

  |   /* avoid shutdown by watchdog */
  |   hw_watchdog_reset();
 
   WATCHDOG_RESET();

Fixed!

  |   if (timeout = 0) {
  |   debug(ERROR: Timeout waiting for PHY reset!\n);
 
 Error messages should not use debug macro.

Fixed!

 For timeout stuff you could also use get_timer(0) to get current
 timestamp and compare against another timestamp.

Fixed!

Cheers, Markus
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-02 Thread Markus Hubig 
On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
 On 30.07.12 20:01, Markus Hubig wrote:
 

snipp

  +int board_early_init_f(void)
  +{
  +   struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
  +
  +   /* Enable clocks for all PIOs */
  +   writel((1  ATMEL_ID_PIOA) | (1  ATMEL_ID_PIOB) |
  +   (1  ATMEL_ID_PIOC), pmc-pcer);
 
 you should initialize seriald_hw here to avoid strange characters on
 serial line when switching from at91bootstrap to u-boot.

snip

  +   /* adress of boot parameters */
  +   gd-bd-bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
  +
  +   at91_set_gpio_output(AT91_PIN_PC9, 1);
  +   at91_set_gpio_output(AT91_PIN_PC5, 1);
 
 Can you please add some comment why switching these pins?

OK now I now that PC5 switches the red LED on and there since is a
LED framework in u-boot I will use that in a later patch.

PC9 is somewhat strange. If I set it to 0 I don't have a console!

So I tried to put both

| at91_set_gpio_output(AT91_PIN_PC9, 1);
| at91_seriald_hw_init();

into board_early_init_f() to avoid the strange characters at boot time,
but again no console output ...

Further tests showed that It seems that I can't use at91_set_gpio_output()
inside board_early_init_f(). Switching on the red LED with PC5 also do not
work in board_early_init_f() ...

Any ideas?

Cheers, Markus
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-01 Thread Andreas Bießmann 
Dear Markus Hubig,

first of all:

---8---
andreas@andreas-mbp % ./tools/checkpatch.pl
U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch
WARNING: Whitespace before semicolon
#214: FILE: board/taskit/stamp9g20/stamp9g20.c:123:
+   ;

total: 0 errors, 1 warnings, 484 lines checked

NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
MULTISTATEMENT_MACRO_USE_DO_WHILE

U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch has style
problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---8---

I know this part is copied from other at91 boards but I wonder how we
should handle these while-loops without content.

On 30.07.12 20:01, Markus Hubig wrote:
 This adds support for the AT91SAM9G20 boards by taskit GmbH.
 Both boards, Stamp9G20 and PortuxG20, are integrated in one
 file. PortuxG20 is basically a SBC built around the Stamp9G20.
 
 Signed-off-by: Markus Hubig mhu...@imko.de
 Cc: Andreas Bießmann andreas.de...@googlemail.com
 ---
  board/taskit/stamp9g20/Makefile|   52 
  board/taskit/stamp9g20/stamp9g20.c |  199 +++
  boards.cfg |2 +
  include/configs/stamp9g20.h|  225 
 

MAINTAINER entry is missing

  4 files changed, 478 insertions(+), 0 deletions(-)
  create mode 100644 board/taskit/stamp9g20/Makefile
  create mode 100644 board/taskit/stamp9g20/stamp9g20.c
  create mode 100644 include/configs/stamp9g20.h
 
 diff --git a/board/taskit/stamp9g20/Makefile b/board/taskit/stamp9g20/Makefile
 new file mode 100644
 index 000..e99bfaa
 --- /dev/null
 +++ b/board/taskit/stamp9g20/Makefile
 @@ -0,0 +1,52 @@
 +#
 +# (C) Copyright 2003-2008
 +# Wolfgang Denk, DENX Software Engineering, w...@denx.de.
 +#
 +# (C) Copyright 2008
 +# Stelian Pop stel...@popies.net
 +# Lead Tech Design www.leadtechdesign.com
 +#
 +# (C) Copyright 2012
 +# Markus Hubig mhu...@imko.de
 +# IMKO GmbH www.imko.de
 +#
 +# See file CREDITS for list of people who contributed to this
 +# project.
 +#
 +# This program is free software; you can redistribute it and/or
 +# modify it under the terms of the GNU General Public License as
 +# published by the Free Software Foundation; either version 2 of
 +# the License, or (at your option) any later version.
 +#
 +# This program is distributed in the hope that it will be useful,
 +# but WITHOUT ANY WARRANTY; without even the implied warranty of
 +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 +# GNU General Public License for more details.
 +#
 +# You should have received a copy of the GNU General Public License
 +# along with this program; if not, write to the Free Software
 +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 +# MA 02111-1307 USA
 +#
 +
 +include $(TOPDIR)/config.mk
 +
 +LIB  = $(obj)lib$(BOARD).o
 +
 +COBJS-y  += stamp9g20.o
 +
 +SRCS := $(SOBJS:.o=.S) $(COBJS-y:.o=.c)
 +OBJS := $(addprefix $(obj),$(COBJS-y))
 +SOBJS:= $(addprefix $(obj),$(SOBJS))
 +
 +$(LIB):  $(obj).depend $(OBJS) $(SOBJS)
 + $(call cmd_link_o_target, $(OBJS) $(SOBJS))
 +
 +#
 +
 +# defines $(obj).depend target
 +include $(SRCTREE)/rules.mk
 +
 +sinclude $(obj).depend
 +
 +#
 diff --git a/board/taskit/stamp9g20/stamp9g20.c 
 b/board/taskit/stamp9g20/stamp9g20.c
 new file mode 100644
 index 000..b87de51
 --- /dev/null
 +++ b/board/taskit/stamp9g20/stamp9g20.c
 @@ -0,0 +1,199 @@
 +/*
 + * (C) Copyright 2007-2008
 + * Stelian Pop stel...@popies.net
 + * Lead Tech Design www.leadtechdesign.com
 + *
 + * Achim Ehrlich aehrl...@taskit.de
 + * taskit GmbH www.taskit.de
 + *
 + * (C) Copyright 2012-
 + * Markus Hubig mhu...@imko.de
 + * IMKO GmbH www.imko.de
 + *
 + * See file CREDITS for list of people who contributed to this
 + * project.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA
 + */
 +
 +#include common.h
 +#include asm/io.h
 +#include asm/arch/at91sam9260_matrix.h
 +#include asm/arch/at91sam9_smc.h
 +#include asm/arch/at91_common.h
 +#include asm/arch/at91_pmc.h
 +#include asm/arch/at91_rstc.h

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-01 Thread Markus Hubig 
Hello Andreas,

thanks for your responce. I will provide an updated patch based on
your comments!

On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
 ---8---
 andreas@andreas-mbp % ./tools/checkpatch.pl
 U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch
 WARNING: Whitespace before semicolon
 #214: FILE: board/taskit/stamp9g20/stamp9g20.c:123:
 + ;
 
 total: 0 errors, 1 warnings, 484 lines checked
 
 NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
 MULTISTATEMENT_MACRO_USE_DO_WHILE
 
 U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch has style
 problems, please review.
 
 If any of these errors are false positives, please report
 them to the maintainer, see CHECKPATCH in MAINTAINERS.
 ---8---
 
 I know this part is copied from other at91 boards but I wonder how we
 should handle these while-loops without content.

Oh I didn't recognice there is a version of checkpatch provided with
u-boot. I used the one from kernel.org which didn't put a warning out
for this one ...

 On 30.07.12 20:01, Markus Hubig wrote:
  This adds support for the AT91SAM9G20 boards by taskit GmbH.
  Both boards, Stamp9G20 and PortuxG20, are integrated in one
  file. PortuxG20 is basically a SBC built around the Stamp9G20.
  
  Signed-off-by: Markus Hubig mhu...@imko.de
  Cc: Andreas Bießmann andreas.de...@googlemail.com
  ---
   board/taskit/stamp9g20/Makefile|   52 
   board/taskit/stamp9g20/stamp9g20.c |  199 +++
   boards.cfg |2 +
   include/configs/stamp9g20.h|  225 
  
 
 MAINTAINER entry is missing

Fixed!

  +#ifdef CONFIG_MACB
  +static void stamp9G20_macb_hw_init(void)
  +{
  +   struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
  +   struct at91_port *pioa = (struct at91_port *)ATMEL_BASE_PIOA;
  +   struct at91_rstc *rstc = (struct at91_rstc *)ATMEL_BASE_RSTC;
  +   unsigned long erstl;
  +
  +   /* Enable MACB Chip, this is the enable PIN on Stamp Adaptor*/
 This comment is a bit misleading. Isn't MACB the SoC MAC component? Why
 should we switch an external element to enable an internal part? Can you
 please rewrite the comment to be more precise?

Hmm yes you're right. That pin enables the PHY Chip which of course is
an external component ...

  +   at91_set_gpio_output(AT91_PIN_PA26, 0);
  +
  +   /* Enable EMAC clock */
  +   writel(1  ATMEL_ID_EMAC0, pmc-pcer);
 
 Is it possible to move this 'enable clock' into at91_macb_hw_init()?
 Does your PHY address setting still work then?
 If yes, can you please send a separate patch first adding the 'enable
 clock' into the correct at91_macb_hw_init()?

Yes it works. I put it into arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c
and will send a separate patch.

  +   /* Need to reset PHY - 500ms reset */
  +   writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
  +   AT91_RSTC_MR_URSTEN, rstc-mr);
 
 Hmm ... is it OK to generate the user reset here? I know this is the
 same in at least at91sam9263ek, can you please check if we should
 instead delete that bit in MR?

MR? Sorry I don't get this one. Please explain a bit ...

  +
  +   writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST, rstc-cr);
  +
  +   /* Wait for end hardware reset */
  +   while (!(readl(rstc-sr)  AT91_RSTC_SR_NRSTL))
  +   ;
 
 Endless loop if bit is not toggling, can you please add some watchdog
 reset (if you still use wdt in your board) and some timeout?
 This will also solve the warning detected by checkpatch.

Maybe something like this?

| /* Wait for end of hardware reset */
| while (!(readl(rstc-sr)  AT91_RSTC_SR_NRSTL))
| {
|   /* avoid shutdown by watchdog */
|   hw_watchdog_reset();
|   
|   mdelay(10);
|   timeout--;
|   
|   /* timeout for not getting stuck in an endless loop */
|   if (timeout = 0) {
|   debug(ERROR: Timeout waiting for PHY reset!\n);
|   break;
|   };
| };

  +int board_early_init_f(void)
  +{
  +   struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
  +
  +   /* Enable clocks for all PIOs */
  +   writel((1  ATMEL_ID_PIOA) | (1  ATMEL_ID_PIOB) |
  +   (1  ATMEL_ID_PIOC), pmc-pcer);
 
 you should initialize seriald_hw here to avoid strange characters on
 serial line when switching from at91bootstrap to u-boot.

If I do so I don't get serial output any more ;-(

  +#ifdef CONFIG_PORTUXG20
  +   gd-bd-bi_arch_number = MACH_TYPE_PORTUXG20;
  +#else
  +   gd-bd-bi_arch_number = MACH_TYPE_STAMP9G20;
  +#endif
 
 I would favor the generic CONFIG_MACH_TYPE here - see
 arch/arm/lib/board.c:401
 Just add the definition in your board config header and remove these
 lines here.

Fixed!

  +   /* adress of boot parameters */
  +   gd-bd-bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
  +
  +   at91_set_gpio_output(AT91_PIN_PC9, 1);
  +   at91_set_gpio_output(AT91_PIN_PC5, 1);
 
 Can you please add some comment why switching

Re: [U-Boot] [PATCH] at91: Add support for taskit AT91SAM9G20 boards.

2012-08-01 Thread Andreas Bießmann 
Hi Markus,

On 01.08.12 21:28, Markus Hubig wrote:
 Hello Andreas,
 
 thanks for your responce. I will provide an updated patch based on
 your comments!
 
 On Wed, Aug 01, 2012 at 11:58:22AM +0200, Andreas Bießmann wrote:
 ---8---
 andreas@andreas-mbp % ./tools/checkpatch.pl
 U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch
 WARNING: Whitespace before semicolon
 #214: FILE: board/taskit/stamp9g20/stamp9g20.c:123:
 +;

 total: 0 errors, 1 warnings, 484 lines checked

 NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
 MULTISTATEMENT_MACRO_USE_DO_WHILE

 U-Boot-at91-Add-support-for-taskit-AT91SAM9G20-boards..patch has style
 problems, please review.

 If any of these errors are false positives, please report
 them to the maintainer, see CHECKPATCH in MAINTAINERS.
 ---8---

 I know this part is copied from other at91 boards but I wonder how we
 should handle these while-loops without content.
 
 Oh I didn't recognice there is a version of checkpatch provided with
 u-boot. I used the one from kernel.org which didn't put a warning out
 for this one ...
 
 On 30.07.12 20:01, Markus Hubig wrote:
 This adds support for the AT91SAM9G20 boards by taskit GmbH.
 Both boards, Stamp9G20 and PortuxG20, are integrated in one
 file. PortuxG20 is basically a SBC built around the Stamp9G20.

 Signed-off-by: Markus Hubig mhu...@imko.de
 Cc: Andreas Bießmann andreas.de...@googlemail.com
 ---
  board/taskit/stamp9g20/Makefile|   52 
  board/taskit/stamp9g20/stamp9g20.c |  199 +++
  boards.cfg |2 +
  include/configs/stamp9g20.h|  225 
 

 MAINTAINER entry is missing
 
 Fixed!
 
 +#ifdef CONFIG_MACB
 +static void stamp9G20_macb_hw_init(void)
 +{
 +   struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
 +   struct at91_port *pioa = (struct at91_port *)ATMEL_BASE_PIOA;
 +   struct at91_rstc *rstc = (struct at91_rstc *)ATMEL_BASE_RSTC;
 +   unsigned long erstl;
 +
 +   /* Enable MACB Chip, this is the enable PIN on Stamp Adaptor*/
 This comment is a bit misleading. Isn't MACB the SoC MAC component? Why
 should we switch an external element to enable an internal part? Can you
 please rewrite the comment to be more precise?
 
 Hmm yes you're right. That pin enables the PHY Chip which of course is
 an external component ...
 
 +   at91_set_gpio_output(AT91_PIN_PA26, 0);
 +
 +   /* Enable EMAC clock */
 +   writel(1  ATMEL_ID_EMAC0, pmc-pcer);

 Is it possible to move this 'enable clock' into at91_macb_hw_init()?
 Does your PHY address setting still work then?
 If yes, can you please send a separate patch first adding the 'enable
 clock' into the correct at91_macb_hw_init()?
 
 Yes it works. I put it into arch/arm/cpu/arm926ejs/at91/at91sam9260_devices.c
 and will send a separate patch.
 
 +   /* Need to reset PHY - 500ms reset */
 +   writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
 +   AT91_RSTC_MR_URSTEN, rstc-mr);

 Hmm ... is it OK to generate the user reset here? I know this is the
 same in at least at91sam9263ek, can you please check if we should
 instead delete that bit in MR?
 
 MR? Sorry I don't get this one. Please explain a bit ...

I talked about URSTEN bit in RSTC_MR (Reset Controller Mode Register;
p99 in at91sam9g20 datasheet). The URSTEN bit set to 1 means disable low
level detection on NRST pin. Which in fact disables external reset with
the reset key. One have to check if this is true or maybe I'm wrong here.

 +
 +   writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST, rstc-cr);
 +
 +   /* Wait for end hardware reset */
 +   while (!(readl(rstc-sr)  AT91_RSTC_SR_NRSTL))
 +   ;

 Endless loop if bit is not toggling, can you please add some watchdog
 reset (if you still use wdt in your board) and some timeout?
 This will also solve the warning detected by checkpatch.
 
 Maybe something like this?
 

 | /* Wait for end of hardware reset */
 | while (!(readl(rstc-sr)  AT91_RSTC_SR_NRSTL))
 | {
 | /* avoid shutdown by watchdog */
 | hw_watchdog_reset();

WATCHDOG_RESET();

 | 
 | mdelay(10);
 | timeout--;
 | 
 | /* timeout for not getting stuck in an endless loop */
 | if (timeout = 0) {
 | debug(ERROR: Timeout waiting for PHY reset!\n);

Error messages should not use debug macro.

 | break;
 | };
 | };

For timeout stuff you could also use get_timer(0) to get current
timestamp and compare against another timestamp.

snip

 
 PS.: I will send a new patch with all your requested changes already
 included, not a couple of small ones based in this one? Right!?

Yes, just a new version (v2), it is still arrived, will have a look then.

Best regards

Andreas Bießmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot