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

[Peter Maydell]

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


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

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

>          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