* [PATCH 0/1] add ast2400/2500 phy-handle support
@ 2020-10-13 12:40 Ivan Mikhaylov
2020-10-13 12:40 ` [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500 Ivan Mikhaylov
0 siblings, 1 reply; 5+ messages in thread
From: Ivan Mikhaylov @ 2020-10-13 12:40 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Po-Yu Chuang
Cc: Ivan Mikhaylov, netdev, linux-kernel, openbmc
This patch introduces ast2400/2500 phy-handle support with an embedded
MDIO controller. At the current moment it is not possible to set options
with this format on ast2400/2500:
mac {
phy-handle = <&phy>;
phy-mode = "rgmii";
mdio {
#address-cells = <1>;
#size-cells = <0>;
phy: ethernet-phy@0 {
compatible = "ethernet-phy-idxxxx.yyyy";
reg = <0>;
};
};
};
The patch fixes it and gets possible PHYs and register them with
of_mdiobus_register.
Ivan Mikhaylov (1):
net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500
drivers/net/ethernet/faraday/ftgmac100.c | 114 ++++++++++++++---------
1 file changed, 69 insertions(+), 45 deletions(-)
--
2.21.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500
2020-10-13 12:40 [PATCH 0/1] add ast2400/2500 phy-handle support Ivan Mikhaylov
@ 2020-10-13 12:40 ` Ivan Mikhaylov
2020-10-14 5:23 ` Joel Stanley
2020-10-17 2:10 ` Andrew Lunn
0 siblings, 2 replies; 5+ messages in thread
From: Ivan Mikhaylov @ 2020-10-13 12:40 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Po-Yu Chuang
Cc: Ivan Mikhaylov, netdev, linux-kernel, openbmc
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(-)
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) {
--
2.21.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500
2020-10-13 12:40 ` [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500 Ivan Mikhaylov
@ 2020-10-14 5:23 ` Joel Stanley
2020-10-14 13:24 ` Ivan Mikhaylov
2020-10-17 2:10 ` Andrew Lunn
1 sibling, 1 reply; 5+ messages in thread
From: Joel Stanley @ 2020-10-14 5:23 UTC (permalink / raw)
To: Ivan Mikhaylov
Cc: David S . Miller, Jakub Kicinski, Po-Yu Chuang, netdev,
OpenBMC Maillist, Linux Kernel Mailing List
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500
2020-10-14 5:23 ` Joel Stanley
@ 2020-10-14 13:24 ` Ivan Mikhaylov
0 siblings, 0 replies; 5+ messages in thread
From: Ivan Mikhaylov @ 2020-10-14 13:24 UTC (permalink / raw)
To: Joel Stanley
Cc: David S . Miller, Jakub Kicinski, Po-Yu Chuang, netdev,
OpenBMC Maillist, Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500
2020-10-13 12:40 ` [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500 Ivan Mikhaylov
2020-10-14 5:23 ` Joel Stanley
@ 2020-10-17 2:10 ` Andrew Lunn
1 sibling, 0 replies; 5+ messages in thread
From: Andrew Lunn @ 2020-10-17 2:10 UTC (permalink / raw)
To: Ivan Mikhaylov
Cc: David S . Miller, Jakub Kicinski, Po-Yu Chuang, netdev,
linux-kernel, openbmc
> + 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-17 5:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-13 12:40 [PATCH 0/1] add ast2400/2500 phy-handle support Ivan Mikhaylov
2020-10-13 12:40 ` [PATCH 1/1] net: ftgmac100: add handling of mdio/phy nodes for ast2400/2500 Ivan Mikhaylov
2020-10-14 5:23 ` Joel Stanley
2020-10-14 13:24 ` Ivan Mikhaylov
2020-10-17 2:10 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).