Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
2014-01-31 Max Filippov :
> On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli wrote:
>> 2014-01-31 Florian Fainelli :
> Maybe they boot up with gigabit advertisement disabled in their PHY
> and thus they don't see the problem?
>
>> Other drivers do the following:
>>
>> - connect to the PHY
>> - phydev->supported = PHY_BASIC_FEATURES
>> - phydev->advertising &= phydev->supported
>> - start the PHY state machine
>>
>> And they work just fine. Is the PHY driver you are bound to the "Generic
>> PHY" or something else which does something funky in config_aneg()?
>
> It's marvell 88E from the KC-705 board, but the behaviour doesn't
> change if I disable it and the generic phy is used.
Florian,
I don't see how the generic genphy_config_advert can ever change
gigabit advertisement if phydev->supported has gigabit speeds masked
off.
>>>
>>> It does not, but it makes sure that phydev->advertising gets masked
>>> with phydev->supported. So, prior to starting your PHY state machine,
>>> if you do:
>>>
>>> phydev->supported &= PHY_BASIC_FEATURES;
>>> phydev->advertising = phydev->supported;
>>
>> It looks like there might be one problem however, if we have the following:
>>
>> - phydev->supported masks off the Gigabit features
>> - PHY device comes out of reset/default with Gigabit features set in
>> MII_CTRL1000
>
> That's exactly my case.
>
>> Could you try the following patch:
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 78bf1a4..b607f4a 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device
>> *phydev)
>> }
>>
>> /* Configure gigabit if it's supported */
>> + adv = phy_read(phydev, MII_CTRL1000);
>> + if (adv < 0)
>> + return adv;
>> +
>> + oldadv = adv;
>> + adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
>> +
>> if (phydev->supported & (SUPPORTED_1000baseT_Half |
>> SUPPORTED_1000baseT_Full)) {
>> - adv = phy_read(phydev, MII_CTRL1000);
>> - if (adv < 0)
>> - return adv;
>> -
>> - oldadv = adv;
>> - adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
>> adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
>> -
>> - if (adv != oldadv) {
>> - err = phy_write(phydev, MII_CTRL1000, adv);
>> -
>> - if (err < 0)
>> - return err;
>> + if (adv != oldadv)
>> changed = 1;
>> - }
>> }
>>
>> + err = phy_write(phydev, MII_CTRL1000, adv);
>
>
> I don't think this is correct: MII_CTRL1000 is reserved for 10/100 PHYs,
> we probably need to read/write it conditionally, depending on MII_BMSR
> BMSR_ESTATEN bit.
> Otherwise yes, it works for me too.
You are right, that needs to be made conditional. I will give this
patch some more proper testing on a truly non-gigabit PHY. Thanks!
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli wrote:
> 2014-01-31 Florian Fainelli :
Maybe they boot up with gigabit advertisement disabled in their PHY
and thus they don't see the problem?
> Other drivers do the following:
>
> - connect to the PHY
> - phydev->supported = PHY_BASIC_FEATURES
> - phydev->advertising &= phydev->supported
> - start the PHY state machine
>
> And they work just fine. Is the PHY driver you are bound to the "Generic
> PHY" or something else which does something funky in config_aneg()?
It's marvell 88E from the KC-705 board, but the behaviour doesn't
change if I disable it and the generic phy is used.
>>>
>>> Florian,
>>>
>>> I don't see how the generic genphy_config_advert can ever change
>>> gigabit advertisement if phydev->supported has gigabit speeds masked
>>> off.
>>
>> It does not, but it makes sure that phydev->advertising gets masked
>> with phydev->supported. So, prior to starting your PHY state machine,
>> if you do:
>>
>> phydev->supported &= PHY_BASIC_FEATURES;
>> phydev->advertising = phydev->supported;
>
> It looks like there might be one problem however, if we have the following:
>
> - phydev->supported masks off the Gigabit features
> - PHY device comes out of reset/default with Gigabit features set in
> MII_CTRL1000
That's exactly my case.
> Could you try the following patch:
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 78bf1a4..b607f4a 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device
> *phydev)
> }
>
> /* Configure gigabit if it's supported */
> + adv = phy_read(phydev, MII_CTRL1000);
> + if (adv < 0)
> + return adv;
> +
> + oldadv = adv;
> + adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
> +
> if (phydev->supported & (SUPPORTED_1000baseT_Half |
> SUPPORTED_1000baseT_Full)) {
> - adv = phy_read(phydev, MII_CTRL1000);
> - if (adv < 0)
> - return adv;
> -
> - oldadv = adv;
> - adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
> adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
> -
> - if (adv != oldadv) {
> - err = phy_write(phydev, MII_CTRL1000, adv);
> -
> - if (err < 0)
> - return err;
> + if (adv != oldadv)
> changed = 1;
> - }
> }
>
> + err = phy_write(phydev, MII_CTRL1000, adv);
I don't think this is correct: MII_CTRL1000 is reserved for 10/100 PHYs,
we probably need to read/write it conditionally, depending on MII_BMSR
BMSR_ESTATEN bit.
Otherwise yes, it works for me too.
> + if (err < 0)
> + return err;
> +
> return changed;
> }
--
Thanks.
-- Max
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
2014-01-31 Florian Fainelli :
>>> Maybe they boot up with gigabit advertisement disabled in their PHY
>>> and thus they don't see the problem?
>>>
Other drivers do the following:
- connect to the PHY
- phydev->supported = PHY_BASIC_FEATURES
- phydev->advertising &= phydev->supported
- start the PHY state machine
And they work just fine. Is the PHY driver you are bound to the "Generic
PHY" or something else which does something funky in config_aneg()?
>>>
>>> It's marvell 88E from the KC-705 board, but the behaviour doesn't
>>> change if I disable it and the generic phy is used.
>>
>> Florian,
>>
>> I don't see how the generic genphy_config_advert can ever change
>> gigabit advertisement if phydev->supported has gigabit speeds masked
>> off.
>
> It does not, but it makes sure that phydev->advertising gets masked
> with phydev->supported. So, prior to starting your PHY state machine,
> if you do:
>
> phydev->supported &= PHY_BASIC_FEATURES;
> phydev->advertising = phydev->supported;
It looks like there might be one problem however, if we have the following:
- phydev->supported masks off the Gigabit features
- PHY device comes out of reset/default with Gigabit features set in
MII_CTRL1000
Could you try the following patch:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 78bf1a4..b607f4a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device *phydev)
}
/* Configure gigabit if it's supported */
+ adv = phy_read(phydev, MII_CTRL1000);
+ if (adv < 0)
+ return adv;
+
+ oldadv = adv;
+ adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
+
if (phydev->supported & (SUPPORTED_1000baseT_Half |
SUPPORTED_1000baseT_Full)) {
- adv = phy_read(phydev, MII_CTRL1000);
- if (adv < 0)
- return adv;
-
- oldadv = adv;
- adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
-
- if (adv != oldadv) {
- err = phy_write(phydev, MII_CTRL1000, adv);
-
- if (err < 0)
- return err;
+ if (adv != oldadv)
changed = 1;
- }
}
+ err = phy_write(phydev, MII_CTRL1000, adv);
+ if (err < 0)
+ return err;
+
return changed;
}
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
2014-01-30 Max Filippov : > On Wed, Jan 29, 2014 at 10:32 PM, Max Filippov wrote: >> On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli >> wrote: >>> On Jan 28, 2014 11:01 PM, "Max Filippov" wrote: On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli wrote: > Hi Max, > > Le 28/01/2014 22:00, Max Filippov a écrit : > >> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but >> does >> not disable advertisement when PHY supports them. This results in >> non-functioning network when the MAC is connected to a gigabit PHY >> connected >> to a gigabit switch. >> >> The fix is to disable gigabit speed advertisement on attached PHY >> unconditionally. >> >> Signed-off-by: Max Filippov >> --- >> Changes v1->v2: >> - disable both gigabit advertisement and support. >> >> drivers/net/ethernet/ethoc.c | 8 >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/ethoc.c >> b/drivers/net/ethernet/ethoc.c >> index 4de8cfd..5643b2d 100644 >> --- a/drivers/net/ethernet/ethoc.c >> +++ b/drivers/net/ethernet/ethoc.c >> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device >> *dev) >> } >> >> priv->phy = phy; >> + phy_update_advert(phy, >> + ADVERTISED_1000baseT_Full | >> + ADVERTISED_1000baseT_Half, 0); >> + phy_start_aneg(phy); > > > This does not look necessary, you should not have to call > phy_start_aneg() > because the PHY state machine is not yet started, at best this calls > does > nothing. This call actually makes the whole thing work, because otherwise once gigabit support is cleared from the supported mask genphy_config_advert does not update gigabit advertisement register, leaving it enabled. >>> >>> OK, then we need to figure out what is wrong with ethoc since this is >>> unusual. >> >> Maybe they boot up with gigabit advertisement disabled in their PHY >> and thus they don't see the problem? >> >>> Other drivers do the following: >>> >>> - connect to the PHY >>> - phydev->supported = PHY_BASIC_FEATURES >>> - phydev->advertising &= phydev->supported >>> - start the PHY state machine >>> >>> And they work just fine. Is the PHY driver you are bound to the "Generic >>> PHY" or something else which does something funky in config_aneg()? >> >> It's marvell 88E from the KC-705 board, but the behaviour doesn't >> change if I disable it and the generic phy is used. > > Florian, > > I don't see how the generic genphy_config_advert can ever change > gigabit advertisement if phydev->supported has gigabit speeds masked > off. It does not, but it makes sure that phydev->advertising gets masked with phydev->supported. So, prior to starting your PHY state machine, if you do: phydev->supported &= PHY_BASIC_FEATURES; phydev->advertising = phydev->supported; you should get the expected results. Just make sure that phydev->autoneg is not set to AUTONEG_DISABLE, otherwise phy_start_aneg() will not call phy_sanitize_settings(), and this might be the problem. > So I'm pretty sure that other 10/100 cards would exhibit the same > issue if their PHY started with gigabit advertisement enabled. If the speed is not restricted, yes that will be the case, but most drivers seem to correctly set both phydev->supported and phydev->advertising. > Maybe > we need to fix those other drivers? Or maybe we need to track what > PHY really supports vs. what we report it supports, so that gigabit > advertisement could be changed even when the PHY no longer > appears to support gigabit? This is supposedly already taken care of provided that you correctly restrict the PHY supported/advertising bits with respect to what the HW really supports. -- Florian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
2014-01-30 Max Filippov jcmvb...@gmail.com: On Wed, Jan 29, 2014 at 10:32 PM, Max Filippov jcmvb...@gmail.com wrote: On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli f.faine...@gmail.com wrote: On Jan 28, 2014 11:01 PM, Max Filippov jcmvb...@gmail.com wrote: On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli f.faine...@gmail.com wrote: Hi Max, Le 28/01/2014 22:00, Max Filippov a écrit : OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does not disable advertisement when PHY supports them. This results in non-functioning network when the MAC is connected to a gigabit PHY connected to a gigabit switch. The fix is to disable gigabit speed advertisement on attached PHY unconditionally. Signed-off-by: Max Filippov jcmvb...@gmail.com --- Changes v1-v2: - disable both gigabit advertisement and support. drivers/net/ethernet/ethoc.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 4de8cfd..5643b2d 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) } priv-phy = phy; + phy_update_advert(phy, + ADVERTISED_1000baseT_Full | + ADVERTISED_1000baseT_Half, 0); + phy_start_aneg(phy); This does not look necessary, you should not have to call phy_start_aneg() because the PHY state machine is not yet started, at best this calls does nothing. This call actually makes the whole thing work, because otherwise once gigabit support is cleared from the supported mask genphy_config_advert does not update gigabit advertisement register, leaving it enabled. OK, then we need to figure out what is wrong with ethoc since this is unusual. Maybe they boot up with gigabit advertisement disabled in their PHY and thus they don't see the problem? Other drivers do the following: - connect to the PHY - phydev-supported = PHY_BASIC_FEATURES - phydev-advertising = phydev-supported - start the PHY state machine And they work just fine. Is the PHY driver you are bound to the Generic PHY or something else which does something funky in config_aneg()? It's marvell 88E from the KC-705 board, but the behaviour doesn't change if I disable it and the generic phy is used. Florian, I don't see how the generic genphy_config_advert can ever change gigabit advertisement if phydev-supported has gigabit speeds masked off. It does not, but it makes sure that phydev-advertising gets masked with phydev-supported. So, prior to starting your PHY state machine, if you do: phydev-supported = PHY_BASIC_FEATURES; phydev-advertising = phydev-supported; you should get the expected results. Just make sure that phydev-autoneg is not set to AUTONEG_DISABLE, otherwise phy_start_aneg() will not call phy_sanitize_settings(), and this might be the problem. So I'm pretty sure that other 10/100 cards would exhibit the same issue if their PHY started with gigabit advertisement enabled. If the speed is not restricted, yes that will be the case, but most drivers seem to correctly set both phydev-supported and phydev-advertising. Maybe we need to fix those other drivers? Or maybe we need to track what PHY really supports vs. what we report it supports, so that gigabit advertisement could be changed even when the PHY no longer appears to support gigabit? This is supposedly already taken care of provided that you correctly restrict the PHY supported/advertising bits with respect to what the HW really supports. -- Florian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
2014-01-31 Florian Fainelli f.faine...@gmail.com:
Maybe they boot up with gigabit advertisement disabled in their PHY
and thus they don't see the problem?
Other drivers do the following:
- connect to the PHY
- phydev-supported = PHY_BASIC_FEATURES
- phydev-advertising = phydev-supported
- start the PHY state machine
And they work just fine. Is the PHY driver you are bound to the Generic
PHY or something else which does something funky in config_aneg()?
It's marvell 88E from the KC-705 board, but the behaviour doesn't
change if I disable it and the generic phy is used.
Florian,
I don't see how the generic genphy_config_advert can ever change
gigabit advertisement if phydev-supported has gigabit speeds masked
off.
It does not, but it makes sure that phydev-advertising gets masked
with phydev-supported. So, prior to starting your PHY state machine,
if you do:
phydev-supported = PHY_BASIC_FEATURES;
phydev-advertising = phydev-supported;
It looks like there might be one problem however, if we have the following:
- phydev-supported masks off the Gigabit features
- PHY device comes out of reset/default with Gigabit features set in
MII_CTRL1000
Could you try the following patch:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 78bf1a4..b607f4a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device *phydev)
}
/* Configure gigabit if it's supported */
+ adv = phy_read(phydev, MII_CTRL1000);
+ if (adv 0)
+ return adv;
+
+ oldadv = adv;
+ adv = ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
+
if (phydev-supported (SUPPORTED_1000baseT_Half |
SUPPORTED_1000baseT_Full)) {
- adv = phy_read(phydev, MII_CTRL1000);
- if (adv 0)
- return adv;
-
- oldadv = adv;
- adv = ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
-
- if (adv != oldadv) {
- err = phy_write(phydev, MII_CTRL1000, adv);
-
- if (err 0)
- return err;
+ if (adv != oldadv)
changed = 1;
- }
}
+ err = phy_write(phydev, MII_CTRL1000, adv);
+ if (err 0)
+ return err;
+
return changed;
}
--
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli f.faine...@gmail.com wrote:
2014-01-31 Florian Fainelli f.faine...@gmail.com:
Maybe they boot up with gigabit advertisement disabled in their PHY
and thus they don't see the problem?
Other drivers do the following:
- connect to the PHY
- phydev-supported = PHY_BASIC_FEATURES
- phydev-advertising = phydev-supported
- start the PHY state machine
And they work just fine. Is the PHY driver you are bound to the Generic
PHY or something else which does something funky in config_aneg()?
It's marvell 88E from the KC-705 board, but the behaviour doesn't
change if I disable it and the generic phy is used.
Florian,
I don't see how the generic genphy_config_advert can ever change
gigabit advertisement if phydev-supported has gigabit speeds masked
off.
It does not, but it makes sure that phydev-advertising gets masked
with phydev-supported. So, prior to starting your PHY state machine,
if you do:
phydev-supported = PHY_BASIC_FEATURES;
phydev-advertising = phydev-supported;
It looks like there might be one problem however, if we have the following:
- phydev-supported masks off the Gigabit features
- PHY device comes out of reset/default with Gigabit features set in
MII_CTRL1000
That's exactly my case.
Could you try the following patch:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 78bf1a4..b607f4a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device
*phydev)
}
/* Configure gigabit if it's supported */
+ adv = phy_read(phydev, MII_CTRL1000);
+ if (adv 0)
+ return adv;
+
+ oldadv = adv;
+ adv = ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
+
if (phydev-supported (SUPPORTED_1000baseT_Half |
SUPPORTED_1000baseT_Full)) {
- adv = phy_read(phydev, MII_CTRL1000);
- if (adv 0)
- return adv;
-
- oldadv = adv;
- adv = ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
-
- if (adv != oldadv) {
- err = phy_write(phydev, MII_CTRL1000, adv);
-
- if (err 0)
- return err;
+ if (adv != oldadv)
changed = 1;
- }
}
+ err = phy_write(phydev, MII_CTRL1000, adv);
I don't think this is correct: MII_CTRL1000 is reserved for 10/100 PHYs,
we probably need to read/write it conditionally, depending on MII_BMSR
BMSR_ESTATEN bit.
Otherwise yes, it works for me too.
+ if (err 0)
+ return err;
+
return changed;
}
--
Thanks.
-- Max
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
2014-01-31 Max Filippov jcmvb...@gmail.com:
On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli f.faine...@gmail.com wrote:
2014-01-31 Florian Fainelli f.faine...@gmail.com:
Maybe they boot up with gigabit advertisement disabled in their PHY
and thus they don't see the problem?
Other drivers do the following:
- connect to the PHY
- phydev-supported = PHY_BASIC_FEATURES
- phydev-advertising = phydev-supported
- start the PHY state machine
And they work just fine. Is the PHY driver you are bound to the Generic
PHY or something else which does something funky in config_aneg()?
It's marvell 88E from the KC-705 board, but the behaviour doesn't
change if I disable it and the generic phy is used.
Florian,
I don't see how the generic genphy_config_advert can ever change
gigabit advertisement if phydev-supported has gigabit speeds masked
off.
It does not, but it makes sure that phydev-advertising gets masked
with phydev-supported. So, prior to starting your PHY state machine,
if you do:
phydev-supported = PHY_BASIC_FEATURES;
phydev-advertising = phydev-supported;
It looks like there might be one problem however, if we have the following:
- phydev-supported masks off the Gigabit features
- PHY device comes out of reset/default with Gigabit features set in
MII_CTRL1000
That's exactly my case.
Could you try the following patch:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 78bf1a4..b607f4a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device
*phydev)
}
/* Configure gigabit if it's supported */
+ adv = phy_read(phydev, MII_CTRL1000);
+ if (adv 0)
+ return adv;
+
+ oldadv = adv;
+ adv = ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
+
if (phydev-supported (SUPPORTED_1000baseT_Half |
SUPPORTED_1000baseT_Full)) {
- adv = phy_read(phydev, MII_CTRL1000);
- if (adv 0)
- return adv;
-
- oldadv = adv;
- adv = ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
-
- if (adv != oldadv) {
- err = phy_write(phydev, MII_CTRL1000, adv);
-
- if (err 0)
- return err;
+ if (adv != oldadv)
changed = 1;
- }
}
+ err = phy_write(phydev, MII_CTRL1000, adv);
I don't think this is correct: MII_CTRL1000 is reserved for 10/100 PHYs,
we probably need to read/write it conditionally, depending on MII_BMSR
BMSR_ESTATEN bit.
Otherwise yes, it works for me too.
You are right, that needs to be made conditional. I will give this
patch some more proper testing on a truly non-gigabit PHY. Thanks!
--
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
On Wed, Jan 29, 2014 at 10:32 PM, Max Filippov wrote: > On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli > wrote: >> On Jan 28, 2014 11:01 PM, "Max Filippov" wrote: >>> >>> On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli >>> wrote: >>> > Hi Max, >>> > >>> > Le 28/01/2014 22:00, Max Filippov a écrit : >>> > >>> >> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but >>> >> does >>> >> not disable advertisement when PHY supports them. This results in >>> >> non-functioning network when the MAC is connected to a gigabit PHY >>> >> connected >>> >> to a gigabit switch. >>> >> >>> >> The fix is to disable gigabit speed advertisement on attached PHY >>> >> unconditionally. >>> >> >>> >> Signed-off-by: Max Filippov >>> >> --- >>> >> Changes v1->v2: >>> >> - disable both gigabit advertisement and support. >>> >> >>> >> drivers/net/ethernet/ethoc.c | 8 >>> >> 1 file changed, 8 insertions(+) >>> >> >>> >> diff --git a/drivers/net/ethernet/ethoc.c >>> >> b/drivers/net/ethernet/ethoc.c >>> >> index 4de8cfd..5643b2d 100644 >>> >> --- a/drivers/net/ethernet/ethoc.c >>> >> +++ b/drivers/net/ethernet/ethoc.c >>> >> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device >>> >> *dev) >>> >> } >>> >> >>> >> priv->phy = phy; >>> >> + phy_update_advert(phy, >>> >> + ADVERTISED_1000baseT_Full | >>> >> + ADVERTISED_1000baseT_Half, 0); >>> >> + phy_start_aneg(phy); >>> > >>> > >>> > This does not look necessary, you should not have to call >>> > phy_start_aneg() >>> > because the PHY state machine is not yet started, at best this calls >>> > does >>> > nothing. >>> >>> This call actually makes the whole thing work, because otherwise once >>> gigabit >>> support is cleared from the supported mask genphy_config_advert does not >>> update gigabit advertisement register, leaving it enabled. >> >> OK, then we need to figure out what is wrong with ethoc since this is >> unusual. > > Maybe they boot up with gigabit advertisement disabled in their PHY > and thus they don't see the problem? > >> Other drivers do the following: >> >> - connect to the PHY >> - phydev->supported = PHY_BASIC_FEATURES >> - phydev->advertising &= phydev->supported >> - start the PHY state machine >> >> And they work just fine. Is the PHY driver you are bound to the "Generic >> PHY" or something else which does something funky in config_aneg()? > > It's marvell 88E from the KC-705 board, but the behaviour doesn't > change if I disable it and the generic phy is used. Florian, I don't see how the generic genphy_config_advert can ever change gigabit advertisement if phydev->supported has gigabit speeds masked off. So I'm pretty sure that other 10/100 cards would exhibit the same issue if their PHY started with gigabit advertisement enabled. Maybe we need to fix those other drivers? Or maybe we need to track what PHY really supports vs. what we report it supports, so that gigabit advertisement could be changed even when the PHY no longer appears to support gigabit? -- Thanks. -- Max -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
On Wed, Jan 29, 2014 at 10:32 PM, Max Filippov jcmvb...@gmail.com wrote: On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli f.faine...@gmail.com wrote: On Jan 28, 2014 11:01 PM, Max Filippov jcmvb...@gmail.com wrote: On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli f.faine...@gmail.com wrote: Hi Max, Le 28/01/2014 22:00, Max Filippov a écrit : OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does not disable advertisement when PHY supports them. This results in non-functioning network when the MAC is connected to a gigabit PHY connected to a gigabit switch. The fix is to disable gigabit speed advertisement on attached PHY unconditionally. Signed-off-by: Max Filippov jcmvb...@gmail.com --- Changes v1-v2: - disable both gigabit advertisement and support. drivers/net/ethernet/ethoc.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 4de8cfd..5643b2d 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) } priv-phy = phy; + phy_update_advert(phy, + ADVERTISED_1000baseT_Full | + ADVERTISED_1000baseT_Half, 0); + phy_start_aneg(phy); This does not look necessary, you should not have to call phy_start_aneg() because the PHY state machine is not yet started, at best this calls does nothing. This call actually makes the whole thing work, because otherwise once gigabit support is cleared from the supported mask genphy_config_advert does not update gigabit advertisement register, leaving it enabled. OK, then we need to figure out what is wrong with ethoc since this is unusual. Maybe they boot up with gigabit advertisement disabled in their PHY and thus they don't see the problem? Other drivers do the following: - connect to the PHY - phydev-supported = PHY_BASIC_FEATURES - phydev-advertising = phydev-supported - start the PHY state machine And they work just fine. Is the PHY driver you are bound to the Generic PHY or something else which does something funky in config_aneg()? It's marvell 88E from the KC-705 board, but the behaviour doesn't change if I disable it and the generic phy is used. Florian, I don't see how the generic genphy_config_advert can ever change gigabit advertisement if phydev-supported has gigabit speeds masked off. So I'm pretty sure that other 10/100 cards would exhibit the same issue if their PHY started with gigabit advertisement enabled. Maybe we need to fix those other drivers? Or maybe we need to track what PHY really supports vs. what we report it supports, so that gigabit advertisement could be changed even when the PHY no longer appears to support gigabit? -- Thanks. -- Max -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli wrote: > On Jan 28, 2014 11:01 PM, "Max Filippov" wrote: >> >> On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli >> wrote: >> > Hi Max, >> > >> > Le 28/01/2014 22:00, Max Filippov a écrit : >> > >> >> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but >> >> does >> >> not disable advertisement when PHY supports them. This results in >> >> non-functioning network when the MAC is connected to a gigabit PHY >> >> connected >> >> to a gigabit switch. >> >> >> >> The fix is to disable gigabit speed advertisement on attached PHY >> >> unconditionally. >> >> >> >> Signed-off-by: Max Filippov >> >> --- >> >> Changes v1->v2: >> >> - disable both gigabit advertisement and support. >> >> >> >> drivers/net/ethernet/ethoc.c | 8 >> >> 1 file changed, 8 insertions(+) >> >> >> >> diff --git a/drivers/net/ethernet/ethoc.c >> >> b/drivers/net/ethernet/ethoc.c >> >> index 4de8cfd..5643b2d 100644 >> >> --- a/drivers/net/ethernet/ethoc.c >> >> +++ b/drivers/net/ethernet/ethoc.c >> >> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device >> >> *dev) >> >> } >> >> >> >> priv->phy = phy; >> >> + phy_update_advert(phy, >> >> + ADVERTISED_1000baseT_Full | >> >> + ADVERTISED_1000baseT_Half, 0); >> >> + phy_start_aneg(phy); >> > >> > >> > This does not look necessary, you should not have to call >> > phy_start_aneg() >> > because the PHY state machine is not yet started, at best this calls >> > does >> > nothing. >> >> This call actually makes the whole thing work, because otherwise once >> gigabit >> support is cleared from the supported mask genphy_config_advert does not >> update gigabit advertisement register, leaving it enabled. > > OK, then we need to figure out what is wrong with ethoc since this is > unusual. Maybe they boot up with gigabit advertisement disabled in their PHY and thus they don't see the problem? > Other drivers do the following: > > - connect to the PHY > - phydev->supported = PHY_BASIC_FEATURES > - phydev->advertising &= phydev->supported > - start the PHY state machine > > And they work just fine. Is the PHY driver you are bound to the "Generic > PHY" or something else which does something funky in config_aneg()? It's marvell 88E from the KC-705 board, but the behaviour doesn't change if I disable it and the generic phy is used. -- Thanks. -- Max -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli f.faine...@gmail.com wrote: On Jan 28, 2014 11:01 PM, Max Filippov jcmvb...@gmail.com wrote: On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli f.faine...@gmail.com wrote: Hi Max, Le 28/01/2014 22:00, Max Filippov a écrit : OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does not disable advertisement when PHY supports them. This results in non-functioning network when the MAC is connected to a gigabit PHY connected to a gigabit switch. The fix is to disable gigabit speed advertisement on attached PHY unconditionally. Signed-off-by: Max Filippov jcmvb...@gmail.com --- Changes v1-v2: - disable both gigabit advertisement and support. drivers/net/ethernet/ethoc.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 4de8cfd..5643b2d 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) } priv-phy = phy; + phy_update_advert(phy, + ADVERTISED_1000baseT_Full | + ADVERTISED_1000baseT_Half, 0); + phy_start_aneg(phy); This does not look necessary, you should not have to call phy_start_aneg() because the PHY state machine is not yet started, at best this calls does nothing. This call actually makes the whole thing work, because otherwise once gigabit support is cleared from the supported mask genphy_config_advert does not update gigabit advertisement register, leaving it enabled. OK, then we need to figure out what is wrong with ethoc since this is unusual. Maybe they boot up with gigabit advertisement disabled in their PHY and thus they don't see the problem? Other drivers do the following: - connect to the PHY - phydev-supported = PHY_BASIC_FEATURES - phydev-advertising = phydev-supported - start the PHY state machine And they work just fine. Is the PHY driver you are bound to the Generic PHY or something else which does something funky in config_aneg()? It's marvell 88E from the KC-705 board, but the behaviour doesn't change if I disable it and the generic phy is used. -- Thanks. -- Max -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli wrote: > Hi Max, > > Le 28/01/2014 22:00, Max Filippov a écrit : > >> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does >> not disable advertisement when PHY supports them. This results in >> non-functioning network when the MAC is connected to a gigabit PHY >> connected >> to a gigabit switch. >> >> The fix is to disable gigabit speed advertisement on attached PHY >> unconditionally. >> >> Signed-off-by: Max Filippov >> --- >> Changes v1->v2: >> - disable both gigabit advertisement and support. >> >> drivers/net/ethernet/ethoc.c | 8 >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c >> index 4de8cfd..5643b2d 100644 >> --- a/drivers/net/ethernet/ethoc.c >> +++ b/drivers/net/ethernet/ethoc.c >> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) >> } >> >> priv->phy = phy; >> + phy_update_advert(phy, >> + ADVERTISED_1000baseT_Full | >> + ADVERTISED_1000baseT_Half, 0); >> + phy_start_aneg(phy); > > > This does not look necessary, you should not have to call phy_start_aneg() > because the PHY state machine is not yet started, at best this calls does > nothing. This call actually makes the whole thing work, because otherwise once gigabit support is cleared from the supported mask genphy_config_advert does not update gigabit advertisement register, leaving it enabled. >> + phy_update_supported(phy, >> +SUPPORTED_1000baseT_Full | >> +SUPPORTED_1000baseT_Half, 0); >> + >> return 0; >> } >> >> > -- Thanks. -- Max -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
Hi Max, Le 28/01/2014 22:00, Max Filippov a écrit : OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does not disable advertisement when PHY supports them. This results in non-functioning network when the MAC is connected to a gigabit PHY connected to a gigabit switch. The fix is to disable gigabit speed advertisement on attached PHY unconditionally. Signed-off-by: Max Filippov --- Changes v1->v2: - disable both gigabit advertisement and support. drivers/net/ethernet/ethoc.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 4de8cfd..5643b2d 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) } priv->phy = phy; + phy_update_advert(phy, + ADVERTISED_1000baseT_Full | + ADVERTISED_1000baseT_Half, 0); + phy_start_aneg(phy); This does not look necessary, you should not have to call phy_start_aneg() because the PHY state machine is not yet started, at best this calls does nothing. + phy_update_supported(phy, +SUPPORTED_1000baseT_Full | +SUPPORTED_1000baseT_Half, 0); + return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does not disable advertisement when PHY supports them. This results in non-functioning network when the MAC is connected to a gigabit PHY connected to a gigabit switch. The fix is to disable gigabit speed advertisement on attached PHY unconditionally. Signed-off-by: Max Filippov --- Changes v1->v2: - disable both gigabit advertisement and support. drivers/net/ethernet/ethoc.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 4de8cfd..5643b2d 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) } priv->phy = phy; + phy_update_advert(phy, + ADVERTISED_1000baseT_Full | + ADVERTISED_1000baseT_Half, 0); + phy_start_aneg(phy); + phy_update_supported(phy, +SUPPORTED_1000baseT_Full | +SUPPORTED_1000baseT_Half, 0); + return 0; } -- 1.8.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does not disable advertisement when PHY supports them. This results in non-functioning network when the MAC is connected to a gigabit PHY connected to a gigabit switch. The fix is to disable gigabit speed advertisement on attached PHY unconditionally. Signed-off-by: Max Filippov jcmvb...@gmail.com --- Changes v1-v2: - disable both gigabit advertisement and support. drivers/net/ethernet/ethoc.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 4de8cfd..5643b2d 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) } priv-phy = phy; + phy_update_advert(phy, + ADVERTISED_1000baseT_Full | + ADVERTISED_1000baseT_Half, 0); + phy_start_aneg(phy); + phy_update_supported(phy, +SUPPORTED_1000baseT_Full | +SUPPORTED_1000baseT_Half, 0); + return 0; } -- 1.8.1.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
Hi Max, Le 28/01/2014 22:00, Max Filippov a écrit : OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does not disable advertisement when PHY supports them. This results in non-functioning network when the MAC is connected to a gigabit PHY connected to a gigabit switch. The fix is to disable gigabit speed advertisement on attached PHY unconditionally. Signed-off-by: Max Filippov jcmvb...@gmail.com --- Changes v1-v2: - disable both gigabit advertisement and support. drivers/net/ethernet/ethoc.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 4de8cfd..5643b2d 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) } priv-phy = phy; + phy_update_advert(phy, + ADVERTISED_1000baseT_Full | + ADVERTISED_1000baseT_Half, 0); + phy_start_aneg(phy); This does not look necessary, you should not have to call phy_start_aneg() because the PHY state machine is not yet started, at best this calls does nothing. + phy_update_supported(phy, +SUPPORTED_1000baseT_Full | +SUPPORTED_1000baseT_Half, 0); + return 0; } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli f.faine...@gmail.com wrote: Hi Max, Le 28/01/2014 22:00, Max Filippov a écrit : OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does not disable advertisement when PHY supports them. This results in non-functioning network when the MAC is connected to a gigabit PHY connected to a gigabit switch. The fix is to disable gigabit speed advertisement on attached PHY unconditionally. Signed-off-by: Max Filippov jcmvb...@gmail.com --- Changes v1-v2: - disable both gigabit advertisement and support. drivers/net/ethernet/ethoc.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c index 4de8cfd..5643b2d 100644 --- a/drivers/net/ethernet/ethoc.c +++ b/drivers/net/ethernet/ethoc.c @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev) } priv-phy = phy; + phy_update_advert(phy, + ADVERTISED_1000baseT_Full | + ADVERTISED_1000baseT_Half, 0); + phy_start_aneg(phy); This does not look necessary, you should not have to call phy_start_aneg() because the PHY state machine is not yet started, at best this calls does nothing. This call actually makes the whole thing work, because otherwise once gigabit support is cleared from the supported mask genphy_config_advert does not update gigabit advertisement register, leaving it enabled. + phy_update_supported(phy, +SUPPORTED_1000baseT_Full | +SUPPORTED_1000baseT_Half, 0); + return 0; } -- Thanks. -- Max -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
