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

Groups > linux.kernel > #1333339 > unrolled thread

[PATCH] net: macb: Add support for phy-handle binding

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: macb: Add support for phy-handle binding Nathan Rossi <nathan@nathanrossi.com> - 2016-02-13 17:10 +0100
    Re: [PATCH] net: macb: Add support for phy-handle binding David Miller <davem@davemloft.net> - 2016-02-17 18:20 +0100
      Re: [PATCH] net: macb: Add support for phy-handle binding Nathan Rossi <nathan@nathanrossi.com> - 2016-02-17 19:50 +0100
        Re: [PATCH] net: macb: Add support for phy-handle binding David Miller <davem@davemloft.net> - 2016-02-17 20:00 +0100

#1333339 — [PATCH] net: macb: Add support for phy-handle binding

This patch adds support for the 'phy-handle' binding which allows for a
system to specifically select a phy which can be attached via any MDIO
bus available in the system.

Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 58 ++++++++++++++++++++++---------------
 drivers/net/ethernet/cadence/macb.h |  1 +
 2 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 50c9410..ea8ec32 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -375,27 +375,37 @@ static int macb_mii_probe(struct net_device *dev)
 	int phy_irq;
 	int ret;
 
-	phydev = phy_find_first(bp->mii_bus);
-	if (!phydev) {
-		netdev_err(dev, "no PHY found\n");
-		return -ENXIO;
-	}
+	if (bp->phy_node) {
+		phydev = of_phy_connect(dev, bp->phy_node,
+					&macb_handle_link_change, 0,
+					bp->phy_interface);
+		if (!phydev)
+			return -EPROBE_DEFER;
+	} else {
+		phydev = phy_find_first(bp->mii_bus);
+		if (!phydev) {
+			netdev_err(dev, "no PHY found\n");
+			return -ENXIO;
+		}
 
-	pdata = dev_get_platdata(&bp->pdev->dev);
-	if (pdata && gpio_is_valid(pdata->phy_irq_pin)) {
-		ret = devm_gpio_request(&bp->pdev->dev, pdata->phy_irq_pin, "phy int");
-		if (!ret) {
-			phy_irq = gpio_to_irq(pdata->phy_irq_pin);
-			phydev->irq = (phy_irq < 0) ? PHY_POLL : phy_irq;
+		pdata = dev_get_platdata(&bp->pdev->dev);
+		if (pdata && gpio_is_valid(pdata->phy_irq_pin)) {
+			ret = devm_gpio_request(&bp->pdev->dev,
+						pdata->phy_irq_pin, "phy int");
+			if (!ret) {
+				phy_irq = gpio_to_irq(pdata->phy_irq_pin);
+				phydev->irq = (phy_irq < 0) ? PHY_POLL :
+						phy_irq;
+			}
 		}
-	}
 
-	/* attach the mac to the phy */
-	ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
-				 bp->phy_interface);
-	if (ret) {
-		netdev_err(dev, "Could not attach to PHY\n");
-		return ret;
+		/* attach the mac to the phy */
+		ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
+					 bp->phy_interface);
+		if (ret) {
+			netdev_err(dev, "Could not attach to PHY\n");
+			return ret;
+		}
 	}
 
 	/* mask with MAC supported features */
@@ -2821,7 +2831,6 @@ static int macb_probe(struct platform_device *pdev)
 					      = macb_clk_init;
 	int (*init)(struct platform_device *) = macb_init;
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *phy_node;
 	const struct macb_config *macb_config = NULL;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
 	unsigned int queue_mask, num_queues;
@@ -2909,15 +2918,18 @@ static int macb_probe(struct platform_device *pdev)
 	else
 		macb_get_hwaddr(bp);
 
+	bp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
+	/* If phy-handle not provided assume first child is the phy node. */
+	if (!bp->phy_node)
+		bp->phy_node = of_get_next_available_child(np, NULL);
+
 	/* Power up the PHY if there is a GPIO reset */
