Re: [PATCH v5 3/3] hw/net/imx_fec: improve PHY implementation.

[Jean-Christophe DUBOIS]

Le 15/06/2020 à 15:03, Peter Maydell a écrit :
> On Thu, 4 Jun 2020 at 13:39, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
> > improve the PHY implementation with more generic code.
> >
> > This patch remove a lot of harcoded values to replace them with
> > generic symbols from header files.
> >
> > Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> > ---
> >   v2: Not present
> >   v3: Not present
> >   v4: Not present
> >   v5: improve PHY implementation.
> >
> >   hw/net/imx_fec.c     | 76 +++++++++++++++++++++++++++-----------------
> >   include/hw/net/mii.h |  4 +++
> >   2 files changed, 50 insertions(+), 30 deletions(-)
>
> > -    case 5:     /* Auto-neg Link Partner Ability */
> > -        val = 0x0f71;
> > +    case MII_ANLPAR:     /* Auto-neg Link Partner Ability */
> > +        val = / | MII_ANLPAR_10 | MII_ANLPAR_10FD |
> > +              MII_ANLPAR_TX | MII_ANLPAR_TXFD | MII_ANLPAR_PAUSE |
> > +              MII_ANLPAR_PAUSEASY;
> The old value is 0x0f71, but the new one with the constants
> is 0x0de1.

First of I should say that this PHY, first borrowed by the mfc_fec.c 
(coldfire ethernet device) from lan9118 (and now by imx_fec.c) is not 
one used on any real i.MX (i.MX6, i.MX7, i.MX31, i.MX25, ...) based 
board that I know of (this particular PHY is embedded n the lan9118 
ethernet device)

It is there because we were in need of a PHY and this PHY needs to be 
simple and more or less standard.

I might have missed something but I am not really aware of way in Qemu 
to swap PHYs for a given ethernet emulator depending on the emulated board.

So here this PHY was just a blind cut and paste of the lan9118.c PHY 
part to get a reasonable working PHY for the FEC/ENET device.

So here the previous value of this register is not really meaningful. It 
is a mix of standard MII defined bits and LAN911X specific bits (for 
which I don't necessarily have definition ).

Here I decided to restrict the implementation of this rather "virtual" 
PHY to only standard defined bits

actually I think, I should have removed a lot more lan911x specific 
bits/registers to get to a really simple/trivial standard PHY.

> > -    case 30:    /* Interrupt mask */
> > +    case MII_SMC911X_IM:    /* Interrupt mask */
> >           val = s->phy_int_mask;
> >           break;
> > -    case 17:
> > -    case 18:
> > +    case MII_NSR:
> > +        val = 1 << 6;
> > +        break;
> The old code didn't have a case for MII_NSR (16).

I am not sure anymore why I added MII_NSR register. It is not present on 
lan9118 ethernet device but it is a standard defined register.

> > +    case MII_LBREMR:
> > +    case MII_REC:
> >       case 27:
> >       case 31:
>
> > -    case 4:     /* Auto-neg advertisement */
> > -        s->phy_advertise = (val & 0x2d7f) | 0x80;
> > +    case MII_ANAR:     /* Auto-neg advertisement */
> > +        s->phy_advertise = (val & (MII_ANAR_PAUSE_ASYM | MII_ANAR_PAUSE |
> > +                                   MII_ANAR_TXFD | MII_ANAR_TX |
> > +                                   MII_ANAR_10FD | MII_ANAR_10 | 0x1f)) |
> > +                                   MII_ANAR_TX;
> The old code does & 0x2d7f; the new code is & 0xdff.
Same reason as the ANLPAR register.
> >           break;
> If some of these are bug fixes, please can you put them in a separate
> patch, so that the "use symbolic constants" change can be reviewed
> as making no functional changes?
>
> thanks
> -- PMM