Re: [PATCH net-next 4/5] net: macb: implement timestamped MDIO writes

[James Clark]

Hi Theo,

Thanks for looking at this! I am very new to this, so I really
appreciate any guidance.

(For some reason your message didn't make it into Lore, although your
followup did.)

On Sun, Oct 11, 2026 at 4:24 PM Théo Lebrun <theo.lebrun@xxxxxxxxxxx> wrote:

> I'm invoking my colleague @Maxime to get his opinion. What impacts the
> MDIO write_c22 duration and how likely is it that this duration varies
> (1) on the same MAC+PHY combo, (2) with a different PHY or (3) with a
> different MACB revision?

(3) is the one case where I think it could possibly vary.  But 63.5
periods makes sense assuming the first bit starts at the moment the
command is written and the PHY acts on it when it samples the last of
64 bits at the rising edge half-way through the last period. In 5/5
there's a different situation: there UniMAC's MDC is free-running, so
the frame can start up to a period after the command write, leading to
a possible range of 63.5-64.5. So I don't think 63.5 is as arbitrary
as it might at first seem.

> Sashiko makes an interesting point with PCLK that might change over
> time, giving an example (which I've not checked) where it would be
> derived from the CPU clk which might change with cpufreq.

I tried a bunch of different approaches before landing on what I sent
in v1, and ran Sashiko locally to review. And every approach had its
problems. Sashiko is right here, but on the other hand the existing
macb_mdc_clk_div() (it's only called at probe, open and resume) has
the same problem.

My plan for v2 was to limit this to RP1, as Sashiko suggested, with a
MACB_CAPS_MDIO_WRITE_STS flag
set in raspberrypi_rp1_config. This would solve the PCLK point since
the PCLK is fixed in the RP1.

> If you share more details on your testing setup it would help for us to
> reproduce on other hardware to gather more data points.

I created a gist with the code I used (this is all agent-written)

https://gist.github.com/jclark/372822d80ba27a9648d1aea939191b53

Key point is that the measurement technique depends on there being a
PHY with a PHC connected over MDIO, which is the case when write_sts
is useful. I believe the RP1 is the only such case in the tree at the
moment. If in the future there's another case, then it can be measured
and added using the same technique.

> I'm wondering if we could do better than a hardcoded delay? For example
> we could busy-wait for NSR.IDLE inside the prets/postts region, but
> that implies at least 25µs with IRQs disabled. Or we could do a few
> measurements at init to sample our platform delay?

On the RP1, IDLE is not the edge: IDLE comes at 65.0 periods. The only
way we could measure the edge is by using the PHY as an external
reference.

> > +     unsigned long rate = READ_ONCE(bp->mdio_sts_rate);
> > +     u32 config = macb_readl(bp, NCFGR);
> > +     u32 divisor;
> > +
> > +     if (macb_is_gem(bp))
> > +             divisor = gem_divisors[GEM_BFEXT(CLK, config)];
> > +     else
> > +             divisor = macb_divisors[MACB_BFEXT(CLK, config)];
>
> I'm a bit surprised to see you reading NCFGR.CLK and derive the current
> divisor from that. The divisor is set by us in macb_mdc_clk_div(), with
> only input being the PCLK rate.

This was something a local Sashiko review found in an earlier version.
The divider in hardware isn't always the one macb_mdc_clk_div() chose:
after suspend with the interface down, macb_resume() doesn't reprogram
NCFGR.

Incidentally, it is also what made the measurement technique work: the
bounds follow a divider changed through /dev/mem.

> > +     if (sts) {
> > +             delay_ns = macb_mdio_sts_delay(bp);
> > +             local_irq_save(flags);
> > +             ptp_read_system_prets(sts);
> > +             /* macb_writel() is relaxed; order it after the timestamp. */
> > +             mb();
> > +     }
> > +     macb_writel(bp, MAN, cmd);
> > +     if (sts) {
> > +             /* Flush the posted write before taking the upper bound. */
> > +             macb_readl(bp, NSR);
>
> An unconditional readl for flushing the posted write won't do it.
> MACB is over PCI only on very few platforms. I guess the straightforward
> solution is a POSTED_WRITES flag that is then read from here.

This is only under sts.  Assuming write_sts becomes RP1 only, then it
is only over PCIe. I am not sure if I have understood what you are
getting at here.

> > +             /* Order the read-back before the system timestamp. */
> > +             rmb();
>
> Why rmb() and not mb()?

The rmb() here only has to order the NSR read before the post
timestamp read, since by the same device rule that read can't complete
before the MAN write has arrived.

> > +     /* Record the pclk rate the MDC divider is chosen from, for
> > +      * write_sts, which can't call clk_get_rate() under the MDIO bus
> > +      * lock.
> > +      */
> > +     rate = clk_get_rate(bp->pclk);
> > +     if (rate)
> > +             WRITE_ONCE(bp->mdio_sts_rate, rate);
>
> Can you clarify here and in the commit message why clk_get_rate() is
> unacceptable under the MDIO bus lock?
>
> Seeing your comment, my guess is an ABBA deadlock risk between
> clk_prepare_lock and mii_bus->mdio_lock but please write it out.

Your guess is right. I tried this earlier and a local Sashiko review
found the problem:

bcm_ptp_gettimex()
  bcm_phy_write_exp_sts()
    phy_lock_mdio_bus()
    __mdiobus_write_sts()
      macb_mdio_write_c22_sts()
        macb_mdio_sts_delay()
          clk_get_rate()
            clk_prepare_lock()

clk_summary_show()
  clk_prepare_lock()
  clk_summary_show_one()
    clk_core_is_enabled()
      en8811h_clk_is_enabled()
        air_phy_buckpbus_reg_read()
          phy_select_page()
            phy_save_page()
              phy_lock_mdio_bus()

I will clarify this in the comment and commit message.

James