Re: [PATCH v5 2/3] hw/net/imx_fec: Allow phy not to be the first device on the mii bus.

[Peter Maydell]

On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>
> Up to now we were allowing only one PHY device and it had to be the
> first device on the bus.
>
> The i.MX6UL has 2 Ethernet devices and can therefore have several
> PHY devices on the bus (and not necessarilly as device 0).
>
> This patch allows for PHY devices on 2nd, 3rd or any position.
>
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>

> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index eefedc252de..29e613699ee 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -280,11 +280,9 @@ static void imx_phy_reset(IMXFECState *s)
>  static uint32_t imx_phy_read(IMXFECState *s, int reg)
>  {
>      uint32_t val;
> +    uint32_t phy = reg / 32;
>
> -    if (reg > 31) {
> -        /* we only advertise one phy */
> -        return 0;
> -    }
> +    reg %= 32;

This change means we now support multiple PHYs...

>
>      switch (reg) {
>      case 0:     /* Basic Control */
> @@ -331,19 +329,18 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
>          break;
>      }
>
> -    trace_imx_phy_read(val, reg);
> +    trace_imx_phy_read(val, phy, reg);

...but the only change we actually make is to trace the phy number.
Surely if there is more than one phy then each phy needs to have
its own state (phy_control/phy_status/phy_advertise/etc), rather
than all these PHYs all sharing the same state under the hood?

It also sounds from your commit message as if there isn't a
large number of PHYs, but only perhaps two. In that case
don't we need to fail attempts to access non-existent PHYs
and only work with the ones which actually exist on any
given board?

thanks
-- PMM