Patchwork [U-Boot,1/2] net: fix some bugs in LL TEMAC driver

login
register
mail settings
Submitter Stephan Linz
Date Nov. 19, 2010, 6:54 p.m.
Message ID <66901067543efe59d87b3a805a4b19ce52a780c5.1290192590.git.linz@li-pro.net>
Download mbox | patch
Permalink /patch/72292/
State Changes Requested
Headers show

Comments

Stephan Linz - Nov. 19, 2010, 6:54 p.m.
* avoid using link variable uninitialized
  * avoid using phy_addr variable with invalid value
  * reorganize phy control: first looking for phy than link
  * return with error (result value -1) if no phy/link was found
  * fix boolean mistake in wait for link: wait as long as we got
    phy register 1 has no link indication (BMSR != 0x24)
  * expand the 'first run' flag handling in ll_temac_init() in
    respect to possible error detection in xps_ll_temac_phy_ctrl()

Signed-off-by: Stephan Linz <linz@li-pro.net>
---
 drivers/net/xilinx_ll_temac.c |   44 ++++++++++++++++++++++++++++++----------
 1 files changed, 33 insertions(+), 11 deletions(-)
Michal Simek - Nov. 20, 2010, 9:27 a.m.
2010/11/19 Stephan Linz <linz@li-pro.net>

>  * avoid using link variable uninitialized
>  * avoid using phy_addr variable with invalid value
>  * reorganize phy control: first looking for phy than link
>  * return with error (result value -1) if no phy/link was found
>  * fix boolean mistake in wait for link: wait as long as we got
>    phy register 1 has no link indication (BMSR != 0x24)
>  * expand the 'first run' flag handling in ll_temac_init() in
>    respect to possible error detection in xps_ll_temac_phy_ctrl()
>


I will test your changes on real hw.

Some comments below.

Thanks,
Michal



>
> Signed-off-by: Stephan Linz <linz@li-pro.net>
> ---
>  drivers/net/xilinx_ll_temac.c |   44
> ++++++++++++++++++++++++++++++----------
>  1 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/xilinx_ll_temac.c b/drivers/net/xilinx_ll_temac.c
> index 72a1632..a72e072 100644
> --- a/drivers/net/xilinx_ll_temac.c
> +++ b/drivers/net/xilinx_ll_temac.c
> @@ -225,24 +225,21 @@ static void read_phy_reg (struct eth_device *dev, int
> phy_addr)
>  #endif
>
>  static int phy_addr = -1;
> -static int link;
> +static int link = 0;
>
>  /* setting ll_temac and phy to proper setting */
>  static int xps_ll_temac_phy_ctrl(struct eth_device *dev)
>  {
> -       int i;
> +       int i, retries;
>        unsigned int result;
> -       unsigned retries = 10;
>
> +        /* link is setup */
>        if (link == 1)
> -               return 1; /* link is setup */
> -
> -       /* wait for link up */
> -       while (retries-- &&
> -               ((xps_ll_temac_hostif_get(dev, 0, phy_addr, 1) & 0x24) ==
> 0x24))
> -               ;
> +               return 1;
>
> +       /* try out if have ever found the right phy? */
>        if (phy_addr == -1) {
> +               printf("Looking for phy ... ");
>

use puts instead.



>                for (i = 31; i >= 0; i--) {
>                        result = xps_ll_temac_hostif_get(dev, 0, i, 1);
>                        if ((result & 0x0ffff) != 0x0ffff) {
> @@ -251,7 +248,27 @@ static int xps_ll_temac_phy_ctrl(struct eth_device
> *dev)
>                                break;
>                        }
>                }
> +
> +               /* no success? -- wery bad */
> +               if (phy_addr == -1) {
> +                       printf("ERROR\n");
>

same here


> +                       return -1;
> +               }
> +               printf("OK\n");
> +       }
> +
> +       /* wait for link up */
> +       printf("Waiting for link ... ");
>

and here.


> +       retries = 10;
> +       while (retries-- &&
> +               ((xps_ll_temac_hostif_get(dev, 0, phy_addr, 1) & 0x24) !=
> 0x24))
> +               ;
> +
> +       if (retries < 0) {
> +               printf("ERROR\n");
>

and here


> +               return 0;
>        }
> +       printf("OK\n");
>

and here.


>
>        /* get PHY id */
>        i = (xps_ll_temac_hostif_get(dev, 0, phy_addr, 2) << 16) | \
> @@ -491,7 +508,6 @@ static int ll_temac_init(struct eth_device *dev, bd_t
> *bis)
>  #endif
>        if (!first)
>                return 0;
> -       first = 0;
>
>        xps_ll_temac_init(dev, bis);
>
> @@ -502,7 +518,13 @@ static int ll_temac_init(struct eth_device *dev, bd_t
> *bis)
>        for (i = 0; i < 32; i++)
>                read_phy_reg(dev, i);
>  #endif
> -       xps_ll_temac_phy_ctrl(dev);
> +
> +       if (xps_ll_temac_phy_ctrl(dev) == 0) {
> +               xps_ll_temac_halt(dev);
> +               return -1;
> +       }
> +
> +       first = 0;
>        return 1;
>  }
>
> --
> 1.6.0.4
>
>
Stephan Linz - Nov. 20, 2010, 6:48 p.m.
Am Samstag, 20. November 2010, um 10:27:45 schrieb Michal Simek:
> 2010/11/19 Stephan Linz <linz@li-pro.net>
>
> >  * avoid using link variable uninitialized
> >  * avoid using phy_addr variable with invalid value
> >  * reorganize phy control: first looking for phy than link
> >  * return with error (result value -1) if no phy/link was found
> >  * fix boolean mistake in wait for link: wait as long as we got
> >    phy register 1 has no link indication (BMSR != 0x24)
> >  * expand the 'first run' flag handling in ll_temac_init() in
> >    respect to possible error detection in xps_ll_temac_phy_ctrl()
>
> I will test your changes on real hw.
>
> Some comments below.

