[U-Boot] [PATCH 6/9] fec_mxc: add support for MX51 processor

[Wolfgang Denk]

Dear Stefano Babic,

In message <1263212760-27272-7-git-send-email-sbabic at denx.de> you wrote:
> The patch add support for the Freescale mx51 processor
> to the FEC ethernet driver.
> 
> Signed-off-by: Stefano Babic <sbabic at denx.de>
> ---
>  drivers/net/fec_mxc.c |   57 ++++++++++++++++++++++++++++++-------------------
>  1 files changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 19116f2..203832e 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -27,8 +27,10 @@
>  #include <miiphy.h>
>  #include "fec_mxc.h"
>  
> -#include <asm/arch/clock.h>
>  #include <asm/arch/imx-regs.h>
> +#ifndef CONFIG_MX51
> +#include <asm/arch/clock.h>
> +#endif

Can we not implement the clock stuff for the iMX51 in such a way that
we don't need #ifdef's here? 

> @@ -108,6 +110,23 @@ static int fec_miiphy_read(char *dev, uint8_t phyAddr, uint8_t regAddr,
>  	return 0;
>  }
>  
> +static void fec_mii_setspeed(struct fec_priv *fec)
> +{
> +#ifdef CONFIG_MX51
> +	writel((((mxc_get_clock(MXC_IPG_CLK) + 499999) / 5000000) << 1),
> +			&fec->eth->mii_speed);
> +#else
> +	/*
> +	 * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
> +	 * and do not drop the Preamble.
> +	 */
> +	writel((((imx_get_ahbclk() / 1000000) + 2) / 5) << 1,
> +			&fec->eth->mii_speed);

What exactly, in addition to the (technically irrelevant) names and
the different way how the rounding is implemented, requires the #ifdef
here?

The code looks pretty much the same to me. I think we should just use
common names for common functions, and get rid of such #ifdef's.

> @@ -236,7 +255,7 @@ static int fec_rbd_init(struct fec_priv *fec, int count, int size)
>  		fec->rdb_ptr = malloc(size * count + DB_DATA_ALIGNMENT);
>  	p = (uint32_t)fec->rdb_ptr;
>  	if (!p) {
> -		puts("fec_imx27: not enough malloc memory!\n");
> +		puts("fec_mxc: not enough malloc memory!\n");

Please also drop the '!' while you are changing this line.

>  static int fec_get_hwaddr(struct eth_device *dev, unsigned char *mac)
>  {
> +#ifndef CONFIG_MX51
>  	struct iim_regs *iim = (struct iim_regs *)IMX_IIM_BASE;
>  	int i;
>  
> @@ -306,6 +326,9 @@ static int fec_get_hwaddr(struct eth_device *dev, unsigned char *mac)
>  		mac[6-1-i] = readl(&iim->iim_bank_area0[IIM0_MAC + i]);
>  
>  	return is_valid_ether_addr(mac);
> +#else
> +	return -1;
> +#endif
>  }

General remark: please use positive logic in cases like this, as it
results in simpler code which is much easier to read.

Instead of:

	#ifndef CONFIG_MX51
		...
		many
		lines
		of
		code here
		followed
		by
		a
		return ...;
	#else
		return something_else;
	#endif

please write:

	#ifdef CONFIG_MX51
		return something_else;
	#endif

	many
	lines
	of
	code here
	followed by a
	return ...;

Less nesting, shorter lines, easier to read and understand.

But why do we need this #ifdef here?  If this function is of no use
on i.MX51, then why provide it?  And why call such a dummy function at
all?  Please let's get rid of these #ifdef's.


>  static int fec_set_hwaddr(struct eth_device *dev, unsigned char *mac)
> @@ -373,7 +396,7 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
>  				sizeof(struct fec_bd) + DB_ALIGNMENT);
>  	base = (uint32_t)fec->base_ptr;
>  	if (!base) {
> -		puts("fec_imx27: not enough malloc memory!\n");
> +		puts("fec_mxc: not enough malloc memory!\n");

See comment above; please apply globally.

>  static int fec_probe(bd_t *bd)
>  {
> -	struct pll_regs *pll = (struct pll_regs *)IMX_PLL_BASE;
>  	struct eth_device *edev;
>  	struct fec_priv *fec = &gfec;
>  	unsigned char ethaddr_str[20];
>  	unsigned char ethaddr[6];
> -	char *tmp = getenv("ethaddr");
> +	char *tmp;
>  	char *end;
> +#ifndef CONFIG_MX51
> +	struct pll_regs *pll = (struct pll_regs *)IMX_PLL_BASE;
>  
>  	/* enable FEC clock */
>  	writel(readl(&pll->pccr1) | PCCR1_HCLK_FEC, &pll->pccr1);
>  	writel(readl(&pll->pccr0) | PCCR0_FEC_EN, &pll->pccr0);
> +#endif

Can we implement this clock enable in a way that goes without #ifdef ?

>  	/* create and fill edev struct */
>  	edev = (struct eth_device *)malloc(sizeof(struct eth_device));
>  	if (!edev) {
> -		puts("fec_imx27: not enough malloc memory!\n");
> +		puts("fec_mxc: not enough malloc memory!\n");
>  		return -ENOMEM;
>  	}
>  	edev->priv = fec;
> @@ -702,14 +721,7 @@ static int fec_probe(bd_t *bd)
>  	 * Frame length=1518; MII mode;
>  	 */
>  	writel(0x05ee0024, &fec->eth->r_cntrl);	/* FIXME 0x05ee0004 */
> -	/*
> -	 * Set MII_SPEED = (1/(mii_speed * 2)) * System Clock
> -	 * and do not drop the Preamble.
> -	 */
> -	writel((((imx_get_ahbclk() / 1000000) + 2) / 5) << 1,
> -			&fec->eth->mii_speed);
> -	debug("fec_init: mii_speed %#lx\n",
> -			(((imx_get_ahbclk() / 1000000) + 2) / 5) << 1);
> +	fec_mii_setspeed(fec);
>  
>  	sprintf(edev->name, "FEC_MXC");
>  
> @@ -717,6 +729,7 @@ static int fec_probe(bd_t *bd)
>  
>  	eth_register(edev);
>  
> +	tmp = getenv("ethaddr");
>  	if ((NULL != tmp) && (12 <= strlen(tmp))) {
>  		int i;
>  		/* convert MAC from string to int */

This is wrong and should be fixed. We don't convert to "int" but to
"uchar[]"; While we touch this, please dump the code completely and
use eth_getenv_enetaddr() instead.

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
Once upon a time, "PC" meant "personal computer".  Now  it  seems  to
only  mean  "Prisoner  of Bill". Alas! All my PCs run Unix, and those
include Intel, Sparc, and other processors.
          -- Tom "Bring back the non-PC meaning of `PC'" Christiansen