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

["Cédric Le Goater" <clg@kaod.org>]

On 1/14/19 4:29 AM, Joel Stanley wrote:
> On Fri, 11 Jan 2019 at 23:58, Cédric Le Goater <clg@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 <clg@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 <joel@jms.id.au>


Thanks,

C.


> Cheers,
> 
> Joel
>