diff mbox

[U-Boot,4/5] net: fec_mxc: Nove autonegoatiate restart after mii_postcall

Message ID 1326456604-25137-4-git-send-email-dirk.behme@de.bosch.com
State Changes Requested
Headers show

Commit Message

Dirk Behme Jan. 13, 2012, 12:10 p.m. UTC 
From: Troy Kisky <troy.kisky@boundarydevices.com>

Allow boards to change what is advertised before an autoneg
restart happens.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
CC: Troy Kisky <troy.kisky@boundarydevices.com>
CC: Stefano Babic <sbabic@denx.de>
CC: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/net/fec_mxc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Comments

Marek Vasut Jan. 13, 2012, 12:44 p.m. UTC | #1 
> From: Troy Kisky <troy.kisky@boundarydevices.com>
> 
> Allow boards to change what is advertised before an autoneg
> restart happens.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> CC: Troy Kisky <troy.kisky@boundarydevices.com>
> CC: Stefano Babic <sbabic@denx.de>
> CC: Marek Vasut <marek.vasut@gmail.com>
> ---
>  drivers/net/fec_mxc.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index b50e01c..6271df5 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device
> *dev) miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE,
>  			LPA_100FULL | LPA_100HALF | LPA_10FULL |
>  			LPA_10HALF | PHY_ANLPAR_PSB_802_3);
> -	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
> -			BMCR_ANENABLE | BMCR_ANRESTART);
> -
>  	if (fec->mii_postcall)
>  		ret = fec->mii_postcall(fec->phy_id);
> 
> +	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
> +			BMCR_ANENABLE | BMCR_ANRESTART);
> +
>  	return ret;
>  }

I think this might break boards using the mii_postcall.
Stefano Babic Jan. 13, 2012, 1:54 p.m. UTC | #2 
On 13/01/2012 13:10, Dirk Behme wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com>
> 
> Allow boards to change what is advertised before an autoneg
> restart happens.
> 
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> CC: Troy Kisky <troy.kisky@boundarydevices.com>
> CC: Stefano Babic <sbabic@denx.de>
> CC: Marek Vasut <marek.vasut@gmail.com>
> ---
>  drivers/net/fec_mxc.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index b50e01c..6271df5 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device *dev)
>  	miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE,
>  			LPA_100FULL | LPA_100HALF | LPA_10FULL |
>  			LPA_10HALF | PHY_ANLPAR_PSB_802_3);
> -	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
> -			BMCR_ANENABLE | BMCR_ANRESTART);
> -
>  	if (fec->mii_postcall)
>  		ret = fec->mii_postcall(fec->phy_id);
>  
> +	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
> +			BMCR_ANENABLE | BMCR_ANRESTART);
> +

...but you change the postcall into a precall...

The current implementation is correct. Something is set / written , and
after that a specific supplied function (if any) is called, then the
name postcall.

Changing the order is not correct if the fec->mii_postcall thinks (as it
should be) that the MII_BMCR register was already written.

But you missed to mention why you need such as change : which is the
failure / bug you discovered ?

Best regards,
Stefano Babic
Troy Kisky Jan. 13, 2012, 9:22 p.m. UTC | #3 
On 1/13/2012 6:54 AM, Stefano Babic wrote:
> On 13/01/2012 13:10, Dirk Behme wrote:
>> From: Troy Kisky<troy.kisky@boundarydevices.com>
>>
>> Allow boards to change what is advertised before an autoneg
>> restart happens.
>>
>> Signed-off-by: Troy Kisky<troy.kisky@boundarydevices.com>
>> CC: Troy Kisky<troy.kisky@boundarydevices.com>
>> CC: Stefano Babic<sbabic@denx.de>
>> CC: Marek Vasut<marek.vasut@gmail.com>
>> ---
>>   drivers/net/fec_mxc.c |    6 +++---
>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>> index b50e01c..6271df5 100644
>> --- a/drivers/net/fec_mxc.c
>> +++ b/drivers/net/fec_mxc.c
>> @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device *dev)
>>   	miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE,
>>   			LPA_100FULL | LPA_100HALF | LPA_10FULL |
>>   			LPA_10HALF | PHY_ANLPAR_PSB_802_3);
>> -	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
>> -			BMCR_ANENABLE | BMCR_ANRESTART);
>> -
>>   	if (fec->mii_postcall)
>>   		ret = fec->mii_postcall(fec->phy_id);
>>
>> +	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
>> +			BMCR_ANENABLE | BMCR_ANRESTART);
>> +
> ...but you change the postcall into a precall...

