| Message ID | 1487191445-5353-1-git-send-email-mdf@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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
> @@ -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
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
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;