| Message ID | 20241011082827.2205979-1-jacky_chou@aspeedtech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ftgmac100: corrcet the phy interface of NC-SI mode | expand |
On Fri, Oct 11, 2024 at 04:28:27PM +0800, Jacky Chou wrote: > In NC-SI specification, NC-SI is using RMII, not MII. > > Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for NC-SI") > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> > --- > drivers/net/ethernet/faraday/ftgmac100.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c > index ae0235a7a74e..85fea13b2879 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -1913,7 +1913,7 @@ static int ftgmac100_probe(struct platform_device *pdev) > goto err_phy_connect; > } > err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, > - PHY_INTERFACE_MODE_MII); > + PHY_INTERFACE_MODE_RMII); > if (err) { > dev_err(&pdev->dev, "Connecting PHY failed\n"); > goto err_phy_connect; I'm a but confused here. Please could you expand the commit message. When i look at the code: phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL); err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, PHY_INTERFACE_MODE_MII); if (err) { dev_err(&pdev->dev, "Connecting PHY failed\n"); goto err_phy_connect; } The phy being connected to is a fixed PHY. So the interface mode should not matter, at least to the PHY, since there is no physical PHY. Does the MAC driver get this value returned to it, e.g. as part of ftgmac100_adjust_link, and the MAC then configures itself into the wrong interface mode? For a patch with a Fixes: it is good to describe the problem the user sees. Andrew
Hi Andrew, Thanks for the review. > > In NC-SI specification, NC-SI is using RMII, not MII. > > > > Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for > > NC-SI") > > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> > > --- > > drivers/net/ethernet/faraday/ftgmac100.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > > b/drivers/net/ethernet/faraday/ftgmac100.c > > index ae0235a7a74e..85fea13b2879 100644 > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > @@ -1913,7 +1913,7 @@ static int ftgmac100_probe(struct platform_device > *pdev) > > goto err_phy_connect; > > } > > err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, > > - PHY_INTERFACE_MODE_MII); > > + PHY_INTERFACE_MODE_RMII); > > if (err) { > > dev_err(&pdev->dev, "Connecting PHY failed\n"); > > goto err_phy_connect; > > I'm a but confused here. Please could you expand the commit message. When i > look at the code: > > phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL); > err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, > PHY_INTERFACE_MODE_MII); > if (err) { > dev_err(&pdev->dev, "Connecting PHY failed\n"); > goto err_phy_connect; > } > > The phy being connected to is a fixed PHY. So the interface mode should not > matter, at least to the PHY, since there is no physical PHY. Does the MAC driver > get this value returned to it, e.g. as part of ftgmac100_adjust_link, and the > MAC then configures itself into the wrong interface mode? > > For a patch with a Fixes: it is good to describe the problem the user sees. Although it is connected to a fixed PHY and do not care what interface mode is, the driver still configures the correct interface. In the ftgmac100 driver, the MAC driver does not actually need to know the interface mode connecting the MAC and PHY. The driver just needs to get some information from the PHY, like speed, duplex and so on, to configure the MAC. Perhaps it is not matter on PHY interface to MAC, it should correct to the correct interface mode for code. Jacky
On Mon, Oct 14, 2024 at 02:23:40AM +0000, Jacky Chou wrote: > Hi Andrew, > > Thanks for the review. > > > > In NC-SI specification, NC-SI is using RMII, not MII. > > > > > > Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for > > > NC-SI") > > > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> > > > --- > > > drivers/net/ethernet/faraday/ftgmac100.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > > > b/drivers/net/ethernet/faraday/ftgmac100.c > > > index ae0235a7a74e..85fea13b2879 100644 > > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > > @@ -1913,7 +1913,7 @@ static int ftgmac100_probe(struct platform_device > > *pdev) > > > goto err_phy_connect; > > > } > > > err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, > > > - PHY_INTERFACE_MODE_MII); > > > + PHY_INTERFACE_MODE_RMII); > > > if (err) { > > > dev_err(&pdev->dev, "Connecting PHY failed\n"); > > > goto err_phy_connect; > > > > I'm a but confused here. Please could you expand the commit message. When i > > look at the code: > > > > phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL); > > err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, > > PHY_INTERFACE_MODE_MII); > > if (err) { > > dev_err(&pdev->dev, "Connecting PHY failed\n"); > > goto err_phy_connect; > > } > > > > The phy being connected to is a fixed PHY. So the interface mode should not > > matter, at least to the PHY, since there is no physical PHY. Does the MAC driver > > get this value returned to it, e.g. as part of ftgmac100_adjust_link, and the > > MAC then configures itself into the wrong interface mode? > > > > For a patch with a Fixes: it is good to describe the problem the user sees. > > Although it is connected to a fixed PHY and do not care what interface mode is, > the driver still configures the correct interface. > > In the ftgmac100 driver, the MAC driver does not actually need to know the interface mode > connecting the MAC and PHY. > The driver just needs to get some information from the PHY, like speed, duplex and so on, to > configure the MAC. > > Perhaps it is not matter on PHY interface to MAC, it should correct to the correct interface mode > for code. So nothing is actually broken which a user would notice. So please drop the Fixes: tag, and submit this for net-next. Andrew
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index ae0235a7a74e..85fea13b2879 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1913,7 +1913,7 @@ static int ftgmac100_probe(struct platform_device *pdev) goto err_phy_connect; } err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, - PHY_INTERFACE_MODE_MII); + PHY_INTERFACE_MODE_RMII); if (err) { dev_err(&pdev->dev, "Connecting PHY failed\n"); goto err_phy_connect;
In NC-SI specification, NC-SI is using RMII, not MII. Fixes: e24a6c874601 ("net: ftgmac100: Get link speed and duplex for NC-SI") Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> --- drivers/net/ethernet/faraday/ftgmac100.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)