| Message ID | 20250317063452.3072784-1-JJLIU0@nuvoton.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure | expand |
On Mon, Mar 17, 2025 at 02:34:52PM +0800, Jim Liu wrote: > Use "BRCM_PHY_MODEL" can be applied to the entire 5221 family of PHYs. > > Fixes: 3abbd0699b67 ("net: phy: broadcom: add support for BCM5221 phy") > Signed-off-by: Jim Liu <jim.t90615@gmail.com> Looking at BRCM_PHY_MODEL() and BRCM_PHY_REV(), I think there's more issues with this driver. E.g.: #define BRCM_PHY_MODEL(phydev) \ ((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask) #define BRCM_PHY_REV(phydev) \ ((phydev)->drv->phy_id & ~((phydev)->drv->phy_id_mask)) #define PHY_ID_BCM50610 0x0143bd60 #define PHY_ID_BCM50610M 0x0143bd70 if ((BRCM_PHY_MODEL(phydev) == PHY_ID_BCM50610 || BRCM_PHY_MODEL(phydev) == PHY_ID_BCM50610M) && BRCM_PHY_REV(phydev) >= 0x3) { and from the PHY driver table: .phy_id = PHY_ID_BCM50610, .phy_id_mask = 0xfffffff0, .phy_id = PHY_ID_BCM50610M, .phy_id_mask = 0xfffffff0, BRCM_PHY_REV() looks at _this_ .phy_id in the table, and tries to match it against the revision field bits 0-3 being >= 3 - but as we can see, this field is set to the defined value which has bits 0-3 always as zero. So, this if() statement is always false. So, BRCM_PHY_REV() should be: #define BRCM_PHY_REV(phydev) \ ((phydev)->phy_id & ~(phydev)->drv->phy_id_mask) Next, I question why BRCM_PHY_MODEL() exists in the first place. phydev->drv->phy_id is initialised to the defined value(s), and then we end up doing: (phydev->drv->phy_id & phydev->drv->phy_id_mask) == one-of-those-defined-values which is pointless, because we know that what is in phydev->drv->phy_id /is/ one-of-those-defined-values. Therefore, I would suggest: #define BRCM_PHY_MODEL(phydev) ((phydev)->drv->phy_id) is entirely sufficient, and with such a simple definition, I question the value of BRCM_PHY_MODEL() existing.
On 3/17/2025 3:24 AM, Russell King (Oracle) wrote: > On Mon, Mar 17, 2025 at 02:34:52PM +0800, Jim Liu wrote: >> Use "BRCM_PHY_MODEL" can be applied to the entire 5221 family of PHYs. >> >> Fixes: 3abbd0699b67 ("net: phy: broadcom: add support for BCM5221 phy") >> Signed-off-by: Jim Liu <jim.t90615@gmail.com> > > Looking at BRCM_PHY_MODEL() and BRCM_PHY_REV(), I think there's more > issues with this driver. E.g.: > > #define BRCM_PHY_MODEL(phydev) \ > ((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask) > > #define BRCM_PHY_REV(phydev) \ > ((phydev)->drv->phy_id & ~((phydev)->drv->phy_id_mask)) > > #define PHY_ID_BCM50610 0x0143bd60 > #define PHY_ID_BCM50610M 0x0143bd70 > > if ((BRCM_PHY_MODEL(phydev) == PHY_ID_BCM50610 || > BRCM_PHY_MODEL(phydev) == PHY_ID_BCM50610M) && > BRCM_PHY_REV(phydev) >= 0x3) { > > and from the PHY driver table: > > .phy_id = PHY_ID_BCM50610, > .phy_id_mask = 0xfffffff0, > > .phy_id = PHY_ID_BCM50610M, > .phy_id_mask = 0xfffffff0, > > BRCM_PHY_REV() looks at _this_ .phy_id in the table, and tries to match > it against the revision field bits 0-3 being >= 3 - but as we can see, > this field is set to the defined value which has bits 0-3 always as > zero. So, this if() statement is always false. > > So, BRCM_PHY_REV() should be: > > #define BRCM_PHY_REV(phydev) \ > ((phydev)->phy_id & ~(phydev)->drv->phy_id_mask) > > > Next, I question why BRCM_PHY_MODEL() exists in the first place. > phydev->drv->phy_id is initialised to the defined value(s), and then > we end up doing: > > (phydev->drv->phy_id & phydev->drv->phy_id_mask) == > one-of-those-defined-values > > which is pointless, because we know that what is in phydev->drv->phy_id > /is/ one-of-those-defined-values. > > Therefore, I would suggest: > > #define BRCM_PHY_MODEL(phydev) ((phydev)->drv->phy_id) > > is entirely sufficient, and with such a simple definition, I question > the value of BRCM_PHY_MODEL() existing. If I were to make a guess, BRCM_PHY_MODEL() might have existed to ease the porting of a non-Linux PHY driver to a Linux PHY driver environment at a time where the subsystem was not as mature as it is now. In the interest of a targeted bug fix, I would be keen on taking this patch in its current form and follow up in net next with a removal of BRCM_PHY_MODEL() later on.
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 22edb7e4c1a1..3529289e9d13 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -859,7 +859,7 @@ static int brcm_fet_config_init(struct phy_device *phydev) return reg; /* Unmask events we are interested in and mask interrupts globally. */ - if (phydev->phy_id == PHY_ID_BCM5221) + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM5221) reg = MII_BRCM_FET_IR_ENABLE | MII_BRCM_FET_IR_MASK; else @@ -888,7 +888,7 @@ static int brcm_fet_config_init(struct phy_device *phydev) return err; } - if (phydev->phy_id != PHY_ID_BCM5221) { + if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM5221) { /* Set the LED mode */ reg = __phy_read(phydev, MII_BRCM_FET_SHDW_AUXMODE4); if (reg < 0) { @@ -1009,7 +1009,7 @@ static int brcm_fet_suspend(struct phy_device *phydev) return err; } - if (phydev->phy_id == PHY_ID_BCM5221) + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM5221) /* Force Low Power Mode with clock enabled */ reg = BCM5221_SHDW_AM4_EN_CLK_LPM | BCM5221_SHDW_AM4_FORCE_LPM; else
Use "BRCM_PHY_MODEL" can be applied to the entire 5221 family of PHYs. Fixes: 3abbd0699b67 ("net: phy: broadcom: add support for BCM5221 phy") Signed-off-by: Jim Liu <jim.t90615@gmail.com> --- v2: - add target tree - add maintainer in mail thread - modify checkpatch warning --- drivers/net/phy/broadcom.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)