net: freescale: ucc_geth: Return early when TBI found can't be found

[PATCH net] net: freescale: ucc_geth: Return early when TBI found can't be found

Posted by Maxime Chevallier 3 weeks, 4 days ago

In ucc_geth's .mac_config(), we configure the TBI block represented by a
struct phy_device that we get from firmware.

While porting to phylink, a check was missed to make sure we don't try
to access the TBI PHY if we can't get it. Let's add it and return early
in case of error

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202601130843.rFGNXA5a-lkp@intel.com/
Fixes: 53036aa8d031 ("net: freescale: ucc_geth: phylink conversion")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index affd5a6c44e7..131d1210dc4a 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -1602,8 +1602,10 @@ static void ugeth_mac_config(struct phylink_config *config, unsigned int mode,
 			pr_warn("TBI mode requires that the device tree specify a tbi-handle\n");
 
 		tbiphy = of_phy_find_device(ug_info->tbi_node);
-		if (!tbiphy)
+		if (!tbiphy) {
 			pr_warn("Could not get TBI device\n");
+			return;
+		}
 
 		value = phy_read(tbiphy, ENET_TBI_MII_CR);
 		value &= ~0x1000;	/* Turn off autonegotiation */
-- 
2.49.0

Re: [PATCH net] net: freescale: ucc_geth: Return early when TBI found can't be found

Posted by Maxime Chevallier 3 weeks, 4 days ago

Hi,

On 13/01/2026 08:43, Maxime Chevallier wrote:
> In ucc_geth's .mac_config(), we configure the TBI block represented by a
> struct phy_device that we get from firmware.
> 
> While porting to phylink, a check was missed to make sure we don't try
> to access the TBI PHY if we can't get it. Let's add it and return early
> in case of error
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202601130843.rFGNXA5a-lkp@intel.com/
> Fixes: 53036aa8d031 ("net: freescale: ucc_geth: phylink conversion")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Heh that's what I get from sending patches while having mild fever, the
patch title is all wrong and should be :

net: freescale: ucc_geth: Return early when TBI PHY can't be found

I'll wait for the 24h cooldown, grab some honey + milk and resend after :)

Maxime

> ---
>  drivers/net/ethernet/freescale/ucc_geth.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
> index affd5a6c44e7..131d1210dc4a 100644
> --- a/drivers/net/ethernet/freescale/ucc_geth.c
> +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> @@ -1602,8 +1602,10 @@ static void ugeth_mac_config(struct phylink_config *config, unsigned int mode,
>  			pr_warn("TBI mode requires that the device tree specify a tbi-handle\n");
>  
>  		tbiphy = of_phy_find_device(ug_info->tbi_node);
> -		if (!tbiphy)
> +		if (!tbiphy) {
>  			pr_warn("Could not get TBI device\n");
> +			return;
> +		}
>  
>  		value = phy_read(tbiphy, ENET_TBI_MII_CR);
>  		value &= ~0x1000;	/* Turn off autonegotiation */

Re: [PATCH net] net: freescale: ucc_geth: Return early when TBI found can't be found

Posted by Russell King (Oracle) 3 weeks, 3 days ago

On Tue, Jan 13, 2026 at 09:16:29AM +0100, Maxime Chevallier wrote:
> Hi,
> 
> On 13/01/2026 08:43, Maxime Chevallier wrote:
> > In ucc_geth's .mac_config(), we configure the TBI block represented by a
> > struct phy_device that we get from firmware.
> > 
> > While porting to phylink, a check was missed to make sure we don't try
> > to access the TBI PHY if we can't get it. Let's add it and return early
> > in case of error
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202601130843.rFGNXA5a-lkp@intel.com/
> > Fixes: 53036aa8d031 ("net: freescale: ucc_geth: phylink conversion")
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> 
> Heh that's what I get from sending patches while having mild fever, the
> patch title is all wrong and should be :
> 
> net: freescale: ucc_geth: Return early when TBI PHY can't be found
> 
> I'll wait for the 24h cooldown, grab some honey + milk and resend after :)

A question - based on dwmac:

When implementing dwmac to support 1000base-X, the dwmac doesn't
implement the _full_ 1000base-X, but only up to the PCS. The PCS
provides a TBI interface to the SerDes PHY provided by the SoC
designer which acts as the PMA layer.

The talk here of TBI makes me wonder whether the same thing is going
on with ucc_geth. Is the "TBI PHY" in fact the SerDes ?

Traditionally, we've represented the SerDes using drivers/phy rather
than the drivers/net/phy infrastructure, mainly because implementations
hvaen't provided anything like an 802.3 PHY register set, but moreover
because the SerDes tends to be generic across ethernet, PCIe, USB, SATA
etc (basically, anything that is a high speed balanced pair serial
communication) and thus the "struct phy" from drivers/phy can be used
by any of these subsystems.

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

Re: [PATCH net] net: freescale: ucc_geth: Return early when TBI found can't be found

Posted by Maxime Chevallier 3 weeks, 3 days ago

Hi Russell,

On 13/01/2026 19:44, Russell King (Oracle) wrote:
> On Tue, Jan 13, 2026 at 09:16:29AM +0100, Maxime Chevallier wrote:
>> Hi,
>>
>> On 13/01/2026 08:43, Maxime Chevallier wrote:
>>> In ucc_geth's .mac_config(), we configure the TBI block represented by a
>>> struct phy_device that we get from firmware.
>>>
>>> While porting to phylink, a check was missed to make sure we don't try
>>> to access the TBI PHY if we can't get it. Let's add it and return early
>>> in case of error
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Closes: https://lore.kernel.org/r/202601130843.rFGNXA5a-lkp@intel.com/
>>> Fixes: 53036aa8d031 ("net: freescale: ucc_geth: phylink conversion")
>>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>>
>> Heh that's what I get from sending patches while having mild fever, the
>> patch title is all wrong and should be :
>>
>> net: freescale: ucc_geth: Return early when TBI PHY can't be found
>>
>> I'll wait for the 24h cooldown, grab some honey + milk and resend after :)
> 
> A question - based on dwmac:
> 
> When implementing dwmac to support 1000base-X, the dwmac doesn't
> implement the _full_ 1000base-X, but only up to the PCS. The PCS
> provides a TBI interface to the SerDes PHY provided by the SoC
> designer which acts as the PMA layer.
> 
> The talk here of TBI makes me wonder whether the same thing is going
> on with ucc_geth. Is the "TBI PHY" in fact the SerDes ?

Yeah I think it is indeed.

> Traditionally, we've represented the SerDes using drivers/phy rather
> than the drivers/net/phy infrastructure, mainly because implementations
> hvaen't provided anything like an 802.3 PHY register set, but moreover
> because the SerDes tends to be generic across ethernet, PCIe, USB, SATA
> etc (basically, anything that is a high speed balanced pair serial
> communication) and thus the "struct phy" from drivers/phy can be used
> by any of these subsystems.
> 

True, and I completely agree with that. The reason I didn't touch that
when porting to phylink is that the device I'm using, that has a
Motorola/Freescale/NXP MPC832x, doesn't have that TBI/RTBI block, so I
can't test that at all should we move to a more modern SerDes driver
(modern w.r.t when this driver was written) :(

Maxime

Re: [PATCH net] net: freescale: ucc_geth: Return early when TBI found can't be found

Posted by Russell King (Oracle) 3 weeks, 3 days ago

On Tue, Jan 13, 2026 at 08:24:49PM +0100, Maxime Chevallier wrote:
> Hi Russell,
> > Traditionally, we've represented the SerDes using drivers/phy rather
> > than the drivers/net/phy infrastructure, mainly because implementations
> > hvaen't provided anything like an 802.3 PHY register set, but moreover
> > because the SerDes tends to be generic across ethernet, PCIe, USB, SATA
> > etc (basically, anything that is a high speed balanced pair serial
> > communication) and thus the "struct phy" from drivers/phy can be used
> > by any of these subsystems.
> > 
> 
> True, and I completely agree with that. The reason I didn't touch that
> when porting to phylink is that the device I'm using, that has a
> Motorola/Freescale/NXP MPC832x, doesn't have that TBI/RTBI block, so I
> can't test that at all should we move to a more modern SerDes driver
> (modern w.r.t when this driver was written) :(

Over the last few days, I've been adding "generic" stmmac SerDes
support (which basically means not in the platform glue) to replace
the qcom-ethqos stuff, and while doing so, the thought did cross my
mind whether I should be adding that to phylink rather than stmmac.

stmmac's "I can't reset without all the clocks running" makes it
rather special though, but we already have phylink_rx_clk_stop_block()
to guarantee that the PHY itself won't stop its receive clock when
entering LPI, so phylink already knows when the clock is required
(although with a slight abuse of the names of these functions.)

Given that the two qcom-ethqos patches I sent last night failed to
build (oops) I may change the patch order... it does need the stmmac
PCS work to be merged first though.

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

Re: [PATCH net] net: freescale: ucc_geth: Return early when TBI found can't be found

Posted by Maxime Chevallier 3 weeks, 3 days ago

Hi Russell,

On 13/01/2026 21:03, Russell King (Oracle) wrote:
> On Tue, Jan 13, 2026 at 08:24:49PM +0100, Maxime Chevallier wrote:
>> Hi Russell,
>>> Traditionally, we've represented the SerDes using drivers/phy rather
>>> than the drivers/net/phy infrastructure, mainly because implementations
>>> hvaen't provided anything like an 802.3 PHY register set, but moreover
>>> because the SerDes tends to be generic across ethernet, PCIe, USB, SATA
>>> etc (basically, anything that is a high speed balanced pair serial
>>> communication) and thus the "struct phy" from drivers/phy can be used
>>> by any of these subsystems.
>>>
>>
>> True, and I completely agree with that. The reason I didn't touch that
>> when porting to phylink is that the device I'm using, that has a
>> Motorola/Freescale/NXP MPC832x, doesn't have that TBI/RTBI block, so I
>> can't test that at all should we move to a more modern SerDes driver
>> (modern w.r.t when this driver was written) :(
> 
> Over the last few days, I've been adding "generic" stmmac SerDes
> support (which basically means not in the platform glue) to replace
> the qcom-ethqos stuff, and while doing so, the thought did cross my
> mind whether I should be adding that to phylink rather than stmmac.

You mean controlling the generic PHY (phy_power_on / off,
phy_set_mode_ext and so on) from phylink instead of the MAC driver, like
we also do in mvneta / mvpp2 ?

That would also interest the Meta folks working on fbnic I guess :)

Maxime