On 8/24/2025 6:05 PM, Hauke Mehrtens wrote:

On 8/22/25 22:36, Heiner Kallweit wrote:

quoted

We have only two users of fixed_phy_add(), both use address 0 and
ignore the return value. So simplify fixed_phy_add() accordingly.

Whilst at it, constify the fixed_phy_status configs.

Note:
fixed_phy_add() is a legacy function which shouldn't be used in new
code, as it's use may be problematic:
- No check whether a fixed phy exists already at the given address
- If fixed_phy_register() is called afterwards by any other driver,
   then it will also use phy_addr 0, because fixed_phy_add() ignores
   the ida which manages address assignment
Drivers using a fixed phy created by fixed_phy_add() in platform code,
should dynamically create a fixed phy with fixed_phy_register()
instead.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
  arch/m68k/coldfire/m5272.c  | 4 ++--
  arch/mips/bcm47xx/setup.c   | 4 ++--
  drivers/net/phy/fixed_phy.c | 4 ++--
  include/linux/phy_fixed.h   | 8 ++------
  4 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/m68k/coldfire/m5272.c b/arch/m68k/coldfire/m5272.c
index 5b70dfdab..918e2a323 100644
--- a/arch/m68k/coldfire/m5272.c
+++ b/arch/m68k/coldfire/m5272.c

@@ -108,7 +108,7 @@ void __init config_BSP(char *commandp, int size)

   * an ethernet switch. In this case we need to use the fixed phy type,
   * and we need to declare it early in boot.
   */
