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

[James Clark]

On Thu, Oct 15, 2026 at 10:52 PM Théo Lebrun <theo.lebrun@xxxxxxxxxxx> wrote:
>
> I'm not convinced by the idea of a flag only allowed to RP1. Our caps
> are usually to restrict what IP revisions can do; here we're talking
> about a driver implementation detail.

I take your point. I will drop that idea.

But I wonder if it makes sense to have a cap that says that the delay
is exactly 63.5 periods as it is on RP1. Without this, it would assume
the MDC is free-running and so would use bounds of 63.5 to 64.5 MDC
periods as on unimac.

> Sashiko is technically correct. But then the bug is not in your MDIO
> delay computation, it is in the PCLK/divisor/suspend code. You should
> be capable of determining the current divisor rate without reading the
> NCFGR register.

OK. In v2 I'll record the MDC rate where macb_mdc_clk_div() chooses
the divisor and use that instead of reading NCFGR.

> >> > +     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.
>
> On any platform but RP1, such a readl() is useless. My remark was that
> the readl()-for-posted-writes must be made conditional, and I don't
> think we should make the whole of write_sts conditional.

I may be missing something, but I don't think the read-back is
specific to PCIe. device-io.rst says that on many platforms and buses
writes through ioremap() are posted, so the CPU doesn't wait for the
write to reach the device, and it gives a dummy read as the way to
wait for it that works on all platforms. On arm64 the mapping is
Device-nGnRE, where the E means an intermediate point in the fabric
may acknowledge the write before it reaches the MAC. The unimac patch
in this series does the same read-back, on an on-chip controller.

James