cgem: fix media changes on SiFive platforms.

[skibo]

Summary

The SiFive SoCs handle the TX clock differently than Zynq platforms. At
Gigabit speed, the GEMGXLPLL in prci ("pclk" in the DTS) provides the reference
clock. But, at 100M and 10M speeds, the clock comes from the MII interface.
A bit in the "GEMGXL Management" block needs to be set to choose the clock
source.

Test Plan

Testing on a SiFive HiFive Unmatched.

[jrtc27]

I was thinking it would be better to shim the clock like Linux does so cgem_mediachange doesn't need to be changed and all the new logic is localised

[skibo]

In D31148#700947, @jrtc27 wrote:

I was thinking it would be better to shim the clock like Linux does so cgem_mediachange doesn't need to be changed and all the new logic is localised

I thought this is how the Linux driver handles it. The "management" block registers are in this driver's device-tree node and the tx clock select is set in the Linux driver in fu540_macb_tx_set_rate().

[jrtc27]

In D31148#701028, @skibo wrote:

In D31148#700947, @jrtc27 wrote:

I was thinking it would be better to shim the clock like Linux does so cgem_mediachange doesn't need to be changed and all the new logic is localised

I thought this is how the Linux driver handles it. The "management" block registers are in this driver's device-tree node and the tx clock select is set in the Linux driver in fu540_macb_tx_set_rate().

Which is called via the set_rate member of their fu540_c000_ops clk_ops struct, ie their equivalent of clknode's set_freq. Their macb_set_tx_clk (equivalent of mediachange) just blindly calls clk_set_rate, i.e. our clk_set_freq.

[skibo]

In D31148#701041, @jrtc27 wrote:

In D31148#701028, @skibo wrote:

In D31148#700947, @jrtc27 wrote:

I was thinking it would be better to shim the clock like Linux does so cgem_mediachange doesn't need to be changed and all the new logic is localised

I thought this is how the Linux driver handles it. The "management" block registers are in this driver's device-tree node and the tx clock select is set in the Linux driver in fu540_macb_tx_set_rate().

Which is called via the set_rate member of their fu540_c000_ops clk_ops struct, ie their equivalent of clknode's set_freq. Their macb_set_tx_clk (equivalent of mediachange) just blindly calls clk_set_rate, i.e. our clk_set_freq.

How would that look? The gemgxclk clock driver in fu540_prci.c calls a routine here to set the clock select bit?

[skibo]

How would that look? The gemgxclk clock driver in fu540_prci.c calls a routine here to set the clock select bit?

Oh wait, I see. They create a new clock, tx_clk, in the ethernet driver. I think I get it. Let me think this over a bit...

[skibo]

cgem: fix media changes on SiFive platforms.

Create a local clock node which controls the GEMGXL Management block
which chooses wether the transmit clock comes from an internal clock
in the PRCI or from a clock from the MII interface.

[jrtc27]

sys/dev/cadence/if_cgem.c

1524

Use sifive not fu540 everywhere, I've deliberately de-FU540'ed everything that's true for both boards.

1525

Just put a pointer to the parent's softc in here and use its resources, don't need to alloc them twice (and then leak them if you ever detach...).

1567

This should be much stricter and return something like EINVAL if it's not one of the three supported frequencies.

1597–1605

This belongs in the clock's softc; cgem's softc should not know anything about it, just that cgem_fu540_clk_setup is a magic function that ensures its ref_clk is set to something that works.

1607–1610

How could this ever be non-NULL?

[skibo]

sys/dev/cadence/if_cgem.c

1525

This is a pointer to the GEMGXL Management block gizmo (0x100a0000) . The memory resources for that aren't in the cgem softc.

1597–1605

Okay but

1607–1610

There is no way to remove the clkdom (or the tx_clk) in the detach. If this were a module and were detached and reattached, the clkdom and tx_clk are already there. If it isn't checked, there would be multiple clkdom's connected to this driver in the linked list of clkdom's. Even though we don't support this as a module, I kind of like it to work because some have asked if it could be a module.

[jrtc27]

sys/dev/cadence/if_cgem.c

1525

Ah, ok

1607–1610

I guess, though detach is pretty dodgy in general for clocks. So long as the device doesn't go away this is fine, but it's still not going to work for devctl clear driver (or maybe it does but just leaks?).

[skibo]

• Address some revision feedback.

[skibo]

sys/dev/cadence/if_cgem.c

1607–1610

Any idea how to use devctl to reattach a driver and save me from building and testing as a module? I always test "devctl detach" but it won't let me attach again.

[mhorne]

Hi. This change appears to be in pretty good shape, but needs to be rebased, I think. What is needed in terms of review or testing? I'd like to help get this in the tree.

I note that this clock selection only happens during attach, is it intended that the media speed can't be changed on-the-fly?