[RFC/PATCH] net: nixge: Add PHYLINK support

[RFC/PATCH] net: nixge: Add PHYLINK support

[Moritz Fischer]

Add basic PHYLINK support to driver.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---

Hi all,

as Andrew suggested in order to enable SFP as
well as fixed-link support add PHYLINK support.

A couple of questions are still open (hence the RFC):

1) It seems odd to implement PHYLINK callbacks that
   are all empty? If so, should we have generic empty
   ones in drivers/net/phy/phylink.c like we have for
   genphys?

2) If this is ok, then I'll go ahead rework this with
   a DT binding update to deprecate the non-'mdio'-subnode
   case (since there are no in-tree users we might just
   change the binding)?

3) I'm again not sure about the 'select PHYLINK', wouldn't
   wanna break the build again...

Thanks again for your time!

Moritz

---
 drivers/net/ethernet/ni/Kconfig |   1 +
 drivers/net/ethernet/ni/nixge.c | 115 +++++++++++++++++++++++---------
 2 files changed, 83 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/ni/Kconfig b/drivers/net/ethernet/ni/Kconfig
index c73978474c4b..80cd72948551 100644
--- a/drivers/net/ethernet/ni/Kconfig
+++ b/drivers/net/ethernet/ni/Kconfig
@@ -21,6 +21,7 @@ config NI_XGE_MANAGEMENT_ENET
 	depends on HAS_IOMEM && HAS_DMA
 	select PHYLIB
 	select OF_MDIO if OF
+	select PHYLINK
 	help
 	  Simple LAN device for debug or management purposes. Can
 	  support either 10G or 1G PHYs via SFP+ ports.
diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index 74cf52e3fb09..a0e790d07b1c 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -11,6 +11,7 @@
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/of_platform.h>
+#include <linux/phylink.h>
 #include <linux/of_irq.h>
 #include <linux/skbuff.h>
 #include <linux/phy.h>
@@ -165,7 +166,7 @@ struct nixge_priv {
 	struct device *dev;
 
 	/* Connection to PHY device */
-	struct device_node *phy_node;
+	struct phylink *phylink;
 	phy_interface_t		phy_mode;
 
 	int link;
@@ -416,20 +417,6 @@ static void nixge_device_reset(struct net_device *ndev)
 	netif_trans_update(ndev);
 }
 
