* [PATCH net-next 00/10] r8169: add phylib support
@ 2018-07-02 19:29 Heiner Kallweit
2018-07-02 19:36 ` [PATCH net-next 01/10] r8169: add basic " Heiner Kallweit
` (9 more replies)
0 siblings, 10 replies; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:29 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
Now that all the basic refactoring has been done we can add phylib
support. This patch series was successfully tested on:
RTL8168h
RTL8168evl
RTL8169sb
Heiner Kallweit (10):
r8169: add basic phylib support
r8169: use phy_resume/phy_suspend
r8169: replace open-coded PHY soft reset with genphy_soft_reset
r8169: use phy_ethtool_(g|s)et_link_ksettings
r8169: use phy_ethtool_nway_reset
r8169: use phy_mii_ioctl
r8169: migrate speed_down function to phylib
r8169: remove rtl8169_set_speed_xmii
r8169: remove mii_if_info member from struct rtl8169_private
r8169: don't read chip phy status register
drivers/net/ethernet/realtek/Kconfig | 2 +-
drivers/net/ethernet/realtek/r8169.c | 454 +++++++++------------------
2 files changed, 152 insertions(+), 304 deletions(-)
--
2.18.0
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH net-next 01/10] r8169: add basic phylib support
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
@ 2018-07-02 19:36 ` Heiner Kallweit
2018-07-02 21:02 ` Andrew Lunn
2018-07-02 19:36 ` [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend Heiner Kallweit
` (8 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:36 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
Add basic phylib support to r8169. All now unneeded old PHY handling code
will be removed in subsequent patches.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/Kconfig | 1 +
drivers/net/ethernet/realtek/r8169.c | 146 +++++++++++++++++++++------
2 files changed, 115 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 7c69f4c8..7fb1af1f 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -99,6 +99,7 @@ config R8169
depends on PCI
select FW_LOADER
select CRC32
+ select PHYLIB
select MII
---help---
Say Y here if you have a Realtek 8169 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f80ac894..7443b230 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -16,6 +16,7 @@
#include <linux/delay.h>
#include <linux/ethtool.h>
#include <linux/mii.h>
+#include <linux/phy.h>
#include <linux/if_vlan.h>
#include <linux/crc32.h>
#include <linux/in.h>
@@ -754,6 +755,7 @@ struct rtl8169_private {
} wk;
struct mii_if_info mii;
+ struct mii_bus *mii_bus;
dma_addr_t counters_phys_addr;
struct rtl8169_counters *counters;
struct rtl8169_tc_offsets tc_offset;
@@ -1444,11 +1446,6 @@ static unsigned int rtl8169_xmii_reset_pending(struct rtl8169_private *tp)
return rtl_readphy(tp, MII_BMCR) & BMCR_RESET;
}
-static unsigned int rtl8169_xmii_link_ok(struct rtl8169_private *tp)
-{
- return RTL_R8(tp, PHYstatus) & LinkStatus;
-}
-
static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp)
{
unsigned int val;
@@ -1513,25 +1510,6 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
}
}
-static void rtl8169_check_link_status(struct net_device *dev,
- struct rtl8169_private *tp)
-{
- struct device *d = tp_to_dev(tp);
-
- if (rtl8169_xmii_link_ok(tp)) {
- rtl_link_chg_patch(tp);
- /* This is to cancel a scheduled suspend if there's one. */
- pm_request_resume(d);
- netif_carrier_on(dev);
- if (net_ratelimit())
- netif_info(tp, ifup, dev, "link up\n");
- } else {
- netif_carrier_off(dev);
- netif_info(tp, ifdown, dev, "link down\n");
- pm_runtime_idle(d);
- }
-}
-
#define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST)
/* Currently we only enable WoL if explicitly told by userspace to circumvent
@@ -6228,7 +6206,6 @@ static void rtl_reset_work(struct rtl8169_private *tp)
napi_enable(&tp->napi);
rtl_hw_start(tp);
netif_wake_queue(dev);
- rtl8169_check_link_status(dev, tp);
}
static void rtl8169_tx_timeout(struct net_device *dev)
@@ -6845,7 +6822,7 @@ static void rtl_slow_event_work(struct rtl8169_private *tp)
rtl8169_pcierr_interrupt(dev);
if (status & LinkChg)
- rtl8169_check_link_status(dev, tp);
+ phy_mac_interrupt(dev->phydev);
rtl_irq_enable_all(tp);
}
@@ -6927,10 +6904,53 @@ static void rtl8169_rx_missed(struct net_device *dev)
RTL_W32(tp, RxMissed, 0);
}
+static void r8169_phylink_handler(struct net_device *ndev)
+{
+ struct rtl8169_private *tp = netdev_priv(ndev);
+
+ if (netif_carrier_ok(ndev)) {
+ rtl_link_chg_patch(tp);
+ pm_request_resume(&tp->pci_dev->dev);
+ } else {
+ pm_runtime_idle(&tp->pci_dev->dev);
+ }
+
+ if (net_ratelimit())
+ phy_print_status(ndev->phydev);
+}
+
+static int r8169_phy_connect(struct rtl8169_private *tp)
+{
+ struct phy_device *phydev;
+ phy_interface_t phy_mode;
+ int ret;
+
+ phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+ PHY_INTERFACE_MODE_MII;
+
+ phydev = mdiobus_get_phy(tp->mii_bus, 0);
+ if (!phydev)
+ return -ENODEV;
+
+ if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+ netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
+ phy_set_max_speed(phydev, SPEED_100);
+ }
+
+ ret = phy_connect_direct(tp->dev, phydev, r8169_phylink_handler,
+ phy_mode);
+ if (!ret)
+ phy_attached_info(phydev);
+
+ return ret;
+}
+
static void rtl8169_down(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
+ phy_stop(dev->phydev);
+
napi_disable(&tp->napi);
netif_stop_queue(dev);
@@ -6970,6 +6990,8 @@ static int rtl8169_close(struct net_device *dev)
cancel_work_sync(&tp->wk.work);
+ phy_disconnect(dev->phydev);
+
pci_free_irq(pdev, 0, tp);
dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
@@ -7030,6 +7052,10 @@ static int rtl_open(struct net_device *dev)
if (retval < 0)
goto err_release_fw_2;
+ retval = r8169_phy_connect(tp);
+ if (retval)
+ goto err_free_irq;
+
rtl_lock_work(tp);
set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -7045,16 +7071,17 @@ static int rtl_open(struct net_device *dev)
if (!rtl8169_init_counter_offsets(tp))
netif_warn(tp, hw, dev, "counter reset/update failed\n");
+ phy_start(dev->phydev);
netif_start_queue(dev);
rtl_unlock_work(tp);
pm_runtime_put_sync(&pdev->dev);
-
- rtl8169_check_link_status(dev, tp);
out:
return retval;
+err_free_irq:
+ pci_free_irq(pdev, 0, tp);
err_release_fw_2:
rtl_release_firmware(tp);
rtl8169_rx_clear(tp);
@@ -7133,6 +7160,7 @@ static void rtl8169_net_suspend(struct net_device *dev)
if (!netif_running(dev))
return;
+ phy_stop(dev->phydev);
netif_device_detach(dev);
netif_stop_queue(dev);
@@ -7165,6 +7193,8 @@ static void __rtl8169_resume(struct net_device *dev)
rtl_pll_power_up(tp);
rtl8169_init_phy(dev, tp);
+ phy_start(tp->dev->phydev);
+
rtl_lock_work(tp);
napi_enable(&tp->napi);
set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -7310,6 +7340,7 @@ static void rtl_remove_one(struct pci_dev *pdev)
netif_napi_del(&tp->napi);
unregister_netdev(dev);
+ mdiobus_unregister(tp->mii_bus);
rtl_release_firmware(tp);
@@ -7395,6 +7426,51 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond)
return (RTL_R8(tp, MCU) & RXTX_EMPTY) == RXTX_EMPTY;
}
+static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
+{
+ struct rtl8169_private *tp = mii_bus->priv;
+
+ return rtl_readphy(tp, phyreg);
+}
+
+static int r8169_mdio_write_reg(struct mii_bus *mii_bus, int phyaddr,
+ int phyreg, u16 val)
+{
+ struct rtl8169_private *tp = mii_bus->priv;
+
+ rtl_writephy(tp, phyreg, val);
+
+ return 0;
+}
+
+static int r8169_mdio_register(struct rtl8169_private *tp)
+{
+ struct pci_dev *pdev = tp->pci_dev;
+ struct mii_bus *new_bus;
+ int ret;
+
+ new_bus = devm_mdiobus_alloc(&pdev->dev);
+ if (!new_bus)
+ return -ENOMEM;
+
+ new_bus->name = "r8169";
+ new_bus->phy_mask = ~1;
+ new_bus->priv = tp;
+ new_bus->parent = &pdev->dev;
+ new_bus->irq[0] = PHY_IGNORE_INTERRUPT;
+ snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x",
+ PCI_DEVID(pdev->bus->number, pdev->devfn));
+
+ new_bus->read = r8169_mdio_read_reg;
+ new_bus->write = r8169_mdio_write_reg;
+
+ ret = mdiobus_register(new_bus);
+ if (!ret)
+ tp->mii_bus = new_bus;
+
+ return ret;
+}
+
static void rtl_hw_init_8168g(struct rtl8169_private *tp)
{
u32 data;
@@ -7651,10 +7727,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
pci_set_drvdata(pdev, dev);
- rc = register_netdev(dev);
- if (rc < 0)
+ rc = r8169_mdio_register(tp);
+ if (rc)
return rc;
+ rc = register_netdev(dev);
+ if (rc)
+ goto err_mdio_unregister;
+
netif_info(tp, probe, dev, "%s, %pM, XID %08x, IRQ %d\n",
rtl_chip_infos[chipset].name, dev->dev_addr,
(u32)(RTL_R32(tp, TxConfig) & 0xfcf0f8ff),
@@ -7669,12 +7749,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
if (r8168_check_dash(tp))
rtl8168_driver_start(tp);
- netif_carrier_off(dev);
-
if (pci_dev_run_wake(pdev))
pm_runtime_put_sync(&pdev->dev);
return 0;
+
+err_mdio_unregister:
+ mdiobus_unregister(tp->mii_bus);
+ return rc;
}
static struct pci_driver rtl8169_pci_driver = {
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
2018-07-02 19:36 ` [PATCH net-next 01/10] r8169: add basic " Heiner Kallweit
@ 2018-07-02 19:36 ` Heiner Kallweit
2018-07-02 21:06 ` Andrew Lunn
2018-07-02 19:36 ` [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset Heiner Kallweit
` (7 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:36 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
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 <hkallweit1@gmail.com>
---
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 7443b230..0fba2581 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4457,47 +4457,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, 0x0000);
- 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, 0x0000);
- 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, 0x0000);
- 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))
@@ -4510,7 +4469,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:
@@ -4563,7 +4523,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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
2018-07-02 19:36 ` [PATCH net-next 01/10] r8169: add basic " Heiner Kallweit
2018-07-02 19:36 ` [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend Heiner Kallweit
@ 2018-07-02 19:36 ` Heiner Kallweit
2018-07-02 21:08 ` Andrew Lunn
2018-07-02 19:37 ` [PATCH net-next 04/10] r8169: use phy_ethtool_(g|s)et_link_ksettings Heiner Kallweit
` (6 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:36 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
Use genphy_soft_reset() instead of open-coding a PHY soft reset. We have
to do an explicit PHY soft reset because some chips use the genphy driver
which uses a no-op as soft_reset callback.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 27 +--------------------------
1 file changed, 1 insertion(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0fba2581..a466647e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1441,19 +1441,6 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
RTL_R8(tp, ChipCmd);
}
-static unsigned int rtl8169_xmii_reset_pending(struct rtl8169_private *tp)
-{
- return rtl_readphy(tp, MII_BMCR) & BMCR_RESET;
-}
-
-static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp)
-{
- unsigned int val;
-
- val = rtl_readphy(tp, MII_BMCR) | BMCR_RESET;
- rtl_writephy(tp, MII_BMCR, val & 0xffff);
-}
-
static void rtl_link_chg_patch(struct rtl8169_private *tp)
{
struct net_device *dev = tp->dev;
@@ -4259,18 +4246,6 @@ static void rtl_schedule_task(struct rtl8169_private *tp, enum rtl_flag flag)
schedule_work(&tp->wk.work);
}
-DECLARE_RTL_COND(rtl_phy_reset_cond)
-{
- return rtl8169_xmii_reset_pending(tp);
-}
-
-static void rtl8169_phy_reset(struct net_device *dev,
- struct rtl8169_private *tp)
-{
- rtl8169_xmii_reset_enable(tp);
- rtl_msleep_loop_wait_low(tp, &rtl_phy_reset_cond, 1, 100);
-}
-
static bool rtl_tbi_enabled(struct rtl8169_private *tp)
{
return (tp->mac_version == RTL_GIGA_MAC_VER_01) &&
@@ -4301,7 +4276,7 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
}
- rtl8169_phy_reset(dev, tp);
+ genphy_soft_reset(dev->phydev);
rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 04/10] r8169: use phy_ethtool_(g|s)et_link_ksettings
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
` (2 preceding siblings ...)
2018-07-02 19:36 ` [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset Heiner Kallweit
@ 2018-07-02 19:37 ` Heiner Kallweit
2018-07-02 19:37 ` [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset Heiner Kallweit
` (5 subsequent siblings)
9 siblings, 0 replies; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
Use phy_ethtool_(g|s)et_link_ksettings() for the respective ethtool_ops
callbacks.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 35 +++-------------------------
1 file changed, 3 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index a466647e..d3a909fb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1816,35 +1816,6 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), swab16(opts2 & 0xffff));
}
-static int rtl8169_get_link_ksettings(struct net_device *dev,
- struct ethtool_link_ksettings *cmd)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- mii_ethtool_get_link_ksettings(&tp->mii, cmd);
-
- return 0;
-}
-
-static int rtl8169_set_link_ksettings(struct net_device *dev,
- const struct ethtool_link_ksettings *cmd)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
- int rc;
- u32 advertising;
-
- if (!ethtool_convert_link_mode_to_legacy_u32(&advertising,
- cmd->link_modes.advertising))
- return -EINVAL;
-
- rtl_lock_work(tp);
- rc = rtl8169_set_speed(dev, cmd->base.autoneg, cmd->base.speed,
- cmd->base.duplex, advertising);
- rtl_unlock_work(tp);
-
- return rc;
-}
-
static void rtl8169_get_regs(struct net_device *dev, struct ethtool_regs *regs,
void *p)
{
@@ -2099,7 +2070,7 @@ static const struct rtl_coalesce_info *rtl_coalesce_info(struct net_device *dev)
const struct rtl_coalesce_info *ci;
int rc;
- rc = rtl8169_get_link_ksettings(dev, &ecmd);
+ rc = phy_ethtool_get_link_ksettings(dev, &ecmd);
if (rc < 0)
return ERR_PTR(rc);
@@ -2258,8 +2229,8 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.get_ethtool_stats = rtl8169_get_ethtool_stats,
.get_ts_info = ethtool_op_get_ts_info,
.nway_reset = rtl8169_nway_reset,
- .get_link_ksettings = rtl8169_get_link_ksettings,
- .set_link_ksettings = rtl8169_set_link_ksettings,
+ .get_link_ksettings = phy_ethtool_get_link_ksettings,
+ .set_link_ksettings = phy_ethtool_set_link_ksettings,
};
static void rtl8169_get_mac_version(struct rtl8169_private *tp,
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
` (3 preceding siblings ...)
2018-07-02 19:37 ` [PATCH net-next 04/10] r8169: use phy_ethtool_(g|s)et_link_ksettings Heiner Kallweit
@ 2018-07-02 19:37 ` Heiner Kallweit
2018-07-02 21:11 ` Andrew Lunn
2018-07-02 19:37 ` [PATCH net-next 06/10] r8169: use phy_mii_ioctl Heiner Kallweit
` (4 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
Switch to using phy_ethtool_nway_reset().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/Kconfig | 1 -
drivers/net/ethernet/realtek/r8169.c | 9 +--------
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 7fb1af1f..e1cd934c 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -100,7 +100,6 @@ config R8169
select FW_LOADER
select CRC32
select PHYLIB
- select MII
---help---
Say Y here if you have a Realtek 8169 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index d3a909fb..6006676b 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1991,13 +1991,6 @@ static void rtl8169_get_strings(struct net_device *dev, u32 stringset, u8 *data)
}
}
-static int rtl8169_nway_reset(struct net_device *dev)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- return mii_nway_restart(&tp->mii);
-}
-
/*
* Interrupt coalescing
*
@@ -2228,7 +2221,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.get_sset_count = rtl8169_get_sset_count,
.get_ethtool_stats = rtl8169_get_ethtool_stats,
.get_ts_info = ethtool_op_get_ts_info,
- .nway_reset = rtl8169_nway_reset,
+ .nway_reset = phy_ethtool_nway_reset,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
};
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 06/10] r8169: use phy_mii_ioctl
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
` (4 preceding siblings ...)
2018-07-02 19:37 ` [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset Heiner Kallweit
@ 2018-07-02 19:37 ` Heiner Kallweit
2018-07-02 21:13 ` Andrew Lunn
2018-07-02 19:37 ` [PATCH net-next 07/10] r8169: migrate speed_down function to phylib Heiner Kallweit
` (3 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
Switch to using phy_mii_ioctl().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 25 +++----------------------
1 file changed, 3 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 6006676b..311321ee 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4290,31 +4290,12 @@ static int rtl_set_mac_address(struct net_device *dev, void *p)
return 0;
}
-static int rtl_xmii_ioctl(struct rtl8169_private *tp,
- struct mii_ioctl_data *data, int cmd)
-{
- switch (cmd) {
- case SIOCGMIIPHY:
- data->phy_id = 32; /* Internal PHY */
- return 0;
-
- case SIOCGMIIREG:
- data->val_out = rtl_readphy(tp, data->reg_num & 0x1f);
- return 0;
-
- case SIOCSMIIREG:
- rtl_writephy(tp, data->reg_num & 0x1f, data->val_in);
- return 0;
- }
- return -EOPNOTSUPP;
-}
-
static int rtl8169_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
- struct rtl8169_private *tp = netdev_priv(dev);
- struct mii_ioctl_data *data = if_mii(ifr);
+ if (!netif_running(dev))
+ return -ENODEV;
- return netif_running(dev) ? rtl_xmii_ioctl(tp, data, cmd) : -ENODEV;
+ return phy_mii_ioctl(dev->phydev, ifr, cmd);
}
static void rtl_init_mdio_ops(struct rtl8169_private *tp)
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
` (5 preceding siblings ...)
2018-07-02 19:37 ` [PATCH net-next 06/10] r8169: use phy_mii_ioctl Heiner Kallweit
@ 2018-07-02 19:37 ` Heiner Kallweit
2018-07-02 21:20 ` Andrew Lunn
2018-07-02 19:37 ` [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii Heiner Kallweit
` (2 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
Change rtl_speed_down() to use phylib.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 311321ee..807fbc75 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
}
+ /* We may have called rtl_speed_down before */
+ dev->phydev->advertising = dev->phydev->supported;
+ genphy_config_aneg(dev->phydev);
+
genphy_soft_reset(dev->phydev);
rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
@@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
}
}
+#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
+#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
+#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
+
static void rtl_speed_down(struct rtl8169_private *tp)
{
- u32 adv;
- int lpa;
+ struct phy_device *phydev = tp->dev->phydev;
+ u32 adv = phydev->lp_advertising & phydev->supported;
- rtl_writephy(tp, 0x1f, 0x0000);
- lpa = rtl_readphy(tp, MII_LPA);
+ if (adv & BASET10)
+ phydev->advertising &= ~(BASET100 | BASET1000);
+ else if (adv & BASET100)
+ phydev->advertising &= ~BASET1000;
- if (lpa & (LPA_10HALF | LPA_10FULL))
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
- else if (lpa & (LPA_100HALF | LPA_100FULL))
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
- else
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
- (tp->mii.supports_gmii ?
- ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full : 0);
-
- rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
- adv);
+ genphy_config_aneg(phydev);
}
static void rtl_wol_suspend_quirk(struct rtl8169_private *tp)
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
` (6 preceding siblings ...)
2018-07-02 19:37 ` [PATCH net-next 07/10] r8169: migrate speed_down function to phylib Heiner Kallweit
@ 2018-07-02 19:37 ` Heiner Kallweit
2018-07-02 21:21 ` Andrew Lunn
2018-07-02 19:37 ` [PATCH net-next 09/10] r8169: remove mii_if_info member from struct rtl8169_private Heiner Kallweit
2018-07-02 19:37 ` [PATCH net-next 10/10] r8169: don't read chip phy status register Heiner Kallweit
9 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
We can remove rtl8169_set_speed_xmii() now that phylib handles all this.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 90 ----------------------------
1 file changed, 90 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 807fbc75..b696b83d 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1670,89 +1670,6 @@ static int rtl8169_get_regs_len(struct net_device *dev)
return R8169_REGS_SIZE;
}
-static int rtl8169_set_speed_xmii(struct net_device *dev,
- u8 autoneg, u16 speed, u8 duplex, u32 adv)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
- int giga_ctrl, bmcr;
- int rc = -EINVAL;
-
- rtl_writephy(tp, 0x1f, 0x0000);
-
- if (autoneg == AUTONEG_ENABLE) {
- int auto_nego;
-
- auto_nego = rtl_readphy(tp, MII_ADVERTISE);
- auto_nego &= ~(ADVERTISE_10HALF | ADVERTISE_10FULL |
- ADVERTISE_100HALF | ADVERTISE_100FULL);
-
- if (adv & ADVERTISED_10baseT_Half)
- auto_nego |= ADVERTISE_10HALF;
- if (adv & ADVERTISED_10baseT_Full)
- auto_nego |= ADVERTISE_10FULL;
- if (adv & ADVERTISED_100baseT_Half)
- auto_nego |= ADVERTISE_100HALF;
- if (adv & ADVERTISED_100baseT_Full)
- auto_nego |= ADVERTISE_100FULL;
-
- auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
-
- giga_ctrl = rtl_readphy(tp, MII_CTRL1000);
- giga_ctrl &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
-
- /* The 8100e/8101e/8102e do Fast Ethernet only. */
- if (tp->mii.supports_gmii) {
- if (adv & ADVERTISED_1000baseT_Half)
- giga_ctrl |= ADVERTISE_1000HALF;
- if (adv & ADVERTISED_1000baseT_Full)
- giga_ctrl |= ADVERTISE_1000FULL;
- } else if (adv & (ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full)) {
- netif_info(tp, link, dev,
- "PHY does not support 1000Mbps\n");
- goto out;
- }
-
- bmcr = BMCR_ANENABLE | BMCR_ANRESTART;
-
- rtl_writephy(tp, MII_ADVERTISE, auto_nego);
- rtl_writephy(tp, MII_CTRL1000, giga_ctrl);
- } else {
- if (speed == SPEED_10)
- bmcr = 0;
- else if (speed == SPEED_100)
- bmcr = BMCR_SPEED100;
- else
- goto out;
-
- if (duplex == DUPLEX_FULL)
- bmcr |= BMCR_FULLDPLX;
- }
-
- rtl_writephy(tp, MII_BMCR, bmcr);
-
- if (tp->mac_version == RTL_GIGA_MAC_VER_02 ||
- tp->mac_version == RTL_GIGA_MAC_VER_03) {
- if ((speed == SPEED_100) && (autoneg != AUTONEG_ENABLE)) {
- rtl_writephy(tp, 0x17, 0x2138);
- rtl_writephy(tp, 0x0e, 0x0260);
- } else {
- rtl_writephy(tp, 0x17, 0x2108);
- rtl_writephy(tp, 0x0e, 0x0000);
- }
- }
-
- rc = 0;
-out:
- return rc;
-}
-
-static int rtl8169_set_speed(struct net_device *dev,
- u8 autoneg, u16 speed, u8 duplex, u32 advertising)
-{
- return rtl8169_set_speed_xmii(dev, autoneg, speed, duplex, advertising);
-}
-
static netdev_features_t rtl8169_fix_features(struct net_device *dev,
netdev_features_t features)
{
@@ -4245,13 +4162,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
genphy_config_aneg(dev->phydev);
genphy_soft_reset(dev->phydev);
-
- rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
- ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
- (tp->mii.supports_gmii ?
- ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full : 0));
}
static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 09/10] r8169: remove mii_if_info member from struct rtl8169_private
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
` (7 preceding siblings ...)
2018-07-02 19:37 ` [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii Heiner Kallweit
@ 2018-07-02 19:37 ` Heiner Kallweit
2018-07-02 19:37 ` [PATCH net-next 10/10] r8169: don't read chip phy status register Heiner Kallweit
9 siblings, 0 replies; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
The only remaining usage of the struct mii_if_info member is to store the
information whether the chip is GMII-capable. So we can replace it with
a simple flag.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 38 +++++-----------------------
1 file changed, 7 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index b696b83d..48c0e77c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -15,7 +15,6 @@
#include <linux/etherdevice.h>
#include <linux/delay.h>
#include <linux/ethtool.h>
-#include <linux/mii.h>
#include <linux/phy.h>
#include <linux/if_vlan.h>
#include <linux/crc32.h>
@@ -754,7 +753,7 @@ struct rtl8169_private {
struct work_struct work;
} wk;
- struct mii_if_info mii;
+ unsigned supports_gmii:1;
struct mii_bus *mii_bus;
dma_addr_t counters_phys_addr;
struct rtl8169_counters *counters;
@@ -1106,21 +1105,6 @@ static void rtl_w0w1_phy(struct rtl8169_private *tp, int reg_addr, int p, int m)
rtl_writephy(tp, reg_addr, (val & ~m) | p);
}
-static void rtl_mdio_write(struct net_device *dev, int phy_id, int location,
- int val)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- rtl_writephy(tp, location, val);
-}
-
-static int rtl_mdio_read(struct net_device *dev, int phy_id, int location)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- return rtl_readphy(tp, location);
-}
-
DECLARE_RTL_COND(rtl_ephyar_cond)
{
return RTL_R32(tp, EPHYAR) & EPHYAR_FLAG;
@@ -2253,15 +2237,15 @@ static void rtl8169_get_mac_version(struct rtl8169_private *tp,
"unknown MAC, using family default\n");
tp->mac_version = default_version;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_42) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_42 :
RTL_GIGA_MAC_VER_43;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_45) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_45 :
RTL_GIGA_MAC_VER_47;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_46) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_46 :
RTL_GIGA_MAC_VER_48;
}
@@ -6714,14 +6698,14 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
phy_interface_t phy_mode;
int ret;
- phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+ phy_mode = tp->supports_gmii ? PHY_INTERFACE_MODE_GMII :
PHY_INTERFACE_MODE_MII;
phydev = mdiobus_get_phy(tp->mii_bus, 0);
if (!phydev)
return -ENODEV;
- if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+ if (!tp->supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
phy_set_max_speed(phydev, SPEED_100);
}
@@ -7317,7 +7301,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
struct rtl8169_private *tp;
- struct mii_if_info *mii;
struct net_device *dev;
int chipset, region, i;
int rc;
@@ -7337,14 +7320,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
tp->dev = dev;
tp->pci_dev = pdev;
tp->msg_enable = netif_msg_init(debug.msg_enable, R8169_MSG_DEFAULT);
-
- mii = &tp->mii;
- mii->dev = dev;
- mii->mdio_read = rtl_mdio_read;
- mii->mdio_write = rtl_mdio_write;
- mii->phy_id_mask = 0x1f;
- mii->reg_num_mask = 0x1f;
- mii->supports_gmii = cfg->has_gmii;
+ tp->supports_gmii = cfg->has_gmii;
/* enable device (incl. PCI PM wakeup and hotplug setup) */
rc = pcim_enable_device(pdev);
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net-next 10/10] r8169: don't read chip phy status register
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
` (8 preceding siblings ...)
2018-07-02 19:37 ` [PATCH net-next 09/10] r8169: remove mii_if_info member from struct rtl8169_private Heiner Kallweit
@ 2018-07-02 19:37 ` Heiner Kallweit
9 siblings, 0 replies; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev
Instead of accessing the PHYstatus register we can use the information
phylib stores in the phy_device structure.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 48c0e77c..7b7de596 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1428,18 +1428,19 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
static void rtl_link_chg_patch(struct rtl8169_private *tp)
{
struct net_device *dev = tp->dev;
+ struct phy_device *phydev = dev->phydev;
if (!netif_running(dev))
return;
if (tp->mac_version == RTL_GIGA_MAC_VER_34 ||
tp->mac_version == RTL_GIGA_MAC_VER_38) {
- if (RTL_R8(tp, PHYstatus) & _1000bpsF) {
+ if (phydev->speed == SPEED_1000) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
ERIAR_EXGMAC);
- } else if (RTL_R8(tp, PHYstatus) & _100bps) {
+ } else if (phydev->speed == SPEED_100) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x0000001f,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
@@ -1457,7 +1458,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
ERIAR_EXGMAC);
} else if (tp->mac_version == RTL_GIGA_MAC_VER_35 ||
tp->mac_version == RTL_GIGA_MAC_VER_36) {
- if (RTL_R8(tp, PHYstatus) & _1000bpsF) {
+ if (phydev->speed == SPEED_1000) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
@@ -1469,7 +1470,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
ERIAR_EXGMAC);
}
} else if (tp->mac_version == RTL_GIGA_MAC_VER_37) {
- if (RTL_R8(tp, PHYstatus) & _10bps) {
+ if (phydev->speed == SPEED_10) {
rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x4d02,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_0011, 0x0060,
--
2.18.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 01/10] r8169: add basic phylib support
2018-07-02 19:36 ` [PATCH net-next 01/10] r8169: add basic " Heiner Kallweit
@ 2018-07-02 21:02 ` Andrew Lunn
2018-07-02 21:15 ` Heiner Kallweit
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2018-07-02 21:02 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
> +{
> + struct rtl8169_private *tp = mii_bus->priv;
> +
> + return rtl_readphy(tp, phyreg);
So there is no support for phyaddr?
It would be better to trap the phyaddr which are not supported and
return 0xffff.
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
2018-07-02 19:36 ` [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend Heiner Kallweit
@ 2018-07-02 21:06 ` Andrew Lunn
2018-07-02 21:24 ` Heiner Kallweit
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2018-07-02 21:06 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
> static void r8168_pll_power_down(struct rtl8169_private *tp)
> {
> if (r8168_check_dash(tp))
> @@ -4510,7 +4469,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));
This could do some more explanation. Why would it not be connected?
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset
2018-07-02 19:36 ` [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset Heiner Kallweit
@ 2018-07-02 21:08 ` Andrew Lunn
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Lunn @ 2018-07-02 21:08 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On Mon, Jul 02, 2018 at 09:36:58PM +0200, Heiner Kallweit wrote:
> Use genphy_soft_reset() instead of open-coding a PHY soft reset. We have
> to do an explicit PHY soft reset because some chips use the genphy driver
> which uses a no-op as soft_reset callback.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset
2018-07-02 19:37 ` [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset Heiner Kallweit
@ 2018-07-02 21:11 ` Andrew Lunn
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Lunn @ 2018-07-02 21:11 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On Mon, Jul 02, 2018 at 09:37:03PM +0200, Heiner Kallweit wrote:
> Switch to using phy_ethtool_nway_reset().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 06/10] r8169: use phy_mii_ioctl
2018-07-02 19:37 ` [PATCH net-next 06/10] r8169: use phy_mii_ioctl Heiner Kallweit
@ 2018-07-02 21:13 ` Andrew Lunn
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Lunn @ 2018-07-02 21:13 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On Mon, Jul 02, 2018 at 09:37:05PM +0200, Heiner Kallweit wrote:
> Switch to using phy_mii_ioctl().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 01/10] r8169: add basic phylib support
2018-07-02 21:02 ` Andrew Lunn
@ 2018-07-02 21:15 ` Heiner Kallweit
2018-07-03 16:42 ` Florian Fainelli
0 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 21:15 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On 02.07.2018 23:02, Andrew Lunn wrote:
>> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
>> +{
>> + struct rtl8169_private *tp = mii_bus->priv;
>> +
>> + return rtl_readphy(tp, phyreg);
>
> So there is no support for phyaddr?
>
Right, the chip can access only the one internal PHY, therefore it
doesn't support phyaddr.
> It would be better to trap the phyaddr which are not supported and
> return 0xffff.
>
OK
> Andrew
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
2018-07-02 19:37 ` [PATCH net-next 07/10] r8169: migrate speed_down function to phylib Heiner Kallweit
@ 2018-07-02 21:20 ` Andrew Lunn
2018-07-02 21:31 ` Heiner Kallweit
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2018-07-02 21:20 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
> Change rtl_speed_down() to use phylib.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 311321ee..807fbc75 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
> rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
> }
>
> + /* We may have called rtl_speed_down before */
> + dev->phydev->advertising = dev->phydev->supported;
> + genphy_config_aneg(dev->phydev);
> +
> genphy_soft_reset(dev->phydev);
>
> rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
> }
> }
>
> +#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
> +#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
> +#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
> +
> static void rtl_speed_down(struct rtl8169_private *tp)
> {
> - u32 adv;
> - int lpa;
> + struct phy_device *phydev = tp->dev->phydev;
> + u32 adv = phydev->lp_advertising & phydev->supported;
>
> - rtl_writephy(tp, 0x1f, 0x0000);
> - lpa = rtl_readphy(tp, MII_LPA);
> + if (adv & BASET10)
> + phydev->advertising &= ~(BASET100 | BASET1000);
> + else if (adv & BASET100)
> + phydev->advertising &= ~BASET1000;
>
> - if (lpa & (LPA_10HALF | LPA_10FULL))
> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
> - else if (lpa & (LPA_100HALF | LPA_100FULL))
> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
> - else
> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
> - (tp->mii.supports_gmii ?
> - ADVERTISED_1000baseT_Half |
> - ADVERTISED_1000baseT_Full : 0);
> -
> - rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
> - adv);
> + genphy_config_aneg(phydev);
> }
It probably it is me being too tired, but i don't get what this is
doing? Changing the local advertisement based on what the remote is
advertising. Why?
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
2018-07-02 19:37 ` [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii Heiner Kallweit
@ 2018-07-02 21:21 ` Andrew Lunn
2018-07-02 21:54 ` Heiner Kallweit
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2018-07-02 21:21 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
> - auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
This bit you probably want to keep. The PHY never says it support
Pause. The MAC needs to enable pause if the MAC supports pause.
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
2018-07-02 21:06 ` Andrew Lunn
@ 2018-07-02 21:24 ` Heiner Kallweit
2018-07-03 16:44 ` Florian Fainelli
0 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 21:24 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On 02.07.2018 23:06, Andrew Lunn wrote:
>> static void r8168_pll_power_down(struct rtl8169_private *tp)
>> {
>> if (r8168_check_dash(tp))
>> @@ -4510,7 +4469,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));
>
> This could do some more explanation. Why would it not be connected?
>
The PHY gets connected when the net_device is opened. If a network
port isn't used then it will be runtime-suspended a few seconds after
boot. In this case we call r8168_pll_power_down() with the PHY not
being connected.
> Andrew
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
2018-07-02 21:20 ` Andrew Lunn
@ 2018-07-02 21:31 ` Heiner Kallweit
2018-07-03 16:48 ` Florian Fainelli
0 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 21:31 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On 02.07.2018 23:20, Andrew Lunn wrote:
> On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
>> Change rtl_speed_down() to use phylib.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
>> 1 file changed, 15 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>> index 311321ee..807fbc75 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>> rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
>> }
>>
>> + /* We may have called rtl_speed_down before */
>> + dev->phydev->advertising = dev->phydev->supported;
>> + genphy_config_aneg(dev->phydev);
>> +
>> genphy_soft_reset(dev->phydev);
>>
>> rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
>> }
>> }
>>
>> +#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
>> +#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
>> +#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
>> +
>> static void rtl_speed_down(struct rtl8169_private *tp)
>> {
>> - u32 adv;
>> - int lpa;
>> + struct phy_device *phydev = tp->dev->phydev;
>> + u32 adv = phydev->lp_advertising & phydev->supported;
>>
>> - rtl_writephy(tp, 0x1f, 0x0000);
>> - lpa = rtl_readphy(tp, MII_LPA);
>> + if (adv & BASET10)
>> + phydev->advertising &= ~(BASET100 | BASET1000);
>> + else if (adv & BASET100)
>> + phydev->advertising &= ~BASET1000;
>>
>> - if (lpa & (LPA_10HALF | LPA_10FULL))
>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
>> - else if (lpa & (LPA_100HALF | LPA_100FULL))
>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
>> - else
>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
>> - (tp->mii.supports_gmii ?
>> - ADVERTISED_1000baseT_Half |
>> - ADVERTISED_1000baseT_Full : 0);
>> -
>> - rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>> - adv);
>> + genphy_config_aneg(phydev);
>> }
>
> It probably it is me being too tired, but i don't get what this is
> doing? Changing the local advertisement based on what the remote is
> advertising. Why?
>
It also took me some time to understand what this speed_down is doing.
If we suspend and wait for a WoL packet, then we don't have to burn all
the energy for a GBit connection. Therefore we switch to the lowest
speed supported by chip and link partner. This is done by removing
higher speeds from the advertised modes and restarting an autonego.
> Andrew
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
2018-07-02 21:21 ` Andrew Lunn
@ 2018-07-02 21:54 ` Heiner Kallweit
2018-07-04 14:46 ` Andrew Lunn
0 siblings, 1 reply; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-02 21:54 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On 02.07.2018 23:21, Andrew Lunn wrote:
>> - auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
>
> This bit you probably want to keep. The PHY never says it support
> Pause. The MAC needs to enable pause if the MAC supports pause.
>
Actually I assumed that phylib would do this for me. But:
In phy_probe() first phydev->supported is copied to
phydev->advertising, and only after this both pause flags are added
to phydev->supported. Therefore I think they are not advertised.
Is this intentional? It sounds a little weird to me to add the
pause flags to the supported features per default, but not
advertise them.
Except e.g. we call by chance phy_set_max_speed(), which copies
phydev->supported to phydev->advertising after having adjusted
the supported speeds.
If this is not a bug, then where would be the right place to add
the pause flags to phydev->advertising?
> Andrew
>
Heiner
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 01/10] r8169: add basic phylib support
2018-07-02 21:15 ` Heiner Kallweit
@ 2018-07-03 16:42 ` Florian Fainelli
2018-07-03 19:48 ` Heiner Kallweit
0 siblings, 1 reply; 30+ messages in thread
From: Florian Fainelli @ 2018-07-03 16:42 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers, netdev
On 07/02/2018 02:15 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:02, Andrew Lunn wrote:
>>> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
>>> +{
>>> + struct rtl8169_private *tp = mii_bus->priv;
>>> +
>>> + return rtl_readphy(tp, phyreg);
>>
>> So there is no support for phyaddr?
>>
> Right, the chip can access only the one internal PHY, therefore it
> doesn't support phyaddr.
Then you might also want to set mii_bus->phy_mask accordingly such that
only the internal PHY address bit is cleared there?
--
Florian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
2018-07-02 21:24 ` Heiner Kallweit
@ 2018-07-03 16:44 ` Florian Fainelli
2018-07-03 19:54 ` Heiner Kallweit
0 siblings, 1 reply; 30+ messages in thread
From: Florian Fainelli @ 2018-07-03 16:44 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers, netdev
On 07/02/2018 02:24 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:06, Andrew Lunn wrote:
>>> static void r8168_pll_power_down(struct rtl8169_private *tp)
>>> {
>>> if (r8168_check_dash(tp))
>>> @@ -4510,7 +4469,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));
>>
>> This could do some more explanation. Why would it not be connected?
>>
> The PHY gets connected when the net_device is opened. If a network
> port isn't used then it will be runtime-suspended a few seconds after
> boot. In this case we call r8168_pll_power_down() with the PHY not
> being connected.
Would not the !netif_running() check in rtl8169_net_suspend() take care
of that though?
--
Florian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
2018-07-02 21:31 ` Heiner Kallweit
@ 2018-07-03 16:48 ` Florian Fainelli
2018-07-09 21:04 ` Heiner Kallweit
0 siblings, 1 reply; 30+ messages in thread
From: Florian Fainelli @ 2018-07-03 16:48 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers, netdev
On 07/02/2018 02:31 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:20, Andrew Lunn wrote:
>> On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
>>> Change rtl_speed_down() to use phylib.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
>>> 1 file changed, 15 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>> index 311321ee..807fbc75 100644
>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>> rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
>>> }
>>>
>>> + /* We may have called rtl_speed_down before */
>>> + dev->phydev->advertising = dev->phydev->supported;
>>> + genphy_config_aneg(dev->phydev);
>>> +
>>> genphy_soft_reset(dev->phydev);
>>>
>>> rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
>>> }
>>> }
>>>
>>> +#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
>>> +#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
>>> +#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
>>> +
>>> static void rtl_speed_down(struct rtl8169_private *tp)
>>> {
>>> - u32 adv;
>>> - int lpa;
>>> + struct phy_device *phydev = tp->dev->phydev;
>>> + u32 adv = phydev->lp_advertising & phydev->supported;
>>>
>>> - rtl_writephy(tp, 0x1f, 0x0000);
>>> - lpa = rtl_readphy(tp, MII_LPA);
>>> + if (adv & BASET10)
>>> + phydev->advertising &= ~(BASET100 | BASET1000);
>>> + else if (adv & BASET100)
>>> + phydev->advertising &= ~BASET1000;
>>>
>>> - if (lpa & (LPA_10HALF | LPA_10FULL))
>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
>>> - else if (lpa & (LPA_100HALF | LPA_100FULL))
>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
>>> - else
>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
>>> - (tp->mii.supports_gmii ?
>>> - ADVERTISED_1000baseT_Half |
>>> - ADVERTISED_1000baseT_Full : 0);
>>> -
>>> - rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>> - adv);
>>> + genphy_config_aneg(phydev);
>>> }
>>
>> It probably it is me being too tired, but i don't get what this is
>> doing? Changing the local advertisement based on what the remote is
>> advertising. Why?
>>
> It also took me some time to understand what this speed_down is doing.
> If we suspend and wait for a WoL packet, then we don't have to burn all
> the energy for a GBit connection. Therefore we switch to the lowest
> speed supported by chip and link partner. This is done by removing
> higher speeds from the advertised modes and restarting an autonego.
This is something that the tg3 driver also does, we should probably
consider doing this as part of a generic PHY library helpers since I was
told by several HW engineers that usually 10Mbits for WoL is much more
energy efficient.
One thing that bothers me a bit is that this should ideally be offered
as both blocking and non-blocking options, because we might want to make
sure that at the time we suspend, and we already had a link established,
we successfully re-negotiate the link with the partner. I agree that
there could be any sort of link disruption happening at any point though..
--
Florian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 01/10] r8169: add basic phylib support
2018-07-03 16:42 ` Florian Fainelli
@ 2018-07-03 19:48 ` Heiner Kallweit
0 siblings, 0 replies; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-03 19:48 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers, netdev
On 03.07.2018 18:42, Florian Fainelli wrote:
>
>
> On 07/02/2018 02:15 PM, Heiner Kallweit wrote:
>> On 02.07.2018 23:02, Andrew Lunn wrote:
>>>> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
>>>> +{
>>>> + struct rtl8169_private *tp = mii_bus->priv;
>>>> +
>>>> + return rtl_readphy(tp, phyreg);
>>>
>>> So there is no support for phyaddr?
>>>
>> Right, the chip can access only the one internal PHY, therefore it
>> doesn't support phyaddr.
>
> Then you might also want to set mii_bus->phy_mask accordingly such that
> only the internal PHY address bit is cleared there?
>
That's something I'm doing already, see following line in r8169_mdio_register():
new_bus->phy_mask = ~1;
But thanks for the hint anyway.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
2018-07-03 16:44 ` Florian Fainelli
@ 2018-07-03 19:54 ` Heiner Kallweit
0 siblings, 0 replies; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-03 19:54 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers, netdev
On 03.07.2018 18:44, Florian Fainelli wrote:
>
>
> On 07/02/2018 02:24 PM, Heiner Kallweit wrote:
>> On 02.07.2018 23:06, Andrew Lunn wrote:
>>>> static void r8168_pll_power_down(struct rtl8169_private *tp)
>>>> {
>>>> if (r8168_check_dash(tp))
>>>> @@ -4510,7 +4469,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));
>>>
>>> This could do some more explanation. Why would it not be connected?
>>>
>> The PHY gets connected when the net_device is opened. If a network
>> port isn't used then it will be runtime-suspended a few seconds after
>> boot. In this case we call r8168_pll_power_down() with the PHY not
>> being connected.
>
> Would not the !netif_running() check in rtl8169_net_suspend() take care
> of that though?
>
We don't come that far ..
If the interface is down then tp->TxDescArray is NULL. Means we call
rtl_pll_power_down() in rtl8169_runtime_suspend() before reaching
rtl8169_net_suspend().
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
2018-07-02 21:54 ` Heiner Kallweit
@ 2018-07-04 14:46 ` Andrew Lunn
2018-07-04 18:13 ` Heiner Kallweit
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Lunn @ 2018-07-04 14:46 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On Mon, Jul 02, 2018 at 11:54:54PM +0200, Heiner Kallweit wrote:
> On 02.07.2018 23:21, Andrew Lunn wrote:
> >> - auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
> >
> > This bit you probably want to keep. The PHY never says it support
> > Pause. The MAC needs to enable pause if the MAC supports pause.
> >
> Actually I assumed that phylib would do this for me. But:
> In phy_probe() first phydev->supported is copied to
> phydev->advertising, and only after this both pause flags are added
> to phydev->supported. Therefore I think they are not advertised.
> Is this intentional? It sounds a little weird to me to add the
> pause flags to the supported features per default, but not
> advertise them.
phylib has no idea if the MAC supports Pause. So it should not enable
it by default. The MAC needs to enable it. And a lot of MAC drivers
get this wrong...
> Except e.g. we call by chance phy_set_max_speed(), which copies
> phydev->supported to phydev->advertising after having adjusted
> the supported speeds.
As you correctly pointed out, phy_set_max_speed() is masking out too
much.
> If this is not a bug, then where would be the right place to add
> the pause flags to phydev->advertising?
Before you call phy_start().
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
2018-07-04 14:46 ` Andrew Lunn
@ 2018-07-04 18:13 ` Heiner Kallweit
0 siblings, 0 replies; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-04 18:13 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers, netdev
On 04.07.2018 16:46, Andrew Lunn wrote:
> On Mon, Jul 02, 2018 at 11:54:54PM +0200, Heiner Kallweit wrote:
>> On 02.07.2018 23:21, Andrew Lunn wrote:
>>>> - auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
>>>
>>> This bit you probably want to keep. The PHY never says it support
>>> Pause. The MAC needs to enable pause if the MAC supports pause.
>>>
>> Actually I assumed that phylib would do this for me. But:
>> In phy_probe() first phydev->supported is copied to
>> phydev->advertising, and only after this both pause flags are added
>> to phydev->supported. Therefore I think they are not advertised.
>> Is this intentional? It sounds a little weird to me to add the
>> pause flags to the supported features per default, but not
>> advertise them.
>
> phylib has no idea if the MAC supports Pause. So it should not enable
> it by default. The MAC needs to enable it. And a lot of MAC drivers
> get this wrong...
>
>> Except e.g. we call by chance phy_set_max_speed(), which copies
>> phydev->supported to phydev->advertising after having adjusted
>> the supported speeds.
>
> As you correctly pointed out, phy_set_max_speed() is masking out too
> much.
>
>> If this is not a bug, then where would be the right place to add
>> the pause flags to phydev->advertising?
>
> Before you call phy_start().
>
Thanks for the clarification. I think beginning of next week I can
provide a v2 of the patch series.
Heiner
> Andrew
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
2018-07-03 16:48 ` Florian Fainelli
@ 2018-07-09 21:04 ` Heiner Kallweit
0 siblings, 0 replies; 30+ messages in thread
From: Heiner Kallweit @ 2018-07-09 21:04 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers, netdev
On 03.07.2018 18:48, Florian Fainelli wrote:
>
>
> On 07/02/2018 02:31 PM, Heiner Kallweit wrote:
>> On 02.07.2018 23:20, Andrew Lunn wrote:
>>> On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
>>>> Change rtl_speed_down() to use phylib.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
>>>> 1 file changed, 15 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>>> index 311321ee..807fbc75 100644
>>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>>> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>>> rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
>>>> }
>>>>
>>>> + /* We may have called rtl_speed_down before */
>>>> + dev->phydev->advertising = dev->phydev->supported;
>>>> + genphy_config_aneg(dev->phydev);
>>>> +
>>>> genphy_soft_reset(dev->phydev);
>>>>
>>>> rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>>> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
>>>> }
>>>> }
>>>>
>>>> +#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
>>>> +#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
>>>> +#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
>>>> +
>>>> static void rtl_speed_down(struct rtl8169_private *tp)
>>>> {
>>>> - u32 adv;
>>>> - int lpa;
>>>> + struct phy_device *phydev = tp->dev->phydev;
>>>> + u32 adv = phydev->lp_advertising & phydev->supported;
>>>>
>>>> - rtl_writephy(tp, 0x1f, 0x0000);
>>>> - lpa = rtl_readphy(tp, MII_LPA);
>>>> + if (adv & BASET10)
>>>> + phydev->advertising &= ~(BASET100 | BASET1000);
>>>> + else if (adv & BASET100)
>>>> + phydev->advertising &= ~BASET1000;
>>>>
>>>> - if (lpa & (LPA_10HALF | LPA_10FULL))
>>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
>>>> - else if (lpa & (LPA_100HALF | LPA_100FULL))
>>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
>>>> - else
>>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
>>>> - (tp->mii.supports_gmii ?
>>>> - ADVERTISED_1000baseT_Half |
>>>> - ADVERTISED_1000baseT_Full : 0);
>>>> -
>>>> - rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>>> - adv);
>>>> + genphy_config_aneg(phydev);
>>>> }
>>>
>>> It probably it is me being too tired, but i don't get what this is
>>> doing? Changing the local advertisement based on what the remote is
>>> advertising. Why?
>>>
>> It also took me some time to understand what this speed_down is doing.
>> If we suspend and wait for a WoL packet, then we don't have to burn all
>> the energy for a GBit connection. Therefore we switch to the lowest
>> speed supported by chip and link partner. This is done by removing
>> higher speeds from the advertised modes and restarting an autonego.
>
> This is something that the tg3 driver also does, we should probably
> consider doing this as part of a generic PHY library helpers since I was
> told by several HW engineers that usually 10Mbits for WoL is much more
> energy efficient.
>
Yes, I agree this should become part of phylib. I'd prefer to do it
after this r8169 patch series, will spend a few thoughts on how to
do it best, also considering your remark below.
Heiner
> One thing that bothers me a bit is that this should ideally be offered
> as both blocking and non-blocking options, because we might want to make
> sure that at the time we suspend, and we already had a link established,
> we successfully re-negotiate the link with the partner. I agree that
> there could be any sort of link disruption happening at any point though..
>
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2018-07-09 21:10 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-02 19:29 [PATCH net-next 00/10] r8169: add phylib support Heiner Kallweit
2018-07-02 19:36 ` [PATCH net-next 01/10] r8169: add basic " Heiner Kallweit
2018-07-02 21:02 ` Andrew Lunn
2018-07-02 21:15 ` Heiner Kallweit
2018-07-03 16:42 ` Florian Fainelli
2018-07-03 19:48 ` Heiner Kallweit
2018-07-02 19:36 ` [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend Heiner Kallweit
2018-07-02 21:06 ` Andrew Lunn
2018-07-02 21:24 ` Heiner Kallweit
2018-07-03 16:44 ` Florian Fainelli
2018-07-03 19:54 ` Heiner Kallweit
2018-07-02 19:36 ` [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset Heiner Kallweit
2018-07-02 21:08 ` Andrew Lunn
2018-07-02 19:37 ` [PATCH net-next 04/10] r8169: use phy_ethtool_(g|s)et_link_ksettings Heiner Kallweit
2018-07-02 19:37 ` [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset Heiner Kallweit
2018-07-02 21:11 ` Andrew Lunn
2018-07-02 19:37 ` [PATCH net-next 06/10] r8169: use phy_mii_ioctl Heiner Kallweit
2018-07-02 21:13 ` Andrew Lunn
2018-07-02 19:37 ` [PATCH net-next 07/10] r8169: migrate speed_down function to phylib Heiner Kallweit
2018-07-02 21:20 ` Andrew Lunn
2018-07-02 21:31 ` Heiner Kallweit
2018-07-03 16:48 ` Florian Fainelli
2018-07-09 21:04 ` Heiner Kallweit
2018-07-02 19:37 ` [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii Heiner Kallweit
2018-07-02 21:21 ` Andrew Lunn
2018-07-02 21:54 ` Heiner Kallweit
2018-07-04 14:46 ` Andrew Lunn
2018-07-04 18:13 ` Heiner Kallweit
2018-07-02 19:37 ` [PATCH net-next 09/10] r8169: remove mii_if_info member from struct rtl8169_private Heiner Kallweit
2018-07-02 19:37 ` [PATCH net-next 10/10] r8169: don't read chip phy status register Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).