[U-Boot] [PATCH 1/1] Add support for Eukrea CPUAT91 SBC

[Wolfgang Denk]

Dear Eric Benard,

In message <1247947830-22602-1-git-send-email-eric at eukrea.com> you wrote:
> CPUAT91 is built around Atmel's AT91RM9200 and has up to 16MB of NOR
> flash, up to 128MB of SDRAM, and includes a Micrel KS8721 PHY in RMII
> mode.
> 
> Signed-off-by: Eric Benard <eric at eukrea.com>
> ---

Your subject line reads:

	[PATCH 1/1] Add support for Eukrea CPUAT91 SBC

Note that the "1/1" part is bogus as you submit just a single patch -
please omit it.

Second, it seems that this is a resubmit of an earlier patch. You are
suppposed to mark it as such, for example by using something like
"[PATCH v2]" in the subject, and you are also supposed to include at
least a short summary what has been changed compared to your previous
version (add this here, i. e. below the "---" line).


> diff --git a/MAINTAINERS b/MAINTAINERS
> index 575a7ec..da64571 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1,4 +1,4 @@
> -#########################################################################
> +9#########################################################################
^^^^^

Here you inserted garbage. Please fix.

> --- /dev/null
> +++ b/board/eukrea/cpuat91/cpuat91.c
> +int board_init(void)
> +{
> +	/* Enable Ctrlc */
> +	console_init_f();
> +
> +	/* memory and cpu-speed are setup before relocation */
> +	/* so we do _nothing_ here */

Incorrect multi-line comment style.

> +	/* arch number of CPUAT91-Board */
> +	gd->bd->bi_arch_number = MACH_TYPE_CPUAT91;
> +	/* adress of boot parameters */
> +	gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
> +
> +	return 0;
> +}
> +
> +int dram_init(void)
> +{
> +	gd->bd->bi_dram[0].start = PHYS_SDRAM;
> +	gd->bd->bi_dram[0].size = PHYS_SDRAM_SIZE;
> +	return 0;
> +}
> +
> +#if defined(CONFIG_DRIVER_ETHER)
> +#if defined(CONFIG_CMD_NET)
> +
> +/*
> + * Name:
> + *	at91rm9200_GetPhyInterface
> + * Description:
> + *	Initialise the interface functions to the PHY
> + * Arguments:
> + *	None
> + * Return value:
> + *	None
> + */
> +void at91rm9200_GetPhyInterface(AT91PS_PhyOps p_phyops)
> +{
> +	p_phyops->Init = ks8721_InitPhy;
> +	p_phyops->IsPhyConnected = ks8721_IsPhyConnected;
> +	p_phyops->GetLinkSpeed = ks8721_GetLinkSpeed;
> +	p_phyops->AutoNegotiate = ks8721_AutoNegotiate;
> +}
> +
> +#endif	/* CONFIG_CMD_NET */
> +#endif	/* CONFIG_DRIVER_ETHER */
> diff --git a/cpu/arm920t/at91rm9200/Makefile b/cpu/arm920t/at91rm9200/Makefile
> index 73aeeac..114d8ad 100644
> --- a/cpu/arm920t/at91rm9200/Makefile
> +++ b/cpu/arm920t/at91rm9200/Makefile
> @@ -31,14 +31,15 @@ COBJS	+= bcm5221.o
>  COBJS	+= dm9161.o
>  COBJS	+= ether.o
>  COBJS	+= i2c.o
> +COBJS-$(CONFIG_KS8721_PHY)	+= ks8721.o
>  COBJS	+= lxt972.o
>  COBJS	+= reset.o
>  COBJS	+= spi.o
>  COBJS	+= timer.o
>  COBJS	+= usb.o
>  
> -SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
> -OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
> +SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c) $(COBJS-y:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS) $(COBJS-y))
>  
>  all:	$(obj).depend $(LIB)
>  
> diff --git a/cpu/arm920t/at91rm9200/ks8721.c b/cpu/arm920t/at91rm9200/ks8721.c
> new file mode 100644
> index 0000000..703e58c
> --- /dev/null
> +++ b/cpu/arm920t/at91rm9200/ks8721.c
...
> + * Return value:
> + *	TRUE - if id read successfully
> + *	FALSE- if error
> + */

Please use 0 and 1 (or -1) as return values. We don't use TRUE and
FALSE here.

...
> +	if (stat1 & KS8721_10BASE_T_HD) {
> +		/*set MII for 10BaseT and Half Duplex  */
> +		printf("10BT HD\n");
> +		p_mac->EMAC_CFG &= ~(AT91C_EMAC_SPD | AT91C_EMAC_FD);
> +		return TRUE;
> +	}
> +	return FALSE;
> +}
> +
> +

Please use only a single blank line here (and in similar places).

> +/*
> + * Name:
> + *	ks8721_InitPhy
> + * Description:
> + *	MAC starts checking its link by using parallel detection and
> + *	Autonegotiation and the same is set in the MAC configuration registers
> + * Arguments:
> + *	p_mac - pointer to struct AT91S_EMAC
> + * Return value:
> + *	TRUE - if link status set succesfully
> + *	FALSE - if link status not set
> + */
> +UCHAR ks8721_InitPhy(AT91PS_EMAC p_mac)
> +{
> +	UCHAR ret = TRUE;

Please get rid of UCHAR an the like.

> +	unsigned short IntValue;
> +
> +	at91rm9200_EmacEnableMDIO(p_mac);

Please do not use CamelCase identifiers.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Our business is run on trust.  We trust you will pay in advance.