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

Cédric Le Goater posted 1 patch 22 weeks ago
Test asan passed
Test checkpatch passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190111125759.31577-1-clg@kaod.org
Maintainers: Joel Stanley <joel@jms.id.au>, Jason Wang <jasowang@redhat.com>, Andrew Jeffery <andrew@aj.id.au>, "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>
hw/net/ftgmac100.c | 80 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 68 insertions(+), 12 deletions(-)

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

Posted by Cédric Le Goater 22 weeks ago
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(-)

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


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

Posted by Peter Maydell 21 weeks ago
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

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

Posted by Joel Stanley 22 weeks ago
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

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

Posted by Cédric Le Goater 22 weeks ago
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
> 


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

Posted by Andrew Jeffery 22 weeks ago

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
>