Re: [PATCH net-next v2 6/6] net: mdio: bcm-unimac: implement timestamped MDIO writes

[James Clark]

> [Severity: Medium]
> The comment says the reference rate is taken to be fixed. Is anything
> actually keeping it fixed? There is no clk_rate_exclusive_get() and no clk
> notifier, so the rate cached at probe can go stale if another consumer or a
> parent change alters it later.

[ ... ]

> Could the driver take clk_rate_exclusive_get() at probe to enforce this
> assumption?

The clock this is for is BCM2711's GISB clock, the SoC's register bus
clock. The firmware sets it, and nothing in Linux changes its rate.
clk_rate_exclusive_get() would also protect its parents, PLLD_PER and
PLLD, which other clocks use, to guard against a change that doesn't
happen.

> [Severity: Medium]
> The commit message says a deferral of the clock lookup is passed on
> so the MDIO bus waits for the clock's provider. Does that work on the
> GENET path?
>
> There the UniMAC device is not bound from DT. bcmgenet_mii_register()
> creates it and calls platform_device_add(ppdev), and the probe result
> of unimac_mdio_probe() is not propagated back through device_add().

[ ... ]

> write_sts is optional, so would it be simpler to treat -EPROBE_DEFER
> like any other error on the pdata path and not offer write_sts? The
> cost is that write_sts would then depend on probe order.
>
> Alternatively, bcmgenet could look up the MDC reference clock itself
> and pass it in struct unimac_mdio_pdata. A deferral would then happen
> in bcmgenet_probe(), where it is actually honoured.

We chose to pass the deferral on because the alternatives seemed worse:
treating a deferral as no clock would make write_sts depend on probe
order; looking up the clock in bcmgenet would hold up Ethernet for an
optional feature, indefinitely if the clock's provider never binds. On
BCM2711 the provider is clk-bcm2835, which is built in and probes before
GENET, so in practice GENET doesn't come up without its MDIO bus.

> [Severity: Medium]
> This isn't a bug introduced by this patch, but the new comment makes it
> more visible. unimac_mdio_clk_set() still computes the MDC divider from
> priv->clk, falling back to 250 MHz:

[ ... ]

> Now that ref_clk is available, should unimac_mdio_clk_set() use its rate
> for the divider too?

I agree it's a bug, but it predates this series and is unrelated to it:
unimac_mdio_clk_set() is unchanged, and the write_sts bounds don't
depend on it, since they read the divider back from MDIO_CFG.

James