All of lore.kernel.org
 help / color / mirror / Atom feed
From: Angelo Dureghello <sysamfw@gmail.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v5 2/2] board: add support for amcore board
Date: Fri, 25 Jan 2013 15:04:31 +0100	[thread overview]
Message-ID: <20130125140431.GA18027@sion.sysam> (raw)
In-Reply-To: <20130125123054.EF1E5202D5B@gemini.denx.de>

Dear Wolfgang,

thanks for the review, 

On Fri, Jan 25, 2013 at 01:30:54PM +0100, Wolfgang Denk wrote:
> Dear Angelo Dureghello,
> 
> In message <20130125104307.GA3223@sion.sysam> you wrote:
> > Add support for Sysam AMCORE mcf5307 (coldfire) based board.
> 
> Sorry for the late catch - this escaped me so far...
> 
> > Changes for v5:
> > - Fix MAINTAINERS bad sorted entry
> > - Fix incorrect indentation
> > - Remove #undef where not needed
> 
> Did you? I still see some...
> 
> > +#if defined(CONFIG_SYS_DRAM_TEST)
> > +/* memory test */
> > +int testdram(void)
> ...
> > +#endif
> 
> This is:
> 
> 1) dead code, as you don't define CONFIG_SYS_DRAM_TEST
> 2) an obsolete approach that you should not use.  We have a several
>    different (and much more reliable) memory tests already - if yuou
>    really need one, then please use the existing code.  On the other
>    hand, you are using get_ram_size, which already includes a simple
>    memory test - so re you sure you really need an extra test?
> 
> 
> > +#undef	CONFIG_SYS_DRAM_TEST		/* default undef */
> 

I used the sdram test for some help when soldering here the prototypes. 
Then i disabled the test. As you already asked me, i removed my own
sdram test function and used one already used from other boards, and
added this in the amcore.c top coments.

 * Copy memory testdram() from sandburst/common/sb_common.c

Anyway, as you said, get_ram_size is enough for me. I will remove
CONFIG_SYS_DRAM_TEST and so "int testdram(void)" content.


> ==> remove.
> 
> > +/* bypass PLL for test purpose */
> > +#undef	CONFIG_SYS_PLL_BYPASS
> 
> ???
> 

I apologize, forget out, fixed.

If you haven't seen anything else, i'll post next V6 shortly.

Best Regards,
Angelo Dureghello

      reply	other threads:[~2013-01-25 14:04 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-01-25 10:43 [U-Boot] [PATCH v5 2/2] board: add support for amcore board Angelo Dureghello
2013-01-25 12:30 ` Wolfgang Denk
2013-01-25 14:04   ` Angelo Dureghello [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130125140431.GA18027@sion.sysam \
    --to=sysamfw@gmail.com \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.