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

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