On 1/14/19 4:29 AM, Joel Stanley wrote:
> On Fri, 11 Jan 2019 at 23:58, Cédric Le Goater <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
Thanks,
C.
> Cheers,
>
> Joel
>
