| Message ID | C1587837D62D1BC0+20240806082520.29193-1-mengyuanlou@net-swift.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ngbe: Fix phy mode set to external phy | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/apply | fail | Patch does not apply to net-0 |
On 8/6/24 10:25, Mengyuan Lou wrote: > When use rgmmi to attach to external phy, set > PHY_INTERFACE_MODE_RGMII_RXID to phy drivers. > And it is does matter to internal phy. > 107│ * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface 108│ * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay 109│ * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay 110│ * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay Your change effectively disables Internal Tx delay, but your commit message does not tell about that. It also does not tell about why, nor what is wrong in current behavior. > Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.") This commit indeed has introduced the line you are changing, but without explanation, this is not a bugfix. > Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com> > --- > drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c > index ba33a57b42c2..be99ef5833da 100644 > --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c > +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c > @@ -218,7 +218,7 @@ int ngbe_phy_connect(struct wx *wx) > ret = phy_connect_direct(wx->netdev, > wx->phydev, > ngbe_handle_link_change, > - PHY_INTERFACE_MODE_RGMII_ID); > + PHY_INTERFACE_MODE_RGMII_RXID); > if (ret) { > wx_err(wx, "PHY connect failed.\n"); > return ret;
> 2024年8月6日 19:13，Przemek Kitszel <przemyslaw.kitszel@intel.com> 写道： > > On 8/6/24 10:25, Mengyuan Lou wrote: >> When use rgmmi to attach to external phy, set >> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers. >> And it is does matter to internal phy. > > 107│ * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface > 108│ * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay > 109│ * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay > 110│ * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay > > Your change effectively disables Internal Tx delay, but your commit > message does not tell about that. It also does not tell about why, > nor what is wrong in current behavior. > I will add it, when wangxun em Nics are used as a Mac to attach to external phy. We should disable tx delay. >> Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.") > Fixes: bc2426d74aa3 ("net: ngbe: convert phylib to phylink") I just want to fix it both in a1cf597b99a7 and bc2426d74aa3 commits. How can I do it. > This commit indeed has introduced the line you are changing, > but without explanation, this is not a bugfix. > >> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com> >> --- >> drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c >> index ba33a57b42c2..be99ef5833da 100644 >> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c >> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c >> @@ -218,7 +218,7 @@ int ngbe_phy_connect(struct wx *wx) >> ret = phy_connect_direct(wx->netdev, >> wx->phydev, >> ngbe_handle_link_change, >> - PHY_INTERFACE_MODE_RGMII_ID); >> + PHY_INTERFACE_MODE_RGMII_RXID); >> if (ret) { >> wx_err(wx, "PHY connect failed.\n"); >> return ret; >
On Wed, Aug 07, 2024 at 01:42:06PM +0800, mengyuanlou@net-swift.com wrote: > > > > 2024年8月6日 19:13，Przemek Kitszel <przemyslaw.kitszel@intel.com> 写道： > > > > On 8/6/24 10:25, Mengyuan Lou wrote: > >> When use rgmmi to attach to external phy, set > >> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers. > >> And it is does matter to internal phy. > > > > 107│ * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface > > 108│ * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay > > 109│ * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay > > 110│ * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay > > > > Your change effectively disables Internal Tx delay, but your commit > > message does not tell about that. It also does not tell about why, > > nor what is wrong in current behavior. > > > > I will add it, when wangxun em Nics are used as a Mac to attach to external phy. > We should disable tx delay. Why should you disable TX delay? What is providing that delay? Something needs to add a 2ns delay. Does the PCB have an extra long clock line? Andrew
> 2024年8月7日 20:58，Andrew Lunn <andrew@lunn.ch> 写道： > > On Wed, Aug 07, 2024 at 01:42:06PM +0800, mengyuanlou@net-swift.com wrote: >> >> >>> 2024年8月6日 19:13，Przemek Kitszel <przemyslaw.kitszel@intel.com> 写道： >>> >>> On 8/6/24 10:25, Mengyuan Lou wrote: >>>> When use rgmmi to attach to external phy, set >>>> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers. >>>> And it is does matter to internal phy. >>> >>> 107│ * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface >>> 108│ * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay >>> 109│ * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay >>> 110│ * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay >>> >>> Your change effectively disables Internal Tx delay, but your commit >>> message does not tell about that. It also does not tell about why, >>> nor what is wrong in current behavior. >>> >> >> I will add it, when wangxun em Nics are used as a Mac to attach to external phy. >> We should disable tx delay. > > Why should you disable TX delay? > > What is providing that delay? Something needs to add a 2ns delay. Does > the PCB have an extra long clock line? > Mac only has add the Tx delay，and it can not be modified. So just disable TX delay in PHY. Mengyuan Lou > Andrew
On Thu, Aug 08, 2024 at 03:23:58PM +0800, mengyuanlou@net-swift.com wrote: > > > > 2024年8月7日 20:58，Andrew Lunn <andrew@lunn.ch> 写道： > > > > On Wed, Aug 07, 2024 at 01:42:06PM +0800, mengyuanlou@net-swift.com wrote: > >> > >> > >>> 2024年8月6日 19:13，Przemek Kitszel <przemyslaw.kitszel@intel.com> 写道： > >>> > >>> On 8/6/24 10:25, Mengyuan Lou wrote: > >>>> When use rgmmi to attach to external phy, set > >>>> PHY_INTERFACE_MODE_RGMII_RXID to phy drivers. > >>>> And it is does matter to internal phy. > >>> > >>> 107│ * @PHY_INTERFACE_MODE_RGMII: Reduced gigabit media-independent interface > >>> 108│ * @PHY_INTERFACE_MODE_RGMII_ID: RGMII with Internal RX+TX delay > >>> 109│ * @PHY_INTERFACE_MODE_RGMII_RXID: RGMII with Internal RX delay > >>> 110│ * @PHY_INTERFACE_MODE_RGMII_TXID: RGMII with Internal RX delay > >>> > >>> Your change effectively disables Internal Tx delay, but your commit > >>> message does not tell about that. It also does not tell about why, > >>> nor what is wrong in current behavior. > >>> > >> > >> I will add it, when wangxun em Nics are used as a Mac to attach to external phy. > >> We should disable tx delay. > > > > Why should you disable TX delay? > > > > What is providing that delay? Something needs to add a 2ns delay. Does > > the PCB have an extra long clock line? > > > > Mac only has add the Tx delay，and it can not be modified. > > So just disable TX delay in PHY. So slowly we are starting to understand the problem.... You need to document all this in the justification of the patch. This asymmetric setup is also very unusual, so you should add a comment in the code explaining it. For Linux in general, we let the PHY add the delays, and if the MAC can add delays, we generally don't make use of that ability. There are a few exceptions, because there are a few PHY which lack the capabilities to add delays. Anybody with a general Linux PHY background are going to look at your code and question it, because it is very odd. Hence the need for a comment. Andrew
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c index ba33a57b42c2..be99ef5833da 100644 --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c @@ -218,7 +218,7 @@ int ngbe_phy_connect(struct wx *wx) ret = phy_connect_direct(wx->netdev, wx->phydev, ngbe_handle_link_change, - PHY_INTERFACE_MODE_RGMII_ID); + PHY_INTERFACE_MODE_RGMII_RXID); if (ret) { wx_err(wx, "PHY connect failed.\n"); return ret;
When use rgmmi to attach to external phy, set PHY_INTERFACE_MODE_RGMII_RXID to phy drivers. And it is does matter to internal phy. Fixes: a1cf597b99a7 ("net: ngbe: Add ngbe mdio bus driver.") Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com> --- drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)