Re: [PATCH net-next v2 02/10] r8169: use phy_resume/phy_suspend
On 10.07.2018 22:52, Andrew Lunn wrote: >>> Why is it powered up, but not connected? Is it powered down before it >>> is disconnected? Is it the bootloader which is powering it up? >>> >> Exactly, if the device is active when driver is loaded and the >> interface isn't used and therefore not brought up, then, when runtime- >> suspending, we face this situation. > > Well, it is more complex than that. If the interface is not used, why > bother even loading the MAC driver? > > If you are trying to make a really low power system, the bootloader > also needs to be involved. It needs to shut down anything it starts > before handing over control the linux. > > Another approach is to handle this in phylib. When the mdio bus is > suspended, look for any PHYs which are not connected and power them > down. The assumption being, any MAC driver using WoL via a PHY does > not disconnect the PHY before suspending. > Tricky part may be the differentiation between system- and runtime- suspend. IIRC the MDIO bus doesn't have runtime pm ops (yet) and most likely we would have to add them. > Andrew >
Re: [PATCH net-next v2 02/10] r8169: use phy_resume/phy_suspend
On 10.07.2018 22:52, Andrew Lunn wrote: >>> Why is it powered up, but not connected? Is it powered down before it >>> is disconnected? Is it the bootloader which is powering it up? >>> >> Exactly, if the device is active when driver is loaded and the >> interface isn't used and therefore not brought up, then, when runtime- >> suspending, we face this situation. > > Well, it is more complex than that. If the interface is not used, why > bother even loading the MAC driver? > It would be nice to be able to detect at driver load time whether interface is used. But driver loading is based on existence of the PCI device and we don't know about any netctl- or whatever-based network configuration. > If you are trying to make a really low power system, the bootloader > also needs to be involved. It needs to shut down anything it starts > before handing over control the linux. > I agree but in most cases wed don't have any influence on the bootloader / BIOS. > Another approach is to handle this in phylib. When the mdio bus is > suspended, look for any PHYs which are not connected and power them > down. The assumption being, any MAC driver using WoL via a PHY does > not disconnect the PHY before suspending. > > Andrew >
Re: [PATCH net-next v2 02/10] r8169: use phy_resume/phy_suspend
> > Why is it powered up, but not connected? Is it powered down before it > > is disconnected? Is it the bootloader which is powering it up? > > > Exactly, if the device is active when driver is loaded and the > interface isn't used and therefore not brought up, then, when runtime- > suspending, we face this situation. Well, it is more complex than that. If the interface is not used, why bother even loading the MAC driver? If you are trying to make a really low power system, the bootloader also needs to be involved. It needs to shut down anything it starts before handing over control the linux. Another approach is to handle this in phylib. When the mdio bus is suspended, look for any PHYs which are not connected and power them down. The assumption being, any MAC driver using WoL via a PHY does not disconnect the PHY before suspending. Andrew
Re: [PATCH net-next v2 02/10] r8169: use phy_resume/phy_suspend
On 10.07.2018 21:15, Andrew Lunn wrote:
>> static void r8168_pll_power_down(struct rtl8169_private *tp)
>> {
>> if (r8168_check_dash(tp))
>> @@ -4503,7 +4462,8 @@ static void r8168_pll_power_down(struct
>> rtl8169_private *tp)
>> if (rtl_wol_pll_power_down(tp))
>> return;
>>
>> -r8168_phy_power_down(tp);
>> +/* cover the case that PHY isn't connected */
>> +phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
>
> I don't particularly like this, because no other MAC driver does it.
>
I have to agree, it doesn't look too nice.
In general quite few network drivers seem to use runtime pm.
> Why is it powered up, but not connected? Is it powered down before it
> is disconnected? Is it the bootloader which is powering it up?
>
Exactly, if the device is active when driver is loaded and the
interface isn't used and therefore not brought up, then, when runtime-
suspending, we face this situation.
This could be changed by connecting the PHY in probe() already instead
of doing it in open(). I had this in the beginning, based on a
recommendation from Florian or you (don't remember) I changed this to
connect in open() only.
Do you have any preference or see a good way to deal with the situation?
Heiner
>Andrew
>
Re: [PATCH net-next v2 02/10] r8169: use phy_resume/phy_suspend
> static void r8168_pll_power_down(struct rtl8169_private *tp)
> {
> if (r8168_check_dash(tp))
> @@ -4503,7 +4462,8 @@ static void r8168_pll_power_down(struct rtl8169_private
> *tp)
> if (rtl_wol_pll_power_down(tp))
> return;
>
> - r8168_phy_power_down(tp);
> + /* cover the case that PHY isn't connected */
> + phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
I don't particularly like this, because no other MAC driver does it.
Why is it powered up, but not connected? Is it powered down before it
is disconnected? Is it the bootloader which is powering it up?
Andrew
[PATCH net-next v2 02/10] r8169: use phy_resume/phy_suspend
Use phy_resume() / phy_suspend() instead of open coding this functionality.
The chip version specific differences are handled by the respective PHY
drivers.
Signed-off-by: Heiner Kallweit
---
v2:
- no changes
---
drivers/net/ethernet/realtek/r8169.c | 48 +++-
1 file changed, 5 insertions(+), 43 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index d1951f32..e22d4e64 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4450,47 +4450,6 @@ static bool rtl_wol_pll_power_down(struct
rtl8169_private *tp)
return true;
}
-static void r8168_phy_power_up(struct rtl8169_private *tp)
-{
- rtl_writephy(tp, 0x1f, 0x);
- switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_11:
- case RTL_GIGA_MAC_VER_12:
- case RTL_GIGA_MAC_VER_17 ... RTL_GIGA_MAC_VER_28:
- case RTL_GIGA_MAC_VER_31:
- rtl_writephy(tp, 0x0e, 0x);
- break;
- default:
- break;
- }
- rtl_writephy(tp, MII_BMCR, BMCR_ANENABLE);
-
- /* give MAC/PHY some time to resume */
- msleep(20);
-}
-
-static void r8168_phy_power_down(struct rtl8169_private *tp)
-{
- rtl_writephy(tp, 0x1f, 0x);
- switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_32:
- case RTL_GIGA_MAC_VER_33:
- case RTL_GIGA_MAC_VER_40:
- case RTL_GIGA_MAC_VER_41:
- rtl_writephy(tp, MII_BMCR, BMCR_ANENABLE | BMCR_PDOWN);
- break;
-
- case RTL_GIGA_MAC_VER_11:
- case RTL_GIGA_MAC_VER_12:
- case RTL_GIGA_MAC_VER_17 ... RTL_GIGA_MAC_VER_28:
- case RTL_GIGA_MAC_VER_31:
- rtl_writephy(tp, 0x0e, 0x0200);
- default:
- rtl_writephy(tp, MII_BMCR, BMCR_PDOWN);
- break;
- }
-}
-
static void r8168_pll_power_down(struct rtl8169_private *tp)
{
if (r8168_check_dash(tp))
@@ -4503,7 +4462,8 @@ static void r8168_pll_power_down(struct rtl8169_private
*tp)
if (rtl_wol_pll_power_down(tp))
return;
- r8168_phy_power_down(tp);
+ /* cover the case that PHY isn't connected */
+ phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
switch (tp->mac_version) {
case RTL_GIGA_MAC_VER_25 ... RTL_GIGA_MAC_VER_33:
@@ -4556,7 +4516,9 @@ static void r8168_pll_power_up(struct rtl8169_private *tp)
break;
}
- r8168_phy_power_up(tp);
+ phy_resume(tp->dev->phydev);
+ /* give MAC/PHY some time to resume */
+ msleep(20);
}
static void rtl_pll_power_down(struct rtl8169_private *tp)
--
2.18.0
