Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581650 > unrolled thread

[PATCH] net: ethernet: cadence: Add fixed-link functionality

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: ethernet: cadence: Add fixed-link functionality mdf@kernel.org - 2017-02-15 21:50 +0100
    Re: [PATCH] net: ethernet: cadence: Add fixed-link functionality Florian Fainelli <f.fainelli@gmail.com> - 2017-02-15 22:00 +0100
      Re: [PATCH] net: ethernet: cadence: Add fixed-link functionality Moritz Fischer <mdf@kernel.org> - 2017-02-15 22:10 +0100
    Re: [PATCH] net: ethernet: cadence: Add fixed-link functionality Andrew Lunn <andrew@lunn.ch> - 2017-02-15 23:20 +0100
      Re: [PATCH] net: ethernet: cadence: Add fixed-link functionality Moritz Fischer <mdf@kernel.org> - 2017-02-15 23:40 +0100

#1581650 — [PATCH] net: ethernet: cadence: Add fixed-link functionality

From: Moritz Fischer <mdf@kernel.org>

This allows 'fixed-link' direct MAC connections to be declared
in devicetree.

Signed-off-by: Moritz Fischer <mdf@kernel.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb.c | 61 ++++++++++++++++++++++++++++++++++---
 drivers/net/ethernet/cadence/macb.h |  1 +
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 30606b1..af443a8 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -469,6 +469,34 @@ static int macb_mii_probe(struct net_device *dev)
 	return 0;
 }
 