-	phy_node =  of_get_next_available_child(np, NULL);
-	if (phy_node) {
-		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
+	if (bp->phy_node) {
+		int gpio = of_get_named_gpio(bp->phy_node, "reset-gpios", 0);
 		if (gpio_is_valid(gpio))
 			bp->reset_gpio = gpio_to_desc(gpio);
 		gpiod_set_value(bp->reset_gpio, GPIOD_OUT_HIGH);
 	}
-	of_node_put(phy_node);
 
 	err = of_get_phy_mode(np);
 	if (err < 0) {
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 0d4ecfc..0373aa47 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -822,6 +822,7 @@ struct macb {
 
 	struct mii_bus		*mii_bus;
 	struct phy_device	*phy_dev;
+	struct device_node	*phy_node;
 	int 			link;
 	int 			speed;
 	int 			duplex;
-- 
2.7.0

[toc] | [next] | [standalone]

#1336588

From: Nathan Rossi <nathan@nathanrossi.com>
Date: Sun, 14 Feb 2016 02:02:48 +1000

> This patch adds support for the 'phy-handle' binding which allows for a
> system to specifically select a phy which can be attached via any MDIO
> bus available in the system.
> 
> Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>

I don't see how this can be backwards compatible at all.

In my opinion we are way too loose about handling things like this.

Existing chips that happened to have an OF node but lack a phy-handle
property are going to be broken by that change.

I know you are now going to bombard me with all kinds of reasons why
this won't happen.

Don't bother, I'm simply not interested.

All of these special cases we use (all the DT bindings are in text
files in the kernel sources, we control all of the bootloaders, etc.)
is the most shaky foundation I've ever seen upon which to erect a
stable device probing mechanism.

I'm not applying this patch until you add an error handling path from
the of_phy_connect() call that will do the existing PHY probing sequence
by hand using phy_find_first().

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

#1336655

On Thu, Feb 18, 2016 at 3:14 AM, David Miller <davem@davemloft.net> wrote:
> From: Nathan Rossi <nathan@nathanrossi.com>
> Date: Sun, 14 Feb 2016 02:02:48 +1000
>
>> This patch adds support for the 'phy-handle' binding which allows for a
>> system to specifically select a phy which can be attached via any MDIO
>> bus available in the system.
>>
>> Signed-off-by: Nathan Rossi <nathan@nathanrossi.com>
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> I don't see how this can be backwards compatible at all.
>
> In my opinion we are way too loose about handling things like this.
>
> Existing chips that happened to have an OF node but lack a phy-handle
> property are going to be broken by that change.
>
> I know you are now going to bombard me with all kinds of reasons why
> this won't happen.
>
> Don't bother, I'm simply not interested.

The intention with this patch is to maintain existing dt bindings
alongside being able to use phy-handle, but I am not completely
familiar with all the use cases of the macb driver so I can't know for
sure if this change does not break certain cases. So I fully
understand your point.

>
> All of these special cases we use (all the DT bindings are in text
> files in the kernel sources, we control all of the bootloaders, etc.)
> is the most shaky foundation I've ever seen upon which to erect a
> stable device probing mechanism.
>
> I'm not applying this patch until you add an error handling path from
> the of_phy_connect() call that will do the existing PHY probing sequence
> by hand using phy_find_first().

I am not quite sure how to handle that in a way such as to also allow
for the probe deferral in the event that the phydev/mdio bus which the
phy-handle points at is not yet probed itself. But I guess the real
question is whether or not to handle the deferral at all?

Thanks,
Nathan

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

#1336658

From: Nathan Rossi <nathan@nathanrossi.com>
Date: Thu, 18 Feb 2016 04:44:35 +1000

> On Thu, Feb 18, 2016 at 3:14 AM, David Miller <davem@davemloft.net> wrote:
>> All of these special cases we use (all the DT bindings are in text
>> files in the kernel sources, we control all of the bootloaders, etc.)
>> is the most shaky foundation I've ever seen upon which to erect a
>> stable device probing mechanism.
>>
>> I'm not applying this patch until you add an error handling path from
>> the of_phy_connect() call that will do the existing PHY probing sequence
>> by hand using phy_find_first().
> 
> I am not quite sure how to handle that in a way such as to also allow
> for the probe deferral in the event that the phydev/mdio bus which the
> phy-handle points at is not yet probed itself. But I guess the real
> question is whether or not to handle the deferral at all?

I think we need some experts in this area to chime in on this issue.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web