[PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[Josef Raschen]

Currently, for a LAN8870 phy, before link up, a call to ethtool to set
master-slave fails with 'operation not supported'. Reason: speed, duplex
and master/slave are not properly initialized.

This change sets proper initial states for speed and duplex and publishes
master-slave states. A default link up for speed 1000, full duplex and
slave mode then works without having to call ethtool.

Signed-off-by: Josef Raschen <josef@raschen.org>
---
 drivers/net/phy/microchip_t1.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
index 62b36a318100..5fcbfa730910 100644
--- a/drivers/net/phy/microchip_t1.c
+++ b/drivers/net/phy/microchip_t1.c
@@ -1319,7 +1319,12 @@ static int lan887x_phy_init(struct phy_device *phydev)
 		return ret;
 
 	/* PHY interface setup */
-	return lan887x_config_phy_interface(phydev);
+	ret = lan887x_config_phy_interface(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* Make configuration visible for ethtool. */
+	return genphy_c45_pma_baset1_read_master_slave(phydev);
 }
 
 static int lan887x_phy_config(struct phy_device *phydev,
@@ -1489,7 +1494,12 @@ static int lan887x_config_aneg(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
-	return lan887x_phy_reconfig(phydev);
+	ret = lan887x_phy_reconfig(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* Make configuration changes visible for ethtool. */
+	return genphy_c45_pma_baset1_read_master_slave(phydev);
 }
 
 static int lan887x_probe(struct phy_device *phydev)
@@ -1503,6 +1513,10 @@ static int lan887x_probe(struct phy_device *phydev)
 	priv->init_done = false;
 	phydev->priv = priv;
 
+	/* Set default link parameters. */
+	phydev->duplex = DUPLEX_FULL;
+	phydev->speed = SPEED_1000;
+
 	return lan887x_phy_setup(phydev);
 }
 
-- 
2.43.0

Re: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[Andrew Lunn]

On Thu, Sep 25, 2025 at 10:52:22PM +0200, Josef Raschen wrote:
> Currently, for a LAN8870 phy, before link up, a call to ethtool to set
> master-slave fails with 'operation not supported'. Reason: speed, duplex
> and master/slave are not properly initialized.
> 
> This change sets proper initial states for speed and duplex and publishes
> master-slave states. A default link up for speed 1000, full duplex and
> slave mode then works without having to call ethtool.

Hi Josef

What you are missing from the commit message is an explanation why the
LAN8870 is special, it needs to do something no other PHY does. Is
there something broken with this PHY? Some register not following
802.3?

    Andrew

---
pw-bot: cr

Re: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[Josef Raschen]

Hello Andrew,

Thanks for your feedback.

On 9/26/25 00:00, Andrew Lunn wrote:
> On Thu, Sep 25, 2025 at 10:52:22PM +0200, Josef Raschen wrote:
>> Currently, for a LAN8870 phy, before link up, a call to ethtool to set
>> master-slave fails with 'operation not supported'. Reason: speed, duplex
>> and master/slave are not properly initialized.
>>
>> This change sets proper initial states for speed and duplex and publishes
>> master-slave states. A default link up for speed 1000, full duplex and
>> slave mode then works without having to call ethtool.
> 
> Hi Josef
> 
> What you are missing from the commit message is an explanation why the
> LAN8870 is special, it needs to do something no other PHY does. Is
> there something broken with this PHY? Some register not following
> 802.3?
> 
>      Andrew
> 
> ---
> pw-bot: cr
> 

Special about the LAN8870 might be that it is a dual speed T1 phy.
As most other T1 pyhs have only one possible configuration the unknown
speed configuration was not a problem so far. But here, when
calling link up without manually setting the speed before, it seems to
pick speed 100 in phy_sanitize_settings(). I assume that this is not the
desired behavior?

The second problem is that ethtool initially does not allow to set
master-slave at all. You first have to call ethtool without the
master-slave argument, then again with it. This is fixed by reading
the master slave configuration from the device which seems to be missing
in the .config_init and .config_aneg functions. I took this solution 
from net/phy/dp83tg720.c.

Regards,
Josef

Re: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[Andrew Lunn]

On Fri, Sep 26, 2025 at 11:24:56PM +0200, Josef Raschen wrote:
> Hello Andrew,
> 
> Thanks for your feedback.
> 
> On 9/26/25 00:00, Andrew Lunn wrote:
> > On Thu, Sep 25, 2025 at 10:52:22PM +0200, Josef Raschen wrote:
> > > Currently, for a LAN8870 phy, before link up, a call to ethtool to set
> > > master-slave fails with 'operation not supported'. Reason: speed, duplex
> > > and master/slave are not properly initialized.
> > > 
> > > This change sets proper initial states for speed and duplex and publishes
> > > master-slave states. A default link up for speed 1000, full duplex and
> > > slave mode then works without having to call ethtool.
> > 
> > Hi Josef
> > 
> > What you are missing from the commit message is an explanation why the
> > LAN8870 is special, it needs to do something no other PHY does. Is
> > there something broken with this PHY? Some register not following
> > 802.3?
> > 
> >      Andrew
> > 
> > ---
> > pw-bot: cr
> > 
> 
> Special about the LAN8870 might be that it is a dual speed T1 phy.
> As most other T1 pyhs have only one possible configuration the unknown
> speed configuration was not a problem so far. But here, when
> calling link up without manually setting the speed before, it seems to
> pick speed 100 in phy_sanitize_settings(). I assume that this is not the
> desired behavior?

What speeds does the PHY say it supports?

phy_sanitize_settings() should pick the highest speed the PHY supports
as the default. So if it is picking 100, it suggests the PHY is not
reporting it supports 1000? Or phy_sanitize_settings() is broken for
1000Base-T1? Please try understand why it is picking 100.

> The second problem is that ethtool initially does not allow to set
> master-slave at all. You first have to call ethtool without the
> master-slave argument, then again with it. This is fixed by reading
> the master slave configuration from the device which seems to be missing
> in the .config_init and .config_aneg functions. I took this solution from
> net/phy/dp83tg720.c.

How does this work with a regular T4 or T2 PHY? Ideally, A T1 should
be no different. And ideally, we want a solution for all T1 PHYs,
assuming it is not something which is special for this PHY.

	Andrew

Re: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[Josef Raschen]

On 9/26/25 23:46, Andrew Lunn wrote:
> On Fri, Sep 26, 2025 at 11:24:56PM +0200, Josef Raschen wrote:
>> Hello Andrew,
>>
>> Thanks for your feedback.
>>
>> On 9/26/25 00:00, Andrew Lunn wrote:
>>> On Thu, Sep 25, 2025 at 10:52:22PM +0200, Josef Raschen wrote:
>>>> Currently, for a LAN8870 phy, before link up, a call to ethtool to set
>>>> master-slave fails with 'operation not supported'. Reason: speed, duplex
>>>> and master/slave are not properly initialized.
>>>>
>>>> This change sets proper initial states for speed and duplex and publishes
>>>> master-slave states. A default link up for speed 1000, full duplex and
>>>> slave mode then works without having to call ethtool.
>>>
>>> Hi Josef
>>>
>>> What you are missing from the commit message is an explanation why the
>>> LAN8870 is special, it needs to do something no other PHY does. Is
>>> there something broken with this PHY? Some register not following
>>> 802.3?
>>>
>>>       Andrew
>>>
>>> ---
>>> pw-bot: cr
>>>
>>
>> Special about the LAN8870 might be that it is a dual speed T1 phy.
>> As most other T1 pyhs have only one possible configuration the unknown
>> speed configuration was not a problem so far. But here, when
>> calling link up without manually setting the speed before, it seems to
>> pick speed 100 in phy_sanitize_settings(). I assume that this is not the
>> desired behavior?
> 
> What speeds does the PHY say it supports?
> 
> phy_sanitize_settings() should pick the highest speed the PHY supports
> as the default. So if it is picking 100, it suggests the PHY is not
> reporting it supports 1000? Or phy_sanitize_settings() is broken for
> 1000Base-T1? Please try understand why it is picking 100.

It should pick 1000 then for this phy. I checked already that the device 
is actually reporting being 100Base-T1 and 1000Base-T1 capable. I will 
have a look, why it doesn't pick SPEED_1000 then. I did not know that 
phy_sanitize_settings() is supposed to pick the highest speed already.

>> The second problem is that ethtool initially does not allow to set
>> master-slave at all. You first have to call ethtool without the
>> master-slave argument, then again with it. This is fixed by reading
>> the master slave configuration from the device which seems to be missing
>> in the .config_init and .config_aneg functions. I took this solution from
>> net/phy/dp83tg720.c.
> 
> How does this work with a regular T4 or T2 PHY? Ideally, A T1 should
> be no different. And ideally, we want a solution for all T1 PHYs,
> assuming it is not something which is special for this PHY.

I agree, a generic solution would be best. I will read a bit into the 
code to see if there is a better solution.

Thanks,
Josef

RE: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[Divya.Koppera]

> -----Original Message-----
> From: Josef Raschen <josef@raschen.org>
> Sent: Tuesday, September 30, 2025 1:00 AM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; David S.
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 9/26/25 23:46, Andrew Lunn wrote:
> > On Fri, Sep 26, 2025 at 11:24:56PM +0200, Josef Raschen wrote:
> >> Hello Andrew,
> >>
> >> Thanks for your feedback.
> >>
> >> On 9/26/25 00:00, Andrew Lunn wrote:
> >>> On Thu, Sep 25, 2025 at 10:52:22PM +0200, Josef Raschen wrote:
> >>>> Currently, for a LAN8870 phy, before link up, a call to ethtool to
> >>>> set master-slave fails with 'operation not supported'. Reason:
> >>>> speed, duplex and master/slave are not properly initialized.
> >>>>
> >>>> This change sets proper initial states for speed and duplex and
> >>>> publishes master-slave states. A default link up for speed 1000,
> >>>> full duplex and slave mode then works without having to call ethtool.
> >>>
> >>> Hi Josef
> >>>
> >>> What you are missing from the commit message is an explanation why
> >>> the
> >>> LAN8870 is special, it needs to do something no other PHY does. Is
> >>> there something broken with this PHY? Some register not following
> >>> 802.3?
> >>>
> >>>       Andrew
> >>>
> >>> ---
> >>> pw-bot: cr
> >>>
> >>
> >> Special about the LAN8870 might be that it is a dual speed T1 phy.
> >> As most other T1 pyhs have only one possible configuration the
> >> unknown speed configuration was not a problem so far. But here, when
> >> calling link up without manually setting the speed before, it seems
> >> to pick speed 100 in phy_sanitize_settings(). I assume that this is
> >> not the desired behavior?
> >
> > What speeds does the PHY say it supports?
> >
> > phy_sanitize_settings() should pick the highest speed the PHY supports
> > as the default. So if it is picking 100, it suggests the PHY is not
> > reporting it supports 1000? Or phy_sanitize_settings() is broken for
> > 1000Base-T1? Please try understand why it is picking 100.
> 
> It should pick 1000 then for this phy. I checked already that the device is
> actually reporting being 100Base-T1 and 1000Base-T1 capable. I will have a
> look, why it doesn't pick SPEED_1000 then. I did not know that
> phy_sanitize_settings() is supposed to pick the highest speed already.
> 
Hi Josef & Andrew,

phy_sanitize_settings() is supposed to pick the least supported speed from the supported list when speed is not initialized.

In latest stable kernel(v6.12.x) we see a bug(Incorrect data types in phylink data structure and function arguments).
Due to which we are observing the highest supported speed is being selected.
Refer https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/net/phy/phy-core.c?h=v6.12.43#n305
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/include/linux/phy.h?h=v6.12.43#n1277In 

Here comparison is done between unsigned and signed speed, due to which the match is been select for the highest speed itself.

Tested code:
308                 if (!match && p->speed <= speed) {
 309             	pr_info("lookup: speed = %d rcvd speed = %d\n", p->speed, speed);
 310                     /* Candidate */
 311                     match = p;
 312                    }

Output print:
[Fri Oct  3 18:24:27 2025] lookup: speed = 1000 rcvd speed = -1   ==> the comparison is false and this should not executed.

Latest net-next doesn't have this inconsistency with data type for the same speed parameter.
So there is no issue and we are seeing the correct lowest supported speed which is 100m.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/phy_caps.c?h=v6.17#n210
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/phy-caps.h?h=v6.17#n39

May be we need to fix the stable kernels to address the data type issue.

Thanks,
Divya
> >> The second problem is that ethtool initially does not allow to set
> >> master-slave at all. You first have to call ethtool without the
> >> master-slave argument, then again with it. This is fixed by reading
> >> the master slave configuration from the device which seems to be
> >> missing in the .config_init and .config_aneg functions. I took this
> >> solution from net/phy/dp83tg720.c.
> >
> > How does this work with a regular T4 or T2 PHY? Ideally, A T1 should
> > be no different. And ideally, we want a solution for all T1 PHYs,
> > assuming it is not something which is special for this PHY.
> 
> I agree, a generic solution would be best. I will read a bit into the code to see if
> there is a better solution.
> 
> Thanks,
> Josef

Re: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[Andrew Lunn]

> phy_sanitize_settings() is supposed to pick the least supported
> speed from the supported list when speed is not initialized.

What makes you think it should pick the slowest speed? The kdoc for
the function is:

/**
 * phy_sanitize_settings - make sure the PHY is set to supported speed and duplex
 * @phydev: the target phy_device struct
 *
 * Description: Make sure the PHY is set to supported speeds and
 *   duplexes.  Drop down by one in this order:  1000/FULL,
 *   1000/HALF, 100/FULL, 100/HALF, 10/FULL, 10/HALF.

So it should pick 1000Full if available. If not it will try 1000Half,
if not 100Full etc.

And the comment is actually a bit out of date. It will actually start
from 800G Full, 400G Full, 200G Full, 100G Full, not that anybody does
Copper at these speeds.

	Andrew

RE: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.

[Divya.Koppera]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, October 7, 2025 12:15 AM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> Cc: josef@raschen.org; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] net: phy: microchip_t1: LAN887X: Fix device init issues.
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > phy_sanitize_settings() is supposed to pick the least supported speed
> > from the supported list when speed is not initialized.
> 
> What makes you think it should pick the slowest speed? The kdoc for the
> function is:
> 
> /**
>  * phy_sanitize_settings - make sure the PHY is set to supported speed and
> duplex
>  * @phydev: the target phy_device struct
>  *
>  * Description: Make sure the PHY is set to supported speeds and
>  *   duplexes.  Drop down by one in this order:  1000/FULL,
>  *   1000/HALF, 100/FULL, 100/HALF, 10/FULL, 10/HALF.
> 
> So it should pick 1000Full if available. If not it will try 1000Half, if not 100Full
> etc.
> 

As per code referred below, 
if (!match && p->speed <= speed)     ==> This condition will NEVER meet for any supported speed(p->speed), when speed is not initialized(i.e., speed is -1 and p->speed is 1000 and 100 in lan887x case).
	/* Candidate */
	match = p;

if (!match && !exact)
	match = last;     ==> As there is no match this condition hits and i.e., the LOWEST speed supported as per above documentation.

return match;

For reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/net/phy/phy-core.c?h=v6.12.43#n305

> And the comment is actually a bit out of date. It will actually start from 800G
> Full, 400G Full, 200G Full, 100G Full, not that anybody does Copper at these
> speeds.
> 

True.

Thanks,
Divya

>         Andrew