[RFC net-next v1 2/7] net: macb: warn on pclk use as a tsu_clk fallback

[Conor Dooley <conor@kernel.org>]

From: Conor Dooley <conor.dooley@microchip.com>

Not a serious patch as-is, but I think this pclk fallback code is not
really a good idea.

I think the reason that it exists is that there is a parameter for the
IP that will make the hardware use the pclk to clock the timestamper,
but from a devicetree and driver point of view I don't think this is
actually really relevant and this code is just bug prone.

It makes more sense for the binding/driver if the tsu_clk clock
represents whatever clock is clocking the timestamper, not specifically
the tsu_clk input to the IP block, because what it does at the moment
will register the ptp clock with an incorrect clock in the "right" (or
wrong I guess) circumstances. This can happen when the capability
MACB_CAPS_GEM_HAS_PTP is set for the integration, MACB_USE_HWSTAMP is
set (which a multiplatform kernel would) but the devicetree does not
provide a tsu_clk. Sure, that probably means the devicetree is wrong,
but there's no per-compatible clocks enforcement in the binding so
getting it right might not be so easy!

It's my opinion that, regardless of the way the parameter is set, it
makes sense for the binding/driver if the "tsu_clk" clock actually
represents the clock being used by the timestamper, not strictly the
clock provided to the tsu_clk input to the IP block. That's because
there appears to be nothing done differently between the two cases
w.r.t. configuring the hardware at runtime and it allows us to be sure
the ptp clock will not be registered with the wrong clock. Obviously,
for compatibility reasons I can't just delete this fallback though,
because there are devices using it, so just warn in the hopes that the
devices that actually use pclk for the timestamper can be updated to
explicitly provide it.

Out of the devices that claim MACB_CAPS_GEM_HAS_PTP the fu540, mpfs,
sama5d2 and sama7g5-emac (but not sama7g5-gem) are at risk of having
this problem. It may be that these platforms actually do use the pclk
for the timestamper (either by supplying pclk to the tsu_clk input of
the IP, or by having the IP block configured to use pclk instead of the
tsu_clk input). mpfs is wrong though, it does not use pclk for the
tsu_clk, so the driver is registering the ptp clock incorrectly.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
As I say, not a serious patch at the moment, but I'd like to know what
folks think here. All of the Xilinx platforms I looked at explicitly
provide the tsu_clk, so aren't impacted.
I know this changes the meaning of the dt-binding, but I don't think it
is harmful to do so, as it is a nop for any existing devices that
provide tsu_clk.
---
 drivers/net/ethernet/cadence/macb_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ca2386b83473..b9248f52dd5b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3567,6 +3567,7 @@ static unsigned int gem_get_tsu_rate(struct macb *bp)
 	else if (!IS_ERR(bp->pclk)) {
 		tsu_clk = bp->pclk;
 		tsu_rate = clk_get_rate(tsu_clk);
+		dev_warn(&bp->pdev->dev, "devicetree missing tsu_clk, using pclk as fallback\n");
 	} else
 		return -ENOTSUPP;
 	return tsu_rate;
-- 
2.51.0