[PATCH v5 net-next] net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[PATCH v5 net-next] net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[Caleb James DeLisle]

The EcoNet EN751221 multi-chip module implementation of the MT7530
requires some additional configuration of the PHYs on startup.
The reason for this is not known, but it is possible that it has
to do with the fact that the EN751221 MCM implementation of the
MT7530 runs at an abnormal PLL frequency (362.5Mhz).

Detect whether the MT7530 PHY is attached to the MDIO bus of an
EcoNet EN751221 switch and if so, apply the necessary register
updates. Additionally, never attempt to configure an MT7530
identified PHY which does not have gigabit support because the same
ID is used for another (FE) PHY.

Co-developed-by: Matheus Sampaio Queiroga <srherobrine20@gmail.com>
Signed-off-by: Matheus Sampaio Queiroga <srherobrine20@gmail.com>
Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/net/phy/mediatek/mtk-ge.c | 81 +++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
index 96d8ac5154e5..eddb66218d7c 100644
--- a/drivers/net/phy/mediatek/mtk-ge.c
+++ b/drivers/net/phy/mediatek/mtk-ge.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0+
+#include <linux/of.h>
 #include <linux/bitfield.h>
 #include <linux/module.h>
 #include <linux/phy.h>
@@ -101,6 +102,69 @@ static int mt7530_phy_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+/*
+ * The EcoNet EN751221 "G" multi-chip module MT7530 requires additional PHY
+ * configuration.
+ */
+static int en751221_mcm_phy_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_soft_reset(phydev);
+	if (ret)
+		return ret;
+
+	ret = phy_write_paged(phydev, MTK_PHY_PAGE_EXTENDED_1,
+			      MTK_PHY_AUX_CTRL_AND_STATUS, 0x3a04);
+	if (ret < 0)
+		return ret;
+
+	/* Clause 45 global/local data from mt7530GePhyCfgLoad(E3.0). */
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0417, 0x7775);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x00a6, 0x0350);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x0012, 0xd210);
+	if (ret < 0)
+		return ret;
+
+	return mt7530_phy_config_init(phydev);
+}
+
+static bool en751221_is_mcm_phy(struct phy_device *phydev)
+{
+	struct device *parent = phydev->mdio.bus->parent;
+
+	return parent && parent->of_node &&
+	       of_device_is_compatible(parent->of_node, "econet,en751221");
+}
+
+/*
+ * MTK_GPHY_ID_MT7530 ID is also used for an EcoNet SoC FE phy, but that PHY
+ * does not advertise ESTATUS_1000_TFULL.
+ */
+static bool mt7530_is_gphy(struct phy_device *phydev)
+{
+	return phydev->phy_id == MTK_GPHY_ID_MT7530 &&
+	       (phy_read(phydev, MII_ESTATUS) & ESTATUS_1000_TFULL) != 0;
+}
+
+static int mt7530_phy_match(struct phy_device *phydev,
+			    const struct phy_driver *phydrv)
+{
+	return mt7530_is_gphy(phydev) && !en751221_is_mcm_phy(phydev);
+}
+
+static int en751221_phy_match(struct phy_device *phydev,
+			      const struct phy_driver *phydrv)
+{
+	return mt7530_is_gphy(phydev) && en751221_is_mcm_phy(phydev);
+}
+
 static int mt7531_phy_config_init(struct phy_device *phydev)
 {
 	mtk_gephy_config_init(phydev);
@@ -135,6 +199,23 @@ static struct phy_driver mtk_gephy_driver[] = {
 		 */
 		.config_intr	= genphy_no_config_intr,
 		.handle_interrupt = genphy_handle_interrupt_no_ack,
+		.match_phy_device = mt7530_phy_match,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.read_page	= mtk_phy_read_page,
+		.write_page	= mtk_phy_write_page,
+	},
+	{
+		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
+		.name		= "EcoNet EN751221 MCM PHY",
+		.probe		= mt7530_phy_probe,
+		.config_init	= en751221_mcm_phy_config_init,
+		/* Interrupts are handled by the switch, not the PHY
+		 * itself.
+		 */
+		.config_intr	= genphy_no_config_intr,
+		.handle_interrupt = genphy_handle_interrupt_no_ack,
+		.match_phy_device = en751221_phy_match,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.read_page	= mtk_phy_read_page,
-- 
2.39.5

Re: [PATCH v5 net-next] net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[Daniel Golle]

On Tue, Sep 15, 2026 at 11:34:27AM +0000, Caleb James DeLisle wrote:
> [...]
> +/*
> + * MTK_GPHY_ID_MT7530 ID is also used for an EcoNet SoC FE phy, but that PHY
> + * does not advertise ESTATUS_1000_TFULL.
> + */
> +static bool mt7530_is_gphy(struct phy_device *phydev)
> +{
> +	return phydev->phy_id == MTK_GPHY_ID_MT7530 &&
> +	       (phy_read(phydev, MII_ESTATUS) & ESTATUS_1000_TFULL) != 0;
> +}
> +
> +static int mt7530_phy_match(struct phy_device *phydev,
> +			    const struct phy_driver *phydrv)
> +{
> +	return mt7530_is_gphy(phydev) && !en751221_is_mcm_phy(phydev);
> +}
> +
> +static int en751221_phy_match(struct phy_device *phydev,
> +			      const struct phy_driver *phydrv)
> +{
> +	return mt7530_is_gphy(phydev) && en751221_is_mcm_phy(phydev);
> +}
> +
>  static int mt7531_phy_config_init(struct phy_device *phydev)
>  {
>  	mtk_gephy_config_init(phydev);
> @@ -135,6 +199,23 @@ static struct phy_driver mtk_gephy_driver[] = {
>  		 */
>  		.config_intr	= genphy_no_config_intr,
>  		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.match_phy_device = mt7530_phy_match,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.read_page	= mtk_phy_read_page,
> +		.write_page	= mtk_phy_write_page,
> +	},
> +	{
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),

I'd suggest to actually use phy_id and phy_id_mask assigned by the
PHY_ID_MATCH_EXACT macro by calling genphy_match_phy_device() in your
match functions above instead of open-coding the ID match.
Or drop PHY_ID_MATCH_EXACT from *both* drivers.

Re: [PATCH v5 net-next] net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[Caleb James DeLisle]

On 15/09/2026 13:47, Daniel Golle wrote:
> On Tue, Sep 15, 2026 at 11:34:27AM +0000, Caleb James DeLisle wrote:
>> [...]
>> +/*
>> + * MTK_GPHY_ID_MT7530 ID is also used for an EcoNet SoC FE phy, but that PHY
>> + * does not advertise ESTATUS_1000_TFULL.
>> + */
>> +static bool mt7530_is_gphy(struct phy_device *phydev)
>> +{
>> +	return phydev->phy_id == MTK_GPHY_ID_MT7530 &&
>> +	       (phy_read(phydev, MII_ESTATUS) & ESTATUS_1000_TFULL) != 0;
>> +}
>> +
>> +static int mt7530_phy_match(struct phy_device *phydev,
>> +			    const struct phy_driver *phydrv)
>> +{
>> +	return mt7530_is_gphy(phydev) && !en751221_is_mcm_phy(phydev);
>> +}
>> +
>> +static int en751221_phy_match(struct phy_device *phydev,
>> +			      const struct phy_driver *phydrv)
>> +{
>> +	return mt7530_is_gphy(phydev) && en751221_is_mcm_phy(phydev);
>> +}
>> +
>>   static int mt7531_phy_config_init(struct phy_device *phydev)
>>   {
>>   	mtk_gephy_config_init(phydev);
>> @@ -135,6 +199,23 @@ static struct phy_driver mtk_gephy_driver[] = {
>>   		 */
>>   		.config_intr	= genphy_no_config_intr,
>>   		.handle_interrupt = genphy_handle_interrupt_no_ack,
>> +		.match_phy_device = mt7530_phy_match,
>> +		.suspend	= genphy_suspend,
>> +		.resume		= genphy_resume,
>> +		.read_page	= mtk_phy_read_page,
>> +		.write_page	= mtk_phy_write_page,
>> +	},
>> +	{
>> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
> I'd suggest to actually use phy_id and phy_id_mask assigned by the
> PHY_ID_MATCH_EXACT macro by calling genphy_match_phy_device() in your
> match functions above instead of open-coding the ID match.
> Or drop PHY_ID_MATCH_EXACT from *both* drivers.


I suppose the latter is easier because then I don't have to re-think 
mt7530_is_gphy() which would be lying if it wasn't actually checking ID 
is MTK_GPHY_ID_MT7530.


But I guess I should come back tomorrow since I'm already in trouble 
with the 24 hour bot.


Thanks,

Caleb

Re: [PATCH v5 net-next] net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 1

- [High] The new MCM detection in en751221_is_mcm_phy()
  (drivers/net/phy/mediatek/mtk-ge.c:138-144) tests the MDIO bus *parent…
- [Medium] mt7530_is_gphy() (drivers/net/phy/mediatek/mtk-ge.c:150-154)
  masks the raw return value of phy_read(phydev, MII_ESTATUS) with…
- [Low] The commit subject reads "net: phy: mediatek: support MT7530 PHYs
  on EN71221 MCM" while the commit body, the new code…

--

net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

This adds a second phy_driver entry for MTK_GPHY_ID_MT7530 with an extra
register sequence for the EcoNet EN751221 multi-chip module, selects
between the two entries with new match_phy_device callbacks, and skips
MT7530-ID PHYs that report no gigabit support.

> net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[Severity: Low]
This isn't a bug, but the subject says EN71221 while the body, the code
(en751221_is_mcm_phy(), en751221_mcm_phy_config_init(), the driver name
"EcoNet EN751221 MCM PHY"), the compatible "econet,en751221" and every
other in-tree reference say EN751221.  Grepping the tree finds no
EN71221 identifier at all.

> diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
> index 96d8ac5154e5e..eddb66218d7c0 100644
> --- a/drivers/net/phy/mediatek/mtk-ge.c
> +++ b/drivers/net/phy/mediatek/mtk-ge.c

[ ... ]

> @@ -101,6 +102,69 @@ static int mt7530_phy_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/*
> + * The EcoNet EN751221 "G" multi-chip module MT7530 requires additional PHY
> + * configuration.
> + */
> +static int en751221_mcm_phy_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_soft_reset(phydev);
> +	if (ret)
> +		return ret;

[ ... ]

> +static bool en751221_is_mcm_phy(struct phy_device *phydev)
> +{
> +	struct device *parent = phydev->mdio.bus->parent;
> +
> +	return parent && parent->of_node &&
> +	       of_device_is_compatible(parent->of_node, "econet,en751221");
> +}

[Severity: High]
Can this predicate ever return true?

"econet,en751221" is documented only as a machine compatible on the DT
root node:

Documentation/devicetree/bindings/mips/econet.yaml
  properties:
    $nodename:
      const: '/'
    compatible:
      oneOf:
        - description: Boards with EcoNet EN751221 family SoC
          items:
            - enum:
                - smartfiber,xp8421-b
            - const: econet,en751221

and the only in-tree users put it on '/':

arch/mips/boot/dts/econet/en751221.dtsi
  / {
        compatible = "econet,en751221";

arch/mips/boot/dts/econet/en751221_smartfiber_xp8421-b.dts
  / {
        model = "SmartFiber XP8421-B";
        compatible = "smartfiber,xp8421-b", "econet,en751221";

The node handed to of_device_is_compatible() here is the MDIO bus parent
device, which for MT7530-internal PHYs is the switch device:

drivers/net/dsa/mt7530.c:mt7530_setup_mdio() {
    ...
	bus->parent = dev;
    ...
}

of_device_is_compatible() only inspects the compatible list of the node it
is given, so a switch node that is a child of '/' will not match a root
node compatible.  Does that make en751221_phy_match() always return zero,
leaving the new "EcoNet EN751221 MCM PHY" entry and
en751221_mcm_phy_config_init() unreachable, with mt7530_phy_match()
binding the plain MT7530 entry on the MCM hardware instead?

Would matching the switch's own compatible work better here?  The EcoNet
MCM switch already has one in-tree:

drivers/net/dsa/mt7530-mmio.c
  { .compatible = "econet,en7528-switch", .data = &mt753x_table[ID_EN7528], },

Alternatively, of_machine_is_compatible("econet,en751221") would test the
root node, or a new documented compatible for the MCM switch/MDIO node
could be added.

> +
> +/*
> + * MTK_GPHY_ID_MT7530 ID is also used for an EcoNet SoC FE phy, but that PHY
> + * does not advertise ESTATUS_1000_TFULL.
> + */
> +static bool mt7530_is_gphy(struct phy_device *phydev)
> +{
> +	return phydev->phy_id == MTK_GPHY_ID_MT7530 &&
> +	       (phy_read(phydev, MII_ESTATUS) & ESTATUS_1000_TFULL) != 0;
> +}

[Severity: Medium]
Should the phy_read() return value be checked for an error before it is
masked?

ESTATUS_1000_TFULL is 0x2000, i.e. bit 13, and every small negative errno
has bit 13 set in two's complement (-EIO is 0xfffffffb, -ETIMEDOUT is
0xffffff92).  So a failed MDIO read makes mt7530_is_gphy() report
"gigabit capable", which is the opposite of the intended exclusion of the
FE PHY that shares MTK_GPHY_ID_MT7530.

phy_bus_match() returns the callback result verbatim:

drivers/net/phy/phy_device.c:phy_bus_match() {
    ...
	if (phydrv->match_phy_device)
		return phydrv->match_phy_device(phydev, phydrv);
    ...
}

so on a transient bus error the FE PHY would be claimed and programmed
with the GbE sequence in en751221_mcm_phy_config_init() (paged write of
0x3a04, MMD VEND2 0x0417, VEND1 0xa6 and 0x12) or mt7530_phy_config_init().

Should MII_ESTATUS also be gated on BMSR_ESTATEN, the way phylib does it
elsewhere?

drivers/net/phy/phy_device.c:genphy_read_abilities() {
    ...
	if (val & BMSR_ESTATEN) {
		val = phy_read(phydev, MII_ESTATUS);
		if (val < 0)
			return val;
    ...
}

Without that check, the value read from register 15 on a PHY that does not
implement it is not defined by Clause 22.

> +
> +static int mt7530_phy_match(struct phy_device *phydev,
> +			    const struct phy_driver *phydrv)
> +{
> +	return mt7530_is_gphy(phydev) && !en751221_is_mcm_phy(phydev);
> +}
> +
> +static int en751221_phy_match(struct phy_device *phydev,
> +			      const struct phy_driver *phydrv)
> +{
> +	return mt7530_is_gphy(phydev) && en751221_is_mcm_phy(phydev);
> +}
> +
>  static int mt7531_phy_config_init(struct phy_device *phydev)
>  {
>  	mtk_gephy_config_init(phydev);
> @@ -135,6 +199,23 @@ static struct phy_driver mtk_gephy_driver[] = {
>  		 */
>  		.config_intr	= genphy_no_config_intr,
>  		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.match_phy_device = mt7530_phy_match,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.read_page	= mtk_phy_read_page,
> +		.write_page	= mtk_phy_write_page,
> +	},
> +	{
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
> +		.name		= "EcoNet EN751221 MCM PHY",
> +		.probe		= mt7530_phy_probe,
> +		.config_init	= en751221_mcm_phy_config_init,
> +		/* Interrupts are handled by the switch, not the PHY
> +		 * itself.
> +		 */
> +		.config_intr	= genphy_no_config_intr,
> +		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.match_phy_device = en751221_phy_match,
>  		.suspend	= genphy_suspend,
>  		.resume		= genphy_resume,
>  		.read_page	= mtk_phy_read_page,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915113427.113296-1-cjd%40cjdns.fr

Re: [PATCH v5 net-next] net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[Daniel Golle]

On Tue, Sep 15, 2026 at 02:01:29PM +0200, Caleb James DeLisle wrote:
> On 15/09/2026 13:47, Daniel Golle wrote:
> > On Tue, Sep 15, 2026 at 11:34:27AM +0000, Caleb James DeLisle wrote:
> > > @@ -135,6 +199,23 @@ static struct phy_driver mtk_gephy_driver[] = {
> > >   		 */
> > >   		.config_intr	= genphy_no_config_intr,
> > >   		.handle_interrupt = genphy_handle_interrupt_no_ack,
> > > +		.match_phy_device = mt7530_phy_match,
> > > +		.suspend	= genphy_suspend,
> > > +		.resume		= genphy_resume,
> > > +		.read_page	= mtk_phy_read_page,
> > > +		.write_page	= mtk_phy_write_page,
> > > +	},
> > > +	{
> > > +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
> > I'd suggest to actually use phy_id and phy_id_mask assigned by the
> > PHY_ID_MATCH_EXACT macro by calling genphy_match_phy_device() in your
> > match functions above instead of open-coding the ID match.
> > Or drop PHY_ID_MATCH_EXACT from *both* drivers.
> 
> I suppose the latter is easier because then I don't have to re-think
> mt7530_is_gphy() which would be lying if it wasn't actually checking ID is
> MTK_GPHY_ID_MT7530.
> 

I would have preferred to call genphy_match_phy_device() in your match
functions instead of open-coding phy_id_compare() which is best
reached via genphy_match_phy_device() in this situation -- that would
express the code intent in the most obvious way imho.

Re: [PATCH v5 net-next] net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[Caleb James DeLisle]

On 17/09/2026 16:48, Daniel Golle wrote:
> On Tue, Sep 15, 2026 at 02:01:29PM +0200, Caleb James DeLisle wrote:
>> On 15/09/2026 13:47, Daniel Golle wrote:
>>> On Tue, Sep 15, 2026 at 11:34:27AM +0000, Caleb James DeLisle wrote:
>>>> @@ -135,6 +199,23 @@ static struct phy_driver mtk_gephy_driver[] = {
>>>>    		 */
>>>>    		.config_intr	= genphy_no_config_intr,
>>>>    		.handle_interrupt = genphy_handle_interrupt_no_ack,
>>>> +		.match_phy_device = mt7530_phy_match,
>>>> +		.suspend	= genphy_suspend,
>>>> +		.resume		= genphy_resume,
>>>> +		.read_page	= mtk_phy_read_page,
>>>> +		.write_page	= mtk_phy_write_page,
>>>> +	},
>>>> +	{
>>>> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
>>> I'd suggest to actually use phy_id and phy_id_mask assigned by the
>>> PHY_ID_MATCH_EXACT macro by calling genphy_match_phy_device() in your
>>> match functions above instead of open-coding the ID match.
>>> Or drop PHY_ID_MATCH_EXACT from *both* drivers.
>> I suppose the latter is easier because then I don't have to re-think
>> mt7530_is_gphy() which would be lying if it wasn't actually checking ID is
>> MTK_GPHY_ID_MT7530.
>>
> I would have preferred to call genphy_match_phy_device() in your match
> functions instead of open-coding phy_id_compare() which is best
> reached via genphy_match_phy_device() in this situation -- that would
> express the code intent in the most obvious way imho.


I did it this way because the name mt7530_is_gphy() implies "Is this an 
MT7530 gigabit PHY?" which if it doesn't match on MTK_GPHY_ID_MT7530 
then that's not what it does so there's a little bit more thought involved.


If I'd have known this was really your preference I'd have done that, 
but I already just sent v6 so I guess I can send v7 tomorrow.


Thanks,

Caleb