[U-Boot] [PATCH 0/2] 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(-)
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 vz@mleia.com --- drivers/net/phy/phy.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index ce69c19..8da7688 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -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);
Hi Vladimir,
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 dzu@denx.de
Signed-off-by: Vladimir Zapolskiy vz@mleia.com
Cheers Detlev
Dear Vladimir Zapolskiy,
In message 1315243448-29959-2-git-send-email-vz@mleia.com 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 vz@mleia.com
drivers/net/phy/phy.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
On Mon, Sep 5, 2011 at 12:24 PM, Vladimir Zapolskiy vz@mleia.com 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 vz@mleia.com
drivers/net/phy/phy.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index ce69c19..8da7688 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -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);
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
On Thursday, September 22, 2011 17:44:11 Andy Fleming wrote:
On Mon, Sep 5, 2011 at 12:24 PM, Vladimir Zapolskiy vz@mleia.com wrote:
This change allows to cope with a mii bus device registered using miiphy_register(), which doesn't assign a default reset handler.
--- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -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);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
On Thu, Sep 22, 2011 at 5:13 PM, Mike Frysinger vapier@gentoo.org wrote:
On Thursday, September 22, 2011 17:44:11 Andy Fleming wrote:
On Mon, Sep 5, 2011 at 12:24 PM, Vladimir Zapolskiy vz@mleia.com wrote:
/* 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 ...
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
This change slightly improves readability of the phydev speed/duplex assignment logic.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com --- drivers/net/phy/phy.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 8da7688..833a051 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -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);
Hi Vladimir,
This change slightly improves readability of the phydev speed/duplex assignment logic.
Agreed.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com
Acked-by: Detlev Zundel dzu@denx.de
Cheers Detlev
Dear Vladimir Zapolskiy,
In message 1315243448-29959-3-git-send-email-vz@mleia.com you wrote:
This change slightly improves readability of the phydev speed/duplex assignment logic.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com
drivers/net/phy/phy.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
On Mon, Sep 5, 2011 at 12:24 PM, Vladimir Zapolskiy vz@mleia.com wrote:
This change slightly improves readability of the phydev speed/duplex assignment logic.
Shoot, I just saw this patch in my tree. It's incorrect.
@@ -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;
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)
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.
Andy
Dear Andy Fleming,
In message CAKWjMd5HGT9df76vPFs8B5sFQYWoAN1bGmt2vRihN0cTa1boug@mail.gmail.com you wrote:
Shoot, I just saw this patch in my tree. It's incorrect.
Argh...
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)
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
Hello Wolfgang,
On 23.09.2011 09:06, Wolfgang Denk wrote:
Dear Andy Fleming,
In messageCAKWjMd5HGT9df76vPFs8B5sFQYWoAN1bGmt2vRihN0cTa1boug@mail.gmail.com you wrote:
Shoot, I just saw this patch in my tree. It's incorrect.
Argh...
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)
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.
Dear Vladimir Zapolskiy,
In message 1315243448-29959-3-git-send-email-vz@mleia.com you wrote:
This change slightly improves readability of the phydev speed/duplex assignment logic.
Signed-off-by: Vladimir Zapolskiy vz@mleia.com
drivers/net/phy/phy.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
Patch reverted.
Best regards,
Wolfgang Denk
participants (5)
-
Andy Fleming -
Detlev Zundel -
Mike Frysinger -
Vladimir Zapolskiy -
Wolfgang Denk