I assumed mii_postcall meant mii_post_init_call
not mii_post_aneg_call.

A rename of the function may be desired.

Perhaps we should let the mii_postcall function do the

miiphy_write(dev->name, fec->phy_id, MII_BMCR,
			BMCR_ANENABLE | BMCR_ANRESTART);

as well.





>
> The current implementation is correct. Something is set / written , and
> after that a specific supplied function (if any) is called, then the
> name postcall.
>
> Changing the order is not correct if the fec->mii_postcall thinks (as it
> should be) that the MII_BMCR register was already written.
>
> But you missed to mention why you need such as change : which is the
> failure / bug you discovered ?

I wanted the sabrelite board to be able to remove 1000BaseT 
advertisement before aneg starts.

I think it works either way, but is clearer this way and doesn't rely on 
luck.

>
> Best regards,
> Stefano Babic
>
Troy Kisky Jan. 13, 2012, 9:42 p.m. UTC | #4 
On 1/13/2012 5:44 AM, Marek Vasut wrote:
>> From: Troy Kisky<troy.kisky@boundarydevices.com>
>>
>> Allow boards to change what is advertised before an autoneg
>> restart happens.
>>
>> Signed-off-by: Troy Kisky<troy.kisky@boundarydevices.com>
>> CC: Troy Kisky<troy.kisky@boundarydevices.com>
>> CC: Stefano Babic<sbabic@denx.de>
>> CC: Marek Vasut<marek.vasut@gmail.com>
>> ---
>>   drivers/net/fec_mxc.c |    6 +++---
>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>> index b50e01c..6271df5 100644
>> --- a/drivers/net/fec_mxc.c
>> +++ b/drivers/net/fec_mxc.c
>> @@ -206,12 +206,12 @@ static int miiphy_restart_aneg(struct eth_device
>> *dev) miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE,
>>   			LPA_100FULL | LPA_100HALF | LPA_10FULL |
>>   			LPA_10HALF | PHY_ANLPAR_PSB_802_3);
>> -	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
>> -			BMCR_ANENABLE | BMCR_ANRESTART);
>> -
>>   	if (fec->mii_postcall)
>>   		ret = fec->mii_postcall(fec->phy_id);
>>
>> +	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
>> +			BMCR_ANENABLE | BMCR_ANRESTART);
>> +
>>   	return ret;
>>   }
> I think this might break boards using the mii_postcall.
>
Since only 3 boards use this. Perhaps I should code as

if (fec->mii_start_aneg_call)
	ret = fec->mii_start_aneg_call(fec->phy_id);
else
	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
		BMCR_ANENABLE | BMCR_ANRESTART);



and copy the
	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
		BMCR_ANENABLE | BMCR_ANRESTART);


into all three callbacks?

Troy
diff mbox

Patch

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index b50e01c..6271df5 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -206,12 +206,12 @@  static int miiphy_restart_aneg(struct eth_device *dev)
 	miiphy_write(dev->name, fec->phy_id, MII_ADVERTISE,
 			LPA_100FULL | LPA_100HALF | LPA_10FULL |
 			LPA_10HALF | PHY_ANLPAR_PSB_802_3);
-	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
-			BMCR_ANENABLE | BMCR_ANRESTART);
-
 	if (fec->mii_postcall)
 		ret = fec->mii_postcall(fec->phy_id);
 
+	miiphy_write(dev->name, fec->phy_id, MII_BMCR,
+			BMCR_ANENABLE | BMCR_ANRESTART);
+
 	return ret;
 }