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

[Stefano Babic]

Wolfgang Denk wrote:
> Dear Stefano Babic,
> 

Hi Wolfgang,

>> -#include <asm/arch/clock.h>
>>  #include <asm/arch/imx-regs.h>
>> +#ifndef CONFIG_MX51
>> +#include <asm/arch/clock.h>imx_get_ahbclk
>> +#endif
> 
> Can we not implement the clock stuff for the iMX51 in such a way that
> we don't need #ifdef's here? 

Good hint, I do it !

> 
>> @@ -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?

You are right, there is no technical reason. The only thing here is to
get the correct clock source. I will get as in serial_mxc and setting a
imx_get_fecclk() for both processors (i.MX27 and i.MX51), removing the
ifdef.

>>  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.

Understood, thanks.

>> +#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 ?

I think this should be dropped from the driver. The driver should be
responsible to set up the FEC controller and nothing else. Enabling the
clock should be done in another place (probably in the cpu related part
?), but not here. However, this is related to the i.MX27, I am not sure
where we have to move this code.

>> +	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.

Thanks, understood.

Regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================