[v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[Jim Liu]

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(-)

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
-- 
2.34.1

Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[Florian Fainelli]

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.
-- 
Florian

Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[Russell King (Oracle)]

On Mon, Mar 17, 2025 at 06:18:17AM -0700, Florian Fainelli wrote:
> 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.

Note that commit 32e5a8d651c0 ("tg3 / broadcom: Add code to disable rxc
refclk") is still wrong (which introduced BRCM_PHY_REV().)

Given its age, I would suggest that the commit I reference above was
not properly tested, and *possibly* fixing the bug might actually
cause a regression on TG3.

Also, the original commit description (which references RXC) which is
supposed to be synchronous to the received data, but the code talks
about CLK125 which is *technically* a different clock.

We know that the current driver logic works, even though it doesn't
do what the original author of the code wanted it to do.

Taking these three points together, I don't think it would be wise
to fix the logical error (and actually test the revision field).
Instead, I think getting rid of the always-false if() and simplifying
the code would be better to avoid causing a possible regression on
TG3.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[Jim Liu]

Hi Russell King  & Florian Fainelli

Very thanks for your comment.
Our EVB uses BCM5221A4K so the id is 0x004061e4
If using the new kernel, the condition in the if statement prevents
the PHY from functioning.
So I followed the driver 's style to modify this condition and use
BRCM_PHY_MODEL().

After checking the net framework I see phydev_id_compare can be used.
It is a good feature for all phy drivers.
but now I just followed the driver style to modify it.

I think Perhaps this obvious error should be addressed first.
And the next step is to  transition Broadcom or other drivers to use
the net framework functions.

Maybe i can add this modify in patch

#define BRCM_PHY_MODEL(phydev) \
-       ((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask)
+       ((phydev)->phy_id & (phydev)->drv->phy_id_mask)

 #define BRCM_PHY_REV(phydev) \
-       ((phydev)->drv->phy_id & ~((phydev)->drv->phy_id_mask))
+       ((phydev)->phy_id & ~((phydev)->drv->phy_id_mask))


Feel free to provide any suggestions to improve v3.


Best regards,
Jim

On Mon, Mar 17, 2025 at 9:29 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Mon, Mar 17, 2025 at 06:18:17AM -0700, Florian Fainelli wrote:
> > 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.
>
> Note that commit 32e5a8d651c0 ("tg3 / broadcom: Add code to disable rxc
> refclk") is still wrong (which introduced BRCM_PHY_REV().)
>
> Given its age, I would suggest that the commit I reference above was
> not properly tested, and *possibly* fixing the bug might actually
> cause a regression on TG3.
>
> Also, the original commit description (which references RXC) which is
> supposed to be synchronous to the received data, but the code talks
> about CLK125 which is *technically* a different clock.
>
> We know that the current driver logic works, even though it doesn't
> do what the original author of the code wanted it to do.
>
> Taking these three points together, I don't think it would be wise
> to fix the logical error (and actually test the revision field).
> Instead, I think getting rid of the always-false if() and simplifying
> the code would be better to avoid causing a possible regression on
> TG3.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[Russell King (Oracle)]

On Tue, Mar 18, 2025 at 11:15:09AM +0800, Jim Liu wrote:
> Maybe i can add this modify in patch
> 
> #define BRCM_PHY_MODEL(phydev) \
> -       ((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask)
> +       ((phydev)->phy_id & (phydev)->drv->phy_id_mask)

I would suggest that this becomes merely:

#define BRCM_PHY_MODEL(phydev)	((phydev))->drv->phy_id)

because the constants that are being used to check against this are the
constants used to initialise that member.

Or even get rid of BRCM_PHY_MODEL() altogether, thus the tests become
(e.g.):

        /* Unmask events we are interested in and mask interrupts globally. */
-       if (phydev->phy_id == PHY_ID_BCM5221)
+       if (phydev->drv->phy_id == PHY_ID_BCM5221)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[Jakub Kicinski]

On Mon, 17 Mar 2025 14:34:52 +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>

Please post a series of patches, you can keep this fix as the first
patch and then the subsequent patches should address the issues pointed
out by Russell.
-- 
pw-bot: cr