Patchwork ftgmac100: implement the new MDIO interface on Aspeed SoC

login
register
mail settings
Submitter Cédric Le Goater
Date Jan. 11, 2019, 12:57 p.m.
Message ID <20190111125759.31577-1-clg@kaod.org>
Download mbox | patch
Permalink /patch/697669/
State New
Headers show

Comments

Cédric Le Goater - Jan. 11, 2019, 12:57 p.m.
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(-)
Andrew Jeffery - Jan. 13, 2019, 10:52 p.m.
On Fri, 11 Jan 2019, at 23:27, Cédric Le Goater 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>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  hw/net/ftgmac100.c | 80 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 68 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 909c1182eebe..790430346b51 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -89,6 +89,18 @@
>  #define FTGMAC100_PHYDATA_MIIWDATA(x)       ((x) & 0xffff)
>  #define FTGMAC100_PHYDATA_MIIRDATA(x)       (((x) >> 16) & 0xffff)
>  
> +/*
> + * PHY control register - New MDC/MDIO interface
> + */
> +#define FTGMAC100_PHYCR_NEW_DATA(x)     (((x) >> 16) & 0xffff)
> +#define FTGMAC100_PHYCR_NEW_FIRE        (1 << 15)
> +#define FTGMAC100_PHYCR_NEW_ST_22       (1 << 12)
> +#define FTGMAC100_PHYCR_NEW_OP(x)       (((x) >> 10) & 3)
> +#define   FTGMAC100_PHYCR_NEW_OP_WRITE    0x1
> +#define   FTGMAC100_PHYCR_NEW_OP_READ     0x2
> +#define FTGMAC100_PHYCR_NEW_DEV(x)      (((x) >> 5) & 0x1f)
> +#define FTGMAC100_PHYCR_NEW_REG(x)      ((x) & 0x1f)
> +
>  /*
>   * Feature Register
>   */
> @@ -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;
>  
>      switch (reg) {
>      case MII_BMCR: /* Basic Control */
> @@ -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)
>  {
>      switch (reg) {
>      case MII_BMCR:     /* Basic Control */
> @@ -373,6 +385,55 @@ static void do_phy_write(FTGMAC100State *s, int 
> reg, uint32_t val)
>      }
>  }
>  
> +static void do_phy_new_ctl(FTGMAC100State *s)
> +{
> +    uint8_t reg;
> +    uint16_t data;
> +
> +    if (!(s->phycr & FTGMAC100_PHYCR_NEW_ST_22)) {
> +        qemu_log_mask(LOG_UNIMP, "%s: unsupported ST code\n", __func__);
> +        return;
> +    }
> +
> +    /* Nothing to do */
> +    if (!(s->phycr & FTGMAC100_PHYCR_NEW_FIRE)) {
> +        return;
> +    }
> +
> +    reg = FTGMAC100_PHYCR_NEW_REG(s->phycr);
> +    data = FTGMAC100_PHYCR_NEW_DATA(s->phycr);
> +
> +    switch (FTGMAC100_PHYCR_NEW_OP(s->phycr)) {
> +    case FTGMAC100_PHYCR_NEW_OP_WRITE:
> +        do_phy_write(s, reg, data);
> +        break;
> +    case FTGMAC100_PHYCR_NEW_OP_READ:
> +        s->phydata = do_phy_read(s, reg) & 0xffff;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid OP code %08x\n",
> +                      __func__, s->phycr);
> +    }
> +
> +    s->phycr &= ~FTGMAC100_PHYCR_NEW_FIRE;
> +}
> +
> +static void do_phy_ctl(FTGMAC100State *s)
> +{
> +    uint8_t reg = FTGMAC100_PHYCR_REG(s->phycr);
> +
> +    if (s->phycr & FTGMAC100_PHYCR_MIIWR) {
> +        do_phy_write(s, reg, s->phydata & 0xffff);
> +        s->phycr &= ~FTGMAC100_PHYCR_MIIWR;
> +    } else if (s->phycr & FTGMAC100_PHYCR_MIIRD) {
> +        s->phydata = do_phy_read(s, reg) << 16;
> +        s->phycr &= ~FTGMAC100_PHYCR_MIIRD;
> +    } else {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: no OP code %08x\n",
> +                      __func__, s->phycr);
> +    }
> +}
> +
>  static int ftgmac100_read_bd(FTGMAC100Desc *bd, dma_addr_t addr)
>  {
>      if (dma_memory_read(&address_space_memory, addr, bd, sizeof(*bd))) {
> @@ -628,7 +689,6 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>                            uint64_t value, unsigned size)
>  {
>      FTGMAC100State *s = FTGMAC100(opaque);
> -    int reg;
>  
>      switch (addr & 0xff) {
>      case FTGMAC100_ISR: /* Interrupt status */
> @@ -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);
>          }
>          break;
>      case FTGMAC100_PHYDATA:
> @@ -728,8 +785,7 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>          s->dblac = value;
>          break;
>      case FTGMAC100_REVR:  /* Feature Register */
> -        /* TODO: Only Old MDIO interface is supported */
> -        s->revr = value & ~FTGMAC100_REVR_NEW_MDIO_INTERFACE;
> +        s->revr = value;
>          break;
>      case FTGMAC100_FEAR1: /* Feature Register 1 */
>          s->fear1 = value;
> -- 
> 2.20.1
>
Joel Stanley - Jan. 14, 2019, 3:29 a.m.
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?

> @@ -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 <joel@jms.id.au>

Cheers,

Joel
Cédric Le Goater - Jan. 14, 2019, 7:32 a.m.
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
>
Peter Maydell - Jan. 17, 2019, 6:44 p.m.
On Fri, 11 Jan 2019 at 12: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>
> ---



Applied to target-arm.next, thanks.

-- PMM

Patch

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 909c1182eebe..790430346b51 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -89,6 +89,18 @@ 
 #define FTGMAC100_PHYDATA_MIIWDATA(x)       ((x) & 0xffff)
 #define FTGMAC100_PHYDATA_MIIRDATA(x)       (((x) >> 16) & 0xffff)
 
+/*
+ * PHY control register - New MDC/MDIO interface
+ */
+#define FTGMAC100_PHYCR_NEW_DATA(x)     (((x) >> 16) & 0xffff)
+#define FTGMAC100_PHYCR_NEW_FIRE        (1 << 15)
+#define FTGMAC100_PHYCR_NEW_ST_22       (1 << 12)
+#define FTGMAC100_PHYCR_NEW_OP(x)       (((x) >> 10) & 3)
+#define   FTGMAC100_PHYCR_NEW_OP_WRITE    0x1
+#define   FTGMAC100_PHYCR_NEW_OP_READ     0x2
+#define FTGMAC100_PHYCR_NEW_DEV(x)      (((x) >> 5) & 0x1f)
+#define FTGMAC100_PHYCR_NEW_REG(x)      ((x) & 0x1f)
+
 /*
  * Feature Register
  */
@@ -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;
 
     switch (reg) {
     case MII_BMCR: /* Basic Control */
@@ -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)
 {
     switch (reg) {
     case MII_BMCR:     /* Basic Control */
@@ -373,6 +385,55 @@  static void do_phy_write(FTGMAC100State *s, int reg, uint32_t val)
     }
 }
 
+static void do_phy_new_ctl(FTGMAC100State *s)
+{
+    uint8_t reg;
+    uint16_t data;
+
+    if (!(s->phycr & FTGMAC100_PHYCR_NEW_ST_22)) {
+        qemu_log_mask(LOG_UNIMP, "%s: unsupported ST code\n", __func__);
+        return;
+    }
+
+    /* Nothing to do */
+    if (!(s->phycr & FTGMAC100_PHYCR_NEW_FIRE)) {
+        return;
+    }
+
+    reg = FTGMAC100_PHYCR_NEW_REG(s->phycr);
+    data = FTGMAC100_PHYCR_NEW_DATA(s->phycr);
+
+    switch (FTGMAC100_PHYCR_NEW_OP(s->phycr)) {
+    case FTGMAC100_PHYCR_NEW_OP_WRITE:
+        do_phy_write(s, reg, data);
+        break;
+    case FTGMAC100_PHYCR_NEW_OP_READ:
+        s->phydata = do_phy_read(s, reg) & 0xffff;
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid OP code %08x\n",
+                      __func__, s->phycr);
+    }
+
+    s->phycr &= ~FTGMAC100_PHYCR_NEW_FIRE;
+}
+
+static void do_phy_ctl(FTGMAC100State *s)
+{
+    uint8_t reg = FTGMAC100_PHYCR_REG(s->phycr);
+
+    if (s->phycr & FTGMAC100_PHYCR_MIIWR) {
+        do_phy_write(s, reg, s->phydata & 0xffff);
+        s->phycr &= ~FTGMAC100_PHYCR_MIIWR;
+    } else if (s->phycr & FTGMAC100_PHYCR_MIIRD) {
+        s->phydata = do_phy_read(s, reg) << 16;
+        s->phycr &= ~FTGMAC100_PHYCR_MIIRD;
+    } else {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: no OP code %08x\n",
+                      __func__, s->phycr);
+    }
+}
+
 static int ftgmac100_read_bd(FTGMAC100Desc *bd, dma_addr_t addr)
 {
     if (dma_memory_read(&address_space_memory, addr, bd, sizeof(*bd))) {
@@ -628,7 +689,6 @@  static void ftgmac100_write(void *opaque, hwaddr addr,
                           uint64_t value, unsigned size)
 {
     FTGMAC100State *s = FTGMAC100(opaque);
-    int reg;
 
     switch (addr & 0xff) {
     case FTGMAC100_ISR: /* Interrupt status */
@@ -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);
         }
         break;
     case FTGMAC100_PHYDATA:
@@ -728,8 +785,7 @@  static void ftgmac100_write(void *opaque, hwaddr addr,
         s->dblac = value;
         break;
     case FTGMAC100_REVR:  /* Feature Register */
-        /* TODO: Only Old MDIO interface is supported */
-        s->revr = value & ~FTGMAC100_REVR_NEW_MDIO_INTERFACE;
+        s->revr = value;
         break;
     case FTGMAC100_FEAR1: /* Feature Register 1 */
         s->fear1 = value;