[PATCH net] net: mdio: mdio-bcm-unimac: Correct rate fallback logic

[PATCH net] net: mdio: mdio-bcm-unimac: Correct rate fallback logic

[Florian Fainelli]

In case the rate for the parent clock is zero, make sure that we still
fallback to using a fixed rate for the divider calculation, otherwise we
simply ignore the desired MDIO bus clock frequency which can prevent us
from interfacing with Ethernet PHYs properly.

Fixes: ee975351cf0c ("net: mdio: mdio-bcm-unimac: Manage clock around I/O accesses")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/mdio/mdio-bcm-unimac.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mdio/mdio-bcm-unimac.c b/drivers/net/mdio/mdio-bcm-unimac.c
index b6e30bdf5325..9c0a11316cfd 100644
--- a/drivers/net/mdio/mdio-bcm-unimac.c
+++ b/drivers/net/mdio/mdio-bcm-unimac.c
@@ -209,10 +209,9 @@ static int unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
 	if (ret)
 		return ret;
 
-	if (!priv->clk)
+	rate = clk_get_rate(priv->clk);
+	if (!priv->clk || !rate)
 		rate = 250000000;
-	else
-		rate = clk_get_rate(priv->clk);
 
 	div = (rate / (2 * priv->clk_freq)) - 1;
 	if (div & ~MDIO_CLK_DIV_MASK) {
-- 
2.34.1

Re: [PATCH net] net: mdio: mdio-bcm-unimac: Correct rate fallback logic

[Andrew Lunn]

On Tue, Jul 29, 2025 at 02:31:48PM -0700, Florian Fainelli wrote:
> In case the rate for the parent clock is zero,

Is there a legitimate reason the parent clock would be zero?

I can understand an optional clock being missing, but it seems odd
that a clock is available, but it is ticking at 0Hz?

Maybe for this case, a warning should be issued to indicate something
odd is going on?

	Andrew

Re: [PATCH net] net: mdio: mdio-bcm-unimac: Correct rate fallback logic

[Florian Fainelli]

On 7/29/25 15:20, Andrew Lunn wrote:
> On Tue, Jul 29, 2025 at 02:31:48PM -0700, Florian Fainelli wrote:
>> In case the rate for the parent clock is zero,
> 
> Is there a legitimate reason the parent clock would be zero?

Yes there is, the parent clock might be a gated clock that aggregates 
multiple sub-clocks and therefore has multiple "parents" technically. 
Because it has multiple parents, we can't really return a particular 
rate (clock provider is SCMI/firmware).

> 
> I can understand an optional clock being missing, but it seems odd
> that a clock is available, but it is ticking at 0Hz?
> 
> Maybe for this case, a warning should be issued to indicate something
> odd is going on?
> 
> 	Andrew
> 

-- 
Florian

Re: [PATCH net] net: mdio: mdio-bcm-unimac: Correct rate fallback logic

[Andrew Lunn]

On Tue, Jul 29, 2025 at 03:22:57PM -0700, Florian Fainelli wrote:
> On 7/29/25 15:20, Andrew Lunn wrote:
> > On Tue, Jul 29, 2025 at 02:31:48PM -0700, Florian Fainelli wrote:
> > > In case the rate for the parent clock is zero,
> > 
> > Is there a legitimate reason the parent clock would be zero?
> 
> Yes there is, the parent clock might be a gated clock that aggregates
> multiple sub-clocks and therefore has multiple "parents" technically.
> Because it has multiple parents, we can't really return a particular rate
> (clock provider is SCMI/firmware).

O.K. Maybe add this to the commit message?

	Andrew

Re: [PATCH net] net: mdio: mdio-bcm-unimac: Correct rate fallback logic

[Florian Fainelli]

On 7/29/25 15:44, Andrew Lunn wrote:
> On Tue, Jul 29, 2025 at 03:22:57PM -0700, Florian Fainelli wrote:
>> On 7/29/25 15:20, Andrew Lunn wrote:
>>> On Tue, Jul 29, 2025 at 02:31:48PM -0700, Florian Fainelli wrote:
>>>> In case the rate for the parent clock is zero,
>>>
>>> Is there a legitimate reason the parent clock would be zero?
>>
>> Yes there is, the parent clock might be a gated clock that aggregates
>> multiple sub-clocks and therefore has multiple "parents" technically.
>> Because it has multiple parents, we can't really return a particular rate
>> (clock provider is SCMI/firmware).
> 
> O.K. Maybe add this to the commit message?

That makes sense, v2 tomorrow, thanks!
-- 
Florian

Re: [PATCH net] net: mdio: mdio-bcm-unimac: Correct rate fallback logic

[Simon Horman]

On Tue, Jul 29, 2025 at 02:31:48PM -0700, Florian Fainelli wrote:
> In case the rate for the parent clock is zero, make sure that we still
> fallback to using a fixed rate for the divider calculation, otherwise we
> simply ignore the desired MDIO bus clock frequency which can prevent us
> from interfacing with Ethernet PHYs properly.
> 
> Fixes: ee975351cf0c ("net: mdio: mdio-bcm-unimac: Manage clock around I/O accesses")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/mdio/mdio-bcm-unimac.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-bcm-unimac.c b/drivers/net/mdio/mdio-bcm-unimac.c
> index b6e30bdf5325..9c0a11316cfd 100644
> --- a/drivers/net/mdio/mdio-bcm-unimac.c
> +++ b/drivers/net/mdio/mdio-bcm-unimac.c
> @@ -209,10 +209,9 @@ static int unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
>  	if (ret)
>  		return ret;
>  
> -	if (!priv->clk)
> +	rate = clk_get_rate(priv->clk);
> +	if (!priv->clk || !rate)

If priv->clk is NULL then drivers/clk/clk.c:clk_get_rate()
(are there other relevant implementations?) will return 0.
So I think the condition above is tautological, and could be expressed as:

	if (!rate)

>  		rate = 250000000;
> -	else
> -		rate = clk_get_rate(priv->clk);
>  
>  	div = (rate / (2 * priv->clk_freq)) - 1;
>  	if (div & ~MDIO_CLK_DIV_MASK) {
> -- 
> 2.34.1
> 
>