Re: [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500
> + 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
Re: [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500
On Wed, 2020-10-14 at 05:23 +, Joel Stanley wrote: > Hi Ivan, > > On Tue, 13 Oct 2020 at 12:38, Ivan Mikhaylov 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 > > --- > > 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 */ > > -
Re: [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500
Hi Ivan, On Tue, 13 Oct 2020 at 12:38, Ivan Mikhaylov 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 > --- > 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