-static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
+static const struct fixed_phy_status nettel_fixed_phy_status __initconst = {
      .link    = 1,
      .speed    = 100,
      .duplex    = 0,

@@ -119,7 +119,7 @@ static struct fixed_phy_status nettel_fixed_phy_status __initdata = {

  static int __init init_BSP(void)
  {
      m5272_uarts_init();
-    fixed_phy_add(0, &nettel_fixed_phy_status);
+    fixed_phy_add(&nettel_fixed_phy_status);
      clkdev_add_table(m5272_clk_lookup, ARRAY_SIZE(m5272_clk_lookup));
      return 0;
  }

diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
index de426a474..a93a4266d 100644
--- a/arch/mips/bcm47xx/setup.c
+++ b/arch/mips/bcm47xx/setup.c

@@ -256,7 +256,7 @@ static int __init bcm47xx_cpu_fixes(void)

  }
  arch_initcall(bcm47xx_cpu_fixes);
  -static struct fixed_phy_status bcm47xx_fixed_phy_status __initdata = {
+static const struct fixed_phy_status bcm47xx_fixed_phy_status __initconst = {
      .link    = 1,
      .speed    = SPEED_100,
      .duplex    = DUPLEX_FULL,

@@ -282,7 +282,7 @@ static int __init bcm47xx_register_bus_complete(void)

      bcm47xx_leds_register();
      bcm47xx_workarounds();
  -    fixed_phy_add(0, &bcm47xx_fixed_phy_status);
+    fixed_phy_add(&bcm47xx_fixed_phy_status);
      return 0;
  }
  device_initcall(bcm47xx_register_bus_complete);

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 7902b35c5..b39532abf 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c

@@ -153,9 +153,9 @@ static int fixed_phy_add_gpiod(unsigned int irq, int phy_addr,

      return 0;
  }
  -int fixed_phy_add(int phy_addr, const struct fixed_phy_status *status)
+void fixed_phy_add(const struct fixed_phy_status *status)
  {
-    return fixed_phy_add_gpiod(PHY_POLL, phy_addr, status, NULL);
+    fixed_phy_add_gpiod(PHY_POLL, 0, status, NULL);
  }
  EXPORT_SYMBOL_GPL(fixed_phy_add);
  diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index 5399b9e41..6227a1bde 100644

--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h

@@ -17,7 +17,7 @@ struct net_device;

    #if IS_ENABLED(CONFIG_FIXED_PHY)
  extern int fixed_phy_change_carrier(struct net_device *dev, bool new_carrier);
-int fixed_phy_add(int phy_id, const struct fixed_phy_status *status);
+void fixed_phy_add(const struct fixed_phy_status *status);
  struct phy_device *fixed_phy_register(const struct fixed_phy_status *status,
                        struct device_node *np);
  @@ -26,11 +26,7 @@ extern int fixed_phy_set_link_update(struct phy_device *phydev,
              int (*link_update)(struct net_device *,
                         struct fixed_phy_status *));
  #else
-static inline int fixed_phy_add(int phy_id,
-                const struct fixed_phy_status *status)
-{
-    return -ENODEV;
-}
+static inline void fixed_phy_add(const struct fixed_phy_status *status) {}
  static inline struct phy_device *
  fixed_phy_register(const struct fixed_phy_status *status,
             struct device_node *np)

Hi,

I do not use this hardware any more, but Takumi reported that fixed_phy_add() is not working for the PHY registration on brcm47xx any more and we have to use fixed_phy_register(), see:
https://github.com/openwrt/openwrt/pull/19610

I suspected already that fixed_phy_add() usage doesn't work any longer, but don't
have hw to test. I think bfa54812f0bc ("net: phy: fixed_phy: set phy_mask before
calling mdiobus_register()") is to blame.
Reverting this commit or changing the line to fmb->mii_bus->phy_mask = ~1;
should fix the problem. Would be great if Takumi could test this.

The cleaner alternative would be:
- remove call to fixed_phy_add() from platform code
- let b44 call fixed_phy_register() if a fixed phy is needed
  Below is a WIP version of this change.

Does this need a bigger refactoring anyway?

Hauke

Heiner


---
 drivers/net/ethernet/broadcom/Kconfig |  1 +
 drivers/net/ethernet/broadcom/b44.c   | 33 ++++++++++++++-------------
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index 0fc10e6c6..99292392d 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig

@@ -25,6 +25,7 @@ config B44
 	select SSB
 	select MII
 	select PHYLIB
+	select FIXED_PHY
 	help
 	  If you have a network (Ethernet) controller of this type, say Y
 	  or M here.

diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 0353359c3..abbb5953c 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c

@@ -31,6 +31,7 @@
 #include <linux/ssb/ssb.h>
 #include <linux/slab.h>
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 
 #include <linux/uaccess.h>
 #include <asm/io.h>

@@ -2233,7 +2234,6 @@ static int b44_register_phy_one(struct b44 *bp)
 	struct mii_bus *mii_bus;
 	struct ssb_device *sdev = bp->sdev;
 	struct phy_device *phydev;
-	char bus_id[MII_BUS_ID_SIZE + 3];
 	struct ssb_sprom *sprom = &sdev->bus->sprom;
 	int err;
 

@@ -2260,27 +2260,26 @@ static int b44_register_phy_one(struct b44 *bp)
 		goto err_out_mdiobus;
 	}
 
-	if (!mdiobus_is_registered_device(bp->mii_bus, bp->phy_addr) &&
-	    (sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM))) {
-
+	phydev = mdiobus_get_phy(bp->mii_bus, bp->phy_addr);
+	if (!phydev &&
+	    sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM)) {
 		dev_info(sdev->dev,
 			 "could not find PHY at %i, use fixed one\n",
 			 bp->phy_addr);
 
-		bp->phy_addr = 0;
-		snprintf(bus_id, sizeof(bus_id), PHY_ID_FMT, "fixed-0",
-			 bp->phy_addr);
-	} else {
-		snprintf(bus_id, sizeof(bus_id), PHY_ID_FMT, mii_bus->id,
-			 bp->phy_addr);
+		phydev = fixed_phy_register(NULL, NULL);
+		if (!IS_ERR(phydev))
+			bp->phy_addr = phydev->mdio.addr;
 	}
 
-	phydev = phy_connect(bp->dev, bus_id, &b44_adjust_link,
-			     PHY_INTERFACE_MODE_MII);
-	if (IS_ERR(phydev)) {
+	if (IS_ERR_OR_NULL(phydev))
+		err = -ENODEV;
+	else
+		err = phy_connect_direct(bp->dev, phydev, &b44_adjust_link,
+					 PHY_INTERFACE_MODE_MII);
+	if (err) {
 		dev_err(sdev->dev, "could not attach PHY at %i\n",
 			bp->phy_addr);
-		err = PTR_ERR(phydev);
 		goto err_out_mdiobus_unregister;
 	}
 

@@ -2293,7 +2292,6 @@ static int b44_register_phy_one(struct b44 *bp)
 	linkmode_copy(phydev->advertising, phydev->supported);
 
 	bp->old_link = 0;
-	bp->phy_addr = phydev->mdio.addr;
 
 	phy_attached_info(phydev);
 

@@ -2313,8 +2311,11 @@ static void b44_unregister_phy_one(struct b44 *bp)
 {
 	struct net_device *dev = bp->dev;
 	struct mii_bus *mii_bus = bp->mii_bus;
+	struct phy_device *phydev = dev->phydev);
 
-	phy_disconnect(dev->phydev);
+	phy_disconnect(phydev);
+	if (phy_is_pseudo_fixed_link(phydev))
+		fixed_phy_unregister(phydev);
 	mdiobus_unregister(mii_bus);
 	mdiobus_free(mii_bus);
 }

-- 
2.50.1