Re: [Qemu-devel] [PATCH] ftgmac100: implement the new MDIO interface on Aspeed SoC

[Joel Stanley]

On Fri, 11 Jan 2019 at 23:58, Cédric Le Goater <address@hidden> wrote:
>
> The PHY behind the MAC of an Aspeed SoC can be controlled using two
> different MDC/MDIO interfaces. The same registers PHYCR (MAC60) and
> PHYDATA (MAC64) are involved but they have a different layout.
>
> BIT31 of the Feature Register (MAC40) controls which MDC/MDIO
> interface is active.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  hw/net/ftgmac100.c | 80 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 68 insertions(+), 12 deletions(-)

> @@ -269,9 +281,9 @@ static void phy_reset(FTGMAC100State *s)
>      s->phy_int = 0;
>  }
>
> -static uint32_t do_phy_read(FTGMAC100State *s, int reg)
> +static uint16_t do_phy_read(FTGMAC100State *s, uint8_t reg)
>  {
> -    uint32_t val;
> +    uint16_t val;

Unrelated?

> @@ -336,7 +348,7 @@ static uint32_t do_phy_read(FTGMAC100State *s, int reg)
>                         MII_BMCR_FD | MII_BMCR_CTST)
>  #define MII_ANAR_MASK 0x2d7f
>
> -static void do_phy_write(FTGMAC100State *s, int reg, uint32_t val)
> +static void do_phy_write(FTGMAC100State *s, uint8_t reg, uint16_t val)

Also unrelated?

> @@ -711,14 +771,11 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>          break;
>
>      case FTGMAC100_PHYCR:  /* PHY Device control */
> -        reg = FTGMAC100_PHYCR_REG(value);
>          s->phycr = value;
> -        if (value & FTGMAC100_PHYCR_MIIWR) {
> -            do_phy_write(s, reg, s->phydata & 0xffff);
> -            s->phycr &= ~FTGMAC100_PHYCR_MIIWR;
> +        if (s->revr & FTGMAC100_REVR_NEW_MDIO_INTERFACE) {
> +            do_phy_new_ctl(s);
>          } else {
> -            s->phydata = do_phy_read(s, reg) << 16;
> -            s->phycr &= ~FTGMAC100_PHYCR_MIIRD;
> +            do_phy_ctl(s);

I assume the guest code programs the correct phy mode so this will
work fine. This change appears to keep the existing default of the old
mode.

Did you give this a go with both -kernel and a u-boot mtd image?

Looks good to me. If you feel like splitting out the unrelated changes
do that, but I'm not fussed either way.

Reviewed-by: Joel Stanley <address@hidden>

Cheers,

Joel