[U-Boot] [PATCH 0/2] phylib: a couple of minor fixes in generic phy library

[U-Boot] [PATCH 0/2] phylib: a couple of minor fixes in generic phy library

[Vladimir Zapolskiy]

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(-)

-- 
1.7.5.4

[U-Boot] [PATCH 1/2] phylib: reset mii bus only if reset handler is registered

[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 <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);
-- 
1.7.5.4

[U-Boot] [PATCH 2/2] phylib: remove a couple of redundant code lines

[Vladimir Zapolskiy]

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);
-- 
1.7.5.4

[U-Boot] [PATCH 2/2] phylib: remove a couple of redundant code lines

[Detlev Zundel]

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

-- 
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

[U-Boot] [PATCH 1/2] phylib: reset mii bus only if reset handler is registered

[Detlev Zundel]

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

-- 
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

[U-Boot] [PATCH 1/2] phylib: reset mii bus only if reset handler is registered

[Wolfgang Denk]

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

-- 
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?

[U-Boot] [PATCH 2/2] phylib: remove a couple of redundant code lines

[Wolfgang Denk]

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

-- 
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.

[U-Boot] [PATCH 2/2] phylib: remove a couple of redundant code lines

[Andy Fleming]

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)

[U-Boot] [PATCH 1/2] phylib: reset mii bus only if reset handler is registered

[Andy Fleming]

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

[U-Boot] [PATCH 1/2] phylib: reset mii bus only if reset handler is registered

[Mike Frysinger]

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
-------------- 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 

[U-Boot] [PATCH 1/2] phylib: reset mii bus only if reset handler is registered

[Andy Fleming]

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

[U-Boot] [PATCH 2/2] phylib: remove a couple of redundant code lines

[Wolfgang Denk]

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

-- 
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.

[U-Boot] [PATCH 2/2] phylib: remove a couple of redundant code lines

[Vladimir Zapolskiy]

Hello Wolfgang,

On 23.09.2011 09:06, Wolfgang Denk wrote:
> 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?
>
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.

-- 
With best wishes,
Vladimir

[U-Boot] [PATCH 2/2] phylib: remove a couple of redundant code lines

[Wolfgang Denk]

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

-- 
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