* [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value @ 2025-08-22 20:36 Heiner Kallweit 2025-08-24 16:05 ` Hauke Mehrtens 2025-08-27 0:40 ` patchwork-bot+netdevbpf 0 siblings, 2 replies; 5+ messages in thread From: Heiner Kallweit @ 2025-08-22 20:36 UTC (permalink / raw) To: Greg Ungerer, Geert Uytterhoeven, Hauke Mehrtens, Rafał Miłecki, Thomas Bogendoerfer, Andrew Lunn, Andrew Lunn, Russell King - ARM Linux, Paolo Abeni, Eric Dumazet, Jakub Kicinski, David Miller Cc: netdev@vger.kernel.org, linux-m68k, linux-mips 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) -- 2.50.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value 2025-08-22 20:36 [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value Heiner Kallweit @ 2025-08-24 16:05 ` Hauke Mehrtens 2025-08-24 16:36 ` Heiner Kallweit 2025-08-27 0:40 ` patchwork-bot+netdevbpf 1 sibling, 1 reply; 5+ messages in thread From: Hauke Mehrtens @ 2025-08-24 16:05 UTC (permalink / raw) To: Heiner Kallweit, Greg Ungerer, Geert Uytterhoeven, Rafał Miłecki, Thomas Bogendoerfer, Andrew Lunn, Andrew Lunn, Russell King - ARM Linux, Paolo Abeni, Eric Dumazet, Jakub Kicinski, David Miller Cc: netdev@vger.kernel.org, linux-m68k, linux-mips, Takumi Sueda On 8/22/25 22:36, Heiner Kallweit wrote: > 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 Does this need a bigger refactoring anyway? Hauke ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value 2025-08-24 16:05 ` Hauke Mehrtens @ 2025-08-24 16:36 ` Heiner Kallweit 2025-10-04 11:20 ` Heiner Kallweit 0 siblings, 1 reply; 5+ messages in thread From: Heiner Kallweit @ 2025-08-24 16:36 UTC (permalink / raw) To: Hauke Mehrtens, Greg Ungerer, Geert Uytterhoeven, Rafał Miłecki, Thomas Bogendoerfer, Andrew Lunn, Andrew Lunn, Russell King - ARM Linux, Paolo Abeni, Eric Dumazet, Jakub Kicinski, David Miller, Takumi Sueda Cc: netdev@vger.kernel.org, linux-m68k, linux-mips On 8/24/2025 6:05 PM, Hauke Mehrtens wrote: > On 8/22/25 22:36, Heiner Kallweit wrote: >> 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value 2025-08-24 16:36 ` Heiner Kallweit @ 2025-10-04 11:20 ` Heiner Kallweit 0 siblings, 0 replies; 5+ messages in thread From: Heiner Kallweit @ 2025-10-04 11:20 UTC (permalink / raw) To: Hauke Mehrtens, Greg Ungerer, Geert Uytterhoeven, Rafał Miłecki, Thomas Bogendoerfer, Andrew Lunn, Andrew Lunn, Russell King - ARM Linux, Paolo Abeni, Eric Dumazet, Jakub Kicinski, David Miller, Takumi Sueda Cc: netdev@vger.kernel.org, linux-m68k, linux-mips On 8/24/2025 6:36 PM, Heiner Kallweit wrote: > On 8/24/2025 6:05 PM, Hauke Mehrtens wrote: >> On 8/22/25 22:36, Heiner Kallweit wrote: >>> 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. > Did you have the chance to 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); > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value 2025-08-22 20:36 [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value Heiner Kallweit 2025-08-24 16:05 ` Hauke Mehrtens @ 2025-08-27 0:40 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 5+ messages in thread From: patchwork-bot+netdevbpf @ 2025-08-27 0:40 UTC (permalink / raw) To: Heiner Kallweit Cc: gerg, geert, hauke, zajec5, tsbogend, andrew, andrew+netdev, linux, pabeni, edumazet, kuba, davem, netdev, linux-m68k, linux-mips Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 22 Aug 2025 22:36:11 +0200 you wrote: > 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. > > [...] Here is the summary with links: - [net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value https://git.kernel.org/netdev/net-next/c/39e94fdce45f You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-10-04 11:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-22 20:36 [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value Heiner Kallweit 2025-08-24 16:05 ` Hauke Mehrtens 2025-08-24 16:36 ` Heiner Kallweit 2025-10-04 11:20 ` Heiner Kallweit 2025-08-27 0:40 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).