[PATCH] net: macb: ignore tx_clk if MII is used

[PATCH] net: macb: ignore tx_clk if MII is used

[Michael Walle]

If the MII interface is used, the PHY is the clock master, thus don't
set the clock rate. On Zynq-7000, this will prevent the following
warning:
  macb e000b000.ethernet eth0: unable to generate target frequency: 25000000 Hz

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/ethernet/cadence/macb_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 814a5b10141d..472bf8f220bc 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -470,6 +470,10 @@ static void macb_set_tx_clk(struct macb *bp, int speed)
 	if (!bp->tx_clk || (bp->caps & MACB_CAPS_CLK_HW_CHG))
 		return;
 
+	/* In case of MII the PHY is the clock master */
+	if (bp->phy_interface == PHY_INTERFACE_MODE_MII)
+		return;
+
 	switch (speed) {
 	case SPEED_10:
 		rate = 2500000;
-- 
2.20.1

Re: [PATCH] net: macb: ignore tx_clk if MII is used

[Andrew Lunn]

On Wed, Jan 20, 2021 at 08:43:03PM +0100, Michael Walle wrote:
> If the MII interface is used, the PHY is the clock master, thus don't
> set the clock rate. On Zynq-7000, this will prevent the following
> warning:
>   macb e000b000.ethernet eth0: unable to generate target frequency: 25000000 Hz
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: macb: ignore tx_clk if MII is used

[Claudiu.Beznea]

Hi Michael,

On 20.01.2021 21:43, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> If the MII interface is used, the PHY is the clock master, thus don't
> set the clock rate. On Zynq-7000, this will prevent the following
> warning:
>   macb e000b000.ethernet eth0: unable to generate target frequency: 25000000 Hz
> 

Since in this case the PHY provides the TX clock and it provides the proper
rate based on link speed, the MACB driver should not handle the bp->tx_clk
at all (MACB driver uses this clock only for setting the proper rate on it
based on link speed). So, I believe the proper fix would be to not pass the
tx_clk at all in device tree. This clock is optional for MACB driver.

Thank you,
Claudiu Beznea

> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 814a5b10141d..472bf8f220bc 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -470,6 +470,10 @@ static void macb_set_tx_clk(struct macb *bp, int speed)
>         if (!bp->tx_clk || (bp->caps & MACB_CAPS_CLK_HW_CHG))
>                 return;
> 
> +       /* In case of MII the PHY is the clock master */
> +       if (bp->phy_interface == PHY_INTERFACE_MODE_MII)
> +               return;
> +
>         switch (speed) {
>         case SPEED_10:
>                 rate = 2500000;
> --
> 2.20.1
> 

Re: [PATCH] net: macb: ignore tx_clk if MII is used

[Michael Walle]

Hi Claudiu,

Am 2021-01-21 10:19, schrieb Claudiu.Beznea@microchip.com:
> On 20.01.2021 21:43, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> If the MII interface is used, the PHY is the clock master, thus don't
>> set the clock rate. On Zynq-7000, this will prevent the following
>> warning:
>>   macb e000b000.ethernet eth0: unable to generate target frequency: 
>> 25000000 Hz
>> 
> 
> Since in this case the PHY provides the TX clock and it provides the 
> proper
> rate based on link speed, the MACB driver should not handle the 
> bp->tx_clk
> at all (MACB driver uses this clock only for setting the proper rate on 
> it
> based on link speed). So, I believe the proper fix would be to not pass 
> the
> tx_clk at all in device tree. This clock is optional for MACB driver.

Thanks for looking into this.

I had the same thought. But shouldn't the driver handle this case 
gracefully?
I mean it does know that the clock isn't needed at all. Ususually that 
clock
is defined in a device tree include. So you'd have to redefine that node 
in
an actual board file which means duplicating the other clocks.

-michael

Re: [PATCH] net: macb: ignore tx_clk if MII is used

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Wed, 20 Jan 2021 20:43:03 +0100 you wrote:
> If the MII interface is used, the PHY is the clock master, thus don't
> set the clock rate. On Zynq-7000, this will prevent the following
> warning:
>   macb e000b000.ethernet eth0: unable to generate target frequency: 25000000 Hz
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> [...]

Here is the summary with links:
  - net: macb: ignore tx_clk if MII is used
    https://git.kernel.org/netdev/net-next/c/43e5763152e2

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] net: macb: ignore tx_clk if MII is used

[Claudiu.Beznea]

On 21.01.2021 11:41, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Claudiu,
> 
> Am 2021-01-21 10:19, schrieb Claudiu.Beznea@microchip.com:
>> On 20.01.2021 21:43, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> If the MII interface is used, the PHY is the clock master, thus don't
>>> set the clock rate. On Zynq-7000, this will prevent the following
>>> warning:
>>>   macb e000b000.ethernet eth0: unable to generate target frequency:
>>> 25000000 Hz
>>>
>>
>> Since in this case the PHY provides the TX clock and it provides the
>> proper
>> rate based on link speed, the MACB driver should not handle the
>> bp->tx_clk
>> at all (MACB driver uses this clock only for setting the proper rate on
>> it
>> based on link speed). So, I believe the proper fix would be to not pass
>> the
>> tx_clk at all in device tree. This clock is optional for MACB driver.
> 
> Thanks for looking into this.
> 
> I had the same thought. But shouldn't the driver handle this case
> gracefully?
> I mean it does know that the clock isn't needed at all.

Currently it may knows that by checking the bp->tx_clk. Moreover the clock
could be provided by PHY not only for MII interface.

Moreover the IP has the bit "refclk" of register at offset 0xc (userio)
that tells it to use the clock provided by PHY or to use one internal to
the SoC. If a SoC generated clock would be used the IP logic may have the
option to do the proper division based on link speed (if IP has this option
enabled then this should be selected in driver with capability
MACB_CAPS_CLK_HW_CHG).

If the clock provided by the PHY is the one to be used then this is
selected with capability MACB_CAPS_USRIO_HAS_CLKEN. So, if the change you
proposed in this patch is still imperative then checking for this
capability would be the best as the clock could be provided by PHY not only
for MII interface.

> Ususually that
> clock
> is defined in a device tree include. So you'd have to redefine that node
> in
> an actual board file which means duplicating the other clocks.
> 
> -michael

Re: [PATCH] net: macb: ignore tx_clk if MII is used

[Michael Walle]

Am 2021-01-22 10:10, schrieb Claudiu.Beznea@microchip.com:
> On 21.01.2021 11:41, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> 
>> Hi Claudiu,
>> 
>> Am 2021-01-21 10:19, schrieb Claudiu.Beznea@microchip.com:
>>> On 20.01.2021 21:43, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> If the MII interface is used, the PHY is the clock master, thus 
>>>> don't
>>>> set the clock rate. On Zynq-7000, this will prevent the following
>>>> warning:
>>>>   macb e000b000.ethernet eth0: unable to generate target frequency:
>>>> 25000000 Hz
>>>> 
>>> 
>>> Since in this case the PHY provides the TX clock and it provides the
>>> proper
>>> rate based on link speed, the MACB driver should not handle the
>>> bp->tx_clk
>>> at all (MACB driver uses this clock only for setting the proper rate 
>>> on
>>> it
>>> based on link speed). So, I believe the proper fix would be to not 
>>> pass
>>> the
>>> tx_clk at all in device tree. This clock is optional for MACB driver.
>> 
>> Thanks for looking into this.
>> 
>> I had the same thought. But shouldn't the driver handle this case
>> gracefully?
>> I mean it does know that the clock isn't needed at all.
> 
> Currently it may knows that by checking the bp->tx_clk. Moreover the 
> clock
> could be provided by PHY not only for MII interface.

That doesn't make this patch wrong, does it? It just doesn't cover
all use cases (which also wasn't covered before).

> Moreover the IP has the bit "refclk" of register at offset 0xc (userio)
> that tells it to use the clock provided by PHY or to use one internal 
> to
> the SoC. If a SoC generated clock would be used the IP logic may have 
> the
> option to do the proper division based on link speed (if IP has this 
> option
> enabled then this should be selected in driver with capability
> MACB_CAPS_CLK_HW_CHG).
> 
> If the clock provided by the PHY is the one to be used then this is
> selected with capability MACB_CAPS_USRIO_HAS_CLKEN. So, if the change 
> you
> proposed in this patch is still imperative then checking for this
> capability would be the best as the clock could be provided by PHY not 
> only
> for MII interface.

Fair enough, but this register doesn't seem to be implemented on
Zynq-7000. Albeit MACB_CAPS_USRIO_DISABLED isn't defined for the
Zynq MACB. It isn't defined in the Zynq-7000 reference manual and
you cannot set any bits:

=> mw 0xE000B00C 0xFFFFFFFF
=> md 0xE000B00C 1
e000b00c: 00000000

Also please note, that tx_clk may be an arbitrary clock which doesn't
necessarily need to be the clock which is controlled by CLK_EN. Or
am I missing something here?

-michael

>> Ususually that
>> clock
>> is defined in a device tree include. So you'd have to redefine that 
>> node
>> in
>> an actual board file which means duplicating the other clocks.
>> 
>> -michael

Re: [PATCH] net: macb: ignore tx_clk if MII is used

[Claudiu.Beznea]

On 22.01.2021 13:20, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Am 2021-01-22 10:10, schrieb Claudiu.Beznea@microchip.com:
>> On 21.01.2021 11:41, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the
>>> content is safe
>>>
>>> Hi Claudiu,
>>>
>>> Am 2021-01-21 10:19, schrieb Claudiu.Beznea@microchip.com:
>>>> On 20.01.2021 21:43, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> If the MII interface is used, the PHY is the clock master, thus
>>>>> don't
>>>>> set the clock rate. On Zynq-7000, this will prevent the following
>>>>> warning:
>>>>>   macb e000b000.ethernet eth0: unable to generate target frequency:
>>>>> 25000000 Hz
>>>>>
>>>>
>>>> Since in this case the PHY provides the TX clock and it provides the
>>>> proper
>>>> rate based on link speed, the MACB driver should not handle the
>>>> bp->tx_clk
>>>> at all (MACB driver uses this clock only for setting the proper rate
>>>> on
>>>> it
>>>> based on link speed). So, I believe the proper fix would be to not
>>>> pass
>>>> the
>>>> tx_clk at all in device tree. This clock is optional for MACB driver.
>>>
>>> Thanks for looking into this.
>>>
>>> I had the same thought. But shouldn't the driver handle this case
>>> gracefully?
>>> I mean it does know that the clock isn't needed at all.
>>
>> Currently it may knows that by checking the bp->tx_clk. Moreover the
>> clock
>> could be provided by PHY not only for MII interface.
> 
> That doesn't make this patch wrong, does it? It just doesn't cover
> all use cases (which also wasn't covered before).

I would say that it breaks setups using MII interface and with clock
provided via DT that need to be handled by macb_set_tx_clk().

> 
>> Moreover the IP has the bit "refclk" of register at offset 0xc (userio)
>> that tells it to use the clock provided by PHY or to use one internal
>> to
>> the SoC. If a SoC generated clock would be used the IP logic may have
>> the
>> option to do the proper division based on link speed (if IP has this
>> option
>> enabled then this should be selected in driver with capability
>> MACB_CAPS_CLK_HW_CHG).
>>
>> If the clock provided by the PHY is the one to be used then this is
>> selected with capability MACB_CAPS_USRIO_HAS_CLKEN. So, if the change
>> you
>> proposed in this patch is still imperative then checking for this
>> capability would be the best as the clock could be provided by PHY not
>> only
>> for MII interface.
> 
> Fair enough, but this register doesn't seem to be implemented on
> Zynq-7000. Albeit MACB_CAPS_USRIO_DISABLED isn't defined for the
> Zynq MACB. It isn't defined in the Zynq-7000 reference manual and
> you cannot set any bits:
> 
> => mw 0xE000B00C 0xFFFFFFFF
> => md 0xE000B00C 1
> e000b00c: 00000000

I wasn't aware of this. In this case, maybe adding the
MACB_CAPS_USRIO_DISABLED to the Zync-7000 capability list and checking this
one plus MACB_CAPS_USRIO_HAS_CLKEN would be better instead of checking the
MAC-PHY interface?

> 
> Also please note, that tx_clk may be an arbitrary clock which doesn't
> necessarily need to be the clock which is controlled by CLK_EN. Or
> am I missing something here?

I suppose that whoever creates the device tree knows what is doing and it
passes the proper clock to macb driver.

Thank you,
Claudiu Beznea

> 
> -michael
> 
>>> Ususually that
>>> clock
>>> is defined in a device tree include. So you'd have to redefine that
>>> node
>>> in
>>> an actual board file which means duplicating the other clocks.
>>>
>>> -michael

Re: [PATCH] net: macb: ignore tx_clk if MII is used

[Michael Walle]

Am 2021-01-22 12:38, schrieb Claudiu.Beznea@microchip.com:
> On 22.01.2021 13:20, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> 
>> Am 2021-01-22 10:10, schrieb Claudiu.Beznea@microchip.com:
>>> On 21.01.2021 11:41, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the
>>>> content is safe
>>>> 
>>>> Hi Claudiu,
>>>> 
>>>> Am 2021-01-21 10:19, schrieb Claudiu.Beznea@microchip.com:
>>>>> On 20.01.2021 21:43, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>> know
>>>>>> the content is safe
>>>>>> 
>>>>>> If the MII interface is used, the PHY is the clock master, thus
>>>>>> don't
>>>>>> set the clock rate. On Zynq-7000, this will prevent the following
>>>>>> warning:
>>>>>>   macb e000b000.ethernet eth0: unable to generate target 
>>>>>> frequency:
>>>>>> 25000000 Hz
>>>>>> 
>>>>> 
>>>>> Since in this case the PHY provides the TX clock and it provides 
>>>>> the
>>>>> proper
>>>>> rate based on link speed, the MACB driver should not handle the
>>>>> bp->tx_clk
>>>>> at all (MACB driver uses this clock only for setting the proper 
>>>>> rate
>>>>> on
>>>>> it
>>>>> based on link speed). So, I believe the proper fix would be to not
>>>>> pass
>>>>> the
>>>>> tx_clk at all in device tree. This clock is optional for MACB 
>>>>> driver.
>>>> 
>>>> Thanks for looking into this.
>>>> 
>>>> I had the same thought. But shouldn't the driver handle this case
>>>> gracefully?
>>>> I mean it does know that the clock isn't needed at all.
>>> 
>>> Currently it may knows that by checking the bp->tx_clk. Moreover the
>>> clock
>>> could be provided by PHY not only for MII interface.
>> 
>> That doesn't make this patch wrong, does it? It just doesn't cover
>> all use cases (which also wasn't covered before).
> 
> I would say that it breaks setups using MII interface and with clock
> provided via DT that need to be handled by macb_set_tx_clk().

But the MII interface has by definition no tx clock? At the moment
tx_clk is set to the correct frequency for RGMII interfaces, right?
How would it break boards then?

And if you use tx_clk for like driving the PHY refclk input, that
would be an abuse of this clock (and shouldn't even work, because it
just sets the correct RGMII frequencies).

>>> Moreover the IP has the bit "refclk" of register at offset 0xc 
>>> (userio)
>>> that tells it to use the clock provided by PHY or to use one internal
>>> to
>>> the SoC. If a SoC generated clock would be used the IP logic may have
>>> the
>>> option to do the proper division based on link speed (if IP has this
>>> option
>>> enabled then this should be selected in driver with capability
>>> MACB_CAPS_CLK_HW_CHG).
>>> 
>>> If the clock provided by the PHY is the one to be used then this is
>>> selected with capability MACB_CAPS_USRIO_HAS_CLKEN. So, if the change
>>> you
>>> proposed in this patch is still imperative then checking for this
>>> capability would be the best as the clock could be provided by PHY 
>>> not
>>> only
>>> for MII interface.
>> 
>> Fair enough, but this register doesn't seem to be implemented on
>> Zynq-7000. Albeit MACB_CAPS_USRIO_DISABLED isn't defined for the
>> Zynq MACB. It isn't defined in the Zynq-7000 reference manual and
>> you cannot set any bits:
>> 
>> => mw 0xE000B00C 0xFFFFFFFF
>> => md 0xE000B00C 1
>> e000b00c: 00000000
> 
> I wasn't aware of this. In this case, maybe adding the
> MACB_CAPS_USRIO_DISABLED to the Zync-7000 capability list and checking 
> this
> one plus MACB_CAPS_USRIO_HAS_CLKEN would be better instead of checking 
> the
> MAC-PHY interface?

But then RGMII would be broken. Zynq and ZynqMP are the only users of
the tx_clk as far as I can see. There, it will set the clock generated
by the clock controller which is driving the TX_CLK for RGMII and the
corresponding input of the GEM/MACB.

I mean you have the same SoC, thus the same caps and usrio settings,
but you have two possible interfaces: MII and RGMII. (At least, because
there might also be other interfaces like GMII). Therefore, you cannot
check this bit, right? It will be the same for both MII and RGMII mode.

In any case, it should be correct to add MACB_CAPS_USRIO_DISABLED to
caps for the zynq family (I'd need to double check the ZynqMP RM).

>> Also please note, that tx_clk may be an arbitrary clock which doesn't
>> necessarily need to be the clock which is controlled by CLK_EN. Or
>> am I missing something here?
> 
> I suppose that whoever creates the device tree knows what is doing and 
> it
> passes the proper clock to macb driver.

Mh. If CLK_EN is supported this might be the case. Unfortunalty, Zynq
is the only user and doesn't have this bit.

-michael

PS. dont' get me wrong, I'm all for fixing this "the right way".