I do it so on Monday ... printf() --> puts()

Regards,
Stephan

>
> Thanks,
> Michal
>
> > Signed-off-by: Stephan Linz <linz@li-pro.net>
> > ---
> >  drivers/net/xilinx_ll_temac.c |   44
> > ++++++++++++++++++++++++++++++----------
> >  1 files changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/xilinx_ll_temac.c
> > b/drivers/net/xilinx_ll_temac.c index 72a1632..a72e072 100644
> > --- a/drivers/net/xilinx_ll_temac.c
> > +++ b/drivers/net/xilinx_ll_temac.c
> > @@ -225,24 +225,21 @@ static void read_phy_reg (struct eth_device *dev,
> > int phy_addr)
> >  #endif
> >
> >  static int phy_addr = -1;
> > -static int link;
> > +static int link = 0;
> >
> >  /* setting ll_temac and phy to proper setting */
> >  static int xps_ll_temac_phy_ctrl(struct eth_device *dev)
> >  {
> > -       int i;
> > +       int i, retries;
> >        unsigned int result;
> > -       unsigned retries = 10;
> >
> > +        /* link is setup */
> >        if (link == 1)
> > -               return 1; /* link is setup */
> > -
> > -       /* wait for link up */
> > -       while (retries-- &&
> > -               ((xps_ll_temac_hostif_get(dev, 0, phy_addr, 1) & 0x24) ==
> > 0x24))
> > -               ;
> > +               return 1;
> >
> > +       /* try out if have ever found the right phy? */
> >        if (phy_addr == -1) {
> > +               printf("Looking for phy ... ");
>
> use puts instead.
>
> >                for (i = 31; i >= 0; i--) {
> >                        result = xps_ll_temac_hostif_get(dev, 0, i, 1);
> >                        if ((result & 0x0ffff) != 0x0ffff) {
> > @@ -251,7 +248,27 @@ static int xps_ll_temac_phy_ctrl(struct eth_device
> > *dev)
> >                                break;
> >                        }
> >                }
> > +
> > +               /* no success? -- wery bad */
> > +               if (phy_addr == -1) {
> > +                       printf("ERROR\n");
>
> same here
>
> > +                       return -1;
> > +               }
> > +               printf("OK\n");
> > +       }
> > +
> > +       /* wait for link up */
> > +       printf("Waiting for link ... ");
>
> and here.
>
> > +       retries = 10;
> > +       while (retries-- &&
> > +               ((xps_ll_temac_hostif_get(dev, 0, phy_addr, 1) & 0x24) !=
> > 0x24))
> > +               ;
> > +
> > +       if (retries < 0) {
> > +               printf("ERROR\n");
>
> and here
>
> > +               return 0;
> >        }
> > +       printf("OK\n");
>
> and here.
>
> >        /* get PHY id */
> >        i = (xps_ll_temac_hostif_get(dev, 0, phy_addr, 2) << 16) | \
> > @@ -491,7 +508,6 @@ static int ll_temac_init(struct eth_device *dev, bd_t
> > *bis)
> >  #endif
> >        if (!first)
> >                return 0;
> > -       first = 0;
> >
> >        xps_ll_temac_init(dev, bis);
> >
> > @@ -502,7 +518,13 @@ static int ll_temac_init(struct eth_device *dev,
> > bd_t *bis)
> >        for (i = 0; i < 32; i++)
> >                read_phy_reg(dev, i);
> >  #endif
> > -       xps_ll_temac_phy_ctrl(dev);
> > +
> > +       if (xps_ll_temac_phy_ctrl(dev) == 0) {
> > +               xps_ll_temac_halt(dev);
> > +               return -1;
> > +       }
> > +
> > +       first = 0;
> >        return 1;
> >  }
> >
> > --
> > 1.6.0.4
Stephan Linz - Nov. 22, 2010, 9:46 a.m.
Hi Michal,

