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