Skip to contentSkip to conversation

phylib: a couple of minor fixes in generic phy library

PATCH New Vladimir Zapolskiy 5 participants 14 messages Sep 5, 2011 – Sep 28, 2011

Subsystems (2)
Network drivers Ethernet PHY
lore
Share & tools
lore

Vladimir Zapolskiy

author
#0 lore raw

cover phylib: a couple of minor fixes in generic phy library

Hello,

recently I decided to port one (not integrated to U-boot yet) MAC to use phylib, and found that it serves quite good. I managed to find only one bug related to miiphy_register() and phylib cooperative usage, the small fix is provided. Additionally let me send a tiny readability improvement change.

Vladimir Zapolskiy (2):
phylib: reset mii bus only if reset handler is registered
phylib: remove a couple of redundant code lines

 drivers/net/phy/phy.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
Signature · 1 line
1.7.5.4
author
#1 lore raw

PATCH 1/2 phylib: reset mii bus only if reset handler is registered

This change allows to cope with a mii bus device registered using miiphy_register(), which doesn't assign a default reset handler.

Signed-off-by Vladimir Zapolskiy <[email protected]>
---
 drivers/net/phy/phy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/net/phy/phy.c +2 -1
index ce69c19..8da7688 100644
@@ -692,7 +692,8 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr,
 	struct phy_device *phydev;
 
 	/* Reset the bus */
-	bus->reset(bus);
+	if (bus->reset)
+		bus->reset(bus);
 
 	/* Wait 15ms to make sure the PHY has come out of hard reset */
 	udelay(15000);
Signature · 1 line
1.7.5.4
#2 lore raw

PATCH 1/2 phylib: reset mii bus only if reset handler is registered

Hi Vladimir,

Quoted 2 lines

This change allows to cope with a mii bus device registered using miiphy_register(), which doesn't assign a default reset handler.

Looks good, so

Acked-by Detlev Zundel <[email protected]>
Quoted 1 line
Signed-off-by Vladimir Zapolskiy <[email protected]>

Cheers
Detlev

Signature · 8 lines
But in terms of creative  information, information that people can use
or enjoy, and that will be  used and enjoyed more  the more people who
have it, always we should encourage the copying.
	                                    -- Richard M. Stallman
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
Applied
#3 lore raw

PATCH 1/2 phylib: reset mii bus only if reset handler is registered

In reply to Vladimir Zapolskiy

Dear Vladimir Zapolskiy,

Quoted 7 lines · In message <[email protected]> you wrote:

This change allows to cope with a mii bus device registered using miiphy_register(), which doesn't assign a default reset handler.

Signed-off-by Vladimir Zapolskiy <[email protected]>
---
 drivers/net/phy/phy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

Signature · 5 lines
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 at denx.de
If a train station is a place where a train stops,
                                           then what's a workstation?
#4 lore raw

PATCH 1/2 phylib: reset mii bus only if reset handler is registered

In reply to Vladimir Zapolskiy
Quoted 15 lines · Vladimir Zapolskiy

This change allows to cope with a mii bus device registered using miiphy_register(), which doesn't assign a default reset handler.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
?drivers/net/phy/phy.c | ? ?3 ++-
?1 files changed, 2 insertions(+), 1 deletions(-)

drivers/net/phy/phy.c +0 -0
index ce69c19..8da7688 100644
@@ -692,7 +692,8 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr,
 ? ? ? ?struct phy_device *phydev;
Quoted 4 lines

? ? ? ?/* Reset the bus */
- ? ? ? bus->reset(bus);
+ ? ? ? if (bus->reset)
+ ? ? ? ? ? ? ? bus->reset(bus);

The change is a good idea, but I find the motivation for it strange. If you register a bus with miiphy_register, you are declaring your intent to use the legacy PHY interface. But phy_connect() is part of the new phylib API. It was not intended that combining the two work at all. Looking at the code, I see no reason it wouldn't work, but I question why you would do that, instead of creating a proper MDIO driver?

Andy

#5 lore raw

PATCH 1/2 phylib: reset mii bus only if reset handler is registered

Quoted 9 lines · On Thursday, September 22, 2011 17:44:11 Andy Fleming wrote:
Quoted 8 lines · Vladimir Zapolskiy

This change allows to cope with a mii bus device registered using miiphy_register(), which doesn't assign a default reset handler.