as announced here is the correction for the LL TEMAC bugfixing. I've changed
all printf() to puts(). Moreover I've corrected the wrong result code (-1)
if we can't found a real phy -- my mistake ;-).

Please use the new patch for your tests on real hw.


Best regards,
Stephan Linz

Patch

diff --git a/drivers/net/xilinx_ll_temac.c b/drivers/net/xilinx_ll_temac.c
index 72a1632..a72e072 100644
--- a/drivers/net/xilinx_ll_temac.c
+++ b/drivers/net/xilinx_ll_temac.c
@@ -225,24 +225,21 @@  static void read_phy_reg (struct eth_device *dev, int phy_addr)
 #endif
 
 static int phy_addr = -1;
-static int link;
+static int link = 0;
 
 /* setting ll_temac and phy to proper setting */
 static int xps_ll_temac_phy_ctrl(struct eth_device *dev)
 {
-	int i;
+	int i, retries;
 	unsigned int result;
-	unsigned retries = 10;
 
+	 /* link is setup */
 	if (link == 1)
-		return 1; /* link is setup */
-
-	/* wait for link up */
-	while (retries-- &&
-		((xps_ll_temac_hostif_get(dev, 0, phy_addr, 1) & 0x24) == 0x24))
-		;
+		return 1;
 
+	/* try out if have ever found the right phy? */
 	if (phy_addr == -1) {
+		printf("Looking for phy ... ");
 		for (i = 31; i >= 0; i--) {
 			result = xps_ll_temac_hostif_get(dev, 0, i, 1);
 			if ((result & 0x0ffff) != 0x0ffff) {
@@ -251,7 +248,27 @@  static int xps_ll_temac_phy_ctrl(struct eth_device *dev)
 				break;
 			}
 		}
+
+		/* no success? -- wery bad */
+		if (phy_addr == -1) {
+			printf("ERROR\n");
+			return -1;
+		}
+		printf("OK\n");
+	}
+
+	/* wait for link up */
+	printf("Waiting for link ... ");
+	retries = 10;
+	while (retries-- &&
+		((xps_ll_temac_hostif_get(dev, 0, phy_addr, 1) & 0x24) != 0x24))
+		;
+
+	if (retries < 0) {
+		printf("ERROR\n");
+		return 0;
 	}
+	printf("OK\n");
 
 	/* get PHY id */
 	i = (xps_ll_temac_hostif_get(dev, 0, phy_addr, 2) << 16) | \
@@ -491,7 +508,6 @@  static int ll_temac_init(struct eth_device *dev, bd_t *bis)
 #endif
 	if (!first)
 		return 0;
-	first = 0;
 
 	xps_ll_temac_init(dev, bis);
 
@@ -502,7 +518,13 @@  static int ll_temac_init(struct eth_device *dev, bd_t *bis)
 	for (i = 0; i < 32; i++)
 		read_phy_reg(dev, i);
 #endif
-	xps_ll_temac_phy_ctrl(dev);
+
+	if (xps_ll_temac_phy_ctrl(dev) == 0) {
+		xps_ll_temac_halt(dev);
+		return -1;
+	}
+
+	first = 0;
 	return 1;
 }