On 1/14/19 4:29 AM, Joel Stanley wrote: > On Fri, 11 Jan 2019 at 23:58, Cédric Le Goater <c...@kaod.org> 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 <c...@kaod.org> >> --- >> 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?
It is. Check do_phy_new_ctl() passing a 'uint8_t reg'. There is not much point of adding these small changes without the bigger one adding the new MDC/MDIO interfaces. That's why I merged them all in one single patch. >> @@ -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. Yes. 0 is the default setting of 'Feature Register' > Did you give this a go with both -kernel and a u-boot mtd image? Yes. > 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 <j...@jms.id.au> Thanks, C. > Cheers, > > Joel >