+static int macb_fixed_init(struct macb *bp)
+{
+	struct phy_device *phydev;
+
+	phydev = of_phy_connect(bp->dev, bp->phy_node,
+				&macb_handle_link_change, 0,
+				bp->phy_interface);
+	if (!phydev)
+		return -ENODEV;
+
+	/* mask with MAC supported features */
+	if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
+		phydev->supported &= PHY_GBIT_FEATURES;
+	else
+		phydev->supported &= PHY_BASIC_FEATURES;
+
+	if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
+		phydev->supported &= ~SUPPORTED_1000baseT_Half;
+
+	phydev->advertising = phydev->supported;
+
+	bp->link = 0;
+	bp->speed = 0;
+	bp->duplex = -1;
+
+	return 0;
+}
+
 static int macb_mii_init(struct macb *bp)
 {
 	struct macb_platform_data *pdata;
@@ -3245,6 +3273,7 @@ static int macb_probe(struct platform_device *pdev)
 	const char *mac;
 	struct macb *bp;
 	int err;
+	bool fixed_link = false;
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	mem = devm_ioremap_resource(&pdev->dev, regs);
@@ -3342,8 +3371,18 @@ static int macb_probe(struct platform_device *pdev)
 		macb_get_hwaddr(bp);
 
 	/* Power up the PHY if there is a GPIO reset */
-	phy_node =  of_get_next_available_child(np, NULL);
-	if (phy_node) {
+	phy_node = of_parse_phandle(np, "phy-handle", 0);
+	if (!phy_node && of_phy_is_fixed_link(np)) {
+		err = of_phy_register_fixed_link(np);
+		if (err < 0) {
+			dev_err(&pdev->dev, "broken fixed-link specification");
+			goto failed_phy;
+		}
+		/* in case of a fixed PHY, the DT node is the ethernet MAC */
+		phy_node = of_node_get(np);
+		bp->phy_node = phy_node;
+		fixed_link = true;
+	} else {
 		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
 
 		if (gpio_is_valid(gpio)) {
@@ -3369,7 +3408,10 @@ static int macb_probe(struct platform_device *pdev)
 	if (err)
 		goto err_out_free_netdev;
 
-	err = macb_mii_init(bp);
+	if (!fixed_link)
+		err = macb_mii_init(bp);
+	else
+		err = macb_fixed_init(bp);
 	if (err)
 		goto err_out_free_netdev;
 
@@ -3400,6 +3442,9 @@ static int macb_probe(struct platform_device *pdev)
 	if (bp->reset_gpio)
 		gpiod_set_value(bp->reset_gpio, 0);
 
+failed_phy:
+	of_node_put(phy_node);
+
 err_out_free_netdev:
 	free_netdev(dev);
 
@@ -3423,9 +3468,14 @@ static int macb_remove(struct platform_device *pdev)
 		bp = netdev_priv(dev);
 		if (dev->phydev)
 			phy_disconnect(dev->phydev);
-		mdiobus_unregister(bp->mii_bus);
+
+		if (!bp->phy_node)
+			mdiobus_unregister(bp->mii_bus);
+
 		dev->phydev = NULL;
-		mdiobus_free(bp->mii_bus);
+
+		if (!bp->phy_node)
+			mdiobus_free(bp->mii_bus);
 
 		/* Shutdown the PHY if there is a GPIO reset */
 		if (bp->reset_gpio)
@@ -3435,6 +3485,7 @@ static int macb_remove(struct platform_device *pdev)
 		clk_disable_unprepare(bp->tx_clk);
 		clk_disable_unprepare(bp->hclk);
 		clk_disable_unprepare(bp->pclk);
+		of_node_put(bp->phy_node);
 		clk_disable_unprepare(bp->rx_clk);
 		free_netdev(dev);
 	}
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 234a49e..d9a9b6f 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -931,6 +931,7 @@ struct macb {
 	struct macb_or_gem_ops	macbgem_ops;
 
 	struct mii_bus		*mii_bus;
+	struct device_node	*phy_node;
 	int 			link;
 	int 			speed;
 	int 			duplex;
-- 
2.7.4

[toc] | [next] | [standalone]

#1581652

On 02/15/2017 12:44 PM, mdf@kernel.org wrote:
> From: Moritz Fischer <mdf@kernel.org>
> 
> This allows 'fixed-link' direct MAC connections to be declared
> in devicetree.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> ---
>  drivers/net/ethernet/cadence/macb.c | 61 ++++++++++++++++++++++++++++++++++---
>  drivers/net/ethernet/cadence/macb.h |  1 +
>  2 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 30606b1..af443a8 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -469,6 +469,34 @@ static int macb_mii_probe(struct net_device *dev)
>  	return 0;
>  }
>  
> +static int macb_fixed_init(struct macb *bp)
> +{
> +	struct phy_device *phydev;
> +
> +	phydev = of_phy_connect(bp->dev, bp->phy_node,
> +				&macb_handle_link_change, 0,
> +				bp->phy_interface);
> +	if (!phydev)
> +		return -ENODEV;
> +
> +	/* mask with MAC supported features */
> +	if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
> +		phydev->supported &= PHY_GBIT_FEATURES;
> +	else
> +		phydev->supported &= PHY_BASIC_FEATURES;
> +
> +	if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
> +		phydev->supported &= ~SUPPORTED_1000baseT_Half;
> +
> +	phydev->advertising = phydev->supported;
> +
> +	bp->link = 0;
> +	bp->speed = 0;
> +	bp->duplex = -1;
> +
> +	return 0;
> +}

This is nearly identical to macb_mii_probe(), can you try to re-use what
is done there and just extract the phy_find_first() part which is
different here?

> +
>  static int macb_mii_init(struct macb *bp)
>  {
>  	struct macb_platform_data *pdata;
> @@ -3245,6 +3273,7 @@ static int macb_probe(struct platform_device *pdev)
>  	const char *mac;
>  	struct macb *bp;
>  	int err;
> +	bool fixed_link = false;
>  
>  	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	mem = devm_ioremap_resource(&pdev->dev, regs);
> @@ -3342,8 +3371,18 @@ static int macb_probe(struct platform_device *pdev)
>  		macb_get_hwaddr(bp);
>  
>  	/* Power up the PHY if there is a GPIO reset */
> -	phy_node =  of_get_next_available_child(np, NULL);
> -	if (phy_node) {
> +	phy_node = of_parse_phandle(np, "phy-handle", 0);
> +	if (!phy_node && of_phy_is_fixed_link(np)) {
> +		err = of_phy_register_fixed_link(np);
> +		if (err < 0) {
> +			dev_err(&pdev->dev, "broken fixed-link specification");
> +			goto failed_phy;
> +		}
> +		/* in case of a fixed PHY, the DT node is the ethernet MAC */
> +		phy_node = of_node_get(np);
> +		bp->phy_node = phy_node;
> +		fixed_link = true;
> +	} else {
>  		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>  
>  		if (gpio_is_valid(gpio)) {
> @@ -3369,7 +3408,10 @@ static int macb_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_out_free_netdev;
>  
> -	err = macb_mii_init(bp);
> +	if (!fixed_link)
> +		err = macb_mii_init(bp);
> +	else
> +		err = macb_fixed_init(bp);
>  	if (err)
>  		goto err_out_free_netdev;
>  
> @@ -3400,6 +3442,9 @@ static int macb_probe(struct platform_device *pdev)
>  	if (bp->reset_gpio)
>  		gpiod_set_value(bp->reset_gpio, 0);
>  
> +failed_phy:
> +	of_node_put(phy_node);
> +
>  err_out_free_netdev:
>  	free_netdev(dev);
>  
> @@ -3423,9 +3468,14 @@ static int macb_remove(struct platform_device *pdev)
>  		bp = netdev_priv(dev);
>  		if (dev->phydev)
>  			phy_disconnect(dev->phydev);
> -		mdiobus_unregister(bp->mii_bus);
> +
> +		if (!bp->phy_node)
> +			mdiobus_unregister(bp->mii_bus);
> +
>  		dev->phydev = NULL;
> -		mdiobus_free(bp->mii_bus);
> +
> +		if (!bp->phy_node)
> +			mdiobus_free(bp->mii_bus);

Humm, I'd have to read the code a bit more, but conceptually, you could
declare child MDIO device nodes (e.g: Ethernet switch) and a fixed-link
that describes how you are connecting to that Ethernet switch. The MDIO
devices require the MDIO bus to be available, so I don't think you can
treat fixed-link as mutually exclusive with an absence of PHY nodes.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1581659

Hi Florian,

thanks for the quick reply.

On Wed, Feb 15, 2017 at 12:57 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 02/15/2017 12:44 PM, mdf@kernel.org wrote:
>> From: Moritz Fischer <mdf@kernel.org>
>>
>> This allows 'fixed-link' direct MAC connections to be declared
>> in devicetree.
>>
>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
>> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
>> ---
>>  drivers/net/ethernet/cadence/macb.c | 61 ++++++++++++++++++++++++++++++++++---
>>  drivers/net/ethernet/cadence/macb.h |  1 +
>>  2 files changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index 30606b1..af443a8 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -469,6 +469,34 @@ static int macb_mii_probe(struct net_device *dev)
>>       return 0;
>>  }
>>
>> +static int macb_fixed_init(struct macb *bp)
>> +{
>> +     struct phy_device *phydev;
>> +
>> +     phydev = of_phy_connect(bp->dev, bp->phy_node,
>> +                             &macb_handle_link_change, 0,
>> +                             bp->phy_interface);
>> +     if (!phydev)
>> +             return -ENODEV;
>> +
>> +     /* mask with MAC supported features */
>> +     if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>> +             phydev->supported &= PHY_GBIT_FEATURES;
>> +     else
>> +             phydev->supported &= PHY_BASIC_FEATURES;
>> +
>> +     if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
>> +             phydev->supported &= ~SUPPORTED_1000baseT_Half;
>> +
>> +     phydev->advertising = phydev->supported;
>> +
>> +     bp->link = 0;
>> +     bp->speed = 0;
>> +     bp->duplex = -1;
>> +
>> +     return 0;
>> +}
>
> This is nearly identical to macb_mii_probe(), can you try to re-use what
> is done there and just extract the phy_find_first() part which is
> different here?

Yeah. It probably still needs some work.
>
>> +
>>  static int macb_mii_init(struct macb *bp)
>>  {
>>       struct macb_platform_data *pdata;
>> @@ -3245,6 +3273,7 @@ static int macb_probe(struct platform_device *pdev)
>>       const char *mac;
>>       struct macb *bp;
>>       int err;
>> +     bool fixed_link = false;
>>
>>       regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>       mem = devm_ioremap_resource(&pdev->dev, regs);
>> @@ -3342,8 +3371,18 @@ static int macb_probe(struct platform_device *pdev)
>>               macb_get_hwaddr(bp);
>>
>>       /* Power up the PHY if there is a GPIO reset */
>> -     phy_node =  of_get_next_available_child(np, NULL);
>> -     if (phy_node) {
>> +     phy_node = of_parse_phandle(np, "phy-handle", 0);
>> +     if (!phy_node && of_phy_is_fixed_link(np)) {
>> +             err = of_phy_register_fixed_link(np);
>> +             if (err < 0) {
>> +                     dev_err(&pdev->dev, "broken fixed-link specification");
>> +                     goto failed_phy;
>> +             }
>> +             /* in case of a fixed PHY, the DT node is the ethernet MAC */
>> +             phy_node = of_node_get(np);
>> +             bp->phy_node = phy_node;
>> +             fixed_link = true;
>> +     } else {
>>               int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>
>>               if (gpio_is_valid(gpio)) {
>> @@ -3369,7 +3408,10 @@ static int macb_probe(struct platform_device *pdev)
>>       if (err)
>>               goto err_out_free_netdev;
>>
>> -     err = macb_mii_init(bp);
>> +     if (!fixed_link)
>> +             err = macb_mii_init(bp);
>> +     else
>> +             err = macb_fixed_init(bp);
>>       if (err)
>>               goto err_out_free_netdev;
>>
>> @@ -3400,6 +3442,9 @@ static int macb_probe(struct platform_device *pdev)
>>       if (bp->reset_gpio)
>>               gpiod_set_value(bp->reset_gpio, 0);
>>
>> +failed_phy:
>> +     of_node_put(phy_node);
>> +
>>  err_out_free_netdev:
>>       free_netdev(dev);
>>
>> @@ -3423,9 +3468,14 @@ static int macb_remove(struct platform_device *pdev)
>>               bp = netdev_priv(dev);
>>               if (dev->phydev)
>>                       phy_disconnect(dev->phydev);
>> -             mdiobus_unregister(bp->mii_bus);
>> +
>> +             if (!bp->phy_node)
>> +                     mdiobus_unregister(bp->mii_bus);
>> +
>>               dev->phydev = NULL;
>> -             mdiobus_free(bp->mii_bus);
>> +
>> +             if (!bp->phy_node)
>> +                     mdiobus_free(bp->mii_bus);
>
> Humm, I'd have to read the code a bit more, but conceptually, you could
> declare child MDIO device nodes (e.g: Ethernet switch) and a fixed-link
> that describes how you are connecting to that Ethernet switch. The MDIO
> devices require the MDIO bus to be available, so I don't think you can
> treat fixed-link as mutually exclusive with an absence of PHY nodes.

Huh, yeah. In my case the switch configuration is done over SPI.
Probably someone
out there will have hardware that does MDIO, possible.
I'll do some more research on my end. In my understanding (which might
be wrong),
the fixed-link was the alternative to a PHY hooked up with MDIO.

If you find something in your research, feel free to keep me posted ;-)

Cheers,
Moritz

[toc] | [prev] | [next] | [standalone]

#1581714

> @@ -3342,8 +3371,18 @@ static int macb_probe(struct platform_device *pdev)
>  		macb_get_hwaddr(bp);
>  
>  	/* Power up the PHY if there is a GPIO reset */
> -	phy_node =  of_get_next_available_child(np, NULL);
> -	if (phy_node) {
> +	phy_node = of_parse_phandle(np, "phy-handle", 0);
> +	if (!phy_node && of_phy_is_fixed_link(np)) {
> +		err = of_phy_register_fixed_link(np);

Hi Moritz

I don't see any calls to of_phy_deregister_fixed_link(), either in the
error path, or the remove code.

      Andrew

[toc] | [prev] | [next] | [standalone]

#1581723

Andrew,

On Wed, Feb 15, 2017 at 2:12 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> @@ -3342,8 +3371,18 @@ static int macb_probe(struct platform_device *pdev)
>>               macb_get_hwaddr(bp);
>>
>>       /* Power up the PHY if there is a GPIO reset */
>> -     phy_node =  of_get_next_available_child(np, NULL);
>> -     if (phy_node) {
>> +     phy_node = of_parse_phandle(np, "phy-handle", 0);
>> +     if (!phy_node && of_phy_is_fixed_link(np)) {
>> +             err = of_phy_register_fixed_link(np);
>
> Hi Moritz
>
> I don't see any calls to of_phy_deregister_fixed_link(), either in the
> error path, or the remove code.

Whoops, yeah I rebased it from like a year ago. Must've not survived my
merge conflict resolution.

I'll rework it.

Thanks,

Moritz

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web