Re: [PATCH 10/21] net: phy: add X-Powers AC200 EPHY control driver

On Mon, Aug 3, 2026 at 8:07=E2=80=AFPM Chen-Yu Tsai <[email protected]> wrote=
:
>
> On Tue, Aug 4, 2026 at 2:45=E2=80=AFAM James Hilliard <james.hilliard1@gm=
ail.com> wrote:
> >
> > On Mon, Aug 3, 2026 at 11:57=E2=80=AFAM Andrew Lunn <[email protected]> wr=
ote:
> > >
> > > On Mon, Aug 03, 2026 at 10:09:01AM -0600, James Hilliard wrote:
> > > > On Mon, Aug 3, 2026 at 7:51=E2=80=AFAM Andrew Lunn <[email protected]>=
 wrote:
> > > > >
> > > > > > +static int
> > > > > > +ac200_ephy_ctl_set_led_outputs(struct acx00_ephy_control *cont=
rol,
> > > > > > +                            unsigned long outputs)
> > > > > > +{
> > > > > > +}
> > > > >
> > > > > > +static int
> > > > > > +ac200_ephy_ctl_set_led_polarity(struct acx00_ephy_control *con=
trol,
> > > > > > +                             bool active_low)
> > > > > > +{
> > > > > > +}
> > > > >
> > > > > I've not looked into the details, but these look like pinmux. Doe=
s
> > > > > this I2C device have a GPIO controller? Can these pins be used fo=
r
> > > > > other things than LEDs? Maybe you should be implementing a GPIO a=
nd
> > > > > pinmux driver. The Ethernet PHY driver can then just select the
> > > > > correct pinmux configuration.
> > > >
> > > > These do not appear to be GPIO or muxable pins. The AC200
> > > > documentation describes them as three dedicated EPHY LED
> > > > outputs: link/activity, speed, and duplex. The bits in SYS_EPHY_CTL=
1
> > > > enable those output pads, while EPHY_CTL provides their shared
> > > > polarity setting. I have not found any documented GPIO data
> > > > registers or alternative functions for these pins.
> > >
> > > So what i'm trying to do with both of these is make it look more like
> > > standard Linux building blocks.
> > >
> > > If that is not possible, i would actually put all this code into the
> > > PHY driver. You then have a regmap property to gain access to the I2C
> > > registers.
> >
> > I understand the goal of using standard Linux building blocks where the=
y
> > fit, but I do not think folding these control paths into the common PHY
> > driver gives the right device model here.
> >
> > The AC200 and AC300 expose the same Clause 22 PHY interface, but their
> > control functions are separate devices with different transports and
> > lifetimes. The AC200 control function is an MFD child accessed through
> > the parent I2C regmap. The AC300 control function is a separately
> > addressed non-PHY MDIO device. A regmap reference only describes the
> > AC200 transport; it does not represent the AC300 control device.
> >
> > Folding both implementations into the common PHY driver would make that
> > driver manage AC200 MFD registers and resources as well as an AC300 MDI=
O
> > control endpoint. That would mix the package-specific control transport=
s
> > with the otherwise common Clause 22 implementation.
> >
> > The current split uses standard subsystems wherever they provide a
> > matching abstraction: MFD, regmap, MDIO devices, clocks, regulators,
> > NVMEM, device links, and the PHY LED framework. The narrow private
> > interface covers only the compound operation which those frameworks
> > cannot express: calibrating, addressing, configuring, and sequencing th=
e
> > package control block before the Clause 22 interface becomes usable.
> >
> > The link address is an important example. It is defined once by
> > phydev->mdio.addr. AC200 must program that value through its I2C contro=
l
> > interface, while AC300 must validate it against the address of its MDIO
> > control endpoint. Generic PHY operations do not carry a consumer MDIO
> > address. Encoding it in a phys specifier or control-node property would
> > duplicate the PHY node's reg value. Giving the common PHY driver a raw
> > regmap avoids passing the address only by moving the provider-specific
> > implementation across the driver boundary.
> >
> > The LED pad-enable and polarity bits are similarly part of the package
> > control function. They control dedicated EPHY outputs rather than GPIOs
> > or muxable pins. The common PHY driver still owns the PHY LED semantics=
,
> > while the selected control driver performs the package-specific registe=
r
> > access.
> >
> > For those reasons, I think keeping the transport-specific control drive=
rs
> > and the common Clause 22 PHY driver separate is the cleaner division.
>
> I think what Andrew is saying is that you can expose the regmap from the
> MFD node, which you then reference from the PHY node with a custom phandl=
e.
>
> In the PHY driver, the first thing to do is retrieve the syscon, and if i=
t
> is not available yet, return -EPROBE_DEFER. Then you program all the bits
> you currently do, just without going through a layer of redirection.
> You can fetch the PHY address to program from the device tree. The downsi=
de
> to doing all the setup in the PHY driver is that the PHY node has to have
> the PHY ID in the compatible, as it is no longer generic. But you already
> have that.

I agree that direct regmap access would be reasonable for an
AC200-only driver, but I do not think it is a good fit for the
combined AC200/AC300 case.

The proposed phandle only models AC200. AC300 has no corresponding
MFD regmap: its control block is a separate non-PHY Clause 22
endpoint, with its own clock, regulator and sequencing requirements.
Folding everything into the link-PHY driver would therefore still
require two transport-specific implementations -- raw AC200 I2C
regmap access and raw AC300 MDIO access -- plus conditional resource
acquisition and cleanup. It would remove the function-pointer calls,
but merely replace them with AC200/AC300 branches throughout the
common driver.

This also matters for boards sharing one board identity but
containing either package, as selected by SID. Fixed boards can
reference one control provider, while variant boards can reference
both and let the common link-PHY driver select exactly one. The
selected provider then owns its transport-specific resources and
lifetime. That avoids making an MDIO PHY driver directly manage an
I2C MFD function on one system and a second MDIO device on another.

The control interface is also used for more than initial probe. It
participates in suspend/resume and reset sequencing, interface-mode
changes once the MAC attaches, and package LED-pad configuration.
Consequently, exposing a raw regmap would not reduce this to a
one-time register setup; the common driver would still need
package-specific implementations of all those operations.

The PHY address should not be read from firmware again. The MDIO core
has already parsed reg, and the authoritative address is
phydev->mdio.addr. Passing that address to the selected provider
avoids duplicating or independently interpreting the firmware
description. Similarly, the forced ethernet-phy-id0044.1400
compatible is required in either design because the identification
registers are inaccessible until the package control block has been
initialized; it is not a consequence of using a separate control
driver.

Also, AC200 is an I2C MFD rather than a syscon. Exposing its regmap as
a syscon solely so another bus device can perform raw accesses would
blur the ownership of the AC200 function.

The private interface is intentionally narrow: it contains only the
operations whose implementation differs between the two packages,
while all normal Clause 22 behavior remains in one shared PHY driver.
Given the AC200-only, AC300-only and runtime-selected board
configurations, I think that separation provides a cleaner boundary
than moving both control backends into the common link-PHY driver.