| Message ID | 20221223015029.24978-2-biao.huang@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: mt8195: Add Ethernet controller | expand |
Il 23/12/22 02:50, Biao Huang ha scritto: > In current driver, MAC will always enable 2ns delay in RGMII mode, > but that will lead to transmission failures for "rgmii-id"/"rgmii-txid" > cases. > > Modify the implementation of fix_mac_speed() to ensure the 2ns delay > will only take effect for "rgmii-rxid"/"rgmii" cases, then user can > choose phy-mode freely. > > Fixes: f2d356a6ab71 ("stmmac: dwmac-mediatek: add support for mt8195") > Signed-off-by: Biao Huang <biao.huang@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Dear Andrew, Thanks for your comments. On Fri, 2022-12-23 at 15:18 +0100, Andrew Lunn wrote: > On Fri, Dec 23, 2022 at 09:50:28AM +0800, Biao Huang wrote: > > In current driver, MAC will always enable 2ns delay in RGMII mode, > > but that will lead to transmission failures for "rgmii-id"/"rgmii- > > txid" > > cases. > > > > Modify the implementation of fix_mac_speed() to ensure the 2ns > > delay > > will only take effect for "rgmii-rxid"/"rgmii" cases, then user can > > choose phy-mode freely. > > This does not seem correct. There are three ways the delays can be > added: > > 1) The MAC > 2) The PHY > 3) Extra long lines on the board. > > What the four RGMII modes tell you is what is needed in addition to > whatever the board provides. So it describes the combination of 1) > and > 2). Your board does not appear to be applying any delays, so you > should be using rgmii-id. > > The MAC and PHY driver then need to decide how to add these delays, > and in most cases, the MAC does nothing, and passes phy-mode to the > PHY and the PHY adds the delay. > > The MAC can add delays, but if it does, it need to mask out the > delays > it added to the value passed to the PHY. Otherwise the PHY will add > the delay as well. in the ethernet-controller.yaml, 77 # RX and TX delays are added by the MAC when required 78 - rgmii 79 80 # RGMII with internal RX and TX delays provided by the PHY, 81 # the MAC should not add the RX or TX delays in this case 82 - rgmii-id 83 84 # RGMII with internal RX delay provided by the PHY, the MAC 85 # should not add an RX delay in this case 86 - rgmii-rxid 87 88 # RGMII with internal TX delay provided by the PHY, the MAC 89 # should not add an TX delay in this case 90 - rgmii-txid so, why don't MAC set delay according to rgmii-** ? rgmii-rxid --> mac set tx, but not set rx delay rgmii-txid --> mac set rx, but not set tx delay ... and PHY seems set delay according to rgmii-** in their dirver. > > > > > - if ((phy_interface_mode_is_rgmii(priv_plat->phy_mode))) { > > + switch (priv_plat->phy_mode) { > > + case PHY_INTERFACE_MODE_RGMII: > > + case PHY_INTERFACE_MODE_RGMII_RXID: > > /* prefer 2ns fixed delay which is controlled by > > TXC_PHASE_CTRL, > > * when link speed is 1Gbps with RGMII interface, > > * Fall back to delay macro circuit for 10/100Mbps link > > speed. > > So this is wrong. PHY_INTERFACE_MODE_RGMII means the board is adding > the delay via long lines. You should not be added any delay at all. > > For PHY_INTERFACE_MODE_RGMII_RXID, you need to mask the RXID bit from > phy_mode when connecting the MAC to the PHY. Otherwise the PHY is > going to add this delay as well. These lines only set tx delay when phy_mode is RGMII/RGMI_RXID, when phy will not add tx delay internally. We don't want to restrict phy-mode to rgmii-id only, so I don't think we need mask RXID bit, then, User can use rgmii-rxid/-txid/-id as their will. If I misunderstood, please correct me. Thanks. > > Andrew Best Regards! Biao
> We don't want to restrict phy-mode to rgmii-id only, so I don't think > we need mask RXID bit, then, User can use rgmii-rxid/-txid/-id as their > will. The phy-mode is a property of the board. It describes what delays are required, in addition to what the board itself provides via extra long lines. As such, there is no choice, it is set by physics. How you implement those delays is then up to the MAC and PHY. 95% of all MAC drivers simply pass phy-mode to the PHY and let the PHY add the delays. The Renesas MAC driver is one of the exceptions, it often adds the delays, and then passes a different phy-mode to the PHY in order that there are not double delays. So you can be different to everybody else, have MAC do one delay, and the PHY do the other. Just be aware that nobody else does this, no DT developers will expect it, etc. Or you can just do what everybody else does and let the PHY add the delays. What some MACs do is implement rx-internal-delay-ps and tx-internal-delay-ps These allow fine tuning of the delays. These are expected to be small values, adding 10s or 100s of pico seconds. They are not expected to be 2000ps. Andrew
Dear Andrew, Thanks for your comments~ On Mon, 2022-12-26 at 16:19 +0100, Andrew Lunn wrote: > > We don't want to restrict phy-mode to rgmii-id only, so I don't > > think > > we need mask RXID bit, then, User can use rgmii-rxid/-txid/-id as > > their > > will. > > The phy-mode is a property of the board. It describes what delays are > required, in addition to what the board itself provides via extra > long > lines. As such, there is no choice, it is set by physics. > > How you implement those delays is then up to the MAC and PHY. 95% of > all MAC drivers simply pass phy-mode to the PHY and let the PHY add > the delays. The Renesas MAC driver is one of the exceptions, it often > adds the delays, and then passes a different phy-mode to the PHY in > order that there are not double delays. > > So you can be different to everybody else, have MAC do one delay, and > the PHY do the other. Just be aware that nobody else does this, no DT > developers will expect it, etc. > > Or you can just do what everybody else does and let the PHY add the > delays. > > What some MACs do is implement rx-internal-delay-ps and > tx-internal-delay-ps These allow fine tuning of the delays. These are > expected to be small values, adding 10s or 100s of pico seconds. They > are not expected to be 2000ps. OK, as to this series, Is the following modification enough? 1. phy-mode = rgmii-id; in dts. 2. remove the fix_mac_speed() in driver. > > Andrew Best Regards! Biao
> OK, as to this series, Is the following modification enough? > 1. phy-mode = rgmii-id; in dts. > 2. remove the fix_mac_speed() in driver. Yep, that is good. Andrew
Dear Andrew, Thanks for your comments~ On Tue, 2022-12-27 at 17:20 +0100, Andrew Lunn wrote: > > OK, as to this series, Is the following modification enough? > > 1. phy-mode = rgmii-id; in dts. > > 2. remove the fix_mac_speed() in driver. > > Yep, that is good. OK, will update in next send. > > Andrew Best Regards! Biao
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c index d42e1afb6521..cde4fb81d596 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c @@ -447,7 +447,9 @@ static void mt8195_fix_mac_speed(void *priv, unsigned int speed) { struct mediatek_dwmac_plat_data *priv_plat = priv; - if ((phy_interface_mode_is_rgmii(priv_plat->phy_mode))) { + switch (priv_plat->phy_mode) { + case PHY_INTERFACE_MODE_RGMII: + case PHY_INTERFACE_MODE_RGMII_RXID: /* prefer 2ns fixed delay which is controlled by TXC_PHASE_CTRL, * when link speed is 1Gbps with RGMII interface, * Fall back to delay macro circuit for 10/100Mbps link speed. @@ -462,6 +464,9 @@ static void mt8195_fix_mac_speed(void *priv, unsigned int speed) MT8195_RGMII_TXC_PHASE_CTRL); else mt8195_set_delay(priv_plat); + break; + default: + break; } }
In current driver, MAC will always enable 2ns delay in RGMII mode, but that will lead to transmission failures for "rgmii-id"/"rgmii-txid" cases. Modify the implementation of fix_mac_speed() to ensure the 2ns delay will only take effect for "rgmii-rxid"/"rgmii" cases, then user can choose phy-mode freely. Fixes: f2d356a6ab71 ("stmmac: dwmac-mediatek: add support for mt8195") Signed-off-by: Biao Huang <biao.huang@mediatek.com> --- drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)