[U-Boot] net, phy: wrong 1000BASE detection with a lan9303 switch

[U-Boot] net, phy: wrong 1000BASE detection with a lan9303 switch

[Heiko Schocher]

Hello,

I have problems with a lan9303 switch on an am335x based board,
which does not support 1000FD/HD modes, but as it set BMSR_ERCAP
bit in BMSR and returns 0xffff for reads on the MII_STAT1000 and
MII_CTRL1000 registers, u-boot code detects SPEED_1000 in
drivers/net/phy/phy.c genphy_parse_link() ... which is wrong,
as this switch does not support 1000 modes ...

I found in ./common/miiphyutil.c miiphy_speed() the define
CONFIG_PHY_GIGE which it seems lacks in drivers/net/phy/phy.c
genphy_parse_link() ? Or is there another option?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] net, phy: wrong 1000BASE detection with a lan9303 switch

[Heiko Schocher]

Hello,

Am 10.07.2013 12:09, schrieb Heiko Schocher:
> Hello,
>
> I have problems with a lan9303 switch on an am335x based board,
> which does not support 1000FD/HD modes, but as it set BMSR_ERCAP
> bit in BMSR and returns 0xffff for reads on the MII_STAT1000 and
> MII_CTRL1000 registers, u-boot code detects SPEED_1000 in
> drivers/net/phy/phy.c genphy_parse_link() ... which is wrong,
> as this switch does not support 1000 modes ...
>
> I found in ./common/miiphyutil.c miiphy_speed() the define
> CONFIG_PHY_GIGE which it seems lacks in drivers/net/phy/phy.c
> genphy_parse_link() ? Or is there another option?

Just an update ... the switch does not support reads for the
registers 0x09 and 0x0a, but sets the BMSR_ERCAP bit in BMSR.

A phy_read on register  0x09 or 0x0a fails ... In my case
the drivers/net/cpsw.c returns -1 ... and code in
drivers/net/phy/phy.c:

int genphy_parse_link(struct phy_device *phydev)
{
	int mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMSR);

         /* We're using autonegotiation */
         if (mii_reg & BMSR_ANEGCAPABLE) {
                 u32 lpa = 0;
                 u32 gblpa = 0;
                 u32 estatus = 0;

                 /* Check for gigabit capability */
                 if (mii_reg & BMSR_ERCAP) {
                         /* We want a list of states supported by
                          * both PHYs in the link
                          */
                         gblpa = phy_read(phydev, MDIO_DEVAD_NONE, MII_STAT1000);
                         gblpa &= phy_read(phydev,
                                         MDIO_DEVAD_NONE, MII_CTRL1000) << 2;
                 }

[...]

does not check, if phy_read returns with an error ...

Would this be an acceptable patch for it:

diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c
index 379b679..52c08ed 100644
--- a/drivers/net/cpsw.c
+++ b/drivers/net/cpsw.c
@@ -489,7 +489,7 @@ static inline void wait_for_idle(void)
  static int cpsw_mdio_read(struct mii_dev *bus, int phy_id,
                                 int dev_addr, int phy_reg)
  {
-       unsigned short data;
+       int data;
         u32 reg;

         if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7c0eaec..4522382 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -291,7 +291,7 @@ int genphy_parse_link(struct phy_device *phydev)
         /* We're using autonegotiation */
         if (mii_reg & BMSR_ANEGCAPABLE) {
                 u32 lpa = 0;
-               u32 gblpa = 0;
+               int gblpa = 0;
                 u32 estatus = 0;

                 /* Check for gigabit capability */
@@ -300,6 +300,10 @@ int genphy_parse_link(struct phy_device *phydev)
                          * both PHYs in the link
                          */
                         gblpa = phy_read(phydev, MDIO_DEVAD_NONE, MII_STAT1000);
+                       if (gblpa < 0) {
+                               printf ("Could not read MII_STAT1000. Ignoring gigabit capability\n");
+                               gblpa = 0;
+                       }
                         gblpa &= phy_read(phydev,
                                         MDIO_DEVAD_NONE, MII_CTRL1000) << 2;
                 }
bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] net, phy: wrong 1000BASE detection with a lan9303 switch

[Wolfgang Denk]

Dear Heiko,

In message <51DD3D92.8050701@denx.de> you wrote:
> 
> Would this be an acceptable patch for it:
...
> @@ -300,6 +300,10 @@ int genphy_parse_link(struct phy_device *phydev)
>                           * both PHYs in the link
>                           */
>                          gblpa = phy_read(phydev, MDIO_DEVAD_NONE, MII_STAT1000);
> +                       if (gblpa < 0) {
> +                               printf ("Could not read MII_STAT1000. Ignoring gigabit capability\n");
> +                               gblpa = 0;
> +                       }
>                          gblpa &= phy_read(phydev,
>                                          MDIO_DEVAD_NONE, MII_CTRL1000) << 2;

Well, this other phy_read() probably needs exactly the same error
handling - and I doubt if we should actually try riding the reg when
the first one failed already?

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@denx.de
Without freedom of choice there is no creativity.
	-- Kirk, "The return of the Archons", stardate 3157.4

[U-Boot] net, phy: wrong 1000BASE detection with a lan9303 switch

[Heiko Schocher]

Hello Wolfgang,

Am 10.07.2013 13:15, schrieb Wolfgang Denk:
> Dear Heiko,
>
> In message<51DD3D92.8050701@denx.de>  you wrote:
>>
>> Would this be an acceptable patch for it:
> ...
>> @@ -300,6 +300,10 @@ int genphy_parse_link(struct phy_device *phydev)
>>                            * both PHYs in the link
>>                            */
>>                           gblpa = phy_read(phydev, MDIO_DEVAD_NONE, MII_STAT1000);
>> +                       if (gblpa<  0) {
>> +                               printf ("Could not read MII_STAT1000. Ignoring gigabit capability\n");
>> +                               gblpa = 0;
>> +                       }
>>                           gblpa&= phy_read(phydev,
>>                                           MDIO_DEVAD_NONE, MII_CTRL1000)<<  2;
>
> Well, this other phy_read() probably needs exactly the same error
> handling - and I doubt if we should actually try riding the reg when
> the first one failed already?

Yes, you are right. The first question is, if such a patch is the
right way for fixing it, or if there is a better way ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany