| Message ID | 20241011081633.2171603-1-jacky_chou@aspeedtech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ftgmac100: refactor getting phy device handle | expand |
On Fri, Oct 11, 2024 at 04:16:33PM +0800, Jacky Chou wrote: > The ftgmac100 supports NC-SI mode, dedicated PHY and fixed-link > PHY. The dedicated PHY is using the phy_handle property to get > phy device handle and the fixed-link phy is using the fixed-link > property to register a fixed-link phy device. > > In of_phy_get_and_connect function, it help driver to get and register > these PHYs handle. > Therefore, here refactors this part by using of_phy_get_and_connect. > > Fixes: 38561ded50d0 ("net: ftgmac100: support fixed link") > Fixes: 39bfab8844a0 ("net: ftgmac100: Add support for DT phy-handle property") Fixes: implies something is broken. What is actually wrong with this code? What sort of problem does a user see? > - phy_support_asym_pause(phy); > + if (of_get_property(np, "phy-handle", NULL)) > + phy_support_asym_pause(phy); This is probably wrong. This is the MAC layer telling phylib that the MAC supports asym pause. It should makes no difference to the MAC what sort of PHY is being used, all the MAC is looking at/sending is pause frames. Andrew --- pw-bot: cr
Hi Andrew, Thanks for the review. > > The ftgmac100 supports NC-SI mode, dedicated PHY and fixed-link PHY. > > The dedicated PHY is using the phy_handle property to get phy device > > handle and the fixed-link phy is using the fixed-link property to > > register a fixed-link phy device. > > > > In of_phy_get_and_connect function, it help driver to get and register > > these PHYs handle. > > Therefore, here refactors this part by using of_phy_get_and_connect. > > > > Fixes: 38561ded50d0 ("net: ftgmac100: support fixed link") > > Fixes: 39bfab8844a0 ("net: ftgmac100: Add support for DT phy-handle > > property") > > Fixes: implies something is broken. What is actually wrong with this code? > What sort of problem does a user see? I am apology what I am not sure if using fixes to send my patch. I just refactor this part and let the relevant people know that I have adjusted their code. This patch is not a bug fix. > > > - phy_support_asym_pause(phy); > > + if (of_get_property(np, "phy-handle", NULL)) > > + phy_support_asym_pause(phy); > > This is probably wrong. This is the MAC layer telling phylib that the MAC > supports asym pause. It should makes no difference to the MAC what sort of > PHY is being used, all the MAC is looking at/sending is pause frames. Agree. It is MAC function, not PHY. I will adjust it on next version. Thank you.
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index 0b61f548fd18..ae0235a7a74e 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1918,35 +1918,17 @@ static int ftgmac100_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Connecting PHY failed\n"); goto err_phy_connect; } - } else if (np && of_phy_is_fixed_link(np)) { - struct phy_device *phy; - - err = of_phy_register_fixed_link(np); - if (err) { - dev_err(&pdev->dev, "Failed to register fixed PHY\n"); - goto err_phy_connect; - } - - phy = of_phy_get_and_connect(priv->netdev, np, - &ftgmac100_adjust_link); - if (!phy) { - dev_err(&pdev->dev, "Failed to connect to fixed PHY\n"); - of_phy_deregister_fixed_link(np); - err = -EINVAL; - goto err_phy_connect; - } - - /* Display what we found */ - phy_attached_info(phy); - } else if (np && of_get_property(np, "phy-handle", NULL)) { + } else if (np && (of_phy_is_fixed_link(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")) { + if (of_get_property(np, "phy-handle", NULL) && + (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; @@ -1963,7 +1945,8 @@ static int ftgmac100_probe(struct platform_device *pdev) /* Indicate that we support PAUSE frames (see comment in * Documentation/networking/phy.rst) */ - phy_support_asym_pause(phy); + if (of_get_property(np, "phy-handle", NULL)) + phy_support_asym_pause(phy); /* Display what we found */ phy_attached_info(phy);
The ftgmac100 supports NC-SI mode, dedicated PHY and fixed-link PHY. The dedicated PHY is using the phy_handle property to get phy device handle and the fixed-link phy is using the fixed-link property to register a fixed-link phy device. In of_phy_get_and_connect function, it help driver to get and register these PHYs handle. Therefore, here refactors this part by using of_phy_get_and_connect. Fixes: 38561ded50d0 ("net: ftgmac100: support fixed link") Fixes: 39bfab8844a0 ("net: ftgmac100: Add support for DT phy-handle property") Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> --- drivers/net/ethernet/faraday/ftgmac100.c | 31 ++++++------------------ 1 file changed, 7 insertions(+), 24 deletions(-)