On 2026-10-29 at 09:25:23, Peter Robinson ([email protected]) wrote:
Hi Peter, Tom,
Thanks for the review, and sorry for the late reply.
> I would typically expect a driver, even if explicitly labelled not
> ready for merge, to enable people to see how this would work.
Agreed. I will post the MAC driver as a separate RFC series and link
it from the v2 cover letter, so the interface can be reviewed together
with its user.
> Do you happen to know if there's other drivers in U-Boot currently
> that are poking the PCS interfaces directly? Not asking you to convert
> them, more just wondering if it's been hacked up elsewhere.
Yes, more than just Airoha. I went through the tree. The same
Synopsys XPCS is already driven by hand in four places:
drivers/net/fsl_enetc_xpcs_phy.c NXP ENETC / i.MX94, over MDIO
drivers/net/dwc_eth_qos_intel.c Intel mGBE, over MDIO
drivers/net/sja1105.c NXP SJA1105/SJA1110, over SPI
drivers/net/designware.c NPCM8xx only, MMIO with a
hardcoded base and the same
indirect page register scheme
> Is the DW driver based on the proposed Linux driver for this? [1]
No, it is not based on that series.
The register names and sequences
for the Clause 73 auto-negotiation and 10GBASE-R programming come from
mainline drivers/net/pcs/pcs-xpcs*.c, and I will add the Linux
copyright and SPDX lines to the file header in v2.
> Can you explain why you chose to depend on UCLASS_MISC for the
> functionality other than scope? I think I would sooner see a
> UCLASS_PCS now rather than a lot of driver duplicated work that will
> need to be fixed later. Maybe work with the Airoha maintainer to
> convert that driver as an example, the Airoha support is quite new.
I agree that a dedicated UCLASS_PCS is the better design, and I am
planning to add one in v2.
> > Questions for reviewers
> > -----------------------
> > 1. Is UCLASS_MISC with the ops in include/pcs.h acceptable as a
> > starting point?
> I would sooner we go straight to UCLASS_PCS, it might be a bit more
> work now, but will be less maintenance overall.
Agreed. Will work on this.
> > 2. Are struct pcs_config and struct pcs_state enough for other PCS
> > drivers, or is something missing?
> I would like to see what other drivers poke the PCS stuff in tree as I
> doubt this is just Airoha.
Besides Airoha, I found these drivers that program PCS :
- drivers/net/fsl_enetc_xpcs_phy.c
- drivers/net/mtk_eth/...
> > 3. Auto-negotiation completion is polled from get_state() and the
> > interrupt is not used. Is that fine for U-Boot?
> I suspect it's probably fine, does get state use the event loop interface?
No. pcs_get_state() does not block and does not use the event,
cyclic or schedule() interfaces. It reads the status registers once
and returns. The caller decides how often to call it, normally from
the MAC driver's link wait loop. The only bounded waits are in
probe and set_config() (reset and power-up, 100 ms timeouts).
> > Testing
> > -------
> > Not tested on hardware yet.
> > Build tested with CONFIG_PCS_XPCS=y on qemu_arm64.
> > checkpatch.pl --strict reports no errors.
> I would like to see actual tests as part of the patch series.
Agreed.
> > Not done
> Are you intending to do this list below? It's not clear.
> > --------
> > - Interface modes other than the three above. More modes are planned
> > once this is approved.
> Please outline where you expect those to go, it helps provide context
> to the actual reviewers rather than telling a small part of the story,
> it also may show that a PCS uclass is more appropriate than using
> misc.
> > - Clause 73 Next Pages: a link partner that uses them is rejected
> > - Register access over MDIO and the "direct" mapping
> > - Clocks and the interrupt from the binding
> > - PMA/SerDes initialisation: not done by this driver, it must be set
> > up before pcs_set_config() is called
> How is that done? How is it documented? Again the sample driver would
> likely show this.
I will document these things in the v2.
Apart from these I will have SPDX headers in the files.
Thanks,
Akash Nair
