Dear Stefano Babic,
In message <1263212760-27272-7-git-send-email-sba...@denx.de> you wrote:
> The patch add support for the Freescale mx51 processor
> to the FEC ethernet driver.
>
> Signed-off-by: Stefano Babic <sba...@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: w...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
