Re: [PATCH net-next v6 4/9] net: dsa: realtek: rtl8365mb: set RGMII mode for family D
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-20 01:41:30
Also in:
lkml
Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin. Low priority / nit picks for your consideration: - [Low] rtl8365mb_phylink_get_caps() drops RGMII without logging anything when TOP_CON0.MAC4_SEL_EXT1 (BIT(12) of 0x1d70) is set on family D… - [Low] At this commit the two parts of the code describe EXT1 differently. - [Low] On family D EXT1, rtl8365mb_ext_config_rgmii() writes in this order: RGMXF delays, DIGITAL_INTERFACE_SELECT = RGMII, clear… -- net: dsa: realtek: rtl8365mb: set RGMII mode for family D This patch adds family D handling for RGMII on extension interface 1. It clears the EXT1 RGMII TX delay field in EXT_TXC_DLY, binds the pins to MAC7 through TOP_CON0, and disables SerDes 1. RGMII is also no longer advertised for port 7 in rtl8365mb_phylink_get_caps() when TOP_CON0 shows that MAC4 owns EXT1.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c index 2dabc4da861f4..f00c1c24014a0 100644 --- a/drivers/net/dsa/realtek/rtl8365mb_main.c +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1270,6 +1290,33 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port, if (ret) return ret; + if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D && + extint->id == 1) {
[Severity: Low] At this point in the series, does this model of EXT1 disagree with the existing SerDes comment and constants near the top of rtl8365mb_main.c? /* The embedded SerDes can only be muxed to external interface 1 (MAC8), * which is port 6. */ #define RTL8365MB_SDS_EXT_INTERFACE_ID 1 #define RTL8365MB_SDS_EXT_INTERFACE_PORT 6 Those apply to every family. The new family D code treats extint id 1 as the RGMII interface of MAC7 and disables SerDes 1 on it. Later patches in the series fix this. The comment is rewritten to say family D has the SerDes on external interface 0 (MAC6), and rtl8365mb_pcs_config() skips the EXT1 DIGITAL_INTERFACE_SELECT write on family D. Family D also can't be reached until the last patch. Could the comment be qualified in this patch so both descriptions agree at every point in the series?
quoted hunk ↗ jump to hunk
+ ret = regmap_update_bits(priv->map, + RTL8365MB_D_REG_EXT_TXC_DLY, + RTL8365MB_D_EXT1_RGMII_TX_DLY_MASK, 0); + if (ret) + return ret; + + /* Bind the RGMII/MII pins to MAC7. MAC4 does not own them: + * RGMII is not advertised otherwise, see + * rtl8365mb_phylink_get_caps(). + */ + ret = regmap_update_bits(priv->map, + RTL8365MB_D_REG_TOP_CON0, + RTL8365MB_D_MAC7_SEL_EXT1_MASK, + RTL8365MB_D_MAC7_SEL_EXT1_MASK); + if (ret) + return ret; + + ret = regmap_update_bits(priv->map, + RTL8365MB_D_REG_SDS1_MISC0, + RTL8365MB_D_SDS_MISC_MODE_FIELD_MASK, + RTL8365MB_D_PORT_SDS_MODE_DISABLE); + if (ret) + return ret; + }
[Severity: Low] Is this write order intended? The TOP_CON0 pin mux and the SDS1_MISC0 SerDes 1 disable are written only after DIGITAL_INTERFACE_SELECT has already been switched to RGMII just above. Suppose SerDes 1 was active before mac_config (from strap or EEPROM, for example), and the hardware drives the shared pins as soon as RGMII is selected. Could RGMII and SerDes 1 then both be set up on the same pins for a few SMI transactions? Nothing is unwound if one of these writes fails partway through, either. rtl8365mb_phylink_mac_config() only logs the error: ret = rtl8365mb_ext_config_rgmii(priv, port, state->interface); if (ret) dev_err(priv->dev, "failed to configure RGMII mode on port %d: %pe\n", port, ERR_PTR(ret)); After that, rtl8365mb_phylink_mac_link_up() can still force the MAC link up through rtl8365mb_ext_config_forcemode(), with the mux or TX delay only partly programmed. The existing RGMXF and DIGITAL_INTERFACE_SELECT writes, and rtl8365mb_pcs_config(), behave the same way. The concern also depends on hardware behaviour that hasn't been confirmed. Does the vendor RTL8367D code set the mux and disable SerDes 1 before it selects RGMII mode? [ ... ]
quoted hunk ↗ jump to hunk
@@ -1802,8 +1851,23 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port, * which remain unimplemented. */ - if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_RGMII) - phy_interface_set_rgmii(config->supported_interfaces); + if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_RGMII) { + /* On family D the EXT1 pins are shared with MAC4 (UTP port 4). + * If MAC4 owns them, RGMII on this port cannot work. + */ + if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D && + extint->id == 1) { + ret = regmap_read(priv->map, RTL8365MB_D_REG_TOP_CON0, &data); + if (ret) + dev_warn(priv->dev, + "failed to read TOP_CON0, RGMII on port %d disabled: %pe\n", + port, ERR_PTR(ret)); + else if (!(data & RTL8365MB_D_MAC4_SEL_EXT1_MASK)) + phy_interface_set_rgmii(config->supported_interfaces);
[Severity: Low]
When MAC4_SEL_EXT1 is set, RGMII is dropped here with no message. Only
the regmap_read() failure branch warns.
The commit message says:
If the internal MAC4 block occupies extension 1, do not advertise RGMII
for port 7 in phylink_get_caps(), so the configuration is refused before
any EXT1 register is touched.
How would a user find out why it was refused?
The last patch in the series adds the RTL8367S-VB entry, where port 7 is
{ 7, 1, MII|TMII|RMII|RGMII }. MII, TMII and RMII are still not
implemented in rtl8365mb_phylink_mac_config(), so supported_interfaces
ends up empty. phylink_create() then fails with only the generic message:
if (phy_interface_empty(config->supported_interfaces)) {
dev_err(config->dev,
"phylink: error: empty supported_interfaces\n");
return ERR_PTR(-EINVAL);
}
If port 7 is the CPU port, DSA tree setup fails. Nothing in the log says
that MAC4 (UTP port 4) owns the EXT1 pins.
Could this branch print a dev_warn() that names the MAC4/EXT1 pin
conflict?
quoted hunk ↗ jump to hunk
+ } else { + phy_interface_set_rgmii(config->supported_interfaces); + } + }
-- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/5a4715dd-89e0-4868-9ffb-c8fb73b18d04%40yahoo.com