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

[Peter Maydell]

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
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