| Message ID | 20201013124014.2989-2-i.mikhaylov@yadro.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | add ast2400/2500 phy-handle support | expand |
Hi Ivan, On Tue, 13 Oct 2020 at 12:38, Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > > phy-handle can't be handled well for ast2400/2500 which has an embedded > MDIO controller. Add ftgmac100_mdio_setup for ast2400/2500 and initialize > PHYs from mdio child node with of_mdiobus_register. Good idea. The driver has become a mess of different ways to connect the phy and it needs to be cleaned up. I have a patch that fixes rmmod, which is currently broken. > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> > --- > drivers/net/ethernet/faraday/ftgmac100.c | 114 ++++++++++++++--------- > 1 file changed, 69 insertions(+), 45 deletions(-) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c > index 87236206366f..e32066519ec1 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -1044,11 +1044,47 @@ static void ftgmac100_adjust_link(struct net_device *netdev) > schedule_work(&priv->reset_task); > } > > -static int ftgmac100_mii_probe(struct ftgmac100 *priv, phy_interface_t intf) > +static int ftgmac100_mii_probe(struct net_device *netdev) > { > - struct net_device *netdev = priv->netdev; > + struct ftgmac100 *priv = netdev_priv(netdev); > + struct platform_device *pdev = to_platform_device(priv->dev); > + struct device_node *np = pdev->dev.of_node; > + phy_interface_t phy_intf = PHY_INTERFACE_MODE_RGMII; > struct phy_device *phydev; > > + /* Get PHY mode from device-tree */ > + if (np) { > + /* Default to RGMII. It's a gigabit part after all */ > + phy_intf = of_get_phy_mode(np, &phy_intf); > + if (phy_intf < 0) > + phy_intf = PHY_INTERFACE_MODE_RGMII; > + > + /* Aspeed only supports these. I don't know about other IP > + * block vendors so I'm going to just let them through for > + * now. Note that this is only a warning if for some obscure > + * reason the DT really means to lie about it or it's a newer > + * part we don't know about. > + * > + * On the Aspeed SoC there are additionally straps and SCU > + * control bits that could tell us what the interface is > + * (or allow us to configure it while the IP block is held > + * in reset). For now I chose to keep this driver away from > + * those SoC specific bits and assume the device-tree is > + * right and the SCU has been configured properly by pinmux > + * or the firmware. > + */ > + if (priv->is_aspeed && > + phy_intf != PHY_INTERFACE_MODE_RMII && > + phy_intf != PHY_INTERFACE_MODE_RGMII && > + phy_intf != PHY_INTERFACE_MODE_RGMII_ID && > + phy_intf != PHY_INTERFACE_MODE_RGMII_RXID && > + phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) { > + netdev_warn(netdev, > + "Unsupported PHY mode %s !\n", > + phy_modes(phy_intf)); > + } Why do we move this? > + } > + > phydev = phy_find_first(priv->mii_bus); > if (!phydev) { > netdev_info(netdev, "%s: no PHY found\n", netdev->name); > @@ -1056,7 +1092,7 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv, phy_interface_t intf) > } > > phydev = phy_connect(netdev, phydev_name(phydev), > - &ftgmac100_adjust_link, intf); > + &ftgmac100_adjust_link, phy_intf); > > if (IS_ERR(phydev)) { > netdev_err(netdev, "%s: Could not attach to PHY\n", netdev->name); > @@ -1601,8 +1637,8 @@ static int ftgmac100_setup_mdio(struct net_device *netdev) > { > struct ftgmac100 *priv = netdev_priv(netdev); > struct platform_device *pdev = to_platform_device(priv->dev); > - phy_interface_t phy_intf = PHY_INTERFACE_MODE_RGMII; > struct device_node *np = pdev->dev.of_node; > + struct device_node *mdio_np; > int i, err = 0; > u32 reg; > > @@ -1623,39 +1659,6 @@ static int ftgmac100_setup_mdio(struct net_device *netdev) > iowrite32(reg, priv->base + FTGMAC100_OFFSET_REVR); > } > > - /* Get PHY mode from device-tree */ > - if (np) { > - /* Default to RGMII. It's a gigabit part after all */ > - err = of_get_phy_mode(np, &phy_intf); > - if (err) > - phy_intf = PHY_INTERFACE_MODE_RGMII; > - > - /* Aspeed only supports these. I don't know about other IP > - * block vendors so I'm going to just let them through for > - * now. Note that this is only a warning if for some obscure > - * reason the DT really means to lie about it or it's a newer > - * part we don't know about. > - * > - * On the Aspeed SoC there are additionally straps and SCU > - * control bits that could tell us what the interface is > - * (or allow us to configure it while the IP block is held > - * in reset). For now I chose to keep this driver away from > - * those SoC specific bits and assume the device-tree is > - * right and the SCU has been configured properly by pinmux > - * or the firmware. > - */ > - if (priv->is_aspeed && > - phy_intf != PHY_INTERFACE_MODE_RMII && > - phy_intf != PHY_INTERFACE_MODE_RGMII && > - phy_intf != PHY_INTERFACE_MODE_RGMII_ID && > - phy_intf != PHY_INTERFACE_MODE_RGMII_RXID && > - phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) { > - netdev_warn(netdev, > - "Unsupported PHY mode %s !\n", > - phy_modes(phy_intf)); > - } > - } > - > priv->mii_bus->name = "ftgmac100_mdio"; > snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%d", > pdev->name, pdev->id); > @@ -1667,22 +1670,22 @@ static int ftgmac100_setup_mdio(struct net_device *netdev) > for (i = 0; i < PHY_MAX_ADDR; i++) > priv->mii_bus->irq[i] = PHY_POLL; > > - err = mdiobus_register(priv->mii_bus); > + mdio_np = of_get_child_by_name(np, "mdio"); > + if (mdio_np) > + err = of_mdiobus_register(priv->mii_bus, mdio_np); > + else > + err = mdiobus_register(priv->mii_bus); > + > if (err) { > dev_err(priv->dev, "Cannot register MDIO bus!\n"); > goto err_register_mdiobus; > } > > - err = ftgmac100_mii_probe(priv, phy_intf); > - if (err) { > - dev_err(priv->dev, "MII Probe failed!\n"); > - goto err_mii_probe; > - } > + if (mdio_np) > + of_node_put(mdio_np); By the time I get down here I'm lost. Do you think you could split the change up into a few smaller patches? If not, try to explain what the various hunks of your change are trying to do. Cheers, Joel > > return 0; > > -err_mii_probe: > - mdiobus_unregister(priv->mii_bus); > err_register_mdiobus: > mdiobus_free(priv->mii_bus); > return err; > @@ -1836,10 +1839,23 @@ static int ftgmac100_probe(struct platform_device *pdev) > } else if (np && of_get_property(np, "phy-handle", NULL)) { > struct phy_device *phy; > > + /* Support "mdio"/"phy" child nodes for ast2400/2500 with > + * an embedded MDIO controller. Automatically scan the DTS for > + * available PHYs and register them. > + */ > + if (of_device_is_compatible(np, "aspeed,ast2400-mac") || > + of_device_is_compatible(np, "aspeed,ast2500-mac")) { > + err = ftgmac100_setup_mdio(netdev); > + if (err) > + goto err_setup_mdio; > + } > + > phy = of_phy_get_and_connect(priv->netdev, np, > &ftgmac100_adjust_link); > if (!phy) { > dev_err(&pdev->dev, "Failed to connect to phy\n"); > + if (priv->mii_bus) > + mdiobus_unregister(priv->mii_bus); > goto err_setup_mdio; > } > > @@ -1860,6 +1876,14 @@ static int ftgmac100_probe(struct platform_device *pdev) > err = ftgmac100_setup_mdio(netdev); > if (err) > goto err_setup_mdio; > + > + err = ftgmac100_mii_probe(netdev); > + if (err) { > + dev_err(priv->dev, "MII probe failed!\n"); > + mdiobus_unregister(priv->mii_bus); > + goto err_setup_mdio; > + } > + > } > > if (priv->is_aspeed) { > -- > 2.21.1 >
On Wed, 2020-10-14 at 05:23 +0000, Joel Stanley wrote: > Hi Ivan, > > On Tue, 13 Oct 2020 at 12:38, Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > > phy-handle can't be handled well for ast2400/2500 which has an embedded > > MDIO controller. Add ftgmac100_mdio_setup for ast2400/2500 and initialize > > PHYs from mdio child node with of_mdiobus_register. > > Good idea. The driver has become a mess of different ways to connect > the phy and it needs to be cleaned up. I have a patch that fixes > rmmod, which is currently broken. > > > > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> > > --- > > drivers/net/ethernet/faraday/ftgmac100.c | 114 ++++++++++++++--------- > > 1 file changed, 69 insertions(+), 45 deletions(-) > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > > b/drivers/net/ethernet/faraday/ftgmac100.c > > index 87236206366f..e32066519ec1 100644 > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > @@ -1044,11 +1044,47 @@ static void ftgmac100_adjust_link(struct net_device > > *netdev) > > schedule_work(&priv->reset_task); > > } > > > > -static int ftgmac100_mii_probe(struct ftgmac100 *priv, phy_interface_t > > intf) > > +static int ftgmac100_mii_probe(struct net_device *netdev) > > { > > - struct net_device *netdev = priv->netdev; > > + struct ftgmac100 *priv = netdev_priv(netdev); > > + struct platform_device *pdev = to_platform_device(priv->dev); > > + struct device_node *np = pdev->dev.of_node; > > + phy_interface_t phy_intf = PHY_INTERFACE_MODE_RGMII; > > struct phy_device *phydev; > > > > + /* Get PHY mode from device-tree */ > > + if (np) { > > + /* Default to RGMII. It's a gigabit part after all */ > > + phy_intf = of_get_phy_mode(np, &phy_intf); > > + if (phy_intf < 0) > > + phy_intf = PHY_INTERFACE_MODE_RGMII; > > + > > + /* Aspeed only supports these. I don't know about other IP > > + * block vendors so I'm going to just let them through for > > + * now. Note that this is only a warning if for some obscure > > + * reason the DT really means to lie about it or it's a > > newer > > + * part we don't know about. > > + * > > + * On the Aspeed SoC there are additionally straps and SCU > > + * control bits that could tell us what the interface is > > + * (or allow us to configure it while the IP block is held > > + * in reset). For now I chose to keep this driver away from > > + * those SoC specific bits and assume the device-tree is > > + * right and the SCU has been configured properly by pinmux > > + * or the firmware. > > + */ > > + if (priv->is_aspeed && > > + phy_intf != PHY_INTERFACE_MODE_RMII && > > + phy_intf != PHY_INTERFACE_MODE_RGMII && > > + phy_intf != PHY_INTERFACE_MODE_RGMII_ID && > > + phy_intf != PHY_INTERFACE_MODE_RGMII_RXID && > > + phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) { > > + netdev_warn(netdev, > > + "Unsupported PHY mode %s !\n", > > + phy_modes(phy_intf)); > > + } > > Why do we move this? I've tried to detach PHY connect from ftgmac100_setup_mdio register function. Tried to decouple MDIO and PHY levels. > > > + } > > + > > phydev = phy_find_first(priv->mii_bus); > > if (!phydev) { > > netdev_info(netdev, "%s: no PHY found\n", netdev->name); > > @@ -1056,7 +1092,7 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv, > > phy_interface_t intf) > > } > > > > phydev = phy_connect(netdev, phydev_name(phydev), > > - &ftgmac100_adjust_link, intf); > > + &ftgmac100_adjust_link, phy_intf); > > > > if (IS_ERR(phydev)) { > > netdev_err(netdev, "%s: Could not attach to PHY\n", netdev- > > >name); > > @@ -1601,8 +1637,8 @@ static int ftgmac100_setup_mdio(struct net_device > > *netdev) > > { > > struct ftgmac100 *priv = netdev_priv(netdev); > > struct platform_device *pdev = to_platform_device(priv->dev); > > - phy_interface_t phy_intf = PHY_INTERFACE_MODE_RGMII; > > struct device_node *np = pdev->dev.of_node; > > + struct device_node *mdio_np; > > int i, err = 0; > > u32 reg; > > > > @@ -1623,39 +1659,6 @@ static int ftgmac100_setup_mdio(struct net_device > > *netdev) > > iowrite32(reg, priv->base + FTGMAC100_OFFSET_REVR); > > } > > > > - /* Get PHY mode from device-tree */ > > - if (np) { > > - /* Default to RGMII. It's a gigabit part after all */ > > - err = of_get_phy_mode(np, &phy_intf); > > - if (err) > > - phy_intf = PHY_INTERFACE_MODE_RGMII; > > - > > - /* Aspeed only supports these. I don't know about other IP > > - * block vendors so I'm going to just let them through for > > - * now. Note that this is only a warning if for some obscure > > - * reason the DT really means to lie about it or it's a > > newer > > - * part we don't know about. > > - * > > - * On the Aspeed SoC there are additionally straps and SCU > > - * control bits that could tell us what the interface is > > - * (or allow us to configure it while the IP block is held > > - * in reset). For now I chose to keep this driver away from > > - * those SoC specific bits and assume the device-tree is > > - * right and the SCU has been configured properly by pinmux > > - * or the firmware. > > - */ > > - if (priv->is_aspeed && > > - phy_intf != PHY_INTERFACE_MODE_RMII && > > - phy_intf != PHY_INTERFACE_MODE_RGMII && > > - phy_intf != PHY_INTERFACE_MODE_RGMII_ID && > > - phy_intf != PHY_INTERFACE_MODE_RGMII_RXID && > > - phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) { > > - netdev_warn(netdev, > > - "Unsupported PHY mode %s !\n", > > - phy_modes(phy_intf)); > > - } > > - } > > - > > priv->mii_bus->name = "ftgmac100_mdio"; > > snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%d", > > pdev->name, pdev->id); > > @@ -1667,22 +1670,22 @@ static int ftgmac100_setup_mdio(struct net_device > > *netdev) > > for (i = 0; i < PHY_MAX_ADDR; i++) > > priv->mii_bus->irq[i] = PHY_POLL; > > > > - err = mdiobus_register(priv->mii_bus); > > + mdio_np = of_get_child_by_name(np, "mdio"); > > + if (mdio_np) > > + err = of_mdiobus_register(priv->mii_bus, mdio_np); > > + else > > + err = mdiobus_register(priv->mii_bus); > > + > > if (err) { > > dev_err(priv->dev, "Cannot register MDIO bus!\n"); > > goto err_register_mdiobus; > > } > > > > - err = ftgmac100_mii_probe(priv, phy_intf); > > - if (err) { > > - dev_err(priv->dev, "MII Probe failed!\n"); > > - goto err_mii_probe; > > - } > > + if (mdio_np) > > + of_node_put(mdio_np); > > By the time I get down here I'm lost. Do you think you could split the > change up into a few smaller patches? > > If not, try to explain what the various hunks of your change are trying to do. > Yes, you're right, I see two patches for now from your comments: 1. phy connect detach from ftgmac100_setup_mdio 2. add ast2400/2500 phy-handle support Thanks.
> + if (priv->is_aspeed && > + phy_intf != PHY_INTERFACE_MODE_RMII && > + phy_intf != PHY_INTERFACE_MODE_RGMII && > + phy_intf != PHY_INTERFACE_MODE_RGMII_ID && > + phy_intf != PHY_INTERFACE_MODE_RGMII_RXID && > + phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) { phy_interface_mode_is_rgmii() Andrew
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index 87236206366f..e32066519ec1 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1044,11 +1044,47 @@ static void ftgmac100_adjust_link(struct net_device *netdev) schedule_work(&priv->reset_task); } -static int ftgmac100_mii_probe(struct ftgmac100 *priv, phy_interface_t intf) +static int ftgmac100_mii_probe(struct net_device *netdev) { - struct net_device *netdev = priv->netdev; + struct ftgmac100 *priv = netdev_priv(netdev); + struct platform_device *pdev = to_platform_device(priv->dev); + struct device_node *np = pdev->dev.of_node; + phy_interface_t phy_intf = PHY_INTERFACE_MODE_RGMII; struct phy_device *phydev; + /* Get PHY mode from device-tree */ + if (np) { + /* Default to RGMII. It's a gigabit part after all */ + phy_intf = of_get_phy_mode(np, &phy_intf); + if (phy_intf < 0) + phy_intf = PHY_INTERFACE_MODE_RGMII; + + /* Aspeed only supports these. I don't know about other IP + * block vendors so I'm going to just let them through for + * now. Note that this is only a warning if for some obscure + * reason the DT really means to lie about it or it's a newer + * part we don't know about. + * + * On the Aspeed SoC there are additionally straps and SCU + * control bits that could tell us what the interface is + * (or allow us to configure it while the IP block is held + * in reset). For now I chose to keep this driver away from + * those SoC specific bits and assume the device-tree is + * right and the SCU has been configured properly by pinmux + * or the firmware. + */ + if (priv->is_aspeed && + phy_intf != PHY_INTERFACE_MODE_RMII && + phy_intf != PHY_INTERFACE_MODE_RGMII && + phy_intf != PHY_INTERFACE_MODE_RGMII_ID && + phy_intf != PHY_INTERFACE_MODE_RGMII_RXID && + phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) { + netdev_warn(netdev, + "Unsupported PHY mode %s !\n", + phy_modes(phy_intf)); + } + } + phydev = phy_find_first(priv->mii_bus); if (!phydev) { netdev_info(netdev, "%s: no PHY found\n", netdev->name); @@ -1056,7 +1092,7 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv, phy_interface_t intf) } phydev = phy_connect(netdev, phydev_name(phydev), - &ftgmac100_adjust_link, intf); + &ftgmac100_adjust_link, phy_intf); if (IS_ERR(phydev)) { netdev_err(netdev, "%s: Could not attach to PHY\n", netdev->name); @@ -1601,8 +1637,8 @@ static int ftgmac100_setup_mdio(struct net_device *netdev) { struct ftgmac100 *priv = netdev_priv(netdev); struct platform_device *pdev = to_platform_device(priv->dev); - phy_interface_t phy_intf = PHY_INTERFACE_MODE_RGMII; struct device_node *np = pdev->dev.of_node; + struct device_node *mdio_np; int i, err = 0; u32 reg; @@ -1623,39 +1659,6 @@ static int ftgmac100_setup_mdio(struct net_device *netdev) iowrite32(reg, priv->base + FTGMAC100_OFFSET_REVR); } - /* Get PHY mode from device-tree */ - if (np) { - /* Default to RGMII. It's a gigabit part after all */ - err = of_get_phy_mode(np, &phy_intf); - if (err) - phy_intf = PHY_INTERFACE_MODE_RGMII; - - /* Aspeed only supports these. I don't know about other IP - * block vendors so I'm going to just let them through for - * now. Note that this is only a warning if for some obscure - * reason the DT really means to lie about it or it's a newer - * part we don't know about. - * - * On the Aspeed SoC there are additionally straps and SCU - * control bits that could tell us what the interface is - * (or allow us to configure it while the IP block is held - * in reset). For now I chose to keep this driver away from - * those SoC specific bits and assume the device-tree is - * right and the SCU has been configured properly by pinmux - * or the firmware. - */ - if (priv->is_aspeed && - phy_intf != PHY_INTERFACE_MODE_RMII && - phy_intf != PHY_INTERFACE_MODE_RGMII && - phy_intf != PHY_INTERFACE_MODE_RGMII_ID && - phy_intf != PHY_INTERFACE_MODE_RGMII_RXID && - phy_intf != PHY_INTERFACE_MODE_RGMII_TXID) { - netdev_warn(netdev, - "Unsupported PHY mode %s !\n", - phy_modes(phy_intf)); - } - } - priv->mii_bus->name = "ftgmac100_mdio"; snprintf(priv->mii_bus->id, MII_BUS_ID_SIZE, "%s-%d", pdev->name, pdev->id); @@ -1667,22 +1670,22 @@ static int ftgmac100_setup_mdio(struct net_device *netdev) for (i = 0; i < PHY_MAX_ADDR; i++) priv->mii_bus->irq[i] = PHY_POLL; - err = mdiobus_register(priv->mii_bus); + mdio_np = of_get_child_by_name(np, "mdio"); + if (mdio_np) + err = of_mdiobus_register(priv->mii_bus, mdio_np); + else + err = mdiobus_register(priv->mii_bus); + if (err) { dev_err(priv->dev, "Cannot register MDIO bus!\n"); goto err_register_mdiobus; } - err = ftgmac100_mii_probe(priv, phy_intf); - if (err) { - dev_err(priv->dev, "MII Probe failed!\n"); - goto err_mii_probe; - } + if (mdio_np) + of_node_put(mdio_np); return 0; -err_mii_probe: - mdiobus_unregister(priv->mii_bus); err_register_mdiobus: mdiobus_free(priv->mii_bus); return err; @@ -1836,10 +1839,23 @@ static int ftgmac100_probe(struct platform_device *pdev) } else if (np && of_get_property(np, "phy-handle", NULL)) { struct phy_device *phy; + /* Support "mdio"/"phy" child nodes for ast2400/2500 with + * an embedded MDIO controller. Automatically scan the DTS for + * available PHYs and register them. + */ + if (of_device_is_compatible(np, "aspeed,ast2400-mac") || + of_device_is_compatible(np, "aspeed,ast2500-mac")) { + err = ftgmac100_setup_mdio(netdev); + if (err) + goto err_setup_mdio; + } + phy = of_phy_get_and_connect(priv->netdev, np, &ftgmac100_adjust_link); if (!phy) { dev_err(&pdev->dev, "Failed to connect to phy\n"); + if (priv->mii_bus) + mdiobus_unregister(priv->mii_bus); goto err_setup_mdio; } @@ -1860,6 +1876,14 @@ static int ftgmac100_probe(struct platform_device *pdev) err = ftgmac100_setup_mdio(netdev); if (err) goto err_setup_mdio; + + err = ftgmac100_mii_probe(netdev); + if (err) { + dev_err(priv->dev, "MII probe failed!\n"); + mdiobus_unregister(priv->mii_bus); + goto err_setup_mdio; + } + } if (priv->is_aspeed) {
phy-handle can't be handled well for ast2400/2500 which has an embedded MDIO controller. Add ftgmac100_mdio_setup for ast2400/2500 and initialize PHYs from mdio child node with of_mdiobus_register. Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> --- drivers/net/ethernet/faraday/ftgmac100.c | 114 ++++++++++++++--------- 1 file changed, 69 insertions(+), 45 deletions(-)