Re: [PATCH 1/5] hw/net/imx_fec: Support two Ethernet interfaces connected to single MDIO bus

[Guenter Roeck]

On Thu, Mar 30, 2023 at 05:31:13PM +0100, Peter Maydell wrote:
> On Wed, 15 Mar 2023 at 14:52, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > The SOC on i.MX6UL and i.MX7 has 2 Ethernet interfaces. The PHY on each may
> > be connected to separate MDIO busses, or both may be connected on the same
> > MDIO bus using different PHY addresses. Commit 461c51ad4275 ("Add a phy-num
> > property to the i.MX FEC emulator") added support for specifying PHY
> > addresses, but it did not provide support for linking the second PHY on
> > a given MDIO bus to the other Ethernet interface.
> >
> > To be able to support two PHY instances on a single MDIO bus, two properties
> > are needed: First, there needs to be a flag indicating if the MDIO bus on
> > a given Ethernet interface is connected. If not, attempts to read from this
> > bus must always return 0xffff. Implement this property as phy-connected.
> > Second, if the MDIO bus on an interface is active, it needs a link to the
> > consumer interface to be able to provide PHY access for it. Implement this
> > property as phy-consumer.
> >
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> > @@ -282,11 +282,19 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
> >      uint32_t val;
> >      uint32_t phy = reg / 32;
> >
> > -    if (phy != s->phy_num) {
> > -        trace_imx_phy_read_num(phy, s->phy_num);
> > +    if (!s->phy_connected) {
> >          return 0xffff;
> >      }
> >
> > +    if (phy != s->phy_num) {
> > +        if (s->phy_consumer && phy == s->phy_consumer->phy_num) {
> > +            s = s->phy_consumer;
> 
> This does work, but it leaves me wondering if we should really
> be modelling the phy as a separate device object, so that we
> can use link properties to connect the right phy to the right
> IMXFECState rather than having this odd "actually use the pointer
> to this other instance of the device"... A quick glance through

Possibly, but I don't understand well enough how this would work
to be able to implement it. I'll be happy to test patches from others,
of course.

Thanks,
Guenter

> the code suggests that the phy and the ethernet controller
> proper don't really care about each others' internals.
> (imx_phy_update_irq() does call imx_eth_update() but AFAICT
> that is unnecessary because imx_eth_update() doesn't care about
> any of the phy state...)
> 
> thanks
> -- PMM