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

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

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

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

    Andrew

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

Hi Claudiu,

Am 2021-01-21 10:19, schrieb Claudiu.Beznea@microchip.com:

On 20.01.2021 21:43, Michael Walle wrote:

quoted

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

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

[...]

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

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:

quoted

On 20.01.2021 21:43, Michael Walle wrote:

quoted

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

Am 2021-01-22 10:10, schrieb Claudiu.Beznea@microchip.com:

On 21.01.2021 11:41, Michael Walle wrote:

quoted

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:

quoted

On 20.01.2021 21:43, Michael Walle wrote:

quoted

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

quoted

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

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:

quoted

On 21.01.2021 11:41, Michael Walle wrote:

quoted

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:

quoted

On 20.01.2021 21:43, Michael Walle wrote:

quoted

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

quoted

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

quoted

quoted

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

Am 2021-01-22 12:38, schrieb Claudiu.Beznea@microchip.com:

On 22.01.2021 13:20, Michael Walle wrote:

quoted

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:

quoted

On 21.01.2021 11:41, Michael Walle wrote:

quoted

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:

quoted

On 20.01.2021 21:43, Michael Walle wrote:

quoted

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

quoted

quoted

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

quoted

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