| Message ID | f448b546-5b0a-79e0-f09a-dcfabb4fc8a5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] r8169: add rtl_disable_exit_l1() | expand |
> -----Original Message----- > From: Heiner Kallweit [mailto:hkallweit1@gmail.com] > Sent: Friday, January 28, 2022 6:15 AM > To: Jakub Kicinski <kuba@kernel.org>; David Miller <davem@davemloft.net>; > nic_swsd <nic_swsd@realtek.com> > Cc: netdev@vger.kernel.org; Hau <hau@realtek.com> > Subject: [PATCH net-next] r8169: add rtl_disable_exit_l1() > > Add rtl_disable_exit_l1() for ensuring that the chip doesn't inadvertently exit > ASPM L1 when being in a low-power mode. > The new function is called from rtl_prepare_power_down() which has to be > moved in the code to avoid a forward declaration. > > According to Realtek OCP register 0xc0ac shadows ERI register 0xd4 on > RTL8168 versions from RTL8168g. This allows to simplify the code a little. > > Suggested-by: Chun-Hao Lin <hau@realtek.com> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 65 ++++++++++++++--------- > 1 file changed, 39 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c > b/drivers/net/ethernet/realtek/r8169_main.c > index 3c3d1506b..104ebc0fb 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -2231,28 +2231,6 @@ static int rtl_set_mac_address(struct net_device > *dev, void *p) > return 0; > } > > -static void rtl_wol_enable_rx(struct rtl8169_private *tp) -{ > - if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > - RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > - AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > -} > - > -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{ > - if (tp->dash_type != RTL_DASH_NONE) > - return; > - > - if (tp->mac_version == RTL_GIGA_MAC_VER_32 || > - tp->mac_version == RTL_GIGA_MAC_VER_33) > - rtl_ephy_write(tp, 0x19, 0xff64); > - > - if (device_may_wakeup(tp_to_dev(tp))) { > - phy_speed_down(tp->phydev, false); > - rtl_wol_enable_rx(tp); > - } > -} > - > static void rtl_init_rxcfg(struct rtl8169_private *tp) { > switch (tp->mac_version) { > @@ -2667,10 +2645,7 @@ static void rtl_enable_exit_l1(struct > rtl8169_private *tp) > case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38: > rtl_eri_set_bits(tp, 0xd4, 0x0c00); > break; > - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53: > - rtl_eri_set_bits(tp, 0xd4, 0x1f80); > - break; > - case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63: > + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63: > r8168_mac_ocp_modify(tp, 0xc0ac, 0, 0x1f80); > break; > default: > @@ -2678,6 +2653,20 @@ static void rtl_enable_exit_l1(struct > rtl8169_private *tp) > } > } > > +static void rtl_disable_exit_l1(struct rtl8169_private *tp) { > + switch (tp->mac_version) { > + case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_38: > + rtl_eri_clear_bits(tp, 0xd4, 0x1f00); > + break; > + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63: > + r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0); > + break; > + default: > + break; > + } > +} > + > static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool > enable) { > /* Don't enable ASPM in the chip if OS can't control ASPM */ @@ - > 4689,6 +4678,30 @@ static int r8169_phy_connect(struct rtl8169_private *tp) > return 0; > } > > +static void rtl_wol_enable_rx(struct rtl8169_private *tp) { > + if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > + RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > + AcceptBroadcast | AcceptMulticast | AcceptMyPhys); } > + > +static void rtl_prepare_power_down(struct rtl8169_private *tp) { > + if (tp->dash_type != RTL_DASH_NONE) > + return; > + > + if (tp->mac_version == RTL_GIGA_MAC_VER_32 || > + tp->mac_version == RTL_GIGA_MAC_VER_33) > + rtl_ephy_write(tp, 0x19, 0xff64); > + > + if (device_may_wakeup(tp_to_dev(tp))) { > + phy_speed_down(tp->phydev, false); > + rtl_wol_enable_rx(tp); > + } else { > + rtl_disable_exit_l1(tp); > + } > +} > + Hi Heiner, rtl_disable_exit_l1(tp) can be called before device enter D3 state. I think you don’t need to check wol status. You may update the code link following. static void rtl_prepare_power_down(struct rtl8169_private *tp) { rtl_disable_exit_l1(tp); if (tp->dash_type != RTL_DASH_NONE) return; if (tp->mac_version == RTL_GIGA_MAC_VER_32 || tp->mac_version == RTL_GIGA_MAC_VER_33) rtl_ephy_write(tp, 0x19, 0xff64); if (device_may_wakeup(tp_to_dev(tp))) { phy_speed_down(tp->phydev, false); rtl_wol_enable_rx(tp); } } Thanks. > static void rtl8169_down(struct rtl8169_private *tp) { > /* Clear all task flags */ > -- > 2.35.0 > > ------Please consider the environment before printing this e-mail.
On 28.01.2022 09:09, Hau wrote: > > >> -----Original Message----- >> From: Heiner Kallweit [mailto:hkallweit1@gmail.com] >> Sent: Friday, January 28, 2022 6:15 AM >> To: Jakub Kicinski <kuba@kernel.org>; David Miller <davem@davemloft.net>; >> nic_swsd <nic_swsd@realtek.com> >> Cc: netdev@vger.kernel.org; Hau <hau@realtek.com> >> Subject: [PATCH net-next] r8169: add rtl_disable_exit_l1() >> >> Add rtl_disable_exit_l1() for ensuring that the chip doesn't inadvertently exit >> ASPM L1 when being in a low-power mode. >> The new function is called from rtl_prepare_power_down() which has to be >> moved in the code to avoid a forward declaration. >> >> According to Realtek OCP register 0xc0ac shadows ERI register 0xd4 on >> RTL8168 versions from RTL8168g. This allows to simplify the code a little. >> >> Suggested-by: Chun-Hao Lin <hau@realtek.com> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/ethernet/realtek/r8169_main.c | 65 ++++++++++++++--------- >> 1 file changed, 39 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c >> b/drivers/net/ethernet/realtek/r8169_main.c >> index 3c3d1506b..104ebc0fb 100644 >> --- a/drivers/net/ethernet/realtek/r8169_main.c >> +++ b/drivers/net/ethernet/realtek/r8169_main.c >> @@ -2231,28 +2231,6 @@ static int rtl_set_mac_address(struct net_device >> *dev, void *p) >> return 0; >> } >> >> -static void rtl_wol_enable_rx(struct rtl8169_private *tp) -{ >> - if (tp->mac_version >= RTL_GIGA_MAC_VER_25) >> - RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | >> - AcceptBroadcast | AcceptMulticast | AcceptMyPhys); >> -} >> - >> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{ >> - if (tp->dash_type != RTL_DASH_NONE) >> - return; >> - >> - if (tp->mac_version == RTL_GIGA_MAC_VER_32 || >> - tp->mac_version == RTL_GIGA_MAC_VER_33) >> - rtl_ephy_write(tp, 0x19, 0xff64); >> - >> - if (device_may_wakeup(tp_to_dev(tp))) { >> - phy_speed_down(tp->phydev, false); >> - rtl_wol_enable_rx(tp); >> - } >> -} >> - >> static void rtl_init_rxcfg(struct rtl8169_private *tp) { >> switch (tp->mac_version) { >> @@ -2667,10 +2645,7 @@ static void rtl_enable_exit_l1(struct >> rtl8169_private *tp) >> case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38: >> rtl_eri_set_bits(tp, 0xd4, 0x0c00); >> break; >> - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53: >> - rtl_eri_set_bits(tp, 0xd4, 0x1f80); >> - break; >> - case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63: >> + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63: >> r8168_mac_ocp_modify(tp, 0xc0ac, 0, 0x1f80); >> break; >> default: >> @@ -2678,6 +2653,20 @@ static void rtl_enable_exit_l1(struct >> rtl8169_private *tp) >> } >> } >> >> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) { >> + switch (tp->mac_version) { >> + case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_38: >> + rtl_eri_clear_bits(tp, 0xd4, 0x1f00); >> + break; >> + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63: >> + r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0); >> + break; >> + default: >> + break; >> + } >> +} >> + >> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool >> enable) { >> /* Don't enable ASPM in the chip if OS can't control ASPM */ @@ - >> 4689,6 +4678,30 @@ static int r8169_phy_connect(struct rtl8169_private *tp) >> return 0; >> } >> >> +static void rtl_wol_enable_rx(struct rtl8169_private *tp) { >> + if (tp->mac_version >= RTL_GIGA_MAC_VER_25) >> + RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | >> + AcceptBroadcast | AcceptMulticast | AcceptMyPhys); } >> + >> +static void rtl_prepare_power_down(struct rtl8169_private *tp) { >> + if (tp->dash_type != RTL_DASH_NONE) >> + return; >> + >> + if (tp->mac_version == RTL_GIGA_MAC_VER_32 || >> + tp->mac_version == RTL_GIGA_MAC_VER_33) >> + rtl_ephy_write(tp, 0x19, 0xff64); >> + >> + if (device_may_wakeup(tp_to_dev(tp))) { >> + phy_speed_down(tp->phydev, false); >> + rtl_wol_enable_rx(tp); >> + } else { >> + rtl_disable_exit_l1(tp); >> + } >> +} >> + > Hi Heiner, > Hi Hau, > rtl_disable_exit_l1(tp) can be called before device enter D3 state. I think you don’t need to check wol status. > You may update the code link following. my thought was that if DASH or WoL are enabled then we might miss something on the bus if not waking from L1 in time. You think this shouldn't be a problem? By the way, because I'm no DASH expert: Should we go to D3 at all if DASH is enabled? Will it still work? > > static void rtl_prepare_power_down(struct rtl8169_private *tp) { > rtl_disable_exit_l1(tp); > > if (tp->dash_type != RTL_DASH_NONE) > return; > > if (tp->mac_version == RTL_GIGA_MAC_VER_32 || > tp->mac_version == RTL_GIGA_MAC_VER_33) > rtl_ephy_write(tp, 0x19, 0xff64); > > if (device_may_wakeup(tp_to_dev(tp))) { > phy_speed_down(tp->phydev, false); > rtl_wol_enable_rx(tp); > } > } > > Thanks. >> static void rtl8169_down(struct rtl8169_private *tp) { >> /* Clear all task flags */ >> -- >> 2.35.0 >> >> ------Please consider the environment before printing this e-mail.
> On 28.01.2022 09:09, Hau wrote: > > > > > >> -----Original Message----- > >> From: Heiner Kallweit [mailto:hkallweit1@gmail.com] > >> Sent: Friday, January 28, 2022 6:15 AM > >> To: Jakub Kicinski <kuba@kernel.org>; David Miller > >> <davem@davemloft.net>; nic_swsd <nic_swsd@realtek.com> > >> Cc: netdev@vger.kernel.org; Hau <hau@realtek.com> > >> Subject: [PATCH net-next] r8169: add rtl_disable_exit_l1() > >> > >> Add rtl_disable_exit_l1() for ensuring that the chip doesn't > >> inadvertently exit ASPM L1 when being in a low-power mode. > >> The new function is called from rtl_prepare_power_down() which has to > >> be moved in the code to avoid a forward declaration. > >> > >> According to Realtek OCP register 0xc0ac shadows ERI register 0xd4 on > >> RTL8168 versions from RTL8168g. This allows to simplify the code a little. > >> > >> Suggested-by: Chun-Hao Lin <hau@realtek.com> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> drivers/net/ethernet/realtek/r8169_main.c | 65 > >> ++++++++++++++--------- > >> 1 file changed, 39 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c > >> b/drivers/net/ethernet/realtek/r8169_main.c > >> index 3c3d1506b..104ebc0fb 100644 > >> --- a/drivers/net/ethernet/realtek/r8169_main.c > >> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >> @@ -2231,28 +2231,6 @@ static int rtl_set_mac_address(struct > >> net_device *dev, void *p) > >> return 0; > >> } > >> > >> -static void rtl_wol_enable_rx(struct rtl8169_private *tp) -{ > >> - if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > >> - RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > >> - AcceptBroadcast | AcceptMulticast | AcceptMyPhys); > >> -} > >> - > >> -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{ > >> - if (tp->dash_type != RTL_DASH_NONE) > >> - return; > >> - > >> - if (tp->mac_version == RTL_GIGA_MAC_VER_32 || > >> - tp->mac_version == RTL_GIGA_MAC_VER_33) > >> - rtl_ephy_write(tp, 0x19, 0xff64); > >> - > >> - if (device_may_wakeup(tp_to_dev(tp))) { > >> - phy_speed_down(tp->phydev, false); > >> - rtl_wol_enable_rx(tp); > >> - } > >> -} > >> - > >> static void rtl_init_rxcfg(struct rtl8169_private *tp) { > >> switch (tp->mac_version) { > >> @@ -2667,10 +2645,7 @@ static void rtl_enable_exit_l1(struct > >> rtl8169_private *tp) > >> case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38: > >> rtl_eri_set_bits(tp, 0xd4, 0x0c00); > >> break; > >> - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53: > >> - rtl_eri_set_bits(tp, 0xd4, 0x1f80); > >> - break; > >> - case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63: > >> + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63: > >> r8168_mac_ocp_modify(tp, 0xc0ac, 0, 0x1f80); > >> break; > >> default: > >> @@ -2678,6 +2653,20 @@ static void rtl_enable_exit_l1(struct > >> rtl8169_private *tp) > >> } > >> } > >> > >> +static void rtl_disable_exit_l1(struct rtl8169_private *tp) { > >> + switch (tp->mac_version) { > >> + case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_38: > >> + rtl_eri_clear_bits(tp, 0xd4, 0x1f00); > >> + break; > >> + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63: > >> + r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0); > >> + break; > >> + default: > >> + break; > >> + } > >> +} > >> + > >> static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, > >> bool > >> enable) { > >> /* Don't enable ASPM in the chip if OS can't control ASPM */ @@ - > >> 4689,6 +4678,30 @@ static int r8169_phy_connect(struct rtl8169_private > *tp) > >> return 0; > >> } > >> > >> +static void rtl_wol_enable_rx(struct rtl8169_private *tp) { > >> + if (tp->mac_version >= RTL_GIGA_MAC_VER_25) > >> + RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | > >> + AcceptBroadcast | AcceptMulticast | AcceptMyPhys); } > >> + > >> +static void rtl_prepare_power_down(struct rtl8169_private *tp) { > >> + if (tp->dash_type != RTL_DASH_NONE) > >> + return; > >> + > >> + if (tp->mac_version == RTL_GIGA_MAC_VER_32 || > >> + tp->mac_version == RTL_GIGA_MAC_VER_33) > >> + rtl_ephy_write(tp, 0x19, 0xff64); > >> + > >> + if (device_may_wakeup(tp_to_dev(tp))) { > >> + phy_speed_down(tp->phydev, false); > >> + rtl_wol_enable_rx(tp); > >> + } else { > >> + rtl_disable_exit_l1(tp); > >> + } > >> +} > >> + > > Hi Heiner, > > > Hi Hau, > > > rtl_disable_exit_l1(tp) can be called before device enter D3 state. I think > you don’t need to check wol status. > > You may update the code link following. > > my thought was that if DASH or WoL are enabled then we might miss > something on the bus if not waking from L1 in time. > You think this shouldn't be a problem? > I think it should not be a problem. If driver does not call rtl_disable_exit_l1() when wol is enabled, then hardware will not go to L1. > By the way, because I'm no DASH expert: > Should we go to D3 at all if DASH is enabled? Will it still work? > For hardware that support dash, dash will work in D3 state. Before D3 state, driver will give control back to dash firmware by call rtl8168_driver_stop(). > > > > static void rtl_prepare_power_down(struct rtl8169_private *tp) { > > rtl_disable_exit_l1(tp); > > > > if (tp->dash_type != RTL_DASH_NONE) > > return; > > > > if (tp->mac_version == RTL_GIGA_MAC_VER_32 || > > tp->mac_version == RTL_GIGA_MAC_VER_33) > > rtl_ephy_write(tp, 0x19, 0xff64); > > > > if (device_may_wakeup(tp_to_dev(tp))) { > > phy_speed_down(tp->phydev, false); > > rtl_wol_enable_rx(tp); > > } > > } > > > > Thanks. > >> static void rtl8169_down(struct rtl8169_private *tp) { > >> /* Clear all task flags */ > >> -- > >> 2.35.0 > >> > >> ------Please consider the environment before printing this e-mail.
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 3c3d1506b..104ebc0fb 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -2231,28 +2231,6 @@ static int rtl_set_mac_address(struct net_device *dev, void *p) return 0; } -static void rtl_wol_enable_rx(struct rtl8169_private *tp) -{ - if (tp->mac_version >= RTL_GIGA_MAC_VER_25) - RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | - AcceptBroadcast | AcceptMulticast | AcceptMyPhys); -} - -static void rtl_prepare_power_down(struct rtl8169_private *tp) -{ - if (tp->dash_type != RTL_DASH_NONE) - return; - - if (tp->mac_version == RTL_GIGA_MAC_VER_32 || - tp->mac_version == RTL_GIGA_MAC_VER_33) - rtl_ephy_write(tp, 0x19, 0xff64); - - if (device_may_wakeup(tp_to_dev(tp))) { - phy_speed_down(tp->phydev, false); - rtl_wol_enable_rx(tp); - } -} - static void rtl_init_rxcfg(struct rtl8169_private *tp) { switch (tp->mac_version) { @@ -2667,10 +2645,7 @@ static void rtl_enable_exit_l1(struct rtl8169_private *tp) case RTL_GIGA_MAC_VER_37 ... RTL_GIGA_MAC_VER_38: rtl_eri_set_bits(tp, 0xd4, 0x0c00); break; - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_53: - rtl_eri_set_bits(tp, 0xd4, 0x1f80); - break; - case RTL_GIGA_MAC_VER_60 ... RTL_GIGA_MAC_VER_63: + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63: r8168_mac_ocp_modify(tp, 0xc0ac, 0, 0x1f80); break; default: @@ -2678,6 +2653,20 @@ static void rtl_enable_exit_l1(struct rtl8169_private *tp) } } +static void rtl_disable_exit_l1(struct rtl8169_private *tp) +{ + switch (tp->mac_version) { + case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_38: + rtl_eri_clear_bits(tp, 0xd4, 0x1f00); + break; + case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_63: + r8168_mac_ocp_modify(tp, 0xc0ac, 0x1f80, 0); + break; + default: + break; + } +} + static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) { /* Don't enable ASPM in the chip if OS can't control ASPM */ @@ -4689,6 +4678,30 @@ static int r8169_phy_connect(struct rtl8169_private *tp) return 0; } +static void rtl_wol_enable_rx(struct rtl8169_private *tp) +{ + if (tp->mac_version >= RTL_GIGA_MAC_VER_25) + RTL_W32(tp, RxConfig, RTL_R32(tp, RxConfig) | + AcceptBroadcast | AcceptMulticast | AcceptMyPhys); +} + +static void rtl_prepare_power_down(struct rtl8169_private *tp) +{ + if (tp->dash_type != RTL_DASH_NONE) + return; + + if (tp->mac_version == RTL_GIGA_MAC_VER_32 || + tp->mac_version == RTL_GIGA_MAC_VER_33) + rtl_ephy_write(tp, 0x19, 0xff64); + + if (device_may_wakeup(tp_to_dev(tp))) { + phy_speed_down(tp->phydev, false); + rtl_wol_enable_rx(tp); + } else { + rtl_disable_exit_l1(tp); + } +} + static void rtl8169_down(struct rtl8169_private *tp) { /* Clear all task flags */
Add rtl_disable_exit_l1() for ensuring that the chip doesn't inadvertently exit ASPM L1 when being in a low-power mode. The new function is called from rtl_prepare_power_down() which has to be moved in the code to avoid a forward declaration. According to Realtek OCP register 0xc0ac shadows ERI register 0xd4 on RTL8168 versions from RTL8168g. This allows to simplify the code a little. Suggested-by: Chun-Hao Lin <hau@realtek.com> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 65 ++++++++++++++--------- 1 file changed, 39 insertions(+), 26 deletions(-)