Re: [PATCH net-next v2 2/2] net: phy: realtek: support unaligned RTL8224 PHY packages
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-08 08:18:37
Also in:
linux-devicetree, lkml
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 1 · Low: 1 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] The RTL8224 text added to realtek,rtl82xx.yaml (preceding commit "dt-bindings: net: realtek,rtl82xx: document RTL8224 packages")… - [Low] The new RTL8224 package example in realtek,rtl82xx.yaml (preceding commit "dt-bindings: net: realtek,rtl82xx: document RTL8224… -- net: phy: realtek: support unaligned RTL8224 PHY packages This commit makes the RTL8224 driver take the package base address from the reg property of an ethernet-phy-package parent node, rather than always rounding the PHY address down to a multiple of four. The port index relative to the package is stored in a new struct rtl8224_priv and used to pick the MDI pair order and polarity fields. When there is no package node, the old aligned-address assumption still applies.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c index e14960192e791..91cf34d134202 100644 --- a/drivers/net/phy/realtek/realtek_main.c +++ b/drivers/net/phy/realtek/realtek_main.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2783,9 +2789,42 @@ static int rtl8224_config_init(struct phy_device *phydev) static int rtl8224_probe(struct phy_device *phydev) { - /* Chip exposes 4 ports, join all of them in the same package */ - return devm_phy_package_join(&phydev->mdio.dev, phydev, - phydev->mdio.addr & ~3, 0); + struct device *dev = &phydev->mdio.dev; + u32 base_addr = phydev->mdio.addr & ~3; + struct rtl8224_priv *priv; + struct device_node *np; + int ret;
[ ... ]
quoted hunk ↗ jump to hunk
+ np = of_get_parent(dev->of_node); + if (of_node_name_eq(np, "ethernet-phy-package")) { + ret = of_property_read_u32(np, "reg", &base_addr); + of_node_put(np); + if (ret) + return dev_err_probe(dev, ret, + "Missing or invalid PHY package reg\n"); + + if (phydev->mdio.addr < base_addr || + phydev->mdio.addr - base_addr >= 4) + return dev_err_probe(dev, -EINVAL, + "PHY address outside RTL8224 package\n"); + + ret = devm_of_phy_package_join(dev, phydev, 0); + } else { + of_node_put(np); + /* Without a package node, assume four aligned PHY addresses. */ + ret = devm_phy_package_join(dev, phydev, base_addr, 0);
[Severity: Medium]
What happens in this fallback when an RTL8224 is unaligned and has no
ethernet-phy-package parent?
The binding text added in the preceding commit "dt-bindings: net:
realtek,rtl82xx: document RTL8224 packages" says the package node is
optional:
Its PHY nodes may be grouped under an ethernet-phy-package node ...
This address need not be a multiple of four.
That text never says the package node is required when the first PHY
address is not a multiple of four. So a DT for the Hasivo
S1100WP-8GT-2S+ layout (chips at 1-4 and 5-8) that has no package nodes
still passes the schema.
For that DT, this branch uses base_addr = addr & ~3 with no check:
PHYs 1-3 join base 0, which is not part of either chip
PHY 4 joins base 4 with port_offset 0
PHYs 5-7 (ports 0-2 of chip B) also join base 4, with offsets 1-3
rtl8224_package_modify_mmd() then does a read-modify-write of
RTL8224_VND1_MDI_PAIR_SWAP and RTL8224_VND1_MDI_POLARITY_SWAP at package
address 0:
val = __phy_package_read_mmd(phydev, 0, devad, regnum);
...
ret = __phy_package_write_mmd(phydev, 0, devad, regnum, val);
Wouldn't chip B's pair order and polarity settings end up in chip A's
registers? They would also overwrite the settings for chip A port 3, and
nothing reports an error or warning.
Commit 330296ea9e15 ("net: phy: realtek: add RTL8224 pair order
support") says the swap "has to be set correctly for each of the 4 ports
before the chip can detect a link". The affected copper ports may
therefore fail to link.
Could the binding require the package node when the first PHY address
is not a multiple of four? Alternatively, could this fallback reject or
warn about the ambiguous case?
quoted hunk ↗ jump to hunk
+ } + if (ret) + return dev_err_probe(dev, ret, "Failed to join PHY package\n");
[Severity: Low] This isn't a bug, but I have a question about the RTL8224 package example added to realtek,rtl82xx.yaml in the preceding commit "dt-bindings: net: realtek,rtl82xx: document RTL8224 packages". The only compatible on each child PHY in that example is: compatible = "ethernet-phy-ieee802.3-c45"; The schema has no explicit select, and its compatible enum lists only ethernet-phy-id001c.xxxx values. Doesn't that mean dt-schema never applies realtek,rtl82xx.yaml to these nodes? They would then be checked only against ethernet-phy-package.yaml and ethernet-phy.yaml. If so, the example never exercises the RTL8224-specific rules in this file: unevaluatedProperties: false, the leds: false rule and the vendor properties. A wrong property in the example would still pass dt_binding_check. This is partly an existing limitation. The first example has no compatible either, and cameo-rtl9302c-2x-rtl8224-2xge.dts also uses only the c45 compatible for RTL8224.
quoted hunk ↗ jump to hunk
+ + priv->port_offset = phydev->mdio.addr - base_addr; + phydev->priv = priv; + + return 0; }
-- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005201540.913977-1-oskari%40lemmela.net