-static void nixge_handle_link_change(struct net_device *ndev)
-{
-	struct nixge_priv *priv = netdev_priv(ndev);
-	struct phy_device *phydev = ndev->phydev;
-
-	if (phydev->link != priv->link || phydev->speed != priv->speed ||
-	    phydev->duplex != priv->duplex) {
-		priv->link = phydev->link;
-		priv->speed = phydev->speed;
-		priv->duplex = phydev->duplex;
-		phy_print_status(phydev);
-	}
-}
-
 static void nixge_tx_skb_unmap(struct nixge_priv *priv,
 			       struct nixge_tx_skb *tx_skb)
 {
@@ -859,17 +846,15 @@ static void nixge_dma_err_handler(unsigned long data)
 static int nixge_open(struct net_device *ndev)
 {
 	struct nixge_priv *priv = netdev_priv(ndev);
-	struct phy_device *phy;
 	int ret;
 
 	nixge_device_reset(ndev);
 
-	phy = of_phy_connect(ndev, priv->phy_node,
-			     &nixge_handle_link_change, 0, priv->phy_mode);
-	if (!phy)
-		return -ENODEV;
+	ret = phylink_of_phy_connect(priv->phylink, priv->dev->of_node, 0);
+	if (ret < 0)
+		return ret;
 
-	phy_start(phy);
+	phylink_start(priv->phylink);
 
 	/* Enable tasklets for Axi DMA error handling */
 	tasklet_init(&priv->dma_err_tasklet, nixge_dma_err_handler,
@@ -893,8 +878,7 @@ static int nixge_open(struct net_device *ndev)
 err_rx_irq:
 	free_irq(priv->tx_irq, ndev);
 err_tx_irq:
-	phy_stop(phy);
-	phy_disconnect(phy);
+	phylink_disconnect_phy(priv->phylink);
 	tasklet_kill(&priv->dma_err_tasklet);
 	netdev_err(ndev, "request_irq() failed\n");
 	return ret;
@@ -908,9 +892,9 @@ static int nixge_stop(struct net_device *ndev)
 	netif_stop_queue(ndev);
 	napi_disable(&priv->napi);
 
-	if (ndev->phydev) {
-		phy_stop(ndev->phydev);
-		phy_disconnect(ndev->phydev);
+	if (priv->phylink) {
+		phylink_stop(priv->phylink);
+		phylink_disconnect_phy(priv->phylink);
 	}
 
 	cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
@@ -1076,13 +1060,31 @@ static int nixge_ethtools_set_phys_id(struct net_device *ndev,
 	return 0;
 }
 
+static int
+nixge_ethtool_set_link_ksettings(struct net_device *ndev,
+				 const struct ethtool_link_ksettings *cmd)
+{
+	struct nixge_priv *priv = netdev_priv(ndev);
+
+	return phylink_ethtool_ksettings_set(priv->phylink, cmd);
+}
+
+static int
+nixge_ethtool_get_link_ksettings(struct net_device *ndev,
+				 struct ethtool_link_ksettings *cmd)
+{
+	struct nixge_priv *priv = netdev_priv(ndev);
+
+	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
+}
+
 static const struct ethtool_ops nixge_ethtool_ops = {
 	.get_drvinfo    = nixge_ethtools_get_drvinfo,
 	.get_coalesce   = nixge_ethtools_get_coalesce,
 	.set_coalesce   = nixge_ethtools_set_coalesce,
 	.set_phys_id    = nixge_ethtools_set_phys_id,
-	.get_link_ksettings     = phy_ethtool_get_link_ksettings,
-	.set_link_ksettings     = phy_ethtool_set_link_ksettings,
+	.get_link_ksettings     = nixge_ethtool_get_link_ksettings,
+	.set_link_ksettings     = nixge_ethtool_set_link_ksettings,
 	.get_link		= ethtool_op_get_link,
 };
 
@@ -1225,11 +1227,52 @@ static void *nixge_get_nvmem_address(struct device *dev)
 	return mac;
 }
 
+static void nixge_validate(struct net_device *ndev, unsigned long *supported,
+			   struct phylink_link_state *state)
+{
+}
+
+static int nixge_mac_link_state(struct net_device *ndev,
+				struct phylink_link_state *state)
+{
+	return 0;
+}
+
+static void nixge_mac_config(struct net_device *ndev, unsigned int mode,
+			     const struct phylink_link_state *state)
+{
+}
+
+static void nixge_mac_an_restart(struct net_device *ndev)
+{
+}
+
+static void nixge_mac_link_down(struct net_device *ndev, unsigned int mode,
+				phy_interface_t interface)
+{
+}
+
+static void nixge_mac_link_up(struct net_device *ndev, unsigned int mode,
+			      phy_interface_t interface,
+			      struct phy_device *phy)
+{
+}
+
+static const struct phylink_mac_ops nixge_phylink_ops = {
+	.validate = nixge_validate,
+	.mac_link_state = nixge_mac_link_state,
+	.mac_an_restart = nixge_mac_an_restart,
+	.mac_config = nixge_mac_config,
+	.mac_link_down = nixge_mac_link_down,
+	.mac_link_up = nixge_mac_link_up,
+};
+
 static int nixge_probe(struct platform_device *pdev)
 {
 	struct nixge_priv *priv;
 	struct net_device *ndev;
 	struct resource *dmares;
+	struct device_node *mn;
 	const u8 *mac_addr;
 	int err;
 
@@ -1286,7 +1329,13 @@ static int nixge_probe(struct platform_device *pdev)
 	priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD;
 	priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD;
 
-	err = nixge_mdio_setup(priv, pdev->dev.of_node);
+	mn = of_get_child_by_name(pdev->dev.of_node, "mdio");
+	if (!mn) {
+		dev_warn(&pdev->dev, "No \"mdio\" subnode found, defaulting to legacy\n");
+		mn = pdev->dev.of_node;
+	}
+
+	err = nixge_mdio_setup(priv, mn);
 	if (err) {
 		netdev_err(ndev, "error registering mdio bus");
 		goto free_netdev;
@@ -1299,10 +1348,10 @@ static int nixge_probe(struct platform_device *pdev)
 		goto unregister_mdio;
 	}
 
-	priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
-	if (!priv->phy_node) {
-		netdev_err(ndev, "not find \"phy-handle\" property\n");
-		err = -EINVAL;
+	priv->phylink = phylink_create(ndev, pdev->dev.fwnode, priv->phy_mode,
+				       &nixge_phylink_ops);
+	if (IS_ERR(priv->phylink)) {
+		err = PTR_ERR(priv->phylink);
 		goto unregister_mdio;
 	}
 
-- 
2.18.0

Re: [RFC/PATCH] net: nixge: Add PHYLINK support

[Florian Fainelli]

On 09/04/2018 05:15 PM, Moritz Fischer wrote:
> Add basic PHYLINK support to driver.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
> 
> Hi all,
> 
> as Andrew suggested in order to enable SFP as
> well as fixed-link support add PHYLINK support.
> 
> A couple of questions are still open (hence the RFC):
> 
> 1) It seems odd to implement PHYLINK callbacks that
>    are all empty? If so, should we have generic empty
>    ones in drivers/net/phy/phylink.c like we have for
>    genphys?

Yes it is odd, the validate callback most certainly should not be empty,
neither should the mac_config and mac_link_{up,down}, but, with some
luck, you can get things to just work, typically with MDIO PHYs, since a
large amount of what they can do is discoverable.

If you had an existing adjust_link callback from PHYLIB, it's really
about breaking it down such that the MAC configuration of
speed/duplex/pause happens in mac_config, and the link setting (if
necessary), happens in mac_link_{up,down}, and that's pretty much it for
MLO_AN_PHY cases.

> 
> 2) If this is ok, then I'll go ahead rework this with
>    a DT binding update to deprecate the non-'mdio'-subnode
>    case (since there are no in-tree users we might just
>    change the binding)?
> 
> 3) I'm again not sure about the 'select PHYLINK', wouldn't
>    wanna break the build again...
> 
> Thanks again for your time!
> 
> Moritz
> 
> ---
>  drivers/net/ethernet/ni/Kconfig |   1 +
>  drivers/net/ethernet/ni/nixge.c | 115 +++++++++++++++++++++++---------
>  2 files changed, 83 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ni/Kconfig b/drivers/net/ethernet/ni/Kconfig
> index c73978474c4b..80cd72948551 100644
> --- a/drivers/net/ethernet/ni/Kconfig
> +++ b/drivers/net/ethernet/ni/Kconfig
> @@ -21,6 +21,7 @@ config NI_XGE_MANAGEMENT_ENET
>  	depends on HAS_IOMEM && HAS_DMA
>  	select PHYLIB
>  	select OF_MDIO if OF
> +	select PHYLINK
>  	help
>  	  Simple LAN device for debug or management purposes. Can
>  	  support either 10G or 1G PHYs via SFP+ ports.
> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 74cf52e3fb09..a0e790d07b1c 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c
> @@ -11,6 +11,7 @@
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
>  #include <linux/of_platform.h>
> +#include <linux/phylink.h>
>  #include <linux/of_irq.h>
>  #include <linux/skbuff.h>
>  #include <linux/phy.h>
> @@ -165,7 +166,7 @@ struct nixge_priv {
>  	struct device *dev;
>  
>  	/* Connection to PHY device */
> -	struct device_node *phy_node;
> +	struct phylink *phylink;
>  	phy_interface_t		phy_mode;
>  
>  	int link;
> @@ -416,20 +417,6 @@ static void nixge_device_reset(struct net_device *ndev)
>  	netif_trans_update(ndev);
>  }
>  
> -static void nixge_handle_link_change(struct net_device *ndev)
> -{
> -	struct nixge_priv *priv = netdev_priv(ndev);
> -	struct phy_device *phydev = ndev->phydev;
> -
> -	if (phydev->link != priv->link || phydev->speed != priv->speed ||
> -	    phydev->duplex != priv->duplex) {
> -		priv->link = phydev->link;
> -		priv->speed = phydev->speed;
> -		priv->duplex = phydev->duplex;
> -		phy_print_status(phydev);
> -	}
> -}
> -
>  static void nixge_tx_skb_unmap(struct nixge_priv *priv,
>  			       struct nixge_tx_skb *tx_skb)
>  {
> @@ -859,17 +846,15 @@ static void nixge_dma_err_handler(unsigned long data)
>  static int nixge_open(struct net_device *ndev)
>  {
>  	struct nixge_priv *priv = netdev_priv(ndev);
> -	struct phy_device *phy;
>  	int ret;
>  
>  	nixge_device_reset(ndev);
>  
> -	phy = of_phy_connect(ndev, priv->phy_node,
> -			     &nixge_handle_link_change, 0, priv->phy_mode);
> -	if (!phy)
> -		return -ENODEV;
> +	ret = phylink_of_phy_connect(priv->phylink, priv->dev->of_node, 0);
> +	if (ret < 0)
> +		return ret;
>  
> -	phy_start(phy);
> +	phylink_start(priv->phylink);
>  
>  	/* Enable tasklets for Axi DMA error handling */
>  	tasklet_init(&priv->dma_err_tasklet, nixge_dma_err_handler,
> @@ -893,8 +878,7 @@ static int nixge_open(struct net_device *ndev)
>  err_rx_irq:
>  	free_irq(priv->tx_irq, ndev);
>  err_tx_irq:
> -	phy_stop(phy);
> -	phy_disconnect(phy);
> +	phylink_disconnect_phy(priv->phylink);
>  	tasklet_kill(&priv->dma_err_tasklet);
>  	netdev_err(ndev, "request_irq() failed\n");
>  	return ret;
> @@ -908,9 +892,9 @@ static int nixge_stop(struct net_device *ndev)
>  	netif_stop_queue(ndev);
>  	napi_disable(&priv->napi);
>  
> -	if (ndev->phydev) {
> -		phy_stop(ndev->phydev);
> -		phy_disconnect(ndev->phydev);
> +	if (priv->phylink) {
> +		phylink_stop(priv->phylink);
> +		phylink_disconnect_phy(priv->phylink);
>  	}
>  
>  	cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
> @@ -1076,13 +1060,31 @@ static int nixge_ethtools_set_phys_id(struct net_device *ndev,
>  	return 0;
>  }
>  
> +static int
> +nixge_ethtool_set_link_ksettings(struct net_device *ndev,
> +				 const struct ethtool_link_ksettings *cmd)
> +{
> +	struct nixge_priv *priv = netdev_priv(ndev);
> +
> +	return phylink_ethtool_ksettings_set(priv->phylink, cmd);
> +}
> +
> +static int
> +nixge_ethtool_get_link_ksettings(struct net_device *ndev,
> +				 struct ethtool_link_ksettings *cmd)
> +{
> +	struct nixge_priv *priv = netdev_priv(ndev);
> +
> +	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
> +}
> +
>  static const struct ethtool_ops nixge_ethtool_ops = {
>  	.get_drvinfo    = nixge_ethtools_get_drvinfo,
>  	.get_coalesce   = nixge_ethtools_get_coalesce,
>  	.set_coalesce   = nixge_ethtools_set_coalesce,
>  	.set_phys_id    = nixge_ethtools_set_phys_id,
> -	.get_link_ksettings     = phy_ethtool_get_link_ksettings,
> -	.set_link_ksettings     = phy_ethtool_set_link_ksettings,
> +	.get_link_ksettings     = nixge_ethtool_get_link_ksettings,
> +	.set_link_ksettings     = nixge_ethtool_set_link_ksettings,
>  	.get_link		= ethtool_op_get_link,
>  };
>  
> @@ -1225,11 +1227,52 @@ static void *nixge_get_nvmem_address(struct device *dev)
>  	return mac;
>  }
>  
> +static void nixge_validate(struct net_device *ndev, unsigned long *supported,
> +			   struct phylink_link_state *state)
> +{
> +}
> +
> +static int nixge_mac_link_state(struct net_device *ndev,
> +				struct phylink_link_state *state)
> +{
> +	return 0;
> +}
> +
> +static void nixge_mac_config(struct net_device *ndev, unsigned int mode,
> +			     const struct phylink_link_state *state)
> +{
> +}
> +
> +static void nixge_mac_an_restart(struct net_device *ndev)
> +{
> +}
> +
> +static void nixge_mac_link_down(struct net_device *ndev, unsigned int mode,
> +				phy_interface_t interface)
> +{
> +}
> +
> +static void nixge_mac_link_up(struct net_device *ndev, unsigned int mode,
> +			      phy_interface_t interface,
> +			      struct phy_device *phy)
> +{
> +}
> +
> +static const struct phylink_mac_ops nixge_phylink_ops = {
> +	.validate = nixge_validate,
> +	.mac_link_state = nixge_mac_link_state,
> +	.mac_an_restart = nixge_mac_an_restart,
> +	.mac_config = nixge_mac_config,
> +	.mac_link_down = nixge_mac_link_down,
> +	.mac_link_up = nixge_mac_link_up,
> +};
> +
>  static int nixge_probe(struct platform_device *pdev)
>  {
>  	struct nixge_priv *priv;
>  	struct net_device *ndev;
>  	struct resource *dmares;
> +	struct device_node *mn;
>  	const u8 *mac_addr;
>  	int err;
>  
> @@ -1286,7 +1329,13 @@ static int nixge_probe(struct platform_device *pdev)
>  	priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD;
>  	priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD;
>  
> -	err = nixge_mdio_setup(priv, pdev->dev.of_node);
> +	mn = of_get_child_by_name(pdev->dev.of_node, "mdio");
> +	if (!mn) {
> +		dev_warn(&pdev->dev, "No \"mdio\" subnode found, defaulting to legacy\n");
> +		mn = pdev->dev.of_node;
> +	}
> +
> +	err = nixge_mdio_setup(priv, mn);
>  	if (err) {
>  		netdev_err(ndev, "error registering mdio bus");
>  		goto free_netdev;
> @@ -1299,10 +1348,10 @@ static int nixge_probe(struct platform_device *pdev)
>  		goto unregister_mdio;
>  	}
>  
> -	priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> -	if (!priv->phy_node) {
> -		netdev_err(ndev, "not find \"phy-handle\" property\n");
> -		err = -EINVAL;
> +	priv->phylink = phylink_create(ndev, pdev->dev.fwnode, priv->phy_mode,
> +				       &nixge_phylink_ops);
> +	if (IS_ERR(priv->phylink)) {
> +		err = PTR_ERR(priv->phylink);
>  		goto unregister_mdio;
>  	}
>  
> 


-- 
Florian

Re: [RFC/PATCH] net: nixge: Add PHYLINK support

[Andrew Lunn]

> 3) I'm again not sure about the 'select PHYLINK', wouldn't
>    wanna break the build again...

Hi Moritz

I think it is safe. PHYLINK has no stated dependencies on OF. But i
suspect it currently is pretty useless without OF.

> @@ -1286,7 +1329,13 @@ static int nixge_probe(struct platform_device *pdev)
>  	priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD;
>  	priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD;
>  
> -	err = nixge_mdio_setup(priv, pdev->dev.of_node);
> +	mn = of_get_child_by_name(pdev->dev.of_node, "mdio");
> +	if (!mn) {
> +		dev_warn(&pdev->dev, "No \"mdio\" subnode found, defaulting to legacy\n");
> +		mn = pdev->dev.of_node;
> +	}
> +
> +	err = nixge_mdio_setup(priv, mn);

I would suggest making this a patch of its own.

Also, do you need the legacy behaviour? If there are no boards out in
the wild which this will break, just make the change.

Please also update the device tree binding documentation.

       Andrew

Re: [RFC/PATCH] net: nixge: Add PHYLINK support

[Andrew Lunn]

> -static void nixge_handle_link_change(struct net_device *ndev)
> -{
> -	struct nixge_priv *priv = netdev_priv(ndev);
> -	struct phy_device *phydev = ndev->phydev;
> -
> -	if (phydev->link != priv->link || phydev->speed != priv->speed ||
> -	    phydev->duplex != priv->duplex) {
> -		priv->link = phydev->link;
> -		priv->speed = phydev->speed;
> -		priv->duplex = phydev->duplex;
> -		phy_print_status(phydev);
> -	}
> -}

I think this is why you are having trouble with the phylink
callbacks. You currently don't have any configuration of the MAC. You
normally need to tell the MAC what speed it should be doing. What
duplex it should be doing, if the link is up or down, if it should do
pause, or asym pause or no pause, etc.

       Andrew

Re: [RFC/PATCH] net: nixge: Add PHYLINK support

[Moritz Fischer]

Hi Andrew,

On Tue, Sep 4, 2018 at 6:01 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> 3) I'm again not sure about the 'select PHYLINK', wouldn't
>>    wanna break the build again...
>
> Hi Moritz
>
> I think it is safe. PHYLINK has no stated dependencies on OF. But i
> suspect it currently is pretty useless without OF.

Ok, great. Thanks!

>> @@ -1286,7 +1329,13 @@ static int nixge_probe(struct platform_device *pdev)
>>       priv->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD;
>>       priv->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD;
>>
>> -     err = nixge_mdio_setup(priv, pdev->dev.of_node);
>> +     mn = of_get_child_by_name(pdev->dev.of_node, "mdio");
>> +     if (!mn) {
>> +             dev_warn(&pdev->dev, "No \"mdio\" subnode found, defaulting to legacy\n");
>> +             mn = pdev->dev.of_node;
>> +     }
>> +
>> +     err = nixge_mdio_setup(priv, mn);
>
> I would suggest making this a patch of its own.

Yeah, will do.
> Also, do you need the legacy behaviour? If there are no boards out in
> the wild which this will break, just make the change.

Well all users basically use devicetree overlays that we distribute
with the FPGA images
as part of a filesystem update. So I suppose it wouldn't break anyone,
yet... It would certainly
make the code easier to read.

> Please also update the device tree binding documentation.

Yeah will do.

Thanks for the feedback,

Moritz

Re: [RFC/PATCH] net: nixge: Add PHYLINK support

[Moritz Fischer]

Hi Florian,

On Tue, Sep 4, 2018 at 5:27 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 09/04/2018 05:15 PM, Moritz Fischer wrote:
>> Add basic PHYLINK support to driver.
>>
>> Suggested-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
>> ---
>>
>> Hi all,
>>
>> as Andrew suggested in order to enable SFP as
>> well as fixed-link support add PHYLINK support.
>>
>> A couple of questions are still open (hence the RFC):
>>
>> 1) It seems odd to implement PHYLINK callbacks that
>>    are all empty? If so, should we have generic empty
>>    ones in drivers/net/phy/phylink.c like we have for
>>    genphys?
>
> Yes it is odd, the validate callback most certainly should not be empty,
> neither should the mac_config and mac_link_{up,down}, but, with some
> luck, you can get things to just work, typically with MDIO PHYs, since a
> large amount of what they can do is discoverable.
>
> If you had an existing adjust_link callback from PHYLIB, it's really
> about breaking it down such that the MAC configuration of
> speed/duplex/pause happens in mac_config, and the link setting (if
> necessary), happens in mac_link_{up,down}, and that's pretty much it for
> MLO_AN_PHY cases.

Let me check, it seems there is a register that indicates whether the MAC can
do either 1G or 10G. I might be able to use that for some of the above, but
there is not really much in terms of writable registers there.
It's like a DMA engine with a bit of MDIO on the side. Let me see if I can make
it look less weird with that. If not I'll go with a comment explaining
that there
isn't much to do for the MLO_AN_PHY case and the MLO_FIXED cases?

Cheers and thanks for your feedback,
Moritz

Re: [RFC/PATCH] net: nixge: Add PHYLINK support

[Andrew Lunn]

> Let me check, it seems there is a register that indicates whether the MAC can
> do either 1G or 10G. I might be able to use that for some of the above, but
> there is not really much in terms of writable registers there.

Can the MAC do 10 or 100? At the moment, you don't have anything
stopping the PHY anto-neg'ing 10Half. If the MAC does not fully
implement standard Ethernet, you need to tell the PHY driver about
this. That is what the validate call is about. phylink and phylib
knows what the PHY supports. It passes that list to the validate
call. You need to then remove all the modes the MAC does not support.

> It's like a DMA engine with a bit of MDIO on the side. Let me see if
> I can make it look less weird with that. If not I'll go with a
> comment explaining that there isn't much to do for the MLO_AN_PHY
> case and the MLO_FIXED cases?

You again need to configure the MAC to the selected speed, duplex,
etc. If the link is down, you want to disable the MAC. You need this
for both MLO_AN_PHY and MLO_FIXED, because both specify speeds,
duplex, etc.

	Andrew

Re: [RFC/PATCH] net: nixge: Add PHYLINK support

[Moritz Fischer]

Andrew,

On Wed, Sep 5, 2018 at 5:31 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>> Let me check, it seems there is a register that indicates whether the MAC can
>> do either 1G or 10G. I might be able to use that for some of the above, but
>> there is not really much in terms of writable registers there.
>
> Can the MAC do 10 or 100? At the moment, you don't have anything
> stopping the PHY anto-neg'ing 10Half. If the MAC does not fully
> implement standard Ethernet, you need to tell the PHY driver about
> this. That is what the validate call is about. phylink and phylib
> knows what the PHY supports. It passes that list to the validate
> call. You need to then remove all the modes the MAC does not support.

Makes sense, thanks for clarifying. I'll do some more research on this.
>
>> It's like a DMA engine with a bit of MDIO on the side. Let me see if
>> I can make it look less weird with that. If not I'll go with a
>> comment explaining that there isn't much to do for the MLO_AN_PHY
>> case and the MLO_FIXED cases?
>
> You again need to configure the MAC to the selected speed, duplex,
> etc. If the link is down, you want to disable the MAC. You need this
> for both MLO_AN_PHY and MLO_FIXED, because both specify speeds,
> duplex, etc.

I'll look into it.

Moritz