Hello David, Ben, Florian, Chris and everybody,
this series improves ethoc behavior in gigabit environment:
- first patch introduces two phylib setters for 'advertising' and 'supported'
fields of struct phy_device;
- second patch disables gigabit advertisement in the attached PHY making
possible to use gigabit link without any additional setup;
- third patch adds support to set up MII management bus frequency, adding
new fields to platform data and to OF bindings;
- fourth patch adds basic ethtool support to ethoc driver.
These changes allow to use KC-705 board with 50MHz xtensa core and OpenCores
10/100 Mbps MAC connected to gigabit network without any additional setup.
Changes v1->v2:
- new patch "phy: provide accessors for 'advertising' and 'supported' fields";
- disable both gigabit advertisement and support;
- drop MDIO bus frequency configurability, always configure for standard
2.5MHz;
- allow using common clock framework to provide ethoc clock;
- new patch: "net: ethoc: implement ethtool operations";
- drop device tree bindings documentation patch until common bindings format
for network drivers is decided.
Max Filippov (4):
phy: provide accessors for 'advertising' and 'supported' fields
net: ethoc: don't advertise gigabit speed on attached PHY
net: ethoc: set up MII management bus clock
net: ethoc: implement ethtool operations
drivers/net/ethernet/ethoc.c | 130 ++++++++++++++++++++++++++++++++++++++++++-
include/linux/phy.h | 12 ++++
include/net/ethoc.h | 1 +
3 files changed, 141 insertions(+), 2 deletions(-)
--
1.8.1.4
MII management bus clock is derived from the MAC clock by dividing it by
MIIMODER register CLKDIV field value. This value may need to be set up
in case it is undefined or its default value is too high (and
communication with PHY is too slow) or too low (and communication with
PHY is impossible). The value of CLKDIV is not specified directly, but
is derived from the MAC clock for the default MII management bus frequency
of 2.5MHz. The MAC clock may be specified in the platform data, or as
either 'clock-frequency' or 'clocks' device tree attribute.
Signed-off-by: Max Filippov <[email protected]>
---
Changes v1->v2:
- drop MDIO bus frequency configurability, always configure for standard
2.5MHz;
- allow using common clock framework to provide ethoc clock.
drivers/net/ethernet/ethoc.c | 37 +++++++++++++++++++++++++++++++++++--
include/net/ethoc.h | 1 +
2 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index 5643b2d..5854d41 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -13,6 +13,7 @@
#include <linux/dma-mapping.h>
#include <linux/etherdevice.h>
+#include <linux/clk.h>
#include <linux/crc32.h>
#include <linux/interrupt.h>
#include <linux/io.h>
@@ -216,6 +217,7 @@ struct ethoc {
struct phy_device *phy;
struct mii_bus *mdio;
+ struct clk *clk;
s8 phy_id;
};
@@ -925,6 +927,8 @@ static int ethoc_probe(struct platform_device *pdev)
int num_bd;
int ret = 0;
bool random_mac = false;
+ u32 eth_clkfreq = 0;
+ struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
/* allocate networking device */
netdev = alloc_etherdev(sizeof(struct ethoc));
@@ -1038,8 +1042,7 @@ static int ethoc_probe(struct platform_device *pdev)
}
/* Allow the platform setup code to pass in a MAC address. */
- if (dev_get_platdata(&pdev->dev)) {
- struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
+ if (pdata) {
memcpy(netdev->dev_addr, pdata->hwaddr, IFHWADDRLEN);
priv->phy_id = pdata->phy_id;
} else {
@@ -1077,6 +1080,32 @@ static int ethoc_probe(struct platform_device *pdev)
if (random_mac)
netdev->addr_assign_type = NET_ADDR_RANDOM;
+ /* Allow the platform setup code to adjust MII management bus clock. */
+ if (pdata)
+ eth_clkfreq = pdata->eth_clkfreq;
+ else
+ of_property_read_u32(pdev->dev.of_node,
+ "clock-frequency", ð_clkfreq);
+ if (!eth_clkfreq) {
+ struct clk *clk = clk_get(&pdev->dev, NULL);
+
+ if (!IS_ERR(clk)) {
+ priv->clk = clk;
+ clk_prepare_enable(clk);
+ eth_clkfreq = clk_get_rate(clk);
+ }
+ }
+ if (eth_clkfreq) {
+ u32 clkdiv = MIIMODER_CLKDIV(eth_clkfreq / 2500000 + 1);
+
+ if (!clkdiv)
+ clkdiv = 2;
+ dev_dbg(&pdev->dev, "setting MII clkdiv to %u\n", clkdiv);
+ ethoc_write(priv, MIIMODER,
+ (ethoc_read(priv, MIIMODER) & MIIMODER_NOPRE) |
+ clkdiv);
+ }
+
/* register MII bus */
priv->mdio = mdiobus_alloc();
if (!priv->mdio) {
@@ -1141,6 +1170,8 @@ free_mdio:
kfree(priv->mdio->irq);
mdiobus_free(priv->mdio);
free:
+ if (priv->clk)
+ clk_disable_unprepare(priv->clk);
free_netdev(netdev);
out:
return ret;
@@ -1165,6 +1196,8 @@ static int ethoc_remove(struct platform_device *pdev)
kfree(priv->mdio->irq);
mdiobus_free(priv->mdio);
}
+ if (priv->clk)
+ clk_disable_unprepare(priv->clk);
unregister_netdev(netdev);
free_netdev(netdev);
}
diff --git a/include/net/ethoc.h b/include/net/ethoc.h
index 96f3789..2a2d6bb 100644
--- a/include/net/ethoc.h
+++ b/include/net/ethoc.h
@@ -16,6 +16,7 @@
struct ethoc_platform_data {
u8 hwaddr[IFHWADDRLEN];
s8 phy_id;
+ u32 eth_clkfreq;
};
#endif /* !LINUX_NET_ETHOC_H */
--
1.8.1.4
OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
not disable advertisement when PHY supports them. This results in
non-functioning network when the MAC is connected to a gigabit PHY connected
to a gigabit switch.
The fix is to disable gigabit speed advertisement on attached PHY
unconditionally.
Signed-off-by: Max Filippov <[email protected]>
---
Changes v1->v2:
- disable both gigabit advertisement and support.
drivers/net/ethernet/ethoc.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index 4de8cfd..5643b2d 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev)
}
priv->phy = phy;
+ phy_update_advert(phy,
+ ADVERTISED_1000baseT_Full |
+ ADVERTISED_1000baseT_Half, 0);
+ phy_start_aneg(phy);
+ phy_update_supported(phy,
+ SUPPORTED_1000baseT_Full |
+ SUPPORTED_1000baseT_Half, 0);
+
return 0;
}
--
1.8.1.4
The following methods are implemented:
- get/set settings;
- get registers length/registers;
- get link state (standard implementation);
- get/set ring parameters;
- get timestamping info (standard implementation).
Signed-off-by: Max Filippov <[email protected]>
---
Changes v1->v2:
- new patch.
drivers/net/ethernet/ethoc.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 85 insertions(+)
diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index 5854d41..2f8b174 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -52,6 +52,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size");
#define ETH_HASH0 0x48
#define ETH_HASH1 0x4c
#define ETH_TXCTRL 0x50
+#define ETH_END 0x54
/* mode register */
#define MODER_RXEN (1 << 0) /* receive enable */
@@ -180,6 +181,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size");
* @membase: pointer to buffer memory region
* @dma_alloc: dma allocated buffer size
* @io_region_size: I/O memory region size
+ * @num_bd: number of buffer descriptors
* @num_tx: number of send buffers
* @cur_tx: last send buffer written
* @dty_tx: last buffer actually sent
@@ -200,6 +202,7 @@ struct ethoc {
int dma_alloc;
resource_size_t io_region_size;
+ unsigned int num_bd;
unsigned int num_tx;
unsigned int cur_tx;
unsigned int dty_tx;
@@ -900,6 +903,86 @@ out:
return NETDEV_TX_OK;
}
+static int ethoc_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+ struct ethoc *priv = netdev_priv(dev);
+ struct phy_device *phydev = priv->phy;
+
+ if (!phydev)
+ return -ENODEV;
+
+ return phy_ethtool_gset(phydev, cmd);
+}
+
+static int ethoc_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+ struct ethoc *priv = netdev_priv(dev);
+ struct phy_device *phydev = priv->phy;
+
+ if (!phydev)
+ return -ENODEV;
+
+ return phy_ethtool_sset(phydev, cmd);
+}
+
+static int ethoc_get_regs_len(struct net_device *netdev)
+{
+ return ETH_END;
+}
+
+static void ethoc_get_regs(struct net_device *dev, struct ethtool_regs *regs,
+ void *p)
+{
+ struct ethoc *priv = netdev_priv(dev);
+ u32 *regs_buff = p;
+ unsigned i;
+
+ regs->version = 0;
+ for (i = 0; i < ETH_END / sizeof(u32); ++i)
+ regs_buff[i] = ethoc_read(priv, i * sizeof(u32));
+}
+
+static void ethoc_get_ringparam(struct net_device *dev,
+ struct ethtool_ringparam *ring)
+{
+ struct ethoc *priv = netdev_priv(dev);
+
+ ring->rx_max_pending = priv->num_bd;
+ ring->rx_mini_max_pending = 0;
+ ring->rx_jumbo_max_pending = 0;
+ ring->tx_max_pending = priv->num_bd;
+
+ ring->rx_pending = priv->num_rx;
+ ring->rx_mini_pending = 0;
+ ring->rx_jumbo_pending = 0;
+ ring->tx_pending = priv->num_tx;
+}
+
+static int ethoc_set_ringparam(struct net_device *dev,
+ struct ethtool_ringparam *ring)
+{
+ struct ethoc *priv = netdev_priv(dev);
+
+ if (netif_running(dev))
+ return -EBUSY;
+ priv->num_tx = rounddown_pow_of_two(ring->tx_pending);
+ priv->num_rx = priv->num_bd - priv->num_tx;
+ if (priv->num_rx > ring->rx_pending)
+ priv->num_rx = ring->rx_pending;
+ return 0;
+}
+
+const struct ethtool_ops ethoc_ethtool_ops = {
+ .get_settings = ethoc_get_settings,
+ .set_settings = ethoc_set_settings,
+ .get_regs_len = ethoc_get_regs_len,
+ .get_regs = ethoc_get_regs,
+ .get_link = ethtool_op_get_link,
+ .get_ringparam = ethoc_get_ringparam,
+ .set_ringparam = ethoc_set_ringparam,
+ .get_ts_info = ethtool_op_get_ts_info,
+};
+
static const struct net_device_ops ethoc_netdev_ops = {
.ndo_open = ethoc_open,
.ndo_stop = ethoc_stop,
@@ -1028,6 +1111,7 @@ static int ethoc_probe(struct platform_device *pdev)
ret = -ENODEV;
goto error;
}
+ priv->num_bd = num_bd;
/* num_tx must be a power of two */
priv->num_tx = rounddown_pow_of_two(num_bd >> 1);
priv->num_rx = num_bd - priv->num_tx;
@@ -1148,6 +1232,7 @@ static int ethoc_probe(struct platform_device *pdev)
netdev->netdev_ops = ðoc_netdev_ops;
netdev->watchdog_timeo = ETHOC_TIMEOUT;
netdev->features |= 0;
+ netdev->ethtool_ops = ðoc_ethtool_ops;
/* setup NAPI */
netif_napi_add(netdev, &priv->napi, ethoc_poll, 64);
--
1.8.1.4
Many network drivers directly modify phy_device::advertising and
phy_device::supported. Provide accessors to these fields to better
isolate phylib from its users.
Suggested-by: Ben Hutchings <[email protected]>
Signed-off-by: Max Filippov <[email protected]>
---
Changes v1->v2:
- new patch
include/linux/phy.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 48a4dc3..2ae58f8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -559,6 +559,18 @@ static inline int phy_read_status(struct phy_device *phydev) {
return phydev->drv->read_status(phydev);
}
+static inline void phy_update_advert(struct phy_device *phydev, u32 clear,
+ u32 set)
+{
+ phydev->advertising = (phydev->advertising & ~clear) | set;
+}
+
+static inline void phy_update_supported(struct phy_device *phydev, u32 clear,
+ u32 set)
+{
+ phydev->supported = (phydev->supported & ~clear) | set;
+}
+
int genphy_setup_forced(struct phy_device *phydev);
int genphy_restart_aneg(struct phy_device *phydev);
int genphy_config_aneg(struct phy_device *phydev);
--
1.8.1.4
Hi Max,
Le 28/01/2014 22:00, Max Filippov a ?crit :
> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
> not disable advertisement when PHY supports them. This results in
> non-functioning network when the MAC is connected to a gigabit PHY connected
> to a gigabit switch.
>
> The fix is to disable gigabit speed advertisement on attached PHY
> unconditionally.
>
> Signed-off-by: Max Filippov <[email protected]>
> ---
> Changes v1->v2:
> - disable both gigabit advertisement and support.
>
> drivers/net/ethernet/ethoc.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
> index 4de8cfd..5643b2d 100644
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev)
> }
>
> priv->phy = phy;
> + phy_update_advert(phy,
> + ADVERTISED_1000baseT_Full |
> + ADVERTISED_1000baseT_Half, 0);
> + phy_start_aneg(phy);
This does not look necessary, you should not have to call
phy_start_aneg() because the PHY state machine is not yet started, at
best this calls does nothing.
> + phy_update_supported(phy,
> + SUPPORTED_1000baseT_Full |
> + SUPPORTED_1000baseT_Half, 0);
> +
> return 0;
> }
>
>
Le 28/01/2014 22:00, Max Filippov a ?crit :
> The following methods are implemented:
> - get/set settings;
> - get registers length/registers;
> - get link state (standard implementation);
> - get/set ring parameters;
> - get timestamping info (standard implementation).
Ideally you should have one patch per ethtool callback that you
implement just in case something happens to break, only the specific
patch can reverted/referenced.
>
> Signed-off-by: Max Filippov <[email protected]>
> ---
> Changes v1->v2:
> - new patch.
>
> drivers/net/ethernet/ethoc.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 85 insertions(+)
>
> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
> index 5854d41..2f8b174 100644
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> @@ -52,6 +52,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size");
> #define ETH_HASH0 0x48
> #define ETH_HASH1 0x4c
> #define ETH_TXCTRL 0x50
> +#define ETH_END 0x54
>
> /* mode register */
> #define MODER_RXEN (1 << 0) /* receive enable */
> @@ -180,6 +181,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size");
> * @membase: pointer to buffer memory region
> * @dma_alloc: dma allocated buffer size
> * @io_region_size: I/O memory region size
> + * @num_bd: number of buffer descriptors
> * @num_tx: number of send buffers
> * @cur_tx: last send buffer written
> * @dty_tx: last buffer actually sent
> @@ -200,6 +202,7 @@ struct ethoc {
> int dma_alloc;
> resource_size_t io_region_size;
>
> + unsigned int num_bd;
> unsigned int num_tx;
> unsigned int cur_tx;
> unsigned int dty_tx;
> @@ -900,6 +903,86 @@ out:
> return NETDEV_TX_OK;
> }
>
> +static int ethoc_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> + struct phy_device *phydev = priv->phy;
> +
> + if (!phydev)
> + return -ENODEV;
> +
> + return phy_ethtool_gset(phydev, cmd);
> +}
> +
> +static int ethoc_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> + struct phy_device *phydev = priv->phy;
> +
> + if (!phydev)
> + return -ENODEV;
> +
> + return phy_ethtool_sset(phydev, cmd);
> +}
> +
> +static int ethoc_get_regs_len(struct net_device *netdev)
> +{
> + return ETH_END;
> +}
> +
> +static void ethoc_get_regs(struct net_device *dev, struct ethtool_regs *regs,
> + void *p)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> + u32 *regs_buff = p;
> + unsigned i;
> +
> + regs->version = 0;
> + for (i = 0; i < ETH_END / sizeof(u32); ++i)
> + regs_buff[i] = ethoc_read(priv, i * sizeof(u32));
> +}
> +
> +static void ethoc_get_ringparam(struct net_device *dev,
> + struct ethtool_ringparam *ring)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> +
> + ring->rx_max_pending = priv->num_bd;
> + ring->rx_mini_max_pending = 0;
> + ring->rx_jumbo_max_pending = 0;
> + ring->tx_max_pending = priv->num_bd;
> +
> + ring->rx_pending = priv->num_rx;
> + ring->rx_mini_pending = 0;
> + ring->rx_jumbo_pending = 0;
> + ring->tx_pending = priv->num_tx;
> +}
> +
> +static int ethoc_set_ringparam(struct net_device *dev,
> + struct ethtool_ringparam *ring)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> +
> + if (netif_running(dev))
> + return -EBUSY;
> + priv->num_tx = rounddown_pow_of_two(ring->tx_pending);
> + priv->num_rx = priv->num_bd - priv->num_tx;
> + if (priv->num_rx > ring->rx_pending)
> + priv->num_rx = ring->rx_pending;
> + return 0;
> +}
> +
> +const struct ethtool_ops ethoc_ethtool_ops = {
> + .get_settings = ethoc_get_settings,
> + .set_settings = ethoc_set_settings,
> + .get_regs_len = ethoc_get_regs_len,
> + .get_regs = ethoc_get_regs,
> + .get_link = ethtool_op_get_link,
> + .get_ringparam = ethoc_get_ringparam,
> + .set_ringparam = ethoc_set_ringparam,
> + .get_ts_info = ethtool_op_get_ts_info,
> +};
> +
> static const struct net_device_ops ethoc_netdev_ops = {
> .ndo_open = ethoc_open,
> .ndo_stop = ethoc_stop,
> @@ -1028,6 +1111,7 @@ static int ethoc_probe(struct platform_device *pdev)
> ret = -ENODEV;
> goto error;
> }
> + priv->num_bd = num_bd;
> /* num_tx must be a power of two */
> priv->num_tx = rounddown_pow_of_two(num_bd >> 1);
> priv->num_rx = num_bd - priv->num_tx;
> @@ -1148,6 +1232,7 @@ static int ethoc_probe(struct platform_device *pdev)
> netdev->netdev_ops = ðoc_netdev_ops;
> netdev->watchdog_timeo = ETHOC_TIMEOUT;
> netdev->features |= 0;
> + netdev->ethtool_ops = ðoc_ethtool_ops;
>
> /* setup NAPI */
> netif_napi_add(netdev, &priv->napi, ethoc_poll, 64);
>
Le 28/01/2014 22:00, Max Filippov a ?crit :
> MII management bus clock is derived from the MAC clock by dividing it by
> MIIMODER register CLKDIV field value. This value may need to be set up
> in case it is undefined or its default value is too high (and
> communication with PHY is too slow) or too low (and communication with
> PHY is impossible). The value of CLKDIV is not specified directly, but
> is derived from the MAC clock for the default MII management bus frequency
> of 2.5MHz. The MAC clock may be specified in the platform data, or as
> either 'clock-frequency' or 'clocks' device tree attribute.
>
> Signed-off-by: Max Filippov <[email protected]>
> ---
> Changes v1->v2:
> - drop MDIO bus frequency configurability, always configure for standard
> 2.5MHz;
> - allow using common clock framework to provide ethoc clock.
>
> drivers/net/ethernet/ethoc.c | 37 +++++++++++++++++++++++++++++++++++--
> include/net/ethoc.h | 1 +
> 2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
> index 5643b2d..5854d41 100644
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> @@ -13,6 +13,7 @@
>
> #include <linux/dma-mapping.h>
> #include <linux/etherdevice.h>
> +#include <linux/clk.h>
> #include <linux/crc32.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> @@ -216,6 +217,7 @@ struct ethoc {
>
> struct phy_device *phy;
> struct mii_bus *mdio;
> + struct clk *clk;
> s8 phy_id;
> };
>
> @@ -925,6 +927,8 @@ static int ethoc_probe(struct platform_device *pdev)
> int num_bd;
> int ret = 0;
> bool random_mac = false;
> + u32 eth_clkfreq = 0;
> + struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
>
> /* allocate networking device */
> netdev = alloc_etherdev(sizeof(struct ethoc));
> @@ -1038,8 +1042,7 @@ static int ethoc_probe(struct platform_device *pdev)
> }
>
> /* Allow the platform setup code to pass in a MAC address. */
> - if (dev_get_platdata(&pdev->dev)) {
> - struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
> + if (pdata) {
> memcpy(netdev->dev_addr, pdata->hwaddr, IFHWADDRLEN);
> priv->phy_id = pdata->phy_id;
> } else {
> @@ -1077,6 +1080,32 @@ static int ethoc_probe(struct platform_device *pdev)
> if (random_mac)
> netdev->addr_assign_type = NET_ADDR_RANDOM;
>
> + /* Allow the platform setup code to adjust MII management bus clock. */
> + if (pdata)
> + eth_clkfreq = pdata->eth_clkfreq;
Since this is a new member, why not make it a struct clk pointer
directly so you could simplify the code path?
> + else
> + of_property_read_u32(pdev->dev.of_node,
> + "clock-frequency", ð_clkfreq);
> + if (!eth_clkfreq) {
> + struct clk *clk = clk_get(&pdev->dev, NULL);
> +
> + if (!IS_ERR(clk)) {
> + priv->clk = clk;
> + clk_prepare_enable(clk);
> + eth_clkfreq = clk_get_rate(clk);
> + }
> + }
> + if (eth_clkfreq) {
> + u32 clkdiv = MIIMODER_CLKDIV(eth_clkfreq / 2500000 + 1);
> +
> + if (!clkdiv)
> + clkdiv = 2;
> + dev_dbg(&pdev->dev, "setting MII clkdiv to %u\n", clkdiv);
> + ethoc_write(priv, MIIMODER,
> + (ethoc_read(priv, MIIMODER) & MIIMODER_NOPRE) |
> + clkdiv);
> + }
This does look a bit convoluted, and it looks like the clk_get() or
getting the clock-frequency property should boil down to being the same
thing with of_clk_get() as it should resolve all clocks phandles and
fetch their frequencies appropriately.
> +
> /* register MII bus */
> priv->mdio = mdiobus_alloc();
> if (!priv->mdio) {
> @@ -1141,6 +1170,8 @@ free_mdio:
> kfree(priv->mdio->irq);
> mdiobus_free(priv->mdio);
> free:
> + if (priv->clk)
> + clk_disable_unprepare(priv->clk);
This should probbaly be if (!IS_ERR(priv-clk)).
> free_netdev(netdev);
> out:
> return ret;
> @@ -1165,6 +1196,8 @@ static int ethoc_remove(struct platform_device *pdev)
> kfree(priv->mdio->irq);
> mdiobus_free(priv->mdio);
> }
> + if (priv->clk)
> + clk_disable_unprepare(priv->clk);
Same here
> unregister_netdev(netdev);
> free_netdev(netdev);
> }
> diff --git a/include/net/ethoc.h b/include/net/ethoc.h
> index 96f3789..2a2d6bb 100644
> --- a/include/net/ethoc.h
> +++ b/include/net/ethoc.h
> @@ -16,6 +16,7 @@
> struct ethoc_platform_data {
> u8 hwaddr[IFHWADDRLEN];
> s8 phy_id;
> + u32 eth_clkfreq;
> };
>
> #endif /* !LINUX_NET_ETHOC_H */
>
On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli <[email protected]> wrote:
> Hi Max,
>
> Le 28/01/2014 22:00, Max Filippov a écrit :
>
>> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but does
>> not disable advertisement when PHY supports them. This results in
>> non-functioning network when the MAC is connected to a gigabit PHY
>> connected
>> to a gigabit switch.
>>
>> The fix is to disable gigabit speed advertisement on attached PHY
>> unconditionally.
>>
>> Signed-off-by: Max Filippov <[email protected]>
>> ---
>> Changes v1->v2:
>> - disable both gigabit advertisement and support.
>>
>> drivers/net/ethernet/ethoc.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
>> index 4de8cfd..5643b2d 100644
>> --- a/drivers/net/ethernet/ethoc.c
>> +++ b/drivers/net/ethernet/ethoc.c
>> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device *dev)
>> }
>>
>> priv->phy = phy;
>> + phy_update_advert(phy,
>> + ADVERTISED_1000baseT_Full |
>> + ADVERTISED_1000baseT_Half, 0);
>> + phy_start_aneg(phy);
>
>
> This does not look necessary, you should not have to call phy_start_aneg()
> because the PHY state machine is not yet started, at best this calls does
> nothing.
This call actually makes the whole thing work, because otherwise once gigabit
support is cleared from the supported mask genphy_config_advert does not
update gigabit advertisement register, leaving it enabled.
>> + phy_update_supported(phy,
>> + SUPPORTED_1000baseT_Full |
>> + SUPPORTED_1000baseT_Half, 0);
>> +
>> return 0;
>> }
>>
>>
>
--
Thanks.
-- Max
2014-01-28 Max Filippov <[email protected]>:
> Many network drivers directly modify phy_device::advertising and
> phy_device::supported. Provide accessors to these fields to better
> isolate phylib from its users.
>
> Suggested-by: Ben Hutchings <[email protected]>
> Signed-off-by: Max Filippov <[email protected]>
After giving some more thought to this patch, I am not sure this
really adds anything, struct phy_device is already exposed to drivers,
and those drivers have been able to modify phydev->supported and
phydev->advertising to suit their needs.
> ---
> Changes v1->v2:
> - new patch
>
> include/linux/phy.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 48a4dc3..2ae58f8 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -559,6 +559,18 @@ static inline int phy_read_status(struct phy_device *phydev) {
> return phydev->drv->read_status(phydev);
> }
>
> +static inline void phy_update_advert(struct phy_device *phydev, u32 clear,
> + u32 set)
> +{
> + phydev->advertising = (phydev->advertising & ~clear) | set;
> +}
> +
> +static inline void phy_update_supported(struct phy_device *phydev, u32 clear,
> + u32 set)
> +{
> + phydev->supported = (phydev->supported & ~clear) | set;
> +}
> +
> int genphy_setup_forced(struct phy_device *phydev);
> int genphy_restart_aneg(struct phy_device *phydev);
> int genphy_config_aneg(struct phy_device *phydev);
> --
> 1.8.1.4
>
--
Florian
On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli <[email protected]> wrote:
> On Jan 28, 2014 11:01 PM, "Max Filippov" <[email protected]> wrote:
>>
>> On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli <[email protected]>
>> wrote:
>> > Hi Max,
>> >
>> > Le 28/01/2014 22:00, Max Filippov a écrit :
>> >
>> >> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but
>> >> does
>> >> not disable advertisement when PHY supports them. This results in
>> >> non-functioning network when the MAC is connected to a gigabit PHY
>> >> connected
>> >> to a gigabit switch.
>> >>
>> >> The fix is to disable gigabit speed advertisement on attached PHY
>> >> unconditionally.
>> >>
>> >> Signed-off-by: Max Filippov <[email protected]>
>> >> ---
>> >> Changes v1->v2:
>> >> - disable both gigabit advertisement and support.
>> >>
>> >> drivers/net/ethernet/ethoc.c | 8 ++++++++
>> >> 1 file changed, 8 insertions(+)
>> >>
>> >> diff --git a/drivers/net/ethernet/ethoc.c
>> >> b/drivers/net/ethernet/ethoc.c
>> >> index 4de8cfd..5643b2d 100644
>> >> --- a/drivers/net/ethernet/ethoc.c
>> >> +++ b/drivers/net/ethernet/ethoc.c
>> >> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device
>> >> *dev)
>> >> }
>> >>
>> >> priv->phy = phy;
>> >> + phy_update_advert(phy,
>> >> + ADVERTISED_1000baseT_Full |
>> >> + ADVERTISED_1000baseT_Half, 0);
>> >> + phy_start_aneg(phy);
>> >
>> >
>> > This does not look necessary, you should not have to call
>> > phy_start_aneg()
>> > because the PHY state machine is not yet started, at best this calls
>> > does
>> > nothing.
>>
>> This call actually makes the whole thing work, because otherwise once
>> gigabit
>> support is cleared from the supported mask genphy_config_advert does not
>> update gigabit advertisement register, leaving it enabled.
>
> OK, then we need to figure out what is wrong with ethoc since this is
> unusual.
Maybe they boot up with gigabit advertisement disabled in their PHY
and thus they don't see the problem?
> Other drivers do the following:
>
> - connect to the PHY
> - phydev->supported = PHY_BASIC_FEATURES
> - phydev->advertising &= phydev->supported
> - start the PHY state machine
>
> And they work just fine. Is the PHY driver you are bound to the "Generic
> PHY" or something else which does something funky in config_aneg()?
It's marvell 88E1111 from the KC-705 board, but the behaviour doesn't
change if I disable it and the generic phy is used.
--
Thanks.
-- Max
On Wed, Jan 29, 2014 at 11:01 AM, Florian Fainelli <[email protected]> wrote:
> Le 28/01/2014 22:00, Max Filippov a écrit :
>
>> MII management bus clock is derived from the MAC clock by dividing it by
>> MIIMODER register CLKDIV field value. This value may need to be set up
>> in case it is undefined or its default value is too high (and
>> communication with PHY is too slow) or too low (and communication with
>> PHY is impossible). The value of CLKDIV is not specified directly, but
>> is derived from the MAC clock for the default MII management bus frequency
>> of 2.5MHz. The MAC clock may be specified in the platform data, or as
>> either 'clock-frequency' or 'clocks' device tree attribute.
>>
>> Signed-off-by: Max Filippov <[email protected]>
>> ---
>> Changes v1->v2:
>> - drop MDIO bus frequency configurability, always configure for standard
>> 2.5MHz;
>> - allow using common clock framework to provide ethoc clock.
>>
>> drivers/net/ethernet/ethoc.c | 37 +++++++++++++++++++++++++++++++++++--
>> include/net/ethoc.h | 1 +
>> 2 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
>> index 5643b2d..5854d41 100644
>> --- a/drivers/net/ethernet/ethoc.c
>> +++ b/drivers/net/ethernet/ethoc.c
>> @@ -13,6 +13,7 @@
>>
>> #include <linux/dma-mapping.h>
>> #include <linux/etherdevice.h>
>> +#include <linux/clk.h>
>> #include <linux/crc32.h>
>> #include <linux/interrupt.h>
>> #include <linux/io.h>
>> @@ -216,6 +217,7 @@ struct ethoc {
>>
>> struct phy_device *phy;
>> struct mii_bus *mdio;
>> + struct clk *clk;
>> s8 phy_id;
>> };
>>
>> @@ -925,6 +927,8 @@ static int ethoc_probe(struct platform_device *pdev)
>> int num_bd;
>> int ret = 0;
>> bool random_mac = false;
>> + u32 eth_clkfreq = 0;
>> + struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);
>>
>> /* allocate networking device */
>> netdev = alloc_etherdev(sizeof(struct ethoc));
>> @@ -1038,8 +1042,7 @@ static int ethoc_probe(struct platform_device *pdev)
>> }
>>
>> /* Allow the platform setup code to pass in a MAC address. */
>> - if (dev_get_platdata(&pdev->dev)) {
>> - struct ethoc_platform_data *pdata =
>> dev_get_platdata(&pdev->dev);
>> + if (pdata) {
>> memcpy(netdev->dev_addr, pdata->hwaddr, IFHWADDRLEN);
>> priv->phy_id = pdata->phy_id;
>> } else {
>> @@ -1077,6 +1080,32 @@ static int ethoc_probe(struct platform_device
>> *pdev)
>> if (random_mac)
>> netdev->addr_assign_type = NET_ADDR_RANDOM;
>>
>> + /* Allow the platform setup code to adjust MII management bus
>> clock. */
>> + if (pdata)
>> + eth_clkfreq = pdata->eth_clkfreq;
>
> Since this is a new member, why not make it a struct clk pointer directly so
> you could simplify the code path?
Basically this is to provide flexibility for the user: it may be more
appropriate to
specify frequency if it's known and fixed, otherwise clk_get below
would find the
clock registered for this device/generic clock.
>> + else
>> + of_property_read_u32(pdev->dev.of_node,
>> + "clock-frequency", ð_clkfreq);
>> + if (!eth_clkfreq) {
>> + struct clk *clk = clk_get(&pdev->dev, NULL);
This should have been devm_clk_get.
>> +
>> + if (!IS_ERR(clk)) {
>> + priv->clk = clk;
>> + clk_prepare_enable(clk);
>> + eth_clkfreq = clk_get_rate(clk);
>> + }
>> + }
>> + if (eth_clkfreq) {
>> + u32 clkdiv = MIIMODER_CLKDIV(eth_clkfreq / 2500000 + 1);
>> +
>> + if (!clkdiv)
>> + clkdiv = 2;
>> + dev_dbg(&pdev->dev, "setting MII clkdiv to %u\n", clkdiv);
>> + ethoc_write(priv, MIIMODER,
>> + (ethoc_read(priv, MIIMODER) & MIIMODER_NOPRE)
>> |
>> + clkdiv);
>> + }
>
>
> This does look a bit convoluted, and it looks like the clk_get() or getting
> the clock-frequency property should boil down to being the same thing with
> of_clk_get() as it should resolve all clocks phandles and fetch their
> frequencies appropriately.
I can drop clock-frequency property checking to encourage usage of common
clock framework. I don't quite understand the rest of the objection, could you
please rephrase it? clk_get calls of_clk_get internally.
>> +
>> /* register MII bus */
>> priv->mdio = mdiobus_alloc();
>> if (!priv->mdio) {
>> @@ -1141,6 +1170,8 @@ free_mdio:
>> kfree(priv->mdio->irq);
>> mdiobus_free(priv->mdio);
>> free:
>> + if (priv->clk)
>> + clk_disable_unprepare(priv->clk);
>
>
> This should probbaly be if (!IS_ERR(priv-clk)).
No, I haven't changed priv->clk when get_clk returned error.
>> free_netdev(netdev);
>> out:
>> return ret;
>> @@ -1165,6 +1196,8 @@ static int ethoc_remove(struct platform_device
>> *pdev)
>> kfree(priv->mdio->irq);
>> mdiobus_free(priv->mdio);
>> }
>> + if (priv->clk)
>> + clk_disable_unprepare(priv->clk);
>
>
> Same here
Ditto.
>> unregister_netdev(netdev);
>> free_netdev(netdev);
>> }
>> diff --git a/include/net/ethoc.h b/include/net/ethoc.h
>> index 96f3789..2a2d6bb 100644
>> --- a/include/net/ethoc.h
>> +++ b/include/net/ethoc.h
>> @@ -16,6 +16,7 @@
>> struct ethoc_platform_data {
>> u8 hwaddr[IFHWADDRLEN];
>> s8 phy_id;
>> + u32 eth_clkfreq;
>> };
>>
>> #endif /* !LINUX_NET_ETHOC_H */
>>
--
Thanks.
-- Max
On Wed, 2014-01-29 at 10:00 +0400, Max Filippov wrote:
> The following methods are implemented:
> - get/set settings;
> - get registers length/registers;
> - get link state (standard implementation);
> - get/set ring parameters;
> - get timestamping info (standard implementation).
[...]
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> [...]
> +static int ethoc_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> + struct phy_device *phydev = priv->phy;
> +
> + if (!phydev)
> + return -ENODEV;
> +
> + return phy_ethtool_gset(phydev, cmd);
> +}
> +
> +static int ethoc_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> + struct phy_device *phydev = priv->phy;
> +
> + if (!phydev)
> + return -ENODEV;
> +
> + return phy_ethtool_sset(phydev, cmd);
> +}
I think these should return -EOPNOTSUPP in the PHY-less case, just as if
the set_settings pointer was not set.
[...]
> +static void ethoc_get_ringparam(struct net_device *dev,
> + struct ethtool_ringparam *ring)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> +
> + ring->rx_max_pending = priv->num_bd;
> + ring->rx_mini_max_pending = 0;
> + ring->rx_jumbo_max_pending = 0;
> + ring->tx_max_pending = priv->num_bd;
> +
> + ring->rx_pending = priv->num_rx;
> + ring->rx_mini_pending = 0;
> + ring->rx_jumbo_pending = 0;
> + ring->tx_pending = priv->num_tx;
> +}
> +
> +static int ethoc_set_ringparam(struct net_device *dev,
> + struct ethtool_ringparam *ring)
> +{
> + struct ethoc *priv = netdev_priv(dev);
> +
> + if (netif_running(dev))
> + return -EBUSY;
This is unhelpful. Most implementations of this operation will restart
the interface if it's running.
> + priv->num_tx = rounddown_pow_of_two(ring->tx_pending);
Range check?
> + priv->num_rx = priv->num_bd - priv->num_tx;
> + if (priv->num_rx > ring->rx_pending)
> + priv->num_rx = ring->rx_pending;
So the RX ring may only ever be shrunk?! Did you mean to compare with
priv->num_bd instead?
> + return 0;
> +}
> +
> +const struct ethtool_ops ethoc_ethtool_ops = {
static
> + .get_settings = ethoc_get_settings,
> + .set_settings = ethoc_set_settings,
> + .get_regs_len = ethoc_get_regs_len,
> + .get_regs = ethoc_get_regs,
> + .get_link = ethtool_op_get_link,
> + .get_ringparam = ethoc_get_ringparam,
> + .set_ringparam = ethoc_set_ringparam,
> + .get_ts_info = ethtool_op_get_ts_info,
> +};
[...]
--
Ben Hutchings
It is a miracle that curiosity survives formal education. - Albert Einstein
On Thu, Jan 30, 2014 at 5:59 AM, Ben Hutchings <[email protected]> wrote:
> On Wed, 2014-01-29 at 10:00 +0400, Max Filippov wrote:
>> The following methods are implemented:
>> - get/set settings;
>> - get registers length/registers;
>> - get link state (standard implementation);
>> - get/set ring parameters;
>> - get timestamping info (standard implementation).
> [...]
>> --- a/drivers/net/ethernet/ethoc.c
>> +++ b/drivers/net/ethernet/ethoc.c
>> [...]
>> +static int ethoc_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
>> +{
>> + struct ethoc *priv = netdev_priv(dev);
>> + struct phy_device *phydev = priv->phy;
>> +
>> + if (!phydev)
>> + return -ENODEV;
>> +
>> + return phy_ethtool_gset(phydev, cmd);
>> +}
>> +
>> +static int ethoc_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
>> +{
>> + struct ethoc *priv = netdev_priv(dev);
>> + struct phy_device *phydev = priv->phy;
>> +
>> + if (!phydev)
>> + return -ENODEV;
>> +
>> + return phy_ethtool_sset(phydev, cmd);
>> +}
>
> I think these should return -EOPNOTSUPP in the PHY-less case, just as if
> the set_settings pointer was not set.
Ok
> [...]
>> +static void ethoc_get_ringparam(struct net_device *dev,
>> + struct ethtool_ringparam *ring)
>> +{
>> + struct ethoc *priv = netdev_priv(dev);
>> +
>> + ring->rx_max_pending = priv->num_bd;
>> + ring->rx_mini_max_pending = 0;
>> + ring->rx_jumbo_max_pending = 0;
>> + ring->tx_max_pending = priv->num_bd;
>> +
>> + ring->rx_pending = priv->num_rx;
>> + ring->rx_mini_pending = 0;
>> + ring->rx_jumbo_pending = 0;
>> + ring->tx_pending = priv->num_tx;
>> +}
>> +
>> +static int ethoc_set_ringparam(struct net_device *dev,
>> + struct ethtool_ringparam *ring)
>> +{
>> + struct ethoc *priv = netdev_priv(dev);
>> +
>> + if (netif_running(dev))
>> + return -EBUSY;
>
> This is unhelpful. Most implementations of this operation will restart
> the interface if it's running.
Ok
>> + priv->num_tx = rounddown_pow_of_two(ring->tx_pending);
>
> Range check?
May there be requested more than ring->tx_max_pending that we
indicated in the get_ringparam?
>> + priv->num_rx = priv->num_bd - priv->num_tx;
>> + if (priv->num_rx > ring->rx_pending)
>> + priv->num_rx = ring->rx_pending;
>
> So the RX ring may only ever be shrunk?! Did you mean to compare with
> priv->num_bd instead?
First all non-TX descriptors are made RX, and if that's more than user
requested I trim it.
>> + return 0;
>> +}
>> +
>> +const struct ethtool_ops ethoc_ethtool_ops = {
>
> static
Ok
--
Thanks.
-- Max
On Thu, 2014-01-30 at 07:04 +0400, Max Filippov wrote:
> On Thu, Jan 30, 2014 at 5:59 AM, Ben Hutchings <[email protected]> wrote:
> > On Wed, 2014-01-29 at 10:00 +0400, Max Filippov wrote:
[...]
> >> + priv->num_tx = rounddown_pow_of_two(ring->tx_pending);
> >
> > Range check?
>
> May there be requested more than ring->tx_max_pending that we
> indicated in the get_ringparam?
Yes, the ethtool core doesn't check that for you.
> >> + priv->num_rx = priv->num_bd - priv->num_tx;
> >> + if (priv->num_rx > ring->rx_pending)
> >> + priv->num_rx = ring->rx_pending;
> >
> > So the RX ring may only ever be shrunk?! Did you mean to compare with
> > priv->num_bd instead?
>
> First all non-TX descriptors are made RX, and if that's more than user
> requested I trim it.
[...]
OK, I get it. But it would be clearer if you used min().
Ben.
--
Ben Hutchings
It is a miracle that curiosity survives formal education. - Albert Einstein
On Wed, Jan 29, 2014 at 10:32 PM, Max Filippov <[email protected]> wrote:
> On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli <[email protected]> wrote:
>> On Jan 28, 2014 11:01 PM, "Max Filippov" <[email protected]> wrote:
>>>
>>> On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli <[email protected]>
>>> wrote:
>>> > Hi Max,
>>> >
>>> > Le 28/01/2014 22:00, Max Filippov a écrit :
>>> >
>>> >> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but
>>> >> does
>>> >> not disable advertisement when PHY supports them. This results in
>>> >> non-functioning network when the MAC is connected to a gigabit PHY
>>> >> connected
>>> >> to a gigabit switch.
>>> >>
>>> >> The fix is to disable gigabit speed advertisement on attached PHY
>>> >> unconditionally.
>>> >>
>>> >> Signed-off-by: Max Filippov <[email protected]>
>>> >> ---
>>> >> Changes v1->v2:
>>> >> - disable both gigabit advertisement and support.
>>> >>
>>> >> drivers/net/ethernet/ethoc.c | 8 ++++++++
>>> >> 1 file changed, 8 insertions(+)
>>> >>
>>> >> diff --git a/drivers/net/ethernet/ethoc.c
>>> >> b/drivers/net/ethernet/ethoc.c
>>> >> index 4de8cfd..5643b2d 100644
>>> >> --- a/drivers/net/ethernet/ethoc.c
>>> >> +++ b/drivers/net/ethernet/ethoc.c
>>> >> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device
>>> >> *dev)
>>> >> }
>>> >>
>>> >> priv->phy = phy;
>>> >> + phy_update_advert(phy,
>>> >> + ADVERTISED_1000baseT_Full |
>>> >> + ADVERTISED_1000baseT_Half, 0);
>>> >> + phy_start_aneg(phy);
>>> >
>>> >
>>> > This does not look necessary, you should not have to call
>>> > phy_start_aneg()
>>> > because the PHY state machine is not yet started, at best this calls
>>> > does
>>> > nothing.
>>>
>>> This call actually makes the whole thing work, because otherwise once
>>> gigabit
>>> support is cleared from the supported mask genphy_config_advert does not
>>> update gigabit advertisement register, leaving it enabled.
>>
>> OK, then we need to figure out what is wrong with ethoc since this is
>> unusual.
>
> Maybe they boot up with gigabit advertisement disabled in their PHY
> and thus they don't see the problem?
>
>> Other drivers do the following:
>>
>> - connect to the PHY
>> - phydev->supported = PHY_BASIC_FEATURES
>> - phydev->advertising &= phydev->supported
>> - start the PHY state machine
>>
>> And they work just fine. Is the PHY driver you are bound to the "Generic
>> PHY" or something else which does something funky in config_aneg()?
>
> It's marvell 88E1111 from the KC-705 board, but the behaviour doesn't
> change if I disable it and the generic phy is used.
Florian,
I don't see how the generic genphy_config_advert can ever change
gigabit advertisement if phydev->supported has gigabit speeds masked
off. So I'm pretty sure that other 10/100 cards would exhibit the same
issue if their PHY started with gigabit advertisement enabled. Maybe
we need to fix those other drivers? Or maybe we need to track what
PHY really supports vs. what we report it supports, so that gigabit
advertisement could be changed even when the PHY no longer
appears to support gigabit?
--
Thanks.
-- Max
2014-01-30 Max Filippov <[email protected]>:
> On Wed, Jan 29, 2014 at 10:32 PM, Max Filippov <[email protected]> wrote:
>> On Wed, Jan 29, 2014 at 9:12 PM, Florian Fainelli <[email protected]> wrote:
>>> On Jan 28, 2014 11:01 PM, "Max Filippov" <[email protected]> wrote:
>>>>
>>>> On Wed, Jan 29, 2014 at 10:47 AM, Florian Fainelli <[email protected]>
>>>> wrote:
>>>> > Hi Max,
>>>> >
>>>> > Le 28/01/2014 22:00, Max Filippov a écrit :
>>>> >
>>>> >> OpenCores 10/100 Mbps MAC does not support speeds above 100 Mbps, but
>>>> >> does
>>>> >> not disable advertisement when PHY supports them. This results in
>>>> >> non-functioning network when the MAC is connected to a gigabit PHY
>>>> >> connected
>>>> >> to a gigabit switch.
>>>> >>
>>>> >> The fix is to disable gigabit speed advertisement on attached PHY
>>>> >> unconditionally.
>>>> >>
>>>> >> Signed-off-by: Max Filippov <[email protected]>
>>>> >> ---
>>>> >> Changes v1->v2:
>>>> >> - disable both gigabit advertisement and support.
>>>> >>
>>>> >> drivers/net/ethernet/ethoc.c | 8 ++++++++
>>>> >> 1 file changed, 8 insertions(+)
>>>> >>
>>>> >> diff --git a/drivers/net/ethernet/ethoc.c
>>>> >> b/drivers/net/ethernet/ethoc.c
>>>> >> index 4de8cfd..5643b2d 100644
>>>> >> --- a/drivers/net/ethernet/ethoc.c
>>>> >> +++ b/drivers/net/ethernet/ethoc.c
>>>> >> @@ -688,6 +688,14 @@ static int ethoc_mdio_probe(struct net_device
>>>> >> *dev)
>>>> >> }
>>>> >>
>>>> >> priv->phy = phy;
>>>> >> + phy_update_advert(phy,
>>>> >> + ADVERTISED_1000baseT_Full |
>>>> >> + ADVERTISED_1000baseT_Half, 0);
>>>> >> + phy_start_aneg(phy);
>>>> >
>>>> >
>>>> > This does not look necessary, you should not have to call
>>>> > phy_start_aneg()
>>>> > because the PHY state machine is not yet started, at best this calls
>>>> > does
>>>> > nothing.
>>>>
>>>> This call actually makes the whole thing work, because otherwise once
>>>> gigabit
>>>> support is cleared from the supported mask genphy_config_advert does not
>>>> update gigabit advertisement register, leaving it enabled.
>>>
>>> OK, then we need to figure out what is wrong with ethoc since this is
>>> unusual.
>>
>> Maybe they boot up with gigabit advertisement disabled in their PHY
>> and thus they don't see the problem?
>>
>>> Other drivers do the following:
>>>
>>> - connect to the PHY
>>> - phydev->supported = PHY_BASIC_FEATURES
>>> - phydev->advertising &= phydev->supported
>>> - start the PHY state machine
>>>
>>> And they work just fine. Is the PHY driver you are bound to the "Generic
>>> PHY" or something else which does something funky in config_aneg()?
>>
>> It's marvell 88E1111 from the KC-705 board, but the behaviour doesn't
>> change if I disable it and the generic phy is used.
>
> Florian,
>
> I don't see how the generic genphy_config_advert can ever change
> gigabit advertisement if phydev->supported has gigabit speeds masked
> off.
It does not, but it makes sure that phydev->advertising gets masked
with phydev->supported. So, prior to starting your PHY state machine,
if you do:
phydev->supported &= PHY_BASIC_FEATURES;
phydev->advertising = phydev->supported;
you should get the expected results. Just make sure that
phydev->autoneg is not set to AUTONEG_DISABLE, otherwise
phy_start_aneg() will not call phy_sanitize_settings(), and this might
be the problem.
> So I'm pretty sure that other 10/100 cards would exhibit the same
> issue if their PHY started with gigabit advertisement enabled.
If the speed is not restricted, yes that will be the case, but most
drivers seem to correctly set both phydev->supported and
phydev->advertising.
> Maybe
> we need to fix those other drivers? Or maybe we need to track what
> PHY really supports vs. what we report it supports, so that gigabit
> advertisement could be changed even when the PHY no longer
> appears to support gigabit?
This is supposedly already taken care of provided that you correctly
restrict the PHY supported/advertising bits with respect to what the
HW really supports.
--
Florian
2014-01-31 Florian Fainelli <[email protected]>:
>>> Maybe they boot up with gigabit advertisement disabled in their PHY
>>> and thus they don't see the problem?
>>>
>>>> Other drivers do the following:
>>>>
>>>> - connect to the PHY
>>>> - phydev->supported = PHY_BASIC_FEATURES
>>>> - phydev->advertising &= phydev->supported
>>>> - start the PHY state machine
>>>>
>>>> And they work just fine. Is the PHY driver you are bound to the "Generic
>>>> PHY" or something else which does something funky in config_aneg()?
>>>
>>> It's marvell 88E1111 from the KC-705 board, but the behaviour doesn't
>>> change if I disable it and the generic phy is used.
>>
>> Florian,
>>
>> I don't see how the generic genphy_config_advert can ever change
>> gigabit advertisement if phydev->supported has gigabit speeds masked
>> off.
>
> It does not, but it makes sure that phydev->advertising gets masked
> with phydev->supported. So, prior to starting your PHY state machine,
> if you do:
>
> phydev->supported &= PHY_BASIC_FEATURES;
> phydev->advertising = phydev->supported;
It looks like there might be one problem however, if we have the following:
- phydev->supported masks off the Gigabit features
- PHY device comes out of reset/default with Gigabit features set in
MII_CTRL1000
Could you try the following patch:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 78bf1a4..b607f4a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device *phydev)
}
/* Configure gigabit if it's supported */
+ adv = phy_read(phydev, MII_CTRL1000);
+ if (adv < 0)
+ return adv;
+
+ oldadv = adv;
+ adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
+
if (phydev->supported & (SUPPORTED_1000baseT_Half |
SUPPORTED_1000baseT_Full)) {
- adv = phy_read(phydev, MII_CTRL1000);
- if (adv < 0)
- return adv;
-
- oldadv = adv;
- adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
-
- if (adv != oldadv) {
- err = phy_write(phydev, MII_CTRL1000, adv);
-
- if (err < 0)
- return err;
+ if (adv != oldadv)
changed = 1;
- }
}
+ err = phy_write(phydev, MII_CTRL1000, adv);
+ if (err < 0)
+ return err;
+
return changed;
}
--
Florian
On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli <[email protected]> wrote:
> 2014-01-31 Florian Fainelli <[email protected]>:
>>>> Maybe they boot up with gigabit advertisement disabled in their PHY
>>>> and thus they don't see the problem?
>>>>
>>>>> Other drivers do the following:
>>>>>
>>>>> - connect to the PHY
>>>>> - phydev->supported = PHY_BASIC_FEATURES
>>>>> - phydev->advertising &= phydev->supported
>>>>> - start the PHY state machine
>>>>>
>>>>> And they work just fine. Is the PHY driver you are bound to the "Generic
>>>>> PHY" or something else which does something funky in config_aneg()?
>>>>
>>>> It's marvell 88E1111 from the KC-705 board, but the behaviour doesn't
>>>> change if I disable it and the generic phy is used.
>>>
>>> Florian,
>>>
>>> I don't see how the generic genphy_config_advert can ever change
>>> gigabit advertisement if phydev->supported has gigabit speeds masked
>>> off.
>>
>> It does not, but it makes sure that phydev->advertising gets masked
>> with phydev->supported. So, prior to starting your PHY state machine,
>> if you do:
>>
>> phydev->supported &= PHY_BASIC_FEATURES;
>> phydev->advertising = phydev->supported;
>
> It looks like there might be one problem however, if we have the following:
>
> - phydev->supported masks off the Gigabit features
> - PHY device comes out of reset/default with Gigabit features set in
> MII_CTRL1000
That's exactly my case.
> Could you try the following patch:
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 78bf1a4..b607f4a 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device *phydev)
> }
>
> /* Configure gigabit if it's supported */
> + adv = phy_read(phydev, MII_CTRL1000);
> + if (adv < 0)
> + return adv;
> +
> + oldadv = adv;
> + adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
> +
> if (phydev->supported & (SUPPORTED_1000baseT_Half |
> SUPPORTED_1000baseT_Full)) {
> - adv = phy_read(phydev, MII_CTRL1000);
> - if (adv < 0)
> - return adv;
> -
> - oldadv = adv;
> - adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
> adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
> -
> - if (adv != oldadv) {
> - err = phy_write(phydev, MII_CTRL1000, adv);
> -
> - if (err < 0)
> - return err;
> + if (adv != oldadv)
> changed = 1;
> - }
> }
>
> + err = phy_write(phydev, MII_CTRL1000, adv);
I don't think this is correct: MII_CTRL1000 is reserved for 10/100 PHYs,
we probably need to read/write it conditionally, depending on MII_BMSR
BMSR_ESTATEN bit.
Otherwise yes, it works for me too.
> + if (err < 0)
> + return err;
> +
> return changed;
> }
--
Thanks.
-- Max
2014-01-31 Max Filippov <[email protected]>:
> On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli <[email protected]> wrote:
>> 2014-01-31 Florian Fainelli <[email protected]>:
>>>>> Maybe they boot up with gigabit advertisement disabled in their PHY
>>>>> and thus they don't see the problem?
>>>>>
>>>>>> Other drivers do the following:
>>>>>>
>>>>>> - connect to the PHY
>>>>>> - phydev->supported = PHY_BASIC_FEATURES
>>>>>> - phydev->advertising &= phydev->supported
>>>>>> - start the PHY state machine
>>>>>>
>>>>>> And they work just fine. Is the PHY driver you are bound to the "Generic
>>>>>> PHY" or something else which does something funky in config_aneg()?
>>>>>
>>>>> It's marvell 88E1111 from the KC-705 board, but the behaviour doesn't
>>>>> change if I disable it and the generic phy is used.
>>>>
>>>> Florian,
>>>>
>>>> I don't see how the generic genphy_config_advert can ever change
>>>> gigabit advertisement if phydev->supported has gigabit speeds masked
>>>> off.
>>>
>>> It does not, but it makes sure that phydev->advertising gets masked
>>> with phydev->supported. So, prior to starting your PHY state machine,
>>> if you do:
>>>
>>> phydev->supported &= PHY_BASIC_FEATURES;
>>> phydev->advertising = phydev->supported;
>>
>> It looks like there might be one problem however, if we have the following:
>>
>> - phydev->supported masks off the Gigabit features
>> - PHY device comes out of reset/default with Gigabit features set in
>> MII_CTRL1000
>
> That's exactly my case.
>
>> Could you try the following patch:
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 78bf1a4..b607f4a 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device *phydev)
>> }
>>
>> /* Configure gigabit if it's supported */
>> + adv = phy_read(phydev, MII_CTRL1000);
>> + if (adv < 0)
>> + return adv;
>> +
>> + oldadv = adv;
>> + adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
>> +
>> if (phydev->supported & (SUPPORTED_1000baseT_Half |
>> SUPPORTED_1000baseT_Full)) {
>> - adv = phy_read(phydev, MII_CTRL1000);
>> - if (adv < 0)
>> - return adv;
>> -
>> - oldadv = adv;
>> - adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
>> adv |= ethtool_adv_to_mii_ctrl1000_t(advertise);
>> -
>> - if (adv != oldadv) {
>> - err = phy_write(phydev, MII_CTRL1000, adv);
>> -
>> - if (err < 0)
>> - return err;
>> + if (adv != oldadv)
>> changed = 1;
>> - }
>> }
>>
>> + err = phy_write(phydev, MII_CTRL1000, adv);
>
>
> I don't think this is correct: MII_CTRL1000 is reserved for 10/100 PHYs,
> we probably need to read/write it conditionally, depending on MII_BMSR
> BMSR_ESTATEN bit.
> Otherwise yes, it works for me too.
You are right, that needs to be made conditional. I will give this
patch some more proper testing on a truly non-gigabit PHY. Thanks!
--
Florian