drivers/net/phy/phy.c +0 -0
@@ -692,7 +692,8 @@ struct phy_device *phy_connect(struct mii_dev *bus,
 int addr, struct phy_device *phydev;
Quoted 12 lines

drivers/net/phy/phy.c struct phy_device *phy_connect(struct mii_dev *bus, int addr, · line 696

Quoted 4 lines
drivers/net/phy/phy.c +2 -1
       /* Reset the bus */
-       bus->reset(bus);
+       if (bus->reset)
+               bus->reset(bus);

The change is a good idea, but I find the motivation for it strange. If you register a bus with miiphy_register, you are declaring your intent to use the legacy PHY interface. But phy_connect() is part of the new phylib API. It was not intended that combining the two work at all. Looking at the code, I see no reason it wouldn't work, but I question why you would do that, instead of creating a proper MDIO driver?

hmm, is there an #ifdef check we could add that would cause an #error if people mix the old and the new ? i think it makes sense to just force everyone to migrate to the new ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110922/7f609d17/attachment.pgp

#6 lore raw

PATCH 1/2 phylib: reset mii bus only if reset handler is registered

Quoted 15 lines · Mike Frysinger
Quoted 6 lines · On Thursday, September 22, 2011 17:44:11 Andy Fleming wrote:
Quoted 4 lines · Vladimir Zapolskiy

? ? ? ?/* Reset the bus */
- ? ? ? bus->reset(bus);
+ ? ? ? if (bus->reset)
+ ? ? ? ? ? ? ? bus->reset(bus);

Quoted 7 lines

The change is a good idea, but I find the motivation for it strange. If you register a bus with miiphy_register, you are declaring your intent to use the legacy PHY interface. But phy_connect() is part of the new phylib API. It was not intended that combining the two work at all. Looking at the code, I see no reason it wouldn't work, but I question why you would do that, instead of creating a proper MDIO driver?

Quoted 3 lines

hmm, is there an #ifdef check we could add that would cause an #error if people mix the old and the new ? ?i think it makes sense to just force everyone to migrate to the new ...

I suppose we could add a check in phy_connect to see whether the read function is actually legacy_miiphy_read. If you don't call phy_connect(), then you don't get a handle to the PHY device, and aren't using phylib.

I don't think we're ready to disallow using the two concurrently in separate drivers.

Andy

author
#7 lore raw

PATCH 2/2 phylib: remove a couple of redundant code lines

In reply to Vladimir Zapolskiy

This change slightly improves readability of the phydev speed/duplex assignment logic.

Signed-off-by Vladimir Zapolskiy <[email protected]>
---
 drivers/net/phy/phy.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)
drivers/net/phy/phy.c +2 -5
index 8da7688..833a051 100644
@@ -318,13 +318,10 @@ static int genphy_parse_link(struct phy_device *phydev)
 		lpa = phy_read(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE);
 		lpa &= phy_read(phydev, MDIO_DEVAD_NONE, MII_LPA);
 
-		if (lpa & (LPA_100FULL | LPA_100HALF)) {
+		if (lpa & (LPA_100FULL | LPA_100HALF))
 			phydev->speed = SPEED_100;
 
-			if (lpa & LPA_100FULL)
-				phydev->duplex = DUPLEX_FULL;
-
-		} else if (lpa & LPA_10FULL)
+		if (lpa & (LPA_100FULL | LPA_10FULL))
 			phydev->duplex = DUPLEX_FULL;
 	} else {
 		u32 bmcr = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
Signature · 1 line
1.7.5.4
#8 lore raw

PATCH 2/2 phylib: remove a couple of redundant code lines

Hi Vladimir,

Quoted 2 lines

This change slightly improves readability of the phydev speed/duplex assignment logic.

Agreed.

Quoted 1 line
Signed-off-by Vladimir Zapolskiy <[email protected]>
Acked-by Detlev Zundel <[email protected]>

Cheers
Detlev

Signature · 5 lines
Every time history repeats itself the price goes up.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
Applied
#9 lore raw

PATCH 2/2 phylib: remove a couple of redundant code lines

In reply to Vladimir Zapolskiy

Dear Vladimir Zapolskiy,

Quoted 7 lines · In message <[email protected]> you wrote:

This change slightly improves readability of the phydev speed/duplex assignment logic.

Signed-off-by Vladimir Zapolskiy <[email protected]>
---
 drivers/net/phy/phy.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

Signature · 5 lines
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 at denx.de
Philosophy is a game with objectives and no rules.
Mathematics is a game with rules and no objectives.
#10 lore raw

PATCH 2/2 phylib: remove a couple of redundant code lines

In reply to Vladimir Zapolskiy
Quoted 2 lines · Vladimir Zapolskiy

This change slightly improves readability of the phydev speed/duplex assignment logic.

Shoot, I just saw this patch in my tree. It's incorrect.

Quoted 4 lines
(quoted diff) +0 -0
@@ -318,13 +318,10 @@ static int genphy_parse_link(struct phy_device *phydev)
 ? ? ? ? ? ? ? ?lpa = phy_read(phydev, MDIO_DEVAD_NONE, MII_ADVERTISE);
 ? ? ? ? ? ? ? ?lpa &= phy_read(phydev, MDIO_DEVAD_NONE, MII_LPA);

Review static int genphy_parse_link(struct phy_device *phydev) · line 325

Quoted 10 lines
(quoted diff) +2 -5
- ? ? ? ? ? ? ? if (lpa & (LPA_100FULL | LPA_100HALF)) {
+ ? ? ? ? ? ? ? if (lpa & (LPA_100FULL | LPA_100HALF))
 ? ? ? ? ? ? ? ? ? ? ? ?phydev->speed = SPEED_100;
 
- ? ? ? ? ? ? ? ? ? ? ? if (lpa & LPA_100FULL)
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? phydev->duplex = DUPLEX_FULL;
-
- ? ? ? ? ? ? ? } else if (lpa & LPA_10FULL)
+ ? ? ? ? ? ? ? if (lpa & (LPA_100FULL | LPA_10FULL))
 ? ? ? ? ? ? ? ? ? ? ? ?phydev->duplex = DUPLEX_FULL;

The lines weren't redundant. The logic is (and probably should be better commented):

Find the intersection of the advertised capabilities of both sides of the link (lpa)

#11 lore raw

PATCH 2/2 phylib: remove a couple of redundant code lines

Dear Andy Fleming,

Quoted 2 lines · In message <CAKWjMd5HGT9df76vPFs8B5sFQYWoAN1bGmt2vRihN0cTa1boug@mail.gmail.com>…

Shoot, I just saw this patch in my tree. It's incorrect.

Argh...

Quoted 8 lines

The lines weren't redundant. The logic is (and probably should be better commented):

Find the intersection of the advertised capabilities of both sides of the link (lpa)
From that intersection, find the highest capability we can run at (that will be the negotiated link)

Quoted 4 lines

Now imagine that the intersection (lpa) is (LPA_100HALF | LPA_10FULL).

The code will now set phydev->speed to 100, and phydev->duplex to 1, but this link does not support 100FULL.

Do we agree that I should revert this commit?

Best regards,

Wolfgang Denk

Signature · 6 lines
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 at denx.de
Felson's Law:
	To steal ideas from one person is plagiarism; to steal from
	many is research.
author
#12 lore raw

PATCH 2/2 phylib: remove a couple of redundant code lines

Hello Wolfgang,

Quoted 16 lines · Wolfgang Denk

Dear Andy Fleming,

Quoted 2 lines · In message<CAKWjMd5HGT9df76vPFs8B5sFQYWoAN1bGmt2vRihN0cTa1boug@mail.gmail.com>…

Shoot, I just saw this patch in my tree. It's incorrect.

Argh...

Quoted 8 lines

The lines weren't redundant. The logic is (and probably should be better commented):

Find the intersection of the advertised capabilities of both sides of the link (lpa)
From that intersection, find the highest capability we can run at
(that will be the negotiated link)

Quoted 7 lines
Quoted 4 lines

Now imagine that the intersection (lpa) is (LPA_100HALF | LPA_10FULL).

The code will now set phydev->speed to 100, and phydev->duplex to 1, but this link does not support 100FULL.

Do we agree that I should revert this commit?

due to Andy's explanation I agree that this change is fishy and it should be reverted.

As a U-Boot user I'd greatly appreciate, if you add the above note to the commit message.

Sorry for inconvenience.

Signature · 2 lines
With best wishes,
Vladimir
#13 lore raw

PATCH 2/2 phylib: remove a couple of redundant code lines

In reply to Vladimir Zapolskiy

Dear Vladimir Zapolskiy,

Quoted 7 lines · In message <[email protected]> you wrote:

This change slightly improves readability of the phydev speed/duplex assignment logic.

Signed-off-by Vladimir Zapolskiy <[email protected]>
---
 drivers/net/phy/phy.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

Patch reverted.

Best regards,

Wolfgang Denk

Signature · 4 lines
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 at denx.de
"It is easier to port a shell than a shell script."      - Larry Wall